Skip to content

feat(coordinator): UI components, control banners, and coordinator entry points (PR 4/4)#125

Merged
johannesjo merged 4 commits into
johannesjo:mainfrom
brooksc:coordinator-4-ui
May 21, 2026
Merged

feat(coordinator): UI components, control banners, and coordinator entry points (PR 4/4)#125
johannesjo merged 4 commits into
johannesjo:mainfrom
brooksc:coordinator-4-ui

Conversation

@brooksc
Copy link
Copy Markdown
Contributor

@brooksc brooksc commented May 17, 2026

Overview

This is PR 4 of 4 in the coordinator series splitting #100 as requested in the round-4 review. It is stacked on PR 3 (#124) (coordinator-3-store-ipc) and must be merged after that one. The diff shown here includes PRs 1–3's content; the meaningful delta for this PR is the UI components and entry points described below.

PR sequence:

PR Branch Status Contents
1 (#118) coordinator-1-security Open Atomic writes, input validators, static analysis configs
2 (#120) coordinator-2-mcp-backend Open MCP coordinator engine + REST API hardening
3 (#124) coordinator-3-store-ipc Open Frontend store wiring + IPC handlers
4 (this PR) coordinator-4-ui Open UI components + coordinator entry points

This PR makes coordinator mode user-visible for the first time. After it merges, users can enable coordinator mode in Settings and create coordinator tasks from NewTaskDialog.


What's in this PR (delta over PR 3)

New component: SubTaskStrip

Nested status strip rendered inside a coordinator task's panel. Shows each sub-task's name, status dot, branch, and action buttons (send prompt, merge, close). Collapses to a compact summary when the coordinator panel is not focused.

Sidebar — coordinator grouping

Coordinator tasks render with their children nested beneath them in the sidebar. Children are hidden from the flat task list (already excluded by PR 3's isCoordinatedChild guard). A control banner appears below the coordinator's terminal when controlledBy === 'coordinator' to show the user they can take control.

TaskPanel — coordinator detail view

Full coordinator task panel with sub-task list, per-sub-task diff summary, merge/close actions, and MCP startup status display (pending / ready / error with retry button).

PromptInput — staged notification and autofire

  • Staged coordinator notification is displayed as a pending message above the input, with Send / Edit / Dismiss controls
  • autofire-tick.ts: fires the staged notification after the configured delay; 2-minute grace period for coordinator mode when no prompt marker is visible, so notifications fire even during long tool-call loops
  • prompt-control.ts: shared coordinator handoff helper; PromptInput.handleSend owns the coordinator send gate

SettingsDialog — coordinator settings

New "Coordinator Mode" section with:

  • Enable/disable toggle (sets coordinatorModeEnabled)
  • Notification delay slider (5 s – 5 min, default 60 s)

NewTaskDialog — coordinator entry point

New "Coordinator mode" checkbox in the advanced section. When checked, exposes a "Propagate skip permissions" option. Creates the task with coordinatorMode: true.

Other component updates

  • TaskTitleBar: shows coordinator badge; sub-tasks show "controlled by coordinator" indicator
  • TaskAITerminal: renders SubTaskStrip for coordinator tasks
  • TerminalView: disables stdin when controlledBy === 'coordinator'; re-enables on hand-off
  • SidebarFooter: shows MCP server status indicator
  • StatusDot: renders 'review' state (orange dot) for tasks with needsReview
  • CloseTaskDialog: shows warning when closing a coordinator with active children
  • PlanViewerDialog: minor coordinator-aware rendering fix

Bug fixes (live e2e testing)

  • show_notification IPC: ipcMain.onipcMain.handle to satisfy ipcRenderer.invoke
  • merge_task git lock: retry once after 2 s when index.lock is held by a crashed process
  • getBranchCommits: guard against missing worktree (existsSync before git spawn)
  • getWorktreeStatus: wrap exec in try/catch to handle ENOENT race after worktree deletion
  • wait_for_signal_done timeout: resolve with { timedOut: true } instead of reject, so coordinator Claude receives a structured result rather than isError: true

CI / tooling

  • Pin GitHub Actions to exact SHAs (supply chain hardening on pre-existing workflows)
  • Husky pre-push: runs full test suite before push
  • Husky commit-msg: enforces conventional commit format
  • pre-commit: lockfile integrity check (fails if package.json changes without package-lock.json)

Tests

File New / updated cases
src/components/PromptInput.test.ts 284 new — autofire tick, staged notification rendering, coordinator control state, send gate, 2-min grace period
src/components/prompt-control.test.ts 28 new — send gate behaviour for coordinator vs human control
src/lib/terminalDisableStdin.test.ts 16 new — stdin disable/re-enable logic
electron/ipc/register-mcp.test.ts TOCTOU fix — all findFreePort() calls replaced with port: 0
electron/mcp/coordinator.test.ts 5 updated — timeout assertions changed from rejects.toThrow to timedOut === true

Assumptions and important notes

  1. PR 3 prerequisite — depends on the store functions, IPC listeners, and type definitions introduced in PR 3. Without PR 3, nothing here compiles.
  2. Feature now livecoordinatorModeEnabled can be set to true via the Settings toggle. First coordinator task can be created from NewTaskDialog.
  3. autofire grace period — 2-minute grace is intentional. Coordinators running long tool-call loops (no visible) need a time-based fallback to fire staged notifications; without it, the notification would never send until the agent returned to prompt.
  4. CI/hooks commits — The GitHub Actions SHA pinning modifies pre-existing workflow files only (no new workflows). Husky hooks and lockfile integrity check are also included here; they can be cherry-picked to an earlier PR if preferred.

🤖 Generated with Claude Code

@johannesjo
Copy link
Copy Markdown
Owner

Thanks for the coordinator UI work. I did another pass with multiple reviewers focused on autofire/control flow, backend lifecycle sync, and UI entry points. A few blockers stood out:

  1. Autofire can drop coordinator notifications before the configured delay/grace elapses. processAutoFireTick skips autoFireAt for controlledBy === coordinator, so promptless ticks become misses immediately and PromptInput drop-acks after 10 misses, well before the default 60s delay and 2-minute promptless grace. Refs: src/components/autofire-tick.ts:32, src/components/PromptInput.tsx:469, src/components/PromptInput.tsx:481.

  2. Manual/global send still bypasses coordinator control. The visible input handlers block coordinator-controlled input, but the global send action still calls handleSend(), and handleSend() does not guard props.controlledBy === coordinator. Cmd/Ctrl+Enter can therefore send hidden staged text immediately in auto mode. Refs: src/components/PromptInput.tsx:583, src/components/PromptInput.tsx:622.

  3. Release Control immediate autofire bypasses the question guard. On human -> coordinator, the immediate path only checks for ❯/› in the terminal tail, then sends. It does not check questionActive(), unlike the normal autofire helper, so active TUI dialogs can receive the staged notification. Ref: src/components/PromptInput.tsx:525.

  4. Backend prompt sends do not clear frontend review/done flags. Coordinator.sendPrompt() sets backend status back to running, but no MCP_TaskStateSync clears needsReview, signalDoneReceived, etc. A child can keep showing stale done/review UI after receiving new work. Refs: electron/mcp/coordinator.ts:769, src/store/tasks.ts:1193.

  5. Coordinator close/detach has conflicting child state updates. Deregistration always emits needsReview: true, while the renderer close path clears coordinator fields and review flags. Depending on IPC order, this can either lose the orphan/review marker or re-mark detached standalone children unexpectedly. It also leaves mcpStartupStatus/mcpStartupError, which can keep a detached child gated by MCP startup. Refs: electron/mcp/coordinator.ts:1420, src/store/tasks.ts:443, src/components/TerminalView.tsx:675.

  6. The "one coordinator per project" UI guard can be bypassed by changing projects. coordinatorMode() is not cleared when the selected project changes, and submit does not re-check hasActiveCoordinator(). A user can enable coordinator mode on one project, switch to a project that already has a coordinator, and submit another coordinator. Refs: src/components/NewTaskDialog.tsx:461, src/components/NewTaskDialog.tsx:544, src/components/NewTaskDialog.tsx:1086.

Smaller follow-ups: SubTaskStrip can select collapsed children without uncollapsing them, regular TaskRow appears to have lost the offscreen attention state/badge path, and visible nested subtasks are excluded from computeSidebarTaskOrder(), so keyboard navigation cannot reach them.

I would add regression coverage around the PromptInput call site, not only the pure processAutoFireTick helper, plus coordinator close/detach state synchronization.

@brooksc brooksc force-pushed the coordinator-4-ui branch from 05344e2 to 5af7cf1 Compare May 20, 2026 06:09
@brooksc
Copy link
Copy Markdown
Contributor Author

brooksc commented May 20, 2026

Addressed the review feedback in the squashed PR commit 5af7cf1:

  1. Autofire delay/grace handling

    • Autofire now respects autoFireAt for coordinator-controlled notifications.
    • Promptless coordinator ticks wait through the promptless grace period instead of incrementing miss/drop counters early.
    • Added regression coverage for the coordinator autofire timing path.
  2. Manual/global send coordinator-control bypass

    • Manual/global send paths now guard against coordinator-controlled tasks.
    • Hidden staged text can no longer be sent via Cmd/Ctrl+Enter or registered actions while the coordinator owns control.
  3. Release Control immediate autofire question guard

    • Release Control immediate autofire now uses the same processAutoFireTick decision path as normal autofire.
    • This includes question/dialog protection instead of checking only for a prompt marker.
  4. Backend prompt sends clearing frontend review/done flags

    • Backend sendPrompt() now emits MCP_TaskStateSync to clear stale frontend done/review flags when new work is sent.
    • Added regression coverage for backend prompt state sync.
  5. Coordinator close/detach child state synchronization

    • Coordinator deregistration now sends a single detach-state sync for children.
    • It clears coordinator wiring and MCP startup state while preserving review state only when prompt delivery actually happened.
    • Renderer close/detach cleanup no longer clears backend review/done markers and now clears stale MCP startup fields.
    • Added regression coverage for coordinator detach cleanup state.
  6. One-coordinator-per-project UI guard

    • NewTaskDialog clears coordinator mode when switching to a project that already has a coordinator.
    • Submit now re-checks the one-active-coordinator invariant before creating the task.
  7. SubTaskStrip collapsed child selection

    • Selecting a collapsed child from SubTaskStrip now uncollapses the child before selecting it.
  8. Regular TaskRow offscreen attention state

    • Regular sidebar task rows regain offscreen attention highlighting/badges.
  9. Sidebar keyboard navigation for nested subtasks

    • Sidebar keyboard navigation order now includes visible nested coordinator children.
    • Added regression coverage for sidebar coordinator child grouping/navigation order.
  10. Codex coordinator MCP launch args

    • Codex coordinator tasks no longer receive Claude-style --mcp-config <path> arguments, which the installed Codex CLI rejects.
    • MCP launch argument construction is centralized for backend-created subtasks and renderer/root coordinator spawns.
    • Codex agents receive inline --config mcp_servers.parallel-code={...} MCP wiring.
    • Claude-compatible agents continue to receive --mcp-config <path>.
    • StartMCPServer now returns the exact mcpLaunchArgs needed by the selected agent, and restored/retried coordinator tasks refresh those args before spawning.
    • Added backend and renderer argument-builder tests so Codex cannot regress back to --mcp-config.

Verification:

  • codex -c 'mcp_servers.parallel-code={...}' debug prompt-input 'x'
  • npm test -- electron/mcp/agent-args.test.ts src/lib/agent-args.test.ts src/store/tasks.test.ts electron/mcp/coordinator.test.ts
  • npm run check
  • npm test
  • push hook reran compile, typecheck, lint, format:check, and full npm test successfully (913 passed | 12 skipped)
  • git diff --check

@johannesjo
Copy link
Copy Markdown
Owner

johannesjo commented May 20, 2026

Thank you very much! Looks good to merge after conflicts are resolved!

Optional feedback:

Verified all six blockers from prior review landed in 5af7cf1 with regression coverage. Spot-checked each:

  1. Autofire graceautofire-tick.ts returns too-soon (not no-prompt) for coordinator promptless ticks, so the miss counter no longer increments before the 2-min grace. Tests cover the path.
  2. Manual send bypasshandleSend now guards mode === 'manual' && controlledBy === 'coordinator'. Cmd/Ctrl+Enter blocked.
  3. Release-control immediate fire — routes through processAutoFireTick, picking up the questionActive guard.
  4. sendPrompt clears stale flags — emits MCP_TaskStateSync clearing signalDoneReceived/At/Consumed and needsReview.
  5. Deregister detach — single sync clears coordinator wiring + MCP startup fields; preserves needsReview only when assignedPromptDelivered.
  6. One-coordinator-per-projectcreateEffect clears coordinatorMode on project switch into a project that already has one; submit re-checks hasActiveCoordinator().

Two follow-ups I would not block on:

  • electron/mcp/agent-args.ts:25tomlStringMap emits map keys raw (${key} = ...). Today only fed app-controlled mcpConfig.env, so not exploitable, but a stray non-bare-key char (anything outside [A-Za-z0-9_-]) would break TOML parse. Suggest quoting keys via tomlString(key) for defence in depth.
  • src/components/NewTaskDialog.tsxmaxConcurrentTasksonInput only enforces >= 1; HTML max="20" is advisory. User can submit an arbitrary value. Worth clamping client-side (or confirming the backend clamps).

Nit: prompt-control.ts holds only the handoff helper; the actual coordinator send gate lives inline in PromptInput.handleSend. PR description calls the file the "send gate" — slightly misleading name, no behavioural impact.

Strengths worth noting: pure decision helpers (processAutoFireTick, shouldHandoffCoordinatorQuestion, buildTaskAgentArgs) are isolated and unit-tested; new IPC handlers all run through assertString/validateUUID/validatePath/sharedValidateBranchName; REST API uses a three-token scheme with timingSafeEqual + coordinator-scoped filter; notifyRenderer guards isDestroyed; getWorktreeStatus/getBranchCommits tolerate a missing worktree; fs.rmSync retry-with-backoff covers VirtioFS lag.

LGTM once PR 3 (#124) lands. The two follow-ups above can ship as a separate fix.

@brooksc
Copy link
Copy Markdown
Contributor Author

brooksc commented May 21, 2026

Pushed 2f14d3d to address the optional feedback from the latest review comment. Keeping the same order:

  1. Autofire grace

    No new code change needed in this pass. This remains covered by the existing processAutoFireTick path: coordinator promptless ticks return too-soon before the grace elapses, so the miss counter does not advance early.

  2. Manual send bypass

    No new code change needed in this pass. PromptInput.handleSend still guards manual sends when controlledBy === 'coordinator', including Cmd/Ctrl+Enter/global send.

  3. Release-control immediate fire

    No new code change needed in this pass. Release-control immediate fire still routes through processAutoFireTick, so it keeps the questionActive guard.

  4. sendPrompt clears stale flags

    No new code change needed in this pass. Backend prompt sends still emit MCP_TaskStateSync clearing stale signalDoneReceived/At/Consumed and needsReview.

  5. Deregister detach

    No new code change needed in this pass. Coordinator detach sync still clears coordinator wiring and MCP startup fields while preserving needsReview only when assignedPromptDelivered.

  6. One-coordinator-per-project

    No new code change needed in this pass. Project switching still clears coordinatorMode for projects that already have an active coordinator, and submit re-checks hasActiveCoordinator().

  7. electron/mcp/agent-args.ts:25 — quote TOML env keys

    Fixed. tomlStringMap() now serializes env keys with tomlString(key) instead of emitting raw keys. This keeps Codex inline MCP config valid if an env key contains non-bare TOML characters such as dots.

    Added regression coverage in electron/mcp/agent-args.test.ts for an env key like TOKEN.WITH.DOTS.

  8. src/components/NewTaskDialog.tsx — clamp maxConcurrentTasks

    Fixed. Added shared coordinator concurrency limits and a clamp helper in src/lib/coordinator-limits.ts.

    NewTaskDialog now uses the shared min/max/default values, clamps typed input, and clamps again before passing maxConcurrentTasks into createTask().

    createTask() also clamps before substituting {{MAX_CONCURRENT}} into the coordinator preamble, so the boundary is protected even if another caller passes an out-of-range value.

    Added regression coverage in src/lib/coordinator-limits.test.ts.

  9. PR description nit — prompt-control.ts wording

    Fixed in the PR description. The bullet now says:

    prompt-control.ts: shared coordinator handoff helper; PromptInput.handleSend owns the coordinator send gate.

  10. Strengths noted

No changes needed. The existing structure called out in the review remains intact: pure decision helpers stay isolated and tested, IPC handlers continue using validation helpers, REST auth remains coordinator-scoped, renderer notification sends guard destroyed windows, worktree status/commit helpers tolerate missing worktrees, and fs.rmSync keeps retry-with-backoff for VirtioFS lag.

Verification:

  • npm test -- electron/mcp/agent-args.test.ts src/lib/coordinator-limits.test.ts
  • npm test -- electron/mcp/agent-args.test.ts src/lib/coordinator-limits.test.ts src/store/tasks.test.ts
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • git diff --check
  • commit hook ran npm run check
  • pre-push hook ran npm run check and full npm test (916 passed | 12 skipped)

@brooksc brooksc force-pushed the coordinator-4-ui branch from 2f14d3d to 3f1e2cf Compare May 21, 2026 06:49
@johannesjo
Copy link
Copy Markdown
Owner

Thank you very much! <3

@johannesjo johannesjo merged commit 21f7e57 into johannesjo:main May 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants