From 493d2636dad8b64abbb72b1ee91afbf7bc5124f1 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 28 Jul 2026 17:22:35 +0200 Subject: [PATCH 1/2] feat(dispatch): remove a chosen lane from Tiled Dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 and leaves the user re-selecting the rest by hand. Closing that 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. There was no way at all to reclaim a slot at a position of the user's choosing. 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: Close Agent and Remove Lane — closes the agent, then removes its lane Remove Lane — lane goes, agent keeps running The titles carry the difference. `Close` is this catalog's established verb for ending a session (Close Focused Session, Close Tab, Close Old Agents), so the destructive one leads with it; putting "Remove Lane" first would bury the irreversible half. "Remove Agent" was rejected outright — it reads non-destructive and is not. The destructive command closes FIRST and only splices if the close actually happened. closeSession runs its own confirmation for irreversible closes, so splicing first would shrink the grid while the user was still deciding, and a declined confirm would leave the layout changed with the agent alive. That required closeSession to report whether it closed, so it now returns Promise; the two orchestration call sites adapt to their Promise contract locally rather than widening a cross-process type for one caller. Lane 0 is a real agent lane that simply has no mini-list of its own — it is selected from the full index — so removing it promotes lane 1 into that role. No special-casing, but the descriptions say so rather than pretending the grid is homogeneous. The splice/clamp/ratio rules live in a pure removeLaneFromTiled so they are testable without a hook. Note 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. Co-Authored-By: Claude Opus 5 (1M context) --- .../plans/2026-07-28-dispatch-lane-removal.md | 263 ++++++++++++++++++ .../features/command-palette/catalog.test.ts | 33 ++- .../workspace/commands/layoutCommands.ts | 59 ++++ .../dispatch/tiledDispatchSelectors.ts | 56 +++- .../dispatch/tiledLaneRemoval.test.ts | 96 +++++++ .../src/workspace/hook/actions/dispatch.ts | 25 ++ .../src/workspace/hook/actions/pane.ts | 23 +- .../paneRecoveryOwnership.renderer.test.tsx | 2 +- src/renderer/src/workspace/hook/index.ts | 14 +- 9 files changed, 546 insertions(+), 25 deletions(-) create mode 100644 docs/superpowers/plans/2026-07-28-dispatch-lane-removal.md create mode 100644 src/renderer/src/workspace/dispatch/tiledLaneRemoval.test.ts 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..8d981507 --- /dev/null +++ b/docs/superpowers/plans/2026-07-28-dispatch-lane-removal.md @@ -0,0 +1,263 @@ +# Dispatch Lane Removal Implementation Plan + +> **Status:** PROPOSED — awaiting review. Ships on `feat/dispatch-lane-removal`. + +**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..6179876d 100644 --- a/src/renderer/src/features/command-palette/catalog.test.ts +++ b/src/renderer/src/features/command-palette/catalog.test.ts @@ -10,7 +10,7 @@ import type { CommandDef } from '@renderer/features/command-palette/types' // 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 +// after-state: five durable preferences retired to Settings, seven approved // additions. 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 @@ -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..9475e836 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,64 @@ 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 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 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 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 declining leaves the grid untouched.', + keywords: ['close', '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. + 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, 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..716a5b75 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' // ============================================================================ @@ -162,6 +168,54 @@ export function clampTileCount(n: number): number { * put the same agent in two lanes afterwards (duplicates are allowed and the * views mirror; see DispatchLane). */ +/** + * 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, + ), + // Stored boundary ratios are positional, so removing a lane invalidates + // them. Dropping them lets the layout recompute — exactly what + // setTiledLaneCount does on any count change. + ratios: undefined, + } +} + export function buildAutoLanes( state: WorkspaceState, count: number, 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..439f02f8 --- /dev/null +++ b/src/renderer/src/workspace/dispatch/tiledLaneRemoval.test.ts @@ -0,0 +1,96 @@ +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) + expect(next?.focusedLane).toBe(1) + expect(next?.lanes[next.focusedLane]?.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('always drops stored ratios', () => { + // Ratios are positional. Carrying them across a removal lays the surviving + // lanes out against boundaries that no longer exist. + const next = removeLaneFromTiled(tiled(['a', 'b', 'c'], 0, [0.2, 0.3, 0.5]), 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..08a12ca4 100644 --- a/src/renderer/src/workspace/hook/index.ts +++ b/src/renderer/src/workspace/hook/index.ts @@ -412,7 +412,13 @@ export function useWorkspace( state: snapshot, parentSessionId: request.parentSessionId, sessionId: request.sessionId, - closeSession: closeOrchestrationSessionRef.current, + // Adapted to the orchestration contract's Promise: the MCP + // layer has no use for whether the close was declined, and + // widening a shared cross-process type for one local caller would + // be the tail wagging the dog. + closeSession: async (id, opts) => { + await closeOrchestrationSessionRef.current(id, opts) + }, }) await window.api.resolveOrchestrationRequest({ requestId: request.requestId, @@ -487,7 +493,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 +972,7 @@ export function useWorkspace( exitTiledDispatch: dispatchActions.exitTiledDispatch, setTiledLaneSession: dispatchActions.setTiledLaneSession, setTiledLaneCount: dispatchActions.setTiledLaneCount, + removeTiledLane: dispatchActions.removeTiledLane, setTiledFocusedLane: dispatchActions.setTiledFocusedLane, setTiledRatios: dispatchActions.setTiledRatios, } From 07a8667d2d3f64c1f9660eb383cf9c212dd03fcd Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 28 Jul 2026 17:46:08 +0200 Subject: [PATCH 2/2] fix(dispatch): resolve two orchestrated reviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both returned NON-BLOCKING, and the mechanics came back clean: every closeSession exit path reports correctly, the orchestration wrappers preserve rejection/options/timing, all six focus combinations are right, and the cross-await race I most suspected does not exist — clearTiledLaneSessions only maps selectedSessionId and never touches lanes.length or order, so the captured laneIndex stays valid. Four real findings fixed. ratios was being dropped wholesale, and index 0 is not a lane boundary — it is the index-SIDEBAR fraction, with only ratios.slice(1) as lane weights. So removing a lane also snapped a deliberately-dragged sidebar back to its default. Copying setTiledLaneCount was wrong here: a count increase has no honest answer because it would have to invent a weight for a new lane, but a removal does. Now keeps ratios[0], drops the removed lane's weight, and yields exactly lanes.length weights so normalizedLaneWeights does not discard them. removeLaneFromTiled had been inserted between buildAutoLanes' doc block and buildAutoLanes itself, orphaning a load-bearing comment onto the wrong symbol. Moved below. The destructive command's guard tested that a lane's session id was SET, not that it resolved. A lane holds a dead id for the render between a session disappearing and the heal effect clearing it, so the command could be admitted and then do neither of the two things its title promises. Now tests liveness. "remove agent" matched neither command — the fuzzy matcher works over title and keywords only, and the subsequence fails against both titles while no keyword carried both tokens. That is the phrase a user types for the destructive one. Added as a keyword. Rejecting "Remove Agent" as a title was right; leaving it unreachable was not. Also: this PR had introduced "grid" as a third noun for one concept alongside "tiles" and "lanes", in a Dispatch command, where the style doc says Dispatch is deliberately not a grid. Now "layout". Two focus combinations were untested — including the most common real invocation, where neither the adjust nor the clamp fires — plus a test that never asserted the lane list. catalog.test.ts's header still claimed a 102-id after-state. NOT fixed, deliberately: an orchestrating agent whose close is declined is told the child closed. requestCloseConfirmation resolves false rather than throwing, so the result builders report success unconditionally and skippedSessionIds stays empty. The Promise added here is the missing half of that fix, but threading it changes what a cross-process caller is told and deserves its own review. The comment claiming the MCP layer has no use for the signal was false and now says so. Co-Authored-By: Claude Opus 5 (1M context) --- .../plans/2026-07-28-dispatch-lane-removal.md | 29 +++++- .../features/command-palette/catalog.test.ts | 6 +- .../workspace/commands/layoutCommands.ts | 15 ++- .../dispatch/tiledDispatchSelectors.ts | 97 ++++++++++++------- .../dispatch/tiledLaneRemoval.test.ts | 45 ++++++++- src/renderer/src/workspace/hook/index.ts | 20 +++- 6 files changed, 160 insertions(+), 52 deletions(-) diff --git a/docs/superpowers/plans/2026-07-28-dispatch-lane-removal.md b/docs/superpowers/plans/2026-07-28-dispatch-lane-removal.md index 8d981507..fbe7e5c2 100644 --- a/docs/superpowers/plans/2026-07-28-dispatch-lane-removal.md +++ b/docs/superpowers/plans/2026-07-28-dispatch-lane-removal.md @@ -1,6 +1,33 @@ # Dispatch Lane Removal Implementation Plan -> **Status:** PROPOSED — awaiting review. Ships on `feat/dispatch-lane-removal`. +> **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. diff --git a/src/renderer/src/features/command-palette/catalog.test.ts b/src/renderer/src/features/command-palette/catalog.test.ts index 6179876d..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 +// 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. Keeping ONE snapshot that moved — rather +// 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. @@ -336,7 +336,7 @@ 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`), and the two lane-removal + // `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 9475e836..88880246 100644 --- a/src/renderer/src/features/workspace/commands/layoutCommands.ts +++ b/src/renderer/src/features/workspace/commands/layoutCommands.ts @@ -75,7 +75,7 @@ export const layoutCommands: CommandDef[] = [ 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 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.', + 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 @@ -92,14 +92,21 @@ export const layoutCommands: CommandDef[] = [ 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 declining leaves the grid untouched.', - keywords: ['close', 'agent', 'lane', 'tile', 'tiled dispatch', 'shrink', 'finished', 'done'], + 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. - return Boolean(tiled.lanes[tiled.focusedLane]?.selectedSessionId) + // + // 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 diff --git a/src/renderer/src/workspace/dispatch/tiledDispatchSelectors.ts b/src/renderer/src/workspace/dispatch/tiledDispatchSelectors.ts index 716a5b75..35042417 100644 --- a/src/renderer/src/workspace/dispatch/tiledDispatchSelectors.ts +++ b/src/renderer/src/workspace/dispatch/tiledDispatchSelectors.ts @@ -168,6 +168,40 @@ export function clampTileCount(n: number): number { * put the same agent in two lanes afterwards (duplicates are allowed and the * views mirror; see DispatchLane). */ +export function buildAutoLanes( + state: WorkspaceState, + count: number, + preserve: DispatchLane[] = [], +): DispatchLane[] { + const rows = buildVisibleDispatchRows(state) + const claimed = new Set( + preserve + .map(lane => lane.selectedSessionId) + .filter((id): id is SessionId => Boolean(id)), + ) + const lanes: DispatchLane[] = [] + for (let i = 0; i < count; i++) { + if (preserve[i]) { + lanes.push(preserve[i]) + continue + } + const next = rows.find(row => !claimed.has(row.sessionId)) + if (next) { + claimed.add(next.sessionId) + lanes.push({ selectedSessionId: next.sessionId }) + } else { + lanes.push({}) + } + } + return lanes +} + +// NOTE: render still performs scope validation before mounting a lane, but the +// durability boundary must not rely on a later React effect. Autosave routes +// 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. @@ -209,43 +243,34 @@ export function removeLaneFromTiled( laneIndex < tiled.focusedLane ? tiled.focusedLane - 1 : tiled.focusedLane, lanes.length - 1, ), - // Stored boundary ratios are positional, so removing a lane invalidates - // them. Dropping them lets the layout recompute — exactly what - // setTiledLaneCount does on any count change. - ratios: undefined, + // `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), } } -export function buildAutoLanes( - state: WorkspaceState, - count: number, - preserve: DispatchLane[] = [], -): DispatchLane[] { - const rows = buildVisibleDispatchRows(state) - const claimed = new Set( - preserve - .map(lane => lane.selectedSessionId) - .filter((id): id is SessionId => Boolean(id)), - ) - const lanes: DispatchLane[] = [] - for (let i = 0; i < count; i++) { - if (preserve[i]) { - lanes.push(preserve[i]) - continue - } - const next = rows.find(row => !claimed.has(row.sessionId)) - if (next) { - claimed.add(next.sessionId) - lanes.push({ selectedSessionId: next.sessionId }) - } else { - lanes.push({}) - } - } - return lanes +/** + * 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)] } - -// NOTE: render still performs scope validation before mounting a lane, but the -// durability boundary must not rely on a later React effect. Autosave routes -// 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. diff --git a/src/renderer/src/workspace/dispatch/tiledLaneRemoval.test.ts b/src/renderer/src/workspace/dispatch/tiledLaneRemoval.test.ts index 439f02f8..b8c221c8 100644 --- a/src/renderer/src/workspace/dispatch/tiledLaneRemoval.test.ts +++ b/src/renderer/src/workspace/dispatch/tiledLaneRemoval.test.ts @@ -49,10 +49,36 @@ describe('removeLaneFromTiled', () => { 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. @@ -61,10 +87,21 @@ describe('removeLaneFromTiled', () => { expect(next?.focusedLane).toBe(1) }) - it('always drops stored ratios', () => { - // Ratios are positional. Carrying them across a removal lays the surviving - // lanes out against boundaries that no longer exist. - const next = removeLaneFromTiled(tiled(['a', 'b', 'c'], 0, [0.2, 0.3, 0.5]), 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() }) diff --git a/src/renderer/src/workspace/hook/index.ts b/src/renderer/src/workspace/hook/index.ts index 08a12ca4..94470c92 100644 --- a/src/renderer/src/workspace/hook/index.ts +++ b/src/renderer/src/workspace/hook/index.ts @@ -412,10 +412,22 @@ export function useWorkspace( state: snapshot, parentSessionId: request.parentSessionId, sessionId: request.sessionId, - // Adapted to the orchestration contract's Promise: the MCP - // layer has no use for whether the close was declined, and - // widening a shared cross-process type for one local caller would - // be the tail wagging the dog. + // 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) },