diff --git a/CHANGELOG.md b/CHANGELOG.md index da9a103..fe68118 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,39 @@ 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 (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 + `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..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 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", 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/core/filter-execution.ts b/src/core/filter-execution.ts index daf59ae..fbfb956 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.')); } @@ -72,21 +71,178 @@ 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, }; } + +// #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. `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) — + * without this, a type-conflicted source would classify `runnable` in + * `prepareFilterSource` and get sent. 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.`, + )); + } + } + 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: + * `'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` (`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. + */ +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 params = { ...filterOwnedParams(), ...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/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index 6325f6c..626b7df 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,24 @@ 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[]; + /** #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 + * 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 @@ -304,8 +327,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 => @@ -413,15 +446,25 @@ 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])); + // #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 +476,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 +529,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 +701,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 +715,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 +748,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,11 +804,24 @@ 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; } + /** #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 * collected provider exactly ONCE, then for every consumer of every * source derives its `ViewerFilterStatus`, replaces its curated options @@ -721,8 +829,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). */ - function applyFilterProviders(plan: { source: FilterSourceRuntime; generation: number }[]): void { + * 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`), @@ -732,7 +844,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. @@ -749,17 +861,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: 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 + // 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 +903,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,31 +919,113 @@ 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 { status: 'applied', flipped: merged.changed }; } - async function runFilterWave(): Promise { - filterDiagnostics = []; - const sources = [...filterSources.values()]; + /** 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 + * 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'; + 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)); - 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"). 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 }); + } + + /** #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. 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 + // 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))); + 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 }); } @@ -840,9 +1058,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, @@ -863,8 +1097,15 @@ 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 { + // 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) { const field = analysis.fields[parameter]; @@ -879,6 +1120,39 @@ 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), 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. + // + // 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))); + if (!hasAffectedSource) { await runAffectedWave(changed); return; } + if (!(await preflight())) return; + const result = await runFilterSourceWave(changed); + // 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); + } + async function setFilter(filterId: string, value: unknown): Promise { if (destroyed) return; const filter = filterById.get(filterId); @@ -886,7 +1160,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 +1172,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 +1182,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 +1197,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/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/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/dashboard.ts b/src/ui/dashboard.ts index a886724..8932421 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,16 +591,35 @@ export async function renderDashboard(app: DashboardApp): Promise { wallNow: () => app.wallNow(), }; let filterBarDispose: (() => void) | null = null; + // #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(); - const curatedFields: Record = {}; + // #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']; + 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.sourceId != null) { + 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); @@ -610,6 +629,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' }); @@ -1534,6 +1554,16 @@ export async function renderDashboard(app: DashboardApp): Promise { // silently skipping that cleanup. let lastEngineRendered: 'flow' | 'grafana-grid' | null = null; let barSig = ''; + // #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])); + 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 => { @@ -1563,11 +1593,32 @@ export async function renderDashboard(app: DashboardApp): Promise { return; } 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])); - 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. `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); + 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 348dfee..aecf4ac 100644 --- a/src/ui/filter-bar.ts +++ b/src/ui/filter-bar.ts @@ -53,13 +53,101 @@ export interface BuildFilterBarOptions { curatedFields?: Record; } +/** #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 { + 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 { +interface CuratedFieldConfig extends CuratedFieldStatus { options: FilterFieldOption[]; } +/** 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 + * 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 gets exactly the DOM it expects). */ +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) — 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 @@ -102,10 +190,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: `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; } /** @@ -138,8 +237,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: 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; }); @@ -167,7 +271,6 @@ export function buildFilterBar( }, onCommit: () => onCommit(p.name), }); - field.input.title = baseTitle; // #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 +280,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. + // 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); - return h('label', { class: 'var-field is-curated' + (p.optional ? ' is-optional' : '') }, + 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: 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; @@ -250,5 +366,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/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..a754c29 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 @@ -254,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; } @@ -270,7 +292,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 +319,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 +343,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,8 +379,8 @@ 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 - : { ...hooks.sessionParamsFor(tab, [srcSql]), ...mergedSourceArgs(src), ...kpiExecution.params }, + ? filterPrep!.params + : { ...hooks.sessionParamsFor(tab, [srcSql]), ...mergedSourceArgs(src!), ...kpiExecution.params }, onChunk: () => hooks.renderResults(), }); } finally { @@ -414,8 +436,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/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'); + }); }); 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/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index 8ee1b23..211ca5b 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -1082,3 +1082,743 @@ 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("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) => { + 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 ensureFreshToken = vi.fn(async () => tokenOk); + const session = createDashboardViewerSession(makeDeps({ + document, exec, onAuthFailed, + 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' } }), + ], + })); + 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 + ensureFreshToken.mockClear(); + onAuthFailed.mockClear(); + // Committing 'from' makes srcFrom affected (it depends on 'from'), so + // 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(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']]] }; + 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). +}); + +// 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. 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 + // 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 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; + 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, 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 — 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("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; }); + 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'); + }); +}); diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index de29f7f..0c5ae20 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -2582,6 +2582,143 @@ describe('renderDashboard — filter-source runtime rebuild + diagnostics (#359) }); }); +// #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({ + 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'); + }); +}); + +// #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 9827afe..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', () => { @@ -182,6 +183,191 @@ describe('buildFilterBar (shared filter row)', () => { }); }); + // #360: a source-backed curated field now stays in this rich-combobox + // 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, { + 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); + }); + }); + + // #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 { diff --git a/tests/unit/filter-execution.test.ts b/tests/unit/filter-execution.test.ts index 7adc6e4..3888501 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,168 @@ 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([]); + }); + 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)', () => { + 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('#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; + 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); + }); }); 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();