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

feat(platform): Add warning sonner toast for invalid otp #335

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

PriyobrotoKar
Copy link
Contributor

@PriyobrotoKar PriyobrotoKar commented Jul 10, 2024

User description

Description

Added sonner rich text warning message for invalid otp

Fixes #332

Dependencies

shadcn/ui-Sonner

Future Improvements

Mention any improvements to be done in future related to any file/feature

Mentions

Mention and tag the people

Screenshots of relevant screens

Screenshot 2024-07-10 at 10 11 02 AM

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement, Bug fix


Description

  • Added a warning toast message using sonner for invalid OTP input.
  • Prevented default form submission behavior on OTP verification button click.
  • Simplified the setOtp function call in the OTP input component.
  • Added type definition for OTPInputProps in the InputOTP component.

Changes walkthrough 📝

Relevant files
Enhancement
page.tsx
Add warning toast and improve OTP handling                             

apps/platform/src/app/auth/otp/page.tsx

  • Added toast.warning for invalid OTP input.
  • Prevented default form submission on OTP verification.
  • Simplified setOtp function call.
  • +7/-2     
    input-otp.tsx
    Add type definition and simplify InputOTP component           

    apps/platform/src/components/ui/input-otp.tsx

  • Added type definition for OTPInputProps.
  • Simplified InputOTP component definition.
  • +15/-13 

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Possible Bug:
    The toast.warning function is called when the response status is 401, which is appropriate for showing an error. However, the function might not handle other error statuses that could also indicate issues with the OTP or other parts of the process. Consider handling other relevant HTTP status codes.

    Code Duplication:
    The setOtp function call has been simplified from setOtp(otpVal as string) to setOtp(otpVal). Ensure that this change does not affect the expected type and behavior of the setOtp function, especially if otpVal is not always a string.

    UI Feedback:
    The addition of e.preventDefault() in the button's onClick handler is a good practice to stop the form from submitting traditionally, but ensure that this change is tested across all browsers for consistency in behavior.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Prevent multiple toast popups during loading state

    Consider adding a check for the isLoading state before displaying the toast to
    prevent multiple toast popups if the user repeatedly submits while the request is
    still processing.

    apps/platform/src/app/auth/otp/page.tsx [65-67]

    -toast.warning(
    -  'The OTP you entered is not valid, Please enter the right OTP'
    -)
    +if (!isLoading) {
    +  toast.warning(
    +    'The OTP you entered is not valid, Please enter the right OTP'
    +  )
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion prevents multiple toast popups during the loading state, improving user experience and avoiding potential confusion.

    9
    Best practice
    Add type to the event parameter for better type safety

    Ensure that the event e is typed to provide better type safety and to leverage
    TypeScript's capabilities.

    apps/platform/src/app/auth/otp/page.tsx [140-142]

    -onClick={(e) => {
    +onClick={(e: React.MouseEvent<HTMLButtonElement>) => {
       e.preventDefault()
       void handleVerifyOTP(email, otp)
     }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding type to the event parameter enhances type safety and leverages TypeScript's capabilities, which is a good practice.

    8
    Handle exceptions from asynchronous operations

    It's a good practice to handle potential exceptions from asynchronous operations.
    Consider adding a try-catch block around the handleVerifyOTP function call.

    apps/platform/src/app/auth/otp/page.tsx [142]

    -void handleVerifyOTP(email, otp)
    +try {
    +  void handleVerifyOTP(email, otp)
    +} catch (error) {
    +  console.error('Failed to verify OTP:', error)
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Handling exceptions from asynchronous operations is a good practice, but the suggestion could be more specific about where to place the try-catch block.

    7
    Explicitly manage value and onChange props for controlled component behavior

    To ensure that the InputOTP component is fully controlled and its state is
    predictable, consider managing the value and onChange props explicitly if not
    already handled.

    apps/platform/src/components/ui/input-otp.tsx [11-19]

     <OTPInput
       className={cn('disabled:cursor-not-allowed', className)}
       containerClassName={cn(
         'flex items-center justify-center gap-2 has-[:disabled]:opacity-50',
         containerClassName
       )}
       ref={ref}
    +  value={props.value}
    +  onChange={props.onChange}
       {...props}
     />
     
    Suggestion importance[1-10]: 6

    Why: Explicitly managing value and onChange props can improve the predictability of the component's state, but it assumes these props are not already handled elsewhere.

    6

    Copy link
    Contributor

    @kriptonian1 kriptonian1 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM 🚀

    @kriptonian1
    Copy link
    Contributor

    @rajdip-b you can merge this

    @rajdip-b rajdip-b changed the title feat(platform): add warning sonner toast for invalid otp feat(platform): Add warning sonner toast for invalid otp Jul 15, 2024
    apps/platform/src/app/auth/otp/page.tsx Outdated Show resolved Hide resolved
    Copy link
    Contributor

    @kriptonian1 kriptonian1 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM

    @rajdip-b rajdip-b force-pushed the feat/sonner-toast branch from 05ed794 to aea6c19 Compare July 17, 2024 05:04
    @rajdip-b rajdip-b merged commit 4bd12f0 into keyshade-xyz:develop Jul 17, 2024
    3 checks passed
    rajdip-b pushed a commit that referenced this pull request Jul 29, 2024
    ## [2.3.0](v2.2.0...v2.3.0) (2024-07-29)
    
    ### 🚀 Features
    
    * **api:** Add pagination metadata to Environment module ([#382](#382)) ([9baa344](9baa344))
    * **api:** Add pagination metadata to Event module ([#394](#394)) ([60010b4](60010b4))
    * **api:** Add pagination metadata to Integration module ([#391](#391)) ([0372e36](0372e36))
    * **api:** Add pagination metadata to Project module ([#393](#393)) ([bc274fd](bc274fd))
    * **api:** Add pagination metadata to Secret module ([#389](#389)) ([c4cc667](c4cc667))
    * **api:** Add pagination metadata to Variable module ([#390](#390)) ([be6aabf](be6aabf))
    * **api:** Add pagination metadata to Workspace module  ([#387](#387)) ([a08c924](a08c924))
    * **api:** Add pagination metadata to Workspace Role module ([#388](#388)) ([d8e8f49](d8e8f49))
    * **api:** Create a paginate method ([#379](#379)) ([09576f1](09576f1))
    * **api:** Create endpoint for fetching all revisions of a [secure] ([#303](#303)) ([de2b602](de2b602))
    * **api:** Create endpoint for fetching all revisions of a variable ([#304](#304)) ([9abddc1](9abddc1))
    * **cli:** Improved the DX for list profile ([#334](#334)) ([6bff496](6bff496))
    * **platform:** Add warning sonner toast for invalid otp ([#335](#335)) ([21513f5](21513f5))
    
    ### 🐛 Bug Fixes
    
    * **cli:** Added parent directory check ([#359](#359)) ([538ea7f](538ea7f))
    * **platform:** Platform types fixes ([#374](#374)) ([8e9d9ff](8e9d9ff))
    
    ### 📚 Documentation
    
    * Added docker details in setting-things-up.md ([#358](#358)) ([ed5093a](ed5093a))
    * Update postman workspace link ([d6aba27](d6aba27))
    * Updated env and cli docs ([1213d2a](1213d2a))
    
    ### 🔧 Miscellaneous Chores
    
    * Added next backend url in .env.example ([5695254](5695254))
    * **api-client:** Added pagination structure ([a70e957](a70e957))
    * **api-client:** Fixed test script ([ad70819](ad70819))
    * **api-client:** Removed try-catch from tests in environment ([a64e48c](a64e48c))
    * **api:** Add user cache for optimization ([#386](#386)) ([8d730b5](8d730b5))
    * **api:** Alter cache rehydration interval ([f5f9eec](f5f9eec))
    * **api:** Fixed naming error in variable controller ([0c5a380](0c5a380))
    * **api:** Improve handling of edge cases for paginate module ([#402](#402)) ([8591487](8591487))
    * **api:** Minor updates to user service ([249d778](249d778))
    * **api:** Skip workspace creation when user is admin ([#376](#376)) ([13f6c59](13f6c59))
    * **ci:** Add docker check   ([#383](#383)) ([3119001](3119001))
    * **ci:** Add names to CI files ([1a7e5f6](1a7e5f6))
    * **ci:** Add validate CLI pipeline ([#373](#373)) ([a91df6c](a91df6c))
    * **ci:** Adding validate pipeline ([#372](#372)) ([23cf3b3](23cf3b3))
    * **ci:** Disabled platform and api deployments ([74d601a](74d601a))
    * **ci:** Fixed deployment scripts ([12e35db](12e35db))
    * **ci:** Fixed platform script ([d783f2a](d783f2a))
    * **CI:** Include migration deployment in API deploy pipeline ([dbd5222](dbd5222))
    * **CI:** Separated deployment and docker build jobs ([090e193](090e193))
    * **CI:** Setup inter-job dependency ([1756727](1756727))
    * **ci:** Update auto-assign.yaml ([#375](#375)) ([91e0ec1](91e0ec1))
    * **cli:** Changed objects to classes ([#306](#306)) ([c83f2db](c83f2db))
    * Removed Minio config ([8feb83a](8feb83a))
    * Updated deployment scripts and added health check in platform ([fcc1c3f](fcc1c3f))
    
    ### 🔨 Code Refactoring
    
    * **api:** Updated path of some endpoints in project controller ([9502678](9502678))
    * **api:** Updated Redis provider ([33491a1](33491a1))
    rajdip-b pushed a commit that referenced this pull request Jul 29, 2024
    ## [2.3.0](v2.2.0...v2.3.0) (2024-07-29)
    
    ### 🚀 Features
    
    * **api:** Add pagination metadata to Environment module ([#382](#382)) ([9baa344](9baa344))
    * **api:** Add pagination metadata to Event module ([#394](#394)) ([60010b4](60010b4))
    * **api:** Add pagination metadata to Integration module ([#391](#391)) ([0372e36](0372e36))
    * **api:** Add pagination metadata to Project module ([#393](#393)) ([bc274fd](bc274fd))
    * **api:** Add pagination metadata to Secret module ([#389](#389)) ([c4cc667](c4cc667))
    * **api:** Add pagination metadata to Variable module ([#390](#390)) ([be6aabf](be6aabf))
    * **api:** Add pagination metadata to Workspace module  ([#387](#387)) ([a08c924](a08c924))
    * **api:** Add pagination metadata to Workspace Role module ([#388](#388)) ([d8e8f49](d8e8f49))
    * **api:** Create a paginate method ([#379](#379)) ([09576f1](09576f1))
    * **api:** Create endpoint for fetching all revisions of a [secure] ([#303](#303)) ([de2b602](de2b602))
    * **api:** Create endpoint for fetching all revisions of a variable ([#304](#304)) ([9abddc1](9abddc1))
    * **cli:** Improved the DX for list profile ([#334](#334)) ([6bff496](6bff496))
    * **platform:** Add warning sonner toast for invalid otp ([#335](#335)) ([21513f5](21513f5))
    
    ### 🐛 Bug Fixes
    
    * **cli:** Added parent directory check ([#359](#359)) ([538ea7f](538ea7f))
    * **platform:** Platform types fixes ([#374](#374)) ([8e9d9ff](8e9d9ff))
    
    ### 📚 Documentation
    
    * Added docker details in setting-things-up.md ([#358](#358)) ([ed5093a](ed5093a))
    * Update postman workspace link ([d6aba27](d6aba27))
    * Updated env and cli docs ([1213d2a](1213d2a))
    
    ### 🔧 Miscellaneous Chores
    
    * Added next backend url in .env.example ([5695254](5695254))
    * **api-client:** Added pagination structure ([a70e957](a70e957))
    * **api-client:** Fixed test script ([ad70819](ad70819))
    * **api-client:** Removed try-catch from tests in environment ([a64e48c](a64e48c))
    * **api:** Add user cache for optimization ([#386](#386)) ([8d730b5](8d730b5))
    * **api:** Alter cache rehydration interval ([f5f9eec](f5f9eec))
    * **api:** Fixed naming error in variable controller ([0c5a380](0c5a380))
    * **api:** Improve handling of edge cases for paginate module ([#402](#402)) ([8591487](8591487))
    * **api:** Minor updates to user service ([249d778](249d778))
    * **api:** Skip workspace creation when user is admin ([#376](#376)) ([13f6c59](13f6c59))
    * **ci:** Add docker check   ([#383](#383)) ([3119001](3119001))
    * **ci:** Add names to CI files ([1a7e5f6](1a7e5f6))
    * **ci:** Add validate CLI pipeline ([#373](#373)) ([a91df6c](a91df6c))
    * **ci:** Adding validate pipeline ([#372](#372)) ([23cf3b3](23cf3b3))
    * **ci:** Disabled platform and api deployments ([74d601a](74d601a))
    * **ci:** Fixed deployment scripts ([12e35db](12e35db))
    * **ci:** Fixed platform script ([d783f2a](d783f2a))
    * **CI:** Include migration deployment in API deploy pipeline ([dbd5222](dbd5222))
    * **CI:** Separated deployment and docker build jobs ([090e193](090e193))
    * **CI:** Setup inter-job dependency ([1756727](1756727))
    * **ci:** Update auto-assign.yaml ([#375](#375)) ([91e0ec1](91e0ec1))
    * **cli:** Changed objects to classes ([#306](#306)) ([c83f2db](c83f2db))
    * Removed Minio config ([8feb83a](8feb83a))
    * Updated deployment scripts and added health check in platform ([fcc1c3f](fcc1c3f))
    
    ### 🔨 Code Refactoring
    
    * **api:** Updated path of some endpoints in project controller ([9502678](9502678))
    * **api:** Updated Redis provider ([33491a1](33491a1))
    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.

    PLATFORM: Show warning sonner toaster if OTP is invalid
    3 participants