diff --git a/CHANGELOG.md b/CHANGELOG.md index ae7999ca..4e0e25e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,40 @@ 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. 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 + 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 +59,20 @@ 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, 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 malformed value (a real drag always carries a finite `clientX`), so + reaching it takes a hand-edited or foreign-origin `localStorage` entry. The + 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/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..edf84c1b --- /dev/null +++ b/src/core/left-nav-layout.ts @@ -0,0 +1,497 @@ +// 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); +} + +/** + * 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 normalizeLeftNavigationLayout(layout) === layout; +} + +/** 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); +} + +/** + * 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`. + * '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( + 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 + // 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; +} + +/** 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 }; +} + +/** + * 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. + * + * **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: + * + * - 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( + input: LeftNavigationLayout, event: LeftNavigationKey, +): LeftNavigationLayout | null { + if (event.ctrlKey || event.metaKey || event.altKey) return 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; + } + // 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); + } + const step = event.shiftKey ? LEFT_NAV_LARGE_STEP_PX : LEFT_NAV_STEP_PX; + return resolveLeftNavigationDrag(layout, leftNavigationWidthPx(layout) + (towardWide ? step : -step)); +} + +/** + * 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 + * 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( + 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 + * 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( + input: LeftNavigationLayout, isMobile: boolean, +): LeftNavigationLayout { + const layout = normalizeLeftNavigationLayout(input); + if (!isMobile) return layout; + 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 + * 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, + // 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 150de7f6..94d4811b 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, decodeStoredPx, +} 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,14 @@ 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. 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 @@ -762,6 +805,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( + 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 // 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..028a468f --- /dev/null +++ b/tests/unit/left-nav-layout.test.ts @@ -0,0 +1,737 @@ +// #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, 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'; + +/** 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, 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]) { + const out = resolveLeftNavigationKey(rail({ wideWidthPx }), { key: 'ArrowRight', shiftKey })!; + expect(out.mode).toBe('wide'); + expect(out.wideWidthPx).toBe(wideWidthPx); + } + } + }); + 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 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 + LEFT_NAV_LARGE_STEP_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('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 })); + }); + 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); + }); +}); + +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..5d82e091 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,106 @@ 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('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. + 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 });