From c92b51cacbbfacc3aa218ecb3bdbdd78fa349c9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 7 Aug 2026 09:55:50 +0000 Subject: [PATCH] kill a terminal's shell when its panel is actually closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three wrappers held three copies of the same `panelId -> sessionId` bookkeeping, and one of the three still had the unmount cleanup the other two had removed: `HostTerminalWrapper` dropped its map entry on every layout or route change, minted a new uuid on the way back, and left the host shell running with nothing pointing at it. All three now share `useTerminalSession`, which forgets the session and kills the shell from `usePanelClose` — a real close, and nothing that merely looks like one. The kill request goes to `/terminal/_officer/sessions/:id`, which is also a fix. `RunningShells` was asking for `/terminal/sessions`; the proxy strips `/api/terminal` and forwards the rest verbatim, and the pty sidecar only answers under `/_officer`, so that route 404s. Verified against the live sidecar: `/sessions` returns `{"error":"not found"}` and `/_officer/sessions` returns the list. The panel has therefore always read "No shells running" and its kill button has always been a no-op — which is why the orphaned shells it exists to surface were never actually visible. --- .../apps/Terminal/CommandTerminalWrapper.tsx | 22 ++------ .../src/apps/Terminal/HostTerminalWrapper.tsx | 35 +++---------- .../src/apps/Terminal/RunningShells.tsx | 15 ++++-- .../src/apps/Terminal/TerminalWrapper.tsx | 30 ++--------- .../src/apps/Terminal/use-terminal-session.ts | 52 +++++++++++++++++++ 5 files changed, 75 insertions(+), 79 deletions(-) create mode 100644 src/workspaces/officerdev/src/apps/Terminal/use-terminal-session.ts diff --git a/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx b/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx index bbef2a49..1b6f8f3c 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx @@ -1,12 +1,10 @@ -import { useCallback, useEffect } from 'react'; +import { useCallback } from 'react'; import { useWorkspace } from '../../components/Workspace'; -import { useDashboardState } from 'state/useDashboardState'; import { useGlobal } from 'hooks/useGlobal'; import type { TerminalConnectionState } from './Terminal'; import { TerminalView } from './Terminal'; import { terminalStateKey } from './state-key'; - -const EMPTY_TERMINALS: Record = {}; +import { useTerminalSession } from './use-terminal-session'; type CommandTerminalWrapperProps = { panelId: string; @@ -16,21 +14,7 @@ type CommandTerminalWrapperProps = { export const CommandTerminalWrapper = ({ panelId, command, statePrefix }: CommandTerminalWrapperProps) => { const { workspace, cwd } = useWorkspace(); - const stateKey = terminalStateKey(statePrefix, workspace); - const { value: terminals, setValue: setTerminals } = useDashboardState>( - stateKey, - EMPTY_TERMINALS, - ); - const sessionId = terminals[panelId]; - - useEffect(() => { - if (!sessionId) { - setTerminals((prev) => ({ ...prev, [panelId]: crypto.randomUUID() })); - } - }, [panelId, sessionId, setTerminals]); - - // Kept across unmounts, same as TerminalWrapper: deleting it here meant every layout or route change - // minted a new uuid, abandoned the running shell, and re-ran the command from scratch. + const sessionId = useTerminalSession(panelId, terminalStateKey(statePrefix, workspace)); const [, setConnState] = useGlobal(`terminal-conn-${panelId}`, 'disconnected'); const onConnectionChange = useCallback((state: TerminalConnectionState) => setConnState(state), [setConnState]); diff --git a/src/workspaces/officerdev/src/apps/Terminal/HostTerminalWrapper.tsx b/src/workspaces/officerdev/src/apps/Terminal/HostTerminalWrapper.tsx index d0fb608f..6eaf4ab1 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/HostTerminalWrapper.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/HostTerminalWrapper.tsx @@ -1,39 +1,18 @@ -import { useCallback, useEffect, useRef } from 'react'; +import { useCallback } from 'react'; import { useWorkspace } from '../../components/Workspace'; -import { useDashboardState } from 'state/useDashboardState'; import { useGlobal } from 'hooks/useGlobal'; import type { TerminalConnectionState } from './Terminal'; import { TerminalView } from './Terminal'; import { terminalStateKey } from './state-key'; +import { useTerminalSession } from './use-terminal-session'; -const EMPTY_TERMINALS: Record = {}; - +// This one kept the unmount cleanup the other two wrappers had already removed, so every layout or route +// change dropped its `panelId → sessionId` entry, minted a new uuid on the way back, and left the previous +// host shell running with nothing pointing at it. It now forgets the session where the others do: when the +// panel is actually closed. export const HostTerminalWrapper = ({ panelId }: { panelId: string }) => { const { workspace, cwd } = useWorkspace(); - const stateKey = terminalStateKey('host-terminals', workspace); - const { value: terminals, setValue: setTerminals } = useDashboardState>( - stateKey, - EMPTY_TERMINALS, - ); - const setTerminalsRef = useRef(setTerminals); - setTerminalsRef.current = setTerminals; - - const sessionId = terminals[panelId]; - - useEffect(() => { - if (!sessionId) { - setTerminals((prev) => ({ ...prev, [panelId]: crypto.randomUUID() })); - } - }, [panelId, sessionId, setTerminals]); - - useEffect(() => { - return () => { - setTerminalsRef.current((prev) => { - const { [panelId]: _, ...rest } = prev; - return rest; - }); - }; - }, [panelId]); + const sessionId = useTerminalSession(panelId, terminalStateKey('host-terminals', workspace)); const [, setConnState] = useGlobal(`terminal-conn-${panelId}`, 'disconnected'); const onConnectionChange = useCallback((state: TerminalConnectionState) => setConnState(state), [setConnState]); diff --git a/src/workspaces/officerdev/src/apps/Terminal/RunningShells.tsx b/src/workspaces/officerdev/src/apps/Terminal/RunningShells.tsx index 1574b499..fc2cd962 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/RunningShells.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/RunningShells.tsx @@ -5,9 +5,14 @@ import { useClient } from 'hooks/useClient'; // Every shell the pty sidecar is holding, whether or not a panel is pointing at it. // -// A terminal panel keeps its session id across unmounts so reopening re-attaches — which means a panel -// deleted for good leaves its shell running with nothing pointing at it. This is where those become -// visible and killable. Without it the only way to find one was curl. +// A terminal panel keeps its session id across unmounts so reopening re-attaches, and now kills the shell +// when the panel is genuinely closed (`usePanelClose`). This is the backstop for the rest: shells orphaned +// before that landed, ones whose kill request failed, and anything started outside a panel. +// +// `/_officer/…` is the sidecar's own prefix for the routes it exposes to the platform rather than to a +// terminal. The proxy strips `/api/terminal` and forwards the remainder verbatim, so leaving it out — as +// this file did — reaches the sidecar as `/sessions`, which it answers 404 to. That is a panel that always +// read "No shells running" and a kill button that quietly did nothing. type PtySession = { sessionId: string; @@ -40,13 +45,13 @@ export const RunningShells = () => { const { data, isLoading, refetch } = useQuery({ queryKey: ['terminal-sessions'], - queryFn: () => client.get<{ sessions: PtySession[] }>('/terminal/sessions'), + queryFn: () => client.get<{ sessions: PtySession[] }>('/terminal/_officer/sessions'), refetchInterval: 5000, }); const kill = useCallback( async (sessionId: string) => { - await client.delete(`/terminal/sessions/${sessionId}`); + await client.delete(`/terminal/_officer/sessions/${sessionId}`); // The shell dies asynchronously (the sidecar answers on its pty:exit broadcast, not to us), so give // it a beat before asking again rather than showing a row that is already gone. setTimeout(() => queryClient.invalidateQueries({ queryKey: ['terminal-sessions'] }), 300); diff --git a/src/workspaces/officerdev/src/apps/Terminal/TerminalWrapper.tsx b/src/workspaces/officerdev/src/apps/Terminal/TerminalWrapper.tsx index d2078baa..cff55900 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/TerminalWrapper.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/TerminalWrapper.tsx @@ -1,38 +1,14 @@ -import { useCallback, useEffect } from 'react'; +import { useCallback } from 'react'; import { useWorkspace } from '../../components/Workspace'; -import { useDashboardState } from 'state/useDashboardState'; import { useGlobal } from 'hooks/useGlobal'; import type { TerminalConnectionState } from './Terminal'; import { TerminalView } from './Terminal'; import { terminalStateKey } from './state-key'; - -const EMPTY_TERMINALS: Record = {}; +import { useTerminalSession } from './use-terminal-session'; export const TerminalWrapper = ({ panelId }: { panelId: string }) => { const { workspace, cwd } = useWorkspace(); - const stateKey = terminalStateKey('terminals', workspace); - const { value: terminals, setValue: setTerminals } = useDashboardState>( - stateKey, - EMPTY_TERMINALS, - ); - const sessionId = terminals[panelId]; - - useEffect(() => { - if (!sessionId) { - const id = crypto.randomUUID?.() ?? Math.random().toString(36).slice(2) + Date.now().toString(36); - setTerminals((prev) => ({ ...prev, [panelId]: id })); - } - }, [panelId, sessionId, setTerminals]); - - // The panel → session mapping deliberately OUTLIVES the mount. It used to be deleted on unmount, so any - // layout or route change generated a fresh uuid on the way back and abandoned the previous shell — alive, - // unreachable, and never killed, because nothing sends pty:close. Keeping it means reopening a terminal - // panel re-attaches to the shell you left running, which is also what makes the sidecar's replay buffer - // worth having. The map is persisted dashboard state, so this survives a reload too. - // - // The cost is that a panel deleted for good leaves its entry behind; the running-shells list is how you - // find and kill those. Killing on unmount is not an option until the panel system can tell a real close - // from an incidental remount. + const sessionId = useTerminalSession(panelId, terminalStateKey('terminals', workspace)); const [, setConnState] = useGlobal(`terminal-conn-${panelId}`, 'disconnected'); const onConnectionChange = useCallback((state: TerminalConnectionState) => setConnState(state), [setConnState]); diff --git a/src/workspaces/officerdev/src/apps/Terminal/use-terminal-session.ts b/src/workspaces/officerdev/src/apps/Terminal/use-terminal-session.ts new file mode 100644 index 00000000..82c65a19 --- /dev/null +++ b/src/workspaces/officerdev/src/apps/Terminal/use-terminal-session.ts @@ -0,0 +1,52 @@ +import { useEffect } from 'react'; + +import { useClient } from 'hooks/useClient'; +import { useDashboardState } from 'state/useDashboardState'; + +import { usePanelClose } from '../../components/Workspace'; + +const EMPTY_TERMINALS: Record = {}; + +const newSessionId = (): string => + crypto.randomUUID?.() ?? Math.random().toString(36).slice(2) + Date.now().toString(36); + +/** + * The shell a terminal panel is attached to: minted on first mount, remembered until the panel is closed. + * + * The `panelId → sessionId` map deliberately outlives the component. It used to be dropped on unmount, so + * any layout or route change minted a fresh uuid and abandoned the previous shell — alive, unreachable, + * and never killed. Keeping it is what makes reopening a terminal re-attach to what you left running, and + * what makes the sidecar's replay buffer worth having. It is persisted dashboard state, so it survives a + * reload too. + * + * What was missing was the other end: a panel closed for good left its shell running with nothing pointing + * at it, and the only way to find one was the running-shells list. `usePanelClose` fires on a real close + * and on nothing else — not a drag, not a swap, not a mobile panel switch — so the shell can finally be + * killed with it. If that request fails the shell is still in the running-shells list, which is where it + * was before this hook existed; a lost shell is worse than a stale row, so the map entry goes either way. + */ +export function useTerminalSession(panelId: string, stateKey: string): string | undefined { + const client = useClient(); + const { value: terminals, setValue: setTerminals } = useDashboardState>( + stateKey, + EMPTY_TERMINALS, + ); + const sessionId = terminals[panelId]; + + useEffect(() => { + if (!sessionId) setTerminals((prev) => (prev[panelId] ? prev : { ...prev, [panelId]: newSessionId() })); + }, [panelId, sessionId, setTerminals]); + + usePanelClose(panelId, () => { + if (!sessionId) return; + setTerminals((prev) => { + const { [panelId]: _drop, ...rest } = prev; + return rest; + }); + client.delete(`/terminal/_officer/sessions/${sessionId}`).catch((err: unknown) => { + console.error(`[terminal] could not kill ${sessionId} after its panel closed`, err); + }); + }); + + return sessionId; +}