diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f729f28..0881d439 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index bbddfe42..caae0ae4 100644 --- a/src/ui/dashboard.ts +++ b/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: () => {}, diff --git a/src/ui/results.ts b/src/ui/results.ts index b48fc252..5bdce953 100644 --- a/src/ui/results.ts +++ b/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(), diff --git a/src/ui/variable-bar.ts b/src/ui/variable-bar.ts index 83c9f85f..f7ba0f47 100644 --- a/src/ui/variable-bar.ts +++ b/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; - 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, diff --git a/tests/e2e/time-range.html b/tests/e2e/time-range.html index 4c211e9b..518c2586 100644 --- a/tests/e2e/time-range.html +++ b/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, }; diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index 6ddaf435..ecd7477d 100644 --- a/tests/unit/dashboard.test.ts +++ b/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(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(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)', () => { diff --git a/tests/unit/results.test.ts b/tests/unit/results.test.ts index 3e14486d..ba59faef 100644 --- a/tests/unit/results.test.ts +++ b/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(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(overlay, '.detached-variable-row .var-field input'); + input.dispatchEvent(new Event('focus')); + expect(qs(overlay, '[role="option"]')).not.toBeNull(); // 'Warning' is listed + qs(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'; diff --git a/tests/unit/variable-bar.test.ts b/tests/unit/variable-bar.test.ts index 7c6a34e5..1d2d7d9d 100644 --- a/tests/unit/variable-bar.test.ts +++ b/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): 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');