P1: fix(auth): clear the OTP after resending - #220
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 selected for processing (1)
📝 WalkthroughWalkthroughThe resend-code handler now clears previously entered OTP digits and focuses the first input. The passwordless authentication scenario adds coverage for partially entered expired codes and verifies the reset state after a fresh code is sent. ChangesOTP resend reset
Estimated code review effort: 2 (Simple) | ~10 minutes 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: d73d017 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 30584642228Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage remained the same at 57.281%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions3 previously-covered lines in 1 file lost coverage.
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. |
|
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, focus returns to the first box, and the success banner confirms the new code. The preview responses were controlled to isolate successful resend.
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