From dbe585fd72dc9d52743d8a0f7454279ecb90410d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 7 Aug 2026 09:29:34 +0000 Subject: [PATCH] stop handing apps a key to reverse-engineer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dashboardId` in the workspace context was `workspace.key` — `ws-layout-` or `screens/` — and three apps parsed its format to work out what they were mounted on. It is now `workspace`, a `{kind, id, key}` parsed once by the framework. The key survives on the result and is still what gets stored: `agent_panels.dashboard_id` holds it, so the wire value is byte-identical and no named agent orphans. `kind` and `id` are for deciding. Two behaviour changes fall out. An unrecognised key is no longer treated as a dashboard — the old `!startsWith('screens/')` test called anything that was not a screen a dashboard, which would have let a panel register an agent against a workspace with no row to hang it on. And `ChatPanelWrapper`'s `dashboardId === 'email'` branch is gone: it compared against a bare id no producer ever emits, because the only writer is `WorkspaceView` and the only other one, `WorkspaceLayout`'s `dashboardId` prop, was passed by zero callers. That prop is deleted. Also here because it is the same defect as b0a32ae one file over: `WorkspaceLayout`'s resize handler computed a tree from a captured `layout` and `WorkspaceRenderer` debounces it 500 ms. Updater now. --- .../src/apps/Chat/ChatPanelWrapper.tsx | 11 ++-- .../officerdev/src/apps/Chat/useAgentPanel.ts | 21 ++++--- .../apps/Terminal/CommandTerminalWrapper.tsx | 4 +- .../src/apps/Terminal/HostTerminalWrapper.tsx | 4 +- .../src/apps/Terminal/TerminalWrapper.tsx | 4 +- .../officerdev/src/apps/Terminal/state-key.ts | 16 +++--- .../components/Workspace/WorkspaceContext.ts | 6 +- .../components/Workspace/WorkspaceLayout.tsx | 16 ++++-- .../components/Workspace/WorkspaceView.tsx | 9 ++- .../src/components/Workspace/index.ts | 2 + .../Workspace/workspace-identity.test.ts | 55 +++++++++++++++++++ .../Workspace/workspace-identity.ts | 41 ++++++++++++++ 12 files changed, 155 insertions(+), 34 deletions(-) create mode 100644 src/workspaces/officerdev/src/components/Workspace/workspace-identity.test.ts create mode 100644 src/workspaces/officerdev/src/components/Workspace/workspace-identity.ts diff --git a/src/workspaces/officerdev/src/apps/Chat/ChatPanelWrapper.tsx b/src/workspaces/officerdev/src/apps/Chat/ChatPanelWrapper.tsx index 236b428a..864b2bfe 100644 --- a/src/workspaces/officerdev/src/apps/Chat/ChatPanelWrapper.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/ChatPanelWrapper.tsx @@ -56,14 +56,17 @@ const ChatPanelInner = ({ type ChatPanelWrapperProps = { panelId: string }; export const ChatPanelWrapper = ({ panelId }: ChatPanelWrapperProps) => { - const { dashboardId, cwd, root, promptPrefix } = useWorkspace(); + const { workspace, cwd, root, promptPrefix } = useWorkspace(); const scoped = cwd !== '~'; + // `contextId` stays the raw workspace key: it is the same string `agent_panels.dashboard_id` holds, and + // a stored address should not be two different strings depending on which feature wrote it. The bare id + // is for deciding, which is what `kind` and `id` are doing above it. const chatContext = - dashboardId === 'email' || dashboardId === 'screens/email' + workspace?.kind === 'screen' && workspace.id === 'email' ? { context: 'email' as const } - : dashboardId && !dashboardId.startsWith('screens/') - ? { context: 'dashboard' as const, contextId: dashboardId } + : workspace?.kind === 'dashboard' + ? { context: 'dashboard' as const, contextId: workspace.key } : {}; const agentPanel = useAgentPanel(panelId); diff --git a/src/workspaces/officerdev/src/apps/Chat/useAgentPanel.ts b/src/workspaces/officerdev/src/apps/Chat/useAgentPanel.ts index 383b02f8..23482c71 100644 --- a/src/workspaces/officerdev/src/apps/Chat/useAgentPanel.ts +++ b/src/workspaces/officerdev/src/apps/Chat/useAgentPanel.ts @@ -34,11 +34,16 @@ type AgentPanelConfig = { agentName?: string }; * exactly as before this existed. */ export function useAgentPanel(panelId: string) { - const { dashboardId, cwd } = useWorkspace(); + const { workspace, cwd } = useWorkspace(); const [config, setConfig] = usePanelConfig(panelId); const agentName = config?.agentName; - const addressable = !!dashboardId && !dashboardId.startsWith('screens/'); + // The address book is keyed on the workspace *key*, which is what the row stores. Only a real dashboard + // is addressable: a screen is not a place you assemble a team, and an unrecognised key is not a place at + // all — `parseWorkspaceKey` returns null for it rather than calling it a dashboard, as the old + // `!startsWith('screens/')` test did. + const dashboardKey = workspace?.kind === 'dashboard' ? workspace.key : null; + const addressable = dashboardKey !== null; // `useClient()` rebuilds its verbs on every render, so a verb in a dependency list changes identity // every render and silently re-runs whatever depends on it. Hold it in a ref instead. @@ -47,13 +52,13 @@ export function useAgentPanel(panelId: string) { clientRef.current = client; const queryClient = useQueryClient(); - const queryKey = ['agent-panels', dashboardId]; + const queryKey = ['agent-panels', dashboardKey]; const { data, isLoading } = useQuery({ queryKey, queryFn: () => clientRef.current.get<{ agents: AgentPanelView[] }>( - `/chat/agent-panels?dashboardId=${encodeURIComponent(dashboardId ?? '')}`, + `/chat/agent-panels?dashboardId=${encodeURIComponent(dashboardKey ?? '')}`, ), enabled: addressable, staleTime: 30_000, @@ -75,13 +80,13 @@ export function useAgentPanel(panelId: string) { reanchoredRef.current = `${agent.id}:${panelId}`; void clientRef.current .patch<{ agent: AgentPanelView }>(`/chat/agent-panels/${agent.id}`, { panelId }) - .then(() => queryClient.invalidateQueries({ queryKey: ['agent-panels', dashboardId] })) + .then(() => queryClient.invalidateQueries({ queryKey: ['agent-panels', dashboardKey] })) .catch(() => { // Non-fatal: the panel already knows its own name, which is the address that matters. Allow a // later render to try again rather than pinning the failed attempt. reanchoredRef.current = null; }); - }, [agent?.id, agent?.panelId, panelId, dashboardId, queryClient]); + }, [agent?.id, agent?.panelId, panelId, dashboardKey, queryClient]); const claim = useMutation({ mutationFn: (input: { name: string; cwd?: string; rolePrompt?: string }) => { @@ -92,7 +97,7 @@ export function useAgentPanel(panelId: string) { return Promise.reject(new Error(`There is already an agent named "${input.name}" on this dashboard`)); } return clientRef.current.post<{ agent: AgentPanelView; created: boolean }>('/chat/agent-panels', { - dashboardId, + dashboardId: dashboardKey, panelId, // Record the directory the panel is already scoped to. A row with no cwd runs incoming handoffs // in the owner's home while the human's own turns run in the dashboard's directory — the same @@ -105,7 +110,7 @@ export function useAgentPanel(panelId: string) { // Write the name into the panel before invalidating: the config is what survives a drag, and a // refetch that landed first would show an agent this panel does not yet claim. setConfig({ agentName: created.name }); - void queryClient.invalidateQueries({ queryKey: ['agent-panels', dashboardId] }); + void queryClient.invalidateQueries({ queryKey: ['agent-panels', dashboardKey] }); }, }); diff --git a/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx b/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx index 03e16f07..bbef2a49 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx @@ -15,8 +15,8 @@ type CommandTerminalWrapperProps = { }; export const CommandTerminalWrapper = ({ panelId, command, statePrefix }: CommandTerminalWrapperProps) => { - const { dashboardId, cwd } = useWorkspace(); - const stateKey = terminalStateKey(statePrefix, dashboardId); + const { workspace, cwd } = useWorkspace(); + const stateKey = terminalStateKey(statePrefix, workspace); const { value: terminals, setValue: setTerminals } = useDashboardState>( stateKey, EMPTY_TERMINALS, diff --git a/src/workspaces/officerdev/src/apps/Terminal/HostTerminalWrapper.tsx b/src/workspaces/officerdev/src/apps/Terminal/HostTerminalWrapper.tsx index e683f030..d0fb608f 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/HostTerminalWrapper.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/HostTerminalWrapper.tsx @@ -9,8 +9,8 @@ import { terminalStateKey } from './state-key'; const EMPTY_TERMINALS: Record = {}; export const HostTerminalWrapper = ({ panelId }: { panelId: string }) => { - const { dashboardId, cwd } = useWorkspace(); - const stateKey = terminalStateKey('host-terminals', dashboardId); + const { workspace, cwd } = useWorkspace(); + const stateKey = terminalStateKey('host-terminals', workspace); const { value: terminals, setValue: setTerminals } = useDashboardState>( stateKey, EMPTY_TERMINALS, diff --git a/src/workspaces/officerdev/src/apps/Terminal/TerminalWrapper.tsx b/src/workspaces/officerdev/src/apps/Terminal/TerminalWrapper.tsx index ecbcded1..d2078baa 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/TerminalWrapper.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/TerminalWrapper.tsx @@ -9,8 +9,8 @@ import { terminalStateKey } from './state-key'; const EMPTY_TERMINALS: Record = {}; export const TerminalWrapper = ({ panelId }: { panelId: string }) => { - const { dashboardId, cwd } = useWorkspace(); - const stateKey = terminalStateKey('terminals', dashboardId); + const { workspace, cwd } = useWorkspace(); + const stateKey = terminalStateKey('terminals', workspace); const { value: terminals, setValue: setTerminals } = useDashboardState>( stateKey, EMPTY_TERMINALS, diff --git a/src/workspaces/officerdev/src/apps/Terminal/state-key.ts b/src/workspaces/officerdev/src/apps/Terminal/state-key.ts index a1859dad..b6b951b5 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/state-key.ts +++ b/src/workspaces/officerdev/src/apps/Terminal/state-key.ts @@ -1,16 +1,18 @@ +import type { WorkspaceIdentity } from '../../components/Workspace'; + /** * The dashboard-state key a terminal panel stores its `panelId → sessionId` map under. * - * `useWorkspace().dashboardId` is the workspace *key*, not an id — `ws-layout-` for a dashboard and - * `screens/` for a screen. Three wrappers derived a key from it by three different rules, and two of - * them left the prefix on: `ws-host-terminals-ws-layout-my-dash` parses back out as a dashboard called + * Three wrappers used to derive this from the raw workspace key by three different rules, and two of them + * left the prefix on: `ws-host-terminals-ws-layout-my-dash` parses back out as a dashboard called * `ws-layout-my-dash`, which the server then created, and which showed up in the Dashboards list as a real - * dashboard. One rule, in one place, is the fix for that class. + * dashboard. One rule, in one place, is the fix for that class — and the framework now hands over the + * parsed id rather than a string to re-parse, so there is nothing left here to get wrong. * * Screens fall back to `-default`, which is the behaviour `TerminalWrapper` already had: a screen has no * dashboard row to hang the map on, and panel ids are unique across the app. */ -export function terminalStateKey(prefix: string, dashboardId: string | null): string { - const id = dashboardId?.match(/^ws-layout-(.+)$/)?.[1]; - return `ws-${prefix}-${id ?? 'default'}`; +export function terminalStateKey(prefix: string, workspace: WorkspaceIdentity | null): string { + const id = workspace?.kind === 'dashboard' ? workspace.id : 'default'; + return `ws-${prefix}-${id}`; } diff --git a/src/workspaces/officerdev/src/components/Workspace/WorkspaceContext.ts b/src/workspaces/officerdev/src/components/Workspace/WorkspaceContext.ts index 9c236031..e6920ba7 100644 --- a/src/workspaces/officerdev/src/components/Workspace/WorkspaceContext.ts +++ b/src/workspaces/officerdev/src/components/Workspace/WorkspaceContext.ts @@ -2,6 +2,7 @@ import { createContext, useContext } from 'react'; import type { DropPosition } from './layout-utils'; import type { PanelConfig } from './types'; +import type { WorkspaceIdentity } from './workspace-identity'; export type DefaultFileSort = { field: 'name' | 'size' | 'type' | 'date'; @@ -9,7 +10,8 @@ export type DefaultFileSort = { }; type WorkspaceContextValue = { - dashboardId: string | null; + /** Which dashboard or screen this panel is on. Null in a preview or a settings pane. */ + workspace: WorkspaceIdentity | null; cwd: string; root?: string; initialFilePath?: string; @@ -36,7 +38,7 @@ type WorkspaceContextValue = { const noop = () => {}; const WorkspaceContext = createContext({ - dashboardId: null, + workspace: null, cwd: '~', panelConfigs: {}, setPanelConfig: noop, diff --git a/src/workspaces/officerdev/src/components/Workspace/WorkspaceLayout.tsx b/src/workspaces/officerdev/src/components/Workspace/WorkspaceLayout.tsx index 6c5d2874..42cd9ffc 100644 --- a/src/workspaces/officerdev/src/components/Workspace/WorkspaceLayout.tsx +++ b/src/workspaces/officerdev/src/components/Workspace/WorkspaceLayout.tsx @@ -7,10 +7,9 @@ import { useAppRegistry } from '../../AppRegistry/useAppRegistry'; type WorkspaceLayoutProps = { layout: LayoutNode; - onLayoutChange: (layout: LayoutNode) => void; + onLayoutChange: (layout: LayoutNode | ((prev: LayoutNode) => LayoutNode)) => void; registry?: AppRegistryMap; components?: PanelComponents; - dashboardId?: string; cwd?: string; promptPrefix?: string; noHeader?: boolean; @@ -21,19 +20,24 @@ type WorkspaceLayoutProps = { const noop = () => {}; -export const WorkspaceLayout = ({ layout, onLayoutChange, registry: registryProp, components, dashboardId, cwd, promptPrefix, noHeader, isMobile, mobilePanelId, onMobileBack }: WorkspaceLayoutProps) => { +export const WorkspaceLayout = ({ layout, onLayoutChange, registry: registryProp, components, cwd, promptPrefix, noHeader, isMobile, mobilePanelId, onMobileBack }: WorkspaceLayoutProps) => { const { registry: globalRegistry } = useAppRegistry(); const registry = registryProp ?? globalRegistry; + // An updater, for the same reason `WorkspaceView`'s eight mutations are: `WorkspaceRenderer` debounces + // this by 500 ms, so a tree computed here is 500 ms stale by the time it is written. const handleResized = useCallback( (groupId: string, sizes: number[]) => { - onLayoutChange(updateSizes(layout, groupId, sizes)); + onLayoutChange((prev) => updateSizes(prev, groupId, sizes)); }, - [layout, onLayoutChange], + [onLayoutChange], ); + // No workspace identity: this is the locked/preview renderer, used for job detail panes, settings and + // dashboard previews. It is not a dashboard and not a screen, and a panel that asks should be told so + // rather than handed something that parses. return ( - + collectPanelConfigs(layout), [layout]); + // Memoised on the key, not recomputed inline: the provider value is a fresh object every render + // anyway, but this one ends up in consumers' dependency arrays (`useAgentPanel`'s query key, the + // terminal state key). A new identity per render is how `useClient()` silently disabled every + // playback report in the Jellyfin player — same shape, so don't reproduce it here. + const identity = useMemo(() => parseWorkspaceKey(workspace.key), [workspace.key]); + const handleSetPanelConfig = useCallback( (panelId: string, config: PanelConfig | undefined) => { if (setPanelConfig(layout, panelId, config) === layout) return; @@ -183,7 +190,7 @@ export const WorkspaceView = ({ workspace, locked, cwd = '~', root, initialFileP return ( { + test('reads a dashboard key', () => { + expect(parseWorkspaceKey('ws-layout-agent-mvp')).toEqual({ + kind: 'dashboard', + id: 'agent-mvp', + key: 'ws-layout-agent-mvp', + }); + }); + + test('reads a screen key', () => { + expect(parseWorkspaceKey('screens/email')).toEqual({ kind: 'screen', id: 'email', key: 'screens/email' }); + }); + + // Both halves may contain dashes and slashes; only the first prefix is structural. + test('keeps the whole remainder as the id', () => { + expect(parseWorkspaceKey('ws-layout-my-dash-2')?.id).toBe('my-dash-2'); + expect(parseWorkspaceKey('screens/soulseek-v2')?.id).toBe('soulseek-v2'); + }); + + test('is null for anything that is neither', () => { + expect(parseWorkspaceKey(null)).toBeNull(); + expect(parseWorkspaceKey(undefined)).toBeNull(); + expect(parseWorkspaceKey('')).toBeNull(); + expect(parseWorkspaceKey('email')).toBeNull(); + // The prefixes alone name nothing. + expect(parseWorkspaceKey('ws-layout-')).toBeNull(); + expect(parseWorkspaceKey('screens/')).toBeNull(); + }); +}); + +describe('terminalStateKey', () => { + // The regression this rule exists for: a wrapper that passed the raw key through produced + // `ws-host-terminals-ws-layout-my-dash`, which the server parsed back out as a dashboard id and created. + test('strips the layout prefix rather than nesting it', () => { + const key = terminalStateKey('host-terminals', parseWorkspaceKey('ws-layout-my-dash')); + expect(key).toBe('ws-host-terminals-my-dash'); + expect(key).not.toContain('ws-layout'); + }); + + test('screens and unknown workspaces share the defaults row', () => { + expect(terminalStateKey('terminals', parseWorkspaceKey('screens/chat'))).toBe('ws-terminals-default'); + expect(terminalStateKey('terminals', null)).toBe('ws-terminals-default'); + }); + + test('carries the panel-state prefixes the dispatcher allow-lists', () => { + const dashboard = parseWorkspaceKey('ws-layout-d1'); + expect(terminalStateKey('tmux', dashboard)).toBe('ws-tmux-d1'); + expect(terminalStateKey('nvim', dashboard)).toBe('ws-nvim-d1'); + expect(terminalStateKey('claude-code', dashboard)).toBe('ws-claude-code-d1'); + }); +}); diff --git a/src/workspaces/officerdev/src/components/Workspace/workspace-identity.ts b/src/workspaces/officerdev/src/components/Workspace/workspace-identity.ts new file mode 100644 index 00000000..c98ca969 --- /dev/null +++ b/src/workspaces/officerdev/src/components/Workspace/workspace-identity.ts @@ -0,0 +1,41 @@ +/** + * What a panel is mounted on, parsed once by the framework instead of by each app that cares. + * + * The framework persists a workspace under a dashboard-state key: `ws-layout-` for a user-created + * dashboard, `screens/` for one of the fixed screens. That key used to be handed to apps raw, as + * `dashboardId`, and three of them reverse-engineered meaning out of its shape — a regex here, a + * `startsWith('screens/')` there, an equality test against a literal somewhere else. Two consequences, + * both real: a key that failed one parser and passed another produced `ws-host-terminals-ws-layout-my-dash`, + * which the server obligingly created as a dashboard; and the agent address book is keyed on this string, + * so a change to how the key is derived orphans every named agent on the dashboard — rows and sessions + * alive, panels unable to find them. + * + * So: parse in one place, and keep `key` on the result. Storage keys are addresses that outlive the + * session, and the parsed halves are for deciding, never for writing. + */ +export type WorkspaceIdentity = { + /** `dashboard` is user-created and can host named agents; `screen` is a fixed route like `screens/email`. */ + kind: 'dashboard' | 'screen'; + /** The bare id — `agent-mvp` for `ws-layout-agent-mvp`, `email` for `screens/email`. For deciding. */ + id: string; + /** The dashboard-state key this workspace persists under, and `agent_panels.dashboard_id`. For storing. */ + key: string; +}; + +/** + * Null for anything that is not one of the two known shapes — including a workspace that has no key at + * all (a preview, a settings pane). Callers should read that as "not a dashboard and not a screen", which + * is what it is; the previous `!key.startsWith('screens/')` test called an unrecognised key a dashboard + * and would have let a panel try to register an agent against it. + */ +export function parseWorkspaceKey(key: string | null | undefined): WorkspaceIdentity | null { + if (!key) return null; + + const dashboard = /^ws-layout-(.+)$/.exec(key); + if (dashboard) return { kind: 'dashboard', id: dashboard[1]!, key }; + + const screen = /^screens\/(.+)$/.exec(key); + if (screen) return { kind: 'screen', id: screen[1]!, key }; + + return null; +}