give panels a close signal that only fires on a real close

A panel that owns something on the server — a pty, a lock — has had no way to be told it was closed. `TerminalWrapper` says so in a comment: it cannot kill on unmount, because a drag, a swap, a mobile panel switch and a genuine close are the same event from inside the component. So the signal is raised where the intent is, not where the teardown is. `usePanelClose(panelId, fn)` registers a handler; `WorkspaceView` fires it from `handleRemove` and from `handleSetApp` when the app actually changes, and from nowhere else. Registration is deliberately never torn down — "unmounted" is the ambiguous signal being replaced, so honouring it would reintroduce the bug — and handlers are stamped with the workspace they were registered on so one dashboard's panel id cannot fire another's. `findPanelApp` is what tells a real app change from re-picking the app already there, which `setApp` treats as a no-op; without it every pick from the app menu would close a panel that never closed. No app uses the hook yet. The terminals come next; a chat panel deliberately never will, since a chat panel is a pointer to a server-side session and closing the window must not delete what it points at.
2026-08-07 09:52:40 +00:00
parent 04371a9d99
commit 198dc71137
6 changed files with 123 additions and 3 deletions
@@ -13,6 +13,10 @@ type WorkspaceContextValue = {
  // panel that owns it — apps read this through `usePanelConfig`, not directly.
  panelConfigs: Record<string, PanelConfig>;
  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,
@@ -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<string, { workspaceKey: string; run: () => 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,
@@ -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';
@@ -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');
+  });
+});
@@ -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<string, PanelConfig> {
  const out: Record<string, PanelConfig> = {};
@@ -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]);
+}