diff --git a/docs/superpowers/plans/2026-07-28-dispatch-lane-removal.md b/docs/superpowers/plans/2026-07-28-dispatch-lane-removal.md new file mode 100644 index 00000000..fbe7e5c2 --- /dev/null +++ b/docs/superpowers/plans/2026-07-28-dispatch-lane-removal.md @@ -0,0 +1,290 @@ +# Dispatch Lane Removal Implementation Plan + +> **Status:** IMPLEMENTED and reviewed (PR #625). +> +> **Where the implementation differs from this plan:** +> +> - **The focus rule is not a plain clamp.** A lane removed BEFORE the focused +> one shifts indices down, so holding `focusedLane` constant would silently +> move focus to the next agent. It adjusts, then clamps. D5 described only the +> clamp half. +> - **`closeSession` had to change signature** to `Promise` so D2's +> "only splice if it actually closed" is expressible. Two orchestration call +> sites adapt locally to their `Promise` contract. +> - **`ratios` is not dropped wholesale.** Index 0 is the index-SIDEBAR +> fraction, not a lane boundary — only `ratios.slice(1)` are lane weights. D4 +> was wrong to copy `setTiledLaneCount` here: a removal can preserve the +> sidebar width and drop just the removed lane's weight, which a count +> *increase* cannot (it would have to invent a weight for the new lane). +> - **The destructive command's `when` tests liveness, not presence.** A lane +> can hold a set-but-dead id for one render; admitting on presence alone let +> the command run and do neither of the two things its title promises. +> +> **Known pre-existing bug this PR did NOT fix, deliberately:** an orchestrating +> agent whose close is declined by the user is told the child closed. +> `requestCloseConfirmation` resolves `false` rather than throwing, so +> `closeOrchestrationAgent`/`closeOrchestrationRun` report success +> unconditionally and `skippedSessionIds` stays empty. The `Promise` +> added here is the missing half of the fix, but threading it changes what a +> cross-process caller is told and deserves its own review rather than a +> ride-along. See the comment at the wrapper sites. + +**Goal:** Let a user remove a *specific* lane from Tiled Dispatch, instead of only being able to shrink the grid from the tail. + +--- + +## The problem + +Tiled Dispatch's lane count is a single number, and shrinking it always drops the **tail**: + +```ts +// actions/dispatch.ts — setTiledLaneCount +const lanes = next < tiled.lanes.length + ? tiled.lanes.slice(0, next) // <- always the last lanes + : buildAutoLanes(prev, next, tiled.lanes) +``` + +So with 7 lanes open and the agent in lane 3 finished, going 7 → 6 removes lane **7**. The user then has to re-select several lanes by hand to get back to the arrangement they wanted. + +**The obvious workaround does not work either.** Closing the agent in lane 3 does not shrink the grid: `clearTiledLaneSessions` sets that lane's `selectedSessionId` to undefined, and `buildAutoLanes`' auto-fill re-homes another agent into the now-empty lane. Count stays 7. + +**Net: there is currently no way to shrink the tiled grid at a position of the user's choosing.** That is the gap. + +--- + +## Two commands, not one + +The product default is *remove the lane and close the agent*. But a command that sometimes destroys a session and sometimes does not is exactly the kind of thing that surprises someone at speed, so the destructive and non-destructive behaviours are separate commands with separate names. + +| | Destructive (the default) | Non-destructive | +|---|---|---| +| id | `close-agent-remove-lane` | `remove-tiled-lane` | +| title | **Close Agent and Remove Lane** | **Remove Lane** | +| agent | closed via `workspace.closeSession` | keeps running, stays in the index | +| lane | spliced out, count −1 | spliced out, count −1 | + +### Why these names + +- **The destructive one leads with the destruction.** `Close` is already this repo's verb for ending a session (`Close Focused Session`, `Close Tab`, `Close Old Agents`), so a title that starts with it is legible at a glance. Putting `Remove Lane` first would bury the irreversible half. +- **`Remove Agent` was rejected outright.** It reads non-destructive and is not — the worst possible name for the default. +- **`Kill Lane` was rejected**: `kill` is reserved in this catalog for buried sessions (`Kill Buried Session…`). +- **The safe command gets the shorter title**, because it is the one a user runs casually. +- Both are imperative one-shot verbs per `docs/command-style.md` rule 4, and neither takes further input, so neither carries an ellipsis (rule 8). + +--- + +## Design decisions + +### D1 — One state action, two callers + +`removeTiledLane(laneIndex)` lands in `actions/dispatch.ts` beside `setTiledLaneCount`, which it mirrors. The destructive command is that action **plus** a `closeSession` call; it is not a second code path through the lane state. Two lane-splicing implementations would drift. + +### D2 — Order: close first, then splice + +`closeSession` is async and runs its own confirmation dialog for irreversible closes. Splicing the lane first would leave the grid already shrunk while the user is still deciding, and a cancelled confirm would leave the layout changed with the agent alive — the worst of both. So: await the close, and only splice if it actually happened. + +**This means `closeSession` must report whether it closed.** It currently returns `Promise`. Widening it to return a boolean is in scope; the alternative — re-reading state to infer whether the session survived — is a guess. + +### D3 — Refuse at the minimum lane count + +`when` requires `lanes.length > MIN_DISPATCH_TILES`. Removing the last lane would leave a tiled layout with nothing in it; the command for that is **Dispatch Mode** (exit tiled), and offering a lane-removal that silently becomes a mode-exit would be two different actions wearing one name. + +### D4 — Reset `ratios`, matching `setTiledLaneCount` + +Stored lane-boundary ratios are positional. Removing a lane invalidates them, so `ratios: undefined` and let the layout recompute — exactly what `setTiledLaneCount` already does on any count change. + +### D5 — Clamp `focusedLane` + +Removing the focused lane leaves `focusedLane` pointing past the end when it was the last one. Clamp to `lanes.length - 1`, the same clamp `setTiledLaneCount` applies. + +### D6 — Lane 0 is a real lane, and removing it has a visible consequence + +Lane 0 is not the index sidebar — the index is a separate column. Lane 0 is an ordinary agent lane that simply has no mini-list of its own, because it is selected from the full index (`TiledDispatchLayout.tsx`: `{laneIndex > 0 && }`). + +So removing lane 0 promotes lane 1 into position 0, and that lane **loses its own selector**. Mechanically fine, and no special-casing is warranted — but the command descriptions should not pretend the grid is homogeneous. + +### D7 — Surface is `dispatch` + +Both commands are meaningless outside Tiled Dispatch. `surface: 'dispatch'` per `docs/command-style.md` rules 10–11. + +--- + +## Files + +| File | Change | +|---|---| +| `src/renderer/src/workspace/hook/actions/dispatch.ts` | new `removeTiledLane(laneIndex)`; add to the returned object and its type | +| `src/renderer/src/workspace/hook/actions/pane.ts` | `closeSession` returns `Promise` | +| `src/renderer/src/workspace/hook/index.ts` | expose `removeTiledLane` | +| `src/renderer/src/features/workspace/commands/layoutCommands.ts` | the two commands | +| `src/renderer/src/features/command-palette/catalog.test.ts` | snapshot + counts 102 → 104 | +| `src/renderer/src/workspace/dispatch/tiledLaneRemoval.test.ts` | **new** — unit tests for the splice/clamp logic | + +--- + +## Tasks + +### Task 1: `removeTiledLane` state action + +- [ ] **Step 1:** In `actions/dispatch.ts`, beside `setTiledLaneCount`: + +```ts +/** + * Remove ONE lane by index, shrinking the grid by one. + * + * WHY this exists next to setTiledLaneCount rather than being expressible + * through it: that action only takes a COUNT, and shrinking by count always + * drops the tail (`lanes.slice(0, next)`). With seven lanes open and the + * finished agent in lane three, 7 -> 6 removes lane seven and leaves the user + * re-selecting the rest by hand. + * + * Closing the agent instead does not shrink anything either: the lane empties + * and buildAutoLanes' auto-fill re-homes another agent into it. So before this + * action there was no way at all to shrink the tiled grid at a chosen position. + */ +const removeTiledLane = useCallback( + (laneIndex: number) => { + setState(prev => { + const tiled = prev.dispatchMode?.tiled + if (!tiled) return prev + // Refuse below the floor. Emptying the layout is Dispatch Mode's job; + // a lane-removal that silently becomes a mode-exit is two actions + // sharing one name. + if (tiled.lanes.length <= MIN_DISPATCH_TILES) return prev + if (laneIndex < 0 || laneIndex >= tiled.lanes.length) return prev + const lanes = tiled.lanes.filter((_, i) => i !== laneIndex) + return { + ...prev, + dispatchMode: { + ...prev.dispatchMode!, + tiled: { + lanes, + // Same clamp setTiledLaneCount applies: removing the last lane + // leaves focusedLane past the end. + focusedLane: Math.min(tiled.focusedLane, lanes.length - 1), + // Ratios are positional, so removing a lane invalidates them. + ratios: undefined, + }, + }, + } + }) + }, + [setState], +) +``` + +- [ ] **Step 2:** Add `removeTiledLane: (laneIndex: number) => void` to the hook's return type and the returned object; expose it in `workspace/hook/index.ts`. + +- [ ] **Step 3:** `npx tsc -b --pretty false` → exit 0. + +- [ ] **Step 4:** Commit. + +### Task 2: `closeSession` reports whether it closed + +- [ ] **Step 1:** Widen `closeSession` in `actions/pane.ts` from `Promise` to `Promise` — `true` when the session was closed, `false` when it did not exist or the user cancelled the confirmation. Update the declared signature and every `return` in that function. + +- [ ] **Step 2:** Existing callers ignore the value, so no call-site changes are required. Verify with `tsc`. + +- [ ] **Step 3:** Commit. + +### Task 3: The two commands + +- [ ] **Step 1:** In `layoutCommands.ts`, after `tiled-dispatch`: + +```ts +{ + id: 'remove-tiled-lane', + category: 'layout-dispatch', + surface: 'dispatch', + title: 'Remove Lane', + description: '**What it does:** Removes the **focused lane** from Tiled Dispatch and shrinks the grid by one. The agent keeps running and stays in the index.\n\n**Use when:** You are done watching one agent but want to keep the others exactly where they are.\n\n**Notes:** Changing the tile count instead always drops the LAST lane. Removing the leftmost lane promotes the next one into its place, where it is selected from the full index rather than its own compact selector.', + keywords: ['remove', 'lane', 'tile', 'tiled dispatch', 'shrink', 'close lane'], + when: ({ workspace }) => { + const tiled = workspace.state.dispatchMode?.tiled + return Boolean(tiled && tiled.lanes.length > MIN_DISPATCH_TILES) + }, + run: ({ workspace }) => { + const tiled = workspace.state.dispatchMode?.tiled + if (!tiled) return + workspace.removeTiledLane(tiled.focusedLane) + }, +}, +{ + id: 'close-agent-remove-lane', + category: 'layout-dispatch', + surface: 'dispatch', + title: 'Close Agent and Remove Lane', + description: '**What it does:** Closes the agent in the **focused lane**, then removes that lane and shrinks the grid by one.\n\n**Use when:** An agent has finished and you want it gone along with its slot.\n\n**Notes:** This ends the session. Use **Remove Lane** to reclaim the slot while leaving the agent running. Irreversible closes still confirm first, and cancelling leaves the grid untouched.', + keywords: ['close', 'agent', 'lane', 'tile', 'tiled dispatch', 'shrink', 'done'], + when: ({ workspace }) => { + const tiled = workspace.state.dispatchMode?.tiled + if (!tiled || tiled.lanes.length <= MIN_DISPATCH_TILES) return false + return Boolean(tiled.lanes[tiled.focusedLane]?.selectedSessionId) + }, + run: async ({ workspace }) => { + const tiled = workspace.state.dispatchMode?.tiled + if (!tiled) return + const laneIndex = tiled.focusedLane + const sessionId = tiled.lanes[laneIndex]?.selectedSessionId + if (!sessionId) return + // Close FIRST. closeSession runs its own confirmation for irreversible + // closes; splicing before it resolves would shrink the grid while the + // user was still deciding, and a cancelled confirm would leave the + // layout changed with the agent still alive. + const closed = await workspace.closeSession(sessionId) + if (closed) workspace.removeTiledLane(laneIndex) + }, +}, +``` + +- [ ] **Step 2:** Import `MIN_DISPATCH_TILES` from `tiledDispatchSelectors`. + +- [ ] **Step 3:** Update `catalog.test.ts` — ordered snapshot (both ids after `tiled-dispatch`), `toHaveLength(104)`, the two test names, and **both** arithmetic assertions. Note the second one: its subtracted term is the count of approved additions and its expected value is the pre-governance baseline of 102, so raise the **subtrahend** to 7, never the right-hand side. + +- [ ] **Step 4:** `npm run check:keybindings` → OK. + +- [ ] **Step 5:** Commit. + +### Task 4: Tests + +Tests are welcome in this repo (`docs/testing/standard.md`); the splice/clamp logic is pure state and worth pinning. + +- [ ] **Step 1:** Extract the reducer body into a pure exported helper in `tiledDispatchSelectors.ts` so it can be tested without a hook: + +```ts +export function removeLaneFromTiled( + tiled: TiledDispatchState, + laneIndex: number, +): TiledDispatchState | null // null = refused +``` + +Have `removeTiledLane` call it. + +- [ ] **Step 2:** `tiledLaneRemoval.test.ts` covering: + - removing a middle lane keeps the lanes either side, in order + - removing the focused lane clamps `focusedLane` into range + - removing a lane before the focused one keeps the same lane focused + - refuses at `MIN_DISPATCH_TILES` + - refuses an out-of-range index + - always clears `ratios` + +- [ ] **Step 3:** `NODE_ENV=test npx vitest run` → all green. + +- [ ] **Step 4:** Commit. + +### Task 5: Full gate + +- [ ] `npx tsc -b --pretty false` → exit 0 +- [ ] `NODE_ENV=test npx vitest run` → green +- [ ] `npm run check:keybindings` → OK +- [ ] `npm run test:contract` → satisfied + +--- + +## Self-review + +**Covers the reported problem:** yes — the user can now remove lane 3 specifically, in both the keep-the-agent and close-the-agent flavours. + +**Type consistency:** `removeTiledLane(laneIndex: number) => void` produced in Task 1, consumed in Task 3. `closeSession` widened in Task 2, consumed in Task 3. `removeLaneFromTiled` produced in Task 4 Step 1, consumed by Task 1's action. + +**Known limitation, recorded not fixed:** both commands act on the **focused** lane only. Removing an arbitrary lane by pointer — a small "×" on each lane header — is the natural mouse-first follow-up and is deliberately out of scope here. diff --git a/src/renderer/src/features/command-palette/catalog.test.ts b/src/renderer/src/features/command-palette/catalog.test.ts index 5f6ce15a..f69be885 100644 --- a/src/renderer/src/features/command-palette/catalog.test.ts +++ b/src/renderer/src/features/command-palette/catalog.test.ts @@ -9,9 +9,9 @@ import type { CommandDef } from '@renderer/features/command-palette/types' // Phase 0 of the command-governance plan (docs/superpowers/plans/ // 2026-07-23-command-surface-audit.md): CHARACTERIZE THE CURRENT CATALOG. // -// This file pinned the exact 102-id before-state, and now pins the 102-id -// after-state: five durable preferences retired to Settings, five approved -// additions. Keeping ONE snapshot that moved — rather +// This file pinned the exact 102-id before-state, and now pins the 104-id +// after-state: five durable preferences retired to Settings, seven approved +// additions (102 - 5 + 7 = 104). 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. @@ -67,10 +67,13 @@ const BASELINE_COMMAND_IDS: readonly string[] = [ 'clear-composer', 'undo-clear-composer', 'send-composer', - // layoutCommands (8, was 9: toggle-status-mode retired) + // layoutCommands (10, was 9: toggle-status-mode retired, two lane-removal + // commands added) 'dispatch-mode', 'global-dispatch', 'tiled-dispatch', + 'remove-tiled-lane', + 'close-agent-remove-lane', 'normalize-layout', 'hard-normalize-layout', 'rotate-layout', @@ -173,21 +176,22 @@ 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 102 governed commands in registration order', () => { + it('contains exactly the 104 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 102 commands', () => { + it('has exactly 104 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. // // 102 baseline → 98 after governance (5 retirements, 1 addition) → 99 with - // `open-keyboard-shortcuts` → 102 with the three composer commands. Each + // `open-keyboard-shortcuts` → 102 with the three composer commands → 104 + // with the two lane-removal commands. Each // step of that arithmetic was a deliberate edit to this line, which is the // entire point of pinning it. - expect(builtInCommandCatalog).toHaveLength(102) + expect(builtInCommandCatalog).toHaveLength(104) }) it('reports no structural defects', () => { @@ -204,7 +208,7 @@ describe('built-in command catalog — baseline characterization', () => { }) describe('generated per-provider split commands', () => { - // The arithmetic is 98 literal ids + 4 generated = 102. If a provider + // The arithmetic is 100 literal ids + 4 generated = 104. If a provider // is ever added to AGENT_PROVIDER_KINDS, this invariant is what tells the // author that the catalog count moved for a legitimate reason, and forces the // baseline snapshot above to be updated deliberately. @@ -218,10 +222,10 @@ describe('generated per-provider split commands', () => { }) it('accounts for the difference between literal and total command count', () => { - // 102 total - 4 generated = 98 literal `id:` fields across the command + // 104 total - 4 generated = 100 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 five additions. - expect(builtInCommandCatalog.length - nonDefaultProviders.length * 2).toBe(98) + // the five retirements, then back up by the seven additions. + expect(builtInCommandCatalog.length - nonDefaultProviders.length * 2).toBe(100) }) it('emits both directions for every non-default provider', () => { @@ -320,7 +324,7 @@ describe('governance targets', () => { }) it('lands on the arithmetic the plan predicted', () => { - // 102 baseline - 5 retirements + 5 additions = 102, checked against the + // 102 baseline - 5 retirements + 7 additions = 104, checked against the // real catalog rather than trusted as prose. // // The subtracted term is the count of APPROVED ADDITIONS and the expected @@ -332,9 +336,10 @@ describe('governance targets', () => { // Additions so far: `open-command-palette` (governance: the palette could // not be rebound because it had no command id), `open-keyboard-shortcuts`, // and the three composer commands (`clear-composer`, - // `undo-clear-composer`, `send-composer`). - expect(builtInCommandCatalog.length + RETIRED_COMMAND_IDS.length - 5).toBe(102) - expect(builtInCommandCatalog).toHaveLength(102) + // `undo-clear-composer`, `send-composer`) and the two lane-removal + // commands (`remove-tiled-lane`, `close-agent-remove-lane`). + expect(builtInCommandCatalog.length + RETIRED_COMMAND_IDS.length - 7).toBe(102) + expect(builtInCommandCatalog).toHaveLength(104) }) }) diff --git a/src/renderer/src/features/workspace/commands/layoutCommands.ts b/src/renderer/src/features/workspace/commands/layoutCommands.ts index 48b2d2dd..88880246 100644 --- a/src/renderer/src/features/workspace/commands/layoutCommands.ts +++ b/src/renderer/src/features/workspace/commands/layoutCommands.ts @@ -1,5 +1,6 @@ import type { CommandDef } from '@renderer/features/command-palette/types' import { status, toggle, value } from '@renderer/features/command-palette/commandState' +import { MIN_DISPATCH_TILES } from '@renderer/workspace/dispatch/tiledDispatchSelectors' export const layoutCommands: CommandDef[] = [ { @@ -57,6 +58,71 @@ export const layoutCommands: CommandDef[] = [ keywords: ['tiled dispatch', 'multi agent', 'lanes', 'split dispatch', 'cockpit', 'parallel agents', 'grid of agents'], run: ({ ui }) => ui.openTiledDispatchPrompt(), }, + { + // WHY these two exist at all: Tiled Dispatch's size is a single count, and + // shrinking by count always drops the TAIL (`lanes.slice(0, next)`). With + // seven lanes open and the finished agent in lane three, 7 -> 6 removes + // lane seven. Closing that agent instead does not shrink anything either — + // the lane empties and auto-fill re-homes another agent into it. So there + // was no way to reclaim a slot at a position of the user's choosing. + // + // WHY two commands rather than one with a flag: the default is destructive, + // and a command that sometimes ends a session and sometimes does not is + // the kind of thing that surprises someone moving fast. The titles carry + // the difference — `Close` is this catalog's established verb for ending a + // session, so the destructive one leads with it. + id: 'remove-tiled-lane', + category: 'layout-dispatch', + surface: 'dispatch', + title: 'Remove Lane', + description: '**What it does:** Removes the **focused lane** from Tiled Dispatch, shrinking the layout by one lane. The agent keeps running and stays in the index.\n\n**Use when:** You are done watching one agent but want the others to stay exactly where they are.\n\n**Notes:** Changing the tile count instead always drops the LAST lane. Removing the leftmost lane promotes the next one into its place, where it is selected from the full index rather than its own compact selector.', + keywords: ['remove', 'lane', 'tile', 'tiled dispatch', 'shrink', 'slot'], + when: ({ workspace }) => { + const tiled = workspace.state.dispatchMode?.tiled + return Boolean(tiled && tiled.lanes.length > MIN_DISPATCH_TILES) + }, + run: ({ workspace }) => { + const tiled = workspace.state.dispatchMode?.tiled + if (!tiled) return + workspace.removeTiledLane(tiled.focusedLane) + }, + }, + { + id: 'close-agent-remove-lane', + category: 'layout-dispatch', + surface: 'dispatch', + title: 'Close Agent and Remove Lane', + description: '**What it does:** Closes the agent in the **focused lane**, then removes that lane, shrinking the layout by one.\n\n**Use when:** An agent has finished and you want it gone along with its slot.\n\n**Notes:** This ends the session. Use **Remove Lane** to reclaim the slot while leaving the agent running. Irreversible closes still confirm first, and declining leaves the layout untouched.', + keywords: ['close', 'agent', 'remove agent', 'lane', 'tile', 'tiled dispatch', 'shrink', 'finished', 'done'], + when: ({ workspace }) => { + const tiled = workspace.state.dispatchMode?.tiled + if (!tiled || tiled.lanes.length <= MIN_DISPATCH_TILES) return false + // An empty lane has no agent to close, so this collapses to Remove Lane — + // admission has to agree with what the command will do. + // + // Liveness, not mere presence: a lane can hold a set-but-dead id for the + // render between a session disappearing (killed from Agent Activity, tab + // closed) and the layout's heal effect clearing it. Admitting on presence + // alone let the command run, find nothing to close, and silently do + // neither of the two things its title promises. + const sessionId = tiled.lanes[tiled.focusedLane]?.selectedSessionId + return Boolean(sessionId && workspace.state.sessions[sessionId]) + }, + run: async ({ workspace }) => { + const tiled = workspace.state.dispatchMode?.tiled + if (!tiled) return + const laneIndex = tiled.focusedLane + const sessionId = tiled.lanes[laneIndex]?.selectedSessionId + if (!sessionId) return + // Close FIRST, and only splice if it actually happened. closeSession runs + // its own confirmation for irreversible closes; splicing before it + // resolves would shrink the grid while the user was still deciding, and + // a declined confirm would leave the layout changed with the agent alive + // — the worst of both outcomes. + const closed = await workspace.closeSession(sessionId) + if (closed) workspace.removeTiledLane(laneIndex) + }, + }, // REMOVED: the 'toggle-dispatch-terminal' command, then its replacement // `settings.dispatchProjectTerminal`, and now the feature itself. The // opt-in auto-created companion terminal and its dedicated Dispatch side diff --git a/src/renderer/src/workspace/dispatch/tiledDispatchSelectors.ts b/src/renderer/src/workspace/dispatch/tiledDispatchSelectors.ts index ccbe7d86..35042417 100644 --- a/src/renderer/src/workspace/dispatch/tiledDispatchSelectors.ts +++ b/src/renderer/src/workspace/dispatch/tiledDispatchSelectors.ts @@ -1,4 +1,10 @@ -import type { DispatchLane, DispatchModeState, SessionId, WorkspaceState } from '@renderer/workspace/types' +import type { + DispatchLane, + DispatchModeState, + SessionId, + TiledDispatchState, + WorkspaceState, +} from '@renderer/workspace/types' import { buildVisibleDispatchRows } from '@renderer/workspace/dispatch/dispatchSelectors' // ============================================================================ @@ -195,3 +201,76 @@ export function buildAutoLanes( // through keepTiledLaneSessions so stale lane ids do not survive to the next // launch; render-time healing remains the user-facing repair for scope changes // and temporarily empty lanes. + +/** + * Remove ONE lane by index. Returns null when the removal is refused, so the + * caller can leave state untouched rather than writing back an identical object. + * + * WHY this exists at all, given `setTiledLaneCount` already resizes the grid: + * that action takes only a COUNT, and shrinking by count always drops the tail + * (`lanes.slice(0, next)`). With seven lanes open and the finished agent in + * lane three, 7 -> 6 removes lane SEVEN and leaves the user re-selecting the + * rest by hand. + * + * Closing the agent instead does not shrink anything either: the lane empties + * and `buildAutoLanes`' auto-fill re-homes another agent into it, so the count + * stays put. Before this, there was no way to shrink the tiled grid at a + * position of the user's choosing. + * + * WHY it is a pure function rather than living inside the reducer: the + * splice/clamp/ratio rules are the whole behaviour, and they are worth testing + * without standing up a hook. + */ +export function removeLaneFromTiled( + tiled: TiledDispatchState, + laneIndex: number, +): TiledDispatchState | null { + // Refuse at the floor. Emptying the layout is Dispatch Mode's job; a + // lane-removal that silently became a mode-exit would be two different + // actions sharing one name. + if (tiled.lanes.length <= MIN_DISPATCH_TILES) return null + if (!Number.isInteger(laneIndex)) return null + if (laneIndex < 0 || laneIndex >= tiled.lanes.length) return null + + const lanes = tiled.lanes.filter((_, i) => i !== laneIndex) + return { + lanes, + // Same clamp `setTiledLaneCount` applies: removing the last lane would + // otherwise leave focusedLane pointing past the end. Note a lane removed + // BEFORE the focused one shifts it down by one, which Math.min does not + // do — so adjust explicitly rather than only clamping. + focusedLane: Math.min( + laneIndex < tiled.focusedLane ? tiled.focusedLane - 1 : tiled.focusedLane, + lanes.length - 1, + ), + // `ratios` is NOT a uniform array of lane boundaries: index 0 is the + // INDEX-SIDEBAR fraction (TiledDispatchLayout reads `ratios?.[0]`), and + // only `ratios.slice(1)` are lane weights. Dropping the whole array — what + // setTiledLaneCount does — therefore also snaps the sidebar back to its + // default, undoing a width the user deliberately dragged and never asked + // to change. + // + // A count *increase* has no honest answer (there is a new lane with no + // weight to invent), which is why setTiledLaneCount resets wholesale. A + // removal does: keep the sidebar fraction, drop the removed lane's weight, + // and let normalizedLaneWeights re-normalize what is left. + ratios: removeLaneWeight(tiled.ratios, laneIndex), + } +} + +/** + * Drop one lane's weight from a `ratios` array while preserving index 0, the + * index-sidebar fraction. Returns undefined when there is nothing stored, so + * the layout falls back to even distribution exactly as before. + */ +function removeLaneWeight( + ratios: number[] | undefined, + laneIndex: number, +): number[] | undefined { + if (!ratios || ratios.length === 0) return undefined + const [indexFraction, ...laneWeights] = ratios + // A ratios array written before this lane existed simply has no weight to + // drop; keeping the sidebar fraction is still the right call. + if (laneIndex >= laneWeights.length) return [indexFraction] + return [indexFraction, ...laneWeights.filter((_, i) => i !== laneIndex)] +} diff --git a/src/renderer/src/workspace/dispatch/tiledLaneRemoval.test.ts b/src/renderer/src/workspace/dispatch/tiledLaneRemoval.test.ts new file mode 100644 index 00000000..b8c221c8 --- /dev/null +++ b/src/renderer/src/workspace/dispatch/tiledLaneRemoval.test.ts @@ -0,0 +1,133 @@ +import { describe, expect, it } from 'vitest' + +import { + MIN_DISPATCH_TILES, + removeLaneFromTiled, +} from '@renderer/workspace/dispatch/tiledDispatchSelectors' +import type { SessionId, TiledDispatchState } from '@renderer/workspace/types' + +// removeLaneFromTiled is the whole behaviour of the two lane-removal commands. +// It is pinned here rather than through the hook because the interesting part +// is pure: which lane survives, where focus lands, and when the removal is +// refused. Driving it through React would test the wiring, not the rules. + +const lane = (id: string): { selectedSessionId: SessionId } => ({ + selectedSessionId: id as SessionId, +}) + +const tiled = ( + ids: string[], + focusedLane: number, + ratios?: number[], +): TiledDispatchState => ({ + lanes: ids.map(lane), + focusedLane, + ...(ratios ? { ratios } : {}), +}) + +const idsOf = (state: TiledDispatchState | null): (string | undefined)[] => + (state?.lanes ?? []).map(l => l.selectedSessionId) + +describe('removeLaneFromTiled', () => { + it('removes the lane at the given index and keeps the rest in order', () => { + // The bug this whole feature exists for: shrinking by COUNT drops the tail, + // so the finished agent in the middle survives and everything after it + // shifts. Removing by index has to leave both neighbours untouched. + const next = removeLaneFromTiled(tiled(['a', 'b', 'c', 'd'], 0), 1) + expect(idsOf(next)).toEqual(['a', 'c', 'd']) + }) + + it('keeps the same lane focused when an earlier lane is removed', () => { + // Indices shift down by one, so holding focusedLane constant would silently + // move focus to the NEXT agent. The user removed some other lane; the lane + // they were watching must stay the lane they are watching. + const next = removeLaneFromTiled(tiled(['a', 'b', 'c', 'd'], 2), 0) + expect(idsOf(next)).toEqual(['b', 'c', 'd']) + expect(next?.focusedLane).toBe(1) + expect(next?.lanes[next.focusedLane]?.selectedSessionId).toBe('c') + }) + + it('leaves focus alone when a later lane is removed', () => { + const next = removeLaneFromTiled(tiled(['a', 'b', 'c', 'd'], 1), 3) + // Asserting the lane list too, not just the index: without it this passes + // even if the wrong lane were spliced, since index 1 would still be 'b'. + expect(idsOf(next)).toEqual(['a', 'b', 'c']) + expect(next?.focusedLane).toBe(1) + expect(next?.lanes[next.focusedLane]?.selectedSessionId).toBe('b') + }) + + it('keeps the index when the focused lane itself is removed mid-list', () => { + // The arm where NEITHER the -1 adjust nor the clamp does anything, and the + // most common real invocation (Close Agent and Remove Lane on a middle + // lane). Focus stays put and now shows the ex-successor, which is the + // cursor-trails-deletion convention the rest of Dispatch already follows. + // A regression swapping `<` for `<=` in the adjust would leave every other + // test in this file green and break exactly this case. + const next = removeLaneFromTiled(tiled(['a', 'b', 'c'], 1), 1) + expect(idsOf(next)).toEqual(['a', 'c']) + expect(next?.focusedLane).toBe(1) + expect(next?.lanes[next.focusedLane]?.selectedSessionId).toBe('c') + }) + + it('keeps focus on lane 0 when lane 0 is removed', () => { + // Lane 0 is the one selected from the full index rather than its own + // mini-list, so removing it promotes lane 1 into that role. Focus must not + // drift off the leftmost lane in the process. + const next = removeLaneFromTiled(tiled(['a', 'b', 'c'], 0), 0) + expect(idsOf(next)).toEqual(['b', 'c']) + expect(next?.focusedLane).toBe(0) + expect(next?.lanes[0]?.selectedSessionId).toBe('b') + }) + + it('clamps focus into range when the last lane was the focused one', () => { + // Without the clamp, focusedLane points one past the end and the layout + // renders no focused lane at all. + const next = removeLaneFromTiled(tiled(['a', 'b', 'c'], 2), 2) + expect(idsOf(next)).toEqual(['a', 'b']) + expect(next?.focusedLane).toBe(1) + }) + + it('drops the removed lane weight but keeps the index-sidebar fraction', () => { + // ratios[0] is the INDEX-SIDEBAR fraction, not a lane boundary — only + // ratios.slice(1) are lane weights. Dropping the array wholesale would + // snap a deliberately-dragged sidebar back to its default, which is an + // unrelated setting the user never asked to change. + // + // The surviving weights must number exactly lanes.length, or + // normalizedLaneWeights discards them and falls back to even distribution. + const next = removeLaneFromTiled(tiled(['a', 'b', 'c'], 0, [0.25, 0.1, 0.6, 0.3]), 1) + expect(next?.ratios).toEqual([0.25, 0.1, 0.3]) + expect(next?.ratios?.length).toBe((next?.lanes.length ?? 0) + 1) + }) + + it('leaves ratios undefined when none were stored', () => { + const next = removeLaneFromTiled(tiled(['a', 'b', 'c'], 0), 1) + expect(next?.ratios).toBeUndefined() + }) + + it('refuses at the minimum lane count', () => { + // Emptying the layout is Dispatch Mode's job. Returning null lets the + // caller hand back the previous state untouched instead of writing an + // identical object. + const atFloor = tiled(Array.from({ length: MIN_DISPATCH_TILES }, (_, i) => `a${i}`), 0) + expect(removeLaneFromTiled(atFloor, 0)).toBeNull() + }) + + it('refuses an out-of-range or non-integer index', () => { + // A stale keybind or a command firing against a since-shrunk grid must be + // inert rather than throwing or silently removing the wrong lane. + const state = tiled(['a', 'b', 'c'], 0) + expect(removeLaneFromTiled(state, -1)).toBeNull() + expect(removeLaneFromTiled(state, 3)).toBeNull() + expect(removeLaneFromTiled(state, 1.5)).toBeNull() + }) + + it('does not mutate the input state', () => { + // The reducer spreads this result into workspace state; a mutated input + // would corrupt the snapshot the caller compared against. + const state = tiled(['a', 'b', 'c'], 1) + removeLaneFromTiled(state, 0) + expect(idsOf(state)).toEqual(['a', 'b', 'c']) + expect(state.focusedLane).toBe(1) + }) +}) diff --git a/src/renderer/src/workspace/hook/actions/dispatch.ts b/src/renderer/src/workspace/hook/actions/dispatch.ts index bc910296..14d6ecbf 100644 --- a/src/renderer/src/workspace/hook/actions/dispatch.ts +++ b/src/renderer/src/workspace/hook/actions/dispatch.ts @@ -7,6 +7,7 @@ import { buildAutoLanes, clampTileCount, dispatchFocusedSessionId, + removeLaneFromTiled, } from '@renderer/workspace/dispatch/tiledDispatchSelectors' import type { WorkspaceSetState, @@ -36,6 +37,7 @@ export function useDispatchActions( exitTiledDispatch: () => void setTiledLaneSession: (laneIndex: number, sessionId: SessionId) => void setTiledLaneCount: (count: number) => void + removeTiledLane: (laneIndex: number) => void setTiledFocusedLane: (laneIndex: number) => void setTiledRatios: (ratios: number[]) => void } { @@ -203,6 +205,28 @@ export function useDispatchActions( [setState], ) + /** + * Remove ONE lane, shrinking the grid by one. + * + * The splice/clamp/ratio rules live in `removeLaneFromTiled` so they can be + * tested as a pure function; this is only the state wiring. A null return + * means the removal was refused (at the lane floor, or a bad index), in + * which case we hand back `prev` untouched rather than writing an identical + * object and forcing a re-render. + */ + const removeTiledLane = useCallback( + (laneIndex: number) => { + setState(prev => { + const tiled = prev.dispatchMode?.tiled + if (!tiled) return prev + const next = removeLaneFromTiled(tiled, laneIndex) + if (!next) return prev + return { ...prev, dispatchMode: { ...prev.dispatchMode!, tiled: next } } + }) + }, + [setState], + ) + // Move keyboard-selection focus between lanes. Clamped. Must never touch // any lane's selection — that's what keeps lanes independent. const setTiledFocusedLane = useCallback( @@ -324,6 +348,7 @@ export function useDispatchActions( exitTiledDispatch, setTiledLaneSession, setTiledLaneCount, + removeTiledLane, setTiledFocusedLane, setTiledRatios, } diff --git a/src/renderer/src/workspace/hook/actions/pane.ts b/src/renderer/src/workspace/hook/actions/pane.ts index 25661092..c8be9344 100644 --- a/src/renderer/src/workspace/hook/actions/pane.ts +++ b/src/renderer/src/workspace/hook/actions/pane.ts @@ -310,7 +310,12 @@ export function usePaneActions( attachAllDetachedForTab: (tabId: string) => Promise detachFocusedToDispatch: () => void closeFocused: () => Promise - closeSession: (targetId: SessionId, options?: CloseSessionOptions) => Promise + /** Resolves true when the session was actually closed, false when it did + * not exist or the user declined the confirmation. Callers that need to + * follow a close with a dependent mutation (e.g. shrinking a Dispatch lane) + * must branch on this rather than assume success — a cancelled confirm + * would otherwise leave the layout changed with the agent still alive. */ + closeSession: (targetId: SessionId, options?: CloseSessionOptions) => Promise requestBuryFocused: () => void buryFocused: (note?: string, targetSessionId?: SessionId) => void reviveBuried: (buriedId: string) => Promise @@ -320,7 +325,7 @@ export function usePaneActions( navigate: (direction: 'left' | 'right' | 'up' | 'down') => void } { const closeSessionRef = useRef< - ((targetId: SessionId, options?: CloseSessionOptions) => Promise) | null + ((targetId: SessionId, options?: CloseSessionOptions) => Promise) | null >(null) // Spawns a new session in the parent pane's cwd, inserts a new @@ -1433,7 +1438,7 @@ export function usePaneActions( !initial.tabs.some(t => collectLeaves(t.root).includes(targetId)) && !initial.detachedSessions[targetId] ) { - return + return false } // CONFIRMATION GATE. This path was previously ungated entirely, which @@ -1451,7 +1456,7 @@ export function usePaneActions( }) if (!gate.ok) { if (gate.reason === 'changed') showToast(CLOSE_CHANGED_TOAST) - return + return false } } @@ -1463,7 +1468,7 @@ export function usePaneActions( const owningTab = snapshot.tabs.find(t => collectLeaves(t.root).includes(targetId)) const sessionMeta = snapshot.sessions[targetId] const detached = snapshot.detachedSessions[targetId] - if (!owningTab && !detached) return + if (!owningTab && !detached) return false // Linked agents are lifecycle-bound to their parent — close // any session that named `targetId` as its linkedParentId @@ -1499,9 +1504,9 @@ export function usePaneActions( const kindLabel = sessionMeta?.kind ?? DEFAULT_PROVIDER const cwdBase = sessionMeta?.cwd.split('/').filter(Boolean).pop() ?? sessionMeta?.cwd ?? 'session' showToast(`Closed detached ${kindLabel} session (${cwdBase})`) - return + return true } - if (!owningTab) return + if (!owningTab) return false // Same two-case undo capture as closeFocused: pane-in-split // vs. last-pane-in-tab. Keeps ⌘⇧T working for modal-driven @@ -1608,6 +1613,10 @@ export function usePaneActions( dispatchMode: dispatchModeAfterSessionRemoval(prev, next, targetId), } }) + // Reached only after the pane/tab close actually committed. Every + // earlier exit returns false, so a caller can distinguish "closed" from + // "declined at the confirmation" or "session was already gone". + return true }, [ closeLinkedChildren, diff --git a/src/renderer/src/workspace/hook/actions/paneRecoveryOwnership.renderer.test.tsx b/src/renderer/src/workspace/hook/actions/paneRecoveryOwnership.renderer.test.tsx index 7e1fadf3..349bff3a 100644 --- a/src/renderer/src/workspace/hook/actions/paneRecoveryOwnership.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/actions/paneRecoveryOwnership.renderer.test.tsx @@ -218,7 +218,7 @@ describe('pane recovery ownership', () => { // ceremony — it is the assertion that the dialog names BOTH sessions. // Before the gate counted detached children, this close reported one target // and silently took two. - let closing: Promise | undefined + let closing: Promise | undefined await act(async () => { closing = harness.result.current.closeSession(paneId) await Promise.resolve() diff --git a/src/renderer/src/workspace/hook/index.ts b/src/renderer/src/workspace/hook/index.ts index f861fef7..94470c92 100644 --- a/src/renderer/src/workspace/hook/index.ts +++ b/src/renderer/src/workspace/hook/index.ts @@ -412,7 +412,25 @@ export function useWorkspace( state: snapshot, parentSessionId: request.parentSessionId, sessionId: request.sessionId, - closeSession: closeOrchestrationSessionRef.current, + // Adapted to the orchestration contract's Promise. + // + // NOT because the signal is useless — it is not. `OrchestrationClose- + // Result` carries `skippedSessionIds` for exactly this, and + // closeOrchestrationRun's own comment claims "declining it throws, + // which the catch below turns into a skip" — which is false: + // requestCloseConfirmation RESOLVES false, it never rejects. So an + // orchestrating agent that has its close declined by the user is + // currently told the child closed. That is a real pre-existing bug + // and this boolean is the missing half of its fix. + // + // It is deliberately NOT fixed here: changing what an orchestrating + // agent is told about a close is a cross-process behaviour change + // that deserves its own review, not a ride-along in a + // Dispatch-lane PR. Filed as follow-up work; do not "tidy" this + // comment away without doing it. + closeSession: async (id, opts) => { + await closeOrchestrationSessionRef.current(id, opts) + }, }) await window.api.resolveOrchestrationRequest({ requestId: request.requestId, @@ -487,7 +505,10 @@ export function useWorkspace( state: snapshot, parentSessionId: request.parentSessionId, runId: request.runId, - closeSession: closeOrchestrationSessionRef.current, + // Same adaptation as the single-agent close above. + closeSession: async (id, opts) => { + await closeOrchestrationSessionRef.current(id, opts) + }, }) await window.api.resolveOrchestrationRequest({ requestId: request.requestId, @@ -963,6 +984,7 @@ export function useWorkspace( exitTiledDispatch: dispatchActions.exitTiledDispatch, setTiledLaneSession: dispatchActions.setTiledLaneSession, setTiledLaneCount: dispatchActions.setTiledLaneCount, + removeTiledLane: dispatchActions.removeTiledLane, setTiledFocusedLane: dispatchActions.setTiledFocusedLane, setTiledRatios: dispatchActions.setTiledRatios, }