Repository navigation
Fix sign-in links, the OAuth callback and per-account state (T2: AUTH-01, 18, 17, 02, 19, 06, 07, 13) - #202
Merged
Merged
Conversation
…batch) Google and the emailed sign-in link now return to /auth/callback, which holds a loading screen until the code exchange settles and then opens the deep link or /login. PKCE flow ids are appended to redirects and passed to the exchange, so a failed resend or another tab's sign-in no longer deletes the verifier a pending link needs. Auth errors in the callback URL and failed exchanges show a localized message on /login instead of a console warning. Links already in inboxes (/?code=, /app?code=) are moved onto the callback route before the first render. Sign-out uses local scope. After a sign-out, /login returns only the same account to the page it left. The onboarding tour is tracked per account, and on phones it no longer blocks the header menu. The local confirmation email template is code-only with the code out of the subject. Fixes AUTH-01, AUTH-18, AUTH-17, AUTH-02, AUTH-19, AUTH-06, AUTH-07, AUTH-13. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ep link errors - AuthProvider keeps loading while a callback exchange runs even when the browser's stored session (another account) is announced meanwhile, so the callback page never opens `next` as the wrong account. - A callback error is no longer cleared by any SIGNED_IN (a session restore fires one). When the link fails but another session survives, the callback page opens that account and shows the failure as a toast. - The onboarding store validates its stored copy on every rehydrate (merge), not only on migration. - A tour left running never opens for an account that already finished it. - RequireAuth records the last account in an effect, not during render (docs/standards/async-and-state.md). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex review r2: clearing `code` and `sb_flow_id` before the exchange meant a reload during the spinner found no code and sent the user to /login with an unused code and verifier. The URL is now cleaned once the exchange succeeds, fails or times out; auth errors and empty callbacks are cleaned at once, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A timeout is not a final answer: the code may still be good, so the URL keeps it and a reload can retry, as main did before. Only a success or a definitive failure cleans it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…okup Codex review r3: - After a timeout the provider kept the code, but the callback page still navigated to /login and lost it. A timeout now has its own error kind: the page stays on the callback URL and offers Try again (a reload that reuses the code) and Back to sign-in. A late success still opens the app. The provider stays the only owner of stripping callback parameters. - confirmRestoredSession's user lookup had no deadline, and during a callback nothing else ends loading. It now times out after 4 s and, like any unanswered lookup, keeps the local session. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # public/theme-init.js
Fresh review (Claude, replacing Codex r4, which hit a 429): - While a callback signs an account in, the listener no longer checks or loads a session the browser already had. A dead stored account's lookup could sign out, or block, the account just signed in. init loads the outcome: the new account, or the stored one only if the callback fails. The blocked-session branch also respects callbackPending. - After a timeout the exchange's late answer is final: a success clears the timeout notice and opens the app; a failure shows its real reason instead of a retry that would report a missing verifier. Either way the used code leaves the URL, and only while the callback page is still showing. - The timeout page shows before an existing session would open the app, and its Back to sign-in keeps the deep link. - A dead stored session's sign-out is bounded, so it cannot hold startup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Follow-up review: after a timeout, init restored and checked the stored account while the late exchange could still sign someone else in. A late success during that check got overwritten by the stored account (and the new account's rows loaded under the old owner), and a dead stored account could block the late success. While the code exchange has no answer, even past the timeout, the listener now ignores auth-js's events. init applies the answer itself: the new account on success, or the stored account through the usual server check once the exchange definitively failed. After a timeout nothing is restored until the late answer arrives. A late failure sets its error only while the callback page is still showing. applySession now also does what the listener did for these sign-ins: keep Google's provider token for Drive export and drop the previous account's Crossfire draft. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…restore Follow-up review of 7f1fc05: - The listener ignored auth events until the slow exchange answered, so after the timeout a sign-in on /login was never seen (and never, if the request stalled for good). It now ignores them only until the exchange answers or times out. A late success arrives like any sign-in; it is applied directly only if a blocked restore dropped it. A late failure restores the stored session only if nobody signed in meanwhile. - restoreStoredSession drops its result when a different account was applied while the stored one was being checked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Follow-up review of 074c9fe (Low): the guard that keeps an account signed in during the stored session's check ran only for a live stored account. It now runs before the dead branch's drop as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedEnable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Next included review available in 11 minutes. View limit details
Note 🎁 Summarized by CodeRabbit Free
Comment |
This was referenced Oct 23, 2026
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
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.
Fixes the sign-in bugs from the 22 Oct QA report on prod: AUTH-01, 18, 17, 02, 19, 06, 07 and 13. Most of the change is in
AuthProvider, plus a new/auth/callbackroute.Before you merge: Supabase Auth redirect allowlist
These are not changed by this PR. Add them in the Supabase dashboard (Authentication → URL Configuration → Redirect URLs):
uziwdtkzofptihahfooo):https://debate.pi-stack.org/auth/callback**brifeyypjauyusmkptkx):https://debatelab-staging.vercel.app/auth/callback**The
**matters. Redirects now carry?sb_flow_id=…and sometimes?next=…, and an exact entry stops matching once a query is added. GoTrue also accepts any redirect on the Site URL's own host, so if prod's Site URL ishttps://debate.pi-stack.orgthe callback works before the entry is added. Until then, a host that isn't the Site URL host (previews, the tailnet) falls back to the Site URL. The existing preview wildcards (…vercel.app/**, tailnet:91*/**) already cover/auth/callback.What changed, per issue
AUTH-01 (High), the emailed link lands on the homepage signed out. I reproduced this first in the local stack on main's code by sending a code, forcing a 429 on Resend, then opening the emailed link. It failed with
AuthPKCECodeVerifierMissingError, swallowed as[Auth] init failed, and ended on/signed out. NowsignInWithOtpsetsemailRedirectToto/auth/callback. The redirect carries this sign-in's own flow id, and the exchange uses that flow's verifier (see AUTH-18). A failure shows a localized message on/logininstead of a console warning.AUTH-18 (Medium), every flow shares one PKCE verifier. The client sets
experimental.appendPkceFlowIdToRedirects(auth-js 2.114). The exchange passes{ flowId }fromsb_flow_id, so a failed resend or a Google sign-in in another tab only drops its own verifier.AUTH-17 (Medium), /login flashes after Google, and a first sign-in can stick. Google and email links return to
/auth/callback, which RequireAuth does not guard.AuthProviderholdsloadinguntil the code exchange answers. The page shows "Signing you in…" and then opensnext(default/app).How the callback works:
/loginworks.AUTH-02 (Medium), a used or expired link shows nothing.
error/error_codein the callback URL (query or fragment) and failed exchanges map to localized messages (EN/ES/HU):otp_expiredand flow-state errors: "This sign-in link has expired or was already used. Send a new code."They show on
/login. If the browser still has another session, that account opens and the message shows as a toast.Links already in inboxes.
/?code=,/app?code=and error-only variants are moved onto/auth/callbackbefore the first render (mountApp). They still sign in, which I checked in the real stack. They carry no flow id, so an old link plus a failed resend still fails, now with the "only works in the browser where you asked for it" message instead of silently.AUTH-19 (Low), sign-out is global.
signOut()uses{ scope: 'local' }.AUTH-06 (Medium), the next account lands on the previous account's last page. There was no persisted route store. The leak was the
fromthat RequireAuth puts in the/loginhistory state when the session ends. RequireAuth now stamps that state with the account that was signed in, and LoginPage honorsfromonly for that same account. A different account lands on/app; the same account returning after an expired session goes back to its page.AUTH-07 (Medium), the tour shows only for the first account and blocks taps on phones. The onboarding store is per account (
completedUserIds, persist version 1, validated on every rehydrate). The old ownerless "done" flag (v0) goes to the next account that signs in, so existing users don't see the tour again, and every other account sees it once. Belowlg(where the header menu is the navigation) the tour has no overlay, and its tooltip sits under the header and the open menu. I checked that the menu opens and navigates while the tour shows.AUTH-13 (Nit), the code is in the email subject.
supabase/config.tomladds[auth.email.template.confirmation](a first email sign-in sends this one) with the subject "Your DebateLab sign-in code" and a code-only body, matchingmagic_link. There is no invite flow (inviteUserByEmailisn't used), so no invite template. Prod templates are being changed separately; nothing here touches Supabase settings.Also,
vercel.jsonrewrites/auth/callbackto the app shell withnoindex, andtheme-init.jstreats it as a private app-shell route.docs/architecture.mddescribes the callback. Theverify-debatelabskill has two new recipes and updated feature docs.Verification
Local only; GitHub Actions minutes are used up.
npm run verifyon the final headf291919passes (exit 0). It ran docs check, lint, typecheck, 500 unit tests, 30 collaboration tests, 406 edge tests, the build, the service worker check and 43/43 desktop@smokee2e tests, with no flakes. Earlier heads had one unrelated flaky e2e each (free-toolstimer,admin-announcements); both passed on retry.Real app,
verify-debatelab(local Supabase Auth + Mailpit), desktop-chromium and mobile-chromium, on the first version (b046055). Allrecipes/auth.spec.tsrecipes pass (8/8):auth-email-link: Send code, Resend forced to 429, then the emailed link (/verify?token=pkce_…, the link prod's email carries). It signs in on/appwith/auth/callbackin the path history and no/login. Reopening the used link shows the expired message on/login.auth-switch-account: Sign out sendslogout?scope=local. A second account signs in on the same/loginpage, lands on/app(not/app/settings) and gets its own tour. On mobile the menu opens and navigates to History while the tour shows.recipes/current-debate.spec.ts(including the tour recipe),e2e/full-stack/first-run.spec.tsande2e/full-stack/auth.spec.tsalso pass./?code=link without a flow id still signs in. With a failed resend it ends on/loginwith the localized message. Before this PR it ended on/silently.Browser checks of the timeout paths on the final code (dev server with real auth-js, desktop and mobile, 4/4 pass). With
/auth/v1/tokenstalled, the page stays on/auth/callbackwithcodeandsb_flow_id, shows the message, and Try again sends a new exchange. With/tokenanswering after 18 s, the page shows the timeout and then opens/app/historyby itself.New unit tests cover:
nextallowlist (open-redirect cases) and the legacy rewrite;e2e/login.spec.tsnow expects the callbackredirect_towith a flow id, and its OTP mock matches the request's new?redirect_to=query.Not rerun on the final head. The review fixes after
b046055changed the callback's timeout, late-answer and event handling. I could not rerun the real-stack recipes on the final head, because another worktree held the shared local Supabase lock for over 90 minutes. Those paths are covered by unit tests and by the dev-server browser checks above, which use real auth-js. Run.claude/skills/verify-debatelab/bin/verify.sh drive recipes/auth.spec.tson the final head before merging.Review
Three Codex rounds on
codex_cpa/ gpt-6.1-sol, xhigh. The fourth hit Codex's 429 limit, so a fresh-context Claude (Opus) reviewer took over and rechecked each fix.b046055), 4 findings, fixed in3891981:SIGNED_INcleared the error;docs/standards/async-and-state.md.3891981), 1 finding, fixed in6b26b63+42eb5a5: (M) the code was stripped from the URL before the exchange settled, so a reload lost a pending sign-in. It is now stripped only after success or a definitive failure, and kept on timeout.1d718c7), 2 findings, fixed ind2b067a:/loginand lost the code. It now shows a retry instead.a84cd22): Codex returned 429 and the round was cancelled. The Claude reviewer found:17e13f0.17e13f0: (M) after a timeout, the stored account was checked while the late exchange could still sign someone in, so the wrong account could win. Fixed structurally in7f1fc05, so only the exchange's answer signs an account in. Reading that commit, I found two of my own regressions and fixed them in the same commit. Google's Drive token and the previous account's Crossfire draft had only been handled by the listener.7f1fc05: (M) after a timeout, auth events stayed ignored until the late answer, so a sign-in on/loginwas missed. (L) the stored-session restore could overwrite a newer sign-in. Both fixed in074c9fe.074c9fe: two Lows. The newer-sign-in guard now also covers the dead-session branch (f291919). The other is listed under known issues.Known issues (Low)
loadingwaiting after 4 s, but auth-js's local sign-out continues. If its request hangs, then finishes after the user has signed in to another account, it removes that newer session. That needs a hung/logout, a new sign-in in between, and, for the second trigger, a tab switch during a callback timeout with a revoked but unexpired stored session. Before this PR, the same hang held the spinner forever. auth-js offers no clean hook to cancel it.Open / needs a manual check
/auth/callback, a first-ever Google account) can't run locally or from automation. Check on staging or a preview once the allowlist entry exists.config.tomlcovers the local stack only. Staging needs the confirmation template applied the same way itsmagic_linkone was.🤖 Generated with Claude Code