Merge pull request #574 from Altinity/feat/left-rail-focused-drawer-487p3

feat(#487): rail, focused drawer and resize separator (phase 3)

Author: BorisTyshkevich

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

docs/ADR-0001-reactivity.md

@@ -0,0 +1,175 @@
+// #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<LeftNavigationMode>;
+  readonly leftNavSection: Signal<LeftNavigationSection | null>;
+  readonly upperRole: Signal<'databases' | 'dashboards'>;
+  readonly sidePanel: Signal<SidePanelKey>;
+}
+
+/** 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 };
+  /**
+   * #487 phase-3 review, major issue 2 — called BEFORE either function below
+   * runs its own batched write. An active pointer resize session
+   * (`ui/left-nav-separator.ts`) keeps its own uncommitted layout snapshot
+   * independent of `state`; without this, a semantic command that runs while
+   * a drag is still live (Escape closing a drawer, a rail click, a
+   * programmatic reveal) writes `state` correctly, but the drag's eventual
+   * `mouseup`/`blur` commit still fires from its OWN stale snapshot and can
+   * silently overwrite (or resurrect) exactly what this command just did.
+   * `app-shell.ts` wires this to cancel the active session (no commit) and
+   * repaint from the now-current committed layout, so there is nothing left
+   * to fight this write once it runs. This is the ONE choke point every
+   * caller of `openFocusedSection`/`toggleFocusedSection` gets it through —
+   * optional, so a caller with no separator session to preempt (a test, or a
+   * call before the shell has mounted one) simply omits it and gets a no-op.
+   */
+  preemptActiveResize?(): 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. 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.
+    if (app.state.upperRole.value !== section) app.state.upperRole.value = section;
+    return;
+  }
+  const panel = sidePanelKeyFor(section);
+  if (app.state.sidePanel.value !== panel) {
+    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 {
+  app.preemptActiveResize?.();
+  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 {
+  app.preemptActiveResize?.();
+  batch(() => {
+    selectSectionInExistingPane(app, section);
+    writeLeftNavigationLayout(
+      app.state, resolveRailActivation(readLeftNavigationLayout(app.state), section),
+    );
+  });
+}

src/application/left-nav.ts

@@ -146,8 +146,10 @@ export interface SavedQueryService {
   commit(tab: QueryTab, evaluated: { parsed: unknown; diagnostics: SpecValidationDiagnostic[] }): Promise<CommitLinkedResult>;
   /** 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. */

src/application/saved-query-service.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