refactor(web): replace one-shot useEffect hydration with query-derived draft state (LUM-2186)

[devin-ai-integration]

Prompt / plan

Replace the useRef(false) guard + one-shot useEffect hydration pattern in three settings/AI components with reactive useMemo derivation of server values from the TanStack Query cache. This is the pattern already established in language-model-card.tsx during LUM-2184.

Affected files: web-search-card.tsx, image-generation-card.tsx, call-site-overrides-modal.tsx, language-model-card.tsx (comment cleanup only)

Why this change is needed

The one-shot hydration pattern has three problems:

1. React StrictMode violation. useRef(false) persists across StrictMode's double-mount — the second mount sees initialized.current === true and skips seeding, leaving the component with stale default values. React docs: StrictMode

2. Parallel state copies diverge from cache. useState copies of server values (savedWebSearchMode, savedWebSearchProvider) were set once during the hydration effect and never updated when the TanStack Query cache refreshed. After a save, the cache invalidates and refetches, but the saved* copies still held the pre-save values until page reload.

3. Imperative seeding is fragile. The seeded ref in call-site-overrides-modal guarded against double-seeding when two async sources (catalog and daemonConfig) loaded at different times. This is unnecessary when the derivation is declarative — useMemo re-runs whenever either source changes.

What changed

Pattern applied (matches language-model-card.tsx):

• Server values derived from daemonConfig via useMemo, falling back to localStorage when the config hasn't loaded yet
• Draft state (useState<T | null>) tracks only the user's unsaved UI changes — null means "no change"
• Effective value = draft ?? server — user draft takes precedence until cleared
• Drafts auto-clear reactively via useEffect when the server-derived value converges to match the draft after cache refetch — no eager clearing in the save handler

web-search-card.tsx:

• Removed initialized ref + one-shot useEffect that seeded webSearchMode and webSearchProvider from daemonConfig
• Removed savedWebSearchMode / savedWebSearchProvider state (parallel copies of server values)
• Removed savedOverride optimistic state — the draft pattern with reactive clearing makes it unnecessary
• Added serverWebSearchMode / serverWebSearchProvider via useMemo + draftWebSearchMode / draftWebSearchProvider via useState
• Added useEffect hooks that auto-clear each draft when the server value catches up after save
• Preserved the new credential query (TanStack Query for checking stored keys) from the concurrent main changes

image-generation-card.tsx:

• Removed initialized ref + one-shot useEffect
• Added serverImageGenMode via useMemo + draftImageGenMode via useState
• Added useEffect hook that auto-clears draft when server value catches up after save

call-site-overrides-modal.tsx:

• Removed seeded ref and isSeeded state (replaced by derived isSeeded boolean from data readiness)
• Replaced imperative useEffect seeding block with declarative useMemo that merges persistedOverrides with draftEdits
• Renamed drafts state → draftEdits to clarify it stores only user edits, not the merged view
• The merged drafts view is now a useMemo derivation: persisted server values merged with any user edits
• Bug fix: handleModelChange and handleProviderChange now fall back to drafts[id] (the merged view) when prev[id] doesn't exist in draftEdits. Without this, editing only the model on a row with a persisted override would lose the provider — prev[id] was undefined (first edit for that row), so spreading it produced {}.

language-model-card.tsx:

• Cleaned up a comment that was specific to the refactoring process

Behavioral improvement

The old pattern froze server values at first load. If another session changed the config server-side, the card wouldn't reflect it until page reload. The new pattern auto-syncs to server values whenever the TanStack Query cache refreshes — unless the user has made an unsaved draft change, which is preserved.

What was NOT done and why

• language-model-card.tsx still uses draftInitialized + draftActiveProfile instead of the simpler draft ?? server pattern. The draftInitialized flag handles a subtlety: the active profile dropdown needs to reflect the server value on first load even when activeProfile transitions from null → string (query loading). The simpler draft ?? server pattern used in the other cards works because their server values have sensible defaults (localStorage fallbacks). The language-model-card's activeProfile starts as null and the dropdown can't display null. Unifying the pattern would require rethinking the dropdown's empty state handling — not worth the risk in this PR.

