From b3b65a5ffd6b44081fb53b143bfc800acf9f3a48 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 14:32:02 +0000 Subject: [PATCH 01/16] feat(#360): allow parameters in Filter sources; add shared prepareFilterSource (core) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Filter-role source SQL may now declare its own {name:Type} query parameters: the filter-source-parameters diagnostic is removed from filterSqlDiagnostics (structural rules — empty / statement-count / row-returning / owned-FORMAT — stay). Two new pure functions wrap the shared param-pipeline so both the Dashboard viewer and the Workbench Filter preview prepare a source identically (no independently-implemented binding): - analyzeFilterSource(sql, {sourceBackedParams, label}) -> {sql, analysis, dependsOn, diagnostics}: structural + single-layer cascading diagnostics (a dep backed by ANOTHER Filter source -> filter-source-cascading) and the declared param names, derived once per SQL edit. - prepareFilterSource(analyzed, {values, active, wallNowMs}) -> readiness ('runnable'|'waiting'|'error') + owned transport params (caps only, no readonly) merged with native param_ args + materialized execSql + bound-param snapshots. missing -> waiting; invalid/errors/diagnostics -> error; else runnable. validationMode is always 'execute'. Foundation for #360 (Wave 1). Dashboard runtime + Workbench parity follow. filter-execution.ts stays 100/100/100/100; full gate + build green. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/filter-execution.ts | 147 +++++++++++++++++++++++++++- tests/unit/filter-execution.test.ts | 122 ++++++++++++++++++++++- 2 files changed, 262 insertions(+), 7 deletions(-) diff --git a/src/core/filter-execution.ts b/src/core/filter-execution.ts index daf59ae..0691b7c 100644 --- a/src/core/filter-execution.ts +++ b/src/core/filter-execution.ts @@ -1,6 +1,8 @@ import { detectSqlFormat as _detectSqlFormat } from './format.js'; -import { analysisView } from './param-pipeline.js'; -import { scanParamDeclarations } from './param-scan.js'; +import { + analyzeParameterizedSources, prepareParameterizedBatch, mergedSourceArgs, mergedSourceSql, +} from './param-pipeline.js'; +import type { ParameterAnalysis, BoundParamSnapshot } from './param-pipeline.js'; import { isRowReturning as _isRowReturning, splitStatements as _splitStatements } from './sql-split.js'; import { diagnostic as makeDiagnostic } from './diagnostics.js'; import type { Diagnostic } from './diagnostics.js'; @@ -46,9 +48,6 @@ export function filterSqlDiagnostics(sql?: string | null): FilterSqlDiagnostic[] } else if (!isRowReturning(statements[0])) { out.push(diagnostic('filter-sql-not-row-returning', 'Filter SQL must be a row-returning statement.')); } - if (scanParamDeclarations(analysisView(text)).length) { - out.push(diagnostic('filter-source-parameters', 'Filter SQL cannot declare query parameters.')); - } if (detectSqlFormat(text)) { out.push(diagnostic('filter-owned-format', 'Filter SQL cannot include a trailing FORMAT clause.')); } @@ -90,3 +89,141 @@ export function filterExecution(sql?: string | null, defaults: FilterExecutionDe error: diagnostics.length ? diagnostics[0].message : null, }; } + +// #360: a Filter-role source may now declare its OWN `{name:Type}` parameters +// (previously banned outright by the `filter-source-parameters` diagnostic +// removed above) — as long as every one of them is backed by ANOTHER source's +// own control (a workbench tab param, a dashboard-level filter), never by a +// second Filter source: a Filter depending on a Filter would need to re-run +// in a strict dependency order this app has no scheduler for (the single- +// layer cascading rule). `analyzeFilterSource` wraps the shared +// `param-pipeline.js` analysis phase for exactly one Filter source and folds +// that rule in as a diagnostic alongside the structural ones +// (`filterSqlDiagnostics`). + +/** `analyzeFilterSource`'s return shape: the analyzed pipeline source (kept, + * not re-derived, by `prepareFilterSource`), the parameter names this + * source's SQL depends on, and every static reason it can't run. */ +export interface FilterSourceAnalysis { + sql: string; + analysis: ParameterAnalysis; + dependsOn: string[]; + diagnostics: FilterSqlDiagnostic[]; +} + +/** + * Analyze one Filter source's SQL: the structural contract + * (`filterSqlDiagnostics`) plus the shared parameter pipeline's analysis of + * its own declared `{name:Type}` params (#360). `dependsOn` is every + * parameter name this source's SQL declares — required outside any block or + * confined to an optional block — in first-appearance order. + * `opts.sourceBackedParams` is the set of names backed by ANOTHER Filter + * source in the same dashboard; depending on any of them is the one + * cascading violation this dashboard model disallows, and each becomes its + * own `filter-source-cascading` diagnostic naming `opts.label` and the + * offending parameter. Pure. + */ +export function analyzeFilterSource( + sql: string | null | undefined, + opts: { sourceBackedParams?: Iterable; label?: string } = {}, +): FilterSourceAnalysis { + const text = String(sql || ''); + const structural = filterSqlDiagnostics(text); + const analysis = analyzeParameterizedSources([ + { id: 'filter', label: opts.label, kind: 'filter', sql: text, bindPolicy: 'row-returning' }, + ]); + const dependsOn = Object.keys(analysis.fields).filter((name) => { + const f = analysis.fields[name]; + return f.requiredIn.includes('filter') || f.optionalIn.includes('filter'); + }); + const backed = new Set(opts.sourceBackedParams || []); + const cascading: FilterSqlDiagnostic[] = []; + for (const name of dependsOn) { + if (backed.has(name)) { + cascading.push(diagnostic( + 'filter-source-cascading', + `Filter source "${opts.label || 'source'}" depends on source-backed parameter "${name}". Cascading Filter sources are not supported.`, + )); + } + } + return { sql: text, analysis, dependsOn, diagnostics: [...structural, ...cascading] }; +} + +/** The three states `prepareFilterSource` classifies a Filter source into: + * `'error'` (a structural/cascading diagnostic, an invalid committed value, + * or a source-level template error), `'waiting'` (a required param has no + * value yet — a normal mid-fill state, not an error), or `'runnable'`. */ +export type FilterSourceReadiness = 'runnable' | 'waiting' | 'error'; + +/** `prepareFilterSource`'s return shape: everything a caller needs to either + * show a banner/spinner or actually send the request. */ +export interface FilterSourcePreparation { + readiness: FilterSourceReadiness; + diagnostics: FilterSqlDiagnostic[]; + dependsOn: string[]; + missing: string[]; + invalid: string[]; + errors: string[]; + error: string | null; + execSql: string; + params: Record; + format: 'Filter'; + rowLimit: number; + boundParams: BoundParamSnapshot[]; +} + +/** + * Prepare one already-analyzed Filter source against concrete `values` + * (#360): wraps `prepareParameterizedBatch` (always `validationMode: + * 'execute'` — a Filter source runs as soon as it's ready; there's no + * separate blur/Enter commit step) and folds the result together with + * `analyzed.diagnostics` into one readiness verdict, the owned transport + * `params` (`filterExecution`'s caps ∪ the bound `param_` args), and + * the materialized `execSql` to send. Takes the pre-analyzed source so a + * caller re-running this every value edit derives `dependsOn` (and re-scans + * the SQL) exactly once per SQL edit, not once per value edit. Pure. + */ +export function prepareFilterSource( + analyzed: FilterSourceAnalysis, + opts: { values?: Record; active?: Record; wallNowMs?: number } = {}, +): FilterSourcePreparation { + const prepared = prepareParameterizedBatch(analyzed.analysis, { + values: opts.values, + active: opts.active, + wallNowMs: opts.wallNowMs, + validationMode: 'execute', + }); + // `analyzed.analysis` was built from exactly one source (`analyzeFilterSource` + // above) — `prepareParameterizedBatch` preserves that 1:1 source cardinality, + // so `sources[0]` always exists. + const src = prepared.sources[0]; + const diagnostics = analyzed.diagnostics; + const readiness: FilterSourceReadiness = diagnostics.length || src.invalid.length || src.errors.length + ? 'error' + : src.missing.length + ? 'waiting' + : 'runnable'; + const owned = filterExecution(analyzed.sql).params; + const params = { ...owned, ...mergedSourceArgs(src) }; + const error = diagnostics.length + ? diagnostics[0].message + : src.invalid.length + ? `Invalid value for: ${src.invalid.join(', ')}` + : src.errors.length + ? src.errors[0] + : null; + return { + readiness, + diagnostics, + dependsOn: analyzed.dependsOn, + missing: src.missing.slice(), + invalid: src.invalid.slice(), + errors: src.errors.slice(), + error, + execSql: mergedSourceSql(src, analyzed.sql), + params, + format: 'Filter', + rowLimit: FILTER_TOP_LEVEL_ROW_LIMIT, + boundParams: src.statements.flatMap((s) => s.boundParams), + }; +} diff --git a/tests/unit/filter-execution.test.ts b/tests/unit/filter-execution.test.ts index 7adc6e4..8ebd170 100644 --- a/tests/unit/filter-execution.test.ts +++ b/tests/unit/filter-execution.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import { FILTER_RESULT_BYTE_CAP, FILTER_TOP_LEVEL_ROW_LIMIT, filterExecution, filterSqlDiagnostics, + analyzeFilterSource, prepareFilterSource, } from '../../src/core/filter-execution.js'; describe('Filter execution', () => { @@ -16,9 +17,126 @@ describe('Filter execution', () => { expect(filterSqlDiagnostics('')).toMatchObject([{ code: 'filter-sql-empty' }]); expect(filterSqlDiagnostics('SELECT 1; SELECT 2').map((d) => d.code)).toContain('filter-sql-statement-count'); expect(filterSqlDiagnostics('CREATE TABLE t (x Int8)').map((d) => d.code)).toContain('filter-sql-not-row-returning'); - expect(filterSqlDiagnostics('SELECT {x:String}').map((d) => d.code)).toContain('filter-source-parameters'); - expect(filterSqlDiagnostics('SELECT 1 /*[ WHERE x={x:String} ]*/').map((d) => d.code)).toContain('filter-source-parameters'); expect(filterSqlDiagnostics('SELECT 1 FORMAT JSON').map((d) => d.code)).toContain('filter-owned-format'); expect(filterExecution('SELECT 1 FORMAT JSON').error).toContain('FORMAT'); }); + it('#360: Filter SQL may now declare its own query parameters — no filter-source-parameters diagnostic', () => { + expect(filterSqlDiagnostics('SELECT {x:String}').map((d) => d.code)).not.toContain('filter-source-parameters'); + expect(filterSqlDiagnostics('SELECT 1 /*[ WHERE x={x:String} ]*/').map((d) => d.code)).not.toContain('filter-source-parameters'); + }); +}); + +describe('analyzeFilterSource (#360)', () => { + it('derives dependsOn from both required (outside every block) and block-confined optional params', () => { + const a = analyzeFilterSource('SELECT 1 WHERE a = {a:String} /*[ AND b = {b:String} ]*/'); + expect(a.dependsOn).toEqual(['a', 'b']); + expect(a.diagnostics).toEqual([]); + expect(a.sql).toBe('SELECT 1 WHERE a = {a:String} /*[ AND b = {b:String} ]*/'); + }); + it('rejects a param backed by another Filter source (cascading) but not one that is unbacked', () => { + const a = analyzeFilterSource('SELECT 1 WHERE a = {a:String} /*[ AND b = {b:String} ]*/', { + sourceBackedParams: ['a'], + label: 'MyFilter', + }); + expect(a.diagnostics).toHaveLength(1); + expect(a.diagnostics[0]).toMatchObject({ code: 'filter-source-cascading' }); + expect(a.diagnostics[0].message).toBe( + 'Filter source "MyFilter" depends on source-backed parameter "a". Cascading Filter sources are not supported.', + ); + }); + it('defaults the label to "source" in the cascading message when omitted', () => { + const a = analyzeFilterSource('SELECT 1 WHERE q = {q:String}', { sourceBackedParams: ['q'] }); + expect(a.diagnostics[0].message).toContain('Filter source "source" depends on source-backed parameter "q".'); + }); + it('structural diagnostics still fire alongside the pipeline analysis', () => { + expect(analyzeFilterSource('').diagnostics.map((d) => d.code)).toContain('filter-sql-empty'); + expect(analyzeFilterSource('SELECT 1; SELECT 2').diagnostics.map((d) => d.code)).toContain('filter-sql-statement-count'); + expect(analyzeFilterSource('CREATE TABLE t (x Int8)').diagnostics.map((d) => d.code)).toContain('filter-sql-not-row-returning'); + expect(analyzeFilterSource('SELECT 1 FORMAT JSON').diagnostics.map((d) => d.code)).toContain('filter-owned-format'); + }); + it('has no dependsOn when the SQL declares no params', () => { + expect(analyzeFilterSource('SELECT 1').dependsOn).toEqual([]); + expect(analyzeFilterSource(null).dependsOn).toEqual([]); + }); + it('excludes a param confined to a non-row-returning (unbound) statement from dependsOn', () => { + // CREATE isn't row-returning, so this statement never binds — its + // declaration is recorded (for cross-source type-conflict detection) but + // never enters requiredIn/optionalIn for this source. + const a = analyzeFilterSource('CREATE TABLE t (x Int8) COMMENT {z:String}'); + expect(a.dependsOn).toEqual([]); + }); +}); + +describe('prepareFilterSource (#360)', () => { + it('runnable: materializes execSql, binds param_ args, snapshots boundParams, error is null', () => { + const a = analyzeFilterSource('SELECT 1 WHERE n = {n:UInt8}'); + const p = prepareFilterSource(a, { values: { n: '42' } }); + expect(p.readiness).toBe('runnable'); + expect(p.error).toBeNull(); + expect(p.missing).toEqual([]); + expect(p.invalid).toEqual([]); + expect(p.errors).toEqual([]); + expect(p.dependsOn).toEqual(['n']); + expect(p.format).toBe('Filter'); + expect(p.rowLimit).toBe(FILTER_TOP_LEVEL_ROW_LIMIT); + expect(p.execSql).toBe('SELECT 1 WHERE n = {n:UInt8}'); + expect(p.params).toMatchObject({ param_n: '42', max_result_bytes: FILTER_RESULT_BYTE_CAP }); + expect(p.params).not.toHaveProperty('readonly'); + expect(p.params).not.toHaveProperty('read_only'); + expect(p.boundParams).toHaveLength(1); + expect(p.boundParams[0]).toMatchObject({ name: 'n', serializedValue: '42' }); + }); + it('waiting: a required param with an empty value gates as missing, no error, params still built', () => { + const a = analyzeFilterSource('SELECT 1 WHERE d = {d:DateTime}'); + const p = prepareFilterSource(a, { values: {} }); + expect(p.readiness).toBe('waiting'); + expect(p.error).toBeNull(); + expect(p.missing).toEqual(['d']); + expect(p.invalid).toEqual([]); + expect(p.params).not.toHaveProperty('param_d'); + expect(p.params).toMatchObject({ max_result_bytes: FILTER_RESULT_BYTE_CAP }); + }); + it('error (invalid): a bad committed value under execute-mode validation gates and names the field', () => { + const a = analyzeFilterSource('SELECT 1 WHERE n = {n:UInt8}'); + const p = prepareFilterSource(a, { values: { n: '256' } }); + expect(p.readiness).toBe('error'); + expect(p.invalid).toEqual(['n']); + expect(p.error).toBe('Invalid value for: n'); + }); + it('error (diagnostics): a cascading violation surfaces as the reported error, ahead of runnable value state', () => { + const a = analyzeFilterSource('SELECT 1 WHERE a = {a:String}', { sourceBackedParams: ['a'], label: 'F' }); + const p = prepareFilterSource(a, { values: { a: 'x' } }); + expect(p.readiness).toBe('error'); + expect(p.error).toBe(a.diagnostics[0].message); + }); + it('error (source-level template error): a whole dropped block-only statement surfaces src.errors as the reported error', () => { + // The second "statement" is entirely an optional block, so the splitter + // drops it whole (comment-only) — analyzeParameterizedSources treats that + // mismatch as a source-level template error distinct from any structural + // Filter-SQL diagnostic or invalid/missing value. + const a = analyzeFilterSource('SELECT 1; /*[ SELECT 2 WHERE {a:String} ]*/'); + expect(a.diagnostics).toEqual([]); + const p = prepareFilterSource(a, { values: {} }); + expect(p.readiness).toBe('error'); + expect(p.errors).toEqual(['optional block: a block cannot wrap a whole statement']); + expect(p.error).toBe('optional block: a block cannot wrap a whole statement'); + }); + it('defaults opts to {} when omitted (no values/active/wallNowMs)', () => { + const a = analyzeFilterSource('SELECT 1'); + const p = prepareFilterSource(a); + expect(p.readiness).toBe('runnable'); + expect(p.error).toBeNull(); + }); + it('resolves two relative params in one statement against a single pinned wallNowMs', () => { + const a = analyzeFilterSource('SELECT 1 WHERE a >= {from:DateTime} AND a < {to:DateTime}'); + const nowMs = 1751200000000; + const p = prepareFilterSource(a, { values: { from: '-1h', to: '-30m' }, wallNowMs: nowMs }); + const expectedFrom = String(Math.round((nowMs - 3600000) / 1000)); + const expectedTo = String(Math.round((nowMs - 1800000) / 1000)); + expect(p.readiness).toBe('runnable'); + expect(p.params).toMatchObject({ param_from: expectedFrom, param_to: expectedTo }); + const byName = Object.fromEntries(p.boundParams.map((b) => [b.name, b])); + expect(byName.from.resolvedValue).toBe(expectedFrom); + expect(byName.to.resolvedValue).toBe(expectedTo); + }); }); From 644a6704377d52c6caab12dbdc7f1a84ce34c282 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 15:10:43 +0000 Subject: [PATCH 02/16] feat(#360): parameterized Filter sources in the Dashboard viewer runtime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Dashboard viewer now runs parameterized Filter-role source queries fed by committed root filter values (native param_), with single-layer dependency scheduling on top of #359's one-runtime-per-source boundary. - FilterSourceRuntime gains `analyzed` (analyzeFilterSource at construction, with the dashboard's source-backed param set for cascading rejection) and a `waiting` status; each wave prepares the source via prepareFilterSource against committedRootValues() (inactive root filters blanked so they gate) and one deps.wallNow() reading per wave (relative {from}/{to} share one clock). - A source issues NO request while waiting (missing deps) or errored (invalid value / structural / cascading); runnable sources execute the materialized execSql with the bound args, then recordBoundParams for parity. - Selective rerun: committing a root filter reruns ONLY the sources whose SQL declares it (runFilterSourceWave), then folds the changed param plus any reconciliation-deactivated names into ONE combined affected-panel wave (commitAndRerun); the selective wave preflights its own token. - applyFilterProviders returns the flipped (deactivated) names and, per the plan review, skips re-deriving a mid-flight non-plan source's consumers so an overlapping selective wave can't flip a still-loading source back to stale options. Published per-filter view gains optional `stale`/`waitingFor`. - Fixes the stray NUL byte in optionsSignature (from #361). Wave 2a of #360. Filter-bar waiting/stale UI (2b) + Workbench parity (3) follow. dashboard-viewer-session.ts: 100% stmts/fns/lines, 94.75% branches (gate ≥90). Full gate + build green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../application/dashboard-viewer-session.ts | 233 +++++++++-- tests/unit/dashboard-viewer-session.test.ts | 395 ++++++++++++++++++ 2 files changed, 598 insertions(+), 30 deletions(-) diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index 6325f6c..55edb5d 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -35,7 +35,8 @@ import { detectSqlFormat } from '../../core/format.js'; import { DASH_TILE_ROW_CAP, DASH_TILE_BYTE_CAP } from '../../core/dashboard.js'; import { queryName } from '../../core/saved-query.js'; import { panelExecution } from '../../core/panel-execution.js'; -import { filterExecution } from '../../core/filter-execution.js'; +import { analyzeFilterSource, prepareFilterSource } from '../../core/filter-execution.js'; +import type { FilterSourceAnalysis } from '../../core/filter-execution.js'; import { readFilterOptions } from '../../core/filter-options.js'; import { mergeDashboardFilterHelpers } from '../../core/dashboard-filters.js'; import type { @@ -100,8 +101,12 @@ export interface ViewerTileState { * invalid option, unused, …). * - `source-error` — the shared source query itself failed (missing query, * invalid SQL, transport/exec error). + * - `waiting` (#360) — the shared source has a runnable query but one of its + * OWN `{name:Type}` parameters (fed by another, root Dashboard filter) has + * no value yet. Not an error — a normal mid-fill state. * A plain filter (no `sourceQueryId`) never leaves `idle`. */ -export type ViewerFilterStatus = 'idle' | 'loading' | 'ready' | 'missing-helper' | 'helper-error' | 'source-error'; +export type ViewerFilterStatus = + 'idle' | 'loading' | 'waiting' | 'ready' | 'missing-helper' | 'helper-error' | 'source-error'; /** One Dashboard filter's runtime state. */ export interface ViewerFilterState { @@ -118,6 +123,16 @@ export interface ViewerFilterState { * filter-bar rebuild signature so a same-length-but-different option set * (or a null->non-null->null cycle) still triggers a rebuild (#359). */ optionsRev: number; + /** #360: true while this filter's shared source is mid-flight (`loading`) + * or genuinely blocked pending a dependency (`waiting`) — the currently + * published `options` should not be trusted as a fresh, actionable answer. + * False for every settled terminal status (`ready`, and every error + * status). Optional so no other consumer of this shape breaks. */ + stale?: boolean; + /** #360: when `status` is `'waiting'`, the missing dependency parameter + * names (from `FilterSourcePreparation.missing`) the filter-bar UI/ + * diagnostics can name. Absent otherwise. */ + waitingFor?: string[]; } /** The Dashboard's per-render layout view (#291) — a discriminated union over @@ -304,8 +319,18 @@ interface FilterSourceRuntime { consumers: FilterRuntime[]; gen: number; abortController: AbortController | null; - status: 'idle' | 'loading' | 'ready' | 'error'; + status: 'idle' | 'waiting' | 'loading' | 'ready' | 'error'; provider: FilterProvider | null; + /** #360: this source's own static analysis (structural + cascading + * diagnostics, and the root parameter names — `dependsOn` — its OWN + * `{name:Type}` declarations depend on). Computed once at construction + * from the source's SQL; `prepareFilterSource` re-prepares it against + * concrete committed values every wave without re-scanning the SQL. */ + analyzed: FilterSourceAnalysis; + /** #360: the last-known missing dependency parameter names (from + * `FilterSourcePreparation.missing`) while `status === 'waiting'` — read + * by `applyFilterProviders` to publish each consumer's `waitingFor`. */ + missing: string[]; } const cfgType = (panel: unknown): string | undefined => @@ -422,6 +447,16 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa }); const filterById = new Map(filters.map((filter) => [filter.def.id, filter])); + // #360: parameters BACKED BY a Filter source (every filter definition that + // has a `sourceQueryId`) — a shared source's OWN `{name:Type}` declarations + // may never depend on one of these (a Filter depending on a Filter would + // need a strict dependency order this app has no scheduler for); passed to + // `analyzeFilterSource` below so that cascading dependency becomes its own + // `filter-source-cascading` diagnostic per source, at construction time. + const sourceBackedParams = new Set( + filters.filter((filter) => filter.sourceId).map((filter) => filter.def.parameter), + ); + // One `FilterSourceRuntime` per UNIQUE `sourceQueryId` (#359 — the bug this // refactor fixes: N definitions sharing a source used to run it N times, // each provider keyed by the wrong id, so the merge rejected every helper @@ -433,9 +468,14 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa if (!filter.sourceId) continue; let source = filterSources.get(filter.sourceId); if (!source) { + const sourceQuery = queryById.get(filter.sourceId); source = { - id: filter.sourceId, query: queryById.get(filter.sourceId), + id: filter.sourceId, query: sourceQuery, consumers: [], gen: 0, abortController: null, status: 'idle', provider: null, + analyzed: analyzeFilterSource(sourceQuery?.sql, { + sourceBackedParams, label: sourceQuery ? queryName(sourceQuery) : filter.sourceId, + }), + missing: [], }; filterSources.set(filter.sourceId, source); } @@ -481,6 +521,22 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa const activeMap = (): Record => Object.fromEntries(filters.map((filter) => [filter.def.parameter, filter.state.active])); + // #360: the committed values of every ROOT filter (no `sourceQueryId`) — the + // only filters whose values a shared Filter source's own `{name:Type}` + // declarations can depend on. An INACTIVE root's value is deliberately + // blanked (never its retained-but-inactive raw value): `clearFilter` keeps + // the typed value and only flips `active` to false, so without blanking + // here the pipeline's own missing-check would see a non-empty stale value + // and never gate the dependent source to `waiting`. + const committedRootValues = (): Record => { + const out: Record = {}; + for (const filter of filters) { + if (filter.sourceId) continue; + out[filter.def.parameter] = filter.state.active ? toValueString(filter.state.value) : ''; + } + return out; + }; + // Prepare a batch, optionally against a caller's DRAFT values/active (the // filter bar's in-progress typing) rather than the committed filter state — // so live #170 validation can run without mutating committed state. @@ -637,7 +693,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa * are DISTINCT signatures (a clear must still bump `optionsRev` when the * prior state had real options), so this is not just `JSON.stringify`. */ function optionsSignature(options: FilterHelperOption[] | null): string { - return options === null ? 'null' : JSON.stringify(options.map((option) => [option.value, option.label])); + return options === null ? 'null' : JSON.stringify(options.map((option) => [option.value, option.label])); } /** Replace one consumer's curated options, bumping `optionsRev` ONLY when @@ -651,15 +707,24 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa /** Runs ONE shared source's query (was per-filter — #359's fix: N filter * definitions sharing a `sourceQueryId` now execute it exactly once per - * wave). Sets the TRANSPORT terminal (`ready`/`error`) and STORES the - * normalized `FilterProvider` on `source.provider` — but ONLY for the - * current generation: every stale-gen guard bails BEFORE the status/provider - * write, so a superseded run leaves the last-known provider intact (returns - * `null`). Per-consumer curation status is derived afterward in - * `applyFilterProviders`, which merges the COMPLETE provider set from all - * source runtimes (the #360 boundary — retained providers survive a - * selective wave). */ - async function runFilterSource(source: FilterSourceRuntime, generation: number): Promise { + * wave). #360: the source may now declare its OWN `{name:Type}` params + * (fed by root Dashboard filters) — `prepareFilterSource` (over the + * source's construction-time `analyzed` analysis) classifies it + * `'runnable'` | `'waiting'` | `'error'` against the wave's committed root + * values BEFORE any request is sent. Sets the TRANSPORT terminal + * (`ready`/`waiting`/`error`) and STORES the normalized `FilterProvider` on + * `source.provider` — but ONLY for the current generation: every stale-gen + * guard bails BEFORE the status/provider write, so a superseded run leaves + * the last-known provider intact (returns `null`). Per-consumer curation + * status is derived afterward in `applyFilterProviders`, which merges the + * COMPLETE provider set from all source runtimes (the #360 boundary — + * retained providers survive a selective wave). `waveMs` is the ONE wall + * clock reading the whole wave shares (`runFilterWave`/ + * `runFilterSourceWave` each capture it exactly once, before building their + * plan) — never read again per-source here. */ + async function runFilterSource( + source: FilterSourceRuntime, generation: number, waveMs: number, + ): Promise { if (!source.query) { // REUSE the static-validation code `filter-source-missing` // (workspace-semantics.ts) — this is the RUNTIME analog: the source @@ -675,23 +740,45 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa return provider; } const query = source.query; - const execution = filterExecution(query.sql); - if (execution.error) { + const prep = prepareFilterSource(source.analyzed, { + values: committedRootValues(), + active: effectiveActive(committedRootValues(), activeMap()), + wallNowMs: waveMs, + }); + if (prep.readiness === 'error') { + // A structural/cascading diagnostic (`analyzed.diagnostics`) always + // wins as the reported reason when present; otherwise this is purely an + // invalid-committed-value verdict from `prepareParameterizedBatch` with + // no diagnostic of its own — synthesize one from `prep.error` so the + // provider's `diagnostics` is never empty for an `'error'` readiness. const provider: FilterProvider = { - sourceId: source.id, sourceName: queryName(query), helpers: [], diagnostics: execution.diagnostics, + sourceId: source.id, sourceName: queryName(query), helpers: [], + diagnostics: prep.diagnostics.length + ? prep.diagnostics + : [coreDiagnostic('error', 'filter-source-invalid', prep.error || 'Filter source is invalid.', { sourceId: source.id })], }; if (source.gen !== generation) return null; source.status = 'error'; source.provider = provider; return provider; } + if (prep.readiness === 'waiting') { + // Blocked on a missing dependency — a normal mid-fill state, not an + // error: no request, no error diagnostic. `applyFilterProviders` reads + // `source.missing` to publish each consumer's `waitingFor`. + if (source.gen !== generation) return null; + source.status = 'waiting'; + source.missing = prep.missing.slice(); + source.provider = null; + return null; + } if (source.gen !== generation) return null; - const result = newResult(execution.format, execution.rowLimit); + const result = newResult(prep.format, prep.rowLimit); const controller = new AbortController(); source.abortController = controller; await deps.exec.executeRead(result, { - sql: query.sql, format: execution.format, rowLimit: execution.rowLimit, - params: execution.params, signal: controller.signal, + sql: prep.execSql, format: prep.format, rowLimit: prep.rowLimit, + params: prep.params, signal: controller.signal, }); if (source.gen !== generation) return null; source.abortController = null; @@ -709,6 +796,9 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa }); provider = { sourceId: source.id, sourceName: queryName(query), ...normalized }; source.status = normalized.helpers.length ? 'ready' : 'error'; + // #171 bound-param recording, for parity with `runTile` (~:614) — the + // shared source may itself now bind real params (#360). + deps.recordBoundParams?.(prep.boundParams); } source.provider = provider; return provider; @@ -721,8 +811,10 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa * reconciles `merged.changed` — all before this function returns, so the * affected-tile wave `refresh()` runs next sees the reconciled `active` * (the PRECONDITION from plan review: this must NOT be deferred to a - * microtask). */ - function applyFilterProviders(plan: { source: FilterSourceRuntime; generation: number }[]): void { + * microtask). Returns the deactivated (flipped) parameter names — + * `merged.changed` — so a selective (#360) caller can fold them into the + * SAME affected-panel wave as the parameter that triggered the rerun. */ + function applyFilterProviders(plan: { source: FilterSourceRuntime; generation: number }[]): string[] { // Stale-wave guard (#359 "a stale generation cannot publish options or // activation changes"): a superseded (or destroyed) wave's own source // results were already discarded in `runFilterSource` (returned `null`), @@ -732,7 +824,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // already-correct state. Source generations move all-at-once (every // `runFilterWave`/`destroy` bumps them together), so any mismatch means // this whole wave is stale and must publish nothing. - if (plan.some(({ source, generation }) => source.gen !== generation)) return; + if (plan.some(({ source, generation }) => source.gen !== generation)) return []; // Merge the COMPLETE set of source providers — not just the ones a wave // re-ran — so retained (last-known) providers survive a future selective // wave (#360). A source that has never produced one contributes nothing. @@ -749,17 +841,39 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // renders diagnostics, not per-filter status, so a bare `missing-helper` // left an unexplained empty control). const diagnostics: FilterDiagnostic[] = [...merged.diagnostics]; + // #360 BLOCKER-1 (cross-source race): a source NOT in THIS plan that is + // still `loading` (a different, possibly-overlapping selective wave is + // running it right now) or settled `waiting` on its own dependency must + // not have its consumers re-derived here — its own wave's + // `applyFilterProviders` call owns that. Every source that IS in this + // plan, or a settled non-plan source (`ready`/`error`/`idle`), derives + // normally — only the per-consumer status/options assignment is skipped + // for a mid-flight non-plan source; the merge above still ran over the + // complete provider set. + const planIds = new Set(plan.map(({ source }) => source.id)); for (const source of filterSources.values()) { + if (!planIds.has(source.id) && (source.status === 'loading' || source.status === 'waiting')) continue; for (const consumer of source.consumers) { if (source.status === 'error') { setConsumerOptions(consumer, null); consumer.state.status = 'source-error'; + consumer.state.stale = false; + consumer.state.waitingFor = undefined; + continue; + } + if (source.status === 'waiting') { + setConsumerOptions(consumer, null); + consumer.state.status = 'waiting'; + consumer.state.stale = true; + consumer.state.waitingFor = source.missing.slice(); continue; } const field = merged.fields[consumer.def.parameter]; if (field) { setConsumerOptions(consumer, field.options); consumer.state.status = 'ready'; + consumer.state.stale = false; + consumer.state.waitingFor = undefined; continue; } // The source succeeded but this consumer's parameter never got a @@ -769,6 +883,8 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // Simple helperName match only — no code enumeration, no sourceId key. const namedByDiagnostic = merged.diagnostics.some((diag) => diag.helperName === consumer.def.parameter); setConsumerOptions(consumer, null); + consumer.state.stale = false; + consumer.state.waitingFor = undefined; if (namedByDiagnostic) { consumer.state.status = 'helper-error'; } else { @@ -783,14 +899,22 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // Reconcile: a value no longer among its curated options is deactivated // (value KEPT — matches clearFilter's reactivation semantics). `changed` // only ever names source-backed parameters, whose tiles are already in - // `affectedByFilterWave` — no separate union step is needed. + // `affectedByFilterWave` — no separate union step is needed for the FULL + // wave; a selective (#360) caller still folds `changed` into its own + // affected-panel wave via the returned array below. for (const parameter of merged.changed) { for (const filter of filters) if (filter.def.parameter === parameter) filter.state.active = false; } + return merged.changed; } async function runFilterWave(): Promise { filterDiagnostics = []; + // One wall-clock reading for the WHOLE wave (mirrors the tile wave's own + // `deps.wallNow()` capture in `refresh()`) — every source in this wave's + // plan resolves any relative `{name:DateTime}` dependency against the + // exact same instant, never a per-source read. + const waveMs = deps.wallNow(); const sources = [...filterSources.values()]; const plan = sources.map((source) => ({ source, generation: supersede(source) })); for (const { source } of plan) { @@ -799,17 +923,47 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // and WITHOUT touching `options`/`optionsRev`: clearing options mid-wave // would flip a non-empty filter bar to empty on every refresh, churn // the UI's rebuild signature, and destroy in-progress typing. - for (const consumer of source.consumers) consumer.state.status = 'loading'; + for (const consumer of source.consumers) { consumer.state.status = 'loading'; consumer.state.stale = true; } } publish(); // Each source stores its own provider on `source.provider`; the terminal // merge reads the complete set, so the pool's returns are awaited only for // sequencing, not collected. await runPool(plan, VIEWER_TILE_CONCURRENCY, - ({ source, generation }) => runFilterSource(source, generation)); + ({ source, generation }) => runFilterSource(source, generation, waveMs)); applyFilterProviders(plan); } + /** #360: rerun only the shared Filter sources whose OWN `{name:Type}` + * declarations depend on one of `changedParams` (a root filter's just- + * committed parameter names) — a source with no such dependency is never + * superseded/rerun. One wall-clock reading for this whole selective wave, + * same discipline as `runFilterWave`. Returns the deactivated (flipped) + * parameter names from `applyFilterProviders` (`[]` when no source was + * affected) so the caller can fold them into ONE combined affected-panel + * wave alongside `changedParams`. */ + async function runFilterSourceWave(changedParams: string[]): Promise { + // A commit-triggered rerun issues a REAL `executeRead` for the affected + // source(s) — unlike `runFilterWave` (always reached via `refresh()`, + // which already preflighted once for the whole cycle) this selective wave + // is entered directly from `commitAndRerun`, with no preflight upstream. + // Same guard `runAffectedWave` uses before its own executeReads. + if (!(await preflight())) return []; + const waveMs = deps.wallNow(); + const affected = [...filterSources.values()].filter((source) => + source.analyzed.dependsOn.some((name) => changedParams.includes(name))); + if (affected.length === 0) return []; + const plan = affected.map((source) => ({ source, generation: supersede(source) })); + for (const source of affected) { + source.status = 'loading'; + for (const consumer of source.consumers) { consumer.state.status = 'loading'; consumer.state.stale = true; } + } + publish(); + await runPool(plan, VIEWER_TILE_CONCURRENCY, + ({ source, generation }) => runFilterSource(source, generation, waveMs)); + return applyFilterProviders(plan); + } + // ── Waves ───────────────────────────────────────────────────────────────── @@ -879,6 +1033,25 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa (runtime) => runTile(runtime, prepared.get(runtime.tile.id), generations.get(runtime.tile.id)!)); } + // #360: after committing a value (these four are commit paths only — never + // in-progress typing), rerun any shared Filter source that depends on the + // just-committed root parameter(s) FIRST, then fold both the committed + // parameter(s) AND any names `runFilterSourceWave` itself deactivated + // (`merged.changed`, e.g. a now-out-of-range curated value elsewhere) into + // ONE combined affected-panel wave — never two separate waves. A synchronous + // pre-check skips `runFilterSourceWave` (and the microtask hop awaiting it + // costs) entirely when nothing depends on `changed` — behaviorally + // identical to awaiting it (an unaffected wave always resolves to `[]` with + // no side effect), just without the extra tick for the overwhelmingly + // common case of a Dashboard with no parameterized Filter sources (#360). + async function commitAndRerun(changed: string[]): Promise { + const hasAffectedSource = [...filterSources.values()] + .some((source) => source.analyzed.dependsOn.some((name) => changed.includes(name))); + if (!hasAffectedSource) { await runAffectedWave(changed); return; } + const flipped = await runFilterSourceWave(changed); + await runAffectedWave([...new Set([...changed, ...flipped])]); + } + async function setFilter(filterId: string, value: unknown): Promise { if (destroyed) return; const filter = filterById.get(filterId); @@ -886,7 +1059,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa filter.state.value = value; filter.state.active = value != null && value !== ''; publish(); - await runAffectedWave([filter.def.parameter]); + await commitAndRerun([filter.def.parameter]); } async function applyFilter(filterId: string, value: unknown, active: boolean): Promise { @@ -898,7 +1071,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa filter.state.value = value; filter.state.active = active; publish(); - await runAffectedWave([filter.def.parameter]); + await commitAndRerun([filter.def.parameter]); } async function clearFilter(filterId: string): Promise { @@ -908,7 +1081,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // Deactivate but keep the value so reactivation restores it. filter.state.active = false; publish(); - await runAffectedWave([filter.def.parameter]); + await commitAndRerun([filter.def.parameter]); } async function clearAllFilters(): Promise { @@ -923,7 +1096,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa } publish(); // Coalesce every reset into ONE affected-panel wave (#188 clear-all). - if (changed.length) await runAffectedWave(changed); + if (changed.length) await commitAndRerun(changed); } function cancelTile(tileId: string): void { diff --git a/tests/unit/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index 8ee1b23..bbba37e 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -1082,3 +1082,398 @@ describe('flow layout (mobile normalization)', () => { mobile = false; }); }); + +// #360: a shared Filter source may now declare its OWN `{name:Type}` params, +// fed by ROOT Dashboard filters (no `sourceQueryId`) rather than the source's +// consumers. `analyzeFilterSource`/`prepareFilterSource` (src/core/ +// filter-execution.ts) classify each source `'runnable'` | `'waiting'` | +// `'error'` against the wave's COMMITTED root values before any request is +// sent, and committing a root filter's value selectively reruns only the +// sources that actually depend on it — folded into the SAME affected-panel +// wave as the committed parameter(s). +describe('parameterized Filter sources (#360)', () => { + it('a source depending on an inactive/blank root param is waiting — no execution, waitingFor published, its consumer marked stale', async () => { + const { exec, calls } = makeExec((sql) => (sql.includes('depsrc') + ? { columns: [{ name: 'region', type: 'Array(String)' }], rows: [[['east', 'west']]] } + : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: false, defaultValue: '' }, + { id: 'f-region', parameter: 'region', sourceQueryId: 'src' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT 1 AS n'), + query('src', "SELECT ['east','west'] AS region FROM t WHERE ts >= {from:String} /* depsrc */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + expect(calls.some((c) => c.sql.includes('depsrc'))).toBe(false); + const f = session.state.value.filters.find((flt) => flt.id === 'f-region')!; + expect(f.status).toBe('waiting'); + expect(f.waitingFor).toEqual(['from']); + expect(f.options).toBeNull(); + expect(f.stale).toBe(true); + }); + + it('a runnable dependent source executes bound to the committed root-param values; the curated field applies', async () => { + const { exec, calls } = makeExec((sql) => (sql.includes('depsrc') + ? { columns: [{ name: 'region', type: 'Array(String)' }], rows: [[['east']]] } + : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: true, defaultValue: '2024-01-01' }, + { id: 'to-root', parameter: 'to', defaultActive: true, defaultValue: '2024-02-01' }, + { id: 'f-region', parameter: 'region', sourceQueryId: 'src' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT {region:String} AS n'), + query('src', "SELECT ['east'] AS region FROM t WHERE ts >= {from:String} AND ts < {to:String} /* depsrc */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + const srcCall = calls.find((c) => c.sql.includes('depsrc')); + expect(srcCall).toBeDefined(); + expect(srcCall!.params.param_from).toBe('2024-01-01'); + expect(srcCall!.params.param_to).toBe('2024-02-01'); + const f = session.state.value.filters.find((flt) => flt.id === 'f-region')!; + expect(f.status).toBe('ready'); + expect(f.stale).toBe(false); + }); + + it('an invalid committed root value gates the dependent source to source-error, no execution', async () => { + const { exec, calls } = makeExec(() => ({ columns: [{ name: 'n' }], rows: [[1]] })); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'n-root', parameter: 'n', defaultActive: true, defaultValue: 'not-a-number' }, + { id: 'f-region', parameter: 'region', sourceQueryId: 'src' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT {region:String} AS n2'), + query('src', "SELECT ['east'] AS region FROM t WHERE code = {n:UInt16} /* depsrc */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + expect(calls.some((c) => c.sql.includes('depsrc'))).toBe(false); + const f = session.state.value.filters.find((flt) => flt.id === 'f-region')!; + expect(f.status).toBe('source-error'); + }); + + it('a source declaring a dependency on ANOTHER source-backed parameter never executes and reports the cascading diagnostic', async () => { + const { exec, calls } = makeExec((sql) => { + if (sql.includes('srcA')) return { columns: [{ name: 'catA', type: 'Array(String)' }], rows: [[['x']]] }; + if (sql.includes('srcB')) return { columns: [{ name: 'catB', type: 'Array(String)' }], rows: [[['y']]] }; + return { columns: [{ name: 'n' }], rows: [[1]] }; + }); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'fa', parameter: 'catA', sourceQueryId: 'srcA' }, + { id: 'fb', parameter: 'catB', sourceQueryId: 'srcB' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT {catB:String} AS n'), + query('srcA', "SELECT ['x'] AS catA /* srcA */", { dashboard: { role: 'filter' } }), + query('srcB', "SELECT ['y'] AS catB FROM t WHERE cat = {catA:String} /* srcB */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + expect(calls.some((c) => c.sql.includes('srcB'))).toBe(false); + const fb = session.state.value.filters.find((flt) => flt.id === 'fb')!; + expect(fb.status).toBe('source-error'); + const cascadeDiag = session.state.value.filterDiagnostics.find((d) => d.code === 'filter-source-cascading'); + expect(cascadeDiag?.message).toContain('Cascading'); + }); + + it('selective rerun: committing a dependency reruns ONLY the sources that depend on it', async () => { + const { exec, calls } = makeExec((sql) => { + if (sql.includes('srcFrom')) return { columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['a']]] }; + if (sql.includes('srcCat')) return { columns: [{ name: 'dep2', type: 'Array(String)' }], rows: [[['b']]] }; + return { columns: [{ name: 'n' }], rows: [[1]] }; + }); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: true, defaultValue: 'v0' }, + { id: 'cat-root', parameter: 'category', defaultActive: true, defaultValue: 'X' }, + { id: 'f-dep1', parameter: 'dep1', sourceQueryId: 'srcFrom' }, + { id: 'f-dep2', parameter: 'dep2', sourceQueryId: 'srcCat' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT 1 AS n'), + query('srcFrom', "SELECT ['a'] AS dep1 FROM t WHERE ts >= {from:String} /* srcFrom */", { dashboard: { role: 'filter' } }), + query('srcCat', "SELECT ['b'] AS dep2 FROM t WHERE cat = {category:String} /* srcCat */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + const base = calls.length; + await session.setFilter('from-root', 'v1'); + const added = calls.slice(base); + expect(added.some((c) => c.sql.includes('srcFrom'))).toBe(true); + expect(added.some((c) => c.sql.includes('srcCat'))).toBe(false); + }); + + it("a reconciliation deactivation from the selective source rerun runs its dependent panel in the SAME wave as the changed root param", async () => { + let fromValue = 'v0'; + const { exec, calls } = makeExec((sql) => { + if (sql.includes('regsrc')) { + return fromValue === 'v0' + ? { columns: [{ name: 'region', type: 'Array(String)' }], rows: [[['R1', 'R2']]] } + : { columns: [{ name: 'region', type: 'Array(String)' }], rows: [[['R2']]] }; + } + return { columns: [{ name: 'n' }], rows: [[1]] }; + }); + const document = doc({ + tiles: [tile('t-region', 'q-region')], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: true, defaultValue: 'v0' }, + { id: 'f-region', parameter: 'region', sourceQueryId: 'regsrc', defaultActive: true, defaultValue: 'R1' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('q-region', 'SELECT {region:String} AS n'), + query('regsrc', "SELECT ['R1','R2'] AS region FROM t WHERE ts >= {from:String} /* regsrc */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + expect(session.state.value.tiles[0].status).toBe('ready'); // bound to R1 initially + fromValue = 'v1'; // regsrc will now only offer R2 once rerun + const before = calls.length; + await session.setFilter('from-root', 'v1'); + // The region tile's own rerun never bound the now-stale 'R1' — the + // reconciliation (region deactivated) applied BEFORE this SAME wave's + // affected-panel run, not in some later, separate wave. + expect(calls.slice(before).some((c) => c.params.param_region === 'R1')).toBe(false); + expect(session.state.value.tiles[0].status).toBe('unfilled'); + expect(session.state.value.filters.find((flt) => flt.id === 'f-region')!.active).toBe(false); + expect(session.state.value.filters.find((flt) => flt.id === 'f-region')!.value).toBe('R1'); // value retained + }); + + it('clearAllFilters resets every dependency-bearing root param in ONE selective wave — both dependent sources transition together', async () => { + const { exec, calls } = makeExec((sql) => { + if (sql.includes('srcFrom')) return { columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['a']]] }; + if (sql.includes('srcCat')) return { columns: [{ name: 'dep2', type: 'Array(String)' }], rows: [[['b']]] }; + return { columns: [{ name: 'n' }], rows: [[1]] }; + }); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + // Defaults are BLANK/inactive — the reset target `clearAllFilters` + // restores. Both roots are activated below via `setFilter` first, so + // the clear genuinely changes them (a default already equal to the + // active value would make `clearAllFilters` a no-op). + { id: 'from-root', parameter: 'from', defaultActive: false, defaultValue: '' }, + { id: 'cat-root', parameter: 'category', defaultActive: false, defaultValue: '' }, + { id: 'f-dep1', parameter: 'dep1', sourceQueryId: 'srcFrom' }, + { id: 'f-dep2', parameter: 'dep2', sourceQueryId: 'srcCat' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT {dep1:String} AS a, {dep2:String} AS b'), + query('srcFrom', "SELECT ['a'] AS dep1 FROM t WHERE ts >= {from:String} /* srcFrom */", { dashboard: { role: 'filter' } }), + query('srcCat', "SELECT ['b'] AS dep2 FROM t WHERE cat = {category:String} /* srcCat */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('waiting'); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep2')!.status).toBe('waiting'); + await session.setFilter('from-root', 'v0'); + await session.setFilter('cat-root', 'X'); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('ready'); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep2')!.status).toBe('ready'); + const base = calls.length; + await session.clearAllFilters(); + // Neither source executed again — both roots reset to blank/inactive, so + // both correctly gate to 'waiting' rather than firing a stale request — + // but BOTH transitioned together, in this ONE clearAllFilters commit. + expect(calls.slice(base).some((c) => c.sql.includes('srcFrom') || c.sql.includes('srcCat'))).toBe(false); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('waiting'); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep2')!.status).toBe('waiting'); + }); + + it("overlapping selective waves: a settling wave for one source does not flip a different, still-in-flight source's consumer to a settled state (BLOCKER-1)", async () => { + let releaseA!: () => void; + const gateA = new Promise((resolve) => { releaseA = resolve; }); + let srcFromCalls = 0; + const { exec } = makeExec((sql) => { + if (sql.includes('srcFrom')) { + srcFromCalls += 1; + return srcFromCalls === 1 + ? { columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['a1']]] } + : gateA.then(() => ({ columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['a2']]] })); + } + if (sql.includes('srcRegion')) return { columns: [{ name: 'dep2', type: 'Array(String)' }], rows: [[['b']]] }; + return { columns: [{ name: 'n' }], rows: [[1]] }; + }); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: true, defaultValue: 'v0' }, + { id: 'region-root', parameter: 'region', defaultActive: true, defaultValue: 'r0' }, + { id: 'f-dep1', parameter: 'dep1', sourceQueryId: 'srcFrom' }, + { id: 'f-dep2', parameter: 'dep2', sourceQueryId: 'srcRegion' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT {dep1:String} AS a, {dep2:String} AS b'), + query('srcFrom', "SELECT ['a'] AS dep1 FROM t WHERE ts >= {from:String} /* srcFrom */", { dashboard: { role: 'filter' } }), + query('srcRegion', "SELECT ['b'] AS dep2 FROM t WHERE r = {region:String} /* srcRegion */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('ready'); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep2')!.status).toBe('ready'); + + const waveA = session.setFilter('from-root', 'v1'); // rerun srcFrom — gated (2nd call) + await flush(); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('loading'); + + const waveB = session.setFilter('region-root', 'r1'); // unrelated: rerun srcRegion only, resolves immediately + await waveB; + // BLOCKER-1: B's settling wave must NOT have flipped A's (still in-flight) + // consumer to a settled state from A's stale provider. + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('loading'); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep2')!.status).toBe('ready'); + + releaseA(); + await waveA; + const f1 = session.state.value.filters.find((flt) => flt.id === 'f-dep1')!; + expect(f1.status).toBe('ready'); + expect(f1.options).toEqual([{ value: 'a2', label: 'a2' }]); + }); + + it('a superseded SELECTIVE-wave source response never publishes (stale-gen guard holds for runFilterSourceWave too)', async () => { + let releaseFirst!: () => void; + const gate = new Promise((resolve) => { releaseFirst = resolve; }); + let call = 0; + const { exec } = makeExec((sql) => { + if (!sql.includes('srcFrom')) return { columns: [{ name: 'n' }], rows: [[1]] }; + call += 1; + if (call === 1) return { columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['init']]] }; + return call === 2 + ? gate.then(() => ({ columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['STALE']]] })) + : { columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['FRESH']]] }; + }); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: true, defaultValue: 'v0' }, + { id: 'f-dep1', parameter: 'dep1', sourceQueryId: 'srcFrom' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT {dep1:String} AS n'), + query('srcFrom', "SELECT ['x'] AS dep1 FROM t WHERE ts >= {from:String} /* srcFrom */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + const first = session.setFilter('from-root', 'v1'); // call #2 — gated + await flush(); + const second = session.setFilter('from-root', 'v2'); // supersedes; call #3 resolves immediately + releaseFirst(); + await Promise.all([first, second]); + const f = session.state.value.filters.find((flt) => flt.id === 'f-dep1')!; + expect(f.options).toEqual([{ value: 'FRESH', label: 'FRESH' }]); + }); + + it('preflights before executing an affected Filter source — a stale token blocks the request rather than firing it', async () => { + const { exec, calls } = makeExec((sql) => (sql.includes('srcFrom') + ? { columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['a']]] } + : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: false, defaultValue: '' }, + { id: 'f-dep1', parameter: 'dep1', sourceQueryId: 'srcFrom' }, + ], + }); + let tokenOk = true; + const onAuthFailed = vi.fn(); + const session = createDashboardViewerSession(makeDeps({ + document, exec, onAuthFailed, + connection: { ensureFreshToken: async () => tokenOk }, + queries: [ + query('qt', 'SELECT {dep1:String} AS n'), + query('srcFrom', "SELECT ['a'] AS dep1 FROM t WHERE ts >= {from:String} /* srcFrom */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); // construction wave, token fresh — srcFrom stays 'waiting' ('from' is blank) + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('waiting'); + const base = calls.length; + tokenOk = false; // token now stale + // Committing 'from' makes srcFrom affected (it depends on 'from'), so + // commitAndRerun enters runFilterSourceWave — which must preflight BEFORE + // issuing any executeRead, exactly like runAffectedWave/refresh already do. + await expect(session.setFilter('from-root', 'v1')).resolves.toBeUndefined(); + expect(calls.slice(base).some((c) => c.sql.includes('srcFrom'))).toBe(false); + expect(onAuthFailed).toHaveBeenCalled(); + // No rerun happened at all — status is untouched. + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('waiting'); + }); + + it('resolves two DIFFERENT sources\' relative-time dependency against ONE waveMs per wave (no per-source clock read)', async () => { + const { exec, calls } = makeExec((sql) => { + if (sql.includes('src1')) return { columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['a']]] }; + if (sql.includes('src2')) return { columns: [{ name: 'dep2', type: 'Array(String)' }], rows: [[['b']]] }; + return { columns: [{ name: 'n' }], rows: [[1]] }; + }); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 't-root', parameter: 't', defaultActive: true, defaultValue: '-1h' }, + { id: 'f-dep1', parameter: 'dep1', sourceQueryId: 'src1' }, + { id: 'f-dep2', parameter: 'dep2', sourceQueryId: 'src2' }, + ], + }); + let n = 0; + const wallNow = vi.fn(() => { n += 1000; return n; }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, wallNow, + queries: [ + query('qt', 'SELECT 1 AS n'), + query('src1', "SELECT ['a'] AS dep1 FROM t WHERE ts >= {t:DateTime} /* src1 */", { dashboard: { role: 'filter' } }), + query('src2', "SELECT ['b'] AS dep2 FROM t WHERE ts >= {t:DateTime} /* src2 */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + const call1 = calls.find((c) => c.sql.includes('src1'))!; + const call2 = calls.find((c) => c.sql.includes('src2'))!; + expect(call1.params.param_t).toBe(call2.params.param_t); // one shared clock reading for the whole wave + }); + + // The historical embedded-NUL-byte bug in `optionsSignature`'s `null` arm + // (fixed in this same change) is verified by direct byte inspection during + // implementation, not by an in-suite file read: a clean Node `fs` read from + // a strict `.ts` test needs its own ambient module declaration (see the + // `tests/types/node-crypto.d.ts` precedent -- `declare module` must live in + // its own import/export-free file), which is a new file outside this + // worker's two-file scope. The `null`-signature branch itself is already + // exercised end-to-end above (every `waiting`/`source-error` transition + // clears `options` to `null` via `setConsumerOptions`, and the `ready` + // transitions set it back to a real array). +}); From b374f8a986ed86e81fcc0ae4fcb14f7d20010fa9 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 15:31:26 +0000 Subject: [PATCH 03/16] feat(#360): filter-bar waiting/stale/error affordance for source-backed filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A source-backed Dashboard filter with no options (waiting on a root dependency, mid-flight, or errored) used to fall out of the curated rendering path and render as a bare, unlabelled plain field — the plan-review BLOCKER-2. Now: - rebuildFilterBar gates the curated path on `f.status !== 'idle'` (a plain filter stays 'idle' forever), forwarding status/stale/waitingFor, so a source-backed filter stays curated regardless of whether it has options yet. - The barSig rebuild trigger gains a sticky-status term (waiting + the three error statuses + waitingFor) so an idle→waiting transition with unchanged options actually repaints; bare 'loading' is excluded to preserve #359's same-content-refresh-never-rebuilds invariant. - filter-bar renders a STRUCTURAL, test-observable affordance (happy-dom sees no CSS): is-waiting (disabled + "Waiting for: " text), is-error (disabled), is-stale (disabled, last options kept but marked not-current). - filter-option-field.ts is untouched (affordance applied onto the input it already returns) — zero cross-wave contract risk. Wave 2b of #360. filter-bar.ts 100% (branches 97.18); dashboard.ts floor held. Full gate + build green. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ui/dashboard.ts | 40 ++++++++++++++-- src/ui/filter-bar.ts | 58 +++++++++++++++++++++-- tests/unit/dashboard.test.ts | 43 +++++++++++++++++ tests/unit/filter-bar.test.ts | 89 +++++++++++++++++++++++++++++++++++ 4 files changed, 220 insertions(+), 10 deletions(-) diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index a886724..26e50d8 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -595,12 +595,27 @@ export async function renderDashboard(app: DashboardApp): Promise { function rebuildFilterBar(sview: DashboardViewState): void { filterBarDispose?.(); const idByParam = new Map(); - const curatedFields: Record = {}; + // #360 (plan-review BLOCKER-2): a source-backed filter used to fall OUT of + // the curated rendering path the moment it had no options — waiting on a + // root dependency, mid-flight, or errored — silently rendering as a bare, + // unlabelled plain field with zero indication anything was pending. A + // plain (non-source-backed) filter is documented to stay 'idle' forever + // (ViewerFilterStatus), so gating on "not idle" keeps every source-backed + // filter curated regardless of whether it currently has options, and + // leaves the plain-filter path completely untouched. + const curatedFields: Record; + status: ViewerFilterState['status']; + stale?: boolean; + waitingFor?: string[]; + }> = {}; for (const f of sview.filters) { draftValues[f.parameter] = valueString(f.value); draftActive[f.parameter] = f.active; idByParam.set(f.parameter, f.id); - if (f.options && f.options.length) curatedFields[f.parameter] = { options: f.options }; + if (f.status !== 'idle') { + curatedFields[f.parameter] = { options: f.options ?? [], status: f.status, stale: f.stale, waitingFor: f.waitingFor }; + } } const onCommit = (name: string): void => { const id = idByParam.get(name); @@ -1564,9 +1579,24 @@ export async function renderDashboard(app: DashboardApp): Promise { } lastMobile = mobileNow; // Rebuild the shared filter bar only when its field structure changes - // (activation, committed value, or curated options arriving) — not on tile - // progress ticks — so in-progress typing is not disturbed mid-wave. - const sig = JSON.stringify(sview.filters.map((f) => [f.id, f.active, valueString(f.value), !!(f.options && f.options.length), f.optionsRev])); + // (activation, committed value, curated options arriving, or — #360 — a + // source-backed filter settling into `waiting` or an error status) — not + // on tile progress ticks — so in-progress typing is not disturbed + // mid-wave. Without a status term here, an idle→waiting transition + // (`options`/`optionsRev` unchanged throughout — a blocked source never + // ran) left the SAME signature and never rebuilt, so the waiting + // affordance silently never painted (plan-review BLOCKER-2). A bare + // `'loading'` blip is deliberately EXCLUDED from this term: it always + // pairs with `stale: true` but leaves `options`/`optionsRev` untouched + // (`runFilterWave` never clears them mid-wave), so a refresh that settles + // right back to the exact same curated content still must NOT rebuild — + // preserving #359's own invariant (verified by its own suite) that an + // unchanged republish never disturbs in-progress typing. + const sig = JSON.stringify(sview.filters.map((f) => { + const stickyStatus = f.status === 'waiting' || f.status === 'source-error' + || f.status === 'helper-error' || f.status === 'missing-helper' ? [f.status, f.waitingFor] : null; + return [f.id, f.active, valueString(f.value), !!(f.options && f.options.length), f.optionsRev, stickyStatus]; + })); if (sig !== barSig) { barSig = sig; rebuildFilterBar(sview); } // #303: persist committed filter value/active into the isolated per-dashboard // store — isolated from the Workbench's asb:varValues/asb:filterActive keys. diff --git a/src/ui/filter-bar.ts b/src/ui/filter-bar.ts index 348dfee..2f89d68 100644 --- a/src/ui/filter-bar.ts +++ b/src/ui/filter-bar.ts @@ -55,9 +55,22 @@ export interface BuildFilterBarOptions { /** One curated Dashboard Filter field's config (#160) — the shape * `options.curatedFields[name]` carries, structurally read from the - * otherwise-`unknown` bag above. */ + * otherwise-`unknown` bag above. + * + * #360: `status`/`stale`/`waitingFor` mirror `ViewerFilterState`'s own + * fields (dashboard-viewer-session.ts) — `dashboard.ts`'s `rebuildFilterBar` + * forwards them straight through for every source-backed filter (gated on + * `status !== 'idle'`, never just "has options"), so a filter that's + * `waiting`/`loading`/errored with NO options yet still renders in this + * curated branch instead of silently falling out to the plain-text one + * below (plan-review BLOCKER-2). All three are optional so a caller that + * never supplies them (an older/simpler fixture) renders exactly like + * today's plain 'ready' combobox. */ interface CuratedFieldConfig { options: FilterFieldOption[]; + status?: string; + stale?: boolean; + waitingFor?: string[]; } // A combobox-based field controller's DOM wiring surface, PLUS the `el` @@ -155,6 +168,21 @@ export function buildFilterBar( + (conflictNote ? ' — ' + conflictNote : ''); const curated = options.curatedFields?.[p.name] as CuratedFieldConfig | undefined; if (curated) { + // #360: a shared source's transport/curation status drives a STRUCTURAL, + // test-observable affordance (a class + `disabled`/`aria-disabled`, and + // — for `waiting` — literal text naming the missing root params) rather + // than relying on styling alone (happy-dom never evaluates CSS layout). + // Absent `status` (an older/simpler fixture) behaves exactly like + // 'ready' — today's normal curated combobox, untouched. + const status = curated.status ?? 'ready'; + const isWaiting = status === 'waiting'; + const isError = status === 'source-error' || status === 'helper-error' || status === 'missing-helper'; + // 'loading' is the only other non-terminal status; `stale` mirrors it + // 1:1 per `ViewerFilterState`'s own contract — checking both is + // defensive, not redundant coverage of two different real states. + const isStale = !isWaiting && !isError && (status === 'loading' || !!curated.stale); + const waitingNote = isWaiting ? `Waiting for: ${(curated.waitingFor ?? []).join(', ')}` : ''; + const fieldTitle = isWaiting ? waitingNote : baseTitle; const field = buildFilterOptionField({ document, name: p.name, options: curated.options, value: app.state.varValues[p.name] ?? '', active: !!app.state.filterActive[p.name], @@ -167,7 +195,7 @@ export function buildFilterBar( }, onCommit: () => onCommit(p.name), }); - field.input.title = baseTitle; + field.input.title = fieldTitle; // #345: a curated field is always the 'enum' width band (short option // labels) regardless of the declared param type behind it. applyFieldWidth(field.input, p.type, true); @@ -177,9 +205,29 @@ export function buildFilterBar( // invalid against the prepared batch (e.g. a type conflict across // favorites), and without this it silently showed none of the // is-invalid class/tooltip/aria-invalid a plain filter field would. - applyFieldState(field.input, getField(p.name, 'execute'), baseTitle); - return h('label', { class: 'var-field is-curated' + (p.optional ? ' is-optional' : '') }, - h('span', { class: 'var-name' }, p.name), field.el); + applyFieldState(field.input, getField(p.name, 'execute'), fieldTitle); + if (isWaiting) { + field.input.classList.add('is-waiting'); + field.input.disabled = true; + field.input.setAttribute('aria-disabled', 'true'); + field.input.placeholder = waitingNote; + } else if (isError) { + field.input.classList.add('is-error'); + field.input.disabled = true; + field.input.setAttribute('aria-disabled', 'true'); + } else if (isStale) { + // Loading/stale: keep whatever options/value were last known (never + // cleared — the merge itself never blanks `options` mid-wave, see + // dashboard-viewer-session.ts's `runFilterWave`) but mark it plainly + // not-current rather than presenting a stale answer as fresh. + field.input.classList.add('is-stale'); + field.input.disabled = true; + field.input.setAttribute('aria-disabled', 'true'); + } + const stateClass = isWaiting ? ' is-waiting' : isError ? ' is-error' : isStale ? ' is-stale' : ''; + return h('label', { class: 'var-field is-curated' + (p.optional ? ' is-optional' : '') + stateClass }, + h('span', { class: 'var-name' }, p.name), field.el, + isWaiting ? h('span', { class: 'var-field-note' }, waitingNote) : null); } const commitNow = (): void => { if (timer == null) return; diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index de29f7f..fbc18a1 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -2582,6 +2582,49 @@ describe('renderDashboard — filter-source runtime rebuild + diagnostics (#359) }); }); +// #360 plan-review BLOCKER-2: `rebuildFilterBar` used to gate a filter into +// the curated (rich combobox) rendering path only `if (f.options && +// f.options.length)` — so a source-backed filter with NO options yet +// (waiting on a root dependency, mid-flight, or errored) fell OUT of that +// path entirely and rendered as a bare, unlabelled plain field with zero +// indication anything was pending. The gate is now "every non-idle filter" — +// a plain (non-source-backed) filter is documented to stay 'idle' forever, +// so this never touches the plain-filter path. +describe('renderDashboard — curated field stays curated while its shared source is waiting (#360)', () => { + it('a source-backed filter whose source waits on a root dependency renders the curated waiting affordance, not a bare plain field', async () => { + const { app, calls } = dashApp({ + workspace: wsWith({ + queries: [ + q('q1', 'SELECT k, v FROM a WHERE region = {region:String}'), + // 'src' depends on the ROOT filter 'from', which starts inactive/blank + // below — so this source is 'waiting', never executes ("depsrc" never + // appears in `calls`), and publishes `options: null` for 'region'. + q('src', "SELECT ['east','west'] AS region FROM a WHERE ts >= {from:String} -- depsrc", { dashboard: { role: 'filter' } }), + ], + tiles: [{ id: 't1', queryId: 'q1' }], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: false, defaultValue: '' }, + { id: 'f-region', parameter: 'region', sourceQueryId: 'src' }, + ], + }), + }); + await render(app); + expect(calls.some((c) => c.sql.includes('depsrc'))).toBe(false); // still waiting — never ran + const fields = qsa(app.root, '.dash-filter-host .var-field'); + const regionField = fields.find((f) => qs(f, '.var-name')?.textContent === 'region'); + expect(regionField).toBeDefined(); + const region = regionField as HTMLElement; + // Stays curated (not the old silent fall-out to a bare plain field) and + // carries the structural waiting affordance: class + disabled input + + // literal text naming the missing root param. + expect(region.classList.contains('is-curated')).toBe(true); + expect(region.classList.contains('is-waiting')).toBe(true); + const input = qs(region, 'input'); + expect(input.disabled).toBe(true); + expect(region.textContent).toContain('Waiting for: from'); + }); +}); + // #303: the isolated per-dashboard filter store (`asb:dashFilters`) — the // #280 viewer session used to init every filter purely from // `def.defaultValue`/`defaultActive`, so a committed value lived only in diff --git a/tests/unit/filter-bar.test.ts b/tests/unit/filter-bar.test.ts index 9827afe..b0acb0a 100644 --- a/tests/unit/filter-bar.test.ts +++ b/tests/unit/filter-bar.test.ts @@ -182,6 +182,95 @@ describe('buildFilterBar (shared filter row)', () => { }); }); + // #360: a source-backed curated field now stays in this rich-combobox + // rendering path for EVERY non-idle status — not just when it currently + // has options — and shows a structural (class + disabled/aria-disabled +, + // for 'waiting', literal text) affordance instead of silently rendering + // (or falling out to) an unmarked control (plan-review BLOCKER-2). + describe('curated field status affordance (#360)', () => { + it('an explicit status: "ready" (or an absent status) renders the normal curated combobox — no new classes, not disabled', () => { + const app = makeApp(); + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { + curatedFields: { x: { options: [{ value: 'a', label: 'Alpha' }], status: 'ready' } }, + }); + const label = bar.el.querySelector('.var-field')!; + const input = bar.el.querySelector('input') as HTMLInputElement; + expect(label.classList.contains('is-curated')).toBe(true); + expect(label.classList.contains('is-waiting')).toBe(false); + expect(label.classList.contains('is-error')).toBe(false); + expect(label.classList.contains('is-stale')).toBe(false); + expect(input.disabled).toBe(false); + expect(input.hasAttribute('aria-disabled')).toBe(false); + }); + + it('status: "waiting" disables the field, adds is-waiting, and names the missing params as literal text', () => { + const app = makeApp(); + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { + curatedFields: { x: { options: [], status: 'waiting', waitingFor: ['from', 'to'] } }, + }); + const label = bar.el.querySelector('.var-field')!; + const input = bar.el.querySelector('input') as HTMLInputElement; + expect(label.classList.contains('is-curated')).toBe(true); + expect(label.classList.contains('is-waiting')).toBe(true); + expect(input.disabled).toBe(true); + expect(input.getAttribute('aria-disabled')).toBe('true'); + expect(input.placeholder).toBe('Waiting for: from, to'); + expect(label.textContent).toContain('Waiting for: from, to'); + }); + + it('status: "waiting" with no waitingFor still renders (empty missing-list text, no throw)', () => { + const app = makeApp(); + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { + curatedFields: { x: { options: [], status: 'waiting' } }, + }); + const input = bar.el.querySelector('input') as HTMLInputElement; + expect(input.placeholder).toBe('Waiting for: '); + }); + + it.each(['source-error', 'helper-error', 'missing-helper'])( + 'status: "%s" disables the field and adds is-error, without the waiting note', (status) => { + const app = makeApp(); + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { + curatedFields: { x: { options: [], status } }, + }); + const label = bar.el.querySelector('.var-field')!; + const input = bar.el.querySelector('input') as HTMLInputElement; + expect(label.classList.contains('is-error')).toBe(true); + expect(label.classList.contains('is-waiting')).toBe(false); + expect(input.disabled).toBe(true); + expect(input.getAttribute('aria-disabled')).toBe('true'); + expect(label.querySelector('.var-field-note')).toBeNull(); + }, + ); + + it('status: "loading" marks the field is-stale + disabled while keeping its last-known options, not clearing them', () => { + const app = makeApp(); + app.state.varValues.x = 'a'; + app.state.filterActive.x = true; + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { + curatedFields: { x: { options: [{ value: 'a', label: 'Alpha' }], status: 'loading' } }, + }); + const label = bar.el.querySelector('.var-field')!; + const input = bar.el.querySelector('input') as HTMLInputElement; + expect(label.classList.contains('is-stale')).toBe(true); + expect(input.disabled).toBe(true); + expect(input.getAttribute('aria-disabled')).toBe('true'); + // The last-known selection is still shown (not blanked as "no longer + // current") — only marked stale, per the #360 acceptance criterion. + expect(input.value).toBe('Alpha'); + }); + + it('stale: true (independent of status) also marks the field is-stale, e.g. a ready-but-just-superseded read', () => { + const app = makeApp(); + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { + curatedFields: { x: { options: [{ value: 'a', label: 'Alpha' }], status: 'ready', stale: true } }, + }); + const label = bar.el.querySelector('.var-field')!; + expect(label.classList.contains('is-stale')).toBe(true); + expect((bar.el.querySelector('input') as HTMLInputElement).disabled).toBe(true); + }); + }); + it('dispose() clears a pending debounce timer so a later value edit never fires the stale commit (#276)', () => { vi.useFakeTimers(); try { From 08df743f8606983f04f00a4aaea1b61e26c070d7 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 15:34:41 +0000 Subject: [PATCH 04/16] fix(#360): clear a filter's options during a selective rerun so stale ones can't appear current Per the issue's "Error and stale-result behavior": when a dependency changes, prior options must not continue to appear current while the new source loads. runFilterSourceWave (the dependency-changed path only) now clears each affected consumer's options (setConsumerOptions null) when marking it loading, so the filter-bar repaints the is-stale/loading affordance instead of showing the old options as fresh; applyFilterProviders repopulates from the settled merge. runFilterWave (full refresh) is untouched, preserving #359's no-flicker-on-same-content invariant. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../application/dashboard-viewer-session.ts | 15 +++++- tests/unit/dashboard-viewer-session.test.ts | 48 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index 55edb5d..6676a07 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -956,7 +956,20 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa const plan = affected.map((source) => ({ source, generation: supersede(source) })); for (const source of affected) { source.status = 'loading'; - for (const consumer of source.consumers) { consumer.state.status = 'loading'; consumer.state.stale = true; } + for (const consumer of source.consumers) { + consumer.state.status = 'loading'; + consumer.state.stale = true; + // Selective-path ONLY (never `runFilterWave`'s full-refresh loop, + // which deliberately keeps options to preserve #359's no-flicker- + // on-same-content invariant): a committed dependency change means the + // OLD options are no longer known-current for the new inputs, so they + // must not keep rendering as if they were — clear them (via the + // shared helper, so `optionsRev` bumps) rather than leave a stale + // "current" value on screen through the loading window. + // `applyFilterProviders` repopulates from the fresh merge once this + // wave settles, so the clear is transient. + setConsumerOptions(consumer, null); + } } publish(); await runPool(plan, VIEWER_TILE_CONCURRENCY, diff --git a/tests/unit/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index bbba37e..27fc863 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -1230,6 +1230,54 @@ describe('parameterized Filter sources (#360)', () => { expect(added.some((c) => c.sql.includes('srcCat'))).toBe(false); }); + it("clears (not just marks stale) the affected consumer's options during a selective rerun's loading window — no stale-current options rendered before repopulating", async () => { + let releaseSecond!: () => void; + const gate = new Promise((resolve) => { releaseSecond = resolve; }); + let call = 0; + const { exec } = makeExec((sql) => { + if (!sql.includes('srcFrom')) return { columns: [{ name: 'n' }], rows: [[1]] }; + call += 1; + return call === 1 + ? { columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['a1']]] } // construction + : gate.then(() => ({ columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['a2']]] })); // selective rerun + }); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: true, defaultValue: 'v0' }, + { id: 'f-dep1', parameter: 'dep1', sourceQueryId: 'srcFrom' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT {dep1:String} AS n'), + query('srcFrom', "SELECT ['a'] AS dep1 FROM t WHERE ts >= {from:String} /* srcFrom */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + const initial = session.state.value.filters.find((flt) => flt.id === 'f-dep1')!; + expect(initial.status).toBe('ready'); + expect(initial.options).toEqual([{ value: 'a1', label: 'a1' }]); + + const wave = session.setFilter('from-root', 'v1'); // commit a dependency change -> selective rerun + await flush(); // let the rerun reach its (gated) executeRead + const midFlight = session.state.value.filters.find((flt) => flt.id === 'f-dep1')!; + expect(midFlight.status).toBe('loading'); + expect(midFlight.stale).toBe(true); + // The OLD options ('a1') must NOT still render as current while a + // committed dependency change is loading a fresh answer — cleared, not + // left stale-current, per the issue's error/stale-result acceptance. + expect(midFlight.options).toBeNull(); + + releaseSecond(); + await wave; + const settled = session.state.value.filters.find((flt) => flt.id === 'f-dep1')!; + expect(settled.status).toBe('ready'); + expect(settled.stale).toBe(false); + expect(settled.options).toEqual([{ value: 'a2', label: 'a2' }]); // repopulated once the wave settles + }); + it("a reconciliation deactivation from the selective source rerun runs its dependent panel in the SAME wave as the changed root param", async () => { let fromValue = 'v0'; const { exec, calls } = makeExec((sql) => { From 48116018bdeee305d5e1e43106943ac33fc8578b Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 15:53:07 +0000 Subject: [PATCH 05/16] feat(#360): Workbench Filter preview uses the shared prepareFilterSource (parity) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Workbench Filter-tab preview now prepares its source through the exact same analyzeFilterSource/prepareFilterSource pipeline the Dashboard uses, instead of the old standalone filterExecution() — so structural diagnostics, relative-value resolution (one wave clock), optional-block materialization, validation, serialization, and native param_ args are identical on both surfaces (the issue's "do not independently implement parameter binding"). - WorkbenchParameterSession.prepareFilterPreview(sql, wallNowMs) wraps the shared op with the var-strip's values/active (no sourceBackedParams — the Workbench has no Dashboard topology), mirroring prepareTabSource; wired through app.ts's createWorkbenchSession hooks and the fake-app test stub. - run()'s Filter branch: error/waiting return before any request; runnable executes execSql with the bound params and records boundParams. The generic var-gate/toast stays bypassed for Filter tabs (kept the !isFilter guard) so a missing param yields the non-error 'waiting' preview, not a toast-block. - filter-preview gains a 'waiting' state ("Waiting for: "). The role-agnostic variable strip still surfaces the tab's {from:DateTime} controls — the intended way to type test values. Wave 3 of #360. workbench-parameter-session.ts + filter-preview.ts 100%; workbench-session.ts 100% stmts/fns/lines. Full gate + build green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workbench-parameter-session.ts | 31 +++++++ src/ui/app.ts | 1 + src/ui/filter-preview.ts | 12 ++- src/ui/workbench/workbench-session.ts | 55 ++++++++---- tests/helpers/fake-app.ts | 4 + tests/unit/filter-preview.test.ts | 6 +- .../unit/workbench-parameter-session.test.ts | 55 ++++++++++++ tests/unit/workbench-session.test.ts | 86 ++++++++++++++++++- 8 files changed, 229 insertions(+), 21 deletions(-) diff --git a/src/application/workbench-parameter-session.ts b/src/application/workbench-parameter-session.ts index 65e6742..63ba7b0 100644 --- a/src/application/workbench-parameter-session.ts +++ b/src/application/workbench-parameter-session.ts @@ -52,6 +52,8 @@ import { import type { ParameterAnalysis, PreparedSource, PreparedBatch, PreparedFieldState, ValidationMode, FieldControl, } from '../core/param-pipeline.js'; +import { analyzeFilterSource, prepareFilterSource } from '../core/filter-execution.js'; +import type { FilterSourcePreparation } from '../core/filter-execution.js'; import { isRowReturning } from '../core/sql-split.js'; import { effectiveFilterActive } from '../state.js'; import type { QueryTab, SaveJSON } from '../state.js'; @@ -118,6 +120,16 @@ export interface WorkbenchParameterSession { prepareAnalyzedBatch(analysis: ParameterAnalysis, wallNowMs: number, validationMode?: ValidationMode): PreparedBatch; prepareTabBatch(sql: string, wallNowMs: number, validationMode?: ValidationMode): PreparedBatch; prepareTabSource(sql: string, wallNowMs: number, validationMode?: ValidationMode): PreparedSource; + /** #360 Workbench parity: a Filter tab's own preview, prepared through the + * SAME shared `analyzeFilterSource`/`prepareFilterSource` pipeline the + * Dashboard's `runFilterSource` calls (`core/filter-execution.js`) — + * never a second, independently-maintained parameter-binding path (issue + * #360's explicit rule). Driven by the same live `varValues`/ + * `filterActive` (#165 activation map) every other `prepare*` method here + * reads; no `sourceBackedParams` — the Workbench has no Dashboard + * topology (no other source could be "backing" one of these params), so + * the cascading-Filter-sources rule has nothing to check here. */ + prepareFilterPreview(sql: string, wallNowMs: number): FilterSourcePreparation; execStatementSql(stmt: string): string; varGateBlocked(wallNowMs?: number): boolean; hardenVar(name: string, field?: PreparedFieldState): void; @@ -182,6 +194,24 @@ export function createWorkbenchParameterSession(deps: WorkbenchParameterSessionD return prepareTabBatch(sql, wallNowMs, validationMode).sources[0]; } + // #360 Workbench parity: the Filter tab's OWN preview shares the EXACT same + // analyze/prepare pipeline the Dashboard's runFilterSource calls — never a + // second, independently-maintained parameter-binding path (issue #360's + // explicit rule: "Do not independently implement parameter binding in + // workbench-session.ts and dashboard-viewer-session.ts"). Unlike the + // Dashboard session's own once-per-construction `analyzed` (cached against + // a stable per-source runtime), `analyzeFilterSource` re-scans `sql` fresh + // on every call here — a Workbench tab's Filter SQL can change on every + // keystroke, so there is no stable runtime to cache the analysis against. + // No `sourceBackedParams`: the Workbench has no Dashboard topology (no + // other source could be "backing" one of these params), so the cascading- + // Filter-sources rule has nothing to check. + function prepareFilterPreview(sql: string, wallNowMs: number): FilterSourcePreparation { + return prepareFilterSource(analyzeFilterSource(sql), { + values: deps.varValues(), active: activeMap(), wallNowMs, + }); + } + // The execution text of one statement (#165): only active optional blocks // retained, markers stripped — byte-identical for SQL without blocks. Follows // the #134 bind gate: a non-row-returning statement passes through verbatim. @@ -311,6 +341,7 @@ export function createWorkbenchParameterSession(deps: WorkbenchParameterSessionD prepareAnalyzedBatch, prepareTabBatch, prepareTabSource, + prepareFilterPreview, execStatementSql, varGateBlocked, hardenVar, diff --git a/src/ui/app.ts b/src/ui/app.ts index 03c657e..5e17100 100644 --- a/src/ui/app.ts +++ b/src/ui/app.ts @@ -635,6 +635,7 @@ export function createApp(env: CreateAppEnv = {}): App { recordHistory: (tab, sql) => app.recordHistory(tab, sql), recordBoundParams: (bp) => params.recordBoundParams([...bp]), prepareTabSource: params.prepareTabSource, varGateBlocked: params.varGateBlocked, + prepareFilterPreview: params.prepareFilterPreview, execStatementSql: params.execStatementSql, sessionParamsFor, getSelectionText: () => app.sqlEditor.getSelection().text, tickElapsed, diff --git a/src/ui/filter-preview.ts b/src/ui/filter-preview.ts index e7e9b09..487f6ad 100644 --- a/src/ui/filter-preview.ts +++ b/src/ui/filter-preview.ts @@ -29,11 +29,16 @@ export interface FilterPreviewApp { } /** `tab.filterPreview`'s real shape (state.ts declares it opaque — - * `Record | null`, owned by ui/app.js's run path, which - * writes exactly this discriminated shape once a Filter-role query - * finishes: `normalized` is `readFilterOptions`'s own return contract). */ + * `Record | null`, owned by ui/workbench/workbench- + * session.ts's run() path, which writes exactly this discriminated shape + * once a Filter-role query finishes: `normalized` is `readFilterOptions`'s + * own return contract). `waiting` (#360) is a normal mid-fill state — a + * required `{name:Type}` param the Filter SQL depends on has no value yet — + * set from `FilterSourcePreparation.missing` (core/filter-execution.js) + * BEFORE any request is sent; it is deliberately distinct from `error`. */ type FilterPreviewState = | { status: 'running' } + | { status: 'waiting'; missing: string[] } | { status: 'error'; error?: string } | { status: 'success'; normalized: { helpers: FilterOptionHelper[]; diagnostics: Diagnostic[] } }; @@ -44,6 +49,7 @@ export function renderFilterPreview(app: FilterPreviewApp): HTMLElement { const preview = app.activeTab().filterPreview as FilterPreviewState | null; if (!preview) return message('Run the query to preview Filter options.'); if (preview.status === 'running') return message('Filter preview appears when the query completes.'); + if (preview.status === 'waiting') return message(`Waiting for: ${preview.missing.join(', ')}`, 'is-waiting'); if (preview.status === 'error') return message(preview.error || 'Filter options failed.', 'is-error'); const { helpers, diagnostics } = preview.normalized; const out = h('div', { class: 'filter-preview' }); diff --git a/src/ui/workbench/workbench-session.ts b/src/ui/workbench/workbench-session.ts index 050d9f8..d5b0007 100644 --- a/src/ui/workbench/workbench-session.ts +++ b/src/ui/workbench/workbench-session.ts @@ -30,7 +30,7 @@ import type { PreparedSource, BoundParamSnapshot } from '../../core/param-pipeli import { supportsExplainPretty, detectSqlFormat, isSchemaMutatingSql } from '../../core/format.js'; import { EXPLAIN_VIEWS, parseExplain, detectExplainView, buildExplainQuery } from '../../core/explain.js'; import { effectiveDashboardRole } from '../../core/result-choice.js'; -import { filterExecution } from '../../core/filter-execution.js'; +import type { FilterSourcePreparation } from '../../core/filter-execution.js'; import { readFilterOptions } from '../../core/filter-options.js'; import { isKpiPanel, panelExecution } from '../../core/panel-execution.js'; import { newResult } from '../../core/stream.js'; @@ -100,6 +100,14 @@ export interface WorkbenchHooks { * only mode run()/runScript() ever use) — Phase 4 moves this into a * WorkbenchParameterSession; injected until then. */ prepareTabSource(sql: string, waveMs: number): PreparedSource; + /** #360 Workbench parity: a Filter tab's own preview, prepared through the + * SAME shared analyze/prepare pipeline the Dashboard's runFilterSource + * calls (`WorkbenchParameterSession.prepareFilterPreview` — issue #360's + * explicit "do not independently implement parameter binding" rule). + * run()'s Filter branch is gated ENTIRELY by this preparation's own + * readiness ('runnable' | 'waiting' | 'error') — the generic + * `varGateBlocked` below stays bypassed for Filter tabs. */ + prepareFilterPreview(sql: string, waveMs: number): FilterSourcePreparation; /** True (and already toasted, shell-owned) when the active tab's * {name:Type} variables block execution. */ varGateBlocked(waveMs: number): boolean; @@ -234,17 +242,29 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes const srcSql = opts && opts.sql != null ? opts.sql : tab.sqlDraft; if (!srcSql.trim()) return; const isFilter = effectiveDashboardRole(tab.specParsed) === 'filter'; - const filterRun = isFilter ? filterExecution(srcSql) : null; - if (filterRun?.error) { - const filterErrorResult: QueryResult = newResult('Filter', filterRun.rowLimit); - filterErrorResult.error = filterRun.error; - Object.assign(tab, { result: filterErrorResult }); - tab.filterPreview = { status: 'error', error: filterRun.error }; + const waveMs = deps.wallNow(); // one wall clock for this run wave: gate + args see the same instant + // #360 Workbench parity: a Filter tab's own preview runs through the SAME + // shared analyze/prepare pipeline the Dashboard's runFilterSource calls — + // never a second, independently-maintained parameter-binding path (issue + // #360's explicit rule). `prep`'s own readiness is the ONLY gate Filter + // execution respects below; the generic varGateBlocked stays bypassed for + // Filter tabs (next line) — removing that bypass would toast-and-block a + // missing-param Filter tab before the 'waiting' preview state below is + // ever reached. + const filterPrep = isFilter ? hooks.prepareFilterPreview(srcSql, waveMs) : null; + if (filterPrep && filterPrep.readiness !== 'runnable') { + tab.filterPreview = filterPrep.readiness === 'waiting' + ? { status: 'waiting', missing: filterPrep.missing } + : { status: 'error', error: filterPrep.error ?? undefined }; + if (filterPrep.readiness === 'error') { + const filterErrorResult: QueryResult = newResult(filterPrep.format, filterPrep.rowLimit); + filterErrorResult.error = filterPrep.error; + Object.assign(tab, { result: filterErrorResult }); + } state.resultView.value = 'filter'; hooks.renderResults(); - return; + return; // NO network for either non-runnable state } - const waveMs = deps.wallNow(); // one wall clock for this run wave: gate + args see the same instant if (!isFilter && hooks.varGateBlocked(waveMs)) return; // Filter parameters fail statically above // One prepared source for the whole run wave (#173), captured NOW — // synchronously with the gate check above, BEFORE the auth awaits below @@ -270,7 +290,7 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes // execution view (#165): inactive optional blocks removed, markers // stripped — byte-identical to srcSql for SQL without blocks. History still // records the template (srcSql / tab.sqlDraft). - const execSql = isFilter ? srcSql : hooks.execStatementSql(srcSql); + const execSql = isFilter ? filterPrep!.execSql : hooks.execStatementSql(srcSql); const kpiExecution: KpiExecutionTransport = isFilter ? { format: 'Table', rowLimit: state.resultRowLimit, params: {}, error: null } @@ -297,7 +317,7 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes let fmt: string; let explainView: string | null = null; if (isFilter) { - fmt = filterRun!.format; + fmt = filterPrep!.format; } else if (explainMode) { // View precedence: an explicit tab click wins; otherwise a *typed* EXPLAIN // is honored exactly (canonical match → its rich view, else the verbatim @@ -321,7 +341,7 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes // row limit; EXPLAIN/PIPELINE/ESTIMATE are exempt (small output, and a cap // would truncate a plan oddly). The streaming guard reads it off the result; // runQuery adds the server-side max_result_rows for the Table path. - const rowLimit = isFilter ? filterRun!.rowLimit : explainMode ? 0 : panelIsKpi ? kpiExecution.rowLimit! : state.resultRowLimit; + const rowLimit = isFilter ? filterPrep!.rowLimit : explainMode ? 0 : panelIsKpi ? kpiExecution.rowLimit! : state.resultRowLimit; const t0 = deps.now(); const result: QueryResult = newResult(fmt, rowLimit); Object.assign(tab, { result }); @@ -357,7 +377,7 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes // as param_ so the server substitutes them (only row-returning // statements bind — a CREATE VIEW / DDL source stays verbatim). params: isFilter - ? filterRun!.params + ? filterPrep!.params : { ...hooks.sessionParamsFor(tab, [srcSql]), ...mergedSourceArgs(src), ...kpiExecution.params }, onChunk: () => hooks.renderResults(), }); @@ -414,8 +434,13 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes hooks.recordHistory(tab, opts && opts.sql); // #171: this statement succeeded — record its bound params (exactly // what was actually sent; an omitted-optional-block param never - // reached `src.statements[*].boundParams` in the first place). - hooks.recordBoundParams(src.statements.flatMap((s) => s.boundParams) as BoundParamSnapshot[]); + // reached `src.statements[*].boundParams` in the first place). A + // Filter tab records `filterPrep.boundParams` instead — #360 parity + // with the Dashboard's own runFilterSource, which records the SAME + // shared preparation's boundParams. + hooks.recordBoundParams( + (isFilter ? filterPrep!.boundParams : src.statements.flatMap((s) => s.boundParams)) as BoundParamSnapshot[], + ); if (isSchemaMutatingSql(runSql)) hooks.loadSchema(); // not awaited — fire and forget } } diff --git a/tests/helpers/fake-app.ts b/tests/helpers/fake-app.ts index 8147bb7..68c6eec 100644 --- a/tests/helpers/fake-app.ts +++ b/tests/helpers/fake-app.ts @@ -210,6 +210,10 @@ const paramsDefaults: WorkbenchParameterSession = { prepareAnalyzedBatch: vi.fn(() => ({ fields: {}, sources: [], diagnostics: [] })), prepareTabBatch: vi.fn(() => ({ fields: {}, sources: [], diagnostics: [] })), prepareTabSource: vi.fn(() => ({ id: 'tab', statements: [], missing: [], invalid: [], errors: [], runnable: true })), + prepareFilterPreview: vi.fn(() => ({ + readiness: 'runnable' as const, diagnostics: [], dependsOn: [], missing: [], invalid: [], errors: [], + error: null, execSql: '', params: {}, format: 'Filter' as const, rowLimit: 2, boundParams: [], + })), execStatementSql: vi.fn((stmt: string) => stmt), varGateBlocked: vi.fn(() => false), hardenVar: vi.fn(), diff --git a/tests/unit/filter-preview.test.ts b/tests/unit/filter-preview.test.ts index fed502e..33c9bcf 100644 --- a/tests/unit/filter-preview.test.ts +++ b/tests/unit/filter-preview.test.ts @@ -3,11 +3,15 @@ import { renderFilterPreview } from '../../src/ui/filter-preview.js'; import { makeApp } from '../helpers/fake-app.js'; describe('Filter preview', () => { - it('renders no-result, running, and error states', () => { + it('renders no-result, running, waiting, and error states', () => { const app = makeApp(); expect(renderFilterPreview(app).textContent).toContain('Run the query'); app.activeTab().filterPreview = { status: 'running' }; expect(renderFilterPreview(app).textContent).toContain('when the query completes'); + app.activeTab().filterPreview = { status: 'waiting', missing: ['from', 'to'] }; + const waiting = renderFilterPreview(app); + expect(waiting.textContent).toBe('Waiting for: from, to'); + expect(waiting.classList.contains('is-waiting')).toBe(true); app.activeTab().filterPreview = { status: 'error', error: 'boom' }; expect(renderFilterPreview(app).textContent).toBe('boom'); }); diff --git a/tests/unit/workbench-parameter-session.test.ts b/tests/unit/workbench-parameter-session.test.ts index 16f9a77..01260a1 100644 --- a/tests/unit/workbench-parameter-session.test.ts +++ b/tests/unit/workbench-parameter-session.test.ts @@ -130,6 +130,61 @@ describe('tabAnalysis / prepare*', () => { }); }); +// ── prepareFilterPreview (#360 Workbench parity) ──────────────────────────── +// Same shared analyze/prepare pipeline (core/filter-execution.js) the +// Dashboard's runFilterSource calls — driven by the SAME live varValues/ +// activeMap every other prepare* method above reads. + +describe('prepareFilterPreview', () => { + it('is runnable once every required {name:Type} param has a value, binding it as param_', () => { + const { deps, state } = makeDeps(); + const session = createWorkbenchParameterSession(deps); + state.varValues.year = '2024'; + const prep = session.prepareFilterPreview('SELECT {year:UInt16} AS y', 1700000000000); + expect(prep.readiness).toBe('runnable'); + expect(prep.params.param_year).toBe('2024'); + expect(prep.execSql).toBe('SELECT {year:UInt16} AS y'); + expect(prep.format).toBe('Filter'); + }); + + it('is waiting (not an error) when a required {name:Type} param has no value yet', () => { + const { deps } = makeDeps(); + const session = createWorkbenchParameterSession(deps); + const prep = session.prepareFilterPreview('SELECT {year:UInt16} AS y', 1700000000000); + expect(prep.readiness).toBe('waiting'); + expect(prep.missing).toEqual(['year']); + expect(prep.error).toBeNull(); + }); + + it('is an error for a structurally invalid Filter source (more than one statement)', () => { + const { deps } = makeDeps(); + const session = createWorkbenchParameterSession(deps); + const prep = session.prepareFilterPreview('SELECT 1; SELECT 2;', 1700000000000); + expect(prep.readiness).toBe('error'); + expect(prep.error).toMatch(/exactly one statement/); + }); + + it('is an error for an invalid committed value', () => { + const { deps, state } = makeDeps(); + const session = createWorkbenchParameterSession(deps); + state.varValues.year = '999999'; // out of UInt16 range — invalid, not missing + const prep = session.prepareFilterPreview('SELECT {year:UInt16} AS y', 1700000000000); + expect(prep.readiness).toBe('error'); + expect(prep.invalid).toEqual(['year']); + }); + + it('reads the SAME live varValues/filterActive (#165 activation map) every other prepare* method reads', () => { + const { deps, state } = makeDeps(); + const session = createWorkbenchParameterSession(deps); + const sql = 'SELECT 1 /*[ AND a = {a:String} ]*/'; + state.varValues.a = 'x'; + state.filterActive.a = false; // explicit inactive wins over "has a value" (#165) + const prep = session.prepareFilterPreview(sql, 1700000000000); + expect(prep.execSql).toBe('SELECT 1 '); // the inactive block is dropped + expect(prep.readiness).toBe('runnable'); // the block-confined param was never required + }); +}); + // ── execStatementSql ───────────────────────────────────────────────────────── describe('execStatementSql', () => { diff --git a/tests/unit/workbench-session.test.ts b/tests/unit/workbench-session.test.ts index 90bf62d..f9b66a7 100644 --- a/tests/unit/workbench-session.test.ts +++ b/tests/unit/workbench-session.test.ts @@ -13,6 +13,7 @@ import type { } from '../../src/application/query-execution-service.js'; import type { StreamResult } from '../../src/core/stream.js'; import type { PreparedSource, PreparedStatement, BoundParamSnapshot } from '../../src/core/param-pipeline.js'; +import type { FilterSourcePreparation } from '../../src/core/filter-execution.js'; // ── Small deferred helper (mirrors the pattern query-execution-service.test.ts // uses for scripting async runQuery behaviors, adapted to a single promise a @@ -49,6 +50,19 @@ function boundParam(name: string): BoundParamSnapshot { }; } +// #360 Workbench parity: `hooks.prepareFilterPreview` stands in for the real +// `WorkbenchParameterSession.prepareFilterPreview` (unit-tested against the +// real shared pipeline in workbench-parameter-session.test.ts) — this fake +// only needs to hand back whatever `FilterSourcePreparation` shape a given +// test wants run()'s Filter branch to react to. +function filterPreparation(over: Partial = {}): FilterSourcePreparation { + return { + readiness: 'runnable', diagnostics: [], dependsOn: [], missing: [], invalid: [], errors: [], + error: null, execSql: 'SELECT 1', params: {}, format: 'Filter', rowLimit: 2, boundParams: [], + ...over, + }; +} + // ── Fakes ──────────────────────────────────────────────────────────────────── function makeExec(): { @@ -91,6 +105,7 @@ function makeHooks(over: Partial = {}): WorkbenchHooks { recordHistory: vi.fn(), recordBoundParams: vi.fn(), prepareTabSource: vi.fn(() => preparedSource()), + prepareFilterPreview: vi.fn((sql: string) => filterPreparation({ execSql: sql })), varGateBlocked: vi.fn(() => false), execStatementSql: vi.fn((stmt: string) => stmt), sessionParamsFor: vi.fn(() => ({})), @@ -154,8 +169,15 @@ describe('createWorkbenchSession: run()', () => { expect(h.execFakes.executeRead).not.toHaveBeenCalled(); }); - it('Filter role: a statically invalid Filter SQL sets an error result and never executes', async () => { - const h = makeHarness({ tab: { sqlDraft: 'SELECT 1; SELECT 2;', specParsed: { name: 'f', favorite: false, dashboard: { role: 'filter' } } } }); + it('Filter role: an error-readiness preparation sets an error result and never executes', async () => { + const h = makeHarness({ + tab: { sqlDraft: 'SELECT 1; SELECT 2;', specParsed: { name: 'f', favorite: false, dashboard: { role: 'filter' } } }, + hooks: { + prepareFilterPreview: vi.fn(() => filterPreparation({ + readiness: 'error', error: 'Filter SQL must contain exactly one statement.', + })), + }, + }); const session = createWorkbenchSession(h.deps); await session.run(); expect(h.execFakes.executeRead).not.toHaveBeenCalled(); @@ -165,6 +187,61 @@ describe('createWorkbenchSession: run()', () => { expect(h.hooks.renderResults).toHaveBeenCalled(); }); + it('Filter role: a waiting-readiness preparation blocks execution WITHOUT the generic var-gate firing (#360)', async () => { + const h = makeHarness({ + tab: { sqlDraft: 'SELECT {from:DateTime} AS x', specParsed: { name: 'f', favorite: false, dashboard: { role: 'filter' } } }, + hooks: { + prepareFilterPreview: vi.fn(() => filterPreparation({ readiness: 'waiting', missing: ['from'] })), + varGateBlocked: vi.fn(() => true), // would block a non-Filter tab — proves it's never even consulted here + }, + }); + const session = createWorkbenchSession(h.deps); + await session.run(); + expect(h.execFakes.executeRead).not.toHaveBeenCalled(); + expect(h.tab.filterPreview).toEqual({ status: 'waiting', missing: ['from'] }); + expect(h.hooks.varGateBlocked).not.toHaveBeenCalled(); + expect(h.hooks.renderResults).toHaveBeenCalled(); + }); + + it('Filter role: an error-readiness preparation from an invalid committed value blocks execution (#360)', async () => { + const h = makeHarness({ + tab: { sqlDraft: 'SELECT {year:UInt16} AS x', specParsed: { name: 'f', favorite: false, dashboard: { role: 'filter' } } }, + hooks: { + prepareFilterPreview: vi.fn(() => filterPreparation({ + readiness: 'error', error: 'Invalid value for: year', invalid: ['year'], + })), + }, + }); + const session = createWorkbenchSession(h.deps); + await session.run(); + expect(h.execFakes.executeRead).not.toHaveBeenCalled(); + expect(h.tab.filterPreview).toEqual({ status: 'error', error: 'Invalid value for: year' }); + }); + + it('Filter role: a runnable preparation executes with its own prepared params/execSql and records boundParams (#360)', async () => { + const h = makeHarness({ + tab: { sqlDraft: 'SELECT {from:DateTime} AS x', specParsed: { name: 'f', favorite: false, dashboard: { role: 'filter' } } }, + hooks: { + prepareFilterPreview: vi.fn(() => filterPreparation({ + execSql: 'SELECT {from:DateTime} AS x', + params: { param_from: '2024-01-01 00:00:00' }, + boundParams: [boundParam('from')], + })), + }, + }); + h.execFakes.executeRead.mockImplementation(async (result: StreamResult) => { + Object.assign(result, { columns: [{ name: 'x', type: 'DateTime' }], rows: [['2024-01-01 00:00:00']] }); + return result; + }); + const session = createWorkbenchSession(h.deps); + await session.run(); + const req = h.execFakes.executeRead.mock.calls[0][1] as ExecuteReadRequest; + expect(req.sql).toBe('SELECT {from:DateTime} AS x'); + expect(req.params).toEqual({ param_from: '2024-01-01 00:00:00' }); + expect(h.tab.filterPreview).toMatchObject({ status: 'success' }); + expect(h.hooks.recordBoundParams).toHaveBeenCalledWith([boundParam('from')]); + }); + it('Filter role: a valid Filter SQL runs to a successful preview', async () => { const h = makeHarness({ tab: { sqlDraft: 'SELECT 1 AS x', specParsed: { name: 'f', favorite: false, dashboard: { role: 'filter' } } } }); h.execFakes.executeRead.mockImplementation(async (result: StreamResult) => { @@ -602,6 +679,11 @@ describe('createWorkbenchSession: runEntry()', () => { it('a Filter-role tab always runs (never scripts), even with multiple statements', async () => { const h = makeHarness({ tab: { sqlDraft: 'SELECT 1; SELECT 2;', specParsed: { name: 'f', favorite: false, dashboard: { role: 'filter' } } }, + hooks: { + prepareFilterPreview: vi.fn(() => filterPreparation({ + readiness: 'error', error: 'Filter SQL must contain exactly one statement.', + })), + }, }); const session = createWorkbenchSession(h.deps); await session.runEntry(); From ff60716000f355a0ea0df33e054cb65c669ab883 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 16:13:42 +0000 Subject: [PATCH 06/16] fix(#360): preflight once per commit for the selective source+panel waves (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review found commitAndRerun double-preflighted: when a committed root param feeds a Filter source, both runFilterSourceWave and runAffectedWave called preflight() — so ensureFreshToken ran twice per commit and, on a stale token, onAuthFailed fired twice (double toast/redirect). Now the affected path preflights exactly once in commitAndRerun; runFilterSourceWave (only ever reached from that path) no longer preflights, and runAffectedWave keeps its own guard (it is still reached directly on the no-affected-source fast path, whose timing is unchanged). The regression test now asserts ensureFreshToken and onAuthFailed fire exactly once for one affected commit; added a test that clearFilter on a dependency root re-gates the dependent source to 'waiting' (the retained value is blanked, not stale-executed). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../application/dashboard-viewer-session.ts | 24 +++++---- tests/unit/dashboard-viewer-session.test.ts | 49 +++++++++++++++++-- 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index 6676a07..6bede5d 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -943,12 +943,12 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa * affected) so the caller can fold them into ONE combined affected-panel * wave alongside `changedParams`. */ async function runFilterSourceWave(changedParams: string[]): Promise { - // A commit-triggered rerun issues a REAL `executeRead` for the affected - // source(s) — unlike `runFilterWave` (always reached via `refresh()`, - // which already preflighted once for the whole cycle) this selective wave - // is entered directly from `commitAndRerun`, with no preflight upstream. - // Same guard `runAffectedWave` uses before its own executeReads. - if (!(await preflight())) return []; + // Entered only from `commitAndRerun`'s affected path, which preflights ONCE + // for the whole commit (source wave + affected-panel wave) — avoiding a + // double `ensureFreshToken()`/`onAuthFailed` on a stale token — so this wave + // never preflights itself. (`runAffectedWave` keeps its own `preflighted` + // flag because it is ALSO reached directly on the no-affected-source fast + // path, where nothing has preflighted yet.) const waveMs = deps.wallNow(); const affected = [...filterSources.values()].filter((source) => source.analyzed.dependsOn.some((name) => changedParams.includes(name))); @@ -1030,8 +1030,8 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa } // Re-run only the tiles some active filter parameter feeds into. - async function runAffectedWave(parameters: string[]): Promise { - if (!(await preflight())) return; + async function runAffectedWave(parameters: string[], preflighted = false): Promise { + if (!preflighted && !(await preflight())) return; const affectedIds = new Set(); for (const parameter of parameters) { const field = analysis.fields[parameter]; @@ -1057,12 +1057,18 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // identical to awaiting it (an unaffected wave always resolves to `[]` with // no side effect), just without the extra tick for the overwhelmingly // common case of a Dashboard with no parameterized Filter sources (#360). + // The fast (no-affected-source) path lets `runAffectedWave` self-preflight + // exactly as before (one await, unchanged timing); the affected path + // preflights ONCE here for the whole commit and passes `preflighted: true` + // into both waves — otherwise a stale token would fail `ensureFreshToken()` + // (and fire `onAuthFailed`) twice for one commit. async function commitAndRerun(changed: string[]): Promise { const hasAffectedSource = [...filterSources.values()] .some((source) => source.analyzed.dependsOn.some((name) => changed.includes(name))); if (!hasAffectedSource) { await runAffectedWave(changed); return; } + if (!(await preflight())) return; const flipped = await runFilterSourceWave(changed); - await runAffectedWave([...new Set([...changed, ...flipped])]); + await runAffectedWave([...new Set([...changed, ...flipped])], true); } async function setFilter(filterId: string, value: unknown): Promise { diff --git a/tests/unit/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index 27fc863..7eb2dc0 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -1462,9 +1462,10 @@ describe('parameterized Filter sources (#360)', () => { }); let tokenOk = true; const onAuthFailed = vi.fn(); + const ensureFreshToken = vi.fn(async () => tokenOk); const session = createDashboardViewerSession(makeDeps({ document, exec, onAuthFailed, - connection: { ensureFreshToken: async () => tokenOk }, + connection: { ensureFreshToken }, queries: [ query('qt', 'SELECT {dep1:String} AS n'), query('srcFrom', "SELECT ['a'] AS dep1 FROM t WHERE ts >= {from:String} /* srcFrom */", { dashboard: { role: 'filter' } }), @@ -1474,16 +1475,56 @@ describe('parameterized Filter sources (#360)', () => { expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('waiting'); const base = calls.length; tokenOk = false; // token now stale + ensureFreshToken.mockClear(); + onAuthFailed.mockClear(); // Committing 'from' makes srcFrom affected (it depends on 'from'), so - // commitAndRerun enters runFilterSourceWave — which must preflight BEFORE - // issuing any executeRead, exactly like runAffectedWave/refresh already do. + // commitAndRerun enters its affected path (runFilterSourceWave THEN + // runAffectedWave) — which must preflight exactly ONCE for the whole + // commit (a stale token must not double-fire `ensureFreshToken`/ + // `onAuthFailed` — one wave passing `preflighted: true` into the other), + // and BEFORE issuing any executeRead. await expect(session.setFilter('from-root', 'v1')).resolves.toBeUndefined(); expect(calls.slice(base).some((c) => c.sql.includes('srcFrom'))).toBe(false); - expect(onAuthFailed).toHaveBeenCalled(); + expect(ensureFreshToken).toHaveBeenCalledTimes(1); + expect(onAuthFailed).toHaveBeenCalledTimes(1); // No rerun happened at all — status is untouched. expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('waiting'); }); + it('clearFilter on a dependency root re-gates its dependent source to waiting — the retained (but now inactive) value is blanked, not stale-executed', async () => { + const { exec, calls } = makeExec((sql) => (sql.includes('srcFrom') + ? { columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['a']]] } + : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: true, defaultValue: 'v0' }, + { id: 'f-dep1', parameter: 'dep1', sourceQueryId: 'srcFrom' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT {dep1:String} AS n'), + query('srcFrom', "SELECT ['a'] AS dep1 FROM t WHERE ts >= {from:String} /* srcFrom */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + expect(session.state.value.filters.find((flt) => flt.id === 'f-dep1')!.status).toBe('ready'); + const base = calls.length; + // clearFilter deactivates 'from' but RETAINS its value ('v0', non-empty) — + // committedRootValues() must still blank it for an inactive root, so the + // dependent source correctly re-gates to 'waiting' instead of executing + // against a stale/retained-but-no-longer-committed value. + await session.clearFilter('from-root'); + expect(session.state.value.filters.find((flt) => flt.id === 'from-root')!.value).toBe('v0'); // retained + expect(session.state.value.filters.find((flt) => flt.id === 'from-root')!.active).toBe(false); + expect(calls.slice(base).some((c) => c.sql.includes('srcFrom'))).toBe(false); // no stale execution + const f = session.state.value.filters.find((flt) => flt.id === 'f-dep1')!; + expect(f.status).toBe('waiting'); + expect(f.options).toBeNull(); + }); + it('resolves two DIFFERENT sources\' relative-time dependency against ONE waveMs per wave (no per-source clock read)', async () => { const { exec, calls } = makeExec((sql) => { if (sql.includes('src1')) return { columns: [{ name: 'dep1', type: 'Array(String)' }], rows: [[['a']]] }; From b3f550c1cd72e34c97d4f7bed07f566c94df0149 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 16:18:04 +0000 Subject: [PATCH 07/16] refactor(#360): skip redundant tab analysis for Filter tabs; style the filter affordance (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - workbench-session run(): a Filter tab prepares via the shared filterPrep, so it no longer also runs the generic prepareTabSource over the same SQL (`src` is null for Filter tabs — it was only ever read on the non-Filter path). - styles.css: distinguish the source-backed filter transport states so a disabled field's reason reads at a glance — is-error (red), is-waiting (dashed + "Waiting for: …" note), is-stale (dimmed/italic while reloading); plus the waiting filter-preview message. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/styles.css | 10 ++++++++++ src/ui/workbench/workbench-session.ts | 8 +++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/styles.css b/src/styles.css index 3866a27..38ac12b 100644 --- a/src/styles.css +++ b/src/styles.css @@ -770,6 +770,7 @@ body { .filter-preview-diagnostic.is-warning { color: var(--warn-fg); } .filter-preview-diagnostic.is-error, .filter-preview-message.is-error { color: var(--error-fg); } .filter-preview-message { padding: 24px; color: var(--fg-mute); } +.filter-preview-message.is-waiting { color: var(--warn-fg); } /* Curated Filter field (#160, filter-option-field.js): the strict single-select is the same `.var-combo`/`.var-input`/`.var-combo-list` control every other combobox field uses — it only adds an inline clear (×) button for its "All"/ @@ -1464,6 +1465,15 @@ body.detached-tab .graph-overlay-panel { border-color: var(--error-bd); box-shadow: 0 0 0 3px color-mix(in oklab, var(--error-fg) 22%, transparent); } + +/* #360 source-backed Dashboard filter transport states. The field is also + `disabled`; these mark WHY, so a benign 'waiting' (dashed, with a + "Waiting for: …" note) or a 'stale'/refreshing field (dimmed) reads + differently from a real 'error' (red) at a glance. */ +.var-input.is-error { border-color: var(--error-bd); background: var(--error-bg); } +.var-input.is-waiting { border-style: dashed; } +.var-input.is-stale { opacity: 0.55; font-style: italic; } +.var-field-note { grid-column: 2; font-size: 11px; color: var(--fg-mute); white-space: nowrap; } /* Relative-time preset combobox (#169): the accessible dropdown-on-focus + type-to-filter control for date-like {name:Type} variables (#174 §1) — shared by the workbench var-strip and the Dashboard filter bar diff --git a/src/ui/workbench/workbench-session.ts b/src/ui/workbench/workbench-session.ts index d5b0007..a754c29 100644 --- a/src/ui/workbench/workbench-session.ts +++ b/src/ui/workbench/workbench-session.ts @@ -274,7 +274,9 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes // the server as a never-gate-checked binding. Reused on success for the // recent-value recording (#171), so it reads exactly the boundParams that // were sent. - const src = hooks.prepareTabSource(srcSql, waveMs); + // A Filter tab prepares via `filterPrep` (the shared #360 pipeline) above, so + // it never reads `src` — skip the redundant generic tab analysis for it. + const src = isFilter ? null : hooks.prepareTabSource(srcSql, waveMs); await deps.ensureConfig(); if (!(await deps.getToken())) { hooks.onAuthFailed(); return; } @@ -378,7 +380,7 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes // statements bind — a CREATE VIEW / DDL source stays verbatim). params: isFilter ? filterPrep!.params - : { ...hooks.sessionParamsFor(tab, [srcSql]), ...mergedSourceArgs(src), ...kpiExecution.params }, + : { ...hooks.sessionParamsFor(tab, [srcSql]), ...mergedSourceArgs(src!), ...kpiExecution.params }, onChunk: () => hooks.renderResults(), }); } finally { @@ -439,7 +441,7 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes // with the Dashboard's own runFilterSource, which records the SAME // shared preparation's boundParams. hooks.recordBoundParams( - (isFilter ? filterPrep!.boundParams : src.statements.flatMap((s) => s.boundParams)) as BoundParamSnapshot[], + (isFilter ? filterPrep!.boundParams : src!.statements.flatMap((s) => s.boundParams)) as BoundParamSnapshot[], ); if (isSchemaMutatingSql(runSql)) hooks.loadSchema(); // not awaited — fire and forget } From 7278a5bcb84105ad77119fd8f46facc86e885d19 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 16:18:04 +0000 Subject: [PATCH 08/16] docs(#360): migrate the query-log-explorer example + CHANGELOG (reconcile) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - examples/query-log-explorer.json: the qle-filter source now scopes its user / query_kind option lists to the selected half-open {from:DateTime}/{to:DateTime} window (matching the panels, which already use from/to); from/to default to -1d/now active so the whole example runs out-of-box and demonstrates the waiting→ready transition when the range changes. Re-normalized. - CHANGELOG [Unreleased]: document #360 (Added) and the stray-NUL fix (Fixed). Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 30 ++++++++++++++++++++++++++++++ examples/query-log-explorer.json | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da9a103..361c834 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,37 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] +### Added +- **Parameterized Dashboard Filter sources with single-layer dependencies** + (#360). A Filter-role source query may now declare its own `{name:Type}` + query parameters and bind committed *root* Dashboard filter values through the + existing native `param_` pipeline (no textual SQL interpolation) — so a + query-log option source can compute its user / query-kind / exception-code + lists for the selected `{from:DateTime}` / `{to:DateTime}` window instead of a + hard-coded range. A source **waits** (issues no ClickHouse request) until every + required root parameter is committed, active, valid, and serializable; relative + values (`-1d`, `now`) resolve against one wall clock per wave; changing a root + filter reruns **only** the sources that declare it, then the affected panels, in + one combined wave; a refreshed option set that drops a filter's active value + deactivates it before panels run; and a Filter source that depends on *another* + source-backed parameter is rejected with a visible diagnostic (cascading Filter + sources are unsupported — single-layer only). Dashboard execution and the + Workbench Filter preview share one preparation operation (`prepareFilterSource` + in `core/filter-execution.ts`), so structural diagnostics, relative-value + resolution, optional-block materialization, validation/serialization, and + native arguments are identical on both surfaces. Source-backed filters render a + waiting / stale / error affordance in the filter bar instead of silently + degrading to a plain control; a superseded source response can never publish + stale results, and a dependency change clears the affected options so a stale + set can't appear current while the new source loads. Filter execution injects + no `readonly` setting (kept from #359). `examples/query-log-explorer.json`'s + `qle-filter` source is migrated to the half-open `{from:DateTime}`/`{to:DateTime}` + window as a worked example. + ### Fixed +- Removed a stray NUL byte embedded in `dashboard-viewer-session.ts`'s + `optionsSignature` (introduced by #361), which silently made the file look + binary to plain `grep`/`rg`. - **Dashboard filters sharing one Filter-source query now populate** (#359). When several Dashboard filter definitions referenced the same Filter-role source query, the viewer ran that query once per definition and keyed each diff --git a/examples/query-log-explorer.json b/examples/query-log-explorer.json index 2fe22a2..eab9cb9 100644 --- a/examples/query-log-explorer.json +++ b/examples/query-log-explorer.json @@ -10,7 +10,7 @@ "queries": [ { "id": "qle-filter", - "sql": "SELECT\n arraySort(item -> item.label, groupUniqArray((user AS value, splitByChar('@', user)[1] AS label))) AS user,\n arraySort(groupUniqArray(query_kind)) AS query_kind\nFROM system.query_log WHERE query_log.user != ''\nSETTINGS enable_named_columns_in_function_tuple = 1", + "sql": "SELECT\n arraySort(item -> item.label, groupUniqArray((user AS value, splitByChar('@', user)[1] AS label))) AS user,\n arraySort(groupUniqArray(query_kind)) AS query_kind\nFROM system.query_log\nWHERE query_log.user != ''\n AND event_time >= {from:DateTime}\n AND event_time < {to:DateTime}\nSETTINGS enable_named_columns_in_function_tuple = 1", "specVersion": 1, "spec": { "name": "Filter", From 1c49dc84fee58bb741d44e7b1a7097876ef3b1c3 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 16:20:55 +0000 Subject: [PATCH 09/16] =?UTF-8?q?test(#360):=20e2e=20=E2=80=94=20parameter?= =?UTF-8?q?ized=20Filter=20source=20waits=20then=20runs=20once=20with=20na?= =?UTF-8?q?tive=20params?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the raw filter-source harness with a parameterized source (`geo`) depending on two root filters (from/to). Asserts, in a real browser (chromium + webkit): both deps inactive → status 'waiting', waitingFor [from, to], zero requests; committing from+to → status 'ready', exactly one executeRead, params carry param_from/param_to (no textual interpolation). Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/e2e/filter-source.html | 50 +++++++++++++++++++++++++++++++++ tests/e2e/filter-source.spec.js | 18 ++++++++++++ 2 files changed, 68 insertions(+) diff --git a/tests/e2e/filter-source.html b/tests/e2e/filter-source.html index e55212c..5486e64 100644 --- a/tests/e2e/filter-source.html +++ b/tests/e2e/filter-source.html @@ -115,6 +115,56 @@ mountField('user', '__userSel'); mountField('query_kind', '__kindSel'); + // #360: a PARAMETERIZED Filter source (`geo`) that depends on two root + // filters (`from`/`to`). Both start inactive, so the source WAITS (issues no + // request) until they are committed; activating them reruns only the source + // that declares them, which then executes exactly once with native + // param_ args and settles the `region` filter to ready. + let paramRequests = 0; + window.__paramRequests = 0; + window.__paramLastParams = null; + const paramExec = { + async executeRead(result, req) { + paramRequests += 1; + window.__paramRequests = paramRequests; + window.__paramLastParams = req.params; + result.columns = [{ name: 'region', type: 'Array(String)' }]; + result.rows = [[['US', 'EU']]]; + result.progress.bytes = 10; + result.error = null; + result.cancelled = false; + }, + }; + const paramSession = createDashboardViewerSession({ + document: { + documentVersion: 1, id: 'dp', title: 'DP', revision: 1, + layout: { type: 'flow', version: 1, preset: 'columns-2', items: {} }, + filters: [ + { id: 'f-from', parameter: 'from', label: 'from' }, + { id: 'f-to', parameter: 'to', label: 'to' }, + { id: 'f-region', parameter: 'region', label: 'region', sourceQueryId: 'geo' }, + ], + tiles: [{ id: 'tp', queryId: 'qp' }], + }, + queries: [ + savedQuery('qp', 'SELECT {region:String} AS n'), + savedQuery('geo', + "SELECT ['US','EU'] AS region FROM system.one WHERE event_time >= {from:DateTime} AND event_time < {to:DateTime}", + { dashboard: { role: 'filter' } }), + ], + exec: paramExec, + connection: { ensureFreshToken: async () => true }, + now: () => (clock += 5), + wallNow: () => 2000, + }); + await paramSession.start(); + const paramRegion = () => paramSession.state.value.filters.find((f) => f.parameter === 'region'); + window.__paramRegion = () => ({ status: paramRegion().status, waitingFor: paramRegion().waitingFor || [] }); + window.__paramActivate = async () => { + await paramSession.setFilter('f-from', '-1d'); + await paramSession.setFilter('f-to', 'now'); + }; + window.__ready = true; diff --git a/tests/e2e/filter-source.spec.js b/tests/e2e/filter-source.spec.js index 4e16509..da0e2fd 100644 --- a/tests/e2e/filter-source.spec.js +++ b/tests/e2e/filter-source.spec.js @@ -91,4 +91,22 @@ test.describe('Dashboard Filter sources', () => { await expect(userInput).toHaveValue('alice'); expect(await page.evaluate(() => window.__userSel.value)).toBe('alice'); }); + + test('a parameterized Filter source waits for its root deps, then runs once with native params (#360)', async ({ page }) => { + // Both root filters (from/to) start inactive, so the source is blocked on a + // missing dependency: no request, a non-error waiting state naming the + // missing params. + expect(await page.evaluate(() => window.__paramRegion())).toEqual({ status: 'waiting', waitingFor: ['from', 'to'] }); + expect(await page.evaluate(() => window.__paramRequests)).toBe(0); + + // Committing from + to makes the source runnable — it executes exactly once, + // with native param_from / param_to arguments (no textual interpolation), + // and the region filter settles to ready. + await page.evaluate(() => window.__paramActivate()); + expect(await page.evaluate(() => window.__paramRegion().status)).toBe('ready'); + expect(await page.evaluate(() => window.__paramRequests)).toBe(1); + const params = await page.evaluate(() => window.__paramLastParams); + expect(params).toHaveProperty('param_from'); + expect(params).toHaveProperty('param_to'); + }); }); From 28e994778b2aac517a8d5fa847512be81e26aece Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 16:51:42 +0000 Subject: [PATCH 10/16] fix(#360): Filter source param type-conflicts are errors, not runnable (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit analyzeFilterSource now folds the shared pipeline's own analysis.diagnostics (param type-conflict, e.g. {x:UInt8} vs {x:String}) into its diagnostics as filter-source-param-type-conflict — so prepareFilterSource classifies such a source 'error' (no request) instead of runnable, restoring parity with the generic param pipeline on both Dashboard and Workbench. Also extract filterOwnedParams() so prepareFilterSource no longer calls filterExecution just to recover the transport caps (which re-ran the structural diagnostics). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/filter-execution.ts | 45 ++++++++++++++++++++--------- tests/unit/filter-execution.test.ts | 42 +++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/src/core/filter-execution.ts b/src/core/filter-execution.ts index 0691b7c..db8efbf 100644 --- a/src/core/filter-execution.ts +++ b/src/core/filter-execution.ts @@ -71,20 +71,30 @@ export interface FilterExecutionPlan { error: string | null; } +/** The owned, lossless, bounded transport params every Filter-role query runs + * under — `max_result_bytes` plus the four JSON `output_format` flags that + * make the numeric/decimal round-trip lossless over JSON. Shared by + * `filterExecution` and `prepareFilterSource` so the caps are defined once; + * `overrides` layers on top (a caller-supplied extra/overriding param, same + * as `FilterExecutionDefaults.params`). Pure. */ +function filterOwnedParams(overrides: Record = {}): Record { + return { + max_result_bytes: FILTER_RESULT_BYTE_CAP, + output_format_json_named_tuples_as_objects: 1, + output_format_json_quote_64bit_integers: 1, + output_format_json_quote_decimals: 1, + output_format_json_quote_64bit_floats: 1, + ...overrides, + }; +} + export function filterExecution(sql?: string | null, defaults: FilterExecutionDefaults = {}): FilterExecutionPlan { const diagnostics = filterSqlDiagnostics(sql); return { owned: true, format: 'Filter', rowLimit: FILTER_TOP_LEVEL_ROW_LIMIT, - params: { - max_result_bytes: FILTER_RESULT_BYTE_CAP, - output_format_json_named_tuples_as_objects: 1, - output_format_json_quote_64bit_integers: 1, - output_format_json_quote_decimals: 1, - output_format_json_quote_64bit_floats: 1, - ...(defaults.params || {}), - }, + params: filterOwnedParams(defaults.params), diagnostics, error: diagnostics.length ? diagnostics[0].message : null, }; @@ -121,7 +131,12 @@ export interface FilterSourceAnalysis { * source in the same dashboard; depending on any of them is the one * cascading violation this dashboard model disallows, and each becomes its * own `filter-source-cascading` diagnostic naming `opts.label` and the - * offending parameter. Pure. + * offending parameter. `analysis.diagnostics` (the shared pipeline's own + * cross-declaration type-conflict findings, e.g. `{x:UInt8}` vs `{x:String}` + * in the same source) are folded in too, each as its own + * `filter-source-param-type-conflict` diagnostic (#360 review finding 3) — + * without this, a type-conflicted source would classify `runnable` in + * `prepareFilterSource` and get sent. Pure. */ export function analyzeFilterSource( sql: string | null | undefined, @@ -146,7 +161,9 @@ export function analyzeFilterSource( )); } } - return { sql: text, analysis, dependsOn, diagnostics: [...structural, ...cascading] }; + const typeConflicts = analysis.diagnostics.map((d) => + diagnostic('filter-source-param-type-conflict', d.message)); + return { sql: text, analysis, dependsOn, diagnostics: [...structural, ...cascading, ...typeConflicts] }; } /** The three states `prepareFilterSource` classifies a Filter source into: @@ -178,7 +195,10 @@ export interface FilterSourcePreparation { * 'execute'` — a Filter source runs as soon as it's ready; there's no * separate blur/Enter commit step) and folds the result together with * `analyzed.diagnostics` into one readiness verdict, the owned transport - * `params` (`filterExecution`'s caps ∪ the bound `param_` args), and + * `params` (`filterOwnedParams()`'s caps ∪ the bound `param_` args — + * the same helper `filterExecution` builds its own `params` from, called + * directly here rather than through `filterExecution` so this doesn't re-run + * `filterSqlDiagnostics`, already computed by `analyzeFilterSource`), and * the materialized `execSql` to send. Takes the pre-analyzed source so a * caller re-running this every value edit derives `dependsOn` (and re-scans * the SQL) exactly once per SQL edit, not once per value edit. Pure. @@ -203,8 +223,7 @@ export function prepareFilterSource( : src.missing.length ? 'waiting' : 'runnable'; - const owned = filterExecution(analyzed.sql).params; - const params = { ...owned, ...mergedSourceArgs(src) }; + const params = { ...filterOwnedParams(), ...mergedSourceArgs(src) }; const error = diagnostics.length ? diagnostics[0].message : src.invalid.length diff --git a/tests/unit/filter-execution.test.ts b/tests/unit/filter-execution.test.ts index 8ebd170..3888501 100644 --- a/tests/unit/filter-execution.test.ts +++ b/tests/unit/filter-execution.test.ts @@ -65,6 +65,17 @@ describe('analyzeFilterSource (#360)', () => { const a = analyzeFilterSource('CREATE TABLE t (x Int8) COMMENT {z:String}'); expect(a.dependsOn).toEqual([]); }); + it('#360 review finding 3: folds a same-name conflicting-type declaration into a diagnostic', () => { + const a = analyzeFilterSource('SELECT {x:UInt8} AS a, {x:String} AS b'); + expect(a.analysis.diagnostics).toHaveLength(1); + expect(a.analysis.diagnostics[0]).toMatchObject({ kind: 'type-conflict', name: 'x' }); + expect(a.diagnostics.map((d) => d.code)).toContain('filter-source-param-type-conflict'); + const conflict = a.diagnostics.find((d) => d.code === 'filter-source-param-type-conflict'); + expect(conflict?.message).toBe(a.analysis.diagnostics[0].message); + expect(conflict?.message).toContain('x'); + expect(conflict?.message).toContain('UInt8'); + expect(conflict?.message).toContain('String'); + }); }); describe('prepareFilterSource (#360)', () => { @@ -127,6 +138,37 @@ describe('prepareFilterSource (#360)', () => { expect(p.readiness).toBe('runnable'); expect(p.error).toBeNull(); }); + it('#360 review finding 3: a type-conflicted source classifies error, not runnable — no request would be sent', () => { + const a = analyzeFilterSource('SELECT {x:UInt8} AS a, {x:String} AS b'); + const p = prepareFilterSource(a, { values: { x: '1' } }); + expect(p.readiness).toBe('error'); + expect(p.error).toBe(a.diagnostics[0].message); + expect(p.error).toContain('x'); + }); + it('a non-conflicting parameterized source still classifies runnable/waiting as before', () => { + const runnable = analyzeFilterSource('SELECT {x:UInt8} AS a, {y:String} AS b'); + expect(runnable.diagnostics).toEqual([]); + const pr = prepareFilterSource(runnable, { values: { x: '1', y: 'ok' } }); + expect(pr.readiness).toBe('runnable'); + const waiting = analyzeFilterSource('SELECT {x:UInt8} AS a'); + expect(waiting.diagnostics).toEqual([]); + const pw = prepareFilterSource(waiting, { values: {} }); + expect(pw.readiness).toBe('waiting'); + }); + it('owned-params helper: params still carries the caps ∪ param_ args, no readonly, even bypassing filterExecution', () => { + const a = analyzeFilterSource('SELECT 1 WHERE n = {n:UInt8}'); + const p = prepareFilterSource(a, { values: { n: '42' } }); + expect(p.params).toMatchObject({ + param_n: '42', + max_result_bytes: FILTER_RESULT_BYTE_CAP, + output_format_json_named_tuples_as_objects: 1, + output_format_json_quote_64bit_integers: 1, + output_format_json_quote_decimals: 1, + output_format_json_quote_64bit_floats: 1, + }); + expect(p.params).not.toHaveProperty('readonly'); + expect(p.params).not.toHaveProperty('read_only'); + }); it('resolves two relative params in one statement against a single pinned wallNowMs', () => { const a = analyzeFilterSource('SELECT 1 WHERE a >= {from:DateTime} AND a < {to:DateTime}'); const nowMs = 1751200000000; From bd81cccb6d799c8eebb4eea8d12a0b40945c4479 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 17:11:35 +0000 Subject: [PATCH 11/16] fix(#360): a superseded or destroyed selective commit no longer runs its panel wave (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings 1+2: commitAndRerun ran the affected-panel wave unconditionally after the source wave, so a commit superseded by a newer one (or by destroy()) still issued panel requests and could bind stale source-backed values. Now: - a commit-level generation guards both phases: after the source wave, commitAndRerun bails (no panel wave) if destroyed, if a newer commit bumped the generation, or if the source wave reports 'superseded' — flushing any genuinely-applied source merge via publish() first (a superseded result mutated nothing, so that's a harmless replay); - applyFilterProviders / runFilterSourceWave now return a discriminated { status:'applied', flipped } | { status:'superseded' } (an empty array couldn't distinguish stale from applied-nothing-flipped); - runAffectedWave gets an unconditional destroyed guard (the preflighted:true fast path previously had none). Finding 6: extract executeFilterSourcePlan shared by runFilterWave (keep options, reset diagnostics) and runFilterSourceWave (clear options, keep diagnostics), removing the duplicated clock/plan/supersede/loading/publish/pool/ merge scaffolding. Also publishes explicit ViewerFilterState.sourceId so the filter bar can choose the curated renderer by topology, not transient status (UI fix follows). dashboard-viewer-session.ts: 100% stmts/fns/lines, 94.24% branches. Gate green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../application/dashboard-viewer-session.ts | 222 +++++++++++++----- tests/unit/dashboard-viewer-session.test.ts | 133 +++++++++++ 2 files changed, 297 insertions(+), 58 deletions(-) diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index 6bede5d..e2208fc 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -133,6 +133,14 @@ export interface ViewerFilterState { * names (from `FilterSourcePreparation.missing`) the filter-bar UI/ * diagnostics can name. Absent otherwise. */ waitingFor?: string[]; + /** #360 finding 4: the shared `FilterSourceRuntime.id` this filter is a + * consumer of, set once at construction from the filter DEFINITION's + * `sourceQueryId` — undefined for a plain root filter (no source at all). + * This is TOPOLOGY, not transport state (unlike `status`/`stale`, it never + * changes across a session), so a later UI wave can pick the curated + * renderer for a source-backed filter by construction rather than by + * inferring it from a transient status value. */ + sourceId?: string; } /** The Dashboard's per-render layout view (#291) — a discriminated union over @@ -386,6 +394,11 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // outside `documentRef` — never read/written by any command, never // persisted. A fresh session always starts at 'tiles'. let gridRenderMode: GridRenderMode = 'tiles'; + // #360 findings 1+2: bumped once per `commitAndRerun` call, covering BOTH + // phases of its affected path (the source wave, then the panel wave) — see + // the rationale on `commitAndRerun` itself for why a per-phase generation + // guard alone (each phase already has one) is not enough. + let commitGeneration = 0; const queryById = new Map(); for (const query of queries) { @@ -438,11 +451,11 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa const seed = deps.initialFilters ? deps.initialFilters[def.id] : undefined; const value = seed !== undefined ? (seed.value ?? defaultValue) : defaultValue; const active = seed !== undefined ? !!seed.active : defaultActive; + const sourceId = typeof def.sourceQueryId === 'string' ? def.sourceQueryId : undefined; const state: ViewerFilterState = { id: def.id, parameter: def.parameter, label: def.label || def.parameter, - active, value, status: 'idle', options: null, optionsRev: 0, + active, value, status: 'idle', options: null, optionsRev: 0, sourceId, }; - const sourceId = typeof def.sourceQueryId === 'string' ? def.sourceQueryId : undefined; return { def, sourceId, state }; }); const filterById = new Map(filters.map((filter) => [filter.def.id, filter])); @@ -804,6 +817,19 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa return provider; } + /** #360 findings 1+2: the terminal result of one source-wave pass — either + * every source in the plan settled at ITS OWN reserved generation and the + * merge ran (`'applied'`, carrying `merged.changed` as `flipped`), or the + * plan was stale (superseded by a later wave that reserved a fresh + * generation on one of the SAME sources) and nothing was published + * (`'superseded'`). Before this type existed both cases returned `[]`, + * which made "ran, nothing flipped" indistinguishable from "this whole + * wave was discarded" — so a superseded selective source wave still went + * on to launch its own panel wave in `commitAndRerun`. `runFilterWave` + * ignores the return either way (a full refresh has no further phase to + * gate). */ + type SourceWaveResult = { status: 'applied'; flipped: string[] } | { status: 'superseded' }; + /** Terminal, SYNCHRONOUS step of the filter wave (#359): merges every * collected provider exactly ONCE, then for every consumer of every * source derives its `ViewerFilterStatus`, replaces its curated options @@ -811,10 +837,12 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa * reconciles `merged.changed` — all before this function returns, so the * affected-tile wave `refresh()` runs next sees the reconciled `active` * (the PRECONDITION from plan review: this must NOT be deferred to a - * microtask). Returns the deactivated (flipped) parameter names — - * `merged.changed` — so a selective (#360) caller can fold them into the - * SAME affected-panel wave as the parameter that triggered the rerun. */ - function applyFilterProviders(plan: { source: FilterSourceRuntime; generation: number }[]): string[] { + * microtask). Returns a `SourceWaveResult`: `{status:'applied', flipped}` + * with the deactivated (flipped) parameter names — `merged.changed` — so a + * selective (#360) caller can fold them into the SAME affected-panel wave + * as the parameter that triggered the rerun; or `{status:'superseded'}` + * when this plan was stale (see the stale-wave guard immediately below). */ + function applyFilterProviders(plan: { source: FilterSourceRuntime; generation: number }[]): SourceWaveResult { // Stale-wave guard (#359 "a stale generation cannot publish options or // activation changes"): a superseded (or destroyed) wave's own source // results were already discarded in `runFilterSource` (returned `null`), @@ -824,7 +852,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // already-correct state. Source generations move all-at-once (every // `runFilterWave`/`destroy` bumps them together), so any mismatch means // this whole wave is stale and must publish nothing. - if (plan.some(({ source, generation }) => source.gen !== generation)) return []; + if (plan.some(({ source, generation }) => source.gen !== generation)) return { status: 'superseded' }; // Merge the COMPLETE set of source providers — not just the ones a wave // re-ran — so retained (last-known) providers survive a future selective // wave (#360). A source that has never produced one contributes nothing. @@ -905,44 +933,87 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa for (const parameter of merged.changed) { for (const filter of filters) if (filter.def.parameter === parameter) filter.state.active = false; } - return merged.changed; + return { status: 'applied', flipped: merged.changed }; } - async function runFilterWave(): Promise { - filterDiagnostics = []; - // One wall-clock reading for the WHOLE wave (mirrors the tile wave's own - // `deps.wallNow()` capture in `refresh()`) — every source in this wave's - // plan resolves any relative `{name:DateTime}` dependency against the - // exact same instant, never a per-source read. - const waveMs = deps.wallNow(); - const sources = [...filterSources.values()]; + /** Finding 6 (dedup): the executor shared by `runFilterWave` (every known + * source, a full refresh) and `runFilterSourceWave` (only the sources a + * just-committed root parameter affects, #360) — both used to duplicate + * the same five steps: supersede each source into a plan, flip loading/ + * stale on the source AND every consumer, decide whether to clear + * consumer options, `publish()` once, run the plan through the bounded + * pool, then merge terminally via `applyFilterProviders`. The two callers + * differ only in `opts`: + * - `clearOptions` — a FULL refresh deliberately KEEPS the current options + * through the loading window: clearing them would flip a non-empty + * filter bar to empty on every refresh, churn the UI's rebuild + * signature, and destroy in-progress typing (the #359 no-flicker + * invariant). A SELECTIVE rerun clears them via `setConsumerOptions` + * (so `optionsRev` bumps too) because a committed dependency change + * means the OLD options are no longer known-current for the new inputs + * and must not keep rendering as if they were — `applyFilterProviders` + * repopulates them from the fresh merge once this wave settles, so the + * clear is transient. + * - `resetDiagnostics` — a FULL refresh blanks `filterDiagnostics` + * immediately: every source is being re-run, so any previously-published + * diagnostic could be stale for the whole loading window. A SELECTIVE + * rerun leaves the prior wave's diagnostics in place through its own + * loading window (only a few sources are affected; the untouched + * sources' diagnostics are still valid) — either way, + * `applyFilterProviders` unconditionally overwrites `filterDiagnostics` + * from the COMPLETE merge once THIS plan settles. + * `waveMs` is the one wall-clock reading the whole wave shares (captured + * once by the caller before building this plan, mirroring the tile wave's + * own `deps.wallNow()` capture in `refresh()`) — every source in the plan + * resolves any relative `{name:DateTime}` dependency against the exact + * same instant, never a per-source read. */ + async function executeFilterSourcePlan( + sources: FilterSourceRuntime[], + opts: { clearOptions: boolean; resetDiagnostics: boolean; waveMs: number }, + ): Promise { + if (opts.resetDiagnostics) filterDiagnostics = []; const plan = sources.map((source) => ({ source, generation: supersede(source) })); for (const { source } of plan) { + // Loading is set here (and only here) — NEVER in the terminal step. source.status = 'loading'; - // Loading is set here (and only here) — NEVER in the terminal step — - // and WITHOUT touching `options`/`optionsRev`: clearing options mid-wave - // would flip a non-empty filter bar to empty on every refresh, churn - // the UI's rebuild signature, and destroy in-progress typing. - for (const consumer of source.consumers) { consumer.state.status = 'loading'; consumer.state.stale = true; } + for (const consumer of source.consumers) { + consumer.state.status = 'loading'; + consumer.state.stale = true; + if (opts.clearOptions) setConsumerOptions(consumer, null); + } } publish(); // Each source stores its own provider on `source.provider`; the terminal // merge reads the complete set, so the pool's returns are awaited only for // sequencing, not collected. await runPool(plan, VIEWER_TILE_CONCURRENCY, - ({ source, generation }) => runFilterSource(source, generation, waveMs)); - applyFilterProviders(plan); + ({ source, generation }) => runFilterSource(source, generation, opts.waveMs)); + return applyFilterProviders(plan); + } + + async function runFilterWave(): Promise { + // One wall-clock reading for the WHOLE wave (mirrors the tile wave's own + // `deps.wallNow()` capture in `refresh()`). + const waveMs = deps.wallNow(); + // Full refresh: every known source, keep options through loading (#359 + // no-flicker invariant), reset diagnostics immediately (see + // `executeFilterSourcePlan`'s doc comment for the "why"). The returned + // `SourceWaveResult` is ignored — a full refresh has no further phase to + // gate on it. + await executeFilterSourcePlan([...filterSources.values()], + { clearOptions: false, resetDiagnostics: true, waveMs }); } /** #360: rerun only the shared Filter sources whose OWN `{name:Type}` * declarations depend on one of `changedParams` (a root filter's just- * committed parameter names) — a source with no such dependency is never - * superseded/rerun. One wall-clock reading for this whole selective wave, - * same discipline as `runFilterWave`. Returns the deactivated (flipped) - * parameter names from `applyFilterProviders` (`[]` when no source was - * affected) so the caller can fold them into ONE combined affected-panel - * wave alongside `changedParams`. */ - async function runFilterSourceWave(changedParams: string[]): Promise { + * superseded/rerun. Returns `{status:'applied', flipped:[]}` when no + * source is affected (nothing ran, so nothing can be stale); otherwise + * delegates to `executeFilterSourcePlan`, whose `SourceWaveResult` the + * caller (`commitAndRerun`) uses to fold the flipped parameter names into + * ONE combined affected-panel wave alongside `changedParams`, or to skip + * that wave entirely when this plan was superseded. */ + async function runFilterSourceWave(changedParams: string[]): Promise { // Entered only from `commitAndRerun`'s affected path, which preflights ONCE // for the whole commit (source wave + affected-panel wave) — avoiding a // double `ensureFreshToken()`/`onAuthFailed` on a stale token — so this wave @@ -952,29 +1023,10 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa const waveMs = deps.wallNow(); const affected = [...filterSources.values()].filter((source) => source.analyzed.dependsOn.some((name) => changedParams.includes(name))); - if (affected.length === 0) return []; - const plan = affected.map((source) => ({ source, generation: supersede(source) })); - for (const source of affected) { - source.status = 'loading'; - for (const consumer of source.consumers) { - consumer.state.status = 'loading'; - consumer.state.stale = true; - // Selective-path ONLY (never `runFilterWave`'s full-refresh loop, - // which deliberately keeps options to preserve #359's no-flicker- - // on-same-content invariant): a committed dependency change means the - // OLD options are no longer known-current for the new inputs, so they - // must not keep rendering as if they were — clear them (via the - // shared helper, so `optionsRev` bumps) rather than leave a stale - // "current" value on screen through the loading window. - // `applyFilterProviders` repopulates from the fresh merge once this - // wave settles, so the clear is transient. - setConsumerOptions(consumer, null); - } - } - publish(); - await runPool(plan, VIEWER_TILE_CONCURRENCY, - ({ source, generation }) => runFilterSource(source, generation, waveMs)); - return applyFilterProviders(plan); + if (affected.length === 0) return { status: 'applied', flipped: [] }; + // Selective rerun: clear stale-for-new-inputs options, but keep the prior + // wave's diagnostics until THIS wave's own merge settles. + return executeFilterSourcePlan(affected, { clearOptions: true, resetDiagnostics: false, waveMs }); } @@ -1031,6 +1083,13 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // Re-run only the tiles some active filter parameter feeds into. async function runAffectedWave(parameters: string[], preflighted = false): Promise { + // Finding 2: unconditional destroyed guard. The `preflighted: true` fast + // path (from `commitAndRerun`'s affected branch) skips `preflight()` + // entirely below — and `preflight()` was the ONLY place on this path that + // checked `destroyed` — so without this a `destroy()` firing between the + // source wave settling and this wave starting could still reserve tile + // generations and issue requests after teardown. + if (destroyed) return; if (!preflighted && !(await preflight())) return; const affectedIds = new Set(); for (const parameter of parameters) { @@ -1050,25 +1109,72 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // in-progress typing), rerun any shared Filter source that depends on the // just-committed root parameter(s) FIRST, then fold both the committed // parameter(s) AND any names `runFilterSourceWave` itself deactivated - // (`merged.changed`, e.g. a now-out-of-range curated value elsewhere) into + // (`result.flipped`, e.g. a now-out-of-range curated value elsewhere) into // ONE combined affected-panel wave — never two separate waves. A synchronous // pre-check skips `runFilterSourceWave` (and the microtask hop awaiting it // costs) entirely when nothing depends on `changed` — behaviorally - // identical to awaiting it (an unaffected wave always resolves to `[]` with - // no side effect), just without the extra tick for the overwhelmingly - // common case of a Dashboard with no parameterized Filter sources (#360). + // identical to awaiting it (an unaffected wave always resolves to + // `{status:'applied', flipped:[]}` with no side effect), just without the + // extra tick for the overwhelmingly common case of a Dashboard with no + // parameterized Filter sources (#360). // The fast (no-affected-source) path lets `runAffectedWave` self-preflight // exactly as before (one await, unchanged timing); the affected path // preflights ONCE here for the whole commit and passes `preflighted: true` // into both waves — otherwise a stale token would fail `ensureFreshToken()` // (and fire `onAuthFailed`) twice for one commit. + // + // Findings 1+2 (maintainer review): a commit-level `commitGeneration` + // guards BOTH phases of the affected path together — not just each phase's + // own internal generation checks — because two failure modes were + // otherwise indistinguishable from "ran, nothing to do": + // - Superseded (finding 1): an overlapping LATER commit (B, a different + // `commitAndRerun` call) can supersede THIS commit's (A) source wave + // mid-flight by reserving a fresh generation on one of the same sources. + // `runFilterSourceWave`/`applyFilterProviders` correctly discard A's + // stale results (`{status:'superseded'}`), but without a check here A + // would still go on to launch its OWN panel wave — issuing avoidable + // requests and binding source-backed filter values that have already + // moved past while B is still in flight. Comparing `generation` (this + // call's own snapshot) against `commitGeneration` (bumped by every NEW + // `commitAndRerun` call) after the source wave catches a commit-level + // supersession even in cases the plan-level `SourceWaveResult` alone + // would not (e.g. a concurrent full `refresh()`, which bumps every + // source's generation but never touches `commitGeneration`) — so both + // checks are kept, each catching a case the other does not. + // - Destroyed (finding 2): `runAffectedWave(parameters, true)` passes + // `preflighted: true`, which skips its own `preflight()` call — and + // `preflight()` was the ONLY place that checked `destroyed` on this + // path. A `destroy()` firing while the source wave is in flight would + // otherwise let the panel wave still reserve tile generations and issue + // requests after teardown. The explicit `destroyed` check here, plus the + // new unconditional guard at the top of `runAffectedWave` itself + // (belt-and-braces — `runAffectedWave` is also reachable directly from + // the fast path), closes both entry points. async function commitAndRerun(changed: string[]): Promise { + const generation = ++commitGeneration; const hasAffectedSource = [...filterSources.values()] .some((source) => source.analyzed.dependsOn.some((name) => changed.includes(name))); if (!hasAffectedSource) { await runAffectedWave(changed); return; } if (!(await preflight())) return; - const flipped = await runFilterSourceWave(changed); - await runAffectedWave([...new Set([...changed, ...flipped])], true); + const result = await runFilterSourceWave(changed); + if (destroyed || generation !== commitGeneration || result.status === 'superseded') { + // The source wave's own `applyFilterProviders` merge (options/status + // updates on `consumer.state`) already happened synchronously inside + // `runFilterSourceWave` — but unlike the normal path below, we are + // skipping `runAffectedWave` entirely, and `runAffectedWave` is the + // step that normally `publish()`es afterward. Without this publish, a + // commit that lost the commit-level race (`generation !== + // commitGeneration`, e.g. an UNRELATED overlapping commit) would still + // have applied a real, non-superseded merge that never reaches + // `state.value` — the mutation sits in the runtime record until some + // LATER, unrelated publish happens to flush it. A genuinely + // `'superseded'` result changed nothing, so this is a harmless replay + // of the current state in that case. Skipped entirely once `destroyed` + // — no further publish is attributable to a torn-down session. + if (!destroyed) publish(); + return; + } + await runAffectedWave([...new Set([...changed, ...result.flipped])], true); } async function setFilter(filterId: string, value: unknown): Promise { diff --git a/tests/unit/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index 7eb2dc0..7a36dcc 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -1566,3 +1566,136 @@ describe('parameterized Filter sources (#360)', () => { // clears `options` to `null` via `setConsumerOptions`, and the `ready` // transitions set it back to a real array). }); + +// Maintainer review of #360 (findings 1, 2, 4, 6): a commit's affected path +// (source wave THEN panel wave) must not launch its own panel wave once its +// source wave turns out to have been superseded or the session was +// destroyed — previously `runFilterSourceWave`/`applyFilterProviders` +// returned a bare `[]` for BOTH "applied, nothing flipped" and "discarded, +// stale plan", so a stale commit still ran `runAffectedWave` afterward. +describe('commit-level generation guard for a superseded/destroyed selective wave (#360 review findings 1/2)', () => { + // A root filter ('from') feeds a shared Filter source ('src') that a + // second, source-backed filter ('f-dep') consumes; a tile binds 'from' + // directly so a fired panel wave for it is unambiguous and distinguishable + // by COUNT (both a buggy stale wave and the correct settling wave would + // bind the SAME, already-current 'from' value by the time either runs, so + // only call-count — not the bound param — can tell them apart). + const depDoc = () => doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'from-root', parameter: 'from', defaultActive: true, defaultValue: 'v0' }, + { id: 'f-dep', parameter: 'dep', sourceQueryId: 'src', defaultActive: false, defaultValue: '' }, + ], + }); + const depQueries = () => [ + query('qt', 'SELECT {from:String} AS n'), + query('src', "SELECT ['x'] AS dep FROM t WHERE ts >= {from:String} /* source */", { dashboard: { role: 'filter' } }), + ]; + const tileCallCount = (calls: { sql: string }[]) => calls.filter((c) => !c.sql.includes('source')).length; + + it('finding 1: a commit superseded by a newer overlapping commit issues no panel requests of its own — only the fresher commit runs panels', async () => { + let releaseA!: () => void; + const gateA = new Promise((resolve) => { releaseA = resolve; }); + let sourceCalls = 0; + const { exec, calls } = makeExec((sql) => { + if (!sql.includes('source')) return { columns: [{ name: 'n' }], rows: [[1]] }; + sourceCalls += 1; + return sourceCalls === 2 + ? gateA.then(() => ({ columns: [{ name: 'dep', type: 'Array(String)' }], rows: [[['stale']]] })) + : { columns: [{ name: 'dep', type: 'Array(String)' }], rows: [[['fresh']]] }; + }); + const session = createDashboardViewerSession(makeDeps({ document: depDoc(), exec, queries: depQueries() })); + await session.start(); + const before = tileCallCount(calls); + + const waveA = session.setFilter('from-root', 'v1'); // source wave A starts; its executeRead is call #2, gated + await flush(); + const waveB = session.setFilter('from-root', 'v2'); // supersedes A's source generation before A settles + await waveB; // B's own source wave (call #3) resolves immediately and runs B's panel wave + releaseA(); + await waveA; // A's held call finally resolves, but discovers its plan is stale + + // Only ONE panel request fired for the tile — B's. A's superseded commit + // never launched its own `runAffectedWave` (the pre-fix bug: it would + // have, since a stale `[]` was indistinguishable from an applied `[]`). + expect(tileCallCount(calls) - before).toBe(1); + expect(sourceCalls).toBe(3); + }); + + it('finding 1 (isolated): a source wave superseded by an UNRELATED concurrent full refresh (not another commit) still skips the panel wave, even though commitGeneration itself never changed', async () => { + let releaseGate!: () => void; + const gate = new Promise((resolve) => { releaseGate = resolve; }); + let sourceCalls = 0; + const { exec, calls } = makeExec((sql) => { + if (!sql.includes('source')) return { columns: [{ name: 'n' }], rows: [[1]] }; + sourceCalls += 1; + return sourceCalls === 2 + ? gate.then(() => ({ columns: [{ name: 'dep', type: 'Array(String)' }], rows: [[['stale']]] })) + : { columns: [{ name: 'dep', type: 'Array(String)' }], rows: [[['fresh']]] }; + }); + const session = createDashboardViewerSession(makeDeps({ document: depDoc(), exec, queries: depQueries() })); + await session.start(); + const before = tileCallCount(calls); + + const commit = session.setFilter('from-root', 'v1'); // selective source wave starts; gated (call #2) + await flush(); + const refreshP = session.refresh(); // a full refresh — bumps EVERY source's generation, never commitGeneration + await refreshP; + releaseGate(); + await commit; + + // `refresh()` legitimately fires the tile once (it is not source-targeted, + // so it runs unaffected/parallel); the selective commit must NOT have + // fired an additional, now-stale panel request of its own even though + // `commitGeneration` was never touched by `refresh()` — only the + // `SourceWaveResult` status caught this one. + expect(tileCallCount(calls) - before).toBe(1); + }); + + it('finding 2: destroy() during an in-flight selective source wave prevents its panel wave from ever firing', async () => { + let releaseGate!: () => void; + const gate = new Promise((resolve) => { releaseGate = resolve; }); + let sourceCalls = 0; + const { exec, calls } = makeExec((sql) => { + if (!sql.includes('source')) return { columns: [{ name: 'n' }], rows: [[1]] }; + sourceCalls += 1; + return sourceCalls === 2 + ? gate.then(() => ({ columns: [{ name: 'dep', type: 'Array(String)' }], rows: [[['x']]] })) + : { columns: [{ name: 'dep', type: 'Array(String)' }], rows: [[['x']]] }; + }); + const session = createDashboardViewerSession(makeDeps({ document: depDoc(), exec, queries: depQueries() })); + await session.start(); + const before = tileCallCount(calls); + + const wave = session.setFilter('from-root', 'v1'); // selective source wave starts; gated + await flush(); + session.destroy(); + releaseGate(); + await wave; + + expect(tileCallCount(calls)).toBe(before); // no panel request fired after destroy + }); +}); + +describe('published sourceId on ViewerFilterState (#360 review finding 4)', () => { + it('a source-backed filter publishes sourceId equal to its sourceQueryId; a plain filter leaves it undefined', () => { + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'f-plain', parameter: 'plain', defaultActive: false, defaultValue: '' }, + { id: 'f-src', parameter: 'srcp', sourceQueryId: 'src' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, + queries: [ + query('qt', 'SELECT 1 AS n'), + query('src', "SELECT ['x'] AS srcp /* source */", { dashboard: { role: 'filter' } }), + ], + })); + const plain = session.state.value.filters.find((f) => f.id === 'f-plain')!; + const sourced = session.state.value.filters.find((f) => f.id === 'f-src')!; + expect(plain.sourceId).toBeUndefined(); + expect(sourced.sourceId).toBe('src'); + }); +}); From 4b01df186cc2b8a0248f869985b88ff016b4f847 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 17:31:06 +0000 Subject: [PATCH 12/16] fix(#360): render source-backed filters by topology, update status imperatively (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer review: a source-backed filter's curated identity was inferred from transient status (`f.status !== 'idle'`), so it rendered as a plain text control on initial load, and the loading/stale affordance never showed on a full refresh with existing options (barSig excludes 'loading'). Status is execution state, not topology. - rebuildFilterBar now gates the curated renderer on the explicit `ViewerFilterState.sourceId` (set once at construction) — a source-backed filter is curated at every status, including its initial 'idle'. - buildFilterBar returns `updateStatus(states)`; each curated field keeps a handle and a shared applyFieldStatus() maps status → affordance (ready=enabled; waiting=disabled+is-waiting+note; error states=disabled+ is-error; loading/idle/stale=disabled+is-stale). Applied at build time and on status-only changes WITHOUT rebuilding, so the same input persists and in-progress typing survives. - The render effect splits its signature: barSig is now purely structural ([id, active, value, optionsRev, sourceId!=null]); a separate statusSig drives the imperative updateStatus. Preserves #359's no-flicker-on-same-content invariant while actually showing loading/stale/waiting. filter-bar.ts 100/100/100 (97.53 branches); dashboard.ts floor held. Gate green. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ui/dashboard.ts | 86 +++++++++------ src/ui/filter-bar.ts | 192 ++++++++++++++++++++++++---------- tests/unit/dashboard.test.ts | 110 +++++++++++++++++-- tests/unit/filter-bar.test.ts | 105 ++++++++++++++++++- 4 files changed, 394 insertions(+), 99 deletions(-) diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index 26e50d8..2f53bfd 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -52,7 +52,7 @@ import type { ValidationMode } from '../core/param-pipeline.js'; import { queryDashboardRole } from '../dashboard/model/workspace-semantics.js'; import { renderKpiCards, KPI_STREAM_ARIA } from './kpi-panel.js'; import { buildFilterBar } from './filter-bar.js'; -import type { FilterBarApp } from './filter-bar.js'; +import type { FilterBarApp, FilterBarHandle } from './filter-bar.js'; import { createDashboardViewerSession } from '../dashboard/application/dashboard-viewer-session.js'; import type { DashboardViewerSession, DashboardViewState, ViewerTileState, ViewerFilterState, @@ -591,18 +591,23 @@ export async function renderDashboard(app: DashboardApp): Promise { wallNow: () => app.wallNow(), }; let filterBarDispose: (() => void) | null = null; + // #360 maintainer-review follow-up: the retained bar's `updateStatus` — a + // status-only publish (below, the `barSig`/status-signal split) calls this + // directly instead of tearing down and rebuilding the whole bar. + let filterBarUpdateStatus: FilterBarHandle['updateStatus'] | null = null; function rebuildFilterBar(sview: DashboardViewState): void { filterBarDispose?.(); const idByParam = new Map(); - // #360 (plan-review BLOCKER-2): a source-backed filter used to fall OUT of - // the curated rendering path the moment it had no options — waiting on a - // root dependency, mid-flight, or errored — silently rendering as a bare, - // unlabelled plain field with zero indication anything was pending. A - // plain (non-source-backed) filter is documented to stay 'idle' forever - // (ViewerFilterStatus), so gating on "not idle" keeps every source-backed - // filter curated regardless of whether it currently has options, and - // leaves the plain-filter path completely untouched. + // #360 maintainer-review follow-up: curation is gated on TOPOLOGY + // (`sourceId != null`, set once at construction from the filter + // definition's `sourceQueryId`), never on the transient `status` — status + // is execution state, not topology. The old `f.status !== 'idle'` gate + // meant a source-backed filter rendered as a bare, enabled plain-text + // control on INITIAL load (status starts 'idle' before its source has + // even run) until the source settled — the maintainer-review finding this + // fixes. A plain (non-source-backed) filter has no `sourceId` and is + // never gated into this path. const curatedFields: Record; status: ViewerFilterState['status']; @@ -613,7 +618,7 @@ export async function renderDashboard(app: DashboardApp): Promise { draftValues[f.parameter] = valueString(f.value); draftActive[f.parameter] = f.active; idByParam.set(f.parameter, f.id); - if (f.status !== 'idle') { + if (f.sourceId != null) { curatedFields[f.parameter] = { options: f.options ?? [], status: f.status, stale: f.stale, waitingFor: f.waitingFor }; } } @@ -625,6 +630,7 @@ export async function renderDashboard(app: DashboardApp): Promise { const bar = buildFilterBar(filterBarApp, session.controls, onCommit, getField, { curatedFields, document: doc }); filterHost.replaceChildren(bar.el); filterBarDispose = bar.dispose; + filterBarUpdateStatus = bar.updateStatus; } const filterDiagnosticsHost = h('div', { class: 'dash-filter-diagnostics' }); @@ -1549,6 +1555,16 @@ export async function renderDashboard(app: DashboardApp): Promise { // silently skipping that cleanup. let lastEngineRendered: 'flow' | 'grafana-grid' | null = null; let barSig = ''; + // #360 maintainer-review follow-up: a SEPARATE signature from `barSig` — + // status/stale/waitingFor no longer participate in `barSig` at all (see the + // effect below), so a status-only change is detected here instead and + // applied to the EXISTING bar via `filterBarUpdateStatus` (no rebuild). + let statusSig = ''; + const statusSigOf = (filters: readonly ViewerFilterState[]): string => + JSON.stringify(filters.map((f) => [f.parameter, f.status, !!f.stale, f.waitingFor ?? null])); + const statesByParam = (filters: readonly ViewerFilterState[]): Record => Object.fromEntries(filters.map((f) => [f.parameter, { status: f.status, stale: f.stale, waitingFor: f.waitingFor }])); // #303: the committed-filter bag for a published view, built exactly the way // the persist step below and the seed just under it both need it. const persistBagOf = (filters: readonly ViewerFilterState[]): DashboardFilterBag => { @@ -1578,26 +1594,36 @@ export async function renderDashboard(app: DashboardApp): Promise { return; } lastMobile = mobileNow; - // Rebuild the shared filter bar only when its field structure changes - // (activation, committed value, curated options arriving, or — #360 — a - // source-backed filter settling into `waiting` or an error status) — not - // on tile progress ticks — so in-progress typing is not disturbed - // mid-wave. Without a status term here, an idle→waiting transition - // (`options`/`optionsRev` unchanged throughout — a blocked source never - // ran) left the SAME signature and never rebuilt, so the waiting - // affordance silently never painted (plan-review BLOCKER-2). A bare - // `'loading'` blip is deliberately EXCLUDED from this term: it always - // pairs with `stale: true` but leaves `options`/`optionsRev` untouched - // (`runFilterWave` never clears them mid-wave), so a refresh that settles - // right back to the exact same curated content still must NOT rebuild — - // preserving #359's own invariant (verified by its own suite) that an - // unchanged republish never disturbs in-progress typing. - const sig = JSON.stringify(sview.filters.map((f) => { - const stickyStatus = f.status === 'waiting' || f.status === 'source-error' - || f.status === 'helper-error' || f.status === 'missing-helper' ? [f.status, f.waitingFor] : null; - return [f.id, f.active, valueString(f.value), !!(f.options && f.options.length), f.optionsRev, stickyStatus]; - })); - if (sig !== barSig) { barSig = sig; rebuildFilterBar(sview); } + // Rebuild the shared filter bar only on a STRUCTURAL change (activation, + // committed value, curated option CONTENT arriving/changing via + // `optionsRev`, or a filter gaining/losing its source topology) — not on + // a bare status flip and not on tile progress ticks — so in-progress + // typing is never disturbed mid-wave. #360 maintainer-review follow-up: + // `status`/`stale`/`waitingFor` are deliberately EXCLUDED from this + // signature (they used to be, for a "sticky" subset of statuses, to force + // the `waiting`/error affordance to paint — but that patched over the + // real bug: curation itself gated on status, so an idle-or-loading + // source-backed filter fell out of the curated path entirely). Now that + // `rebuildFilterBar` gates curation on topology (`sourceId != null`) + // rather than status, a pure status change only needs its OWN existing + // curated DOM updated in place — see `statusSig` below — never a rebuild. + // This also preserves #359's own invariant (verified by its own suite) + // that an unchanged republish never disturbs in-progress typing. + const sig = JSON.stringify(sview.filters.map((f) => + [f.id, f.active, valueString(f.value), f.optionsRev, f.sourceId != null])); + const newStatusSig = statusSigOf(sview.filters); + if (sig !== barSig) { + barSig = sig; + rebuildFilterBar(sview); + // A fresh rebuild already applies the CURRENT status to every curated + // field (buildFilterBar applies it at build time) — refresh the stored + // status signature too, so this same publish doesn't ALSO fire a + // redundant `updateStatus` immediately after. + statusSig = newStatusSig; + } else if (newStatusSig !== statusSig) { + statusSig = newStatusSig; + filterBarUpdateStatus?.(statesByParam(sview.filters)); + } // #303: persist committed filter value/active into the isolated per-dashboard // store — isolated from the Workbench's asb:varValues/asb:filterActive keys. const filterBag = persistBagOf(sview.filters); diff --git a/src/ui/filter-bar.ts b/src/ui/filter-bar.ts index 2f89d68..fa91fa3 100644 --- a/src/ui/filter-bar.ts +++ b/src/ui/filter-bar.ts @@ -53,26 +53,102 @@ export interface BuildFilterBarOptions { curatedFields?: Record; } -/** One curated Dashboard Filter field's config (#160) — the shape - * `options.curatedFields[name]` carries, structurally read from the - * otherwise-`unknown` bag above. - * - * #360: `status`/`stale`/`waitingFor` mirror `ViewerFilterState`'s own - * fields (dashboard-viewer-session.ts) — `dashboard.ts`'s `rebuildFilterBar` - * forwards them straight through for every source-backed filter (gated on - * `status !== 'idle'`, never just "has options"), so a filter that's - * `waiting`/`loading`/errored with NO options yet still renders in this - * curated branch instead of silently falling out to the plain-text one - * below (plan-review BLOCKER-2). All three are optional so a caller that - * never supplies them (an older/simpler fixture) renders exactly like - * today's plain 'ready' combobox. */ -interface CuratedFieldConfig { - options: FilterFieldOption[]; +/** #360 maintainer-review follow-up: `status`/`stale`/`waitingFor` mirror + * `ViewerFilterState`'s own fields (dashboard-viewer-session.ts) — but + * WHETHER a filter is curated at all is `dashboard.ts`'s `rebuildFilterBar` + * gating on `f.sourceId != null` (topology, set once at construction), + * never on this transient status. Status is execution state, not topology: + * a source-backed filter starts `status: 'idle'` before its source has even + * run, so gating curation on "not idle" used to render it as a bare, + * enabled plain-text control on initial load until the source settled. + * These three fields are only the AFFORDANCE this already-curated field + * shows — all optional so a caller that never supplies them (an older/ + * simpler fixture) renders exactly like today's plain 'ready' combobox. */ +export interface CuratedFieldStatus { status?: string; stale?: boolean; waitingFor?: string[]; } +/** One curated Dashboard Filter field's config (#160) — the shape + * `options.curatedFields[name]` carries, structurally read from the + * otherwise-`unknown` bag above. */ +interface CuratedFieldConfig extends CuratedFieldStatus { + options: FilterFieldOption[]; +} + +/** A built curated field's retained handle (#360 follow-up) — kept in + * `buildFilterBar`'s `curatedHandles` map so a LATER status-only change can + * update this exact field's affordance in place (`applyFieldStatus`, via + * the returned `updateStatus`) without rebuilding the whole input — the + * rebuild would otherwise blow away in-progress typing on every other field + * in the bar and drop this field's own combobox/focus state. `baseTitle`/ + * `basePlaceholder` are this field's non-status tooltip/placeholder + * (`applyFieldStatus` restores them once a status stops overriding either); + * `noteEl` is the "Waiting for: …" node, present in the DOM only while + * `status === 'waiting'` (created/removed on demand, not just hidden, so a + * caller that queries for it — same as the pre-existing per-status build + * tests — sees exactly what it saw before this change). */ +interface CuratedFieldHandle { + input: HTMLInputElement; + label: HTMLElement; + baseTitle: string; + basePlaceholder: string; + noteEl: HTMLElement | null; +} + +/** + * Applies a curated field's status affordance to its already-built DOM + * (#360 follow-up) — the SAME class/disabled/note logic `buildFilterBar` + * used to inline once per field build, factored out so both the initial + * build (a fresh rebuild must show the right affordance immediately) and a + * later `updateStatus` call (no rebuild) share one recipe. See + * `CuratedFieldStatus`'s header comment for why status, not topology, drives + * this; see the module header's `buildFilterBar` doc for the full + * ready/waiting/error/stale mapping. + */ +function applyFieldStatus(handle: CuratedFieldHandle, s: CuratedFieldStatus): void { + const status = s.status ?? 'ready'; + const isWaiting = status === 'waiting'; + const isError = status === 'source-error' || status === 'helper-error' || status === 'missing-helper'; + // 'idle' (never yet run) and 'loading' (mid-flight) both read as "pending" — + // the same is-stale/disabled affordance as a superseded-but-not-yet-stale + // `stale: true` read, since none of the three is an actionable answer yet. + const isStale = !isWaiting && !isError && (status === 'loading' || status === 'idle' || !!s.stale); + const waitingNote = isWaiting ? `Waiting for: ${(s.waitingFor ?? []).join(', ')}` : ''; + const { input, label } = handle; + + input.classList.remove('is-waiting', 'is-error', 'is-stale'); + label.classList.remove('is-waiting', 'is-error', 'is-stale'); + const disabled = isWaiting || isError || isStale; + input.disabled = disabled; + if (disabled) input.setAttribute('aria-disabled', 'true'); + else input.removeAttribute('aria-disabled'); + if (isWaiting) { input.classList.add('is-waiting'); label.classList.add('is-waiting'); } + else if (isError) { input.classList.add('is-error'); label.classList.add('is-error'); } + else if (isStale) { input.classList.add('is-stale'); label.classList.add('is-stale'); } + + // Title/placeholder: the waiting note takes over both while waiting; + // otherwise they revert to the field's own non-status text. `is-invalid` + // (applied once at build time from the validated batch, unrelated to this + // status) owns the title instead whenever it's set — a status update never + // steps on the invalid-reason tooltip. + if (!input.classList.contains('is-invalid')) input.title = isWaiting ? waitingNote : handle.baseTitle; + input.placeholder = isWaiting ? waitingNote : handle.basePlaceholder; + + if (isWaiting) { + if (!handle.noteEl) { + handle.noteEl = h('span', { class: 'var-field-note' }, waitingNote); + label.appendChild(handle.noteEl); + } else { + handle.noteEl.textContent = waitingNote; + } + } else if (handle.noteEl) { + handle.noteEl.remove(); + handle.noteEl = null; + } +} + // A combobox-based field controller's DOM wiring surface, PLUS the `el` // wrapper and (relative-time fields only) the live preview element // `applyFieldState`'s `descEl` points at — the shape every one of @@ -115,10 +191,21 @@ export const FILTER_DEBOUNCE_MS = 500; * debounce timer. A caller that rebuilds the bar (a filter-value merge * repaint) must dispose the previous bar first — and dispose on its own * teardown — so an in-flight debounce never fires against a detached field - * (the orphan-timer gap a bare `replaceChildren` rebuild used to leave). */ + * (the orphan-timer gap a bare `replaceChildren` rebuild used to leave). + * + * #360 follow-up: `updateStatus` applies a per-param `CuratedFieldStatus` + * update to whichever curated fields this SAME bar instance already built + * (`curatedHandles`, keyed by parameter) — a param this bar never curated + * (absent from `curatedFields` at build time, or a plain field) is silently + * ignored. The caller (`dashboard.ts`'s `rebuildFilterBar`) uses this for a + * status-only change (e.g. `loading` → `ready`, no value/active/options + * change) instead of tearing down and rebuilding the whole bar — preserving + * in-progress typing on every OTHER field, and this field's own combobox/ + * focus state, neither of which a status flip should ever disturb. */ export interface FilterBarHandle { el: HTMLElement; dispose(): void; + updateStatus(states: Record): void; } /** @@ -151,8 +238,13 @@ export function buildFilterBar( const document = options.document || app.document; const attrs: Record = { class: 'dash-filters' }; if (options.ariaLabel) { attrs.role = 'group'; attrs['aria-label'] = options.ariaLabel; } - if (!params.length) return { el: h('div', { ...attrs, style: { display: 'none' } }), dispose: () => {} }; + if (!params.length) { + return { el: h('div', { ...attrs, style: { display: 'none' } }), dispose: () => {}, updateStatus: () => {} }; + } const timerClears: Array<() => void> = []; + // #360 follow-up: every curated field's retained handle, keyed by + // parameter — see `CuratedFieldHandle` and `FilterBarHandle.updateStatus`. + const curatedHandles = new Map(); const el = h('div', attrs, ...params.map((p) => { let timer: ReturnType | null = null; timerClears.push(() => { if (timer != null) clearTimeout(timer); timer = null; }); @@ -168,21 +260,6 @@ export function buildFilterBar( + (conflictNote ? ' — ' + conflictNote : ''); const curated = options.curatedFields?.[p.name] as CuratedFieldConfig | undefined; if (curated) { - // #360: a shared source's transport/curation status drives a STRUCTURAL, - // test-observable affordance (a class + `disabled`/`aria-disabled`, and - // — for `waiting` — literal text naming the missing root params) rather - // than relying on styling alone (happy-dom never evaluates CSS layout). - // Absent `status` (an older/simpler fixture) behaves exactly like - // 'ready' — today's normal curated combobox, untouched. - const status = curated.status ?? 'ready'; - const isWaiting = status === 'waiting'; - const isError = status === 'source-error' || status === 'helper-error' || status === 'missing-helper'; - // 'loading' is the only other non-terminal status; `stale` mirrors it - // 1:1 per `ViewerFilterState`'s own contract — checking both is - // defensive, not redundant coverage of two different real states. - const isStale = !isWaiting && !isError && (status === 'loading' || !!curated.stale); - const waitingNote = isWaiting ? `Waiting for: ${(curated.waitingFor ?? []).join(', ')}` : ''; - const fieldTitle = isWaiting ? waitingNote : baseTitle; const field = buildFilterOptionField({ document, name: p.name, options: curated.options, value: app.state.varValues[p.name] ?? '', active: !!app.state.filterActive[p.name], @@ -195,7 +272,6 @@ export function buildFilterBar( }, onCommit: () => onCommit(p.name), }); - field.input.title = fieldTitle; // #345: a curated field is always the 'enum' width band (short option // labels) regardless of the declared param type behind it. applyFieldWidth(field.input, p.type, true); @@ -205,29 +281,22 @@ export function buildFilterBar( // invalid against the prepared batch (e.g. a type conflict across // favorites), and without this it silently showed none of the // is-invalid class/tooltip/aria-invalid a plain filter field would. - applyFieldState(field.input, getField(p.name, 'execute'), fieldTitle); - if (isWaiting) { - field.input.classList.add('is-waiting'); - field.input.disabled = true; - field.input.setAttribute('aria-disabled', 'true'); - field.input.placeholder = waitingNote; - } else if (isError) { - field.input.classList.add('is-error'); - field.input.disabled = true; - field.input.setAttribute('aria-disabled', 'true'); - } else if (isStale) { - // Loading/stale: keep whatever options/value were last known (never - // cleared — the merge itself never blanks `options` mid-wave, see - // dashboard-viewer-session.ts's `runFilterWave`) but mark it plainly - // not-current rather than presenting a stale answer as fresh. - field.input.classList.add('is-stale'); - field.input.disabled = true; - field.input.setAttribute('aria-disabled', 'true'); - } - const stateClass = isWaiting ? ' is-waiting' : isError ? ' is-error' : isStale ? ' is-stale' : ''; - return h('label', { class: 'var-field is-curated' + (p.optional ? ' is-optional' : '') + stateClass }, - h('span', { class: 'var-name' }, p.name), field.el, - isWaiting ? h('span', { class: 'var-field-note' }, waitingNote) : null); + // Uses `baseTitle` (not a status-derived title) as the non-invalid + // fallback — `applyFieldStatus` below layers the status-derived title + // back on top when the field isn't currently `is-invalid`. + applyFieldState(field.input, getField(p.name, 'execute'), baseTitle); + const label = h('label', { class: 'var-field is-curated' + (p.optional ? ' is-optional' : '') }, + h('span', { class: 'var-name' }, p.name), field.el); + const handle: CuratedFieldHandle = { + input: field.input, label, baseTitle, basePlaceholder: field.input.placeholder, noteEl: null, + }; + // #360 follow-up: apply the CURRENT status immediately at build time + // (a fresh rebuild shows the right affordance right away) and retain + // the handle so a LATER status-only change updates this same field in + // place via `updateStatus`, never a rebuild. + applyFieldStatus(handle, { status: curated.status, stale: curated.stale, waitingFor: curated.waitingFor }); + curatedHandles.set(p.name, handle); + return label; } const commitNow = (): void => { if (timer == null) return; @@ -298,5 +367,14 @@ export function buildFilterBar( return h('label', { class: 'var-field' + (p.optional ? ' is-optional' : '') }, h('span', { class: 'var-name' }, p.name), combo.el); })); - return { el, dispose: () => timerClears.forEach((clear) => clear()) }; + return { + el, + dispose: () => timerClears.forEach((clear) => clear()), + updateStatus: (states) => { + for (const [name, handle] of curatedHandles) { + const s = states[name]; + if (s) applyFieldStatus(handle, s); + } + }, + }; } diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index fbc18a1..0c5ae20 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -2582,14 +2582,17 @@ describe('renderDashboard — filter-source runtime rebuild + diagnostics (#359) }); }); -// #360 plan-review BLOCKER-2: `rebuildFilterBar` used to gate a filter into -// the curated (rich combobox) rendering path only `if (f.options && -// f.options.length)` — so a source-backed filter with NO options yet -// (waiting on a root dependency, mid-flight, or errored) fell OUT of that -// path entirely and rendered as a bare, unlabelled plain field with zero -// indication anything was pending. The gate is now "every non-idle filter" — -// a plain (non-source-backed) filter is documented to stay 'idle' forever, -// so this never touches the plain-filter path. +// #360 plan-review BLOCKER-2 (+ maintainer-review follow-up): `rebuildFilterBar` +// used to gate a filter into the curated (rich combobox) rendering path only +// `if (f.options && f.options.length)`, then — a first fix — `if (f.status +// !== 'idle')` — so a source-backed filter with NO options yet (waiting on a +// root dependency, mid-flight, or errored, OR simply not yet run) fell OUT of +// that path entirely and rendered as a bare, unlabelled plain field with zero +// indication anything was pending. The gate is now `f.sourceId != null` — +// TOPOLOGY, set once at construction, never the transient `status` — so a +// source-backed filter is ALWAYS curated, at any status, from the very first +// (still-'idle') render onward. A plain (non-source-backed) filter has no +// `sourceId` and never touches this path. describe('renderDashboard — curated field stays curated while its shared source is waiting (#360)', () => { it('a source-backed filter whose source waits on a root dependency renders the curated waiting affordance, not a bare plain field', async () => { const { app, calls } = dashApp({ @@ -2625,6 +2628,97 @@ describe('renderDashboard — curated field stays curated while its shared sourc }); }); +// #360 maintainer-review follow-up: `rebuildFilterBar` gates curation on +// TOPOLOGY (`sourceId != null`) and updates a settled bar's STATUS in place +// (`filterBar.updateStatus`, never a rebuild) — the effect's `barSig` (a +// structural signature: id/active/value/optionsRev/sourceId) is now separate +// from its own status signature (status/stale/waitingFor). These tests pin +// that split directly: a source-backed filter is curated from its very first +// (still-'idle'/'loading') render, a pure status change never disturbs the +// bar's DOM (same `` instance — proof no rebuild happened), and a +// genuinely structural change (option content) still rebuilds it. +describe('renderDashboard — filter status vs. structural rebuild split (#360 follow-up)', () => { + it('a source-backed filter is curated and shows the pending affordance before its shared source has ever settled — never an enabled plain control', async () => { + let resolveSrc!: (v: ExecResp) => void; + const pendingSrc = new Promise((resolve) => { resolveSrc = resolve; }); + const { app } = dashApp({ + responder: (sql) => (sql.includes('pendingsrc') ? pendingSrc : {}), + workspace: wsWith({ + queries: [ + q('q1', 'SELECT k, v FROM a WHERE region = {region:String}'), + q('src', "SELECT ['east','west'] AS region -- pendingsrc", { dashboard: { role: 'filter' } }), + ], + tiles: [{ id: 't1', queryId: 'q1' }], + filters: [{ id: 'f-region', parameter: 'region', sourceQueryId: 'src' }], + }), + }); + const rendering = render(app); + // Flush the microtasks up to (but not past) the in-flight source query's + // own await — same technique as the KPI "Loading…" card test above: the + // session sets status 'loading' (mirroring its still-'idle' construction- + // time value — both read as "pending" — see `applyFieldStatus`) and + // publishes synchronously before awaiting the responder. + await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); + const fields = qsa(app.root, '.dash-filter-host .var-field'); + const regionField = fields.find((f) => qs(f, '.var-name')?.textContent === 'region') as HTMLElement; + expect(regionField).toBeDefined(); + expect(regionField.classList.contains('is-curated')).toBe(true); + expect(regionField.classList.contains('is-stale')).toBe(true); + const input = qs(regionField, 'input'); + expect(input.disabled).toBe(true); + resolveSrc({ columns: [{ name: 'region', type: 'Array(String)' }], rows: [[['east', 'west']]] }); + await rendering; + // Settles to 'ready' — the SAME curated field, now enabled with options. + const settledInput = qs(app.root, '.dash-filter-host input'); + expect(settledInput.disabled).toBe(false); + }); + + it('a status-only refresh (unchanged options/value/active) updates the field in place — its instance survives, no rebuild', async () => { + const { app } = dashApp({ + responder: (sql) => (sql.includes('samesrc') + ? { columns: [{ name: 'region', type: 'Array(String)' }], rows: [[['east', 'west']]] } : {}), + workspace: wsWith({ + queries: [ + q('q1', 'SELECT k, v FROM a WHERE region = {region:String}'), + q('src', "SELECT ['east','west'] AS region -- samesrc", { dashboard: { role: 'filter' } }), + ], + tiles: [{ id: 't1', queryId: 'q1' }], + filters: [{ id: 'f-region', parameter: 'region', sourceQueryId: 'src' }], + }), + }); + await render(app); + const inputBefore = qs(app.root, '.dash-filter-host input'); + // A refresh republishes the exact same option content — status cycles + // loading → ready with optionsRev unchanged, so only `updateStatus` runs. + await (runOnclick(qs(app.root, '.dash-refresh')) as Promise); + const inputAfter = qs(app.root, '.dash-filter-host input'); + expect(inputAfter).toBe(inputBefore); + expect(inputAfter.disabled).toBe(false); + }); + + it('a structural change (option content changes) still rebuilds the bar — a fresh instance', async () => { + let call = 0; + const { app } = dashApp({ + responder: (sql) => (sql.includes('changingsrc') + ? (() => { call++; return { columns: [{ name: 'region', type: 'Array(String)' }], rows: [[call === 1 ? ['east', 'west'] : ['north', 'south']]] }; })() + : {}), + workspace: wsWith({ + queries: [ + q('q1', 'SELECT k, v FROM a WHERE region = {region:String}'), + q('src', "SELECT ['east','west'] AS region -- changingsrc", { dashboard: { role: 'filter' } }), + ], + tiles: [{ id: 't1', queryId: 'q1' }], + filters: [{ id: 'f-region', parameter: 'region', sourceQueryId: 'src' }], + }), + }); + await render(app); + const inputBefore = qs(app.root, '.dash-filter-host input'); + await (runOnclick(qs(app.root, '.dash-refresh')) as Promise); + const inputAfter = qs(app.root, '.dash-filter-host input'); + expect(inputAfter).not.toBe(inputBefore); // optionsRev bumped — a real rebuild + }); +}); + // #303: the isolated per-dashboard filter store (`asb:dashFilters`) — the // #280 viewer session used to init every filter purely from // `def.defaultValue`/`defaultActive`, so a committed value lived only in diff --git a/tests/unit/filter-bar.test.ts b/tests/unit/filter-bar.test.ts index b0acb0a..6b3736d 100644 --- a/tests/unit/filter-bar.test.ts +++ b/tests/unit/filter-bar.test.ts @@ -37,6 +37,7 @@ describe('buildFilterBar (shared filter row)', () => { expect(bar.el.getAttribute('aria-label')).toBe('Query filters'); expect(bar.el.querySelectorAll('.var-field').length).toBe(0); expect(() => bar.dispose()).not.toThrow(); // no fields, no timers — a no-op + expect(() => bar.updateStatus({})).not.toThrow(); // no curated fields — a no-op }); it('defaults to app.document and no group role when no options are passed', () => { @@ -183,11 +184,27 @@ describe('buildFilterBar (shared filter row)', () => { }); // #360: a source-backed curated field now stays in this rich-combobox - // rendering path for EVERY non-idle status — not just when it currently - // has options — and shows a structural (class + disabled/aria-disabled +, - // for 'waiting', literal text) affordance instead of silently rendering - // (or falling out to) an unmarked control (plan-review BLOCKER-2). + // rendering path for EVERY status (including 'idle', its FIRST-ever value + // before the shared source has run at all — dashboard.ts's rebuildFilterBar + // now gates curation on `sourceId != null`, topology, never on `status`) and + // shows a structural (class + disabled/aria-disabled +, for 'waiting', + // literal text) affordance instead of silently rendering (or falling out + // to) an unmarked control (plan-review BLOCKER-2). describe('curated field status affordance (#360)', () => { + it('status: "idle" (a source-backed field that has never run yet) still renders CURATED, marked pending — not an enabled plain control', () => { + const app = makeApp(); + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { + curatedFields: { x: { options: [], status: 'idle' } }, + }); + const label = bar.el.querySelector('.var-field')!; + const input = bar.el.querySelector('input') as HTMLInputElement; + expect(label.classList.contains('is-curated')).toBe(true); + // 'idle' reads exactly like 'loading' — pending, not yet actionable. + expect(label.classList.contains('is-stale')).toBe(true); + expect(input.disabled).toBe(true); + expect(input.getAttribute('aria-disabled')).toBe('true'); + }); + it('an explicit status: "ready" (or an absent status) renders the normal curated combobox — no new classes, not disabled', () => { const app = makeApp(); const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { @@ -271,6 +288,86 @@ describe('buildFilterBar (shared filter row)', () => { }); }); + // #360 follow-up: `updateStatus` updates a curated field's affordance IN + // PLACE — the whole point being that the caller (dashboard.ts) never has to + // rebuild the bar (and disturb every other field's in-progress typing) just + // because ONE source-backed filter's status changed. + describe('updateStatus (#360 follow-up)', () => { + it('flips a curated field idle → loading → ready → waiting → error without ever rebuilding it (same input instance throughout)', () => { + const app = makeApp(); + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { + curatedFields: { x: { options: [], status: 'idle' } }, + }); + const label = bar.el.querySelector('.var-field')!; + const input = bar.el.querySelector('input') as HTMLInputElement; + + bar.updateStatus({ x: { status: 'loading' } }); + expect(bar.el.querySelector('input')).toBe(input); // same instance — no rebuild + expect(label.classList.contains('is-stale')).toBe(true); + expect(input.disabled).toBe(true); + + bar.updateStatus({ x: { status: 'ready' } }); + expect(bar.el.querySelector('input')).toBe(input); + expect(label.classList.contains('is-stale')).toBe(false); + expect(input.disabled).toBe(false); + expect(input.hasAttribute('aria-disabled')).toBe(false); + + bar.updateStatus({ x: { status: 'waiting', waitingFor: ['from'] } }); + expect(bar.el.querySelector('input')).toBe(input); + expect(label.classList.contains('is-waiting')).toBe(true); + expect(label.classList.contains('is-stale')).toBe(false); + expect(input.disabled).toBe(true); + expect(label.textContent).toContain('Waiting for: from'); + const noteEl = label.querySelector('.var-field-note'); + + // A SECOND consecutive 'waiting' update (still missing a different root + // param) reuses the SAME note element — updates its text in place + // rather than removing and recreating it. + bar.updateStatus({ x: { status: 'waiting', waitingFor: ['to'] } }); + expect(label.querySelector('.var-field-note')).toBe(noteEl); + expect(label.textContent).toContain('Waiting for: to'); + + bar.updateStatus({ x: { status: 'source-error' } }); + expect(bar.el.querySelector('input')).toBe(input); + expect(label.classList.contains('is-error')).toBe(true); + expect(label.classList.contains('is-waiting')).toBe(false); + expect(input.disabled).toBe(true); + // The waiting note is removed once the field leaves 'waiting'. + expect(label.querySelector('.var-field-note')).toBeNull(); + }); + + it('preserves an in-progress typed draft across an updateStatus call (no rebuild disturbs it)', () => { + const app = makeApp(); + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { + curatedFields: { x: { options: [{ value: 'a', label: 'Alpha' }], status: 'ready' } }, + }); + const input = bar.el.querySelector('input') as HTMLInputElement; + input.value = 'still typing…'; + bar.updateStatus({ x: { status: 'loading' } }); + expect(input.value).toBe('still typing…'); // untouched by the status update + expect(bar.el.querySelector('input')).toBe(input); + }); + + it('ignores a status for a param this bar never curated (a plain field), and a plain field is never disabled by it', () => { + const app = makeApp(); + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField); + const input = bar.el.querySelector('input') as HTMLInputElement; + expect(() => bar.updateStatus({ x: { status: 'waiting' } })).not.toThrow(); + expect(input.disabled).toBe(false); + expect(input.classList.contains('is-waiting')).toBe(false); + }); + + it('an updateStatus() call that names no curated field is a no-op (leaves its current affordance untouched)', () => { + const app = makeApp(); + const bar = buildFilterBar(app, paramsFor('SELECT {x:String}'), () => {}, okField, { + curatedFields: { x: { options: [{ value: 'a', label: 'Alpha' }], status: 'ready' } }, + }); + const input = bar.el.querySelector('input') as HTMLInputElement; + expect(() => bar.updateStatus({})).not.toThrow(); + expect(input.disabled).toBe(false); // still ready — never touched + }); + }); + it('dispose() clears a pending debounce timer so a later value edit never fires the stale commit (#276)', () => { vi.useFakeTimers(); try { From a49f02c4ddebe903f0690858a17a857c0af1a32f Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 17:32:34 +0000 Subject: [PATCH 13/16] docs(#360): make the example qle-filter {to} optional to match its panels (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer review: the migrated source made both from and to required, but the dashboard's panels treat to as optional (/*[ ... <= {to} ]*/, blank = up to now). Align the source to the same contract — from required, to optional and inclusive — so clearing to no longer forces the source into waiting while the panels keep running. CHANGELOG wording updated to match. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 16 +++++++++------- examples/query-log-explorer.json | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 361c834..fe68118 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,13 +28,15 @@ auto-generated per-PR notes; this file is the curated, human-readable history. in `core/filter-execution.ts`), so structural diagnostics, relative-value resolution, optional-block materialization, validation/serialization, and native arguments are identical on both surfaces. Source-backed filters render a - waiting / stale / error affordance in the filter bar instead of silently - degrading to a plain control; a superseded source response can never publish - stale results, and a dependency change clears the affected options so a stale - set can't appear current while the new source loads. Filter execution injects - no `readonly` setting (kept from #359). `examples/query-log-explorer.json`'s - `qle-filter` source is migrated to the half-open `{from:DateTime}`/`{to:DateTime}` - window as a worked example. + waiting / stale / error affordance in the filter bar (chosen by source + topology, not transient status) instead of silently degrading to a plain + control; a superseded or session-destroyed commit can never publish stale + results or run its panels, and a dependency change clears the affected options + so a stale set can't appear current while the new source loads. Filter + execution injects no `readonly` setting (kept from #359). + `examples/query-log-explorer.json`'s `qle-filter` source is migrated to a + `{from:DateTime}` / optional `{to:DateTime}` window (matching its panels, where + `to` is optional and means "up to now") as a worked example. ### Fixed - Removed a stray NUL byte embedded in `dashboard-viewer-session.ts`'s diff --git a/examples/query-log-explorer.json b/examples/query-log-explorer.json index eab9cb9..dafff81 100644 --- a/examples/query-log-explorer.json +++ b/examples/query-log-explorer.json @@ -10,7 +10,7 @@ "queries": [ { "id": "qle-filter", - "sql": "SELECT\n arraySort(item -> item.label, groupUniqArray((user AS value, splitByChar('@', user)[1] AS label))) AS user,\n arraySort(groupUniqArray(query_kind)) AS query_kind\nFROM system.query_log\nWHERE query_log.user != ''\n AND event_time >= {from:DateTime}\n AND event_time < {to:DateTime}\nSETTINGS enable_named_columns_in_function_tuple = 1", + "sql": "SELECT\n arraySort(item -> item.label, groupUniqArray((user AS value, splitByChar('@', user)[1] AS label))) AS user,\n arraySort(groupUniqArray(query_kind)) AS query_kind\nFROM system.query_log\nWHERE query_log.user != ''\n AND event_time >= {from:DateTime}\n /*[ AND event_time <= {to:DateTime} ]*/\nSETTINGS enable_named_columns_in_function_tuple = 1", "specVersion": 1, "spec": { "name": "Filter", From 2d2c1ec7d313a58787a7fc04a4da7c3320722feb Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 17:50:57 +0000 Subject: [PATCH 14/16] fix(#360): drop over-broad commit-generation guard; rely on per-source supersession (review round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The commit-level generation counter (added last round) was bumped by EVERY commit — including unrelated fast-path ones — so an unrelated overlapping commit (e.g. Tab-then-Enter across two fields) made a legitimate, non-superseded commit skip its affected-panel wave, leaving its tile silently bound to the old value. The per-source generation check in applyFilterProviders (surfaced as result.status === 'superseded') already covers every real supersession — the same-param overlap (the newer commit supersede()s the shared source), a concurrent full refresh() (runFilterWave supersedes all sources), and destroy() — without penalizing two commits that touch genuinely unrelated sources. So commitAndRerun now guards only on `destroyed || result.status === 'superseded'`; the commitGeneration counter and its publish() workaround are removed. Added a regression test: two unrelated overlapping commits each run their own panel wave. dashboard-viewer-session.ts: 100% stmts/fns/lines, 94.21% branches. Gate green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../application/dashboard-viewer-session.ts | 87 ++++++++----------- tests/unit/dashboard-viewer-session.test.ts | 81 +++++++++++++++-- 2 files changed, 111 insertions(+), 57 deletions(-) diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index e2208fc..9267851 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -394,11 +394,6 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // outside `documentRef` — never read/written by any command, never // persisted. A fresh session always starts at 'tiles'. let gridRenderMode: GridRenderMode = 'tiles'; - // #360 findings 1+2: bumped once per `commitAndRerun` call, covering BOTH - // phases of its affected path (the source wave, then the panel wave) — see - // the rationale on `commitAndRerun` itself for why a per-phase generation - // guard alone (each phase already has one) is not enough. - let commitGeneration = 0; const queryById = new Map(); for (const query of queries) { @@ -1123,57 +1118,49 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // into both waves — otherwise a stale token would fail `ensureFreshToken()` // (and fire `onAuthFailed`) twice for one commit. // - // Findings 1+2 (maintainer review): a commit-level `commitGeneration` - // guards BOTH phases of the affected path together — not just each phase's - // own internal generation checks — because two failure modes were - // otherwise indistinguishable from "ran, nothing to do": - // - Superseded (finding 1): an overlapping LATER commit (B, a different - // `commitAndRerun` call) can supersede THIS commit's (A) source wave - // mid-flight by reserving a fresh generation on one of the same sources. - // `runFilterSourceWave`/`applyFilterProviders` correctly discard A's - // stale results (`{status:'superseded'}`), but without a check here A - // would still go on to launch its OWN panel wave — issuing avoidable - // requests and binding source-backed filter values that have already - // moved past while B is still in flight. Comparing `generation` (this - // call's own snapshot) against `commitGeneration` (bumped by every NEW - // `commitAndRerun` call) after the source wave catches a commit-level - // supersession even in cases the plan-level `SourceWaveResult` alone - // would not (e.g. a concurrent full `refresh()`, which bumps every - // source's generation but never touches `commitGeneration`) — so both - // checks are kept, each catching a case the other does not. - // - Destroyed (finding 2): `runAffectedWave(parameters, true)` passes - // `preflighted: true`, which skips its own `preflight()` call — and - // `preflight()` was the ONLY place that checked `destroyed` on this - // path. A `destroy()` firing while the source wave is in flight would - // otherwise let the panel wave still reserve tile generations and issue - // requests after teardown. The explicit `destroyed` check here, plus the - // new unconditional guard at the top of `runAffectedWave` itself - // (belt-and-braces — `runAffectedWave` is also reachable directly from - // the fast path), closes both entry points. + // Findings 1+2 (maintainer review): a superseded or destroyed source wave + // must not go on to launch its own panel wave. Both are already caught by + // checks that exist independently of any commit-to-commit bookkeeping here + // — no extra "commit generation" counter is needed (an earlier version of + // this fix added one bumped on EVERY `commitAndRerun` call, including the + // no-affected-source fast path; that made two commits affecting DIFFERENT, + // non-overlapping sources spuriously supersede one another — a legitimate + // concurrent commit's own, correctly-`'applied'` result would still get + // discarded and its panel wave skipped just because an unrelated commit + // happened to be in flight at the same time. Reverted: see PR review): + // - Superseded (finding 1): `result.status === 'superseded'` already + // covers every real case. Two commits overlapping on the SAME (or a + // cascading-dependent) source: the later one's `runFilterSourceWave` + // calls `supersede()` on that shared `FilterSourceRuntime`, reserving a + // fresh generation; when the earlier commit's held request finally + // settles, its `applyFilterProviders` plan no longer matches that + // generation, so it returns `{status:'superseded'}` — see + // `applyFilterProviders`'s own stale-wave guard. A concurrent + // UNRELATED full `refresh()` supersedes EVERY known source (not just + // the ones a param change affects), so it catches this too, the same + // way. Two commits affecting genuinely DIFFERENT, non-dependent sources + // never touch each other's generations, so neither is ever marked + // superseded — exactly the case that must still run its own panel wave. + // - Destroyed (finding 2): `destroy()` bumps every source's (and every + // tile's) generation too, so an in-flight source wave racing a + // `destroy()` is ALSO naturally caught by the same `'superseded'` check + // above; the explicit `destroyed` check here plus the new unconditional + // guard at the top of `runAffectedWave` close the remaining gap — the + // `preflighted: true` fast path into `runAffectedWave` (the only caller + // that skips `preflight()`, previously the only `destroyed` check on + // this path) reachable right after the two checks below pass. async function commitAndRerun(changed: string[]): Promise { - const generation = ++commitGeneration; const hasAffectedSource = [...filterSources.values()] .some((source) => source.analyzed.dependsOn.some((name) => changed.includes(name))); if (!hasAffectedSource) { await runAffectedWave(changed); return; } if (!(await preflight())) return; const result = await runFilterSourceWave(changed); - if (destroyed || generation !== commitGeneration || result.status === 'superseded') { - // The source wave's own `applyFilterProviders` merge (options/status - // updates on `consumer.state`) already happened synchronously inside - // `runFilterSourceWave` — but unlike the normal path below, we are - // skipping `runAffectedWave` entirely, and `runAffectedWave` is the - // step that normally `publish()`es afterward. Without this publish, a - // commit that lost the commit-level race (`generation !== - // commitGeneration`, e.g. an UNRELATED overlapping commit) would still - // have applied a real, non-superseded merge that never reaches - // `state.value` — the mutation sits in the runtime record until some - // LATER, unrelated publish happens to flush it. A genuinely - // `'superseded'` result changed nothing, so this is a harmless replay - // of the current state in that case. Skipped entirely once `destroyed` - // — no further publish is attributable to a torn-down session. - if (!destroyed) publish(); - return; - } + // A `'superseded'` result means `applyFilterProviders` returned BEFORE + // merging anything (its own stale-wave guard, `applyFilterProviders`'s + // doc comment) — no consumer state changed, so there is nothing new to + // `publish()`; the wave that superseded this one owns publishing the + // eventual settled state. + if (destroyed || result.status === 'superseded') return; await runAffectedWave([...new Set([...changed, ...result.flipped])], true); } diff --git a/tests/unit/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index 7a36dcc..37828a0 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -1572,8 +1572,15 @@ describe('parameterized Filter sources (#360)', () => { // source wave turns out to have been superseded or the session was // destroyed — previously `runFilterSourceWave`/`applyFilterProviders` // returned a bare `[]` for BOTH "applied, nothing flipped" and "discarded, -// stale plan", so a stale commit still ran `runAffectedWave` afterward. -describe('commit-level generation guard for a superseded/destroyed selective wave (#360 review findings 1/2)', () => { +// stale plan", so a stale commit still ran `runAffectedWave` afterward. The +// fix relies ENTIRELY on `SourceWaveResult` (`applyFilterProviders`'s own +// per-source generation check) plus the `destroyed` flag — no separate +// "commit generation" counter: an earlier version of this fix added one, +// bumped on every `commitAndRerun` call including the no-affected-source +// fast path, which made two commits affecting completely unrelated, +// non-overlapping sources spuriously supersede one another (see the +// "unrelated overlapping commits" test below, added after that review round). +describe('superseded/destroyed selective-wave guard (#360 review findings 1/2)', () => { // A root filter ('from') feeds a shared Filter source ('src') that a // second, source-backed filter ('f-dep') consumes; a tile binds 'from' // directly so a fired panel wave for it is unambiguous and distinguishable @@ -1622,7 +1629,7 @@ describe('commit-level generation guard for a superseded/destroyed selective wav expect(sourceCalls).toBe(3); }); - it('finding 1 (isolated): a source wave superseded by an UNRELATED concurrent full refresh (not another commit) still skips the panel wave, even though commitGeneration itself never changed', async () => { + it('finding 1 (isolated): a source wave superseded by a concurrent full refresh() (not another commit) still skips the panel wave', async () => { let releaseGate!: () => void; const gate = new Promise((resolve) => { releaseGate = resolve; }); let sourceCalls = 0; @@ -1639,19 +1646,79 @@ describe('commit-level generation guard for a superseded/destroyed selective wav const commit = session.setFilter('from-root', 'v1'); // selective source wave starts; gated (call #2) await flush(); - const refreshP = session.refresh(); // a full refresh — bumps EVERY source's generation, never commitGeneration + const refreshP = session.refresh(); // a full refresh — bumps EVERY source's generation, including this one's await refreshP; releaseGate(); await commit; // `refresh()` legitimately fires the tile once (it is not source-targeted, // so it runs unaffected/parallel); the selective commit must NOT have - // fired an additional, now-stale panel request of its own even though - // `commitGeneration` was never touched by `refresh()` — only the - // `SourceWaveResult` status caught this one. + // fired an additional, now-stale panel request of its own — its own + // source plan is stale the instant `refresh()`'s `runFilterWave` reserves + // a fresh generation on the SAME source, so `applyFilterProviders` + // returns `{status:'superseded'}` regardless of anything commit-specific. expect(tileCallCount(calls) - before).toBe(1); }); + it('unrelated overlapping commits (different, non-dependent sources/params) each still run their own panel wave — neither supersedes the other', async () => { + // Reproduces the bug the maintainer review caught in a `commitGeneration` + // counter this fix does NOT use: 'region' feeds source 'src' (curating + // 'city', targeting tile t1); 'status' is a wholly unrelated PLAIN filter + // (no source at all) feeding tile t2 directly. Committing 'status' while + // 'region''s source wave is still in flight must not stop 'region''s own + // commit from running t1's panel wave once its (genuinely unraced, + // `'applied'`) source wave settles. + let releaseSrc!: () => void; + const gate = new Promise((resolve) => { releaseSrc = resolve; }); + let sourceCalls = 0; + const { exec, calls } = makeExec((sql) => { + if (!sql.includes('source')) return { columns: [{ name: 'n' }], rows: [[1]] }; + sourceCalls += 1; + return sourceCalls === 2 + ? gate.then(() => ({ columns: [{ name: 'city', type: 'Array(String)' }], rows: [[['x1']]] })) + : { columns: [{ name: 'city', type: 'Array(String)' }], rows: [[['x0']]] }; + }); + const document = doc({ + tiles: [tile('t1', 'q1'), tile('t2', 'q2')], + filters: [ + { id: 'region-root', parameter: 'region', defaultActive: true, defaultValue: 'v0' }, + { id: 'f-city', parameter: 'city', sourceQueryId: 'src', defaultActive: false, defaultValue: '' }, + { id: 'status-root', parameter: 'status', defaultActive: true, defaultValue: 's0' }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('q1', 'SELECT {region:String} AS n'), + query('q2', 'SELECT {status:String} AS n'), + query('src', "SELECT ['x'] AS city FROM t WHERE ts >= {region:String} /* source */", { dashboard: { role: 'filter' } }), + ], + })); + // The 'src' source SQL ALSO contains the substring '{region:String}' (it + // depends on 'region' too), so t1's own calls must be distinguished from + // 'src''s by excluding anything tagged `/* source */`. + const t1Calls = () => calls.filter((c) => !c.sql.includes('source') && c.sql.includes('{region:String}')).length; + const t2Calls = () => calls.filter((c) => c.sql.includes('{status:String}')).length; + await session.start(); // sourceCalls -> 1 (immediate); t1 and t2 each ran once. + const t1Before = t1Calls(); + const t2Before = t2Calls(); + + const commitRegion = session.setFilter('region-root', 'v1'); // affected path; src's exec is call #2, gated + await flush(); + const commitStatus = session.setFilter('status-root', 's1'); // unrelated: no affected source, fast path + await commitStatus; // resolves immediately — fires t2, never touches 'src' + + releaseSrc(); + await commitRegion; // src settles normally (never superseded) — must still run t1's panel wave + + expect(t2Calls() - t2Before).toBe(1); + // The regression: t1 must ALSO have rerun. A `commitGeneration` counter + // bumped by the unrelated 'status' commit would have made 'region''s + // commit see a generation mismatch and skip this even though its own + // source data was fresh and correctly `'applied'`. + expect(t1Calls() - t1Before).toBe(1); + }); + it('finding 2: destroy() during an in-flight selective source wave prevents its panel wave from ever firing', async () => { let releaseGate!: () => void; const gate = new Promise((resolve) => { releaseGate = resolve; }); From 58063aad582ab51d29e0d4b74f5dc75d9b90771b Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 18:58:58 +0000 Subject: [PATCH 15/16] fix(#360): full refresh defers affected panels when its Filter wave was superseded (review round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runFilterWave discarded its SourceWaveResult, claiming a full refresh has no later phase — but refresh() runs the affected-panel wave after the filter wave. So a refresh whose Filter wave was superseded by a concurrent selective commit still ran its affected panels before the superseding wave's source updates + reconciliation, unprotected by tile generations (the selective commit reserves its own tile generations only later, inside its runAffectedWave). runFilterWave now returns SourceWaveResult, and refresh() skips its affected-panel wave when that result is 'superseded' — the superseding selective commit is the current source-of-truth and runs those panels after it settles. Regression test added (verified it fails without the guard). dashboard-viewer-session.ts: 100% stmts/fns/lines, 93.6% branches. Gate green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../application/dashboard-viewer-session.ts | 37 +++++++++--- tests/unit/dashboard-viewer-session.test.ts | 56 +++++++++++++++++++ 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index 9267851..0ade117 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -986,16 +986,23 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa return applyFilterProviders(plan); } - async function runFilterWave(): Promise { + async function runFilterWave(): Promise { // One wall-clock reading for the WHOLE wave (mirrors the tile wave's own // `deps.wallNow()` capture in `refresh()`). const waveMs = deps.wallNow(); // Full refresh: every known source, keep options through loading (#359 // no-flicker invariant), reset diagnostics immediately (see - // `executeFilterSourcePlan`'s doc comment for the "why"). The returned - // `SourceWaveResult` is ignored — a full refresh has no further phase to - // gate on it. - await executeFilterSourcePlan([...filterSources.values()], + // `executeFilterSourcePlan`'s doc comment for the "why"). Unlike a + // stand-alone call, THIS caller (`refresh()`) has a later phase that + // depends on the result: its own affected-panel wave runs AFTER this + // settles, using the merged/reconciled values. A `{status:'superseded'}` + // here means a concurrent SELECTIVE commit reserved a fresher generation + // on one of these sources and is now the source-of-truth for them — + // `refresh()` must skip its own affected-panel wave in that case (see its + // caller, just below) rather than run it against source data that + // predates that commit's own update, and defer to that commit's own + // `runAffectedWave` instead. + return executeFilterSourcePlan([...filterSources.values()], { clearOptions: false, resetDiagnostics: true, waveMs }); } @@ -1054,9 +1061,25 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa const firstBatch = sourcesById(prepareBatch('execute').sources); const unaffectedWave = runPool(unaffected, VIEWER_TILE_CONCURRENCY, (runtime) => runTile(runtime, firstBatch.get(runtime.tile.id), generations.get(runtime.tile.id)!)); - const filterWave = runFilterWave(); - await filterWave; + const filterResult = await runFilterWave(); if (destroyed) { await unaffectedWave; return; } + if (filterResult.status === 'superseded') { + // A concurrent selective commit superseded this refresh's Filter wave + // and is now the source-of-truth; IT will run the affected panels + // after it settles + reconciles (`commitAndRerun` -> `runAffectedWave`). + // Running them here would execute panels before the current source + // updates/reconciliation are applied — the exact ordering this session + // must never allow — and would NOT be preempted by tile generations: + // the selective commit reserves its OWN tile generations only after + // its source wave settles (inside its own `runAffectedWave`), so during + // this window these tiles' generations are still whatever `refresh()` + // reserved up front and would run un-preempted. `refresh()`'s own work + // is done either way — the unaffected tiles already ran/are running, + // and the affected side is delegated to the superseding commit. + await unaffectedWave; + publish(false, destroyed ? null : deps.now()); + return; + } // Affected tiles run AFTER the filter wave, with the merged/blanked values. const secondBatch = sourcesById(prepareBatch('execute').sources); const affectedWave = runPool(affected, VIEWER_TILE_CONCURRENCY, diff --git a/tests/unit/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index 37828a0..211ca5b 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -1719,6 +1719,62 @@ describe('superseded/destroyed selective-wave guard (#360 review findings 1/2)', expect(t1Calls() - t1Before).toBe(1); }); + it("a full refresh()'s Filter wave superseded by a concurrent selective commit skips refresh's OWN affected-panel wave; the selective commit's runAffectedWave still runs it", async () => { + // Tile 't' references the ROOT parameter 'region' directly (so the later + // selective commit's own `runAffectedWave(['region'])` can target it + // without depending on reconciliation), and the source-backed filter + // 'f-city' explicitly `targets` it (so `refresh()`'s OWN + // `affectedByFilterWave` classification puts 't' in its "affected" + // bucket — waiting for the filter wave — rather than "unaffected"). + let releaseRefreshExec!: () => void; + const gateRefresh = new Promise((resolve) => { releaseRefreshExec = resolve; }); + let releaseCommitExec!: () => void; + const gateCommit = new Promise((resolve) => { releaseCommitExec = resolve; }); + let sourceCalls = 0; + const { exec, calls } = makeExec((sql) => { + if (!sql.includes('source')) return { columns: [{ name: 'n' }], rows: [[1]] }; + sourceCalls += 1; + return sourceCalls === 1 + ? gateRefresh.then(() => ({ columns: [{ name: 'city', type: 'Array(String)' }], rows: [[['stale']]] })) + : gateCommit.then(() => ({ columns: [{ name: 'city', type: 'Array(String)' }], rows: [[['fresh']]] })); + }); + const document = doc({ + tiles: [tile('t', 'qt')], + filters: [ + { id: 'region-root', parameter: 'region', defaultActive: true, defaultValue: 'v0' }, + { id: 'f-city', parameter: 'city', sourceQueryId: 'src', targets: ['t'] }, + ], + }); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT {region:String} AS n'), + query('src', "SELECT ['x'] AS city FROM t WHERE ts >= {region:String} /* source */", { dashboard: { role: 'filter' } }), + ], + })); + const tileCalls = () => calls.filter((c) => !c.sql.includes('source')).length; + + const refreshP = session.start(); // refresh()'s filter wave starts; its source exec is call #1, gated + await flush(); + const commit = session.setFilter('region-root', 'v1'); // supersedes 'src'; its OWN source exec is call #2, ALSO gated + await flush(); + expect(tileCalls()).toBe(0); // neither wave has reached its affected-panel phase yet + + releaseRefreshExec(); // refresh's held call resolves — but 'src' has since moved on; its plan is stale + await refreshP; + // The regression: refresh's OWN affected-panel wave must NOT have fired — + // its Filter wave settled `{status:'superseded'}`, and (pre-fix) this + // discarded result was ignored, so refresh ran its affected tile with + // pre-commit/pre-merge values while the selective commit was still loading. + expect(tileCalls()).toBe(0); + + releaseCommitExec(); // the selective commit's OWN call settles normally (not superseded) + await commit; + // Only the selective commit's own `runAffectedWave` (after it settled and + // reconciled) ran the affected tile — exactly once. + expect(tileCalls()).toBe(1); + }); + it('finding 2: destroy() during an in-flight selective source wave prevents its panel wave from ever firing', async () => { let releaseGate!: () => void; const gate = new Promise((resolve) => { releaseGate = resolve; }); From 6161d9c38a8a0e10c28b15c0cac41268c56dee50 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 19:24:55 +0000 Subject: [PATCH 16/16] docs(#360): trim review-history comments to stable invariants (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer review: several production comments documented review rounds, "finding N", and abandoned approaches rather than stable invariants — notably commitAndRerun's ~30-line description of the reverted commit-generation counter. Condensed the #360-introduced review narrative to present-tense invariants that link #360, leaving the review history in GitHub. commitAndRerun's doc comment: 49→17 lines. Comment-only (build byte-identical); comments belonging to other issues (#359/#170/#173/#276/#291/#321/#344/#347) left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/filter-execution.ts | 2 +- .../application/dashboard-viewer-session.ts | 100 ++++++------------ src/ui/dashboard.ts | 49 ++++----- src/ui/filter-bar.ts | 31 +++--- 4 files changed, 71 insertions(+), 111 deletions(-) diff --git a/src/core/filter-execution.ts b/src/core/filter-execution.ts index db8efbf..fbfb956 100644 --- a/src/core/filter-execution.ts +++ b/src/core/filter-execution.ts @@ -134,7 +134,7 @@ export interface FilterSourceAnalysis { * offending parameter. `analysis.diagnostics` (the shared pipeline's own * cross-declaration type-conflict findings, e.g. `{x:UInt8}` vs `{x:String}` * in the same source) are folded in too, each as its own - * `filter-source-param-type-conflict` diagnostic (#360 review finding 3) — + * `filter-source-param-type-conflict` diagnostic (#360) — * without this, a type-conflicted source would classify `runnable` in * `prepareFilterSource` and get sent. Pure. */ diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index 0ade117..626b7df 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -133,7 +133,7 @@ export interface ViewerFilterState { * names (from `FilterSourcePreparation.missing`) the filter-bar UI/ * diagnostics can name. Absent otherwise. */ waitingFor?: string[]; - /** #360 finding 4: the shared `FilterSourceRuntime.id` this filter is a + /** #360: the shared `FilterSourceRuntime.id` this filter is a * consumer of, set once at construction from the filter DEFINITION's * `sourceQueryId` — undefined for a plain root filter (no source at all). * This is TOPOLOGY, not transport state (unlike `status`/`stale`, it never @@ -812,17 +812,14 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa return provider; } - /** #360 findings 1+2: the terminal result of one source-wave pass — either - * every source in the plan settled at ITS OWN reserved generation and the - * merge ran (`'applied'`, carrying `merged.changed` as `flipped`), or the - * plan was stale (superseded by a later wave that reserved a fresh - * generation on one of the SAME sources) and nothing was published - * (`'superseded'`). Before this type existed both cases returned `[]`, - * which made "ran, nothing flipped" indistinguishable from "this whole - * wave was discarded" — so a superseded selective source wave still went - * on to launch its own panel wave in `commitAndRerun`. `runFilterWave` - * ignores the return either way (a full refresh has no further phase to - * gate). */ + /** #360: the terminal result of one source-wave pass — either every source + * in the plan settled at ITS OWN reserved generation and the merge ran + * (`'applied'`, carrying `merged.changed` as `flipped`), or the plan was + * stale (superseded by a later wave that reserved a fresh generation on + * one of the SAME sources) and nothing was published (`'superseded'`) — a + * superseded wave must not go on to launch its own panel wave in + * `commitAndRerun`. `runFilterWave` ignores the return either way (a full + * refresh has no further phase to gate). */ type SourceWaveResult = { status: 'applied'; flipped: string[] } | { status: 'superseded' }; /** Terminal, SYNCHRONOUS step of the filter wave (#359): merges every @@ -864,8 +861,8 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // renders diagnostics, not per-filter status, so a bare `missing-helper` // left an unexplained empty control). const diagnostics: FilterDiagnostic[] = [...merged.diagnostics]; - // #360 BLOCKER-1 (cross-source race): a source NOT in THIS plan that is - // still `loading` (a different, possibly-overlapping selective wave is + // #360: a source NOT in THIS plan (a cross-source race) that is still + // `loading` (a different, possibly-overlapping selective wave is // running it right now) or settled `waiting` on its own dependency must // not have its consumers re-derived here — its own wave's // `applyFilterProviders` call owns that. Every source that IS in this @@ -931,10 +928,10 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa return { status: 'applied', flipped: merged.changed }; } - /** Finding 6 (dedup): the executor shared by `runFilterWave` (every known - * source, a full refresh) and `runFilterSourceWave` (only the sources a - * just-committed root parameter affects, #360) — both used to duplicate - * the same five steps: supersede each source into a plan, flip loading/ + /** The executor shared by `runFilterWave` (every known source, a full + * refresh) and `runFilterSourceWave` (only the sources a just-committed + * root parameter affects, #360) — avoiding duplicating the same five + * steps: supersede each source into a plan, flip loading/ * stale on the source AND every consumer, decide whether to clear * consumer options, `publish()` once, run the plan through the bounded * pool, then merge terminally via `applyFilterProviders`. The two callers @@ -1101,9 +1098,9 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // Re-run only the tiles some active filter parameter feeds into. async function runAffectedWave(parameters: string[], preflighted = false): Promise { - // Finding 2: unconditional destroyed guard. The `preflighted: true` fast - // path (from `commitAndRerun`'s affected branch) skips `preflight()` - // entirely below — and `preflight()` was the ONLY place on this path that + // Unconditional destroyed guard: the `preflighted: true` fast path (from + // `commitAndRerun`'s affected branch) skips `preflight()` entirely below + // — and `preflight()` was the ONLY place on this path that // checked `destroyed` — so without this a `destroy()` firing between the // source wave settling and this wave starting could still reserve tile // generations and issue requests after teardown. @@ -1125,53 +1122,22 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // #360: after committing a value (these four are commit paths only — never // in-progress typing), rerun any shared Filter source that depends on the - // just-committed root parameter(s) FIRST, then fold both the committed - // parameter(s) AND any names `runFilterSourceWave` itself deactivated - // (`result.flipped`, e.g. a now-out-of-range curated value elsewhere) into - // ONE combined affected-panel wave — never two separate waves. A synchronous - // pre-check skips `runFilterSourceWave` (and the microtask hop awaiting it - // costs) entirely when nothing depends on `changed` — behaviorally - // identical to awaiting it (an unaffected wave always resolves to - // `{status:'applied', flipped:[]}` with no side effect), just without the - // extra tick for the overwhelmingly common case of a Dashboard with no - // parameterized Filter sources (#360). - // The fast (no-affected-source) path lets `runAffectedWave` self-preflight - // exactly as before (one await, unchanged timing); the affected path - // preflights ONCE here for the whole commit and passes `preflighted: true` - // into both waves — otherwise a stale token would fail `ensureFreshToken()` - // (and fire `onAuthFailed`) twice for one commit. + // just-committed root parameter(s), then run ONE combined affected-panel + // wave over both the committed parameter(s) and any names the source wave + // itself deactivated (`result.flipped`). A synchronous pre-check skips + // `runFilterSourceWave` entirely when nothing depends on `changed` — + // behaviorally identical to awaiting it, since an unaffected wave always + // resolves to `{status:'applied', flipped:[]}` with no side effect. The + // fast (no-affected-source) path lets `runAffectedWave` self-preflight as + // usual; the affected path preflights ONCE here for the whole commit and + // passes `preflighted: true` into both waves so a stale token only fails + // `ensureFreshToken()` once. // - // Findings 1+2 (maintainer review): a superseded or destroyed source wave - // must not go on to launch its own panel wave. Both are already caught by - // checks that exist independently of any commit-to-commit bookkeeping here - // — no extra "commit generation" counter is needed (an earlier version of - // this fix added one bumped on EVERY `commitAndRerun` call, including the - // no-affected-source fast path; that made two commits affecting DIFFERENT, - // non-overlapping sources spuriously supersede one another — a legitimate - // concurrent commit's own, correctly-`'applied'` result would still get - // discarded and its panel wave skipped just because an unrelated commit - // happened to be in flight at the same time. Reverted: see PR review): - // - Superseded (finding 1): `result.status === 'superseded'` already - // covers every real case. Two commits overlapping on the SAME (or a - // cascading-dependent) source: the later one's `runFilterSourceWave` - // calls `supersede()` on that shared `FilterSourceRuntime`, reserving a - // fresh generation; when the earlier commit's held request finally - // settles, its `applyFilterProviders` plan no longer matches that - // generation, so it returns `{status:'superseded'}` — see - // `applyFilterProviders`'s own stale-wave guard. A concurrent - // UNRELATED full `refresh()` supersedes EVERY known source (not just - // the ones a param change affects), so it catches this too, the same - // way. Two commits affecting genuinely DIFFERENT, non-dependent sources - // never touch each other's generations, so neither is ever marked - // superseded — exactly the case that must still run its own panel wave. - // - Destroyed (finding 2): `destroy()` bumps every source's (and every - // tile's) generation too, so an in-flight source wave racing a - // `destroy()` is ALSO naturally caught by the same `'superseded'` check - // above; the explicit `destroyed` check here plus the new unconditional - // guard at the top of `runAffectedWave` close the remaining gap — the - // `preflighted: true` fast path into `runAffectedWave` (the only caller - // that skips `preflight()`, previously the only `destroyed` check on - // this path) reachable right after the two checks below pass. + // The affected-panel wave runs only if the source wave was neither + // superseded nor destroyed: a superseded result changed nothing + // (`applyFilterProviders`'s own stale-wave guard already discarded it), and + // `destroy()` bumps every source generation too, so a source wave racing a + // `destroy()` is caught by the same check. async function commitAndRerun(changed: string[]): Promise { const hasAffectedSource = [...filterSources.values()] .some((source) => source.analyzed.dependsOn.some((name) => changed.includes(name))); diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index 2f53bfd..8932421 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -591,23 +591,22 @@ export async function renderDashboard(app: DashboardApp): Promise { wallNow: () => app.wallNow(), }; let filterBarDispose: (() => void) | null = null; - // #360 maintainer-review follow-up: the retained bar's `updateStatus` — a - // status-only publish (below, the `barSig`/status-signal split) calls this - // directly instead of tearing down and rebuilding the whole bar. + // #360: the retained bar's `updateStatus` — a status-only publish (below, + // the `barSig`/status-signal split) calls this directly instead of tearing + // down and rebuilding the whole bar. let filterBarUpdateStatus: FilterBarHandle['updateStatus'] | null = null; function rebuildFilterBar(sview: DashboardViewState): void { filterBarDispose?.(); const idByParam = new Map(); - // #360 maintainer-review follow-up: curation is gated on TOPOLOGY - // (`sourceId != null`, set once at construction from the filter - // definition's `sourceQueryId`), never on the transient `status` — status - // is execution state, not topology. The old `f.status !== 'idle'` gate - // meant a source-backed filter rendered as a bare, enabled plain-text - // control on INITIAL load (status starts 'idle' before its source has - // even run) until the source settled — the maintainer-review finding this - // fixes. A plain (non-source-backed) filter has no `sourceId` and is - // never gated into this path. + // #360: curation is gated on TOPOLOGY (`sourceId != null`, set once at + // construction from the filter definition's `sourceQueryId`), never on + // the transient `status` — status is execution state, not topology. A + // source-backed filter starts `status: 'idle'` before its source has even + // run, so gating curation on status instead would render it as a bare, + // enabled plain-text control until the source settled. A plain + // (non-source-backed) filter has no `sourceId` and is never gated into + // this path. const curatedFields: Record; status: ViewerFilterState['status']; @@ -1555,10 +1554,10 @@ export async function renderDashboard(app: DashboardApp): Promise { // silently skipping that cleanup. let lastEngineRendered: 'flow' | 'grafana-grid' | null = null; let barSig = ''; - // #360 maintainer-review follow-up: a SEPARATE signature from `barSig` — - // status/stale/waitingFor no longer participate in `barSig` at all (see the - // effect below), so a status-only change is detected here instead and - // applied to the EXISTING bar via `filterBarUpdateStatus` (no rebuild). + // #360: a SEPARATE signature from `barSig` — status/stale/waitingFor never + // participate in `barSig` (see the effect below), so a status-only change + // is detected here instead and applied to the EXISTING bar via + // `filterBarUpdateStatus` (no rebuild). let statusSig = ''; const statusSigOf = (filters: readonly ViewerFilterState[]): string => JSON.stringify(filters.map((f) => [f.parameter, f.status, !!f.stale, f.waitingFor ?? null])); @@ -1598,17 +1597,13 @@ export async function renderDashboard(app: DashboardApp): Promise { // committed value, curated option CONTENT arriving/changing via // `optionsRev`, or a filter gaining/losing its source topology) — not on // a bare status flip and not on tile progress ticks — so in-progress - // typing is never disturbed mid-wave. #360 maintainer-review follow-up: - // `status`/`stale`/`waitingFor` are deliberately EXCLUDED from this - // signature (they used to be, for a "sticky" subset of statuses, to force - // the `waiting`/error affordance to paint — but that patched over the - // real bug: curation itself gated on status, so an idle-or-loading - // source-backed filter fell out of the curated path entirely). Now that - // `rebuildFilterBar` gates curation on topology (`sourceId != null`) - // rather than status, a pure status change only needs its OWN existing - // curated DOM updated in place — see `statusSig` below — never a rebuild. - // This also preserves #359's own invariant (verified by its own suite) - // that an unchanged republish never disturbs in-progress typing. + // typing is never disturbed mid-wave. `status`/`stale`/`waitingFor` are + // deliberately EXCLUDED from this signature (#360): `rebuildFilterBar` + // gates curation on topology (`sourceId != null`), not status, so a pure + // status change only needs its OWN existing curated DOM updated in place + // — see `statusSig` below — never a rebuild. This also preserves #359's + // own invariant that an unchanged republish never disturbs in-progress + // typing. const sig = JSON.stringify(sview.filters.map((f) => [f.id, f.active, valueString(f.value), f.optionsRev, f.sourceId != null])); const newStatusSig = statusSigOf(sview.filters); diff --git a/src/ui/filter-bar.ts b/src/ui/filter-bar.ts index fa91fa3..aecf4ac 100644 --- a/src/ui/filter-bar.ts +++ b/src/ui/filter-bar.ts @@ -53,15 +53,15 @@ export interface BuildFilterBarOptions { curatedFields?: Record; } -/** #360 maintainer-review follow-up: `status`/`stale`/`waitingFor` mirror - * `ViewerFilterState`'s own fields (dashboard-viewer-session.ts) — but - * WHETHER a filter is curated at all is `dashboard.ts`'s `rebuildFilterBar` - * gating on `f.sourceId != null` (topology, set once at construction), - * never on this transient status. Status is execution state, not topology: - * a source-backed filter starts `status: 'idle'` before its source has even - * run, so gating curation on "not idle" used to render it as a bare, - * enabled plain-text control on initial load until the source settled. - * These three fields are only the AFFORDANCE this already-curated field +/** #360: `status`/`stale`/`waitingFor` mirror `ViewerFilterState`'s own + * fields (dashboard-viewer-session.ts) — but WHETHER a filter is curated at + * all is `dashboard.ts`'s `rebuildFilterBar` gating on `f.sourceId != null` + * (topology, set once at construction), never on this transient status. + * Status is execution state, not topology: a source-backed filter starts + * `status: 'idle'` before its source has even run, so gating curation on + * status instead would render it as a bare, enabled plain-text control + * until the source settled. These three fields are only the AFFORDANCE this + * already-curated field * shows — all optional so a caller that never supplies them (an older/ * simpler fixture) renders exactly like today's plain 'ready' combobox. */ export interface CuratedFieldStatus { @@ -77,7 +77,7 @@ interface CuratedFieldConfig extends CuratedFieldStatus { options: FilterFieldOption[]; } -/** A built curated field's retained handle (#360 follow-up) — kept in +/** A built curated field's retained handle (#360) — kept in * `buildFilterBar`'s `curatedHandles` map so a LATER status-only change can * update this exact field's affordance in place (`applyFieldStatus`, via * the returned `updateStatus`) without rebuilding the whole input — the @@ -87,8 +87,7 @@ interface CuratedFieldConfig extends CuratedFieldStatus { * (`applyFieldStatus` restores them once a status stops overriding either); * `noteEl` is the "Waiting for: …" node, present in the DOM only while * `status === 'waiting'` (created/removed on demand, not just hidden, so a - * caller that queries for it — same as the pre-existing per-status build - * tests — sees exactly what it saw before this change). */ + * caller that queries for it gets exactly the DOM it expects). */ interface CuratedFieldHandle { input: HTMLInputElement; label: HTMLElement; @@ -99,7 +98,7 @@ interface CuratedFieldHandle { /** * Applies a curated field's status affordance to its already-built DOM - * (#360 follow-up) — the SAME class/disabled/note logic `buildFilterBar` + * (#360) — the SAME class/disabled/note logic `buildFilterBar` * used to inline once per field build, factored out so both the initial * build (a fresh rebuild must show the right affordance immediately) and a * later `updateStatus` call (no rebuild) share one recipe. See @@ -193,7 +192,7 @@ export const FILTER_DEBOUNCE_MS = 500; * teardown — so an in-flight debounce never fires against a detached field * (the orphan-timer gap a bare `replaceChildren` rebuild used to leave). * - * #360 follow-up: `updateStatus` applies a per-param `CuratedFieldStatus` + * #360: `updateStatus` applies a per-param `CuratedFieldStatus` * update to whichever curated fields this SAME bar instance already built * (`curatedHandles`, keyed by parameter) — a param this bar never curated * (absent from `curatedFields` at build time, or a plain field) is silently @@ -242,7 +241,7 @@ export function buildFilterBar( return { el: h('div', { ...attrs, style: { display: 'none' } }), dispose: () => {}, updateStatus: () => {} }; } const timerClears: Array<() => void> = []; - // #360 follow-up: every curated field's retained handle, keyed by + // #360: every curated field's retained handle, keyed by // parameter — see `CuratedFieldHandle` and `FilterBarHandle.updateStatus`. const curatedHandles = new Map(); const el = h('div', attrs, ...params.map((p) => { @@ -290,7 +289,7 @@ export function buildFilterBar( const handle: CuratedFieldHandle = { input: field.input, label, baseTitle, basePlaceholder: field.input.placeholder, noteEl: null, }; - // #360 follow-up: apply the CURRENT status immediately at build time + // #360: apply the CURRENT status immediately at build time // (a fresh rebuild shows the right affordance right away) and retain // the handle so a LATER status-only change updates this same field in // place via `updateStatus`, never a rebuild.