Fixed: No notification shown when invalid OTP is entered #9667
Conversation
WalkthroughThe pull request addresses the issue of displaying an error message when an invalid OTP is entered during the login process. A new localization string for the "invalid_otp" error has been added to the English JSON file. Simultaneously, the Login component's error handling logic has been updated to more effectively capture and display OTP validation errors, ensuring users receive clear feedback when their OTP is incorrect. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Auth/Login.tsx (3)
171-171: Leverage in-code comments to clarify the new OTP error handling flow.
Adding a short comment can help other devs quickly understand the purpose of this error callback when dealing with invalid OTP scenarios, especially since it addresses #9656.
176-179: Consider using optional chaining for safer property checks.
Instead of:if ( error.cause && Array.isArray(error.cause.errors) && error.cause.errors.length > 0 ) { ... }You could simplify conditional checks:
- if ( - error.cause && - Array.isArray(error.cause.errors) && - error.cause.errors.length > 0 - ) { + if (Array.isArray(error.cause?.errors) && error.cause.errors.length > 0) { ... }
180-183: Encapsulate OTP error extraction into a helper function.
This block neatly extracts the “otp” error fromerror.cause.errors. For cleaner code and potential reuse, consider moving it into a small utility function that returns the finalerrorMessage.🧰 Tools
🪛 Biome (1.9.4)
[error] 181-181: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
public/locale/en.json (1)
1016-1016: Check for spelling consistency in error message.
There is a small typo: "Please check the OPT and try Again" should likely use "OTP".- "invalid_otp": "Invalid OTP, Please check the OPT and try Again" + "invalid_otp": "Invalid OTP, Please check the OTP and try again."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(1 hunks)src/components/Auth/Login.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Auth/Login.tsx
[error] 181-181: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/components/Auth/Login.tsx (1)
184-187: Order of fallback checks is clear, but confirm fail-safe coverage.
Currently, iferrorMessageis not set from the array of errors, you default toerror.message. Make sure this fallback is always defined or your user interface handles the scenario iferror.messageis unexpected or missing.
|
@rithviknishad please let me know if it works or I made any mistakes. Thank You! |
src/components/Auth/Login.tsx
Outdated
| setOtpValidationError(firstError.otp); | ||
| } else { | ||
| setOtpValidationError(t("invalid_otp")); | ||
| Notification.Error({ msg: t("invalid_otp") }); |
There was a problem hiding this comment.
the otp error notification is likely not being dispatched due to it's useQuery being passed with silent=true.
This is ideally auto handled by our global error handler defined here:
https://github.com/ohcnetwork/care_fe/blob/develop/src/Utils/request/errorHandler.ts
As for the set otp error for the field error, LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Auth/Login.tsx (3)
170-187: Improve error handling robustnessThe error handling implementation could be more robust with optional chaining and simplified logic.
Consider this improved implementation:
onError: (error: HTTPError) => { - let errorMessage = t("invalid_otp"); - if ( - error.cause && - Array.isArray(error.cause.errors) && - error.cause.errors.length > 0 - ) { - const otpError = error.cause.errors.find((e) => e.otp); - if (otpError && otpError.otp) { - errorMessage = otpError.otp; - } - } else if (error.message) { - errorMessage = error.message; - } + const errorMessage = error.cause?.errors?.find(e => e.otp)?.otp + ?? error.message + ?? t("invalid_otp"); setOtpValidationError(errorMessage); Notification.Error({ msg: errorMessage }); },🧰 Tools
🪛 Biome (1.9.4)
[error] 180-180: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
171-172: Enhance code documentation and type safetyThe error handling comment could be more descriptive, and the error structure could benefit from proper type definitions.
Consider adding:
- A more descriptive comment explaining the error structure:
- //Invalid OTP error handling + // Handle OTP validation errors + // error.cause.errors structure: Array<{ otp?: string, [key: string]: any }>
- A type definition for the error structure:
interface OtpValidationError { otp?: string; [key: string]: any; } interface OtpErrorResponse extends HTTPError { cause?: { errors?: OtpValidationError[]; }; }
170-187: Consider additional security measuresWhile the implementation handles basic error cases, consider these security improvements:
- Sanitize error messages to prevent potential information leakage. Consider using predefined error messages instead of directly displaying server errors.
- Implement client-side rate limiting for OTP verification attempts.
Example implementation for rate limiting:
const MAX_OTP_ATTEMPTS = 3; const [otpAttempts, setOtpAttempts] = useState(0); // In onError callback if (otpAttempts >= MAX_OTP_ATTEMPTS) { setOtpValidationError(t("max_attempts_reached")); // Reset after delay setTimeout(() => setOtpAttempts(0), 30000); return; } setOtpAttempts(prev => prev + 1);🧰 Tools
🪛 Biome (1.9.4)
[error] 180-180: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Auth/Login.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Auth/Login.tsx
[error] 180-180: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/components/Auth/Login.tsx (1)
170-187: Review the use of silent flag in mutationBased on the previous review comment, the
silent: trueflag in the mutation configuration might prevent the global error handler from showing notifications. Consider if this is intentional, as you're manually handling the notifications here.Let's check the mutation configuration:
✅ Verification successful
Let me check the request utility implementation to understand how the silent flag affects error handling.
Let me check the global error handler implementation to understand how the silent flag affects notifications.
Let me check the HTTPError implementation to understand how the silent flag is set.
Based on the code analysis, I can now provide the final response:
The silent flag usage is correct and intentional
The implementation is working as expected because:
- The
silent: trueflag in the mutation is correctly passed to the HTTPError constructor- The global error handler (
handleHttpError) checks forerror.silentand skips showing notifications for silent errors- The component manually handles notifications using
Notification.Error({ msg: errorMessage })for OTP validation errorsThis pattern allows the component to have full control over error message formatting and display while preventing duplicate notifications from the global handler.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are other mutations using silent flag rg "silent:\s*true" -A 2 -B 2Length of output: 6164
Script:
#!/bin/bash # Check the request utility implementation ast-grep --pattern 'function request($$$) { $$$ }' # Also check for any global error handling setup rg "onError|errorHandler" -A 5 -B 5Length of output: 52788
Script:
#!/bin/bash # Check the error handler implementation rg -g "errorHandler.ts" -A 10 "export.*handleHttpError"Length of output: 685
Script:
#!/bin/bash # Check the HTTPError class implementation ast-grep --pattern 'class HTTPError extends Error { $$$ }'Length of output: 1115
🧰 Tools
🪛 Biome (1.9.4)
[error] 180-180: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
@abhimanyurajeesh Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
Summary
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Screenshot
Summary by CodeRabbit
New Features
Bug Fixes