refactor(web): split call-site-overrides-modal, fix audit findings (LUM-2228)#33352
Conversation
…LUM-2228) - Move isDraftActive, draftsEqual, CUSTOM_SENTINEL to call-site-helpers.ts - Extract CallSiteOverrideRow to call-site-overrides-row.tsx - Remove redundant availableProviders alias (use INFERENCE_PROVIDERS directly) - Stabilize row callbacks with useCallback (onDraftChange, onToggle) - Update test imports to point at call-site-helpers Modal: 639 -> 358 lines. Row: 155 lines. Helpers: 32 lines. 57/57 tests pass, typecheck clean, lint clean. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 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:
|
| id: string; | ||
| displayName: string; | ||
| description?: string; | ||
| defaultProfile?: string; |
There was a problem hiding this comment.
🟡 Unused defaultProfile prop defined, passed, but never consumed in CallSiteOverrideRow
The defaultProfile prop is defined in CallSiteOverrideRowProps (line 28), passed by the parent at call-site-overrides-modal.tsx:424, but is never destructured or used inside the CallSiteOverrideRow function (lines 40-49). This is dead code introduced in this PR, violating the AGENTS.md "Dead Code Removal" rule: "Proactively remove unused code during every change. Remove code your change makes unused, clean up adjacent dead code." The prop should either be removed from the interface and the call site, or destructured and used.
Prompt for agents
The `defaultProfile` prop is defined in `CallSiteOverrideRowProps` at call-site-overrides-row.tsx:28 and passed from the parent at call-site-overrides-modal.tsx:424, but it is never destructured or referenced in the `CallSiteOverrideRow` component body (lines 40-49). This violates the repository's dead code removal rule in AGENTS.md.
To fix: remove `defaultProfile?: string;` from the `CallSiteOverrideRowProps` interface in call-site-overrides-row.tsx:28, and remove the `defaultProfile={cs.defaultProfile}` prop from the JSX in call-site-overrides-modal.tsx:424.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed — removed defaultProfile from both the CallSiteOverrideRowProps interface and the parent JSX call site. It was passed through during extraction but never consumed by the row component.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…age, standardize form label tokens - TTS/STT cards: remove dead `saving` state and `Loader2` spinner that could never render (synchronous save wrapped in fake-async pattern) - Image-gen card: stop writing raw API key to localStorage (daemon already has it via provisionProviderKey; the localStorage copy was never read back) - Remove dead `LS_IMAGE_GEN_CREDENTIAL` constant - Standardize form label color token to `--content-tertiary` (matches design library Input component) from `--content-quiet` in TTS/STT/email-managed cards Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
There was a problem hiding this comment.
✦ APPROVE
Value: Cuts the Settings/AI call-site overrides modal from 639 → ~380 lines, lifts pure helpers + the per-row UI into focused modules, and clears four real bugs that were hiding in the same domain — closing the last big file from the Settings/AI decomposition arc.
What this does:
call-site-helpers.ts— pureisDraftActive/draftsEqual/CUSTOM_SENTINELextracted (already covered bycall-site-helpers.test.ts, import path updated).call-site-overrides-row.tsx— 112-line inline.map()body lifted into a real component with a clean prop contract (draft,profileOptions,onDraftChange,onToggle).- Modal collapses 4 row-mutator closures into a single
onDraftChange(id, draft | null)viauseCallback;handleToggleresolvesdefaultProfilefromgatedCallSitesinstead of threading it through props. - Drops the
availableProviders = INFERENCE_PROVIDERSalias. - Four bug fixes carved out by the audit:
- TTS/STT cards: removed dead
savingstate —handleSaveis synchronous (localStorage only), React batched both updates, so the<Loader2>spinner +disabled={saving}never actually rendered. - Image-gen card: stopped writing the raw Gemini key to
localStorageunderLS_IMAGE_GEN_CREDENTIAL— it was provisioned to the daemon viaprovisionProviderKeyand the LS copy was never read back. Constant removed. - Removed unused
defaultProfileprop fromCallSiteOverrideRowProps(Devin caught this on the first commit, fixed inf77a7bdf). - Form-label color token standardized
--content-quiet→--content-tertiaryacross TTS/STT/email-managed cards (matches<Input>and every other card in the domain).
- TTS/STT cards: removed dead
Anti-pattern / checklist sweep
- Runtime-boundary casts: none. No
ason API responses, localStorage reads, IPC, or parse output. - Stale refs:
LS_IMAGE_GEN_CREDENTIALgrep across the repo returns only the two files this PR is cleaning up — safe to delete the constant. - React hooks:
handleDraftChangeisuseCallback(_, [])over a stable setter — correct.handleToggledeps[gatedCallSites, orderedProfiles, queryComplexityRoutingEnabled]are accurate (no missing closures); row isn't memoized so identity churn is moot. - Row contract:
CallSiteOverrideRowis now pure — no store reads, no async side effects, all state flows down viadraft+ callbacks. Easy to memoize later if needed. - Saving-state argument: Verified —
handleSavebody is fully synchronous (setLocalSetting/removeLocalSetting/toast.successonly). React batches thesetSaving(true) → ... → setSaving(false)pair inside the same callback, so the intermediatetruerender never commits. Dead UI removal is correct. - localStorage cleanup:
provisionProviderKey("gemini", trimmed)already routes the key to the daemon; the LS write was orphaned. No data-migration concern — the key was write-only. - Color token migration:
--content-tertiarymatches design-library<Input>labels and the rest of the AI domain. Visual-only, no token-pair drift. - Territory check: Settings/AI domain — Boss's turf, closing out the recent decomposition arc (#33246/#33256/#33286/#33321). No collisions with active work (Vargas/SSE, Mahmoud/vembda, awlevin/schedule, pragunseth/ACP, emmiekehoe/vellum pair).
- Tech-debt callout: LUM-2239 (dissolve
reconcileFromDaemonConfig) is the right follow-up — defer until daemon-config endpoint gets a typed response schema.
Merge gate: Vex ✦, Codex 👍 on PR description, all 7 checks green (Lint / Type Check / Build / Test / Socket Security ×2 / aggregate). Last commit pushed by @ashleeradka, no Devin-last-pusher block. Merging.
Vellum Constitution — Distinct: a 639-line modal hiding three dead-code bugs and a token drift is the opposite of legible; this is what "distinct, well-shaped" looks like inside a settings domain.
Prompt / plan
Decompose
call-site-overrides-modal.tsx(639 lines) into focused modules per the 20-point architectural audit checklist. Also fix pre-existing bugs and consistency issues found during the holistic Settings/AI domain audit.Structural decomposition
call-site-helpers.ts— MoveisDraftActive,draftsEqual,CUSTOM_SENTINELfrom the component file to their own utility module (already tested bycall-site-helpers.test.ts)call-site-overrides-row.tsx— Extract the 112-line inline.map()body as a sub-component owning per-row conditional disclosure renderingavailableProvidersalias — wasconst availableProviders = INFERENCE_PROVIDERSwith no added value; useINFERENCE_PROVIDERSdirectlyonDraftChangeviauseCallbackto the row componentPre-existing bug fixes (found during domain audit)
savingstate —handleSaveis synchronous (localStorage only) but was wrapped in fake-asyncsetSaving(true)/setSaving(false)pattern. React batches both updates → component never renders withsaving === true→<Loader2>spinner anddisabled={saving}were dead UI. Removedsavingstate and dead spinner from both cards.provisionProviderKey("gemini", trimmed), the card also wrote the raw key tolocalStorageunderLS_IMAGE_GEN_CREDENTIAL. This copy was never read back (only written and cleared). Removed the dead localStorage write and deleted the unusedLS_IMAGE_GEN_CREDENTIALconstant.defaultProfileprop —CallSiteOverrideRowPropsdefineddefaultProfile?: stringwhich was passed from the parent but never destructured in the component. Removed from interface and call site.--content-quietfor form labels while all other cards (web-search, image-gen, profile-editor, call-site-overrides) used--content-tertiary. The design library's own<Input>component uses--content-tertiaryfor its label. Standardized to--content-tertiary.Tech debt tracked
reconcileFromDaemonConfigonce the daemon config endpoint gets a typed response schema.Result
Why this is safe
Test plan
bunx tsc --noEmitpassesbun run lintpassescall-site-helpers.test.tsstill passes (imports updated)Link to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
Requested by: @ashleeradka