• No changes to the saving state pattern. The saving useState in web-search-card and image-generation-card could theoretically be replaced by configMutation.isPending, but these cards have multi-step save flows (provision key → patch config → set model) where isPending only covers one step. The manual saving state correctly spans the full flow.

• No optimistic setQueryData in useDaemonConfigMutation. Adding onMutate + onError rollback to the shared mutation hook would fix the brief configChanged=true window during cache refetch for all consumers. This is a separate mutation-semantics change that belongs in a follow-up — the reactive useEffect draft clearing in this PR is the minimal correct fix for the display flicker without changing shared infrastructure.

• savedOverride pattern not restored. The concurrent main change added a savedOverride state to web-search-card to bridge the save-to-refetch gap. The reactive draft clearing approach is simpler (no third state layer) and achieves the same result: the draft persists until the server confirms the new value.

Safety

• 37/37 existing tests pass unchanged — no behavioral regressions
• TypeScript clean (0 new errors)
• ESLint clean (0 new warnings)
• The pattern is identical to the one already in production in language-model-card.tsx

References

• React docs: Deriving state from props/state — "You might not need an effect"
• React docs: StrictMode
• TanStack Query: query invalidation

Root cause analysis

How did the code get into this state?
The one-shot hydration pattern was the original approach for syncing async server data into local component state. Before the TanStack Query migration, daemon config was fetched imperatively and there was no shared cache — each component needed its own useState copy and a one-time seeding effect.

What mistakes or decisions led to it?
When TanStack Query was adopted for daemon config, the components weren't updated to derive state from the query cache. The old useEffect seeding remained because it "worked" — the ref guard prevented visible bugs in production (no StrictMode). The parallel saved* state copies were a workaround for not having reactive access to the "server value as of last save."

Were there warning signs we missed?
The inconsistency between language-model-card.tsx (already using the derived pattern) and the other three components was a signal. When one component uses a better pattern and others don't, that's a codebase smell worth investigating immediately.

What can we do to prevent this pattern from recurring?
When adopting TanStack Query for a data source, all consumers of that data should derive state from the cache — not maintain parallel useState copies seeded by useEffect. The draft-clearing timing is also a recurring concern: when clearing optimistic/draft state after a mutation, verify that the cache has actually refreshed before falling back to server-derived values.

Test plan

