refactor(#587): a side-panel registry so adding a navigation panel is a one-file change (phase 2)

CHANGELOG.md

@@ -58,6 +58,47 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
   detached browser tab (`results.ts`'s Data Pane) — keeps a self-contained
   modal overlay (renamed `.cell-detail-overlay`), still built on
   `SurfaceLifecycle`.
+- **A side-panel registry replaces hard-composed sidebar switching** (#587,
+  phase 2 of the #593 refactor umbrella). `core/side-panels.ts` is a single
+  `as const satisfies` manifest (`SIDE_PANELS`: id, pane, persisted key) every
+  id/pane/key union elsewhere derives from via `typeof`, plus the
+  `asb:sidePanel` load-boundary decoder; `ui/side-panel-registry.ts` is the
+  generic, DOM-owning half — persistent per-panel hosts built once and never
+  rebuilt, a mount-once/activate-per-transition lifecycle (`MountedSidePanel`:
+  `render`/`activate?`/`deactivate?`/`onRunComplete?`/`dispose`), pane-scoped
+  `showPanel` (the wide sidebar shows one Databases-or-Dashboards panel AND
+  one Library-or-History panel simultaneously — never a global "exactly one of
+  four"), and one tab-row renderer shared by both panes. `app-shell.ts`'s
+  sidebar composition, `sidebar-upper.ts` (which now only builds the two upper
+  bodies — schema search+list, Dashboard search+tree — registering them
+  through `databasesPanelDef`/`dashboardsPanelDef` rather than owning their
+  tab-row vocabulary), and `saved-history.ts` (which stops building the lower
+  tab row at all; `libraryPanelDef`/`historyPanelDef` each own one persistent
+  search+list host) all address panels only through this registry now.
+  `state.sidePanel: Signal<SidePanelKey>` decodes the raw stored value
+  fail-closed at load (`decodeSidePanelKey`) — **this is the `'library'` ↔
+  `'saved'` persisted-key bridge**: on `main` before this phase there was no
+  bridge at all (a raw, unvalidated `localStorage` read), so an unrecognized
+  stored value silently painted the History body with neither tab visually
+  active; it now resolves to the documented default (Library), and the
+  registry's own id `'library'` is never itself a persisted value (a
+  downgrade-safe invariant — #591 must not re-implement this bridge).
+  `app-preferences.ts`'s `save` is now generic over a `PreferenceValues` map,
+  so `prefs.save('sidePanel', 'library')` is a compile error, not a runtime
+  discipline. `workbench-session.ts` drops `sidePanel` from
+  `WorkbenchStateSlice` entirely and renames `WorkbenchHooks.renderSavedHistory`
+  to `onRunComplete` (fired unconditionally on a clean run now — dispatch to
+  whichever panel, if any, is scoped entirely to the hook's own wiring in
+  `app.ts`, via `app.shell.sidePanels.notifyRunComplete()`). The three
+  `AppDom` fields the per-panel pattern used (`savedList`/`savedSearch`/
+  `savedTabsRow`) are gone; adding a panel now touches only the registry's two
+  files plus the panel's own module. Two criteria are adapted from the
+  issue's literal wording (recorded as deliberate, not missed): mount-once-
+  per-shell lifecycle wins over the issue's Tests-section "per activation"
+  phrasing, which contradicts the persistent-host decision the same issue's
+  Deliverable/AC6 makes binding; and the registry is two files
+  (`core/side-panels.ts` + `ui/side-panel-registry.ts`), not the "(one file)"
+  AC5 names, forced by this repo's core/no-DOM purity rule.
 
 ### Changed
 - **The project wiki moved in-repo, as tracked `.wiki/`.** The maintainer/agent

docs/ADR-0004-ui-shell.md

@@ -227,3 +227,39 @@ re-deriving them from scratch.
   large-list need, or measurably rising invalidation-bug rate despite
   signals) — this evaluation adds no new trigger, it confirms the existing
   ones haven't fired.
+
+## Addendum — #587 (phase 2 of the #593 shell-primitive investment)
+
+The forward investment named above now includes a delivered second primitive:
+`core/side-panels.ts` + `ui/side-panel-registry.ts` (#587), a generic
+persistent-host/mount-once registry over the sidebar's four panels
+(Databases/Dashboards/Library/History), replacing the hard-composed switching
+`app-shell.ts`/`sidebar-upper.ts`/`saved-history.ts` used to own
+independently. It reuses #487 phase 2's salvaged `nav-sections.ts` design
+decisions verbatim in spirit (icon-as-factory, a separate `accessibleLabel`,
+pane-scoped exposure, a load-boundary persisted-key bridge) — this ADR's own
+salvage guidance above is what pointed at that branch.
+
+Two items from #587's plan landed in an ADAPTED form — AC5 below, and the
+mount-once-per-shell lifecycle decision further down — both forced by
+constraints this ADR's own vanilla-shell stance already commits to, not by
+any new tradeoff. AC4 itself landed literally, not adapted:
+
+- **AC4** ("the persisted-value union is derived from the registry, not
+  hand-maintained") landed literally — `app-preferences.ts`'s
+  `save` became generic over a `PreferenceValues` map keyed by
+  `core/side-panels.ts`'s derived `SidePanelKey`, so a mismatched
+  `prefs.save('sidePanel', 'library')` is a compile error.
+- **AC5** ("one file" to add a panel) is delivered as *two* files
+  (`core/side-panels.ts` + `ui/side-panel-registry.ts`), not one — this repo's
+  pure-core/no-DOM rule (CLAUDE.md hard rule 2) puts the vocabulary in `core/`
+  and the DOM-owning mount/lifecycle machinery in `ui/`; inverting that split
+  to satisfy the letter of "one file" would violate the same layering
+  discipline this ADR's own vanilla-imperative-adapter stance depends on.
+
+Also adapted: the issue's Tests-section wording ("mount/teardown runs exactly
+once per activation") directly contradicts its own Deliverable/AC6
+("persistent hosts, built once, never rebuilt") — the persistent-host decision
+wins, since it is the one #487 phase 2 already proved and #587 explicitly
+retains. `MountedSidePanel` is therefore mount-once-per-shell,
+activate/deactivate/render-per-transition, dispose-once-at-teardown.

src/application/app-preferences.ts

@@ -20,19 +20,40 @@
 
 import type { SaveStr } from '../state.js';
 import { KEYS } from '../state.js';
+import type { SidePanelKey } from '../core/side-panels.js';
 
-/** The true-preference subset of state.ts's own `KEYS` map — every OTHER key
- *  there (saved/history/libraryName/varValues/filterActive/
- *  varRecent/varRecentDisabled) is a domain record with its own dedicated
- *  `save*` method on `App` (`saveJSON`/`saveVarValues`/`saveFilterActive`/…),
- *  untouched by this service. */
-export type PreferenceKey =
-  | 'theme' | 'sidebarPx' | 'editorPct' | 'sideSplitPct'
-  | 'sidePanel' | 'resultRowLimit'
+/**
+ * The true-preference subset of state.ts's own `KEYS` map, keyed by the VALUE
+ * each preference accepts — every OTHER key there (saved/history/libraryName/
+ * varValues/filterActive/varRecent/varRecentDisabled) is a domain record with
+ * its own dedicated `save*` method on `App`
+ * (`saveJSON`/`saveVarValues`/`saveFilterActive`/…), untouched by this
+ * service.
+ *
+ * #587 AC4: `sidePanel`'s value is `SidePanelKey` (from `core/side-panels.ts`,
+ * the registry's own derived persisted-key vocabulary), not `unknown` — so
+ * `prefs.save('sidePanel', 'library')` (the registry's OWN id, never a
+ * persisted value — see `decodeSidePanelKey`'s downgrade-safety comment) is a
+ * COMPILE error, not just a runtime discipline every call site has to
+ * maintain by hand.
+ */
+export interface PreferenceValues {
+  theme: string;
+  sidebarPx: number;
+  editorPct: number;
+  sideSplitPct: number;
+  sidePanel: SidePanelKey;
+  resultRowLimit: number;
   // #586 — the single canonical docked right-inspector width, replacing the
   // former cellDrawerPx/docPanePx pair (see splitters.ts's 'rightInspector'
   // axis and state.ts's compat-read `rightInspectorPx` comment).
-  | 'rightInspectorPx';
+  rightInspectorPx: number;
+}
+
+/** Kept as a type alias so existing `PreferenceKey`-typed imports/casts
+ *  (`app-shell.ts`'s dynamic splitter/drawer call sites) keep compiling
+ *  unchanged. */
+export type PreferenceKey = keyof PreferenceValues;
 
 /** The one state field this service reads/writes (`toggleTheme` only) — a
  *  plain settable property, not a signal (matches `AppState.theme`). */
@@ -52,8 +73,11 @@ export interface AppPreferences {
    *  directly now). This IS the service's write API: per-key typed setters
    *  were considered and dropped (review) — every real call site already
    *  holds a validated `{name, value}` pair, so a per-key surface would ship
-   *  with zero callers (CLAUDE.md rule 5: no speculative primitives). */
-  save(name: PreferenceKey, value: unknown): void;
+   *  with zero callers (CLAUDE.md rule 5: no speculative primitives).
+   *  Generic over `PreferenceValues` (#587 AC4): `value`'s type follows
+   *  `name`, so a mismatched pair (e.g. `save('sidePanel', 'library')`) is a
+   *  compile error rather than a runtime-only discipline. */
+  save<K extends PreferenceKey>(name: K, value: PreferenceValues[K]): void;
   /** Flips `state.theme` light↔dark AND persists it in one call (issue
    *  ruling — the one preference whose state mutation moves here, not just
    *  its persist half); returns the new value so the DOM-half caller
@@ -67,7 +91,7 @@ export interface AppPreferences {
 export function createAppPreferences(deps: AppPreferencesDeps): AppPreferences {
   const { state } = deps;
 
-  function save(name: PreferenceKey, value: unknown): void {
+  function save<K extends PreferenceKey>(name: K, value: PreferenceValues[K]): void {
     deps.saveStr(KEYS[name], String(value));
   }
 

src/core/side-panels.ts

@@ -0,0 +1,143 @@
+// #587 — the side-panel manifest. Pure, no DOM, no globals: the ONE table both
+// panes' registries (`ui/side-panel-registry.ts`) and the persisted-key load
+// boundary (`state.ts`) read ids/panes/persisted keys FROM, rather than each
+// hand-listing its own copy (the duplication #587 exists to remove).
+//
+// Two independent panes sit in the wide sidebar SIMULTANEOUSLY (a splitter
+// between them, not a tab switcher over one): 'upper' (Databases | Dashboards,
+// #426) and 'lower' (Library | History). Exactly one panel is active PER PANE
+// — never "exactly one of four" globally, which would blank half the sidebar.
+//
+// Only the 'lower' pane persists its active panel (`asb:sidePanel`,
+// unchanged key — #459). 'upper' is deliberately session-only (state.ts
+// documents why: a persisted role would break "default to Databases on a
+// fresh session"). So only 'lower' entries carry a `persistedKey`.
+
+/** Which pane a panel lives in — a splitter-separated region of the wide
+ *  sidebar, NOT `AppState.mobileTab`'s narrow-viewport axis (a separate,
+ *  session-only choice that selects between these same two panes; see
+ *  `ui/side-panel-registry.ts`'s own small `MOBILE_PANES` table). */
+export type SidePanelPane = 'upper' | 'lower';
+
+interface SidePanelModel {
+  readonly id: string;
+  readonly pane: SidePanelPane;
+  /** The value written to `localStorage` under `KEYS.sidePanel` (`asb:sidePanel`)
+   *  for this panel — present ONLY for 'lower' entries. `'library'` persists as
+   *  `'saved'`: #427 renamed the visible label, not the stored string, since
+   *  migrating it would discard every user's persisted lower-pane choice for no
+   *  behavioural gain. */
+  readonly persistedKey?: string;
+}
+
+/**
+ * THE manifest — the one place `id`, `pane`, and the persisted-key mapping are
+ * declared. Every id/pane/key type below is DERIVED from this array via
+ * `typeof`, not hand-written beside it (#587 AC1/AC4: one authority, not two
+ * that can drift).
+ */
+export const SIDE_PANELS = [
+  { id: 'databases', pane: 'upper' },
+  { id: 'dashboards', pane: 'upper' },
+  { id: 'library', pane: 'lower', persistedKey: 'saved' },
+  { id: 'history', pane: 'lower', persistedKey: 'history' },
+] as const satisfies readonly SidePanelModel[];
+
+// A `SidePanelModel[]`-typed VIEW of the same array, used by every lookup
+// below — `SIDE_PANELS` itself keeps its precise `as const` literal type so
+// `typeof SIDE_PANELS` can derive the id/key unions; indexing into the union
+// of literal element types directly (e.g. `SIDE_PANELS.find(...).persistedKey`)
+// would not type-check, since not every element has that property.
+const PANELS: readonly SidePanelModel[] = SIDE_PANELS;
+
+export type SidePanelId = (typeof SIDE_PANELS)[number]['id'];
+// `UpperPanelId`/`LowerPanelId` used to be hand-written literal unions
+// (`Extract<SidePanelId, 'databases' | 'dashboards'>` etc.) — a SECOND
+// authority listing the same ids by hand, so adding a manifest row above
+// silently failed to extend either (PR #600 review, #587 finding 2: no test
+// caught it, because the "extended manifest" tests only exercise runtime
+// helpers over copied arrays, never these two TYPES). Both are now derived
+// from the manifest's own `pane` column: `PanelSpec` is the precise
+// element-union type `SIDE_PANELS` carries, and `PanelIdInPane<P>` extracts
+// the `id` of every element whose `pane` is `P` — so a new row's pane
+// assignment is the only thing that decides which union it joins, with no
+// second list to fall out of sync.
+//
+// `tests/types/side-panels.test-d.ts` pins coverage and disjointness of the
+// two derived unions AGAINST TODAY'S MANIFEST — not against a silent revert
+// to hand-written literals in isolation (PR #600 review, #587 finding 3): for
+// the current four-row manifest, hand-written `Extract<SidePanelId,
+// 'databases' | 'dashboards'>` literals and this derivation produce
+// IDENTICAL types, so that type-level test alone stays green either way. It
+// only goes red once a manifest row is added without extending whichever
+// union it should have joined — proving detection-after-expansion, not
+// detection-of-removal. Catching a plain revert with no accompanying
+// manifest change is `side-panel-source-contract.test.ts`'s job instead — its
+// "no literal panel-id allowlist in a type alias" check is a source-level,
+// best-effort regex over this file, not a type-level proof.
+type PanelSpec = (typeof SIDE_PANELS)[number];
+type PanelIdInPane<P extends SidePanelPane> = Extract<PanelSpec, { pane: P }>['id'];
+export type UpperPanelId = PanelIdInPane<'upper'>;
+export type LowerPanelId = PanelIdInPane<'lower'>;
+/** The `asb:sidePanel` persisted-value vocabulary — DERIVED from the manifest's
+ *  `persistedKey` column, not a second hand-written `'saved' | 'history'`
+ *  union declared beside it. `Extract` (rather than indexing the whole
+ *  element union directly) narrows to only the rows that HAVE a
+ *  `persistedKey` first — the upper two rows' literal types don't carry that
+ *  property at all, so indexing the unfiltered union would not type-check. */
+export type SidePanelKey = Extract<(typeof SIDE_PANELS)[number], { persistedKey: string }>['persistedKey'];
+
+/** The lower pane's panel ids, in manifest order — DERIVED by filtering
+ *  `specs` (default: the live manifest) rather than hand-listed a second time.
+ *  Exported as a function (not only a precomputed constant) so a test can
+ *  prove the derivation by feeding it a manifest with an extra panel and
+ *  observing the output grow (#587 AC4's falsifiability requirement) without
+ *  mutating the real, frozen `SIDE_PANELS`. */
+export function lowerPanelIdsOf(specs: readonly SidePanelModel[] = PANELS): string[] {
+  return specs.filter((spec) => spec.pane === 'lower').map((spec) => spec.id);
+}
+
+/** The `asb:sidePanel` persisted-value vocabulary, DERIVED from `specs` (same
+ *  derivation contract as `lowerPanelIdsOf`). */
+export function sidePanelKeysOf(specs: readonly SidePanelModel[] = PANELS): string[] {
+  return specs.filter((spec) => spec.persistedKey !== undefined).map((spec) => spec.persistedKey as string);
+}
+
+export const LOWER_PANEL_IDS: readonly LowerPanelId[] = lowerPanelIdsOf() as readonly LowerPanelId[];
+export const SIDE_PANEL_KEYS: readonly SidePanelKey[] = sidePanelKeysOf() as readonly SidePanelKey[];
+export const UPPER_PANEL_IDS: readonly UpperPanelId[] =
+  PANELS.filter((spec) => spec.pane === 'upper').map((spec) => spec.id) as readonly UpperPanelId[];
+
+/** Lower panel id -> its persisted value. The reverse of `decodeSidePanelKey`. */
+export function sidePanelKeyFor(id: LowerPanelId): SidePanelKey {
+  // `!`: every member of `LOWER_PANEL_IDS` (the only values `LowerPanelId`
+  // admits) has a manifest row with a `persistedKey`, by construction of the
+  // manifest above.
+  return PANELS.find((spec) => spec.id === id)!.persistedKey as SidePanelKey;
+}
+
+/**
+ * Fail-closed decode of the persisted `asb:sidePanel` raw value, applied ONCE
+ * at the state-load boundary (`state.ts`): anything other than a recognized
+ * `persistedKey` — missing, corrupt, or an obsolete/future value — resolves to
+ * `'saved'` (Library), the documented default, rather than propagating an
+ * unrecognized string for every consumer to compare against independently.
+ *
+ * Returns a `SidePanelKey`, not a `LowerPanelId` — `state.sidePanel` holds the
+ * PERSISTED vocabulary directly (so a write is `prefs.save('sidePanel', v)`
+ * with no re-encoding step), matching today's shape. Downgrade-safety (#587
+ * R2.9): the registry id `'library'` is never assigned to `state.sidePanel`
+ * or written to storage — only `'saved'`/`'history'` ever are, so a reverted
+ * build reads back a value it already understood.
+ */
+export function decodeSidePanelKey(raw: unknown): SidePanelKey {
+  const spec = PANELS.find((s) => s.pane === 'lower' && s.persistedKey === raw);
+  return spec ? (spec.persistedKey as SidePanelKey) : 'saved';
+}
+
+/** Persisted value -> lower panel id (the registry's own vocabulary). */
+export function lowerIdForKey(key: SidePanelKey): LowerPanelId {
+  // `!`: every `SidePanelKey` value originates from a manifest `persistedKey`
+  // (see the type derivation above), so the reverse lookup always finds a row.
+  return PANELS.find((spec) => spec.persistedKey === key)!.id as LowerPanelId;
+}