Unbundle #165: hide Resend when sign-in can't recover + demo cookie outlasts OTP wait#187
Conversation
🦋 Changeset detectedLatest commit: 647475b 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.
|
📝 WalkthroughWalkthroughThe PR extends OTP heartbeat handling with PAR-expiry recovery, increases the demo OAuth state cookie lifetime to one hour, distinguishes expired OAuth sessions from authentication failures, and adds E2E coverage for both recovery paths. ChangesAuthentication session recovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant OTPPage
participant AuthService
participant DemoClient
User->>OTPPage: Enter emailed code
OTPPage->>AuthService: Check PAR heartbeat
AuthService-->>OTPPage: Return PAR liveness
OTPPage->>DemoClient: Submit OAuth callback
DemoClient-->>User: Redirect with session_expired or auth_failed
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Coverage Report for CI Build 29262960040Coverage increased (+0.1%) to 56.327%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
🚅 Deployed to the ePDS-pr-187 environment in ePDS
|
c1c2ba1 to
c4240cd
Compare
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>
…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>
c4240cd to
dcc9590
Compare
Expand the sign-in timeout release note to cover both keeping active code-entry flows alive and recovering clearly when a flow has expired. Rename the changeset to match the broader scope. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Pull request overview
This PR unbundles the Tier-1 OTP sign-in resiliency fixes from #165, ensuring users aren’t offered “Resend code” actions that cannot succeed after upstream PAR expiry, and extending the demo client’s OAuth-state cookie lifetime so it outlasts realistic email-OTP wait times while surfacing a more accurate session_expired error when state is missing.
Changes:
- Auth-service login page now proactively hides Resend code (replacing it with Start over) when the PAR is likely dead, reconciling on heartbeat ticks and
visibilitychange. - Demo client increases
oauth_statecookiemaxAgeto 1 hour and introducessession_expiredmessaging for missing OAuth state in the callback. - Adds/updates E2E coverage for resend-visibility behavior and demo cookie expiry behavior; adds changesets for both slices.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/demo/src/lib/session.ts | Adds OAUTH_COOKIE_MAX_AGE_SECONDS and a helper for resolving callback error codes. |
| packages/demo/src/app/components/LoginForm.tsx | Adds user-facing copy for the new session_expired error. |
| packages/demo/src/app/api/oauth/login/route.ts | Uses the shared max-age constant when setting the OAuth-state cookie. |
| packages/demo/src/app/api/oauth/callback/route.ts | Redirects to session_expired when the signed OAuth-state cookie is missing. |
| packages/demo/src/tests/session.test.ts | Unit tests for the cookie lifetime constant and error-code mapping. |
| packages/auth-service/src/routes/login-page.ts | Implements parLikelyDead() and toggles Resend vs Start over based on PAR liveness. |
| features/passwordless-authentication.feature | Adds scenarios for resend-button hiding and demo cookie expiry behavior. |
| e2e/step-definitions/auth.steps.ts | Adds Playwright steps to force PAR re-check and simulate demo cookie expiry. |
| .changeset/keep-sign-in-alive-and-recover.md | Changeset for the auth-service OTP-screen behavior. |
| .changeset/demo-cookie-outlasts-otp-wait.md | Changeset for the demo cookie lifetime + callback error surfacing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/demo/src/app/api/oauth/callback/route.ts (1)
52-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the inner
codevariable to avoid shadowing the OAuth authorization code.Line 39 declares
const code(the authorization code from the query string), and line 65 redeclaresconst codeinside theif (!stateData)block. Although the block returns immediately so there's no functional defect, the shadowing makes the code harder to read and is a footgun for future edits.♻️ Proposed rename
if (!stateData) { console.error('[oauth/callback] Missing oauth_state cookie') - const code = resolveCallbackErrorCode({ oauthCookiePresent: false }) - return NextResponse.redirect(new URL(`/?error=${code}`, baseUrl)) + const errorCode = resolveCallbackErrorCode({ oauthCookiePresent: false }) + return NextResponse.redirect(new URL(`/?error=${errorCode}`, baseUrl)) }🤖 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 `@packages/demo/src/app/api/oauth/callback/route.ts` around lines 52 - 67, Rename the block-scoped `code` variable in the `if (!stateData)` branch of the OAuth callback to a distinct error-code name, preserving the existing `resolveCallbackErrorCode` result and redirect behavior while avoiding shadowing the authorization `code` declared earlier.
🤖 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 @.changeset/demo-cookie-outlasts-otp-wait.md:
- Line 11: Update the changeset paragraph to remove the claim that
token-endpoint invalid_grant failures redirect to session_expired. State only
that a missing or expired oauth_state cookie redirects to session_expired, while
token-endpoint failures continue redirecting to auth_failed; retain the cookie
maxAge and user-facing message details.
In `@e2e/step-definitions/auth.steps.ts`:
- Around line 898-906: Update the stale cookie-expiry comments surrounding the
programmatic cookie deletion step to reference the new one-hour lifetime,
including replacing the 10-minute timing examples and describing the callback
outcome as redirecting with session_expired instead of auth_failed. Keep the
explanation of why the cookie is deleted programmatically unchanged.
---
Nitpick comments:
In `@packages/demo/src/app/api/oauth/callback/route.ts`:
- Around line 52-67: Rename the block-scoped `code` variable in the `if
(!stateData)` branch of the OAuth callback to a distinct error-code name,
preserving the existing `resolveCallbackErrorCode` result and redirect behavior
while avoiding shadowing the authorization `code` declared earlier.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62d26a85-449e-483d-9c9a-5fcab2cfef47
📒 Files selected for processing (10)
.changeset/demo-cookie-outlasts-otp-wait.md.changeset/keep-sign-in-alive-and-recover.mde2e/step-definitions/auth.steps.tsfeatures/passwordless-authentication.featurepackages/auth-service/src/routes/login-page.tspackages/demo/src/__tests__/session.test.tspackages/demo/src/app/api/oauth/callback/route.tspackages/demo/src/app/api/oauth/login/route.tspackages/demo/src/app/components/LoginForm.tsxpackages/demo/src/lib/session.ts
Correct the timeout release notes and E2E commentary, avoid shadowing the authorization code, and use a boolean cookie-presence check. Co-authored-by: Codex <noreply@openai.com>
|
(reply generated by Codex) Addressed the remaining actionable summary and top-level findings in
|
Apply the final review wording correction to the demo cookie changeset. Co-authored-by: Codex <noreply@openai.com>
|
@coderabbitai review Final head is ready for review. (request posted by Codex) |
|
✅ Action performedReview finished.
|
|
@coderabbitai review Please review the final green head ecc91fb. (request posted by Codex) |
|
✅ Action performedReview finished.
|
|
(reply generated by Codex) Addressed the CodeRabbit docstring-coverage summary warning in |
|
|
@coderabbitai review Please review final head 647475b when the review window opens. (request posted by Codex) |
|
✅ Action performedReview finished.
|
|
Saw this from testing with faked network disconnect then reconnect ~6m later: 
|
Summary
Unbundles the Tier 1 OTP sign-in resiliency work from #165 into a focused, reviewable branch. The PR covers three user-visible failure modes in two logical changesets:
Reconcile the existing heartbeat after interruption. The email-code screen already refreshed the upstream authorization request every three minutes before this PR. This change records when a heartbeat last succeeded and checks immediately when a backgrounded tab returns to the foreground, so the UI promptly reflects a request that expired while timers or networking were suspended.
Stop offering actions that cannot succeed. If the upstream authorization request or local auth flow has expired, Resend code is hidden and a single Start over action is shown. This avoids sending a fresh code that can no longer complete the OAuth flow.
Keep the demo client's callback state for the whole recoverable window. The demo's OAuth-state cookie previously expired after 10 minutes, with its timer starting before the OTP was issued. It now lasts one hour, matching the auth-flow lifetime. If that cookie is genuinely missing, the callback shows an honest “sign-in took too long” message instead of generic “Authentication failed” copy; a present but invalid cookie remains an authentication failure.
The heartbeat-liveness tracking and unrecoverable-flow UI are grouped in one changeset because they are complementary halves of the same behavior: use the existing heartbeat's result while recovery is possible, then stop offering Resend once it is not. The demo cookie/error handling has its own changeset because it affects a separate component and failure boundary.
Manual testing
Use the PR's Railway environment, either demo client, and an email inbox that can receive OTP messages. These scenarios use ordinary user/browser interactions; no test hooks, console commands, or developer-tools cookie editing are required.
1. A sign-in that really died is reconciled on return
Expected: returning to the tab triggers an immediate liveness check. Resend code becomes absent and exactly one Start over action is visible, without waiting up to three minutes for the next interval tick. Clicking it returns to the client cleanly; beginning a new sign-in then works normally.
This distinguishes the PR from earlier deployments: they already had the periodic heartbeat, but did not immediately reconcile on
visibilitychangeor hide Resend based on the age of the last successful heartbeat.2. A short network interruption does not falsely kill the flow
Expected: the immediate check succeeds, Resend code remains available, and the sign-in can complete. A transient network failure must not be mistaken for a definitively expired authorization request.
3. A resent code works after the old demo-cookie boundary
Expected: sign-in completes. This verifies that the demo's OAuth-state cookie no longer disappears at the original 10-minute boundary. The heartbeat itself is pre-existing behavior and is only a prerequisite for keeping this long test flow recoverable.
4. Missing demo callback state gets the honest error
Expected: the demo redirects to its sign-in page with “Your sign-in took too long to finish. Please sign in again.” It must not show the generic “Authentication failed” message.
Realistic alternatives: use a browser profile or privacy extension configured to clear site data for the demo origin when it is closed. This models users who selectively clear or automatically purge client-site cookies.
Automated test plan
origin/mainin a fresh worktree.pnpm format:check,pnpm lint, andpnpm typecheckpass.pnpm testpasses: 1,045 tests across 71 files.pnpm test:coveragepasses the ratcheted thresholds.@resend-hidden-when-par-deadand@demo-cookie-expiry.Relationship to #165
Carves the OTP/PAR liveness and demo callback-state fixes out of #165. The remaining OTP-UX adjacencies, accessibility polish, error-copy rewrites, handle-picker fixes, and Account Settings flash banners are independent follow-ups.
Summary by CodeRabbit