refactor(#478): caller-neutral VariableBar activation port

CHANGELOG.md

@@ -9,6 +9,21 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
 
 ## [Unreleased]
 
+### Changed
+- **`VariableBarApp`'s shared activation port is now caller-neutral** (#478).
+  `state.filterActive`/`params.saveFilterActive` — named after Workbench
+  persistence even though Dashboard's own caller uses them for an unpersisted
+  in-memory draft — are renamed to `state.activeByName`/`params.saveActive`
+  on the shared `variable-bar.ts` contract only. Every persisted name and
+  storage key is unchanged (`AppState.filterActive`,
+  `WorkbenchParameterSession.saveFilterActive`, `effectiveFilterActive`,
+  `asb:filterActive`; no storage migration). Both callers now construct an
+  explicit adapter instead of a same-shape cast: Dashboard aliases its local
+  draft activation map with a no-op `saveActive` (the draft is never
+  persisted); detached Data aliases the real `AppState.filterActive` object
+  and routes `saveActive` to `app.params.saveFilterActive()`. Pure rename +
+  adapter refactor — no user-visible behavior changes.
+
 ### Fixed
 - **Dashboard styles now persist independent dimensions and temporary column
   previews no longer mutate authored layouts** (behavioral correction to

src/ui/dashboard.ts

@@ -819,10 +819,14 @@ export async function renderDashboard(
   const draftActive: Record<string, boolean> = {};
   const variableBarApp: VariableBarApp = {
     document: doc,
-    state: { varValues: draftValues, filterActive: draftActive, varRecent: state.varRecent },
+    // #478: aliases the local draft maps above — an in-memory-only activation
+    // 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 },
     params: {
       saveVarValues: () => {},
-      saveFilterActive: () => {},
+      saveActive: () => {},
       clearVarRecent: (name: string) => app.params.clearVarRecent(name),
     },
     wallNow: () => app.wallNow(),

src/ui/results.ts

@@ -174,8 +174,10 @@ export interface ResultsApp {
    *  only ever needs `executeRead` (the detached Data view's own re-run). */
   exec: Pick<QueryExecutionService, 'executeRead'>;
   /** #276 Phase 5: no flat `App` delegates for the params-group members this
-   *  module (and, via the `as VariableBarApp` cast below, variable-bar.ts) needs —
-   *  `app.params.*` directly. */
+   *  module needs — `app.params.*` directly. #478 replaced the former
+   *  `as VariableBarApp` cast with an explicit adapter below, so `saveFilterActive`
+   *  is now reached through that adapter's caller-neutral `saveActive` rather than
+   *  by variable-bar.ts reading this shape directly. */
   params: Pick<WorkbenchParameterSession, 'recordBoundParams' | 'saveVarValues' | 'saveFilterActive' | 'clearVarRecent'>;
   /** #276 Phase 5: no flat `App.savePref` delegate — `app.prefs.save(name,
    *  value)` directly (the cell-detail drawer's own resize persist). */
@@ -1111,7 +1113,22 @@ export function expandDataPane(app: ResultsApp, r: QueryResult): DetachedView {
         }).fields[name];
         // A committed field re-runs only this detached query (rerun ignores the
         // param name buildVariableBar passes — a single source re-runs wholesale).
-        const variableBar = buildVariableBar(app as VariableBarApp, fields, rerun, getField, { document: doc, ariaLabel: 'Query variables' });
+        // #478: an explicit adapter, not a same-shape cast — `activeByName`
+        // ALIASES the real `AppState.filterActive` object (the bar mutates it
+        // in place; a copy would silently stop `effectiveFilterActive` above,
+        // and every other Workbench reader, from observing the edit), and
+        // `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 },
+          params: {
+            saveVarValues: () => app.params.saveVarValues(),
+            saveActive: () => app.params.saveFilterActive(),
+            clearVarRecent: (name: string) => app.params.clearVarRecent(name),
+          },
+          wallNow: () => app.wallNow(),
+        };
+        const variableBar = buildVariableBar(variableBarApp, fields, rerun, getField, { document: doc, ariaLabel: 'Query variables' });
         variableBarDispose = variableBar.dispose;
         refreshBtn = h('button', {
           class: 'res-act detached-refresh', title: 'Re-run this query with the current variable values',

src/ui/variable-bar.ts

@@ -1,5 +1,5 @@
 // The shared `{name:Type}` variable bar: one field per parameter, driving the
-// same `state.varValues`/`state.filterActive` machinery the SQL Browser
+// same `state.varValues`/`state.activeByName` machinery the SQL Browser
 // workbench uses. Extracted from the dashboard (#149 D3) when the detached Data
 // view (#185) became its second consumer (CLAUDE.md rule 5) — both the
 // dashboard's global variables and the detached view's per-query variable row
@@ -45,7 +45,6 @@ import { Icon } from './icons.js';
 import type { KeyboardOwner } from './app.types.js';
 import type { VariableOption } from '../core/variable-options.types.js';
 import type { DashboardTimeRangeGroup, TimeRangeRecent } from '../core/time-range.js';
-import type { WorkbenchParameterSession } from '../application/workbench-parameter-session.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
@@ -56,12 +55,27 @@ export interface VariableBarApp {
   document: Document;
   state: {
     varValues: Record<string, string>;
-    filterActive: Record<string, boolean>;
+    /** #478: caller-neutral activation port — this module is shared by
+     *  Dashboard (an in-memory draft map, no persistence) and detached Data
+     *  (the persisted Workbench `AppState.filterActive`), so it must not name
+     *  either caller's storage model. ALIASED by both callers, never copied:
+     *  this module mutates it in place (a select's commit, a text field's
+     *  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;
   };
   /** #276 Phase 5: no flat `App.saveVarValues`/`saveFilterActive`/
-   *  `clearVarRecent` delegates — this module reads `app.params.*` directly. */
-  params: Pick<WorkbenchParameterSession, 'saveVarValues' | 'saveFilterActive' | 'clearVarRecent'>;
+   *  `clearVarRecent` delegates — this module reads `app.params.*` directly.
+   *  #478: `saveActive` replaces the Workbench-named `saveFilterActive` — a
+   *  caller-neutral port every caller supplies its OWN explicit adapter for
+   *  (Dashboard: a no-op, its draft is never persisted; detached Data: routes
+   *  to `app.params.saveFilterActive()`, the real persisted Workbench save). */
+  params: {
+    saveVarValues(): void;
+    saveActive(): void;
+    clearVarRecent(name: string): void;
+  };
   wallNow(): number;
 }
 
@@ -295,7 +309,7 @@ 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.filterActive` / `app.state.varRecent` with every other surface.
+ * `app.state.activeByName` / `app.state.varRecent` with every other surface.
  * 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
@@ -435,7 +449,7 @@ export function buildVariableBar(
       name: p.name,
       options: spec.options ?? [],
       selected: spec.selection ?? [],
-      active: !!app.state.filterActive[p.name],
+      active: !!app.state.activeByName[p.name],
       loading: !!spec.loading,
       incomplete: !!spec.optionsIncomplete,
       title: p.name + ': ' + p.type,
@@ -444,7 +458,7 @@ export function buildVariableBar(
       // Activation travels with the value: a non-empty selection is active, and
       // Clear-then-Apply returns the variable to unset.
       onApply: (values, active) => {
-        app.state.filterActive[p.name] = active;
+        app.state.activeByName[p.name] = active;
         options.onCommitVariableSelection?.(p.name, values, active);
       },
       onKeyboardOwnerChange: options.onKeyboardOwnerChange,
@@ -471,7 +485,7 @@ export function buildVariableBar(
       name: p.name,
       options: spec.options ?? [],
       value: app.state.varValues[p.name] || '',
-      active: !!app.state.filterActive[p.name],
+      active: !!app.state.activeByName[p.name],
       title: p.name + ': ' + p.type,
       onCommit: (value, active) => {
         // A select commits value AND activation together — a pick (or the × that
@@ -480,7 +494,7 @@ export function buildVariableBar(
         // every other reader (the invalid-field affordance, a sibling rebuild)
         // still sees one source of truth.
         app.state.varValues[p.name] = value;
-        app.state.filterActive[p.name] = active;
+        app.state.activeByName[p.name] = active;
         options.onCommitVariable?.(p.name, value, active);
       },
     });
@@ -501,7 +515,7 @@ export function buildVariableBar(
    * a native checkbox is indeterminate while inactive, then emits the same
    * canonical strings the former true/false combobox options emitted. */
   const buildBoolField = (p: FieldControl): HTMLElement => {
-    const active = !!app.state.filterActive[p.name];
+    const active = !!app.state.activeByName[p.name];
     const optionalHint = p.optional ? ' — optional: unset leaves its filter block out' : '';
     const hintId = `var-bool-${idSafe(p.name)}-hint`;
     const input = h('input', {
@@ -512,7 +526,7 @@ export function buildVariableBar(
       onchange: () => {
         const value = input.checked ? 'true' : 'false';
         app.state.varValues[p.name] = value;
-        app.state.filterActive[p.name] = true;
+        app.state.activeByName[p.name] = true;
         options.onCommitVariable?.(p.name, value, true);
       },
     });
@@ -568,9 +582,9 @@ export function buildVariableBar(
       // Text controls sync activation with the value (#165): an activation
       // flip re-runs affected tiles exactly like a value change (same
       // debounce + generation guard downstream).
-      app.state.filterActive[p.name] = input.value !== '';
+      app.state.activeByName[p.name] = input.value !== '';
       app.params.saveVarValues();
-      app.params.saveFilterActive();
+      app.params.saveActive();
       applyFieldState(input, getField(p.name, 'input'), baseTitle, combo?.previewEl);
       // `!`: DOM's clearTimeout is a documented no-op on `null`/`undefined` —
       // the original .js called it unconditionally (`timer` starts `null`).

tests/e2e/time-range.html

@@ -176,8 +176,8 @@
     const draftActive = {};
     const variableBarApp = {
       document,
-      state: { varValues: draftValues, filterActive: draftActive, varRecent: {} },
-      params: { saveVarValues() {}, saveFilterActive() {}, clearVarRecent() {} },
+      state: { varValues: draftValues, activeByName: draftActive, varRecent: {} },
+      params: { saveVarValues() {}, saveActive() {}, clearVarRecent() {} },
       wallNow: () => WALL,
     };
     // Session-lifetime, per-group "Recently used" ranges (owner decision: not

tests/unit/results.test.ts

@@ -10,6 +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 { formatRows } from '../../src/core/format.js';
 import { queryPanel } from '../../src/core/saved-query.js';
 import type { AppState, ResultSort } from '../../src/state.js';
@@ -902,6 +903,64 @@ describe('expandDataPane', () => {
     expect(app.exec.executeRead).not.toHaveBeenCalled(); // open = snapshot, no request
   });
 
+  // #478: `variable-bar.ts`'s `VariableBarApp.state.activeByName` is a
+  // caller-neutral port; this detached-Data caller must ALIAS the real
+  // `AppState.filterActive` object into it, never copy it — the bar mutates
+  // that map in place (a text field's activation-follows-value, #165), so a
+  // copy would silently strand `effectiveFilterActive`/every other Workbench
+  // reader on the old, never-updated object. The identity check below is
+  // what makes a copy-instead-of-alias regression fail: capture the object
+  // reference BEFORE the bar is ever built, then prove the bar's own edit
+  // lands on that SAME object, and that it routes through the real,
+  // persisted `saveFilterActive` (not a Dashboard-style no-op).
+  it('#478: aliases the real AppState.filterActive object (not a copy) and calls saveFilterActive', () => {
+    const app = makeApp();
+    const activeRef = app.state.filterActive;
+    expandDataPane(app, paramResult());
+    const overlay = qs(document, '.graph-overlay');
+    const input = qs<HTMLInputElement>(overlay, '.detached-variable-row .var-field input');
+    input.value = 'Warning';
+    input.dispatchEvent(new Event('input', { bubbles: true })); // #165: activation syncs synchronously
+    // Still the exact same object — the adapter aliased it at construction,
+    // it never swapped `app.state.filterActive` for a fresh reference.
+    expect(app.state.filterActive).toBe(activeRef);
+    // ...and the bar's own write landed on that real object, not a private copy.
+    expect(activeRef.level).toBe(true);
+    // The adapter's `saveActive` routed to the real persisted Workbench save.
+    expect(app.params.saveFilterActive).toHaveBeenCalled();
+  });
+
+  // #478: the adapter's remaining two members — `wallNow` and `clearVarRecent`
+  // — are each closures over the real `app`, not the real `app` reached
+  // around; these two tests are this file's only exercise of either closure
+  // body (a same-shape cast would need neither, so these regress a silent
+  // revert back to `app as VariableBarApp`).
+  it('#478: a DateTime field resolves its preview through the adapter\'s wallNow closure', () => {
+    const wallNow = vi.fn(() => 0);
+    const app = makeApp({ wallNow });
+    const r = tableResult();
+    r.source = {
+      sql: 'SELECT n, s FROM t WHERE d = {asOf:DateTime}',
+      tabId: 't1', rowLimit: 100, title: 'Filtered', description: 'warnings only',
+    };
+    app.state.varValues.asOf = '-1d';
+    expandDataPane(app, r);
+    expect(wallNow).toHaveBeenCalled();
+  });
+
+  it('#478: Clear recent on the field footer routes through the adapter\'s clearVarRecent closure', () => {
+    const app = makeApp();
+    app.state.varRecent = recordRecent(emptyRecentMap(), 'level', 'Warning');
+    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'));
+    qs<HTMLButtonElement>(overlay, '.var-combo-footer button').dispatchEvent(
+      new MouseEvent('mousedown', { bubbles: true, cancelable: true }),
+    );
+    expect(app.params.clearVarRecent).toHaveBeenCalledWith('level');
+  });
+
   it('a retained Refresh button is inert after its detached view closes', async () => {
     const app = makeApp();
     app.state.varValues.level = 'Warning';