Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: bugs && release v0.7.4 #432

Merged
merged 1 commit into from
Aug 24, 2024
Merged

fix: bugs && release v0.7.4 #432

merged 1 commit into from
Aug 24, 2024

Conversation

dreamhunter2333
Copy link
Owner

@dreamhunter2333 dreamhunter2333 commented Aug 24, 2024

User description

#430
#431


PR Type

Bug fix, Enhancement


Description

  • Enhanced name validation by adding a block list check in the newAddress function.
  • Fixed handling of DEFAULT_DOMAINS configuration to ensure proper fallback when it is undefined or null.
  • Updated the changelog to reflect the latest changes and fixes in version 0.7.4.

Changes walkthrough 📝

Relevant files
Enhancement
common.ts
Enhance name validation and add block list check                 

worker/src/common.ts

  • Added checkNameBlockList function to validate names against a block
    list.
  • Modified newAddress function to include checkNameBlockList and
    improved name validation.
  • Imported getJsonSetting from utils.
  • +21/-3   
    Bug fix
    utils.ts
    Fix default domains configuration handling                             

    worker/src/utils.ts

  • Updated getDefaultDomains to handle undefined or null DEFAULT_DOMAINS.
  • Ensured fallback to getDomains if DEFAULT_DOMAINS is not set.
  • +4/-2     
    Documentation
    CHANGELOG.md
    Update changelog for version 0.7.4                                             

    CHANGELOG.md

  • Updated changelog for version 0.7.4.
  • Documented fixes for name validation and default domains
    configuration.
  • +3/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The checkNameBlockList function catches and logs errors but does not rethrow them or handle them in a way that informs the caller of the failure. This could lead to silent failures where the caller assumes the name is valid even if an error occurred during the block list check.

    Copy link

    github-actions bot commented Aug 24, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add await to the checkNameBlockList function call to ensure it completes before proceeding

    To ensure that the checkNameBlockList function is properly awaited, add the await
    keyword before calling it. This will ensure that the function completes before
    proceeding.

    worker/src/common.ts [67]

    -checkNameBlockList(c, name);
    +await checkNameBlockList(c, name);
     
    Suggestion importance[1-10]: 10

    Why: This suggestion is crucial as it ensures that the asynchronous checkNameBlockList function completes before proceeding, preventing potential issues with unhandled promises.

    10
    Best practice
    Rethrow the error after logging it in the checkNameBlockList function to ensure the caller is aware of the error

    To improve error handling, rethrow the error after logging it in the
    checkNameBlockList function. This will ensure that the caller is aware of the error.

    worker/src/common.ts [144]

     console.error(error);
    +throw error;
     
    Suggestion importance[1-10]: 9

    Why: Rethrowing the error after logging it improves error handling by ensuring that the caller is aware of the error, which is a best practice.

    9
    Enhancement
    Simplify the getDefaultDomains function by avoiding redundant checks

    Simplify the getDefaultDomains function by directly returning the result of
    getDomains if c.env.DEFAULT_DOMAINS is undefined or null, and avoid redundant
    checks.

    worker/src/utils.ts [100-104]

    -if (c.env.DEFAULT_DOMAINS == undefined || c.env.DEFAULT_DOMAINS == null) {
    +if (!c.env.DEFAULT_DOMAINS) {
         return getDomains(c);
     }
    -const domains = getStringArray(c.env.DEFAULT_DOMAINS);
    -return domains || getDomains(c);
    +return getStringArray(c.env.DEFAULT_DOMAINS) || getDomains(c);
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves code readability and efficiency by removing redundant checks, making the function more concise.

    8
    Maintainability
    Move the name.replace line after the name checks for better readability and maintainability

    To improve readability and maintainability, move the name.replace line after the
    name checks are completed.

    worker/src/common.ts [64-69]

    -name = name.replace(getNameRegex(c), '');
     // check name
     if (enableCheckNameRegex) {
    -    checkNameBlockList(c, name);
    +    await checkNameBlockList(c, name);
         checkNameRegex(c, name);
     }
    +// remove special characters
    +name = name.replace(getNameRegex(c), '');
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves code readability and maintainability by logically ordering the operations, although it is not crucial for functionality.

    7

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant