Repository navigation
fix(dispatch): file Dispatch terminals as detached rows in the natural flow - #672
Conversation
…t flow Terminals created from Dispatch become grid leaves while agents become detached rows, and buildDispatchGroups emits grid before detached — so a Dispatch terminal is structurally guaranteed to sort above every agent. Record why the leaf-based justification no longer holds before changing it. Refs #671
…l flow A terminal created from Dispatch always rendered at the top of its project group, above every agent, regardless of when it was created. The cause was structural rather than a sort bug. splitFocused carried two Dispatch branches: agents became detached rows, terminals were spliced into the owning tab's grid tree. buildDispatchGroups emits each group as [...gridSessionIds, ...detachedSessionIds], so a grid terminal could never sort below a detached agent. It was pinned to the very top rather than merely last in the grid slice because the split anchor degraded to leafIds[0] — the focused Dispatch row is normally a detached agent, which has no grid leaf to split beside. Merge the two branches into one kind-agnostic Dispatch flow. Terminals now resolve their project through resolveDispatchSpawnTarget like every other kind, which is also what preserves the #366 invariant (project tab and cwd both come from the focused lane) without a parallel resolver to keep in sync, and they land in the focused tiled lane through applyDispatchSpawnFocus. The leaf-based justification the old branch cited — tmux name, resize lifecycle, undo/close history, persistence — is session-scoped, not grid-scoped, and detached terminals were already reachable through detachFocusedToDispatch. The one real consequence is hibernation across restart, which matches agents and is what TerminalLeaf's own ensureSessionLive-on-mount already handles. Merging also lets the orphan guard cover both kinds: a tab closed between the awaited spawn and the state commit used to leak a live agent backend that belonged to no tree and no detached record. Only the terminal branch guarded that; now both do. Two cleanups ride along because they are the same code and would otherwise be left describing a world that no longer exists: - Extract detachedDispatchRecord. Five call sites hand-wrote the identical DetachedSessionRecord literal, and the merged branch would have been a sixth. detachedAt is the only thing ordering rows inside a project group, so the shape is worth having one definition of. - Delete resolveDispatchTerminalSplitTarget (its only purpose was picking a grid split anchor) plus findTerminalSessionInTab and findTerminalInLatestTab, which were already dead before this change — leftovers from the removed Dispatch project-terminal column, confirmed by grep across src/ and packages/. Command descriptions and the undoClose comment that asserted the old behaviour are updated to match. Refs #671
… placement fix Two orchestrated reviews of this branch found one regression it introduced, one test suite that could not detect the bug it fixes, and several comments asserting mechanisms that are not real. Undo-close (the regression). Moving Dispatch terminals onto the detached path put them on closeSession's detached arm, which captured no undo entry. For an agent that was a lost convenience; for a terminal it is data loss. Closing one stops the attach PTY but leaves the tmux session alive, and once the row is out of workspace.json the next launch's tmux reconcile sees a live session with no persisted owner, classifies it as an orphan, and kills it. Before this branch the same terminal was a grid leaf, so ⌘⇧T restored it with recoverTmuxName and the scrollback came back. Adds a third ClosedEntry variant, 'detached'. It cannot reuse ClosedPane, whose every placement field — direction, ratio, sibling anchor — is a grid concept a detached row has no answer for. The DetachedSessionRecord is stored verbatim so detachedAt survives and undo returns the row to its position instead of the bottom of the list. Restore refuses as stale when the project tab is gone, since a record filed under a dead tab renders in no group and would strand a live backend. Detached agents gain an undo they never had. Tests that actually fail on main. The shipped tests hand-built the detached record and asserted buildDispatchGroups' concatenation — which passes identically against unfixed code, so they could not have caught the grid branch returning. Review proved this by running them with the modules aliased to main. They are replaced with specs that drive splitFocused itself, each confirmed to fail on main: grid untouched plus terminal last, focused-lane placement, normal-mode grid split still intact, and the undo path above. The existing usePaneActions setup is extracted to a shared harness whose mock spawn now registers SessionMeta the way the real one does — without that, every selector filtering on state.sessions drops the new session and placement assertions pass against a session that never appeared. Also restores the resolveDispatchSpawnTarget coverage for a DETACHED session in the focused lane, which the deleted resolver's test used to own and nothing replaced. That is the normal Dispatch state and the cwd a new terminal inherits; without it, ⌥T beside a worktree agent could silently spawn in the parent repo. Corrected comments. The claim that the split anchor degraded to leafIds[0] and pinned the terminal to the very top was wrong: focusDispatchSession deliberately never writes Tab.focusedSessionId, so that fallback is unreachable, and splitLeaf inserts after its anchor. The grid-before-detached concatenation is the whole cause and is sufficient alone. The claim that spawn drops resumeSessionId for terminals was also wrong — only builtInMcpDomains is kind-gated. The overlay's terminal branch is unreachable in Dispatch and is now labelled as the guard it is, including the projectIntent difference that makes "exactly like an agent" false. Two smaller fixes in the same blast radius: the orphan guard now kills the backend with the kind/cwd it already resolved rather than through an ownership proof that re-reads a render-refreshed ref, and Bury explains itself on a Dispatch row instead of opening a prompt that silently does nothing. Plus the remaining project-terminal-column leftovers in dispatch.ts and two stale docs. Refs #671
…est provenance Second review pass on the undo-close work. Ten findings, all minor; none was a regression against main, all sat in capability the previous commit introduced. The stale-ref hazard was reintroduced in the code that fixed it. restoreDetachedEntry's failed-placement bail called killSession, whose ownership proof re-reads kind and cwd from refs.stateRef — a render-body mirror that, immediately after an awaited spawn, does not yet contain the new session. Review confirmed the consequence is not theoretical: the proof bails on missing metadata, so the original orphan guard never reclaimed anything in production. Both call sites now kill with locally resolved values, and both awaits are guarded so a rejecting IPC cannot skip the renderer-side cleanup that follows. Undo dropped the fields that define a linked child. spawn builds SessionMeta from cwd, kind, tmuxName, providerSessionId and builtInMcpDomains only; title, linkedParentId and the orchestration ids are durable metadata it never sees. Linked and orchestration children are always detached, so they are exactly the population this undo path covers: without the patch an undone child returned un-nested, stopped cascading when its parent closed, and a titled row silently relabelled to its cwd basename. Test provenance was overstated. The claim that every test fails on main holds for the ordering and undo specs only. Lane placement, normal-mode grid split, and the resolver test all pass on main — resolveDispatchSpawnTarget is untouched by this PR, so it cannot discriminate. They remain worth keeping, each catching a distinct mutation, but the plan now separates discriminators from invariant guards rather than claiming evidence it does not have. The cwd assertion was inert. Every session in the fixture shared one cwd, so removing target.cwdSessionId from the cwd chain — breaking the #366 wire this PR claims to preserve — left all six tests green. The focused detached agent now sits in a worktree, a path reachable only through that link. Two harness comments asserted the opposite of two correct comments in the same commit. The harness writes stateRef synchronously; production does not. It is therefore strictly more current than the app and cannot reproduce stateRef-lag bugs, which is worth stating where someone would otherwise trust it. The claim that a non-registering spawn makes placement assertions pass silently was also backwards: row-stream assertions fail loudly, only direct dispatchMode reads pass vacuously. Remaining: the undo-close command description still said detached rows were only restorable via a closed tab; the restore-focus comment overclaimed in Tiled Dispatch, where the lane is not restored and deliberately so; and a blockquote added last commit sat between two table rows, which in GFM terminates the table and renders the remaining eight rows as literal text. Refs #671
Addresses the review of this branch that completed after its coordinating
session had already died, so its findings were never collected.
F1 (blocker) — restoreDetachedEntry never set activeTabId. Every other path
that files a detached row sets it in the same updater (splitFocused's Dispatch
branch, createDetachedDispatchAgent, restoreTabEntry), because
buildDispatchGroups filters sourceTabs to activeTabId outside global scope.
Undoing a row belonging to a different project tab therefore spawned a live
backend that rendered NOWHERE: filed correctly, filtered out of the list, and
the focus we set was immediately overwritten by ClassicDispatchLayout's focus
effect. With no toast on this path the user saw undo do nothing while a provider
process — or a re-attached tmux session — ran invisibly.
F2/F3 — every detached close captured an undo entry, including the bulk and
programmatic ones. The stack is a 10-entry LIFO, so one "Close Old Agents" purge
or one orchestration run close could flush the pane the user closed by mistake
out of it, and the next undo would resurrect an agent they had deliberately
cleared. Cascades were worse: closeLinkedChildren runs BEFORE the parent's entry
is pushed, so a parent with two children left three entries — undo restored the
parent under a NEW id and the children then came back carrying the OLD
linkedParentId, un-nested, no longer cascading, with a dead id written into
workspace.json that nothing scrubs.
CloseSessionOptions grows `captureUndo`, defaulting to capture. Close Old
Agents, closeOrchestrationRun, and cascade children opt out; single closes —
including closeOrchestrationAgent, which is one deliberate act at human scale —
still capture.
F4 — carry orchestrationBootstrapPromptDelivered through the restore. Its own
doc comment says it must survive with the child and spawn never sets it, so an
undone orchestration child would re-run its bootstrap handoff.
F5 — the new bail's killSession is now guarded like restorePaneEntry's. It
reaches an IPC invoke that can reject, and the bail runs with the entry already
popped, so a throw lost the entry, skipped bumpUndoCloseVersion, and escaped
into the keybinding handler.
F6 — drop three parameters from useDispatchActions that became dead when
findTerminalInLatestTab and pendingTerminalByTabRef were deleted. tsconfig sets
no noUnusedParameters, so tsc stayed green on them.
The F1 regression is verified to fail without the fix ("expected 'tabB' to be
'tabA'") and asserts VISIBILITY through buildVisibleDispatchRows rather than the
activeTabId field, since the visible row is the contract and the field is only
the mechanism. The prior fixture had one tab, which is why this could not
surface. Also merges current main.
Verification: typecheck clean both projects; full suite 2096/2097, sole failure
the pre-existing #669 corpus fixture.
Refs #671
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XyCQ6Q4r6iCmuSyCTtUa95
Review findings resolvedThe independent review of this branch completed at 14:36 the day after its coordinating session died, so its findings were never collected. Recovered from the transcript on disk and resolved here. F1 (blocker) — undo restored an invisible session. F2/F3 — bulk closes ate the undo history. The stack is a 10-entry LIFO and every detached close captured one, so a single "Close Old Agents" purge or orchestration run close could flush out the pane the user closed by mistake — and ⌘⇧T would then resurrect an agent they had deliberately cleared. Cascades were worse:
F4 — carry F5 — guard the new bail's F6 — drop the three parameters On the reviewer's own critiqueThey proved this PR's first test suite passed against One existing test legitimately changed: Also merged current Verification: typecheck clean both projects; full suite 2096/2097, the sole failure being the pre-existing #669 corpus fixture. |
Fixes #671
Problem
A terminal created from Dispatch — most visibly in Tiled Dispatch — always rendered at the top of its project group, above every agent, regardless of when it was created. Agents created from the same surface append in creation order. The one row you did not just create took the top slot, and the index stopped reading as a record of what you have open.
Root cause
Not a sort bug — a structural one.
splitFocusedcarried two Dispatch branches:!== 'terminal'detachedSessions[id], ordered bydetachedAt)=== 'terminal'tab.rootviasplitLeafbuildDispatchGroupsemits every project group as[...gridSessionIds, ...detachedSessionIds]. Every Dispatch agent is detached and every Dispatch terminal was a grid leaf, so a terminal could never sort below an agent. That concatenation is the entire cause and is sufficient on its own — the terminal's position within the grid slice never mattered, because the whole grid slice precedes the detached rows.Implemented behavior
The two branches are now one kind-agnostic Dispatch flow. A Dispatch terminal is a detached dispatch row filed under the same resolved project an agent would get, appearing in creation order, and landing in the focused tiled lane via
applyDispatchSpawnFocus. Terminals created outside Dispatch are untouched — ⌥T in normal mode still splits the grid.Design decisions and tradeoffs
Why the old leaf-based justification no longer holds. The removed comment cited tmux name, resize lifecycle, undo/close history, and persistence as leaf-based. Each is session-scoped, and a detached terminal was already a reachable, supported state:
detachFocusedToDispatchnever excluded terminals;renderWorkspaceLeafrenderskind === 'terminal'inside a Dispatch lane;TerminalLeafwakes its own backend viaensureSessionLive, which passesrecoverTmuxName.The one exception was undo/close history, which really was grid-scoped — see below.
Why route terminals through
resolveDispatchSpawnTarget. It preserves #366 (project tab and cwd both from the focused lane) without a parallel terminal resolver that can drift.Orphan guard extended to both kinds, and it now kills the backend with the kind/cwd this call already resolved rather than through an ownership proof that re-reads a render-refreshed ref — a lagging read would make the kill silently no-op and leak the very orphan the guard exists to prevent.
Undo-close: a regression this change forced, and its fix
Moving Dispatch terminals onto the detached path put them on
closeSession's detached arm, which captured no undo entry. For an agent that is a lost convenience. For a terminal it is data loss: closing one stops the attach PTY but leaves the tmux session alive, and once the row is gone fromworkspace.jsonthe next launch's tmux reconcile sees a live session with no persisted owner, classifies it an orphan, and kills it (src/main/tmux/tmuxRecovery.ts). Before this branch the terminal was a grid leaf, so ⌘⇧T restored it withrecoverTmuxName.Adds a third
ClosedEntryvariant,'detached'. It cannot reuseClosedPane, whose every placement field (direction, ratio, sibling anchor) is a grid concept a detached row has no answer for. TheDetachedSessionRecordis stored verbatim sodetachedAt— the only thing ordering rows in a project group — survives, and undo returns the row to its position rather than the bottom of the list. Restore refuses asstalewhen the project tab is gone, since a record under a dead tab renders in no group and would strand a live backend. Detached agents gain an undo they never had.Cleanups riding along
detachedDispatchRecord— five call sites hand-wrote the identicalDetachedSessionRecord; the merged branch would have been a sixth.resolveDispatchTerminalSplitTarget,findTerminalSessionInTab,findTerminalInLatestTab, plus the sibling leftovers from the same removed project-terminal column (pendingTerminalByTabRef, two unused imports, a dangling comment fragment).paneCommands.ts,NewAgentPlacementOverlay.tsx(its terminal branch is unreachable in Dispatch — now labelled as the guard it is),undoClose.ts,DispatchLayout.tsx, and two stale docs.Tests and verification
The first revision's tests were worthless and review proved it by running them with the modules aliased to
main: they hand-built the detached record and assertedbuildDispatchGroups' concatenation, so they passed identically against unfixed code and could never have caught the grid branch returning.They are replaced with specs that drive
splitFocuseditself. Each was confirmed to fail onmain(verified in a throwawaymainworktree —expected [ 'a1', 'aTerm' ] to deeply equal [ 'a1' ],expected null to match object { type: 'detached' }):tab.rootuntouched, files a detached record, terminal lands last.tmuxName; undo respawns withrecoverTmuxNameand re-files at the olddetachedAt; a dead project tab is stale rather than stranding a backend.The
usePaneActionssetup is extracted to a shared harness whose mockspawnregistersSessionMetathe way the real one does — without that, selectors filtering onstate.sessionsdrop the new session and placement assertions pass against a session that never appeared.Ran:
tsc -b tsconfig.node.json+tsc -p tsconfig.web.json --noEmit— clean on both.NODE_ENV=development npx vitest run— 2030 passed, 294 files, one pre-existing unrelated failure (imageAttachment.test.tscorpus census, open issue #669: cites a deleted personal~/.claudetranscript).Known limitations
reconcilebuilds its persisted list from everykind === 'terminal'row regardless of grid/detached, so a hibernated terminal is not reaped): no scrollback loss, no second shell. Verified by source inspection only — the app was not launched.paneLabelForSessionindices shift for a tab containing a Dispatch terminal, becauseresolveTabSessionsalso orders grid-then-detached. Same coordinate the Dispatch list shows, so the two stay in agreement.🤖 Generated with Claude Code
https://claude.ai/code/session_01KuvoZtYUJiGPe29KAkP94A