refactor(web): split manage-profiles-modal into focused modules (LUM-2226)

[devin-ai-integration]

Prompt / plan

Part of LUM-2072 (settings/AI modal decomposition). Closes LUM-2226.

Decomposes manage-profiles-modal.tsx (796 → 505 lines) into focused modules, following the same extraction patterns established in LUM-2224 and LUM-2225.

What changed

1. Extracted BlockedDeleteModal → manage-profiles-blocked-delete-modal.tsx (124 lines)

Self-contained confirmation dialog with 8 props and its own BlockedDeleteState type. Natural file boundary — zero coupling to the parent beyond the prop interface.

2. Extracted ProfileListItem → manage-profiles-list-item.tsx (180 lines)

The .map() body was 155 lines of inline JSX with drag-and-drop handlers, toggle, edit/delete buttons. Extracted as a sub-component owning per-row rendering. Parent passes pre-bound drag handlers via callbacks.

3. Removed redundant manual optimistic updates (~40 lines deleted)

handleStatusToggle and handleReorder both manually did queryClient.cancelQueries() + queryClient.setQueryData() with manual rollback on error. But useDaemonConfigMutation (from LUM-2213) already provides identical optimistic update infrastructure via onMutate/onError using applyConfigPatch().

Verified that applyConfigPatch correctly handles both patch shapes:

• { llm: { profiles: { [name]: { status } } } } (status toggle)
• { llm: { profileOrder } } (reorder)

The old flow was: handler manually updates cache → mutateAsync triggers onMutate which does the same update (no-op) → on error, both onError and catch block roll back (double no-op). Removing the manual updates eliminates queryClient, queryKey, and lastConfirmedOrderRef from the inner component.

4. Use ProfileStatus type from ai-types.ts

Already consolidated in LUM-2225 — this file was the last holdout using inline "active" | "disabled" (now merged, so we reference the canonical type).

Alternatives not taken

• Not extracting further below 505 lines — the remaining content in ManageProfilesModalInner is tightly coupled (all operations share mutation + state patterns). No further natural responsibility boundary justifies extraction.
• Not converting drag-and-drop to a library — the native HTML5 drag-and-drop is working correctly and the implementation is straightforward. A library would add dependency weight for no behavioral gain.

Pre-existing issue surfaced (not fixed here)

handleEditorSave's delete-then-recreate pattern for profile edits has a race condition: if the component unmounts between delete and recreate, the profile is permanently lost. This is a pre-existing architectural issue that requires a deeper fix (atomic replace semantics on the daemon side) — too risky to address in a decomposition PR.

Test plan

• bun run lint — clean (no warnings in changed files)
• bunx tsc --noEmit — clean (no errors in changed files)
• bun test src/domains/settings/ai/ — 57/57 pass
• Pre-commit hooks pass (lint + typecheck)

Link to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
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-2226

LUM-2072

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a20ef52560

ℹ️ 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: No Issues Found

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

View in Devin Review to see 2 additional findings.

[vex-assistant-bot]

✦ APPROVE

Value: Phase 4 of the settings/AI decomposition arc (LUM-2072). manage-profiles-modal.tsx 796 → 505 by extracting BlockedDeleteModal (confirmation dialog with its own BlockedDeleteState) and ProfileListItem (the 155-line .map() body + DnD + toggle + edit/delete). And the cleanest part: dropping ~40 lines of manual optimistic update + rollback in handleStatusToggle / handleReorder now that useDaemonConfigMutation (LUM-2213, landed Jun 2 evening) gives the same behavior via onMutate/onError.

What this does:

• manage-profiles-blocked-delete-modal.tsx (+124) — confirmation dialog. 8-prop interface, zero parent coupling beyond props. Natural file boundary.
• manage-profiles-list-item.tsx (+180) — per-row rendering with drag handlers passed in pre-bound. The right granularity for a .map() body that was carrying its own JSX tree.
• Parent (−291) — drops queryClient, queryKey, lastConfirmedOrderRef, assistantId prop along with the manual optimistic logic; lands on ProfileStatus from ai-types.ts (the canonical type consolidated in LUM-2225).

