diff --git a/docs/workspace-panel-todo.md b/docs/workspace-panel-todo.md index 07d61102..d8e11d76 100644 --- a/docs/workspace-panel-todo.md +++ b/docs/workspace-panel-todo.md @@ -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. diff --git a/src/workspaces/officerdev/src/apps/Terminal/RunningShells.tsx b/src/workspaces/officerdev/src/apps/Terminal/RunningShells.tsx index fc2cd962..489f3e3a 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/RunningShells.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/RunningShells.tsx @@ -22,6 +22,10 @@ type PtySession = { lastActivityAt: number; title?: string; pid?: number; + // How many sockets are attached. The sidecar has always sent it and this list has always dropped it, + // which left every row looking the same — yet it is the only field that says whether a shell is one + // you are looking at or one nothing is pointing at any more. + clients?: number; }; const RELATIVE_UNITS: [limit: number, div: number, unit: string][] = [ @@ -87,10 +91,19 @@ export const RunningShells = () => {
{/* The title the shell set for itself (OSC 0/2) — usually the running command. It is what makes an abandoned shell identifiable instead of a bare uuid. */} -

{s.title || 'shell'}

+

+ {s.title || 'shell'} + {s.clients === 0 && ( + + orphan + + )} +

{s.sessionId.slice(0, 8)} · {s.cols}×{s.rows} - {s.pid ? ` · pid ${s.pid}` : ''} · idle {since(s.lastActivityAt)} · up {since(s.createdAt)} + {s.pid ? ` · pid ${s.pid}` : ''} + {s.clients === undefined ? '' : ` · ${s.clients} attached`} · idle {since(s.lastActivityAt)} · up{' '} + {since(s.createdAt)}