fix(auth-service): stop "Invalid OTP" flash on successful sign-in - #134
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
🦋 Changeset detectedLatest commit: 9392adc 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughAdds a client-side Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Script
participant API as verifyOtp Endpoint
participant Nav as Browser Navigation
Client->>Client: onSubmit() checks verifying
alt verifying == true
Client-->>Client: return early (ignore)
else verifying == false
Client->>Client: set verifying = true, disable UI, label "Verifying..."
Client->>API: await verifyOtp(currentEmail, otp)
alt API returns error
Client->>Client: showError(error), clearOtpBoxes(), focus first box
Client->>Client: set verifying = false
else API returns success
Client->>Nav: window.location.href = '/auth/complete'
%% intentionally do not reset verifying or re-enable UI
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🚅 Deployed to the ePDS-pr-134 environment in ePDS
|
Coverage Report for CI Build 25197094103Coverage remained the same at 50.397%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…valid OTP" The verify form auto-submits when the 6th digit lands. A second submit triggered shortly after (Enter, mobile SMS autofill firing input on multiple boxes, paste+input race) called /sign-in/email-otp again with a now-consumed code; the response was "Invalid OTP", which rendered briefly while the first call's success-path redirect was unloading the page. Visible symptom: red "Invalid OTP" flash during an otherwise successful sign-in. Fix is a verifying-flag latch on the verify-form submit handler. The guard short-circuits before the in-flight state is set, so a duplicate event drops cleanly. The latch is left set on success — verifyOtp triggers a window.location.href redirect, and we don't want a late input/Enter event to re-open the form mid-navigation and fire a second verify on the consumed OTP. Adds three structural regression tests in login-page.test.ts that pin the guard placement and the error-only reset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sonar S7721 flagged the helper inside the OTP latch regression describe block. Move it above the describe so it isn't recreated per call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sonar S5852 hotspot on `/^\s*verifying = false;/gm`. The pattern isn't actually catastrophic (anchored, single quantifier), but the intent — count `verifying = false;` assignments — is clearer with String#split. Test 1 already pins the declaration count separately, so total substring count of 2 (decl + reset) is equivalent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After an invalid code the boxes stayed populated. The auto-submit handler fires whenever total length === 6, so editing a single digit (replace one wrong char) immediately re-submitted the still-mostly- wrong code on every keystroke — fast enough to trip the per-IP rate limiter and lock the user out with HTTP 429. Clear the boxes on the error branch and refocus the first one. The length drops to 0; the user retypes 6 digits; auto-submit fires once when the 6th digit lands, as originally intended. Adds a regression test pinning the clearOtpBoxes() call inside the error branch and amends the existing changeset to describe both the flash fix and the spam fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8e2b013 to
56c6665
Compare
The "Invalid OTP" / "Code resent" banner was text-align:left inside a 100%-wide coloured box, leaving most of the row empty. Centre the text and reshape the CSS: - New base class .flash-msg (padding, radius, margin, text-align:center) - Modifier classes .flash-msg.error (red) and .flash-msg.success (green) - Container ships with class="flash-msg"; helpers toggle the modifier - Resend-success no longer overrides colour/background via inline style; showSuccess() adds the .success class instead The .flash-msg / .flash-msg.error / .flash-msg.success surface gives client CSS a stable hook to restyle either variant without fighting inline styles. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
End-user paragraph was overly verbose. Trim it. The flash-msg / error/success class surface is for client app developers writing custom CSS, not operators — move that note under the existing Client app developers audience instead of Operators. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply the skill's rules: - summary in plain language (End users is in the audience list) - per-audience sections don't restate the summary - dense End users section becomes bullets (3 distinct points) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
Fixed #125. |



Summary
This PR fixes two distinct OTP-verify UX bugs and clears two Sonar findings the new test code triggered.
Bug 1 — "Invalid OTP" red flash on a successful sign-in
The verify form auto-submits when the 6th digit lands. A second submit fired shortly after (Enter, mobile SMS autofill firing
inputon multiple boxes, paste+input race) called/sign-in/email-otpagain with the now-consumed code. The response was "Invalid OTP", which rendered briefly while the first call's success-path redirect to/auth/completewas unloading the page — visible as a red flash during an otherwise successful sign-in.Fix is a
verifying-flag latch on the verify-form submit handler. The guard short-circuits before any in-flight state is set, so duplicate events drop cleanly. The latch is left set on success —verifyOtptriggerswindow.location.href, and we don't want a late event to re-open the form mid-navigation.Bug 2 — Auto-submit spam loop after an invalid OTP
When the OTP really was wrong, the boxes stayed populated. The auto-submit handler fires whenever
hiddenCode.value.length === otpBoxes.length, so editing a single digit (replace one wrong char) immediately resubmitted the still-mostly-wrong code on every keystroke — fast enough to trip the per-IP rate limiter (60 req/min) and lock the user out with HTTP 429.Fix clears the boxes on the error branch and refocuses the first one. The length drops to 0, the user retypes 6 digits, auto-submit fires once when the 6th lands as originally intended.
Sonar fixes
The new regression tests tripped two Sonar findings, both addressed:
renderDefaulttest helper to module scope (was insidedescribe)./^\s*verifying = false;/gm) with aString#splitsubstring count. Same invariant pinned, no ReDoS surface.Commits
bce65b5fix(auth-service): drop duplicate OTP verify submits that flashed "Invalid OTP"3d659a2test(auth-service): hoist renderDefault helper to module scopeef55606test(auth-service): replace ReDoS-flagged regex with substring count56c6665fix(auth-service): clear OTP boxes on verify errorTest plan
pnpm test— 842 passed (40 inlogin-page.test.ts, including 4 regression tests pinning latch placement, error-only reset, and clear-on-error behaviour)pnpm lint— cleanpnpm format:check— cleanpnpm typecheck— clean/auth/complete🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Style