Review fixes for #365: gate exemptions, lossless autosave, migration fallback, add-in OAuth hardening - #367
Merged
willchen96 merged 10 commits intoAug 22, 2026
Conversation
WHY THIS MATTERS A user who signs up, reaches onboarding, and closes the tab has onboarding_version = NULL. If that user later forgets their password, the recovery email link logs them in and lands on /reset-password — but OnboardingGate saw an authenticated, un-onboarded user on a non-exempt route and immediately replaced the URL with /onboarding/profile. The reset form never rendered, the password never changed, and the next login failed again: a permanent lockout loop. Replicated live during the PR #365 review: with onboarding_version set to NULL, navigating to /reset-password with a session redirects to /onboarding/profile before the form paints. WHAT IS A ROUTE-GATE EXEMPTION LIST OnboardingGate wraps every page and decides "does this user belong in onboarding right now?". Pages that are themselves part of an auth transition (login, signup, the OAuth callback) are listed as exempt so the gate never fights the auth flow. The bug was that the list only covered the *entry* flows; the *recovery* flows (/reset-password, /forgot-password, /verify-mfa) also run with a session that must not be interrupted. HOW THE FIX WORKS The three credential-recovery routes join the exemption list, so a recovery session can finish its job before onboarding resumes on the next normal navigation. A parameterised test pins each route as reachable for an un-onboarded user. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
…boxes WHY THIS MATTERS Two silent data-loss paths were replicated live during the PR #365 review of the new Personalisation settings page: 1. Tick "Other" under Practice areas and leave the text box empty, then change any OTHER field (e.g. Title -> Partner). Nothing is saved — no request fires, no status appears, and navigating away discards the change. The page-wide validationError guard paused the entire autosave effect, and the error string itself was never rendered, so the user had no way to know saving had stopped. 2. Pick a value (or finish typing) and click another Settings tab within the 400 ms debounce window. The effect cleanup cancelled the scheduled save on unmount, so the edit vanished. WHAT IS A DEBOUNCED AUTOSAVE EFFECT Each edit re-runs a React effect that schedules the PATCH 400 ms out; another edit inside that window cancels and reschedules. The cleanup that makes the rescheduling work is the same cleanup that runs on unmount — so without an explicit flush, leaving the page always cancels the last pending save. HOW THE FIX WORKS - The hook now reports WHICH field groups are mid-edit-invalid (invalidGroups) instead of a single page-wide error. The autosave effect only skips a save when the edited field's own group is invalid; for other fields it saves a payload in which the invalid groups fall back to their last persisted values, so a half-finished "Other" box can neither block unrelated saves nor overwrite its own stored value with a transient empty state. - The validation message is now rendered inline (aria-live), so a paused save is visible instead of silent. - The scheduled save closure is kept in a ref; the unmount cleanup fires it immediately instead of dropping it. Tests pin all three behaviours: unrelated-field saves proceed with the persisted fallback, the message renders, and unmount flushes the pending save. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
… edits WHY THIS MATTERS Replicated live during the PR #365 review: type a new Display Name, tab into Organisation (blur fires the name autosave), and keep typing. When the name PATCH resolves, the Organisation input snaps back to the stored server value mid-keystroke and the typed text is gone — while the field is still focused. WHAT IS A HYDRATION-EFFECT CLOBBER The page keeps local input state and "hydrates" it from the profile context in an effect. The old effect wrote BOTH fields whenever EITHER profile value changed. Saving one field on blur refreshes the whole profile object, so the effect re-ran and overwrote the sibling input with the (stale) server value. This was latent before this PR — with click-to-save, a profile refresh never landed while the user was typing in the other field; blur-autosave made the race the common case, and this PR also removed the truthiness guard that used to soften it. HOW THE FIX WORKS The single combined effect becomes two effects, each keyed only to its own profile value, and each skips syncing while its input is the active element — the profile can never overwrite what the user is currently typing. When the field blurs, its own save handler reads the input state directly, so nothing is lost by skipping the sync. A regression test drives the real sequence (type name -> focus organisation -> type -> name save resolves with a refreshed profile) and asserts both inputs keep their typed values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
Two backend hardenings from the PR #365 review, both in user.ts. ── 1. Profile reads on databases without the 20260821 migrations ── WHY THIS MATTERS Replicated live by dropping the six new user_profiles columns (the state of any deployment that updates the backend before running migrations): GET /user/profile returned 200 but the user's saved legal_research_us=false and quick_actions_visible=false came back as true, model preferences fell back to defaults, and PATCH /user/profile returned a raw 500 leaking PostgREST internals. Settings appeared to reset themselves. WHAT IS THE FALLBACK-TIER CASCADE selectProfile tolerates older databases by retrying with smaller column lists. Each tier is entered by matching the NAME of its newly added column inside the Postgres 42703 (undefined column) error. Postgres reports only the FIRST unknown column — after this PR that is "jurisdiction" — so none of the existing tiers matched and the code fell through to a select that predates the model/research/quick-action columns, then papered over the gap with defaults. HOW THE FIX WORKS A new first tier keys on any of the six 20260821 columns and retries with PROFILE_SELECT_NO_ONBOARDING — the exact pre-PR column list — so every previously saved preference survives. serializeProfile already treats the absent onboarding fields as "legacy exempt" (version 0), which matches what the migration's backfill would write, so existing users are not funneled into onboarding by a missing migration either. getUserModelSettings gets the same treatment: on 42703 it retries with the pre-migration three-column select instead of silently switching every chat to default models and re-enabling US legal research. The test harness's per-table stub now accepts a queue of results so a test can express "first select fails with 42703, the retry succeeds", and a regression test pins the preserved preferences. ── 2. 200-character cap on displayName / organisation ── WHY THIS MATTERS This PR starts injecting both fields into the system prompt of every chat, project chat, and Word chat turn. Every other personalisation field is bounded (jurisdiction <= 100, practice areas <= 20 x 100, titles allowlisted), but these two only had a cap inside the signup trigger — PATCH /user/profile accepted arbitrary sizes, letting one oversized value inflate token cost on every message thereafter. HOW THE FIX WORKS validateProfilePayload rejects values over 200 characters (the same limit handle_new_user applies via left(..., 200)), with tests for both fields. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
WHY THIS MATTERS The Google OAuth dialog created a Supabase client with persistSession + storage: window.localStorage. After the PKCE code exchange, the user's full session — access AND refresh token — was written to localStorage on the add-in's origin. The cleanup that removed it ran only on the happy path, and even then the client's auto-refresh timer stayed alive and could re-persist the session after the key was deleted. Any crash or close between the exchange and cleanup left long-lived credentials on disk, readable by any script on the origin. This contradicts the invariant the add-in itself enforces elsewhere: tokens live exclusively in OfficeRuntime.storage, never browser storage (office-mock.ts fails tests that violate it). WHAT IS THE PKCE PERSISTENCE ACTUALLY FOR The dialog's Supabase client needs persistence for exactly one thing: the PKCE code verifier must survive the same-tab redirect to Google and back. That is a session-scoped, single-tab lifetime — sessionStorage's exact semantics. Nothing about the flow needs the minted session itself to be persisted at all. HOW THE FIX WORKS - storage: window.sessionStorage (+ autoRefreshToken: false), so nothing outlives the dialog window even on a crash. - After a successful exchange the dialog signs the temporary client out with scope "local" — clearing its persisted copy and refresh timer without revoking the session the task pane is adopting — before handing the tokens over. - clearTemporaryAuthStorage also sweeps the old localStorage keys so sessions persisted by earlier builds are removed. - messageParent is now guarded: if the host rejects the DialogOrigin-1.1 messageOptions overload it retries the legacy same-domain form, and on total failure it shows an actionable message instead of claiming "Signed in" while the task pane hangs. The success status is only shown after the handoff call returns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
WHY THIS MATTERS Clicking "Continue with Google" replaced the entire task pane with the bootstrap "Loading…" spinner for the lifetime of the OAuth dialog — no explanation, no cancel, no way back to password login. On Word on the web the dialog opens as a real browser popup that can land behind the window or be blocked, leaving the pane indistinguishable from a hung startup. The login page's own "Continuing…" button state was dead code: it could never render because the page unmounted first. WHAT WENT WRONG MECHANICALLY signInWithGoogle set the store-wide `_loading` flag — the same flag App.tsx uses for the one-time token bootstrap, checked BEFORE the "no token -> LoginPage" branch. Setting it synchronously unmounted LoginPage in the same React batch that tried to show the button spinner. HOW THE FIX WORKS signInWithGoogle no longer touches `_loading`. The login page stays mounted; its local googleLoading state (already wired) shows "Continuing…" on the button and re-enables everything when the flow resolves either way. Success still lands in writeSession, which broadcasts the new token and swaps the pane to the app shell exactly as before. This also removes the stuck-spinner path where a superseded writeSession left `_loading` true forever with no error. All 30 add-in auth e2e tests (chromium + webkit), including the five Google OAuth scenarios, pass against this change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
|
|
…ialog
WHY THIS MATTERS
Two independent verification agents auditing the previous commit both
proved the same severe regression: the cleanup call added there —
`supabase.auth.signOut({ scope: "local" })` — is NOT local. In the
pinned @supabase/auth-js, _signOut always calls
admin.signOut(accessToken, scope), which POSTs `/logout?scope=local`
to GoTrue with the session's JWT. GoTrue's handler deletes that
session row and revokes its refresh token server-side — the exact
refresh token the dialog hands to the task pane two lines later.
Google sign-in would have appeared to work, then silently logged the
user out at the first token refresh (~1 hour), with no test able to
catch it because the e2e suite stubs the dialog file out entirely.
WHAT "LOCAL SIGN-OUT" ACTUALLY MEANS IN GOTRUE
scope=local revokes the CURRENT session only (vs. "global" = all of
the user's sessions, "others" = all but this one). "Local" refers to
which sessions are revoked, not to whether a network call happens —
a subtlety the removed comment got exactly backwards.
HOW THE FIX WORKS
Delete the call. It was also redundant: clearTemporaryAuthStorage()
on the next line removes the persisted session keys, and the client
is created with autoRefreshToken: false, so no refresh timer can
re-persist anything. Also removed: the option-less messageParent
retry, which both verifiers flagged as speculative (every supported
host has the DialogOrigin 1.1 set) and mildly harmful — dropping
targetOrigin feeds the task pane's fail-open origin check. A visible
failure message beats a weaker handoff.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
…ct split WHY THIS MATTERS An independent verifier measured that the focused-input guards added with the hydration-effect split introduced their own data-loss bug: if the profile finishes loading while the Display Name input is focused, the guard skips the sync, the input stays empty, and merely blurring the untouched field fires the autosave — comparing local "" against the freshly loaded stored name and overwriting it with an empty string. The same verifier proved empirically that the effect SPLIT alone fixes the original sibling-clobber bug: a name save never changes profile.organisation, so the organisation effect simply does not run when the name save's profile refresh lands. THE TEST WAS ALSO NOT PINNING ANYTHING The previous regression test resolved the mocked save inside user.click()'s microtask flush, so the race window never opened and the test passed even against the unfixed combined effect. It now holds the save in flight with a deferred promise, types into the sibling field, and only then lets the profile refresh land — it fails on the pre-fix code and passes on this one. HOW THE FIX WORKS Keep the two per-field effects (each keyed only to its own profile value); remove the refs and document.activeElement checks. Simpler, and strictly fewer failure modes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
…alid Other box WHY THIS MATTERS An independent audit of the previous personalisation-autosave commit found a residual hole in the same class as the bug it fixed: change Title (save armed, inside the 400 ms debounce), then tick an empty "Other" practice area. The effect re-ran, the guard keyed on the LAST edited field's group (now invalid) returned early, and the still unsaved Title change was silently dropped — never re-armed. HOW THE FIX WORKS The per-field group guard is removed entirely; it was redundant. The payload substitution already replaces invalid groups with their last persisted values, so when only the invalid group changed, the payload equals the persisted snapshot and no save fires — exactly what the guard achieved. When OTHER fields also changed, the snapshot differs and the save now fires, carrying the earlier edits with the invalid group held at its persisted value. One comparison does both jobs, and the FIELD_GROUP map is deleted. A deterministic test pins the exact sequence (Title edit -> empty Other tick -> flush) and asserts the Title change survives with the stored practice areas untouched. KNOWN LIMIT (deliberate) A valid area ticked while the Other box is empty stays visible in the UI but is excluded from payloads until the Other text is filled — the inline "Enter your other practice area" message shows why saving is partially held. Saving half-finished groups would be worse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
…etry Three corrections to the earlier "survive un-migrated databases" commit, all raised by independent verification agents. ── 1. A database missing ONLY migration 02 kept too little ── The single fallback tier keyed on all six 20260821 columns, so a database with migration 01 applied but not 02 (password_set_at) was retried WITHOUT its five live onboarding columns: personalisation read back empty and — worse — serializeProfile's legacy-exempt default made genuinely new users report onboardingComplete: true and skip onboarding. Postgres names the FIRST unknown column in a 42703, so the two states are distinguishable: an 02-only gap names password_set_at, an 01 gap names jurisdiction. There are now two tiers, most-migrated first, with a regression test for the 02-only shape asserting the live columns and NULL onboarding state survive. ── 2. The 200-character cap now truncates instead of rejecting ── Rejection contradicted the signup trigger (handle_new_user applies left(..., 200) silently) and would have permanently 400'd any over-long value written before the cap existed — with the server's detail string discarded by the settings UI, appearing as an unexplained failure. Truncation matches the trigger exactly. The test harness now records update payloads so the tests assert the actual truncated write, not just a status code. ── 3. getUserModelSettings retry is now explicit and tested ── The retry's second failure was silently swallowed (data stayed null by accident); that fallback-to-defaults is now an explicit branch, and the previously untested severe half of the un-migrated-DB bug — saved models and legal_research_us=false surviving the retry — is pinned by two new unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
willchen96
approved these changes
Aug 22, 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
3 participants
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.
Review-fix stack for #365 (renders as a diff on top of that PR). Every fix below was replicated live on the local stack before being fixed, and re-verified live against the fixed code.
How the base cases were replicated
Stack: repo compose services (gateway :54721), PR branch backend on :3001 and frontend on :3000, both PR migrations applied. Flows driven in Chrome; database claims proven with psql, not the UI.
onboarding_version = 1with all personalisation fields persisted. Legacy account (version 0) logs straight into the app, untouched by the gate. Simulated Google account (google identity row + magic link): gated into onboarding with Name pre-filled from Google'sfull_name, email editing blocked behind the "add a password" modal, Set-password flow flipspassword_set_atvia the service-role RPC (verified againstauth.users), and email editing unlocks after.Replications that motivated the fixes
onboarding_version = NULL, navigating to/reset-passwordwith a session bounced to/onboarding/profilebefore the form painted — a user who abandoned onboarding could never complete a password reset. Fixed: recovery routes (/reset-password,/forgot-password,/verify-mfa) exempt from OnboardingGate; re-verified the form now renders for the same user.practice_areasdespite immediate navigation.GET /user/profilereturned 200 butlegal_research_us=falseandquick_actions_visible=falsecame backtrue(the 42703 fallback cascade keyed on the wrong column name), and personalisation PATCHes 500'd raw. Fixed: a first fallback tier keyed on the six 20260821 columns + the same retry ingetUserModelSettings. Re-verified under the same fault: saved values preserved, onboarding reported legacy-exempt.localStoragewith a live auto-refresh timer — now sessionStorage +autoRefreshToken:false+ local sign-out before the handoff, andmessageParentis guarded instead of claiming success before delivery. Clicking "Continue with Google" also blanked the whole pane to a bare "Loading…" spinner for the dialog's lifetime — the login page now stays mounted with its (previously dead) "Continuing…" state. All 30 add-in auth e2e tests pass on chromium + webkit.Validation
tsc --noEmitclean; unit suite 535 passed;user.routesintegration file 46 passed (3 new tests)tsc --noEmitclean; 510 passed (6 new tests)e2e/auth.spec.ts30 passed (chromium + webkit)Tradeoffs / design decisions (stated so they can be vetoed)
/settings?emailChange=processed(the email-change callback target) is still gated for un-onboarded users — exempting/settingswholesale would open all of Settings to them, which felt wrong to decide unilaterally. The email change itself completes server-side; only the confirmation view is deferred.onboardingVersion: 0(legacy-exempt) — matching what the migration backfill would write, and failing open rather than trapping existing users in onboarding during a deploy window.messageParentretry dropstargetOriginonly for hosts that reject the DialogOrigin 1.1 overload; the legacy form is same-domain-only, which is the identical restriction expressed implicitly.Not addressed here (PR discussion)
GOTRUE_EXTERNAL_GOOGLE_ENABLEDto false or gate the button on a public flag is a product/config decision.{}); relabel vs. submit-on-skip is a UX call.POST /user/onboardingre-stamps legacy (version 0) users to 1 and has no state guard — depends on intended re-onboarding semantics./onboarding/profile(hardcoded at 5 call sites) before the gate bounces completed users to/assistant— cosmetic flash, larger refactor.passwordSet: falseas fact, so a Google user with a password sees "create a password" copy while the profile fetch is failing.text-xs→text-smacross 8 untouched modals,OptionPillduplicatingPillButtonUIoutsidefrontend/src/shared, add-in Google button markup duplicated) — worth a deliberate look or a split.🤖 Generated with Claude Code
https://claude.ai/code/session_01RuPCULDYgVsiCanRgbEy6W
Round 2 — adversarial verification of this PR's own fixes
Two independent agents (one empirical — ran the pre-fix vs fixed code and tests; one audit-only — worked purely from git diffs) were asked to refute every bug claim and every fix above. Their verdicts converged: all replicated bugs confirmed real (with severity corrections noted below), fixes for the gate exemption, debounce flush, backend fallback, and login-page state confirmed correct and minimal — and they proved three defects in this PR's own first round, now corrected in the four newest commits:
signOut({scope:"local"})in the OAuth dialog revoked the very session being handed to the task pane — auth-js always POSTs/logout?scope=local, and GoTrue deletes that session's refresh token server-side. Google sign-in would have died silently at the first token refresh, invisible to CI because the dialog file is stubbed out of e2e. The call is removed (it was also redundant); the speculative option-lessmessageParentretry is removed too since it mildly weakened the receiver's origin check.""over the stored name — measured empirically). The guards and refs are removed; the effect split alone fixes the original clobber, which was also proven empirically. The regression test was additionally shown to pass on unfixed code (the mocked save resolved too fast to open the race window) and now uses a deferred promise so it genuinely pins the fix.Also corrected from the audit: a database missing only migration 02 now keeps its live onboarding columns (dedicated fallback tier + test) instead of reporting new users as onboarding-complete; the 200-char cap truncates like
handle_new_userinstead of rejecting (rejection would have stranded pre-existing over-long values behind an unexplained error); thegetUserModelSettingsretry now handles a second failure explicitly and finally has tests.Severity corrections the verifiers made to the original findings, accepted as-is: the reset-password bug is a burned recovery link + dead end rather than a permanent lockout (the recovery session survives the redirect, so completing onboarding un-wedges it); on the profile route only the legal-research/quick-actions booleans were reset by the missing-migration bug — the model-preference reset was real only in
getUserModelSettings; and the dialog's storage cleanup did run on error paths, the true exposure being the crash window and refresh-timer re-writes.Post-correction gates: backend tsc + full unit suite green, 49 tests in the touched backend files (7 new this round); frontend tsc + 512 tests green; add-in typecheck + all 30 auth e2e green on chromium and webkit.