feat(auth): add Google OAuth onboarding and personalisation - #365
Merged
Conversation
- Add OnboardingGate component to manage user onboarding flow based on authentication and profile status. - Create OnboardingShell component for consistent layout during onboarding steps. - Introduce COUNTRY_OPTIONS and PRACTICE_AREA_OPTIONS for user selection in onboarding. - Develop OnboardingPracticePage with form handling for jurisdiction and practice areas. - Implement OnboardingProfilePage for user profile details input. - Add tests for onboarding practice and profile pages to ensure functionality. - Create AuthDividerUI and GoogleIconUI components for UI consistency. - Add OAuth dialog for Google sign-in in Word add-in with Supabase integration. - Implement message protocol for Google OAuth dialog communication.
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
…ng-review-fixes Review fixes for #365: gate exemptions, lossless autosave, migration fallback, add-in OAuth hardening
|
|
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.
Summary
Authentication and account settings
user_profiles.password_set_atand verifies it against Supabase Auth before recording itOnboarding and personalisation
PersonalisationFieldsimplementation used by onboarding and SettingsOnboarding lifecycle
onboarding_version0, identifying them as legacy-exemptNULL, requiring onboarding1when onboarding is completed or skippedDatabase changes
user_profilespassword_set_atplus a service-role-only function that verifies password capability fromauth.usersConfiguration
Google OAuth is enabled by default. Local installations can opt out with:
GOTRUE_EXTERNAL_GOOGLE_ENABLED=false[auth.external.google].enabled = falseWhen enabled, valid Google client credentials and the documented callback URI are required.
Verification
git diff --checkpassed