diff --git a/docs/workspace-panel-todo.md b/docs/workspace-panel-todo.md index 6ce02d6f..136587f4 100644 --- a/docs/workspace-panel-todo.md +++ b/docs/workspace-panel-todo.md @@ -18,6 +18,11 @@ before deciding what to pick up next. Its conclusions in one paragraph: resize-debounce lost-update items (§5.5), the never-invalidated cache (§5.5), and tests for `layout-utils.ts` (§9). Five of the seven are one defect — *a write that silently did not land* — and they matter because panel identity now lives in the layout jsonb. + **Done as of 2026-08-07:** §1's `.catch(() => {})` (`70c2f08`), all of §2 and §3's missing `userId` + predicate (`f4ed740`), §9 (`85452d1`). **Left in Tier A: §4 entirely** — validate-on-read, the `'[]'` + layout default, and the error boundary — **and §5.5's two debounce items plus the stale cache.** §4 is + now the largest remaining piece and the one the objective actually ends on: work proceeds overnight and + the human's only act is to open the dashboard and look at it. - **Tier B:** `dashboardId`-as-a-bag (§5.9) — the agent address book is keyed on it; panel lifecycle (§5.1), but inverted: the requirement is that closing a chat panel must *not* destroy the agent; `normalizeLayout` as framework (§5.4); and the mobile-collapse decision (§6), which swings on one @@ -57,19 +62,32 @@ Both are two-line fixes, and without them you cannot tell whether any later fix field the sidecar returns (`sidecars/pty/sessions.mjs:169`) — the one field that distinguishes an orphan (`clients: 0`) from a live shell. Add it to the type and render it. -- [ ] **Stop swallowing persist failures.** `state/src/useDashboardState.ts:46` is +- [x] **Stop swallowing persist failures.** `state/src/useDashboardState.ts:46` is `.catch(() => { })`. Every 500 in this document is invisible because of it — the optimistic cache keeps the UI correct until reload. At minimum log; better, surface a toast and roll the cache back. **Tier A1 — the single highest-value item in this file against the objective** (`agent-coordination.md` §6.2). A panel's `config.agentName` is written through this path, so a swallowed 500 leaves a panel that shows its name, answers to its name, and forgets it on reload: invisible for exactly as long as nobody is looking, which is the window the whole project serves. + **Resolved `70c2f08`** — both catches now `console.error` and raise a toast naming the key, and + roll the cache back. The rollback is a **compare-and-swap**: it only reverts if the cache still + holds exactly what that call wrote. Writes to one key overlap freely (a window resize fires one + per group), and a blind rollback over a later successful write would turn one failure into two. + **Still swallowed, and not through this hook:** the three direct `client.patch('/dashboards', …)` + calls in `apps/Dashboards/DashboardListApp.tsx:93` and `DashboardPreview.tsx:323,326,356` — dashboard + create, rename and delete. Same defect, different call site; they bypass `useDashboardState` + entirely. A failed delete there leaves a dashboard that reappears on reload. --- ## 2. Live data loss -- [ ] **The PATCH dispatcher silently drops three key families in active use.** +**All four resolved in `f4ed740`** — they were one defect with four faces, so they were fixed as one +change. Each item keeps its diagnosis below and records what was done. Verified against the live server: +an unknown key 400s, the three prefixes round-trip through a GET, a null on a live dashboard is a no-op, +and the rename sequence leaves `workspaces` with no zombie. + +- [x] **The PATCH dispatcher silently drops three key families in active use.** `servers/api/dashboards/dashboards.ts:26-88` is a chain of `if (match) { …; continue; }` that ends with **no `else`** — unmatched keys are dropped and the request returns 200 with a fresh state blob. Matched: `workspaces`, `ws-layout-*`, `ws-terminals-*`, `ws-host-terminals-*`, `screens/*`. @@ -83,13 +101,23 @@ Both are two-line fixes, and without them you cannot tell whether any later fix **Tier A2** — the server half of A1. `ws-layout-*` *is* matched, so panel `config` persists today (verified: the `agent-mvp` layout round-trips with `config: {agentName: …}` intact). The missing `else` means the next key family added for coordination is a silent no-op returning 200. + **Resolved.** The three maps live in a new `dashboards.panel_state` jsonb bag keyed by prefix + (`{tmux: {panelId: sessionId}, …}`), with `dashboard_defaults.panel_state` for panels on a screen. + A bag rather than three columns because the fourth prefix should not need a schema change; an + allow-list rather than a regex because `ws--` cannot be split without one — both halves + may contain dashes. `PANEL_STATE_PREFIXES` in `servers/api/dashboards/dashboards.ts` must stay in + step with the `statePrefix` props in `apps/Terminal/index.tsx`, and **the new fallback `else` — a + 400 naming the key — is what tells you when it does not.** Living on the dashboard row also means + the maps are deleted with it. -- [ ] **`ws-terminals-{id}: null` on a live dashboard is a 500.** Same file, `:61-66` — the +- [x] **`ws-terminals-{id}: null` on a live dashboard is a 500.** Same file, `:61-66` — the `ws-layout-*` branch has a `value === null` → `deleteDashboard` case (`:42`); the terminals branches do not. A null falls to the UPDATE branch and sets a `NOT NULL` column (`databases/officer_db/src/queries/dashboards.ts:70`) → 23502. + **Resolved.** A null on either terminals branch is now a no-op: it means "forget this key", and it + only ever arrives paired with `ws-layout-{id}: null` on a rename, by which point the row is gone. -- [ ] **`HostTerminalWrapper` never strips the prefix, so it mints phantom dashboards.** *(found +- [x] **`HostTerminalWrapper` never strips the prefix, so it mints phantom dashboards.** *(found 2026-08-07, latent — `dashboards` is still 0 rows)* `apps/Terminal/HostTerminalWrapper.tsx:12` is `` `ws-host-terminals-${dashboardId}` `` with **no regex**, while its sibling `TerminalWrapper.tsx:13-14` correctly matches `^ws-layout-(.+)$` first. @@ -101,13 +129,22 @@ Both are two-line fixes, and without them you cannot tell whether any later fix Two apps deriving a state key from the same string by two different rules is the actual defect; the id-shaped-string-as-a-bag problem behind it is §5.9. Latent only because `officerdev/terminal-host` is in no default layout and hidden from the picker. + **Resolved.** All three wrappers now call `terminalStateKey(prefix, dashboardId)` + (`apps/Terminal/state-key.ts`) — `TerminalWrapper`'s rule, extracted, with the reasoning beside it. + Belt and braces on the server: the terminals and panel-state branches use a new `updateDashboard` + that will not INSERT, and **404 on an unknown id instead of creating one**. §5.9 is still the real + fix; this closes the hole a mis-derived key could fall through. -- [ ] **Renaming a dashboard resurrects it as a zombie row.** +- [x] **Renaming a dashboard resurrects it as a zombie row.** `apps/Dashboards/DashboardPreview.tsx:316-318` PATCHes `ws-layout-old: null` *and* `ws-terminals-old: null` together. The first deletes the row; the second then calls `upsertDashboard`, finds nothing, and **re-INSERTs it** with `name = id`. The old slug reappears in `workspaces` on the next GET as a duplicate. Fixed by the item above, but verify this specific sequence after fixing. + **Resolved, and verified as the specific sequence:** create `zombie-test`, then PATCH + `{ws-layout-renamed: …, ws-layout-zombie-test: null, ws-terminals-zombie-test: null, + ws-host-terminals-zombie-test: null}` in one body. `workspaces` comes back as + `['agent-mvp', 'renamed']`. --- @@ -129,12 +166,17 @@ these. Fix: composite PK `(user_id, id)`, or uuid ids. Composite PKs have a known drizzle re-diff quirk (see `databases/CLAUDE.md` → "Composite keys") — harmless churn, but read the plan. -- [ ] **`upsertDashboard`'s UPDATE has no `userId` predicate.** +- [x] **`upsertDashboard`'s UPDATE has no `userId` predicate.** `databases/officer_db/src/queries/dashboards.ts:73` — `db.update(dashboards).set(set).where(eq(dashboards.id, id))`. The `existing` lookup above it *is* scoped, so it cannot reach another user's row today, but it is a non-transactional read-then-write. **It becomes a live cross-user overwrite the moment the PK above is made composite.** Do both in one change or the first fix opens the second. + **Resolved `f4ed740`, incidentally.** `upsertDashboard` is now `updateDashboard` (scoped by + `(userId, id)`, `RETURNING` to say whether it matched) with an INSERT only when that returns + nothing — so the read-then-write is gone as well as the missing predicate. **The composite-PK item + above is still open**, and is still the one that matters; this just no longer opens a second hole + when it lands. --- @@ -481,6 +523,16 @@ Cheap to fix, and prerequisites for the 5.8 migration rather than alternatives t agent it is. Follow-ons: `PanelContents` still drops `fitContent` and `zoom` (§5.3), and the mutators are still untested (§9). +- [x] **A write that does not land no longer looks like one that did.** *(`70c2f08` + `f4ed740`, branch + `agent-coordination-mvp`)* — Tier A1 and A2 of the re-rank, and between them the whole of §2 and + the second half of §3. Client: `useDashboardState` rolls back and toasts instead of `.catch(() => {})`. + Server: the PATCH dispatcher gained the three missing key families, a 400 on anything unmatched, a + 404 rather than an INSERT for an unknown dashboard id, and a null that means "forget" rather than + "write NULL into a NOT NULL column". Framework: one rule for deriving a terminal's state key + (`apps/Terminal/state-key.ts`) where there had been three. + The remaining member of that family is the three direct `client.patch('/dashboards', …)` calls in + `apps/Dashboards/` — see §1. + --- ## 8. Dead code sweep @@ -509,7 +561,7 @@ Low priority, but each line here is a line someone will read and believe. ## 9. Tests -- [ ] **There are none.** No `*.test.*` exists under any `Workspace*` or `*Screen*` path. `layout-utils.ts` +- [x] **There are none.** No `*.test.*` exists under any `Workspace*` or `*Screen*` path. `layout-utils.ts` is 214 lines of pure functions over a serialisable tree — the cheapest high-value test target in the codebase, and every item in 5.2 and 5.5 is a regression test waiting to be written. Start here before the mutator work, not after. @@ -518,3 +570,13 @@ Low priority, but each line here is a line someone will read and believe. `setPanelConfig`, `collectPanelConfigs`, `newPanelFrom` — and shipped them with no tests. A regression in `swapPanels` now silently swaps two agents' identities between panels, which is a worse failure than any layout glitch the same bug could previously cause. + **Resolved `85452d1`** — `layout-utils.test.ts`, 47 tests, 99.56% lines / 100% functions, the + first test under any `Workspace*` path. It paid for itself immediately: `setApp` was preserving + `config` whenever the new `appType` was non-null, so switching a panel from chat to terminal handed + the terminal the chat's `{agentName}` to read as its own settings — the exact leak the comment + beside it claimed to prevent. Not reachable through the UI (the only route out of an app is + `onClearApp` → `null`), but `setApp` is exported from the barrel. Fixed in the same commit. + Two tests are deliberately written to the *current* behaviour and marked `KNOWN GAP` so they fail + the day it changes: a move drops `zoom` and `fitContent` (§5.3). + **Still untested:** the other eleven files of the framework — `WorkspaceView`, `WorkspaceRenderer`, + `PanelSlot`, `normalizeLayout`. §5.5's debounce lost-updates live there, not in `layout-utils`.