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

[PLA-1940] fix name missing #154

Merged
merged 3 commits into from
Aug 19, 2024
Merged

[PLA-1940] fix name missing #154

merged 3 commits into from
Aug 19, 2024

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Aug 15, 2024

PR Type

Bug fix, Enhancement


Description

  • Added a new slot for displaying error text in the FormInput component.
  • Improved search input validation in Collections.vue and Tokens.vue to handle non-integer inputs.
  • Fixed the logic for retrieving collection and token names to ensure proper fallback values.
  • Removed redundant class attributes in Collections.vue.

Changes walkthrough 📝

Relevant files
Enhancement
FormInput.vue
Add slot for error text in FormInput component                     

resources/js/components/FormInput.vue

  • Added a new slot for displaying error text.
+3/-0     
Bug fix
Collections.vue
Improve search input validation and collection name retrieval

resources/js/components/pages/Collections.vue

  • Added error handling for non-integer search input.
  • Removed redundant class attributes.
  • Fixed collection name retrieval logic.
  • +18/-4   
    Tokens.vue
    Enhance search input validation and token name retrieval 

    resources/js/components/pages/Tokens.vue

  • Added error handling for non-integer search input.
  • Fixed token name retrieval logic.
  • +19/-5   

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

    @zlayine zlayine self-assigned this Aug 15, 2024
    @github-actions github-actions bot added enhancement New feature or request Bug fix labels Aug 15, 2024
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug
    The search input validation logic might not handle edge cases properly. Specifically, the check isNaN(e.target.value) will return true for any non-numeric input, including strings that could be numeric but formatted differently (e.g., numbers with commas or spaces). This could lead to unexpected behavior or user input rejection.

    Redundant Code
    The debouncedSearch.cancel(); call is redundant in the searchChange method when searchError.value is set to true because no search will be triggered after setting the error.

    Inconsistent Behavior
    The cancelSearch method sets input.value to undefined, which differs from the previous behavior where it was set to an empty string. This change might affect other parts of the application relying on the value being an empty string rather than undefined.

    Copy link

    github-actions bot commented Aug 15, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Improve input validation to ensure only positive integers are allowed

    Replace the direct isNaN check with a more robust validation that ensures the input
    is a positive integer. This can be done using a regular expression to match only
    numeric characters.

    resources/js/components/pages/Collections.vue [299-301]

    -if (isNaN(e.target.value)) {
    +if (!/^\d+$/.test(e.target.value)) {
         searchError.value = true;
         return;
     }
     
    Suggestion importance[1-10]: 9

    Why: The suggestion improves input validation by ensuring only positive integers are allowed, which is more robust than the current isNaN check. This enhances the reliability of the input validation.

    9
    Performance
    Add debouncing to input changes to improve performance

    Consider adding a debounce mechanism to the input change handler to limit the number
    of validations and searches triggered by rapid typing.

    resources/js/components/pages/Collections.vue [303-304]

    -debouncedSearch.cancel();
    -debouncedSearch();
    +if (debounceTimeout) clearTimeout(debounceTimeout);
    +debounceTimeout = setTimeout(() => {
    +    debouncedSearch();
    +}, 300);
     
    Suggestion importance[1-10]: 8

    Why: Adding debouncing to input changes can significantly improve performance by reducing the number of validations and searches triggered by rapid typing. This is a valuable enhancement for user experience and system efficiency.

    8

    @zlayine zlayine force-pushed the bugfix/pla-1940/name-missing branch from 48d7680 to 859d6c2 Compare August 16, 2024 07:29
    @zlayine zlayine merged commit ff74083 into master Aug 19, 2024
    3 checks passed
    @zlayine zlayine deleted the bugfix/pla-1940/name-missing branch August 19, 2024 07:33
    leonardocustodio pushed a commit that referenced this pull request Sep 11, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    3 participants