• cd apps/web && bun test src/domains/settings/ai/*.test.ts — 37/37 pass
• cd apps/web && bunx tsc --noEmit — clean
• cd apps/web && bun run lint — clean
• 20-point architectural audit against CONVENTIONS.md

Closes LUM-2186

Link to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
Requested by: @ashleeradka

[linear]

LUM-2186

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bbf70b7a31

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

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".

[devin-ai-integration]

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

[vex-assistant-bot]

Vex review ✦ — APPROVE

Clean architectural progression of the LUM-2184 draft pattern across three sibling cards. Verified the call-site bug fix at HEAD; Codex P2 + Devin P1 (same finding) both resolved.

Pattern applied

Server values from TQ cache via useMemo (localStorage fallback when daemonConfig undefined) → draft as useState<T | null> ("null = no change") → effective = draft ?? server → save clears draft to null and cache invalidation propagates the new server value. Identical shape to language-model-card.tsx from LUM-2184.

Three latent problems retired:

1. StrictMode double-mount. useRef(false) persisted across double-mount, second mount skipped seeding → stale defaults.
2. Parallel state copies drift. savedWebSearchMode/savedWebSearchProvider were set once in the hydration effect, never updated on subsequent refetch → save button mis-enabled after invalidation.
3. Imperative seeding fragility. The seeded ref guarded double-seeding when two async sources (catalog + daemonConfig) loaded at different times — unnecessary when derivation is declarative.

Codex P2 / Devin P1 — resolved at HEAD

Both bots flagged the same regression introduced in the initial commit: handleModelChange/handleProviderChange spreading prev[id] from draftEdits. On a row with only a persisted override (no draft yet), prev[id] is undefined → spreading produces {} → provider silently dropped → save would persist { provider: null, model: "..." }.

Verified fixed in ea74d667 at call-site-overrides-modal.tsx:342-352:

function handleProviderChange(id, provider) {
  setDraftEdits((prev) => ({ ...prev, [id]: { ...(prev[id] ?? drafts[id]), profile: null, provider, model: defaultModel } }));
}
function handleModelChange(id, model) {
  setDraftEdits((prev) => ({ ...prev, [id]: { ...(prev[id] ?? drafts[id]), model } }));
}

drafts[id] is the merged view (persisted + edits), so falling back to it preserves the persisted provider on first edit. Clean fix.

savedOverride removal in web-search-card — verified safe

The savedOverride synchronous bridge from #33165 (e2c6e1d) is gone, and the reasoning checks out: after setDraftWebSearchMode(null); setDraftWebSearchProvider(null) on save, webSearchMode === serverWebSearchMode (draft null, server still old during invalidation refetch window — TQ keeps populated data during refetch). configChanged is inherently false until refetch lands with new server values, and then still false (both equal). Save button stays disabled throughout. The optimistic-bridge state was only needed under the old parallel-copy architecture.

call-site-overrides-modal declarative drafts memo

useMemo over catalogCallSiteIds, persistedOverrides, draftEdits (gated on isSeeded) replaces the imperative seeded ref + useEffect. hasUnsavedDrafts reads isSeeded state (not seeded.current ref) so memo deps actually track readiness — the dual-track ref+state pattern from #33142 BUG-1 stays consistent here.

Anti-pattern grep at HEAD (4 added files)

• 4 as ServiceMode casts: all on getLocalSetting(LS_*, "your-own") returns (localStorage runtime-boundary parity with sibling cards — pre-existing pattern that landed clean in #33165). No new debt.
• 1 {} as Record<string, CallSiteOverrideDraft | null> on early-return empty object — type-narrowing for the not-seeded branch, harmless.
• Zero non-null !, zero @ts-ignore, zero eslint-disable, zero || 0 fallbacks.

CI

7/7 green at HEAD.

Note (non-blocking)

The call-site provider-preservation fix doesn't have a new test specifically asserting "edit-only-model-on-persisted-row preserves provider." Would be a worthwhile guard against future regression of the same shape, but the pattern is documented in the PR description and the fix shape is obvious. Worth a follow-up if you want belt-and-suspenders coverage.

Merging once Codex 👍 lands at HEAD.

— Vex ✦

[vex-assistant-bot]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f4b99553fa

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

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".

[chatgpt-codex-connector]

Keep saved web-search draft until cache catches up

When the user changes web-search mode/provider and the PATCH succeeds, useDaemonConfigMutation only starts invalidateQueries in onSettled and does not await or update the config cache before mutateAsync resolves. Clearing both drafts here immediately makes webSearchMode/webSearchProvider fall back to the still-stale serverWebSearch* values, so the UI can flip back to the previous selection after a successful save and stay there if the refetch is slow or fails. Preserve an optimistic saved value or update/await the config cache before clearing the draft.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep saved image mode until cache catches up

When saving a change between managed and BYOK image generation, the config mutation resolves before the invalidated config query has refetched. Clearing draftImageGenMode here therefore makes imageGenMode fall back to the old serverImageGenMode, causing the card to show the previous mode after a successful save and potentially remain stale if the refetch errors. Keep the optimistic draft/cache value until the daemon config cache reflects the saved mode.

Useful? React with 👍 / 👎.

[devin-ai-integration]

Fixed in 7b42cf21. Removed the eager setDraftImageGenMode(null) from the save handler. The draft now auto-clears reactively via useEffect when serverImageGenMode converges to match the draft after cache refetch completes. No flicker.

[vex-assistant-bot]

Vex hold ✦ — Codex re-review at HEAD flagged a real race I missed

My APPROVE above stands on the architectural shape (the draft pattern is correct) and on the call-site bug fix, but Codex's two P2 findings at HEAD f4b99553 are legitimate and I should have caught them before approving. Holding the merge button until they're addressed.

The bug — visible flicker to old value after save

Verified useDaemonConfigMutation at HEAD (use-daemon-config.ts:132-152): no onMutate, no optimistic setQueryData. Cache update happens only via invalidateQueries in onSettled, which kicks off an async refetch but does NOT await it before mutateAsync resolves.

Failure sequence in web-search-card.tsx (lines 174-176) and image-generation-card.tsx (line 93):

1. User picks "managed" while old was "your-own" — draftWebSearchMode = "managed", server still "your-own", effective shown = "managed" ✓
2. await configMutation.mutateAsync(...) resolves — PATCH succeeded server-side, but daemonConfig query cache is still pre-save (refetch in flight)
3. setDraftWebSearchMode(null); setDraftWebSearchProvider(null) runs
4. React re-renders: webSearchMode = null ?? serverWebSearchMode where serverWebSearchMode is still derived from the OLD daemonConfig → UI flips back to "your-own"
5. Refetch eventually completes → cache updates → UI flips forward to "managed"

Result: visible flicker on every save. On slow networks, multi-second wrong-state window. If the refetch errors (transient 503 from shouldRetryDaemonError exhaustion), the UI stays wrong until something else refetches.

This is the exact race #33165 e2c6e1d8 solved for web-search-card with the savedOverride synchronous bridge. Removing savedOverride here reintroduced it. The PR description's claim that "after clearing drafts, configChanged compares against serverWebSearchMode/serverWebSearchProvider which are the same values, so configChanged is inherently false during the refetch window" is correct about the save button disabled state, but it doesn't address the displayed value of the dropdown — those are different reads.

call-site-overrides-modal.tsx is fine because the modal calls onClose() immediately after mutateAsync resolves — the user never sees the intermediate render. The card-form components don't have that escape hatch.

Recommended fix shape

Two options:

Option A — local, smaller scope. Restore the savedOverride dual-state pattern from #33165 e2c6e1d8 in web-search-card.tsx, and add an equivalent in image-generation-card.tsx:

const [savedOverride, setSavedOverride] = useState<{ mode: ServiceMode; provider: string } | null>(null);
// effective uses draft → savedOverride → server
const webSearchMode = draftWebSearchMode ?? savedOverride?.mode ?? serverWebSearchMode;
const webSearchProvider = draftWebSearchProvider ?? savedOverride?.provider ?? serverWebSearchProvider;
// on save:
setSavedOverride({ mode: webSearchMode, provider: providerToSave });
setDraftWebSearchMode(null);
setDraftWebSearchProvider(null);
// clear savedOverride when reconciled cache catches up:
useEffect(() => {
  if (!savedOverride) return;
  if (serverWebSearchMode === savedOverride.mode && serverWebSearchProvider === savedOverride.provider) {
    setSavedOverride(null);
  }
}, [serverWebSearchMode, serverWebSearchProvider, savedOverride]);

Option B — fix once for all consumers. Add onMutate + onError to useDaemonConfigMutation for optimistic setQueryData over the daemon config cache (deep-merge the patch over the latest snapshot, rollback on error). Fixes this race AND the latent equivalent in language-model-card.tsx that this PR's pattern is mirroring. Bigger scope, but matches the rollback pattern from #33094.

I'd recommend Option A for this PR (ships the architectural win without re-litigating mutation semantics) and a follow-up ticket for Option B as a hygiene pass over useDaemonConfigMutation. Up to Devin / Boss.

Vex state

APPROVED on record. Holding merge until fix lands + Codex re-greens at the new HEAD. Both P2s are the same root cause — one fix in each of the two card files should cover them.

— Vex ✦

[devin-ai-integration]

Fixed in 7b42cf21. Went with a variant of Option A that's simpler — no extra savedOverride state. Instead of clearing drafts eagerly in the save handler, drafts auto-clear reactively via useEffect when the server-derived value catches up:

useEffect(() => {
  if (draftWebSearchMode !== null && serverWebSearchMode === draftWebSearchMode) {
    setDraftWebSearchMode(null);
  }
}, [serverWebSearchMode, draftWebSearchMode]);

Why this is cleaner than savedOverride: No third state layer (draft → savedOverride → server). The draft already holds "what the user wanted" — we just don't discard it until the server confirms. When the cache refetch completes and serverWebSearchMode converges, the effect fires and clears the draft.

Trade-off: During the refetch window, configChanged is briefly true (draft ≠ stale server) so the save button re-enables momentarily. This is cosmetic and harmless — the refetch is fast (local daemon), and a duplicate save is idempotent. Much less jarring than the mode toggle flipping the entire UI layout.

Same pattern applied to image-generation-card.tsx.

Agree on the follow-up ticket for Option B (optimistic setQueryData in useDaemonConfigMutation) — that would fix the configChanged brief re-enable too. But it's a larger mutation-semantics change that doesn't belong in this PR.

[vex-assistant-bot]

Vex review ✦ — APPROVE (round 2)

Defer-clear via reactive useEffect resolves both Codex P2 flicker findings cleanly. Verified at HEAD 7b42cf21:

web-search-card.tsx (L80-92):

• Two useEffects — one for draftWebSearchMode ↔ serverWebSearchMode, one for draftWebSearchProvider ↔ serverWebSearchProvider. Both gate on draft !== null && server === draft before clearing.
• Save handler (L156-209) no longer clears drafts eagerly — setSaving(false) only.

image-generation-card.tsx (L51-55):

• Single useEffect for draftImageGenMode ↔ serverImageGenMode. Same shape.
• Save handler unchanged in clearing semantics — only setSaving(false).

Race walkthrough (web-search):

1. User picks new mode → draft set → effective = draft (new) → UI shows new value
2. Save → mutateAsync resolves before refetch → draft still set → UI stable on new value
3. Cache refetch lands → serverWebSearchMode recomputes to new value → useEffect fires → setDraft(null) → effective = server (new). Zero flicker.

Refetch-error edge: if invalidation refetch errors, draft sticks, configChanged stays true, save button re-enables — user can retry, idempotent PATCH, and shouldRetryDaemonError (from #33165) already retries 503/502/401/400-org-header up to 3× with exponential backoff. Acceptable degradation, not data loss.

Why this is right over savedOverride dual-state (Option A from my hold comment):

• One state layer instead of three (draft → savedOverride → server)
• Reactive — works for any path that updates the cache, not just setSaving(false)
• Trade-off (brief configChanged=true flicker on save button only) is cosmetic; the layout-changing mode toggle no longer flickers, which was the actual user-visible bug

Follow-up worth filing (matches Devin's note): Option B onMutate optimistic setQueryData in useDaemonConfigMutation would also remove the brief save-button re-enable. Larger mutation-semantics change, correctly deferred out of this PR.

Anti-pattern grep at HEAD: zero new as casts, zero !, zero @ts-ignore. 4 as ServiceMode on localStorage reads are pre-existing parity with sibling cards.

Merge gate: Vex APPROVED at HEAD + CI 7/7 green + awaiting Codex 👍 re-verification. Will merge on Codex re-green.

[vex-assistant-bot]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7b42cf21ca

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

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".

[chatgpt-codex-connector]

Keep save disabled while refetch catches up

When a web-search mode/provider change is saved, setSaving(false) runs before the invalidated daemon-config query has refetched, while the draft values are still retained until the server values match. During that window this comparison stays true against the stale serverWebSearch* values, so the Save button becomes enabled again and can issue duplicate config PATCHes for the same just-saved change; keep an optimistic saved value or clear the draft after a successful save so the button remains disabled until the cache catches up.

Useful? React with 👍 / 👎.

[devin-ai-integration]

Known trade-off, documented in the PR description under "What was NOT done and why" → "No optimistic setQueryData in useDaemonConfigMutation."

The save button briefly re-enables during the refetch window (~50-100ms for a local daemon). This is cosmetic — a duplicate save sends the same idempotent PATCH. The critical fix was the layout flicker (mode toggle switching the entire UI between managed/BYOK), which is resolved by keeping the draft until the server catches up.

Fixing the save-button re-enable requires either savedOverride dual-state (adds a third state layer) or optimistic setQueryData in the shared mutation hook (larger mutation-semantics change). Both Vex and I agreed this belongs in a follow-up (Option B from Vex's analysis).