fix(web): ref-in-useMemo bug, replace hand-rolled useQuery, extract IIFE, add tests (LUM-2072)

[devin-ai-integration]

Prompt / plan

Final PR in the LUM-2072 modal decomposition series. Implements 4 remaining findings from the architectural audit of call-site-overrides-modal.tsx that weren't covered by #33121 (which landed DRY-1, DRY-2, TYPE-1).

Changes

BUG-1: Fix ref-in-useMemo anti-pattern (pre-existing bug)

hasUnsavedDrafts memo read seeded.current (a ref) inside useMemo with an eslint-disable-next-line react-hooks/refs suppression. Refs are not part of React's dependency tracking — changing a ref does not trigger memo recalculation. The memo could return stale false when the seeding completed but drafts/persistedOverrides hadn't changed yet, causing the unsaved-changes guard to miss real edits.

Fix: Replace seeded.current with the existing reactive isSeeded state variable in the memo deps. Remove the eslint suppression. The seeded ref is kept only in the useEffect guard (correct usage — preventing re-seeding on re-render).

Reference: React docs — referencing values with refs — "Changing a ref does not trigger a re-render."

Root cause analysis

1. How did it get here? The original author used a ref to avoid re-renders when tracking the "seeded" flag, then read it inside a useMemo that needed reactivity. The eslint plugin flagged this correctly, but the suppression comment (// eslint-disable-next-line react-hooks/refs -- synchronous flag set before setState) rationalized it as safe because the ref is set synchronously before setState. That reasoning is wrong — the memo's staleness depends on its dependency array, not the timing of the ref write.
2. Warning signs missed? The eslint suppression itself was the warning sign. react-hooks/refs exists specifically to catch this class of bug.
3. Prevention: Don't suppress react-hooks/refs. If a value needs to participate in memo/effect dependency tracking, it must be state, not a ref.

TD-1: Replace hand-rolled useQuery with generated options

Manual queryKey: ["call-site-catalog", assistantId] and inline queryFn replaced with configLlmCallsitesGetOptions from the HeyAPI-generated TanStack Query options. Benefits:

• Canonical query key (deduplication, cache invalidation work automatically)
• Type-safe path parameters
• Consistent with every other query in the codebase

Reference: HeyAPI TanStack Query plugin docs

TD-2: Replace IIFE in JSX with pre-computed variable

The default-profile label was computed via an immediately-invoked function expression inside JSX ((() => { ... })()). Extracted to a defaultProfileLabel variable before the return statement — standard React pattern, easier to read and debug.

T-1: Add tests for pure helpers

13 tests covering isDraftActive, draftsEqual (exported from call-site-overrides-modal.tsx), and buildOrderedProfiles (from ai-utils.ts):

• Null/undefined/empty-object handling for draft activity checks
• Field-level equality with null vs undefined treated as equivalent
• Profile ordering: explicit order first, then extras alphabetically; missing entries skipped; empty inputs

Alternatives not taken

• Separate helpers file for isDraftActive/draftsEqual: Considered extracting to a dedicated call-site-helpers.ts. Decided against — these are 20 lines tightly coupled to the modal's draft semantics. Exporting from the component file is simpler and avoids a single-purpose file for trivial code.
• Removing staleTime/refetchOnWindowFocus overrides from the query: The generated options don't set these. Kept the overrides (staleTime: 60_000, refetchOnWindowFocus: false) because the call-site catalog is static configuration that changes only on daemon restart — aggressive refetching wastes requests.

Test plan

• bun test src/domains/settings/ai/ — 37 tests pass (13 new + 24 existing)
• bunx tsc --noEmit — clean
• bun run lint — 0 errors
• Pre-commit hooks (secrets, lint, typecheck) — pass
• CI — 7/7 green

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

[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

[linear]

LUM-2072

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

[vex-assistant-bot]

✦ APPROVE

Value: Closes the last 4 findings from the call-site-overrides-modal.tsx audit (LUM-2072 series) — eliminates a real ref-in-useMemo staleness bug, aligns the call-site catalog query with the generated HeyAPI options, and adds 13 tests for the pure draft helpers.

What this does: 4 mechanical fixes verified at HEAD 31d4a634:

BUG-1: ref-in-useMemo → dual-track ref + state (real bug fix)

hasUnsavedDrafts memo previously read seeded.current (a ref) inside useMemo with an eslint-disable react-hooks/refs suppression. Refs don't participate in dep tracking — the memo could return stale false when seeding completed but drafts/persistedOverrides hadn't changed yet, causing the unsaved-changes guard to miss real edits.

Fix at HEAD (lines 139, 187-188, 217-223): Added const [isSeeded, setIsSeeded] = useState(false) alongside the existing seeded ref. Effect now writes BOTH (seeded.current = true; setIsSeeded(true)) when seeding completes. Memo reads isSeeded with [isSeeded, drafts, persistedOverrides] deps. seeded.current stays in the effect ONLY (correct usage — guards against re-seeding on re-render). eslint suppression removed.

The dual-track pattern is the right shape: ref guards effect re-entry (synchronous read), state drives reactivity (memo deps). Future consumers can't blow off react-hooks/refs because the legitimate use case has a documented landing spot.

TD-1: hand-rolled useQuery → generated TanStack options

queryKey: ["call-site-catalog", assistantId] + inline queryFn swapped for ...configLlmCallsitesGetOptions({ path: { assistant_id: assistantId } }). Canonical key flows through dedup + invalidation automatically; type-safe path params; consistent with every other query in the codebase. staleTime: 60_000 + refetchOnWindowFocus: false overrides preserved (catalog is static daemon-restart-scoped config — aggressive refetch wastes requests). Same architectural win as #33056 / #33094 / #33121 — query keys generated from the OpenAPI spec, no infrastructure-imports-from-domain coupling.

TD-2: IIFE in JSX → pre-computed variable

Default-profile label computation lifted out of JSX into defaultProfileLabel declared before the return statement (lines 498-502). JSX collapses to {defaultProfileLabel && (<span>...</span>)}. Easier to read, no closure allocation per render, friendlier in stack traces. Standard React pattern.

T-1: 13 tests for pure helpers

New call-site-helpers.test.ts (+121, zero React imports). Covers: isDraftActive (null/undefined/empty/partial), draftsEqual (null≡undefined field equivalence, field-level diffs), buildOrderedProfiles (explicit order first, alphabetical extras, missing-name skip, empty inputs, name+entry spread). isDraftActive + draftsEqual exported from call-site-overrides-modal.tsx per the PR's documented decision — 20 lines tightly coupled to the modal's draft semantics, a single-purpose helpers file would be over-extraction. Same "substance, not line count" criterion #33091 documented.

Anti-pattern grep (full file at HEAD): zero as casts, zero non-null ! in production file, zero eslint-disable react-hooks/*, zero IIFEs in JSX. Test file has one result[0]! justified by a .length assertion two lines up — fine for tests.

Merge gate: Codex 👍 on PR description at HEAD (single commit, no SHA drift) + Devin "✅ No Issues Found" at HEAD + CI 7/7 green. Single Devin commit → no bot-verdict-staleness risk.

Vellum Constitution — Trust-seeking: a react-hooks/refs suppression rationalized as "synchronous flag set before setState" was masking a real staleness bug; this PR replaces the suppression with the right primitive (state for reactivity, ref for effect guard) and documents the pattern so the next consumer can't repeat the mistake.