From f4ed7401da3ed7e48a77728c8a794914d77ace39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 7 Aug 2026 08:55:01 +0000 Subject: [PATCH] stop the dashboard PATCH dispatcher losing writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects, one shape: a write that returns 200 and lands nowhere. - Unknown keys were dropped by a chain of `if (match) continue` with no `else`. The three prefixes CommandTerminalWrapper actually writes — tmux, nvim, claude-code — were among them, so those panel maps lived in the React Query cache only: every reload minted a fresh uuid and abandoned a running pty. They now live in a `panel_state` bag on the dashboard row, and an unmatched key 400s. - `ws-terminals-{id}: null` fell through to an upsert, writing NULL into a NOT NULL column on a live dashboard and re-INSERTing a deleted one. Renaming a dashboard sends exactly that, paired with `ws-layout-{id}: null`, so the old slug came back as a zombie row in the dashboards list. - HostTerminalWrapper and CommandTerminalWrapper built their state key straight from `dashboardId`, which is a workspace *key* (`ws-layout-`), while TerminalWrapper stripped the prefix. The server read the un-stripped form back as a dashboard id and created it. One rule now, in state-key.ts. Verified against the live server: unknown key 400s, the three prefixes round-trip, a null on a live dashboard is a no-op, and the rename sequence leaves no zombie. Co-Authored-By: Claude Opus 5 --- src/databases/officer_db/src/index.ts | 3 + .../officer_db/src/queries/dashboards.ts | 110 ++++++++++++++---- .../officer_db/src/schema/dashboards.ts | 13 ++- src/servers/api/dashboards/dashboards.ts | 40 ++++++- .../apps/Terminal/CommandTerminalWrapper.tsx | 3 +- .../src/apps/Terminal/HostTerminalWrapper.tsx | 3 +- .../src/apps/Terminal/TerminalWrapper.tsx | 7 +- .../officerdev/src/apps/Terminal/state-key.ts | 16 +++ 8 files changed, 159 insertions(+), 36 deletions(-) create mode 100644 src/workspaces/officerdev/src/apps/Terminal/state-key.ts diff --git a/src/databases/officer_db/src/index.ts b/src/databases/officer_db/src/index.ts index 86488a08..cd12995d 100644 --- a/src/databases/officer_db/src/index.ts +++ b/src/databases/officer_db/src/index.ts @@ -56,11 +56,14 @@ export { export { getAllDashboardState, upsertDashboard, + updateDashboard, deleteDashboard, + setDashboardPanelState, upsertScreen, deleteScreen, getDefaults, upsertDefaults, + setDefaultsPanelState, } from './queries/dashboards'; export { diff --git a/src/databases/officer_db/src/queries/dashboards.ts b/src/databases/officer_db/src/queries/dashboards.ts index e74954e9..bc072e95 100644 --- a/src/databases/officer_db/src/queries/dashboards.ts +++ b/src/databases/officer_db/src/queries/dashboards.ts @@ -26,6 +26,9 @@ export async function getAllDashboardState(userId: number): Promise => + value && typeof value === 'object' && !Array.isArray(value) ? (value as Record) : {}; + // ── Dashboard CRUD ── type UpsertDashboardData = { @@ -54,37 +63,75 @@ type UpsertDashboardData = { sortOrder?: number; }; +function buildDashboardSet(data: UpsertDashboardData, now: Date): Record { + const set: Record = { updatedAt: now }; + if (data.name !== undefined) set.name = data.name; + if (data.config !== undefined) set.config = data.config; + if (data.layout !== undefined) set.layout = data.layout; + if (data.terminals !== undefined) set.terminals = data.terminals; + if (data.hostTerminals !== undefined) set.hostTerminals = data.hostTerminals; + if (data.sortOrder !== undefined) set.sortOrder = data.sortOrder; + return set; +} + export async function upsertDashboard(userId: number, id: string, data: UpsertDashboardData): Promise { const now = new Date(); + if (await updateDashboard(userId, id, data)) return; + + await db.insert(dashboards).values({ + id, + userId, + name: data.name ?? id, + config: data.config ?? {}, + layout: data.layout ?? [], + terminals: data.terminals ?? {}, + hostTerminals: data.hostTerminals ?? {}, + sortOrder: data.sortOrder ?? 0, + createdAt: now, + updatedAt: now, + }); +} + +/** + * Update a dashboard only if it already exists, and report whether it did. + * + * `upsertDashboard` INSERTs on an unknown id, which is right for the two create paths and wrong for every + * other key: renaming a dashboard PATCHes `ws-layout-old: null` and `ws-terminals-old: null` together, and + * the second key used to re-INSERT the row the first had just deleted — named after its own slug, so the + * old name reappeared in the dashboard list as a zombie. + */ +export async function updateDashboard(userId: number, id: string, data: UpsertDashboardData): Promise { + const updated = await db + .update(dashboards) + .set(buildDashboardSet(data, new Date())) + .where(and(eq(dashboards.userId, userId), eq(dashboards.id, id))) + .returning({ id: dashboards.id }); + return updated.length > 0; +} + +/** Write one prefix of a dashboard's `panelState` bag, leaving its siblings alone. */ +export async function setDashboardPanelState( + userId: number, + id: string, + prefix: string, + value: unknown, +): Promise { const existing = await db - .select() + .select({ panelState: dashboards.panelState }) .from(dashboards) .where(and(eq(dashboards.userId, userId), eq(dashboards.id, id))) .then((rows) => rows[0]); + if (!existing) return false; - if (existing) { - const set: Record = { updatedAt: now }; - if (data.name !== undefined) set.name = data.name; - if (data.config !== undefined) set.config = data.config; - if (data.layout !== undefined) set.layout = data.layout; - if (data.terminals !== undefined) set.terminals = data.terminals; - if (data.hostTerminals !== undefined) set.hostTerminals = data.hostTerminals; - if (data.sortOrder !== undefined) set.sortOrder = data.sortOrder; - await db.update(dashboards).set(set).where(eq(dashboards.id, id)); - } else { - await db.insert(dashboards).values({ - id, - userId, - name: data.name ?? id, - config: data.config ?? {}, - layout: data.layout ?? [], - terminals: data.terminals ?? [], - hostTerminals: data.hostTerminals ?? [], - sortOrder: data.sortOrder ?? 0, - createdAt: now, - updatedAt: now, - }); - } + const next = { ...asRecord(existing.panelState) }; + if (value === null) delete next[prefix]; + else next[prefix] = value; + + await db + .update(dashboards) + .set({ panelState: next, updatedAt: new Date() }) + .where(and(eq(dashboards.userId, userId), eq(dashboards.id, id))); + return true; } export async function deleteDashboard(userId: number, id: string): Promise { @@ -157,3 +204,18 @@ export async function upsertDefaults(userId: number, data: UpsertDefaultsData): set, }); } + +/** The `panelState` equivalent of `upsertDefaults` — for panels living on a screen rather than a dashboard. */ +export async function setDefaultsPanelState(userId: number, prefix: string, value: unknown): Promise { + const now = new Date(); + const [row] = await db.select().from(dashboardDefaults).where(eq(dashboardDefaults.userId, userId)); + + const next = { ...asRecord(row?.panelState) }; + if (value === null) delete next[prefix]; + else next[prefix] = value; + + await db + .insert(dashboardDefaults) + .values({ userId, panelState: next as never, updatedAt: now }) + .onConflictDoUpdate({ target: dashboardDefaults.userId, set: { panelState: next, updatedAt: now } }); +} diff --git a/src/databases/officer_db/src/schema/dashboards.ts b/src/databases/officer_db/src/schema/dashboards.ts index 58498a86..01843108 100644 --- a/src/databases/officer_db/src/schema/dashboards.ts +++ b/src/databases/officer_db/src/schema/dashboards.ts @@ -11,8 +11,16 @@ export const dashboards = pgTable( name: text('name').notNull(), config: jsonb('config').notNull().default({}), layout: jsonb('layout').notNull().default([]), - terminals: jsonb('terminals').notNull().default([]), - hostTerminals: jsonb('host_terminals').notNull().default([]), + // `{}`, not `[]` — every writer treats these as a panelId → sessionId map, and `screens` and + // `dashboard_defaults` already default them correctly. The array default only ever produced a shape + // the client had to spread away on first write. + terminals: jsonb('terminals').notNull().default({}), + hostTerminals: jsonb('host_terminals').notNull().default({}), + // Panel-scoped session maps that have no column of their own, keyed by the app's state prefix: + // `{ tmux: { : }, nvim: {…}, 'claude-code': {…} }`. They used to have no home at + // all — the PATCH dispatcher dropped them and returned 200, so every reload minted a fresh uuid and + // abandoned the running pty. Living on the dashboard row means they are deleted with it. + panelState: jsonb('panel_state').notNull().default({}), sortOrder: integer('sort_order').notNull().default(0), createdAt: timestamp('created_at', { withTimezone: true }).notNull().defaultNow(), updatedAt: timestamp('updated_at', { withTimezone: true }).notNull().defaultNow(), @@ -44,5 +52,6 @@ export const dashboardDefaults = pgTable('dashboard_defaults', { .unique(), terminals: jsonb('terminals').notNull().default({}), hostTerminals: jsonb('host_terminals').notNull().default({}), + panelState: jsonb('panel_state').notNull().default({}), updatedAt: timestamp('updated_at', { withTimezone: true }).notNull().defaultNow(), }); diff --git a/src/servers/api/dashboards/dashboards.ts b/src/servers/api/dashboards/dashboards.ts index f1033165..2ed0907b 100644 --- a/src/servers/api/dashboards/dashboards.ts +++ b/src/servers/api/dashboards/dashboards.ts @@ -2,14 +2,23 @@ import { createRouter } from '@@/create-router'; import { getAllDashboardState, upsertDashboard, + updateDashboard, deleteDashboard, upsertScreen, deleteScreen, upsertDefaults, + setDashboardPanelState, + setDefaultsPanelState, } from 'officerdb'; export const dashboardsRouter = createRouter(); +// Panel-scoped session maps with no column of their own, written by `CommandTerminalWrapper` as +// `ws--`. This is an allow-list because `ws--` cannot be split +// without one — both halves may contain dashes. Keep it in step with the `statePrefix` props in +// `apps/Terminal/index.tsx`; the 400 at the bottom of the PATCH loop is what tells you when it is not. +const PANEL_STATE_PREFIXES = ['tmux', 'nvim', 'claude-code']; + // GET /dashboards dashboardsRouter.get('/', async (ctx) => { const userId = ctx.get('user').id; @@ -57,11 +66,18 @@ dashboardsRouter.patch('/', async (ctx) => { continue; } - // ws-terminals-{id} + // ws-terminals-{id} / ws-host-terminals-{id} + // + // A null means "forget this key". It arrives paired with `ws-layout-{id}: null` on a rename, by which + // point the row is already gone — so dropping it silently is the whole job. These branches used to + // fall through to an upsert, which wrote NULL into a NOT NULL column on a live dashboard (23502) and + // re-INSERTed a deleted one as a zombie named after its own slug. const wsTerminalsMatch = key.match(/^ws-terminals-(.+)$/); if (wsTerminalsMatch) { const id = wsTerminalsMatch[1]!; - await upsertDashboard(userId, id, { terminals: value }); + if (value !== null && !(await updateDashboard(userId, id, { terminals: value }))) { + return ctx.json({ error: `no dashboard "${id}" to write "${key}" to` }, 404); + } continue; } @@ -69,7 +85,21 @@ dashboardsRouter.patch('/', async (ctx) => { const wsHostTerminalsMatch = key.match(/^ws-host-terminals-(.+)$/); if (wsHostTerminalsMatch) { const id = wsHostTerminalsMatch[1]!; - await upsertDashboard(userId, id, { hostTerminals: value }); + if (value !== null && !(await updateDashboard(userId, id, { hostTerminals: value }))) { + return ctx.json({ error: `no dashboard "${id}" to write "${key}" to` }, 404); + } + continue; + } + + // ws-{prefix}-{id} for the prefixes that have no column of their own + const prefix = PANEL_STATE_PREFIXES.find((p) => key.startsWith(`ws-${p}-`)); + if (prefix) { + const id = key.slice(`ws-${prefix}-`.length); + if (id === 'default') { + await setDefaultsPanelState(userId, prefix, value); + } else if (!(await setDashboardPanelState(userId, id, prefix, value))) { + return ctx.json({ error: `no dashboard "${id}" to write "${key}" to` }, 404); + } continue; } @@ -85,6 +115,10 @@ dashboardsRouter.patch('/', async (ctx) => { continue; } + // No silent drops. An unmatched key used to return 200 with a fresh state blob, so a whole key family + // in active use could persist nowhere at all and look like it had — which is exactly what happened to + // the three prefixes above. + return ctx.json({ error: `unknown dashboard-state key "${key}"` }, 400); } const state = await getAllDashboardState(userId); diff --git a/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx b/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx index 0a227e2b..03e16f07 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/CommandTerminalWrapper.tsx @@ -4,6 +4,7 @@ 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 = {}; @@ -15,7 +16,7 @@ type CommandTerminalWrapperProps = { export const CommandTerminalWrapper = ({ panelId, command, statePrefix }: CommandTerminalWrapperProps) => { const { dashboardId, cwd } = useWorkspace(); - const stateKey = dashboardId ? `ws-${statePrefix}-${dashboardId}` : `ws-${statePrefix}-default`; + const stateKey = terminalStateKey(statePrefix, dashboardId); 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 638a0069..e683f030 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/HostTerminalWrapper.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/HostTerminalWrapper.tsx @@ -4,12 +4,13 @@ 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 = {}; export const HostTerminalWrapper = ({ panelId }: { panelId: string }) => { const { dashboardId, cwd } = useWorkspace(); - const stateKey = dashboardId ? `ws-host-terminals-${dashboardId}` : 'ws-host-terminals-default'; + const stateKey = terminalStateKey('host-terminals', dashboardId); 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 ed4ecf2b..ecbcded1 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/TerminalWrapper.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/TerminalWrapper.tsx @@ -4,16 +4,13 @@ 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 = {}; export const TerminalWrapper = ({ panelId }: { panelId: string }) => { const { dashboardId, cwd } = useWorkspace(); - const stateKey = (() => { - const wsMatch = dashboardId?.match(/^ws-layout-(.+)$/); - if (wsMatch) return `ws-terminals-${wsMatch[1]}`; - return 'ws-terminals-default'; - })(); + const stateKey = terminalStateKey('terminals', dashboardId); 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 new file mode 100644 index 00000000..a1859dad --- /dev/null +++ b/src/workspaces/officerdev/src/apps/Terminal/state-key.ts @@ -0,0 +1,16 @@ +/** + * 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 + * `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. + * + * 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'}`; +}