feat(settings): sync user preferences across authenticated clients - #4954
feat(settings): sync user preferences across authenticated clients#4954Beautyl0ve wants to merge 9 commits into
Conversation
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at head a252e92 (backend: 48 tests in test_user_preferences + persistence bootstrap/migration files pass locally via uv run pytest; frontend: the full new tests/unit/core/settings + layout-boundaries suite passes via rstest). The overall design is solid: the strict allowlist + extra="forbid"/strict=True schemas, owner derivation from the cookie (never the header), the conditional-update initialization, and the version/outbox race handling in UserSettingsSyncLifecycle all check out — I traced the activation-gap, stale-tab, account-switch, and Web-Lock-claim races and they are guarded. Findings below are one correctness gap in the cross-tab echo path plus two robustness notes. The deliberate 0015 revision gap over 0013 is already documented in the PR description and the migration docstring, so no comment there.
| const persisted = readUserSettingsCache(activeBaseSettingsUserId); | ||
| if (persisted === null) return; | ||
| baseSettings = fromPersistedUserSettings(persisted); | ||
| emitBaseSettingsMutation(); |
There was a problem hiding this comment.
This cross-tab echo emits a full-state patch (emitBaseSettingsMutation() with no key falls through to toFullUserSettingsPatch, which sends explicit null for every unset context field), and the backend treats null as "delete the key". That can erase server-side values this browser never hydrated: with two tabs open, tab A's scoped edit (e.g. a notification toggle → {notification} patch, model untouched) writes the shared cache; tab B then receives this storage event and PATCHes the entire local state, whose context is empty, so model_name/mode/reasoning_effort are sent as null — clearing a default model that another device set on the server after this browser hydrated. Nobody touched the model on this device, yet the other device's selection is gone (and the DOM test "the same account's tab cache still produces a synchronized mutation" pins the full-patch-with-nulls behavior). Since local edits go through updateLocalSettings(key)/updateThreadSettings(key) and produce section-scoped patches, the storage-event path could do the same by diffing the previous baseSettings snapshot against the freshly read cache and emitting only the sections that changed — that confines the echo to what actually changed and stops the null-clearing clobber.
|
|
||
|
|
||
| def _response(settings: dict | None, revision: int) -> UserPreferencesResponse: | ||
| validated = UserPreferences.model_validate(settings) if settings is not None else None |
There was a problem hiding this comment.
Suggestion (robustness/self-heal): _response re-validates the stored JSON with UserPreferences.model_validate, so a stored payload that stops validating — a future enum tightening (this repo already evolved tokenUsage shapes once), or a manual DB fixup — makes GET and PATCH return 500 for that user. Because UserSettingsSyncController.start() swallows every failure and never retries in-session, sync then silently dies, and the client can't recover via PUT (it only PUTs when GET returns settings: null, not when GET 500s). Degrading an invalid stored record to settings: None on GET (keeping revision) would let the normal first-writer-wins PUT re-initialize and self-heal, at the cost of one round-trip.
|
|
||
| async def merge_user_preferences(self, user_id: str, patch: dict) -> tuple[dict, int]: | ||
| """Merge with an optimistic revision CAS supported by SQLite/Postgres.""" | ||
| for _attempt in range(5): |
There was a problem hiding this comment.
Nit: the CAS retry loop does 5 back-to-back attempts with no backoff, and when it exhausts, PATCH surfaces as 409 which the frontend turns into a sticky writeFailed (only cleared by the next user mutation or the next page-load handshake). For a settings blob where contention is rare but possible across Gateway workers, a tiny await asyncio.sleep(...) with jitter between attempts would make exhaustion effectively unreachable; alternatively the frontend could clear writeFailed after a later successful GET.
|
@Beautyl0ve thanks for your contribution, please resolve the conflicts. |
Signed-off-by: 嗜鵼 <hy2010hy2010@qq.com> # Conflicts: # backend/app/gateway/AGENTS.md # backend/packages/harness/deerflow/persistence/migrations/AGENTS.md # backend/tests/test_migration_0004_run_ownership_dedupe.py # backend/tests/test_migration_0007_scheduled_run_active_dedupe.py # backend/tests/test_persistence_bootstrap.py # backend/tests/test_persistence_bootstrap_concurrency.py # backend/tests/test_persistence_bootstrap_regression.py
|
@WillemJiang Thanks for the reminder. I have merged the latest |
willem-bd
left a comment
There was a problem hiding this comment.
Two additional correctness issues found in the settings recovery paths.
| try { | ||
| const response = await this.transport.patch(patch); | ||
| if (response.settings === null) { | ||
| const recovered = await this.transport.initialize( |
There was a problem hiding this comment.
[P2] Reapply the pending patch after recovery initialization
The Web Lock only serializes tabs in this browser, so another device can initialize the record after this PATCH resets it. Because PUT is first-writer-wins, initialize() can then return that device's existing settings rather than store.getSettings(). This branch accepts any non-null result and subsequently acknowledges the original durable batch, even though patch was never applied to the winner; the user's local edit is then lost on the next hydration. Please reapply patch to the recovered record (or otherwise verify it is present) before acknowledging the batch.
There was a problem hiding this comment.
Addressed in 305cc713. After a recovery PATCH returns settings: null, the controller now replays the original pending patch after first-writer-wins initialization and acknowledges the durable batch only after that replay returns a non-null record. The regression test models another device winning initialization without this tab's edit.
| """Deep-merge allowlisted sections; JSON null clears optional fields.""" | ||
| merged = deepcopy(current) | ||
| for section, values in patch.items(): | ||
| target = merged.setdefault(section, {}) |
There was a problem hiding this comment.
[P2] Validate structurally corrupt settings before merging
The new repair path runs only after merge_user_preferences returns, but this merge assumes every persisted section is a dictionary. For example, a stored payload of {'context': []} makes the assignment below raise TypeError, so PATCH returns 500 before _validated_response can reset the bad revision. The current invalid-record test uses {'context': {}}, which remains mergeable and misses this case. Please validate/type-check the current record before merging, or make a malformed shape enter the reset/reinitialization flow.
There was a problem hiding this comment.
Addressed in 305cc713. The repository now detects a non-object stored root or patched section before deep-merging and CAS-clears the malformed revision, returning the normal settings: null recovery signal instead of raising TypeError. Added a route regression test with {"context": []}.
feat(settings): sync user preferences across authenticated clientsFixes #2595
Why
DeerFlow currently stores base UI/model preferences only in one browser's
localStorage. Clearing browser data or signing in on another device loses thosechoices. Uploading the existing blob directly would also be unsafe: it is unscoped,
contains state that must remain local, and can be observed by multiple accounts or
tabs on the same origin.
What changed
GET, first-writer-winsPUT, and nested-mergePATCH /api/user-preferencesendpoints over a strict browser-safe allowlist.users.preferencesJSON plus a revision counter in migration0014_user_preferences, branching directly from0013_mcp_task_notifications.defaults. Thread/workspace state, credentials, and browser/OS notification
permission never enter the contract.
cannot overwrite unrelated settings or acknowledge a newer unsent edit.
without that primitive keep writes local and pending instead of risking reordering.
request or activation-gap edit from erasing newer local state.
read/merge/CAS cycle for SQLite
SQLITE_BUSYandSQLITE_BUSY_SNAPSHOTfailures.claim a valid legacy browser value once; caches and pending mutations are user-scoped.
only from authentication and rejects a stale tab after another tab changes the
origin-wide cookie.
account isolation, concurrency, and data-boundary behavior.
Surface area
frontend/backend/applanggraph.json, or prompt changedocker/or sandboxed executionskills/backend/pyproject.tomlorfrontend/package.json(say what it buys us)Screenshots / Recording
There is no visual delta: existing settings controls keep their current appearance and
gain authenticated persistence behind the scenes. A real two-profile/browser recording
has not been attached.
Validation
The SQLite concurrency coverage uses two WAL connections and exercises the actual
SQLITE_BUSY_SNAPSHOTcode path while verifying that disjoint patches survive. Themigration coverage verifies both a fresh upgrade and downgrade/re-upgrade for existing
users.
Not run: live PostgreSQL transaction integration, a real two-browser/device smoke test,
or frontend build/E2E. PostgreSQL statements were reviewed and dialect-compiled.
Same-field concurrent writes remain last-successful-commit-wins.
Migration publication note: the latest merged
mainused by this branch ends at0013_mcp_task_notifications; this PR therefore claims0014_user_preferencesdirectly from it. Open PRs #4918
and #4843 currently contain
competing
0014revisions. Whichever PR lands later must rebase and renumber/rechainits migration before merge.
AI assistance
Tool(s) used: Codex
How you used it: Assisted with implementation, concurrency review, regression
tests, documentation, and local validation.