From a1bea7df1ff14d92004bf92be6daeb3151b75540 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Thu, 30 Jul 2026 09:46:49 +0200 Subject: [PATCH 01/13] feat(#487): left-nav resize session and centre-width clamp (phase 3, step 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the pure core pieces phase 3 needs before any UI wiring: a resize session (begin/advance/commit) that fixes the sampling-dependent restore memory from phase 1 for both bands without reintroducing it (two rounds of adversarial review caught that "continuously updated" memory has the same bug relocated), and clampLeftNavigationToMaximumTotal + the viewport- aware separator ARIA ceiling for the centre-width safety constraint. No production consumer yet — wiring lands in later steps of this phase. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NZfeGpkpJw2zsfPtysGHUN --- src/core/left-nav-layout.ts | 221 ++++++++++++++++++++++++++- tests/unit/left-nav-layout.test.ts | 236 ++++++++++++++++++++++++++++- 2 files changed, 451 insertions(+), 6 deletions(-) diff --git a/src/core/left-nav-layout.ts b/src/core/left-nav-layout.ts index 436fd99..c6b2c2f 100644 --- a/src/core/left-nav-layout.ts +++ b/src/core/left-nav-layout.ts @@ -524,12 +524,229 @@ export interface LeftNavigationSeparatorAria { readonly valueNow: number; } -export function leftNavigationSeparatorAria(layout: LeftNavigationLayout): LeftNavigationSeparatorAria { +/** + * `maxNavigationTotalPx` is optional and, when given, tightens `valueMax` to + * whatever the viewport currently allows — see `clampLeftNavigationToMaximumTotal` + * below, which computes the layout this ceiling must agree with. Omitted (or a + * non-finite/non-positive value, which cannot be a real viewport budget), the + * ceiling is `LEFT_PANEL_MAX_PX` exactly as before phase 3 — this parameter is + * additive and every existing caller (there is still none in production, but the + * unit tests below stand in for one) keeps its prior behaviour unconditionally. + */ +export function leftNavigationSeparatorAria( + layout: LeftNavigationLayout, maxNavigationTotalPx?: number, +): LeftNavigationSeparatorAria { + const valueMax = Number.isFinite(maxNavigationTotalPx) && (maxNavigationTotalPx as number) > 0 + ? Math.min(LEFT_PANEL_MAX_PX, maxNavigationTotalPx as number) + : LEFT_PANEL_MAX_PX; return { valueMin: LEFT_RAIL_PX, - valueMax: LEFT_PANEL_MAX_PX, + valueMax, // Normalized, so a caller holding a layout with a non-finite width cannot // publish `aria-valuenow="NaN"` to assistive technology. valueNow: leftNavigationWidthPx(normalizeLeftNavigationLayout(layout)), }; } + +/** + * The centre SQL/results surface's documented minimum width. Phase 4's + * viewport-resize handling subtracts this (plus the resize separator's own + * width and any docked panel) from the viewport to get the budget it hands to + * `clampLeftNavigationToMaximumTotal` below — this module does not read the + * viewport itself, so the constant lives here purely so the UI layer has one + * source for it rather than a second copy of the number. + */ +export const LEFT_CENTRE_MIN_PX = 480; + +/** + * Shrink `layout`'s CURRENT mode's own panel width so the navigation's total + * occupied width (`leftNavigationWidthPx`) fits inside `maxNavigationTotalPx`, + * without ever changing `mode`. + * + * This is a narrower job than the mode reducer above. #487 phase 4 is where a + * viewport too small for the centre surface's own minimum gets to fold a wide + * sidebar to rail or close an open drawer — a MODE decision, driven by the + * viewport crossing a breakpoint, not by a pointer or a key. Doing any of that + * here would duplicate `resolveLeftNavigationDrag`'s job with a second, + * viewport-shaped entry point, and the two would disagree about hysteresis the + * first time someone edited only one of them. So this function answers a + * strictly smaller question — "shrink the width THIS mode already has, inside + * the band this mode can already legally occupy" — and leaves whether to fold + * or close entirely to phase 4. + * + * That narrower scope is also why a budget below the mode's own floor is + * "best effort" rather than an error: `wide`'s floor is `LEFT_PANEL_MIN_PX` + * (180) and a bare `rail`'s occupied width is the fixed `LEFT_RAIL_PX` (48, + * nothing to shrink), so the only floor this function can be asked to violate + * is the wide sidebar's 180 or an open drawer's 140. Returning the floor + * anyway — rather than throwing, or returning something outside every mode's + * legal range — keeps the result always renderable; a caller that actually + * needs the navigation narrower than what a mode CAN render must change the + * mode, which is phase 4's job, not this function's. + * + * In practice that floor path is unreachable above the existing mobile + * breakpoint, so it is defensive rather than a state phase 4 has to design + * for today. `MOBILE_BREAKPOINT_PX` (`state.ts`) is 768, the resize separator + * that will subtract from the viewport is 7px wide (`.col-resize` in + * `styles.css`), and `LEFT_CENTRE_MIN_PX` above is 480 — so at the smallest + * viewport this function is ever consulted at (768px wide, the boundary where + * mobile's own two-pane layout stops standing in), the budget phase 4 would + * pass is `768 − 480 − 7 = 281`. That clears the wide sidebar's 180 floor with + * 101px to spare, and clears an open drawer's `281 − LEFT_RAIL_PX(48) = 233` + * against its 140 floor with 93px to spare. Below 768 the mobile layout + * replaces the rail and drawer entirely (`effectiveLeftNavigationLayout` + * above), so this function is never consulted there either. + * + * `maxNavigationTotalPx` itself is a plain number budget, not a viewport: a + * non-finite or NEGATIVE value (a corrupt measurement, or a caller that has + * not measured yet) is treated as "no additional constraint" rather than + * propagated into a NaN or Infinity output. Zero is not in that list — it is + * an extreme but legitimate budget, and clamping into either band's own range + * floors it exactly like any other too-small value. + */ +export function clampLeftNavigationToMaximumTotal( + layout: LeftNavigationLayout, maxNavigationTotalPx: number, +): LeftNavigationLayout { + const normalized = normalizeLeftNavigationLayout(layout); + // Zero is a legitimate (if extreme) budget — clamping into either band's own + // range floors it correctly. Only a NEGATIVE or non-finite value cannot mean + // a real width, so those fall back to "no additional constraint" rather than + // being clamped into a nonsensical floor. + const budget = Number.isFinite(maxNavigationTotalPx) && maxNavigationTotalPx >= 0 + ? maxNavigationTotalPx + : Infinity; + if (normalized.mode === 'wide') { + // The wide sidebar IS the total, so the budget applies to it directly. + const maxWideWidthPx = clamp(budget, LEFT_PANEL_MIN_PX, LEFT_PANEL_MAX_PX); + return normalized.wideWidthPx <= maxWideWidthPx + ? normalized + : { ...normalized, wideWidthPx: maxWideWidthPx }; + } + if (normalized.focusedSection === null) return normalized; // bare rail: fixed width, nothing to shrink. + // An open drawer sits BESIDE the rail, so its own budget is the total minus + // the rail — mirroring `resolveLeftNavigationDrag`'s `panelPx` derivation. + const maxDrawerWidthPx = clamp(budget - LEFT_RAIL_PX, LEFT_FOLD_THRESHOLD_PX, LEFT_WIDE_THRESHOLD_PX); + return normalized.drawerWidthPx <= maxDrawerWidthPx + ? normalized + : { ...normalized, drawerWidthPx: maxDrawerWidthPx }; +} + +/** + * A resize SESSION — the drag/keyboard-resize memory phase 3 needs and a plain + * continuously-advancing width field cannot provide. + * + * **Why not just keep overwriting a remembered width on every frame?** That was + * tried and is exactly the bug this module's own + * "restore memory is sampling-dependent" test (above) already pins for the OLD + * design: which width ends up remembered depends on which intermediate pointer + * samples the browser happened to deliver, because one field was doing duty as + * both "the width currently on screen" and "the width to restore later". A + * session separates those two questions by keeping THREE layouts, not one: + * + * - `preferredAtStart` — the persisted preference as of when the gesture began. + * This is the memory source `commitLeftNavigationResize` reconstructs from, + * and it is captured ONCE, so no intermediate frame can overwrite it. + * - `effectiveAtStart` — what was actually rendered when the gesture began, + * i.e. `preferredAtStart` after `clampLeftNavigationToMaximumTotal` ran + * against whatever the viewport allowed at that moment. This can differ from + * `preferredAtStart` — a maximized preference squeezed by a narrow window — + * and the gap between the two is precisely what lets the commit step tell + * "the user actually resized this band" apart from "the band was just + * rendered smaller than preferred by an unrelated viewport constraint". + * - `effective` — the live, post-clamp layout, replaced wholesale on every + * `advanceLeftNavigationResize` call. This is what gets rendered and reported + * as the gesture continues; it is not memory. + * + * **`commitLeftNavigationResize`'s table, restated as one rule:** only commit a + * band's width if that band is the one `effective` currently renders AND its + * rendered width actually differs from `effectiveAtStart`'s. Every other case — + * a dormant band, a fold-through to bare rail, a click-and-release with no + * movement — preserves `preferredAtStart` for that band UNCONDITIONALLY. Two + * consequences fall out of that one rule rather than needing their own case: + * + * 1. **The dormant-band fix.** A gesture that resizes the drawer and THEN + * crosses into wide mode must not commit the drawer's mid-gesture width, + * because the drawer is no longer the band `effective` renders once the + * session ends — `drawerChanged` requires `effective.mode === 'rail'`, which + * is false at wide, so the drawer memory falls through to + * `preferredAtStart.drawerWidthPx` untouched. Without that mode guard, a + * drag that opened the drawer to 300 before continuing on to a 350px wide + * sidebar would silently overwrite the drawer's remembered width with a + * value the user never asked to keep. + * 2. **Preferred wins over effective on a fold-through.** Ending at bare rail + * preserves BOTH widths from `preferredAtStart`, never from `effectiveAtStart` + * or `effective` — so a maximized 420px preference that a narrow viewport + * rendered at a clamped 313px, then folded to rail by the same gesture, + * still remembers 420 for the next `End`/restore. Committing `313` instead + * would silently downgrade a preference the user never touched, purely + * because the viewport happened to be narrow during an unrelated fold. + * + * `Home`/`End`/a bare-rail `ArrowRight` restore need no special case either: + * they are restore commands, so the `effective` layout they produce typically + * already equals `preferredAtStart`'s remembered width for the band they + * restore, which is exactly the "nothing changed" shape the general rule + * preserves correctly. + * + * A session is deliberately NOT a reducer step in `resolveLeftNavigationDrag`'s + * family — `advanceLeftNavigationResize` does not call the mode reducer or the + * maximum-total clamp itself. The caller runs those first to produce the next + * `effective` layout (a pointer/keyboard event resolves through the existing + * reducers, then `clampLeftNavigationToMaximumTotal` fits it to the viewport), + * and only then advances the session with the result. Session bookkeeping and + * layout arithmetic stay two separate concerns, so the arithmetic keeps its + * one implementation. + */ +export interface LeftNavigationResizeSession { + /** The persisted preference as of session start — the memory source every + * commit is reconstructed from, band by band. */ + readonly preferredAtStart: LeftNavigationLayout; + /** What was actually rendered when the session began, i.e. + * `preferredAtStart` after the viewport's maximum-total clamp. */ + readonly effectiveAtStart: LeftNavigationLayout; + /** The live, post-clamp layout — what is rendered and reported right now. */ + readonly effective: LeftNavigationLayout; +} + +/** Begin a resize session: `effective` starts out equal to `effectiveAtStart`, + * since nothing has moved yet. */ +export function beginLeftNavigationResize( + preferred: LeftNavigationLayout, effective: LeftNavigationLayout, +): LeftNavigationResizeSession { + return { preferredAtStart: preferred, effectiveAtStart: effective, effective }; +} + +/** + * Advance a session to a new live layout. Pure snapshot replacement — the + * caller has already run the layout through `resolveLeftNavigationDrag` / + * `resolveLeftNavigationKey` and `clampLeftNavigationToMaximumTotal` to produce + * `nextEffectiveLayout`; this function does not call either. Returns the SAME + * session when the layout is unchanged by reference, so a caller can use + * identity to skip a repaint exactly as the mode reducers do. + */ +export function advanceLeftNavigationResize( + session: LeftNavigationResizeSession, nextEffectiveLayout: LeftNavigationLayout, +): LeftNavigationResizeSession { + return nextEffectiveLayout === session.effective + ? session + : { ...session, effective: nextEffectiveLayout }; +} + +/** + * Reconstruct the `LeftNavigationLayout` to persist from a resize session — see + * this section's block comment above for the rule and why it is shaped this + * way. `mode` and `focusedSection` always follow wherever the session ended + * (a legitimate mode transition, not a width memory question); only the two + * WIDTHS get the preserve-vs-commit treatment, band by band. + */ +export function commitLeftNavigationResize(session: LeftNavigationResizeSession): LeftNavigationLayout { + const { preferredAtStart, effectiveAtStart, effective } = session; + const wideChanged = effective.mode === 'wide' && effective.wideWidthPx !== effectiveAtStart.wideWidthPx; + const drawerChanged = effective.mode === 'rail' && effective.focusedSection !== null + && effective.drawerWidthPx !== effectiveAtStart.drawerWidthPx; + return normalizeLeftNavigationLayout({ + mode: effective.mode, + focusedSection: effective.focusedSection, + wideWidthPx: wideChanged ? effective.wideWidthPx : preferredAtStart.wideWidthPx, + drawerWidthPx: drawerChanged ? effective.drawerWidthPx : preferredAtStart.drawerWidthPx, + }); +} diff --git a/tests/unit/left-nav-layout.test.ts b/tests/unit/left-nav-layout.test.ts index d393fb2..28737cc 100644 --- a/tests/unit/left-nav-layout.test.ts +++ b/tests/unit/left-nav-layout.test.ts @@ -18,10 +18,12 @@ 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, + LEFT_CENTRE_MIN_PX, 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, + advanceLeftNavigationResize, beginLeftNavigationResize, clampDrawerWidthPx, + clampLeftNavigationToMaximumTotal, clampWideWidthPx, commitLeftNavigationResize, + decodeLeftNavigationMode, decodeStoredPx, effectiveLeftNavigationLayout, isLeftNavigationSection, leftNavigationLayoutIsCoherent, leftNavigationSeparatorAria, leftNavigationWidthPx, normalizeLeftNavigationLayout, resolveLeftNavigationDrag, resolveLeftNavigationKey, resolveRailActivation, resolveRailOpen, @@ -770,4 +772,230 @@ describe('leftNavigationSeparatorAria', () => { expect(valueNow).toBeLessThanOrEqual(valueMax); } }); + + // #487 phase 3: an optional ceiling, added without disturbing any existing + // caller — this is the regression the "omitted" case guards. + describe('the optional maxNavigationTotalPx ceiling (#487 phase 3)', () => { + it('is unchanged from before phase 3 when the parameter is omitted', () => { + // Would fail if a phase-3 edit accidentally tightened the default ceiling. + expect(leftNavigationSeparatorAria(wide({ wideWidthPx: 300 }))) + .toEqual({ valueMin: LEFT_RAIL_PX, valueMax: LEFT_PANEL_MAX_PX, valueNow: 300 }); + }); + it('ignores a non-finite or non-positive ceiling, exactly like omitting it', () => { + for (const bogus of [NaN, Infinity, -Infinity, 0, -100]) { + expect(leftNavigationSeparatorAria(wide({ wideWidthPx: 300 }), bogus).valueMax) + .toBe(LEFT_PANEL_MAX_PX); + } + }); + it('shrinks valueMax to a smaller ceiling', () => { + expect(leftNavigationSeparatorAria(wide({ wideWidthPx: 200 }), 300).valueMax).toBe(300); + }); + it('has no effect when the ceiling exceeds LEFT_PANEL_MAX_PX', () => { + expect(leftNavigationSeparatorAria(wide({ wideWidthPx: 200 }), 9999).valueMax) + .toBe(LEFT_PANEL_MAX_PX); + }); + }); +}); + +describe('clampLeftNavigationToMaximumTotal (#487 phase 3)', () => { + it('shrinks a wide layout that exceeds the budget', () => { + const next = clampLeftNavigationToMaximumTotal(wide({ wideWidthPx: 350 }), 300); + expect(next.mode).toBe('wide'); + expect(next.wideWidthPx).toBe(300); + }); + it('shrinks a drawer layout that exceeds the budget, measured beside the rail', () => { + const next = clampLeftNavigationToMaximumTotal( + rail({ focusedSection: 'library', drawerWidthPx: 220 }), LEFT_RAIL_PX + 180); + expect(next.mode).toBe('rail'); + expect(next.focusedSection).toBe('library'); + expect(next.drawerWidthPx).toBe(180); + }); + it('never changes mode, including for a bare rail (nothing to shrink)', () => { + expect(clampLeftNavigationToMaximumTotal(rail(), 0).mode).toBe('rail'); + expect(clampLeftNavigationToMaximumTotal(rail(), 0)).toEqual(rail()); + expect(clampLeftNavigationToMaximumTotal(wide({ wideWidthPx: 350 }), 0).mode).toBe('wide'); + expect(clampLeftNavigationToMaximumTotal( + rail({ focusedSection: 'library', drawerWidthPx: 220 }), 0).mode).toBe('rail'); + }); + it("returns wide's own floor, not NaN or a thrown error, for a budget below it", () => { + expect(clampLeftNavigationToMaximumTotal(wide({ wideWidthPx: 350 }), 0).wideWidthPx) + .toBe(LEFT_PANEL_MIN_PX); + expect(clampLeftNavigationToMaximumTotal(wide({ wideWidthPx: 350 }), LEFT_PANEL_MIN_PX - 1).wideWidthPx) + .toBe(LEFT_PANEL_MIN_PX); + }); + it("returns the drawer's own floor for a budget below it", () => { + const next = clampLeftNavigationToMaximumTotal( + rail({ focusedSection: 'library', drawerWidthPx: 220 }), LEFT_RAIL_PX + LEFT_FOLD_THRESHOLD_PX - 1); + expect(next.drawerWidthPx).toBe(LEFT_FOLD_THRESHOLD_PX); + }); + it('is exact at the floor boundary — the floor itself still fits', () => { + expect(clampLeftNavigationToMaximumTotal(wide({ wideWidthPx: 350 }), LEFT_PANEL_MIN_PX).wideWidthPx) + .toBe(LEFT_PANEL_MIN_PX); + const next = clampLeftNavigationToMaximumTotal( + rail({ focusedSection: 'library', drawerWidthPx: 220 }), LEFT_RAIL_PX + LEFT_FOLD_THRESHOLD_PX); + expect(next.drawerWidthPx).toBe(LEFT_FOLD_THRESHOLD_PX); + }); + it('does not propagate NaN or Infinity for a non-finite or negative budget', () => { + for (const bogus of [NaN, Infinity, -Infinity, -1, -9999]) { + const wideResult = clampLeftNavigationToMaximumTotal(wide({ wideWidthPx: 300 }), bogus); + expect(Number.isFinite(wideResult.wideWidthPx)).toBe(true); + const drawerResult = clampLeftNavigationToMaximumTotal( + rail({ focusedSection: 'library', drawerWidthPx: 200 }), bogus); + expect(Number.isFinite(drawerResult.drawerWidthPx)).toBe(true); + } + }); + it('treats a non-finite or negative budget as no additional constraint', () => { + // Falls back to the mode's own existing band, not an artificially shrunk one. + for (const bogus of [NaN, Infinity, -Infinity, -1]) { + expect(clampLeftNavigationToMaximumTotal(wide({ wideWidthPx: 300 }), bogus).wideWidthPx).toBe(300); + } + }); + it('is a no-op — by identity — when the layout already fits the budget', () => { + const w = wide({ wideWidthPx: 300 }); + expect(clampLeftNavigationToMaximumTotal(w, 9999)).toBe(w); + const d = rail({ focusedSection: 'library', drawerWidthPx: 200 }); + expect(clampLeftNavigationToMaximumTotal(d, 9999)).toBe(d); + const b = rail(); + expect(clampLeftNavigationToMaximumTotal(b, 0)).toBe(b); + }); + it("LEFT_CENTRE_MIN_PX is exported for the UI layer's budget arithmetic", () => { + expect(LEFT_CENTRE_MIN_PX).toBe(480); + }); +}); + +// The resize-session design's own comment block (above `LeftNavigationResizeSession` +// in left-nav-layout.ts) explains WHY it is shaped the way it is; these tests are +// the counter-examples that shape was reviewed against. +describe('resize session (#487 phase 3)', () => { + it('begin captures both inputs, with effective starting equal to effectiveAtStart', () => { + const preferred = wide({ wideWidthPx: 420 }); + const effective = wide({ wideWidthPx: 313 }); // squeezed by a narrow viewport + const session = beginLeftNavigationResize(preferred, effective); + expect(session.preferredAtStart).toBe(preferred); + expect(session.effectiveAtStart).toBe(effective); + expect(session.effective).toBe(effective); + }); + + it('advance replaces effective and returns the same session by reference when unchanged', () => { + const start = wide({ wideWidthPx: 300 }); + const session = beginLeftNavigationResize(start, start); + const same = advanceLeftNavigationResize(session, start); + expect(same).toBe(session); + const moved = wide({ wideWidthPx: 320 }); + const advanced = advanceLeftNavigationResize(session, moved); + expect(advanced).not.toBe(session); + expect(advanced.effective).toBe(moved); + expect(advanced.preferredAtStart).toBe(session.preferredAtStart); + expect(advanced.effectiveAtStart).toBe(session.effectiveAtStart); + }); + + // Each row of commitLeftNavigationResize's table, driven through a single + // begin -> advance -> commit step. + describe('commit table', () => { + it('a wide resize that changed width commits it, leaving the drawer memory untouched', () => { + const start = wide({ wideWidthPx: 250, drawerWidthPx: 210 }); + let session = beginLeftNavigationResize(start, start); + session = advanceLeftNavigationResize(session, wide({ wideWidthPx: 320, drawerWidthPx: 210 })); + expect(commitLeftNavigationResize(session)).toEqual(wide({ wideWidthPx: 320, drawerWidthPx: 210 })); + }); + + it('a drawer resize that changed width commits it, leaving the wide memory untouched', () => { + const start = rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 200 }); + let session = beginLeftNavigationResize(start, start); + session = advanceLeftNavigationResize( + session, rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 230 })); + expect(commitLeftNavigationResize(session)).toEqual( + rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 230 })); + }); + + it('a fold-through to bare rail preserves both remembered widths', () => { + const start = wide({ wideWidthPx: 300, drawerWidthPx: 210 }); + let session = beginLeftNavigationResize(start, start); + session = advanceLeftNavigationResize(session, resolveLeftNavigationDrag(start, 10)); + const committed = commitLeftNavigationResize(session); + expect(committed.mode).toBe('rail'); + expect(committed.focusedSection).toBeNull(); + expect(committed.wideWidthPx).toBe(300); + expect(committed.drawerWidthPx).toBe(210); + }); + + it('a no-op (advance to the same effective layout) commits both preserved', () => { + const start = wide({ wideWidthPx: 300, drawerWidthPx: 210 }); + let session = beginLeftNavigationResize(start, start); + session = advanceLeftNavigationResize(session, wide({ wideWidthPx: 300, drawerWidthPx: 210 })); + expect(commitLeftNavigationResize(session)).toEqual(start); + }); + }); + + it('the dormant-band case: a drawer resize followed by a crossing into wide does not commit the transient drawer width', () => { + const start = rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 200 }); + let session = beginLeftNavigationResize(start, start); + // Resize the drawer open further … + session = advanceLeftNavigationResize( + session, rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 259 })); + // … then keep dragging past the wide threshold, converting to the wide sidebar. + const wideLayout = resolveLeftNavigationDrag(session.effective, LEFT_RAIL_PX + LEFT_WIDE_THRESHOLD_PX + 40); + session = advanceLeftNavigationResize(session, wideLayout); + const committed = commitLeftNavigationResize(session); + expect(committed.mode).toBe('wide'); + // The wide memory reflects where the session actually ended. + expect(committed.wideWidthPx).toBe(wideLayout.wideWidthPx); + // The drawer memory is the ORIGINAL preferred value, not the 259px it + // transiently passed through mid-session. + expect(committed.drawerWidthPx).toBe(200); + }); + + it('the preferred/effective divergence case: a fold-through commits the PREFERRED wide width, not the viewport-clamped effective one', () => { + const preferred = wide({ wideWidthPx: 420, drawerWidthPx: 210 }); + // The viewport at session start could only render 313px, well inside the + // legal wide band, so this is a legitimate effectiveAtStart on its own. + const effectiveAtStart = wide({ wideWidthPx: 313, drawerWidthPx: 210 }); + let session = beginLeftNavigationResize(preferred, effectiveAtStart); + // The gesture keeps going left and folds all the way to bare rail. + session = advanceLeftNavigationResize(session, resolveLeftNavigationDrag(effectiveAtStart, 10)); + const committed = commitLeftNavigationResize(session); + expect(committed.mode).toBe('rail'); + // 420, the PREFERRED value — not 313, the clamped value the session actually + // rendered at the start. + expect(committed.wideWidthPx).toBe(420); + expect(committed.drawerWidthPx).toBe(210); + }); + + it('a dense sweep and a coarse sweep over the same drag commit the same result — even though the underlying reducer state they pass through provably disagrees', () => { + // This is the exact counter-example the "restore memory is sampling-dependent + // (phase 3 obligation)" test above pins at the raw-reducer level: dragging + // from a 300px wide sidebar to a fold, sampling a dead-zone point (179) along + // the way freezes the underlying layout's OWN wideWidthPx at the 180 floor, + // while jumping straight from 300 to the fold in one step leaves it at 300 — + // two different answers for what is, physically, the same gesture ending at + // the same pointer position. A session must not inherit that disagreement. + const start = wide({ wideWidthPx: 300, drawerWidthPx: 210 }); + + // Dense: samples the dead zone (179) before the fold. + let denseSession = beginLeftNavigationResize(start, start); + let denseLayout: LeftNavigationLayout = start; + for (const x of [200, 179, 139]) { + denseLayout = resolveLeftNavigationDrag(denseLayout, x); + denseSession = advanceLeftNavigationResize(denseSession, denseLayout); + } + // Sanity check on the known artifact this dense path produces. + expect(denseLayout.mode).toBe('rail'); + expect(denseLayout.wideWidthPx).toBe(LEFT_PANEL_MIN_PX); + + // Coarse: one jump straight past the fold threshold, skipping the dead zone. + let coarseSession = beginLeftNavigationResize(start, start); + const coarseLayout = resolveLeftNavigationDrag(start, 139); + coarseSession = advanceLeftNavigationResize(coarseSession, coarseLayout); + // Sanity check on the known — and DIFFERENT — artifact the coarse path + // produces for the very same underlying field. + expect(coarseLayout.mode).toBe('rail'); + expect(coarseLayout.wideWidthPx).toBe(300); + + // Despite that disagreement in the raw layouts, both sessions reconstruct + // from `preferredAtStart` on a fold-through, so both commit identically. + const denseCommitted = commitLeftNavigationResize(denseSession); + const coarseCommitted = commitLeftNavigationResize(coarseSession); + expect(denseCommitted).toEqual(coarseCommitted); + expect(denseCommitted).toEqual(rail({ wideWidthPx: 300, drawerWidthPx: 210 })); + }); }); From c7e3e74c0b84762838183ed4e25adfafd136d145 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Thu, 30 Jul 2026 10:05:25 +0200 Subject: [PATCH 02/13] feat(#487): left navigation controller seam (phase 3, step 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New src/application/left-nav.ts: openFocusedSection/toggleFocusedSection compose the pane-selection write and the layout write into ONE batch(), fixing a real atomicity gap the phase-2 handoff flagged (two independently batched functions can let an effect observe a mismatched intermediate state). Also fixes a confirmed persistence gap: opening a lower section via this seam now persists `sidePanel` exactly like the wide sidebar's own tab switch already does (saved-history.ts's switchTo), so a rail/drawer selection survives a reload. Wires app.openFocusedSection as the deterministic seam #428 needs. No UI calls it yet — the rail/drawer land in a later step of this phase. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NZfeGpkpJw2zsfPtysGHUN --- src/application/left-nav.ts | 147 +++++++++++++++++++++++++ src/ui/app.ts | 5 + src/ui/app.types.ts | 7 ++ tests/helpers/fake-app.ts | 1 + tests/unit/app.test.ts | 15 +++ tests/unit/left-nav.test.ts | 207 ++++++++++++++++++++++++++++++++++++ 6 files changed, 382 insertions(+) create mode 100644 src/application/left-nav.ts create mode 100644 tests/unit/left-nav.test.ts diff --git a/src/application/left-nav.ts b/src/application/left-nav.ts new file mode 100644 index 0000000..15b51b5 --- /dev/null +++ b/src/application/left-nav.ts @@ -0,0 +1,147 @@ +// #487 phase 3 — the desktop left navigation's controller seam. The pure mode +// machine lives in `core/left-nav-layout.ts`; this module is the thin async/ +// stateful glue that reads `AppState`'s scattered fields into one +// `LeftNavigationLayout`, drives the reducers, and writes the result back — +// plus the one persistence gap phase 3 exists to close (see +// `selectSectionInExistingPane` below). +// +// Typed against a narrow structural interface, not `App`/`AppState` from +// `src/ui/`: `src/application/**` must never import `src/ui/**` or +// `src/editor/**` (build/check-boundaries.mjs), and a real `App` satisfies the +// shape below directly — the same convention `library-assignment-service.ts` +// and `app-preferences.ts` use. + +import { batch } from '@preact/signals-core'; +import type { Signal } from '@preact/signals-core'; +import { + resolveRailActivation, resolveRailOpen, sidePanelKeyFor, +} from '../core/left-nav-layout.js'; +import type { + LeftNavigationLayout, LeftNavigationMode, LeftNavigationSection, SidePanelKey, +} from '../core/left-nav-layout.js'; + +/** The `AppState` fields the left navigation reads and writes, named exactly as + * `state.ts` names them. Some are signals (repainted/observed reactively), + * some are plain numbers (written like any other splitter width, persisted + * only on a later resize-session commit) — that split matches `state.ts` + * exactly and matters for every write below. */ +export interface LeftNavStateSlice { + sidebarPx: number; + leftNavDrawerPx: number; + readonly leftNavMode: Signal; + readonly leftNavSection: Signal; + readonly upperRole: Signal<'databases' | 'dashboards'>; + readonly sidePanel: Signal; +} + +/** The one persistence call this module makes — `app.prefs.save`'s real + * signature (`AppPreferences.save`) takes any `PreferenceKey`; this module + * only ever names `'sidePanel'`, so the seam is narrowed to that one key + * rather than importing the full `PreferenceKey` union from + * `application/app-preferences.ts`. */ +export interface LeftNavApp { + readonly state: LeftNavStateSlice; + readonly prefs: { save(name: 'sidePanel', value: SidePanelKey): void }; +} + +/** + * Project the scattered `AppState` fields this module reads into one + * `LeftNavigationLayout` — the shape every reducer in `core/left-nav-layout.ts` + * takes and returns. Exported: later phase-3 steps (the resize separator, the + * app-shell repaint effect) need the same projection. + */ +export function readLeftNavigationLayout(state: LeftNavStateSlice): LeftNavigationLayout { + return { + mode: state.leftNavMode.value, + wideWidthPx: state.sidebarPx, + drawerWidthPx: state.leftNavDrawerPx, + focusedSection: state.leftNavSection.value, + }; +} + +/** + * Drive the pane that ALREADY shows this section when there is no drawer to + * open for it — the wide sidebar's upper role switch, or its lower + * library/history tab. Not exported: it is only ever the one-signal half of + * `openFocusedSection`/`toggleFocusedSection`'s single batched write, never a + * standalone operation (see those functions' own comments for why they must + * not each get their own `batch()`). + * + * The lower-section branch is the fix phase 3 exists to make: today only the + * wide sidebar's own tab click (`ui/saved-history.ts`'s `switchTo`) persists + * `sidePanel`, so a rail/drawer selection of the same section left the + * signal's NEW value unpersisted — a reload would silently revert to + * whichever pane was last chosen through the wide tabs. Persisting BEFORE + * writing the signal mirrors `switchTo` exactly. `state.libraryFilter` is + * deliberately untouched — a later phase-3 step owns per-section filters. + */ +function selectSectionInExistingPane(app: LeftNavApp, section: LeftNavigationSection): void { + if (section === 'databases' || section === 'dashboards') { + // Session-only, like `switchTo`'s counterpart for the upper pane: `upperRole` + // is never persisted (state.ts), so there is nothing to save here. + app.state.upperRole.value = section; + return; + } + const panel = sidePanelKeyFor(section); + app.prefs.save('sidePanel', panel); + app.state.sidePanel.value = panel; +} + +/** + * Write a resolved `LeftNavigationLayout` back onto the scattered signals/ + * fields it was read from. Not exported, for the same reason as + * `selectSectionInExistingPane`: it is only ever the other half of one batched + * write. + * + * Every caller only ever hands this the result of `resolveRailOpen` / + * `resolveRailActivation`, and neither reducer changes `mode` or either width + * in practice (rail-only reducers) — so writing all four fields + * unconditionally is harmless, and simpler than special-casing which changed. + * It does NOT call `prefs.save` for `leftNavMode`/`leftNavDrawerPx`: that + * persistence belongs to a later step's resize-session commit, not to opening + * a section. + */ +function writeLeftNavigationLayout(state: LeftNavStateSlice, layout: LeftNavigationLayout): void { + state.leftNavMode.value = layout.mode; + state.sidebarPx = layout.wideWidthPx; + state.leftNavDrawerPx = layout.drawerWidthPx; + state.leftNavSection.value = layout.focusedSection; +} + +/** + * Open a section IDEMPOTENTLY (`core/left-nav-layout.ts`'s `resolveRailOpen`) — + * the deterministic seam #487 asks the left navigation to expose for #428's + * bounded drag-hover, and the one a plain rail-icon click also uses. + * + * In rail mode this opens (or keeps open) the focused drawer; in wide mode + * `resolveRailOpen` returns the layout unchanged (both panes already show, so + * `selectSectionInExistingPane` is the only effect), and for a lower section it + * ALSO drives the existing wide-mode pane switch, so calling this while wide + * still selects the right lower tab. Both halves are wrapped in exactly ONE + * `batch()` call: composing two independently-`batch()`-wrapped functions is + * NOT one atomic transition, since Preact signals' `batch()` flushes on each + * top-level call's own exit — two separate calls would let an effect observe + * an intermediate, mismatched combination (e.g. `sidePanel` updated but + * `leftNavSection` still stale) on the frame between them. + */ +export function openFocusedSection(app: LeftNavApp, section: LeftNavigationSection): void { + batch(() => { + selectSectionInExistingPane(app, section); + writeLeftNavigationLayout(app.state, resolveRailOpen(readLeftNavigationLayout(app.state), section)); + }); +} + +/** + * Toggle a section (`core/left-nav-layout.ts`'s `resolveRailActivation`) — + * the same shape as `openFocusedSection`, but a second activation of the SAME + * already-open section closes the drawer instead of re-asserting it open. + * Same single-`batch()` requirement and rationale as `openFocusedSection`. + */ +export function toggleFocusedSection(app: LeftNavApp, section: LeftNavigationSection): void { + batch(() => { + selectSectionInExistingPane(app, section); + writeLeftNavigationLayout( + app.state, resolveRailActivation(readLeftNavigationLayout(app.state), section), + ); + }); +} diff --git a/src/ui/app.ts b/src/ui/app.ts index b0df5c9..abe7fde 100644 --- a/src/ui/app.ts +++ b/src/ui/app.ts @@ -43,6 +43,7 @@ import { renderTabs, selectTab, newTab, closeTab, loadIntoNewTab, openVariableTa import type { QueryOrName } from './tabs.js'; import { commitVariableConfig } from '../application/dashboard-variable-config.js'; import { dashboardVariables } from '../application/dashboard-tree-model.js'; +import { openFocusedSection } from '../application/left-nav.js'; import { normalizeVariableSql } from '../core/dashboard-variables.js'; import { batch } from '@preact/signals-core'; import { renderResults } from './results.js'; @@ -2936,6 +2937,10 @@ export function createApp(env: CreateAppEnv = {}): App { if (query) openQueryDocument(query); }; + // #487 phase 3 — the left navigation's controller seam. `app` (state + + // prefs) satisfies `left-nav.ts`'s narrow structural `LeftNavApp` directly. + app.openFocusedSection = (section) => { openFocusedSection(app, section); }; + // #535 — the tile's expand action. Order matters: the tree is revealed FIRST, // exactly as the Library-drop settlement does it (ui/dashboard-tree.ts), so the // row is expanded and armed as the tree's position and then `loadIntoNewTab` diff --git a/src/ui/app.types.ts b/src/ui/app.types.ts index 096d8fb..2f5d4da 100644 --- a/src/ui/app.types.ts +++ b/src/ui/app.types.ts @@ -17,6 +17,7 @@ import type { WorkspaceExternallyChangedInfo, WorkspaceMutationInput, WorkspaceMutationOutcome, } from '../state.js'; import type { DocTarget } from '../core/doc-types.js'; +import type { LeftNavigationSection } from '../core/left-nav-layout.js'; import type { QueryExecutionService } from '../application/query-execution-service.js'; import type { ConnectionSession, SessionChCtx } from '../application/connection-session.js'; import type { AuthenticatedExecutionScope } from '../application/authenticated-execution-scope.js'; @@ -542,6 +543,12 @@ export interface App { * #443 — the id is resolved BEFORE anything moves: one that names no saved * query reports a diagnostic and changes no surface, no route and no tab. */ openSavedQuery(queryId: string): void; + /** #487 phase 3 — open (or re-assert open) the left navigation's focused + * drawer on `section`, idempotently: repeated calls with the drawer already + * showing this section are a no-op (`core/left-nav-layout.ts`'s + * `resolveRailOpen`). In wide mode there is no drawer, so this only drives + * the existing upper/lower pane switch for `section`. */ + openFocusedSection(section: LeftNavigationSection): void; /** * #535 — a Dashboard TILE's own expand action: everything `openSavedQuery` * does, plus the two things that make it an act of "go work on this panel" diff --git a/tests/helpers/fake-app.ts b/tests/helpers/fake-app.ts index 20edb92..f22cdbf 100644 --- a/tests/helpers/fake-app.ts +++ b/tests/helpers/fake-app.ts @@ -597,6 +597,7 @@ const appDefaults: App = { showQuerySurface: () => {}, showDashboardSurface: () => {}, openSavedQuery: () => {}, + openFocusedSection: () => {}, openPanelQuery: () => {}, openVariableTab: () => {}, actions: {} as ActionsRegistry, diff --git a/tests/unit/app.test.ts b/tests/unit/app.test.ts index b785732..6707117 100644 --- a/tests/unit/app.test.ts +++ b/tests/unit/app.test.ts @@ -7093,6 +7093,21 @@ describe('unified /sql routing', () => { expect(toastEl.textContent).toBe('That query is no longer part of this workspace.'); }); + // #487 phase 3 — `app.openFocusedSection` is a thin controller-seam + // delegate to `application/left-nav.ts`'s own `openFocusedSection` + // (exhaustively covered in `left-nav.test.ts`); this just proves the + // wiring actually calls through into real `AppState`. + it('openFocusedSection delegates through to application/left-nav.ts', () => { + const { app } = readyApp(['a']); + app.state.leftNavMode.value = 'rail'; + app.state.sidePanel.value = 'history'; + + app.openFocusedSection('library'); + + expect(app.state.sidePanel.value).toBe('saved'); + expect(app.state.leftNavSection.value).toBe('library'); + }); + // #535 — the tile's expand action. `openSavedQuery` opens a document; // `openPanelQuery` opens a PANEL: same tab, plus a run and a tree reveal, so // leaving the Dashboard neither loses the user's place in it nor drops them on diff --git a/tests/unit/left-nav.test.ts b/tests/unit/left-nav.test.ts new file mode 100644 index 0000000..12d04a1 --- /dev/null +++ b/tests/unit/left-nav.test.ts @@ -0,0 +1,207 @@ +// #487 phase 3 — the left navigation's controller seam (`application/left-nav.ts`). +// The pure mode machine itself (`resolveRailOpen`/`resolveRailActivation`/…) is +// covered exhaustively in `left-nav-layout.test.ts`; what is tested here is the +// glue: the state-slice projection, the persistence gap the module exists to +// close (a rail/drawer selection of a lower section now persists `sidePanel`, +// matching the wide sidebar's own tab switch), and — the specific regression +// this phase's design review caught — that `openFocusedSection`/ +// `toggleFocusedSection` commit as ONE atomic signals transition, never two. + +import { describe, it, expect, vi } from 'vitest'; +import { effect, signal } from '@preact/signals-core'; +import { + openFocusedSection, readLeftNavigationLayout, toggleFocusedSection, +} from '../../src/application/left-nav.js'; +import type { LeftNavApp, LeftNavStateSlice } from '../../src/application/left-nav.js'; +import { + LEFT_DRAWER_DEFAULT_PX, LEFT_WIDE_DEFAULT_PX, +} from '../../src/core/left-nav-layout.js'; +import type { LeftNavigationSection, SidePanelKey } from '../../src/core/left-nav-layout.js'; + +/** A fake `LeftNavStateSlice`, rail mode by default (the mode every reducer + * here actually acts on) with no section focused and Library as the lower + * pane — override via `over`. */ +function makeState(over: Partial<{ + mode: 'wide' | 'rail'; + sidebarPx: number; + leftNavDrawerPx: number; + section: LeftNavigationSection | null; + upperRole: 'databases' | 'dashboards'; + sidePanel: SidePanelKey; +}> = {}): LeftNavStateSlice { + return { + sidebarPx: over.sidebarPx ?? LEFT_WIDE_DEFAULT_PX, + leftNavDrawerPx: over.leftNavDrawerPx ?? LEFT_DRAWER_DEFAULT_PX, + leftNavMode: signal(over.mode ?? 'rail'), + leftNavSection: signal(over.section ?? null), + upperRole: signal(over.upperRole ?? 'databases'), + sidePanel: signal(over.sidePanel ?? 'saved'), + }; +} + +function makeApp(state: LeftNavStateSlice): LeftNavApp & { save: ReturnType } { + const save = vi.fn(); + return { state, prefs: { save }, save }; +} + +describe('readLeftNavigationLayout', () => { + it('maps every field from the state slice', () => { + const state = makeState({ + mode: 'rail', sidebarPx: 300, leftNavDrawerPx: 200, section: 'library', + }); + expect(readLeftNavigationLayout(state)).toEqual({ + mode: 'rail', wideWidthPx: 300, drawerWidthPx: 200, focusedSection: 'library', + }); + }); + + it('maps a wide/no-focus state too', () => { + const state = makeState({ mode: 'wide', section: null }); + expect(readLeftNavigationLayout(state)).toEqual({ + mode: 'wide', + wideWidthPx: LEFT_WIDE_DEFAULT_PX, + drawerWidthPx: LEFT_DRAWER_DEFAULT_PX, + focusedSection: null, + }); + }); +}); + +describe('openFocusedSection — lower sections persist sidePanel', () => { + it('opening Library from a mismatched History state fixes both signals together', () => { + const state = makeState({ mode: 'rail', sidePanel: 'history', section: 'history' }); + const app = makeApp(state); + + openFocusedSection(app, 'library'); + + expect(state.sidePanel.value).toBe('saved'); + expect(state.leftNavSection.value).toBe('library'); + expect(app.save).toHaveBeenCalledWith('sidePanel', 'saved'); + }); + + it('opening History from a mismatched Library state fixes both signals together', () => { + const state = makeState({ mode: 'rail', sidePanel: 'saved', section: 'library' }); + const app = makeApp(state); + + openFocusedSection(app, 'history'); + + expect(state.sidePanel.value).toBe('history'); + expect(state.leftNavSection.value).toBe('history'); + expect(app.save).toHaveBeenCalledWith('sidePanel', 'history'); + }); + + it('does not touch libraryFilter or any field beyond the four it owns', () => { + const state = makeState({ mode: 'rail' }) as LeftNavStateSlice & { libraryFilter: string }; + state.libraryFilter = 'unchanged-marker'; + const app = makeApp(state); + + openFocusedSection(app, 'library'); + + expect(state.libraryFilter).toBe('unchanged-marker'); + }); +}); + +describe('openFocusedSection — upper sections never persist', () => { + it('opening Dashboards from a mismatched Databases state fixes both signals, no persistence', () => { + const state = makeState({ mode: 'rail', upperRole: 'databases', section: 'databases' }); + const app = makeApp(state); + + openFocusedSection(app, 'dashboards'); + + expect(state.upperRole.value).toBe('dashboards'); + expect(state.leftNavSection.value).toBe('dashboards'); + expect(app.save).not.toHaveBeenCalled(); + }); + + it('opening Databases from a mismatched Dashboards state fixes both signals, no persistence', () => { + const state = makeState({ mode: 'rail', upperRole: 'dashboards', section: 'dashboards' }); + const app = makeApp(state); + + openFocusedSection(app, 'databases'); + + expect(state.upperRole.value).toBe('databases'); + expect(state.leftNavSection.value).toBe('databases'); + expect(app.save).not.toHaveBeenCalled(); + }); +}); + +describe('atomicity — one batched transition, not two', () => { + it('an effect reading both the pane signal and leftNavSection runs exactly once, and only ever sees the final combination', () => { + const state = makeState({ mode: 'rail', sidePanel: 'history', section: 'history' }); + const app = makeApp(state); + + const seen: Array<{ panel: SidePanelKey; section: LeftNavigationSection | null }> = []; + const dispose = effect(() => { + seen.push({ panel: state.sidePanel.value, section: state.leftNavSection.value }); + }); + // The effect above runs once on install; clear that baseline observation so + // the assertions below are only about the call under test. + seen.length = 0; + + openFocusedSection(app, 'library'); + + expect(seen).toHaveLength(1); + expect(seen[0]).toEqual({ panel: 'saved', section: 'library' }); + + dispose(); + }); + + it('same atomicity guarantee for toggleFocusedSection', () => { + const state = makeState({ mode: 'rail', upperRole: 'databases', section: 'databases' }); + const app = makeApp(state); + + const seen: Array<{ role: 'databases' | 'dashboards'; section: LeftNavigationSection | null }> = []; + const dispose = effect(() => { + seen.push({ role: state.upperRole.value, section: state.leftNavSection.value }); + }); + seen.length = 0; + + toggleFocusedSection(app, 'dashboards'); + + expect(seen).toHaveLength(1); + expect(seen[0]).toEqual({ role: 'dashboards', section: 'dashboards' }); + + dispose(); + }); +}); + +describe('toggleFocusedSection — closes an already-open section', () => { + it('toggling the section that is already focused closes the drawer (resolveRailActivation semantics)', () => { + const state = makeState({ mode: 'rail', section: 'library', sidePanel: 'saved' }); + const app = makeApp(state); + + toggleFocusedSection(app, 'library'); + + expect(state.leftNavSection.value).toBeNull(); + }); + + it('toggling a DIFFERENT section than the one open focuses the new one instead (no close)', () => { + const state = makeState({ mode: 'rail', section: 'library', sidePanel: 'saved' }); + const app = makeApp(state); + + toggleFocusedSection(app, 'history'); + + expect(state.leftNavSection.value).toBe('history'); + }); + + it('openFocusedSection is idempotent instead of toggling — repeated calls keep the section open', () => { + const state = makeState({ mode: 'rail', section: 'library', sidePanel: 'saved' }); + const app = makeApp(state); + + openFocusedSection(app, 'library'); + + expect(state.leftNavSection.value).toBe('library'); + }); +}); + +describe('wide mode — no drawer, only the pane switch applies', () => { + it('openFocusedSection in wide mode leaves mode/section alone but still switches the pane', () => { + const state = makeState({ mode: 'wide', section: null, sidePanel: 'history' }); + const app = makeApp(state); + + openFocusedSection(app, 'library'); + + expect(state.leftNavMode.value).toBe('wide'); + expect(state.leftNavSection.value).toBeNull(); + expect(state.sidePanel.value).toBe('saved'); + expect(app.save).toHaveBeenCalledWith('sidePanel', 'saved'); + }); +}); From ad36b0cb46e0af8381f3fbfd248612f2adc0e35f Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Thu, 30 Jul 2026 10:30:37 +0200 Subject: [PATCH 03/13] feat(#487): per-section lower-nav filters and independent rendering (phase 3, step 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the shared state.libraryFilter with lowerNavigationFilters, a per-section record — switching between Library and History no longer clears the search box, required by this phase's own acceptance bullet that wide and focused presentations share and preserve all navigation state (a deliberate, user-visible change from phase 2's "unchanged" gate, documented in the CHANGELOG). Splits renderSavedHistory so both lower sections render their own content unconditionally, mirroring the upper pane's renderSchema/ renderDashboardTree pattern, closing two confirmed bugs: a section not active at mount never painted until the first switch, and a section's content going stale when its own data changed while the other section was exposed (History-while-Library-active, and the reverse). The two call sites that guarded a History repaint behind `sidePanel === 'history'` (app.recordHistory, the script-run history path) are now unconditional. Also closes #572: the lower switcher's tabs gain type="button" and aria-pressed, matching the upper switcher. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NZfeGpkpJw2zsfPtysGHUN --- CHANGELOG.md | 14 ++ src/core/dashboard-tree-ui-state.ts | 2 +- src/state.ts | 17 ++- src/ui/app-shell.ts | 43 ++++-- src/ui/app.ts | 19 ++- src/ui/saved-history.ts | 184 ++++++++++++++++---------- src/ui/workbench/workbench-session.ts | 27 ++-- tests/unit/app-shell.test.ts | 86 ++++++++++-- tests/unit/app.test.ts | 18 +++ tests/unit/saved-history.test.ts | 159 +++++++++++++++++++--- tests/unit/state.test.ts | 4 + tests/unit/workbench-session.test.ts | 17 ++- 12 files changed, 455 insertions(+), 135 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 91943d6..2d17159 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,20 @@ auto-generated per-PR notes; this file is the curated, human-readable history. focused drawer and the resize separator arrive in phase 3. ### Changed +- **Switching between Library and History no longer clears the search box** + (#487, phase 3 of 4). Each lower-navigation section now keeps its own filter + text (`state.lowerNavigationFilters`, replacing the single shared + `state.libraryFilter`), preserved across every switch between them — a + deliberate, user-visible behavior change from phase 2's "a section switch + still clears the search exactly as before," required by phase 3's own + acceptance bullet that wide and focused presentations share and preserve all + navigation state. Both sections' own search box and list now also render + unconditionally, independently of which section is currently exposed + (mirroring the upper pane's `renderSchema`/`renderDashboardTree`), which + fixes two latent bugs: the section that wasn't active at mount never painted + until the first switch to it, and the section that wasn't active when its own + data changed (e.g. a query recorded to History while Library was shown) went + stale. - 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 diff --git a/src/core/dashboard-tree-ui-state.ts b/src/core/dashboard-tree-ui-state.ts index 9c5066f..73d6b2f 100644 --- a/src/core/dashboard-tree-ui-state.ts +++ b/src/core/dashboard-tree-ui-state.ts @@ -14,7 +14,7 @@ // Keyed by `StoredWorkspaceV5.id` — the immutable opaque application identity // (#406) — never by the mutable `name` or the rewritable URL `key`. // -// Deliberately NOT a signal, matching `state.libraryFilter`'s precedent +// Deliberately NOT a signal, matching `state.lowerNavigationFilters`'s precedent // (`src/state.ts`): if a repaint effect observed this state, every keystroke in // the search box and every scroll frame would repaint the tree — losing the caret // on the first and doing pointless work on the second. The tree's ONE reactive diff --git a/src/state.ts b/src/state.ts index 9702cf9..a1072ea 100644 --- a/src/state.ts +++ b/src/state.ts @@ -41,7 +41,7 @@ import { sectionForSidePanelKey, sidePanelKeyFor, } from './core/left-nav-layout.js'; import type { - LeftNavigationMode, LeftNavigationSection, SidePanelKey, + LeftNavigationMode, LeftNavigationSection, LowerNavigationSection, SidePanelKey, } from './core/left-nav-layout.js'; // ── Persisted-data types (schema-generated) ───────────────────────────────── @@ -445,7 +445,13 @@ export interface AppState { workspaceId: string; /** Immutable canonical URL key for the active workspace. */ workspaceKey: string; - libraryFilter: string; + /** #487 phase 3: each lower-navigation section (Library, History) keeps its + * OWN search text, independently preserved across every switch between them + * — switching no longer clears anything. Deliberately NOT a signal (see + * `core/dashboard-tree-ui-state.ts`'s comment): a signal here would rebuild + * the search input on every keystroke, and `renderList`-only re-renders is + * how focus/caret survive typing today. */ + lowerNavigationFilters: Record; shortcutsOpen: Signal; /** * #487 — the desktop left navigation's explicit semantic mode: the established @@ -815,9 +821,10 @@ export function createState(read: StateReader = { loadJSON, loadStr }): AppState dashboard: null, workspaceId: mintWorkspaceId(), workspaceKey: deriveWorkspaceKey(initialWorkspaceName), - // Transient search text for the Library/History side panel (session-only, - // cleared on a tab switch); never persisted. - libraryFilter: '', + // Transient search text for the Library/History side panel (session-only; + // never persisted). #487 phase 3: each section keeps its own slot, preserved + // across every switch between them. + lowerNavigationFilters: { library: '', history: '' }, // Whether the keyboard-shortcuts modal is open (shortcuts.js). Session-only; // a signal for consistency with the rest of the state (no reactive reader // today — shortcuts.js drives its own mount/unmount). diff --git a/src/ui/app-shell.ts b/src/ui/app-shell.ts index 2fa9d32..da3f340 100644 --- a/src/ui/app-shell.ts +++ b/src/ui/app-shell.ts @@ -24,7 +24,7 @@ // // `deps.app` is kept for the same reasons `mountWorkbenchShell` keeps it // (see that module's own header comment): the render-module pass-through -// (renderSchema/renderSavedHistory/renderLibraryTitle all +// (renderSchema/renderLowerTabs/renderLibrarySection/renderLibraryTitle all // still take the full `App`), and the `app.dom` reset + population other // modules read `app.dom.*` off of directly. @@ -38,7 +38,7 @@ import { buildSidebarUpper, renderUpperRoleTabs } from './sidebar-upper.js'; import { buildNavSectionRegistry, sectionForSidePanelKey } from './nav-sections.js'; import type { NavSectionPane } from './nav-sections.js'; import { renderDashboardTree, cancelDashboardTreeClicks } from './dashboard-tree.js'; -import { renderSavedHistory } from './saved-history.js'; +import { renderLowerTabs, renderLibrarySection, renderHistorySection } from './saved-history.js'; import { renderLibraryTitle } from './file-menu.js'; import { applyConnectionStatus } from './app-header.js'; import type { DragCtx, DragRect, DragStartEvent, SplitterAxis } from './splitters.js'; @@ -52,7 +52,7 @@ import type { AppPreferences, PreferenceKey } from '../application/app-preferenc * shell's own logic, never through `app.*`. */ export interface AppShellDeps { /** Kept ONLY for: the render-module pass-through (renderSchema/ - * renderSavedHistory/renderLibraryTitle), and the + * renderLowerTabs/renderLibrarySection/renderLibraryTitle), and the * `app.dom` reset + population (other modules read `app.dom.*` * directly — see the header comment). */ app: App; @@ -285,20 +285,47 @@ export function mountAppShell(deps: AppShellDeps): AppShellHandle { state.schemaError.value; updateBanner(); })); - // Reactive repaint of the side panel: re-runs when the active panel changes - // (Library ↔ History). Data-driven repaints (savedQueries/history mutations) - // still call renderSavedHistory directly until those slices are signals too. + // Reactive repaint of the lower tab row: re-runs when the active panel changes + // (Library ↔ History) or the Library count might have (projection revision). + // #487 phase 3 split this from the content repaint below, mirroring the upper + // pane's own split (`renderUpperRoleTabs` vs `renderSchema`) — switching which + // section is exposed repaints only the tab row's active class/count, never + // either section's content. + disposers.push(effect(() => { + state.sidePanel.value; + state.dashboardTreeRevision.value; + renderLowerTabs(app); + })); + // Reactive repaint of Library's own content: re-runs on the projection + // revision alone, regardless of which lower section is exposed (#487 phase 3) + // — mirroring `renderSchema`, which does not subscribe to `upperRole` either. + // Data-driven repaints of ROW-level state (favorite/rename/delete) still call + // the full `renderSavedHistory` facade directly. // // #427 added the projection revision. Library membership is now a function of // `dashboards[]` — a query is in the Library exactly while no Dashboard member // references it — so a committed Dashboard change can move a query in or out of // this list without `savedQueries` changing at all. It is the same one signal // the Dashboard tree subscribes to, bumped from the single projection funnel. + // + // Deliberately no matching reactive effect for History's own content: + // `state.history` is a plain array, not a signal, so History has never been + // signal-driven — it stays current via direct calls at its mutation sites + // (`app.recordHistory`, the script-run history path, and the facade above). disposers.push(effect(() => { - state.sidePanel.value; state.dashboardTreeRevision.value; - renderSavedHistory(app); + renderLibrarySection(app); })); + // History's OWN initial paint: unlike Library, History has no signal to key a + // reactive effect off (see the comment above), so it cannot pick up its first + // paint by registering one. Without this direct, one-time call the History + // host would stay the empty div `nav-sections.ts` built it as until the first + // history-recording event — reintroducing, for History specifically, the exact + // "section that wasn't active at mount never got painted" bug this phase + // fixes for Library via the effect above. Not an effect itself (no signal + // read, so it never re-runs) — every subsequent History repaint still comes + // from its own mutation sites or the full `renderSavedHistory` facade. + renderHistorySection(app); // Reactive repaint of the header library title (name + unsaved-changes dot): // re-runs when the name or dirty flag changes. The edit-mode toggle is driven // separately (editingLibrary is not a signal — file-menu.js renders it directly). diff --git a/src/ui/app.ts b/src/ui/app.ts index abe7fde..d43d98a 100644 --- a/src/ui/app.ts +++ b/src/ui/app.ts @@ -56,7 +56,7 @@ import type { SchemaLineageNode, DetachedGraphApp } from './explain-graph.js'; import { openDetailPane } from './schema-detail.js'; import type { NodeDetail, DetailNode } from './schema-detail.js'; import { openDocEntry, openDocDisambiguation, closeDocPane, isDocPaneOpen } from './doc-pane.js'; -import { renderSavedHistory } from './saved-history.js'; +import { renderSavedHistory, renderHistorySection } from './saved-history.js'; import { applyFieldState, applyFieldWidth } from './var-field.js'; import { buildRelativeTimeField } from './relative-time-field.js'; import type { RelativeTimeField } from './relative-time-field.js'; @@ -986,7 +986,7 @@ export function createApp(env: CreateAppEnv = {}): App { activeTab: () => app.activeTab(), hooks: { renderResults: () => renderResults(app), - renderSavedHistory: () => renderSavedHistory(app), + renderHistorySection: () => renderHistorySection(app), cancelSchemaGraph, loadSchema: () => { void catalog.loadSchema(); }, recordHistory: (tab, sql) => app.recordHistory(tab, sql), @@ -1558,12 +1558,19 @@ export function createApp(env: CreateAppEnv = {}): App { // --- saved / history bridges ------------------------------------------ // The history-recording POLICY itself now lives in `saved.recordHistory` - // (#276 Phase 4C) — this wrapper's own conditional History-panel repaint is - // a rendering concern the service must never own (see its header comment), - // so it stays here, unchanged. + // (#276 Phase 4C) — this wrapper's own History-panel repaint is a rendering + // concern the service must never own (see its header comment), so it stays + // here. + // + // #487 phase 3: unconditional — History's own content must stay current even + // while Library is the exposed section, since History repaints only at its + // own mutation sites (`state.history` is a plain array, not a signal, so it + // is not part of any reactive repaint effect). Only History's own data + // changed here, so this calls `renderHistorySection` rather than the full + // `renderSavedHistory` facade. app.recordHistory = (tab, sqlText) => { saved.recordHistory(tab, sqlText); - if (app.state.sidePanel.value === 'history') renderSavedHistory(app); + renderHistorySection(app); }; // --- share + star ------------------------------------------------------ diff --git a/src/ui/saved-history.ts b/src/ui/saved-history.ts index 496ce97..eebf827 100644 --- a/src/ui/saved-history.ts +++ b/src/ui/saved-history.ts @@ -1,16 +1,24 @@ // The bottom sidebar pane: a Library / History switcher and, per section, its own // search box and list. Saved items support favorite (star), inline rename (pencil) -// and delete (trash). The search filters the active list (name/description/sql for -// Library, sql for History); it re-renders only the list so typing keeps focus. +// and delete (trash). The search filters a section's own list (name/description/sql +// for Library, sql for History); it re-renders only the list so typing keeps focus. // // #487 phase 2 split the two sections' DOM: each renders into its own persistent // search/list pair (`ui/nav-sections.ts` builds them and hosts them), where before -// both shared one pair that a section switch repainted. Everything below still -// renders ONLY the active section, exactly as it always did — the switcher's -// clear-the-search semantics are unchanged, and the inactive host simply keeps the -// DOM it last painted until it is shown again. What the split buys is that a -// container can move one section's live elements (phase 3's focused drawer) -// without taking the other section's content along. +// both shared one pair that a section switch repainted. +// +// #487 phase 3 finishes the split: BOTH sections now render their own content +// unconditionally, independently of which one is exposed — mirroring the upper +// pane's `renderSchema`/`renderDashboardTree`, which have always painted on their +// own data triggers regardless of `state.upperRole`. Exposure (which host is +// visible) is a wholly separate concern, owned by `app-shell.ts`'s registry +// effect. This fixes two real bugs the old "paint only the active section" design +// had: the section that wasn't active at mount never got its first paint (blank +// until the first switch), and the section that wasn't active when its own data +// changed (e.g. a query runs and gets recorded to History while Library is shown) +// went stale until the next unrelated repaint. Each section also now keeps its +// OWN filter text (`state.lowerNavigationFilters`) rather than sharing one string, +// so switching between them no longer clears anything. import { h } from './dom.js'; import { Icon } from './icons.js'; @@ -113,43 +121,55 @@ function libraryEntries(app: App): SavedQueryV2[] { } /** - * The active section, in the registry's vocabulary. EVERY branch in this module - * goes through this one function rather than comparing `sidePanel` to `'saved'` - * directly (#487 phase 2). With two hosts, a reader that resolves an unrecognized - * value differently from the shell's exposure effect would expose one section's - * host and paint into the other's — a blank pane. `state.ts` also decodes the - * stored value at load, so the two guards are belt and braces on purpose. + * The active section, in the registry's vocabulary — used ONLY to decide the tab + * row's "active" class/`aria-pressed` state. It no longer decides which section's + * content renders (#487 phase 3: both sections always render their own, regardless + * of which is exposed). `state.ts` also decodes the stored value at load. */ const activeSection = (app: App): LowerNavigationSection => sectionForSidePanelKey(app.state.sidePanel.value); -/** The ACTIVE section's own search box and list (#487 phase 2) — the two lower - * sections no longer share one pair. */ -const activeEls = (app: App): { search: HTMLElement | undefined; list: HTMLElement | undefined } => - activeSection(app) === 'library' +/** A given section's OWN search box and list hosts. Deliberately keyed by an + * explicit `section` parameter rather than "whichever is active" (#487 phase 3) + * — each section's render functions always target their own hosts. */ +const elsFor = (app: App, section: LowerNavigationSection): { search: HTMLElement | undefined; list: HTMLElement | undefined } => + section === 'library' ? { search: app.dom.savedSearch, list: app.dom.savedList } : { search: app.dom.historySearch, list: app.dom.historyList }; -export function renderSavedHistory(app: App): void { +/** Read a section's own search filter text (#487 phase 3 — each lower-navigation + * section keeps its own, so switching between them preserves both instead of + * clearing one shared string). Exported for later phase-3 steps. */ +export function filterFor(state: AppState, section: LowerNavigationSection): string { + return state.lowerNavigationFilters[section]; +} + +/** Write a section's own search filter text. See `filterFor`. */ +export function setFilterFor(state: AppState, section: LowerNavigationSection, value: string): void { + state.lowerNavigationFilters[section] = value; +} + +/** + * The tab row only: the Library/History switcher plus the Library count badge. + * #487 phase 2: the tab row speaks the registry's SECTION vocabulary and derives + * the persisted value once, through the one mapping — rather than repeating the + * `'library' means 'saved'` knowledge here. + * + * #487 phase 3: switching sections no longer clears anything — each section keeps + * its own filter text (`state.lowerNavigationFilters`), preserved across every + * switch. `switchTo` now only persists the choice and sets which section is + * exposed. + */ +export function renderLowerTabs(app: App): void { const tabsRow = app.dom.savedTabsRow; - const list = activeEls(app).list; - if (!tabsRow || !list) return; + if (!tabsRow) return; const state = app.state; // #427: the count is the LIBRARY count, not every stored query — the owned // copies are reachable through the Dashboard tree, not through this list. const count = libraryEntries(app).length; - // Switching panes clears the search so each tab starts unfiltered. Clear the - // (plain) filter first, then set the sidePanel signal — its render effect runs - // synchronously on assignment and must see the cleared filter. No manual - // re-render call: the effect in createApp() repaints. - // - // #487 phase 2: the tab row speaks the registry's SECTION vocabulary and derives - // the persisted value once, through the one mapping — rather than repeating the - // `'library' means 'saved'` knowledge here. const switchTo = (section: LowerNavigationSection): void => { const panel = sidePanelKeyFor(section); - state.libraryFilter = ''; app.prefs.save('sidePanel', panel); state.sidePanel.value = panel; }; @@ -158,6 +178,8 @@ export function renderSavedHistory(app: App): void { const meta = NAV_SECTION_META[section]; return h('button', { class: 'side-tab' + (active === section ? ' active' : ''), + type: 'button', + 'aria-pressed': active === section ? 'true' : 'false', onclick: () => switchTo(section), }, meta.icon(), h('span', null, meta.label), extra); }; @@ -166,34 +188,34 @@ export function renderSavedHistory(app: App): void { tab('library', count ? h('span', { class: 'side-count' }, '· ' + count) : null), tab('history', null), ); - - renderSearch(app); - renderList(app); } -/** Re-render just the active list (called on every keystroke without rebuilding - * the search input, so the caret/focus survive filtering). */ -function renderList(app: App): void { - // `!`: every caller (renderSavedHistory, renderSearch below) only reaches - // this after confirming the active section's list is mounted. - const list = activeEls(app).list!; +/** Render a given section's own list into a given host — favorites/rename/delete + * for Library, delete for History (`renderSaved`/`renderHistory` below), reading + * that section's OWN filter. Independent of which section is exposed. */ +function renderSectionList(app: App, section: LowerNavigationSection, list: HTMLElement): void { list.replaceChildren(); - if (activeSection(app) === 'library') renderSaved(app, list); + if (section === 'library') renderSaved(app, list); else renderHistory(app, list); } /** - * Render the search box into the ACTIVE section's own search host (built once per - * full render; a section with no items shows nothing). Its `input` handler mutates - * `state.libraryFilter` and re-renders only the list, so it stays focused. + * Render the search box into a given section's OWN search host (built once per + * full render; a section with no items shows nothing). Its `input` handler + * mutates that section's own filter slot and re-renders only that section's own + * list, so it stays focused. + * + * #487 phase 3 removed the old "does this input still own the active list" + * guard: with one shared `libraryFilter` string, a stale/hidden input's events + * could corrupt the OTHER section's filter and repaint the wrong list. With + * per-section storage that is structurally impossible — a hidden Library input + * can only ever write Library's own filter and repaint Library's own list, + * which is correct regardless of whether Library is currently exposed. */ -function renderSearch(app: App): void { - const box = activeEls(app).search; - if (!box) return; +function renderSectionSearch(app: App, section: LowerNavigationSection, box: HTMLElement): void { const state = app.state; // Gated on the LIBRARY count (#427): a workspace whose every query is owned // has an empty list, so a search box over it would filter nothing. - const section = activeSection(app); const isLibrary = section === 'library'; const hasItems = isLibrary ? libraryEntries(app).length > 0 @@ -204,31 +226,22 @@ function renderSearch(app: App): void { const input = h('input', { class: 'sv-search-input', type: 'text', placeholder: isLibrary ? 'Search library queries…' : 'Search history…', - value: state.libraryFilter, + value: filterFor(state, section), }); const clear = h('button', { class: 'sv-search-clear', title: 'Clear' }, Icon.close()); const syncClear = (): void => { clear.style.display = input.value ? '' : 'none'; }; - // These controls belong to the section that was active when they were built, and - // that host now OUTLIVES the switch away from it (#487 phase 2) — the inactive - // host keeps its DOM, listeners included. `state.libraryFilter` is still one - // shared string and `renderList` still paints the ACTIVE section, so an event - // from a stale input would rewrite the filter and repaint the OTHER section's - // list with this section's search text. Unreachable through the UI today (a - // `display: none` subtree receives no pointer or keyboard events) — but phase 3 - // moves hosts between containers, where a host can be visible while a different - // section is active, so ownership is enforced here rather than left to CSS. - // - // A guard, not a redesign: per-section filter state is what actually fixes the - // shared-string design, and #487 phase 3 owns that (see the ship log). - const ownsTheList = (): boolean => activeSection(app) === section; + const list = elsFor(app, section).list; const setFilter = (v: string): void => { - if (!ownsTheList()) return; - input.value = v; state.libraryFilter = v; syncClear(); renderList(app); + input.value = v; + setFilterFor(state, section, v); + syncClear(); + if (list) renderSectionList(app, section, list); }; input.addEventListener('input', () => { - if (!ownsTheList()) return; - state.libraryFilter = input.value; syncClear(); renderList(app); + setFilterFor(state, section, input.value); + syncClear(); + if (list) renderSectionList(app, section, list); }); input.addEventListener('keydown', (e) => { if (e.key === 'Escape') { e.preventDefault(); setFilter(''); } }); clear.addEventListener('click', () => { setFilter(''); input.focus(); }); @@ -237,6 +250,37 @@ function renderSearch(app: App): void { box.append(h('span', { class: 'sv-search-icon' }, Icon.search()), input, clear); } +/** Library's own search + list, into Library's own hosts — unconditionally, + * regardless of whether Library is currently exposed (#487 phase 3). */ +export function renderLibrarySection(app: App): void { + const { search, list } = elsFor(app, 'library'); + if (search) renderSectionSearch(app, 'library', search); + if (list) renderSectionList(app, 'library', list); +} + +/** History's own search + list, into History's own hosts — unconditionally, + * regardless of whether History is currently exposed (#487 phase 3). */ +export function renderHistorySection(app: App): void { + const { search, list } = elsFor(app, 'history'); + if (search) renderSectionSearch(app, 'history', search); + if (list) renderSectionList(app, 'history', list); +} + +/** + * The facade: repaint the tab row and BOTH sections' own content, always (#487 + * phase 3 — mirrors the upper pane, where `renderSchema`/`renderDashboardTree` + * each paint on their own triggers regardless of `state.upperRole`). Existing + * internal call sites (favorite toggle, inline edit commit, row delete, + * history-row delete) keep calling this facade unchanged — repainting all three + * on those broader events is simple, safe, and cheap enough for these small + * lists. + */ +export function renderSavedHistory(app: App): void { + renderLowerTabs(app); + renderLibrarySection(app); + renderHistorySection(app); +} + function renderSaved(app: App, list: HTMLElement): void { const state = app.state; const surfaceGeneration = app.captureSurfaceGeneration(); @@ -250,9 +294,10 @@ function renderSaved(app: App, list: HTMLElement): void { } // Favourites first, then `workspace.queries[]` order — the projection filters, // it never reorders (#427). - const items = filterSaved(sortedSaved({ ...state, savedQueries: library }), state.libraryFilter); + const libraryFilter = filterFor(state, 'library'); + const items = filterSaved(sortedSaved({ ...state, savedQueries: library }), libraryFilter); if (items.length === 0) { - list.appendChild(h('div', { class: 'saved-empty' }, 'No library queries match “' + state.libraryFilter.trim() + '”.')); + list.appendChild(h('div', { class: 'saved-empty' }, 'No library queries match “' + libraryFilter.trim() + '”.')); return; } for (const q of items) { @@ -429,9 +474,10 @@ function renderHistory(app: App, list: HTMLElement): void { list.appendChild(h('div', { class: 'saved-empty' }, 'No history yet.')); return; } - const items = filterHistory(state.history, state.libraryFilter); + const historyFilter = filterFor(state, 'history'); + const items = filterHistory(state.history, historyFilter); if (items.length === 0) { - list.appendChild(h('div', { class: 'saved-empty' }, 'No history matches “' + state.libraryFilter.trim() + '”.')); + list.appendChild(h('div', { class: 'saved-empty' }, 'No history matches “' + historyFilter.trim() + '”.')); return; } for (const ent of items as HistoryEntry[]) { diff --git a/src/ui/workbench/workbench-session.ts b/src/ui/workbench/workbench-session.ts index b08a178..741c2a2 100644 --- a/src/ui/workbench/workbench-session.ts +++ b/src/ui/workbench/workbench-session.ts @@ -67,13 +67,18 @@ export interface WorkbenchStateSlice { resultRowLimit: number; serverVersion: string | null; /** - * Read by runScript's clean-run history branch ('history' ⇒ repaint). + * #487 phase 3: no longer read by this session's own logic — runScript's + * clean-run history repaint (`hooks.renderHistorySection()`) is now + * unconditional, since History's content must stay current regardless of + * which lower-navigation section is exposed. Kept on the slice because + * `AppState` carries it regardless (structural pass-through) and a later + * caller may still need it; if it stays unread, a future cleanup can drop it. * * Derived from `AppState` rather than restated as `Signal` (#487 * phase 2): the real signal holds a decoded `'saved' | 'history'`, and a * structural `Signal` here would leave this session type-authorized to * write an arbitrary string into it — re-opening exactly the divergence the - * load-boundary decode closes. This slice only ever reads it. + * load-boundary decode closes. */ sidePanel: AppState['sidePanel']; isMobile: Signal; @@ -97,15 +102,18 @@ export interface WorkbenchStateSlice { export interface WorkbenchHooks { /** Per-chunk (run) + per-statement (runScript) results-pane repaint. */ renderResults(): void; - /** runScript's clean-run history repaint when `sidePanel === 'history'`. */ - renderSavedHistory(): void; + /** runScript's clean-run History repaint — unconditional (#487 phase 3): + * History's own content must stay current even while Library is the + * exposed section, since History is not part of any reactive repaint + * effect (`state.history` is a plain array, not a signal). */ + renderHistorySection(): void; cancelSchemaGraph(): void; /** Fire-and-forget schema reload after schema-mutating SQL succeeds. */ loadSchema(): void; /** Records a successful single-statement run in history (and, per the real - * app.ts wrapper this replaces, repaints History when it's the open side - * panel — that repaint is this hook's own responsibility, unlike - * `renderSavedHistory` above which the session calls itself for the + * app.ts wrapper this replaces, unconditionally repaints History's own + * content — that repaint is this hook's own responsibility, unlike + * `renderHistorySection` above which the session calls itself for the * script-history path). */ recordHistory(tab: QueryTab, sql?: string): void; recordBoundParams(bp: readonly BoundParamSnapshot[]): void; @@ -755,7 +763,10 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes // run(): no history for an aborted or failed script). if (!aborted && !entries.some((e) => e.status === 'error')) { recordScriptHistory(state, originalInput, scriptResult.elapsedMs!, hooks.saveJSON); - if (state.sidePanel.value === 'history') hooks.renderSavedHistory(); + // #487 phase 3: unconditional — History's own content must stay current + // even while Library is the exposed section (History is not part of any + // reactive repaint effect; see `renderHistorySection`'s own doc comment). + hooks.renderHistorySection(); } retireWave(operation); } diff --git a/tests/unit/app-shell.test.ts b/tests/unit/app-shell.test.ts index 7789404..ab61002 100644 --- a/tests/unit/app-shell.test.ts +++ b/tests/unit/app-shell.test.ts @@ -106,20 +106,24 @@ describe('mountAppShell wide navigation (#487 phase 2)', () => { }); it('switches the exposed lower host on sidePanel without rebuilding either', () => { + // #487 phase 3: content repaint is decoupled from `sidePanel` entirely (the + // Library-content effect keys only on `dashboardTreeRevision`, and History has + // no reactive trigger of its own) — a plain sidePanel flip now repaints ONLY + // the tab row (active class/count), never either section's own content, in + // EITHER direction. const { app, handle } = mount(); const host = hosts(app.root); const libraryList = app.dom.savedList!; const historyList = app.dom.historyList!; - const marker = libraryList.appendChild(document.createElement('span')); + const libraryMarker = libraryList.appendChild(document.createElement('span')); + const historyMarker = historyList.appendChild(document.createElement('span')); app.state.sidePanel.value = 'history'; expect(host.library.hidden).toBe(true); expect(host.history.hidden).toBe(false); - // A hidden host keeps its DOM: History's repaint went into History's OWN list - // and left the Library's content standing. Before the split both sections - // rendered through one pair, so this content could not have survived. - expect(libraryList.contains(marker)).toBe(true); - expect(historyList.textContent).toContain('No history yet.'); + // A hidden host keeps its DOM, and switching TO it does not rebuild it either. + expect(libraryList.contains(libraryMarker)).toBe(true); + expect(historyList.contains(historyMarker)).toBe(true); app.state.sidePanel.value = 'saved'; expect(host.library.hidden).toBe(false); @@ -132,10 +136,49 @@ describe('mountAppShell wide navigation (#487 phase 2)', () => { expect(app.dom.historyList).toBe(historyList); expect(app.root.contains(libraryList)).toBe(true); expect(app.root.contains(historyList)).toBe(true); - // Becoming active DOES repaint the section, exactly as it always has: the - // switcher clears the shared search filter, so the list is rebuilt from - // scratch. #487 phase 3 owns whether a drawer should preserve it instead. - expect(libraryList.contains(marker)).toBe(false); + // Becoming active again STILL does not repaint either section — the + // deliberate #487 phase 3 behavior change from "activating a pane clears and + // rebuilds its search/list." + expect(libraryList.contains(libraryMarker)).toBe(true); + expect(historyList.contains(historyMarker)).toBe(true); + handle.dispose(); + }); + + // #487 phase 3: the search input node inside a section's host survives a plain + // sidePanel flip — proof that switching never triggers a destructive rebuild of + // either section's content (only the tab row/exposure react to it). + it('preserves node identity of a section\'s search input across a sidePanel flip', () => { + const { app, handle } = mount(); + app.state.savedQueries = [savedQuery({ id: 's1', name: 'Q1', sql: 'SELECT 1' })]; + app.state.dashboardTreeRevision.value++; // force one real Library content paint + const libraryInput = app.dom.savedSearch!.querySelector('.sv-search-input'); + expect(libraryInput).not.toBeNull(); + + app.state.sidePanel.value = 'history'; + app.state.sidePanel.value = 'saved'; + + expect(app.dom.savedSearch!.querySelector('.sv-search-input')).toBe(libraryInput); + handle.dispose(); + }); + + it('a bare dashboardTreeRevision bump (no sidePanel change) still repaints Library', () => { + const { app, handle } = mount(); + app.state.savedQueries = [savedQuery({ id: 's1', name: 'Q1', sql: 'SELECT 1' })]; + + app.state.dashboardTreeRevision.value++; + + expect(app.dom.savedList!.querySelectorAll('.saved-row')).toHaveLength(1); + handle.dispose(); + }); + + it('a sidePanel change alone still repaints the tab row\'s active class', () => { + const { app, handle } = mount(); + + app.state.sidePanel.value = 'history'; + + const tabs = [...app.dom.savedTabsRow!.querySelectorAll('.side-tab')]; + expect(tabs[0].classList.contains('active')).toBe(false); + expect(tabs[1].classList.contains('active')).toBe(true); handle.dispose(); }); @@ -150,13 +193,34 @@ describe('mountAppShell wide navigation (#487 phase 2)', () => { const { app, handle } = mount(); const host = hosts(app.root); app.state.savedQueries = [savedQuery({ id: 's1', name: 'Q1', sql: 'SELECT 1' })]; + app.state.dashboardTreeRevision.value++; // force Library's own content effect to (re)paint (app.state.sidePanel as { value: string }).value = 'queries'; expect(host.library.hidden).toBe(false); expect(host.history.hidden).toBe(true); expect(app.dom.savedList!.querySelectorAll('.saved-row')).toHaveLength(1); - expect(app.dom.historyList!.children.length).toBe(0); + // #487 phase 3: History now ALSO always renders its own content — its host is + // never truly blank, even when Library is exposed. + expect(app.dom.historyList!.textContent).toContain('No history yet.'); + handle.dispose(); + }); + + // #487 phase 3 regression test: the section that was NOT active at mount used + // to never get its first paint at all (blank until the first switch to it). + it('paints BOTH lower sections at mount, before either is ever switched to', () => { + const loadSchema = vi.fn(async () => {}); + const loadReference = vi.fn(async () => {}); + const app = makeApp({ catalog: { loadSchema, loadReference }, prefs: { save: vi.fn() } }); + app.state.savedQueries = [savedQuery({ id: 's1', name: 'Q1', sql: 'SELECT 1' })]; + app.state.history = [{ id: 'h1', sql: 'SELECT 1', ts: Date.now(), rows: 1, ms: 1 }]; + // sidePanel defaults to Library ('saved') — History is never exposed here. + const handle = mountAppShell({ + app, root: app.root, document, state: app.state, catalog: app.catalog, + prefs: app.prefs, matchMedia: null, updateBanner: vi.fn(), startDrag, + }); + + expect(app.dom.historyList!.querySelectorAll('.history-row')).toHaveLength(1); handle.dispose(); }); diff --git a/tests/unit/app.test.ts b/tests/unit/app.test.ts index 6707117..97979cb 100644 --- a/tests/unit/app.test.ts +++ b/tests/unit/app.test.ts @@ -2123,6 +2123,24 @@ describe('query run', () => { // a plain SELECT needs no session, so none is opened (avoids the session race) expect(asMock(app.conn.chCtx.fetch).mock.calls.map((c) => c[0]).some((u) => /session_id=/.test(u))).toBe(false); }); + // #487 phase 3 regression test: a run's history recording used to skip its + // repaint of History entirely whenever Library ('saved', the default side + // panel) was exposed — `app.recordHistory` guarded the call on + // `sidePanel.value === 'history'`. That left History's own DOM stale until + // some unrelated event happened to repaint it, which never happens for + // History on its own (`state.history` is a plain array, not a signal). The + // fix makes `app.recordHistory`'s repaint unconditional. + it('a recorded run repaints History even while Library is the exposed side panel', async () => { + const { app } = appForRun([ + [(u, sql) => /SELECT 1/.test(sql), resp({ body: streamBody(['{"meta":[{"name":"a","type":"UInt8"}]}\n', '{"row":{"a":"1"}}\n']) })], + ]); + expect(app.state.sidePanel.value).toBe('saved'); // Library is the exposed section + app.activeTab().sqlDraft = 'SELECT 1'; + await app.actions.run(); + expect(app.state.history.length).toBe(1); + expect(app.dom.historyList!.querySelectorAll('.history-row')).toHaveLength(1); + expect(app.dom.historyList!.textContent).toContain('SELECT 1'); + }); it('captures result.source for a normal row-returning result (#185)', async () => { const { app } = appForRun([ [(u, sql) => /SELECT 1/.test(sql), resp({ body: streamBody(['{"meta":[{"name":"a","type":"UInt8"}]}\n', '{"row":{"a":"1"}}\n']) })], diff --git a/tests/unit/saved-history.test.ts b/tests/unit/saved-history.test.ts index 9be988d..6b4d9ff 100644 --- a/tests/unit/saved-history.test.ts +++ b/tests/unit/saved-history.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi } from 'vitest'; -import { renderSavedHistory } from '../../src/ui/saved-history.js'; +import { renderSavedHistory, renderLibrarySection, renderHistorySection } from '../../src/ui/saved-history.js'; import { LIBRARY_QUERY_MIME, SUBQUERY_MIME } from '../../src/ui/dnd-mime.js'; import { queryDescription, queryFavorite, queryName } from '../../src/core/saved-query.js'; import { makeApp } from '../helpers/fake-app.js'; @@ -614,17 +614,23 @@ describe('renderSavedHistory', () => { // `state.ts` decodes `asb:sidePanel` at load, so this value cannot come from // storage — but the signal is settable by any module, and the two readers must // not be able to disagree. `app-shell.ts`'s exposure effect resolves anything - // that is not 'history' to the Library host; this renderer has to paint into - // the LIBRARY pair for the same input, or the pane shows an exposed empty host - // while the content sits inside the hidden one. + // that is not 'history' to the Library host. + // + // #487 phase 3: content rendering no longer depends on this decoding at all + // — both sections always render their own content regardless of which is + // exposed. What this decoding still controls is only the tab row's "active" + // class / `aria-pressed`, asserted here. const app = makeApp(); (app.state.sidePanel as { value: string }).value = 'queries'; setSaved(app, [{ id: 's1', name: 'Q1', sql: 'SELECT 1' }]); renderSavedHistory(app); expect(qsa(savedList(app), '.saved-row')).toHaveLength(1); - expect(historyList(app).children.length).toBe(0); - expect(qsa(savedTabsRow(app), '.side-tab')[0].classList.contains('active')).toBe(true); + const tabs = qsa(savedTabsRow(app), '.side-tab'); + expect(tabs[0].classList.contains('active')).toBe(true); + expect(tabs[0].getAttribute('aria-pressed')).toBe('true'); + expect(tabs[1].classList.contains('active')).toBe(false); + expect(tabs[1].getAttribute('aria-pressed')).toBe('false'); }); it('takes the lower tabs\' labels and icons FROM the registry', () => { @@ -651,13 +657,12 @@ describe('renderSavedHistory', () => { } }); - it('ignores input from a retained search box whose section is no longer active', () => { + it('a retained search box whose section is no longer exposed still writes only its OWN section (#487 phase 3)', () => { // #487 phase 2: the inactive section's host keeps its DOM, so its search input - // and listeners OUTLIVE the switch away from it. `state.libraryFilter` is still - // one shared string and `renderList` paints the ACTIVE section, so a stale - // event would rewrite the filter and repaint the OTHER section's list with this - // section's text. CSS makes it unreachable today; phase 3 moves hosts into - // containers where a host can be visible while another section is active. + // and listeners OUTLIVE the switch away from it. #487 phase 3 gives each + // section its own filter slot (`state.lowerNavigationFilters`), so a "stale" + // Library input firing while History is exposed can only ever write Library's + // own filter and repaint Library's own (hidden) list — never History's. const app = makeApp(); app.state.sidePanel.value = 'saved'; setSaved(app, [{ id: 's1', name: 'Carrier delays', sql: 'SELECT 1' }]); @@ -672,14 +677,20 @@ describe('renderSavedHistory', () => { staleInput.value = 'zzzz'; staleInput.dispatchEvent(new Event('input', { bubbles: true })); - // The shared filter is untouched and History still shows its row — no - // cross-section rewrite, no "No history matches “zzzz”". - expect(app.state.libraryFilter).toBe(''); + // Library's OWN filter took the input; History's own filter and rendered + // content are untouched — no cross-section rewrite, no "No history matches + // “zzzz”". + expect(app.state.lowerNavigationFilters.library).toBe('zzzz'); + expect(app.state.lowerNavigationFilters.history).toBe(''); expect(qsa(historyList(app), '.history-row')).toHaveLength(1); expect(historyList(app).textContent).not.toContain('zzzz'); + expect(savedList(app).textContent).toContain('No library queries match'); + expect(savedList(app).textContent).toContain('zzzz'); - // Escape on the stale input is inert for the same reason. + // Escape on the stale input clears LIBRARY's own filter, still without + // touching History. staleInput.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); + expect(app.state.lowerNavigationFilters.library).toBe(''); expect(qsa(historyList(app), '.history-row')).toHaveLength(1); }); @@ -695,6 +706,77 @@ describe('renderSavedHistory', () => { expect(app.state.sidePanel.value).toBe('saved'); expect(app.prefs.save).toHaveBeenCalledWith('sidePanel', 'saved'); }); + + // #572: the lower switcher's tabs had neither attribute, unlike the upper + // switcher's (`sidebar-upper.ts`), which already has both. + it('renders the lower tabs as real buttons with correct aria-pressed (#572)', () => { + const app = makeApp(); + app.state.sidePanel.value = 'saved'; + renderSavedHistory(app); + const [libraryTab, historyTab] = qsa(savedTabsRow(app), '.side-tab'); + expect(libraryTab.type).toBe('button'); + expect(libraryTab.getAttribute('aria-pressed')).toBe('true'); + expect(historyTab.type).toBe('button'); + expect(historyTab.getAttribute('aria-pressed')).toBe('false'); + + click(historyTab); + renderSavedHistory(app); + const [libraryTab2, historyTab2] = qsa(savedTabsRow(app), '.side-tab'); + expect(libraryTab2.getAttribute('aria-pressed')).toBe('false'); + expect(historyTab2.getAttribute('aria-pressed')).toBe('true'); + }); +}); + +describe('renderLibrarySection / renderHistorySection (#487 phase 3: independent of exposure)', () => { + // Direct regression test for the "first switch to a section shows empty" bug: + // before this phase, a section's content only ever painted while it was the + // ACTIVE section, so the section that was not active when the pane first + // mounted had never been painted. + it('renderHistorySection paints History\'s own host even while Library is the exposed section', () => { + const app = makeApp(); + app.state.sidePanel.value = 'saved'; // Library is exposed, not History + app.state.history = [{ id: 'h1', sql: 'SELECT 1', ts: Date.now(), rows: 1, ms: 1 }]; + renderHistorySection(app); + expect(qsa(historyList(app), '.history-row')).toHaveLength(1); + }); + + it('renderLibrarySection paints Library\'s own host even while History is the exposed section', () => { + const app = makeApp(); + app.state.sidePanel.value = 'history'; // History is exposed, not Library + setSaved(app, [{ id: 's1', name: 'Q1', sql: 'SELECT 1' }]); + renderLibrarySection(app); + expect(qsa(savedList(app), '.saved-row')).toHaveLength(1); + }); + + // Direct regression test for the "the section that wasn't active when its own + // data changed goes stale" bug, and its mirror. + it('a History-only mutation does not touch Library\'s DOM', () => { + const app = makeApp(); + app.state.sidePanel.value = 'saved'; + setSaved(app, [{ id: 's1', name: 'Q1', sql: 'SELECT 1' }]); + renderSavedHistory(app); + const before = savedList(app).innerHTML; + + app.state.history = [{ id: 'h1', sql: 'SELECT 1', ts: Date.now(), rows: 1, ms: 1 }]; + renderHistorySection(app); + + expect(savedList(app).innerHTML).toBe(before); + expect(qsa(historyList(app), '.history-row')).toHaveLength(1); + }); + + it('a Library-only mutation does not touch History\'s DOM', () => { + const app = makeApp(); + app.state.sidePanel.value = 'history'; + app.state.history = [{ id: 'h1', sql: 'SELECT 1', ts: Date.now(), rows: 1, ms: 1 }]; + renderSavedHistory(app); + const before = historyList(app).innerHTML; + + setSaved(app, [{ id: 's1', name: 'Q1', sql: 'SELECT 1' }]); + renderLibrarySection(app); + + expect(historyList(app).innerHTML).toBe(before); + expect(qsa(savedList(app), '.saved-row')).toHaveLength(1); + }); }); describe('renderSavedHistory — search/filter', () => { @@ -763,12 +845,12 @@ describe('renderSavedHistory — search/filter', () => { expect(savedList(app).textContent).toContain('No library queries match'); expect(savedList(app).textContent).toContain('zzzz'); click(qs(savedSearch(app), '.sv-search-clear')); - expect(app.state.libraryFilter).toBe(''); + expect(app.state.lowerNavigationFilters.library).toBe(''); expect(names(app)).toHaveLength(3); type(app, 'busiest'); expect(names(app)).toEqual(['Busiest airports']); input(app).dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); - expect(app.state.libraryFilter).toBe(''); + expect(app.state.lowerNavigationFilters.library).toBe(''); expect(names(app)).toHaveLength(3); }); @@ -788,12 +870,47 @@ describe('renderSavedHistory — search/filter', () => { expect(historyList(app).textContent).toContain('No history matches'); }); - it('clears the filter when switching tabs', () => { + // #487 phase 3: the OPPOSITE of the old behavior — switching used to clear + // the (single, shared) filter; now each section keeps its own, preserved + // across every switch. Sabotage-checked: reintroducing a clear on `switchTo` + // (or gating either section's render behind `activeSection`) makes this fail. + it('preserves each section\'s own filter text across every switch', () => { const app = savedApp(); type(app, 'delay'); - expect(app.state.libraryFilter).toBe('delay'); + expect(app.state.lowerNavigationFilters.library).toBe('delay'); + click(qsa(savedTabsRow(app), '.side-tab')[1]); // → History - expect(app.state.libraryFilter).toBe(''); + expect(app.state.sidePanel.value).toBe('history'); + expect(app.state.lowerNavigationFilters.library).toBe('delay'); // NOT cleared + expect(app.state.lowerNavigationFilters.history).toBe(''); + + // Give History its OWN, independent filter text. + app.state.history = [ + { id: 'h1', sql: 'SELECT 1', ts: Date.now(), rows: 1, ms: 1 }, + { id: 'h2', sql: 'INSERT INTO t', ts: Date.now(), rows: null, ms: 1 }, + ]; + renderSavedHistory(app); + const historyInput = qs(historySearch(app), '.sv-search-input'); + historyInput.value = 'insert'; + historyInput.dispatchEvent(new Event('input', { bubbles: true })); + expect(app.state.lowerNavigationFilters.history).toBe('insert'); + + click(qsa(savedTabsRow(app), '.side-tab')[0]); // → Library + expect(app.state.sidePanel.value).toBe('saved'); + expect(app.state.lowerNavigationFilters.library).toBe('delay'); // still preserved + expect(app.state.lowerNavigationFilters.history).toBe('insert'); // still preserved + + // Re-render (as the real app-shell repaint effects would) and confirm the + // preserved text is actually what's shown, not just stored. + renderSavedHistory(app); + expect(input(app).value).toBe('delay'); + expect(names(app)).toEqual(['Carrier delays']); + + click(qsa(savedTabsRow(app), '.side-tab')[1]); // → History again + renderSavedHistory(app); + expect(qs(historySearch(app), '.sv-search-input').value).toBe('insert'); + expect(qsa(historyList(app), '.history-row')).toHaveLength(1); + expect(historyList(app).textContent).toContain('INSERT INTO t'); }); }); diff --git a/tests/unit/state.test.ts b/tests/unit/state.test.ts index 861443c..5ccb008 100644 --- a/tests/unit/state.test.ts +++ b/tests/unit/state.test.ts @@ -209,6 +209,10 @@ describe('createState', () => { expect(s.expanded.value.size).toBe(0); expect(s.libraryName.value).toBe(DEFAULT_LIBRARY_NAME); expect(s.libraryDirty.value).toBe(false); + // #487 phase 3: each lower-navigation section keeps its own filter slot — + // both start empty, and (unlike the old single `libraryFilter` string) + // neither is ever cleared by the other switching. + expect(s.lowerNavigationFilters).toEqual({ library: '', history: '' }); // #287 W4: no aggregate loaded yet — `dashboard` starts null; // `loadWorkspaceOnBoot` (app.ts's async boot step) projects the real // aggregate onto both after this synchronous constructor. `workspaceId` diff --git a/tests/unit/workbench-session.test.ts b/tests/unit/workbench-session.test.ts index e21c966..89721aa 100644 --- a/tests/unit/workbench-session.test.ts +++ b/tests/unit/workbench-session.test.ts @@ -103,7 +103,7 @@ function makeState(over: Partial = {}): WorkbenchStateSlice function makeHooks(over: Partial = {}): WorkbenchHooks { return { renderResults: vi.fn(), - renderSavedHistory: vi.fn(), + renderHistorySection: vi.fn(), cancelSchemaGraph: vi.fn(), loadSchema: vi.fn(), recordHistory: vi.fn(), @@ -843,10 +843,10 @@ describe('createWorkbenchSession: runScript()', () => { await session.runScript(['SELECT 1'], 'SELECT 1'); expect(h.hooks.recordBoundParams).not.toHaveBeenCalled(); expect(h.state.history).toEqual([]); - expect(h.hooks.renderSavedHistory).not.toHaveBeenCalled(); + expect(h.hooks.renderHistorySection).not.toHaveBeenCalled(); }); - it('a clean run records one script history entry, and repaints History when open', async () => { + it('a clean run records one script history entry, and repaints History when it is the open panel', async () => { const h = makeHarness({ state: { sidePanel: signal('history') } }); h.execFakes.executeScript.mockImplementation(async (req: ScriptExecutionRequest) => { const entry = { sql: 'SELECT 1', status: 'rows' as const, columns: [], rows: [], truncated: false, preview: '', ms: 5 }; @@ -857,11 +857,16 @@ describe('createWorkbenchSession: runScript()', () => { await session.runScript(['SELECT 1'], 'SELECT 1; SELECT 1;'); expect(h.state.history).toHaveLength(1); expect(h.state.history[0].sql).toBe('SELECT 1; SELECT 1;'); - expect(h.hooks.renderSavedHistory).toHaveBeenCalled(); + expect(h.hooks.renderHistorySection).toHaveBeenCalled(); expect(h.hooks.saveJSON).toHaveBeenCalled(); }); - it('a clean run does not repaint History when a different side panel is open', async () => { + // #487 phase 3 regression test: History used to skip its repaint entirely + // whenever Library ('saved') was the exposed side panel, leaving History's + // content stale until some unrelated repaint happened to fire (which, for + // History, never does — `state.history` is a plain array, not part of any + // reactive effect). The fix makes this call unconditional. + it('a clean run repaints History even while a different side panel is open', async () => { const h = makeHarness({ state: { sidePanel: signal('saved') } }); h.execFakes.executeScript.mockImplementation(async (req: ScriptExecutionRequest) => { const entry = { sql: 'SELECT 1', status: 'ok' as const, ms: 5 }; @@ -871,7 +876,7 @@ describe('createWorkbenchSession: runScript()', () => { const session = createWorkbenchSession(h.deps); await session.runScript(['SELECT 1'], 'SELECT 1;'); expect(h.state.history).toHaveLength(1); - expect(h.hooks.renderSavedHistory).not.toHaveBeenCalled(); + expect(h.hooks.renderHistorySection).toHaveBeenCalled(); }); it('sets `cancelled` on the script result when aborted', async () => { From 2a115f04fa70bae5d4be6fe1e6428f75dec8cfae Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Thu, 30 Jul 2026 11:17:50 +0200 Subject: [PATCH 04/13] fix(#487): resize-session must compare raw proposals, not clamped ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent ChatGPT review of the phase-3 steps 1-3 diff found a real bug, confirmed by hand: commitLeftNavigationResize compared two POST-CLAMP layouts (effective vs effectiveAtStart) to decide whether a band's width changed. That's wrong for a restore command (Home/End/a bare-rail ArrowRight) under an active viewport clamp — a bare rail's dormant preferred width passes through effectiveAtStart unclamped (there is nothing to clamp yet), so a restore's honest, unclamped proposal gets compared against it, differs, and the TRANSIENT clamped render value gets committed instead of the user's real preference. E.g. a 420px preference restored on a ~800px window would silently downgrade to ~313px. Fix: the session now tracks the RAW reducer proposal separately from the clamped effective layout, and the commit decision compares the raw proposal against the preference, never the clamped value. This also folds the viewport clamp into advanceLeftNavigationResize itself, so a future caller can no longer forget to apply it. Also: openFocusedSection/toggleFocusedSection no longer re-persist sidePanel when the value is already current (relevant since #428's bounded drag-hover reasserts the same section repeatedly), a stale test that checked a field name already renamed in step 3, a dead field on WorkbenchStateSlice, and stale doc references in ADR-0001 and saved-query-service.ts. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NZfeGpkpJw2zsfPtysGHUN --- docs/ADR-0001-reactivity.md | 6 +- src/application/left-nav.ts | 19 ++- src/application/saved-query-service.ts | 6 +- src/core/left-nav-layout.ts | 194 +++++++++++++++---------- src/ui/workbench/workbench-session.ts | 15 -- tests/unit/left-nav-layout.test.ts | 183 ++++++++++++++++++----- tests/unit/left-nav.test.ts | 49 ++++++- tests/unit/workbench-session.test.ts | 15 +- 8 files changed, 343 insertions(+), 144 deletions(-) diff --git a/docs/ADR-0001-reactivity.md b/docs/ADR-0001-reactivity.md index 7bb8c76..7eaa78f 100644 --- a/docs/ADR-0001-reactivity.md +++ b/docs/ADR-0001-reactivity.md @@ -175,8 +175,10 @@ forgettable as the old manual `renderSchema` calls, revisit via a fresh ADR. `state.shortcutsOpen`, `state.editingSavedId`, and `state.bannerDismissedFor` (previously bare fields — the latter two lived on `app` directly, not `app.state`) were converted to `signal(...)` and consolidated into `state.js` -alongside the other session-only, non-persisted fields (`libraryFilter`, -`resultSort`). None had a reactive reader before or after — each site that sets +alongside the other session-only, non-persisted fields (`lowerNavigationFilters` +— renamed from `libraryFilter` by #487 phase 3, which split the one field into +per-section search text — and `resultSort`). None had a reactive reader before +or after — each site that sets one already calls its own repaint (`renderSavedHistory`, `updateBanner`, `openShortcuts`'s own mount/unmount) — so this is a pure `.value` mechanical edit, not a new `effect()`. Housing them in `state.js` rather than on `app` diff --git a/src/application/left-nav.ts b/src/application/left-nav.ts index 15b51b5..336147b 100644 --- a/src/application/left-nav.ts +++ b/src/application/left-nav.ts @@ -72,19 +72,28 @@ export function readLeftNavigationLayout(state: LeftNavStateSlice): LeftNavigati * `sidePanel`, so a rail/drawer selection of the same section left the * signal's NEW value unpersisted — a reload would silently revert to * whichever pane was last chosen through the wide tabs. Persisting BEFORE - * writing the signal mirrors `switchTo` exactly. `state.libraryFilter` is - * deliberately untouched — a later phase-3 step owns per-section filters. + * writing the signal mirrors `switchTo` exactly. Per-section filters + * (`state.lowerNavigationFilters`, phase 3 step 3) are already implemented + * elsewhere and this module still correctly never touches them. + * + * Both branches are guarded to a no-op when the target value is already + * current: `openFocusedSection`'s documented caller is #428's bounded + * drag-hover, which can re-assert the SAME section repeatedly on every hover + * notification, and `sidePanel`'s write has a real synchronous side effect + * (`app.prefs.save`) that must not fire on every one of those. */ function selectSectionInExistingPane(app: LeftNavApp, section: LeftNavigationSection): void { if (section === 'databases' || section === 'dashboards') { // Session-only, like `switchTo`'s counterpart for the upper pane: `upperRole` // is never persisted (state.ts), so there is nothing to save here. - app.state.upperRole.value = section; + if (app.state.upperRole.value !== section) app.state.upperRole.value = section; return; } const panel = sidePanelKeyFor(section); - app.prefs.save('sidePanel', panel); - app.state.sidePanel.value = panel; + if (app.state.sidePanel.value !== panel) { + app.prefs.save('sidePanel', panel); + app.state.sidePanel.value = panel; + } } /** diff --git a/src/application/saved-query-service.ts b/src/application/saved-query-service.ts index c383261..04392d0 100644 --- a/src/application/saved-query-service.ts +++ b/src/application/saved-query-service.ts @@ -146,8 +146,10 @@ export interface SavedQueryService { commit(tab: QueryTab, evaluated: { parsed: unknown; diagnostics: SpecValidationDiagnostic[] }): Promise; /** Record a successful run in history (state.ts's own `recordHistory`) — * never touches rendering; app.ts's own `app.recordHistory` delegate - * conditionally repaints the History side panel itself after calling - * this. */ + * unconditionally repaints History's own content after calling this + * (#487 phase 3 removed the `sidePanel === 'history'` guard, since + * History's content must stay current regardless of which lower-navigation + * section is currently exposed). */ recordHistory(tab: QueryTab, sqlText?: string): void; /** Build the shareable URL for an already-evaluated Spec, or a typed * rejection reason — never writes `location`/clipboard itself. */ diff --git a/src/core/left-nav-layout.ts b/src/core/left-nav-layout.ts index c6b2c2f..6bdb692 100644 --- a/src/core/left-nav-layout.ts +++ b/src/core/left-nav-layout.ts @@ -532,20 +532,33 @@ export interface LeftNavigationSeparatorAria { * ceiling is `LEFT_PANEL_MAX_PX` exactly as before phase 3 — this parameter is * additive and every existing caller (there is still none in production, but the * unit tests below stand in for one) keeps its prior behaviour unconditionally. + * + * `valueMax` is floored at `valueMin` (`LEFT_RAIL_PX`) and `valueNow` is + * clamped into `[valueMin, valueMax]`, so the ARIA invariant `valueMin <= + * valueNow <= valueMax` holds for ANY budget, including one pathologically + * below the rail's own width or below the current mode's own floor. #487's + * planned production UI never reaches that range in practice — see + * `clampLeftNavigationToMaximumTotal`'s own comment for the arithmetic showing + * the realistic budget floor sits around 280px, well above every mode's own + * minimum — so this is defensive robustness on a PUBLIC pure helper rather + * than a behaviour change for any realistic budget: neither clamp does + * anything when `maxNavigationTotalPx` is omitted or at least `LEFT_RAIL_PX`. */ export function leftNavigationSeparatorAria( layout: LeftNavigationLayout, maxNavigationTotalPx?: number, ): LeftNavigationSeparatorAria { - const valueMax = Number.isFinite(maxNavigationTotalPx) && (maxNavigationTotalPx as number) > 0 + const valueMin = LEFT_RAIL_PX; + const uncappedMax = Number.isFinite(maxNavigationTotalPx) && (maxNavigationTotalPx as number) > 0 ? Math.min(LEFT_PANEL_MAX_PX, maxNavigationTotalPx as number) : LEFT_PANEL_MAX_PX; - return { - valueMin: LEFT_RAIL_PX, - valueMax, - // Normalized, so a caller holding a layout with a non-finite width cannot - // publish `aria-valuenow="NaN"` to assistive technology. - valueNow: leftNavigationWidthPx(normalizeLeftNavigationLayout(layout)), - }; + const valueMax = Math.max(valueMin, uncappedMax); + // Normalized, so a caller holding a layout with a non-finite width cannot + // publish `aria-valuenow="NaN"` to assistive technology; clamped into the + // final [valueMin, valueMax] range so an occupied width that exceeds a + // pathologically small budget cannot publish an out-of-range valueNow either. + const rawValueNow = leftNavigationWidthPx(normalizeLeftNavigationLayout(layout)); + const valueNow = clamp(rawValueNow, valueMin, valueMax); + return { valueMin, valueMax, valueNow }; } /** @@ -646,107 +659,142 @@ export function clampLeftNavigationToMaximumTotal( * - `preferredAtStart` — the persisted preference as of when the gesture began. * This is the memory source `commitLeftNavigationResize` reconstructs from, * and it is captured ONCE, so no intermediate frame can overwrite it. - * - `effectiveAtStart` — what was actually rendered when the gesture began, - * i.e. `preferredAtStart` after `clampLeftNavigationToMaximumTotal` ran - * against whatever the viewport allowed at that moment. This can differ from - * `preferredAtStart` — a maximized preference squeezed by a narrow window — - * and the gap between the two is precisely what lets the commit step tell - * "the user actually resized this band" apart from "the band was just - * rendered smaller than preferred by an unrelated viewport constraint". - * - `effective` — the live, post-clamp layout, replaced wholesale on every - * `advanceLeftNavigationResize` call. This is what gets rendered and reported - * as the gesture continues; it is not memory. + * - `proposed` — the latest RAW reducer proposal, BEFORE the viewport's + * maximum-total clamp runs. This is the OTHER memory source + * `commitLeftNavigationResize` reads: comparing the raw proposal against + * `preferredAtStart` is what lets a restore command commit the user's actual + * remembered width even while a narrow viewport cannot render it in full — + * see the bug history below. + * - `effective` — `proposed` after `clampLeftNavigationToMaximumTotal` has run + * against the current viewport budget. This is what gets rendered and + * reported as the gesture continues; it is not memory, and + * `commitLeftNavigationResize` never reads it for its width decision. + * + * **A real bug this shape fixes, not a hypothetical one.** An earlier version + * of this session compared `effective` against an `effectiveAtStart` snapshot — + * i.e. two POST-CLAMP layouts — to decide whether a band's width had "changed". + * That is wrong whenever a restore command (`Home`, `End`, or a bare-rail + * `ArrowRight`) runs while the clamp is active: starting at a bare rail, + * `effectiveAtStart.wideWidthPx` passes the dormant preferred width straight + * through unclamped (there is nothing to clamp — 'wide' is not the rendered + * mode yet), so it read as the full preference, e.g. 420. `End` then proposed + * exactly 420 back — correct — but if the viewport only allows 313, `effective` + * rendered 313, `effective.wideWidthPx (313) !== effectiveAtStart.wideWidthPx + * (420)` read as true, and the old logic committed 313: a transient, + * viewport-driven value the user never asked to keep, silently overwriting + * their real preference. Comparing the RAW `proposed` (420) against + * `preferredAtStart` (420) instead finds no change, because + * `clampLeftNavigationToMaximumTotal` never touches `mode` — so `mode`/ + * `focusedSection` read identically off `proposed` or `effective`, and only the + * WIDTH needs the pre/post-clamp distinction the session now keeps. * * **`commitLeftNavigationResize`'s table, restated as one rule:** only commit a - * band's width if that band is the one `effective` currently renders AND its - * rendered width actually differs from `effectiveAtStart`'s. Every other case — - * a dormant band, a fold-through to bare rail, a click-and-release with no - * movement — preserves `preferredAtStart` for that band UNCONDITIONALLY. Two - * consequences fall out of that one rule rather than needing their own case: + * band's width if that band is the one the session's FINAL `proposed` layout is + * in, AND its raw proposed width actually differs from `preferredAtStart`'s. + * Every other case — a dormant band, a fold-through to bare rail, a + * click-and-release with no movement — preserves `preferredAtStart` for that + * band UNCONDITIONALLY. Two consequences fall out of that one rule rather than + * needing their own case: * * 1. **The dormant-band fix.** A gesture that resizes the drawer and THEN * crosses into wide mode must not commit the drawer's mid-gesture width, - * because the drawer is no longer the band `effective` renders once the - * session ends — `drawerChanged` requires `effective.mode === 'rail'`, which - * is false at wide, so the drawer memory falls through to - * `preferredAtStart.drawerWidthPx` untouched. Without that mode guard, a - * drag that opened the drawer to 300 before continuing on to a 350px wide - * sidebar would silently overwrite the drawer's remembered width with a - * value the user never asked to keep. - * 2. **Preferred wins over effective on a fold-through.** Ending at bare rail - * preserves BOTH widths from `preferredAtStart`, never from `effectiveAtStart` - * or `effective` — so a maximized 420px preference that a narrow viewport - * rendered at a clamped 313px, then folded to rail by the same gesture, - * still remembers 420 for the next `End`/restore. Committing `313` instead - * would silently downgrade a preference the user never touched, purely - * because the viewport happened to be narrow during an unrelated fold. + * because the drawer is no longer the band the FINAL `proposed` is in — + * `drawerChanged` requires `proposed.mode === 'rail'`, which is false once + * the session ends at wide, so the drawer memory falls through to + * `preferredAtStart.drawerWidthPx` untouched regardless of what the + * drawer's transient width was mid-drag. + * 2. **Preferred wins over a viewport clamp on ANY commit, not only a + * fold-through.** Because the comparison is always RAW-proposed vs + * preferred, never rendered-effective vs anything, a maximized 420px + * preference that a narrow viewport can only render at a clamped 313px + * still commits the user's honest 420 whenever the session ends without an + * actual new proposal for that band — restoring it in full the next time + * there is room, exactly as #487's "a viewport clamp must never downgrade a + * stored preference" requires. * * `Home`/`End`/a bare-rail `ArrowRight` restore need no special case either: - * they are restore commands, so the `effective` layout they produce typically - * already equals `preferredAtStart`'s remembered width for the band they - * restore, which is exactly the "nothing changed" shape the general rule + * they are restore commands, so the RAW `proposed` layout they produce + * typically already equals `preferredAtStart`'s remembered width for the band + * they restore, which is exactly the "nothing changed" shape the general rule * preserves correctly. * - * A session is deliberately NOT a reducer step in `resolveLeftNavigationDrag`'s - * family — `advanceLeftNavigationResize` does not call the mode reducer or the - * maximum-total clamp itself. The caller runs those first to produce the next - * `effective` layout (a pointer/keyboard event resolves through the existing - * reducers, then `clampLeftNavigationToMaximumTotal` fits it to the viewport), - * and only then advances the session with the result. Session bookkeeping and - * layout arithmetic stay two separate concerns, so the arithmetic keeps its - * one implementation. + * `advanceLeftNavigationResize` performs the viewport clamp ITSELF now (taking + * the raw proposal and the current budget as arguments), rather than asking the + * caller to clamp first and hand in an already-clamped layout — a future + * caller (the pointer/keyboard handler a later phase-3 step builds, which does + * not exist yet) cannot forget the clamp, because it is part of this + * function's contract rather than caller discipline. The MODE arithmetic + * itself stays out of this module either way — `advanceLeftNavigationResize` + * still does not call `resolveLeftNavigationDrag`/`resolveLeftNavigationKey`; + * the caller runs the proposal through those first, then hands the raw result + * here alongside the viewport budget. Session bookkeeping and layout + * arithmetic stay two separate concerns, so the arithmetic keeps its one + * implementation. */ export interface LeftNavigationResizeSession { /** The persisted preference as of session start — the memory source every * commit is reconstructed from, band by band. */ readonly preferredAtStart: LeftNavigationLayout; - /** What was actually rendered when the session began, i.e. - * `preferredAtStart` after the viewport's maximum-total clamp. */ - readonly effectiveAtStart: LeftNavigationLayout; - /** The live, post-clamp layout — what is rendered and reported right now. */ + /** The latest RAW reducer proposal, BEFORE the viewport clamp — the other + * memory source `commitLeftNavigationResize` reads from. */ + readonly proposed: LeftNavigationLayout; + /** The latest proposal AFTER the viewport clamp — what is rendered and + * reported right now. Never read by the commit decision. */ readonly effective: LeftNavigationLayout; } -/** Begin a resize session: `effective` starts out equal to `effectiveAtStart`, - * since nothing has moved yet. */ +/** Begin a resize session: `proposed` starts out equal to `preferred` (nothing + * has moved yet), and `effective` is `preferred` clamped to the viewport + * budget at hand — mirroring what a caller would render before any gesture + * begins. */ export function beginLeftNavigationResize( - preferred: LeftNavigationLayout, effective: LeftNavigationLayout, + preferred: LeftNavigationLayout, maxNavigationTotalPx: number, ): LeftNavigationResizeSession { - return { preferredAtStart: preferred, effectiveAtStart: effective, effective }; + return { + preferredAtStart: preferred, + proposed: preferred, + effective: clampLeftNavigationToMaximumTotal(preferred, maxNavigationTotalPx), + }; } /** - * Advance a session to a new live layout. Pure snapshot replacement — the - * caller has already run the layout through `resolveLeftNavigationDrag` / - * `resolveLeftNavigationKey` and `clampLeftNavigationToMaximumTotal` to produce - * `nextEffectiveLayout`; this function does not call either. Returns the SAME - * session when the layout is unchanged by reference, so a caller can use - * identity to skip a repaint exactly as the mode reducers do. + * Advance a session to a new RAW proposal. The caller has already run the + * layout through `resolveLeftNavigationDrag`/`resolveLeftNavigationKey` to + * produce `proposedLayout`; this function applies the viewport clamp itself + * (see this section's block comment for why the clamp lives here rather than + * in the caller) and records both the raw proposal and its clamped `effective` + * counterpart. Returns the SAME session when neither changes by reference, so + * a caller can use identity to skip a repaint exactly as the mode reducers do. */ export function advanceLeftNavigationResize( - session: LeftNavigationResizeSession, nextEffectiveLayout: LeftNavigationLayout, + session: LeftNavigationResizeSession, proposedLayout: LeftNavigationLayout, maxNavigationTotalPx: number, ): LeftNavigationResizeSession { - return nextEffectiveLayout === session.effective + const effective = clampLeftNavigationToMaximumTotal(proposedLayout, maxNavigationTotalPx); + return proposedLayout === session.proposed && effective === session.effective ? session - : { ...session, effective: nextEffectiveLayout }; + : { ...session, proposed: proposedLayout, effective }; } /** * Reconstruct the `LeftNavigationLayout` to persist from a resize session — see * this section's block comment above for the rule and why it is shaped this * way. `mode` and `focusedSection` always follow wherever the session ended - * (a legitimate mode transition, not a width memory question); only the two - * WIDTHS get the preserve-vs-commit treatment, band by band. + * (a legitimate mode transition, not a width memory question) — read off + * `effective`, since `clampLeftNavigationToMaximumTotal` never changes `mode`, + * so `effective.mode`/`effective.focusedSection` agree with `proposed`'s + * exactly. Only the two WIDTHS get the preserve-vs-commit treatment, band by + * band, and that decision is made against the RAW `proposed` width, never + * `effective`'s — the fix this function exists for. */ export function commitLeftNavigationResize(session: LeftNavigationResizeSession): LeftNavigationLayout { - const { preferredAtStart, effectiveAtStart, effective } = session; - const wideChanged = effective.mode === 'wide' && effective.wideWidthPx !== effectiveAtStart.wideWidthPx; - const drawerChanged = effective.mode === 'rail' && effective.focusedSection !== null - && effective.drawerWidthPx !== effectiveAtStart.drawerWidthPx; + const { preferredAtStart, proposed, effective } = session; + const wideChanged = proposed.mode === 'wide' && proposed.wideWidthPx !== preferredAtStart.wideWidthPx; + const drawerChanged = proposed.mode === 'rail' && proposed.focusedSection !== null + && proposed.drawerWidthPx !== preferredAtStart.drawerWidthPx; return normalizeLeftNavigationLayout({ mode: effective.mode, focusedSection: effective.focusedSection, - wideWidthPx: wideChanged ? effective.wideWidthPx : preferredAtStart.wideWidthPx, - drawerWidthPx: drawerChanged ? effective.drawerWidthPx : preferredAtStart.drawerWidthPx, + wideWidthPx: wideChanged ? proposed.wideWidthPx : preferredAtStart.wideWidthPx, + drawerWidthPx: drawerChanged ? proposed.drawerWidthPx : preferredAtStart.drawerWidthPx, }); } diff --git a/src/ui/workbench/workbench-session.ts b/src/ui/workbench/workbench-session.ts index 741c2a2..b30aa20 100644 --- a/src/ui/workbench/workbench-session.ts +++ b/src/ui/workbench/workbench-session.ts @@ -66,21 +66,6 @@ export interface WorkbenchStateSlice { forceExplain: boolean; resultRowLimit: number; serverVersion: string | null; - /** - * #487 phase 3: no longer read by this session's own logic — runScript's - * clean-run history repaint (`hooks.renderHistorySection()`) is now - * unconditional, since History's content must stay current regardless of - * which lower-navigation section is exposed. Kept on the slice because - * `AppState` carries it regardless (structural pass-through) and a later - * caller may still need it; if it stays unread, a future cleanup can drop it. - * - * Derived from `AppState` rather than restated as `Signal` (#487 - * phase 2): the real signal holds a decoded `'saved' | 'history'`, and a - * structural `Signal` here would leave this session type-authorized to - * write an arbitrary string into it — re-opening exactly the divergence the - * load-boundary decode closes. - */ - sidePanel: AppState['sidePanel']; isMobile: Signal; mobileView: Signal<'tables' | 'editor' | 'results'>; /** Read by the Run-button effect (Run ↔ "Run selection" label). */ diff --git a/tests/unit/left-nav-layout.test.ts b/tests/unit/left-nav-layout.test.ts index 28737cc..6dbe796 100644 --- a/tests/unit/left-nav-layout.test.ts +++ b/tests/unit/left-nav-layout.test.ts @@ -795,6 +795,36 @@ describe('leftNavigationSeparatorAria', () => { .toBe(LEFT_PANEL_MAX_PX); }); }); + + // A pathologically small budget (below LEFT_RAIL_PX, or below an open + // drawer's own floor) is unreachable in the planned production UI — + // `clampLeftNavigationToMaximumTotal`'s own comment works out the realistic + // budget floor at ~281px — but this is a PUBLIC pure helper, so the + // valueMin <= valueNow <= valueMax invariant must hold defensively for any + // budget rather than relying on a caller never passing one this small. + describe('the valueMin <= valueNow <= valueMax invariant for pathologically small budgets', () => { + it.each([1, 47, 100])('holds for a bare rail at budget %ipx', (budget) => { + const { valueMin, valueMax, valueNow } = leftNavigationSeparatorAria(rail(), budget); + expect(valueMin).toBeLessThanOrEqual(valueNow); + expect(valueNow).toBeLessThanOrEqual(valueMax); + }); + it('holds for an open drawer at a budget below the drawer\'s own floor', () => { + // The drawer's floor is LEFT_RAIL_PX + LEFT_FOLD_THRESHOLD_PX (188); pick a + // budget well below that so valueNow (the drawer's occupied width) would + // exceed an unclamped valueMax. + const layout = rail({ focusedSection: 'library', drawerWidthPx: LEFT_WIDE_THRESHOLD_PX }); + const { valueMin, valueMax, valueNow } = leftNavigationSeparatorAria(layout, 50); + expect(valueMin).toBeLessThanOrEqual(valueNow); + expect(valueNow).toBeLessThanOrEqual(valueMax); + // valueMax never drops below valueMin, even under a budget below the rail. + expect(valueMax).toBeGreaterThanOrEqual(valueMin); + }); + it('does not change behaviour for any realistic budget (existing tests above stay exact)', () => { + // Re-assert one exact case from the "shrinks valueMax" test above: the + // floor-at-valueMin change must be a no-op once the budget clears LEFT_RAIL_PX. + expect(leftNavigationSeparatorAria(wide({ wideWidthPx: 200 }), 300).valueMax).toBe(300); + }); + }); }); describe('clampLeftNavigationToMaximumTotal (#487 phase 3)', () => { @@ -865,28 +895,45 @@ describe('clampLeftNavigationToMaximumTotal (#487 phase 3)', () => { // The resize-session design's own comment block (above `LeftNavigationResizeSession` // in left-nav-layout.ts) explains WHY it is shaped the way it is; these tests are -// the counter-examples that shape was reviewed against. +// the counter-examples that shape was reviewed against. `Infinity` is used +// throughout as "no viewport constraint" — `clampLeftNavigationToMaximumTotal` +// treats any non-finite budget as unconstrained, so it is a clean way to +// exercise the session's bookkeeping without an unrelated clamp in the way. describe('resize session (#487 phase 3)', () => { - it('begin captures both inputs, with effective starting equal to effectiveAtStart', () => { + it('begin captures preferred as the initial proposal, and clamps effective to the budget', () => { const preferred = wide({ wideWidthPx: 420 }); - const effective = wide({ wideWidthPx: 313 }); // squeezed by a narrow viewport - const session = beginLeftNavigationResize(preferred, effective); + const session = beginLeftNavigationResize(preferred, 313); // a narrow viewport expect(session.preferredAtStart).toBe(preferred); - expect(session.effectiveAtStart).toBe(effective); - expect(session.effective).toBe(effective); + expect(session.proposed).toBe(preferred); + expect(session.effective).toEqual(wide({ wideWidthPx: 313 })); + }); + + it('begin leaves effective equal to preferred when the budget does not constrain it', () => { + const preferred = wide({ wideWidthPx: 300 }); + const session = beginLeftNavigationResize(preferred, Infinity); + expect(session.effective).toBe(preferred); }); - it('advance replaces effective and returns the same session by reference when unchanged', () => { + it('advance replaces proposed/effective and returns the same session by reference when unchanged', () => { const start = wide({ wideWidthPx: 300 }); - const session = beginLeftNavigationResize(start, start); - const same = advanceLeftNavigationResize(session, start); + const session = beginLeftNavigationResize(start, Infinity); + const same = advanceLeftNavigationResize(session, start, Infinity); expect(same).toBe(session); const moved = wide({ wideWidthPx: 320 }); - const advanced = advanceLeftNavigationResize(session, moved); + const advanced = advanceLeftNavigationResize(session, moved, Infinity); expect(advanced).not.toBe(session); - expect(advanced.effective).toBe(moved); + expect(advanced.proposed).toBe(moved); + expect(advanced.effective).toEqual(moved); expect(advanced.preferredAtStart).toBe(session.preferredAtStart); - expect(advanced.effectiveAtStart).toBe(session.effectiveAtStart); + }); + + it('advance clamps a new proposal against the given budget', () => { + const start = wide({ wideWidthPx: 300 }); + const session = beginLeftNavigationResize(start, Infinity); + const proposedLayout = wide({ wideWidthPx: 350 }); + const advanced = advanceLeftNavigationResize(session, proposedLayout, 313); + expect(advanced.proposed).toBe(proposedLayout); // raw, unclamped + expect(advanced.effective.wideWidthPx).toBe(313); // clamped for rendering }); // Each row of commitLeftNavigationResize's table, driven through a single @@ -894,24 +941,24 @@ describe('resize session (#487 phase 3)', () => { describe('commit table', () => { it('a wide resize that changed width commits it, leaving the drawer memory untouched', () => { const start = wide({ wideWidthPx: 250, drawerWidthPx: 210 }); - let session = beginLeftNavigationResize(start, start); - session = advanceLeftNavigationResize(session, wide({ wideWidthPx: 320, drawerWidthPx: 210 })); + let session = beginLeftNavigationResize(start, Infinity); + session = advanceLeftNavigationResize(session, wide({ wideWidthPx: 320, drawerWidthPx: 210 }), Infinity); expect(commitLeftNavigationResize(session)).toEqual(wide({ wideWidthPx: 320, drawerWidthPx: 210 })); }); it('a drawer resize that changed width commits it, leaving the wide memory untouched', () => { const start = rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 200 }); - let session = beginLeftNavigationResize(start, start); + let session = beginLeftNavigationResize(start, Infinity); session = advanceLeftNavigationResize( - session, rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 230 })); + session, rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 230 }), Infinity); expect(commitLeftNavigationResize(session)).toEqual( rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 230 })); }); it('a fold-through to bare rail preserves both remembered widths', () => { const start = wide({ wideWidthPx: 300, drawerWidthPx: 210 }); - let session = beginLeftNavigationResize(start, start); - session = advanceLeftNavigationResize(session, resolveLeftNavigationDrag(start, 10)); + let session = beginLeftNavigationResize(start, Infinity); + session = advanceLeftNavigationResize(session, resolveLeftNavigationDrag(start, 10), Infinity); const committed = commitLeftNavigationResize(session); expect(committed.mode).toBe('rail'); expect(committed.focusedSection).toBeNull(); @@ -919,23 +966,26 @@ describe('resize session (#487 phase 3)', () => { expect(committed.drawerWidthPx).toBe(210); }); - it('a no-op (advance to the same effective layout) commits both preserved', () => { + it('a no-op (advance to the same proposed layout) commits both preserved', () => { const start = wide({ wideWidthPx: 300, drawerWidthPx: 210 }); - let session = beginLeftNavigationResize(start, start); - session = advanceLeftNavigationResize(session, wide({ wideWidthPx: 300, drawerWidthPx: 210 })); + let session = beginLeftNavigationResize(start, Infinity); + session = advanceLeftNavigationResize(session, wide({ wideWidthPx: 300, drawerWidthPx: 210 }), Infinity); expect(commitLeftNavigationResize(session)).toEqual(start); }); }); it('the dormant-band case: a drawer resize followed by a crossing into wide does not commit the transient drawer width', () => { const start = rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 200 }); - let session = beginLeftNavigationResize(start, start); + let session = beginLeftNavigationResize(start, Infinity); // Resize the drawer open further … session = advanceLeftNavigationResize( - session, rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 259 })); + session, rail({ focusedSection: 'library', wideWidthPx: 260, drawerWidthPx: 259 }), Infinity); // … then keep dragging past the wide threshold, converting to the wide sidebar. - const wideLayout = resolveLeftNavigationDrag(session.effective, LEFT_RAIL_PX + LEFT_WIDE_THRESHOLD_PX + 40); - session = advanceLeftNavigationResize(session, wideLayout); + // Dragging continues from the RAW proposal, not the rendered `effective` — + // in this test they agree (no clamp is active), but `proposed` is the + // physically correct thing for a caller to keep dragging from. + const wideLayout = resolveLeftNavigationDrag(session.proposed, LEFT_RAIL_PX + LEFT_WIDE_THRESHOLD_PX + 40); + session = advanceLeftNavigationResize(session, wideLayout, Infinity); const committed = commitLeftNavigationResize(session); expect(committed.mode).toBe('wide'); // The wide memory reflects where the session actually ended. @@ -945,22 +995,81 @@ describe('resize session (#487 phase 3)', () => { expect(committed.drawerWidthPx).toBe(200); }); - it('the preferred/effective divergence case: a fold-through commits the PREFERRED wide width, not the viewport-clamped effective one', () => { + it('a fold-through from a viewport-clamped wide layout still commits the PREFERRED wide width, not the clamped one', () => { const preferred = wide({ wideWidthPx: 420, drawerWidthPx: 210 }); - // The viewport at session start could only render 313px, well inside the - // legal wide band, so this is a legitimate effectiveAtStart on its own. - const effectiveAtStart = wide({ wideWidthPx: 313, drawerWidthPx: 210 }); - let session = beginLeftNavigationResize(preferred, effectiveAtStart); - // The gesture keeps going left and folds all the way to bare rail. - session = advanceLeftNavigationResize(session, resolveLeftNavigationDrag(effectiveAtStart, 10)); + const budget = 313; // narrow viewport: renders 313 even though the mode is wide + let session = beginLeftNavigationResize(preferred, budget); + expect(session.effective.wideWidthPx).toBe(313); + // The gesture continues from the rendered position and folds all the way + // to bare rail. + session = advanceLeftNavigationResize(session, resolveLeftNavigationDrag(session.effective, 10), budget); const committed = commitLeftNavigationResize(session); expect(committed.mode).toBe('rail'); - // 420, the PREFERRED value — not 313, the clamped value the session actually + // 420, the PREFERRED value — never 313, the value the session actually // rendered at the start. expect(committed.wideWidthPx).toBe(420); expect(committed.drawerWidthPx).toBe(210); }); + // THE regression test for the confirmed bug: comparing two POST-CLAMP + // layouts (the old `effective` vs `effectiveAtStart` design) let a viewport + // clamp silently downgrade a stored preference on a plain restore. Comparing + // the RAW proposal against `preferredAtStart` instead — this function's fix — + // must commit the user's honest, full preference regardless of what the + // viewport could render. + it('BUG regression: restoring a bare rail to a viewport-clamped wide width commits the honest 420, not the rendered 313', () => { + const start = rail({ wideWidthPx: 420, drawerWidthPx: 210 }); // dormant wide preference: 420 + const budget = 313; + let session = beginLeftNavigationResize(start, budget); + // Sanity: a bare rail's occupied width is the fixed LEFT_RAIL_PX, so there + // is nothing for the clamp to shrink yet — the dormant 420 preference + // passes through `effective` untouched even though the budget could not + // render it as a wide sidebar. + expect(session.effective.wideWidthPx).toBe(420); + + // `End` (a restore command) proposes restoring the REMEMBERED width — the + // RAW, pre-clamp proposal — exactly. + const proposedLayout = resolveLeftNavigationKey(start, { key: 'End' }); + expect(proposedLayout?.mode).toBe('wide'); + expect(proposedLayout?.wideWidthPx).toBe(420); + + session = advanceLeftNavigationResize(session, proposedLayout as LeftNavigationLayout, budget); + // The viewport clamp still shrinks what gets RENDERED … + expect(session.effective.wideWidthPx).toBe(313); + // … but the commit must reconstruct from the raw proposal (420), which + // equals preferredAtStart.wideWidthPx (420) — no real change — so the + // honest preference survives, rather than the transient 313 the viewport + // happened to allow. + const committed = commitLeftNavigationResize(session); + expect(committed.mode).toBe('wide'); + expect(committed.wideWidthPx).toBe(420); + }); + + it('a genuinely new user-driven proposal commits even when effective was clamped for an unrelated reason earlier in the session', () => { + const start = wide({ wideWidthPx: 300, drawerWidthPx: 210 }); + let session = beginLeftNavigationResize(start, 250); // clamped at session start + expect(session.effective.wideWidthPx).toBe(250); + // The user then drags to a genuinely new width, well inside any constraint. + const proposedLayout = wide({ wideWidthPx: 200, drawerWidthPx: 210 }); + session = advanceLeftNavigationResize(session, proposedLayout, 250); + expect(commitLeftNavigationResize(session).wideWidthPx).toBe(200); + }); + + it('a drag proposal the viewport clamps commits the HONEST proposal, not the rendered value (deliberate design choice)', () => { + // The user's real intent should be restorable later when there is room — + // committing the rendered/clamped value instead would silently downgrade a + // preference the user never asked to change, purely because of a + // transient viewport constraint mid-gesture. This mirrors the fold-through + // case above, but for an ordinary in-band resize rather than a restore. + const start = wide({ wideWidthPx: 300, drawerWidthPx: 210 }); + let session = beginLeftNavigationResize(start, Infinity); + const proposedLayout = wide({ wideWidthPx: 350, drawerWidthPx: 210 }); + session = advanceLeftNavigationResize(session, proposedLayout, 313); + expect(session.effective.wideWidthPx).toBe(313); // rendered, clamped + const committed = commitLeftNavigationResize(session); + expect(committed.wideWidthPx).toBe(350); // committed: the honest proposal + }); + it('a dense sweep and a coarse sweep over the same drag commit the same result — even though the underlying reducer state they pass through provably disagrees', () => { // This is the exact counter-example the "restore memory is sampling-dependent // (phase 3 obligation)" test above pins at the raw-reducer level: dragging @@ -972,20 +1081,20 @@ describe('resize session (#487 phase 3)', () => { const start = wide({ wideWidthPx: 300, drawerWidthPx: 210 }); // Dense: samples the dead zone (179) before the fold. - let denseSession = beginLeftNavigationResize(start, start); + let denseSession = beginLeftNavigationResize(start, Infinity); let denseLayout: LeftNavigationLayout = start; for (const x of [200, 179, 139]) { denseLayout = resolveLeftNavigationDrag(denseLayout, x); - denseSession = advanceLeftNavigationResize(denseSession, denseLayout); + denseSession = advanceLeftNavigationResize(denseSession, denseLayout, Infinity); } // Sanity check on the known artifact this dense path produces. expect(denseLayout.mode).toBe('rail'); expect(denseLayout.wideWidthPx).toBe(LEFT_PANEL_MIN_PX); // Coarse: one jump straight past the fold threshold, skipping the dead zone. - let coarseSession = beginLeftNavigationResize(start, start); + let coarseSession = beginLeftNavigationResize(start, Infinity); const coarseLayout = resolveLeftNavigationDrag(start, 139); - coarseSession = advanceLeftNavigationResize(coarseSession, coarseLayout); + coarseSession = advanceLeftNavigationResize(coarseSession, coarseLayout, Infinity); // Sanity check on the known — and DIFFERENT — artifact the coarse path // produces for the very same underlying field. expect(coarseLayout.mode).toBe('rail'); diff --git a/tests/unit/left-nav.test.ts b/tests/unit/left-nav.test.ts index 12d04a1..05cdeb9 100644 --- a/tests/unit/left-nav.test.ts +++ b/tests/unit/left-nav.test.ts @@ -88,14 +88,55 @@ describe('openFocusedSection — lower sections persist sidePanel', () => { expect(app.save).toHaveBeenCalledWith('sidePanel', 'history'); }); - it('does not touch libraryFilter or any field beyond the four it owns', () => { - const state = makeState({ mode: 'rail' }) as LeftNavStateSlice & { libraryFilter: string }; - state.libraryFilter = 'unchanged-marker'; + it('does not touch lowerNavigationFilters or any field beyond the four it owns', () => { + const state = makeState({ mode: 'rail' }) as LeftNavStateSlice & { + lowerNavigationFilters: Record<'library' | 'history', string>; + }; + state.lowerNavigationFilters = { library: 'unchanged-marker', history: 'also-unchanged' }; const app = makeApp(state); openFocusedSection(app, 'library'); - expect(state.libraryFilter).toBe('unchanged-marker'); + expect(state.lowerNavigationFilters).toEqual({ library: 'unchanged-marker', history: 'also-unchanged' }); + }); +}); + +describe('idempotent side effects — no repeated writes for an already-selected section', () => { + it('opening the same lower section twice only persists sidePanel once (#428 bounded drag-hover re-asserts repeatedly)', () => { + const state = makeState({ mode: 'rail', sidePanel: 'history', section: 'history' }); + const app = makeApp(state); + + openFocusedSection(app, 'library'); + expect(app.save).toHaveBeenCalledTimes(1); + expect(state.sidePanel.value).toBe('saved'); + + openFocusedSection(app, 'library'); + expect(app.save).toHaveBeenCalledTimes(1); + expect(state.sidePanel.value).toBe('saved'); + }); + + it('opening the same upper section twice does not rewrite upperRole redundantly', () => { + const state = makeState({ mode: 'rail', upperRole: 'databases', section: 'databases' }); + const app = makeApp(state); + + openFocusedSection(app, 'databases'); + openFocusedSection(app, 'databases'); + + expect(state.upperRole.value).toBe('databases'); + expect(app.save).not.toHaveBeenCalled(); + }); + + it('toggleFocusedSection also skips the redundant sidePanel persistence when re-activating the open section', () => { + const state = makeState({ mode: 'rail', sidePanel: 'saved', section: 'library' }); + const app = makeApp(state); + + // toggleFocusedSection closes the drawer on the SAME section, but the pane + // switch itself (selectSectionInExistingPane) still runs first with the + // section still 'library' — sidePanel is already 'saved', so no save. + toggleFocusedSection(app, 'library'); + + expect(app.save).not.toHaveBeenCalled(); + expect(state.leftNavSection.value).toBeNull(); }); }); diff --git a/tests/unit/workbench-session.test.ts b/tests/unit/workbench-session.test.ts index 89721aa..2223b2b 100644 --- a/tests/unit/workbench-session.test.ts +++ b/tests/unit/workbench-session.test.ts @@ -89,7 +89,6 @@ function makeState(over: Partial = {}): WorkbenchStateSlice forceExplain: false, resultRowLimit: 500, serverVersion: null, - sidePanel: signal('saved'), isMobile: signal(false), mobileView: signal('editor'), hasSelection: signal(false), @@ -846,8 +845,8 @@ describe('createWorkbenchSession: runScript()', () => { expect(h.hooks.renderHistorySection).not.toHaveBeenCalled(); }); - it('a clean run records one script history entry, and repaints History when it is the open panel', async () => { - const h = makeHarness({ state: { sidePanel: signal('history') } }); + it('a clean run records one script history entry, and repaints History unconditionally', async () => { + const h = makeHarness(); h.execFakes.executeScript.mockImplementation(async (req: ScriptExecutionRequest) => { const entry = { sql: 'SELECT 1', status: 'rows' as const, columns: [], rows: [], truncated: false, preview: '', ms: 5 }; req.onStatementResult(0, entry); @@ -865,9 +864,13 @@ describe('createWorkbenchSession: runScript()', () => { // whenever Library ('saved') was the exposed side panel, leaving History's // content stale until some unrelated repaint happened to fire (which, for // History, never does — `state.history` is a plain array, not part of any - // reactive effect). The fix makes this call unconditional. - it('a clean run repaints History even while a different side panel is open', async () => { - const h = makeHarness({ state: { sidePanel: signal('saved') } }); + // reactive effect). The fix makes this call unconditional — and #487 phase 3 + // step 4 later confirmed `WorkbenchStateSlice.sidePanel` unread by this + // session's own logic and removed the field entirely, so there is no longer + // any side-panel state to vary here; this test just re-confirms the repaint + // fires on every clean run regardless. + it('a clean run repaints History unconditionally, independent of any side-panel state', async () => { + const h = makeHarness(); h.execFakes.executeScript.mockImplementation(async (req: ScriptExecutionRequest) => { const entry = { sql: 'SELECT 1', status: 'ok' as const, ms: 5 }; req.onStatementResult(0, entry); From 7ff6428eecd1eeb56ae1335172cbb34d4d818406 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Thu, 30 Jul 2026 11:44:34 +0200 Subject: [PATCH 05/13] feat(#487): left-rail and left-nav-separator modules (phase 3, step 4a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two new standalone modules, not yet wired into the app shell: - src/ui/left-rail.ts: the compact icon rail, four launchers built from the section registry (nav-sections.ts) so a rail tooltip/aria-label can never disagree with the wide switchers' own label for the same section. A click routes through toggleFocusedSection. - src/ui/left-nav-separator.ts: the resize/mode-changing separator that will replace splitters.ts's 'col' axis in the next step. Mirrors splitters.ts's existing mouse-event drag model (not Pointer Events). Every pixel decision routes through the LeftNavigationResizeSession from left-nav-layout.ts; this module's own job is pointer/keyboard mechanics, session bookkeeping, and ARIA, never the resize arithmetic or painting the sidebar directly (that's the injected applyEffectiveLayout seam a later step implements). Terminates safely on blur and visibilitychange; mouseup processes its own final coordinate rather than the last mousemove's. Caught during review before wiring anything up: commitSession never called applyEffectiveLayout, so a keyboard-driven resize updated state and ARIA but never actually repainted the sidebar (the pointer path only worked because advanceTo already paints on every mousemove/final mouseup). Fixed by having commitSession paint with the session's own final effective layout — the one place a keyboard resize ever reaches the DOM. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NZfeGpkpJw2zsfPtysGHUN --- src/ui/left-nav-separator.ts | 291 ++++++++++++++++ src/ui/left-rail.ts | 100 ++++++ tests/unit/left-nav-separator.test.ts | 469 ++++++++++++++++++++++++++ tests/unit/left-rail.test.ts | 192 +++++++++++ 4 files changed, 1052 insertions(+) create mode 100644 src/ui/left-nav-separator.ts create mode 100644 src/ui/left-rail.ts create mode 100644 tests/unit/left-nav-separator.test.ts create mode 100644 tests/unit/left-rail.test.ts diff --git a/src/ui/left-nav-separator.ts b/src/ui/left-nav-separator.ts new file mode 100644 index 0000000..ec065f4 --- /dev/null +++ b/src/ui/left-nav-separator.ts @@ -0,0 +1,291 @@ +// #487 phase 3 — the left navigation's resize/mode-changing separator. A +// STANDALONE module today: it will replace `splitters.ts`'s `'col'` axis in a +// later step, but nothing here is wired into the app shell yet, and nothing in +// `splitters.ts` is touched by this change. +// +// Deliberately mirrors `splitters.ts`'s existing MOUSE-event drag model +// (mousedown/mousemove/mouseup on an injected `window`-shaped seam) rather than +// switching to Pointer Events — one drag primitive for the whole app, not two. +// +// Every pixel decision routes through `core/left-nav-layout.ts`'s pure +// reducers via a `LeftNavigationResizeSession` (`beginLeftNavigationResize`/ +// `advanceLeftNavigationResize`/`commitLeftNavigationResize`): this module's own +// job is strictly pointer/keyboard mechanics, session bookkeeping, and ARIA — +// never the resize arithmetic itself, and never painting the sidebar (that is +// `deps.applyEffectiveLayout`, a callback a later app-shell step implements). + +import { batch, effect } from '@preact/signals-core'; +import { + advanceLeftNavigationResize, beginLeftNavigationResize, commitLeftNavigationResize, + leftNavigationSeparatorAria, normalizeLeftNavigationLayout, resolveLeftNavigationDrag, + resolveLeftNavigationKey, +} from '../core/left-nav-layout.js'; +import type { + LeftNavigationLayout, LeftNavigationResizeSession, LeftNavigationSeparatorAria, +} from '../core/left-nav-layout.js'; +import { readLeftNavigationLayout } from '../application/left-nav.js'; +import type { LeftNavStateSlice } from '../application/left-nav.js'; +import { NAV_SECTION_META } from './nav-sections.js'; + +/** The one field `mousedown`/`mousemove`/`mouseup` read — a plain `{clientX}` + * fixture satisfies it, exactly like `splitters.ts`'s own `DragPoint`. */ +export interface LeftNavSeparatorPointerEvent { + clientX: number; +} + +/** The `window`-shaped mouse/blur seam — a real `Window` satisfies this + * directly (mirroring `splitters.ts`'s `DragWindow`, widened to also carry + * `blur`, which takes no event payload worth reading). */ +export interface LeftNavSeparatorWindow { + addEventListener(type: string, listener: (ev: LeftNavSeparatorPointerEvent) => void): void; + removeEventListener(type: string, listener: (ev: LeftNavSeparatorPointerEvent) => void): void; +} + +/** The `document`-shaped `visibilitychange` seam — deliberately NOT read for + * `document.visibilityState` (happy-dom cannot fake it): the event firing at + * all, regardless of direction, is treated as "stop and commit now". */ +export interface LeftNavSeparatorTarget { + addEventListener(type: string, listener: () => void): void; + removeEventListener(type: string, listener: () => void): void; +} + +/** The narrowed persistence seam — this module only ever names these three + * keys, the same narrowing precedent `application/left-nav.ts`'s own + * `LeftNavApp.prefs` sets for `'sidePanel'`. */ +export interface LeftNavSeparatorPrefs { + save(name: 'leftNavMode' | 'sidebarPx' | 'leftNavDrawerPx', value: unknown): void; +} + +export interface LeftNavSeparatorDeps { + /** The separator DOM element — `role`/`aria-*`/`tabindex` are applied to + * THIS element, and mousedown/keydown listen on it directly. */ + el: HTMLElement; + /** Mouse-move/up/blur seam. Defaults to the real `window`. */ + win?: LeftNavSeparatorWindow; + /** `visibilitychange` seam. Defaults to the real `document` — deliberately + * a SEPARATE seam from `win`, since visibility fires on the document. */ + target?: LeftNavSeparatorTarget; + /** The same `LeftNavStateSlice` `application/left-nav.ts` reads/writes — + * reused rather than re-declared, so this module calls the real + * `readLeftNavigationLayout(state)` rather than re-implementing the same + * projection. */ + state: LeftNavStateSlice; + prefs: LeftNavSeparatorPrefs; + /** The live navigation width budget (shell width minus the centre surface's + * minimum minus the separator's own width) — a caller's job, not this + * module's; it only ever asks for a number when it needs one. */ + getMaxNavigationTotalPx(): number; + /** The ONE DOM-painting seam: apply a proposed layout's pixels to the + * sidebar. This module never touches sidebar DOM itself. */ + applyEffectiveLayout(layout: LeftNavigationLayout): void; + /** Optional status-announcement seam — a no-op when omitted. Called only on + * a semantic mode (or drawer open/closed) change, never on a plain width + * change within the same mode. */ + announce?(message: string): void; +} + +export interface LeftNavSeparatorHandle { + /** Remove every listener this module registered (mousedown/keydown on `el`; + * blur on `win`; visibilitychange on `target`; and, if a drag happens to be + * in progress, the drag-only mousemove/mouseup on `win` too) and stop the + * ARIA-refresh effect. Safe to call once; idempotent-safe to call again + * (every underlying `removeEventListener` is a no-op for an + * already-removed listener). */ + dispose(): void; +} + +/** Describe the OCCUPIED-width quantity `aria-valuenow` reports, in words — + * mode-aware, but always naming the SAME total `aria-valuenow` carries (never + * a per-mode panel width; see this module's own header comment for the + * earlier design round that got this wrong). */ +function describeOccupiedWidth(layout: LeftNavigationLayout): string { + if (layout.mode === 'wide') return 'Wide sidebar'; + if (layout.focusedSection === null) return 'Rail only'; + return `Rail with ${NAV_SECTION_META[layout.focusedSection].label} drawer`; +} + +function ariaValueText(layout: LeftNavigationLayout, aria: LeftNavigationSeparatorAria): string { + if (layout.mode === 'rail' && layout.focusedSection !== null) { + return `${describeOccupiedWidth(layout)}, ${aria.valueNow} pixels total`; + } + return `${describeOccupiedWidth(layout)}, ${aria.valueNow} pixels`; +} + +/** + * Mount the separator: apply its static ARIA/DOM attributes once, wire mouse, + * keyboard, blur and visibilitychange handling, and return a `dispose()`. + */ +export function mountLeftNavSeparator(deps: LeftNavSeparatorDeps): LeftNavSeparatorHandle { + const { el, state } = deps; + const win: LeftNavSeparatorWindow = deps.win || window; + const target: LeftNavSeparatorTarget = deps.target || document; + + el.setAttribute('role', 'separator'); + el.setAttribute('aria-orientation', 'vertical'); + el.setAttribute('tabindex', '0'); + + // The in-progress drag/keyboard-resize session, or null between gestures. + let session: LeftNavigationResizeSession | null = null; + + function applyAria(layout: LeftNavigationLayout): void { + const normalized = normalizeLeftNavigationLayout(layout); + const aria = leftNavigationSeparatorAria(normalized, deps.getMaxNavigationTotalPx()); + el.setAttribute('aria-valuemin', String(aria.valueMin)); + el.setAttribute('aria-valuemax', String(aria.valueMax)); + el.setAttribute('aria-valuenow', String(aria.valueNow)); + el.setAttribute('aria-valuetext', ariaValueText(normalized, aria)); + } + + // `writeLeftNavigationLayout` (`application/left-nav.ts`) is module-private + // there (only ever the other half of THAT module's own single batched + // write), so this mirrors its four-field write rather than importing it. + // All four fields, including `leftNavSection`: a committed session's `mode` + // and `focusedSection` always travel together (the layout's own coherence + // invariant — see `core/left-nav-layout.ts`), so writing three of the four + // could leave `state` holding an incoherent pair (e.g. `mode: 'wide'` with a + // stale non-null `leftNavSection` from before the gesture converted rail to + // wide). Only the PERSISTENCE call below is narrowed to three keys — + // `leftNavSection` has no preference key at all (`focusedSection` is + // session-only, per #487), so nothing here ever calls `prefs.save` for it. + // Batched so the reactive ARIA effect below observes one coherent write, not + // an intermediate mode/width mismatch mid-assignment. + function writeLayout(layout: LeftNavigationLayout): void { + batch(() => { + state.leftNavMode.value = layout.mode; + state.sidebarPx = layout.wideWidthPx; + state.leftNavDrawerPx = layout.drawerWidthPx; + state.leftNavSection.value = layout.focusedSection; + }); + } + + function announceIfChanged(before: LeftNavigationLayout, after: LeftNavigationLayout): void { + const modeChanged = before.mode !== after.mode; + const openChanged = (before.focusedSection !== null) !== (after.focusedSection !== null); + if (!modeChanged && !openChanged) return; // a plain width change — no chatter. + deps.announce?.(`Left navigation: ${describeOccupiedWidth(after)}`); + } + + function commitSession(finished: LeftNavigationResizeSession): void { + const committed = commitLeftNavigationResize(finished); + // Paint with the session's own final, viewport-clamped `effective` layout — + // never `committed`, which can legitimately hold a larger, un-clamped + // "honest preference" (the restore-while-clamped case below) that would + // overflow the viewport if painted directly. For a pointer drag this is a + // harmless repaint of the identical layout `advanceTo` already applied + // (`onMouseUp` calls `advanceTo` immediately before `endDrag`/ + // `commitSession`); for the keyboard path (`onKeyDown`) this is the ONLY + // place a keyboard-driven resize ever reaches the DOM at all. + deps.applyEffectiveLayout(finished.effective); + writeLayout(committed); + deps.prefs.save('leftNavMode', committed.mode); + deps.prefs.save('sidebarPx', committed.wideWidthPx); + deps.prefs.save('leftNavDrawerPx', committed.drawerWidthPx); + applyAria(committed); + announceIfChanged(finished.preferredAtStart, committed); + } + + // One reactive effect keeps ARIA current whenever `mode`/`focusedSection` + // change for ANY reason — including a rail click elsewhere in the shell + // (`left-rail.ts`'s `toggleFocusedSection`) that this module never + // initiated. Runs once immediately (the mount-time paint), then on every + // dependency change. `sidebarPx`/`leftNavDrawerPx` are plain fields, not + // signals, so a WIDTH-only change from THIS module's own gestures relies on + // `commitSession`'s own `applyAria(committed)` call above, not this effect. + const disposeAriaEffect = effect(() => { + state.leftNavMode.value; + state.leftNavSection.value; + applyAria(readLeftNavigationLayout(state)); + }); + + /** + * Advance the in-progress session to `clientX`. The shell-left offset is 0 + * today (phase 1's note in `core/left-nav-layout.ts`) — a future left + * gutter would subtract it from `clientX` here, once, in this one place. + */ + function advanceTo(clientX: number): void { + // `!`: `onMouseMove`/`onMouseUp` are only ever listening on `win` while a + // session is active — attached in `onMouseDown`, detached in `endDrag` — + // so `advanceTo` is never reached with `session` null. + const proposedLayout = resolveLeftNavigationDrag(session!.proposed, clientX); + session = advanceLeftNavigationResize(session!, proposedLayout, deps.getMaxNavigationTotalPx()); + deps.applyEffectiveLayout(session.effective); + applyAria(session.effective); + } + + function onMouseMove(ev: LeftNavSeparatorPointerEvent): void { + advanceTo(ev.clientX); + } + + /** Stop listening for the mouse half of a drag and commit whatever the + * session currently holds — used by mouseup (after one final `advanceTo`) + * AND by blur/visibilitychange (with no final coordinate — commit the + * session's CURRENT state as-is, never a rollback to session start). */ + function endDrag(): void { + if (!session) return; + el.classList.remove('dragging'); + win.removeEventListener('mousemove', onMouseMove); + win.removeEventListener('mouseup', onMouseUp); + const finished = session; + session = null; + commitSession(finished); + } + + function onMouseUp(ev: LeftNavSeparatorPointerEvent): void { + // The LAST coordinate before committing — never whatever the last + // mousemove happened to leave (that can differ from the release point). + advanceTo(ev.clientX); + endDrag(); + } + + // `el` is always a real DOM element (never an injected fake, unlike + // `win`/`target`), so its own listeners are typed against the real DOM + // event types directly — no cast needed, and a plain fixture is never + // dispatched through it in tests either. + function onMouseDown(ev: MouseEvent): void { + ev.preventDefault(); + el.classList.add('dragging'); + session = beginLeftNavigationResize(readLeftNavigationLayout(state), deps.getMaxNavigationTotalPx()); + win.addEventListener('mousemove', onMouseMove); + win.addEventListener('mouseup', onMouseUp); + } + + // No coordinate to read on either event — happy-dom cannot fake + // `document.visibilityState` regardless, so both simply stop-and-commit + // whatever the session already holds. A no-op when no drag is active. + function onBlur(): void { endDrag(); } + function onVisibilityChange(): void { endDrag(); } + + function onKeyDown(ev: KeyboardEvent): void { + const layout = readLeftNavigationLayout(state); + const resolved = resolveLeftNavigationKey(layout, ev); + if (resolved === null) return; // not one of ours — no preventDefault, no session. + ev.preventDefault(); + // A single keydown is its own complete session: begin → one advance → + // commit, immediately, through the exact same machinery a drag uses. + const keySession = advanceLeftNavigationResize( + beginLeftNavigationResize(layout, deps.getMaxNavigationTotalPx()), + resolved, + deps.getMaxNavigationTotalPx(), + ); + commitSession(keySession); + } + + el.addEventListener('mousedown', onMouseDown); + el.addEventListener('keydown', onKeyDown); + win.addEventListener('blur', onBlur); + target.addEventListener('visibilitychange', onVisibilityChange); + + function dispose(): void { + disposeAriaEffect(); + el.removeEventListener('mousedown', onMouseDown); + el.removeEventListener('keydown', onKeyDown); + win.removeEventListener('blur', onBlur); + target.removeEventListener('visibilitychange', onVisibilityChange); + // Defensive: harmless no-op if a drag isn't in progress, but guarantees no + // lingering mousemove/mouseup handler if dispose() runs mid-drag. + win.removeEventListener('mousemove', onMouseMove); + win.removeEventListener('mouseup', onMouseUp); + } + + return { dispose }; +} diff --git a/src/ui/left-rail.ts b/src/ui/left-rail.ts new file mode 100644 index 0000000..0308753 --- /dev/null +++ b/src/ui/left-rail.ts @@ -0,0 +1,100 @@ +// #487 phase 3 — the compact icon rail. Four launcher buttons, one per +// `LEFT_NAV_SECTIONS` entry (rail order), built from the section registry +// (`nav-sections.ts`) so a rail tooltip/aria-label can never disagree with the +// wide switchers' own label for the same section. +// +// This module owns DOM + behaviour only: the rail's WIDTH (`LEFT_RAIL_PX`) is +// informational here and is CSS's job in a later step, and the drawer this +// rail's buttons control does not exist as a separate element yet — `showSection` +// keeps living on the section registry, this module just points every button's +// `aria-controls` at whatever single id a later step gives the drawer container. +// +// Reactivity: each button's `aria-expanded` is driven by its own `effect()` +// reading `state.leftNavSection` — a section's drawer is "open" exactly when +// `state.leftNavSection.value === section` (`application/left-nav.ts`'s own +// notion of the focused section). One effect per button, not one for the whole +// rail, so a section switch touches only the two buttons whose expanded state +// actually changed. +// +// A rail click is a TOGGLE (`toggleFocusedSection`), not an idempotent open +// (`openFocusedSection`): clicking the already-open section's icon closes the +// drawer, which is #487's own rail-click semantics and is NOT what the +// drag-hover seam (#428) wants — that seam calls `openFocusedSection` directly, +// bypassing this module entirely. + +import { effect } from '@preact/signals-core'; +import type { Signal } from '@preact/signals-core'; +import { h } from './dom.js'; +import { LEFT_NAV_SECTIONS } from '../core/left-nav-layout.js'; +import type { LeftNavigationSection } from '../core/left-nav-layout.js'; +import { toggleFocusedSection } from '../application/left-nav.js'; +import type { LeftNavApp } from '../application/left-nav.js'; +import type { NavSectionRegistry } from './nav-sections.js'; + +/** The state slice the rail reads — just enough to know which section (if any) + * the focused drawer currently shows. */ +export interface LeftRailStateSlice { + readonly leftNavSection: Signal; +} + +export interface LeftRailDeps { + /** Reused verbatim from `application/left-nav.ts` — a click routes through + * its own `toggleFocusedSection`, never a locally re-implemented write. */ + app: LeftNavApp; + /** Only metadata lookup is needed — the rail neither renders a section's own + * content nor decides which pane exposes it. */ + registry: Pick; + state: LeftRailStateSlice; + /** The stable DOM id of the (single, content-swapping) focused-drawer + * container a later step gives the sidebar element — every launcher's + * `aria-controls` points at this same id, since all four buttons control + * the one drawer, just with different content. */ + drawerElementId: string; +} + +export interface LeftRailHandle { + readonly el: HTMLElement; + /** Stop every per-button reactive effect. Idempotent-safe to call once. */ + dispose(): void; +} + +/** + * Build the rail `