diff --git a/src/workspaces/officerdev/src/components/Workspace/PanelSlot.tsx b/src/workspaces/officerdev/src/components/Workspace/PanelSlot.tsx index 2e4c321c..e12ff6b5 100644 --- a/src/workspaces/officerdev/src/components/Workspace/PanelSlot.tsx +++ b/src/workspaces/officerdev/src/components/Workspace/PanelSlot.tsx @@ -138,9 +138,15 @@ const TrafficLights = ({ const { maximizedPanelId, setMaximizedPanelId } = useWorkspace(); const isMaximized = maximizedPanelId === panelId; + // The button says "Close panel" unless this is the only panel left, and it used to call `onClearApp` + // either way — `isLastPanel` reached here only to choose the tooltip. So closing a panel from its own + // chrome was impossible: the panel stayed, emptied, and the only working path was the context menu. + // Worse than cosmetic now that identity lives in the layout — clearing the app drops the `config` that + // named the panel's agent while the panel itself survives to be renamed by the next thing put in it. const handleClose = useCallback(() => { - onClearApp(); - }, [onClearApp]); + if (isLastPanel) onClearApp(); + else onRemove(panelId); + }, [isLastPanel, onClearApp, onRemove, panelId]); const handleRestore = useCallback(() => { setMaximizedPanelId(null); diff --git a/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.test.tsx b/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.test.tsx new file mode 100644 index 00000000..8d27100c --- /dev/null +++ b/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.test.tsx @@ -0,0 +1,225 @@ +import { describe, expect, test, beforeEach } from 'bun:test'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { render, fireEvent, screen } from '@testing-library/react'; +import { globalQueryKey } from 'hooks/useGlobal'; +import type { AppRegistryMap, DashboardState, LayoutGroup, LayoutNode, LayoutPanel } from './types'; +import { countPanels, findPanelApp } from './layout-utils'; +import { WorkspaceView } from './WorkspaceView'; +import { usePanelClose } from './usePanelClose'; + +/** + * `WorkspaceView` owns every mutation of the layout tree, which is now also where panel identity lives — + * a chat panel's `config.agentName` is the address a peer agent is delivered to. Two properties decide + * whether an unattended dashboard survives, and neither had a test: + * + * - **Every write is an updater, never a computed tree.** Two of the paths are not immediate (the + * resize debounce fires 500 ms after the drag, and a window resize fires `onLayout` on every group at + * once), so a write computed from the rendered `layout` can be built on a tree that predates the write + * before it. It then silently undoes it, and the resurrected tree carries an older `config`. + * - **`usePanelClose` fires when a close was *asked for*, and only then** — not on the unmounts that a + * drag, a swap or a mobile switch cause, and not for a workspace you have navigated away from. + * + * Everything below drives the real `WorkspaceView` through the real `WorkspaceRenderer` and `PanelSlot`, + * so the buttons are the buttons. It found one defect on the first run: see "closing a panel". + */ + +type Recorded = { arg: LayoutNode | ((prev: LayoutNode) => LayoutNode); wasUpdater: boolean }; + +const panel = (id: string, appType: string | null = 'probe', config?: Record): LayoutPanel => ({ + type: 'panel', + id, + appType, + ...(config ? { config } : {}), +}); + +const group = (id: string, children: LayoutNode[]): LayoutGroup => ({ + type: 'group', + id, + direction: 'horizontal', + children: children.map((node) => ({ node, size: 100 / children.length })), +}); + +const ProbeIcon = ((props: Record) => ) as AppRegistryMap[string]['icon']; + +// Registered close handlers, by panel id, so a test can say what a panel does when it is retired. +const closeBehaviour = new Map void>(); +const closesFired: string[] = []; + +const Probe = ({ panelId }: { panelId: string }) => { + usePanelClose(panelId, () => { + closesFired.push(panelId); + closeBehaviour.get(panelId)?.(); + }); + return
; +}; + +const registry: AppRegistryMap = { + probe: { name: 'Probe', icon: ProbeIcon, component: Probe }, + other: { name: 'Other', icon: ProbeIcon, component: Probe }, +}; + +let queryClient: QueryClient; +let writes: Recorded[]; + +/** + * A workspace that behaves like the real one: `setValue` accepts either shape, applies it to the *current* + * tree, and re-renders. Recording whether each write arrived as a function is the whole point — a computed + * tree here is the §5.5 lost update, and it is invisible in the resulting layout when only one write is in + * flight. + */ +function mount(initial: LayoutNode, props: Partial[0]> = {}) { + let current = initial; + let view: ReturnType | null = null; + writes = []; + + const workspace = (): DashboardState => ({ + key: 'dashboard:test', + value: current, + isLoaded: true, + setValue: (update) => { + writes.push({ arg: update, wasUpdater: typeof update === 'function' }); + current = typeof update === 'function' ? (update as (prev: LayoutNode) => LayoutNode)(current) : update; + // A write raised from an effect during the very first render arrives before `render` has returned. + // The tree it produced is already the one on screen for `current`; re-rendering is what a real + // `useDashboardState` would do next, and there is nothing to re-render into yet. + view?.rerender(tree()); + }, + }); + + const tree = () => ( + + + + ); + + view = render(tree()); + return { layout: () => current, rerender: () => view?.rerender(tree()) }; +} + +beforeEach(() => { + closeBehaviour.clear(); + closesFired.length = 0; + queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + // `useAppRegistry` reads a `useGlobal` key that `seedAppRegistry` writes before anything renders. Seeding + // the cache directly is the same thing without dragging in the real app list. + queryClient.setQueryData(globalQueryKey('APP_REGISTRY'), registry); +}); + +describe('every write is an updater, not a computed tree', () => { + test('closing a panel writes a function', () => { + mount(group('g', [panel('a'), panel('b')])); + + fireEvent.click(screen.getAllByTitle('Close panel')[0]!); + + expect(writes.length).toBe(1); + expect(writes[0]!.wasUpdater).toBe(true); + }); + + test('two writes compose, even though the second was raised from a stale render', () => { + // The lost-update shape, reproduced deliberately: both callbacks are captured from the same rendered + // tree, and the second is invoked without the first's render having been observed. Composed through + // updaters, `b` is still gone when `a`'s app is cleared. + const view = mount(group('g', [panel('a'), panel('b'), panel('c')])); + const closeButtons = screen.getAllByTitle('Close panel'); + + fireEvent.click(closeButtons[1]!); // remove b + fireEvent.click(closeButtons[0]!); // remove a, from a handler made before b was gone + + expect(countPanels(view.layout())).toBe(1); + expect(writes.every((w) => w.wasUpdater)).toBe(true); + }); +}); + +describe('closing a panel', () => { + test('the last panel is cleared, not removed — there is nothing to fall back to', () => { + const view = mount(panel('a')); + + fireEvent.click(screen.getByTitle('Clear app')); + + expect(countPanels(view.layout())).toBe(1); + expect(findPanelApp(view.layout(), 'a')).toBe(null); + }); + + test('a panel that is NOT the last is removed', () => { + // Regression pin. `TrafficLights` took `onRemove` and `isLastPanel` and used `isLastPanel` only to + // choose the tooltip: the red button said "Close panel" and called `onClearApp` regardless, so it + // emptied the panel and left the box behind. Closing a panel from its own chrome was impossible — + // the only working path was the context menu — and an emptied panel is worse than a cosmetic one + // here, because clearing the app drops the `config` that named the agent while the panel survives. + const view = mount(group('g', [panel('a'), panel('b')])); + + fireEvent.click(screen.getAllByTitle('Close panel')[0]!); + + expect(countPanels(view.layout())).toBe(1); + }); + + test('a locked workspace offers no close at all', () => { + mount(group('g', [panel('a'), panel('b')]), { locked: true }); + + expect(screen.queryAllByTitle('Close panel')).toEqual([]); + expect(screen.queryAllByTitle('Clear app')).toEqual([]); + expect(screen.getAllByTitle('Maximize').length).toBe(2); + }); +}); + +describe('usePanelClose fires on intent, and only on intent', () => { + test('removing a panel retires it', () => { + mount(group('g', [panel('a'), panel('b')])); + + fireEvent.click(screen.getAllByTitle('Close panel')[0]!); + + expect(closesFired).toEqual(['a']); + }); + + test('clearing the last panel retires it too — the app went away', () => { + mount(panel('a')); + + fireEvent.click(screen.getByTitle('Clear app')); + + expect(closesFired).toEqual(['a']); + }); + + test('a handler that throws does not stop the panel from closing', () => { + // The layout write is the user's instruction; the cleanup is best effort on top of it. + const view = mount(group('g', [panel('a'), panel('b')])); + closeBehaviour.set('a', () => { + throw new Error('release failed'); + }); + + fireEvent.click(screen.getAllByTitle('Close panel')[0]!); + + expect(countPanels(view.layout())).toBe(1); + }); + + test('a handler fires once and is not re-run by a later close of another panel', () => { + const view = mount(group('g', [panel('a'), panel('b'), panel('c')])); + const buttons = screen.getAllByTitle('Close panel'); + + fireEvent.click(buttons[0]!); + fireEvent.click(screen.getAllByTitle('Close panel')[0]!); + + expect(closesFired.length).toBe(2); + expect(new Set(closesFired).size).toBe(2); + expect(countPanels(view.layout())).toBe(1); + }); +}); + +describe('appTypes repairs a persisted layout, once', () => { + test('an appType the screen no longer renders is replaced and the repair is written back', () => { + const view = mount(group('g', [panel('a', 'probe'), panel('b', 'a-deleted-app')]), { + appTypes: { allowed: ['probe'], fallback: 'probe' }, + }); + + expect(findPanelApp(view.layout(), 'b')).toBe('probe'); + // Exactly one write: the normalised tree normalises to itself, so the effect does not re-fire. + expect(writes.length).toBe(1); + }); + + test('a layout that is already legal is not written at all', () => { + mount(group('g', [panel('a', 'probe'), panel('b', 'probe')]), { + appTypes: { allowed: ['probe'], fallback: 'probe' }, + }); + + expect(writes).toEqual([]); + }); +}); diff --git a/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.tsx b/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.tsx index d2516a59..4063092e 100644 --- a/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.tsx +++ b/src/workspaces/officerdev/src/components/Workspace/WorkspaceView.tsx @@ -235,10 +235,19 @@ export const WorkspaceView = ({ workspace, locked, appTypes, cwd = '~', root, co const panel = ephemeralPanelRef.current; if (!panel) return; - if (isEphemeralOpen) { - panel.resize(100 - (ephemeral.defaultBaseSize ?? 40)); - } else { - panel.collapse(); + // `react-resizable-panels` asserts rather than no-ops when it is asked to resize a group it has not + // laid out yet — and an assert thrown from an effect aborts the commit, which here means the whole + // dashboard rather than the pane that could not be sized. The pane is a file preview; the panels + // behind it are a night's work. Only the ephemeral pane's own geometry is at stake, and the next + // render tries again. + try { + if (isEphemeralOpen) { + panel.resize(100 - (ephemeral.defaultBaseSize ?? 40)); + } else { + panel.collapse(); + } + } catch (err) { + console.warn('[workspace] could not size the ephemeral pane yet', err); } }, [isEphemeralOpen, ephemeral?.defaultBaseSize]);