P1: fix(auth): improve OTP resend UX - #220
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe login flow now places errors beside the active field using a shared flash region that moves between email and OTP steps. Invalidated or exhausted OTP errors can offer an inline resend action when the flow is active. A successful resend clears and refocuses the OTP inputs, updates the guidance to state that earlier codes are invalid, and advises checking the spam folder. Unit and end-to-end tests validate these behaviors. Estimated code review effort: 3 (Moderate) | ~25 minutes ChangesOTP login recovery
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginPage
participant OTPService
User->>LoginPage: Submit OTP
LoginPage->>OTPService: Verify OTP
OTPService-->>LoginPage: Return invalidated-code or too-many-attempts error
LoginPage-->>User: Move flash to OTP slot and render inline Resend code action
User->>LoginPage: Select Resend code
LoginPage->>OTPService: Request new OTP
OTPService-->>LoginPage: Confirm resend
LoginPage-->>User: Clear OTP inputs, focus first input, show spam-folder guidance
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the ePDS-pr-220 environment in ePDS
|
🦋 Changeset detectedLatest commit: ce3caef The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Coverage Report for CI Build 30934422228Coverage increased (+0.06%) to 57.734%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
aspiers
changed the title
ea04aea to
50f4a5e
Compare
This comment has been minimized.
This comment has been minimized.
50f4a5e to
619ae08
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/step-definitions/auth.steps.ts (1)
628-635: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the old digits before testing the reset.
This makes the scenario prove that the boxes contained the old OTP prefix before resend; otherwise a regression that clears or ignores input before resend could still pass the final empty-state assertion.
Proposed test strengthening
await otpBoxes.nth(0).fill('1') await otpBoxes.nth(1).fill('2') + await expect(otpBoxes.nth(0)).toHaveValue('1') + await expect(otpBoxes.nth(1)).toHaveValue('2')🤖 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 `@e2e/step-definitions/auth.steps.ts` around lines 628 - 635, Update the step definition “the user enters two digits from the old OTP” to assert that the first two OTP boxes contain the entered old digits after filling them. Keep the existing fill behavior and ensure the assertions run before the resend/reset action.
🤖 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.
Nitpick comments:
In `@e2e/step-definitions/auth.steps.ts`:
- Around line 628-635: Update the step definition “the user enters two digits
from the old OTP” to assert that the first two OTP boxes contain the entered old
digits after filling them. Keep the existing fill behavior and ensure the
assertions run before the resend/reset action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a80ed2d-8632-428c-bd64-317aa077a525
📒 Files selected for processing (4)
.changeset/clear-otp-boxes-on-resend.mde2e/step-definitions/auth.steps.tsfeatures/passwordless-authentication.featurepackages/auth-service/src/routes/login-page.ts
The "boxes are empty after resend" assertion only proves the reset happened if the boxes held something beforehand. Without this check a regression that cleared or ignored input *before* resend would still satisfy the final empty-state assertion. Suggested by CodeRabbit in review of #220. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
(reply generated by Claude Opus 5 via Claude Code) @coderabbitai re: the nitpick on Your reasoning is right: without it, a regression that cleared or ignored input before the resend would still satisfy the final empty-state assertion, so the scenario wouldn't actually be proving the reset. await otpBoxes.nth(0).fill('1')
await otpBoxes.nth(1).fill('2')
// Prove the digits actually landed, so the later empty-box assertion
// demonstrates that resend cleared them rather than that they were
// never entered.
await expect(otpBoxes.nth(0)).toHaveValue('1')
await expect(otpBoxes.nth(1)).toHaveValue('2') |
|
(reply generated by Claude Opus 5 via Claude Code) Re: the Blacksmith report of It ran against 50f4a5e, an earlier version of the commit that has since been amended and force-pushed away (it is no longer an ancestor of this branch). The failure was: Cause: my new step asserted Fix, already in 619ae08: assert against the length of the OTP actually captured from the mail trap, which is by definition the number of boxes to expect: await expect(otpBoxes).toHaveCount(this.otpCode.length)This mirrors the existing guard at E2E tests pass on 619ae08 (run 30560777504), and all checks are currently green on the branch tip. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
The "boxes are empty after resend" assertion only proves the reset happened if the boxes held something beforehand. Without this check a regression that cleared or ignored input *before* resend would still satisfy the final empty-state assertion. Suggested by CodeRabbit in review of #220. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d73d017 to
fe45b27
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/auth-service/src/routes/login-page.ts:568
- The PR title/description focus on OTP resend UX, but this change set also repositions the shared flash/error region between steps (so errors render next to the relevant field) and adds an inline “Resend code” CTA for lockout-style errors (“too many attempts/invalidated”). Please update the PR title/description to reflect these additional user-facing behavior changes (or split them into a separate PR) so the review scope matches what’s actually being shipped.
// The flash region is a single element shared by both steps, so there
// is only ever one live region for assistive tech to track. It is
// server-rendered into whichever step is initially visible and moved
// between the two slots on step transitions; both transitions call
// clearError() first, so it is always empty when it moves.
const flashRegionHtml =
'<div id="error-msg" class="flash-msg hidden" role="status" aria-live="polite"></div>'
The two shots in the PR description predate this branch's later commits: the "after" still shows "Code resent!" in a banner above the code boxes, which is neither the current copy nor the current position. Neither showed the error-placement change at all. Recapture against live Railway previews rather than a local mock — PR #229's preview for the before (still serving the old layout) and this branch's own for the after. pr-220-before-error-above-form.png error above the subtitle, no inline action pr-220-error-at-point-of-failure.png error under the boxes with the inline Resend pr-220-resend-confirmation.png the new resend copy Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.changeset/sign-in-errors-at-point-of-failure.md:11
- Changeset body structure: after the initial
**End users:**paragraph, the following paragraph should also be part of the per-audience section (and consistently labeled). Leaving an unlabeled paragraph makes the audience-specific formatting inconsistent with the repo’s changeset guidelines and can make rendered release notes harder to scan.
When the code is rejected because of too many wrong attempts, the message now carries a **Resend code** link beside it. That case wipes the stored code, so retyping cannot work and a fresh code is the only way forward; the standalone **Resend code** button below the form was easy to overlook. A simple mistyped code is unchanged — the boxes clear and refocus so you can just type it again.
Testing the previous commit surfaced the gap: entering a wrong code
shows "Invalid OTP" with no way forward, because the inline action was
gated on the "too many attempts" wording.
That gate was based on a wrong assumption — that "Invalid OTP" means a
typo, which retyping fixes. better-auth 1.4.18 throws it from two
places in email-otp/routes.mjs: the wrong-code comparison, and a
missing stored code. The second is reached in several states where
retyping cannot possibly work:
- after a lockout. TOO_MANY_ATTEMPTS *deletes* the stored value, so
it is reported exactly once; every subsequent submit falls through
to "Invalid OTP". The lockout gate therefore missed the very
attempts where the user is most stuck.
- after the code was consumed elsewhere, e.g. sign-in completed in
another tab.
- after expiry cleanup deleted the value, so a later submit reads
"Invalid OTP" rather than "expired".
The branch cannot distinguish those from a typo, so withholding the
action stranded the users who needed it most. Offering it always costs
a typo-ing user nothing — the boxes are cleared and focused for
retyping either way.
Drops needsFreshCode entirely; the gate is now just parLikelyDead(),
matching refreshResendVisibility().
Reported by @aspiers testing #220.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
These do not belong in the repo, only in the PR description.
Testing the previous commit surfaced the gap: entering a wrong code
shows "Invalid OTP" with no way forward, because the inline action was
gated on the "too many attempts" wording.
That gate was based on a wrong assumption — that "Invalid OTP" means a
typo, which retyping fixes. better-auth 1.4.18 throws it from two
places in email-otp/routes.mjs: the wrong-code comparison, and a
missing stored code. The second is reached in several states where
retyping cannot possibly work:
- after a lockout. TOO_MANY_ATTEMPTS *deletes* the stored value, so
it is reported exactly once; every subsequent submit falls through
to "Invalid OTP". The lockout gate therefore missed the very
attempts where the user is most stuck.
- after the code was consumed elsewhere, e.g. sign-in completed in
another tab.
- after expiry cleanup deleted the value, so a later submit reads
"Invalid OTP" rather than "expired".
The branch cannot distinguish those from a typo, so withholding the
action stranded the users who needed it most. Offering it always costs
a typo-ing user nothing — the boxes are cleared and focused for
retyping either way.
Drops needsFreshCode entirely; the gate is now just parLikelyDead(),
matching refreshResendVisibility().
Reported by @aspiers testing #220.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Screenshots for a pull request are review ephemera. They have no value to the codebase, they are binaries that git keeps forever, and GitHub already hosts images attached to a PR body for free. f4ff99f introduced docs/screenshots/ for #232 and it holds nothing but those two files, so removing them takes the directory with it. The three added for #220 were dropped from that branch before merge. The blobs stay reachable in history; purging them would mean rewriting main, which is not worth it for ~100KB. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
09bc80f to
ce3caef
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/auth-service/src/routes/login-page.ts:1122
- The comment in clearError() looks accidentally truncated ("Clearing after the" → "region is hidden …"), which makes the accessibility rationale harder to follow. Reword it into a single grammatical sentence across the wrapped lines.
// Empty the region before hiding it. Clearing after the
// region is hidden would mutate one that is already out of
// the accessibility tree, which some assistive tech reports
// as a stale announcement.
packages/auth-service/src/routes/login-page.ts:1332
- PR description says only failures that invalidate the stored code should show an inline Resend code action, while a plain mistyped code should keep the message without the action. This branch adds an inline resend for all non-expired verify failures when the PAR is still alive (the
else if (!parLikelyDead())branch). Either update the PR description to match the implemented behavior, or narrow this branch to only the error cases where resending is the intended recovery.
} else if (!parLikelyDead()) {
// Every other verify failure gets the same inline
// shortcut, for the same reason as the expired path: the
// standalone Resend button sits below the form and is
// easy to miss.
//
Two unreleased changesets from #220 name the button as "Resend code". Since this PR renames it, they would ship in the same release describing a control that no longer exists by that name. Also drops "the confirmation message that replaces 'Code resent!'" — that phrasing only made sense as a diff against the previous release, and the string it names is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Clear the previously entered verification code after a successful resend. The fresh code now starts with an empty control focused at its first position.
Changes
Testing
pnpm format:checkpnpm lintpnpm typecheckpnpm testpnpm test:coverageScreenshots
Before: a successful resend left stale digits in the OTP boxes.
After: the old digits are cleared and focus returns to the first box. (This shot predates the later commits on this branch, so its banner shows the older copy and position; both are covered below.)
Errors at the point of failure
Captured against live Railway previews — PR #229's for the before, which still serves the old layout, and this branch's own for the after.
Before: a rejected code reported itself above the subtitle, several elements from the boxes the user had just typed into, with no way forward but the standalone Resend code button further down.
After: the message sits between the boxes and Verify, where the user is already looking. Because this particular failure invalidates the stored code server-side, it also carries an inline Resend code action — there is nothing left to retype. A plain mistyped code keeps the message without the action, since retyping is the right recovery there.
Resend confirmation
After: the confirmation acknowledges the click, then warns that earlier codes have stopped working and points at the spam folder — the two things most likely to be wrong for anyone who got as far as resending.
Notes
Current-branch deployed E2E: run 30560777504.
Focused extraction and review of work originally proposed in [WIP] many usability fixes (needs untangling) #165.
PR auth: support mobile paste in segmented code input #204 introduces a single-input OTP controller; whichever PR lands second should retain this behavior through its
clearOtpBoxes()helper.Summary by CodeRabbit
Bug Fixes
Tests