fix(#478): make the variable bar's recents port a live read, not a snapshot

CHANGELOG.md

@@ -25,6 +25,19 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
   adapter refactor — no user-visible behavior changes.
 
 ### Fixed
+- **A detached Data pane's variable dropdown no longer shows a frozen
+  snapshot of recent values** (#478 follow-up). #555's caller-neutral
+  `VariableBarApp` adapter copied `varRecent` into `state` as a plain data
+  property, captured once at adapter-construction time — but unlike
+  `activeByName` (aliased, mutated in place), `varRecent` is *replaced*
+  wholesale on every record/clear (`workbench-parameter-session.ts`), so a
+  copy went stale immediately: a detached pane's "Clear recent" persisted but
+  the still-open dropdown kept listing the cleared values, and a re-run's
+  newly recorded value never appeared until the pane was closed and reopened.
+  The Dashboard adapter (`dashboard.ts`) had the identical staleness
+  pre-dating #555. `VariableBarApp` now exposes `getVarRecent(): RecentMap`,
+  a live-read callback both adapters implement by reading their own
+  `varRecent` reference at call time, instead of a `state.varRecent` snapshot.
 - **A Dashboard tile whose id is `__proto__` or `constructor` no longer loses
   its placement** (#551, found reviewing #549). Both are legal tile ids under
   `dashboardTileV1.id` (pattern `\S`), and a placement-map write of the shape

src/ui/dashboard.ts

@@ -823,7 +823,13 @@ export async function renderDashboard(
     // map, never persisted, so `saveActive` is a no-op adapter (there is
     // nothing to save; unlike detached Data, this Dashboard draft has no
     // Workbench-persisted counterpart to route to).
-    state: { varValues: draftValues, activeByName: draftActive, varRecent: state.varRecent },
+    state: { varValues: draftValues, activeByName: draftActive },
+    // #478: a live read at call time, not a copied `state.varRecent` data
+    // property — `state.varRecent` (the real `AppState` field) is REPLACED
+    // wholesale by `clearVarRecent`/`recordBoundParams`, never mutated in
+    // place, so a snapshot taken here at adapter-construction time would go
+    // stale the moment either fires while this bar is still mounted.
+    getVarRecent: () => state.varRecent,
     params: {
       saveVarValues: () => {},
       saveActive: () => {},

src/ui/results.ts

@@ -1120,7 +1120,12 @@ export function expandDataPane(app: ResultsApp, r: QueryResult): DetachedView {
         // `saveActive` routes to the real persisted Workbench save.
         const variableBarApp: VariableBarApp = {
           document: doc,
-          state: { varValues: app.state.varValues, activeByName: app.state.filterActive, varRecent: app.state.varRecent },
+          state: { varValues: app.state.varValues, activeByName: app.state.filterActive },
+          // #478: a live read at call time — `app.state.varRecent` is REPLACED
+          // wholesale by `recordBoundParams`/`clearVarRecent` (never mutated in
+          // place), so copying it into `state` above like `activeByName` would
+          // freeze a stale snapshot from adapter-construction time.
+          getVarRecent: () => app.state.varRecent,
           params: {
             saveVarValues: () => app.params.saveVarValues(),
             saveActive: () => app.params.saveFilterActive(),

src/ui/variable-bar.ts

@@ -47,10 +47,16 @@ import type { VariableOption } from '../core/variable-options.types.js';
 import type { DashboardTimeRangeGroup, TimeRangeRecent } from '../core/time-range.js';
 
 /** The narrow slice of the real `app` controller this module reads — not the
- *  full ~50-member `App` contract (app.types.ts). A real `App` satisfies this
- *  directly, and so does tests/helpers/fake-app.js's long-standing minimal
- *  `makeApp()` fixture — no cast needed on either side (same convention
- *  shortcuts.ts established for its own narrow `ShortcutsApp` contract). */
+ *  full ~50-member `App` contract (app.types.ts). #478: this is an
+ *  ADAPTER-facing port, not a same-shape subset a real `App` satisfies
+ *  directly — both callers (`results.ts`'s detached Data pane and
+ *  `dashboard.ts`'s viewer) build an explicit `VariableBarApp` object rather
+ *  than casting `app as VariableBarApp`, because each names its own
+ *  activation/persistence semantics (see `activeByName` and `params.saveActive`
+ *  below). `tests/unit/variable-bar.test.ts`'s `asBarApp()` builds the same
+ *  kind of adapter, over tests/helpers/fake-app.js's fake `App`, to exercise
+ *  this module directly; `results.test.ts`/`dashboard.test.ts` instead drive
+ *  the real production adapters through `expandDataPane`/`renderDashboard`. */
 export interface VariableBarApp {
   document: Document;
   state: {
@@ -63,8 +69,19 @@ export interface VariableBarApp {
      *  activation-follows-value), so a copy would silently stop the caller's
      *  own reads (`effectiveFilterActive`, `activeMap`) from observing edits. */
     activeByName: Record<string, boolean>;
-    varRecent: RecentMap;
   };
+  /** #478 (fixing a #555 regression): a live-read CALLBACK, deliberately not a
+   *  `state.varRecent` data property. `varRecent` is REPLACED wholesale, never
+   *  mutated in place (`workbench-parameter-session.ts`'s `recordBoundParams`/
+   *  `clearVarRecent`/`clearAllVarRecent` all assign a new `RecentMap`) — an
+   *  adapter that copied it into `state` like `activeByName` would freeze a
+   *  snapshot from construction time, so a later replacement (a new recorded
+   *  value, a cleared field) would never reach an already-built bar. Reading
+   *  through a callback at call time (`getRecents` below, on every dropdown
+   *  open/keystroke) is what makes the source live regardless of how many
+   *  times the caller's own `varRecent` reference has been swapped since this
+   *  bar was built. */
+  getVarRecent(): RecentMap;
   /** #276 Phase 5: no flat `App.saveVarValues`/`saveFilterActive`/
    *  `clearVarRecent` delegates — this module reads `app.params.*` directly.
    *  #478: `saveActive` replaces the Workbench-named `saveFilterActive` — a
@@ -309,7 +326,8 @@ export interface VariableBarHandle {
 /**
  * Build a variable bar: one field per `{name:Type}` parameter in `params` (the
  * shape from `fieldControls(analysis)`), sharing `app.state.varValues` /
- * `app.state.activeByName` / `app.state.varRecent` with every other surface.
+ * `app.state.activeByName` with every other surface, and reading recents
+ * live through `app.getVarRecent()`.
  * Hidden entirely (no row, no spacing) when `params` is empty — same convention
  * as the workbench's var-strip. Typing debounces before calling `onCommit(name)`;
  * Enter or blur fires immediately, clearing any pending debounce so a value
@@ -598,7 +616,7 @@ export function buildVariableBar(
     // #171: live-filtered recents for this field (type + typed text), read
     // fresh on every open/keystroke (never a snapshot — see recent-field.js's
     // header comment).
-    const getRecents = (text: string): string[] => recentOptions(app.state.varRecent, p.name, p.type, text);
+    const getRecents = (text: string): string[] => recentOptions(app.getVarRecent(), p.name, p.type, text);
     const onClearRecent = (): void => app.params.clearVarRecent(p.name);
     // A preset/recent pick is a deliberate, complete action (like Enter) —
     // run immediately, bypassing the debounce `onValueInput` just armed,

tests/e2e/time-range.html

@@ -176,7 +176,11 @@
     const draftActive = {};
     const variableBarApp = {
       document,
-      state: { varValues: draftValues, activeByName: draftActive, varRecent: {} },
+      state: { varValues: draftValues, activeByName: draftActive },
+      // #478: a live-read callback, not a copied `varRecent` property — this
+      // fixture never records recents, so an empty map is fine either way, but
+      // the SHAPE must match the port (`getVarRecent(): RecentMap`).
+      getVarRecent: () => ({ version: 1, nextSeq: 1, byName: {} }),
       params: { saveVarValues() {}, saveActive() {}, clearVarRecent() {} },
       wallNow: () => WALL,
     };

tests/unit/dashboard.test.ts

@@ -12,7 +12,7 @@ import { applyCommand } from '../../src/dashboard/application/dashboard-commands
 import { createQueryResolver } from '../../src/dashboard/application/dashboard-query-resolver.js';
 import { resolveLayoutPluginSync } from '../../src/dashboard/layouts/layout-registry.js';
 import { applyStreamLine } from '../../src/core/stream.js';
-import { emptyRecentMap, recordRecent } from '../../src/core/recent-values.js';
+import { emptyRecentMap, recordRecent, clearRecent } from '../../src/core/recent-values.js';
 import { makeApp, FakeChart } from '../helpers/fake-app.js';
 import { fakeIndexedDbFactory } from '../helpers/fake-idb.js';
 import { createApp } from '../../src/ui/app.js';
@@ -1215,6 +1215,58 @@ describe('renderDashboard — reorder (Command/Ctrl pointer-drag) + sort (#153/#
     qs(sField, '.var-combo-footer button')?.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, cancelable: true }));
     expect(app.params.clearVarRecent).toHaveBeenCalledWith('s');
   });
+
+  // #478 regression (pre-dates #555): this viewer's adapter used to copy
+  // `state.varRecent` into `VariableBarApp.state` as a plain data property,
+  // captured once when the Dashboard rendered. `varRecent` is REPLACED
+  // wholesale, never mutated in place (`workbench-parameter-session.ts`'s
+  // `recordBoundParams`), so that copy went stale the instant a later run
+  // recorded a new value while the dashboard stayed open. The adapter now
+  // routes through `getVarRecent()`, read at call time — this must fail if
+  // the adapter reverts to a captured `varRecent` property.
+  it('#478: a value recorded after the dashboard renders is visible on the next dropdown open', async () => {
+    const { app } = dashApp({
+      workspace: wsWith({
+        queries: [q('q1', 'SELECT k, v FROM a WHERE s = {s:String}')],
+        tiles: [{ id: 't1', queryId: 'q1' }],
+      }),
+    });
+    await render(app);
+    const fieldFor = (name: string) => qsa(app.root, '.dash-variable-host .var-field')
+      .find((f) => qs(f, '.var-name')?.textContent === name)!;
+    const input = qs<HTMLInputElement>(fieldFor('s'), 'input');
+    input.dispatchEvent(new Event('focus'));
+    expect(qs(fieldFor('s'), '[role="option"]')).toBeNull(); // nothing recorded yet
+    // Simulate a successful run recording a new value — a fresh map object.
+    app.state.varRecent = recordRecent(emptyRecentMap(), 's', 'newval');
+    input.dispatchEvent(new Event('focus')); // reopen: re-derives options fresh
+    const opt = qs(fieldFor('s'), '[role="option"]');
+    expect(opt.textContent).toContain('newval');
+  });
+
+  it('#478: Clear recent, through a production-like clearVarRecent that replaces app.state.varRecent, empties the dropdown on reopen', async () => {
+    const { app } = dashApp({
+      workspace: wsWith({
+        queries: [q('q1', 'SELECT k, v FROM a WHERE s = {s:String}')],
+        tiles: [{ id: 't1', queryId: 'q1' }],
+      }),
+    });
+    app.state.varRecent = recordRecent(emptyRecentMap(), 's', 'foo');
+    app.params.clearVarRecent = vi.fn((name: string) => {
+      app.state.varRecent = clearRecent(app.state.varRecent, name);
+    });
+    await render(app);
+    const fieldFor = (name: string) => qsa(app.root, '.dash-variable-host .var-field')
+      .find((f) => qs(f, '.var-name')?.textContent === name)!;
+    const sField = fieldFor('s');
+    const input = qs<HTMLInputElement>(sField, 'input');
+    input.dispatchEvent(new Event('focus'));
+    expect(qs(sField, '[role="option"]')).not.toBeNull(); // 'foo' is listed
+    qs(sField, '.var-combo-footer button')?.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, cancelable: true }));
+    expect(app.params.clearVarRecent).toHaveBeenCalledWith('s');
+    input.dispatchEvent(new Event('focus')); // reopen after the clear
+    expect(qs(sField, '[role="option"]')).toBeNull(); // gone, not stale
+  });
 });
 
 describe('renderDashboard — modkey cursor cue (#332)', () => {

tests/unit/results.test.ts

@@ -10,7 +10,7 @@ import type {
 import { makeApp } from '../helpers/fake-app.js';
 import type { FakeChart } from '../helpers/fake-app.js';
 import { newResult as newResultUntyped } from '../../src/core/stream.js';
-import { emptyRecentMap, recordRecent } from '../../src/core/recent-values.js';
+import { emptyRecentMap, recordRecent, clearRecent } from '../../src/core/recent-values.js';
 import { formatRows } from '../../src/core/format.js';
 import { queryPanel } from '../../src/core/saved-query.js';
 import type { AppState, ResultSort } from '../../src/state.js';
@@ -961,6 +961,46 @@ describe('expandDataPane', () => {
     expect(app.params.clearVarRecent).toHaveBeenCalledWith('level');
   });
 
+  // #478 regression: this detached-Data adapter used to copy `app.state.varRecent`
+  // into `VariableBarApp.state` as a plain data property, captured once when the
+  // pane was expanded. `varRecent` is REPLACED wholesale, never mutated in place
+  // (`workbench-parameter-session.ts`'s `recordBoundParams`), so that copy went
+  // stale the instant a later run recorded a new value while the pane stayed
+  // open. The adapter now routes through `getVarRecent()`, read at call time —
+  // this must fail if the adapter reverts to a captured `varRecent` property.
+  it('#478: a value recorded after the pane opens is visible on the next dropdown open', () => {
+    const app = makeApp();
+    expandDataPane(app, paramResult());
+    const overlay = qs(document, '.graph-overlay');
+    const input = qs<HTMLInputElement>(overlay, '.detached-variable-row .var-field input');
+    input.dispatchEvent(new Event('focus'));
+    expect(qs(overlay, '[role="option"]')).toBeNull(); // nothing recorded yet
+    // Simulate a successful re-run recording a new value — a fresh map object.
+    app.state.varRecent = recordRecent(emptyRecentMap(), 'level', 'Critical');
+    input.dispatchEvent(new Event('focus')); // reopen: re-derives options fresh
+    const opt = qs(overlay, '[role="option"]');
+    expect(opt.textContent).toContain('Critical');
+  });
+
+  it('#478: Clear recent, through a production-like clearVarRecent that replaces app.state.varRecent, empties the dropdown on reopen', () => {
+    const app = makeApp();
+    app.state.varRecent = recordRecent(emptyRecentMap(), 'level', 'Warning');
+    app.params.clearVarRecent = vi.fn((name: string) => {
+      app.state.varRecent = clearRecent(app.state.varRecent, name);
+    });
+    expandDataPane(app, paramResult());
+    const overlay = qs(document, '.graph-overlay');
+    const input = qs<HTMLInputElement>(overlay, '.detached-variable-row .var-field input');
+    input.dispatchEvent(new Event('focus'));
+    expect(qs(overlay, '[role="option"]')).not.toBeNull(); // 'Warning' is listed
+    qs<HTMLButtonElement>(overlay, '.var-combo-footer button').dispatchEvent(
+      new MouseEvent('mousedown', { bubbles: true, cancelable: true }),
+    );
+    expect(app.params.clearVarRecent).toHaveBeenCalledWith('level');
+    input.dispatchEvent(new Event('focus')); // reopen after the clear
+    expect(qs(overlay, '[role="option"]')).toBeNull(); // gone, not stale
+  });
+
   it('a retained Refresh button is inert after its detached view closes', async () => {
     const app = makeApp();
     app.state.varValues.level = 'Warning';

tests/unit/variable-bar.test.ts

@@ -3,7 +3,7 @@ import { analyzeParameterizedSources, fieldControls } from '../../src/core/param
 import type { FieldControl, PreparedFieldState } from '../../src/core/param-pipeline.js';
 import { buildVariableBar, VARIABLE_DEBOUNCE_MS } from '../../src/ui/variable-bar.js';
 import type { VariableBarApp } from '../../src/ui/variable-bar.js';
-import { emptyRecentMap, recordRecent } from '../../src/core/recent-values.js';
+import { emptyRecentMap, recordRecent, clearRecent } from '../../src/core/recent-values.js';
 import { parseParamType } from '../../src/core/param-type.js';
 import type { DashboardTimeRangeGroup, TimeRangeRecent } from '../../src/core/time-range.js';
 import { makeApp } from '../helpers/fake-app.js';
@@ -17,7 +17,11 @@ import { makeApp } from '../helpers/fake-app.js';
 // through this adapter still observes the bar's own writes.
 const asBarApp = (app: ReturnType<typeof makeApp>): VariableBarApp => ({
   document: app.document,
-  state: { varValues: app.state.varValues, activeByName: app.state.filterActive, varRecent: app.state.varRecent },
+  state: { varValues: app.state.varValues, activeByName: app.state.filterActive },
+  // #478: a live read, not a captured `app.state.varRecent` snapshot — see
+  // `VariableBarApp.getVarRecent`'s doc comment (`varRecent` is replaced
+  // wholesale, never mutated in place).
+  getVarRecent: () => app.state.varRecent,
   params: {
     saveVarValues: () => app.params.saveVarValues(),
     saveActive: () => app.params.saveFilterActive(),
@@ -161,6 +165,55 @@ describe('buildVariableBar (shared variable row)', () => {
     bar.el.remove();
   });
 
+  it('reads varRecent live: a wholesale replacement of app.state.varRecent after the bar is built surfaces the new value on reopen (#478)', () => {
+    // #478 regression: the detached-Data adapter used to copy `app.state.varRecent`
+    // into `VariableBarApp.state` as a plain data property, captured once at
+    // adapter-construction time. `varRecent` is REPLACED wholesale (never
+    // mutated in place — `workbench-parameter-session.ts`'s `recordBoundParams`),
+    // so that copy went stale the instant a later run recorded a new value.
+    // `asBarApp` now routes through `getVarRecent()`, read at call time — this
+    // must fail if that adapter reverts to `varRecent: app.state.varRecent`.
+    const app = makeApp();
+    const onCommit = vi.fn();
+    const bar = buildVariableBar(asBarApp(app), paramsFor('SELECT {x:String}'), onCommit, okField, { document });
+    document.body.appendChild(bar.el);
+    const input = bar.el.querySelector('input')!;
+    input.dispatchEvent(new Event('focus'));
+    expect(bar.el.querySelector('[role="option"]')).toBeNull(); // nothing recorded yet
+    // A successful run records a new value — a fresh map object, not a mutation.
+    app.state.varRecent = recordRecent(emptyRecentMap(), 'x', 'newval');
+    input.dispatchEvent(new Event('focus')); // reopen: onFocus() always re-derives options
+    const opt = bar.el.querySelector('[role="option"]');
+    expect(opt).not.toBeNull();
+    expect(opt!.textContent).toContain('newval');
+    bar.el.remove();
+  });
+
+  it('Clear recent, through a production-like clearVarRecent that replaces app.state.varRecent, empties the dropdown on reopen (#478)', () => {
+    // #478 regression: the fake `clearVarRecent` is normally a no-op `vi.fn()`
+    // spy (asserted only for the call, never for its effect), which would
+    // still pass even if the adapter fed the bar a frozen snapshot. Here
+    // `clearVarRecent` actually replaces `app.state.varRecent`, mirroring
+    // `workbench-parameter-session.ts`'s real `clearVarRecent` — this exercises
+    // the same wholesale-replacement path a real Clear-recent click takes.
+    const app = makeApp();
+    app.state.varRecent = recordRecent(emptyRecentMap(), 'x', 'foo');
+    app.params.clearVarRecent = vi.fn((name: string) => {
+      app.state.varRecent = clearRecent(app.state.varRecent, name);
+    });
+    const onCommit = vi.fn();
+    const bar = buildVariableBar(asBarApp(app), paramsFor('SELECT {x:String}'), onCommit, okField, { document });
+    document.body.appendChild(bar.el);
+    const input = bar.el.querySelector('input')!;
+    input.dispatchEvent(new Event('focus'));
+    expect(bar.el.querySelector('[role="option"]')).not.toBeNull(); // 'foo' is listed
+    bar.el.querySelector('.var-combo-footer button')!.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, cancelable: true }));
+    expect(app.params.clearVarRecent).toHaveBeenCalledWith('x');
+    input.dispatchEvent(new Event('focus')); // reopen after the clear
+    expect(bar.el.querySelector('[role="option"]')).toBeNull(); // gone, not stale
+    bar.el.remove();
+  });
+
   it('a preset pick with no pending debounce still commits (the un-armed onPick path)', () => {
     const app = makeApp();
     app.state.varRecent = recordRecent(emptyRecentMap(), 'x', 'foo');