Unbundle #165: hide Resend when sign-in can't recover + demo cookie outlasts OTP wait#187
Draft
aspiers wants to merge 2 commits into
Draft
Unbundle #165: hide Resend when sign-in can't recover + demo cookie outlasts OTP wait#187aspiers wants to merge 2 commits into
aspiers wants to merge 2 commits into
Conversation
Previously the OTP screen always offered "Resend code", even when the upstream PAR row had silently lapsed (suspended tab, mobile background, heartbeat throttling). The user could click Resend, receive a fresh email, type the new code, and only then see "Sign in failed" — wasting their time on a code that could not have worked. The screen now never surfaces actions that cannot complete the flow: - Track lastSuccessfulHeartbeatAt; treat the PAR as dead once we cross upstream's 5 min AUTHORIZATION_INACTIVITY_TIMEOUT without a fresh ok ping (the upstream death point is exact — no margin needed). - Hide #btn-resend and surface a #btn-start-over (→ /auth/abort) the moment parLikelyDead() flips. Reconciled on every heartbeat tick (including transient ticks, so a stale-by-time case still hides the button) and on the visibilitychange event (so a backgrounded tab returning to focus reflects reality immediately). - Inline "Send a new code" action on the OTP-expired error now branches: parLikelyDead() → "Start over"; otherwise existing "Send a new code". This is the proactive UI complement to the existing reactive abort gate. Server-side enforcement of the same invariant on /email-otp/send- verification-otp and /sign-in/email-otp is a separate follow-up. Test: new @resend-hidden-when-par-dead scenario; full @otp-and-par-expiry / @par-heartbeat / @resend-after-par-dead / @otp-expiry suite still passes (7 scenarios, 78 steps). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: c4240cd 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.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Coverage Report for CI Build 28809119087Coverage increased (+0.01%) to 56.084%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
🚅 Deployed to the ePDS-pr-187 environment in ePDS
|
…doesn't
Two changes that close the bug-report hole on the demo client:
1. Bump oauth_state cookie maxAge from 600s to 3600s, matching
auth-service's auth_flow row TTL. The demo's cookie carries the
state, code verifier, token endpoint and issuer for the OAuth
callback to complete; if it expires before the user submits the
OTP, the callback can't find any of that and bounces silently.
600s was shorter than realistic OTP-form sit times (the bug
report was an 11-minute wait). Aligning with auth_flow's 60-min
budget means as long as the auth-service can still recover the
flow, the demo can too.
2. Distinguish "cookie missing" from "auth failed" on the callback.
Previously every silent-fail path bounced to /?error=auth_failed
("Authentication failed. Please try again."), which is misleading
when the sign-in itself succeeded — the user typed a fresh OTP
correctly, the auth-service issued the OAuth code, the demo just
couldn't finish because its own session cookie had aged out. Now
the cookie-missing branch redirects to /?error=session_expired
("Your sign-in took too long to finish. Please sign in again.")
so the user understands what happened without thinking they
typed the code wrong.
Test: new @demo-cookie-expiry @bug-report scenario clears the cookie
mid-flow and asserts the session-expired error surfaces.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aspiers
force-pushed
the
unbundle/otp-resend-par-liveness
branch
from
c1c2ba1 to
c4240cd
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Unbundles the two Tier 1 items from #165 into a focused, reviewable branch: the OTP sign-in fixes that defend against a user acting on a code (or a browser session) that has silently died during a long wait. These are the changes that motivated #165 in the first place; the other 18 changesets in that PR are separable follow-ups.
Two commits, cleanly separated:
fix(auth-service): hide Resend on OTP screen when sign-in cannot recover— once the upstream authorization request has lapsed, the OTP screen swaps Resend code for Start over, so the only action offered is one that can actually complete the flow. Reconciles on heartbeat ticks andvisibilitychange. (Cherry-picked from fix(auth-service): hide Resend on OTP screen when sign-in cannot recover #165ccf4ece.)fix(demo): cookie outlasts OTP form sit times + honest error when it doesn't— the demo's OAuth-state cookie expired atmaxAge: 600(10 min), exactly the OTP lifetime, so an email arriving near expiry could outlive the cookie and break the token exchange. Raised to60 * 60(1 h) to match the auth service'sauth_flowTTL, and the callback now surfacessession_expired(honest "took too long") rather than a genericauth_failed. (Cherry-picked from fix(auth-service): hide Resend on OTP screen when sign-in cannot recover #165a5cb519, which shipped without a changeset — one has been added here.)Order matters: commit 1 must land before commit 2, since both touch the shared e2e/feature files. Cherry-picking in this order auto-merges cleanly.
Why draft
session_expiredchangeset for the demo cookie change was missing in fix(auth-service): hide Resend on OTP screen when sign-in cannot recover #165 and is newly authored here — worth a review pass on the wording.Test plan
origin/mainin a fresh worktree; no conflicts in the correct order.pnpm format:check,pnpm lint,pnpm typecheckall green.pnpm test— 1025 unit tests pass (70 files).@resend-hidden-when-par-dead,@demo-cookie-expiry) in CI.Relationship to #165
Carves out 2 of the 20+ changes in #165. The remaining items (OTP-UX adjacencies, accessibility polish, error-copy rewrites, handle-picker fixes, Account Settings flash banners) are independent and will follow as separate PRs.