search

keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
208 stars 105 forks source link

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

Closed PriyobrotoKar closed 4 months ago

PriyobrotoKar commented 4 months ago

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

If changes are made in the code:

Documentation Update

PR Type

Enhancement, Bug fix

Description

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

    codiumai-pr-agent-free[bot] commented 4 months ago

    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.
    codiumai-pr-agent-free[bot] commented 4 months ago

    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]](https://github.com/keyshade-xyz/keyshade/pull/335/files#diff-3ae286985cfeac090cabd429156a45eeaae5875dc9d9e23d93549e3b289387baR65-R67) ```diff -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]](https://github.com/keyshade-xyz/keyshade/pull/335/files#diff-3ae286985cfeac090cabd429156a45eeaae5875dc9d9e23d93549e3b289387baR140-R142) ```diff -onClick={(e) => { +onClick={(e: React.MouseEvent) => { 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]](https://github.com/keyshade-xyz/keyshade/pull/335/files#diff-3ae286985cfeac090cabd429156a45eeaae5875dc9d9e23d93549e3b289387baR142-R142) ```diff -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]](https://github.com/keyshade-xyz/keyshade/pull/335/files#diff-2905a64d2fff6a886f6db34c148dde4386dff9c2b056ae60ffb51cd7b84c32f7R11-R19) ```diff ```
    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
    kriptonian1 commented 4 months ago

    @rajdip-b you can merge this