From 5f656559613bed675ccbcbe4f5f1c6c4d226ab62 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Wed, 29 Jul 2026 23:35:39 +0200 Subject: [PATCH 1/3] feat(#487): navigation section registry (phase 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the four navigation sections hostable by any container, over one shared store per section. The wide sidebar renders exactly as it does today. `src/ui/nav-sections.ts` maps each `LeftNavigationSection` to its label, icon factory, accessible label, wide pane and — the point — its single PERSISTENT host element. `app-shell.ts` composes both sidebar panes out of `registry.entries` filtered by pane, so the shell no longer names which sections belong where. The hosts are built once and never rebuilt, extending #426's `buildSidebarUpper` contract to the lower pane: phase 3's rail and docked drawer MOVE these elements, and a moved element keeps its input values, expansion and scroll, which is how "wide and focused presentations share and preserve all navigation state" becomes structural rather than save/restore logic. The lower pane's Library and History sections gained separate persistent search/list pairs (`historySearch`/`historyList` join `savedSearch`/`savedList`, which are now the Library's). Before this both rendered through one pair a section switch repainted — workable for two tabs in one pane, but neither section's live DOM could be handed to another container without carrying the other's content along. Behaviour is unchanged: only the active section renders, and a switch still clears the shared search filter. Both wide switchers now take their label and icon from `NAV_SECTION_META`. Leaving the upper one hard-coded would have left the registry a second source of truth for exactly the sections phase 3's rail presents again, so a relabel would drift silently. The `'library' ↔ 'saved'` bridge lives in `core/left-nav-layout.ts` beside the other decoders, because `state.ts` applies it at the load boundary and must not import `src/ui/`; the registry re-exports it as its UI-side owner. Fixes a real bug this exposed: `asb:sidePanel` was read undecoded and every reader compared `=== 'saved'`, so an unrecognized or obsolete stored value fell through to History — neither the documented default nor the value's own meaning. Harmless with one shared element pair; with two hosts, two readers resolving one value differently expose one section's host while painting into the other's, i.e. a blank pane. It is decoded once at load now, like `leftNavMode` two lines below it, and `AppState.sidePanel` narrowed from `Signal` to `Signal`. All four hosts share `.nav-section-host` / `data-section` (was `.upper-role-host` / `data-role`, upper pane only) so phase 3's drawer needs no per-section layout rule. Tests: `## Tests` → Wide state 1-4, one host per section, and the exposure rules. `tests/e2e/dashboard-tree.spec.js` gains a lower-pane geometry assertion — the split added a flex wrapper between `.saved-pane` and its scroller, and happy-dom computes no layout, so that pane's box model had no gate in any suite. Part of #487. Follow-up filed: #572 (`inbox`). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NZfeGpkpJw2zsfPtysGHUN --- CHANGELOG.md | 35 ++++- src/core/left-nav-layout.ts | 48 ++++++- src/state.ts | 27 +++- src/styles.css | 20 ++- src/ui/app-shell.ts | 46 +++++-- src/ui/app.types.ts | 9 ++ src/ui/nav-sections.ts | 194 ++++++++++++++++++++++++++++ src/ui/saved-history.ts | 80 ++++++++---- src/ui/sidebar-upper.ts | 25 +++- tests/e2e/dashboard-membership.html | 10 +- tests/e2e/dashboard-tree.spec.js | 69 ++++++++-- tests/helpers/fake-app.ts | 5 + tests/unit/app-shell.test.ts | 124 ++++++++++++++++++ tests/unit/left-nav-layout.test.ts | 36 ++++++ tests/unit/nav-sections.test.ts | 188 +++++++++++++++++++++++++++ tests/unit/saved-history.test.ts | 61 ++++++--- tests/unit/sidebar-upper.test.ts | 39 +++++- tests/unit/state.test.ts | 6 + 18 files changed, 936 insertions(+), 86 deletions(-) create mode 100644 src/ui/nav-sections.ts create mode 100644 tests/unit/nav-sections.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e0e25e6..91943d6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,28 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] ### Added +- **A navigation section registry behind the left sidebar** (#487, phase 2 of 4). + Each of the four navigation sections — Databases, Dashboards, Library, History — + is now addressable through one registry (`src/ui/nav-sections.ts`) that owns its + label, its icon, its accessible label and its single *persistent* host element, + and both sidebar panes are composed out of those hosts. The lower pane's Library + and History sections gained separate persistent search/list element pairs; before + this they shared one pair that a tab switch repainted, which meant neither + section's live DOM could be handed to another container without taking the + other's content along. #426 established the pattern for the upper pane's two + sections; this generalizes it to all four, so the hosts all carry one + `.nav-section-host` class (it was `.upper-role-host`) and one `data-section` + attribute. The `'library' ↔ 'saved'` vocabulary bridge — `AppState.sidePanel` + still persists `'saved'` for the section #427 relabelled "Library" — now lives in + exactly one place instead of being re-derived per caller, and both wide switchers + take their label and icon from the registry so the two presentations of a section + cannot drift. **No user-visible change**: the same two panes, the same switchers, + the same splitters, and a section switch still clears the search exactly as + before. What it buys is + structural: phase 3's compact rail and docked focused drawer can *move* a + section's live DOM instead of rebuilding it, which is what makes "wide and + focused presentations share and preserve all navigation state" true by + construction rather than by save/restore logic. - **The desktop left navigation's layout core and preferences** (#487, phase 1 of 4). A new pure `src/core/left-nav-layout.ts` owns every layout decision the foldable left navigation needs: the named constants and thresholds, the @@ -38,12 +60,12 @@ auto-generated per-PR notes; this file is the curated, human-readable history. invariant is an unconditional postcondition rather than a precondition the caller has to honour. **No user-visible change yet**: the rail, the docked focused drawer and the resize separator arrive in phase 3. + +### Changed - The sidebar's `'col'` drag axis now clamps through the same `LEFT_PANEL_MIN_PX`/`LEFT_PANEL_MAX_PX` constants as the load path, instead of repeating `180`/`420` as literals (#487). Behaviour is unchanged; it removes the second owner of a range whose whole point is having one. - -### Changed - **`VariableBarApp`'s shared activation port is now caller-neutral** (#478). `state.filterActive`/`params.saveFilterActive` — named after Workbench persistence even though Dashboard's own caller uses them for an unpersisted @@ -59,6 +81,15 @@ auto-generated per-PR notes; this file is the curated, human-readable history. adapter refactor — no user-visible behavior changes. ### Fixed +- **A corrupt `asb:sidePanel` decodes to the Library instead of propagating** + (#487). The lower pane's active section was read from localStorage undecoded, + and every reader compared `=== 'saved'`, so an unrecognized or obsolete value + fell through to the History branch — neither the documented default nor the + value's own meaning. It is now decoded once at load, like the `leftNavMode` and + width preferences beside it. This became load-bearing rather than cosmetic in + phase 2: with the pane's two sections on separate hosts, two readers resolving + one value differently expose one section's host while painting into the other's, + which renders as a blank pane. - **A corrupt `asb:sidebarPx` decodes to the default width instead of `NaN`** (#487). The width decoded through a bare `clamp(parseInt(stored), 180, 420)`, and `clamp` is not NaN-safe (`Math.max(180, NaN)` is `NaN`), so a non-numeric diff --git a/src/core/left-nav-layout.ts b/src/core/left-nav-layout.ts index edf84c1b..436fd993 100644 --- a/src/core/left-nav-layout.ts +++ b/src/core/left-nav-layout.ts @@ -73,11 +73,12 @@ export type LeftNavigationMode = 'wide' | 'rail'; * "Library" and deliberately left the stored value alone, since migrating it * would discard every user's persisted lower-pane choice for no behavioural gain. * - * So the vocabularies genuinely differ, and phase 2's navigation section registry - * owns the mapping in exactly one place — `'library' ↔ 'saved'`, with the other - * three sections identical. That mapping is deliberately NOT written here yet: it - * has no caller until the registry exists, and a second copy of it is precisely - * the duplication phase 2 is meant to prevent. + * So the vocabularies genuinely differ, and the mapping lives in exactly one + * place — `sectionForSidePanelKey` / `sidePanelKeyFor` below, added in phase 2. + * `ui/nav-sections.ts` (the navigation section registry) is its UI-side owner and + * only consumer of the section half; the *decode* half belongs here beside + * `decodeLeftNavigationMode`, because `state.ts` applies it at the load boundary + * and must not import from `src/ui/`. */ export type LeftNavigationSection = 'databases' | 'dashboards' | 'library' | 'history'; @@ -87,6 +88,43 @@ export type LeftNavigationSection = 'databases' | 'dashboards' | 'library' | 'hi export const LEFT_NAV_SECTIONS: readonly LeftNavigationSection[] = ['databases', 'dashboards', 'library', 'history']; +/** The lower sidebar pane's two sections, in the registry's vocabulary. Derived + * from `LeftNavigationSection` so the section names have exactly one source. */ +export type LowerNavigationSection = Extract; + +/** + * What `AppState.sidePanel` actually stores. `'saved'` is the Library section: + * #427 renamed the visible label but deliberately left the persisted value at + * `asb:sidePanel` alone, since migrating it would discard every user's lower-pane + * choice for no behavioural gain. + */ +export type SidePanelKey = 'saved' | 'history'; + +/** Section → stored value. */ +export function sidePanelKeyFor(section: LowerNavigationSection): SidePanelKey { + return section === 'library' ? 'saved' : 'history'; +} + +/** + * Stored value → section, and the DECODER for `asb:sidePanel`: a missing, invalid + * or obsolete stored value resolves to the Library section, which is that + * preference's documented default (`state.ts` reads it with `'saved'` as the + * fallback), rather than propagating an unrecognized string. + * + * That fallback direction matters, and it is a deliberate fix rather than + * preserved behaviour. Before phase 2 the lower pane's two sections shared one + * search/list pair and every reader compared `=== 'saved'`, so an unrecognized + * value fell through to the History branch — i.e. to neither the default nor the + * value's own meaning. With two hosts, two readers disagreeing about the fallback + * exposes one section's host while painting into the other's, which renders as a + * blank pane. `state.ts` now decodes once at load, so the signal only ever holds + * a `SidePanelKey` and the disagreement is unreachable rather than merely + * avoided by discipline. + */ +export function sectionForSidePanelKey(key: unknown): LowerNavigationSection { + return key === 'history' ? 'history' : 'library'; +} + /** * The complete left-navigation layout. `wideWidthPx` is deliberately the SAME * value `AppState.sidebarPx` persists at `asb:sidebarPx` — #487 suggests a new diff --git a/src/state.ts b/src/state.ts index 94d4811b..9702cf91 100644 --- a/src/state.ts +++ b/src/state.ts @@ -38,8 +38,11 @@ import { deriveWorkspaceKey } from './core/workspace-key.js'; import { LEFT_DRAWER_DEFAULT_PX, LEFT_WIDE_DEFAULT_PX, clampDrawerWidthPx, clampWideWidthPx, decodeLeftNavigationMode, decodeStoredPx, + sectionForSidePanelKey, sidePanelKeyFor, +} from './core/left-nav-layout.js'; +import type { + LeftNavigationMode, LeftNavigationSection, SidePanelKey, } from './core/left-nav-layout.js'; -import type { LeftNavigationMode, LeftNavigationSection } from './core/left-nav-layout.js'; // ── Persisted-data types (schema-generated) ───────────────────────────────── @@ -383,9 +386,17 @@ 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 section, as `asb:sidePanel` stores it — + * `'saved'` is the Library (#427 relabelled it without migrating the value). + * + * #487 phase 2 narrowed this from `Signal`: the stored value is now + * DECODED at load (below), so an obsolete or corrupt string can never reach a + * reader. It had to be, once the pane's two sections gained separate hosts — + * two readers disagreeing about what an unrecognized value means exposes one + * section while painting into the other, i.e. a blank pane. + */ + sidePanel: Signal; /** * #426 — the UPPER sidebar pane's role. Deliberately NOT persisted (unlike * `sidePanel`): the issue specifies "default to Databases for a fresh session", @@ -763,7 +774,13 @@ 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')), + // Decoded, not passed through (#487 phase 2) — the same discipline + // `leftNavMode` below has, and for the same reason: an unknown stored string + // is not a third section. Round-tripping through the section vocabulary is + // what makes the fallback the documented default rather than whichever branch + // an `=== 'saved'` comparison happens to take. + sidePanel: signal(sidePanelKeyFor( + sectionForSidePanelKey(read.loadStr(KEYS.sidePanel, 'saved')))), upperRole: signal<'databases' | 'dashboards'>('databases'), dashboardTreeRevision: signal(0), dashboardTreeUi: new Map(), diff --git a/src/styles.css b/src/styles.css index df1ba02a..c22535f6 100644 --- a/src/styles.css +++ b/src/styles.css @@ -1704,18 +1704,24 @@ body.detached-tab .graph-overlay-panel { .schema-empty { padding: 24px 14px; color: var(--fg-faint); font-size: var(--text-body); text-align: center; } -/* ------------ upper sidebar roles (#426) ------------ - Databases | Dashboards over two persistent hosts, exactly one exposed. The tab - row reuses .side-tabs/.side-tab/.side-count from the lower switcher verbatim — - DESIGN.md asks for one tab vocabulary across the app. */ -.upper-role-host { +/* ------------ navigation section hosts (#426, generalized in #487) ------------ + One host per navigation section, exactly one exposed per pane: Databases | + Dashboards above, Library | History below. Each pane's tab row reuses + .side-tabs/.side-tab/.side-count verbatim — DESIGN.md asks for one tab + vocabulary across the app. + + #487 phase 2 gave all four sections this one class (it was .upper-role-host, + for the upper pane's two) so that phase 3's focused drawer can host any of them + with no per-section layout rule. */ +.nav-section-host { flex: 1; min-height: 0; display: flex; flex-direction: column; } /* A hidden host contributes no layout, so the exposed one owns the whole pane — but it keeps its DOM, which is what preserves search text, expansion, lazily - loaded columns and scroll across a role switch. */ -.upper-role-host[hidden] { display: none; } + loaded columns and scroll across a section switch, and (phase 3) across a move + between the wide sidebar and the focused drawer. */ +.nav-section-host[hidden] { display: none; } /* ------------ Dashboard hierarchy tree (#426) ------------ */ .dash-tree-row { position: relative; } diff --git a/src/ui/app-shell.ts b/src/ui/app-shell.ts index 506f7074..2fa9d326 100644 --- a/src/ui/app-shell.ts +++ b/src/ui/app-shell.ts @@ -35,6 +35,8 @@ 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 { buildNavSectionRegistry, sectionForSidePanelKey } from './nav-sections.js'; +import type { NavSectionPane } from './nav-sections.js'; import { renderDashboardTree, cancelDashboardTreeClicks } from './dashboard-tree.js'; import { renderSavedHistory } from './saved-history.js'; import { renderLibraryTitle } from './file-menu.js'; @@ -126,21 +128,32 @@ export function mountAppShell(deps: AppShellDeps): AppShellHandle { oninput: (e: Event) => { state.schemaFilter.value = (e.target as HTMLInputElement).value; }, }); app.dom.schemaList = h('div', { class: 'schema-list' }); - // #426: the upper pane now hosts TWO roles. The Databases content is built here - // exactly as before and handed to the role host, which only ever toggles + // #426: the upper pane hosts TWO sections. The Databases content is built here + // exactly as before and handed to the section host, which only ever toggles // `hidden` — so schema search text/focus, expansion, lazily-loaded columns and - // scroll all survive a trip through the Dashboards role by construction. - const upper = buildSidebarUpper(app, [ + // scroll all survive a trip through the Dashboards section by construction. + // + // #487 phase 2: both panes are now composed out of the SAME registry, which owns + // all four sections' persistent hosts, their labels and their icons. This shell + // no longer builds the lower pane's search/list elements, and it does not name + // which sections belong to which pane — it asks the registry, which is the only + // way the claim stays true when phase 3 adds a third container. Phase 3's rail + // and focused drawer address exactly the same four hosts, which is what makes a + // mode change a MOVE of live DOM rather than a rebuild. + const registry = buildNavSectionRegistry(app, buildSidebarUpper(app, [ h('div', { class: 'schema-search' }, h('div', { class: 'search-wrap' }, Icon.search(), app.dom.schemaSearchInput)), app.dom.schemaList, - ]); + ])); + // `entries` is in rail order, so each pane's hosts come out in the order its + // switcher presents them (Databases | Dashboards above, Library | History below). + const hostsIn = (pane: NavSectionPane): HTMLElement[] => + registry.entries.filter((entry) => entry.pane === pane).map((entry) => entry.host); 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!, ...hostsIn('upper')); 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); + const savedPane = h('div', { class: 'side-pane saved-pane', style: { flex: '1', minHeight: '0' } }, + app.dom.savedTabsRow, ...hostsIn('lower')); const sidebar = h('div', { class: 'sidebar', style: { width: state.sidebarPx + 'px' } }); // Only 'col' (sidebar width) and 'sideRow' (schema/saved split) run through @@ -244,10 +257,19 @@ export function mountAppShell(deps: AppShellDeps): AppShellHandle { 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. + // #426: expose exactly one upper section host, and repaint the Dashboard tree. + // Kept separate from the tab effect so a schema load does not rebuild the tree. + disposers.push(effect(() => { + registry.showSection(state.upperRole.value); + })); + // #487 phase 2: the same rule for the lower pane, which until now had no + // exposure step at all — its two sections shared one search/list pair that the + // repaint below simply overwrote. Subscribed to `sidePanel` ALONE (unlike the + // repaint effect, which also tracks the projection revision): a Dashboard + // mutation changes which rows the Library shows, never which section is + // exposed. disposers.push(effect(() => { - upper.showRole(state.upperRole.value); + registry.showSection(sectionForSidePanelKey(state.sidePanel.value)); })); disposers.push(effect(() => { // The ONE reactive input the tree has: every trigger #426 lists (workspace diff --git a/src/ui/app.types.ts b/src/ui/app.types.ts index 32942d94..096d8fb4 100644 --- a/src/ui/app.types.ts +++ b/src/ui/app.types.ts @@ -116,8 +116,17 @@ export interface AppDom { qtabsInner?: HTMLElement; resultsRegion?: HTMLElement; runElapsedEl?: HTMLElement; + /** The LIBRARY section's list and search box. Since #487 phase 2 the lower + * pane's two sections own separate, persistent element pairs (see + * `historyList`/`historySearch`) — before that both rendered through this one + * pair, which a section switch repainted. */ savedList?: HTMLElement; savedSearch?: HTMLElement; + /** #487 phase 2 — the HISTORY section's own list and search box, so each lower + * section has a persistent host a container can move without dragging the + * other section's content along. */ + historyList?: HTMLElement; + historySearch?: HTMLElement; savedTabsRow?: HTMLElement; schemaList?: HTMLElement; specEditorView?: EditorView; diff --git a/src/ui/nav-sections.ts b/src/ui/nav-sections.ts new file mode 100644 index 00000000..c549547c --- /dev/null +++ b/src/ui/nav-sections.ts @@ -0,0 +1,194 @@ +// The navigation section registry (#487 phase 2) — the one place that maps each +// `LeftNavigationSection` to what a container needs in order to *host* it: a +// label, an icon, an accessible label, and its single PERSISTENT host element. +// +// Why a registry at all: the four sections live in two hard-wired pane switchers +// (`Databases | Dashboards` above, `Library | History` below), each of which knew +// both its sections' labels and its sections' DOM. Phase 3 adds a third container +// — the rail's focused drawer — that must be able to show ANY one of the four with +// no switcher inside it. Three containers over one section vocabulary is exactly +// the duplication #487's "Navigation section registry" boundary exists to prevent +// ("Maps rail sections to existing views and labels without duplicating their +// domain state"). +// +// The hosts are built ONCE and never rebuilt, extending #426's `buildSidebarUpper` +// contract from the upper pane to the lower one. That is what makes #487's "Wide +// and focused presentations share and preserve all navigation state" true by +// construction rather than by restoration logic: a phase-3 mode change MOVES a +// host element between containers, and a moved element keeps its input values, its +// expansion, its lazily-loaded rows and its scroll offset. Nothing here restores +// anything, because nothing here destroys anything. +// +// What this module deliberately does NOT own: any section's rendering, search or +// domain behaviour. `buildSidebarUpper` still builds the Databases/Dashboards +// hosts and owns their exposure (this registry is handed that handle, so the +// dependency runs one way and there is no import cycle); `saved-history.ts` still +// renders the Library and History lists into the elements built below. This module +// owns only the *hosting* contract — which is why it can be the single seam all +// three containers address. + +import { h } from './dom.js'; +import { Icon } from './icons.js'; +import { LEFT_NAV_SECTIONS } from '../core/left-nav-layout.js'; +import type { LeftNavigationSection } from '../core/left-nav-layout.js'; +// Re-exported so a UI caller reads the whole section vocabulary from the registry +// (its owner) without also importing `core/`. The pure decode itself lives beside +// `decodeLeftNavigationMode`, because `state.ts` applies it at the load boundary +// and cannot import `src/ui/`. +export { sectionForSidePanelKey, sidePanelKeyFor } from '../core/left-nav-layout.js'; +export type { LowerNavigationSection, SidePanelKey } from '../core/left-nav-layout.js'; +import type { SidebarUpperHandle } from './sidebar-upper.js'; +import type { AppDom } from './app.types.js'; + +/** The slice of `app` the registry needs — the four lower-pane elements it + * attaches to `app.dom`, which `saved-history.ts` then renders into. A real + * `App` satisfies it directly. */ +export interface NavSectionsApp { + dom: Pick; +} + +/** + * Which wide-sidebar pane presents a section. The rail presents all four + * identically, so this is about the WIDE presentation only — it is how + * `showSection` knows which hosts are a section's siblings (i.e. which ones it + * must hide in order to expose this one). + */ +export type NavSectionPane = 'upper' | 'lower'; + +/** A section's presentation, independent of any DOM — so a switcher tab, a rail + * launcher and a drawer header all name a section identically. */ +export interface NavSectionMeta { + /** The visible label, exactly as the wide switchers already show it — #427 + * renamed the Queries tab to "Library" and that is the user-facing name. */ + readonly label: string; + /** A FACTORY, not an element: one SVG node cannot be in the wide switcher and + * the rail launcher at the same time, so each caller mints its own. */ + readonly icon: () => SVGElement; + /** + * For a control whose visible label is absent or insufficient — phase 3's rail + * launchers are icon-only, so this is what they announce. The strings are + * #487's own "Rail state" table verbatim, which is why they are not simply + * `label`: a launcher has to say what activating it *does*, and a tab that + * already sits in a labelled switcher does not. + */ + readonly accessibleLabel: string; + readonly pane: NavSectionPane; +} + +/** + * The four sections' presentation, in one place. Both wide switchers and (in + * phase 3) the rail read it, so a label or icon can never disagree between the two + * presentations of the same section. + */ +export const NAV_SECTION_META: Readonly> = { + databases: { + label: 'Databases', icon: Icon.database, pane: 'upper', + accessibleLabel: 'Open Databases navigation', + }, + dashboards: { + label: 'Dashboards', icon: Icon.dashboard, pane: 'upper', + accessibleLabel: 'Open Dashboards navigation', + }, + library: { + // "Library", not "Queries" — #427 landed, and #487's table says the label and + // the rail tooltip follow it. + label: 'Library', icon: Icon.layers, pane: 'lower', + accessibleLabel: 'Open Library navigation', + }, + history: { + label: 'History', icon: Icon.history, pane: 'lower', + accessibleLabel: 'Open query History', + }, +}; + +export interface NavSectionEntry extends NavSectionMeta { + readonly section: LeftNavigationSection; + /** The single persistent host. Built once, moved between containers, never + * rebuilt. */ + readonly host: HTMLElement; +} + +export interface NavSectionRegistry { + /** All four, in `LEFT_NAV_SECTIONS` order (rail order, top to bottom). */ + readonly entries: readonly NavSectionEntry[]; + entry(section: LeftNavigationSection): NavSectionEntry; + /** + * Expose exactly one section within its own pane, hiding its pane siblings. + * A hidden host contributes no layout but keeps its DOM — the whole point. + * + * Scoped to the pane because the wide sidebar shows one upper section AND one + * lower section simultaneously; a global "exactly one of four" would blank half + * the sidebar. Phase 3's drawer shows one of four, and gets there by moving the + * host rather than by widening this rule. + */ + showSection(section: LeftNavigationSection): void; +} + +/** A section host: the wrapper a container mounts, and the element `showSection` + * toggles. The same class for all four, so phase 3's drawer needs no per-section + * layout rule. */ +const sectionHost = (section: LeftNavigationSection, hidden: boolean, ...content: Node[]): HTMLElement => + h('div', { class: 'nav-section-host', 'data-section': section, hidden }, ...content); + +/** + * Build the registry. Called once per shell mount, right after the `app.dom` reset + * — every host it owns is a singleton for the life of that shell. + * + * `upper` is #426's already-built upper pane: the registry adopts its two hosts + * and delegates their exposure back to it, rather than reaching into another + * module's DOM. The lower pane has no such owner, so the registry builds its two + * hosts here. + */ +export function buildNavSectionRegistry( + app: NavSectionsApp, upper: SidebarUpperHandle, +): NavSectionRegistry { + // Each lower section gets its OWN search box and list. Before #487 both rendered + // through one shared pair that a section switch repainted — workable for two tabs + // in one pane, but it cannot satisfy "search/expansion/scroll state survives + // section and mode changes" for phase 3's drawer, and it cannot be moved into the + // drawer without taking the other section's content along. Two persistent pairs + // is the same shape the upper pane has had since #426. + app.dom.savedSearch = h('div', { class: 'saved-search' }); + app.dom.savedList = h('div', { class: 'saved-list' }); + app.dom.historySearch = h('div', { class: 'saved-search' }); + app.dom.historyList = h('div', { class: 'saved-list' }); + + // The initially-exposed section per pane matches what each pane's own default + // has always been (Databases above, Library below); the shell's exposure effects + // correct both on their first, registration-time run anyway. + const hosts: Readonly> = { + databases: upper.databasesHost, + dashboards: upper.dashboardsHost, + library: sectionHost('library', false, app.dom.savedSearch, app.dom.savedList), + history: sectionHost('history', true, app.dom.historySearch, app.dom.historyList), + }; + + const entries: readonly NavSectionEntry[] = LEFT_NAV_SECTIONS.map((section) => ({ + section, host: hosts[section], ...NAV_SECTION_META[section], + })); + const bySection = new Map(entries.map((entry) => [entry.section, entry])); + + // Each pane exposes its own sections. The upper pane DELEGATES to #426's + // `showRole`, which has owned that pair's exposure since it was written — this + // registry unifies how the containers *address* a section, it does not take over + // another module's hosts. The ternary keeps `showUpper` total without a cast: + // `pane` is a runtime value TypeScript cannot narrow the section union by, and + // only the upper pane's own two sections ever reach it. + const showUpper = (section: LeftNavigationSection): void => + upper.showRole(section === 'dashboards' ? 'dashboards' : 'databases'); + const showLower = (section: LeftNavigationSection): void => { + hosts.library.hidden = section !== 'library'; + hosts.history.hidden = section !== 'history'; + }; + const showers: Readonly void>> = { + upper: showUpper, lower: showLower, + }; + + return { + entries, + // `!`: `entries` is built from LEFT_NAV_SECTIONS, so the map has every member + // of the `LeftNavigationSection` union as a key. + entry: (section) => bySection.get(section)!, + showSection: (section) => { showers[NAV_SECTION_META[section].pane](section); }, + }; +} diff --git a/src/ui/saved-history.ts b/src/ui/saved-history.ts index 592fd8cf..6c7642a5 100644 --- a/src/ui/saved-history.ts +++ b/src/ui/saved-history.ts @@ -1,7 +1,16 @@ -// 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 +// The bottom sidebar pane: a Library / History switcher and, per section, its own +// search box and list. 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. +// +// #487 phase 2 split the two sections' DOM: each renders into its own persistent +// search/list pair (`ui/nav-sections.ts` builds them and hosts them), where before +// both shared one pair that a section switch repainted. Everything below still +// renders ONLY the active section, exactly as it always did — the switcher's +// clear-the-search semantics are unchanged, and the inactive host simply keeps the +// DOM it last painted until it is shown again. What the split buys is that a +// container can move one section's live elements (phase 3's focused drawer) +// without taking the other section's content along. import { h } from './dom.js'; import { Icon } from './icons.js'; @@ -20,6 +29,8 @@ import { isQuerylessPanel } from '../core/panel-cfg.js'; import { queryDescription, queryFavorite, queryName, queryPanel, queryView } from '../core/saved-query.js'; import { libraryQueries } from '../dashboard/model/query-ownership.js'; import { openLibraryAssignMenu } from './library-assign-menu.js'; +import { NAV_SECTION_META, sectionForSidePanelKey, sidePanelKeyFor } from './nav-sections.js'; +import type { LowerNavigationSection } from './nav-sections.js'; import type { App } from './app.types.js'; import type { SavedQueryV2 } from '../generated/json-schema.types.js'; @@ -101,9 +112,27 @@ function libraryEntries(app: App): SavedQueryV2[] { return app.state.savedQueries.filter((query) => libraryIds.has(query.id)); } +/** + * The active section, in the registry's vocabulary. EVERY branch in this module + * goes through this one function rather than comparing `sidePanel` to `'saved'` + * directly (#487 phase 2). With two hosts, a reader that resolves an unrecognized + * value differently from the shell's exposure effect would expose one section's + * host and paint into the other's — a blank pane. `state.ts` also decodes the + * stored value at load, so the two guards are belt and braces on purpose. + */ +const activeSection = (app: App): LowerNavigationSection => + sectionForSidePanelKey(app.state.sidePanel.value); + +/** The ACTIVE section's own search box and list (#487 phase 2) — the two lower + * sections no longer share one pair. */ +const activeEls = (app: App): { search: HTMLElement | undefined; list: HTMLElement | undefined } => + activeSection(app) === 'library' + ? { search: app.dom.savedSearch, list: app.dom.savedList } + : { search: app.dom.historySearch, list: app.dom.historyList }; + export function renderSavedHistory(app: App): void { const tabsRow = app.dom.savedTabsRow; - const list = app.dom.savedList; + const list = activeEls(app).list; if (!tabsRow || !list) return; const state = app.state; // #427: the count is the LIBRARY count, not every stored query — the owned @@ -114,22 +143,28 @@ export function renderSavedHistory(app: App): void { // (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 => { + // + // #487 phase 2: the tab row speaks the registry's SECTION vocabulary and derives + // the persisted value once, through the one mapping — rather than repeating the + // `'library' means 'saved'` knowledge here. + const switchTo = (section: LowerNavigationSection): void => { + const panel = sidePanelKeyFor(section); state.libraryFilter = ''; app.prefs.save('sidePanel', panel); state.sidePanel.value = panel; }; + const active = activeSection(app); + const tab = (section: LowerNavigationSection, extra: Node | null): HTMLButtonElement => { + const meta = NAV_SECTION_META[section]; + return h('button', { + class: 'side-tab' + (active === section ? ' active' : ''), + onclick: () => switchTo(section), + }, meta.icon(), h('span', null, meta.label), extra); + }; 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')), + tab('library', count ? h('span', { class: 'side-count' }, '· ' + count) : null), + tab('history', null), ); renderSearch(app); @@ -140,25 +175,26 @@ export function renderSavedHistory(app: App): void { * 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!; + // this after confirming the active section's list is mounted. + const list = activeEls(app).list!; list.replaceChildren(); - if (app.state.sidePanel.value === 'saved') renderSaved(app, list); + if (activeSection(app) === 'library') renderSaved(app, list); else renderHistory(app, list); } /** - * Render the search box into `app.dom.savedSearch` (built once per full render; - * a tab with no items shows nothing). Its `input` handler mutates + * Render the search box into the ACTIVE section's own search host (built once per + * full render; a section 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; + const box = activeEls(app).search; 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' + const isLibrary = activeSection(app) === 'library'; + const hasItems = isLibrary ? libraryEntries(app).length > 0 : state.history.length > 0; box.replaceChildren(); @@ -166,7 +202,7 @@ function renderSearch(app: App): void { const input = h('input', { class: 'sv-search-input', type: 'text', - placeholder: state.sidePanel.value === 'saved' ? 'Search library queries…' : 'Search history…', + placeholder: isLibrary ? 'Search library queries…' : 'Search history…', value: state.libraryFilter, }); const clear = h('button', { class: 'sv-search-clear', title: 'Clear' }, Icon.close()); diff --git a/src/ui/sidebar-upper.ts b/src/ui/sidebar-upper.ts index 9d7a7f2e..bacc08d5 100644 --- a/src/ui/sidebar-upper.ts +++ b/src/ui/sidebar-upper.ts @@ -12,11 +12,19 @@ // 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). +// +// #487 phase 2 generalized this pattern to all four navigation sections: both +// hosts below now carry the shared `.nav-section-host` class and a `data-section` +// attribute, so the lower pane's Library/History hosts and phase 3's focused +// drawer need no per-pane layout rule. `ui/nav-sections.ts` composes this builder +// as the registry's upper half and delegates upper-pane exposure to `showRole` +// below — this module still owns its own two hosts. import { h } from './dom.js'; import { Icon } from './icons.js'; import { renderDashboardTree, cancelDashboardTreeClicks, type DashboardTreeApp } from './dashboard-tree.js'; import { readTreeUi, setTreeSearch } from '../core/dashboard-tree-ui-state.js'; +import { NAV_SECTION_META } from './nav-sections.js'; import type { AppState } from '../state.js'; import type { AppDom } from './app.types.js'; @@ -51,7 +59,7 @@ export function buildSidebarUpper( app.dom.upperRoleTabs = h('div', { class: 'side-tabs upper-role-tabs' }); - const databasesHost = h('div', { class: 'upper-role-host', 'data-role': 'databases' }, ...databasesContent); + const databasesHost = h('div', { class: 'nav-section-host', 'data-section': 'databases' }, ...databasesContent); // Built ONCE and never inside the repainted row list, so typing keeps the caret // (the same reason `saved-history.ts` builds its search box outside `renderList`). @@ -73,7 +81,7 @@ export function buildSidebarUpper( role: 'tree', 'aria-label': 'Dashboards', }); - const dashboardsHost = h('div', { class: 'upper-role-host', 'data-role': 'dashboards', hidden: true }, + const dashboardsHost = h('div', { class: 'nav-section-host', 'data-section': 'dashboards', hidden: true }, h('div', { class: 'schema-search' }, h('div', { class: 'search-wrap' }, Icon.search(), app.dom.dashboardSearchInput)), app.dom.dashboardTreeList); @@ -102,7 +110,12 @@ export function renderUpperRoleTabs(app: SidebarUpperApp): void { const databaseCount = state.schemaError.value || schema === null ? null : schema.length; const dashboardCount = app.currentWorkspace?.dashboards?.length ?? 0; - const tab = (role: UpperRole, label: string, icon: SVGElement, count: number | null): HTMLButtonElement => + // #487 phase 2: the label and the icon come from the registry, not from here. + // Both wide switchers and phase 3's rail present the same four sections, so a + // second copy of either would let the presentations drift — which is the whole + // reason `NAV_SECTION_META` exists. (No import cycle: `nav-sections.ts` imports + // only this module's *type*, which esbuild erases.) + const tab = (role: UpperRole, count: number | null): HTMLButtonElement => h('button', { class: 'side-tab' + (active === role ? ' active' : ''), type: 'button', @@ -113,11 +126,11 @@ export function renderUpperRoleTabs(app: SidebarUpperApp): void { cancelDashboardTreeClicks(app); state.upperRole.value = role; }, - }, icon, h('span', null, label), + }, NAV_SECTION_META[role].icon(), h('span', null, NAV_SECTION_META[role].label), count === null ? null : h('span', { class: 'side-count' }, '· ' + count)); row.replaceChildren( - tab('databases', 'Databases', Icon.database(), databaseCount), - tab('dashboards', 'Dashboards', Icon.dashboard(), dashboardCount), + tab('databases', databaseCount), + tab('dashboards', dashboardCount), ); } diff --git a/tests/e2e/dashboard-membership.html b/tests/e2e/dashboard-membership.html index 0291f2d2..44b6a2e0 100644 --- a/tests/e2e/dashboard-membership.html +++ b/tests/e2e/dashboard-membership.html @@ -93,13 +93,21 @@ const tabs = document.createElement('div'); const search = document.createElement('div'); const list = document.createElement('div'); + // #487 phase 2: History has its own persistent search/list pair now. This + // harness only ever shows the Library section, so nothing here reads these + // two — they are mounted so the fixture matches the real shell's shape and a + // future spec that switches sections does not silently render into nothing. + const historySearch = document.createElement('div'); + const historyList = document.createElement('div'); const open = document.createElement('button'); open.textContent = 'Open Dashboard'; open.onclick = () => { void renderDashboard(app, dashboardTarget()); }; - root.replaceChildren(tabs, search, list, open, headerSlot, dashboardHost); + root.replaceChildren(tabs, search, list, historySearch, historyList, open, headerSlot, dashboardHost); app.dom.savedTabsRow = tabs; app.dom.savedSearch = search; app.dom.savedList = list; + app.dom.historySearch = historySearch; + app.dom.historyList = historyList; renderSavedHistory(app); } diff --git a/tests/e2e/dashboard-tree.spec.js b/tests/e2e/dashboard-tree.spec.js index a1f3d489..0c76dea1 100644 --- a/tests/e2e/dashboard-tree.spec.js +++ b/tests/e2e/dashboard-tree.spec.js @@ -24,22 +24,22 @@ test.describe('upper sidebar role switcher', () => { // The schema stub loads two databases; the seed has three Dashboards. await expect(roleTab(page, 'Databases')).toContainText('· 2'); await expect(roleTab(page, 'Dashboards')).toContainText('· 3'); - await expect(page.locator('.upper-role-host[data-role="databases"]')).toBeVisible(); - await expect(page.locator('.upper-role-host[data-role="dashboards"]')).toBeHidden(); + await expect(page.locator('.nav-section-host[data-section="databases"]')).toBeVisible(); + await expect(page.locator('.nav-section-host[data-section="dashboards"]')).toBeHidden(); }); test('a hidden role host contributes NO layout, so the visible one owns the pane', async ({ page }) => { await open(page); const geometry = await page.evaluate(() => { const pane = document.querySelector('.schema-pane'); - const databases = document.querySelector('.upper-role-host[data-role="databases"]'); + const databases = document.querySelector('.nav-section-host[data-section="databases"]'); const paneBox = pane.getBoundingClientRect(); const dbBox = databases.getBoundingClientRect(); return { paneHeight: paneBox.height, dbHeight: dbBox.height, tabsHeight: document.querySelector('.upper-role-tabs').getBoundingClientRect().height, - hiddenDisplay: getComputedStyle(document.querySelector('.upper-role-host[data-role="dashboards"]')).display, + hiddenDisplay: getComputedStyle(document.querySelector('.nav-section-host[data-section="dashboards"]')).display, }; }); expect(geometry.hiddenDisplay).toBe('none'); @@ -49,21 +49,70 @@ test.describe('upper sidebar role switcher', () => { expect(Math.abs(geometry.paneHeight - geometry.tabsHeight - geometry.dbHeight)).toBeLessThan(2); }); + // #487 phase 2 wrapped the LOWER pane's two sections in section hosts as well, so + // `.saved-search`/`.saved-list` now sit one level deeper than the pane. happy-dom + // computes no layout, so the unit suite cannot see whether the scroller still + // fills its host — and this pane is the one the phase actually changed. + test('the lower pane\'s exposed section host fills it, and its list still scrolls', async ({ page }) => { + await open(page); + const geometry = await page.evaluate(() => { + const box = (selector) => { + const rect = document.querySelector(selector).getBoundingClientRect(); + return { top: rect.top, height: rect.height, width: rect.width }; + }; + const library = document.querySelector('.nav-section-host[data-section="library"]'); + const list = library.querySelector('.saved-list'); + // Force the scroller past its host so overflow is actually exercised. + for (let i = 0; i < 40; i += 1) { + const row = document.createElement('div'); + row.className = 'saved-row'; + row.textContent = 'filler row ' + i; + list.appendChild(row); + } + const listBox = list.getBoundingClientRect(); + return { + pane: box('.saved-pane'), + tabsHeight: box('.saved-pane .side-tabs').height, + host: { top: listBox.top, height: library.getBoundingClientRect().height }, + hostTop: library.getBoundingClientRect().top, + listHeight: listBox.height, + searchHeight: library.querySelector('.saved-search').getBoundingClientRect().height, + overflowY: getComputedStyle(list).overflowY, + scrolls: list.scrollHeight > list.clientHeight, + horizontalOverflow: list.scrollWidth > list.clientWidth, + hiddenDisplay: getComputedStyle(document.querySelector('.nav-section-host[data-section="history"]')).display, + }; + }); + + expect(geometry.hiddenDisplay).toBe('none'); + // The exposed host starts below the tab row and fills the rest of the pane. + expect(Math.abs(geometry.hostTop - (geometry.pane.top + geometry.tabsHeight))).toBeLessThan(2); + expect(Math.abs(geometry.pane.height - geometry.tabsHeight - geometry.host.height)).toBeLessThan(2); + // Inside the host, the search box keeps its intrinsic height and the list + // takes the remainder — the flex chain the extra wrapper could have broken. + expect(geometry.searchHeight).toBeGreaterThan(0); + expect(Math.abs(geometry.host.height - geometry.searchHeight - geometry.listHeight)).toBeLessThan(2); + // Still a scroller, and still no sideways overflow at the sidebar's width. + expect(geometry.overflowY).toBe('auto'); + expect(geometry.scrolls).toBe(true); + expect(geometry.horizontalOverflow).toBe(false); + }); + test('switching roles preserves the schema search text, scroll and expansion', async ({ page }) => { await open(page); - const schemaSearch = page.locator('.upper-role-host[data-role="databases"] input'); + const schemaSearch = page.locator('.nav-section-host[data-section="databases"] input'); await schemaSearch.fill('events'); // Expand a database so there is lazily-built row state to lose. - await page.locator('.upper-role-host[data-role="databases"] .tree-row').first().click(); - const rowsBefore = await page.locator('.upper-role-host[data-role="databases"] .tree-row').count(); + await page.locator('.nav-section-host[data-section="databases"] .tree-row').first().click(); + const rowsBefore = await page.locator('.nav-section-host[data-section="databases"] .tree-row').count(); await roleTab(page, 'Dashboards').click(); - await expect(page.locator('.upper-role-host[data-role="dashboards"]')).toBeVisible(); + await expect(page.locator('.nav-section-host[data-section="dashboards"]')).toBeVisible(); await roleTab(page, 'Databases').click(); // Preserved BY CONSTRUCTION: the host is never rebuilt, only un-hidden. await expect(schemaSearch).toHaveValue('events'); - expect(await page.locator('.upper-role-host[data-role="databases"] .tree-row').count()).toBe(rowsBefore); + expect(await page.locator('.nav-section-host[data-section="databases"] .tree-row').count()).toBe(rowsBefore); }); test('the sidebar width and the upper/lower splitter survive a role switch', async ({ page }) => { @@ -449,7 +498,7 @@ test.describe('Dashboard hierarchy tree', () => { test('search narrows the tree and clearing it restores the prior state', async ({ page }) => { await open(page); await roleTab(page, 'Dashboards').click(); - const search = page.locator('.upper-role-host[data-role="dashboards"] input'); + const search = page.locator('.nav-section-host[data-section="dashboards"] input'); await search.fill('zone'); // The variable's own NAME matches, so its ancestors are exposed. await expect(page.locator('.dash-tree-row .label')).toHaveText(['Sales revenue', 'Variables', 'zone', 'Panels']); diff --git a/tests/helpers/fake-app.ts b/tests/helpers/fake-app.ts index fae310d8..20edb92c 100644 --- a/tests/helpers/fake-app.ts +++ b/tests/helpers/fake-app.ts @@ -848,8 +848,13 @@ export function makeApp>(override schemaList: document.createElement('div'), resultsRegion: document.createElement('div'), savedTabsRow: document.createElement('div'), + // #487 phase 2: the lower pane's two sections own separate persistent + // search/list pairs, so a fixture needs both — `renderSavedHistory` targets + // whichever pair the active `sidePanel` names. savedSearch: document.createElement('div'), savedList: document.createElement('div'), + historySearch: document.createElement('div'), + historyList: document.createElement('div'), saveBtn: document.createElement('button'), }, actions: { diff --git a/tests/unit/app-shell.test.ts b/tests/unit/app-shell.test.ts index 2d77f7b6..01c2e817 100644 --- a/tests/unit/app-shell.test.ts +++ b/tests/unit/app-shell.test.ts @@ -25,6 +25,130 @@ function mount() { return { app, handle, loadSchema, loadReference }; } +/** Every mounted section host, keyed by its `data-section`. */ +const hosts = (root: ParentNode): Record => Object.fromEntries( + [...root.querySelectorAll('.nav-section-host')].map((h) => [h.dataset.section!, h]), +); + +// #487 phase 2 — `## Tests` → "Wide state" bullets 1-4. The wide sidebar is now +// composed out of the navigation section registry rather than out of hard-wired +// per-pane DOM, and this is the gate on "existing navigation behaviour is +// unchanged": the same two panes, the same switchers, the same splitters, and the +// rail that phase 3 introduces is not here yet. +describe('mountAppShell wide navigation (#487 phase 2)', () => { + it('renders no rail — the sidebar is the only container hosting a section', () => { + const { app, handle } = mount(); + const sidebar = app.root.querySelector('.sidebar')!; + + expect(app.root.querySelectorAll('.sidebar')).toHaveLength(1); + // Stated positively, so it is falsifiable TODAY rather than an assertion about + // class names no code emits yet: every section host lives inside the one + // sidebar, and the `.main-row` holds only the sidebar, its width handle and the + // two work-surface hosts. Phase 3 moving a host into a rail-side drawer — or + // adding a second navigation column — has to fail this. + const hosts = [...app.root.querySelectorAll('.nav-section-host')]; + expect(hosts).toHaveLength(4); + expect(hosts.every((host) => sidebar.contains(host))).toBe(true); + expect([...app.root.querySelector('.main-row')!.children].map((el) => el.className)) + .toEqual(['sidebar', 'col-resize', 'query-host', 'dashboard-host']); + handle.dispose(); + }); + + it('renders the upper and lower panes together, one exposed host each', () => { + const { app, handle } = mount(); + const sidebar = app.root.querySelector('.sidebar')!; + const panes = [...sidebar.querySelectorAll('.side-pane')].map((p) => p.className); + + expect(panes).toEqual(['side-pane schema-pane', 'side-pane saved-pane']); + const host = hosts(sidebar); + // Both panes are exposed at once — this is what makes a "one of four" exposure + // rule wrong for the wide presentation and a per-pane rule right. + expect(host.databases.hidden).toBe(false); + expect(host.dashboards.hidden).toBe(true); + expect(host.library.hidden).toBe(false); + expect(host.history.hidden).toBe(true); + handle.dispose(); + }); + + it('keeps both switchers and both splitters', () => { + const { app, handle } = mount(); + const sidebar = app.root.querySelector('.sidebar')!; + + // Upper role tabs (#426), lower Library/History tabs (#427 labels). + expect(sidebar.querySelectorAll('.upper-role-tabs')).toHaveLength(1); + expect([...sidebar.querySelectorAll('.side-tabs')]).toHaveLength(2); + expect([...app.dom.savedTabsRow!.querySelectorAll('.side-tab')].map((t) => t.textContent)) + .toEqual(['Library', 'History']); + // The horizontal upper/lower splitter and the vertical sidebar-width handle. + expect(sidebar.querySelectorAll('.row-resize.side-split')).toHaveLength(1); + expect(app.root.querySelectorAll('.col-resize')).toHaveLength(1); + handle.dispose(); + }); + + it('mounts EXACTLY ONE host per section, each holding that section\'s own elements', () => { + const { app, handle } = mount(); + const host = hosts(app.root); + + expect(Object.keys(host).sort()).toEqual(['dashboards', 'databases', 'history', 'library']); + expect(app.root.querySelectorAll('.nav-section-host')).toHaveLength(4); + // The section's content is the live DOM other modules render into, reached + // through `app.dom` exactly as before — the registry hosts it, it does not + // copy or re-create it. + expect(host.databases.contains(app.dom.schemaList!)).toBe(true); + expect(host.dashboards.contains(app.dom.dashboardTreeList!)).toBe(true); + expect(host.library.contains(app.dom.savedList!)).toBe(true); + expect(host.history.contains(app.dom.historyList!)).toBe(true); + // Each list belongs to exactly one host: no section renders into another's. + expect(host.history.contains(app.dom.savedList!)).toBe(false); + expect(host.library.contains(app.dom.historyList!)).toBe(false); + handle.dispose(); + }); + + it('switches the exposed lower host on sidePanel without rebuilding either', () => { + const { app, handle } = mount(); + const host = hosts(app.root); + const libraryList = app.dom.savedList!; + const historyList = app.dom.historyList!; + const marker = libraryList.appendChild(document.createElement('span')); + + app.state.sidePanel.value = 'history'; + expect(host.library.hidden).toBe(true); + expect(host.history.hidden).toBe(false); + // A hidden host keeps its DOM: History's repaint went into History's OWN list + // and left the Library's content standing. Before the split both sections + // rendered through one pair, so this content could not have survived. + expect(libraryList.contains(marker)).toBe(true); + expect(historyList.textContent).toContain('No history yet.'); + + app.state.sidePanel.value = 'saved'; + expect(host.library.hidden).toBe(false); + expect(host.history.hidden).toBe(true); + // The same element objects throughout — never rebuilt, only exposed or hidden. + // That identity is what makes phase 3's mode change a MOVE of live DOM. + expect(app.dom.savedList).toBe(libraryList); + expect(app.dom.historyList).toBe(historyList); + // Becoming active DOES repaint the section, exactly as it always has: the + // switcher clears the shared search filter, so the list is rebuilt from + // scratch. #487 phase 3 owns whether a drawer should preserve it instead. + expect(libraryList.contains(marker)).toBe(false); + handle.dispose(); + }); + + it('switches the exposed upper host on upperRole', () => { + const { app, handle } = mount(); + const host = hosts(app.root); + + app.state.upperRole.value = 'dashboards'; + expect(host.databases.hidden).toBe(true); + expect(host.dashboards.hidden).toBe(false); + + app.state.upperRole.value = 'databases'; + expect(host.databases.hidden).toBe(false); + expect(host.dashboards.hidden).toBe(true); + handle.dispose(); + }); +}); + describe('mountAppShell authentication host', () => { it('exposes one stable, hidden, labelled host immediately below the header', () => { const { app, handle, loadSchema, loadReference } = mount(); diff --git a/tests/unit/left-nav-layout.test.ts b/tests/unit/left-nav-layout.test.ts index 028a468f..d393fb24 100644 --- a/tests/unit/left-nav-layout.test.ts +++ b/tests/unit/left-nav-layout.test.ts @@ -25,6 +25,7 @@ import { effectiveLeftNavigationLayout, isLeftNavigationSection, leftNavigationLayoutIsCoherent, leftNavigationSeparatorAria, leftNavigationWidthPx, normalizeLeftNavigationLayout, resolveLeftNavigationDrag, resolveLeftNavigationKey, resolveRailActivation, resolveRailOpen, + sectionForSidePanelKey, sidePanelKeyFor, } from '../../src/core/left-nav-layout.js'; import type { LeftNavigationLayout } from '../../src/core/left-nav-layout.js'; @@ -608,6 +609,41 @@ describe('decodeStoredPx', () => { }); }); +// The `'library' ↔ 'saved'` bridge (#487 phase 2). It lives here, beside the other +// decoders, because `state.ts` applies it at the load boundary and must not import +// from `src/ui/`; `ui/nav-sections.ts` is its UI-side owner and re-exports it. +describe('sidePanelKeyFor / sectionForSidePanelKey', () => { + it('maps the Library section to the value `asb:sidePanel` has always stored', () => { + // #427 renamed the visible label and deliberately left the stored value alone. + expect(sidePanelKeyFor('library')).toBe('saved'); + expect(sidePanelKeyFor('history')).toBe('history'); + }); + + it('round-trips both sections', () => { + expect(sectionForSidePanelKey(sidePanelKeyFor('library'))).toBe('library'); + expect(sectionForSidePanelKey(sidePanelKeyFor('history'))).toBe('history'); + }); + + it('resolves a missing, invalid or obsolete stored value to Library, the default', () => { + expect(sectionForSidePanelKey('saved')).toBe('library'); + expect(sectionForSidePanelKey('history')).toBe('history'); + // The fallback direction is load-bearing, and it is a deliberate FIX rather + // than preserved behaviour: before the lower pane's two sections had separate + // hosts, every reader compared `=== 'saved'`, so an unrecognized value fell + // through to the History branch — neither the documented default nor the + // value's own meaning. With two hosts, two readers disagreeing here exposes + // one section's host while painting into the other's, i.e. a blank pane. + expect(sectionForSidePanelKey('queries')).toBe('library'); + expect(sectionForSidePanelKey('')).toBe('library'); + expect(sectionForSidePanelKey(undefined)).toBe('library'); + expect(sectionForSidePanelKey(null)).toBe('library'); + expect(sectionForSidePanelKey(0)).toBe('library'); + // Not merely "anything truthy is History": only the exact key is. + expect(sectionForSidePanelKey('History')).toBe('library'); + expect(sectionForSidePanelKey(' history ')).toBe('library'); + }); +}); + // Documented, deliberately pinned, and phase 3's to change: the remembered wide // width depends on which pointer samples the browser happened to deliver, because // one field is doing duty as both the live drag width and the restore memory. diff --git a/tests/unit/nav-sections.test.ts b/tests/unit/nav-sections.test.ts new file mode 100644 index 00000000..38dda2b4 --- /dev/null +++ b/tests/unit/nav-sections.test.ts @@ -0,0 +1,188 @@ +import { describe, it, expect, vi } from 'vitest'; +import { + buildNavSectionRegistry, NAV_SECTION_META, sectionForSidePanelKey, sidePanelKeyFor, +} from '../../src/ui/nav-sections.js'; +import { + sectionForSidePanelKey as coreSectionFor, sidePanelKeyFor as coreKeyFor, +} from '../../src/core/left-nav-layout.js'; +import type { NavSectionsApp } from '../../src/ui/nav-sections.js'; +import { LEFT_NAV_SECTIONS } from '../../src/core/left-nav-layout.js'; +import type { SidebarUpperHandle } from '../../src/ui/sidebar-upper.js'; +import { h } from '../../src/ui/dom.js'; + +/** + * A stand-in for #426's upper pane. The registry ADOPTS those two hosts and + * delegates their exposure back to `showRole`, so a fake handle is exactly the + * right seam here — `sidebar-upper.test.ts` covers the real one, and this spec + * proves the delegation rather than re-testing it. + */ +const upperHandle = (): SidebarUpperHandle & { showRole: ReturnType } => { + const databasesHost = h('div', { class: 'nav-section-host', 'data-section': 'databases' }); + const dashboardsHost = h('div', { class: 'nav-section-host', 'data-section': 'dashboards', hidden: true }); + const showRole = vi.fn((role: 'databases' | 'dashboards') => { + databasesHost.hidden = role !== 'databases'; + dashboardsHost.hidden = role !== 'dashboards'; + }); + return { databasesHost, dashboardsHost, showRole }; +}; + +const build = () => { + const app: NavSectionsApp = { dom: {} }; + const upper = upperHandle(); + const registry = buildNavSectionRegistry(app, upper); + return { app, upper, registry }; +}; + +describe('the library ↔ saved vocabulary bridge', () => { + // The bridge's own behaviour is specified in `left-nav-layout.test.ts` — the + // pure decode lives in `core/` so `state.ts` can apply it at the load boundary. + // What matters HERE is that the registry re-exports that one implementation + // instead of carrying a second copy: a UI caller must be unable to reach a + // different answer than the state layer did. + it('re-exports the core implementation, not a second copy', () => { + expect(sidePanelKeyFor).toBe(coreKeyFor); + expect(sectionForSidePanelKey).toBe(coreSectionFor); + }); +}); + +describe('NAV_SECTION_META', () => { + it('describes all four sections with a distinct label and an icon FACTORY', () => { + expect(Object.keys(NAV_SECTION_META).sort()).toEqual([...LEFT_NAV_SECTIONS].sort()); + const labels = LEFT_NAV_SECTIONS.map((section) => NAV_SECTION_META[section].label); + expect(labels).toEqual(['Databases', 'Dashboards', 'Library', 'History']); + }); + + it('carries #487\'s own accessible labels for the icon-only rail launchers', () => { + // Verbatim from the issue's "Rail state" table. Pinned as exact strings + // because a launcher has to announce what activating it DOES — a `toBeTruthy` + // assertion would pass for a copy-paste of the wrong section's name, and + // phase 3 would then either announce the wrong thing or hard-code the right + // thing beside the registry. + expect(LEFT_NAV_SECTIONS.map((s) => NAV_SECTION_META[s].accessibleLabel)).toEqual([ + 'Open Databases navigation', + 'Open Dashboards navigation', + 'Open Library navigation', + 'Open query History', + ]); + // Distinct from the tab label in every case, which is why it is its own field. + for (const section of LEFT_NAV_SECTIONS) { + expect(NAV_SECTION_META[section].accessibleLabel).not.toBe(NAV_SECTION_META[section].label); + } + }); + + it('mints a FRESH icon per call, so two presentations can show one section at once', () => { + // One SVG node cannot be in the wide switcher and the rail launcher + // simultaneously — appending it to the second would remove it from the first. + const first = NAV_SECTION_META.library.icon(); + const second = NAV_SECTION_META.library.icon(); + expect(first).not.toBe(second); + expect(first.tagName).toBe(second.tagName); + }); + + it('places two sections in each wide pane', () => { + const panes = LEFT_NAV_SECTIONS.map((section) => NAV_SECTION_META[section].pane); + expect(panes).toEqual(['upper', 'upper', 'lower', 'lower']); + }); +}); + +describe('buildNavSectionRegistry', () => { + it('exposes one entry per section, in rail order', () => { + const { registry } = build(); + expect(registry.entries.map((entry) => entry.section)).toEqual([...LEFT_NAV_SECTIONS]); + for (const section of LEFT_NAV_SECTIONS) { + const entry = registry.entry(section); + expect(entry.section).toBe(section); + expect(entry.label).toBe(NAV_SECTION_META[section].label); + expect(entry.pane).toBe(NAV_SECTION_META[section].pane); + } + }); + + it('gives each section EXACTLY ONE host, and adopts the upper pane\'s two', () => { + const { upper, registry } = build(); + const hosts = registry.entries.map((entry) => entry.host); + + expect(new Set(hosts).size).toBe(4); + // The upper pane's hosts are #426's own elements, not copies — a copy would + // silently strand every schema/Dashboard behaviour bound to the originals. + expect(registry.entry('databases').host).toBe(upper.databasesHost); + expect(registry.entry('dashboards').host).toBe(upper.dashboardsHost); + for (const host of hosts) { + expect(host.classList.contains('nav-section-host')).toBe(true); + } + expect(registry.entry('library').host.dataset.section).toBe('library'); + expect(registry.entry('history').host.dataset.section).toBe('history'); + }); + + it('builds the lower pane\'s two search/list pairs and hands them to app.dom', () => { + const { app, registry } = build(); + const library = registry.entry('library').host; + const history = registry.entry('history').host; + + expect([...library.children]).toEqual([app.dom.savedSearch, app.dom.savedList]); + expect([...history.children]).toEqual([app.dom.historySearch, app.dom.historyList]); + // Separate elements, not one shared pair — that is the split. + expect(app.dom.savedList).not.toBe(app.dom.historyList); + expect(app.dom.savedSearch).not.toBe(app.dom.historySearch); + expect(app.dom.savedSearch!.className).toBe('saved-search'); + expect(app.dom.historySearch!.className).toBe('saved-search'); + expect(app.dom.savedList!.className).toBe('saved-list'); + expect(app.dom.historyList!.className).toBe('saved-list'); + }); + + it('starts each pane on its historical default section', () => { + const { registry } = build(); + expect(registry.entry('databases').host.hidden).toBe(false); + expect(registry.entry('dashboards').host.hidden).toBe(true); + expect(registry.entry('library').host.hidden).toBe(false); + expect(registry.entry('history').host.hidden).toBe(true); + }); + + it('exposes exactly one section per pane, leaving the OTHER pane alone', () => { + const { registry } = build(); + // `!!`: `hidden` is typed `boolean | 'until-found'` in lib.dom; the registry + // only ever assigns booleans, and the shape of the assertion is what matters. + const hidden = (): boolean[] => registry.entries.map((entry) => !!entry.host.hidden); + + registry.showSection('history'); + // The upper pane is untouched: the wide sidebar shows one upper AND one lower + // section at once, so a global "one of four" would blank half the sidebar. + expect(hidden()).toEqual([false, true, true, false]); + + registry.showSection('dashboards'); + expect(hidden()).toEqual([true, false, true, false]); + + registry.showSection('library'); + expect(hidden()).toEqual([true, false, false, true]); + + registry.showSection('databases'); + expect(hidden()).toEqual([false, true, false, true]); + }); + + it('delegates upper-pane exposure to #426\'s showRole rather than setting hidden itself', () => { + const { upper, registry } = build(); + + registry.showSection('dashboards'); + registry.showSection('databases'); + expect(upper.showRole.mock.calls).toEqual([['dashboards'], ['databases']]); + + // A lower section must not reach the upper pane's owner at all. + registry.showSection('history'); + expect(upper.showRole).toHaveBeenCalledTimes(2); + }); + + it('never rebuilds a host when exposure changes', () => { + const { registry } = build(); + const before = registry.entries.map((entry) => entry.host); + const marker = h('div', { class: 'sv-search-input' }); + registry.entry('history').host.appendChild(marker); + + registry.showSection('history'); + registry.showSection('library'); + registry.showSection('history'); + + expect(registry.entries.map((entry) => entry.host)).toEqual(before); + // The hidden host kept its DOM — which is what makes "wide and focused + // presentations share and preserve all navigation state" structural. + expect(registry.entry('history').host.contains(marker)).toBe(true); + }); +}); diff --git a/tests/unit/saved-history.test.ts b/tests/unit/saved-history.test.ts index c3a3e422..65ddbc0f 100644 --- a/tests/unit/saved-history.test.ts +++ b/tests/unit/saved-history.test.ts @@ -36,6 +36,12 @@ const byTitle = (root: ParentNode, t: string): HTMLElement => const savedList = (app: App): HTMLElement => app.dom.savedList!; const savedTabsRow = (app: App): HTMLElement => app.dom.savedTabsRow!; const savedSearch = (app: App): HTMLElement => app.dom.savedSearch!; +// #487 phase 2: the History section renders into its OWN persistent pair, so every +// History assertion below names those elements explicitly. Reading through an +// "active section" helper instead would pass even if the renderer painted History +// rows into the Library's list — which is the one thing this split has to prevent. +const historyList = (app: App): HTMLElement => app.dom.historyList!; +const historySearch = (app: App): HTMLElement => app.dom.historySearch!; describe('renderSavedHistory', () => { it('no-ops without mounts', () => { @@ -558,7 +564,7 @@ describe('renderSavedHistory', () => { const app = makeApp(); app.state.sidePanel.value = 'history'; renderSavedHistory(app); - expect(savedList(app).textContent).toContain('No history yet.'); + expect(historyList(app).textContent).toContain('No history yet.'); }); it('history: lists rows (with + without row count) and loads on click', () => { @@ -569,7 +575,7 @@ describe('renderSavedHistory', () => { { id: 'h2', sql: 'INSERT …', ts: Date.now(), rows: null, ms: 1 }, ]; renderSavedHistory(app); - const rows = qsa(savedList(app), '.history-row'); + const rows = qsa(historyList(app), '.history-row'); expect(rows).toHaveLength(2); expect(rows[0].textContent).toContain('3 rows'); expect(rows[1].textContent).not.toContain('rows'); @@ -583,7 +589,7 @@ describe('renderSavedHistory', () => { app.state.sidePanel.value = 'history'; app.state.history = [{ id: 'h1', sql: 'DROP TABLE t', ts: Date.now(), rows: null, ms: 1 }]; renderSavedHistory(app); - click(qs(savedList(app), '.history-row')); + click(qs(historyList(app), '.history-row')); expect(app.actions.loadIntoNewTab).toHaveBeenCalledWith('From history', 'DROP TABLE t'); expect(app.actions.run).not.toHaveBeenCalled(); }); @@ -596,10 +602,27 @@ describe('renderSavedHistory', () => { { id: 'h2', sql: 'SELECT 2', ts: Date.now(), rows: 1, ms: 2 }, ]; renderSavedHistory(app); - click(qs(savedList(app), '.history-row .del')); + click(qs(historyList(app), '.history-row .del')); expect(app.state.history.map((e: HistoryEntry) => e.id)).toEqual(['h2']); expect(app.actions.loadIntoNewTab).not.toHaveBeenCalled(); - expect(qsa(savedList(app), '.history-row')).toHaveLength(1); + expect(qsa(historyList(app), '.history-row')).toHaveLength(1); + }); + + it('resolves an out-of-union sidePanel the SAME way the shell exposes it', () => { + // `state.ts` decodes `asb:sidePanel` at load, so this value cannot come from + // storage — but the signal is settable by any module, and the two readers must + // not be able to disagree. `app-shell.ts`'s exposure effect resolves anything + // that is not 'history' to the Library host; this renderer has to paint into + // the LIBRARY pair for the same input, or the pane shows an exposed empty host + // while the content sits inside the hidden one. + const app = makeApp(); + (app.state.sidePanel as { value: string }).value = 'queries'; + setSaved(app, [{ id: 's1', name: 'Q1', sql: 'SELECT 1' }]); + renderSavedHistory(app); + + expect(qsa(savedList(app), '.saved-row')).toHaveLength(1); + expect(historyList(app).children.length).toBe(0); + expect(qsa(savedTabsRow(app), '.side-tab')[0].classList.contains('active')).toBe(true); }); it('switching panels persists the choice', () => { @@ -639,9 +662,16 @@ describe('renderSavedHistory — search/filter', () => { expect(() => renderSavedHistory(app)).not.toThrow(); }); - it('collapses the search box when the active list is empty', () => { - const app = makeApp(); - app.state.sidePanel.value = 'saved'; + it('collapses the search box when the active list becomes empty', () => { + // Populate the box FIRST, then empty the list and re-render. Asserting + // `children.length === 0` on a freshly built fixture element proves nothing — + // `makeApp()` creates `savedSearch` empty, so that assertion held even if + // `renderSearch` never touched this element at all (which, since the two lower + // sections own separate boxes, is now a reachable bug rather than a hypothetical). + const app = savedApp(); + expect(savedSearch(app).querySelector('.sv-search-input')).not.toBeNull(); + + app.state.savedQueries = []; renderSavedHistory(app); expect(savedSearch(app).children.length).toBe(0); // :empty → hidden via CSS expect(savedSearch(app).querySelector('.sv-search-input')).toBeNull(); @@ -653,7 +683,8 @@ describe('renderSavedHistory — search/filter', () => { app.state.sidePanel.value = 'history'; app.state.history = [{ id: 'h1', sql: 'SELECT 1', ts: Date.now(), rows: 1, ms: 1 }]; renderSavedHistory(app); - expect(input(app).placeholder).toBe('Search history…'); + expect(qs(historySearch(app), '.sv-search-input').placeholder) + .toBe('Search history…'); }); it('filters saved by name / description / sql, case-insensitively, reusing the input node', () => { @@ -691,12 +722,12 @@ describe('renderSavedHistory — search/filter', () => { { id: 'h2', sql: 'INSERT INTO t', ts: Date.now(), rows: null, ms: 1 }, ]; renderSavedHistory(app); - const i = qs(savedSearch(app), '.sv-search-input'); + const i = qs(historySearch(app), '.sv-search-input'); i.value = 'insert'; i.dispatchEvent(new Event('input', { bubbles: true })); - expect(qsa(savedList(app), '.history-row')).toHaveLength(1); - expect(savedList(app).textContent).toContain('INSERT INTO t'); + expect(qsa(historyList(app), '.history-row')).toHaveLength(1); + expect(historyList(app).textContent).toContain('INSERT INTO t'); i.value = 'nope'; i.dispatchEvent(new Event('input', { bubbles: true })); - expect(savedList(app).textContent).toContain('No history matches'); + expect(historyList(app).textContent).toContain('No history matches'); }); it('clears the filter when switching tabs', () => { @@ -724,7 +755,7 @@ describe('drag a row into the editor', () => { app.state.sidePanel.value = 'history'; app.state.history = [{ id: 'h1', sql: 'SELECT 2', ts: Date.now(), rows: 1, ms: 1 }]; renderSavedHistory(app); - const row = qs(savedList(app), '.history-row'); + const row = qs(historyList(app), '.history-row'); expect(row.getAttribute('draggable')).toBe('true'); const setData = dragStart(row); expect(setData).toHaveBeenCalledWith(SUBQUERY_MIME, 'SELECT 2'); @@ -777,7 +808,7 @@ describe('drag a Library row onto a Dashboard (#428)', () => { app.state.sidePanel.value = 'history'; app.state.history = [{ id: 'h1', sql: 'SELECT 2', ts: Date.now(), rows: 1, ms: 1 }]; renderSavedHistory(app); - const setData = dragStart(qs(savedList(app), '.history-row')); + const setData = dragStart(qs(historyList(app), '.history-row')); expect(setData).toHaveBeenCalledTimes(1); expect(setData).toHaveBeenCalledWith(SUBQUERY_MIME, 'SELECT 2'); diff --git a/tests/unit/sidebar-upper.test.ts b/tests/unit/sidebar-upper.test.ts index 68565599..717583fa 100644 --- a/tests/unit/sidebar-upper.test.ts +++ b/tests/unit/sidebar-upper.test.ts @@ -3,7 +3,8 @@ import { buildSidebarUpper, renderUpperRoleTabs } from '../../src/ui/sidebar-upp import type { SidebarUpperApp } from '../../src/ui/sidebar-upper.js'; import { renderDashboardTree } from '../../src/ui/dashboard-tree.js'; import { makeApp } from '../helpers/fake-app.js'; -import { h } from '../../src/ui/dom.js'; +import { h, s } from '../../src/ui/dom.js'; +import { NAV_SECTION_META } from '../../src/ui/nav-sections.js'; import { readTreeUi, setTreeSearch } from '../../src/core/dashboard-tree-ui-state.js'; import type { TreeWorkspace } from '../../src/application/dashboard-tree-model.js'; @@ -49,6 +50,29 @@ describe('buildSidebarUpper — role tabs', () => { expect(tabText(app)).toEqual(['Databases· 3', 'Dashboards· 2']); }); + it('takes both labels and both icons FROM the registry, not from a local copy', () => { + // Asserting the rendered text equals 'Databases' cannot distinguish reading + // `NAV_SECTION_META` from hard-coding the same string — and hard-coding it is + // exactly the drift #487 phase 2 exists to prevent, since phase 3's rail + // presents these same two sections. So override the registry and require the + // tab row to follow it. + const { app } = mount(); + const meta = NAV_SECTION_META.databases as { label: string; icon: () => SVGElement }; + const label = meta.label; + const icon = meta.icon; + try { + meta.label = 'Explore'; + meta.icon = () => s('svg', { 'data-registry-icon': 'yes' }); + renderUpperRoleTabs(app); + // No count: `mount()` leaves the schema unloaded, which omits it. + expect(tabText(app)[0]).toBe('Explore'); + expect(tabs(app)[0].querySelector('[data-registry-icon="yes"]')).not.toBeNull(); + } finally { + meta.label = label; + meta.icon = icon; + } + }); + it('omits the Databases count while the schema is loading or failed', () => { const { app } = mount(); // `null` schema is the loading state — a confident "· 0" would be a lie. @@ -109,6 +133,19 @@ describe('buildSidebarUpper — role tabs', () => { }); describe('buildSidebarUpper — persistent hosts', () => { + it('marks both hosts with the shared section-host contract (#487 phase 2)', () => { + const { upper } = mount(); + // One class and one attribute vocabulary for all four navigation sections, so + // phase 3's focused drawer can host any of them with no per-section layout + // rule — and so `ui/nav-sections.ts` can address these two the same way it + // addresses the lower pane's. + for (const host of [upper.databasesHost, upper.dashboardsHost]) { + expect(host.classList.contains('nav-section-host')).toBe(true); + } + expect(upper.databasesHost.dataset.section).toBe('databases'); + expect(upper.dashboardsHost.dataset.section).toBe('dashboards'); + }); + it('exposes exactly one host at a time', () => { const { app, upper } = mount(); expect(upper.databasesHost.hidden).toBe(false); diff --git a/tests/unit/state.test.ts b/tests/unit/state.test.ts index 5d82e091..861443c4 100644 --- a/tests/unit/state.test.ts +++ b/tests/unit/state.test.ts @@ -282,9 +282,15 @@ describe('createState — left navigation preferences (#487)', () => { [KEYS.leftNavMode]: 'collapsed', [KEYS.leftNavDrawerPx]: 'not-a-number', [KEYS.sidebarPx]: 'not-a-number', + // #487 phase 2: `sidePanel` is decoded now too, not passed through. It has + // to be: the lower pane's two sections own separate hosts, so a value that + // one reader resolves to Library and another to History exposes one host + // while painting into the other — a visibly blank pane. + [KEYS.sidePanel]: 'queries', })); expect(s.leftNavMode.value).toBe('wide'); expect(s.leftNavDrawerPx).toBe(240); + expect(s.sidePanel.value).toBe('saved'); // The regression this case exists for: `clamp(parseInt('not-a-number'), 180, // 420)` is NaN (`Math.max(180, NaN)` is NaN), and a NaN width reaches the DOM // as `width: NaNpx`, which the browser drops — silently collapsing the From 6177a51b3985e084652006d441906c875f4c1119 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Thu, 30 Jul 2026 00:00:43 +0200 Subject: [PATCH 2/3] fix(#487): address ChatGPT review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all reproduced against the real code first. **The shell/renderer agreement test never ran the shell.** The out-of-union `sidePanel` case lived in `saved-history.test.ts` and called `renderSavedHistory` directly, so it pinned only the renderer's half of the blank-pane invariant — the shell could have gone back to resolving the value inline and it would still have passed. The case now also exists in `app-shell.test.ts` against the real mounted shell, asserting exposure AND painted content together, which is the only place the two halves can be caught disagreeing. **`WorkbenchStateSlice.sidePanel` widened the narrowed signal back to `Signal`**, so that session stayed type-authorized to write an arbitrary string into a signal this branch had just narrowed to `SidePanelKey` — the claim "only ever holds a SidePanelKey" was not mechanically true across the boundary. It derives from `AppState['sidePanel']` now, like the other slices do. **A retained inactive search input could repaint the OTHER section.** The hidden host keeps its listeners, `state.libraryFilter` is still one shared string, and `renderList` paints the active section — so an event from a stale input rewrote the filter and repainted the visible list with the wrong section's text. Unreachable through the UI (a `display: none` subtree gets no events), but phase 3 moves hosts into containers where a host can be visible while another section is active, so the handlers now enforce ownership rather than relying on CSS. A guard, not a redesign: per-section filter state is what fixes the shared string, and phase 3 owns it. **No test proved the LOWER switcher reads the registry** — the mirror of the gap already fixed for the upper one, and hard-coding `Library`/`History` back in passed everything. Added the same override-the-registry test. The icon-factory test now covers all four sections instead of Library alone. Not done here, recorded as a phase-3 prerequisite in the ship log: the lower renderer still requires `savedTabsRow` to exist and resolves its target from the global `sidePanel` at call time, so a switcher-less drawer needs `renderLowerTabs`/`renderLowerSection` split apart. That is phase 3's own refactor, not something to smuggle into a phase whose gate is "behaviour unchanged". Part of #487. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NZfeGpkpJw2zsfPtysGHUN --- src/ui/saved-history.ts | 26 ++++++++++-- src/ui/workbench/workbench-session.ts | 12 +++++- tests/unit/app-shell.test.ts | 22 ++++++++++ tests/unit/nav-sections.test.ts | 15 ++++--- tests/unit/saved-history.test.ts | 58 +++++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 10 deletions(-) diff --git a/src/ui/saved-history.ts b/src/ui/saved-history.ts index 6c7642a5..496ce974 100644 --- a/src/ui/saved-history.ts +++ b/src/ui/saved-history.ts @@ -193,7 +193,8 @@ function renderSearch(app: App): void { 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 isLibrary = activeSection(app) === 'library'; + const section = activeSection(app); + const isLibrary = section === 'library'; const hasItems = isLibrary ? libraryEntries(app).length > 0 : state.history.length > 0; @@ -207,9 +208,28 @@ function renderSearch(app: App): void { }); 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); }; + // These controls belong to the section that was active when they were built, and + // that host now OUTLIVES the switch away from it (#487 phase 2) — the inactive + // host keeps its DOM, listeners included. `state.libraryFilter` is still one + // shared string and `renderList` still paints the ACTIVE section, so an event + // from a stale input would rewrite the filter and repaint the OTHER section's + // list with this section's search text. Unreachable through the UI today (a + // `display: none` subtree receives no pointer or keyboard events) — but phase 3 + // moves hosts between containers, where a host can be visible while a different + // section is active, so ownership is enforced here rather than left to CSS. + // + // A guard, not a redesign: per-section filter state is what actually fixes the + // shared-string design, and #487 phase 3 owns that (see the ship log). + const ownsTheList = (): boolean => activeSection(app) === section; + const setFilter = (v: string): void => { + if (!ownsTheList()) return; + input.value = v; state.libraryFilter = v; syncClear(); renderList(app); + }; - input.addEventListener('input', () => { state.libraryFilter = input.value; syncClear(); renderList(app); }); + input.addEventListener('input', () => { + if (!ownsTheList()) return; + 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(); diff --git a/src/ui/workbench/workbench-session.ts b/src/ui/workbench/workbench-session.ts index 7c81b99c..b08a178b 100644 --- a/src/ui/workbench/workbench-session.ts +++ b/src/ui/workbench/workbench-session.ts @@ -66,8 +66,16 @@ export interface WorkbenchStateSlice { forceExplain: boolean; resultRowLimit: number; serverVersion: string | null; - /** Read by runScript's clean-run history branch ('history' ⇒ repaint). */ - sidePanel: Signal; + /** + * Read by runScript's clean-run history branch ('history' ⇒ repaint). + * + * Derived from `AppState` rather than restated as `Signal` (#487 + * phase 2): the real signal holds a decoded `'saved' | 'history'`, and a + * structural `Signal` here would leave this session type-authorized to + * write an arbitrary string into it — re-opening exactly the divergence the + * load-boundary decode closes. This slice only ever reads it. + */ + sidePanel: AppState['sidePanel']; isMobile: Signal; mobileView: Signal<'tables' | 'editor' | 'results'>; /** Read by the Run-button effect (Run ↔ "Run selection" label). */ diff --git a/tests/unit/app-shell.test.ts b/tests/unit/app-shell.test.ts index 01c2e817..0da650bb 100644 --- a/tests/unit/app-shell.test.ts +++ b/tests/unit/app-shell.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest'; import { mountAppShell } from '../../src/ui/app-shell.js'; import { startDrag } from '../../src/ui/splitters.js'; import { makeApp } from '../helpers/fake-app.js'; +import { savedQuery } from '../helpers/saved-query.js'; function mount() { const loadSchema = vi.fn(async () => {}); @@ -134,6 +135,27 @@ describe('mountAppShell wide navigation (#487 phase 2)', () => { handle.dispose(); }); + it('exposes and PAINTS the same lower section for an out-of-union sidePanel', () => { + // The blank-pane invariant, tested against the real shell rather than the + // renderer alone. `saved-history.test.ts` has a sibling case, but it calls + // `renderSavedHistory` directly — so it pins only the renderer's half and would + // still pass if this shell went back to resolving the value inline. Exposure and + // content have to be asserted in the SAME mounted shell, because the bug is + // precisely that the two halves can disagree: one host exposed, the other + // painted, nothing visible. + const { app, handle } = mount(); + const host = hosts(app.root); + app.state.savedQueries = [savedQuery({ id: 's1', name: 'Q1', sql: 'SELECT 1' })]; + + (app.state.sidePanel as { value: string }).value = 'queries'; + + expect(host.library.hidden).toBe(false); + expect(host.history.hidden).toBe(true); + expect(app.dom.savedList!.querySelectorAll('.saved-row')).toHaveLength(1); + expect(app.dom.historyList!.children.length).toBe(0); + handle.dispose(); + }); + it('switches the exposed upper host on upperRole', () => { const { app, handle } = mount(); const host = hosts(app.root); diff --git a/tests/unit/nav-sections.test.ts b/tests/unit/nav-sections.test.ts index 38dda2b4..fd9bc99c 100644 --- a/tests/unit/nav-sections.test.ts +++ b/tests/unit/nav-sections.test.ts @@ -70,13 +70,18 @@ describe('NAV_SECTION_META', () => { } }); - it('mints a FRESH icon per call, so two presentations can show one section at once', () => { + it('mints a FRESH icon per call, for EVERY section', () => { // One SVG node cannot be in the wide switcher and the rail launcher // simultaneously — appending it to the second would remove it from the first. - const first = NAV_SECTION_META.library.icon(); - const second = NAV_SECTION_META.library.icon(); - expect(first).not.toBe(second); - expect(first.tagName).toBe(second.tagName); + // Checked for all four, not just one: a single reused node among them is + // exactly the kind of asymmetry a one-section spot check misses. + for (const section of LEFT_NAV_SECTIONS) { + const first = NAV_SECTION_META[section].icon(); + const second = NAV_SECTION_META[section].icon(); + expect(first).not.toBe(second); + expect(first.tagName).toBe(second.tagName); + expect(first.isConnected).toBe(false); + } }); it('places two sections in each wide pane', () => { diff --git a/tests/unit/saved-history.test.ts b/tests/unit/saved-history.test.ts index 65ddbc0f..9be988dd 100644 --- a/tests/unit/saved-history.test.ts +++ b/tests/unit/saved-history.test.ts @@ -3,6 +3,8 @@ import { renderSavedHistory } from '../../src/ui/saved-history.js'; import { LIBRARY_QUERY_MIME, SUBQUERY_MIME } from '../../src/ui/dnd-mime.js'; import { queryDescription, queryFavorite, queryName } from '../../src/core/saved-query.js'; import { makeApp } from '../helpers/fake-app.js'; +import { NAV_SECTION_META } from '../../src/ui/nav-sections.js'; +import { s as svgEl } from '../../src/ui/dom.js'; import { savedQuery } from '../helpers/saved-query.js'; import type { SavedQueryFixture } from '../helpers/saved-query.js'; import { setTabSpecDraft, toggleFavorite, deleteSaved } from '../../src/state.js'; @@ -625,6 +627,62 @@ describe('renderSavedHistory', () => { expect(qsa(savedTabsRow(app), '.side-tab')[0].classList.contains('active')).toBe(true); }); + it('takes the lower tabs\' labels and icons FROM the registry', () => { + // The mirror of `sidebar-upper.test.ts`'s equivalent. Asserting the rendered + // text is 'Library' cannot distinguish reading NAV_SECTION_META from + // hard-coding the same string next to it — so override the registry and + // require the tab row to follow. Phase 3's rail is the third consumer of + // these same labels; a second copy here is how the presentations drift. + const app = makeApp(); + app.state.sidePanel.value = 'saved'; + const meta = NAV_SECTION_META.history as { label: string; icon: () => SVGElement }; + const label = meta.label; + const icon = meta.icon; + try { + meta.label = 'Recent runs'; + meta.icon = () => svgEl('svg', { 'data-registry-icon': 'yes' }); + renderSavedHistory(app); + const tabs = qsa(savedTabsRow(app), '.side-tab'); + expect(tabs[1].textContent).toBe('Recent runs'); + expect(tabs[1].querySelector('[data-registry-icon="yes"]')).not.toBeNull(); + } finally { + meta.label = label; + meta.icon = icon; + } + }); + + it('ignores input from a retained search box whose section is no longer active', () => { + // #487 phase 2: the inactive section's host keeps its DOM, so its search input + // and listeners OUTLIVE the switch away from it. `state.libraryFilter` is still + // one shared string and `renderList` paints the ACTIVE section, so a stale + // event would rewrite the filter and repaint the OTHER section's list with this + // section's text. CSS makes it unreachable today; phase 3 moves hosts into + // containers where a host can be visible while another section is active. + const app = makeApp(); + app.state.sidePanel.value = 'saved'; + setSaved(app, [{ id: 's1', name: 'Carrier delays', sql: 'SELECT 1' }]); + renderSavedHistory(app); + const staleInput = qs(savedSearch(app), '.sv-search-input'); + + app.state.sidePanel.value = 'history'; + app.state.history = [{ id: 'h1', sql: 'SELECT 1', ts: Date.now(), rows: 1, ms: 1 }]; + renderSavedHistory(app); + expect(qsa(historyList(app), '.history-row')).toHaveLength(1); + + staleInput.value = 'zzzz'; + staleInput.dispatchEvent(new Event('input', { bubbles: true })); + + // The shared filter is untouched and History still shows its row — no + // cross-section rewrite, no "No history matches “zzzz”". + expect(app.state.libraryFilter).toBe(''); + expect(qsa(historyList(app), '.history-row')).toHaveLength(1); + expect(historyList(app).textContent).not.toContain('zzzz'); + + // Escape on the stale input is inert for the same reason. + staleInput.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); + expect(qsa(historyList(app), '.history-row')).toHaveLength(1); + }); + it('switching panels persists the choice', () => { const app = makeApp(); app.state.sidePanel.value = 'saved'; From c82295b528224057b2c1d51971841e8760884086 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Thu, 30 Jul 2026 07:20:07 +0200 Subject: [PATCH 3/3] fix(#487): address ChatGPT second-review minors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second pass on the updated head approved phase 2 and left two minors, both real. **The registry's icon MAPPING was unpinned.** The tests proved every entry is a factory that mints a fresh node, and two mutation tests proved both switchers read the table — but a wrong icon in the table propagates consistently to every presentation, so swapping Library's and History's icons passed the whole suite. Pinned by identity (`NAV_SECTION_META.library.icon === Icon.layers`). This is the same lesson as the two earlier unfalsifiable tests, one level up: proving the consumers read the source says nothing about the source being right. **The host CSS comment overstated what a hidden host preserves.** It claimed search text and scroll survive "a section switch", which is true for the upper pane's two roles and deliberately FALSE for the lower pane — the switcher clears the shared `libraryFilter` and activating the destination repaints its search box and list. The comment now separates the three cases (upper role switch, lower section switch, and a move between containers), because a phase-3 maintainer reading the old wording would assume lower search preservation was already solved when it is phase 3's job. Also asserts both lower lists stay mounted across a section switch, not merely that their object identity holds — identity alone would survive a host being detached and replaced by a look-alike. Part of #487. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NZfeGpkpJw2zsfPtysGHUN --- src/styles.css | 15 ++++++++++++--- tests/unit/app-shell.test.ts | 6 +++++- tests/unit/nav-sections.test.ts | 12 ++++++++++++ 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/styles.css b/src/styles.css index c22535f6..e65df5ff 100644 --- a/src/styles.css +++ b/src/styles.css @@ -1718,9 +1718,18 @@ body.detached-tab .graph-overlay-panel { display: flex; flex-direction: column; } /* A hidden host contributes no layout, so the exposed one owns the whole pane — - but it keeps its DOM, which is what preserves search text, expansion, lazily - loaded columns and scroll across a section switch, and (phase 3) across a move - between the wide sidebar and the focused drawer. */ + but it keeps its DOM. What that preserves differs by pane, and the distinction + matters to #487 phase 3: + + - UPPER (Databases | Dashboards): everything, across a role switch. Each role + owns its own search and expansion state, so switching only flips `hidden` and + the schema search text, lazily loaded columns and scroll all survive (#426). + - LOWER (Library | History): NOT across a section switch. The switcher clears + the shared `libraryFilter`, and activating the destination repaints its search + box and list. Per-section lower search state is phase 3's, not solved here. + - BOTH, across a MOVE between containers (phase 3's wide sidebar ↔ focused + drawer): everything, because moving an element repaints nothing. That is the + guarantee the persistent hosts exist for. */ .nav-section-host[hidden] { display: none; } /* ------------ Dashboard hierarchy tree (#426) ------------ */ diff --git a/tests/unit/app-shell.test.ts b/tests/unit/app-shell.test.ts index 0da650bb..7789404c 100644 --- a/tests/unit/app-shell.test.ts +++ b/tests/unit/app-shell.test.ts @@ -125,9 +125,13 @@ describe('mountAppShell wide navigation (#487 phase 2)', () => { expect(host.library.hidden).toBe(false); expect(host.history.hidden).toBe(true); // The same element objects throughout — never rebuilt, only exposed or hidden. - // That identity is what makes phase 3's mode change a MOVE of live DOM. + // That identity is what makes phase 3's mode change a MOVE of live DOM. Both + // stay MOUNTED too: identity alone would also hold for a host that had been + // detached from the shell and replaced by a look-alike. expect(app.dom.savedList).toBe(libraryList); expect(app.dom.historyList).toBe(historyList); + expect(app.root.contains(libraryList)).toBe(true); + expect(app.root.contains(historyList)).toBe(true); // Becoming active DOES repaint the section, exactly as it always has: the // switcher clears the shared search filter, so the list is rebuilt from // scratch. #487 phase 3 owns whether a drawer should preserve it instead. diff --git a/tests/unit/nav-sections.test.ts b/tests/unit/nav-sections.test.ts index fd9bc99c..12b9d2e9 100644 --- a/tests/unit/nav-sections.test.ts +++ b/tests/unit/nav-sections.test.ts @@ -9,6 +9,7 @@ import type { NavSectionsApp } from '../../src/ui/nav-sections.js'; import { LEFT_NAV_SECTIONS } from '../../src/core/left-nav-layout.js'; import type { SidebarUpperHandle } from '../../src/ui/sidebar-upper.js'; import { h } from '../../src/ui/dom.js'; +import { Icon } from '../../src/ui/icons.js'; /** * A stand-in for #426's upper pane. The registry ADOPTS those two hosts and @@ -70,6 +71,17 @@ describe('NAV_SECTION_META', () => { } }); + it('maps each section to the RIGHT icon primitive', () => { + // Identity, not shape. The freshness test below proves each entry is a factory + // and the two consumer-mutation tests prove both switchers read this table — + // but a WRONG icon in the table propagates consistently to every presentation, + // so all of those stay green if Library and History are swapped here. + expect(NAV_SECTION_META.databases.icon).toBe(Icon.database); + expect(NAV_SECTION_META.dashboards.icon).toBe(Icon.dashboard); + expect(NAV_SECTION_META.library.icon).toBe(Icon.layers); + expect(NAV_SECTION_META.history.icon).toBe(Icon.history); + }); + it('mints a FRESH icon per call, for EVERY section', () => { // One SVG node cannot be in the wide switcher and the rail launcher // simultaneously — appending it to the second would remove it from the first.