[ENG-635] feat: Add password reset via OTP - #16600
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesAdds phone-based OTP password reset with typed unauthenticated API routes, reusable password validation fields, shared forgot-password UI integration, localized messages, and Playwright coverage for successful, invalid-OTP, and unknown-phone flows. Password Reset Experience
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🎭 Playwright Test ResultsStatus: ✅ Passed
📊 Detailed results are available in the playwright-final-report artifact. Run: #10517 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Adds an OTP-based password reset option (via phone number) to the authentication flow, alongside some refactoring to reuse password-field UI/validation.
Changes:
- Added new OTP API routes and types for password reset via phone/OTP confirmation.
- Introduced a new “Forgot password” panel with a phone-based reset flow and extracted reusable new-password fields/validation.
- Added new English i18n strings for the phone reset UX.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/otp/otpApi.ts | Adds password-reset OTP send/confirm endpoints. |
| src/types/otp/otp.ts | Adds request/response types for password reset OTP confirmation. |
| src/components/Auth/ResetPassword.tsx | Refactors to reuse shared new-password fields + validation; keeps existing reset-by-token flow. |
| src/components/Auth/NewPasswordFields.tsx | New shared component + validator for password/confirm inputs and strength hints. |
| src/components/Auth/Login.tsx | Integrates the new “Forgot password” panel into the login screen. |
| src/components/Auth/ForgotPasswordPhone.tsx | New phone-number OTP password reset UI and mutations. |
| src/components/Auth/ForgotPasswordPanel.tsx | New panel that switches between email reset and phone reset paths. |
| public/locale/en.json | Adds i18n keys for phone reset flow strings. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Auth/ForgotPasswordPanel.tsx`:
- Around line 103-110: Replace the interactive <li> in the forgot-password
method selector with the existing Button-based interactive pattern used
elsewhere in ForgotPasswordPanel, preserving its styling, translation label, and
setForgotMethod("phone") action so it is keyboard and screen-reader accessible.
In `@src/components/Auth/NewPasswordFields.tsx`:
- Line 58: Document the NewPasswordFields component, including its
password-reset use case and accessibility behavior, following the existing
documentation conventions for reusable components under src/components/. Add the
documentation adjacent to the NewPasswordFields export without changing its
implementation.
- Around line 71-126: Update both PasswordInput instances in NewPasswordFields
to provide programmatic labels, expose their invalid state from
errors.password/errors.confirm, and associate each input with its corresponding
error element via stable ids and aria-describedby. Ensure the error elements use
matching ids and retain the existing validation messaging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ac59746-34fc-49f3-925c-f365abde9c43
📒 Files selected for processing (8)
public/locale/en.jsonsrc/components/Auth/ForgotPasswordPanel.tsxsrc/components/Auth/ForgotPasswordPhone.tsxsrc/components/Auth/Login.tsxsrc/components/Auth/NewPasswordFields.tsxsrc/components/Auth/ResetPassword.tsxsrc/types/otp/otp.tssrc/types/otp/otpApi.ts
Deploying care-preview with
|
| Latest commit: |
789e6d4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9bcf6a07.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-635-wire-password-reset.care-preview-a7w.pages.dev |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Auth/ForgotPasswordPhone.tsx (2)
402-411: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConfirm button stays enabled even when a required username is missing.
The
disabledcondition doesn't account forrequiresUsername, so the button is clickable when username is required but empty; the click only surfaces the error after client-side validation runs.🐛 Proposed fix
disabled={ confirmPending || otp.length !== OTP_LENGTH || !passwordForm.password || - !passwordForm.confirm + !passwordForm.confirm || + (requiresUsername && !username.trim()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Auth/ForgotPasswordPhone.tsx` around lines 402 - 411, Update the Confirm button’s disabled condition in ForgotPasswordPhone so it also disables when requiresUsername is true and the required username is empty, while preserving the existing pending, OTP, password, and confirmation checks.
269-282: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd the validation wiring at these inputs.
PhoneInputalready forwards props, but its built-inaria-describedby="phone-input-constraint"gets replaced unless you compose that id with the phone error id.InputOTPshould also expose the OTP error viaaria-invalidandaria-describedby, with the message marked as an alert.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Auth/ForgotPasswordPhone.tsx` around lines 269 - 282, Update the PhoneInput and InputOTP validation wiring in ForgotPasswordPhone: preserve PhoneInput’s existing constraint aria-describedby while composing it with the phone error element ID, and add aria-invalid plus composed aria-describedby for the OTP input. Assign the corresponding error-message IDs and mark both validation messages as alerts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Auth/NewPasswordFields.tsx`:
- Around line 76-93: Add autoComplete="new-password" to both PasswordInput
instances in NewPasswordFields, including the new-password field and the
additional password field referenced by the review. Preserve all existing props
and behavior.
In `@tests/helper/user.ts`:
- Line 36: Update the username generation in the test helper to use Date.now()
or faker.string.alphanumeric() instead of the four-digit faker.string.numeric()
suffix, while preserving the existing lowercase first-name prefix and username
format.
---
Outside diff comments:
In `@src/components/Auth/ForgotPasswordPhone.tsx`:
- Around line 402-411: Update the Confirm button’s disabled condition in
ForgotPasswordPhone so it also disables when requiresUsername is true and the
required username is empty, while preserving the existing pending, OTP,
password, and confirmation checks.
- Around line 269-282: Update the PhoneInput and InputOTP validation wiring in
ForgotPasswordPhone: preserve PhoneInput’s existing constraint aria-describedby
while composing it with the phone error element ID, and add aria-invalid plus
composed aria-describedby for the OTP input. Assign the corresponding
error-message IDs and mark both validation messages as alerts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6688cbc-422d-4535-88c4-31001588fbe3
📒 Files selected for processing (6)
src/components/Auth/ForgotPasswordPanel.tsxsrc/components/Auth/ForgotPasswordPhone.tsxsrc/components/Auth/NewPasswordFields.tsxsrc/components/Auth/ResetPassword.tsxtests/auth/passwordResetOtp.spec.tstests/helper/user.ts
ohc-project-manager
Bot
added
needs testing
needs review
and removed
Work in Progress
labels
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Auth/ForgotPasswordPhone.tsx (1)
269-281: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssociate all field errors with their controls.
The phone, OTP, and username errors are rendered as standalone text, but their inputs lack
aria-invalidandaria-describedbyreferences. Add stable error IDs and associate each message with its corresponding control.Proposed accessibility fix
+const phoneErrorId = "reset-phone-error"; ... <PhoneInput id="reset_phone" + aria-invalid={Boolean(phoneError)} + aria-describedby={phoneError ? phoneErrorId : undefined} /> ... -<p className="text-sm text-red-500">{phoneError}</p> +<p id={phoneErrorId} role="alert" className="text-sm text-red-500"> + {phoneError} +</p>Apply the same pattern to
InputOTP/errors.otpand the username input/error block.As per coding guidelines, React components must meet WCAG 2.1 AA requirements and implement proper ARIA attributes.
Also applies to: 323-351, 363-390
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Auth/ForgotPasswordPhone.tsx` around lines 269 - 281, Associate the phone, OTP, and username error messages with their respective controls in ForgotPasswordPhone by adding stable unique error IDs, setting aria-invalid when each field has an error, and referencing the matching ID through aria-describedby. Apply the pattern to the PhoneInput/phoneError, InputOTP/errors.otp, and username input/error blocks while preserving existing validation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Auth/ResetPassword.tsx`:
- Around line 54-58: Normalize or validate the error response in the
reset-password mutation’s onError handler before passing it to setErrors. Ensure
the resulting value matches the PasswordErrors shape expected by PasswordFields,
handling alternate API error envelopes safely and preserving a useful fallback
message when the response is not compatible.
---
Outside diff comments:
In `@src/components/Auth/ForgotPasswordPhone.tsx`:
- Around line 269-281: Associate the phone, OTP, and username error messages
with their respective controls in ForgotPasswordPhone by adding stable unique
error IDs, setting aria-invalid when each field has an error, and referencing
the matching ID through aria-describedby. Apply the pattern to the
PhoneInput/phoneError, InputOTP/errors.otp, and username input/error blocks
while preserving existing validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5a036fc-29aa-409f-bcd4-42a0d14da687
📒 Files selected for processing (3)
src/components/Auth/ForgotPasswordPhone.tsxsrc/components/Auth/PasswordFields.tsxsrc/components/Auth/ResetPassword.tsx
ohc-project-manager
Bot
added
Changes Required
and removed
needs review
needs testing
labels
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Auth/ForgotPasswordPhone.tsx (1)
121-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass
silent: truetosendOtp.This mutation already handles failures inline with
setPhoneError(...), so leaving it non-silent can surface the same OTP-send error twice: once from the global handler and once here.Proposed fix
- mutationFn: mutate(otpApi.sendPasswordResetOtp), + mutationFn: mutate(otpApi.sendPasswordResetOtp, { silent: true }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Auth/ForgotPasswordPhone.tsx` around lines 121 - 135, Update the sendOtp mutation configuration in ForgotPasswordPhone to pass silent: true when invoking mutate with otpApi.sendPasswordResetOtp, while preserving the existing inline onError handling and setPhoneError behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/Auth/ForgotPasswordPhone.tsx`:
- Around line 121-135: Update the sendOtp mutation configuration in
ForgotPasswordPhone to pass silent: true when invoking mutate with
otpApi.sendPasswordResetOtp, while preserving the existing inline onError
handling and setPhoneError behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78c109a8-62f9-4da4-bad2-7e20666a8bf8
📒 Files selected for processing (3)
public/locale/en.jsonsrc/components/Auth/ForgotPasswordPanel.tsxsrc/components/Auth/ForgotPasswordPhone.tsx
Will fix it |
ohc-project-manager
Bot
added
needs testing
needs review
and removed
Changes Required
labels
Jacobjeevan
left a comment
Jacobjeevan
left a comment
There was a problem hiding this comment.
Just improvements, can be taken up as followup.
| const data = cause as Record<string, unknown>; | ||
| const errors = data.errors; | ||
|
|
||
| if (Array.isArray(errors)) { |
There was a problem hiding this comment.
we should be able to leverage the existing handlePydantic logic w/ a light refactor no?
There was a problem hiding this comment.
We will soon migrate to the shadcn form and RHF, then we will not need this
There was a problem hiding this comment.
Prob don't need this component/can combine with ForgotPassword
Proposed Changes
Fixes https://openhealthcarenetwork.atlassian.net/browse/ENG-635
Password.reset.via.OTP.mp4
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit