feat(web): add onMutate optimistic setQueryData with field-level rollback to useDaemonConfigMutation (LUM-2213)#33191
Conversation
…ation (LUM-2213) Optimistically update the daemon config query cache before the server responds, eliminating the brief window where derived state (configChanged, save button enabled) reverts to stale values during the refetch window. - Add applyConfigPatch() pure utility: deep-merges a DaemonConfigPatch into a cached DaemonConfig, mimicking the daemon's merge semantics (omitted keys unchanged, null deletes at record-entry positions) - Add onMutate callback: cancels in-flight refetches, snapshots the current cache, applies the patch optimistically - Add onError callback: rolls back cache to pre-mutation snapshot - 15 unit tests for applyConfigPatch covering all merge edge cases Closes LUM-2213
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…UM-2213) Replace full-snapshot rollback with field-level rollback per STATE_MANAGEMENT.md concurrent mutation rule. - Add snapshotPatchedFields() — captures only the fields the patch touches, so onError restores just those fields via an updater function, preserving any concurrent mutation's optimistic data. - 5 new tests for snapshotPatchedFields including a concurrent mutation integration test proving field-level rollback preserves the other mutation's changes.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…2213-optimistic-setquerydata
There was a problem hiding this comment.
Vex APPROVE ✦
LUM-2213 — onMutate optimistic setQueryData with field-level rollback. Closes the cosmetic save-button-re-enable trade-off Devin explicitly deferred from #33171.
Verified at HEAD b7079d89:
- Lifecycle shape is TanStack canonical:
cancelQueries→ snapshot →setQueryData(updater form) →onErrorrollback →onSettledinvalidate. Matches the LUM-2173 sounds-mutation pattern (#33044) and the #33094 architecture work. - Field-level rollback is correct per STATE_MANAGEMENT.md:343-349.
snapshotPatchedFieldscaptures only the axes the patch touches;onErrorapplies that snapshot back throughapplyConfigPatch, leaving concurrent mutations' optimistic data intact. The integration test (field-level rollback preserves concurrent mutation) walks the exact A-fails-while-B-pending scenario the doc rule guards against. - Merge semantics mirror the daemon: services/profile-entry/
defaultnull = delete;callSitesnull = explicit reset-to-inherit (NOT delete) — and the snapshot path correctly storesnullfor previously-absent entries so rollback restores the same effective state. Testnull callSite sets value to null (reset override)pins this distinction. - Graceful degradation:
if (!assistantId) returninonMutatemeans the mutation still works during the assistant-list loading window (returns undefined context,onErrorno-ops). Compatible with the lazy-resolve path inmutationFn. - Authoritative refetch preserved:
onSettledstill usesresolvedId ?? assistantIdso the existing #33156 lazy-resolve invalidation behavior is intact — optimistic data always gets replaced by the server response. - Devin self-resolution loop verified: initial
69f0894chad full-snapshot rollback inonError; Devin's own BUG_pr-review-job (citing STATE_MANAGEMENT.md) caught it; fixed inf498b388by introducingsnapshotPatchedFields. Finding resolved at HEAD. - Codex 👍 at HEAD after Boss's
@codex reviewtrigger.
Anti-pattern grep on diff (added lines only):
- Zero
ascasts on runtime-boundary shapes - Zero non-null
!in production code - Zero
@ts-ignore/eslint-disable - Zero
|| 0fallbacks - Zero raw
Sentry.captureException(no telemetry changes)
Test coverage (252 new lines): 13 applyConfigPatch tests covering all 6 patch axes (services merge/delete, activeProfile, profileOrder replace, default merge/delete, profile-entry merge/delete/add, callSite merge/null-reset/add) + immutability + empty-patch + sparse-config edges. 5 snapshotPatchedFields tests including the concurrent-mutation rollback walk.
CI 7/7 green. mergeable_state: blocked is the standard Devin-last-pusher convention — bot APPROVE at HEAD satisfies branch protection.
Merging.
Prompt / plan
Closes LUM-2213. Follow-up to PR #33171 (LUM-2186, query-derived draft state).
PR #33171 introduced a query-derived draft state pattern where
useEffectauto-clears drafts when the server value converges. This works correctly but has a cosmetic trade-off: during the ~50ms cache refetch window betweenmutateAsyncresolving andinvalidateQueriescompleting,configChangedistrueand the save button briefly re-enables.This PR eliminates that window by optimistically updating the TanStack Query cache in
onMutatebefore the server responds.Why needed
When a user saves settings, the mutation sends a PATCH and then
onSettledcallsinvalidateQueriesto refetch. Between mutation completion and refetch completion, the cached config still holds stale values. Consumer-derived state (serverWebSearchMode,serverImageGenMode,effectiveActiveProfile) reads stale, causingconfigChangedto briefly betrue.Benefits
useEffectauto-clear from LUM-2186 fires immediately instead of after refetchonErrorWhy safe
onMutate/onErrorare new callbacks alongside existingonSettled. No existing behavior removed.applyConfigPatchandsnapshotPatchedFieldsare pure functions with 20 unit tests covering all merge and rollback edge cases.onErrorrestores only the fields this mutation touched (viasnapshotPatchedFields), not the full config snapshot. This follows the STATE_MANAGEMENT.md concurrent mutation rule — if mutation A fails while mutation B's optimistic update is in the cache, A's rollback doesn't clobber B's changes.onSettledrefetch is authoritative — the optimistic data is always replaced by the server's real response on settle, so any divergence between optimistic and real data is transient.assistantIdis undefined (loading window),onMutatereturns early and the mutation works without optimistic updates.Approach — TanStack Query optimistic updates
Follows the TanStack Query optimistic updates pattern and TkDodo's concurrent optimistic updates guidance:
onMutate: Cancel in-flight refetches → snapshot only the fields the patch touches (snapshotPatchedFields) → apply patch optimistically (applyConfigPatch)onError: Roll back only the changed fields using an updater function — concurrent mutations' optimistic data stays intactonSettled: Invalidate cache (existing behavior, unchanged)applyConfigPatchdeep-merge semanticsThe daemon uses deep-merge PATCH semantics:
nullat record-entry positions deletes the entry (profiles) or resets it (callSites)applyConfigPatchreplicates these semantics for the client-side cache. It handles all consumer patch shapes: service mode/provider changes,activeProfile,profileOrder, profile CRUD, and call-site override CRUD.snapshotPatchedFields— field-level rollbackPer
STATE_MANAGEMENT.md:343-349, full-snapshot rollback is unsafe when concurrent mutations are possible. SinceuseDaemonConfigMutationis shared across 5+ components (web-search-card,image-generation-card,language-model-card,call-site-overrides-modal,manage-profiles-modal), concurrent mutations are entirely possible.snapshotPatchedFields(config, patch)captures only the previous values of fields the patch will touch, returning aDaemonConfigPatch-shaped snapshot. On error,applyConfigPatch(cache, snapshot)restores exactly those fields via an updater function, leaving the rest of the cache intact.Alternatives considered and rejected
savedOverride) — Considered during LUM-2186 review. Adds complexity (three layers: server, saved, draft) without addressing the root cause. OptimisticsetQueryDatais simpler and eliminates the need.onError— Initial implementation saved the entire previous config and restored it wholesale. Rejected perSTATE_MANAGEMENT.mdconcurrent mutation rule — this would silently revert other mutations' optimistic updates. Replaced with field-level rollback viasnapshotPatchedFields.Test plan
applyConfigPatch(service merges, null deletion,activeProfile/profileOrderupdates,defaultmerge/delete, profile entry merge/add/delete, callSite merge/add/null-reset, empty patch, sparse config, immutability) + 5 forsnapshotPatchedFields(service-only snapshot, missing entries, profile-scoped snapshot, independent field snapshot, concurrent mutation integration test)Link to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
Requested by: @ashleeradka