pastilhasandClaude Opus 5
68f2c55ecf
src/databases/officer_db/src/<feature>/{schema.ts,queries.ts}, replacing the
parallel schema/ and queries/ trees. 24 feature directories, 46 files moved with
git mv so history follows.
The parallel trees had drifted, which is what the restructure is really fixing:
four features were named differently on each side — app-store/sidecar-installs,
email/email-accounts, server/server-config
operations had a schema and NO query file: its task_logs is reached directly
from src/servers/api/task-logger.ts, bypassing this package's own boundary
integrations had queries and NO schema, because it spans two features'
tables — server_integrations and user_integrations
Both lopsided cases survive as directories holding one file, which states the
problem instead of hiding it across two trees.
Nothing outside the package changed how it imports. `officerdb`, `officerdb/types`
and `officerdb/db` resolve exactly as before; index.ts absorbed the path changes.
Added `"./*": "./src/*"` so the new layout is reachable — `officerdb/soulseek/schema`
— which one script needed, because soulseek is a plugin and therefore commented
out of the aggregator.
schema/index.ts became src/schema.ts, keeping the core/plugin split from earlier
tonight. drizzle.config.ts and the package's "./schema" export follow it.
Verified rather than assumed: all 52 files in the package parse, every relative
import resolves against the new layout (checked by walking each specifier to a
real file, since parsing does not check paths), and everything in the tree
importing officerdb still parses. Not typechecked — empty node_modules, frozen
installs.
One rewrite bug worth recording: the rule mapping a query module's sibling import
also matched the './schema' this pass had just written, turning it into
'../schema/queries' in 22 files. Caught by the resolver check, not by parsing —
both spellings parse fine.
Also corrects every path reference the move invalidated: src/databases/CLAUDE.md's
layout diagram, the root CLAUDE.md data section, three docs, and seven sidecar
comments naming queries/<x>.ts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1120 lines
92 KiB
Markdown
1120 lines
92 KiB
Markdown
# Workspaces & Panels — working TODO
|
||
|
||
Living list. Add items as they are found, tick them as they land, and write the resolution _into_ the
|
||
item rather than deleting it — the reason a thing was done is worth more later than a clean list.
|
||
Move anything fully settled to §7.
|
||
|
||
**How the framework actually works is documented separately, in `workspace-panels.md`** — read that
|
||
first if you are new to it. This file is only the defect list and the work queue.
|
||
|
||
**Why any of it matters is in `agent-coordination.md`** — the north star.
|
||
|
||
**This list stays ordered by defect severity. The re-rank against the objective lives in
|
||
`agent-coordination.md` §6** _(done 2026-08-07)_, and the two orderings deliberately disagree — read §6
|
||
before deciding what to pick up next. Its conclusions in one paragraph:
|
||
|
||
- **Tier A, on the critical path:** the swallowed persist failure (§1), the PATCH dispatcher's missing
|
||
`else` (§2), validate-on-read + the `'[]'` layout default + an error boundary (§4), the two
|
||
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.
|
||
**Tier A is empty as of 2026-08-07.** §1's `.catch(() => {})` (`70c2f08`), all of §2 and §3's missing
|
||
`userId` predicate (`f4ed740`), §9 (`85452d1`), all three Tier A items in §4 — the error boundary
|
||
(`64961d4`), validate-on-read and the `'[]'` layout default (`ef036df`) — and §5.5's three: the two
|
||
resize-debounce lost updates (`b0a32ae`) and the never-invalidated cache (`81ad3ef`).
|
||
Everything the re-rank put on the critical path for "the human opens the dashboard in the morning and
|
||
sees the result" has landed. **Pick up from Tier B.**
|
||
- **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
|
||
unanswered question.
|
||
**§5.9 is closed** (`dbe585f`, `717580f`, `585f234`, `d3922bd`, `ca046a3`) — the bag is a parsed
|
||
identity, the app-config fields are deleted or moved onto the app, and the inert context is built
|
||
once. **§5.1's first two items are closed too** (`04371a9`, `198dc71`, `c92b51c`) — panels have a close
|
||
signal that fires on a real close and on nothing else, and the terminals use it; the chat panel
|
||
deliberately does not, which is the inversion above. **§5.4's first item is closed** (`1de1d92`) — the
|
||
allow-list is a `WorkspaceView` prop and the fourteen copies are gone. Tier B is now the pty backstop
|
||
(the third item of §5.1) and the mobile decision (§6) — **both of which are blocked on a question
|
||
only the owner can answer**, not on work. §5.4's second item was inventoried and dropped as
|
||
compression rather than deduplication; see the item for the reasoning and the three defects the
|
||
inventory turned up instead. **Tier B has no unblocked work left.**
|
||
- **Tier C, orthogonal:** everything else — including **§5.2 (remounts) and §5.3 (drag-to-move), both
|
||
large downgrades.** A panel is now a pointer to a server-side session, so a remount costs a replay, and
|
||
the drag hazard is disarmed by resolving identity by name (`e588524` + `bc82086`).
|
||
- **And the result that matters: no item in this file blocked building the MVP.** It was built and ran
|
||
unattended on the framework as it stands. Stop treating this list as the prerequisite queue for the
|
||
objective.
|
||
|
||
Findings and full reasoning: `COMMS/workspace-panel-framework-analysis-2026-08-07.md`. Every `file:line`
|
||
below was opened; DB claims were run against live `officer_dev`. Paths are relative to
|
||
`platform/src/workspaces/officerdev/src/` unless they start with `servers/`, `databases/` or `sidecars/`.
|
||
|
||
**Framework core:** `components/Workspace/` — 12 files, 1,533 lines. No tests exist for any of it.
|
||
|
||
**Two open decisions gate real work — see §6.** Don't start item 4.1 or 5.3 before those are answered.
|
||
|
||
---
|
||
|
||
## 1. Observability first — do these before anything else
|
||
|
||
Both are two-line fixes, and without them you cannot tell whether any later fix worked.
|
||
**Both are done as of 2026-08-07** — they went in with the terminal close wiring (§5.1), which is what
|
||
they were blocking: there was no way to see whether a shell had actually been killed. §1's third item
|
||
(the swallowed persist failure) was already closed. **§1 is empty.**
|
||
|
||
- [x] **Fix the Running Shells paths — the panel has 404'd since 2026-07-31.** — done `c92b51c`.
|
||
`apps/Terminal/RunningShells.tsx:43,49` call `/terminal/sessions` and
|
||
`DELETE /terminal/sessions/:id`. `useClient` prefixes `/api`; `servers/sidecar/create-proxy.ts:69`
|
||
strips the mount prefix, so the sidecar receives `/sessions`. The pty sidecar serves only
|
||
`/_officer/sessions` (`sidecars/pty/server.mjs:30`) and `/_officer/sessions/:id` (`:34`) —
|
||
everything else 404s at `:40`. Correct paths: `/terminal/_officer/sessions` and
|
||
`/terminal/_officer/sessions/:id`. The panel (`fccf212`) predates the proxy move (`7129cd8`) that
|
||
deleted the old `servers/api/terminal/router.ts` and was never updated.
|
||
Symptom today: permanently reads "No shells running", kill button is a silent no-op.
|
||
**Confirmed against the running sidecar before fixing**: `GET /sessions` → `{"error":"not found"}`,
|
||
`GET /_officer/sessions` → the list. Both paths corrected.
|
||
|
||
- [x] **Surface the `clients` count in Running Shells.** — done alongside the paths. `RunningShells.tsx`
|
||
dropped the `clients` field the sidecar returns (`sidecars/pty/sessions.mjs:169`) — the one field
|
||
that distinguishes an orphan (`clients: 0`) from a live shell. Now on the type, rendered as
|
||
"N attached", and `clients === 0` also earns the row an **orphan** badge, since the count is only
|
||
useful if you do not have to read it to notice.
|
||
|
||
- [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.
|
||
**The other three, not through this hook, resolved `81ad3ef`:** the direct `client.patch('/dashboards', …)`
|
||
calls in `apps/Dashboards/DashboardListApp.tsx:93` and `DashboardPreview.tsx:323,326,356` — dashboard
|
||
create, rename and delete, which build multi-key patches and so bypass `useDashboardState`. They now
|
||
call `persistDashboardState`, which shares the hook's in-flight bookkeeping and, on failure,
|
||
**invalidates rather than reverts**: once the roster has been rewritten and keys dropped from the
|
||
blob there is no single previous value to swap back, and a refetch is the only thing that makes the
|
||
list agree with the server. A failed delete used to leave the dashboard gone from the list and alive
|
||
on the server, reappearing at the next reload with no hint why.
|
||
|
||
---
|
||
|
||
## 2. Live data loss
|
||
|
||
**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/*`.
|
||
`apps/Terminal/CommandTerminalWrapper.tsx:18` writes `ws-${statePrefix}-${dashboardId}`, and the
|
||
registered prefixes (`apps/Terminal/index.tsx:22,27,34`) are **`tmux`, `nvim`, `claude-code`**.
|
||
So the panel→session map for those three survives only in the React Query cache, for the life of
|
||
the tab. **Every reload mints a fresh uuid and abandons the previous pty** — alive, unreachable,
|
||
never killed. (Tmux's own state survives via `new-session -A -s off-<panelId>`; the shell running
|
||
`tmux attach` does not.)
|
||
Two parts: add the three families, **and** add a fallback `else` that 400s on an unknown key.
|
||
**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-<prefix>-<id>` 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.
|
||
|
||
- [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/dashboards/queries.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.
|
||
|
||
- [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.
|
||
So the key becomes `ws-host-terminals-ws-layout-<id>` or `ws-host-terminals-screens/terminal`, the
|
||
dispatcher's `^ws-host-terminals-(.+)$` branch captures that whole string as an id, and
|
||
`upsertDashboard` **inserts a row when the id is unknown** (`queries/dashboards.ts:75-86`,
|
||
`name: data.name ?? id`). `getAllDashboardState` maps every `dashboards` row into `workspaces`, so
|
||
the garbage id surfaces in the Dashboards list as a real dashboard.
|
||
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.
|
||
|
||
- [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']`.
|
||
|
||
---
|
||
|
||
## 3. Multi-user correctness — before another member creates a dashboard
|
||
|
||
`dashboards` is **empty (0 rows)** today, so none of this has fired yet. Members can now sign in
|
||
(`d8ee678`), so it is a matter of time. Note `TODO.md`'s preamble still says the platform is collapsing
|
||
to single-user — that predates the capability permission model and should not be used to deprioritise
|
||
these.
|
||
|
||
> **Re-measured 2026-08-07.** The premise above has moved and the section is no longer hypothetical.
|
||
> `users` holds **five rows** (`pastilhas@`, three outside addresses, one `captest@`) and `dashboards`
|
||
> holds **one** (`agent-mvp`, the coordination demo). So the collision needs one member to name a
|
||
> dashboard something the owner already used — it is reachable today, by a person, with no code change.
|
||
>
|
||
> It also puts this section in **direct contradiction with `CLAUDE.md`**, which opens by calling
|
||
> single-user "a hard invariant, not a stage" — no roles, no per-user isolation, and "if a change seems
|
||
> to need *which user is this*, the answer is always the owner." Five rows in `users` says otherwise.
|
||
> One of the two documents is wrong about what this platform is, and that is a **product question, not a
|
||
> defect**: the answer decides whether the item below is urgent or should be deleted along with the rest
|
||
> of the section.
|
||
>
|
||
> **Deliberately not fixed unattended**, and the reasons are worth stating so the next pass doesn't
|
||
> re-litigate them. It is a schema change, so it needs `bun db:push` — which would also apply another
|
||
> agent's uncommitted schema files sitting in the same tree tonight. It has a real fork (composite PK vs
|
||
> uuid ids) with different blast radii for anything storing a dashboard id as an address, and
|
||
> `agent_panels.dashboard_id` is exactly that. And composite PKs carry the drizzle re-diff quirk that
|
||
> `databases/CLAUDE.md` warns about. Small change, several ways to be sorry.
|
||
>
|
||
> **Recommendation, for the owner to accept or reject in one line:** composite PK `(user_id, id)`. It
|
||
> matches the `uq_dashboards_user_id` index that is already there — the evidence the item cites that
|
||
> per-user ids were the original intent — and it leaves every stored `ws-layout-<id>` address valid,
|
||
> which uuid ids would not.
|
||
|
||
- [ ] **`dashboards.id` is a global primary key but ids are `slugify(name)`.**
|
||
`databases/officer_db/src/dashboards/schema.ts` declares `id: text('id').primaryKey()`. Live:
|
||
`"dashboards_pkey" PRIMARY KEY, btree (id)` plus a redundant
|
||
`"uq_dashboards_user_id" UNIQUE, btree (user_id, id)` — evidence per-user ids were intended and
|
||
half-built. Ids come from `DashboardPreview.tsx:300` (`slugify(trimmed) || generateSlug()`) and the
|
||
uniqueness loop at `:340` checks **only the caller's own dashboards**. Two users both naming a
|
||
dashboard "Work" → both get `id = 'work'` → the second violates the PK → 500 → swallowed → the
|
||
dashboard shows, then vanishes on reload.
|
||
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.
|
||
|
||
- [x] **`upsertDashboard`'s UPDATE has no `userId` predicate.**
|
||
`databases/officer_db/src/dashboards/queries.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.
|
||
|
||
---
|
||
|
||
## 4. Resilience — one bad row is currently a white screen
|
||
|
||
_The first three items are Tier A (`agent-coordination.md` §6.2, A3–A4). This is the last mile of the
|
||
objective: work proceeds overnight, and the human's only act is to open the dashboard and look. A
|
||
malformed row turning that into a white screen loses the result at the one moment it is consumed — and
|
||
`config` added a new unvalidated field to an already-unvalidated tree. The work itself is safe in
|
||
`chat_session_events`, which is why recovery must offer "restore default layout" rather than SQL._
|
||
|
||
**§4 is closed** — `64961d4` (the boundary), `ef036df` (validate-on-read and the `'[]'` default),
|
||
`92b8905` (validate-on-write). "A layout is an object" is now stated in four places: the column, the
|
||
route, the read, and the renderer's willingness to survive being handed something else anyway.
|
||
|
||
- [x] **Add an error boundary.** `grep -rln "componentDidCatch\|getDerivedStateFromError\|ErrorBoundary\|errorElement" src` returned **nothing**
|
||
across the whole repo.
|
||
**Resolved `64961d4`** — two of them, because "recover" means different things at different depths.
|
||
`components/ErrorBoundary.tsx` is a generic render-prop class (`resetKeys` so a boundary cannot
|
||
latch); `Screens/Dashboard/Layout/DashboardLayout.tsx` wraps the routed screen with the dock and
|
||
header deliberately _outside_ it, so navigating away is itself a way out, and its fallback offers a
|
||
two-click reset of every `screens/*` layout — dashboards are left alone, being user-created and
|
||
holding content. `PanelSlot.tsx` wraps each app so one bad panel leaves the rest of the workspace
|
||
running, with "clear this panel" as its recovery, offered only when the layout is the user's to edit.
|
||
Not a targeted "reset _this_ screen": React unmounts the subtree before rendering a fallback, so an
|
||
effect-based registry of "which layout key was I rendering" is empty at exactly the moment you need it.
|
||
|
||
- [x] **Validate on read.** `useDashboardState.ts:36` was `key in state ? state[key] as T : defaultValue`
|
||
— an unchecked cast over a value that arrived as `unknown` from jsonb.
|
||
**Resolved `ef036df`** — `readValue()` checks kind-compatibility before casting and falls back to the
|
||
caller's default, warning once per key. Not `isLayoutNode()`: all 24 layout call sites would have had
|
||
to pass a predicate, and the same wrong-container bug applies to the `panelId → sessionId` maps. So
|
||
the rule is generic and only guards **object-shaped defaults** — a wrong primitive is a cosmetic
|
||
surprise, a wrong container is a crash. It deliberately does not write the correction back; a read
|
||
should not overwrite the server, and the next real `setValue` repairs the row.
|
||
|
||
- [x] **`layout` columns default to `'[]'`, which is not a valid `LayoutNode`.**
|
||
`schema/dashboards.ts:13,31` — an empty **array** for a column holding an object. Any path that
|
||
upserts without a layout (e.g. the `workspaces` branch) writes it; `key in state` is then true, so
|
||
the stored `[]` wins over the caller's default and `normalizeLayout` calls `.children.map` on it and
|
||
throws. **Creating a dashboard from the dashboard list is exactly that path**, so this was
|
||
reproducible, not theoretical.
|
||
**Resolved `ef036df`** — both columns are nullable with no default (NULL means "none stored", which
|
||
is the truth), and `getAllDashboardState` omits the key when what is stored is not an object, so a
|
||
row written before this is repaired by the next write instead of crashing the read. Verified live:
|
||
creating a dashboard with no layout emits no `ws-layout` key, and a row hand-set back to `'[]'` is
|
||
omitted too. `bun db:push` planned exactly the four expected `ALTER`s plus the two known-harmless
|
||
`pk_music_now_playing` lines.
|
||
|
||
- [x] **Validate on write.** The whole path is `unknown`: `useDashboardState.ts:46` →
|
||
`servers/api/dashboards/dashboards.ts:45,83` → `queries/dashboards.ts:51,97` → cast `as never` at
|
||
`:114` to satisfy drizzle → jsonb. No zod, no CHECK on either layout column. Given
|
||
`databases/CLAUDE.md`'s stance that the schema is the source of truth for _contents_, a CHECK on
|
||
`jsonb_typeof(layout) = 'object'` is the cheap half.
|
||
**Resolved `92b8905`** — both halves, because they do different jobs.
|
||
`ck_dashboards_layout_object` / `ck_screens_layout_object` are the backstop: nothing reaches those
|
||
columns without passing them, including a psql session or a future writer nobody has written yet.
|
||
NULL stays legal — it means "none stored", which is the honest state of a dashboard created without
|
||
a layout. But a constraint violation surfaces as a 500 that tells the caller nothing, so the PATCH
|
||
dispatcher checks the same rule first and answers `400 "ws-layout-x" must be a layout object`.
|
||
Deliberately **not** a full `LayoutNode` schema: this is the wrong-container class of bug — the one
|
||
that crashes `normalizeLayout` — and a structural validator would need the client's `zoom`/`config`
|
||
vocabulary in the database layer to say anything more.
|
||
Verified live: `db:push` planned exactly the two `ADD CONSTRAINT`s plus the known
|
||
`pk_music_now_playing` churn and was stable on a second run; `update … set layout = '[]'` and
|
||
`= '3'` are both rejected by Postgres; array, number and string bodies to `ws-layout-{id}` and
|
||
`screens/{name}` all 400, and an object still round-trips.
|
||
|
||
- [x] **A bare number is squatting in a framework namespace.** _(verified in the live DB)_
|
||
`apps/Soulseek/shared.ts:12` built `screens/soulseek-zoom/${panelId}` and passed it to
|
||
`useDashboardState<number>`. The server routes `^screens/(.+)$` into `screens.layout`, so:
|
||
`user_id 1 | soulseek-zoom/soulseek-view | jsonb_typeof = number`. That namespace belongs to
|
||
layouts.
|
||
**Resolved `ba664dc`** — Soulseek's private zoom became a framework feature (`LayoutPanel.zoom`),
|
||
so the scalar now rides on the layout node inside `screens/soulseek-v2` and needs no key of its
|
||
own. The squatting row is gone: `select name from screens` returns 15 rows, none `soulseek-zoom/*`.
|
||
This is also the general answer for per-panel scalar prefs — put them on the node, not in a key,
|
||
because `useDashboardState` seeds a row per key on mount.
|
||
|
||
---
|
||
|
||
## 5. The framework work proper
|
||
|
||
### 5.1 Give panels a lifecycle — the highest-value change here
|
||
|
||
- [x] **A close hook, invoked by the mutators, not by unmount.** — done `198dc71`.
|
||
`components/Workspace/types.ts:38-47` had no close hook; the only `onClose` was
|
||
`PanelComponentEntry.onClose` (`:55`), which is a header-button handler for _ephemeral_ panels and
|
||
is suppressed on mobile (`PanelSlot.tsx:382`). So the two ways a panel dies —
|
||
`removePanel(root, id)` and `setApp(root, id, null)` (the red traffic light, `PanelSlot.tsx:295`)
|
||
— were pure tree rewrites that notified nobody.
|
||
|
||
Shipped as `usePanelClose(panelId, handler)` rather than a registry field: what needs releasing is
|
||
known to the component, not to the registry entry, and `CommandTerminalWrapper` shares one registry
|
||
key across several panels with different sessions. `WorkspaceView` fires it from `handleRemove` and
|
||
from `handleSetApp` when the app actually changes, and from nowhere else. Registration is never
|
||
torn down — honouring an unmount would reintroduce the exact bug — and handlers are stamped with
|
||
the workspace they were registered on so one dashboard's panel id cannot fire another's.
|
||
|
||
**Strike the implementation this item used to suggest** ("diff removed panel ids old-tree ∖
|
||
new-tree"). It does not work, and `layout-utils.test.ts` now pins why (`04371a9`): `movePanel`
|
||
inserts through `newPanelFrom`, which mints a fresh `uid()`, so a dragged panel's id is absent from
|
||
the new tree while its app is still on screen; and `swapPanels` exchanges `{appType, config}`
|
||
between two ids that both stay put, so a swap reads as two closes. **A panel id is a position in
|
||
the tree, not an app instance**, and a diff of positions cannot answer a question about instances.
|
||
The second half of the suggestion — have the mutators report it — is what landed, in the form of
|
||
firing at the call site that already knows the user asked for a close.
|
||
|
||
This is the thing `apps/Terminal/TerminalWrapper.tsx:31-38` asked for in writing: *"Killing on
|
||
unmount is not an option until the panel system can tell a real close from an incidental
|
||
remount."* It closes the terminal orphan leak as a consequence rather than as a special case, and
|
||
the same gap affects every panel holding a server-side resource.
|
||
|
||
**The requirement inverts for a chat panel** *(2026-08-07)* — do not wire one into this hook
|
||
without deciding first. `agent-coordination.md` §5 Q2: idle reaping deliberately leaves the
|
||
`sessionKey → claudeSessionId` pointer intact, but the explicit `disconnect` path calls
|
||
`clearClaudeSession`, which destroys it and orphans the transcript. A panel is a pointer to a
|
||
server-side session; **closing the window must not delete what it points at.** What coordination
|
||
wants from `onClose` is a guarantee that nothing rides the disconnect path, not an eager cleanup.
|
||
`ChatPanelWrapper` therefore does **not** call `usePanelClose`, and that is a decision, not an
|
||
omission — the hook's own doc comment says so, so the next person to add one reads it there.
|
||
|
||
- [x] **Then: kill the pty on real close.** — done `c92b51c`. The three wrappers now share
|
||
`useTerminalSession`, which drops the map entry and `DELETE`s the session from `usePanelClose`.
|
||
Two things fell out of doing it. `HostTerminalWrapper` still had the unmount cleanup the other two
|
||
had removed, so it had been orphaning a host shell on every layout or route change. And
|
||
`RunningShells` — the panel that exists to make orphans visible — was calling `/terminal/sessions`,
|
||
while the proxy strips `/api/terminal` and the sidecar only answers under `/_officer`; verified
|
||
live, `/sessions` returns `{"error":"not found"}`. That panel had always read "No shells running"
|
||
and its kill button had always been a no-op.
|
||
|
||
- [ ] **Then: reap orphans in the sidecar as a backstop.** `sidecars/pty/sessions.mjs:127-130`
|
||
`detach()` never checks `clients.size === 0`; there is no idle timeout and no session cap.
|
||
`lastActivityAt` is written (`:110`) and displayed (`:167`) but never read by a timer. Each orphan
|
||
costs a `SHELL -i` with the owner's full env plus up to `BUFFER_MAX = 512 KiB` (`:10`) — bounded
|
||
per session, unbounded in aggregate.
|
||
_Note: `sidecars/` is platform, so in scope — but confirm before touching the pty protocol._
|
||
|
||
### 5.2 Stop the avoidable remounts
|
||
|
||
_Tier C — **the largest downgrade in the re-rank** (`agent-coordination.md` §6.4). A remount used to
|
||
threaten whatever the panel was holding; a panel now holds nothing. A chat panel that remounts re-runs
|
||
`resume-cursor` against the durable log and replays, costing latency, which §2 of the north star
|
||
declares free. Fix these for the interaction quality they are genuinely about — scroll position, media
|
||
playback, transcodes — not as a prerequisite for agent coordination._
|
||
|
||
**Measured `56ca411`.** `WorkspaceRenderer.test.tsx` mounts the real renderer against a mount-counting
|
||
probe app and lets the real `layout-utils` mutators produce the "after" tree. The table below was written
|
||
from reading the code; the test disagrees with its *diagnosis* in every row, and found one row it had
|
||
missed entirely. Read this paragraph before acting on the bullets underneath it.
|
||
|
||
- **The key is not the cause.** A panel's React identity is its position plus `key={child.node.id}` on its
|
||
nearest ancestor group slot, but in each remounting case the **element type** at that position also
|
||
changes — `PanelSlot` becomes `ResizablePanelGroup`, or the reverse — and React reconciles by type
|
||
before it looks at keys. Reusing the panel's id so the key does not flip would therefore have changed
|
||
nothing, at the cost of putting a panel id and a group id into collision, which now matters because a
|
||
panel id is an agent's address (`agent_panels`).
|
||
- **The row that was missing was the only cheap one, and it is fixed.** `ChildEntry` returned
|
||
`<>{children}</>` for the first child and `<><Handle/>{children}</>` for every other, so a panel sat in
|
||
fragment slot 0 when first and slot 1 when not. Remove the leftmost of three panels and the second one
|
||
found a `ResizableHandle` in its old slot and remounted — for nothing that happened to it. The handle
|
||
slot is now always present, holding `null` when unneeded.
|
||
- **What is left is genuinely structural.** Wrapping a panel in a group, and a two-child group collapsing
|
||
to its survivor, both change what kind of thing lives at that position. The only fix that survives
|
||
contact with React is the one the last bullet of this section already points at: render panels from a
|
||
host that does not move when the tree reshapes, the way maximize is a CSS toggle on the same element.
|
||
That is a redesign, not a patch, and it is still Tier C.
|
||
|
||
The original table, kept because its *observations* hold even where its explanation did not:
|
||
|
||
| operation | remounts? | why |
|
||
| ------------------------------------- | ---------------------- | ------------------------------------------------------------------------- |
|
||
| split, direction **matches** parent | no | `layout-utils.ts:34-44` splices children through by reference |
|
||
| split, direction **differs** / leaf | **yes** | `:20-30` wraps in a new group with a fresh `uid()` |
|
||
| split the root when root is one panel | **yes, everything** | `WorkspaceRenderer.tsx:40` renders one _unkeyed_ child whose type changes |
|
||
| remove from a 2-child group | **yes — the survivor** | `:68-70` collapses the group, re-keying the sibling |
|
||
| swap | apps remount | `setApp` twice; the component _type_ at each slot changes |
|
||
| resize / maximize | no | ids preserved; maximize is a CSS toggle |
|
||
| mobile panel switch | yes, by design | only the active child renders |
|
||
| **any ephemeral panel on mobile** | **yes — every panel** | `WorkspaceView.tsx:165` replaces the workspace instead of overlaying |
|
||
| **viewport crossing 768px** | **yes, full** | `useIsMobile` flips the branch at `WorkspaceView.tsx:165` |
|
||
| **a panel stops being second** | **was yes — fixed** | `ChildEntry`'s fragment slots shifted under it (`56ca411`) |
|
||
|
||
- [ ] ~~**Reuse the panel's id when wrapping it in a new group**, so the ancestor slot key doesn't flip.
|
||
Kills rows 2 and 3.~~ **Withdrawn `56ca411`** — measured, and it kills neither. The element type at
|
||
that position changes too, which React acts on first. It would also collide a panel id with a group
|
||
id, and a panel id is an agent's address now.
|
||
- [ ] ~~**Don't re-key the survivor when a group collapses** (`layout-utils.ts:68-70, 83-88`). Kills row
|
||
4.~~ **Withdrawn `56ca411`**, same reason: the survivor changes type as well as key.
|
||
- [ ] **Overlay the mobile ephemeral panel instead of replacing the workspace**
|
||
(`WorkspaceView.tsx:165`). Affects `/files`, `/email`, `/chat`, `/browser`, `/dashboards`.
|
||
- [ ] **Reference for how it should feel:** maximize (`PanelSlot.tsx:430-457`) is a CSS state toggle on
|
||
the same element — no portal, no remount, scroll position and media playback preserved. Every
|
||
mutator should be held to that.
|
||
|
||
### 5.3 Drag-to-move: finish it or delete it
|
||
|
||
- [ ] **Decide, then act — do not patch it.** `dragSourceId` can never become non-null: the only code
|
||
that would set it is commented out at `PanelSlot.tsx:240-256`. So the whole path is dead:
|
||
`DragOverlay.tsx` (99 lines, imported by nothing), `LayoutEditor.tsx` (51, imported by nothing),
|
||
`movePanel` + `insertPanel` + `DropPosition` (`layout-utils.ts:142-169`), and three context fields.
|
||
~180 lines.
|
||
**If finishing it:** `movePanel` mints a _brand-new_ panel id, so it destroys all panel-keyed
|
||
state, and `PanelContents` (`layout-utils.ts:229`) carries only `{appType, config}` — so a move
|
||
**silently drops `fitContent` and `zoom`**. Fix before re-enabling.
|
||
**Downgraded 2026-08-07** — this was on the critical path in `agent-coordination.md` §1.5 (P3:
|
||
"dragging a panel would silently sever its session binding") and no longer is. Q3 made the agent's
|
||
_name_ the address and the panel id merely where it lives, and `e588524` made move and swap carry
|
||
`config` with the panel. `useAgentPanel` resolves by name and re-anchors the row afterwards. Still
|
||
worth deciding — it is dead code — but it gates nothing. See `agent-coordination.md` §6.4.
|
||
|
||
### 5.4 Make `normalizeLayout` framework, not convention
|
||
|
||
- [x] **One normaliser applied inside `WorkspaceView`.** _(`1de1d92`)_ — it landed as a prop rather
|
||
than a factory: `<WorkspaceView appTypes={{ allowed, fallback }} />`, with `normalizeLayout` in
|
||
`layout-utils.ts` and the persist-back inside `WorkspaceView`. All fourteen copies are deleted;
|
||
QrTransfer gained the persist-back it never had. Two notes for whoever reads this next. The
|
||
framework normaliser **drops `config`** when it replaces an app — the copies kept it, which left
|
||
the old app's config on a panel the new app now owns, the opposite of `setApp`. And Headscale
|
||
keeps a local `useMemo`, because its check is "is the server picker present at all", which is
|
||
about a panel being _missing_ and no allow-list can see that.
|
||
Still open from this item: deriving the allow-lists from the AppRegistry (they are still
|
||
hand-written literals, so a type still allow-listed but deleted from the registry passes and
|
||
reaches the unrecoverable empty box at `PanelSlot.tsx:311-317`). The unguarded screens are done
|
||
(`653201f`): Dashboards, Desktop, Terminal and Files now pass one, checked against what is
|
||
persisted rather than against their defaults; Browser and Email deliberately do not, because
|
||
their panels resolve through `components`, which `PanelSlot` keys on the panel id, so their
|
||
`appType` is never read. Home and the two dashboard screens are not `locked` and stay open.
|
||
The original text follows.
|
||
|
||
**One `makeLayoutNormalizer(allowed, fallback)` applied inside `WorkspaceView`.**
|
||
The guard currently exists **14 times, character-identical except for the allow-list and the
|
||
fallback**, plus a matching `useMemo`/persist-back `useEffect` pair 13 times — ~150 duplicated
|
||
lines. Consequences of it being convention: - **10 `WorkspaceView` consumers have no guard at all**, including `screens/HomeScreen.tsx:11`,
|
||
which also omits `locked` — the one screen where a user can set any registry appType with
|
||
nothing pinning it back. - Allow-lists are hand-written literals, never derived from the AppRegistry. They catch a
|
||
_renamed_ appType; they do **not** catch one still allow-listed but deleted from the registry —
|
||
that passes and reaches `PanelSlot.tsx:311-317`, which on a `locked` screen renders an empty
|
||
teal-bordered box with no picker and no way for the user to recover. - `screens/QrTransferScreen.tsx:19-39` has the guard but no persist-back, so it re-normalises on
|
||
every mount forever and never heals the row.
|
||
- [x] **~~Then collapse the three default-layout mechanisms~~ — inventoried and dropped.** Per-screen
|
||
`defaultLayout.ts` (21, not 20 — `Home/defaultLayout.tsx` is misnamed), `createDefaultLayout()`
|
||
in the core, and the 6-entry template array at `DashboardPreview.tsx:33-142`.
|
||
|
||
They are not three copies of one mechanism. `createDefaultLayout()` is a one-liner meaning "the
|
||
empty state of a workspace" and the templates are the picker for a *user-created* dashboard,
|
||
where the user chooses the apps afterwards. The 21 files are *built-in locked screens*, where
|
||
the apps are fixed. Collapsing across that line would merge two different questions.
|
||
|
||
Nor are the 21 duplication. Fourteen share a shape — flat horizontal split, nav on the left —
|
||
but across seven different size pairs, with panel ids that other code hardcodes (`components`
|
||
maps, `mobilePanelId`, `ChatPanelWrapper panelId=…`, and now the `appTypes` allow-lists). A
|
||
`splitH(...)` helper would turn 11 lines into 1 without removing a single decision: still 14
|
||
call sites, still 14 argument sets, plus a layer between "what does /music look like" and the
|
||
answer. That is compression, not deduplication.
|
||
|
||
What the inventory did turn up, none of which is an abstraction:
|
||
- **`DashboardPreview`'s edit path reset the layout on every submit** — renaming a dashboard or
|
||
editing its description threw away the panel arrangement, and dropped the terminal maps on a
|
||
rename. Fixed and verified against the running server (`cd1f161`).
|
||
- `Home/defaultLayout.tsx` is a `vertical` group with one child at `size: 100` — a bare panel
|
||
with a wrapper that does nothing, unlike Files/Terminal/Desktop next to it. Cosmetic: the
|
||
persisted `screens/home` row is what actually renders, so changing it changes nothing today.
|
||
- Browser and Email are the one genuine copy-paste pair (same nesting, same 25/75 and 60/40,
|
||
same trailing chat panel). One duplicate, and the two screens are diverging anyway.
|
||
|
||
### 5.5 Persistence hygiene
|
||
|
||
_Three Tier A items live here (`agent-coordination.md` §6.2, A5 and A7). The two lost-update bugs are
|
||
worse than described now that identity is in the layout: a debounce timer closing over a pre-drag tree
|
||
does not just resurrect a deleted panel, it **re-writes an older `config`**, so a panel that was just
|
||
named reverts to anonymous. And the never-invalidated cache means two windows onto the same server-side
|
||
work disagree permanently about the roster, with neither told — a direct contradiction of §5 Q1._
|
||
|
||
- [x] **The resize debounce can resurrect a deleted panel.** `WorkspaceRenderer.tsx:108-124` held a
|
||
500 ms timer in a ref with **no `useEffect`, therefore no cleanup**, and its callback closed over
|
||
the layout as it was when the drag began. Drag a splitter, remove a panel within 500 ms → the timer
|
||
PATCHed the pre-deletion tree and the panel came back. Same shape for a split.
|
||
**Resolved `b0a32ae`** — the timer now lives behind a `useEffect` that **flushes** it on unmount
|
||
rather than dropping it: with the write expressed as an updater the early write is correct, and
|
||
dropping would lose a splitter drag made just before navigating away, which the no-cleanup version
|
||
did at least persist.
|
||
- [x] **Concurrent same-key writes clobber.** A window resize fires `onLayout` on every group at once;
|
||
each scheduled its own timer against the same base tree and the last won
|
||
(`WorkspaceView.tsx:76-81`).
|
||
**Resolved `b0a32ae`** — all eight mutations in `WorkspaceView` pass an **updater** to `setValue`
|
||
(`onLayoutChange((prev) => …)`) instead of a tree computed from the `layout` their callback closed
|
||
over, so each composes against the current cache. The no-op guards (`countPanels`, the `=== layout`
|
||
identity checks) 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.
|
||
Worth keeping in mind for anything new that writes the layout: **compute from `prev`, always.** The
|
||
cost of getting it wrong is no longer a size that snaps back — it is an older `config`, so a panel
|
||
that was just given an agent's name reverts to anonymous and the agent stops being addressable.
|
||
- [x] **The cache is never invalidated.** `staleTime: Infinity` and no `invalidateQueries` anywhere in
|
||
the repo. Meanwhile every PATCH computed and returned a full fresh state blob which the client
|
||
**discarded** — 3 SELECTs per splitter release, thrown away.
|
||
**Resolved `81ad3ef`** — both halves. The PATCH returns `{ok: true}`; nothing had ever read that
|
||
body, and a caller that did would be reading state assembled *before* whatever concurrent write it
|
||
raced. The client refetches **on focus**, with three non-default guards, because this cache is
|
||
optimistic: a refetch that started before an in-flight PATCH landed would overwrite the value
|
||
already on screen — the same lost-update shape as the two items above, and self-healing only until
|
||
the next mutation composes on top of the stale tree. So `refetchOnMount: false` (splitting a panel
|
||
mounts a fresh consumer, which is exactly when a write is in flight), `refetchOnReconnect: false`,
|
||
and `refetchOnWindowFocus` gated on a module-level in-flight count plus a 2 s quiet period.
|
||
- [x] **Preserve sibling sizes on split.** `splitInner`/`insertPanel` redistribute evenly
|
||
(`100 / newChildren.length`), so one split discards carefully tuned proportions.
|
||
**Resolved `abea7a3`** — the new sibling takes half of the *target's* size and nothing else moves.
|
||
One helper serves both call sites, because the drop path (`movePanel` → `insertPanel`) carried the
|
||
identical bug. Two of the three tests were already in `layout-utils.test.ts` asserting the even
|
||
split, written to the old behaviour deliberately; they now assert the new one. The third documents
|
||
the interaction worth knowing: `removePanel` renormalises the group as the panel leaves, so a move
|
||
reads as renormalise-then-halve, not halve alone.
|
||
- [x] **`tpl-N` panel ids collide across dashboards.** `DashboardPreview.tsx:25-26` —
|
||
`let tplCounter = 0; const tplUid = () => \`tpl-${++tplCounter}\`` — module-level, no entropy,
|
||
resets every page load. Two dashboards created from templates after a reload hold panels with
|
||
**identical ids**, and panel id keys `terminal-conn-${panelId}`and`file-viewer:${panelId}`.
|
||
**Resolved `6fd60e5`** — the templates now call the core's `uid()`, which is exported from the
|
||
Workspace barrel for the first time so that there is exactly one way to mint a panel id. The
|
||
duplicate minter is deleted rather than fixed: a second implementation of "make me an id" is how
|
||
this happened, and the collision was no longer only a settings mix-up — `agent_panels` addresses a
|
||
panel by `(dashboardId, panelId)`, so two dashboards built from templates in the same page load
|
||
could hand two different agents the same address.
|
||
|
||
### 5.6 Registry
|
||
|
||
- [x] **No duplicate-key guard.** `metasToRegistry` is `Object.fromEntries`
|
||
(`AppRegistry/useAppRegistry/useAppRegistry.ts:19`) — a collision silently last-wins and one app
|
||
just disappears.
|
||
**Resolved `6fd60e5`**, but not by throwing, which is what this item asked for. A duplicate key is
|
||
an authoring mistake made at edit time; throwing punishes the owner at runtime for it, by taking
|
||
down every dashboard rather than one app. So it is a **test over the real meta list**
|
||
(`AppRegistry.test.ts`) plus a `console.error` at runtime — the mistake is caught before it ships
|
||
and named if it somehow does. Confirmed: all 44 keys are unique today, and the test says so.
|
||
Getting the real list into a test needed one thing beyond exporting it: `test-setup.ts` was not
|
||
providing `localStorage`, and `MusicPlayer/useLyricsOpen.ts` reads it at *import* time, so the
|
||
whole app graph was unimportable from a test. That is now fixed, which unblocks testing anything
|
||
else that pulls in a panel app.
|
||
- [x] **Seeding depends on undocumented mount ordering.** Three call sites call `useAppRegistry()` with
|
||
no argument, defaulting to `[]`. It works only because `<AppRegistry />` sits at `frontend.tsx:32`,
|
||
above `<App />` at `:34`. Mount a `WorkspaceView` above that and every panel renders empty.
|
||
**Resolved `9fcc9c2`** — and the mechanism is worse than "ordering", which is why the fix is not
|
||
"move the component up". `<AppRegistry />` seeded through `useGlobal`'s `initialData`, and
|
||
**`initialData` is not a write**: it applies only to whichever component reads the slot first. A
|
||
`WorkspaceView` that rendered first would create the slot as `{}` and the seeder had no second
|
||
chance — every panel on that screen an empty box, for a reason visible nowhere near it.
|
||
Both registries are now written into the query cache by `seedAppRegistry` / `seedWidgetRegistry`
|
||
before `createRoot().render()`, so there is no ordering to depend on. They take the `QueryClient`
|
||
rather than running as a module-scope side effect because the app list imports every panel app and
|
||
every panel app imports the Workspace framework; keeping the call in `frontend.tsx` — the one
|
||
module that is nobody's dependency — is what stops that being an import cycle. (Making
|
||
`useAppRegistry` simply default to the static list was the obvious fix and is exactly that cycle.)
|
||
`registerApp` and `registerWidget` are deleted with the components: nothing ever called either, and
|
||
a registry that can be added to at runtime is a registry whose contents depend on what has mounted.
|
||
- [x] **`officerdev/file-viewer` is a dead registration.** Its provider reads
|
||
`usePanelChannel('file-viewer:' + panelId)` and **nothing in the repo writes that channel**. Also
|
||
`availableOnPanel: false`, so it can't be picked. If it ever appeared in a layout it would say
|
||
"No file selected" forever.
|
||
**Resolved `9fcc9c2`** — traced and confirmed dead, then removed rather than repaired. The file
|
||
viewer that users actually see is mounted by `useFileViewerPanels` as an *ephemeral* panel, which
|
||
supplies `FileViewerBody`/`FileViewerHeader` itself with a provider reading the path from
|
||
`?view=`/`?ephemeral=` — it never touched the registry. No stored layout referenced the key
|
||
(checked across `dashboards`, `screens`, `dashboard_defaults`, `user_state`, `user_settings`: zero
|
||
rows), so the meta and `FileViewerPanelWrapper.tsx` are gone. Side effect worth knowing: no app in
|
||
the registry now uses the `provider` field. It is kept, because the ephemeral path
|
||
(`PanelComponentEntry.provider`) still does and `PanelSlot` resolves the two symmetrically.
|
||
- [ ] **`dashboard-list` is pickable in every panel on every dashboard.** `apps/Dashboards/index.ts:12-16`
|
||
omits `availableOnPanel` while its sibling `dashboard-preview` sets `false`. Almost certainly
|
||
unintended — the flag is opt-_out_, and 12 of 44 metas omit it. Consider making it opt-_in_.
|
||
|
||
### 5.7 Effect hygiene in panel apps
|
||
|
||
All the same bug: an app guessing "am I being closed?" from an unmount, or paying for a remount from 5.2.
|
||
|
||
- [ ] **PARKED — `useClient` identity / `DesktopView` deps.** Revisit later, not now.
|
||
`useClient` returns a fresh object every render (`workspaces/hooks/src/useClient.ts:30-38`), but
|
||
it is consumed by calling verbs at call time, which is identity-agnostic. **Do not memoize it.**
|
||
Only two dep-array sites exist: `apps/Desktop/DesktopView.tsx:205` (one redundant connect at
|
||
mount — React bails on the unchanged `setStatus`, so it settles rather than looping) and
|
||
`hooks/useChat.ts:124` (a `useCallback`, harmless). Owner has used this pattern for years without
|
||
issue; my first write-up called it a live bug and that was overstated.
|
||
- [x] **`DictateDialog` never releases the microphone.** — _resolved `c0fae47`_
|
||
`return () => { if (!showDictate) cleanup(); }`. The cleanup that runs on `true → false` is the one
|
||
registered by the `true` render, where `showDictate` is `true`, so it never fires. Mic stream,
|
||
`AudioContext` and the rAF loop leak. `apps/QrTransfer/Receiver.tsx:120` is the correct version.
|
||
Fixed by dropping the guard and reading `cleanup` through a ref, so the teardown runs on close and on
|
||
unmount and is the current one either way. `cleanup` also had to become safe to call twice — the
|
||
success path in `stopRecording` releases the hardware early, and closing an already-closed
|
||
`AudioContext` rejects.
|
||
- [x] **`useAudioRecording` has no unmount cleanup at all** (`apps/Chat/useAudioRecording.ts` — no
|
||
`useEffect`). Unmounting Chat mid-recording leaves the mic open for the life of the tab.
|
||
— _resolved `c0fae47`_, and pinned by `useAudioRecording.test.tsx`: the hook is rendered, put into
|
||
recording, and unmounted, and the test asserts the track was stopped.
|
||
- [x] **`HostTerminalWrapper` still has the cleanup the other two deliberately removed.** — _resolved
|
||
`c92b51c`_, before this section was re-read. All three wrappers now share `useTerminalSession`, which
|
||
keeps the `panelId → sessionId` map across unmounts and drops it on a real `usePanelClose` — the §5.1
|
||
signal is what made a correct answer possible.
|
||
- [ ] **REVISED — `useTaskRunner` does not abandon the running task; it kills it.** The original entry was
|
||
wrong twice, both times by reading only the hook. `stop` **is** sent —
|
||
`TaskRunnerModal.tsx:1320` renders a Stop button while `phase === 'running'`. And a bare `ws.close()`
|
||
is not abandonment: `task-executor.ts:303-309` kills the process tree on socket close, the same
|
||
`killTree` the Stop button reaches. What is true is the last clause: there is no re-attach, so an
|
||
inline run dies with its modal. That is defensible — inline is the *ephemeral* mode and the job path
|
||
exists for everything else — so this is left alone deliberately rather than left undone.
|
||
- [ ] **`VideoPlayer` kills the transcode on incidental unmount.** `apps/Jellyfin/VideoPlayer.tsx:217-223`
|
||
POSTs `stopped`, killing server-side ffmpeg, then renegotiates. Fires on every "yes" row in 5.2 —
|
||
one row fewer since `56ca411`. Left as is: the player renegotiates from scratch on remount anyway, so
|
||
the stop is correct, only expensive. The real fix is 5.2's, not this file's.
|
||
The misleading comment at `:215-216` is corrected in `c0fae47`: `sendReport` is `useCallback(…, [])`, so
|
||
naming it is equivalent to an empty list — and that is now written down as a constraint, because if
|
||
it ever grows a dependency this becomes the Continue Watching bug again.
|
||
- [x] **Two stale-closure sockets.** — _resolved `c0fae47`_, both with the ref pattern rather than by
|
||
restarting the socket. `usePipelineRunner` kept the first render's `handleEvent`, whose `flushStream`
|
||
carries a captured `streamingText`; `AudioStreamPlayer` kept the first `onError`.
|
||
- [x] **`PanelSlot` defines a component inside render.** — _resolved `c0fae47`_. `DefaultHeader` is gone: the
|
||
header is now an element, not a component type, so there is nothing for React to fail to match.
|
||
- [x] **The context value is a fresh literal.** — _resolved `c0fae47`_. `useMemo` over the eighteen members.
|
||
Note what it does *not* buy: the value still changes whenever `layout` does, because half the
|
||
callbacks close over it. What it stops is the renders that change nothing a panel can see — the
|
||
ephemeral pane opening, a mobile panel switch, every frame of a maximize animation.
|
||
|
||
### 5.8 Navigation — finish the refactor
|
||
|
||
- [x] **Move the seven channel-driven apps' selection into the URL.** _Six of the seven are done; the
|
||
seventh (Chat detail) is chat-owned and deliberately untouched — see the foot of this item._
|
||
Split when this was written: 11 URL-driven
|
||
(everything post-refactor), 7 channel-driven, 3 on bare `useState`. The channel-driven set: Music
|
||
(`music:cwd`), Soulseek sections, SystemMonitor (`monitor:scope`), Chat detail
|
||
(`chat:selected-session`), Email, Browser, and all five Settings pages (one fix point,
|
||
`SettingsPanel.tsx:56`).
|
||
**This is chronological, not architectural** — panels render as ordinary children of the screen's
|
||
Route element (`PanelSlot.tsx:333`), so every one of them can call `useParams` today. Nothing in
|
||
the framework needs to change.
|
||
Symptom: `/soulseek` reopens in your last section and `/music` at your last album while the URL
|
||
says the bare route. `usePanelChannel` is a slot on **one process-global QueryClient**
|
||
(`frontend.tsx:15-21`) with no per-route scoping, and `reset` is called nowhere.
|
||
Authority: `docs/navigation-audit.md`.
|
||
|
||
**Settings is done** (audit M6). Five pages moved to `/settings/:page/:section`: the section is
|
||
`useParams`, the sidebar entry is a `<NavLink>` instead of a `<button>` with the key in its closure,
|
||
and each page renders one `SettingsRoute` guard that canonicalises the bare route and an unknown
|
||
section. Six globals went with it — the five `*_SELECTED` keys and `INTEGRATIONS_SETTINGS_TAB`.
|
||
Two things the doc's "one fix point" framing had wrong. Integrations does **not** go through
|
||
`createSettingsPanelComponents` — it builds its own sidebar, so it needed its own conversion. And
|
||
its Enterprise/Personal tab was a *second* global, which is why deep-linking a Personal section
|
||
could never have worked: the link set the section, the tab stayed on Enterprise, and the content
|
||
pane rendered "Select a section" for a section that existed. The tab is derived from the section
|
||
key now, so there is nothing left to disagree with.
|
||
**SystemMonitor is done too** (audit M10) — `/system-monitor/:scope`, scope list as `NavLink`s,
|
||
`monitor:scope` deleted. Smallest of the seven and the same shape as `/photos`.
|
||
**Soulseek is done** — `/soulseek/:section`, and the peer moved to `?user=<name>` rather than the
|
||
`/soulseek/users/:name` the audit sketched: a second path segment would have needed a nested route
|
||
just to keep the nav's `NavLink` highlight, and `?search=` was already establishing the convention
|
||
for that workspace. Deleting `soulseek:user` also deleted the nonce it carried — a request stamped
|
||
`{username, nonce}`, consumed-once by the Users panel so that returning to the section did not
|
||
re-run the lookup, and nonce-stamped so that asking for the *same* peer twice still counted twice.
|
||
A link needs none of that: it is idempotent, so there is nothing to consume and nothing to
|
||
disambiguate. That is the shape of most of this section's remaining work.
|
||
**Email is done** (audit H5) — and it is the one of the seven where the route already existed and
|
||
the work was pure deletion. `/email/:emailId` has been a route pair all along; what sat on top of
|
||
it was an `EMAIL_SELECTED` global plus two effects copying state into the URL and the URL back into
|
||
state. That made the address a *mirror* rather than the source: rows stayed `<button>`s with the id
|
||
in a closure, and Back raced the effect that had just rewritten the URL with `replace`. List,
|
||
reader and the mobile-panel switch now each read `useSelectedEmailId()` (`Email/shared.ts`) and the
|
||
rows are `<Link>`s, so cmd-click opens a mail in a tab for the first time. Two decisions worth
|
||
recording: no `<Navigate>` guard, because unlike a section route the bare `/email` is a real state —
|
||
the list with nothing open, exactly as `/chat` and `/jobs` — and arrow-key browsing navigates with
|
||
`replace`, since sweeping a folder would otherwise stack one history entry per row and Back would
|
||
walk the sweep instead of leaving the mailbox. `EMAIL_FOLDER` stays a global for now: it is read in
|
||
one component and is view state rather than selection. Putting it in `?folder=` is a real but
|
||
separate item.
|
||
**Browser is done** — `/browser/:tabId`, `BROWSER_SELECTED_TAB` deleted from its three readers.
|
||
The audit had this one filed under "probably leave as selection", on the grounds that a CDP target
|
||
id dies with the browser session so a durable URL is dubious. That is an argument about
|
||
*bookmarking*, and bookmarking is the least of what the URL was buying: the id lived in an onClick
|
||
closure, three components shared a global, and a tab row could not be cmd-clicked. The staleness
|
||
the audit worried about is real and is now handled where it shows — the preview tells "no tab open"
|
||
from "that tab is no longer attached" by checking the polled target list, which both panels read
|
||
from one React Query key, so it costs no extra request. Two defects fell out on the way: the row's
|
||
Focus and Close buttons were nested inside the row `<button>` (invalid HTML, held together by two
|
||
`stopPropagation` calls — they are siblings of the anchor now), and "Set up in Integrations" was a
|
||
raw `<a href>` that reloaded the whole SPA.
|
||
**Music is done** — the last of the seven that is mine, and the largest: the location touched four
|
||
files across two packages. It is `/music?path=<rel>`, `rel` relative to the `Music` root, and
|
||
`music:cwd` is deleted. A query param rather than the nested route the audit left open, for two
|
||
reasons: the location is only one of the things this screen holds (the lyrics split and the
|
||
favorites view are the others), and a splat has to be a route's last segment, which is the same
|
||
constraint `/chat` hit when it spelled its group `/chat/g/*`. `useMusicCwd()` returns the location
|
||
in the home-relative `Music/…` vocabulary the panels already spoke, so reading the URL cost each
|
||
panel one line and no rewrite.
|
||
What fell out of it is the point of the whole section. `MusicBrowser` published, `MusicDetail` and
|
||
`FavoritesView` subscribed, and `MusicPlayerHost` — mounted *outside* `<Routes>` — wrote the channel
|
||
and then called `navigate('/music')` to make the write visible, which is navigate-site 13 and the
|
||
only "navigate with a side-effect" in the audit's table. With the location in the URL that pair
|
||
collapses to one `<Link>`, and the host stops importing the library's channel at all. Every
|
||
drill-in is a link now: library rows, folder rows, album and artist cards, both "up" affordances,
|
||
the favorites rows, and the dock's now-playing tile. Track rows stay `<button>`s — they play, which
|
||
is a mutation, not a location. `MusicDetail`'s once-only auto-nav to the playing album navigates
|
||
with `replace`, on the same reasoning as Email's arrow keys: landing on `/music` and being moved to
|
||
the playing album is one arrival, so Back should leave the screen rather than undo a jump the user
|
||
never asked for. Two channels survived deliberately: `music:resync` (a refresh signal, never
|
||
selection) and `music:favorites` (a view of one panel). The favorites one needs a note — going Home
|
||
or opening a favorite has to close it *by hand*, because navigating to where you already are fires
|
||
no route change and the effect that clears it keys on the location.
|
||
Remaining here: **nothing that is mine.** Chat detail (`chat:selected-session`) is chat-owned —
|
||
leave it; it is written up in `COMMS/`. Soulseek's rooms and conversations are still `useState`,
|
||
noted in the audit and not scheduled.
|
||
|
||
### 5.9 The context has grown an app-config section — _(found 2026-08-07)_
|
||
|
||
`WorkspaceContext` is 18 fields, of which the framework itself reads none of the first six. Apps never
|
||
touch the framework half, so the abstraction holds in one direction; the leak is entirely outbound.
|
||
|
||
**§5.9 is closed as of 2026-08-07.** The context is 15 fields, and the outbound half is `workspace`,
|
||
`cwd`, `root` — all three facts about *where the panel is*, which is the one thing a framework of this
|
||
shape genuinely owes an app. Nothing left on it is an app's vocabulary: the file-browser pair is
|
||
deleted, the chat's system prompt is a prop on the chat, and the key three apps used to parse is a
|
||
parsed identity. The two hand-written copies of the inert half are one named constant.
|
||
|
||
- [x] **Delete `initialFilePath` and `defaultFileSort`.** Declared `WorkspaceContext.ts:14-15`, plumbed
|
||
through `WorkspaceView.tsx:19-20,30`, read only by
|
||
`apps/FileBrowser/FileBrowserApp/FileBrowserPanelWrapper.tsx:7,9,10` — and **set by zero callers**.
|
||
`DefaultFileSort` (`{field: 'name'|'size'|'type'|'date'}`) is file-browser vocabulary living in the
|
||
framework's type file, and it is re-exported from the barrel (`Workspace/index.ts:27`). Pure leak,
|
||
no payoff, entirely deletable.
|
||
**Gone in `717580f`**, along with the two barrel re-exports. Deleting them orphaned the props they
|
||
were feeding, so `585f234` took out the layer below: `FileBrowserApp`'s `initialPath`/`defaultSort`,
|
||
the duplicate `DefaultSort` type in two files, and `useFileBrowserApp`'s `isolated` — which was
|
||
`!!initialPath` and so had always been false. No behaviour changed at either level; both were
|
||
already running on the defaults.
|
||
- [x] **Move `promptPrefix` onto the component, not the context.** `WorkspaceContext.ts:16` →
|
||
`Chat/ChatPanelWrapper.tsx:78` → `useEmbeddableChat.ts:107`. Set by `EmailScreen.tsx:53` and
|
||
`BrowserScreen.tsx:35`, each a screen-local ~40-word system prompt. The framework is a courier for
|
||
a string only one app understands, and the `components` prop already exists for exactly this —
|
||
Email can supply a pre-configured chat by panel id.
|
||
**Done in `d3922bd`**, exactly that way: both screens put their own `ChatPanelWrapper` in
|
||
`components` under the chat panel's id and pass the prefix as a prop. `PanelSlot` prefers a
|
||
`components` entry over the registry for the *body* only, so the panel keeps its registry header —
|
||
the screens did not have to reproduce any chrome. `ChatPanelWrapper` is exported from the barrel
|
||
for it. The same prop came off `WorkspaceLayout`, where it had no callers at all: every settings
|
||
pane and job detail rendering through it had always been passing its chat panels `undefined`.
|
||
- [x] **`dashboardId` is a bag whose _format_ three apps parse.** It is literally `workspace.key`
|
||
(`WorkspaceView.tsx:163`). Consumers reverse-engineer meaning from its shape:
|
||
`Chat/ChatPanelWrapper.tsx:51-56` does `dashboardId === 'email' || dashboardId === 'screens/email'`
|
||
→ email context, and `!startsWith('screens/')` → dashboard context — **so renaming a screen key
|
||
silently changes the agent's system context**. **Worse since 2026-08-07:** the agent address book
|
||
(`agent_panels.dashboard_id`, via `useAgentPanel`) is keyed on this same string, so changing the
|
||
derivation orphans every named agent on that dashboard — the rows and sessions survive and the
|
||
panels can no longer find them. Promoted to Tier B in `agent-coordination.md` §6.3.
|
||
`TerminalWrapper.tsx:13-16` regexes it;
|
||
`HostTerminalWrapper.tsx:12` doesn't (§2). Give the context the parsed facts
|
||
(`{ kind: 'screen'|'dashboard', id }`) instead of the raw key, and the three parsers collapse.
|
||
**Done in `dbe585f`.** `workspace: WorkspaceIdentity | null` replaces `dashboardId`, parsed once in
|
||
`WorkspaceView` by `parseWorkspaceKey` and memoised on the key — the identity lands in
|
||
`useAgentPanel`'s query key, and a fresh object per render there is the `useClient()` trap that
|
||
disabled the Jellyfin playback reports. The identity carries `key` as well as `kind`/`id`, because
|
||
the raw key is the address `agent_panels.dashboard_id` stores: parsed halves are for deciding, the
|
||
key is for storing. Two behaviours tightened deliberately — an unrecognised key is no longer
|
||
treated as a dashboard (the old `!startsWith('screens/')` would have let a panel register an agent
|
||
against a workspace with no row to hang it on), and `terminalStateKey` now takes the identity, so
|
||
it can no longer nest a key inside itself. Seven tests in `workspace-identity.test.ts`, and the
|
||
two live agents on `ws-layout-agent-mvp` were confirmed still addressable after the restart.
|
||
- [x] **`WorkspaceLayout.tsx:36` silently omits `root`** — apps inside a `DashboardPreview` fall through
|
||
to the `createContext` defaults. (`initialFilePath` and `defaultFileSort` were on this list too and
|
||
are now deleted; `workspace` is omitted there on purpose, since a preview is not a place an agent
|
||
can be addressed.) Whatever survives the three items above should be constructed in one place, not
|
||
twice by hand.
|
||
**Done in `ca046a3`.** The eleven interaction fields — everything a panel uses to rearrange the
|
||
workspace, plus the state those interactions run on — are one exported `inertInteraction`, spread
|
||
by `WorkspaceLayout` and by the `createContext` default. `root` stays omitted, and that is now a
|
||
stated decision rather than an oversight: it is only ever read when `cwd` is scoped, and no caller
|
||
of `WorkspaceLayout` passes a `cwd` at all, so there is nothing for it to be the root *of*.
|
||
|
||
### 5.10 Channel hygiene — _(found 2026-08-07)_
|
||
|
||
Cheap to fix, and prerequisites for the 5.8 migration rather than alternatives to it.
|
||
|
||
- [x] **Four channels are bare string literals with no constant.** `'files:refresh-signal'` (repeated in
|
||
4 files), `'chat:selected-session'` (3 files — plus a module-private `CHANNEL` const in a 4th place
|
||
that only one of them uses), `'chat:active-session'`, `'preview:refresh'`. A typo silently yields a
|
||
fresh channel pinned to `initialData`; nothing errors. Export a constant per channel, next to its
|
||
payload type.
|
||
- [x] **Payload types are per-call-site, not per-channel.** `usePanelChannel<T>` takes `T` from each
|
||
caller, so a publisher and a subscriber can disagree and nothing checks.
|
||
`'files:refresh-signal'` is `number` in all four places by convention only. A
|
||
`defineChannel<T>(name)` helper returning a typed hook would fix both this and the item above.
|
||
**Both resolved `c973558`** — `defineChannel<T>(name, initial)` in `hooks/usePanelChannel.ts`
|
||
returns the hook, and `officerdev/src/channels.ts` declares the four. Every one of the twenty
|
||
remaining `usePanelChannel` call sites in the repo now passes a shared constant; there are no bare
|
||
literals left.
|
||
- [x] **Two write idioms disagree on the same channel.** `files:refresh-signal` is bumped with
|
||
`Date.now()` at the Chat sites and `setRefreshSignal((n) => n + 1)` at the FileViewer sites — and
|
||
`useGlobal`'s functional form applies against the **render-time** snapshot (`useGlobal.ts:18`), so
|
||
two increments in one render window collapse into one. Standardise on the nonce.
|
||
(`useLyricsOpen.ts:19-23` already documents avoiding the functional form for this reason.)
|
||
**Resolved `c973558`**, but not on `Date.now()` — that has the same bug at millisecond scale, and
|
||
the four FileViewer bump sites sit close enough together (save, delete, extract, transcribe) to hit
|
||
it. `useFilesRefresh()` exposes a `bump` over a module-level counter that never reads React state,
|
||
so it is correct however many times it is called between renders. `bump` is also identity-stable
|
||
through a ref, because `useGlobal`'s setter is a fresh closure every render and this one goes into
|
||
dependency lists.
|
||
- [x] **`system-settings:run-command` has no writer.** `run-command-channel.ts:6`; the only two writes
|
||
(`SystemSettings.tsx:87,133`) are both _clears_, and the sibling ServerSettings sections never
|
||
import it. The panel it drives — a terminal that opens with a command pre-loaded — appears
|
||
unreachable. Wire it or delete it; add to §8 either way.
|
||
**Deleted `c973558`.** It had a writer once: `7c0b11c` ("sudo commands through ephemeral
|
||
terminal") wired the AI harness installer to it. That install moved server-side to
|
||
`POST /server-settings/chat-providers/install` and the write went with it, leaving the channel, the
|
||
`SystemTerminalPanel`, and a whole second `splitLayout` that nothing could select. Gone — the path
|
||
it existed for no longer wants a terminal.
|
||
- [x] **`PanelComponentEntry.component` is typed with no props** (`types.ts:60-62`) but
|
||
`PanelSlot.tsx:521` passes `panelId` at runtime. `components`-supplied panels get a prop they
|
||
cannot see; registry apps get the honest `{ panelId: string }`. One-line type fix.
|
||
**Resolved `c973558`** — `component`, `header` and `provider` all take `{ panelId: string }` now,
|
||
since `PanelSlot` renders all three with it. A no-prop component is still assignable, so no screen
|
||
changed.
|
||
|
||
---
|
||
|
||
## 6. Decisions needed — not defects, don't guess
|
||
|
||
- [ ] **Mobile: thread `mobilePanelId` everywhere, or delete the collapse?**
|
||
`WorkspaceRenderer.tsx:143-165` collapses any horizontal group with >1 child to a single child,
|
||
choosing `mobilePanelId` or falling back to `children[0]`. **Only 4 of 23 consumers pass it.** So
|
||
`/wallet`, `/music`, `/calendar`, `/contacts`, `/soulseek`, `/system-monitor`, `/headscale`,
|
||
`/gitea`, `/photos`, `/invoices`, `/jellyfin`, `/transmission` render **the nav sidebar only** —
|
||
content permanently unreachable, no tab bar, no indicator, no affordance. User-created dashboards
|
||
(`screens/DashboardScreen.tsx:22`) show only their left column, forever.
|
||
Deleting the collapse _repairs_ 16 screens (cramped but complete) at the cost of the 4 using it
|
||
correctly. Threading it everywhere is 19 small edits and a design question about how you switch.
|
||
Also note the collapse only applies to **horizontal** groups.
|
||
|
||
- [ ] **The known "invisible edit form on mobile" bug is a sub-case of the above.** It is a mutual
|
||
exclusion, not a layout glitch: the edit pencil is reachable only while `selected === null`, and
|
||
the form renders only while `selected !== null`. **Commit `4970e7e` could not have fixed it** —
|
||
its change is a no-op on mobile. Recorded as deliberately-unfixed in `CLAUDE.md` pending the
|
||
native app; worth re-deciding now that the cause is known.
|
||
|
||
- [ ] **Drag-to-move: finish or delete?** See 5.3. ~180 lines of dead code with a known defect in it.
|
||
|
||
---
|
||
|
||
## 7. Landed
|
||
|
||
_(move items here with the commit and a one-line resolution)_
|
||
|
||
- [x] **Editing a dashboard no longer resets it.** _(`cd1f161`, branch `agent-coordination-mvp`)_ —
|
||
the edit form rebuilt the layout from the template on every submit, so a rename or a typo fix in
|
||
the description silently discarded the panel arrangement; a rename additionally dropped
|
||
`ws-terminals-<id>`/`ws-host-terminals-<id>` instead of carrying them, abandoning every shell the
|
||
dashboard held. Found while inventorying §5.4's second item, which was itself dropped.
|
||
|
||
- [x] **The appType allow-list is a prop, not a convention.** _(`1de1d92`, branch
|
||
`agent-coordination-mvp`)_ — closes the first item of §5.4. Fourteen character-identical
|
||
normalisers, each with its `useMemo` and persist-back `useEffect`, collapse to
|
||
`appTypes={{ allowed, fallback }}`. 402 lines deleted for 215 added, most of the additions being
|
||
the seven tests that pin the normaliser — including idempotence, because a normaliser that does
|
||
not normalise to itself turns the persist-back into an infinite write loop.
|
||
|
||
- [x] **Panels can carry per-panel settings, and keep them through a move.** _(`e588524`, branch
|
||
`agent-coordination-mvp`)_ — `LayoutPanel.config`, opaque to the framework, exposed as
|
||
`usePanelConfig(panelId)`. The substantive half is in `layout-utils.ts`: `swapPanels` and
|
||
`movePanel` now carry `{appType, config}` as one unit via a `PanelContents` type, where before
|
||
they carried only `appType` and therefore reset _any_ per-panel state on a drag. This is the one
|
||
framework change the agent-coordination MVP needed; it is what lets a panel remember which named
|
||
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 — the three direct `client.patch('/dashboards', …)` calls in
|
||
`apps/Dashboards/` — followed in `81ad3ef`. See §1.
|
||
|
||
- [x] **A bad stored value is no longer a white screen.** _(`64961d4` + `ef036df`, branch
|
||
`agent-coordination-mvp`)_ — Tier A3 and A4, and with them the first three items of §4. Three
|
||
layers, none of which was enforcing anything before: the columns stopped defaulting to a value that
|
||
could not be a layout, the read stopped handing over one that is not, and the render stopped taking
|
||
the whole app down when something else does. Per-item detail in §4.
|
||
What this did **not** do is validate on write; that followed in `92b8905`, closing §4.
|
||
|
||
- [x] **A layout write can no longer be quietly undone by an older one.** _(`b0a32ae` + `81ad3ef`, branch
|
||
`agent-coordination-mvp`)_ — Tier A5 and A7, and with them the whole of §5.5, which empties Tier A.
|
||
One defect in three places, all of it the same shape: a write computed from a snapshot that a later
|
||
write had already superseded. `WorkspaceView`'s eight mutations pass **updaters** rather than trees;
|
||
`WorkspaceRenderer`'s resize debounce gained the `useEffect` cleanup it never had, flushing on
|
||
unmount rather than firing against a captured layout; and the query no longer sits on
|
||
`staleTime: Infinity` with nothing to converge it — it refetches on focus, and deliberately _not_ on
|
||
mount or reconnect, because an optimistic cache makes a badly-timed refetch the same lost update
|
||
again. The PATCH stopped assembling a full state blob nobody read.
|
||
Worth remembering for anything new here: the cost of getting this wrong is no longer a panel size
|
||
that snaps back. Panel identity lives in the layout jsonb, so a resurrected tree carries an older
|
||
`config` — the panel that was just given an agent's name goes anonymous, and the agent stops being
|
||
addressable through it.
|
||
|
||
- [x] **A layout column cannot hold a non-layout.** _(`92b8905`, branch `agent-coordination-mvp`)_ —
|
||
§4's last item, and the end of §4. A CHECK on each layout column (NULL or `jsonb_typeof = 'object'`)
|
||
plus the same rule in the PATCH dispatcher, so the caller gets a 400 naming the key rather than a
|
||
500 naming nothing. Detail in §4.
|
||
|
||
---
|
||
|
||
## 8. Dead code sweep
|
||
|
||
Low priority, but each line here is a line someone will read and believe.
|
||
|
||
Swept 2026-08-07 (`b419af3`). Every claim was re-checked against the code first; three were stale, and
|
||
they are marked below, because a dead-code list that is itself wrong is the worst kind.
|
||
|
||
- [ ] `DragOverlay.tsx` (99) + `LayoutEditor.tsx` (51) — imported by nothing. Gated on 5.3.
|
||
**Deliberately left**: 5.3 is an owner decision ("decide, then act"), and deleting these makes it
|
||
for him.
|
||
- [x] `WorkspaceLayout`'s `isMobile`/`mobilePanelId`/`onMobileBack` props — no caller passes them.
|
||
Confirmed across all ten call sites; removed. `WorkspaceView` still passes the same props to
|
||
`WorkspaceRenderer`, where they are live — it was only the `WorkspaceLayout` pass-through that was
|
||
dead.
|
||
- [x] `fixedHeight` on `AppRegistryEntry` — declared, read at `WorkspaceRenderer.tsx:76`, set by **zero**
|
||
metas. Removed, along with `getFixedHeight` and the pixel-height branch it fed; the flex-column
|
||
layout it shared with `fitContent` stays, now keyed on `fitContent` alone.
|
||
- [x] `registerApp` (`useAppRegistry`) — zero callers. Already deleted in `9fcc9c2` (§5.6), with
|
||
`registerWidget`.
|
||
- [x] `getDefaults` (~~`queries/dashboards.ts:136`~~ `:195` — **line was stale**) — zero callers.
|
||
Removed. `getAllDashboardState` already folds the defaults row into the one state payload, which
|
||
is why the single-row getter never acquired one.
|
||
- [x] `screens.terminals` / `screens.hostTerminals` columns — never read (confirmed), **but "never
|
||
written" was stale**: `upsertScreen` accepted and inserted them, so all 15 rows hold the `{}` it
|
||
wrote. The dead parameters and inserts are gone. **The columns themselves are not dropped** — that
|
||
needs `bun db:push`, which diffs the *whole* schema, and this tree currently holds another agent's
|
||
uncommitted `schema/agent-panels.ts`. Drop them in a push of their own.
|
||
- [x] ~~`SELECTED_DASHBOARD`~~ **`SELECTED_DASHBOARD_KEY`** constant — zero consumers. The parenthetical
|
||
claiming `SELECTED_DASHBOARD_KEY` was the live one was **backwards**: `'SELECTED_DASHBOARD'` is the
|
||
string value, `SELECTED_DASHBOARD_KEY` the constant, and H2 (`01365cb`) took its last reader four
|
||
months ago. Deleted, with its two barrel re-exports. Its file-mates survive — they are dialog form
|
||
state, not selection.
|
||
- [ ] `preview:refresh` and `chat:active-session` channels — written by `ChatPanelWrapper.tsx:58-59`,
|
||
**read by nobody**. **Deliberately left**: deleting the publishers changes the chat panel, which is
|
||
another agent's. Declared with the fact stated in `officerdev/src/channels.ts` and written up in
|
||
`COMMS/chat-agent-handoff-2026-08-07.md` §6. The doc half **is** done —
|
||
`docs/navigation-audit.md:126` and `CLAUDE.md` no longer offer `preview:refresh` as the exemplar
|
||
channel; `files:refresh-signal` is.
|
||
- [x] `components/ui/hooks/use-mobile.tsx` + `ui/sidebar.tsx` (~720 lines) — a second `useIsMobile`
|
||
implementation, and a sidebar imported by nothing. Both deleted. **Nuance the entry missed**:
|
||
`use-mobile.tsx` was not orphaned, it was imported by `sidebar.tsx` — dead only once the sidebar
|
||
went. The whole `ui/hooks/` directory turned out to be a stray shadcn copy, its `use-toast.ts` a
|
||
near-identical duplicate of the live `ui/use-toast.ts`, referenced by nothing; it went too.
|
||
_Noticed, not fixed:_ `ui/toaster.tsx` + `ui/use-toast.ts` are their own dead chain (the app uses
|
||
`sonner`), and `globals.css` carries eight `--sidebar-*` tokens now used by nothing.
|
||
- [x] `WorkspaceRenderer.tsx:84` — unreachable duplicate condition inside `findChildById`. While there:
|
||
the doc comment says "find a panel node by id anywhere in the tree", but it returns _the direct
|
||
child whose subtree contains the id_, which is what the mobile collapse needs. **The behaviour is
|
||
right and the description is wrong** — fix the comment, not the code. Done exactly that. (Worth
|
||
recording: a reviewer sent at this function without the entry in hand reported the `return child`
|
||
as the bug and the dead line as incidental. The entry is right and it is the reason the code
|
||
survived review.)
|
||
|
||
---
|
||
|
||
## 9. Tests
|
||
|
||
- [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.
|
||
**Promoted to the critical path 2026-08-07** (`agent-coordination.md` §6.2, A6). `e588524` put
|
||
_agent identity_ inside those functions — `setContents`, `swapPanels`, `movePanel`,
|
||
`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).
|
||
**The "still untested" list this line used to carry was stale by the time it was written, and two of
|
||
its four entries were wrong.** `normalizeLayout` is not a separate file — it lives in `layout-utils.ts`
|
||
and has nine tests of its own (identity-by-reference on a clean tree, `config` dropped along with the
|
||
app it belonged to, idempotence, nested repair, `null` always allowed). `WorkspaceRenderer` got eight
|
||
tests in `56ca411`. The Workspace directory is 76 tests across three files and green.
|
||
**Genuinely still untested: `WorkspaceView` and `PanelSlot`.** But note §5.5's lost updates are no
|
||
longer what makes that urgent — every mutation in `WorkspaceView` now goes through `onLayoutChange`
|
||
as an *updater*, never as a computed tree, which is the structural fix; a test there would be
|
||
guarding the fix rather than finding the bug. Checked, not assumed — `bun test src/workspaces/officerdev/src/components/Workspace/`.
|
||
|
||
- [x] **`useDashboardState`, and the strongest argument this section has for itself.** _(`4f8046d`,
|
||
branch `agent-coordination-mvp`)_ — 14 tests over the store every layout and every
|
||
`config.agentName` is persisted through. They found a live Tier-A-class defect on the first run,
|
||
in code written three days earlier to *stop* silent write loss: `revert` decided whether to roll
|
||
back by asking "does the cache still hold exactly what I wrote?" **by reference**, and
|
||
`setQueryData` runs React Query's structural sharing, which rebuilds the object it stores rather
|
||
than keeping the one it was handed. Measured against @tanstack/react-query 5.101.4 — an object
|
||
value comes back `!==`, a string comes back `===`. So the guard was false for every *container*
|
||
the store exists to hold, and a refused write kept its optimistic value in the cache while the
|
||
toast said it had been rolled back; the change then vanished at the next reload. Only primitives
|
||
ever reverted, which is exactly why nobody saw it. Replaced with a per-key write sequence, which
|
||
asks the question the identity check meant to ask and does not depend on identity at all. The
|
||
regression pin is the test named "a refused OBJECT write is rolled back too".
|
||
Two things worth carrying forward. **A test found a bug that review had not**, in a file that had
|
||
been read carefully twice — which is the case for §9 stated better than any argument. And
|
||
`mock.module` is **process-wide and permanent** in Bun: a stub that does not spread the real
|
||
module deletes exports out from under files that never heard of it. Likewise
|
||
`@testing-library/react` auto-registers `afterEach(cleanup)` at *import* time, so it lands in
|
||
whichever test file imports the library first and every later file silently gets none — that is
|
||
now registered in `test-setup.ts`, where preload's lack of a file scope makes it global. Adding
|
||
one test file broke fourteen assertions in `DataTable.test.tsx` before both were understood.
|
||
|
||
- [x] **`WorkspaceView`, and the second consecutive bug a test found that review had not.** _(`bfa9967`,
|
||
branch `agent-coordination-mvp`)_ — 11 tests over the last untested mutator, driving the real
|
||
`WorkspaceView` through the real `WorkspaceRenderer` and `PanelSlot`, so the buttons under test
|
||
are the buttons. Two properties: every layout write is an *updater* rather than a computed tree
|
||
(two of the paths are deferred — the 500 ms resize debounce, and a window resize firing `onLayout`
|
||
on every group at once — so a computed tree silently undoes the write before it and resurrects an
|
||
older `config`); and `usePanelClose` fires on close *intent* only, never on the unmounts a drag,
|
||
a swap or a mobile switch cause.
|
||
Four of the eleven failed on the first run, all on one defect. `TrafficLights` took `onRemove`
|
||
**and** `isLastPanel` and used `isLastPanel` only to pick the tooltip: the red button read "Close
|
||
panel" and called `onClearApp` regardless, so closing a panel from its own chrome was impossible —
|
||
the panel stayed, emptied, and the context menu was the only path that worked. Introduced by
|
||
`25f5f74`. Cosmetic before identity lived in the layout; not now, because clearing the app drops
|
||
the `config` that named the panel's agent while the panel survives to be renamed by whatever is
|
||
put in it next. The pin is "a panel that is NOT the last is removed".
|
||
One hardening came out of the same pass, and it stands on its own merits: the ephemeral pane's
|
||
sizing effect now try/catches. `react-resizable-panels` **asserts** rather than no-ops when asked
|
||
to size a group it has not laid out yet, and an assert thrown from an effect aborts the commit —
|
||
trading a file preview's geometry for the whole dashboard. Also worth recording as method: the
|
||
assert fires in happy-dom (no layout, so the group's size array never populates) and **not** in a
|
||
real browser — measured, six panels on `/dashboards`, empty console — and a `ResizeObserver` +
|
||
`getBoundingClientRect` stub written to "fix the environment" turned out to fix nothing. The
|
||
try/catch was the whole difference, and the stub was deleted rather than left behind as a comment
|
||
claiming credit for it.
|
||
|
||
- [x] **`PanelSlot`, and with it this section.** _(`2c00c6a`, branch `agent-coordination-mvp`)_ — 25 tests
|
||
over the panel chrome, driving `PanelSlot` directly so the workspace can be put into states a whole
|
||
view cannot easily be pushed into: maximized, mid-swap, mobile. The substance is the mode matrix —
|
||
`interactive`, `locked`, `isMobile`, `isLastPanel`, `maximizedPanelId` and an app's own `zoomable`
|
||
/`transparent` flags combine in six separate ternaries spread over the file, and a control present
|
||
in a mode that should not have it is a way to edit a locked screen. Also pinned: the empty panel's
|
||
picker honours `availableOnPanel: false`; a screen-supplied `components` entry beats the registry
|
||
and its `header`/`provider`/`onClose` are all honoured; a throwing app is contained and offered a
|
||
clear only when the panel is the user's to edit; and the scale is CSS `zoom`, absent entirely at
|
||
100%, never `transform: scale`.
|
||
Nothing new found, and that is the honest result to record after two consecutive files where a test
|
||
found what review had not: the close button was the chrome's one defect and it was pinned by the
|
||
commit before this. **The Workspace directory is 112 tests across five files, green, and this
|
||
section is closed.**
|
||
|
||
- [x] **A panel is told where it is, instead of being handed a key to reverse-engineer.** _(`dbe585f` +
|
||
`717580f` + `585f234`, branch `agent-coordination-mvp`)_ — the first three items of §5.9. The
|
||
context field `dashboardId: string` — which was never an id, always the whole `workspace.key` —
|
||
is now `workspace: WorkspaceIdentity | null`, parsed once by `parseWorkspaceKey` and memoised on
|
||
the key. Three apps had been parsing the format independently, each with its own idea of what a
|
||
non-dashboard looks like; they now read `kind` and `id`. The raw `key` rides along because it is
|
||
the address `agent_panels.dashboard_id` stores, and a stored address must not depend on which
|
||
feature wrote it.
|
||
In the same pass, the two file-browser fields the context had grown were deleted, and with them
|
||
the prop chain underneath that had no other caller. Net: the context is 16 fields from 18, and
|
||
four dead parameters and a permanently-false flag are gone from the file browser.
|
||
|
||
- [x] **The framework stopped carrying things only one app understands.** _(`d3922bd` + `ca046a3`,
|
||
branch `agent-coordination-mvp`)_ — the last two items of §5.9, and with them the section. A
|
||
screen's ~40-word system prompt used to travel screen → `WorkspaceView` → context →
|
||
`ChatPanelWrapper`, three of those four steps by a framework that has no idea what the string is.
|
||
It is now a prop on the chat panel the screen mounts itself through `components` — the prop that
|
||
already existed for supplying a panel's component, and which `PanelSlot` honours for the body
|
||
while still taking header and provider from the registry entry, so nothing about the panel's
|
||
chrome had to be reproduced.
|
||
The second half is the inert context: `WorkspaceLayout` and the `createContext` default had each
|
||
written out the same eleven switched-off fields by hand. `inertInteraction` names the idea once.
|
||
Net across all of §5.9: the context is 15 fields from 18, and everything still on it answers
|
||
"where is this panel" rather than "what should this panel do".
|
||
|
||
- [x] **A panel can be told it was closed, and only when it was.** _(`04371a9` + `198dc71` + `c92b51c`,
|
||
branch `agent-coordination-mvp`)_ — the first two items of §5.1, and the terminal orphan leak with
|
||
them. `usePanelClose(panelId, handler)`, fired by `WorkspaceView` from `handleRemove` and from
|
||
`handleSetApp` when the app actually changes, and from nowhere else.
|
||
The interesting part is what it is *not*. This item used to propose diffing the layout before and
|
||
after; two tests now stand in `layout-utils.test.ts` to stop anyone trying it, because `movePanel`
|
||
mints a fresh panel id on the way and `swapPanels` exchanges contents between stationary ones — so
|
||
a drag reads as a close and a swap reads as two. A panel id is a position in the tree, not an app
|
||
instance. Nor is it an unmount hook: registration is deliberately never torn down, since "this
|
||
component went away" is the ambiguous signal the whole thing exists to stop trusting.
|
||
The terminals use it through a shared `useTerminalSession`, which turned up two more defects while
|
||
being written: `HostTerminalWrapper` had kept the unmount cleanup the other two wrappers had
|
||
removed, orphaning a host shell on every layout change; and `RunningShells`, the panel whose entire
|
||
job is to make orphans visible, was asking the pty sidecar for a route it does not serve, so it had
|
||
always shown an empty list and its kill button had never done anything.
|
||
A chat panel deliberately does **not** use the hook — it is a pointer to a server-side session, and
|
||
closing a window must not delete what it points at. That is recorded in the hook's own doc comment,
|
||
where the next person to add one will read it.
|