pastilhasandClaude Opus 5
ecc7fb90d8
The re-rank the north star deferred, done now that the MVP is built and
running — so it is ranked against what the mechanism turned out to need.
The finding is that most of the list is not on this path. The mechanism is
server-side and a panel is a pointer to it, so a remount, a re-render or a
drag costs a replay, not a session. Section 5.2 and 5.3 are large downgrades;
5.3 was on the critical path when the north star was written and is disarmed
by resolving identity by name.
What is left is small and mostly one defect wearing four hats: a write that
silently does not land. Panel identity lives in the layout jsonb now, so the
swallowed persist catch, the dispatcher's missing else and the two debounce
lost-updates each become a panel that forgets which agent it is — invisibly,
for exactly as long as nobody is looking.
Also corrects two items the MVP made stale, and promotes layout-utils tests:
e588524 put agent identity inside those mutators and shipped them untested.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
521 lines
37 KiB
Markdown
521 lines
37 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 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.
|
||
- **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.
|
||
|
||
- [ ] **Fix the Running Shells paths — the panel has 404'd since 2026-07-31.**
|
||
`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.
|
||
|
||
- [ ] **Surface the `clients` count in Running Shells.** `RunningShells.tsx:12-20` drops the `clients`
|
||
field the sidecar returns (`sidecars/pty/sessions.mjs:169`) — the one field that distinguishes an
|
||
orphan (`clients: 0`) from a live shell. Add it to the type and render it.
|
||
|
||
- [ ] **Stop swallowing persist failures.** `state/src/useDashboardState.ts:46` is
|
||
`.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.
|
||
|
||
---
|
||
|
||
## 2. Live data loss
|
||
|
||
- [ ] **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.
|
||
|
||
- [ ] **`ws-terminals-{id}: null` on a live dashboard is a 500.** Same file, `:61-66` — the
|
||
`ws-layout-*` branch has a `value === null` → `deleteDashboard` case (`:42`); the terminals
|
||
branches do not. A null falls to the UPDATE branch and sets a `NOT NULL` column
|
||
(`databases/officer_db/src/queries/dashboards.ts:70`) → 23502.
|
||
|
||
- [ ] **`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.
|
||
|
||
- [ ] **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.
|
||
|
||
---
|
||
|
||
## 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.
|
||
|
||
- [ ] **`dashboards.id` is a global primary key but ids are `slugify(name)`.**
|
||
`databases/officer_db/src/schema/dashboards.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.
|
||
|
||
- [ ] **`upsertDashboard`'s UPDATE has no `userId` predicate.**
|
||
`databases/officer_db/src/queries/dashboards.ts:73` —
|
||
`db.update(dashboards).set(set).where(eq(dashboards.id, id))`. The `existing` lookup above it *is*
|
||
scoped, so it cannot reach another user's row today, but it is a non-transactional read-then-write.
|
||
**It becomes a live cross-user overwrite the moment the PK above is made composite.**
|
||
Do both in one change or the first fix opens the second.
|
||
|
||
---
|
||
|
||
## 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.*
|
||
|
||
- [ ] **Add an error boundary.** `grep -rln "componentDidCatch\|getDerivedStateFromError\|ErrorBoundary\|errorElement" src` returns **nothing**
|
||
across the whole repo. Wrap `WorkspaceRenderer` at minimum, with a reset that offers "restore
|
||
default layout". Today the only recovery from a malformed stored layout is SQL.
|
||
|
||
- [ ] **Validate on read.** `useDashboardState.ts:36` is `key in state ? state[key] as T : defaultValue`
|
||
— an unchecked cast over a value that arrived as `unknown` from jsonb. Add an `isLayoutNode()`
|
||
guard that falls back to `defaultValue`.
|
||
This also neutralises the next item without a migration.
|
||
|
||
- [ ] **`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, `dashboards.ts:33`) 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. Change the default, or drop it and make the column nullable.
|
||
|
||
- [ ] **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 any of the three layout columns. 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.
|
||
|
||
- [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
|
||
|
||
- [ ] **`onClose` on `AppRegistryEntry`, invoked by the mutators, not by unmount.**
|
||
`components/Workspace/types.ts:38-47` has no close hook; the only `onClose` is
|
||
`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`)
|
||
— are pure tree rewrites that notify nobody.
|
||
**Invoke it from `WorkspaceView`'s `handleRemove`/`handleSetApp`, never from a `PanelSlot`
|
||
unmount** — a `PanelSlot` unmount is precisely the ambiguous signal this exists to replace.
|
||
Implementation: diff removed panel ids old-tree ∖ new-tree, or have the mutators return them.
|
||
|
||
This is the thing `apps/Terminal/TerminalWrapper.tsx:31-38` asks 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.
|
||
|
||
- [ ] **Then: kill the pty on real close.** Once the hook exists, `TerminalWrapper` /
|
||
`CommandTerminalWrapper` / `HostTerminalWrapper` can `DELETE /terminal/_officer/sessions/:id` and
|
||
drop the map entry — the thing they each explain they cannot currently do.
|
||
|
||
- [ ] **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.*
|
||
|
||
A panel's React identity is its position plus `key={child.node.id}` on its **nearest ancestor group
|
||
slot** (`WorkspaceRenderer.tsx:178,204`) — the panel's own id is a key nowhere. That's the root cause of
|
||
the whole table:
|
||
|
||
| 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` |
|
||
|
||
- [ ] **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.
|
||
- [ ] **Don't re-key the survivor when a group collapses** (`layout-utils.ts:68-70, 83-88`). Kills row 4.
|
||
- [ ] **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
|
||
|
||
- [ ] **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.
|
||
- [ ] **Then collapse the three default-layout mechanisms**: per-screen `defaultLayout.ts` (×20),
|
||
`createDefaultLayout()` in the core, and the 6-entry template array at `DashboardPreview.tsx:33-142`.
|
||
|
||
### 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.*
|
||
|
||
- [ ] **The resize debounce can resurrect a deleted panel.** `WorkspaceRenderer.tsx:108-124` holds a
|
||
500 ms timer in a ref with **no `useEffect`, therefore no cleanup**, and its callback closes over
|
||
the layout as it was when the drag began. Drag a splitter, remove a panel within 500 ms → the timer
|
||
PATCHes the pre-deletion tree and the panel comes back. Same shape for a split.
|
||
Fix: clean up on unmount, and make `setValue` accept an updater so it composes against current
|
||
state instead of a captured one.
|
||
- [ ] **Concurrent same-key writes clobber.** A window resize fires `onLayout` on every group at once;
|
||
each schedules its own timer against the same base tree and the last wins
|
||
(`WorkspaceView.tsx:76-81`). Same fix as above.
|
||
- [ ] **The cache is never invalidated.** `staleTime: Infinity` and no `invalidateQueries` anywhere in
|
||
the repo. Meanwhile every PATCH already computes and returns a full fresh state blob
|
||
(`servers/api/dashboards/dashboards.ts:90`) which the client **discards** — 3 SELECTs per splitter
|
||
release, thrown away. Consume the response, or stop computing it. Two tabs currently diverge
|
||
permanently and neither is told.
|
||
- [ ] **Preserve sibling sizes on split.** `splitInner`/`insertPanel` redistribute evenly
|
||
(`100 / newChildren.length`), so one split discards carefully tuned proportions.
|
||
- [ ] **`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}`.
|
||
Use the core's `uid()` (`layout-utils.ts:4`), which stamps `p-${Date.now()}-${n}`. One line.
|
||
|
||
### 5.6 Registry
|
||
|
||
- [ ] **No duplicate-key guard.** `metasToRegistry` is `Object.fromEntries`
|
||
(`state/src/useAppRegistry.ts:19`) — a collision silently last-wins and one app just disappears.
|
||
All 44 keys are unique today. Throw in dev.
|
||
- [ ] **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.
|
||
- [ ] **`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.
|
||
- [ ] **`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.
|
||
- [ ] **`DictateDialog` never releases the microphone.**
|
||
`apps/FileBrowser/FileBrowserApp/components/DictateDialog.tsx:142-149` —
|
||
`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.
|
||
- [ ] **`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.
|
||
- [ ] **`HostTerminalWrapper` still has the cleanup the other two deliberately removed.**
|
||
`apps/Terminal/HostTerminalWrapper.tsx:28-35` deletes the panel→session mapping from persisted
|
||
state on *any* unmount — exactly what `TerminalWrapper.tsx:30-38` and
|
||
`CommandTerminalWrapper.tsx:31-32` document removing, and why. Latent only because
|
||
`officerdev/terminal-host` is in no default layout and hidden from the picker
|
||
(`apps/Terminal/index.tsx:51`).
|
||
- [ ] **`useTaskRunner` abandons the running task.** `:60-63` is a bare `ws.close()`. A `stop` message
|
||
exists at `:76-79` and is never sent, and there is no re-attach path — unlike `usePipelineRunner`,
|
||
which re-attaches by `jobId` (`:281-283`).
|
||
- [ ] **`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.
|
||
Also: the comment at `:215-216` says the dep list "must stay empty" while the code passes
|
||
`[sendReport]` — harmless today, misleading.
|
||
- [ ] **Two stale-closure sockets.** `usePipelineRunner.ts:270-301` (deps `[]` but calls a `useCallback`
|
||
that changes identity — keeps the first-render copy forever) and
|
||
`apps/FileBrowser/AudioStreamPlayer.tsx:70-143` (safe today, latent).
|
||
- [ ] **`PanelSlot` defines a component inside render.** `:341-348` — `DefaultHeader` is a new component
|
||
*type* every render, so the header subtree remounts constantly. Harmless while stateless; a trap
|
||
the moment it isn't.
|
||
- [ ] **The context value is a fresh literal.** `WorkspaceView.tsx:145-163` — every `useWorkspace()`
|
||
consumer in every panel re-renders on every `WorkspaceView` render, including each maximize
|
||
animation frame. Multiplies the cost of the `DesktopView` bug.
|
||
|
||
### 5.8 Navigation — finish the refactor
|
||
|
||
- [ ] **Move the seven channel-driven apps' selection into the URL.** Split today: 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`.
|
||
|
||
### 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.
|
||
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **`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.
|
||
- [ ] **`WorkspaceLayout.tsx:36` silently omits `root`, `initialFilePath`, `defaultFileSort`** — apps
|
||
inside a `DashboardPreview` fall through to the `createContext` defaults. Whatever survives the
|
||
three items above should be constructed in one place, not twice by hand.
|
||
|
||
### 5.10 Channel hygiene — *(found 2026-08-07)*
|
||
|
||
Cheap to fix, and prerequisites for the 5.8 migration rather than alternatives to it.
|
||
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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.)
|
||
- [ ] **`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.
|
||
- [ ] **`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.
|
||
|
||
---
|
||
|
||
## 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] **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).
|
||
|
||
---
|
||
|
||
## 8. Dead code sweep
|
||
|
||
Low priority, but each line here is a line someone will read and believe.
|
||
|
||
- [ ] `DragOverlay.tsx` (99) + `LayoutEditor.tsx` (51) — imported by nothing. Gated on 5.3.
|
||
- [ ] `WorkspaceLayout`'s `isMobile`/`mobilePanelId`/`onMobileBack` props — no caller passes them.
|
||
- [ ] `fixedHeight` on `AppRegistryEntry` — declared, read at `WorkspaceRenderer.tsx:76`, set by **zero**
|
||
metas.
|
||
- [ ] `registerApp` (`useAppRegistry`) — zero callers.
|
||
- [ ] `getDefaults` (`queries/dashboards.ts:136`) — zero callers.
|
||
- [ ] `screens.terminals` / `screens.hostTerminals` columns — never written, never read.
|
||
- [ ] `SELECTED_DASHBOARD` constant — zero consumers.
|
||
- [ ] `preview:refresh` and `chat:active-session` channels — written by `ChatPanelWrapper.tsx:58-59`,
|
||
**read by nobody**. Note `preview:refresh` is the exemplar `docs/navigation-audit.md:126` cites as
|
||
the canonical *good* channel; fix the doc too.
|
||
- [ ] `components/ui/hooks/use-mobile.tsx` + `ui/sidebar.tsx` (~720 lines) — a second `useIsMobile`
|
||
implementation, and a sidebar imported by nothing.
|
||
- [ ] `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.
|
||
|
||
---
|
||
|
||
## 9. Tests
|
||
|
||
- [ ] **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.
|