From bdb1fcab5329b75e212b93717e15d4f0a06f27b9 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Thu, 10 Sep 2026 16:10:16 -0700 Subject: [PATCH 1/9] docs(dispatch): plan New Agent In command Filling empty Grid Dispatch lanes currently requires selecting an unrelated agent in the target project first, only to move the spawn target. The plan records the approved design: a Dispatch-only two-step picker (agent, then project) that reuses createDetachedDispatchAgent's explicit project override. Refs #852 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SVRSHn7c7qnBoUQo1e2X4k --- .../plans/2026-09-10-new-agent-in-project.md | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-10-new-agent-in-project.md diff --git a/docs/superpowers/plans/2026-09-10-new-agent-in-project.md b/docs/superpowers/plans/2026-09-10-new-agent-in-project.md new file mode 100644 index 000000000..ec59ce2fa --- /dev/null +++ b/docs/superpowers/plans/2026-09-10-new-agent-in-project.md @@ -0,0 +1,86 @@ +# New Agent In… — spawn a Dispatch agent into a chosen project + +**Issue:** #852 +**Branch:** `feat/new-agent-in-project` (worktree `.worktrees/new-agent-in`, from `origin/main` @ `48ea6912`) + +## Problem + +Filling empty Grid Dispatch lanes needs a navigational detour. When the focused +lane is empty and its row is not bound to a project, `resolveDispatchSpawnTarget` +(`workspace/dispatch/dispatchSelectors.ts`) files a new agent under the classic +Dispatch focus (the last agent selected) or `activeTabId`. To put an agent for +project C into an empty lane, the user first selects *some existing agent in +project C* purely to move "the current project", returns to the empty lane, then +runs **New Agent…**. Five fresh lanes means five detours. + +## What already exists (do not rebuild) + +- `createDetachedDispatchAgent(selection, { tabId, anchorSessionId })` + (`workspace/hook/actions/pane.ts`) spawns into an explicit project, borrows + `cwd` from the anchor session, and still places the agent into the focused + lane (`laneIndex` stays focus-derived — the override only names the project). + The Dispatch project header's "+" and the external `agents.create` control + capability both use it. +- `AGENT_PROVIDER_CHOICES` (`workspace/providerChoices.ts`) is the shared + Claude / Codex / OpenCode / OpenCode Terminal choice list. +- `ProviderSwitchPickerModal` is the house pattern for a keyboard-driven + provider list inside the shared `DialogContent` primitive. + +## Decisions (approved in chat) + +1. **Dispatch only** (`surface: 'dispatch'`). In the grid a project is a tab one + keystroke away, and a detached agent spawned from the grid is invisible there. + Also hidden under Tiled Tabs (mirrors `new-agent`): Tiled Tabs covers Dispatch, + so a lane spawn would land somewhere the user cannot see. +2. **Row bindings restrict the project list.** If the focused lane's row is bound + to projects, only those are offered — the binding contract (see + `resolveDispatchSpawnTarget` and `DispatchRowProjectModal`) says a row's index + only lists its bound projects. Unbound rows / classic Dispatch offer every + project. The bound-set lookup is extracted into one selector shared with + `resolveDispatchSpawnTarget` so the list and the spawn resolver cannot disagree. +3. **Projects with no anchor are listed, disabled, with a reason** — not omitted. +4. **No default keybinding.** +5. **Initial highlight** on the project step is the project plain New Agent… + would have used (`resolveDispatchSpawnTarget(state).tabId`) when it is + eligible, so Enter-Enter is never worse than today. +6. **A dedicated dialog**, not a fifth mode on `NewAgentPlacementOverlay` (already + four intents; its "kind picked ⇒ placement step" invariant would need another + branch) and not a palette sub-mode (`CommandPalette.tsx` is 2.5k lines and each + mode threads ~6 ternary chains; the two-step choice would also need store state). + +## Units + +| Unit | File | Purpose | +|---|---|---| +| Bound-projects selector | `workspace/dispatch/dispatchSelectors.ts` | `focusedLaneBoundProjectTabIds(state)` — the focused Tiled Dispatch lane's row binding, `[]` when unbound/classic. `resolveDispatchSpawnTarget` switches to it. | +| Project model | `features/workspace/lib/newAgentInProjects.ts` | Pure: eligible projects (tab order, A/B/C labels from the full tab list), anchor per project (first session of `resolveTabSessions` with a cwd — same order the header "+" uses), disabled reason, initial tab id. | +| Dialog | `features/workspace/ui/NewAgentInDialog.tsx` | Two steps: agent → project. ↑/↓ (+ Ctrl-N/P), Enter, click, Backspace back, Esc/Cancel. Commit latch. Calls `createDetachedDispatchAgent`. Derives nothing while closed. | +| Surface | `features/workspace/surfaces/NewAgentInSurface.tsx` + `app/surfaces/registry.tsx` | Mount; appended at the END of `modalSurfaces` per the registry's paint-order contract. | +| Store intent | `app-state/uiShell/{types,slice}.ts`, `app-state/types.ts` | `newAgentInOpen` + `openNewAgentIn` / `closeNewAgentIn`. | +| Command | `features/workspace/commands/paneCommands.ts` | `new-agent-in`, right after `new-agent`; `ui.openNewAgentIn` threaded through `command-palette/types.ts` + `CommandPalette.tsx`. | +| Catalog snapshot | `features/command-palette/catalog.test.ts` | Same commit as the command (the file's own rule). 116 → 117. | +| Control reference | `features/workspace/controlReference.ts` | List `new-agent-in` under the Dispatch feature so operators can point users at it. | + +## Tasks + +1. **Selector + model (TDD).** Write `newAgentInProjects.test.ts` first: + unbound → all tabs in order with labels; bound row → only bound tabs; anchor is + the first session with a cwd, grid leaf before detached; project with no + session is disabled with reason; initial tab = spawn-target tab when eligible, + else first enabled. Then implement the selector extraction and the model. +2. **Dialog (TDD).** Write `NewAgentInDialog.renderer.test.tsx` first (pattern: + `ProviderSwitchPickerModal.renderer.test.tsx`): keyboard two-step commit calls + `createDetachedDispatchAgent` with `{kind, providerRuntime}` and + `{tabId, anchorSessionId}`; click path; Backspace returns to agent step; + disabled project cannot be committed; double Enter spawns once; Cancel never + commits. Then implement. +3. **Wiring.** Store intent, surface + registry entry, command, palette `ui` + threading, catalog snapshot, control reference. +4. **Verify once at the end** (Node 24): `npm run typecheck`, `npm test`, + `npm run test:contract`, `npm run check:keybindings`. + +## Out of scope + +- A default chord. +- Changing New Agent… or the header "+" behavior. +- Grid-mode variant. From c80f254ca985c02041d8b93800a3d2ed5c074cad Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Thu, 10 Sep 2026 16:46:32 -0700 Subject: [PATCH 2/9] refactor(dispatch): share the focused lane's row binding as one selector resolveDispatchSpawnTarget read the focused Grid Dispatch row's project binding inline. New Agent In needs the same answer to decide which projects it may offer for that lane, and two separate derivations could drift (the legacy projectTabId fold and row-length repair live in normalizeGridShape), letting the picker offer a project the resolver would refuse. Behavior is unchanged. The resolver's bound-row branch had no coverage, so its two rules (bound project over stale classic focus; active tab when it is one of several bound projects) are pinned before the move. Refs #852 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SVRSHn7c7qnBoUQo1e2X4k --- .../dispatch/dispatchSelectors.test.ts | 66 +++++++++++++++++++ .../workspace/dispatch/dispatchSelectors.ts | 31 +++++++-- 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/src/renderer/src/workspace/dispatch/dispatchSelectors.test.ts b/src/renderer/src/workspace/dispatch/dispatchSelectors.test.ts index 0c2543527..05cd8bc7c 100644 --- a/src/renderer/src/workspace/dispatch/dispatchSelectors.test.ts +++ b/src/renderer/src/workspace/dispatch/dispatchSelectors.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import { dispatchSessionIdsForTab, + focusedLaneBoundProjectTabIds, resolveDispatchSpawnTarget, } from '@renderer/workspace/dispatch/dispatchSelectors' import { resolveDispatchAttachTarget } from '@renderer/workspace/dispatch/dispatchTarget' @@ -113,6 +114,71 @@ describe('resolveDispatchSpawnTarget with a detached focused lane', () => { }) }) +// Grid Dispatch row bindings (#681). Two consumers must agree on "which +// projects may this lane hold": the spawn resolver (where plain New Agent… +// lands) and New Agent In…'s project list (#852). Both read the SAME selector, +// and these cases pin the resolver's bound-row branch that had no coverage +// before it was routed through that selector. +describe('focusedLaneBoundProjectTabIds', () => { + it('returns the binding of the row that owns the focused lane', () => { + const state = makeState({ + scope: 'global', + focusedSessionId: 'a1', + tiled: { + focusedLane: 1, + lanes: [{ selectedSessionId: 'a1' }, {}], + rows: [{ length: 1 }, { length: 1, projectTabIds: ['tabB'] }], + }, + }) + expect(focusedLaneBoundProjectTabIds(state)).toEqual(['tabB']) + }) + + it('returns nothing for an unbound row, classic Dispatch, or no Dispatch at all', () => { + const unbound = makeState({ + scope: 'global', + focusedSessionId: 'a1', + tiled: { + focusedLane: 0, + lanes: [{ selectedSessionId: 'a1' }, {}], + rows: [{ length: 1 }, { length: 1, projectTabIds: ['tabB'] }], + }, + }) + expect(focusedLaneBoundProjectTabIds(unbound)).toEqual([]) + expect(focusedLaneBoundProjectTabIds(makeState({ scope: 'global', focusedSessionId: 'a1' }))).toEqual([]) + expect(focusedLaneBoundProjectTabIds(makeState(null))).toEqual([]) + }) +}) + +describe('resolveDispatchSpawnTarget with a bound row', () => { + it('files an empty bound lane under the bound project, not the stale classic focus', () => { + const state = makeState({ + scope: 'global', + focusedSessionId: 'a1', + tiled: { + focusedLane: 1, + lanes: [{ selectedSessionId: 'a1' }, {}], + rows: [{ length: 1 }, { length: 1, projectTabIds: ['tabB'] }], + }, + }) + expect(resolveDispatchSpawnTarget(state)).toEqual({ tabId: 'tabB', cwdSessionId: null, laneIndex: 1 }) + }) + + it('prefers the active tab when it is one of several bound projects', () => { + const state = makeState({ + scope: 'global', + focusedSessionId: 'b1', + tiled: { + focusedLane: 1, + lanes: [{ selectedSessionId: 'b1' }, {}], + rows: [{ length: 1 }, { length: 1, projectTabIds: ['tabB', 'tabA'] }], + }, + }) + // activeTabId is tabA (see makeState), and tabA is bound — it wins over the + // first-listed tabB and over the classic focus b1. + expect(resolveDispatchSpawnTarget(state)).toEqual({ tabId: 'tabA', cwdSessionId: null, laneIndex: 1 }) + }) +}) + describe('dispatchSessionIdsForTab', () => { it('includes pinned rows owned by the tab even though project groups strip them', () => { const state = makeState({ scope: 'global', focusedSessionId: 'b1' }) diff --git a/src/renderer/src/workspace/dispatch/dispatchSelectors.ts b/src/renderer/src/workspace/dispatch/dispatchSelectors.ts index 7ee5ceb5b..4d3821562 100644 --- a/src/renderer/src/workspace/dispatch/dispatchSelectors.ts +++ b/src/renderer/src/workspace/dispatch/dispatchSelectors.ts @@ -340,6 +340,30 @@ export type DispatchSpawnTarget = { laneIndex: number | null } +/** + * The projects the focused Grid Dispatch lane's ROW is bound to, or `[]` when + * that row is unbound, the layout is classic Dispatch, or Dispatch is off. + * + * WHY this is its own selector rather than staying inline in + * `resolveDispatchSpawnTarget`: two creation paths must agree on it. The spawn + * resolver uses it to pick where plain New Agent… lands, and New Agent In… + * (#852) uses it to decide which projects it may OFFER for that lane. If the + * two re-derived it separately, a change to how bindings are read (the legacy + * single `projectTabId` fold, zero-length row repair) could make the picker + * offer a project the resolver would refuse — or hide one it would pick. + * + * Reads through `normalizeGridShape` for the same reason every row reader does: + * persisted rows can be stale or hand-edited, and the normalizer is the one + * place that folds the legacy field and repairs lengths. + */ +export function focusedLaneBoundProjectTabIds(state: WorkspaceState): readonly TabId[] { + const tiled = state.dispatchMode?.tiled + if (!tiled) return [] + const grid = normalizeGridShape(tiled) + const rowIndex = rowIndexForLane(grid.rows, tiled.focusedLane) + return (rowIndex >= 0 ? grid.rows[rowIndex]?.projectTabIds : undefined) ?? [] +} + export function resolveDispatchSpawnTarget(state: WorkspaceState): DispatchSpawnTarget { const dm = state.dispatchMode if (!dm) { @@ -366,15 +390,14 @@ export function resolveDispatchSpawnTarget(state: WorkspaceState): DispatchSpawn // stale classic focus would file the new agent under a project the row does // not even list. Bindings constrain what may live in a row, and a spawn is // something coming to live there. - const grid = normalizeGridShape(dm.tiled) - const rowIndex = rowIndexForLane(grid.rows, laneIndex) + // // A row can be bound to SEVERAL projects, so "which project does a new // agent belong to" needs a rule rather than a lookup. The active tab when // it is one of them, otherwise the first: deterministic, and "the project // you were last in" is the least surprising answer. The per-group `+` in // the index is unaffected — it already carries an explicit tabId. - const bound = rowIndex >= 0 ? grid.rows[rowIndex]?.projectTabIds : undefined - if (bound && bound.length > 0) { + const bound = focusedLaneBoundProjectTabIds(state) + if (bound.length > 0) { const tabId = bound.includes(state.activeTabId) ? state.activeTabId : bound[0]! return { tabId, cwdSessionId: null, laneIndex } } From 35bddde20e7f92c030a34d20bda2af4146cb9fdb Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Thu, 10 Sep 2026 16:46:33 -0700 Subject: [PATCH 3/9] feat(dispatch): add New Agent In to spawn an agent into a chosen project Filling an empty Grid Dispatch lane with an agent for another project meant first selecting some unrelated agent in that project, only to move the spawn target. New Agent In asks for the agent and then the project, and spawns through createDetachedDispatchAgent's existing explicit-project override, so the agent still fills the focused lane exactly like New Agent. It is Dispatch-only, honours the focused row's project binding, starts on the project plain New Agent would have used, and lists projects it cannot anchor as disabled with the reason. The dialog is a dedicated DialogContent surface rather than another intent on the placement overlay, and it derives nothing while closed. The catalog snapshot moves to 117 in this commit, per its own rule; its two test titles had drifted to 115 and now track the asserted count again. Refs #852 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SVRSHn7c7qnBoUQo1e2X4k --- src/renderer/src/app-state/types.ts | 3 + src/renderer/src/app-state/uiShell/slice.ts | 5 + src/renderer/src/app-state/uiShell/types.ts | 18 ++ src/renderer/src/app/surfaces/registry.tsx | 5 + .../features/command-palette/catalog.test.ts | 41 +-- .../src/features/command-palette/types.ts | 4 + .../command-palette/ui/CommandPalette.tsx | 3 + .../workspace/commands/paneCommands.ts | 23 ++ .../features/workspace/controlReference.ts | 3 +- .../workspace/lib/newAgentInProjects.test.ts | 147 ++++++++++ .../workspace/lib/newAgentInProjects.ts | 112 +++++++ .../workspace/surfaces/NewAgentInSurface.tsx | 12 + .../ui/NewAgentInDialog.renderer.test.tsx | 170 +++++++++++ .../workspace/ui/NewAgentInDialog.tsx | 273 ++++++++++++++++++ 14 files changed, 801 insertions(+), 18 deletions(-) create mode 100644 src/renderer/src/features/workspace/lib/newAgentInProjects.test.ts create mode 100644 src/renderer/src/features/workspace/lib/newAgentInProjects.ts create mode 100644 src/renderer/src/features/workspace/surfaces/NewAgentInSurface.tsx create mode 100644 src/renderer/src/features/workspace/ui/NewAgentInDialog.renderer.test.tsx create mode 100644 src/renderer/src/features/workspace/ui/NewAgentInDialog.tsx diff --git a/src/renderer/src/app-state/types.ts b/src/renderer/src/app-state/types.ts index 5d2e61423..bfc795574 100644 --- a/src/renderer/src/app-state/types.ts +++ b/src/renderer/src/app-state/types.ts @@ -77,6 +77,9 @@ export type UiShellSlice = UiShellState & { * is captured here rather than resolved at commit time. */ openNewAgentForProject: (tabId: TabId, anchorSessionId: SessionId) => void closeNewAgentPlacement: () => void + /** Open/close the New Agent In… dialog (#852). See `newAgentInOpen`. */ + openNewAgentIn: () => void + closeNewAgentIn: () => void openTiledDispatchPrompt: () => void openDispatchRowProjectPicker: (rowIndex: number) => void closeDispatchRowProjectPicker: () => void diff --git a/src/renderer/src/app-state/uiShell/slice.ts b/src/renderer/src/app-state/uiShell/slice.ts index c2e857e1b..033f0030b 100644 --- a/src/renderer/src/app-state/uiShell/slice.ts +++ b/src/renderer/src/app-state/uiShell/slice.ts @@ -27,6 +27,7 @@ export const createUiShellSlice: StateCreator< viewPromptsSessionId: null, newAgentPlacementOpen: false, newAgentProjectIntent: null, + newAgentInOpen: false, tiledDispatchPromptOpen: false, dispatchRowProjectPickerRow: null, dispatchAttachIntent: null, @@ -176,6 +177,10 @@ export const createUiShellSlice: StateCreator< false, 'uiShell/closeNewAgentPlacement', ), + openNewAgentIn: () => + set({ newAgentInOpen: true }, false, 'uiShell/openNewAgentIn'), + closeNewAgentIn: () => + set({ newAgentInOpen: false }, false, 'uiShell/closeNewAgentIn'), openTiledDispatchPrompt: () => set({ tiledDispatchPromptOpen: true }, false, 'uiShell/openTiledDispatchPrompt'), closeTiledDispatchPrompt: () => diff --git a/src/renderer/src/app-state/uiShell/types.ts b/src/renderer/src/app-state/uiShell/types.ts index 0d3144960..28aa42f5f 100644 --- a/src/renderer/src/app-state/uiShell/types.ts +++ b/src/renderer/src/app-state/uiShell/types.ts @@ -173,6 +173,24 @@ export type UiShellState = { * clicked group is a correct and always-present source. */ newAgentProjectIntent: { tabId: TabId; anchorSessionId: SessionId } | null + /** + * The New Agent In… dialog (#852) is on screen. + * + * WHY a plain boolean when `newAgentProjectIntent` above and + * `dispatchRowProjectPickerRow` below capture a target up front: those are + * opened FROM a specific place (a project header, a row header) whose + * identity the later commit must not re-derive. This dialog is opened from a + * command and its whole job is to ASK for the project, so there is nothing + * to capture. The lane is deliberately not captured either — like New + * Agent…, the spawn fills whichever lane is focused when it commits, and the + * modal dialog owns app input in between, so the user cannot move lane focus + * under it. + * + * A separate flag rather than another `newAgentPlacementOpen` intent because + * it is a separate surface (NewAgentInDialog), not a mode of the placement + * overlay — see that component's header for why. + */ + newAgentInOpen: boolean gitBarOpen: boolean worktreesBarOpen: boolean debugPanelOpen: boolean diff --git a/src/renderer/src/app/surfaces/registry.tsx b/src/renderer/src/app/surfaces/registry.tsx index d6737a220..9cf7dbc32 100644 --- a/src/renderer/src/app/surfaces/registry.tsx +++ b/src/renderer/src/app/surfaces/registry.tsx @@ -30,6 +30,7 @@ import { RewindToPromptSurface } from '@renderer/features/workspace/surfaces/Rew import { AgentTitlePromptSurface } from '@renderer/features/workspace/surfaces/AgentTitlePromptSurface' import { ProviderSwitchPickerSurface } from '@renderer/features/workspace/surfaces/ProviderSwitchPickerSurface' import { KeyVaultModalSurface } from '@renderer/features/key-vault/surfaces/KeyVaultModalSurface' +import { NewAgentInSurface } from '@renderer/features/workspace/surfaces/NewAgentInSurface' // The surface registry (issue #494). Adding a surface = write a wrapper // in the owning feature's surfaces/ folder + add ONE import + ONE array @@ -89,6 +90,10 @@ export const modalSurfaces: SurfaceEntry[] = [ // established surface below one it used to cover; see the registry contract. { id: 'provider-switch-picker', Component: ProviderSwitchPickerSurface }, { id: 'key-vault', Component: KeyVaultModalSurface }, + // Appended per the contract above. It is only opened from a command, which + // closes the palette first, so it has no stacking relationship to reason + // about beyond "a new modal paints over the established ones". + { id: 'new-agent-in', Component: NewAgentInSurface }, ] /** diff --git a/src/renderer/src/features/command-palette/catalog.test.ts b/src/renderer/src/features/command-palette/catalog.test.ts index 90d4286b0..58c98b37a 100644 --- a/src/renderer/src/features/command-palette/catalog.test.ts +++ b/src/renderer/src/features/command-palette/catalog.test.ts @@ -12,8 +12,9 @@ import type { CommandDef } from '@renderer/features/command-palette/types' // This file pinned the exact 102-id before-state, then the 106-id governance // after-state (102 - 5 retired + 9 added), then 112 with Grid Dispatch's six // row commands (#681), 113 with New Window (#688), 114 with Clear -// Agent Composer (#683), 115 with API Key Vault (#831), and 116 with -// Remove Cybersecurity Block (#848). Keeping ONE snapshot that moved — rather +// Agent Composer (#683), 115 with API Key Vault (#831), 116 with +// Remove Cybersecurity Block (#848), and 117 with New Agent In… (#852). +// Keeping ONE snapshot that moved — rather // than a "baseline" file and an "after" file — is what makes the plan's // headline count an assertion anyone can check against running code instead of // prose. @@ -39,8 +40,11 @@ const BASELINE_COMMAND_IDS: readonly string[] = [ 'resume-session', // windowCommands (1) 'new-window', - // paneCommands (31: 27 literal + 4 generated provider splits) + // paneCommands (32: 28 literal + 4 generated provider splits) 'new-agent', + // Registered directly after New Agent… so the two creation entry points sit + // together in the empty-query browse order (#852). + 'new-agent-in', 'split-vertical', 'split-horizontal', 'close-pane', @@ -191,12 +195,12 @@ const NAVIGATION_COMMAND_GROUP: readonly string[] = [ const ids = (): string[] => builtInCommandCatalog.map(c => c.id) describe('built-in command catalog — baseline characterization', () => { - it('contains exactly the 115 governed commands in registration order', () => { + it('contains exactly the 117 governed commands in registration order', () => { // Order matters: this is the palette's empty-query browse order. expect(ids()).toEqual([...BASELINE_COMMAND_IDS]) }) - it('has exactly 115 commands', () => { + it('has exactly 117 commands', () => { // Stated separately from the order assertion because this number is the // thing that moves, and a bare count failure is a clearer signal than a // 99-line array diff. @@ -206,10 +210,12 @@ describe('built-in command catalog — baseline characterization', () => { // with the two lane-removal commands → 105 with Set Agent Title → 106 with // New Lane → 112 with Grid Dispatch's six row commands → 113 with New // Window → 114 with Clear Agent Composer (#683) → 115 with API Key - // Vault (#831). Each - // step of that arithmetic was a deliberate edit to this line, which is the - // entire point of pinning it. - expect(builtInCommandCatalog).toHaveLength(116) + // Vault (#831) → 116 with Remove Cybersecurity Block (#848) → 117 with + // New Agent In… (#852). Each step of that arithmetic was a deliberate edit + // to this line, which is the entire point of pinning it. (The two test + // titles above had drifted to "115" while this line said 116; they now + // track it again.) + expect(builtInCommandCatalog).toHaveLength(117) }) it('reports no structural defects', () => { @@ -240,11 +246,12 @@ describe('generated per-provider split commands', () => { }) it('accounts for the difference between literal and total command count', () => { - // 116 total - 4 generated = 112 literal `id:` fields across the command + // 117 total - 4 generated = 113 literal `id:` fields across the command // modules. At the original baseline this read 102 - 4 = 98; it moved down by // the five retirements, then back up by the nine additions, Grid Dispatch's - // six row commands, and New Window. - expect(builtInCommandCatalog.length - nonDefaultProviders.length * 2).toBe(112) + // six row commands, New Window, and the later single additions recorded in + // the count test above (through New Agent In…, #852). + expect(builtInCommandCatalog.length - nonDefaultProviders.length * 2).toBe(113) }) it('emits both directions for every non-default provider', () => { @@ -343,7 +350,7 @@ describe('governance targets', () => { }) it('lands on the arithmetic the plan predicted', () => { - // 102 baseline - 5 retirements + 18 additions = 115, checked against the + // 102 baseline - 5 retirements + 20 additions = 117, checked against the // real catalog rather than trusted as prose. // // The subtracted term is the count of APPROVED ADDITIONS and the expected @@ -361,10 +368,10 @@ describe('governance targets', () => { // `new-dispatch-row`, `remove-dispatch-row`, `dispatch-row-project`, // `dispatch-row-child-cap`, `dispatch-focus-row-up`, // `dispatch-focus-row-down`, `new-window` (#688), and - // `clear-agent-composer` (#683), `api-key-vault` (#831), and - // `remove-cybersecurity-block` (#848). - expect(builtInCommandCatalog.length + RETIRED_COMMAND_IDS.length - 19).toBe(102) - expect(builtInCommandCatalog).toHaveLength(116) + // `clear-agent-composer` (#683), `api-key-vault` (#831), + // `remove-cybersecurity-block` (#848), and `new-agent-in` (#852). + expect(builtInCommandCatalog.length + RETIRED_COMMAND_IDS.length - 20).toBe(102) + expect(builtInCommandCatalog).toHaveLength(117) }) }) diff --git a/src/renderer/src/features/command-palette/types.ts b/src/renderer/src/features/command-palette/types.ts index fa93307d3..24e777e27 100644 --- a/src/renderer/src/features/command-palette/types.ts +++ b/src/renderer/src/features/command-palette/types.ts @@ -250,6 +250,10 @@ export type CommandContext = { * session id is the parent agent; the overlay only asks for * Claude/Codex and then delegates to workspace.createLinkedAgent. */ openLinkedAgent: (sessionId: string) => void + /** Open the New Agent In… dialog (#852): agent, then project, into the + * focused Dispatch lane. Takes no target — choosing one is the dialog's + * job; see uiShell `newAgentInOpen`. */ + openNewAgentIn: () => void /** Open the Pin Agents multi-select modal. Lives on uiShell as a * transient flag — the draft selection state is owned by the * modal itself, not the store. */ diff --git a/src/renderer/src/features/command-palette/ui/CommandPalette.tsx b/src/renderer/src/features/command-palette/ui/CommandPalette.tsx index 66ad43414..3ea9ce1b0 100644 --- a/src/renderer/src/features/command-palette/ui/CommandPalette.tsx +++ b/src/renderer/src/features/command-palette/ui/CommandPalette.tsx @@ -317,6 +317,7 @@ function OpenCommandPalette({ const openTiledDispatchPrompt = useAppStore(state => state.openTiledDispatchPrompt) const openDispatchAttach = useAppStore(state => state.openDispatchAttach) const openLinkedAgent = useAppStore(state => state.openLinkedAgent) + const openNewAgentIn = useAppStore(state => state.openNewAgentIn) const openPinAgents = useAppStore(state => state.openPinAgents) const toggleCaffeinate = useCaffeinateStore(state => state.toggle) const caffeinateStatus = useCaffeinateStore(state => state.status) @@ -659,6 +660,7 @@ function OpenCommandPalette({ openTiledDispatchPrompt, openDispatchAttach, openLinkedAgent, + openNewAgentIn, openPinAgents, setAggressiveDebugPersistence, enterResumeMode, @@ -768,6 +770,7 @@ function OpenCommandPalette({ openTiledDispatchPrompt, openDispatchAttach, openLinkedAgent, + openNewAgentIn, openPinAgents, setAggressiveDebugPersistence, enterResumeMode, diff --git a/src/renderer/src/features/workspace/commands/paneCommands.ts b/src/renderer/src/features/workspace/commands/paneCommands.ts index 576677505..307becff9 100644 --- a/src/renderer/src/features/workspace/commands/paneCommands.ts +++ b/src/renderer/src/features/workspace/commands/paneCommands.ts @@ -49,6 +49,29 @@ export const paneCommands: CommandDef[] = [ when: ({ workspace }) => Boolean(workspace.activeTab && !workspace.tileTabs), run: ({ workspace }) => workspace.startNewAgentPlacement(), }, + { + // New Agent… files a Dispatch agent under whatever project the spawn + // resolver derives from FOCUS. With an empty lane in an unbound row that is + // the last agent you selected, so aiming at another project meant first + // selecting some unrelated agent in it purely to move the target, then + // coming back to the lane (#852). This asks for the project instead. It + // still fills the focused lane, exactly like New Agent…. + id: 'new-agent-in', + category: 'create', + // `dispatch`, not `app`: in the grid a project is a tab one keystroke away, + // and a detached agent spawned from the grid lands nowhere visible — the + // grid has no lanes to fill and Dispatch rows are not on screen. + surface: 'dispatch', + // Title per docs/command-style.md: "New X" for creation, and the ellipsis + // because the command asks for more input (agent, then project). + title: 'New Agent In…', + description: '**What it does:** Starts a **new agent in a project you choose**, in the focused Dispatch lane.\n\n**Use when:** You are filling an empty lane with an agent for a different project than the one you last selected.\n\n**Notes:** Pick the agent, then the project. A row limited to certain projects only offers those.', + keywords: ['new', 'agent', 'project', 'lane', 'fill', 'empty', 'dispatch', 'claude', 'codex', 'opencode'], + // Same data gate as New Agent…. Tiled Tabs covers Dispatch, so the lane the + // agent would fill is not the thing on screen. + when: ({ workspace }) => Boolean(workspace.activeTab && !workspace.tileTabs), + run: ({ ui }) => ui.openNewAgentIn(), + }, { // `grid` surface — applies to split-vertical, split-horizontal, // codex-vertical, codex-horizontal, terminal-horizontal and diff --git a/src/renderer/src/features/workspace/controlReference.ts b/src/renderer/src/features/workspace/controlReference.ts index f351d7d59..6eba67e38 100644 --- a/src/renderer/src/features/workspace/controlReference.ts +++ b/src/renderer/src/features/workspace/controlReference.ts @@ -47,7 +47,8 @@ export const controlReference = [ "global-dispatch", "tiled-dispatch", "new-dispatch-row", - "new-tiled-lane" + "new-tiled-lane", + "new-agent-in" ] }, { diff --git a/src/renderer/src/features/workspace/lib/newAgentInProjects.test.ts b/src/renderer/src/features/workspace/lib/newAgentInProjects.test.ts new file mode 100644 index 000000000..2c612b232 --- /dev/null +++ b/src/renderer/src/features/workspace/lib/newAgentInProjects.test.ts @@ -0,0 +1,147 @@ +import { describe, expect, it } from 'vitest' + +import { buildNewAgentInModel } from '@renderer/features/workspace/lib/newAgentInProjects' +import type { DispatchModeState, TileNode, WorkspaceState } from '@renderer/workspace/types' + +// Three projects, each with one grid agent, so labels A/B/C are all in play and +// a filtered list can prove it keeps the GLOBAL letter rather than re-lettering. +function leaf(sessionId: string): TileNode { + return { type: 'leaf', sessionId } +} + +function makeState(dispatchMode: DispatchModeState | null): WorkspaceState { + return { + tabs: [ + { id: 'tabA', title: 'project-a', root: leaf('a1'), focusedSessionId: 'a1' }, + { id: 'tabB', title: 'project-b', root: leaf('b1'), focusedSessionId: 'b1' }, + { id: 'tabC', title: 'project-c', root: leaf('c1'), focusedSessionId: 'c1' }, + ], + activeTabId: 'tabA', + dispatchMode, + sessions: { + a1: { cwd: '/work/project-a', kind: 'claude' }, + b1: { cwd: '/work/project-b', kind: 'codex' }, + c1: { cwd: '/work/project-c', kind: 'claude' }, + }, + detachedSessions: {}, + buried: [], + pinnedSessionIds: [], + } +} + +/** Grid Dispatch with two rows of two lanes; lane 2 (row 1) focused and empty. */ +function gridWithFocusedEmptyLane(rowOneProjects?: string[]): DispatchModeState { + return { + scope: 'global', + focusedSessionId: 'a1', + tiled: { + focusedLane: 2, + lanes: [{ selectedSessionId: 'a1' }, {}, {}, {}], + rows: [ + { length: 2 }, + rowOneProjects ? { length: 2, projectTabIds: rowOneProjects } : { length: 2 }, + ], + }, + } +} + +describe('buildNewAgentInModel', () => { + it('offers every project in tab order with the Dispatch letter labels when the row is unbound', () => { + const model = buildNewAgentInModel(makeState(gridWithFocusedEmptyLane())) + + expect(model.projects.map(p => [p.tabId, p.label, p.title])).toEqual([ + ['tabA', 'A', 'project-a'], + ['tabB', 'B', 'project-b'], + ['tabC', 'C', 'project-c'], + ]) + }) + + it('offers only the focused row s bound projects, keeping their global letters', () => { + // Bound to C then B: the list still follows TAB order and still says "C", + // because the letter is the name Dispatch uses for that project everywhere. + const model = buildNewAgentInModel(makeState(gridWithFocusedEmptyLane(['tabC', 'tabB']))) + + expect(model.projects.map(p => [p.tabId, p.label])).toEqual([ + ['tabB', 'B'], + ['tabC', 'C'], + ]) + }) + + it('anchors a project on its first session with a directory, grid leaf before detached rows', () => { + const state = makeState(gridWithFocusedEmptyLane()) + state.sessions.b2 = { cwd: '/work/project-b/.worktrees/task', kind: 'codex' } + state.detachedSessions.b2 = { + sessionId: 'b2', + surface: 'dispatch', + projectTabId: 'tabB', + projectTabTitle: 'project-b', + projectTabIndex: 1, + detachedAt: 10, + } + + const projectB = buildNewAgentInModel(state).projects.find(p => p.tabId === 'tabB') + + // The grid leaf is the project's own directory; the detached worktree + // agent is only the fallback. Same order the Dispatch header "+" uses. + expect(projectB).toMatchObject({ anchorSessionId: 'b1', disabledReason: null }) + }) + + it('falls back to a detached row when the grid leaf has no live session behind it', () => { + const state = makeState(gridWithFocusedEmptyLane()) + delete state.sessions.b1 + state.sessions.b2 = { cwd: '/work/project-b', kind: 'codex' } + state.detachedSessions.b2 = { + sessionId: 'b2', + surface: 'dispatch', + projectTabId: 'tabB', + projectTabTitle: 'project-b', + projectTabIndex: 1, + detachedAt: 10, + } + + const projectB = buildNewAgentInModel(state).projects.find(p => p.tabId === 'tabB') + + expect(projectB).toMatchObject({ anchorSessionId: 'b2', disabledReason: null }) + }) + + it('lists a project with no session to borrow a directory from as disabled, with the reason', () => { + const state = makeState(gridWithFocusedEmptyLane()) + delete state.sessions.c1 + + const projectC = buildNewAgentInModel(state).projects.find(p => p.tabId === 'tabC') + + expect(projectC?.anchorSessionId).toBeNull() + expect(projectC?.disabledReason).toMatch(/no agent/i) + }) + + it('first highlights the project plain New Agent would have used', () => { + // Classic focus b1 + unbound empty lane => the spawn resolver picks tabB. + const dispatchMode = gridWithFocusedEmptyLane() + dispatchMode.focusedSessionId = 'b1' + + expect(buildNewAgentInModel(makeState(dispatchMode)).initialTabId).toBe('tabB') + }) + + it('first highlights the first enabled project when the spawn target is not on offer', () => { + // A row bound to C whose focused lane still shows A's agent (binding + // filters, it never evicts), so plain New Agent would target A — which + // this row does not offer. + const dispatchMode = gridWithFocusedEmptyLane(['tabC']) + dispatchMode.tiled!.lanes[2] = { selectedSessionId: 'a1' } + + expect(buildNewAgentInModel(makeState(dispatchMode)).initialTabId).toBe('tabC') + }) + + it('never highlights a disabled project', () => { + // b1 still exists (so the spawn resolver still targets tabB) but has no + // directory, so tabB cannot be anchored. Active tab is C so the expected A + // can only come from the "first enabled project" rule, not from activeTabId. + const dispatchMode = gridWithFocusedEmptyLane() + dispatchMode.focusedSessionId = 'b1' + const state = makeState(dispatchMode) + state.activeTabId = 'tabC' + state.sessions.b1 = { cwd: '', kind: 'codex' } + + expect(buildNewAgentInModel(state).initialTabId).toBe('tabA') + }) +}) diff --git a/src/renderer/src/features/workspace/lib/newAgentInProjects.ts b/src/renderer/src/features/workspace/lib/newAgentInProjects.ts new file mode 100644 index 000000000..825d7e573 --- /dev/null +++ b/src/renderer/src/features/workspace/lib/newAgentInProjects.ts @@ -0,0 +1,112 @@ +import { + focusedLaneBoundProjectTabIds, + resolveDispatchSpawnTarget, +} from '@renderer/workspace/dispatch/dispatchSelectors' +import { resolveTabSessions } from '@renderer/workspace/queries' +import { tabIndexLabel } from '@renderer/workspace/tile-tree/paneLabels' +import type { SessionId, TabId, WorkspaceState } from '@renderer/workspace/types' + +// --------------------------------------------------------------------------- +// Project list for New Agent In… (#852). +// +// The command exists because filling an empty Grid Dispatch lane used to need +// a navigational detour: plain New Agent… files the agent under whatever +// project the spawn resolver derives from focus, so the only way to aim it at +// project C was to go select some unrelated agent in C first. This module +// answers "which projects can the picker offer, and how would each one be +// spawned into?" as a pure function of workspace state, so the dialog stays a +// dumb list and the rules below are testable without React. +// --------------------------------------------------------------------------- + +export type NewAgentInProject = { + tabId: TabId + /** + * A/B/C — the tab's position in the FULL tab list, never re-lettered after + * row-binding filtering. The Dispatch index, pane labels and the row-project + * picker all name projects this way; a picker that called project C "B" + * because B was filtered out would contradict every other surface. + */ + label: string + title: string + /** + * The existing session whose cwd the new agent borrows, or null when the + * project has none. + * + * WHY an anchor session at all: a Tab has no directory of its own — cwd lives + * on sessions — and `createDetachedDispatchAgent` deliberately takes the + * override as `{ tabId, anchorSessionId }` for exactly this reason (see the + * WHY on `newAgentProjectIntent` in uiShell/types.ts). + */ + anchorSessionId: SessionId | null + /** Set exactly when `anchorSessionId` is null; shown on the disabled row. */ + disabledReason: string | null +} + +export type NewAgentInModel = { + projects: NewAgentInProject[] + /** Row to highlight when the project step opens; null when none is enabled. */ + initialTabId: TabId | null +} + +const NO_ANCHOR_REASON = 'No agent in this project to take a working directory from' + +/** + * First session of the project that has a directory to borrow. + * + * Order comes from `resolveTabSessions`: grid leaves first, then detached rows + * by `detachedAt`. That is the same order the Dispatch project header's "+" + * reaches for (it passes the group's first row), so the two entry points spawn + * a given project into the same directory. Grid leaves first also matters on + * its own: they are usually the project's root checkout, while detached rows + * are where worktree agents live — borrowing a worktree path would start the + * new agent inside someone else's task branch. + * + * A grid leaf can outlive its SessionMeta (resolveTabSessions does not filter + * grid ids by existence), and a meta can carry an empty cwd; both are skipped + * rather than trusted, because `createDetachedDispatchAgent` would otherwise + * fail late with a toast instead of the picker saying so up front. + */ +function anchorFor(state: WorkspaceState, tabId: TabId): SessionId | null { + for (const sessionId of resolveTabSessions(state, tabId)) { + if (state.sessions[sessionId]?.cwd) return sessionId + } + return null +} + +export function buildNewAgentInModel(state: WorkspaceState): NewAgentInModel { + // Row bindings restrict the offer. A binding says "this row holds these + // projects" and the row's own index only lists them; spawning another + // project's agent into one of its lanes would put an agent where its own + // index cannot show it. The spawn resolver honours the same selector. + const bound = new Set(focusedLaneBoundProjectTabIds(state)) + + const projects: NewAgentInProject[] = [] + state.tabs.forEach((tab, tabIndex) => { + // Iterate TABS (not the binding) so the list keeps tab order: the binding + // is a set the user built by clicking, and its insertion order means + // nothing to someone scanning A, B, C. + if (bound.size > 0 && !bound.has(tab.id)) return + const anchorSessionId = anchorFor(state, tab.id) + projects.push({ + tabId: tab.id, + label: tabIndexLabel(tabIndex), + title: tab.title, + anchorSessionId, + disabledReason: anchorSessionId ? null : NO_ANCHOR_REASON, + }) + }) + + // Start on the project plain New Agent… would have used, so Enter, Enter is + // never worse than the command it sits beside — the new command only adds + // the ability to point somewhere else. When that project is not on offer (a + // bound row whose lane still shows another project's agent) or cannot be + // anchored, fall back to the first project that can actually take an agent. + const spawnTabId = resolveDispatchSpawnTarget(state).tabId + const enabled = projects.filter(project => project.anchorSessionId !== null) + const initialTabId = + enabled.find(project => project.tabId === spawnTabId)?.tabId ?? + enabled[0]?.tabId ?? + null + + return { projects, initialTabId } +} diff --git a/src/renderer/src/features/workspace/surfaces/NewAgentInSurface.tsx b/src/renderer/src/features/workspace/surfaces/NewAgentInSurface.tsx new file mode 100644 index 000000000..9d3dbca1e --- /dev/null +++ b/src/renderer/src/features/workspace/surfaces/NewAgentInSurface.tsx @@ -0,0 +1,12 @@ +import { NewAgentInDialog } from '@renderer/features/workspace/ui/NewAgentInDialog' +import { useAppStore } from '@renderer/app-state/hooks' +import { useWorkspaceContext } from '@renderer/workspace/WorkspaceContext' + +// New Agent In… (#852). A boolean flag, not a captured target — choosing the +// project is what the dialog is for; see UiShellState.newAgentInOpen. +export function NewAgentInSurface() { + const workspace = useWorkspaceContext() + const open = useAppStore(state => state.newAgentInOpen) + const close = useAppStore(state => state.closeNewAgentIn) + return +} diff --git a/src/renderer/src/features/workspace/ui/NewAgentInDialog.renderer.test.tsx b/src/renderer/src/features/workspace/ui/NewAgentInDialog.renderer.test.tsx new file mode 100644 index 000000000..8327915ce --- /dev/null +++ b/src/renderer/src/features/workspace/ui/NewAgentInDialog.renderer.test.tsx @@ -0,0 +1,170 @@ +import { fireEvent, render, screen } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' + +import * as workspaceQueries from '@renderer/workspace/queries' +import type { Workspace } from '@renderer/workspace/workspaceStore' +import type { WorkspaceState } from '@renderer/workspace/types' + +import { NewAgentInDialog } from './NewAgentInDialog' + +afterEach(() => { + vi.restoreAllMocks() +}) + +// Three projects in Grid Dispatch. The focused lane (2) is EMPTY and the classic +// Dispatch focus is B's agent — the exact situation that used to force the +// "go select an agent in the right project first" detour. Plain New Agent… +// would therefore target project B. +function workspaceState(): WorkspaceState { + return { + tabs: [ + { id: 'tabA', title: 'project-a', root: { type: 'leaf', sessionId: 'a1' }, focusedSessionId: 'a1' }, + { id: 'tabB', title: 'project-b', root: { type: 'leaf', sessionId: 'b1' }, focusedSessionId: 'b1' }, + { id: 'tabC', title: 'project-c', root: { type: 'leaf', sessionId: 'c1' }, focusedSessionId: 'c1' }, + ], + activeTabId: 'tabA', + dispatchMode: { + scope: 'global', + focusedSessionId: 'b1', + tiled: { + focusedLane: 2, + lanes: [{ selectedSessionId: 'a1' }, { selectedSessionId: 'b1' }, {}], + }, + }, + sessions: { + a1: { cwd: '/work/project-a', kind: 'claude' }, + b1: { cwd: '/work/project-b', kind: 'codex' }, + c1: { cwd: '/work/project-c', kind: 'claude' }, + }, + detachedSessions: {}, + buried: [], + pinnedSessionIds: [], + } +} + +function harness(options: { open?: boolean; state?: WorkspaceState } = {}) { + const createDetachedDispatchAgent = vi.fn(async () => 'new-agent') + const onClose = vi.fn() + const workspace = { + state: options.state ?? workspaceState(), + createDetachedDispatchAgent, + } as unknown as Workspace + const mounted = render( + , + ) + const dialog = () => screen.getByRole('dialog') + const press = (key: string) => fireEvent.keyDown(dialog(), { key }) + return { createDetachedDispatchAgent, onClose, mounted, workspace, press } +} + +describe('NewAgentInDialog', () => { + it('Enter, Enter creates the default agent where plain New Agent would have put it', () => { + // The new command must never be worse than the one beside it: accepting + // both defaults reproduces New Agent…'s own target (project B here). + const { createDetachedDispatchAgent, onClose, press } = harness() + + press('Enter') + press('Enter') + + expect(onClose).toHaveBeenCalledOnce() + expect(createDetachedDispatchAgent).toHaveBeenCalledWith( + { kind: 'claude', providerRuntime: undefined }, + { tabId: 'tabB', anchorSessionId: 'b1' }, + ) + }) + + it('creates the chosen agent in the chosen project from the keyboard, without refocusing anything first', () => { + const { createDetachedDispatchAgent, press } = harness() + + press('ArrowDown') // Claude -> Codex + press('Enter') + expect(screen.getByText('C · project-c')).toBeInTheDocument() + press('ArrowDown') // B (initial) -> C + press('Enter') + + expect(createDetachedDispatchAgent).toHaveBeenCalledWith( + { kind: 'codex', providerRuntime: undefined }, + { tabId: 'tabC', anchorSessionId: 'c1' }, + ) + }) + + it('keeps OpenCode Terminal a distinct runtime choice on the mouse path', () => { + const { createDetachedDispatchAgent } = harness() + + fireEvent.click(screen.getByText('OpenCode Terminal').closest('button')!) + fireEvent.click(screen.getByText('A · project-a').closest('button')!) + + expect(createDetachedDispatchAgent).toHaveBeenCalledWith( + { kind: 'opencode', providerRuntime: 'terminal' }, + { tabId: 'tabA', anchorSessionId: 'a1' }, + ) + }) + + it('Backspace on the project step goes back to the agent step without creating anything', () => { + const { createDetachedDispatchAgent, press } = harness() + + press('Enter') + expect(screen.queryByText('Codex')).not.toBeInTheDocument() + press('Backspace') + + expect(screen.getByText('Codex')).toBeInTheDocument() + expect(createDetachedDispatchAgent).not.toHaveBeenCalled() + }) + + it('shows a project it cannot anchor as disabled, and neither click nor arrows can choose it', () => { + const state = workspaceState() + delete state.sessions.c1 + const { createDetachedDispatchAgent, press } = harness({ state }) + + press('Enter') + expect(screen.getByText(/no agent in this project/i)).toBeInTheDocument() + fireEvent.click(screen.getByText('C · project-c').closest('button')!) + expect(createDetachedDispatchAgent).not.toHaveBeenCalled() + + // From B, the only row below is the disabled C: the highlight must stay on + // B rather than parking on a row Enter cannot act on. + press('ArrowDown') + press('Enter') + expect(createDetachedDispatchAgent).toHaveBeenCalledWith( + { kind: 'claude', providerRuntime: undefined }, + { tabId: 'tabB', anchorSessionId: 'b1' }, + ) + }) + + it('cannot create a second agent from a repeated Enter while the first spawn is in flight', () => { + // The dialog's `open` prop only drops after the parent reacts to onClose; + // until then a fast double Enter would otherwise spawn twice. + const { createDetachedDispatchAgent, press } = harness() + + press('Enter') + press('Enter') + press('Enter') + + expect(createDetachedDispatchAgent).toHaveBeenCalledOnce() + }) + + it('Cancel closes without creating whichever row is highlighted', () => { + const { createDetachedDispatchAgent, onClose, press } = harness() + + press('Enter') + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })) + + expect(onClose).toHaveBeenCalledOnce() + expect(createDetachedDispatchAgent).not.toHaveBeenCalled() + }) + + it('derives no project list while closed', () => { + // The surface is always mounted and re-renders with every workspace change; + // building the list walks every tab's sessions and the Dispatch rows, which + // is pure waste for a dialog nobody can see. + const enumerateSessions = vi.spyOn(workspaceQueries, 'resolveTabSessions') + const { mounted, workspace, onClose } = harness({ open: false }) + + expect(enumerateSessions).not.toHaveBeenCalled() + expect(screen.queryByRole('dialog')).not.toBeInTheDocument() + + mounted.rerender() + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Enter' }) + expect(enumerateSessions).toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/features/workspace/ui/NewAgentInDialog.tsx b/src/renderer/src/features/workspace/ui/NewAgentInDialog.tsx new file mode 100644 index 000000000..459b87240 --- /dev/null +++ b/src/renderer/src/features/workspace/ui/NewAgentInDialog.tsx @@ -0,0 +1,273 @@ +import { useEffect, useMemo, useRef, useState } from 'react' + +import { Button } from '@renderer/components/ui/button' +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from '@renderer/components/ui/dialog' +import { + buildNewAgentInModel, + type NewAgentInModel, + type NewAgentInProject, +} from '@renderer/features/workspace/lib/newAgentInProjects' +import { AGENT_PROVIDER_CHOICES } from '@renderer/workspace/providerChoices' +import type { TabId } from '@renderer/workspace/types' +import type { Workspace } from '@renderer/workspace/workspaceStore' + +type Props = { + open: boolean + workspace: Workspace + onClose: () => void +} + +type Step = 'agent' | 'project' + +// Shared empty model so a closed dialog hands the same reference to every +// render instead of allocating one per workspace change. +const CLOSED_MODEL: NewAgentInModel = { projects: [], initialTabId: null } + +/** + * New Agent In… (#852): choose an agent, then the project it starts in. + * + * WHY a dedicated dialog rather than another mode on NewAgentPlacementOverlay: + * that overlay already carries four intents (create, attach, linked, header + * project) and treats "a kind has been picked" as "now show grid placement". + * A second, project step would have to be one more special case inside that + * invariant. The palette was the other candidate, but a two-step choice there + * needs the first answer held in store state across palette modes. This file + * is the whole feature's UI, and it composes `DialogContent` so Escape, the + * focus trap, outside-click and app input ownership come from the one + * primitive that owns them (components/ui/README.md). + * + * WHY no lane choice here: the spawn keeps New Agent…'s lane semantics — the + * agent fills the FOCUSED lane (`createDetachedDispatchAgent` keeps the + * focus-derived laneIndex even under a project override). The user's workflow + * is "focus the empty lane, fill it", so the lane is already chosen. + */ +export function NewAgentInDialog({ open, workspace, onClose }: Props) { + const dialogRef = useRef(null) + // One-shot latch around the spawn. `open` only drops once the parent reacts + // to onClose, so a fast second Enter would otherwise start a second agent. + // A ref, not state: it must gate the synchronous key handler, not re-render. + const committingRef = useRef(false) + const [step, setStep] = useState('agent') + const [agentIndex, setAgentIndex] = useState(0) + // The highlighted project is held by TAB ID, not list index: the model is + // live while the dialog is open (an MCP operator can close an agent or a + // tab meanwhile), and an index would silently slide onto a different + // project when a row above it disappears. + const [projectTabId, setProjectTabId] = useState(null) + + // Derived only while open. The surface is always mounted and re-renders on + // every workspace change; the model walks every tab's sessions plus the + // Dispatch rows, which is waste for a dialog nobody can see. + const model = useMemo( + () => (open ? buildNewAgentInModel(workspace.state) : CLOSED_MODEL), + [open, workspace.state], + ) + + useEffect(() => { + if (!open) return + // The instance survives between invocations, so every piece of one-shot + // state resets on open. A leftover `project` step or a set latch would make + // the next invocation start mid-flow or refuse to spawn at all. + setStep('agent') + setAgentIndex(0) + setProjectTabId(null) + committingRef.current = false + }, [open]) + + useEffect(() => { + if (!open) return + // Keep keyboard focus on the dialog surface across steps. Choosing a row + // with the mouse focuses that row's button, and switching steps unmounts + // it; without this the next arrow key would have nothing listening. + dialogRef.current?.focus() + }, [open, step]) + + const choice = AGENT_PROVIDER_CHOICES[agentIndex] ?? null + const enabledProjects = model.projects.filter(project => project.anchorSessionId !== null) + const highlightedProject = + enabledProjects.find(project => project.tabId === projectTabId) ?? null + + const chooseAgent = (index: number) => { + setAgentIndex(index) + setStep('project') + // Start on the project plain New Agent… would have used (the model owns + // that rule), so accepting both defaults never does worse than today. + setProjectTabId(model.initialTabId) + } + + const commit = (project: NewAgentInProject) => { + if (!choice || !project.anchorSessionId || committingRef.current) return + committingRef.current = true + // Close before the spawn: it awaits an IPC round trip, and the agent is + // about to appear in the lane underneath. createDetachedDispatchAgent owns + // failure feedback (toasts), exactly as it does for New Agent… and the + // project header's "+". + onClose() + void workspace.createDetachedDispatchAgent( + { kind: choice.kind, providerRuntime: choice.providerRuntime }, + { tabId: project.tabId, anchorSessionId: project.anchorSessionId }, + ) + } + + const moveAgent = (delta: -1 | 1) => { + setAgentIndex(index => + Math.max(0, Math.min(AGENT_PROVIDER_CHOICES.length - 1, index + delta)), + ) + } + + const moveProject = (delta: -1 | 1) => { + // Arrows walk ENABLED projects only. A highlight parked on a disabled row + // turns Enter into a silent no-op, which reads as a broken dialog; the + // disabled row stays visible so its reason is still readable. + if (enabledProjects.length === 0) return + const current = enabledProjects.findIndex(project => project.tabId === projectTabId) + const next = current < 0 + ? 0 + : Math.max(0, Math.min(enabledProjects.length - 1, current + delta)) + setProjectTabId(enabledProjects[next]!.tabId) + } + + return ( + { + if (!nextOpen) onClose() + }} + > + { + // Focus the surface itself, not the first row: the key handler lives + // here, and a focused row button would also turn Enter into a native + // click on whichever row happened to be first. + event.preventDefault() + dialogRef.current?.focus() + }} + onKeyDown={event => { + const down = event.key === 'ArrowDown' || (event.ctrlKey && event.key === 'n') + const up = event.key === 'ArrowUp' || (event.ctrlKey && event.key === 'p') + if (down || up) { + event.preventDefault() + if (step === 'agent') moveAgent(down ? 1 : -1) + else moveProject(down ? 1 : -1) + return + } + if (event.key === 'Enter') { + // preventDefault also cancels a focused button's native Enter-click, + // so a Tab-focused row cannot fire a second, different action. + event.preventDefault() + if (step === 'agent') chooseAgent(agentIndex) + else if (highlightedProject) commit(highlightedProject) + return + } + if (event.key === 'Backspace' && step === 'project') { + // Back, not cancel: picking the wrong agent should cost one key, + // not the whole flow. The dialog has no text input, so Backspace + // has nothing else it could mean here. + event.preventDefault() + setStep('agent') + } + }} + className="w-[500px] max-w-[calc(100vw-64px)]" + > + + New Agent In + + {step === 'agent' || !choice + ? 'Choose the agent, then the project it starts in.' + : `Choose the project for the new ${choice.label} agent.`} + + + +
+ {step === 'agent' ? ( + AGENT_PROVIDER_CHOICES.map((option, index) => { + const focused = index === agentIndex + return ( + + ) + }) + ) : model.projects.length === 0 ? ( +
+ No projects are open. +
+ ) : ( + model.projects.map(project => { + const enabled = project.anchorSessionId !== null + const focused = enabled && project.tabId === highlightedProject?.tabId + return ( + + ) + }) + )} +
+ + + + {step === 'agent' + ? '↑↓ choose · Enter next · Esc cancel' + : '↑↓ choose · Enter create · ⌫ back · Esc cancel'} + +
+ {step === 'project' ? ( + + ) : null} + +
+ + + + ) +} From 2e793b57ef9fa40b84d6ed3897695531271ad634 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Thu, 10 Sep 2026 17:34:34 -0700 Subject: [PATCH 4/9] fix(ui): let a focused Cancel own Enter in the Switch Provider picker The picker's list handler took every Enter: it called preventDefault, which cancels the focused button's native click, and then committed the highlighted provider. Tab to Cancel and Enter therefore switched the agent's provider instead of cancelling, breaking the rule dialog-actions.tsx documents for focused buttons. Footer buttons now keep their own Enter; the list keeps highlight semantics for rows. Fixes #862 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SVRSHn7c7qnBoUQo1e2X4k --- .../ui/ProviderSwitchPickerModal.renderer.test.tsx | 12 ++++++++++++ .../workspace/ui/ProviderSwitchPickerModal.tsx | 7 +++++++ 2 files changed, 19 insertions(+) diff --git a/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.renderer.test.tsx b/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.renderer.test.tsx index 40e552c3e..740b2c6e6 100644 --- a/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.renderer.test.tsx +++ b/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.renderer.test.tsx @@ -67,6 +67,18 @@ describe('ProviderSwitchPickerModal', () => { ) }) + it('leaves Enter to a focused Cancel instead of switching to the highlighted provider (#862)', () => { + // The list's Enter handler used to preventDefault (killing Cancel's native + // click) and then commit the highlighted row, so Tab -> Cancel -> Enter + // started a provider switch. `true` = default not prevented, so the real + // browser still delivers Cancel's click. + const { switchSessionProvider } = harness() + + expect(fireEvent.keyDown(screen.getByRole('button', { name: 'Cancel' }), { key: 'Enter' })).toBe(true) + + expect(switchSessionProvider).not.toHaveBeenCalled() + }) + it('supports keyboard choice and cancellation without starting an implicit switch', () => { const first = harness() fireEvent.keyDown(screen.getByRole('dialog'), { key: 'ArrowDown' }) diff --git a/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.tsx b/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.tsx index d3b2d4427..427e2749b 100644 --- a/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.tsx +++ b/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.tsx @@ -103,6 +103,13 @@ export function ProviderSwitchPickerModal({ return } if (event.key === 'Enter' && selected) { + // A focused footer button owns its own Enter (the rule + // components/ui/dialog-actions.tsx documents). preventDefault below + // also cancels that button's native click, so without this guard + // Tab to Cancel + Enter switched the agent to the highlighted + // provider instead of cancelling (#862). Rows are exempt from the + // exemption: Enter on the list means "the highlighted row". + if (event.target instanceof Element && event.target.closest('[data-slot="dialog-footer"]')) return event.preventDefault() choose(selected) } From 1b5bd639073b875e7582ab6f1abfa4ad33c00935 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Thu, 10 Sep 2026 17:34:35 -0700 Subject: [PATCH 5/9] fix(dispatch): harden New Agent In keyboard handling after review Review of the new dialog found it copied the Switch Provider Enter bug: Tab to Cancel or Back and Enter spawned an agent. Footer buttons now own their Enter, and auto-repeated Enter is ignored so one long press cannot pick the agent and commit the default project. One-shot state resets on close instead of open, so an open that does not come from a user event never paints a stale project step. The cross-step focus effect is gone because Radix FocusScope already refocuses the container when a focused row unmounts. The empty project list now names the real cause (the row is bound only to closed projects, #863) and the control that fixes it. Comments and the plan state the anchor rule precisely: same project as New Agent, but the project's own checkout. Refs #852 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SVRSHn7c7qnBoUQo1e2X4k --- .../plans/2026-09-10-new-agent-in-project.md | 28 ++++++++- .../workspace/commands/paneCommands.ts | 7 ++- .../workspace/lib/newAgentInProjects.ts | 31 +++++---- .../ui/NewAgentInDialog.renderer.test.tsx | 63 +++++++++++++++++-- .../workspace/ui/NewAgentInDialog.tsx | 62 +++++++++++++----- 5 files changed, 157 insertions(+), 34 deletions(-) diff --git a/docs/superpowers/plans/2026-09-10-new-agent-in-project.md b/docs/superpowers/plans/2026-09-10-new-agent-in-project.md index ec59ce2fa..5850643e7 100644 --- a/docs/superpowers/plans/2026-09-10-new-agent-in-project.md +++ b/docs/superpowers/plans/2026-09-10-new-agent-in-project.md @@ -42,7 +42,10 @@ runs **New Agent…**. Five fresh lanes means five detours. 4. **No default keybinding.** 5. **Initial highlight** on the project step is the project plain New Agent… would have used (`resolveDispatchSpawnTarget(state).tabId`) when it is - eligible, so Enter-Enter is never worse than today. + eligible, so Enter-Enter lands in the same project as today. (Same project, + not necessarily the same directory: the anchor is the project's first + session with a cwd — its own checkout — never the focused/last-selected + agent's worktree, because removing focus from the decision is the point.) 6. **A dedicated dialog**, not a fifth mode on `NewAgentPlacementOverlay` (already four intents; its "kind picked ⇒ placement step" invariant would need another branch) and not a palette sub-mode (`CommandPalette.tsx` is 2.5k lines and each @@ -79,6 +82,29 @@ runs **New Agent…**. Five fresh lanes means five detours. 4. **Verify once at the end** (Node 24): `npm run typecheck`, `npm test`, `npm run test:contract`, `npm run check:keybindings`. +## Review follow-up (after the PR opened) + +An independent review found, and this branch fixed: + +- **Enter on a focused footer button** (Cancel/Back) was captured by the list + handler, which prevented the button's click and committed the highlighted + row — Tab → Cancel → Enter spawned an agent. Footer buttons now own their + Enter (the `dialog-actions.tsx` rule). `ProviderSwitchPickerModal`, whose + pattern this copied, had the same bug on `main`: filed as #862 and fixed + here too. +- A **held Enter** could pick the agent and commit in one press; auto-repeat + is ignored. +- One-shot state now resets on **close**, not open, so a non-user-event open + never paints a stale project step. +- The redundant cross-step focus effect was removed (Radix FocusScope already + refocuses the container when a focused row unmounts). +- The empty project list now says the row's projects are closed and names + **Row Projects…**. Root cause — `closeTab` leaves row bindings in memory — + predates this work and is filed as #863 (out of scope). +- Comment accuracy (header "+" anchor differs when its first row is pinned; + Enter-Enter equivalence is project-level) and a project-scope note in the + command description. + ## Out of scope - A default chord. diff --git a/src/renderer/src/features/workspace/commands/paneCommands.ts b/src/renderer/src/features/workspace/commands/paneCommands.ts index 307becff9..ec30b8ceb 100644 --- a/src/renderer/src/features/workspace/commands/paneCommands.ts +++ b/src/renderer/src/features/workspace/commands/paneCommands.ts @@ -65,7 +65,12 @@ export const paneCommands: CommandDef[] = [ // Title per docs/command-style.md: "New X" for creation, and the ellipsis // because the command asks for more input (agent, then project). title: 'New Agent In…', - description: '**What it does:** Starts a **new agent in a project you choose**, in the focused Dispatch lane.\n\n**Use when:** You are filling an empty lane with an agent for a different project than the one you last selected.\n\n**Notes:** Pick the agent, then the project. A row limited to certain projects only offers those.', + // The scope sentence is there because it surprised the reviewer: in + // project-scope Dispatch, spawning into another project makes it the active + // project (createDetachedDispatchAgent selects what it creates), so the + // other lanes read "Not in this scope" until you switch back. That is the + // scope contract working — the new agent has to be visible — not a bug. + description: '**What it does:** Starts a **new agent in a project you choose**, in the focused Dispatch lane.\n\n**Use when:** You are filling an empty lane with an agent for a different project than the one you last selected.\n\n**Notes:** Pick the agent, then the project. A row limited to certain projects only offers those. In project-scoped Dispatch, choosing another project switches to it.', keywords: ['new', 'agent', 'project', 'lane', 'fill', 'empty', 'dispatch', 'claude', 'codex', 'opencode'], // Same data gate as New Agent…. Tiled Tabs covers Dispatch, so the lane the // agent would fill is not the thing on screen. diff --git a/src/renderer/src/features/workspace/lib/newAgentInProjects.ts b/src/renderer/src/features/workspace/lib/newAgentInProjects.ts index 825d7e573..5fc833f03 100644 --- a/src/renderer/src/features/workspace/lib/newAgentInProjects.ts +++ b/src/renderer/src/features/workspace/lib/newAgentInProjects.ts @@ -54,12 +54,19 @@ const NO_ANCHOR_REASON = 'No agent in this project to take a working directory f * First session of the project that has a directory to borrow. * * Order comes from `resolveTabSessions`: grid leaves first, then detached rows - * by `detachedAt`. That is the same order the Dispatch project header's "+" - * reaches for (it passes the group's first row), so the two entry points spawn - * a given project into the same directory. Grid leaves first also matters on - * its own: they are usually the project's root checkout, while detached rows - * are where worktree agents live — borrowing a worktree path would start the - * new agent inside someone else's task branch. + * by `detachedAt`. That matches the Dispatch project header's "+" (it passes + * the group's first row) except when that row is pinned — pins are lifted out + * of project groups, so the "+" may then anchor on the next row. Grid leaves + * first matters on its own: they are usually the project's root checkout, + * while detached rows are where worktree agents live — borrowing a worktree + * path would start the new agent inside someone else's task branch. + * + * WHY the anchor deliberately ignores focus, unlike plain New Agent… (which + * borrows the focused lane's or last-selected agent's cwd, worktree or not): + * this command exists to remove focus from the decision. The user named a + * PROJECT; its own checkout is what "in project B" means. Inheriting whichever + * worktree agent in B happened to be selected last is the focus-dependence the + * command was built to escape. * * A grid leaf can outlive its SessionMeta (resolveTabSessions does not filter * grid ids by existence), and a meta can carry an empty cwd; both are skipped @@ -96,11 +103,13 @@ export function buildNewAgentInModel(state: WorkspaceState): NewAgentInModel { }) }) - // Start on the project plain New Agent… would have used, so Enter, Enter is - // never worse than the command it sits beside — the new command only adds - // the ability to point somewhere else. When that project is not on offer (a - // bound row whose lane still shows another project's agent) or cannot be - // anchored, fall back to the first project that can actually take an agent. + // Start on the PROJECT plain New Agent… would have used, so Enter, Enter + // lands in the same project as the command it sits beside — the new command + // only adds the ability to point somewhere else. (Same project, not always + // the same directory: see `anchorFor` for why this one uses the project's + // own checkout.) When that project is not on offer (a bound row whose lane + // still shows another project's agent) or cannot be anchored, fall back to + // the first project that can actually take an agent. const spawnTabId = resolveDispatchSpawnTarget(state).tabId const enabled = projects.filter(project => project.anchorSessionId !== null) const initialTabId = diff --git a/src/renderer/src/features/workspace/ui/NewAgentInDialog.renderer.test.tsx b/src/renderer/src/features/workspace/ui/NewAgentInDialog.renderer.test.tsx index 8327915ce..825cdbce9 100644 --- a/src/renderer/src/features/workspace/ui/NewAgentInDialog.renderer.test.tsx +++ b/src/renderer/src/features/workspace/ui/NewAgentInDialog.renderer.test.tsx @@ -58,9 +58,9 @@ function harness(options: { open?: boolean; state?: WorkspaceState } = {}) { } describe('NewAgentInDialog', () => { - it('Enter, Enter creates the default agent where plain New Agent would have put it', () => { - // The new command must never be worse than the one beside it: accepting - // both defaults reproduces New Agent…'s own target (project B here). + it('Enter, Enter creates the default agent in the project plain New Agent would have used', () => { + // Accepting both defaults lands in New Agent…'s own target PROJECT (B + // here). The directory is B's own checkout by design — see anchorFor. const { createDetachedDispatchAgent, onClose, press } = harness() press('Enter') @@ -73,7 +73,7 @@ describe('NewAgentInDialog', () => { ) }) - it('creates the chosen agent in the chosen project from the keyboard, without refocusing anything first', () => { + it('creates the chosen agent in the chosen project from the keyboard, without selecting an agent in that project first', () => { const { createDetachedDispatchAgent, press } = harness() press('ArrowDown') // Claude -> Codex @@ -143,6 +143,61 @@ describe('NewAgentInDialog', () => { expect(createDetachedDispatchAgent).toHaveBeenCalledOnce() }) + it('leaves Enter to a focused footer button instead of advancing or creating', () => { + // A focused button owns its own Enter (components/ui/dialog-actions.tsx). + // Before this guard, Tab to Cancel + Enter on the project step SPAWNED an + // agent: the list handler prevented the button's native click and then + // committed the highlighted row. `fireEvent` returning true means the + // default was not prevented, so the real browser's click still happens. + const { createDetachedDispatchAgent, press } = harness() + + expect(fireEvent.keyDown(screen.getByRole('button', { name: 'Cancel' }), { key: 'Enter' })).toBe(true) + expect(screen.getByText('Codex')).toBeInTheDocument() // still on the agent step + + press('Enter') + expect(fireEvent.keyDown(screen.getByRole('button', { name: 'Back' }), { key: 'Enter' })).toBe(true) + expect(fireEvent.keyDown(screen.getByRole('button', { name: 'Cancel' }), { key: 'Enter' })).toBe(true) + + expect(createDetachedDispatchAgent).not.toHaveBeenCalled() + }) + + it('ignores a held Enter, so one long press cannot pick the agent and spawn in the same breath', () => { + const { createDetachedDispatchAgent, press } = harness() + + press('Enter') + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Enter', repeat: true }) + + expect(createDetachedDispatchAgent).not.toHaveBeenCalled() + }) + + it('reopens on the agent step with a fresh commit latch', () => { + const { createDetachedDispatchAgent, mounted, workspace, onClose, press } = harness() + + press('Enter') + press('Enter') // commits and latches + mounted.rerender() + mounted.rerender() + + expect(screen.getByText('Codex')).toBeInTheDocument() + press('Enter') + press('Enter') + expect(createDetachedDispatchAgent).toHaveBeenCalledTimes(2) + }) + + it('says so when every project the focused row is bound to has been closed', () => { + // Closing a tab does not yet clear row bindings in memory (#863), so a row + // can be bound only to projects that no longer exist. "No projects are + // open" would be false — other projects are — and gives no way forward. + const state = workspaceState() + state.dispatchMode!.tiled!.rows = [{ length: 3, projectTabIds: ['tab-closed'] }] + const { press } = harness({ state }) + + press('Enter') + + expect(screen.getByText(/none of this row.s projects are open/i)).toBeInTheDocument() + expect(screen.getByText(/row projects/i)).toBeInTheDocument() + }) + it('Cancel closes without creating whichever row is highlighted', () => { const { createDetachedDispatchAgent, onClose, press } = harness() diff --git a/src/renderer/src/features/workspace/ui/NewAgentInDialog.tsx b/src/renderer/src/features/workspace/ui/NewAgentInDialog.tsx index 459b87240..c2a01488b 100644 --- a/src/renderer/src/features/workspace/ui/NewAgentInDialog.tsx +++ b/src/renderer/src/features/workspace/ui/NewAgentInDialog.tsx @@ -71,24 +71,23 @@ export function NewAgentInDialog({ open, workspace, onClose }: Props) { ) useEffect(() => { - if (!open) return + if (open) return // The instance survives between invocations, so every piece of one-shot - // state resets on open. A leftover `project` step or a set latch would make - // the next invocation start mid-flow or refuse to spawn at all. + // state resets. A leftover `project` step or a set latch would make the + // next invocation start mid-flow or refuse to spawn at all. + // + // WHY on CLOSE rather than on open: an effect runs after the render it + // belongs to, so resetting on open paints one frame of the previous + // invocation's project step first. That frame is invisible when a click or + // key opens the dialog, but not when something outside a user event does + // (an MCP `commands.run`). Resetting while hidden means the first visible + // frame is always the agent step. setStep('agent') setAgentIndex(0) setProjectTabId(null) committingRef.current = false }, [open]) - useEffect(() => { - if (!open) return - // Keep keyboard focus on the dialog surface across steps. Choosing a row - // with the mouse focuses that row's button, and switching steps unmounts - // it; without this the next arrow key would have nothing listening. - dialogRef.current?.focus() - }, [open, step]) - const choice = AGENT_PROVIDER_CHOICES[agentIndex] ?? null const enabledProjects = model.projects.filter(project => project.anchorSessionId !== null) const highlightedProject = @@ -98,7 +97,7 @@ export function NewAgentInDialog({ open, workspace, onClose }: Props) { setAgentIndex(index) setStep('project') // Start on the project plain New Agent… would have used (the model owns - // that rule), so accepting both defaults never does worse than today. + // that rule), so accepting both defaults lands in the same project. setProjectTabId(model.initialTabId) } @@ -148,6 +147,13 @@ export function NewAgentInDialog({ open, workspace, onClose }: Props) { // Focus the surface itself, not the first row: the key handler lives // here, and a focused row button would also turn Enter into a native // click on whichever row happened to be first. + // + // No focus management is needed across steps. Clicking a row focuses + // that row's button and the step change unmounts it; Radix's + // FocusScope (inside DialogContent) moves focus back to this + // container when the focused node is removed, so the key handler + // keeps listening. Focus is the primitive's job + // (components/ui/README.md), and an extra effect here duplicated it. event.preventDefault() dialogRef.current?.focus() }} @@ -161,9 +167,24 @@ export function NewAgentInDialog({ open, workspace, onClose }: Props) { return } if (event.key === 'Enter') { - // preventDefault also cancels a focused button's native Enter-click, - // so a Tab-focused row cannot fire a second, different action. + // A FOCUSED FOOTER BUTTON OWNS ITS OWN ENTER — the rule + // components/ui/dialog-actions.tsx writes down. Everything below + // calls preventDefault, which also cancels the focused button's + // native Enter-click, so without this guard Tab to Cancel + Enter + // committed the highlighted project and SPAWNED an agent (#862 is + // the same bug in ProviderSwitchPickerModal, whose pattern this + // copied). Only the footer is exempt, not every button: the list + // rows are buttons too, and after Tab-to-row plus arrows the + // focused row and the highlighted row differ — Enter must act on + // the highlight, which is what the user is looking at. + if (event.target instanceof Element && event.target.closest('[data-slot="dialog-footer"]')) return event.preventDefault() + // Held Enter auto-repeats. Without this one long press would pick + // the agent and then commit the default project, starting a real + // agent process the user never chose a project for. + if (event.repeat) return + // preventDefault above also cancels a focused ROW's native click, so + // a Tab-focused row cannot fire a second, different action. if (step === 'agent') chooseAgent(agentIndex) else if (highlightedProject) commit(highlightedProject) return @@ -210,8 +231,13 @@ export function NewAgentInDialog({ open, workspace, onClose }: Props) { ) }) ) : model.projects.length === 0 ? ( + // The command needs an active tab, so an empty list can only mean + // the focused row is bound solely to projects that have since been + // closed (row bindings outlive their tab until #863 is fixed). + // Saying "no projects are open" would be false and leave the user + // stuck; name the control that fixes it instead.
- No projects are open. + None of this row’s projects are open. Change them with Row Projects…
) : ( model.projects.map(project => { @@ -256,13 +282,15 @@ export function NewAgentInDialog({ open, workspace, onClose }: Props) { ? '↑↓ choose · Enter next · Esc cancel' : '↑↓ choose · Enter create · ⌫ back · Esc cancel'} + {/* Ghost, small: the house footer shape (DialogActions renders Cancel + this way). Neither button is a primary action — the rows are. */}
{step === 'project' ? ( - ) : null} -
From 9e4414e352bd58eb149598606dc6a0ba2a40bc9c Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Thu, 10 Sep 2026 18:25:35 -0700 Subject: [PATCH 6/9] refactor(ui): share the focused-control Enter rule from DialogActions DialogActions already encoded "a focused button, link or textarea owns its own Enter", but list-style dialogs that handle Enter on DialogContent could not reach it and copied a footer-slot guard instead (New Agent In, Switch Provider). Exporting the rule as focusedControlOwnsEnter gives those dialogs, and the three more that #867 tracks, one definition to call. Behavior of DialogActions is unchanged; its Enter rule had no tests, so the confirm and Cancel-owns-Enter paths are pinned alongside the extraction. Refs #862 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SVRSHn7c7qnBoUQo1e2X4k --- .../ui/dialog-actions.renderer.test.tsx | 57 +++++++++++++++++++ .../src/components/ui/dialog-actions.tsx | 43 ++++++++++---- 2 files changed, 89 insertions(+), 11 deletions(-) create mode 100644 src/renderer/src/components/ui/dialog-actions.renderer.test.tsx diff --git a/src/renderer/src/components/ui/dialog-actions.renderer.test.tsx b/src/renderer/src/components/ui/dialog-actions.renderer.test.tsx new file mode 100644 index 000000000..9226ecd83 --- /dev/null +++ b/src/renderer/src/components/ui/dialog-actions.renderer.test.tsx @@ -0,0 +1,57 @@ +import { fireEvent, render, screen } from '@testing-library/react' +import { describe, expect, it, vi } from 'vitest' + +import { Dialog, DialogContent, DialogTitle } from './dialog' +import { DialogActions, focusedControlOwnsEnter } from './dialog-actions' + +// DialogActions' Enter rule is shared: list dialogs that handle Enter on +// DialogContent themselves (New Agent In, Switch Provider) call the exported +// `focusedControlOwnsEnter` rather than copying a guard (#862). These pin the +// rule where it lives, so a change to it cannot quietly break every consumer. + +function harness() { + const onConfirm = vi.fn() + const onCancel = vi.fn() + render( + + + Close agents + + + , + ) + return { onConfirm, onCancel } +} + +describe('DialogActions Enter ownership', () => { + it('confirms on an Enter that no control claims', () => { + const { onConfirm } = harness() + + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Enter' }) + + expect(onConfirm).toHaveBeenCalledOnce() + }) + + it('leaves Enter on a focused Cancel to Cancel, so it can never confirm', () => { + // The failure this prevents: preventDefault cancels Cancel's native click + // and the footer confirms instead — Enter-on-Cancel performing the + // destructive action. `true` = default not prevented. + const { onConfirm } = harness() + + expect(fireEvent.keyDown(screen.getByRole('button', { name: 'Cancel' }), { key: 'Enter' })).toBe(true) + + expect(onConfirm).not.toHaveBeenCalled() + }) +}) + +describe('focusedControlOwnsEnter', () => { + it('claims Enter for buttons, links and textareas only', () => { + expect(focusedControlOwnsEnter(document.createElement('button'))).toBe(true) + expect(focusedControlOwnsEnter(document.createElement('a'))).toBe(true) + expect(focusedControlOwnsEnter(document.createElement('textarea'))).toBe(true) + // The dialog surface and ordinary containers do not: Enter there is the + // dialog's to handle. + expect(focusedControlOwnsEnter(document.createElement('div'))).toBe(false) + expect(focusedControlOwnsEnter(null)).toBe(false) + }) +}) diff --git a/src/renderer/src/components/ui/dialog-actions.tsx b/src/renderer/src/components/ui/dialog-actions.tsx index 7b826019d..5bcbc1a93 100644 --- a/src/renderer/src/components/ui/dialog-actions.tsx +++ b/src/renderer/src/components/ui/dialog-actions.tsx @@ -67,6 +67,30 @@ export type DialogActionsProps = { children?: React.ReactNode } +/** + * Whether the element an Enter keydown landed on owns that Enter itself. + * + * A focused button or link activates on Enter (the browser synthesizes a + * click), and a textarea inserts a newline. Any dialog-level Enter handler that + * calls `preventDefault` suppresses those, so it must ask this first — or Tab to + * Cancel + Enter CONFIRMS, which for a destructive dialog means Enter-on-Cancel + * performs the destructive action. + * + * Exported (rather than living only inside DialogActions' listener) because + * list-style dialogs handle Enter on `DialogContent` themselves — New Agent In + * and Switch Provider did, and both committed the highlighted row when Cancel + * had focus (#862). One predicate keeps "which controls own Enter" in one place + * instead of a guard copied into every list dialog. + * + * Such dialogs must also keep their list rows out of the tab order: a + * Tab-focused row would own Enter here and diverge from the arrow-driven + * highlight (and Space would click it regardless of any Enter handling). + */ +export function focusedControlOwnsEnter(target: EventTarget | null): boolean { + if (!(target instanceof HTMLElement)) return false + return target.tagName === 'BUTTON' || target.tagName === 'A' || target.tagName === 'TEXTAREA' +} + export function DialogActions({ confirmLabel, onConfirm, @@ -97,17 +121,14 @@ export function DialogActions({ // Shift+Enter is newline everywhere in this app; a modifier means the // user is composing, not committing. if (event.shiftKey || event.metaKey || event.ctrlKey || event.altKey) return - const target = event.target as HTMLElement | null - // A textarea owns its own Enter. Without this the footer would commit - // the dialog while the user was trying to start a new line — the exact - // reason several dialogs previously hand-rolled `!shiftKey` checks. - if (target?.tagName === 'TEXTAREA') return - // A FOCUSED BUTTON OWNS ITS OWN ENTER. This is the important one: - // calling preventDefault below suppresses the synthesized click that - // Enter would have sent to the focused control, so without this guard - // tabbing to Cancel and pressing Enter would CONFIRM — and for a - // destructive dialog that means Enter-on-Cancel performs the deletion. - if (target?.tagName === 'BUTTON' || target?.tagName === 'A') return + // A FOCUSED BUTTON OWNS ITS OWN ENTER, and a textarea owns its newline. + // This is the important one: calling preventDefault below suppresses the + // synthesized click that Enter would have sent to the focused control, so + // without this guard tabbing to Cancel and pressing Enter would CONFIRM — + // and for a destructive dialog that means Enter-on-Cancel performs the + // deletion. The textarea half is the reason several dialogs previously + // hand-rolled `!shiftKey` checks. See `focusedControlOwnsEnter`. + if (focusedControlOwnsEnter(event.target)) return if (blocked) return event.preventDefault() onConfirm() From c6f778fcd03d3f06782e1e11db7d7fc5c9601590 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Thu, 10 Sep 2026 18:25:35 -0700 Subject: [PATCH 7/9] fix(provider-switch): keep Switch Provider rows out of the tab order A provider row could hold keyboard focus while the arrows moved the highlight elsewhere. Space is a native click on the focused button, so Tab to a row, ArrowDown, Space switched the agent to a provider other than the highlighted one. Rows are no longer tab stops: keyboard focus now rests only on the dialog surface, where arrows and Enter act on the highlight, or on the footer. The Enter guard uses the shared focusedControlOwnsEnter rule. Fixes #862 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SVRSHn7c7qnBoUQo1e2X4k --- ...roviderSwitchPickerModal.renderer.test.tsx | 12 ++++++++++ .../ui/ProviderSwitchPickerModal.tsx | 22 +++++++++++++------ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.renderer.test.tsx b/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.renderer.test.tsx index 740b2c6e6..ff2ff4398 100644 --- a/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.renderer.test.tsx +++ b/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.renderer.test.tsx @@ -79,6 +79,18 @@ describe('ProviderSwitchPickerModal', () => { expect(switchSessionProvider).not.toHaveBeenCalled() }) + it('never lets keyboard focus rest on a provider row (#862)', () => { + // Tab to a row, ArrowDown to move the highlight, Space: the browser clicks + // the FOCUSED row, so the switch went to a provider other than the + // highlighted one. happy-dom does not synthesize keyboard clicks, so the + // pin is the structure that makes the path impossible. + harness() + + const rows = document.querySelectorAll('[data-provider-switch-choice]') + expect(rows.length).toBeGreaterThan(0) + rows.forEach(row => expect(row).toHaveAttribute('tabindex', '-1')) + }) + it('supports keyboard choice and cancellation without starting an implicit switch', () => { const first = harness() fireEvent.keyDown(screen.getByRole('dialog'), { key: 'ArrowDown' }) diff --git a/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.tsx b/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.tsx index 427e2749b..038ca3936 100644 --- a/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.tsx +++ b/src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.tsx @@ -9,6 +9,7 @@ import { DialogHeader, DialogTitle, } from '@renderer/components/ui/dialog' +import { focusedControlOwnsEnter } from '@renderer/components/ui/dialog-actions' import { providerChoiceLabel, providerSwitchChoices, @@ -103,13 +104,13 @@ export function ProviderSwitchPickerModal({ return } if (event.key === 'Enter' && selected) { - // A focused footer button owns its own Enter (the rule - // components/ui/dialog-actions.tsx documents). preventDefault below - // also cancels that button's native click, so without this guard - // Tab to Cancel + Enter switched the agent to the highlighted - // provider instead of cancelling (#862). Rows are exempt from the - // exemption: Enter on the list means "the highlighted row". - if (event.target instanceof Element && event.target.closest('[data-slot="dialog-footer"]')) return + // A focused button owns its own Enter (focusedControlOwnsEnter — + // the rule components/ui/dialog-actions.tsx documents). + // preventDefault below also cancels that button's native click, so + // without this guard Tab to Cancel + Enter switched the agent to the + // highlighted provider instead of cancelling (#862). Rows are not + // tab stops (below), so the only focusable buttons are the footer's. + if (focusedControlOwnsEnter(event.target)) return event.preventDefault() choose(selected) } @@ -139,6 +140,13 @@ export function ProviderSwitchPickerModal({