From 247ecbdccc4f95e64e0b3451c34da2505449790e Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Wed, 29 Jul 2026 22:25:07 +0200 Subject: [PATCH 1/2] feat(#487): left-navigation layout core and preferences (phase 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of 4 for the drag-foldable desktop left navigation. Adds the pure module that owns every layout decision the feature needs, plus the browser preferences behind it. No user-visible change: the rail, the docked focused drawer and the resize separator arrive in phase 3. `src/core/left-nav-layout.ts` holds the named constants and thresholds, the explicit 'wide' | 'rail' mode, the hysteresis reducer, the keyboard separator resolver, rail activation, the separator's ARIA range, and the mobile projection. Hysteresis is the threshold PAIR — folding needs a proposal below 140px, restoring one above 260px — so the mode is sticky between them and no single pointer pixel can oscillate it. The keyboard path routes through the same reducer as the pointer path, so "keyboard operations match pointer transitions" holds by construction rather than by two implementations agreeing. Every function takes the navigation's proposed TOTAL width and derives each mode's own panel width itself. An earlier revision handed the reducer a mode-relative width, and that made a monotone rightward drag snap the navigation 108px BACKWARDS on the frame a drawer converted to the wide sidebar, because the two measurements disagree by exactly the rail's width at the crossing. The regression tests now sweep a whole pointer path and assert the width response is monotone; per-frame assertions could not see it, since each frame's output was individually defensible. Deviations from the issue's *suggested* shape, all deliberate: - the wide width reuses the existing `asb:sidebarPx` rather than adding a parallel `wideWidthPx`. That key already persists exactly this width over exactly this range; two owners of one width is a bug waiting to happen. The new keys are only `asb:leftNavMode` and `asb:leftNavDrawerPx`. - the focused drawer's band is [fold, wide] = [140, 260], not the wide sidebar's [180, 420]. A drawer drag must fold below the fold threshold and convert above the wide one, so the wide range is mostly unreachable for a drawer; MIN/MAX govern the sidebar. - a drag follows the pointer and does NOT restore remembered widths. The issue asks a rail -> wide drag for both "restore the last useful wide width" and "deterministic resize feedback", and those cannot both hold: the next pointermove overwrites whatever the crossing frame installed, so a restored width survives one frame and reads as a flicker. `End` restores the remembered width — it is the discrete counterpart, with no pointer to honour. - the centre-width clamp moves to phase 4, where its caller is. A single total-in/total-out signature is the wrong shape: there is no inverse from a total back to (mode, panel width), so it could return a width no mode can render. Deferring also respects CLAUDE.md hard rule 5. Also drops the second owner of [180, 420]: `splitters.ts`'s 'col' axis, which WRITES `sidebarPx`, kept its own copy of the bounds as literals while the load path moved onto the constants. Behaviour is unchanged (a real drag always carries a finite clientX); the sidebar e2e drag specs pass untouched. `clampWideWidthPx` closes a NaN hole on the way past: `clamp` is not NaN-safe (`Math.max(180, NaN)` is NaN), so a corrupt `asb:sidebarPx` would have decoded to `width: NaNpx`, which the browser drops. Hardening, not a reproducible bug — no code path writes a non-numeric value. The same hole in the four sibling geometry keys is filed as #570 (`inbox`). `'library'` is this module's section name but NOT the value `AppState.sidePanel` stores for it: that is still `'saved'`, because #427 renamed the label and left the persisted value alone. Phase 2's registry owns the one mapping; the type's doc records it so phase 2 does not rediscover it the hard way. npm test 6734 passing, `left-nav-layout.ts` and `splitters.ts` both at 100/100/100/100. Sabotage-checked: reverting the crossing to the remembered width, basing the bare-rail arrow step on `wideWidthPx`, loosening the drawer's fold comparison to `<=`, and adding a field to the workspace projection each fail a specific test. Part of #487 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FPnt3pq98P5Y1ww3sWawp1 --- CHANGELOG.md | 36 ++ src/application/app-preferences.ts | 6 +- src/core/left-nav-layout.ts | 406 +++++++++++++++++++++ src/state.ts | 55 ++- src/styles.css | 3 +- src/ui/splitters.ts | 12 +- tests/unit/app-preferences.test.ts | 4 + tests/unit/left-nav-layout.test.ts | 561 +++++++++++++++++++++++++++++ tests/unit/state.test.ts | 98 +++++ 9 files changed, 1177 insertions(+), 4 deletions(-) create mode 100644 src/core/left-nav-layout.ts create mode 100644 tests/unit/left-nav-layout.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ae7999ca..b60c5788 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,31 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] +### Added +- **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 + explicit `'wide' | 'rail'` mode, the hysteresis reducer that maps a proposed + width onto the next mode, the keyboard separator resolver, rail activation, the + separator's ARIA range, and the mobile projection that makes a phone ignore + rail mode without discarding the preference. Two new browser preferences back + it (`asb:leftNavMode`, `asb:leftNavDrawerPx`); the wide sidebar's width + deliberately stays on the existing `asb:sidebarPx` rather than gaining a second + owner, and the focused section is session state that does not reopen after a + reload. Hysteresis comes from the threshold *pair* — folding needs a proposal + below 140px and restoring one above 260px — so no single pointer pixel can + oscillate the mode, and the keyboard path routes through the same reducer as + the pointer path rather than reimplementing it. Every function takes the + navigation's proposed *total* width, which is what keeps a drag continuous + across a mode change: handing the reducer a mode-relative width instead made a + monotone rightward drag snap the navigation 108px backwards on the frame a + drawer converted to the sidebar. **No user-visible change yet**: the rail, the + docked focused drawer and the resize separator arrive in phase 3. +- 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 @@ -25,6 +50,17 @@ auto-generated per-PR notes; this file is the curated, human-readable history. adapter refactor — no user-visible behavior changes. ### Fixed +- **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 + stored value would reach the DOM as `width: NaNpx` — which the browser drops, + collapsing the sidebar with nothing to explain why, and the bad value would + persist across reloads. It now falls back to the documented 248px default. + Hardening rather than a reproducible user-visible bug: no code path in the app + writes a non-numeric value (a real drag always carries a finite `clientX`), so + reaching it takes a hand-edited or foreign-origin `localStorage` entry. The + same hole in `editorPct`/`sideSplitPct`/`cellDrawerPx`/`docPanePx` is tracked + separately in #570. - **The Dashboard tree no longer reveals two rows' pencil/trash actions at once, and its `· N` count now sits inline after the label** (#568). The hover/focus reveal rule (`.dash-tree-row:focus-within .dash-tree-act`) diff --git a/src/application/app-preferences.ts b/src/application/app-preferences.ts index f79e5b4d..18cb6886 100644 --- a/src/application/app-preferences.ts +++ b/src/application/app-preferences.ts @@ -31,7 +31,11 @@ export type PreferenceKey = | 'sidePanel' | 'resultRowLimit' // #313 — the documentation pane's own persisted resize width, a sibling of // cellDrawerPx (never shared with it — see splitters.ts's 'docPane' axis). - | 'docPanePx'; + | 'docPanePx' + // #487 — the desktop left navigation's semantic mode and its focused drawer's + // width. The WIDE sidebar width stays `sidebarPx` above: `core/left-nav-layout.ts` + // reuses that preference rather than introducing a second owner of one width. + | 'leftNavMode' | 'leftNavDrawerPx'; /** The one state field this service reads/writes (`toggleTheme` only) — a * plain settable property, not a signal (matches `AppState.theme`). */ diff --git a/src/core/left-nav-layout.ts b/src/core/left-nav-layout.ts new file mode 100644 index 00000000..e4cea2b0 --- /dev/null +++ b/src/core/left-nav-layout.ts @@ -0,0 +1,406 @@ +// The desktop left navigation's layout decisions (#487 phase 1) — pure, no DOM, +// no globals. This module owns the *semantic* mode ('wide' two-pane sidebar vs +// compact 'rail'), the widths each presentation may take, and every transition +// between them. It owns none of the rendering: phase 2 extracts the section +// registry, phase 3 builds the rail, the docked focused drawer and the resize +// separator on top of exactly these functions. +// +// Why a mode reducer rather than reading CSS widths back: #487 is explicit that +// "the mode is explicit application-shell state" and must not be re-derived from +// every pointer pixel after every repaint. So the separator's job in phase 3 is +// only to report a proposed width; what that width *means* is decided here, once, +// in one place both the pointer and the keyboard paths go through — which is also +// why the issue's "keyboard separator operations match pointer transitions" test +// is a property of this module rather than something the UI has to keep in sync. +// +// Every function here takes and returns the navigation's proposed *total* width. +// That is deliberate and it is what keeps a drag continuous: a wide sidebar's own +// width IS the total, while an open drawer's width is the total minus the rail +// beside it. Handing the reducer one mode-relative number instead made a +// monotone rightward drag snap the navigation 108px BACKWARDS on the frame it +// converted a drawer to the wide sidebar, because the two measurements disagree +// by exactly the rail's width at the crossing. Totals are the only common +// currency, so the reducer does the per-mode subtraction itself. + +import { clamp } from './format.js'; + +/** The compact rail's fixed visual width. Nothing resizes the rail *to* another + * value — it is the mode, not a width — though it does appear as a lower bound + * elsewhere here (the separator's `aria-valuemin`, and the offset an open + * drawer's own width sits behind). */ +export const LEFT_RAIL_PX = 48; + +/** + * The two mode thresholds, and the pair *is* the hysteresis: folding wide → rail + * requires a proposal below `LEFT_FOLD_THRESHOLD_PX`, restoring rail → wide + * requires one above `LEFT_WIDE_THRESHOLD_PX`. Between the two the mode is + * sticky, so no single pointer pixel can oscillate it — which is what #487 means + * by "must not flicker near a single threshold". + */ +export const LEFT_FOLD_THRESHOLD_PX = 140; +export const LEFT_WIDE_THRESHOLD_PX = 260; + +/** The WIDE two-pane sidebar's resize bounds — the range `asb:sidebarPx` has + * always used, preserved verbatim per #487 ("preserve the existing + * sidebar-width preference range"). These do NOT bound the focused drawer; see + * `clampDrawerWidthPx`. */ +export const LEFT_PANEL_MIN_PX = 180; +export const LEFT_PANEL_MAX_PX = 420; + +/** The wide sidebar's documented default — the value `asb:sidebarPx` has + * defaulted to since it was introduced, and the fallback a corrupt stored value + * falls back to. */ +export const LEFT_WIDE_DEFAULT_PX = 248; + +/** The focused drawer's documented default, inside the drawer's own + * [fold, wide] band below. */ +export const LEFT_DRAWER_DEFAULT_PX = 240; + +/** Keyboard resize steps for the separator: a small step for a bare arrow, a + * large one for Shift+arrow (#487's "Left/Right Arrow resize by a small step; + * Shift+Left/Right resize by a larger step"). */ +export const LEFT_NAV_STEP_PX = 16; +export const LEFT_NAV_LARGE_STEP_PX = 64; + +export type LeftNavigationMode = 'wide' | 'rail'; + +/** + * The four rail sections, named as #487's own section table names them. + * + * **`'library'` is NOT the value `AppState.sidePanel` holds for the same + * section.** That signal still stores `'saved'` (persisted at `asb:sidePanel`, + * compared against in `ui/saved-history.ts`); #427 renamed the visible *label* to + * "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. + */ +export type LeftNavigationSection = 'databases' | 'dashboards' | 'library' | 'history'; + +/** Rail order, top to bottom — the order #487's section table lists, and the + * same order the existing wide switchers present (Databases | Dashboards above, + * Library | History below). */ +export const LEFT_NAV_SECTIONS: readonly LeftNavigationSection[] = + ['databases', 'dashboards', 'library', 'history']; + +/** + * The complete left-navigation layout. `wideWidthPx` is deliberately the SAME + * value `AppState.sidebarPx` persists at `asb:sidebarPx` — #487 suggests a new + * `wideWidthPx` field, but that key already holds exactly this width over + * exactly this range, and two sources of truth for one width is a bug waiting + * to happen. `state.ts` maps the two names at the boundary. + * + * `focusedSection` is session UI state (never persisted, per #487) and is only + * meaningful in 'rail' mode: 'wide' shows both panes, so there is nothing to + * focus. **Every reducer here maintains that invariant** — no sequence of drags, + * keys or rail activations can produce 'wide' with a non-null `focusedSection`, + * and `leftNavigationLayoutIsCoherent` states it as a checkable predicate. + * + * The invariant lives in the reducers, though, NOT in the two signals + * `state.ts` stores it across: those are independently settable, so phase 3 must + * route every write through these functions. In particular a wide-mode + * `openFocusedSection(section)` must drive the existing pane switchers, never + * `state.leftNavSection` — see `resolveRailActivation`. + * + * `readonly` throughout, matching `core/dashboard-tree-ui-state.ts` (the closest + * sibling: also a pure copy-on-write UI-state reducer). It matters here because + * the reducers below return the SAME object when nothing changes, and phase 3 is + * invited to use that identity to skip a repaint — which only holds if a caller + * cannot mutate a layout in place. + */ +export interface LeftNavigationLayout { + readonly mode: LeftNavigationMode; + readonly wideWidthPx: number; + readonly drawerWidthPx: number; + readonly focusedSection: LeftNavigationSection | null; +} + +/** + * True for exactly the four known section names. + * + * No caller yet, and deliberately so: phase 3's `openFocusedSection(section)` is + * a public seam #428's drag-hover path calls from outside this module, and that + * is the boundary an unchecked string could cross. Nothing validates through it + * today — `focusedSection` is session-only state seeded from a literal, so there + * is no persistence path an obsolete section could arrive by. It lands with the + * type it guards rather than being invented later against a half-remembered union. + */ +export function isLeftNavigationSection(value: unknown): value is LeftNavigationSection { + return LEFT_NAV_SECTIONS.some((section) => section === value); +} + +/** The `mode`/`focusedSection` invariant, as a predicate — a focused drawer + * exists only in rail mode. Exported so the reducers' shared postcondition can + * be asserted directly instead of re-derived in each test. */ +export function leftNavigationLayoutIsCoherent(layout: LeftNavigationLayout): boolean { + return layout.mode === 'rail' || layout.focusedSection === null; +} + +/** Decode a persisted mode. Anything that is not exactly `'rail'` — a missing + * key, an obsolete value, a truncated write — decodes to `'wide'`, the + * documented default for a fresh desktop session. */ +export function decodeLeftNavigationMode(value: unknown): LeftNavigationMode { + return value === 'rail' ? 'rail' : 'wide'; +} + +/** + * Clamp a wide-sidebar width into `[LEFT_PANEL_MIN_PX, LEFT_PANEL_MAX_PX]`. + * + * NaN-safe on purpose: `clamp` alone is not. `Math.min(420, NaN)` is NaN and so + * is `Math.max(180, NaN)`, so the bare `clamp(parseInt(stored), 180, 420)` this + * replaces decoded a corrupt `asb:sidebarPx` straight through to NaN — and a NaN + * width reaches the DOM as `width: NaNpx`, which the browser drops, silently + * collapsing the sidebar. #487 requires invalid widths to "clamp safely". + * + * Only NaN takes the default: `±Infinity` has an unambiguous clamp target and + * still returns the bound it is pressed against, exactly as the bare `clamp` did. + * Guarding both would have introduced a discontinuity — `-1` → 180 but + * `-Infinity` → 248 — and quietly changed this key's existing behaviour. + */ +export function clampWideWidthPx(px: number): number { + if (Number.isNaN(px)) return LEFT_WIDE_DEFAULT_PX; + return clamp(px, LEFT_PANEL_MIN_PX, LEFT_PANEL_MAX_PX); +} + +/** + * Clamp a focused-drawer width into `[LEFT_FOLD_THRESHOLD_PX, + * LEFT_WIDE_THRESHOLD_PX]` — the drawer's own band, NOT the wide sidebar's + * `[MIN, MAX]`. + * + * That is a reading of #487's drawer-resize rules rather than of its constant + * list: a drawer drag must fold closed below the fold threshold and convert to + * the wide sidebar above the wide threshold, so everything in between is the + * only width a drawer can hold. Giving the drawer the wide sidebar's 180 floor + * and 420 ceiling would make most of that range unreachable — the drag would + * have converted to wide long before 420. `MIN`/`MAX` govern the wide sidebar. + */ +export function clampDrawerWidthPx(px: number): number { + if (Number.isNaN(px)) return LEFT_DRAWER_DEFAULT_PX; + return clamp(px, LEFT_FOLD_THRESHOLD_PX, LEFT_WIDE_THRESHOLD_PX); +} + +/** + * The width the navigation actually occupies in the shell row — what the centre + * surface is pushed by, and what the separator reports as `aria-valuenow`. + * 'rail' with a drawer open occupies both: #487 requires every rail icon to stay + * visible while a drawer is open, so the drawer sits beside the rail rather than + * replacing it. + */ +export function leftNavigationWidthPx(layout: LeftNavigationLayout): number { + if (layout.mode === 'wide') return layout.wideWidthPx; + return layout.focusedSection === null ? LEFT_RAIL_PX : LEFT_RAIL_PX + layout.drawerWidthPx; +} + +/** + * The mode machine. Given the current layout and the proposed TOTAL navigation + * width, return the next layout. + * + * `totalPx` is the navigation's full width measured from its own left edge — for + * a pointer drag, `clientX` minus whatever the shell offsets the navigation by + * (zero today; phase 3 must subtract a real offset if a left gutter ever appears, + * or every threshold here silently shifts by it). The reducer derives each mode's + * own panel width from that total itself, which is what keeps a drag continuous + * across a mode change. + * + * Returns the SAME object when nothing changes — a bare rail drag below the wide + * threshold has no effect at all, since the rail's width is the mode. + * + * **A drag follows the pointer; it does not restore remembered widths.** #487 + * asks a rail → wide drag to "restore the last useful wide width" *and* to "show + * deterministic resize feedback", and those two cannot both hold: whatever width + * the crossing frame installs, the very next pointermove overwrites with the + * pointer's own position, so a restored width would survive exactly one frame and + * read as a flicker. Direct manipulation wins during a gesture — the panel edge + * stays under the finger — and the remembered width is restored by `End`, which + * has no pointer to follow. See `resolveLeftNavigationKey`. + * + * Width memory: `wideWidthPx` is only committed for proposals inside + * `[MIN, MAX]`, and a fold leaves it untouched, so it survives a trip through + * rail mode for `End` and for the persisted preference. The documented + * consequence is that dragging through the 140–180 dead zone rests the width at + * the 180 floor, so a later `End` restores 180 rather than the pre-drag width — + * deterministic, and the alternative (freezing the width while the pointer keeps + * moving) would show a sidebar that refuses to shrink to its own floor. + */ +export function resolveLeftNavigationDrag( + layout: LeftNavigationLayout, totalPx: number, +): LeftNavigationLayout { + if (layout.mode === 'wide') { + // A wide sidebar IS the whole navigation, so its panel width is the total. + // Past the fold threshold: commit rail. The wide width is frozen at whatever + // the drag last rested at inside the wide range, and the rail opens with no + // focused section — #487 gives it none automatically. + if (totalPx < LEFT_FOLD_THRESHOLD_PX) { + return { ...layout, mode: 'rail', focusedSection: null }; + } + // Ordinary wide resize. Between the fold threshold and the 180 floor the + // sidebar sits AT the floor rather than clipping, so the user has to pull + // decisively past 140 to fold — "do not leave a partially clipped wide + // sidebar". + return { ...layout, wideWidthPx: clampWideWidthPx(totalPx) }; + } + // An open drawer sits BESIDE the rail, so its own width is the total minus the + // rail; a bare rail has no panel, and a rightward drag is a bid for a sidebar + // whose width would be the whole total. + const hasDrawer = layout.focusedSection !== null; + const panelPx = hasDrawer ? totalPx - LEFT_RAIL_PX : totalPx; + // Past the wide threshold: restore the two-pane sidebar AT THE POINTER, not at + // the remembered width (see this function's doc — a restored width would live + // one frame). `totalPx`, not `panelPx`: the sidebar replaces the rail as well + // as the drawer, so the total is what it must fill to stay under the finger. + if (panelPx > LEFT_WIDE_THRESHOLD_PX) { + return { ...layout, mode: 'wide', wideWidthPx: clampWideWidthPx(totalPx), focusedSection: null }; + } + // A bare rail below the wide threshold has nothing to resize. + if (!hasDrawer) return layout; + // Fold the focused drawer closed, keeping its width for the next open — the + // rail itself stays visible, per #487. + if (panelPx < LEFT_FOLD_THRESHOLD_PX) return { ...layout, focusedSection: null }; + return { ...layout, drawerWidthPx: clampDrawerWidthPx(panelPx) }; +} + +/** The subset of a keyboard event the separator's key handling reads — so a + * plain `{ key }` fixture satisfies it without a DOM event. The modifiers are + * read to REJECT chords: `Ctrl+Home` must not fold the navigation, and + * `Alt+ArrowLeft` is the browser's Back on some platforms. Shift is the one + * modifier with a meaning here (the large step). */ +export interface LeftNavigationKey { + key: string; + shiftKey?: boolean; + ctrlKey?: boolean; + metaKey?: boolean; + altKey?: boolean; +} + +/** + * The TOTAL width an arrow step starts from — the same currency the reducer + * takes, so for a wide sidebar and for an open drawer this is simply the width + * the navigation already occupies. + * + * A bare rail is the exception: its base is `LEFT_WIDE_THRESHOLD_PX`, not its own + * 48px. That is not a fudge — it is the only base that keeps the keyboard honest. + * A pointer can leave a bare rail because `clientX` is absolute, so dragging to + * x=300 proposes 300; an arrow key only has a *relative* step, so a base of 48 + * would propose 64, land in the sticky band, and change nothing — forever — while + * the separator still advertised `aria-valuemax: 420`, a control that lies about + * being resizable. Basing it at the threshold makes both directions come out + * right through the ordinary reducer, with no special-casing there: a rightward + * step crosses into wide (nothing legal exists between the rail and the 180 + * floor), and a leftward step lands in the sticky band and correctly does + * nothing, because the rail is already as folded as it goes. + * + * Deliberately a CONSTANT and not `wideWidthPx`: any remembered width at or below + * 244 would otherwise leave a bare rail's small ArrowRight stuck in the sticky + * band again, reintroducing exactly the dead end this exists to prevent. + */ +function keyboardBaseTotalPx(layout: LeftNavigationLayout): number { + if (layout.mode === 'rail' && layout.focusedSection === null) return LEFT_WIDE_THRESHOLD_PX; + return leftNavigationWidthPx(layout); +} + +/** + * Resolve a keyboard separator operation, or `null` when the key is not one of + * ours — phase 3 must not swallow keys it does not handle, and must not treat a + * Ctrl/Meta/Alt chord as a resize. + * + * Every arrow step routes through `resolveLeftNavigationDrag`, which is what + * makes #487's "keyboard separator operations match pointer transitions" true by + * construction rather than by two implementations agreeing. + * + * `End` is the one place a remembered width is restored — it is the discrete + * counterpart to a drag, with no pointer position to honour instead. + */ +export function resolveLeftNavigationKey( + layout: LeftNavigationLayout, event: LeftNavigationKey, +): LeftNavigationLayout | null { + if (event.ctrlKey || event.metaKey || event.altKey) return null; + // Home folds to rail and End restores wide, both regardless of the current + // mode — pressed twice they are idempotent, not a toggle. + if (event.key === 'Home') { + return layout.mode === 'rail' && layout.focusedSection === null + ? layout + : { ...layout, mode: 'rail', focusedSection: null }; + } + if (event.key === 'End') { + return layout.mode === 'wide' + ? layout + : { ...layout, mode: 'wide', wideWidthPx: clampWideWidthPx(layout.wideWidthPx), focusedSection: null }; + } + if (event.key !== 'ArrowLeft' && event.key !== 'ArrowRight') return null; + const step = event.shiftKey ? LEFT_NAV_LARGE_STEP_PX : LEFT_NAV_STEP_PX; + const delta = event.key === 'ArrowRight' ? step : -step; + return resolveLeftNavigationDrag(layout, keyboardBaseTotalPx(layout) + delta); +} + +/** + * Resolve a rail launcher activation — a click, or phase 3's + * `openFocusedSection(section)` seam (#428's deterministic entry point). + * Activating the ACTIVE section closes the drawer; activating a different one + * switches content in place without closing first, as #487 requires. + * + * **Returns the layout unchanged in wide mode, and that is a hard limit, not a + * silent fallback.** There is no drawer in wide mode and both panes are already + * showing, so "open this section" there means selecting a pane, which is the + * registry's job and not a width decision. Phase 3 must branch on the mode and + * drive the existing upper/lower switchers — writing `state.leftNavSection` + * directly to force a wide-mode drawer would break the `mode`/`focusedSection` + * invariant, and `leftNavigationWidthPx` would then push the centre surface by a + * width that omits the drawer entirely. + */ +export function resolveRailActivation( + layout: LeftNavigationLayout, section: LeftNavigationSection, +): LeftNavigationLayout { + if (layout.mode !== 'rail') return layout; + return { ...layout, focusedSection: layout.focusedSection === section ? null : section }; +} + +/** + * The layout that actually applies at this viewport. Below the mobile breakpoint + * #487 requires the desktop rail and focused drawer not to render at all, and the + * established mobile segmented/bottom navigation to stand in — so the effective + * mode is always the two-pane presentation mobile already styles, with no focused + * section. + * + * The argument is returned UNTOUCHED for desktop, and the mobile branch is a + * projection, never a write: the persisted `mode` and both widths keep whatever + * the user last chose, which is what "ignore desktop folding preferences for + * effective mobile layout; preserve those preferences for the next desktop + * session" asks for. Phase 3 renders through this rather than reading `mode` + * directly. + */ +export function effectiveLeftNavigationLayout( + layout: LeftNavigationLayout, isMobile: boolean, +): LeftNavigationLayout { + if (!isMobile) return layout; + return layout.mode === 'wide' && layout.focusedSection === null + ? layout + : { ...layout, mode: 'wide', focusedSection: null }; +} + +/** The separator's ARIA range: the rail's width is the floor (the navigation can + * never be narrower than the mode it folds into) and the wide sidebar's ceiling + * is the max, with the live occupied width as `aria-valuenow`. + * + * Both extremes are genuinely reachable — a rail+drawer drag that keeps going + * right converts to wide and on to 420 — but the interior is not continuous: + * 49–179 is no resting width in any mode, because a wide sidebar folds before it + * gets there. That gap is inherent to one control spanning two modes rather than + * a bug in the range, and phase 3's assistive-technology pass is where the + * announcement wording gets judged against it. */ +export interface LeftNavigationSeparatorAria { + readonly valueMin: number; + readonly valueMax: number; + readonly valueNow: number; +} + +export function leftNavigationSeparatorAria(layout: LeftNavigationLayout): LeftNavigationSeparatorAria { + return { + valueMin: LEFT_RAIL_PX, + valueMax: LEFT_PANEL_MAX_PX, + valueNow: leftNavigationWidthPx(layout), + }; +} diff --git a/src/state.ts b/src/state.ts index 150de7f6..dd74f759 100644 --- a/src/state.ts +++ b/src/state.ts @@ -35,6 +35,11 @@ import type { LinkedTabSnapshot } from './workspace/workspace-sync.js'; import { materializeQueryTimeRange } from './core/query-time-range.js'; import type { QueryTimeRangeInferenceDiagnostic } from './core/query-time-range.js'; import { deriveWorkspaceKey } from './core/workspace-key.js'; +import { + LEFT_DRAWER_DEFAULT_PX, LEFT_WIDE_DEFAULT_PX, + clampDrawerWidthPx, clampWideWidthPx, decodeLeftNavigationMode, +} from './core/left-nav-layout.js'; +import type { LeftNavigationMode, LeftNavigationSection } from './core/left-nav-layout.js'; // ── Persisted-data types (schema-generated) ───────────────────────────────── @@ -431,6 +436,31 @@ export interface AppState { workspaceKey: string; libraryFilter: string; shortcutsOpen: Signal; + /** + * #487 — the desktop left navigation's explicit semantic mode: the established + * two-pane sidebar, or the compact icon rail. A signal because phase 3 repaints + * the shell from it; persisted at `asb:leftNavMode` because #487 makes it a + * browser preference. + * + * The WIDE width this mode pairs with is `sidebarPx` above — #487 suggests a + * separate `wideWidthPx`, but `asb:sidebarPx` already persists exactly that + * width over exactly the range the issue specifies, and one width with two + * owners is a bug waiting to happen. `core/left-nav-layout.ts` names it + * `wideWidthPx`; the mapping happens where the two meet. + */ + leftNavMode: Signal; + /** #487 — the focused drawer's persisted width, inside the drawer's own + * [fold, wide] band (never the wide sidebar's [180, 420] — see + * `clampDrawerWidthPx`). A plain number like the other splitter widths: the + * drag writes it, then persists it on mouseup. */ + leftNavDrawerPx: number; + /** + * #487 — which section the focused drawer is showing, or `null` for a bare + * rail. Deliberately NOT persisted: the issue specifies "`focusedSection` is + * session UI state and need not reopen automatically after reload". Session + * only; never part of `StoredWorkspaceV5`. + */ + leftNavSection: Signal; isMobile: Signal; mobileView: Signal<'tables' | 'editor' | 'results'>; mobileTab: Signal<'schema' | 'library'>; @@ -480,6 +510,12 @@ export const KEYS = { sideSplitPct: 'asb:sideSplitPct', cellDrawerPx: 'asb:cellDrawerPx', docPanePx: 'asb:docPanePx', + /** #487 — the desktop left navigation's semantic mode ('wide' | 'rail') and + * the focused drawer's width. The WIDE sidebar's width is `sidebarPx` above, + * not a third key: see `AppState.leftNavMode`'s comment for why that key is + * reused rather than duplicated. */ + leftNavMode: 'asb:leftNavMode', + leftNavDrawerPx: 'asb:leftNavDrawerPx', sidePanel: 'asb:sidePanel', saved: 'asb:saved', history: 'asb:history', @@ -629,7 +665,13 @@ export function createState(read: StateReader = { loadJSON, loadStr }): AppState // One persisted preference, default 500; a non-option stored value snaps // back to the default so the selector always reflects a real choice. resultRowLimit: normalizeRowLimit(parseInt(read.loadStr(KEYS.resultRowLimit, '500'), 10)), - sidebarPx: clamp(parseInt(read.loadStr(KEYS.sidebarPx, '248'), 10), 180, 420), + // #487 — the WIDE left-navigation width, and the one width the fold/restore + // machine remembers. `clampWideWidthPx` enforces the same [180, 420] range + // and 248 default this key has always had, but is NaN-safe where the bare + // `clamp(parseInt(...))` was not: `Math.max(180, NaN)` is NaN, so a corrupt + // stored value used to decode straight through to `width: NaNpx`, which the + // browser drops — collapsing the sidebar with no way to tell why. + sidebarPx: clampWideWidthPx(parseInt(read.loadStr(KEYS.sidebarPx, String(LEFT_WIDE_DEFAULT_PX)), 10)), editorPct: num(KEYS.editorPct, 45, 15, 85), sideSplitPct: num(KEYS.sideSplitPct, 58, 25, 85), // Cell-detail / rows-viewer drawer width (issue #101). The 92vw upper @@ -762,6 +804,17 @@ export function createState(read: StateReader = { loadJSON, loadStr }): AppState // a signal for consistency with the rest of the state (no reactive reader // today — shortcuts.js drives its own mount/unmount). shortcutsOpen: signal(false), + // #487 — the desktop left navigation. `leftNavMode` and `leftNavDrawerPx` + // are browser preferences; both decoders fall back to the documented default + // for a missing, invalid or obsolete stored value rather than propagating it + // (an unknown mode string is not a third mode, and a NaN width is not a + // width). `leftNavSection` is session-only by design — #487 specifies the + // focused drawer "need not reopen automatically after reload", so a fresh + // desktop session shows a bare rail even when rail mode was persisted. + leftNavMode: signal(decodeLeftNavigationMode(read.loadStr(KEYS.leftNavMode, 'wide'))), + leftNavDrawerPx: clampDrawerWidthPx( + parseInt(read.loadStr(KEYS.leftNavDrawerPx, String(LEFT_DRAWER_DEFAULT_PX)), 10)), + leftNavSection: signal(null), // Best-effort mobile mode (#126). `isMobile` mirrors the viewport width // against MOBILE_BREAKPOINT_PX — set once and on `change` by app.js's // injected matchMedia listener. Read by the schema tree (to drop diff --git a/src/styles.css b/src/styles.css index 4685b913..df1ba02a 100644 --- a/src/styles.css +++ b/src/styles.css @@ -1007,7 +1007,8 @@ h1, h2, h3, h4, h5, h6 { .sv-act:hover { color: var(--fg); background: var(--bg-hover); } .side-count { color: var(--fg-faint); font-weight: var(--fw-regular); } /* #552: at a narrow sidebar (bottom quarter of the 180–420px drag range — - `src/ui/splitters.ts`'s `clamp(ev.clientX, 180, 420)`), both tab rows + `LEFT_PANEL_MIN_PX`/`LEFT_PANEL_MAX_PX` in `src/core/left-nav-layout.ts`, + applied to the 'col' drag axis by `splitters.ts`'s `clampWideWidthPx`), both tab rows (`.upper-role-tabs`'s Databases/Dashboard and this row's Library/History) drop to text-only labels: the icon, and `.side-count`'s "· N" (the count AND its separator dot share one text node, so hiding the element hides both). diff --git a/src/ui/splitters.ts b/src/ui/splitters.ts index caf2fdcf..01a8b81d 100644 --- a/src/ui/splitters.ts +++ b/src/ui/splitters.ts @@ -3,6 +3,7 @@ // (window + persistence) for testing. import { clamp } from '../core/format.js'; +import { clampWideWidthPx } from '../core/left-nav-layout.js'; // 'docPane' (#313): the persistent documentation pane's own bounded-resize // axis — identical geometry to 'drawer' (right-edge anchored, same @@ -45,9 +46,18 @@ export function clampDrawerWidth(px: number, viewportWidth: number): number { * container being split (unused for 'col'; `{ width }` — the viewport width — * for 'drawer'). 'drawer' is anchored to the *right* edge, so its width grows * as the cursor moves left: `viewportWidth - clientX`. + * + * 'col' defers to `clampWideWidthPx` (#487) rather than repeating the sidebar's + * [180, 420] bounds. This axis WRITES `sidebarPx`, the same preference + * `createState` reads back through that function, so a local copy of the bounds + * here would be a second owner of one width — exactly what + * `core/left-nav-layout.ts` exists to prevent. Behaviour is unchanged for a real + * drag (`clientX` is always finite); phase 3 replaces this axis outright with the + * left-navigation separator, which routes the same proposal through the mode + * reducer instead of a bare clamp. */ export function dragValue(axis: SplitterAxis, ev: DragPoint, rect?: DragRect): number { - if (axis === 'col') return clamp(ev.clientX, 180, 420); + if (axis === 'col') return clampWideWidthPx(ev.clientX); // `!`: every real caller (startDrag's onMove, via ctx.rectFor(axis)) supplies // `width` for 'drawer'/'docPane' and `top`/`bottom` for 'sideRow'/'row' — // the axis dispatch above is exactly the contract that guarantees the field diff --git a/tests/unit/app-preferences.test.ts b/tests/unit/app-preferences.test.ts index b2d08f90..19d14fa9 100644 --- a/tests/unit/app-preferences.test.ts +++ b/tests/unit/app-preferences.test.ts @@ -30,6 +30,10 @@ describe('save()', () => { ['sideSplitPct', 58, '58'], ['cellDrawerPx', 560, '560'], ['docPanePx', 420, '420'], + // #487 — the left navigation's two browser preferences. The WIDE width is + // `sidebarPx` above, deliberately not a third key. + ['leftNavMode', 'rail', 'rail'], + ['leftNavDrawerPx', 200, '200'], ['sidePanel', 'history', 'history'], ['resultRowLimit', 1000, '1000'], ]; diff --git a/tests/unit/left-nav-layout.test.ts b/tests/unit/left-nav-layout.test.ts new file mode 100644 index 00000000..72510a3e --- /dev/null +++ b/tests/unit/left-nav-layout.test.ts @@ -0,0 +1,561 @@ +// #487 phase 1 — the desktop left navigation's layout decisions. Pure module, so +// every case here is a plain input/output assertion: no DOM, no fixture app. +// +// Two structural choices, both learned from a review that caught a real bug this +// file had already declared green: +// +// 1. The drag tests sweep a POINTER PATH and assert a property of the whole +// sweep (mode-monotone, width-monotone), not just the transition frames. The +// bug that slipped through was a 108px backwards snap on the single frame a +// drawer converted to a wide sidebar — every per-frame assertion passed, +// because each frame's output was individually defensible. Only the sequence +// was wrong. +// 2. The "keyboard matches pointer" property drives the pointer side from the +// module's OWN public `leftNavigationWidthPx`, never from a copy of the +// private base formula. A property test that recomputes the implementation is +// a tautology with respect to that implementation, and it hid a second latent +// bug in the bare-rail base. + +import { describe, it, expect } from 'vitest'; +import { + LEFT_DRAWER_DEFAULT_PX, LEFT_FOLD_THRESHOLD_PX, LEFT_NAV_LARGE_STEP_PX, LEFT_NAV_SECTIONS, + LEFT_NAV_STEP_PX, LEFT_PANEL_MAX_PX, LEFT_PANEL_MIN_PX, LEFT_RAIL_PX, LEFT_WIDE_DEFAULT_PX, + LEFT_WIDE_THRESHOLD_PX, + clampDrawerWidthPx, clampWideWidthPx, decodeLeftNavigationMode, effectiveLeftNavigationLayout, + isLeftNavigationSection, leftNavigationLayoutIsCoherent, leftNavigationSeparatorAria, + leftNavigationWidthPx, resolveLeftNavigationDrag, resolveLeftNavigationKey, resolveRailActivation, +} from '../../src/core/left-nav-layout.js'; +import type { LeftNavigationLayout } from '../../src/core/left-nav-layout.js'; + +/** A wide layout at the documented default width. */ +const wide = (over: Partial = {}): LeftNavigationLayout => ({ + mode: 'wide', + wideWidthPx: LEFT_WIDE_DEFAULT_PX, + drawerWidthPx: LEFT_DRAWER_DEFAULT_PX, + focusedSection: null, + ...over, +}); + +/** A rail layout; pass `focusedSection` to open its drawer. */ +const rail = (over: Partial = {}): LeftNavigationLayout => + wide({ mode: 'rail', ...over }); + +/** Drive a pointer path through the reducer, returning the occupied width after + * each step — the sequence, which is what per-frame assertions cannot see. */ +function sweep(from: LeftNavigationLayout, xs: readonly number[]): { + widths: number[]; modes: LeftNavigationMode[]; final: LeftNavigationLayout; +} { + let layout = from; + const widths: number[] = []; + const modes: LeftNavigationMode[] = []; + for (const x of xs) { + layout = resolveLeftNavigationDrag(layout, x); + widths.push(leftNavigationWidthPx(layout)); + modes.push(layout.mode); + } + return { widths, modes, final: layout }; +} + +type LeftNavigationMode = LeftNavigationLayout['mode']; + +const range = (lo: number, hi: number): number[] => + Array.from({ length: hi - lo + 1 }, (_, i) => lo + i); + +describe('constants (#487)', () => { + it('uses the values the issue specifies, with the fold/wide pair ordered as hysteresis', () => { + expect(LEFT_RAIL_PX).toBe(48); + expect(LEFT_FOLD_THRESHOLD_PX).toBe(140); + expect(LEFT_WIDE_THRESHOLD_PX).toBe(260); + expect(LEFT_PANEL_MIN_PX).toBe(180); + expect(LEFT_PANEL_MAX_PX).toBe(420); + // The gap between the two thresholds IS the hysteresis. If these ever met, + // one pointer pixel could oscillate the mode — the exact failure #487's + // "must not flicker near a single threshold" names. + expect(LEFT_FOLD_THRESHOLD_PX).toBeLessThan(LEFT_WIDE_THRESHOLD_PX); + // The rail must be narrower than the width that folds into it, and the wide + // range must sit above the fold threshold, or the dead zone inverts. + expect(LEFT_RAIL_PX).toBeLessThan(LEFT_FOLD_THRESHOLD_PX); + expect(LEFT_FOLD_THRESHOLD_PX).toBeLessThan(LEFT_PANEL_MIN_PX); + }); + it('keeps both documented defaults inside their own band', () => { + expect(LEFT_WIDE_DEFAULT_PX).toBeGreaterThanOrEqual(LEFT_PANEL_MIN_PX); + expect(LEFT_WIDE_DEFAULT_PX).toBeLessThanOrEqual(LEFT_PANEL_MAX_PX); + expect(LEFT_DRAWER_DEFAULT_PX).toBeGreaterThanOrEqual(LEFT_FOLD_THRESHOLD_PX); + expect(LEFT_DRAWER_DEFAULT_PX).toBeLessThanOrEqual(LEFT_WIDE_THRESHOLD_PX); + expect(LEFT_NAV_STEP_PX).toBeLessThan(LEFT_NAV_LARGE_STEP_PX); + }); + it('lists the four sections in rail order', () => { + expect(LEFT_NAV_SECTIONS).toEqual(['databases', 'dashboards', 'library', 'history']); + }); +}); + +describe('isLeftNavigationSection', () => { + it('accepts exactly the four known sections', () => { + for (const section of LEFT_NAV_SECTIONS) expect(isLeftNavigationSection(section)).toBe(true); + }); + it("rejects 'saved' — the value AppState.sidePanel actually stores for Library", () => { + // NOT a pre-#427 name: `asb:sidePanel` still persists 'saved', and + // `ui/saved-history.ts` still compares against it. #427 renamed the LABEL to + // "Library" and left the stored value alone. So this guard rejecting 'saved' + // is correct AND is exactly why phase 2's registry has to own a + // 'library' <-> 'saved' mapping. + expect(isLeftNavigationSection('saved')).toBe(false); + expect(isLeftNavigationSection('queries')).toBe(false); + }); + it('rejects a near miss and every non-string', () => { + expect(isLeftNavigationSection('Databases')).toBe(false); + expect(isLeftNavigationSection('')).toBe(false); + expect(isLeftNavigationSection(null)).toBe(false); + expect(isLeftNavigationSection(undefined)).toBe(false); + expect(isLeftNavigationSection(0)).toBe(false); + expect(isLeftNavigationSection(['databases'])).toBe(false); + }); +}); + +describe('decodeLeftNavigationMode', () => { + it('decodes a stored rail preference', () => { + expect(decodeLeftNavigationMode('rail')).toBe('rail'); + }); + it('falls back to wide for a missing, obsolete or malformed value', () => { + expect(decodeLeftNavigationMode('wide')).toBe('wide'); + expect(decodeLeftNavigationMode(undefined)).toBe('wide'); + expect(decodeLeftNavigationMode(null)).toBe('wide'); + expect(decodeLeftNavigationMode('')).toBe('wide'); + expect(decodeLeftNavigationMode('collapsed')).toBe('wide'); // an obsolete third mode + expect(decodeLeftNavigationMode('RAIL')).toBe('wide'); + expect(decodeLeftNavigationMode(1)).toBe('wide'); + }); +}); + +describe('clampWideWidthPx', () => { + it('passes an in-range width through and clamps both bounds', () => { + expect(clampWideWidthPx(300)).toBe(300); + expect(clampWideWidthPx(LEFT_PANEL_MIN_PX)).toBe(LEFT_PANEL_MIN_PX); + expect(clampWideWidthPx(LEFT_PANEL_MAX_PX)).toBe(LEFT_PANEL_MAX_PX); + expect(clampWideWidthPx(10)).toBe(LEFT_PANEL_MIN_PX); + expect(clampWideWidthPx(9999)).toBe(LEFT_PANEL_MAX_PX); + expect(clampWideWidthPx(-1)).toBe(LEFT_PANEL_MIN_PX); + }); + it('sends only NaN to the default, leaving the infinities on their bounds', () => { + // The regression the guard exists for: `clamp(NaN, 180, 420)` is NaN, so the + // bare clamp this replaced decoded a corrupt `asb:sidebarPx` to `width: NaNpx`. + expect(clampWideWidthPx(NaN)).toBe(LEFT_WIDE_DEFAULT_PX); + // ±Infinity has an unambiguous target, so it keeps the bare clamp's answer — + // guarding it too would make -1 → 180 but -Infinity → 248, a discontinuity + // for no reason, and would change this key's long-standing behaviour. + expect(clampWideWidthPx(Infinity)).toBe(LEFT_PANEL_MAX_PX); + expect(clampWideWidthPx(-Infinity)).toBe(LEFT_PANEL_MIN_PX); + }); +}); + +describe('clampDrawerWidthPx', () => { + it('clamps to the drawer band, not the wide sidebar range', () => { + expect(clampDrawerWidthPx(200)).toBe(200); + expect(clampDrawerWidthPx(LEFT_FOLD_THRESHOLD_PX)).toBe(LEFT_FOLD_THRESHOLD_PX); + expect(clampDrawerWidthPx(LEFT_WIDE_THRESHOLD_PX)).toBe(LEFT_WIDE_THRESHOLD_PX); + expect(clampDrawerWidthPx(0)).toBe(LEFT_FOLD_THRESHOLD_PX); + // Explicitly NOT the wide sidebar's bounds: a 400px drawer is impossible, + // because a drag that far right converts to the wide sidebar instead. + expect(clampDrawerWidthPx(400)).toBe(LEFT_WIDE_THRESHOLD_PX); + expect(clampDrawerWidthPx(LEFT_PANEL_MIN_PX)).toBe(LEFT_PANEL_MIN_PX); + }); + it('sends only NaN to the default', () => { + expect(clampDrawerWidthPx(NaN)).toBe(LEFT_DRAWER_DEFAULT_PX); + expect(clampDrawerWidthPx(Infinity)).toBe(LEFT_WIDE_THRESHOLD_PX); + expect(clampDrawerWidthPx(-Infinity)).toBe(LEFT_FOLD_THRESHOLD_PX); + }); +}); + +describe('leftNavigationWidthPx', () => { + it('is the sidebar width when wide', () => { + expect(leftNavigationWidthPx(wide({ wideWidthPx: 300 }))).toBe(300); + }); + it('is the bare rail width when rail with no drawer', () => { + expect(leftNavigationWidthPx(rail())).toBe(LEFT_RAIL_PX); + }); + it('is rail PLUS drawer when a drawer is open — the rail stays visible beside it', () => { + expect(leftNavigationWidthPx(rail({ focusedSection: 'databases', drawerWidthPx: 200 }))) + .toBe(LEFT_RAIL_PX + 200); + }); +}); + +// The regression suite for the bug this file previously certified as green: a +// monotone pointer path must produce a monotone width response and at most one +// mode change. Each assertion is over the whole sweep. +describe('resolveLeftNavigationDrag — a monotone drag never reverses (#487 regression)', () => { + it('never snaps backwards converting an open drawer to the wide sidebar', () => { + // The original defect, exactly: at clientX 308 the drawer was at its 260 + // maximum (total 308); crossing at 309 installed the REMEMBERED 200 for one + // frame before 310 jumped to 310. A 108px backwards snap mid-gesture. + const { widths } = sweep( + rail({ focusedSection: 'databases', drawerWidthPx: LEFT_WIDE_THRESHOLD_PX, wideWidthPx: 200 }), + range(300, 320)); + for (let i = 1; i < widths.length; i++) expect(widths[i]).toBeGreaterThanOrEqual(widths[i - 1]); + expect(widths.at(-1)).toBe(320); + }); + + it('is width-monotone and mode-monotone dragging right across every threshold', () => { + const { widths, modes } = sweep(rail({ focusedSection: 'library', wideWidthPx: 200 }), range(40, 460)); + for (let i = 1; i < widths.length; i++) expect(widths[i]).toBeGreaterThanOrEqual(widths[i - 1]); + // Exactly one rail → wide transition, and never back. + expect(modes.indexOf('wide')).toBeGreaterThan(0); + expect(modes.slice(modes.indexOf('wide')).every((m) => m === 'wide')).toBe(true); + }); + + it('is width-monotone and mode-monotone dragging left across every threshold', () => { + const { widths, modes } = sweep(wide({ wideWidthPx: LEFT_PANEL_MAX_PX }), range(40, 460).reverse()); + for (let i = 1; i < widths.length; i++) expect(widths[i]).toBeLessThanOrEqual(widths[i - 1]); + expect(modes.indexOf('rail')).toBeGreaterThan(0); + expect(modes.slice(modes.indexOf('rail')).every((m) => m === 'rail')).toBe(true); + }); + + it('folds a wide sidebar to the rail without an intermediate clipped width', () => { + // Swept from the 180 floor down, so every step is inside the dead zone or past + // the fold — above 180 a narrowing drag is an ordinary resize, not this claim. + const { widths } = sweep(wide({ wideWidthPx: 300 }), range(120, LEFT_PANEL_MIN_PX).reverse()); + // Only two widths ever appear: the 180 floor, and the rail. Nothing between — + // that is "do not leave a partially clipped wide sidebar". + expect(new Set(widths)).toEqual(new Set([LEFT_PANEL_MIN_PX, LEFT_RAIL_PX])); + }); + + it('keeps the drawer under the pointer through its whole band', () => { + let layout: LeftNavigationLayout = rail({ focusedSection: 'history' }); + for (const x of range(LEFT_RAIL_PX + LEFT_FOLD_THRESHOLD_PX, LEFT_RAIL_PX + LEFT_WIDE_THRESHOLD_PX)) { + layout = resolveLeftNavigationDrag(layout, x); + expect(leftNavigationWidthPx(layout)).toBe(x); + } + }); +}); + +describe('resolveLeftNavigationDrag — wide', () => { + it('resizes within the wide range', () => { + expect(resolveLeftNavigationDrag(wide(), 320)).toEqual(wide({ wideWidthPx: 320 })); + }); + it('clamps to the ceiling instead of growing past it', () => { + expect(resolveLeftNavigationDrag(wide(), 9999).wideWidthPx).toBe(LEFT_PANEL_MAX_PX); + }); + it('sits at the 180 floor through the whole dead zone rather than clipping', () => { + // #487: "do not leave a partially clipped wide sidebar". Between the fold + // threshold and the floor the sidebar holds at 180 and the mode does not + // change, so the user has to pull decisively past 140 to fold. + for (const x of [LEFT_FOLD_THRESHOLD_PX, 150, 179]) { + const next = resolveLeftNavigationDrag(wide({ wideWidthPx: 300 }), x); + expect(next.mode).toBe('wide'); + expect(next.wideWidthPx).toBe(LEFT_PANEL_MIN_PX); + } + }); + it('commits rail once past the fold threshold, exactly once', () => { + const next = resolveLeftNavigationDrag(wide({ wideWidthPx: 300 }), LEFT_FOLD_THRESHOLD_PX - 1); + expect(next.mode).toBe('rail'); + expect(next.focusedSection).toBeNull(); + // Continuing to drag left is idempotent — it does not fold "twice", and it + // does not keep rewriting the remembered width. + expect(resolveLeftNavigationDrag(next, 0)).toEqual(next); + }); + it('carries the remembered wide width through rail mode for End and for persistence', () => { + const folded = resolveLeftNavigationDrag(wide({ wideWidthPx: 300 }), 10); + expect(folded.wideWidthPx).toBe(300); + // A DRAG back out follows the pointer rather than restoring 300 (see the + // reducer's doc: a restored width would survive one frame); `End` is the + // path that restores it, asserted in the keyboard block below. + expect(resolveLeftNavigationDrag(folded, 400).wideWidthPx).toBe(400); + }); + it('keeps the drawer width untouched while folding, ready for the first rail click', () => { + expect(resolveLeftNavigationDrag(wide({ drawerWidthPx: 210 }), 10).drawerWidthPx).toBe(210); + }); +}); + +describe('resolveLeftNavigationDrag — rail', () => { + it('does nothing for a bare rail below the wide threshold — the rail width IS the mode', () => { + const layout = rail(); + // Same object back, so phase 3 can skip the repaint on identity. + expect(resolveLeftNavigationDrag(layout, LEFT_WIDE_THRESHOLD_PX)).toBe(layout); + expect(resolveLeftNavigationDrag(layout, 200)).toBe(layout); + expect(resolveLeftNavigationDrag(layout, 0)).toBe(layout); + }); + it('restores wide AT THE POINTER once past the wide threshold', () => { + const next = resolveLeftNavigationDrag(rail({ wideWidthPx: 330 }), LEFT_WIDE_THRESHOLD_PX + 1); + expect(next.mode).toBe('wide'); + // 261, not the remembered 330 — the panel edge stays under the finger. + expect(next.wideWidthPx).toBe(LEFT_WIDE_THRESHOLD_PX + 1); + expect(next.focusedSection).toBeNull(); + }); + it('does NOT restore wide at the threshold itself — hysteresis needs a decisive pull', () => { + const layout = rail(); + expect(resolveLeftNavigationDrag(layout, LEFT_WIDE_THRESHOLD_PX)).toBe(layout); + }); + it('resizes an open drawer inside its own band, measured beside the rail', () => { + const next = resolveLeftNavigationDrag(rail({ focusedSection: 'dashboards' }), LEFT_RAIL_PX + 200); + expect(next.mode).toBe('rail'); + expect(next.focusedSection).toBe('dashboards'); + expect(next.drawerWidthPx).toBe(200); + }); + it('holds an open drawer at exactly the fold threshold, and folds one pixel below it', () => { + // The closed lower edge of the drawer band: `clampDrawerWidthPx` claims 140 is + // reachable, so the reducer's comparison must agree. A `<=` here would make + // 140 unreachable while the clamp still advertised it. + const open = resolveLeftNavigationDrag( + rail({ focusedSection: 'history' }), LEFT_RAIL_PX + LEFT_FOLD_THRESHOLD_PX); + expect(open.focusedSection).toBe('history'); + expect(open.drawerWidthPx).toBe(LEFT_FOLD_THRESHOLD_PX); + const closed = resolveLeftNavigationDrag( + rail({ focusedSection: 'history' }), LEFT_RAIL_PX + LEFT_FOLD_THRESHOLD_PX - 1); + expect(closed.focusedSection).toBeNull(); + }); + it('holds an open drawer at exactly the wide threshold, and converts one pixel above it', () => { + const open = resolveLeftNavigationDrag( + rail({ focusedSection: 'history' }), LEFT_RAIL_PX + LEFT_WIDE_THRESHOLD_PX); + expect(open.mode).toBe('rail'); + expect(open.drawerWidthPx).toBe(LEFT_WIDE_THRESHOLD_PX); + const converted = resolveLeftNavigationDrag( + rail({ focusedSection: 'history' }), LEFT_RAIL_PX + LEFT_WIDE_THRESHOLD_PX + 1); + expect(converted.mode).toBe('wide'); + }); + it('folds an open drawer closed below the fold threshold, leaving the rail', () => { + const next = resolveLeftNavigationDrag( + rail({ focusedSection: 'history', drawerWidthPx: 220 }), LEFT_RAIL_PX + 100); + expect(next.mode).toBe('rail'); + expect(next.focusedSection).toBeNull(); + // Width kept for the next open — closing is not a reset. + expect(next.drawerWidthPx).toBe(220); + }); + it('cannot oscillate across the two thresholds', () => { + // A pointer parked in the sticky band, arriving from either side, keeps + // whatever mode it already had. + for (const x of [LEFT_FOLD_THRESHOLD_PX, 200, LEFT_WIDE_THRESHOLD_PX]) { + expect(resolveLeftNavigationDrag(rail(), x).mode).toBe('rail'); + expect(resolveLeftNavigationDrag(wide(), x).mode).toBe('wide'); + } + }); + it('heals a NaN proposal into a legal width rather than propagating it', () => { + expect(resolveLeftNavigationDrag(wide(), NaN).wideWidthPx).toBe(LEFT_WIDE_DEFAULT_PX); + expect(resolveLeftNavigationDrag(rail({ focusedSection: 'library' }), NaN).drawerWidthPx) + .toBe(LEFT_DRAWER_DEFAULT_PX); + }); +}); + +describe('resolveLeftNavigationKey', () => { + it('returns null for a key the separator does not own', () => { + // Phase 3 must not swallow Tab, Escape or anything else global. + for (const key of ['Tab', 'Escape', 'Enter', ' ', 'ArrowUp', 'ArrowDown', 'PageUp']) { + expect(resolveLeftNavigationKey(wide(), { key })).toBeNull(); + } + }); + it('returns null for a Ctrl/Meta/Alt chord on a key it otherwise owns', () => { + // Ctrl+Home must not fold the navigation, and Alt+ArrowLeft is the browser's + // Back on some platforms. Shift is the one modifier with a meaning here. + for (const key of ['Home', 'End', 'ArrowLeft', 'ArrowRight']) { + expect(resolveLeftNavigationKey(wide(), { key, ctrlKey: true })).toBeNull(); + expect(resolveLeftNavigationKey(wide(), { key, metaKey: true })).toBeNull(); + expect(resolveLeftNavigationKey(wide(), { key, altKey: true })).toBeNull(); + } + }); + it('Home folds to rail from wide and is idempotent', () => { + expect(resolveLeftNavigationKey(wide({ wideWidthPx: 300 }), { key: 'Home' })) + .toEqual(rail({ wideWidthPx: 300 })); + const bare = rail(); + expect(resolveLeftNavigationKey(bare, { key: 'Home' })).toBe(bare); + }); + it('Home also closes an open focused drawer', () => { + expect(resolveLeftNavigationKey(rail({ focusedSection: 'databases' }), { key: 'Home' })) + .toEqual(rail()); + }); + it('End is the one path that restores the REMEMBERED wide width', () => { + // The counterpart to the drag rule: a discrete restore has no pointer to + // honour, so the memory is what it uses. + expect(resolveLeftNavigationKey(rail({ wideWidthPx: 330, focusedSection: 'library' }), { key: 'End' })) + .toEqual(wide({ wideWidthPx: 330 })); + const already = wide(); + expect(resolveLeftNavigationKey(already, { key: 'End' })).toBe(already); + }); + it('End re-clamps an invalid remembered width', () => { + expect(resolveLeftNavigationKey(rail({ wideWidthPx: NaN }), { key: 'End' })!.wideWidthPx) + .toBe(LEFT_WIDE_DEFAULT_PX); + }); + it('arrows step the wide sidebar by the small and large steps', () => { + const at = (over: Partial, key: string, shiftKey = false) => + resolveLeftNavigationKey(wide(over), { key, shiftKey })!.wideWidthPx; + expect(at({ wideWidthPx: 300 }, 'ArrowRight')).toBe(300 + LEFT_NAV_STEP_PX); + expect(at({ wideWidthPx: 300 }, 'ArrowLeft')).toBe(300 - LEFT_NAV_STEP_PX); + expect(at({ wideWidthPx: 300 }, 'ArrowRight', true)).toBe(300 + LEFT_NAV_LARGE_STEP_PX); + expect(at({ wideWidthPx: 300 }, 'ArrowLeft', true)).toBe(300 - LEFT_NAV_LARGE_STEP_PX); + }); + it('steps the DRAWER width when a drawer is open, not the sidebar width', () => { + const next = resolveLeftNavigationKey( + rail({ focusedSection: 'history', drawerWidthPx: 200 }), { key: 'ArrowRight' })!; + expect(next.drawerWidthPx).toBe(200 + LEFT_NAV_STEP_PX); + expect(next.wideWidthPx).toBe(LEFT_WIDE_DEFAULT_PX); + }); + it('can fold an open drawer closed with an arrow at its floor', () => { + const next = resolveLeftNavigationKey( + rail({ focusedSection: 'history', drawerWidthPx: LEFT_FOLD_THRESHOLD_PX }), { key: 'ArrowLeft' })!; + expect(next.focusedSection).toBeNull(); + }); + it('leaves a bare rail on ONE rightward step, whatever the remembered width', () => { + // The bare-rail base must be the wide THRESHOLD, not `wideWidthPx`. With the + // remembered width as the base, any value at or below 244 would leave a small + // ArrowRight stuck in the sticky band forever — a separator advertising + // `aria-valuemax: 420` while refusing to move. 180 is trivially reachable by + // dragging the sidebar narrow before folding, so this is not a corner case. + for (const wideWidthPx of [LEFT_PANEL_MIN_PX, 200, 244, LEFT_WIDE_DEFAULT_PX, LEFT_PANEL_MAX_PX]) { + for (const shiftKey of [false, true]) { + expect(resolveLeftNavigationKey(rail({ wideWidthPx }), { key: 'ArrowRight', shiftKey })!.mode) + .toBe('wide'); + } + } + }); + it('holds a bare rail on a leftward step — it is already as folded as it goes', () => { + for (const shiftKey of [false, true]) { + const stay = rail(); + expect(resolveLeftNavigationKey(stay, { key: 'ArrowLeft', shiftKey })).toBe(stay); + } + }); + it('matches pointer transitions for the same proposed total width', () => { + // The property the design exists for. The pointer side is driven from the + // module's PUBLIC occupied width, never from a copy of the private base + // formula — recomputing the implementation here is what let a bare-rail base + // bug survive a green suite, so the bare rail (whose base is deliberately the + // threshold, not its own width) is asserted separately above. + const cases: LeftNavigationLayout[] = [ + wide({ wideWidthPx: 300 }), + wide({ wideWidthPx: LEFT_PANEL_MIN_PX }), + wide({ wideWidthPx: LEFT_PANEL_MAX_PX }), + rail({ focusedSection: 'databases', drawerWidthPx: LEFT_FOLD_THRESHOLD_PX }), + rail({ focusedSection: 'databases', drawerWidthPx: 200 }), + rail({ focusedSection: 'databases', drawerWidthPx: LEFT_WIDE_THRESHOLD_PX }), + ]; + for (const layout of cases) { + for (const shiftKey of [false, true]) { + const step = shiftKey ? LEFT_NAV_LARGE_STEP_PX : LEFT_NAV_STEP_PX; + const base = leftNavigationWidthPx(layout); + expect(resolveLeftNavigationKey(layout, { key: 'ArrowRight', shiftKey })) + .toEqual(resolveLeftNavigationDrag(layout, base + step)); + expect(resolveLeftNavigationKey(layout, { key: 'ArrowLeft', shiftKey })) + .toEqual(resolveLeftNavigationDrag(layout, base - step)); + } + } + }); + it('is a no-op at the wide floor and ceiling for a small step, and folds on a large one', () => { + // Unlike the bare rail, this is NOT a dead end: the pointer does the same + // thing at these extremes (clientX 164 also leaves a 180 sidebar at 180), and + // Shift+ArrowLeft escapes. Keyboard and pointer agree, which is the contract. + expect(resolveLeftNavigationKey(wide({ wideWidthPx: LEFT_PANEL_MIN_PX }), { key: 'ArrowLeft' })) + .toEqual(wide({ wideWidthPx: LEFT_PANEL_MIN_PX })); + expect(resolveLeftNavigationKey(wide({ wideWidthPx: LEFT_PANEL_MAX_PX }), { key: 'ArrowRight' })) + .toEqual(wide({ wideWidthPx: LEFT_PANEL_MAX_PX })); + expect(resolveLeftNavigationKey( + wide({ wideWidthPx: LEFT_PANEL_MIN_PX }), { key: 'ArrowLeft', shiftKey: true })!.mode).toBe('rail'); + }); +}); + +describe('resolveRailActivation', () => { + it('opens a section from a bare rail', () => { + expect(resolveRailActivation(rail(), 'dashboards')).toEqual(rail({ focusedSection: 'dashboards' })); + }); + it('closes the drawer when the ACTIVE section is activated again', () => { + expect(resolveRailActivation(rail({ focusedSection: 'dashboards' }), 'dashboards')).toEqual(rail()); + }); + it('switches content in place without closing first', () => { + const next = resolveRailActivation(rail({ focusedSection: 'dashboards' }), 'history'); + expect(next.focusedSection).toBe('history'); + expect(next.mode).toBe('rail'); + }); + it('preserves the remembered widths across every activation', () => { + const layout = rail({ wideWidthPx: 330, drawerWidthPx: 210 }); + for (const section of LEFT_NAV_SECTIONS) { + const next = resolveRailActivation(layout, section); + expect(next.wideWidthPx).toBe(330); + expect(next.drawerWidthPx).toBe(210); + } + }); + it('cannot open a drawer in wide mode — phase 3 must route to the pane switchers', () => { + const layout = wide(); + for (const section of LEFT_NAV_SECTIONS) { + expect(resolveRailActivation(layout, section)).toBe(layout); + } + }); +}); + +// The invariant every reducer shares, asserted over their combined reachable +// space rather than re-derived per test: a focused drawer exists only in rail +// mode. `leftNavigationWidthPx` reads `drawerWidthPx` only in rail mode, so a +// 'wide' layout carrying a section would push the centre surface by a width that +// omits the open drawer. +describe('mode/focusedSection coherence', () => { + it('holds across every reachable drag, key and activation from every seed', () => { + const seeds: LeftNavigationLayout[] = [ + wide(), wide({ wideWidthPx: LEFT_PANEL_MAX_PX }), rail(), + ...LEFT_NAV_SECTIONS.map((s) => rail({ focusedSection: s })), + ]; + const keys = ['Home', 'End', 'ArrowLeft', 'ArrowRight']; + for (const seed of seeds) { + expect(leftNavigationLayoutIsCoherent(seed)).toBe(true); + for (const x of [0, 100, 139, 140, 180, 200, 260, 261, 308, 309, 420, 999, NaN]) { + expect(leftNavigationLayoutIsCoherent(resolveLeftNavigationDrag(seed, x))).toBe(true); + } + for (const key of keys) { + for (const shiftKey of [false, true]) { + const next = resolveLeftNavigationKey(seed, { key, shiftKey }); + if (next) expect(leftNavigationLayoutIsCoherent(next)).toBe(true); + } + } + for (const section of LEFT_NAV_SECTIONS) { + expect(leftNavigationLayoutIsCoherent(resolveRailActivation(seed, section))).toBe(true); + } + } + }); + it('rejects the incoherent shape it exists to forbid', () => { + // Guards the guard: if this predicate were vacuously true, the sweep above + // would prove nothing. + expect(leftNavigationLayoutIsCoherent(wide({ focusedSection: 'databases' }))).toBe(false); + }); +}); + +describe('effectiveLeftNavigationLayout', () => { + it('returns the desktop layout untouched', () => { + for (const layout of [wide(), rail(), rail({ focusedSection: 'library' })]) { + expect(effectiveLeftNavigationLayout(layout, false)).toBe(layout); + } + }); + it('ignores rail mode and any open drawer below the mobile breakpoint', () => { + // #487: "do not render the desktop rail or desktop focused drawer" on mobile. + const effective = effectiveLeftNavigationLayout( + rail({ focusedSection: 'dashboards', wideWidthPx: 330, drawerWidthPx: 210 }), true); + expect(effective.mode).toBe('wide'); + expect(effective.focusedSection).toBeNull(); + }); + it('preserves the desktop preferences it is ignoring, for the next desktop session', () => { + const stored = rail({ focusedSection: 'dashboards', wideWidthPx: 330, drawerWidthPx: 210 }); + const effective = effectiveLeftNavigationLayout(stored, true); + // The projection carries both widths through … + expect(effective.wideWidthPx).toBe(330); + expect(effective.drawerWidthPx).toBe(210); + // … and never writes back: the stored layout still says rail. + expect(stored.mode).toBe('rail'); + expect(stored.focusedSection).toBe('dashboards'); + }); + it('returns an already-wide layout by identity even on mobile', () => { + const layout = wide(); + expect(effectiveLeftNavigationLayout(layout, true)).toBe(layout); + }); +}); + +describe('leftNavigationSeparatorAria', () => { + it('reports the rail floor, the wide ceiling and the live occupied width', () => { + expect(leftNavigationSeparatorAria(wide({ wideWidthPx: 300 }))) + .toEqual({ valueMin: LEFT_RAIL_PX, valueMax: LEFT_PANEL_MAX_PX, valueNow: 300 }); + expect(leftNavigationSeparatorAria(rail()).valueNow).toBe(LEFT_RAIL_PX); + expect(leftNavigationSeparatorAria(rail({ focusedSection: 'library', drawerWidthPx: 200 })).valueNow) + .toBe(LEFT_RAIL_PX + 200); + }); + it('keeps valueNow inside the advertised range in every mode', () => { + for (const layout of [ + wide({ wideWidthPx: LEFT_PANEL_MIN_PX }), wide({ wideWidthPx: LEFT_PANEL_MAX_PX }), rail(), + rail({ focusedSection: 'library', drawerWidthPx: LEFT_WIDE_THRESHOLD_PX }), + ]) { + const { valueMin, valueMax, valueNow } = leftNavigationSeparatorAria(layout); + expect(valueNow).toBeGreaterThanOrEqual(valueMin); + expect(valueNow).toBeLessThanOrEqual(valueMax); + } + }); +}); diff --git a/tests/unit/state.test.ts b/tests/unit/state.test.ts index cc48eee2..ad28cbd6 100644 --- a/tests/unit/state.test.ts +++ b/tests/unit/state.test.ts @@ -146,6 +146,8 @@ describe('KEYS — persisted localStorage key names (#459)', () => { sideSplitPct: 'asb:sideSplitPct', cellDrawerPx: 'asb:cellDrawerPx', docPanePx: 'asb:docPanePx', + leftNavMode: 'asb:leftNavMode', + leftNavDrawerPx: 'asb:leftNavDrawerPx', sidePanel: 'asb:sidePanel', saved: 'asb:saved', history: 'asb:history', @@ -191,6 +193,12 @@ describe('createState', () => { expect(s.sideSplitPct).toBe(58); expect(s.cellDrawerPx).toBe(560); expect(s.docPanePx).toBe(420); // #313 — a sibling default, independent of cellDrawerPx + // #487 — a fresh desktop session starts from the documented default: the + // established two-pane sidebar, a drawer width ready for its first open, and + // NO focused section (the drawer is session state and does not reopen). + expect(s.leftNavMode.value).toBe('wide'); + expect(s.leftNavDrawerPx).toBe(240); + expect(s.leftNavSection.value).toBeNull(); expect(s.tabs.value).toHaveLength(1); expect(s.savedQueries).toEqual([]); expect(s.savedQueryLoadDiagnostics).toEqual([]); @@ -223,6 +231,8 @@ describe('createState', () => { [KEYS.sideSplitPct]: '99', // clamps to 85 [KEYS.cellDrawerPx]: '100', // clamps up to the 320 floor [KEYS.docPanePx]: '50', // clamps up to the 320 floor, independent of cellDrawerPx + [KEYS.leftNavMode]: 'rail', // #487 — a valid persisted mode restores + [KEYS.leftNavDrawerPx]: '200', [KEYS.sidePanel]: 'history', [KEYS.saved]: [{ id: 's1', sql: 'x', name: 'n', starred: true }], [KEYS.history]: [{ id: 'h1', sql: 'y', ts: 1, rows: 1, ms: 2 }], @@ -239,6 +249,11 @@ describe('createState', () => { expect(s.sideSplitPct).toBe(85); expect(s.cellDrawerPx).toBe(320); expect(s.docPanePx).toBe(320); // #313 + expect(s.leftNavMode.value).toBe('rail'); // #487 + expect(s.leftNavDrawerPx).toBe(200); + // …but the focused drawer still does not reopen: #487 makes `focusedSection` + // session UI state, so restoring rail mode restores a BARE rail. + expect(s.leftNavSection.value).toBeNull(); expect(s.sidePanel.value).toBe('history'); expect(s.savedQueries).toHaveLength(1); expect(s.history).toHaveLength(1); @@ -255,6 +270,89 @@ describe('createState', () => { }); }); +// #487 phase 1 — the desktop left navigation's persisted preferences. The +// transitions themselves live in `core/left-nav-layout.ts` (and its own spec); +// what is asserted here is the STORAGE contract: which keys exist, what a fresh +// session gets, that a hostile stored value can never reach the DOM, and that +// none of it leaks into the workspace document. +describe('createState — left navigation preferences (#487)', () => { + it('clamps every invalid or obsolete stored value back to its documented default', () => { + const s = createState(reader({ + // An obsolete third mode from a future/older build is not a third mode. + [KEYS.leftNavMode]: 'collapsed', + [KEYS.leftNavDrawerPx]: 'not-a-number', + [KEYS.sidebarPx]: 'not-a-number', + })); + expect(s.leftNavMode.value).toBe('wide'); + expect(s.leftNavDrawerPx).toBe(240); + // 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 + // sidebar with nothing in the UI to explain it. + expect(s.sidebarPx).toBe(248); + }); + + it('clamps an out-of-range drawer width into the drawer own band', () => { + // Not the wide sidebar's [180, 420]: a drawer wider than the wide threshold + // is unreachable, because a drag that far right converts to the sidebar. + expect(createState(reader({ [KEYS.leftNavDrawerPx]: '9999' })).leftNavDrawerPx).toBe(260); + expect(createState(reader({ [KEYS.leftNavDrawerPx]: '0' })).leftNavDrawerPx).toBe(140); + }); + + it('keeps the wide width on the ONE preference key, with no second owner', () => { + // #487 suggests a separate `wideWidthPx`; `asb:sidebarPx` already persists + // exactly that width over exactly that range, and two owners of one width is + // a bug waiting to happen. This pins the decision: the left-nav keys are the + // mode and the drawer, and nothing else. Order-independent — reordering two + // adjacent declarations in `KEYS` is not a semantic change. + const leftNavKeys = Object.keys(KEYS).filter((k) => k.startsWith('leftNav')); + expect(new Set(leftNavKeys)).toEqual(new Set(['leftNavMode', 'leftNavDrawerPx'])); + expect(leftNavKeys).toHaveLength(2); + const s = createState(reader({ [KEYS.sidebarPx]: '330' })); + expect(s.sidebarPx).toBe(330); + }); + + it('never writes left-navigation state into the workspace document', async () => { + // #487: "none of this state belongs in StoredWorkspaceV3, Dashboard documents + // or query specs" (V5 today). + // + // This drives the REAL commit path — `createSavedQuery` builds its candidate + // through `state.ts`'s own `baselineWorkspace`/`candidateFrom` projection — and + // inspects the candidate that projection actually produced. Asserting over a + // hand-built workspace literal instead would be unfalsifiable: it would only + // prove the test didn't add the fields itself. + // + // Sabotage-checked, and the exercise located where the guarantee really lives: + // adding a field to `baselineWorkspace`'s fallback alone does NOT reach a + // commit, because `candidateFrom` re-enumerates the six aggregate fields + // explicitly and drops anything else. That enumeration is the structural + // guarantee, and adding `leftNavMode` to it fails this test. + const s = savedTestState({ [KEYS.leftNavMode]: 'rail', [KEYS.leftNavDrawerPx]: '200' }); + s.leftNavSection.value = 'dashboards'; + s.tabs.value[0].sqlDraft = 'SELECT 1'; + const mutate = fakeMutateWorkspace(s); + expect(okEntry(await createSavedQuery(s, s.tabs.value[0], 'Q', '', mutate))).toBeTruthy(); + + const candidate = mutate.commit.mock.calls[0]![0] as StoredWorkspaceV5; + // The aggregate's own key set, exactly — no left-navigation field smuggled in. + expect(Object.keys(candidate).sort()) + .toEqual(['dashboards', 'id', 'key', 'name', 'queries', 'storageVersion']); + for (const marker of ['leftNav', 'focusedSection', 'drawerWidth', 'sidebarPx']) { + expect(JSON.stringify(candidate)).not.toContain(marker); + } + // Second layer: the repository validated this candidate against the closed + // stored-workspace schema (`additionalProperties: false`), so an extra field + // would have been REJECTED rather than persisted. The commit succeeded, which + // is that check passing on the real record. + expect(mutate.commit).toHaveBeenCalledTimes(1); + // And the preferences are localStorage keys, never workspace fields. + for (const key of [KEYS.leftNavMode, KEYS.leftNavDrawerPx]) { + expect(key.startsWith('asb:')).toBe(true); + expect(Object.keys(candidate)).not.toContain(key); + } + }); +}); + describe('effectiveFilterActive (#165)', () => { it('an explicit filterActive entry wins over the stored value', () => { expect(effectiveFilterActive({ d: 'stale' }, { d: false })).toEqual({ d: false }); From acc7317ba269b7194b059eed28c3f352391ad8c8 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Wed, 29 Jul 2026 22:47:09 +0200 Subject: [PATCH 2/2] fix(#487): address ChatGPT review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A third-party review pass raised six findings against the phase-1 module. All six reproduced against the real code; all six are fixed here. The most valuable one is the exact mirror of the bug the earlier review caught, which this suite had been blessing with a test. 1. **Plain ArrowLeft could never fold a wide sidebar.** At the 180px floor a -16 step proposes 164, which clamps straight back to 180 — so eleven presses from a 300px sidebar sat at 180 forever while the equivalent pointer path folded: keyboard 284,268,252,236,220,204,188,180,180,180,180 -> wide pointer 284,268,252,236,220,204,188,180,180,180,48 -> rail This is the same class of dead end as the bare-rail one already fixed, at the opposite end, and the previous commit's test explicitly blessed it on the grounds that Home and Shift+Arrow escape. That is not a defence: the W3C splitter pattern makes plain Left/Right the separator's move keys, and `aria-valuemin: 48` was advertised while the control refused to move. Arrows now resize within a band and perform the band edge's semantic transition, symmetric at both ends. `keyboardBaseTotalPx`'s virtual 260 base is gone with it — it was a false relative step (ArrowRight from a 48px rail moved +228, not +16) and it discarded a remembered 420, handing back 276. A bare rail's ArrowRight now restores the remembered width, like End. 2. **`resolveRailActivation` is a toggle, so it cannot be the `openFocusedSection` seam** #487 mandates for #428. Bounded drag-hover re-asserts intent while a query is held over the Dashboards icon, so a toggle flaps the drawer open and shut on alternate notifications. Added `resolveRailOpen`, idempotent and identity-returning when already open; the toggle stays for clicks. 3. **The coherence invariant was a precondition, not a postcondition.** `state.ts` stores `mode` and `focusedSection` as two independently writable signals, and the reducers preserved an illegal pair rather than healing it — `drag({mode: 'wide', focusedSection: 'databases'}, 300)` returned it intact, and `End` handed it straight back. Every reducer now normalizes its input through `normalizeLeftNavigationLayout`, which also lifts non-finite and out-of-band widths, so `leftNavigationLayoutIsCoherent` is "normalizing changes nothing" and covers widths rather than only the mode/section pairing. A NaN width can no longer reach `aria-valuenow`. 4. **`parseInt` accepted a numeric prefix**, so `'12junk'` decoded to 12 and `'200px'` to 200 while the documented contract promised the default. Added `decodeStoredPx`, which requires the whole string to be a finite number — and therefore also rejects a stored `'Infinity'`, which is corruption rather than a width pressed against a bound. 5. **The restore memory is sampling-dependent** and this commit does NOT fix it: from a 300px sidebar, a single coarse sample past the fold remembers 300, but an intermediate sample inside the 140-179 dead zone rests the width at the floor first and remembers 180. One field is serving as both the live drag width and the restore memory; separating them needs a drag-session snapshot, which a pure reducer cannot take. Pinned with a test so phase 3 has to change it deliberately, and recorded as a phase-3 obligation in the ship log. 6. **The centre clamp's phase boundary was unsafe.** Phase 3 turns the feature on while the clamp sat in phase 4, leaving a shippable interval where both docked panels could starve the centre surface. Moved to phase 3, before activation. Two findings were verified and NOT changed, with reasons recorded in the ship log: the drawer's [140, 260] band (a 150px drawer may be unusably narrow, but #487 says these constants are settled only by real-browser verification), and the separator's discontinuous ARIA interior (inherent to one control spanning two modes; phase 3 adds mode-aware `aria-valuetext`). npm test 6755 passing, `left-nav-layout.ts` at 100/100/100/100 (17 functions, 64 lines, 86 branches). Sabotage-checked: dropping the wide-floor transition, making the open seam a toggle, removing reducer normalization, and reverting to parseInt each fail specific tests. Sidebar drag e2e green. Part of #487 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FPnt3pq98P5Y1ww3sWawp1 --- CHANGELOG.md | 24 ++- src/core/left-nav-layout.ts | 207 ++++++++++++++++++-------- src/state.ts | 17 ++- tests/unit/left-nav-layout.test.ts | 228 +++++++++++++++++++++++++---- tests/unit/state.test.ts | 17 +++ 5 files changed, 395 insertions(+), 98 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b60c5788..4e0e25e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,8 +27,17 @@ auto-generated per-PR notes; this file is the curated, human-readable history. navigation's proposed *total* width, which is what keeps a drag continuous across a mode change: handing the reducer a mode-relative width instead made a monotone rightward drag snap the navigation 108px backwards on the frame a - drawer converted to the sidebar. **No user-visible change yet**: the rail, the - docked focused drawer and the resize separator arrive in phase 3. + drawer converted to the sidebar. Arrow keys resize within a band and perform + the band edge's semantic transition, because an arrow's *relative* step cannot + cross a dead zone wider than itself — leaving that relative stranded a bare + rail's `ArrowRight` and a floored sidebar's `ArrowLeft` permanently, while + `aria-valuemin: 48` was advertised throughout. Opening a section is a separate + operation from the click toggle, since #428's bounded drag-hover re-asserts + intent repeatedly and a toggle would flap the drawer shut. Every reducer + normalizes its input, so the "a focused drawer exists only in rail mode" + 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. - 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 @@ -55,12 +64,15 @@ auto-generated per-PR notes; this file is the curated, human-readable history. and `clamp` is not NaN-safe (`Math.max(180, NaN)` is `NaN`), so a non-numeric stored value would reach the DOM as `width: NaNpx` — which the browser drops, collapsing the sidebar with nothing to explain why, and the bad value would - persist across reloads. It now falls back to the documented 248px default. + persist across reloads. It now falls back to the documented 248px default, and + decoding requires the *whole* stored string to be a finite number — `parseInt` + accepted a numeric prefix, so a truncated write or a hand-edited `"200px"` + decoded to a plausible-looking width while the contract promised the default. Hardening rather than a reproducible user-visible bug: no code path in the app - writes a non-numeric value (a real drag always carries a finite `clientX`), so + writes a malformed value (a real drag always carries a finite `clientX`), so reaching it takes a hand-edited or foreign-origin `localStorage` entry. The - same hole in `editorPct`/`sideSplitPct`/`cellDrawerPx`/`docPanePx` is tracked - separately in #570. + same NaN hole in `editorPct`/`sideSplitPct`/`cellDrawerPx`/`docPanePx` is + tracked separately in #570. - **The Dashboard tree no longer reveals two rows' pencil/trash actions at once, and its `· N` count now sits inline after the label** (#568). The hover/focus reveal rule (`.dash-tree-row:focus-within .dash-tree-act`) diff --git a/src/core/left-nav-layout.ts b/src/core/left-nav-layout.ts index e4cea2b0..edf84c1b 100644 --- a/src/core/left-nav-layout.ts +++ b/src/core/left-nav-layout.ts @@ -133,11 +133,42 @@ export function isLeftNavigationSection(value: unknown): value is LeftNavigation return LEFT_NAV_SECTIONS.some((section) => section === value); } -/** The `mode`/`focusedSection` invariant, as a predicate — a focused drawer - * exists only in rail mode. Exported so the reducers' shared postcondition can - * be asserted directly instead of re-derived in each test. */ +/** + * Heal a layout into a renderable one: a focused drawer only in rail mode, both + * widths finite and inside their own band, and a section only if it is one of the + * four. Returns the argument itself when it is already legal, so the reducers' + * identity-skip contract survives. + * + * Every reducer normalizes its input through this, which is what makes their + * postcondition unconditional. Without it the invariant was only ever a + * *precondition* — `resolveLeftNavigationDrag({ mode: 'wide', focusedSection: + * 'databases' }, 300)` preserved the illegal pair rather than fixing it, and + * `End` handed it straight back. That matters because `state.ts` stores `mode` and + * `focusedSection` in two independently writable signals: any caller that writes + * one without the other produces exactly that pair, and `leftNavigationWidthPx` + * would then push the centre surface by a width that omits the open drawer. + * + * Healing here does not make the atomic-write discipline optional — phase 3 should + * still write both signals together — but it means a slip is corrected on the next + * interaction instead of persisting as an unrenderable shell. + */ +export function normalizeLeftNavigationLayout(layout: LeftNavigationLayout): LeftNavigationLayout { + const mode = layout.mode === 'rail' ? 'rail' : 'wide'; + const wideWidthPx = clampWideWidthPx(layout.wideWidthPx); + const drawerWidthPx = clampDrawerWidthPx(layout.drawerWidthPx); + const focusedSection = mode === 'rail' && isLeftNavigationSection(layout.focusedSection) + ? layout.focusedSection + : null; + const unchanged = mode === layout.mode && wideWidthPx === layout.wideWidthPx + && drawerWidthPx === layout.drawerWidthPx && focusedSection === layout.focusedSection; + return unchanged ? layout : { mode, wideWidthPx, drawerWidthPx, focusedSection }; +} + +/** A layout is coherent exactly when normalizing it changes nothing — so this + * covers the `mode`/`focusedSection` pairing AND finite, in-band widths, rather + * than only the pairing (a `NaN` width used to pass). */ export function leftNavigationLayoutIsCoherent(layout: LeftNavigationLayout): boolean { - return layout.mode === 'rail' || layout.focusedSection === null; + return normalizeLeftNavigationLayout(layout) === layout; } /** Decode a persisted mode. Anything that is not exactly `'rail'` — a missing @@ -183,6 +214,28 @@ export function clampDrawerWidthPx(px: number): number { return clamp(px, LEFT_FOLD_THRESHOLD_PX, LEFT_WIDE_THRESHOLD_PX); } +/** + * Decode a persisted pixel width, falling back to `fallbackPx` for anything that + * is not a complete number. + * + * `parseInt` is deliberately not used: it accepts a numeric *prefix*, so + * `'12junk'` decodes to 12 and `'200px'` to 200 — which made the "an invalid + * stored value returns to its documented default" contract false for exactly the + * corruption most likely to occur (a truncated write, or a value someone hand- + * edited with a CSS unit). `Number` requires the whole string, and the + * `Number.isFinite` guard also rejects the literal `'Infinity'` that `Number` + * would otherwise accept — a stored infinity is a corrupt value, not a width + * pressed against a bound. + * + * The clamp still runs afterwards, so a well-formed but out-of-range value is + * pulled into its band rather than discarded. + */ +export function decodeStoredPx(raw: unknown, fallbackPx: number): number { + if (typeof raw !== 'string' || raw.trim() === '') return fallbackPx; + const parsed = Number(raw); + return Number.isFinite(parsed) ? parsed : fallbackPx; +} + /** * The width the navigation actually occupies in the shell row — what the centre * surface is pushed by, and what the separator reports as `aria-valuenow`. @@ -227,8 +280,9 @@ export function leftNavigationWidthPx(layout: LeftNavigationLayout): number { * moving) would show a sidebar that refuses to shrink to its own floor. */ export function resolveLeftNavigationDrag( - layout: LeftNavigationLayout, totalPx: number, + input: LeftNavigationLayout, totalPx: number, ): LeftNavigationLayout { + const layout = normalizeLeftNavigationLayout(input); if (layout.mode === 'wide') { // A wide sidebar IS the whole navigation, so its panel width is the total. // Past the fold threshold: commit rail. The wide width is frozen at whatever @@ -276,30 +330,20 @@ export interface LeftNavigationKey { altKey?: boolean; } -/** - * The TOTAL width an arrow step starts from — the same currency the reducer - * takes, so for a wide sidebar and for an open drawer this is simply the width - * the navigation already occupies. - * - * A bare rail is the exception: its base is `LEFT_WIDE_THRESHOLD_PX`, not its own - * 48px. That is not a fudge — it is the only base that keeps the keyboard honest. - * A pointer can leave a bare rail because `clientX` is absolute, so dragging to - * x=300 proposes 300; an arrow key only has a *relative* step, so a base of 48 - * would propose 64, land in the sticky band, and change nothing — forever — while - * the separator still advertised `aria-valuemax: 420`, a control that lies about - * being resizable. Basing it at the threshold makes both directions come out - * right through the ordinary reducer, with no special-casing there: a rightward - * step crosses into wide (nothing legal exists between the rail and the 180 - * floor), and a leftward step lands in the sticky band and correctly does - * nothing, because the rail is already as folded as it goes. - * - * Deliberately a CONSTANT and not `wideWidthPx`: any remembered width at or below - * 244 would otherwise leave a bare rail's small ArrowRight stuck in the sticky - * band again, reintroducing exactly the dead end this exists to prevent. - */ -function keyboardBaseTotalPx(layout: LeftNavigationLayout): number { - if (layout.mode === 'rail' && layout.focusedSection === null) return LEFT_WIDE_THRESHOLD_PX; - return leftNavigationWidthPx(layout); +/** Fold to a bare rail — `Home`, and the leftward arrow's boundary transition. */ +function foldToRail(layout: LeftNavigationLayout): LeftNavigationLayout { + return layout.mode === 'rail' && layout.focusedSection === null + ? layout + : { ...layout, mode: 'rail', focusedSection: null }; +} + +/** Restore the wide sidebar at its REMEMBERED width — `End`, and the rightward + * arrow's boundary transition out of a bare rail. This is the one place a + * remembered width is restored; a pointer drag follows the pointer instead. */ +function restoreWide(layout: LeftNavigationLayout): LeftNavigationLayout { + return layout.mode === 'wide' + ? layout + : { ...layout, mode: 'wide', wideWidthPx: clampWideWidthPx(layout.wideWidthPx), focusedSection: null }; } /** @@ -307,40 +351,67 @@ function keyboardBaseTotalPx(layout: LeftNavigationLayout): number { * ours — phase 3 must not swallow keys it does not handle, and must not treat a * Ctrl/Meta/Alt chord as a resize. * - * Every arrow step routes through `resolveLeftNavigationDrag`, which is what - * makes #487's "keyboard separator operations match pointer transitions" true by - * construction rather than by two implementations agreeing. + * **Arrows resize within a band and perform the semantic transition at its edge.** + * That edge case is not decoration: an arrow key carries a *relative* step, and + * both bands are bounded by a dead zone wider than one step, so a purely relative + * arrow gets stranded at a boundary forever. + * + * Both ends had that failure, and they are exact mirrors: * - * `End` is the one place a remembered width is restored — it is the discrete - * counterpart to a drag, with no pointer position to honour instead. + * - a bare rail is 48px wide and the nearest legal wide width is 180, so a +16 + * step proposes 64, lands in the sticky band and does nothing; + * - a wide sidebar at its 180 floor folds only below 140, so a −16 step proposes + * 164, clamps straight back to 180 and does nothing. + * + * A *pointer* escapes both because `clientX` is absolute — it keeps travelling + * until it crosses the threshold — so leaving them relative made the keyboard and + * pointer disagree over a *sequence* even while agreeing on every single step. + * Eleven ArrowLeft presses from a 300px sidebar used to sit at 180 forever while + * the equivalent pointer path folded, with `aria-valuemin: 48` advertised + * throughout. `Home`/`Shift+Arrow` escaping is not a defence: the W3C splitter + * pattern makes plain Left/Right the separator's move keys. + * + * So the boundary step performs the transition the band edge implies, and every + * step inside a band still routes through `resolveLeftNavigationDrag` — the + * resize arithmetic has exactly one implementation. */ export function resolveLeftNavigationKey( - layout: LeftNavigationLayout, event: LeftNavigationKey, + input: LeftNavigationLayout, event: LeftNavigationKey, ): LeftNavigationLayout | null { if (event.ctrlKey || event.metaKey || event.altKey) return null; - // Home folds to rail and End restores wide, both regardless of the current - // mode — pressed twice they are idempotent, not a toggle. - if (event.key === 'Home') { - return layout.mode === 'rail' && layout.focusedSection === null - ? layout - : { ...layout, mode: 'rail', focusedSection: null }; + const layout = normalizeLeftNavigationLayout(input); + // Home folds and End restores, regardless of mode — pressed twice they are + // idempotent, not a toggle. + if (event.key === 'Home') return foldToRail(layout); + if (event.key === 'End') return restoreWide(layout); + if (event.key !== 'ArrowLeft' && event.key !== 'ArrowRight') return null; + const towardWide = event.key === 'ArrowRight'; + // A bare rail has no panel to resize: rightward is the restore transition (at + // the remembered width, like End — a fixed threshold-plus-step would silently + // discard a remembered 420 and hand back 276), and leftward is a no-op because + // the rail is already as folded as it goes. + if (layout.mode === 'rail' && layout.focusedSection === null) { + return towardWide ? restoreWide(layout) : layout; } - if (event.key === 'End') { - return layout.mode === 'wide' - ? layout - : { ...layout, mode: 'wide', wideWidthPx: clampWideWidthPx(layout.wideWidthPx), focusedSection: null }; + // A wide sidebar already at its floor: leftward is the fold transition. An open + // drawer needs no equivalent — its own floor IS the fold threshold, so an + // ordinary step below it already closes it. + if (layout.mode === 'wide' && !towardWide && layout.wideWidthPx <= LEFT_PANEL_MIN_PX) { + return foldToRail(layout); } - if (event.key !== 'ArrowLeft' && event.key !== 'ArrowRight') return null; const step = event.shiftKey ? LEFT_NAV_LARGE_STEP_PX : LEFT_NAV_STEP_PX; - const delta = event.key === 'ArrowRight' ? step : -step; - return resolveLeftNavigationDrag(layout, keyboardBaseTotalPx(layout) + delta); + return resolveLeftNavigationDrag(layout, leftNavigationWidthPx(layout) + (towardWide ? step : -step)); } /** - * Resolve a rail launcher activation — a click, or phase 3's - * `openFocusedSection(section)` seam (#428's deterministic entry point). - * Activating the ACTIVE section closes the drawer; activating a different one - * switches content in place without closing first, as #487 requires. + * A rail launcher CLICK. Clicking the active section closes the drawer; clicking a + * different one switches content in place without closing first, as #487 requires. + * + * **This is a toggle, so it is not the `openFocusedSection` seam** — see + * `resolveRailOpen` below. Conflating the two is a real bug rather than a naming + * quibble: #428's bounded drag-hover fires repeatedly while a Library query is + * held over the Dashboards icon, and a toggle would flap the drawer open and shut + * on alternate notifications. * * **Returns the layout unchanged in wide mode, and that is a hard limit, not a * silent fallback.** There is no drawer in wide mode and both panes are already @@ -352,12 +423,31 @@ export function resolveLeftNavigationKey( * width that omits the drawer entirely. */ export function resolveRailActivation( - layout: LeftNavigationLayout, section: LeftNavigationSection, + input: LeftNavigationLayout, section: LeftNavigationSection, ): LeftNavigationLayout { + const layout = normalizeLeftNavigationLayout(input); if (layout.mode !== 'rail') return layout; return { ...layout, focusedSection: layout.focusedSection === section ? null : section }; } +/** + * Open a section IDEMPOTENTLY — the deterministic `openFocusedSection(section)` + * seam #487 requires the left-navigation API to provide for #428. + * + * "Deterministic" is the operative word: repeated calls must leave the section + * open, because the caller is a bounded drag-hover that re-asserts intent rather + * than a click that expresses a change. Already showing this section returns the + * layout by identity; wide mode returns unchanged, for the same reason as + * `resolveRailActivation`. + */ +export function resolveRailOpen( + input: LeftNavigationLayout, section: LeftNavigationSection, +): LeftNavigationLayout { + const layout = normalizeLeftNavigationLayout(input); + if (layout.mode !== 'rail' || layout.focusedSection === section) return layout; + return { ...layout, focusedSection: section }; +} + /** * The layout that actually applies at this viewport. Below the mobile breakpoint * #487 requires the desktop rail and focused drawer not to render at all, and the @@ -373,12 +463,11 @@ export function resolveRailActivation( * directly. */ export function effectiveLeftNavigationLayout( - layout: LeftNavigationLayout, isMobile: boolean, + input: LeftNavigationLayout, isMobile: boolean, ): LeftNavigationLayout { + const layout = normalizeLeftNavigationLayout(input); if (!isMobile) return layout; - return layout.mode === 'wide' && layout.focusedSection === null - ? layout - : { ...layout, mode: 'wide', focusedSection: null }; + return layout.mode === 'wide' ? layout : { ...layout, mode: 'wide', focusedSection: null }; } /** The separator's ARIA range: the rail's width is the floor (the navigation can @@ -401,6 +490,8 @@ export function leftNavigationSeparatorAria(layout: LeftNavigationLayout): LeftN return { valueMin: LEFT_RAIL_PX, valueMax: LEFT_PANEL_MAX_PX, - valueNow: leftNavigationWidthPx(layout), + // Normalized, so a caller holding a layout with a non-finite width cannot + // publish `aria-valuenow="NaN"` to assistive technology. + valueNow: leftNavigationWidthPx(normalizeLeftNavigationLayout(layout)), }; } diff --git a/src/state.ts b/src/state.ts index dd74f759..94d4811b 100644 --- a/src/state.ts +++ b/src/state.ts @@ -37,7 +37,7 @@ import type { QueryTimeRangeInferenceDiagnostic } from './core/query-time-range. import { deriveWorkspaceKey } from './core/workspace-key.js'; import { LEFT_DRAWER_DEFAULT_PX, LEFT_WIDE_DEFAULT_PX, - clampDrawerWidthPx, clampWideWidthPx, decodeLeftNavigationMode, + clampDrawerWidthPx, clampWideWidthPx, decodeLeftNavigationMode, decodeStoredPx, } from './core/left-nav-layout.js'; import type { LeftNavigationMode, LeftNavigationSection } from './core/left-nav-layout.js'; @@ -666,12 +666,13 @@ export function createState(read: StateReader = { loadJSON, loadStr }): AppState // back to the default so the selector always reflects a real choice. resultRowLimit: normalizeRowLimit(parseInt(read.loadStr(KEYS.resultRowLimit, '500'), 10)), // #487 — the WIDE left-navigation width, and the one width the fold/restore - // machine remembers. `clampWideWidthPx` enforces the same [180, 420] range - // and 248 default this key has always had, but is NaN-safe where the bare - // `clamp(parseInt(...))` was not: `Math.max(180, NaN)` is NaN, so a corrupt - // stored value used to decode straight through to `width: NaNpx`, which the - // browser drops — collapsing the sidebar with no way to tell why. - sidebarPx: clampWideWidthPx(parseInt(read.loadStr(KEYS.sidebarPx, String(LEFT_WIDE_DEFAULT_PX)), 10)), + // machine remembers. Same [180, 420] range and 248 default this key has always + // had, but decoded safely: the bare `clamp(parseInt(...))` it replaces was not + // NaN-safe (`Math.max(180, NaN)` is NaN, so a corrupt value reached the DOM as + // `width: NaNpx`, which the browser drops), and `parseInt` also accepted a + // numeric PREFIX, so `'12junk'` silently decoded to 12. `decodeStoredPx` + // requires the whole string to be a finite number before the clamp runs. + sidebarPx: clampWideWidthPx(decodeStoredPx(read.loadStr(KEYS.sidebarPx, ''), LEFT_WIDE_DEFAULT_PX)), editorPct: num(KEYS.editorPct, 45, 15, 85), sideSplitPct: num(KEYS.sideSplitPct, 58, 25, 85), // Cell-detail / rows-viewer drawer width (issue #101). The 92vw upper @@ -813,7 +814,7 @@ export function createState(read: StateReader = { loadJSON, loadStr }): AppState // desktop session shows a bare rail even when rail mode was persisted. leftNavMode: signal(decodeLeftNavigationMode(read.loadStr(KEYS.leftNavMode, 'wide'))), leftNavDrawerPx: clampDrawerWidthPx( - parseInt(read.loadStr(KEYS.leftNavDrawerPx, String(LEFT_DRAWER_DEFAULT_PX)), 10)), + decodeStoredPx(read.loadStr(KEYS.leftNavDrawerPx, ''), LEFT_DRAWER_DEFAULT_PX)), leftNavSection: signal(null), // Best-effort mobile mode (#126). `isMobile` mirrors the viewport width // against MOBILE_BREAKPOINT_PX — set once and on `change` by app.js's diff --git a/tests/unit/left-nav-layout.test.ts b/tests/unit/left-nav-layout.test.ts index 72510a3e..028a468f 100644 --- a/tests/unit/left-nav-layout.test.ts +++ b/tests/unit/left-nav-layout.test.ts @@ -21,9 +21,10 @@ import { LEFT_DRAWER_DEFAULT_PX, LEFT_FOLD_THRESHOLD_PX, LEFT_NAV_LARGE_STEP_PX, LEFT_NAV_SECTIONS, LEFT_NAV_STEP_PX, LEFT_PANEL_MAX_PX, LEFT_PANEL_MIN_PX, LEFT_RAIL_PX, LEFT_WIDE_DEFAULT_PX, LEFT_WIDE_THRESHOLD_PX, - clampDrawerWidthPx, clampWideWidthPx, decodeLeftNavigationMode, effectiveLeftNavigationLayout, - isLeftNavigationSection, leftNavigationLayoutIsCoherent, leftNavigationSeparatorAria, - leftNavigationWidthPx, resolveLeftNavigationDrag, resolveLeftNavigationKey, resolveRailActivation, + clampDrawerWidthPx, clampWideWidthPx, decodeLeftNavigationMode, decodeStoredPx, + effectiveLeftNavigationLayout, isLeftNavigationSection, leftNavigationLayoutIsCoherent, + leftNavigationSeparatorAria, leftNavigationWidthPx, normalizeLeftNavigationLayout, + resolveLeftNavigationDrag, resolveLeftNavigationKey, resolveRailActivation, resolveRailOpen, } from '../../src/core/left-nav-layout.js'; import type { LeftNavigationLayout } from '../../src/core/left-nav-layout.js'; @@ -391,16 +392,16 @@ describe('resolveLeftNavigationKey', () => { rail({ focusedSection: 'history', drawerWidthPx: LEFT_FOLD_THRESHOLD_PX }), { key: 'ArrowLeft' })!; expect(next.focusedSection).toBeNull(); }); - it('leaves a bare rail on ONE rightward step, whatever the remembered width', () => { - // The bare-rail base must be the wide THRESHOLD, not `wideWidthPx`. With the - // remembered width as the base, any value at or below 244 would leave a small - // ArrowRight stuck in the sticky band forever — a separator advertising - // `aria-valuemax: 420` while refusing to move. 180 is trivially reachable by - // dragging the sidebar narrow before folding, so this is not a corner case. + it('leaves a bare rail on ONE rightward step, at the REMEMBERED width', () => { + // A relative +16 from the rail's own 48px would propose 64, land in the sticky + // band and do nothing forever, so the boundary step performs the restore + // transition instead — and at the remembered width, like End. A fixed + // threshold-plus-step base would hand back 276 and silently discard this 420. for (const wideWidthPx of [LEFT_PANEL_MIN_PX, 200, 244, LEFT_WIDE_DEFAULT_PX, LEFT_PANEL_MAX_PX]) { for (const shiftKey of [false, true]) { - expect(resolveLeftNavigationKey(rail({ wideWidthPx }), { key: 'ArrowRight', shiftKey })!.mode) - .toBe('wide'); + const out = resolveLeftNavigationKey(rail({ wideWidthPx }), { key: 'ArrowRight', shiftKey })!; + expect(out.mode).toBe('wide'); + expect(out.wideWidthPx).toBe(wideWidthPx); } } }); @@ -410,15 +411,20 @@ describe('resolveLeftNavigationKey', () => { expect(resolveLeftNavigationKey(stay, { key: 'ArrowLeft', shiftKey })).toBe(stay); } }); - it('matches pointer transitions for the same proposed total width', () => { - // The property the design exists for. The pointer side is driven from the - // module's PUBLIC occupied width, never from a copy of the private base - // formula — recomputing the implementation here is what let a bare-rail base - // bug survive a green suite, so the bare rail (whose base is deliberately the - // threshold, not its own width) is asserted separately above. + it('matches pointer transitions INSIDE a band, for the same proposed total width', () => { + // The property the design exists for: inside a band the keyboard IS the drag + // reducer, so the resize arithmetic has one implementation. The pointer side is + // driven from the module's PUBLIC occupied width, never from a copy of a private + // base formula — recomputing the implementation here is what let a bare-rail + // base bug survive a green suite. + // + // Band EDGES are deliberately excluded and asserted separately: there the + // keyboard performs a semantic transition the pointer reaches by simply + // travelling further, which no single shared proposal can express. A bare rail + // and a 180px sidebar are the two such states. const cases: LeftNavigationLayout[] = [ wide({ wideWidthPx: 300 }), - wide({ wideWidthPx: LEFT_PANEL_MIN_PX }), + wide({ wideWidthPx: LEFT_PANEL_MIN_PX + LEFT_NAV_LARGE_STEP_PX }), wide({ wideWidthPx: LEFT_PANEL_MAX_PX }), rail({ focusedSection: 'databases', drawerWidthPx: LEFT_FOLD_THRESHOLD_PX }), rail({ focusedSection: 'databases', drawerWidthPx: 200 }), @@ -435,16 +441,186 @@ describe('resolveLeftNavigationKey', () => { } } }); - it('is a no-op at the wide floor and ceiling for a small step, and folds on a large one', () => { - // Unlike the bare rail, this is NOT a dead end: the pointer does the same - // thing at these extremes (clientX 164 also leaves a 180 sidebar at 180), and - // Shift+ArrowLeft escapes. Keyboard and pointer agree, which is the contract. - expect(resolveLeftNavigationKey(wide({ wideWidthPx: LEFT_PANEL_MIN_PX }), { key: 'ArrowLeft' })) - .toEqual(wide({ wideWidthPx: LEFT_PANEL_MIN_PX })); + it('folds from the wide floor on a leftward step — the mirror of the bare-rail case', () => { + // The dead end this replaces: at the 180 floor a −16 step proposes 164, which + // clamps back to 180, so plain ArrowLeft did nothing FOREVER while + // `aria-valuemin: 48` was advertised. Home and Shift+Arrow escaping is not a + // defence — the W3C splitter pattern makes plain Left/Right the move keys. + for (const shiftKey of [false, true]) { + expect(resolveLeftNavigationKey( + wide({ wideWidthPx: LEFT_PANEL_MIN_PX }), { key: 'ArrowLeft', shiftKey })!.mode).toBe('rail'); + } + }); + it('holds at the wide ceiling on a rightward step — a real bound, not a dead zone', () => { + // Nothing legal exists to the right of 420, so refusing to move is the correct + // answer rather than a stranded control. expect(resolveLeftNavigationKey(wide({ wideWidthPx: LEFT_PANEL_MAX_PX }), { key: 'ArrowRight' })) .toEqual(wide({ wideWidthPx: LEFT_PANEL_MAX_PX })); - expect(resolveLeftNavigationKey( - wide({ wideWidthPx: LEFT_PANEL_MIN_PX }), { key: 'ArrowLeft', shiftKey: true })!.mode).toBe('rail'); + }); + it('reaches the rail from any wide width by repeated plain ArrowLeft', () => { + // The property the single-step tests could not express: a keyboard SEQUENCE + // must be able to go where the equivalent pointer path goes. Eleven presses + // from 300 used to sit at 180 forever while the pointer folded. + for (const start of [LEFT_PANEL_MAX_PX, 300, 190, LEFT_PANEL_MIN_PX]) { + let layout: LeftNavigationLayout = wide({ wideWidthPx: start }); + for (let i = 0; i < 40 && layout.mode === 'wide'; i++) { + layout = resolveLeftNavigationKey(layout, { key: 'ArrowLeft' })!; + } + expect(layout.mode).toBe('rail'); + } + }); + it('round-trips between rail and wide with plain arrows, in both directions', () => { + // Reversibility of the MODE, which a stranded boundary silently broke. The width + // does not round-trip, and should not: the intervening ArrowLefts really did + // resize the sidebar down to its floor before folding, so 180 is the honest + // remembered width on the way back. + const start = rail({ wideWidthPx: 300 }); + expect(resolveLeftNavigationKey(start, { key: 'ArrowRight' })).toEqual(wide({ wideWidthPx: 300 })); + let back: LeftNavigationLayout = wide({ wideWidthPx: 300 }); + for (let i = 0; i < 40 && back.mode === 'wide'; i++) { + back = resolveLeftNavigationKey(back, { key: 'ArrowLeft' })!; + } + expect(back).toEqual(rail({ wideWidthPx: LEFT_PANEL_MIN_PX })); + // And straight back out again, so neither end is a trap. + expect(resolveLeftNavigationKey(back, { key: 'ArrowRight' })!.mode).toBe('wide'); + }); + it('normalizes an incoherent layout before acting on it', () => { + const healed = resolveLeftNavigationKey( + wide({ focusedSection: 'databases' }) as LeftNavigationLayout, { key: 'ArrowRight' })!; + expect(healed.focusedSection).toBeNull(); + expect(resolveLeftNavigationKey(wide({ wideWidthPx: NaN }), { key: 'End' })!.wideWidthPx) + .toBe(LEFT_WIDE_DEFAULT_PX); + }); +}); + +// #487 requires "a deterministic `openFocusedSection('dashboards')` seam" for +// #428's bounded drag-hover. A toggle cannot serve that: hover re-asserts intent +// repeatedly, so a toggle would flap the drawer open and shut on alternate +// notifications. Open and toggle are therefore separate operations. +describe('resolveRailOpen — the idempotent seam', () => { + it('opens a section from a bare rail', () => { + expect(resolveRailOpen(rail(), 'dashboards')).toEqual(rail({ focusedSection: 'dashboards' })); + }); + it('is IDEMPOTENT — repeated opens leave the section open', () => { + // The exact #428 failure mode this exists to prevent. + let layout: LeftNavigationLayout = rail(); + for (let i = 0; i < 5; i++) layout = resolveRailOpen(layout, 'dashboards'); + expect(layout.focusedSection).toBe('dashboards'); + // …and returns by identity once already open, so a hover notification storm + // cannot cause a repaint per event. + expect(resolveRailOpen(layout, 'dashboards')).toBe(layout); + }); + it('switches from another open section without closing first', () => { + expect(resolveRailOpen(rail({ focusedSection: 'history' }), 'dashboards').focusedSection) + .toBe('dashboards'); + }); + it('never closes a drawer, unlike the click toggle', () => { + const open = rail({ focusedSection: 'dashboards' }); + expect(resolveRailOpen(open, 'dashboards').focusedSection).toBe('dashboards'); + expect(resolveRailActivation(open, 'dashboards').focusedSection).toBeNull(); + }); + it('is a no-op in wide mode, like the toggle', () => { + const layout = wide(); + expect(resolveRailOpen(layout, 'dashboards')).toBe(layout); + }); +}); + +describe('normalizeLeftNavigationLayout', () => { + it('returns a legal layout by identity', () => { + for (const layout of [wide(), rail(), rail({ focusedSection: 'library' })]) { + expect(normalizeLeftNavigationLayout(layout)).toBe(layout); + } + }); + it('drops a focused section that wide mode cannot render', () => { + expect(normalizeLeftNavigationLayout(wide({ focusedSection: 'databases' }))) + .toEqual(wide()); + }); + it('heals a non-finite or out-of-band width', () => { + expect(normalizeLeftNavigationLayout(wide({ wideWidthPx: NaN })).wideWidthPx) + .toBe(LEFT_WIDE_DEFAULT_PX); + expect(normalizeLeftNavigationLayout(wide({ wideWidthPx: 9999 })).wideWidthPx) + .toBe(LEFT_PANEL_MAX_PX); + expect(normalizeLeftNavigationLayout(wide({ drawerWidthPx: 9999 })).drawerWidthPx) + .toBe(LEFT_WIDE_THRESHOLD_PX); + }); + it('rejects an unknown mode and an unknown section', () => { + expect(normalizeLeftNavigationLayout({ ...wide(), mode: 'collapsed' } as unknown as LeftNavigationLayout).mode) + .toBe('wide'); + expect(normalizeLeftNavigationLayout( + { ...rail(), focusedSection: 'saved' } as unknown as LeftNavigationLayout).focusedSection).toBeNull(); + }); + it('makes every reducer heal an incoherent seed rather than preserve it', () => { + // Previously the invariant was only a PRECONDITION: a drag over an incoherent + // layout carried the illegal mode/section pair straight through, and End handed + // it back untouched. `state.ts` stores the two as independently writable + // signals, so that pair is one stray assignment away. + const bad = wide({ focusedSection: 'databases' }); + expect(leftNavigationLayoutIsCoherent(bad)).toBe(false); + expect(resolveLeftNavigationDrag(bad, 300).focusedSection).toBeNull(); + expect(resolveLeftNavigationKey(bad, { key: 'End' })!.focusedSection).toBeNull(); + expect(resolveRailActivation(bad, 'databases').focusedSection).toBeNull(); + expect(resolveRailOpen(bad, 'databases').focusedSection).toBeNull(); + expect(effectiveLeftNavigationLayout(bad, false).focusedSection).toBeNull(); + }); + it('keeps a NaN width out of the ARIA value published to assistive technology', () => { + expect(leftNavigationSeparatorAria(wide({ wideWidthPx: NaN })).valueNow) + .toBe(LEFT_WIDE_DEFAULT_PX); + }); + it('lifts a type-valid but illegal seed into its band before measuring', () => { + // A 150px "wide" sidebar is type-valid and state-invalid. Normalizing on entry + // means the sweep is measured from a legal 180 rather than reporting an occupied + // width no mode can render. + expect(normalizeLeftNavigationLayout(wide({ wideWidthPx: 150 })).wideWidthPx) + .toBe(LEFT_PANEL_MIN_PX); + // 149 is still above the fold threshold, so it resizes to the floor … + expect(resolveLeftNavigationDrag(wide({ wideWidthPx: 150 }), 149)) + .toEqual(wide({ wideWidthPx: LEFT_PANEL_MIN_PX })); + // … and only a proposal past the threshold folds. + expect(resolveLeftNavigationDrag(wide({ wideWidthPx: 150 }), LEFT_FOLD_THRESHOLD_PX - 1).mode) + .toBe('rail'); + }); +}); + +describe('decodeStoredPx', () => { + it('accepts a complete number, with surrounding whitespace', () => { + expect(decodeStoredPx('300', 1)).toBe(300); + expect(decodeStoredPx(' 300 ', 1)).toBe(300); + expect(decodeStoredPx('300.5', 1)).toBe(300.5); + expect(decodeStoredPx('-5', 1)).toBe(-5); + }); + it('rejects a numeric PREFIX, which parseInt would have accepted', () => { + // The contract this fixes: `parseInt('12junk')` is 12 and `parseInt('200px')` + // is 200, so a truncated write or a hand-edited CSS unit decoded to a + // plausible-looking width while the docs promised the default. + expect(decodeStoredPx('12junk', 248)).toBe(248); + expect(decodeStoredPx('200px', 240)).toBe(240); + expect(decodeStoredPx('1e', 248)).toBe(248); + }); + it('rejects a stored infinity, empty string, whitespace and every non-string', () => { + expect(decodeStoredPx('Infinity', 248)).toBe(248); + expect(decodeStoredPx('-Infinity', 248)).toBe(248); + expect(decodeStoredPx('NaN', 248)).toBe(248); + expect(decodeStoredPx('', 248)).toBe(248); + expect(decodeStoredPx(' ', 248)).toBe(248); + expect(decodeStoredPx(null, 248)).toBe(248); + expect(decodeStoredPx(undefined, 248)).toBe(248); + expect(decodeStoredPx(300, 248)).toBe(248); + }); +}); + +// 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. +// Separating them needs a drag-session snapshot, which a pure reducer cannot take. +describe('restore memory is sampling-dependent (phase 3 obligation)', () => { + it('remembers a different width for the same gesture depending on event cadence', () => { + const seed = wide({ wideWidthPx: 300 }); + // One coarse sample straight past the fold keeps the pre-drag width … + expect(resolveLeftNavigationDrag(seed, 139).wideWidthPx).toBe(300); + // … while an intermediate sample inside the 140–179 dead zone rests the width + // at the floor first, so the fold remembers 180 instead. + expect(resolveLeftNavigationDrag(resolveLeftNavigationDrag(seed, 179), 139).wideWidthPx) + .toBe(LEFT_PANEL_MIN_PX); }); }); diff --git a/tests/unit/state.test.ts b/tests/unit/state.test.ts index ad28cbd6..5d82e091 100644 --- a/tests/unit/state.test.ts +++ b/tests/unit/state.test.ts @@ -292,6 +292,23 @@ describe('createState — left navigation preferences (#487)', () => { expect(s.sidebarPx).toBe(248); }); + it('rejects a partially numeric stored width instead of trusting its prefix', () => { + // `parseInt` accepted a numeric PREFIX, so a truncated write or a hand-edited + // CSS unit decoded to a plausible-looking width while the docs promised the + // default: `parseInt('12junk')` is 12 (then clamped to 180) and + // `parseInt('200px')` is 200 (accepted outright). + const s = createState(reader({ + [KEYS.sidebarPx]: '12junk', + [KEYS.leftNavDrawerPx]: '200px', + })); + expect(s.sidebarPx).toBe(248); + expect(s.leftNavDrawerPx).toBe(240); + // A stored infinity is corruption too, not a width pressed against a bound. + expect(createState(reader({ [KEYS.sidebarPx]: 'Infinity' })).sidebarPx).toBe(248); + // …while a well-formed value still decodes, whitespace and decimals included. + expect(createState(reader({ [KEYS.sidebarPx]: ' 330 ' })).sidebarPx).toBe(330); + }); + it('clamps an out-of-range drawer width into the drawer own band', () => { // Not the wide sidebar's [180, 420]: a drawer wider than the wide threshold // is unreachable, because a drag that far right converts to the sidebar.