diff --git a/src/workspaces/officerdev/src/components/Workspace/WorkspaceContext.ts b/src/workspaces/officerdev/src/components/Workspace/WorkspaceContext.ts index 93de7ac8..4606be2a 100644 --- a/src/workspaces/officerdev/src/components/Workspace/WorkspaceContext.ts +++ b/src/workspaces/officerdev/src/components/Workspace/WorkspaceContext.ts @@ -13,6 +13,10 @@ type WorkspaceContextValue = { // panel that owns it — apps read this through `usePanelConfig`, not directly. panelConfigs: Record; setPanelConfig: (panelId: string, config: PanelConfig | undefined) => void; + // Say what to do when this panel is deliberately closed. Apps call it through `usePanelClose`, never + // directly. Registering twice for a panel id replaces the handler; nothing ever unregisters, because + // "this component went away" is the signal we are specifically refusing to trust. + registerPanelClose: (panelId: string, handler: () => void) => void; swapSourceId: string | null; setSwapSourceId: (id: string | null) => void; onSwap: (sourceId: string, targetId: string) => void; @@ -42,6 +46,7 @@ const noop = () => {}; */ export const inertInteraction = { setPanelConfig: noop, + registerPanelClose: noop, swapSourceId: null, setSwapSourceId: noop, onSwap: noop, diff --git a/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.tsx b/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.tsx index ed2e4942..9425357a 100644 --- a/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.tsx +++ b/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.tsx @@ -5,7 +5,7 @@ import { useIsMobile } from 'hooks/useIsMobile'; import { useSessionState } from 'hooks/useSessionState'; import type { LayoutNode, DashboardState, EphemeralPanels, PanelComponents, PanelConfig } from './types'; import type { DropPosition } from './layout-utils'; -import { splitPanel, removePanel, setApp, updateSizes, swapPanels, movePanel, countPanels, setZoom, setPanelConfig, collectPanelConfigs } from './layout-utils'; +import { splitPanel, removePanel, setApp, updateSizes, swapPanels, movePanel, countPanels, setZoom, setPanelConfig, collectPanelConfigs, findPanelApp } from './layout-utils'; import { WorkspaceProvider } from './WorkspaceContext'; import { parseWorkspaceKey } from './workspace-identity'; import { WorkspaceRenderer } from './WorkspaceRenderer'; @@ -54,6 +54,37 @@ export const WorkspaceView = ({ workspace, locked, cwd = '~', root, components, } }, [maximizedPanelId]); + // What each panel wants done when it is closed, by panel id, stamped with the workspace it was + // registered on. A ref rather than state: nothing renders from it, and a panel registering during its + // own mount effect must not schedule a render of the whole workspace. + // + // Nothing is ever unregistered. A handler is dropped when it fires and replaced when the same panel id + // registers again; a panel that merely unmounted keeps its handler, because "unmounted" is precisely + // the signal this exists to avoid trusting. The stamp is what keeps that from leaking across + // dashboards — the same panel id can exist on two of them, and a handler from the one you navigated + // away from must not fire for the one you are on. + const closeHandlers = useRef(new Map void }>()); + const workspaceKeyRef = useRef(workspace.key); + workspaceKeyRef.current = workspace.key; + + const registerPanelClose = useCallback((panelId: string, run: () => void) => { + closeHandlers.current.set(panelId, { workspaceKey: workspaceKeyRef.current, run }); + }, []); + + const firePanelClose = useCallback((panelId: string) => { + const entry = closeHandlers.current.get(panelId); + if (!entry) return; + closeHandlers.current.delete(panelId); + if (entry.workspaceKey !== workspaceKeyRef.current) return; + // A handler that throws must not stop the panel from closing — the layout write is the user's + // instruction, and the cleanup is a best effort on top of it. + try { + entry.run(); + } catch (err) { + console.error(`panel close handler for ${panelId} threw`, err); + } + }, []); + // Every mutation goes through `onLayoutChange` as an UPDATER, never as a computed tree. // // Each of these used to close over `layout` as it was when the callback was made, and two of the paths @@ -66,11 +97,18 @@ export const WorkspaceView = ({ workspace, locked, cwd = '~', root, components, // // The guards below still test the rendered `layout` — they only decide whether a write is worth making, // and being one render stale there costs a redundant no-op write at worst. + // + // `handleSetApp` and `handleRemove` additionally fire the closing panel's `usePanelClose` handler. + // They are the only two places that do, and they are the only two that know a close was *asked for* — + // a drag, a swap and a mobile panel switch all tear a panel's component down without closing anything. const handleSetApp = useCallback( (panelId: string, appType: string | null) => { + // Re-picking the app that is already there is not a close: `setApp` keeps the config and the app + // never goes away. Every other transition retires whatever was running. + if (findPanelApp(layout, panelId) !== appType) firePanelClose(panelId); onLayoutChange((prev) => setApp(prev, panelId, appType)); }, - [onLayoutChange], + [layout, onLayoutChange, firePanelClose], ); const handleSplit = useCallback( @@ -83,9 +121,15 @@ export const WorkspaceView = ({ workspace, locked, cwd = '~', root, components, const handleRemove = useCallback( (panelId: string) => { if (countPanels(layout) <= 1) return; + // Fired before the write, from the rendered tree, rather than from inside the updater — an updater + // has to stay pure and may run twice. The updater's own guard can therefore decline a removal we + // already announced, if a concurrent write shrank the tree to one panel in between. That costs a + // cleanup for a panel still on screen; it needs two writes to race inside one debounce window, and + // the alternative is a side effect in an updater. + firePanelClose(panelId); onLayoutChange((prev) => (countPanels(prev) <= 1 ? prev : removePanel(prev, panelId))); }, - [layout, onLayoutChange], + [layout, onLayoutChange, firePanelClose], ); const handleResized = useCallback( @@ -191,6 +235,7 @@ export const WorkspaceView = ({ workspace, locked, cwd = '~', root, components, root, panelConfigs, setPanelConfig: locked ? noop : handleSetPanelConfig, + registerPanelClose, swapSourceId, setSwapSourceId, onSwap: handleSwap, diff --git a/src/workspaces/officerdev/src/components/Workspace/index.ts b/src/workspaces/officerdev/src/components/Workspace/index.ts index 4acb5483..e29d1fe3 100644 --- a/src/workspaces/officerdev/src/components/Workspace/index.ts +++ b/src/workspaces/officerdev/src/components/Workspace/index.ts @@ -26,10 +26,12 @@ export { hasAnyApp, setPanelConfig, collectPanelConfigs, + findPanelApp, } from './layout-utils'; export type { WorkspaceIdentity } from './workspace-identity'; export { parseWorkspaceKey } from './workspace-identity'; export { WorkspaceProvider, useWorkspace } from './WorkspaceContext'; export { usePanelConfig } from './usePanelConfig'; +export { usePanelClose } from './usePanelClose'; export { WorkspaceView } from './WorkspaceView'; export { WorkspaceLayout } from './WorkspaceLayout'; diff --git a/src/workspaces/officerdev/src/components/Workspace/layout-utils.test.ts b/src/workspaces/officerdev/src/components/Workspace/layout-utils.test.ts index 09b4c8dd..69ced410 100644 --- a/src/workspaces/officerdev/src/components/Workspace/layout-utils.test.ts +++ b/src/workspaces/officerdev/src/components/Workspace/layout-utils.test.ts @@ -4,6 +4,7 @@ import { clampZoom, collectPanelConfigs, countPanels, + findPanelApp, hasAnyApp, movePanel, pruneEmptyPanels, @@ -527,3 +528,23 @@ describe('panel ids do not survive a rearrangement', () => { expect(find(next, 'p2')!.appType).toBe('x'); }); }); + +// `WorkspaceView` asks this before firing a panel's close handler, so the three answers have to stay +// distinguishable: an app, no app, and no such panel. Collapsing the last two would close an empty panel +// on every app pick. +describe('findPanelApp', () => { + const tree = group('g', 'horizontal', [panel('p1', 'terminal'), panel('p2')]); + + test('reports the app a panel is running', () => { + expect(findPanelApp(tree, 'p1')).toBe('terminal'); + }); + + test('distinguishes an empty panel from a missing one', () => { + expect(findPanelApp(tree, 'p2')).toBeNull(); + expect(findPanelApp(tree, 'nope')).toBeUndefined(); + }); + + test('finds a panel that is the whole tree', () => { + expect(findPanelApp(panel('only', 'chat'), 'only')).toBe('chat'); + }); +}); diff --git a/src/workspaces/officerdev/src/components/Workspace/layout-utils.ts b/src/workspaces/officerdev/src/components/Workspace/layout-utils.ts index b45a9c95..40196c86 100644 --- a/src/workspaces/officerdev/src/components/Workspace/layout-utils.ts +++ b/src/workspaces/officerdev/src/components/Workspace/layout-utils.ts @@ -254,6 +254,15 @@ export function setPanelConfig(root: LayoutNode, panelId: string, config: PanelC return setContents(root, panelId, { appType: current.appType, config }); } +/** + * Which app a panel is running right now. `null` is a real answer (an empty panel); `undefined` means + * there is no panel with that id. Callers that only want to know whether something changed can compare + * the result directly, since both absences are distinct from any app key. + */ +export function findPanelApp(node: LayoutNode, panelId: string): string | null | undefined { + return findPanelContents(node, panelId)?.appType; +} + /** Every configured panel in the tree, by id — the reactive view apps read through `usePanelConfig`. */ export function collectPanelConfigs(node: LayoutNode): Record { const out: Record = {}; diff --git a/src/workspaces/officerdev/src/components/Workspace/usePanelClose.ts b/src/workspaces/officerdev/src/components/Workspace/usePanelClose.ts new file mode 100644 index 00000000..dae8f442 --- /dev/null +++ b/src/workspaces/officerdev/src/components/Workspace/usePanelClose.ts @@ -0,0 +1,38 @@ +import { useEffect, useRef } from 'react'; + +import { useWorkspace } from './WorkspaceContext'; + +/** + * Say what to do when this panel is deliberately closed — removed from the workspace, or handed to a + * different app. Use it to release whatever the panel owns on the server: a pty, a lock, a job. + * + * **This is not an unmount hook, and it is deliberately not one.** A panel unmounts when it is dragged + * to a new position, when the mobile view switches panels, when a parent re-renders it under a new key + * — all of which look identical to a close from inside the component. `movePanel` even mints a new panel + * id on the way (see `layout-utils.test.ts`), so the layout cannot be asked either. Only the two call + * sites that carry the intent can answer, so only they fire this. The cost of getting it wrong is a + * killed terminal or a dropped session, which is why the framework refuses to guess. + * + * Consequences worth knowing before you use it: + * + * - The handler runs *while the panel is being torn down*, so it must not set React state. Call an API, + * close a socket, drop a map entry — nothing that expects a render afterwards. + * - Registration is never undone. A component that unmounts leaves its handler in place, because + * "unmounted" is exactly the signal we are refusing to trust; re-registering for the same panel id + * replaces it. Handlers are scoped to the workspace they were registered on, and one only ever fires + * for the panel id it was registered under. + * - `handler` does not need to be memoised — the latest one always runs. + * - Nothing fires in a locked or inert workspace, where no panel can be closed in the first place. + * + * A panel that is a *pointer* to server-side state rather than the owner of it should not use this at + * all. Closing a chat panel must not delete the session it points at. + */ +export function usePanelClose(panelId: string, handler: () => void): void { + const { registerPanelClose } = useWorkspace(); + const latest = useRef(handler); + latest.current = handler; + + useEffect(() => { + registerPanelClose(panelId, () => latest.current()); + }, [panelId, registerPanelClose]); +}