diff --git a/CHANGELOG.md b/CHANGELOG.md index e7d8985d..e2e18f30 100644 --- a/CHANGELOG.md +++ b/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` 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 diff --git a/docs/ADR-0004-ui-shell.md b/docs/ADR-0004-ui-shell.md index 7b29347f..06378b16 100644 --- a/docs/ADR-0004-ui-shell.md +++ b/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. diff --git a/src/application/app-preferences.ts b/src/application/app-preferences.ts index ead84191..d30e71e4 100644 --- a/src/application/app-preferences.ts +++ b/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(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(name: K, value: PreferenceValues[K]): void { deps.saveStr(KEYS[name], String(value)); } diff --git a/src/core/side-panels.ts b/src/core/side-panels.ts new file mode 100644 index 00000000..126564de --- /dev/null +++ b/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` 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

` 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` 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

= Extract['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; +} diff --git a/src/state.ts b/src/state.ts index 10ef65a9..b0960787 100644 --- a/src/state.ts +++ b/src/state.ts @@ -35,6 +35,8 @@ import type { LinkedTabSnapshot } from './workspace/workspace-sync.js'; import { materializeQueryTimeRange } from './core/query-time-range.js'; import type { QueryTimeRangeInferenceDiagnostic } from './core/query-time-range.js'; import { deriveWorkspaceKey } from './core/workspace-key.js'; +import { decodeSidePanelKey } from './core/side-panels.js'; +import type { SidePanelKey, UpperPanelId } from './core/side-panels.js'; // ── Persisted-data types (schema-generated) ───────────────────────────────── @@ -385,16 +387,22 @@ export interface AppState { filterActive: Record; varRecent: RecentMap; varRecentDisabled: boolean; - /** 'saved' | 'history' at every write site; typed string because the - * initial value is an undecoded localStorage read (`asb:sidePanel`). */ - sidePanel: Signal; + /** The lower sidebar pane's active panel, in the PERSISTED vocabulary + * (`core/side-panels.ts`'s `SidePanelKey` — `'saved'` means the Library + * panel, `'history'` means History; #427 renamed the visible label, not + * the stored string). `createState` decodes the raw localStorage read + * through `decodeSidePanelKey` (fail-closed) before this signal ever sees + * it, so it only ever holds one of the two recognized values — never the + * registry's own id `'library'` (#587 R2.9 downgrade-safety). */ + sidePanel: Signal; /** * #426 — the UPPER sidebar pane's role. Deliberately NOT persisted (unlike * `sidePanel`): the issue specifies "default to Databases for a fresh session", * which a localStorage-backed preference would break on every reload. Session - * UI state, never workspace JSON. + * UI state, never workspace JSON. `UpperPanelId` (#587) is DERIVED from the + * side-panel manifest rather than a hand-written union declared here. */ - upperRole: Signal<'databases' | 'dashboards'>; + upperRole: Signal; /** * #426 — the Dashboard tree's EXPLICIT repaint invalidation. The tree is a * projection of the committed workspace aggregate plus main-surface navigation @@ -757,8 +765,11 @@ export function createState(read: StateReader = { loadJSON, loadStr }): AppState // cleared (Clear all recent values / per-field Clear recent). // The `as` trusts the localStorage shape verbatim — no decoder exists today. varRecentDisabled: read.loadJSON(KEYS.varRecentDisabled, false) as boolean, - sidePanel: signal(read.loadStr(KEYS.sidePanel, 'saved')), - upperRole: signal<'databases' | 'dashboards'>('databases'), + // #587: fail-closed decode at the load boundary — anything other than a + // recognized persisted value (missing, corrupt, or the registry's own + // id) resolves to 'saved' (Library), the documented default. + sidePanel: signal(decodeSidePanelKey(read.loadStr(KEYS.sidePanel, 'saved'))), + upperRole: signal('databases'), dashboardTreeRevision: signal(0), dashboardTreeUi: new Map(), // The localStorage startup ingress: v1 entries become canonical v2 in diff --git a/src/styles.css b/src/styles.css index e86d63b0..e5a63c04 100644 --- a/src/styles.css +++ b/src/styles.css @@ -1737,6 +1737,19 @@ body.detached-tab .graph-overlay-panel { loaded columns and scroll across a role switch. */ .upper-role-host[hidden] { display: none; } +/* ------------ side-panel registry hosts (#587) ------------ + The LOWER pane's persistent hosts (Library | History) — the same layout + contract as `.upper-role-host` above (a generic wrapper `side-panel- + registry.ts` builds for any panel that supplies no host of its own), kept + as its own class rather than reusing `.upper-role-host` verbatim: e2e specs + (`tests/e2e/dashboard-tree.spec.js`) address `.upper-role-host[data-role=…]` + directly, and this avoids any risk of an unrelated selector collision. */ +.side-panel-host { + flex: 1; min-height: 0; + display: flex; flex-direction: column; +} +.side-panel-host[hidden] { display: none; } + /* ------------ Dashboard hierarchy tree (#426) ------------ */ .dash-tree-row { position: relative; } /* The group rows (Variables / Panels) are structure, not content. */ diff --git a/src/ui/app-shell.ts b/src/ui/app-shell.ts index cd1a8cfa..8c9940d3 100644 --- a/src/ui/app-shell.ts +++ b/src/ui/app-shell.ts @@ -34,9 +34,12 @@ import { MOBILE_BREAKPOINT_PX } from '../state.js'; import type { AppState as State } from '../state.js'; import { effect } from '@preact/signals-core'; import { renderSchema } from './schema.js'; -import { buildSidebarUpper, renderUpperRoleTabs } from './sidebar-upper.js'; +import { buildSidebarUpper } from './sidebar-upper.js'; import { renderDashboardTree, cancelDashboardTreeClicks } from './dashboard-tree.js'; -import { renderSavedHistory } from './saved-history.js'; +import { buildProductionSidePanelRegistry, renderSidePanelTabs, MOBILE_PANES } from './side-panel-registry.js'; +import type { SidePanelRegistry } from './side-panel-registry.js'; +import { sidePanelKeyFor, lowerIdForKey } from '../core/side-panels.js'; +import type { SidePanelId, UpperPanelId, LowerPanelId } from '../core/side-panels.js'; import { renderLibraryTitle } from './file-menu.js'; import { applyConnectionStatus } from './app-header.js'; import type { DragCtx, DragRect, DragStartEvent, SplitterAxis } from './splitters.js'; @@ -95,6 +98,11 @@ export interface AppShellHandle { * one. */ showHost(kind: SurfaceHostKind): void; + /** #587 — the side-panel registry (Databases/Dashboards/Library/History). + * Reachable via `app.shell?.sidePanels` from anywhere `app` is held — + * `saved-history.ts`'s `renderSavedHistory` compatibility export and the + * workbench's clean-run hook both address panels only through this. */ + sidePanels: SidePanelRegistry; dispose(): void; } @@ -145,13 +153,39 @@ export function mountAppShell(deps: AppShellDeps): AppShellHandle { h('div', { class: 'schema-search' }, h('div', { class: 'search-wrap' }, Icon.search(), app.dom.schemaSearchInput)), app.dom.schemaList, ]); + // #587 — the ONE registry: all four panels (Databases/Dashboards over the + // upper pane's existing hosts; Library/History over fresh persistent hosts + // `side-panel-registry.ts`'s own factory builds for them). This shell names + // no concrete panel-def or panel id — it hands the factory only what it + // genuinely owns (the two upper hosts `buildSidebarUpper` just built, and + // `app`). Adding a fifth panel means adding one def to + // `buildProductionSidePanelRegistry`'s array plus that panel's own module — + // never touching this file, `app-preferences.ts`, `state.ts`, or + // `workbench-session.ts` (#587 AC5; PR #600 fixed this file's own prior + // violation, where the four concrete defs were listed right here). + const registry = buildProductionSidePanelRegistry(app, upper); + // #600 review finding 1 (round 2): `schemaPane` is composed from the + // registry's OWN upper-pane entries — never by naming + // `upper.databasesHost`/`upper.dashboardsHost` here — exactly like + // `savedPane` below already does for the lower pane (`lowerHosts`). A host + // named literally by this shell would still get a tab-row entry (the + // generic renderer reads `registry.entries` for that) but, for any FUTURE + // upper panel that isn't one of today's two, no route into the document: + // selecting it would hide the visible panel and reveal a host that was + // never appended anywhere. `upperEntries` is also read by the tab-row + // effect further down, so there is exactly one filtered view of the upper + // pane, not two that could disagree. + const upperEntries = registry.entries.filter((entry) => entry.pane === 'upper'); + app.dom.upperRoleTabs = h('div', { class: 'side-tabs upper-role-tabs' }); const schemaPane = h('div', { class: 'side-pane schema-pane', style: { height: state.sideSplitPct + '%', flexShrink: '0', minHeight: '0' } }, - app.dom.upperRoleTabs!, upper.databasesHost, upper.dashboardsHost); + app.dom.upperRoleTabs, ...upperEntries.map((entry) => entry.host)); - app.dom.savedTabsRow = h('div', { class: 'side-tabs' }); - app.dom.savedSearch = h('div', { class: 'saved-search' }); - app.dom.savedList = h('div', { class: 'saved-list' }); - const savedPane = h('div', { class: 'side-pane saved-pane', style: { flex: '1', minHeight: '0' } }, app.dom.savedTabsRow, app.dom.savedSearch, app.dom.savedList); + // The lower pane's tab row is a plain local element now (#587 — no AppDom + // field: nothing outside this closure needs to address it by name; the + // registry's own hosts are what `app.shell.sidePanels` exposes instead). + const lowerTabsRow = h('div', { class: 'side-tabs' }); + const lowerHosts = registry.entries.filter((entry) => entry.pane === 'lower').map((entry) => entry.host); + const savedPane = h('div', { class: 'side-pane saved-pane', style: { flex: '1', minHeight: '0' } }, lowerTabsRow, ...lowerHosts); const sidebar = h('div', { class: 'sidebar', style: { width: state.sidebarPx + 'px' } }); // #586 — the docked right-inspector's own resize handle runs through this @@ -207,9 +241,19 @@ export function mountAppShell(deps: AppShellDeps): AppShellHandle { // shows. The internal `data-seg`/`data-mobile-tab` values and the `.schema-pane` // selectors they key are deliberately unchanged — this is a label change, not a // restructuring of the mobile CSS (touch behaviour stays out of scope per #426). + // + // #587: driven from `MOBILE_PANES` (side-panel-registry.ts) — a SEPARATE + // small table from the panel manifest, because this control picks a PANE + // (which of the two persists to `mobileTab`, session-only), never a + // specific panel; each segment's icon is the registry's OWN icon for that + // pane's first panel, so it can never disagree with the desktop tab row. + // Exact same labels/icons/`data-seg` values as before this phase — no + // behaviour or visual change intended. app.dom.mobileSegmented = h('div', { class: 'mobile-segmented' }, - h('button', { class: 'mseg-btn', 'data-seg': 'schema', onclick: () => { state.mobileTab.value = 'schema'; } }, Icon.database(), h('span', null, 'Explore')), - h('button', { class: 'mseg-btn', 'data-seg': 'library', onclick: () => { state.mobileTab.value = 'library'; } }, Icon.layers(), h('span', null, 'Library'))); + ...MOBILE_PANES.map((seg) => h('button', { + class: 'mseg-btn', 'data-seg': seg.seg, + onclick: () => { state.mobileTab.value = seg.seg; }, + }, registry.entries.find((entry) => entry.pane === seg.pane)!.icon(), h('span', null, seg.label)))); sidebar.append(app.dom.mobileSegmented, schemaPane, app.dom.sideSplit, savedPane); const sideHandle = h('div', { class: 'col-resize', onmousedown: (e: DragStartEvent) => doStartDrag(e, 'col', dragCtx) }); @@ -340,21 +384,26 @@ export function mountAppShell(deps: AppShellDeps): AppShellHandle { state.isMobile.value; renderSchema(app); })); - // #426: the upper role tabs. Both counts are reactive — the Databases count - // tracks the schema load (and is omitted while it is pending or failed), and the - // Dashboards count tracks the committed collection through the tree's explicit - // invalidation signal, since `currentWorkspace` is not itself a signal. + // #426/#587: the upper pane's tab row — now the SAME generic renderer the + // lower pane uses, reading label/icon/count straight from the registry + // entries rather than a second hard-coded table. Both counts are reactive — + // the Databases count tracks the schema load (and is omitted while it is + // pending or failed), and the Dashboards count tracks the committed + // collection through the tree's explicit invalidation signal, since + // `currentWorkspace` is not itself a signal — both live inside each entry's + // own `tabAdornment()` (sidebar-upper.ts), read here only through the + // generic renderer. Also exposes exactly one role host via the registry's + // pane-scoped `showPanel` (replacing #426's own `upper.showRole`). + // `upperEntries` itself is declared once, above, alongside `schemaPane`'s + // own composition from the same filtered view. + const selectUpperPanel = (id: SidePanelId): void => { state.upperRole.value = id as UpperPanelId; }; disposers.push(effect(() => { state.upperRole.value; state.schema.value; state.schemaError.value; state.dashboardTreeRevision.value; - renderUpperRoleTabs(app); - })); - // #426: expose exactly one role host, and repaint the Dashboard tree. Kept - // separate from the tab effect so a schema load does not rebuild the tree. - disposers.push(effect(() => { - upper.showRole(state.upperRole.value); + renderSidePanelTabs(app.dom.upperRoleTabs!, upperEntries, state.upperRole.value, selectUpperPanel); + registry.showPanel(state.upperRole.value); })); disposers.push(effect(() => { // The ONE reactive input the tree has: every trigger #426 lists (workspace @@ -370,19 +419,36 @@ export function mountAppShell(deps: AppShellDeps): AppShellHandle { state.schemaError.value; updateBanner(); })); - // Reactive repaint of the side panel: re-runs when the active panel changes - // (Library ↔ History). Data-driven repaints (savedQueries/history mutations) - // still call renderSavedHistory directly until those slices are signals too. + // Reactive repaint of the lower pane's tab row + active panel — re-runs when + // the active panel changes (Library ↔ History) or the Library count might + // have (see below). Data-driven repaints (savedQueries/history mutations) + // still call the `renderSavedHistory` compatibility export directly until + // those slices are signals too — it delegates to `refreshLowerPane` below + // too (not the registry's own bare `refreshActiveSidePanels`), because the + // Library tab's live count must repaint alongside the list on exactly the + // same events (a star/delete/rename doesn't bump any signal this effect + // depends on). // // #427 added the projection revision. Library membership is now a function of // `dashboards[]` — a query is in the Library exactly while no Dashboard member // references it — so a committed Dashboard change can move a query in or out of // this list without `savedQueries` changing at all. It is the same one signal // the Dashboard tree subscribes to, bumped from the single projection funnel. + const lowerEntries = registry.entries.filter((entry) => entry.pane === 'lower'); + const selectLowerPanel = (id: SidePanelId): void => { + const key = sidePanelKeyFor(id as LowerPanelId); + prefs.save('sidePanel', key); + state.sidePanel.value = key; + }; + const refreshLowerPane = (): void => { + const activeId = lowerIdForKey(state.sidePanel.value); + renderSidePanelTabs(lowerTabsRow, lowerEntries, activeId, selectLowerPanel); + registry.showPanel(activeId); + }; disposers.push(effect(() => { state.sidePanel.value; state.dashboardTreeRevision.value; - renderSavedHistory(app); + refreshLowerPane(); })); // Reactive repaint of the header library title (name + unsaved-changes dot): // re-runs when the name or dirty flag changes. The edit-mode toggle is driven @@ -432,6 +498,12 @@ export function mountAppShell(deps: AppShellDeps): AppShellHandle { dashboardHost.hidden = kind !== 'dashboard'; mainRow.dataset.surface = kind; }, + // `refreshActiveSidePanels` is NOT the registry's own bare method here — + // it wraps `refreshLowerPane` (declared above, alongside this file's own + // lower-pane effect) so the compatibility `renderSavedHistory(app)` seam + // (10 call sites, none of which bump a signal this shell's effects watch) + // also repaints the Library tab's live count, not just the active body. + sidePanels: { ...registry, refreshActiveSidePanels: refreshLowerPane }, dispose: () => { // #426: a deferred single-click must not fire against a tree that is being // torn down (sign-out, a surface teardown) — the arbiter's timer outlives @@ -446,6 +518,10 @@ export function mountAppShell(deps: AppShellDeps): AppShellHandle { win.removeEventListener('resize', onWindowResize); for (const dispose of disposers) dispose(); mq?.removeEventListener('change', onMobileChange); + // #587 — tear every panel down once (each panel's own `dispose`; none of + // the four today does more than close over nothing, but a future panel + // might own a real resource). + registry.dispose(); }, }; } diff --git a/src/ui/app.ts b/src/ui/app.ts index fb3fd974..bcaa7922 100644 --- a/src/ui/app.ts +++ b/src/ui/app.ts @@ -235,6 +235,11 @@ export function createApp(env: CreateAppEnv = {}): App { matchMedia: env.matchMedia || (typeof win.matchMedia === 'function' ? win.matchMedia.bind(win) : null), }; const app = appBase as App; + // #587: null until `ensureShell()`'s first call, and null again after + // `disposeShell()` — see both for the mirroring. Reachable as `app.shell` + // from controller-construction time (this line) onward, including every + // wiring point below that runs before any shell exists. + app.shell = null; // Chromium (+ a secure context) only — Firefox/Safari and plain-HTTP have no // File System Access API. The Export button feature-detects this at build // time and renders aria-disabled + a tooltip rather than hiding outright. @@ -991,7 +996,12 @@ export function createApp(env: CreateAppEnv = {}): App { activeTab: () => app.activeTab(), hooks: { renderResults: () => renderResults(app), - renderSavedHistory: () => renderSavedHistory(app), + // #587 AC3: renamed from `renderSavedHistory` — called UNCONDITIONALLY + // on every clean run now (`workbench-session.ts` no longer knows + // `sidePanel` exists at all); which panel (if any) actually repaints is + // this hook's own decision, delegated to the registry exactly like + // `app.recordHistory`'s single-statement path above. + onRunComplete: () => app.shell?.sidePanels.notifyRunComplete(), cancelSchemaGraph, loadSchema: () => { void catalog.loadSchema(); }, recordHistory: (tab, sql) => app.recordHistory(tab, sql), @@ -1563,12 +1573,17 @@ export function createApp(env: CreateAppEnv = {}): App { // --- saved / history bridges ------------------------------------------ // The history-recording POLICY itself now lives in `saved.recordHistory` - // (#276 Phase 4C) — this wrapper's own conditional History-panel repaint is - // a rendering concern the service must never own (see its header comment), - // so it stays here, unchanged. + // (#276 Phase 4C) — this wrapper's own History-panel repaint is a rendering + // concern the service must never own (see its header comment), so it stays + // here. #587: the "only repaint when History is the active panel" decision + // moved INTO the registry's `notifyRunComplete` (it dispatches to the + // active lower panel only, and only if that panel defines the hook — today + // only History does) — this wrapper no longer string-compares a panel id. + // `app.shell` is null before the first shell mount, so this is always a + // safe no-op that early. app.recordHistory = (tab, sqlText) => { saved.recordHistory(tab, sqlText); - if (app.state.sidePanel.value === 'history') renderSavedHistory(app); + app.shell?.sidePanels.notifyRunComplete(); }; // --- share + star ------------------------------------------------------ @@ -2110,6 +2125,10 @@ export function createApp(env: CreateAppEnv = {}): App { updateBanner: app.updateBanner, startDrag, }); + // #587: mirrored onto `app` so any module holding `app` (not just this + // closure) can reach the side-panel registry through `app.shell` — e.g. + // `saved-history.ts`'s `renderSavedHistory` compatibility export. + app.shell = shell; if (!inlineLogin) { inlineLogin = mountInlineLogin( app as App & { root: Element }, @@ -2127,6 +2146,7 @@ export function createApp(env: CreateAppEnv = {}): App { inlineLogin = null; shell?.dispose(); shell = null; + app.shell = null; }; // What the Dashboard surface renders THIS pass. `dashboardId` is `null` only // for the legacy empty-collection entry point, which lands on the Dashboard's diff --git a/src/ui/app.types.ts b/src/ui/app.types.ts index e53b5bb6..25bdc6e1 100644 --- a/src/ui/app.types.ts +++ b/src/ui/app.types.ts @@ -38,6 +38,14 @@ import type { ExportService } from '../application/export-service.js'; import type { QueryDocumentSession } from '../application/query-document-session.js'; import type { SavedQueryService } from '../application/saved-query-service.js'; import type { OAuthDocumentRecoveryRestoreResult } from '../application/oauth-document-recovery-session.js'; +// Type-only, and circular with `app-shell.ts` (which imports `App` from this +// file) — TypeScript erases `import type` entirely, so this introduces no +// runtime cycle. `AppShellHandle` is the ONE seam `app.shell` exposes: the +// side-panel registry (`refreshActiveSidePanels`/`notifyRunComplete`), which +// must be reachable from controller-construction time (before any shell +// exists) through shell disposal — see `app-shell.ts`'s own header comment +// and `saved-history.ts`'s `renderSavedHistory` compatibility export. +import type { AppShellHandle } from './app-shell.js'; export type { QueryTab as Tab, AppState as State } from '../state.js'; // #457: the `mutateWorkspace` contract types are DECLARED in `state.ts`, beside @@ -133,9 +141,11 @@ export interface AppDom { cancelInspectorDrag?: () => void; reclampInspectorWidth?: () => void; runElapsedEl?: HTMLElement; - savedList?: HTMLElement; - savedSearch?: HTMLElement; - savedTabsRow?: HTMLElement; + // #587: `savedList`/`savedSearch`/`savedTabsRow` are GONE — the Library and + // History panels each own a persistent host built by + // `side-panel-registry.ts`'s `buildSidePanelRegistry`, reachable via + // `app.shell.sidePanels`, not through named `AppDom` fields. Adding a new + // side panel needs no `AppDom` field at all (#587 AC5). schemaList?: HTMLElement; specEditorView?: EditorView; sqlEditorView?: EditorView; @@ -261,6 +271,14 @@ export interface App { dom: AppDom; root: Element | null; document: Document; + /** #587 (#425/#586 precedent): the persistent app frame's handle, `null` + * before the first `mountAppShell` call and after a teardown + * (`ensureShell`/`disposeShell` in app.ts keep this mirrored). Exposes the + * side-panel registry through `shell.sidePanels` — the seam + * `saved-history.ts`'s `renderSavedHistory` compatibility export and the + * workbench's clean-run hook address, both of which must stay safe to call + * before any shell exists or after it is torn down. */ + shell: AppShellHandle | null; /** Set by shared overlay primitives for the duration of their open lifecycle. */ keyboardOwner: KeyboardOwner | null; /** Acquire exclusive application-keyboard ownership. The returned idempotent diff --git a/src/ui/saved-history.ts b/src/ui/saved-history.ts index 592fd8cf..3ddad222 100644 --- a/src/ui/saved-history.ts +++ b/src/ui/saved-history.ts @@ -1,7 +1,19 @@ -// The bottom sidebar pane: a Saved / History switcher, a search box, and the -// two lists. Saved items support favorite (star), inline rename (pencil) and -// delete (trash). The search filters the active list (name/description/sql for -// Library, sql for History); it re-renders only the list so typing keeps focus. +// The bottom sidebar pane's two panels — Library and History (#587: two +// registry entries, no longer a switcher this module builds itself). Saved +// items support favorite (star), inline rename (pencil) and delete (trash). +// The search filters the active list (name/description/sql for Library, sql +// for History); it re-renders only the list so typing keeps focus. +// +// #587: `libraryPanelDef`/`historyPanelDef` are what `app-shell.ts` hands to +// `buildSidePanelRegistry` — each panel gets its OWN persistent search+list +// host, built once in `mount(host)` and never rebuilt. `state.libraryFilter` +// stays ONE shared string (splitting it per panel is #487 phase 3's job, out +// of scope here) — which is exactly why `ownsTheList` exists below: with two +// PERSISTENT hosts, an event from the INACTIVE panel's leftover search input +// would rewrite the shared filter and repaint the OTHER panel's list. A real +// browser never delivers events to a `hidden` subtree, so this is a guard +// against a host a future caller (or a test) can still reach directly, not a +// redesign of the shared-filter decision. import { h } from './dom.js'; import { Icon } from './icons.js'; @@ -22,6 +34,10 @@ import { libraryQueries } from '../dashboard/model/query-ownership.js'; import { openLibraryAssignMenu } from './library-assign-menu.js'; import type { App } from './app.types.js'; import type { SavedQueryV2 } from '../generated/json-schema.types.js'; +// From the type-only seam file, not `./side-panel-registry.js` itself — see +// `sidebar-upper.ts`'s identical import for why (`side-panel-registry.ts` +// imports THIS module's `libraryPanelDef`/`historyPanelDef` at runtime now). +import type { MountedSidePanel, SidePanelDef } from './side-panel-registry.types.js'; /** The `resultView` signal's value union (state.ts) — `launchView`/`'panel'` * below are proven members of it (SAVED_VIEWS membership, or the queryless @@ -101,84 +117,121 @@ function libraryEntries(app: App): SavedQueryV2[] { return app.state.savedQueries.filter((query) => libraryIds.has(query.id)); } +/** + * Compatibility seam (#587): 10 call sites across the app (5 in this file, 4 + * in `app.ts`, 1 in `file-menu.ts` — counted with `rg`, excluding this + * definition and import lines) call this to repaint whichever lower panel is + * active — a star/delete/rename completion, a Dashboard-membership + * projection bump, or the tab switch itself. It now delegates to the mounted + * shell's registry, which resolves + * "the active lower panel" itself; a no-op before the shell mounts or after + * it is disposed (both real states — `app.shell` starts/ends `null`), never + * a thrown error against a controller wiring that runs before any DOM exists. + */ export function renderSavedHistory(app: App): void { - const tabsRow = app.dom.savedTabsRow; - const list = app.dom.savedList; - if (!tabsRow || !list) return; - const state = app.state; - // #427: the count is the LIBRARY count, not every stored query — the owned - // copies are reachable through the Dashboard tree, not through this list. + app.shell?.sidePanels.refreshActiveSidePanels(); +} + +/** The Library tab's live count (#427: the LIBRARY count, not every stored + * query — the owned copies are reachable through the Dashboard tree, not + * through this list). `null` renders no adornment, exactly like today. */ +function libraryCountNode(app: App): Node | null { const count = libraryEntries(app).length; + return count ? h('span', { class: 'side-count' }, '· ' + count) : null; +} - // Switching panes clears the search so each tab starts unfiltered. Clear the - // (plain) filter first, then set the sidePanel signal — its render effect runs - // synchronously on assignment and must see the cleared filter. No manual - // re-render call: the effect in createApp() repaints. - const switchTo = (panel: string): void => { - state.libraryFilter = ''; - app.prefs.save('sidePanel', panel); - state.sidePanel.value = panel; +/** + * Build ONE lower-pane panel's persistent search+list pair and its + * `MountedSidePanel` controller. Shared by both Library and History + * (`isLibrary` is the only branch) — the DOM shape, search wiring, and + * ownership guard are otherwise identical. + */ +function mountLowerPanel(app: App, host: HTMLElement, isLibrary: boolean): MountedSidePanel { + const search = h('div', { class: 'saved-search' }); + const list = h('div', { class: 'saved-list' }); + host.append(search, list); + + const hasItems = (): boolean => (isLibrary ? libraryEntries(app).length > 0 : app.state.history.length > 0); + + const renderList = (): void => { + list.replaceChildren(); + if (isLibrary) renderSaved(app, list); else renderHistory(app, list); }; - tabsRow.replaceChildren( - h('button', { - class: 'side-tab' + (state.sidePanel.value === 'saved' ? ' active' : ''), - onclick: () => switchTo('saved'), - }, Icon.layers(), h('span', null, 'Library'), - count ? h('span', { class: 'side-count' }, '· ' + count) : null), - h('button', { - class: 'side-tab' + (state.sidePanel.value === 'history' ? ' active' : ''), - onclick: () => switchTo('history'), - }, Icon.history(), h('span', null, 'History')), - ); + // #587: with a PERSISTENT host, this panel's search input can still receive + // a dispatched event while hidden (a real browser never delivers one to a + // `display: none` subtree, but nothing before #587 needed to rely on that — + // there was only ever one shared pair). `state.libraryFilter` stays ONE + // shared string (splitting it per panel is #487 phase 3's job), so an event + // from the INACTIVE panel's stale input must not rewrite it or repaint the + // OTHER panel's list — `ownsTheList` is that guard, checked at the top of + // every handler it wires below. + const ownsTheList = (): boolean => !host.hidden; - renderSearch(app); - renderList(app); -} + const renderSearchBox = (): void => { + const state = app.state; + search.replaceChildren(); + if (!hasItems()) return; -/** Re-render just the active list (called on every keystroke without rebuilding - * the search input, so the caret/focus survive filtering). */ -function renderList(app: App): void { - // `!`: every caller (renderSavedHistory, renderSearch below) only reaches - // this after confirming `app.dom.savedList` is mounted. - const list = app.dom.savedList!; - list.replaceChildren(); - if (app.state.sidePanel.value === 'saved') renderSaved(app, list); - else renderHistory(app, list); -} + const input = h('input', { + class: 'sv-search-input', type: 'text', + placeholder: isLibrary ? 'Search library queries…' : 'Search history…', + value: state.libraryFilter, + }); + const clear = h('button', { class: 'sv-search-clear', title: 'Clear' }, Icon.close()); + const syncClear = (): void => { clear.style.display = input.value ? '' : 'none'; }; + const setFilter = (v: string): void => { + if (!ownsTheList()) return; + input.value = v; state.libraryFilter = v; syncClear(); renderList(); + }; -/** - * Render the search box into `app.dom.savedSearch` (built once per full render; - * a tab with no items shows nothing). Its `input` handler mutates - * `state.libraryFilter` and re-renders only the list, so it stays focused. - */ -function renderSearch(app: App): void { - const box = app.dom.savedSearch; - if (!box) return; - const state = app.state; - // Gated on the LIBRARY count (#427): a workspace whose every query is owned - // has an empty list, so a search box over it would filter nothing. - const hasItems = state.sidePanel.value === 'saved' - ? libraryEntries(app).length > 0 - : state.history.length > 0; - box.replaceChildren(); - if (!hasItems) return; + input.addEventListener('input', () => { + if (!ownsTheList()) return; + state.libraryFilter = input.value; syncClear(); renderList(); + }); + input.addEventListener('keydown', (e) => { if (e.key === 'Escape') { e.preventDefault(); setFilter(''); } }); + clear.addEventListener('click', () => { setFilter(''); input.focus(); }); + syncClear(); - const input = h('input', { - class: 'sv-search-input', type: 'text', - placeholder: state.sidePanel.value === 'saved' ? 'Search library queries…' : 'Search history…', - value: state.libraryFilter, - }); - const clear = h('button', { class: 'sv-search-clear', title: 'Clear' }, Icon.close()); - const syncClear = (): void => { clear.style.display = input.value ? '' : 'none'; }; - const setFilter = (v: string): void => { input.value = v; state.libraryFilter = v; syncClear(); renderList(app); }; + search.append(h('span', { class: 'sv-search-icon' }, Icon.search()), input, clear); + }; - input.addEventListener('input', () => { state.libraryFilter = input.value; syncClear(); renderList(app); }); - input.addEventListener('keydown', (e) => { if (e.key === 'Escape') { e.preventDefault(); setFilter(''); } }); - clear.addEventListener('click', () => { setFilter(''); input.focus(); }); - syncClear(); + const render = (): void => { renderSearchBox(); renderList(); }; - box.append(h('span', { class: 'sv-search-icon' }, Icon.search()), input, clear); + return { + render, + // Switching panes clears the search so each tab starts unfiltered — + // matches the pre-#587 behaviour ('clears the filter when switching + // tabs'), just triggered by the panel becoming inactive rather than by + // the tab-row click handler itself (which no longer lives in this + // module — see `app-shell.ts`'s generic `onSelect`). + deactivate: () => { app.state.libraryFilter = ''; }, + // #587 AC3: only History repaints after a clean run — dispatch itself is + // scoped to "the active lower panel" by the registry's `notifyRunComplete`, + // so this only ever fires while History is genuinely visible. + onRunComplete: isLibrary ? undefined : render, + dispose: () => {}, + }; +} + +/** The registry's Library entry (#587 deliverable 1/3). */ +export function libraryPanelDef(app: App): SidePanelDef { + return { + id: 'library', pane: 'lower', label: 'Library', icon: Icon.layers, + accessibleLabel: 'Open Library navigation', + tabAdornment: () => libraryCountNode(app), + mount: (host) => mountLowerPanel(app, host, true), + }; +} + +/** The registry's History entry (#587 deliverable 1/3). No tab adornment — + * History never carried a count. */ +export function historyPanelDef(app: App): SidePanelDef { + return { + id: 'history', pane: 'lower', label: 'History', icon: Icon.history, + accessibleLabel: 'Open query History', + mount: (host) => mountLowerPanel(app, host, false), + }; } function renderSaved(app: App, list: HTMLElement): void { diff --git a/src/ui/side-panel-registry.ts b/src/ui/side-panel-registry.ts new file mode 100644 index 00000000..d49b24c0 --- /dev/null +++ b/src/ui/side-panel-registry.ts @@ -0,0 +1,237 @@ +// #587 — the side-panel registry: the single place that maps each side-panel +// id to what a container needs to HOST it (a label, an icon factory, an +// accessible label, an optional live tab adornment) and to MOUNT it (a +// persistent host element + a `MountedSidePanel` lifecycle controller). The +// generic tab-row renderer (`renderSidePanelTabs`) and the generic activation +// dispatcher (`buildSidePanelRegistry`'s `showPanel`) are the reason adding a +// panel never touches `app-shell.ts`, `app-preferences.ts`, `state.ts`, or +// `workbench-session.ts` (#587 AC5) — this module's own +// `buildProductionSidePanelRegistry` (below) is now the ONE place the four +// real panel defs are listed, so `app-shell.ts` names no concrete panel at +// all: it hands this factory the two upper hosts it built and `app`, nothing +// more (PR #600 review, #587 finding 1 — the composition literally used to +// live in `app-shell.ts`, which is exactly what AC5 forbids). +// +// Persistent hosts, built ONCE and never rebuilt (#587 AC6, carried over from +// #487 phase 2's `nav-sections.ts`): switching panels only flips `hidden`. +// That is what preserves, by construction rather than by save/restore logic, +// each panel's own search text/focus, scroll, and any lazily-loaded content — +// across BOTH panes uniformly now, not just the upper one (#426's original +// scope). `mount(host)` therefore runs exactly ONCE per shell lifetime, at +// registry construction — never once per activation (the issue's own Tests +// wording says "per activation", which directly contradicts persistent hosts; +// AC6 is the binding decision here, see docs/ADR-0004's #587 addendum). +// `activate`/`deactivate`/`render` run on every transition instead, and +// `dispose` once, at shell teardown. + +import { h } from './dom.js'; +import { SIDE_PANELS } from '../core/side-panels.js'; +import type { SidePanelId, SidePanelPane } from '../core/side-panels.js'; +import { databasesPanelDef, dashboardsPanelDef } from './sidebar-upper.js'; +import type { SidebarUpperHandle } from './sidebar-upper.js'; +import { libraryPanelDef, historyPanelDef } from './saved-history.js'; +import type { App } from './app.types.js'; +import type { + MountedSidePanel, SidePanelDef, SidePanelEntry, SidePanelRegistry, +} from './side-panel-registry.types.js'; + +// Re-exported verbatim so every existing importer of these names from THIS +// module keeps working unchanged (`app-shell.ts`, the unit/e2e fixtures). +// The interfaces themselves now live in `side-panel-registry.types.ts` — see +// that file's own header comment for why: `sidebar-upper.ts`/ +// `saved-history.ts` need these TYPES, and this module needs THEIR concrete +// `*PanelDef` factories at runtime (the import two lines above), and having +// both edges point through this module would be a real module-graph cycle. +export type { MountedSidePanel, SidePanelDef, SidePanelEntry, SidePanelRegistry }; + +/** Build a registry from an explicit list of defs — the generic core every + * production/test caller goes through. Exported so a test can inject a fake + * def (#587 AC5's runtime proof) without touching any of the four files this + * issue forbids editing to add a panel. */ +export function buildSidePanelRegistry(defs: readonly SidePanelDef[]): SidePanelRegistry { + const entries: SidePanelEntry[] = defs.map((def) => { + const host = def.host ?? h('div', { class: 'side-panel-host', 'data-panel': def.id, hidden: true }); + const mounted = def.mount(host); + return { + id: def.id, pane: def.pane, label: def.label, icon: def.icon, + accessibleLabel: def.accessibleLabel, tabAdornment: def.tabAdornment, + host, mounted, + }; + }); + // Reject a duplicate id at CONSTRUCTION (PR #600 review, round 4). Nothing + // upstream enforces uniqueness: `Record` cannot, because a + // TypeScript union collapses duplicates, so a second manifest row reusing an + // existing id needs no additional key; and the manifest-parity test cannot, + // because it compares the registry against the same duplicated manifest and + // both sides mirror the duplicate. This seam also accepts arbitrary INJECTED + // defs (the AC5 fake-panel proof, the e2e fixture), which are not + // manifest-backed at all, so the check has to live here. + // + // Failing loudly beats the silent breakage a duplicate causes: `byId` below + // would keep only the LAST entry; the normalize loop would leave BOTH hosts + // visible (each one's id equals its pane's default active id); and + // `showPanel` skips every candidate whose id equals its target, so it could + // never hide the shadowed sibling — a permanently double-rendered pane. + const seen = new Set(); + for (const entry of entries) { + if (seen.has(entry.id)) throw new Error(`side-panel-registry: duplicate panel id "${entry.id}"`); + seen.add(entry.id); + } + const byId = new Map(entries.map((entry) => [entry.id, entry])); + // One "currently active" id per pane, defaulting to the FIRST entry + // declared for that pane (matches every pane's existing default: Databases + // above, Library below) — corrected to the real value by the caller's own + // reactive exposure effect on its very first run, exactly like #426's + // upper-pane handle already worked. + const activeByPane = new Map(); + for (const entry of entries) if (!activeByPane.has(entry.pane)) activeByPane.set(entry.pane, entry.id); + // Normalize each host's initial `hidden` to match its pane's default active + // id — WITHOUT firing `activate`/`render` (those run only on an explicit + // `showPanel` call, exactly like #426's upper-pane handle already worked: + // the caller's own reactive exposure effect performs the very first + // `showPanel`, synchronously, immediately after construction). This just + // means an already-correctly-shown default panel's first real activation + // is not reported as a transition. + for (const candidate of entries) candidate.host.hidden = activeByPane.get(candidate.pane) !== candidate.id; + + const entry = (id: SidePanelId): SidePanelEntry => { + const found = byId.get(id); + if (!found) throw new Error(`side-panel-registry: unknown panel id "${id}"`); + return found; + }; + + const showPanel = (id: SidePanelId): void => { + const target = entry(id); + // Two passes, deliberately — see the ordering contract in this method's + // own interface doc above. Pass 1 tears down EVERY other visible sibling + // in the target's pane first, so a sibling's `deactivate` (which may + // clear state the target's own `render` reads, e.g. the shared library + // filter) can never run after the target has already painted. Pass 2 + // then reveals/activates/renders the target, once every sibling's + // teardown above is guaranteed complete. A single pass over `entries` + // made this order-dependent on manifest position instead. + for (const candidate of entries) { + if (candidate.pane !== target.pane || candidate.id === id) continue; + if (!candidate.host.hidden) { + candidate.host.hidden = true; + candidate.mounted.deactivate?.(); + } + } + if (target.host.hidden) { + target.host.hidden = false; + target.mounted.activate?.(); + } + target.mounted.render(); + activeByPane.set(target.pane, id); + }; + + const activeId = (pane: SidePanelPane): SidePanelId => { + // `!`: every pane present in `entries` got a default above; a pane with no + // entries at all is a construction error, not a runtime one. + return activeByPane.get(pane)!; + }; + + return { + entries, + entry, + showPanel, + activeId, + refreshActiveSidePanels: () => { entry(activeId('lower')).mounted.render(); }, + notifyRunComplete: () => { entry(activeId('lower')).mounted.onRunComplete?.(); }, + dispose: () => { for (const e of entries) e.mounted.dispose(); }, + }; +} + +type ProductionUpperHosts = Pick; + +/** + * One production factory per `SidePanelId`, keyed by a `Record` over the + * FULL manifest-derived union — adding a `SIDE_PANELS` row without adding its + * key here is a **compile error** ("Property … is missing"), not a silent + * gap a test would need to catch (PR #600 review round 3, finding 1: the old + * hand-written four-call array could drift from the manifest with nothing + * red). Every factory takes the same `(app, upperHosts)` shape so this stays + * a plain exhaustive map rather than special-casing at the call site: the two + * upper factories read `upperHosts`, the two lower ones ignore it. + */ +const SIDE_PANEL_FACTORIES: Record SidePanelDef> = { + databases: (app, upperHosts) => databasesPanelDef(app, upperHosts.databasesHost), + dashboards: (app, upperHosts) => dashboardsPanelDef(app, upperHosts.dashboardsHost), + library: (app) => libraryPanelDef(app), + history: (app) => historyPanelDef(app), +}; + +/** + * The ONE production wiring: all four real panels (Databases/Dashboards over + * the upper pane's existing hosts; Library/History over fresh persistent + * hosts `buildSidePanelRegistry` builds for them), through the exact same + * generic core every other caller (tests, the `dashboard-membership.html` e2e + * fixture) goes through. `app-shell.ts` calls only this — it hands over the + * two upper hosts it already built and `app`, and never imports a concrete + * panel-def factory or names a panel id/label itself (#587 AC5). The def list + * is built by mapping over `SIDE_PANELS` itself (not a separately hand-written + * order), so panel ORDER is decided by the manifest alone; the `SIDE_PANELS.map` + * below reads each row's own `id` to look up its factory, so a mismatched + * `pane` on a def is still possible in principle (defs are independent + * objects) and is what `tests/unit/side-panel-registry.test.ts`'s parity + * check exists to catch. Adding a fifth panel means adding one row to + * `SIDE_PANELS`, one key to `SIDE_PANEL_FACTORIES` above (TypeScript refuses + * to compile without it), and that panel's own module — never touching + * `app-shell.ts`. + */ +export function buildProductionSidePanelRegistry( + app: App, + upperHosts: ProductionUpperHosts, +): SidePanelRegistry { + return buildSidePanelRegistry(SIDE_PANELS.map((spec) => SIDE_PANEL_FACTORIES[spec.id](app, upperHosts))); +} + +/** Generic tab-row renderer, used identically for the upper and lower rows + * (#587 R2.1: one renderer, not a per-pane copy that could disagree about + * labels, icons, or the active state). Rebuilds the row's buttons — the + * ROW itself is a persistent container the caller owns; only its children + * are replaced, exactly like every other repainted-row pattern in this app + * (schema search stays outside the repainted schema list, etc.). */ +export function renderSidePanelTabs( + row: HTMLElement, + entries: readonly SidePanelEntry[], + activeId: SidePanelId, + onSelect: (id: SidePanelId) => void, +): void { + // #600 review finding 2 (round 2): no `aria-label` here. An explicit + // `aria-label` on a button REPLACES the accessible name that would + // otherwise be computed from its descendant content — and this button's + // descendants are exactly the visible label plus `tabAdornment()` (the + // live `.side-count` badge, e.g. "· 3"). Emitting `entry.accessibleLabel` + // here silently deleted the count from every counted tab's accessible + // name ("Databases · 3" became "Open Databases navigation") — a + // regression against the pre-#587 DOM, not a fix for the "dead contract + // surface" finding that motivated adding it. `accessibleLabel` still + // exists on `SidePanelDef`/`SidePanelEntry` for its real consumer (see + // that field's own doc comment) — it is simply never read here. + row.replaceChildren(...entries.map((entry) => h('button', { + class: 'side-tab' + (entry.id === activeId ? ' active' : ''), + type: 'button', + 'aria-pressed': entry.id === activeId ? 'true' : 'false', + onclick: () => onSelect(entry.id), + }, entry.icon(), h('span', null, entry.label), entry.tabAdornment ? entry.tabAdornment() : null))); +} + +/** The two PANES the mobile segmented control switches between (#126) — a + * DIFFERENT axis from the panel manifest above: `mobileTab` picks a PANE + * ('schema' shows the upper pane, 'library' shows the lower one), never a + * specific panel, and is session-only (state.ts documents this — never + * persisted). Kept as its own tiny table rather than derived from + * `SIDE_PANELS`, because "which two panes exist" and "which panels sit in a + * pane" are genuinely different facts; deriving one from the other here + * would force a same-shaped coincidence, not remove real duplication. */ +export const MOBILE_PANES = [ + { pane: 'upper', seg: 'schema', label: 'Explore' }, + { pane: 'lower', seg: 'library', label: 'Library' }, +] as const satisfies readonly { pane: SidePanelPane; seg: string; label: string }[]; + +// Re-exported so a UI caller can read the manifest through this module (its +// presentation-layer owner) without also importing `core/` directly, mirroring +// #487 phase 2's `nav-sections.ts` precedent. +export { SIDE_PANELS }; +export type { SidePanelId, SidePanelPane } from '../core/side-panels.js'; diff --git a/src/ui/side-panel-registry.types.ts b/src/ui/side-panel-registry.types.ts new file mode 100644 index 00000000..cc21da95 --- /dev/null +++ b/src/ui/side-panel-registry.types.ts @@ -0,0 +1,143 @@ +// #587 type-only seam contracts for `ui/side-panel-registry.ts` — extracted +// (PR #600 review, #587 finding 1) so the two DOM-owning panel modules +// (`sidebar-upper.ts`, `saved-history.ts`) can import these shapes WITHOUT a +// module-graph edge back to `side-panel-registry.ts` itself. That edge is +// needed the other direction now: `side-panel-registry.ts`'s +// `buildProductionSidePanelRegistry` imports the two modules' concrete +// `*PanelDef` factories at RUNTIME (not just their types) to be the one place +// that wires all four production panels, so `app-shell.ts` can call it +// without naming a single concrete panel (#587 AC5). Had `sidebar-upper.ts`/ +// `saved-history.ts` kept importing these types FROM `side-panel-registry.ts` +// directly, that would be a real cycle at the module-specifier level — ESM +// tolerates cycles at runtime, but the unbundled e2e harnesses +// (`tests/e2e/*.html`, which load `/src` as raw ESM with no bundler to +// resolve load order) are fragile against them. `import type` alone erases +// at build time and wouldn't have caused a RUNTIME cycle either, but this +// follows the repo's own established `src/**/*.types.ts` convention (ADR-0002 +// phase 0) for a type-only seam rather than relying on that erasure — and +// these interfaces have no executable statements, so (like every other +// `*.types.ts` file) they carry no coverage obligation. +// +// `side-panel-registry.ts` re-exports every name below verbatim, so no +// existing importer of e.g. `SidePanelDef` from `./side-panel-registry.js` +// needs to change. + +import type { SidePanelId, SidePanelPane } from '../core/side-panels.js'; + +/** What a mounted panel exposes to the registry after `mount(host)` runs once. + * Switching panels never calls `mount` again — only these. */ +export interface MountedSidePanel { + /** Refresh this panel's content from current state. Called once right after + * `mount`, and again on every activation (#587 R2.6: a persistent HIDDEN + * host must never show stale DOM once it becomes visible again). */ + render(): void; + /** Runs when this panel transitions from hidden to visible, BEFORE `render`. */ + activate?(): void; + /** Runs when this panel transitions from visible to hidden. */ + deactivate?(): void; + /** Fires after a clean query/script run, but ONLY when this panel is the + * active one in its pane (dispatch is scoped by the caller, not by this + * hook checking its own visibility) — issue Deliverable 1 names this + * `onRunComplete`; only the History panel defines it today. */ + onRunComplete?(): void; + /** Runs once, at shell disposal. */ + dispose(): void; +} + +/** A panel's complete presentation + behaviour, independent of any DOM until + * `mount` runs. */ +export interface SidePanelDef { + readonly id: SidePanelId; + readonly pane: SidePanelPane; + /** The visible label, exactly as today's switchers show it. */ + readonly label: string; + /** A FACTORY, not a prebuilt element — a tab row and (in principle) any + * other presentation each mint their own node from the same source. */ + readonly icon: () => SVGElement; + /** + * The accessible name for an ICON-ONLY presentation of this panel — e.g. + * the rail launchers a later issue adds, which show `icon()` with no + * visible text at all, so there is nothing for a browser to compute an + * accessible name from. Kept separate from `label` (a proven #487 phase-2 + * decision, #587 AC6). + * + * Must NOT be applied as an `aria-label` on the tab-row buttons + * (`renderSidePanelTabs`, `side-panel-registry.ts`): those buttons already + * render a visible label plus `tabAdornment()` (a live count, e.g. + * "· 3"), and an explicit `aria-label` on a button REPLACES the + * accessible name it would otherwise compute from its descendant + * content — so setting it there deletes the count from what assistive + * tech announces. (#600 review finding 2, round 2: exactly this was + * added and then reverted for that reason — see `renderSidePanelTabs`'s + * own comment.) + */ + readonly accessibleLabel: string; + /** + * An optional live badge next to the label — e.g. Databases'/Dashboards' + * row/Dashboard count, Library's live query count (#587 R2.7: three + * `.side-count` adornments exist today; dropping them on a generic tab row + * would be a visual regression against this issue's own non-goal). Called + * on every tab-row repaint; `null` renders nothing. History defines no + * adornment today, matching current behaviour. + */ + tabAdornment?(): Node | null; + /** + * Supply an existing host instead of letting the registry build a bare + * generic wrapper. ONLY the upper pane's two panels use this — their hosts + * (`upper-role-host[data-role=…]`) are read directly by e2e specs + * (`tests/e2e/dashboard-tree.spec.js`) and predate this registry (#426); + * preserving them verbatim avoids an unrelated selector churn. Library and + * History get a fresh generic host. + */ + host?: HTMLElement; + /** Called exactly once, at registry construction, with this entry's + * persistent host (either the one supplied above, or a fresh generic + * wrapper the registry built). Appends whatever content this panel owns + * and returns the lifecycle controller. */ + mount(host: HTMLElement): MountedSidePanel; +} + +/** A def, fully resolved: `host` is always present (built if not supplied), + * and `mount` has already run. */ +export interface SidePanelEntry { + readonly id: SidePanelId; + readonly pane: SidePanelPane; + readonly label: string; + readonly icon: () => SVGElement; + readonly accessibleLabel: string; + tabAdornment?(): Node | null; + readonly host: HTMLElement; + readonly mounted: MountedSidePanel; +} + +export interface SidePanelRegistry { + /** All entries, in manifest order. */ + readonly entries: readonly SidePanelEntry[]; + entry(id: SidePanelId): SidePanelEntry; + /** + * Expose exactly one panel WITHIN ITS OWN PANE, hiding its pane siblings — + * never a global "exactly one of N", which would blank the other pane. + * EVERY pane sibling's `deactivate` runs BEFORE the target's `activate`/ + * `render` — a strict ordering, not an artifact of manifest/registration + * order (review finding 1: a single pass over `entries` let an outgoing + * panel's teardown, e.g. clearing a shared filter, run AFTER the incoming + * panel had already rendered against the stale value, whenever the target + * happened to be visited first). A no-op call (the panel is already + * active) still re-renders it, so an explicit re-activation always + * reflects current state. + */ + showPanel(id: SidePanelId): void; + /** The currently active panel id within `pane`. */ + activeId(pane: SidePanelPane): SidePanelId; + /** Repaint the active LOWER-pane panel's body — the compatibility seam + * `renderSavedHistory(app)` (10 call sites, counted with `rg`: 5 in + * `saved-history.ts`, 4 in `app.ts`, 1 in `file-menu.ts` — excluding the + * function's own definition and import lines) delegates to this. */ + refreshActiveSidePanels(): void; + /** Dispatch `onRunComplete` to the active LOWER-pane panel ONLY, and only if + * it defines the hook (#587 AC3: a clean run always calls this — today only + * History repaints). */ + notifyRunComplete(): void; + /** Tear every panel down once, at shell disposal. */ + dispose(): void; +} diff --git a/src/ui/sidebar-upper.ts b/src/ui/sidebar-upper.ts index 9d7a7f2e..6127552e 100644 --- a/src/ui/sidebar-upper.ts +++ b/src/ui/sidebar-upper.ts @@ -1,17 +1,24 @@ -// The UPPER sidebar pane's role switcher (#426): `Databases | Dashboards` over two -// PERSISTENT hosts, exactly one exposed. +// The UPPER sidebar pane's two panels (#426, registry-driven since #587): +// Databases and Dashboards, over two PERSISTENT hosts, exactly one exposed at +// a time by `side-panel-registry.ts`'s generic `showPanel`. // -// The two hosts are built once and never rebuilt — switching roles only flips -// `hidden`. That is what preserves, by construction rather than by restoration -// logic: the schema search text and its input focus, schema expansion and -// lazily-loaded columns, schema scroll position, and the Dashboard tree's own -// search/expansion/scroll. It also means the upper pane's height, the splitter and -// the sidebar width are untouched — they belong to the `.side-pane` this mounts -// inside, which nothing here replaces. +// The two hosts are built once and never rebuilt — switching panels only +// flips `hidden` (the registry's job now, not this module's). That is what +// preserves, by construction rather than by restoration logic: the schema +// search text and its input focus, schema expansion and lazily-loaded +// columns, schema scroll position, and the Dashboard tree's own +// search/expansion/scroll. It also means the upper pane's height, the +// splitter and the sidebar width are untouched — they belong to the +// `.side-pane` this mounts inside, which nothing here replaces. // -// The tab row reuses the lower switcher's `.side-tabs`/`.side-tab`/`.side-count` -// vocabulary verbatim, as #426 asks and DESIGN.md requires (one tab/segmented -// control language across the app). +// #587: this module used to also own the upper tab row's vocabulary and +// paint it directly (`renderUpperRoleTabs`, `NAV`-style `UpperRole` literals). +// Both are gone — `databasesPanelDef`/`dashboardsPanelDef` below hand the +// SAME label/icon/accessibleLabel/count facts to `side-panel-registry.ts`'s +// generic tab row instead, so the upper and lower rows can never again +// disagree about how a panel presents itself. This module keeps building the +// two panel BODIES (the schema search+list host, the Dashboard search+tree +// host) — the part no registry should take over. import { h } from './dom.js'; import { Icon } from './icons.js'; @@ -19,12 +26,17 @@ import { renderDashboardTree, cancelDashboardTreeClicks, type DashboardTreeApp } import { readTreeUi, setTreeSearch } from '../core/dashboard-tree-ui-state.js'; import type { AppState } from '../state.js'; import type { AppDom } from './app.types.js'; - -export type UpperRole = 'databases' | 'dashboards'; +// From the type-only seam file, not `./side-panel-registry.js` itself: +// `side-panel-registry.ts`'s `buildProductionSidePanelRegistry` imports THIS +// module's `databasesPanelDef`/`dashboardsPanelDef` at runtime now, so this +// module importing back from `side-panel-registry.ts` (even type-only) would +// point the module-graph edge both ways — see that file's `.types.ts` +// sibling for the full rationale. +import type { MountedSidePanel, SidePanelDef } from './side-panel-registry.types.js'; /** The slice of `app` this module reads. A real `App` satisfies it directly. */ export interface SidebarUpperApp extends DashboardTreeApp { - dom: Pick; + dom: Pick; state: AppState; } @@ -33,15 +45,13 @@ export interface SidebarUpperHandle { databasesHost: HTMLElement; /** The Dashboards host — Dashboard search + hierarchy tree. */ dashboardsHost: HTMLElement; - /** Expose exactly one role. */ - showRole(role: UpperRole): void; } /** - * Build the upper pane's tab row and its two hosts. The caller supplies the - * already-built Databases content (the schema search box and list, which - * `app-shell.ts` still owns and which several other modules reach through - * `app.dom`), so this module adds the switcher WITHOUT taking ownership of, or + * Build the upper pane's two hosts. The caller supplies the already-built + * Databases content (the schema search box and list, which `app-shell.ts` + * still owns and which several other modules reach through `app.dom`), so + * this module adds the Dashboards body WITHOUT taking ownership of, or * changing, any schema behaviour. */ export function buildSidebarUpper( @@ -49,8 +59,6 @@ export function buildSidebarUpper( ): SidebarUpperHandle { const state = app.state; - app.dom.upperRoleTabs = h('div', { class: 'side-tabs upper-role-tabs' }); - const databasesHost = h('div', { class: 'upper-role-host', 'data-role': 'databases' }, ...databasesContent); // Built ONCE and never inside the repainted row list, so typing keeps the caret @@ -73,51 +81,58 @@ export function buildSidebarUpper( role: 'tree', 'aria-label': 'Dashboards', }); - const dashboardsHost = h('div', { class: 'upper-role-host', 'data-role': 'dashboards', hidden: true }, + // `hidden` is NOT set here — the registry normalizes every panel's initial + // visibility from the manifest's pane order at construction (#587). + const dashboardsHost = h('div', { class: 'upper-role-host', 'data-role': 'dashboards' }, h('div', { class: 'schema-search' }, h('div', { class: 'search-wrap' }, Icon.search(), app.dom.dashboardSearchInput)), app.dom.dashboardTreeList); - return { - databasesHost, - dashboardsHost, - showRole: (role) => { - databasesHost.hidden = role !== 'databases'; - dashboardsHost.hidden = role !== 'dashboards'; - }, - }; + return { databasesHost, dashboardsHost }; } -/** Repaint the role tabs: active state plus each role's count. */ -export function renderUpperRoleTabs(app: SidebarUpperApp): void { - const row = app.dom.upperRoleTabs; - if (!row) return; - const state = app.state; - const active = state.upperRole.value; +/** The Databases tab's live count — omitted while the schema is still + * loading or failed (a confident "· 0" during a load would be a lie), + * exactly as `renderUpperRoleTabs` used to compute it. */ +function databasesCount(app: SidebarUpperApp): Node | null { + const schema = app.state.schema.value; + const count = app.state.schemaError.value || schema === null ? null : schema.length; + return count === null ? null : h('span', { class: 'side-count' }, '· ' + count); +} - // Omitted while the schema is still loading or failed — the lower switcher omits - // `.side-count` when there is no count to show, and a confident "· 0" during a - // load would be a lie. - const schema = state.schema.value; - const databaseCount = state.schemaError.value || schema === null ? null : schema.length; - const dashboardCount = app.currentWorkspace?.dashboards?.length ?? 0; +/** The Dashboards tab's live count — always shown, including zero, exactly + * as `renderUpperRoleTabs` used to compute it. */ +function dashboardsCount(app: SidebarUpperApp): Node { + const count = app.currentWorkspace?.dashboards?.length ?? 0; + return h('span', { class: 'side-count' }, '· ' + count); +} - const tab = (role: UpperRole, label: string, icon: SVGElement, count: number | null): HTMLButtonElement => - h('button', { - class: 'side-tab' + (active === role ? ' active' : ''), - type: 'button', - 'aria-pressed': active === role ? 'true' : 'false', - onclick: () => { - // Changing role hides one tree and shows the other, so a deferred - // single-click must not land on the tree the user just left. - cancelDashboardTreeClicks(app); - state.upperRole.value = role; - }, - }, icon, h('span', null, label), - count === null ? null : h('span', { class: 'side-count' }, '· ' + count)); +/** The registry's Databases entry. Content already lives in `host` (this + * module's own `databasesHost`, built above) — nothing to mount. */ +export function databasesPanelDef(app: SidebarUpperApp, host: HTMLElement): SidePanelDef { + return { + id: 'databases', pane: 'upper', label: 'Databases', icon: Icon.database, + accessibleLabel: 'Open Databases navigation', + tabAdornment: () => databasesCount(app), + host, + mount: (): MountedSidePanel => ({ render: () => {}, dispose: () => {} }), + }; +} - row.replaceChildren( - tab('databases', 'Databases', Icon.database(), databaseCount), - tab('dashboards', 'Dashboards', Icon.dashboard(), dashboardCount), - ); +/** The registry's Dashboards entry. `render` repaints the tree (cheap and + * idempotent — safe to call on every activation per #587 R2.6); `deactivate` + * cancels a pending deferred single-click on the tree the user is leaving + * (the same guard the old inline `onclick` handler ran before switching). */ +export function dashboardsPanelDef(app: SidebarUpperApp, host: HTMLElement): SidePanelDef { + return { + id: 'dashboards', pane: 'upper', label: 'Dashboards', icon: Icon.dashboard, + accessibleLabel: 'Open Dashboards navigation', + tabAdornment: () => dashboardsCount(app), + host, + mount: (): MountedSidePanel => ({ + render: () => renderDashboardTree(app), + deactivate: () => cancelDashboardTreeClicks(app), + dispose: () => {}, + }), + }; } diff --git a/src/ui/workbench/workbench-session.ts b/src/ui/workbench/workbench-session.ts index 7c81b99c..9b23af5f 100644 --- a/src/ui/workbench/workbench-session.ts +++ b/src/ui/workbench/workbench-session.ts @@ -66,8 +66,6 @@ export interface WorkbenchStateSlice { forceExplain: boolean; resultRowLimit: number; serverVersion: string | null; - /** Read by runScript's clean-run history branch ('history' ⇒ repaint). */ - sidePanel: Signal; isMobile: Signal; mobileView: Signal<'tables' | 'editor' | 'results'>; /** Read by the Run-button effect (Run ↔ "Run selection" label). */ @@ -89,16 +87,23 @@ export interface WorkbenchStateSlice { export interface WorkbenchHooks { /** Per-chunk (run) + per-statement (runScript) results-pane repaint. */ renderResults(): void; - /** runScript's clean-run history repaint when `sidePanel === 'history'`. */ - renderSavedHistory(): void; + /** + * Called UNCONDITIONALLY after a clean script run records its history + * entry (#587 AC3: this session no longer knows a specific side-panel id + * exists, nor imports `state.sidePanel` at all — the decision of WHICH + * panel, if any, actually repaints belongs entirely to the hook's own + * wiring in `app.ts`, via the side-panel registry). Issue Deliverable 1 + * names this `onRunComplete`; only the History panel defines a response to + * it today. + */ + onRunComplete(): void; cancelSchemaGraph(): void; /** Fire-and-forget schema reload after schema-mutating SQL succeeds. */ loadSchema(): void; /** Records a successful single-statement run in history (and, per the real - * app.ts wrapper this replaces, repaints History when it's the open side - * panel — that repaint is this hook's own responsibility, unlike - * `renderSavedHistory` above which the session calls itself for the - * script-history path). */ + * app.ts wrapper this replaces, notifies the side-panel registry itself — + * that dispatch is this hook's own responsibility, unlike `onRunComplete` + * above, which the session calls itself for the script-history path). */ recordHistory(tab: QueryTab, sql?: string): void; recordBoundParams(bp: readonly BoundParamSnapshot[]): void; /** The #173 pipeline's single-source prepare, always in 'execute' mode (the @@ -747,7 +752,10 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes // run(): no history for an aborted or failed script). if (!aborted && !entries.some((e) => e.status === 'error')) { recordScriptHistory(state, originalInput, scriptResult.elapsedMs!, hooks.saveJSON); - if (state.sidePanel.value === 'history') hooks.renderSavedHistory(); + // #587 AC3: unconditional now — this session no longer string-compares + // a panel id (it doesn't import one at all); the hook's own wiring in + // app.ts decides whether/which panel actually repaints. + hooks.onRunComplete(); } retireWave(operation); } diff --git a/tests/e2e/dashboard-membership.html b/tests/e2e/dashboard-membership.html index 0291f2d2..61bbb4af 100644 --- a/tests/e2e/dashboard-membership.html +++ b/tests/e2e/dashboard-membership.html @@ -33,7 +33,9 @@