say which shells nobody is looking at

The sidecar has always sent `clients` on each session and this list has always dropped it, so a shell
you are typing into and a shell nothing is attached to rendered identically. It is now on the type,
shown as "N attached", and a zero earns the row an orphan badge — the count only helps if you do not
have to read it to notice.

Also records §5.1 and the whole of §1 in the todo, including that the diff-the-layout implementation
§5.1 used to propose is struck and why.
This commit is contained in:
2026-08-07 09:58:04 +00:00
parent c92b51cacb
commit 2c84bb34be
2 changed files with 79 additions and 17 deletions
+64 -15
View File
@@ -30,7 +30,10 @@ before deciding what to pick up next. Its conclusions in one paragraph:
unanswered question.
**§5.9 is closed** (`dbe585f`, `717580f`, `585f234`, `d3922bd`, `ca046a3`) — the bag is a parsed
identity, the app-config fields are deleted or moved onto the app, and the inert context is built
once. Tier B is now panel lifecycle (§5.1), `normalizeLayout` (§5.4) and the mobile decision (§6).
once. **§5.1's first two items are closed too** (`04371a9`, `198dc71`, `c92b51c`) — panels have a close
signal that fires on a real close and on nothing else, and the terminals use it; the chat panel
deliberately does not, which is the inversion above. Tier B is now the pty backstop (the third item of
§5.1), `normalizeLayout` (§5.4) and the mobile decision (§6).
- **Tier C, orthogonal:** everything else — including **§5.2 (remounts) and §5.3 (drag-to-move), both
large downgrades.** A panel is now a pointer to a server-side session, so a remount costs a replay, and
the drag hazard is disarmed by resolving identity by name (`e588524` + `bc82086`).
@@ -51,8 +54,11 @@ below was opened; DB claims were run against live `officer_dev`. Paths are relat
## 1. Observability first — do these before anything else
Both are two-line fixes, and without them you cannot tell whether any later fix worked.
**Both are done as of 2026-08-07** — they went in with the terminal close wiring (§5.1), which is what
they were blocking: there was no way to see whether a shell had actually been killed. §1's third item
(the swallowed persist failure) was already closed. **§1 is empty.**
- [ ] **Fix the Running Shells paths — the panel has 404'd since 2026-07-31.**
- [x] **Fix the Running Shells paths — the panel has 404'd since 2026-07-31.** — done `c92b51c`.
`apps/Terminal/RunningShells.tsx:43,49` call `/terminal/sessions` and
`DELETE /terminal/sessions/:id`. `useClient` prefixes `/api`; `servers/sidecar/create-proxy.ts:69`
strips the mount prefix, so the sidecar receives `/sessions`. The pty sidecar serves only
@@ -61,10 +67,14 @@ Both are two-line fixes, and without them you cannot tell whether any later fix
`/terminal/_officer/sessions/:id`. The panel (`fccf212`) predates the proxy move (`7129cd8`) that
deleted the old `servers/api/terminal/router.ts` and was never updated.
Symptom today: permanently reads "No shells running", kill button is a silent no-op.
**Confirmed against the running sidecar before fixing**: `GET /sessions``{"error":"not found"}`,
`GET /_officer/sessions` → the list. Both paths corrected.
- [ ] **Surface the `clients` count in Running Shells.** `RunningShells.tsx:12-20` drops the `clients`
field the sidecar returns (`sidecars/pty/sessions.mjs:169`) — the one field that distinguishes an
orphan (`clients: 0`) from a live shell. Add it to the type and render it.
- [x] **Surface the `clients` count in Running Shells.** — done alongside the paths. `RunningShells.tsx`
dropped the `clients` field the sidecar returns (`sidecars/pty/sessions.mjs:169`) — the one field
that distinguishes an orphan (`clients: 0`) from a live shell. Now on the type, rendered as
"N attached", and `clients === 0` also earns the row an **orphan** badge, since the count is only
useful if you do not have to read it to notice.
- [x] **Stop swallowing persist failures.** `state/src/useDashboardState.ts:46` is
`.catch(() => { })`. Every 500 in this document is invisible because of it — the optimistic cache
@@ -270,17 +280,30 @@ route, the read, and the renderer's willingness to survive being handed somethin
### 5.1 Give panels a lifecycle — the highest-value change here
- [ ] **`onClose` on `AppRegistryEntry`, invoked by the mutators, not by unmount.**
`components/Workspace/types.ts:38-47` has no close hook; the only `onClose` is
- [x] **A close hook, invoked by the mutators, not by unmount.** — done `198dc71`.
`components/Workspace/types.ts:38-47` had no close hook; the only `onClose` was
`PanelComponentEntry.onClose` (`:55`), which is a header-button handler for _ephemeral_ panels and
is suppressed on mobile (`PanelSlot.tsx:382`). So the two ways a panel dies —
`removePanel(root, id)` and `setApp(root, id, null)` (the red traffic light, `PanelSlot.tsx:295`)
are pure tree rewrites that notify nobody.
**Invoke it from `WorkspaceView`'s `handleRemove`/`handleSetApp`, never from a `PanelSlot`
unmount** — a `PanelSlot` unmount is precisely the ambiguous signal this exists to replace.
Implementation: diff removed panel ids old-tree new-tree, or have the mutators return them.
were pure tree rewrites that notified nobody.
This is the thing `apps/Terminal/TerminalWrapper.tsx:31-38` asks for in writing: *"Killing on
Shipped as `usePanelClose(panelId, handler)` rather than a registry field: what needs releasing is
known to the component, not to the registry entry, and `CommandTerminalWrapper` shares one registry
key across several panels with different sessions. `WorkspaceView` fires it from `handleRemove` and
from `handleSetApp` when the app actually changes, and from nowhere else. Registration is never
torn down — honouring an unmount would reintroduce the exact bug — and handlers are stamped with
the workspace they were registered on so one dashboard's panel id cannot fire another's.
**Strike the implementation this item used to suggest** ("diff removed panel ids old-tree
new-tree"). It does not work, and `layout-utils.test.ts` now pins why (`04371a9`): `movePanel`
inserts through `newPanelFrom`, which mints a fresh `uid()`, so a dragged panel's id is absent from
the new tree while its app is still on screen; and `swapPanels` exchanges `{appType, config}`
between two ids that both stay put, so a swap reads as two closes. **A panel id is a position in
the tree, not an app instance**, and a diff of positions cannot answer a question about instances.
The second half of the suggestion — have the mutators report it — is what landed, in the form of
firing at the call site that already knows the user asked for a close.
This is the thing `apps/Terminal/TerminalWrapper.tsx:31-38` asked for in writing: *"Killing on
unmount is not an option until the panel system can tell a real close from an incidental
remount."* It closes the terminal orphan leak as a consequence rather than as a special case, and
the same gap affects every panel holding a server-side resource.
@@ -291,10 +314,17 @@ route, the read, and the renderer's willingness to survive being handed somethin
`clearClaudeSession`, which destroys it and orphans the transcript. A panel is a pointer to a
server-side session; **closing the window must not delete what it points at.** What coordination
wants from `onClose` is a guarantee that nothing rides the disconnect path, not an eager cleanup.
`ChatPanelWrapper` therefore does **not** call `usePanelClose`, and that is a decision, not an
omission — the hook's own doc comment says so, so the next person to add one reads it there.
- [ ] **Then: kill the pty on real close.** Once the hook exists, `TerminalWrapper` /
`CommandTerminalWrapper` / `HostTerminalWrapper` can `DELETE /terminal/_officer/sessions/:id` and
drop the map entry — the thing they each explain they cannot currently do.
- [x] **Then: kill the pty on real close.** — done `c92b51c`. The three wrappers now share
`useTerminalSession`, which drops the map entry and `DELETE`s the session from `usePanelClose`.
Two things fell out of doing it. `HostTerminalWrapper` still had the unmount cleanup the other two
had removed, so it had been orphaning a host shell on every layout or route change. And
`RunningShells` — the panel that exists to make orphans visible — was calling `/terminal/sessions`,
while the proxy strips `/api/terminal` and the sidecar only answers under `/_officer`; verified
live, `/sessions` returns `{"error":"not found"}`. That panel had always read "No shells running"
and its kill button had always been a no-op.
- [ ] **Then: reap orphans in the sidecar as a backstop.** `sidecars/pty/sessions.mjs:127-130`
`detach()` never checks `clients.size === 0`; there is no idle timeout and no session cap.
@@ -722,3 +752,22 @@ Low priority, but each line here is a line someone will read and believe.
written out the same eleven switched-off fields by hand. `inertInteraction` names the idea once.
Net across all of §5.9: the context is 15 fields from 18, and everything still on it answers
"where is this panel" rather than "what should this panel do".
- [x] **A panel can be told it was closed, and only when it was.** _(`04371a9` + `198dc71` + `c92b51c`,
branch `agent-coordination-mvp`)_ — the first two items of §5.1, and the terminal orphan leak with
them. `usePanelClose(panelId, handler)`, fired by `WorkspaceView` from `handleRemove` and from
`handleSetApp` when the app actually changes, and from nowhere else.
The interesting part is what it is *not*. This item used to propose diffing the layout before and
after; two tests now stand in `layout-utils.test.ts` to stop anyone trying it, because `movePanel`
mints a fresh panel id on the way and `swapPanels` exchanges contents between stationary ones — so
a drag reads as a close and a swap reads as two. A panel id is a position in the tree, not an app
instance. Nor is it an unmount hook: registration is deliberately never torn down, since "this
component went away" is the ambiguous signal the whole thing exists to stop trusting.
The terminals use it through a shared `useTerminalSession`, which turned up two more defects while
being written: `HostTerminalWrapper` had kept the unmount cleanup the other two wrappers had
removed, orphaning a host shell on every layout change; and `RunningShells`, the panel whose entire
job is to make orphans visible, was asking the pty sidecar for a route it does not serve, so it had
always shown an empty list and its kill button had never done anything.
A chat panel deliberately does **not** use the hook — it is a pointer to a server-side session, and
closing a window must not delete what it points at. That is recorded in the hook's own doc comment,
where the next person to add one will read it.