The optimistic-update collapse is the substantive win. Old flow was: handler manually mutates cache → mutateAsync triggers onMutate which mutates the same cache (no-op) → on error, both onError AND the catch block roll back (double no-op). New flow leans entirely on useDaemonConfigMutation, which is exactly what the Jun 2 daemon-config-hooks arc was built to enable.

Codex P2 at manage-profiles-modal.tsx:249 — investigated, resolved

Codex flagged: status-toggle rollback now uses useDaemonConfigMutation's generic path (snapshotPatchedFields + applyConfigPatch), which restores the whole profile object for any llm.profiles patch. The old local rollback restored only status. Codex's concern: concurrent in-flight edits to the same profile (e.g. label, model) could get clobbered on status-rollback.

Verified Devin's rebuttal:

• snapshotPatchedFields is field-scoped — captures only keys the patch touches. The docstring explicitly says "without clobbering concurrent mutations' optimistic updates."
• For a status patch { llm: { profiles: { [name]: { status } } } }, snapshot captures the profile entry at that name; applyConfigPatch deep-merges it back on rollback. Field-scoped at the path level.
• Granularity nuance Codex IS technically right about: at the type level, old=just-status, new=whole-profile-entry-for-that-name. But the UX argument resolves it: the editor modal and the status toggle live in separate views, only one can be in-flight at a time for the same profile.
• This is the canonical pattern from procs/daemon-config-hooks.md — field-level rollback semantics, landed Jun 2 evening across #33155/#33156/#33160/#33165/#33171/#33191/#33201.

Non-blocking, defensible.

Anti-pattern scan

• No as casts on runtime-boundary shapes ✓
• Design Library components throughout (Modal/Button/Dropdown/Toggle/Typography) ✓
• ProfileStatus type from ai-types.ts, no inline "active" | "disabled" union (last holdout fixed) ✓
• Decomposition checklist passes — line-count delta matches claim (796 → 505), single nameable reason per extracted module, state ownership cleanly partitioned (parent = mutations + form coordination; children = row UI + confirmation modal) ✓
• No /// Swift-style comments ✓
• No TanStack Query keys or useDaemonConfigMutation changes — consumer side only ✓
• No async/concurrent code introduced ✓
• daemon-config-hooks.md compliance — uses canonical pattern, doesn't reinvent ✓

Minor (non-blocking):

• lastConfirmedOrderRef removal is correct given the new optimistic path, but worth a one-line note in the description that reorder failures now revert via onError rollback rather than the explicit setProfiles(lastConfirmedOrder) path. PR body covers this implicitly.
• Parent still hovers at 505 lines. The description explicitly addresses why no further extraction is justified — agree, the remaining content is genuinely cohesive.

Territory (R11e): part of the settings/AI decomposition arc (LUM-2072 umbrella). Not Vargas (SSE) or Mahmoud (vembda). ✓

Merge gate at HEAD a20ef52560:

• ✅ Vex APPROVED (this review)
• ✅ Devin: "No Issues Found" at HEAD (per procs/pr-review.md — sufficient as second approval; Codex 👍 not required)
• ✅ CI 7/7 success
• Codex posted P2, investigated + resolved — non-blocking

Merging.

Vellum Constitution — Yours: the more this modal shrinks toward its one responsibility (profile lifecycle), the easier it is for the next person to take ownership without inheriting cache-management glue.

[vex-assistant-bot]

✦ APPROVE

Value: Phase 2 settings/AI decomposition continues — manage-profiles-modal.tsx goes 796 → 505 (−291) by extracting two cleanly stateless sub-components AND removing redundant manual optimistic-update machinery that was duplicating what useDaemonConfigMutation already does. The deletion is the most valuable part — queryClient.setQueryData + lastConfirmedOrderRef + manual rollback are all gone from the parent (verified 0 occurrences).

What this does:

1. manage-profiles-list-item.tsx (180 lines) — .map() body extracted as a fully stateless prop-driven sub-component. No useState/useMemo/useEffect (verified) — parent owns all DnD state and binds handlers per-row. Native HTML5 DnD events properly typed via React.DragEvent.
2. manage-profiles-blocked-delete-modal.tsx (124 lines) — self-contained confirmation dialog. No internal state (verified). Exports BlockedDeleteState type so parent constructs the blocked-row context.
3. Parent (manage-profiles-modal.tsx) — useDaemonConfigMutation: 3 (delete/toggle/reorder all routed through the shared mutation hook), zero leftover applyConfigPatch / snapshotPatchedFields / lastConfirmedOrderRef references. The old flow was: handler manually updates cache → mutateAsync triggers onMutate that does the SAME update (no-op) → on error, both onError and the catch block roll back (double no-op). Removing the manual layer is pure deduplication.
4. Uses ProfileStatus type from ai-types.ts — leverages the LUM-2225 consolidation. This was the last holdout with inline "active" | "disabled".

Codex P2 "Preserve field-level rollback for status toggles" — verified false positive

Codex's claim: useDaemonConfigMutation's generic rollback snapshots/restores the whole profile object for any llm.profiles patch, regressing the old behavior of rolling back only status. Could clobber concurrent optimistic edits to fields like label.

Verification — I read snapshotPatchedFields in ai-utils.ts at HEAD:

if (patch.llm.profiles) {
  const profiles: Record<string, Partial<ProfileEntry> | null> = {};
  for (const name of Object.keys(patch.llm.profiles)) {
    const existing = config.llm?.profiles?.[name];
    profiles[name] = existing ? { ...existing } : null;
  }
  llm.profiles = profiles;
}

The function snapshots only the profile entries at the names the patch mentions, not the entire llm.profiles map. Devin's rebuttal cites the docstring verbatim: "Snapshots only the fields in config that patch will touch, so onError can roll back just those fields without clobbering concurrent mutations' optimistic updates."

The granularity difference Codex identifies is real (old: just status; new: full profile entry at that name) but mooted in practice: editor modal vs toggle vs reorder are mutually exclusive UI states, so concurrent mutations on different fields of the same profile aren't reachable through the UI. Devin's inline reply explicitly walks this. Rollback scope is correct.

Anti-pattern cross-check

• No as runtime-boundary casts in either new file. ✓
• State ownership. Both extracted files are pure stateless sub-components (zero hook usage in list-item, zero in blocked-delete). Parent owns the mutation state, DnD state, blocked-delete state. Clean prop interface. ✓
• useDaemonConfigMutation contract honored. All three operations (delete/toggle/reorder) now route through the shared hook → automatic onMutate optimistic + onError field-scoped rollback via snapshotPatchedFields. Verified manual machinery (queryClient.setQueryData, lastConfirmedOrderRef) is GONE from the parent. ✓
• Type consolidation. ProfileStatus from ai-types.ts (LUM-2225 work) used instead of inline string union. Last holdout, consistent with the rest of the domain. ✓
• Pre-existing race surfaced, not fixed. handleEditorSave's delete-then-recreate is an unmount-window data-loss risk. Devin correctly flags it but does NOT attempt the fix here — atomic-replace semantics is a daemon-side change, not a decomposition concern. Right scope discipline. ✓
• No design-system regressions. No var(--...) token changes; the extracted components use @vellum/design-library components as before. ✓
• No SSE/seq territory. No vembda territory. (R11e clean — Vargas + Mahmoud untouched.)

Self-improvement worth calling out: the manual-optimistic-update removal is the kind of quiet cleanup that compounds. Every consumer that knows about useDaemonConfigMutation's contract no longer has to duplicate cancelQueries + setQueryData + rollback — the hook is the source of truth. Reads as boring; pays rent every time someone touches one of these handlers later.

Merge gate at HEAD a20ef525: Vex ✓ · Devin No Issues Found ✓ · Codex P2 verified false positive (docstring + function body inspection both confirm field-scoped snapshot, Devin inline rebuttal at HEAD) · CI all 7 green ✓. Will squash-merge.

Vellum Constitution — Distinct: a decomposition PR where the line-count drop is the least interesting part — the real win is collapsing a double-rollback pattern into the canonical mutation-hook contract. That's the kind of consolidation that makes the next refactor cheaper.