diff --git a/CHANGELOG.md b/CHANGELOG.md index 93cc30b..da9a103 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,43 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] +### Fixed +- **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 + provider by the filter-definition id — so `mergeDashboardFilterHelpers` + rejected every helper as a duplicate provider, every control rendered empty, + and the explanatory diagnostics were silently discarded. The filter runtime + is now split into per-definition state and one `FilterSourceRuntime` per + unique `sourceQueryId`: the source SQL executes exactly once per refresh wave + no matter how many filters/parameters it feeds, the provider is keyed by the + saved source-query id (two DISTINCT sources providing the same helper still + correctly collide with no arbitrary winner), and the merge's diagnostics + (info/warning/error, severity preserved) are now published to the Dashboard + instead of dropped — including a `filter-helper-missing` warning (naming the + source and column) when a source succeeds but omits a consumer's helper, so + that filter no longer renders as an unexplained empty control. Each source + also retains its last-known provider on its runtime and the merge reads the + complete set, so a future selective wave (#360) can re-merge unaffected + sources without clearing or rerunning them. This is the one-source-runtime + boundary #360 builds on. + +### Changed +- **Dashboard Filter execution no longer injects `readonly`** (#359). Server + read-only policy belongs to ClickHouse user/profile configuration, not + Dashboard/Workbench feature code; `filterExecution` keeps only its + result-format/byte-cap transport settings. Panel execution is unchanged. +- **Filter option state is honest on failure** (#359). A source failure, + missing helper, type conflict, or duplicate-provider collision now CLEARS a + filter's curated options (with a visible diagnostic) instead of silently + retaining stale ones, and an unresolvable `sourceQueryId` surfaces a visible + `source-error` rather than a silent plain field. An active value no longer + present in refreshed options is deactivated (its value kept) before dependent + panels run; a non-empty → different-non-empty option replacement now updates + the rendered combobox (a per-filter option revision folded into the + filter-bar rebuild signature); and a superseded refresh wave can no longer + publish options or activation over a fresher one. + ## [0.6.1] - 2026-07-21 ### Changed diff --git a/src/core/filter-execution.ts b/src/core/filter-execution.ts index 092484f..daf59ae 100644 --- a/src/core/filter-execution.ts +++ b/src/core/filter-execution.ts @@ -79,7 +79,6 @@ export function filterExecution(sql?: string | null, defaults: FilterExecutionDe format: 'Filter', rowLimit: FILTER_TOP_LEVEL_ROW_LIMIT, params: { - readonly: 2, max_result_bytes: FILTER_RESULT_BYTE_CAP, output_format_json_named_tuples_as_objects: 1, output_format_json_quote_64bit_integers: 1, diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index e87a353..6325f6c 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -38,7 +38,9 @@ import { panelExecution } from '../../core/panel-execution.js'; import { filterExecution } from '../../core/filter-execution.js'; import { readFilterOptions } from '../../core/filter-options.js'; import { mergeDashboardFilterHelpers } from '../../core/dashboard-filters.js'; -import type { FilterProvider, FilterHelperOption, MergeDashboardFilterHelpersResult } from '../../core/dashboard-filters.js'; +import type { + FilterProvider, FilterHelperOption, FilterDiagnostic, MergeDashboardFilterHelpersResult, +} from '../../core/dashboard-filters.js'; import { diagnostic as coreDiagnostic } from '../../core/diagnostics.js'; import { newResult } from '../../core/stream.js'; import type { StreamResult } from '../../core/stream.js'; @@ -87,7 +89,19 @@ export interface ViewerTileState { progressRows: number; } -export type ViewerFilterStatus = 'idle' | 'loading' | 'error' | 'success'; +/** A filter's own transport/curation status — derived per DEFINITION (not per + * source runtime) after the shared source's merge: + * - `idle`/`loading` mirror the source's transport state directly. + * - `ready` — the merge produced a curated field for this filter's parameter. + * - `missing-helper` — the source succeeded but never returned this column + * (no merge/source diagnostic names it either). + * - `helper-error` — the source succeeded but a merge/source diagnostic + * names this exact parameter as the failing helper (duplicate provider, + * invalid option, unused, …). + * - `source-error` — the shared source query itself failed (missing query, + * invalid SQL, transport/exec error). + * A plain filter (no `sourceQueryId`) never leaves `idle`. */ +export type ViewerFilterStatus = 'idle' | 'loading' | 'ready' | 'missing-helper' | 'helper-error' | 'source-error'; /** One Dashboard filter's runtime state. */ export interface ViewerFilterState { @@ -99,6 +113,11 @@ export interface ViewerFilterState { status: ViewerFilterStatus; /** Curated options from the filter's source query, when it has one. */ options: FilterHelperOption[] | null; + /** Bumped whenever `options`' VALUE CONTENT changes (including a clear to + * `null`) — never on an unchanged re-publish. The UI folds this into its + * 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; } /** The Dashboard's per-render layout view (#291) — a discriminated union over @@ -121,6 +140,12 @@ export interface DashboardViewState { updatedAt: number | null; /** Presentation/structural diagnostics that make a tile invalid. */ diagnostics: WorkspaceDiagnostic[]; + /** Every diagnostic the LAST filter wave's shared-source merge produced + * (info/warning/error, #359) — published as-is, never deduped (a shared + * source now runs exactly once per wave, so its diagnostics already + * appear once by construction). Reset to `[]` at the start of each wave; + * a pre-wave publish (session construction) also reads `[]`. */ + filterDiagnostics: FilterDiagnostic[]; } // ── Narrow injected dependencies (no App / AppState / net imports) ──────────── @@ -248,13 +273,39 @@ interface TileRuntime { state: ViewerTileState; } +/** One per Dashboard filter DEFINITION. Carries no transport state of its own + * (#359) — `sourceId` (when it resolves) points at the shared + * `FilterSourceRuntime` that actually runs the query; a plain filter (no + * `sourceQueryId`) has no `sourceId` and is never a consumer of any source. */ interface FilterRuntime { def: DashboardFilterDefinitionV1; - source: SavedQueryV2 | undefined; + sourceId?: string; + state: ViewerFilterState; +} + +/** One per UNIQUE `sourceQueryId` (#359) — N filter definitions sharing the + * same source query share exactly ONE of these, so the source SQL executes + * once per wave no matter how many filters/parameters it feeds. `status` is + * the TRANSPORT status only (idle/loading/ready/error) — never a per-filter + * curation outcome; per-consumer `ViewerFilterStatus` is derived from this + * plus the merge result in `applyFilterProviders`. `query` is `undefined` + * when `sourceQueryId` does not resolve against `queryById` — that still + * gets a runtime (so every consumer sees a visible `source-error`), it just + * never executes. `provider` is the source's LAST-KNOWN normalized + * contribution — `runFilterSource` updates it only for the current + * generation, and `applyFilterProviders` merges the COMPLETE set from every + * source runtime (not just the sources a wave re-ran). That is the + * source-runtime boundary #360 needs: a selective wave that reruns only the + * Filter sources a changed root parameter feeds can retain and re-merge every + * unaffected source's helpers instead of clearing them or rerunning all. */ +interface FilterSourceRuntime { + id: string; + query: SavedQueryV2 | undefined; + consumers: FilterRuntime[]; gen: number; abortController: AbortController | null; + status: 'idle' | 'loading' | 'ready' | 'error'; provider: FilterProvider | null; - state: ViewerFilterState; } const cfgType = (panel: unknown): string | undefined => @@ -354,7 +405,6 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // Filter runtime records, in filter order. const filters: FilterRuntime[] = (Array.isArray(documentRef.filters) ? documentRef.filters : []).map((def) => { - const source = typeof def.sourceQueryId === 'string' ? queryById.get(def.sourceQueryId) : undefined; const defaultValue = def.defaultValue ?? ''; const defaultActive = def.defaultActive ?? (def.defaultValue != null && def.defaultValue !== ''); // #303: a persisted seed for this filter's id overrides the pure-default @@ -365,12 +415,33 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa const active = seed !== undefined ? !!seed.active : defaultActive; const state: ViewerFilterState = { id: def.id, parameter: def.parameter, label: def.label || def.parameter, - active, value, status: 'idle', options: null, + active, value, status: 'idle', options: null, optionsRev: 0, }; - return { def, source, gen: 0, abortController: null, provider: null, state }; + const sourceId = typeof def.sourceQueryId === 'string' ? def.sourceQueryId : undefined; + return { def, sourceId, state }; }); const filterById = new Map(filters.map((filter) => [filter.def.id, filter])); + // 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 + // as a duplicate). A missing query still gets a runtime (`query: undefined`) + // so every consumer sees a visible `source-error` rather than being + // silently skipped. + const filterSources = new Map(); + for (const filter of filters) { + if (!filter.sourceId) continue; + let source = filterSources.get(filter.sourceId); + if (!source) { + source = { + id: filter.sourceId, query: queryById.get(filter.sourceId), + consumers: [], gen: 0, abortController: null, status: 'idle', provider: null, + }; + filterSources.set(filter.sourceId, source); + } + source.consumers.push(filter); + } + // Parameter analysis over the tile SQL — fixed for the session (structure // only). Text tiles and missing-query tiles contribute empty SQL. const analysis: ParameterAnalysis = analyzeParameterizedSources(tiles.map((runtime) => ({ @@ -398,6 +469,12 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // Curated option bundles from the last filter wave (param name → field). let curated: MergeDashboardFilterHelpersResult['fields'] = {}; + // The last filter wave's merge diagnostics (#359) — a closure var like + // `curated`, reset to `[]` at the START of `runFilterWave` and set (as-is, + // no dedupe) in `applyFilterProviders`. `buildState` reads it on every + // publish, so tile-progress publishes mid-wave carry the PREVIOUS wave's + // diagnostics and a pre-wave publish (construction) sees `[]`. + let filterDiagnostics: FilterDiagnostic[] = []; const rawValues = (): Record => Object.fromEntries(filters.map((filter) => [filter.def.parameter, toValueString(filter.state.value)])); @@ -452,7 +529,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa filters: filters.map((filter) => ({ ...filter.state })), layout, activeFilterCount: filters.filter((filter) => filter.state.active).length, - running, updatedAt, diagnostics: presentationDiagnostics, + running, updatedAt, diagnostics: presentationDiagnostics, filterDiagnostics, }; } @@ -556,68 +633,181 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // ── Filter wave ───────────────────────────────────────────────────────── - async function runFilterSource(filter: FilterRuntime, generation: number): Promise { - // `!`: only called for filters whose source query is present. - const query = filter.source!; + /** Serializes an options list to a comparable signature — `null` and `[]` + * 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])); + } + + /** Replace one consumer's curated options, bumping `optionsRev` ONLY when + * the VALUE CONTENT actually changed (#359) — a same-content republish (or + * an already-null filter staying null) leaves `optionsRev` untouched so + * the UI's filter-bar rebuild signature doesn't churn on every wave. */ + function setConsumerOptions(consumer: FilterRuntime, options: FilterHelperOption[] | null): void { + if (optionsSignature(options) !== optionsSignature(consumer.state.options)) consumer.state.optionsRev++; + consumer.state.options = options; + } + + /** 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 { + if (!source.query) { + // REUSE the static-validation code `filter-source-missing` + // (workspace-semantics.ts) — this is the RUNTIME analog: the source + // query id simply does not resolve against this session's queries. + const provider: FilterProvider = { + sourceId: source.id, sourceName: '', helpers: [], + diagnostics: [coreDiagnostic('error', 'filter-source-missing', + `Filter references unknown source query ${JSON.stringify(source.id)}`, { sourceId: source.id })], + }; + if (source.gen !== generation) return null; + source.status = 'error'; + source.provider = provider; + return provider; + } + const query = source.query; const execution = filterExecution(query.sql); if (execution.error) { const provider: FilterProvider = { - sourceId: filter.def.id, sourceName: queryName(query), helpers: [], diagnostics: execution.diagnostics, + sourceId: source.id, sourceName: queryName(query), helpers: [], diagnostics: execution.diagnostics, }; - if (filter.gen !== generation) return null; - filter.state.status = 'error'; - filter.provider = provider; + if (source.gen !== generation) return null; + source.status = 'error'; + source.provider = provider; return provider; } - if (filter.gen !== generation) return null; - filter.state.status = 'loading'; + if (source.gen !== generation) return null; const result = newResult(execution.format, execution.rowLimit); const controller = new AbortController(); - filter.abortController = controller; + source.abortController = controller; await deps.exec.executeRead(result, { sql: query.sql, format: execution.format, rowLimit: execution.rowLimit, params: execution.params, signal: controller.signal, }); - if (filter.gen !== generation) return null; - filter.abortController = null; + if (source.gen !== generation) return null; + source.abortController = null; let provider: FilterProvider; if (result.error || result.cancelled) { provider = { - sourceId: filter.def.id, sourceName: queryName(query), helpers: [], + sourceId: source.id, sourceName: queryName(query), helpers: [], diagnostics: [coreDiagnostic('error', 'filter-query-failed', - `${queryName(query)}: ${result.error || 'Filter query was cancelled.'}`, { sourceId: filter.def.id })], + `${queryName(query)}: ${result.error || 'Filter query was cancelled.'}`, { sourceId: source.id })], }; - filter.state.status = 'error'; + source.status = 'error'; } else { const normalized = readFilterOptions({ columns: result.columns, row: result.rows[0], rowCount: result.rows.length, }); - provider = { sourceId: filter.def.id, sourceName: queryName(query), ...normalized }; - filter.state.status = normalized.helpers.length ? 'success' : 'error'; + provider = { sourceId: source.id, sourceName: queryName(query), ...normalized }; + source.status = normalized.helpers.length ? 'ready' : 'error'; } - filter.provider = provider; + source.provider = provider; return provider; } - function applyFilterProviders(providers: (FilterProvider | null)[]): void { + /** 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 + * (or clears them), publishes the merge's diagnostics as-is, and + * 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 { + // 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`), + // but this terminal step iterates EVERY source unconditionally — so + // without this check a stale wave would still clobber every consumer's + // options/status and blank `filterDiagnostics` over the fresher wave's + // 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; + // 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. const merged = mergeDashboardFilterHelpers({ - providers: providers.filter((provider): provider is FilterProvider => provider !== null), + providers: [...filterSources.values()] + .map((source) => source.provider) + .filter((provider): provider is FilterProvider => provider !== null), controls, values: rawValues(), active: effectiveActive(rawValues(), activeMap()), }); curated = merged.fields; - // Publish curated options onto each filter whose parameter got a field. - for (const filter of filters) { - const field = merged.fields[filter.def.parameter]; - filter.state.options = field ? field.options : filter.state.options; + // Published as-is (never deduped — a shared source runs once per wave), plus + // one per-consumer `filter-helper-missing` for the otherwise-silent case + // where a source succeeds but omits a consumer's column (finding: the UI + // renders diagnostics, not per-filter status, so a bare `missing-helper` + // left an unexplained empty control). + const diagnostics: FilterDiagnostic[] = [...merged.diagnostics]; + for (const source of filterSources.values()) { + for (const consumer of source.consumers) { + if (source.status === 'error') { + setConsumerOptions(consumer, null); + consumer.state.status = 'source-error'; + continue; + } + const field = merged.fields[consumer.def.parameter]; + if (field) { + setConsumerOptions(consumer, field.options); + consumer.state.status = 'ready'; + continue; + } + // The source succeeded but this consumer's parameter never got a + // curated field — either a merge/source diagnostic explicitly names + // it (helper-error: duplicate provider, unused, invalid option, …) + // or the source simply never returned that column (missing-helper). + // Simple helperName match only — no code enumeration, no sourceId key. + const namedByDiagnostic = merged.diagnostics.some((diag) => diag.helperName === consumer.def.parameter); + setConsumerOptions(consumer, null); + if (namedByDiagnostic) { + consumer.state.status = 'helper-error'; + } else { + consumer.state.status = 'missing-helper'; + diagnostics.push(coreDiagnostic('warning', 'filter-helper-missing', + `Filter source ${JSON.stringify(source.query ? queryName(source.query) : source.id)} returned no "${consumer.def.parameter}" option column.`, + { sourceId: source.id, helperName: consumer.def.parameter })); + } + } + } + filterDiagnostics = diagnostics; + // 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. + for (const parameter of merged.changed) { + for (const filter of filters) if (filter.def.parameter === parameter) filter.state.active = false; } } async function runFilterWave(): Promise { - const sourced = filters.filter((filter) => filter.source); - const plan = sourced.map((filter) => ({ filter, generation: supersede(filter) })); - const providers = await runPool(plan, VIEWER_TILE_CONCURRENCY, - ({ filter, generation }) => runFilterSource(filter, generation)); - applyFilterProviders(providers); + filterDiagnostics = []; + const sources = [...filterSources.values()]; + const plan = sources.map((source) => ({ source, generation: supersede(source) })); + for (const { source } of plan) { + 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'; + } + 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); } @@ -773,9 +963,9 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa runtime.gen++; if (runtime.abortController) { runtime.abortController.abort(); runtime.abortController = null; } } - for (const filter of filters) { - filter.gen++; - if (filter.abortController) { filter.abortController.abort(); filter.abortController = null; } + for (const source of filterSources.values()) { + source.gen++; + if (source.abortController) { source.abortController.abort(); source.abortController = null; } } } diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index 44b5a90..a886724 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -1566,7 +1566,7 @@ export async function renderDashboard(app: DashboardApp): Promise { // 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)])); + 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); } // #303: persist committed filter value/active into the isolated per-dashboard // store — isolated from the Workbench's asb:varValues/asb:filterActive keys. @@ -1584,6 +1584,10 @@ export async function renderDashboard(app: DashboardApp): Promise { // leaves its target tiles in their normal unfilled state. filterDiagnosticsHost.replaceChildren( ...sview.diagnostics.map((d) => h('div', { class: 'dash-config-diagnostic is-error' }, d.message)), + // #359: the shared-source filter wave's own merge diagnostics + // (info/warning/error), separate from the presentation diagnostics + // above — each severity maps directly to its `is-*` class. + ...sview.filterDiagnostics.map((d) => h('div', { class: 'dash-config-diagnostic is-' + d.severity }, d.message)), ); if (sview.layout.engine !== lastEngineRendered) { lastLayoutSig = ''; lastGridSig = ''; lastEngineRendered = sview.layout.engine; } activeEngine = sview.layout.engine; diff --git a/tests/e2e/filter-source.html b/tests/e2e/filter-source.html index cb04bd7..e55212c 100644 --- a/tests/e2e/filter-source.html +++ b/tests/e2e/filter-source.html @@ -4,6 +4,16 @@ Filter source harness + +