Repository navigation
feat(settings): persist account preferences across browsers - #5397
Conversation
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at 4f8d182. Backend side looks solid: the allowlisted strict model, session-source + expected-user fencing, per-key upsert with sorted lock order, and the idempotent 0023 migration all check out, and I ran the new/updated backend tests (test_user_preferences.py, test_auth_me_permissions.py, test_migration_0022) against the head commit — all green. Two important findings on the frontend wiring: the automatic-fallback fence only covers one of InputBox's two consumers, and the boundary gate drops the workspace from server rendering for signed-in users.
| mode: "flash" | "thinking" | "pro" | "ultra" | undefined; | ||
| reasoning_effort?: "minimal" | "low" | "medium" | "high"; | ||
| }, | ||
| options?: { automatic: boolean }, |
There was a problem hiding this comment.
The automatic flag only reaches one of InputBox's two consumers. frontend/src/app/workspace/agents/[agent_name]/chats/[thread_id]/page.tsx (line ~460) still passes onContextChange={(context) => setSettings("context", context)} and ignores options, and that setSettings is useThreadSettings(threadId) → updateThreadSettings, which routes the diff through preferenceEdit into the account outbox. The auto-selection effect fires on that page too — it prefers defaultModelName={agent?.model} (the agent's configured model, #4336) whenever the resolved model/mode differs from current context. Concrete scenario: a signed-in user whose account model_name is still unset opens any agent chat; the automatic fallback writes the agent's model into baseSettings, updateThreadSettings calls preferenceEdit, and the agent's model becomes the account-wide default via PATCH /api/v1/auth/preferences, synced to every browser. That is exactly the "automatic fallback must not enqueue account writes" class this PR fences on the main chat page, and the new unit/E2E coverage only exercises chats/chat-page.tsx. The agent page handler needs the same options?.automatic → resolveThreadContext split, ideally with a regression test on that page.
| return stop; | ||
| }, [owner]); | ||
| // Initialize the account cache before any composer consumes defaults. | ||
| if (owner !== activeOwner) return null; |
There was a problem hiding this comment.
For signed-in users this makes the workspace client-only. workspace/layout.tsx's authenticated branch supplies a non-null initialUser to AuthProvider, so during SSR owner is set while activeOwner is still its useState default (undefined) and useLayoutEffect never runs server-side — the boundary therefore returns null in the server-rendered HTML. The entire subtree that used to be server-rendered in workspace-content.tsx (sidebar layout, offline/model banners, chat page, CommandPalette, SettingsDialogHost, Toaster) now appears only after hydration + this layout effect + the setActiveOwner re-render, which regresses first paint on the primary authenticated route (and trips React's useLayoutEffect SSR warning in dev). If the gate only exists to keep composers from reading pre-init defaults, it may be enough to keep the tree server-rendered and rely on the synchronous cache-based activatePreferences running before paint client-side, or to gate only the client render rather than unmounting during SSR. At minimum, worth confirming this tradeoff is intentional.
willem-bd
left a comment
There was a problem hiding this comment.
Re-reviewed at 8b664b0 (delta since the previous review at 16b6f38 is one frontend-only commit). Both previously flagged important findings are fixed:
-
Custom Agent page no longer uploads automatic fallbacks.
agents/[agent_name]/chats/[thread_id]/page.tsxnow routesoptions?.automatictoresolveThreadContext(threadId, context)— in-memory only, no account PATCH and no masking thread override — while explicit model/mode/effort selections still go throughsetSettingsand sync. I verified these are the only twoInputBoxconsumers at head and both fenceautomatic; the user-action handlers (handleModelSelect/handleModeSelect/handleReasoningEffortSelect) correctly remain explicit. The newsettings-model-sync.spec.tsE2E asserts zero PATCHes for the agent-configured default and a changed-field-only PATCH for an explicit selection, plus the slow-hydration case where an automatic fallback must not mask a later server preference. -
Authenticated workspace stays in server-rendered HTML.
UserPreferencesBoundarynow seedsactiveOwnerwithuseState(owner)and aliases the effect touseEffecton the server, so SSR and initial hydration render the workspace with the settings store's safe snapshot; only a subsequent account switch gates the subtree until the new cache activates. The newuser-preferences-ssr.test.tsxasserts workspace markup inrenderToStringoutput withstartUserPreferencesnever called server-side, and the new hydration test renders server HTML intohydrateRootwithonRecoverableErrorand expects no mismatches.
Verified at a worktree pinned to 8b664b0: all 13 tests/unit/core/settings/ unit tests pass (including the new SSR/hydration cases) and tsc --noEmit is clean. The delta commit does not touch the backend, so the earlier green backend test run still stands. No regressions from the fixes themselves — nothing new to flag.
willem-bd
left a comment
There was a problem hiding this comment.
Two P2 findings from reviewing this revision. Validation: 55 focused backend tests and 13 frontend tests passed (3 backend tests skipped); two additional regression tests reproduced the issues below.
| const changes = Object.fromEntries( | ||
| Object.entries(newValue).filter( | ||
| ([field, value]) => oldValue[field as keyof Preferences] !== value, | ||
| ), |
There was a problem hiding this comment.
[P2] Sync only explicitly edited composer fields
When the account default is model B but an existing thread overrides it with model A, changing only reasoning effort sends the complete thread context through InputBox → updateThreadSettings. Comparing that context against baseSettings incorrectly includes model A in the account PATCH. A regression using the real settings store and preference boundary sends {model_name: "thread-model", reasoning_effort: "high"} when only {reasoning_effort: "high"} was selected. This uploads a thread-local model as the account default across browsers without a model selection. Please track explicitly edited composer fields separately from the thread-resolved context, and add coverage for changing mode/effort in a thread with a different model override.
|
|
||
| if (event.key === LOCAL_SETTINGS_KEY) { | ||
| baseSettings = getLocalSettings(); | ||
| if (!preferenceEdit) baseSettings = getLocalSettings(); |
There was a problem hiding this comment.
[P2] Preserve cross-tab updates for local-only settings
While account preferences are active, this guard ignores every update to the legacy local-settings key, including projectsDisplayMode and tokenUsage, which still remain device-local. Changing the project display from flat to grouped in another tab leaves this tab's baseSettings at flat; its next settings write can overwrite the other tab's change through persistLocalOnlySettings. A regression dispatching the corresponding storage event reproduces the stale value. Please merge the local-only fields from storage while preserving the four account-managed fields, including on storage-clear events.
willem-bd
left a comment
There was a problem hiding this comment.
Re-reviewed the delta since 9df9efd (one frontend-only commit, 3ddde5c). Both P2 findings from that round are fixed:
-
Explicit composer edits no longer upload thread overrides.
onContextChangeis nowPartial<...>andhandleModelSelect/handleModeSelect/handleReasoningEffortSelectsend only the fields the action changed, so thepreferenceEditbefore/after diff carries exactly those fields into the PATCH. I re-verified bothInputBoxconsumers still routeoptions?.automatictoresolveThreadContext(in-memory, no account write) and explicit selections throughsetSettings; the newsettings-model-sync.spec.tscases for effort and mode edits on both the normal and Custom Agent pages assert a changed-fields-only PATCH ({reasoning_effort: "high"}/{mode: "ultra", reasoning_effort: "high"}) whileserver.model_namestays at the account value. -
Local-only settings now merge across tabs.
readSharedSettings()re-reads the legacy key on storage write/remove/clear events for device-local fields (projectsDisplayMode,tokenUsage) while preserving the four account-managed fields from the active account cache, and no unguardedbaseSettings = getLocalSettings()assignment remains on storage paths. The newit.each(["write", "remove", "clear"])DOM test also pins that a later local write cannot restore the stale display value.
Prior fixes remain intact (untouched by this delta): the automatic-fallback fence on both consumers and the server-rendered authenticated workspace. Verified at a worktree pinned to 3ddde5c: all 26 tests/unit/core/settings/ unit tests pass (including the new cross-tab merge, SSR/hydration, and explicit-edit cases), both input-box unit suites pass, and tsc --noEmit is clean. The delta touches no backend code, so the earlier green backend run stands. frontend/src/AGENTS.md guidance was updated to match the new behavior and stays well under its size budget. Nothing new to flag.
Fixes #2595
Why
Clearing browser storage or moving to another browser loses the notification toggle, default model, conversation mode, and reasoning effort. These settings currently have no account-backed copy.
What changed
GET/PATCH /api/v1/auth/preferences. Independent(user_id, key)SQL rows preserve concurrent edits to different fields; the last committed write wins for the same field.nullresets a field.X-Expected-User-Id, cancel obsolete requests, and ignore stopped-account responses. Only the four allowlisted fields are transmitted; arbitrary agent context is excluded.0023_user_preferencesmigration, API documentation, and regression coverage. Existing unscoped local settings are not automatically uploaded because they have no known account owner; users reselect those four preferences once after upgrading. Static/auth-disabled mode, device notification permission, display settings, and thread-specific model overrides remain local.This starts from current main rather than reusing the closed #4954 branch. It covers the failed-bootstrap retry and unchanged-cache echo cases raised on that attempt.
Surface area
Screenshots / Recording
The entry point remains Settings → Notification and the existing composer selectors; there is no new visual control. Real Chromium verification against the replay Gateway exercised saving the toggle, clearing both browser stores and restoring it, and a second account retaining its own defaults.
frontend/tests/e2e/settings-sync.spec.tsmakes the storage-clear UI regression reproducible in the existing mock E2E suite.Bug fix verification
Added backend persistence/API/migration tests and frontend sync/DOM/E2E tests. New persistence/sync tests were run red before their modules existed. The real auth/CSRF test additionally caught UUID-versus-string account comparison, and the full backend suite caught repeated table creation during legacy bootstrap; both were fixed and rechecked.
Validation
ruff checkandruff format --checkacross the backend.pnpm check,pnpm format, all 1,422 unit tests, and production build.AI assistance
Tool(s) used: OpenAI Codex.
How used: Issue investigation, implementation, review of the resulting diff, and automated/local integration validation were performed by Codex. No human-review attestation is claimed.