From 919defd59d76f2e8aa3a11aaaa6e9a9d843064d1 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 14:30:53 +0200 Subject: [PATCH 1/3] refactor(#359): one FilterSourceRuntime per unique Filter source query Dashboard filters sharing one Filter-source query rendered empty: the viewer ran the source once per definition and keyed each provider by the filter-definition id, so mergeDashboardFilterHelpers rejected every helper as a duplicate provider and the diagnostics were silently discarded. Split the filter runtime 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, so two DISTINCT sources providing the same helper still collide with no arbitrary winner; - merge diagnostics (info/warning/error, severity preserved) are published to the Dashboard instead of dropped; - non-ready results CLEAR curated options (no silent stale retention); an unresolvable sourceQueryId surfaces a visible source-error; - an active value absent from refreshed options is deactivated (value kept) before dependent panels run; - a per-filter optionsRev drives the filter-bar rebuild on option-content change (fixes non-empty -> different-non-empty not updating the combobox); - a superseded refresh wave can no longer publish options/activation over a fresher one (stale-generation guard in the terminal merge step). filterExecution no longer injects readonly: server read-only policy belongs to ClickHouse user/profile config, not feature code (panel execution unchanged). This establishes the one-source-runtime boundary #360 builds on. Closes #359. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01UDsUDSPoDYa1M1rbpgdG3f --- CHANGELOG.md | 31 ++ src/core/filter-execution.ts | 1 - .../application/dashboard-viewer-session.ts | 236 ++++++++++++--- src/ui/dashboard.ts | 6 +- tests/e2e/filter-source.html | 37 +++ tests/e2e/filter-source.spec.js | 29 ++ tests/unit/app.test.ts | 4 +- tests/unit/dashboard-viewer-session.test.ts | 283 +++++++++++++++++- tests/unit/dashboard.test.ts | 79 +++++ tests/unit/filter-execution.test.ts | 3 +- 10 files changed, 664 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93cc30b1..4ce9d198 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,37 @@ 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. 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 092484f6..daf59ae6 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 e87a3535..60660659 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,33 @@ 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. No stored `provider` — `runFilterSource` RETURNS it; + * `runFilterWave` collects the returns and merges once. */ +interface FilterSourceRuntime { + id: string; + query: SavedQueryV2 | undefined; + consumers: FilterRuntime[]; gen: number; abortController: AbortController | null; - provider: FilterProvider | null; - state: ViewerFilterState; + status: 'idle' | 'loading' | 'ready' | 'error'; } const cfgType = (panel: unknown): string | undefined => @@ -354,7 +399,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 +409,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', + }; + 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 +463,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 +523,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 +627,155 @@ 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). Returns the `FilterProvider` the caller collects into ONE merge; + * `source.status` is set to the TRANSPORT terminal (`ready`/`error`) — + * per-consumer curation status is derived afterward in + * `applyFilterProviders`. Every stale-gen guard mirrors the tile runner: + * a superseded source's provider is discarded (returns `null`) rather than + * publishing a stale result. */ + 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'; + 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'; 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; 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 }[], providers: (FilterProvider | null)[]): 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; const merged = mergeDashboardFilterHelpers({ providers: providers.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; + filterDiagnostics = merged.diagnostics; // published as-is — never deduped. + 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); + consumer.state.status = namedByDiagnostic ? 'helper-error' : 'missing-helper'; + } + } + // 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) })); + 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(); const providers = await runPool(plan, VIEWER_TILE_CONCURRENCY, - ({ filter, generation }) => runFilterSource(filter, generation)); - applyFilterProviders(providers); + ({ source, generation }) => runFilterSource(source, generation)); + applyFilterProviders(plan, providers); } @@ -773,9 +931,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 44b5a90e..a8867240 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 cb04bd74..46f573ac 100644 --- a/tests/e2e/filter-source.html +++ b/tests/e2e/filter-source.html @@ -12,6 +12,7 @@
+
diff --git a/tests/e2e/filter-source.spec.js b/tests/e2e/filter-source.spec.js index 60da0af5..241be693 100644 --- a/tests/e2e/filter-source.spec.js +++ b/tests/e2e/filter-source.spec.js @@ -48,4 +48,33 @@ test.describe('Dashboard Filter sources', () => { await expect(input).toHaveValue('Atlanta'); expect(await page.evaluate(() => window.__selection)).toEqual({ value: 'ATL', active: true, commits: 1 }); }); + + test('a shared Filter source renders curated values for all configured filters (#359)', async ({ page }) => { + const shared = page.getByRole('main', { name: 'Dashboard filters sharing one source' }); + const userInput = shared.getByRole('combobox', { name: 'user' }); + const kindInput = shared.getByRole('combobox', { name: 'query_kind' }); + await expect(userInput).toHaveAttribute('placeholder', 'All'); + await expect(kindInput).toHaveAttribute('placeholder', 'All'); + + // Both fields built from the ONE shared source bundle render their own + // curated options — not empty controls (the #359 bug). + await userInput.fill('ali'); + await expect(shared.getByRole('option')).toHaveCount(1); + await expect(shared.getByRole('option')).toHaveText('Alice'); + await shared.getByRole('option').click(); + await expect(userInput).toHaveValue('Alice'); + expect(await page.evaluate(() => window.__userSel)).toEqual({ value: 'alice', active: true, commits: 1 }); + + await kindInput.fill('ins'); + await expect(shared.getByRole('option')).toHaveCount(1); + await expect(shared.getByRole('option')).toHaveText('Insert'); + await shared.getByRole('option').click(); + await expect(kindInput).toHaveValue('Insert'); + expect(await page.evaluate(() => window.__kindSel)).toEqual({ value: 'Insert', active: true, commits: 1 }); + + // Committing the second filter must not disturb the first — each + // consumer of the one shared source operates independently. + await expect(userInput).toHaveValue('Alice'); + expect(await page.evaluate(() => window.__userSel)).toEqual({ value: 'alice', active: true, commits: 1 }); + }); }); diff --git a/tests/unit/app.test.ts b/tests/unit/app.test.ts index 93dd4fac..016d96aa 100644 --- a/tests/unit/app.test.ts +++ b/tests/unit/app.test.ts @@ -714,7 +714,9 @@ describe('query run', () => { const request = asMock(app.conn.chCtx.fetch).mock.calls.find(([, init]) => /SELECT \['ATL'\]/.test(init.body))!; expect(request[0]).toContain('default_format=JSONEachRowWithProgress'); expect(request[0]).toContain('max_result_rows=2'); - expect(request[0]).toContain('readonly=2'); + // #359: Filter execution no longer injects `readonly` — server read-only + // policy belongs to ClickHouse user/profile config, not feature code. + expect(request[0]).not.toContain('readonly='); expect(request[0]).toContain('output_format_json_quote_64bit_integers=1'); expect(tab.filterPreview!.status).toBe('success'); expect(filterPreviewNormalizedOf(tab).helpers[0].options).toEqual([ diff --git a/tests/unit/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index 1f14aeca..394d861b 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -331,7 +331,7 @@ describe('filters and the #235 execution planner', () => { queries: [query('qt', 'SELECT {p:String} AS n'), query('src', '', { dashboard: { role: 'filter' } })], })); await session.start(); - expect(session.state.value.filters[0].status).toBe('error'); + expect(session.state.value.filters[0].status).toBe('source-error'); }); it('marks a filter whose source query returns a runtime error as an error', async () => { @@ -348,7 +348,7 @@ describe('filters and the #235 execution planner', () => { ], })); await session.start(); - expect(session.state.value.filters[0].status).toBe('error'); + expect(session.state.value.filters[0].status).toBe('source-error'); }); it('blanks a curated-but-inactive parameter on the next wave', async () => { @@ -436,6 +436,285 @@ describe('filters and the #235 execution planner', () => { }); +// #359: the filter-source runtime split — N filter DEFINITIONS sharing one +// `sourceQueryId` share exactly ONE `FilterSourceRuntime`, so the source SQL +// executes once per wave no matter how many parameters it feeds. Before this +// fix, `runFilterWave` ran the shared source once PER DEFINITION and keyed +// each provider by the definition id, so `mergeDashboardFilterHelpers` +// rejected every helper as a duplicate provider and every field went empty. +describe('shared filter-source runtime (#359)', () => { + const sharedDoc = (filters: DashboardFilterDefinitionV1[], tiles: DashboardTileV1[] = []) => doc({ tiles, filters }); + + it('runs a source shared by two definitions exactly once; both fields populate', async () => { + const { exec, calls } = makeExec((sql) => (sql.includes('shared') + ? { columns: [{ name: 'p1', type: 'Array(String)' }, { name: 'p2', type: 'Array(String)' }], rows: [[['V1', 'V2'], ['W1']]] } + : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = sharedDoc( + [{ id: 'f1', parameter: 'p1', sourceQueryId: 'src' }, { id: 'f2', parameter: 'p2', sourceQueryId: 'src' }], + [tile('ta', 'qa'), tile('tb', 'qb')], + ); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qa', 'SELECT {p1:String} AS n'), query('qb', 'SELECT {p2:String} AS n'), + query('src', "SELECT ['V1','V2'] AS p1, ['W1'] AS p2 /* shared */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + expect(calls.filter((c) => c.sql.includes('shared')).length).toBe(1); // ONE execution, not two. + const f1 = session.state.value.filters.find((f) => f.id === 'f1')!; + const f2 = session.state.value.filters.find((f) => f.id === 'f2')!; + expect(f1).toMatchObject({ status: 'ready', options: [{ value: 'V1', label: 'V1' }, { value: 'V2', label: 'V2' }] }); + expect(f2).toMatchObject({ status: 'ready', options: [{ value: 'W1', label: 'W1' }] }); + }); + + it('keys the merge by the SOURCE query id, not the definition id — two distinct sources sharing a helper name still collide, no winner', async () => { + const { exec } = makeExec((sql) => { + if (sql.includes('srcA')) return { columns: [{ name: 'dup', type: 'Array(String)' }], rows: [[['A1']]] }; + if (sql.includes('srcB')) return { columns: [{ name: 'dup', type: 'Array(String)' }], rows: [[['B1']]] }; + return { columns: [{ name: 'n' }], rows: [[1]] }; + }); + const document = sharedDoc( + [{ id: 'f1', parameter: 'dup', sourceQueryId: 'srcA' }, { id: 'f2', parameter: 'dup', sourceQueryId: 'srcB' }], + [tile('t', 'qt')], + ); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qt', 'SELECT {dup:String} AS n'), + query('srcA', "SELECT ['A1'] AS dup /* srcA */", { dashboard: { role: 'filter' } }), + query('srcB', "SELECT ['B1'] AS dup /* srcB */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + expect(session.state.value.filters.every((f) => f.status === 'helper-error')).toBe(true); + // The diagnostic message names the SOURCE ids ('srcA'/'srcB'), never the + // filter definition ids ('f1'/'f2') — proves provider identity is keyed + // by the source query id (the #359 bug), not the definition id. + const dupDiag = session.state.value.filterDiagnostics.find((d) => d.code === 'filter-duplicate-provider'); + expect(dupDiag?.message).toContain('srcA'); + expect(dupDiag?.message).toContain('srcB'); + expect(dupDiag?.message).not.toContain('f1'); + expect(dupDiag?.message).not.toContain('f2'); + }); + + it('clears options on a subsequent source failure — no stale retention', async () => { + let fail = false; + const { exec } = makeExec((sql) => (sql.includes('source') + ? (fail ? { error: 'source down' } : { columns: [{ name: 'p', type: 'Array(String)' }], rows: [[['V']]] }) + : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = sharedDoc([{ id: 'f1', parameter: 'p', sourceQueryId: 'src' }], [tile('t', 'qt')]); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [query('qt', 'SELECT {p:String} AS n'), query('src', 'SELECT 1 /* source */', { dashboard: { role: 'filter' } })], + })); + await session.start(); + expect(session.state.value.filters[0]).toMatchObject({ status: 'ready', options: [{ value: 'V', label: 'V' }] }); + fail = true; + await session.refresh(); + expect(session.state.value.filters[0].status).toBe('source-error'); + expect(session.state.value.filters[0].options).toBeNull(); + }); + + it('marks a source that returns a row but no valid helper column as source-error (malformed result) and surfaces the diagnostic', async () => { + // The query succeeds (one row) but the column is a plain scalar, not an + // Array/Map — readFilterOptions yields zero helpers, so the SOURCE status + // is the terminal `error` (not a transport failure), every consumer is + // `source-error`, options are cleared, and the merge publishes the + // `filter-no-valid-helpers` diagnostic. + const { exec } = makeExec((sql) => (sql.includes('source') + ? { columns: [{ name: 'p', type: 'String' }], rows: [['x']] } + : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = sharedDoc([{ id: 'f1', parameter: 'p', sourceQueryId: 'src' }], [tile('t', 'qt')]); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [query('qt', 'SELECT {p:String} AS n'), query('src', "SELECT 'x' AS p /* source */", { dashboard: { role: 'filter' } })], + })); + await session.start(); + expect(session.state.value.filters[0].status).toBe('source-error'); + expect(session.state.value.filters[0].options).toBeNull(); + expect(session.state.value.filterDiagnostics.some((d) => d.code === 'filter-no-valid-helpers')).toBe(true); + }); + + it('bumps optionsRev only when the option-value CONTENT changes; a same-content republish leaves it untouched', async () => { + let values = ['V1']; + const { exec } = makeExec((sql) => (sql.includes('source') + ? { columns: [{ name: 'p', type: 'Array(String)' }], rows: [[values]] } + : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = sharedDoc([{ id: 'f1', parameter: 'p', sourceQueryId: 'src' }], [tile('t', 'qt')]); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [query('qt', 'SELECT {p:String} AS n'), query('src', 'SELECT 1 /* source */', { dashboard: { role: 'filter' } })], + })); + await session.start(); + const rev0 = session.state.value.filters[0].optionsRev; + expect(rev0).toBeGreaterThan(0); // null -> non-empty bumped + await session.refresh(); // identical content republished + expect(session.state.value.filters[0].optionsRev).toBe(rev0); // no bump + values = ['V2']; // different content + await session.refresh(); + expect(session.state.value.filters[0].optionsRev).toBe(rev0 + 1); + expect(session.state.value.filters[0].options).toEqual([{ value: 'V2', label: 'V2' }]); + }); + + it("reconciles a removed active option (active=false, value kept) synchronously BEFORE the same refresh's affected-tile wave", async () => { + let values = ['V', 'W']; + const { exec, calls } = makeExec((sql) => (sql.includes('source') + ? { columns: [{ name: 'p', type: 'Array(String)' }], rows: [[values]] } + : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = sharedDoc( + [ + { id: 'f1', parameter: 'p', sourceQueryId: 'src', defaultActive: true, defaultValue: 'V' }, + // A second, PLAIN filter (no source) — exercises the reconcile loop's + // non-matching branch (its own parameter is never in `merged.changed`). + { id: 'f2', parameter: 'other', defaultActive: false, defaultValue: '' }, + ], + [tile('t', 'qt')], + ); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [query('qt', 'SELECT {p:String} AS n'), query('src', 'SELECT 1 /* source */', { dashboard: { role: 'filter' } })], + })); + await session.start(); + expect(session.state.value.filters[0].active).toBe(true); + const before = calls.length; + values = ['W']; // 'V' no longer offered + await session.refresh(); + expect(session.state.value.filters[0].active).toBe(false); + expect(session.state.value.filters[0].value).toBe('V'); // value retained + // The reconciliation applied BEFORE this SAME refresh's affected wave ran + // — the tile never sees a stale param_p binding (the plan-review PRECONDITION). + expect(calls.slice(before).some((c) => 'param_p' in c.params)).toBe(false); + }); + + it('a superseded shared-source wave publishes NOTHING — never clobbers the fresher wave (stale-gen guard)', async () => { + let releaseFirst!: () => void; + const gate = new Promise((resolve) => { releaseFirst = resolve; }); + let call = 0; + const { exec } = makeExec((sql) => { + if (!sql.includes('source')) return { columns: [{ name: 'n' }], rows: [[1]] }; + call += 1; + return call === 1 + ? gate.then(() => ({ columns: [{ name: 'p', type: 'Array(String)' }], rows: [[['STALE']]] })) + : { columns: [{ name: 'p', type: 'Array(String)' }], rows: [[['FRESH']]] }; + }); + const document = sharedDoc( + [{ id: 'f1', parameter: 'p', sourceQueryId: 'src', defaultActive: true, defaultValue: 'FRESH' }], [tile('t', 'qt')], + ); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [query('qt', 'SELECT {p:String} AS n'), query('src', 'SELECT 1 /* source */', { dashboard: { role: 'filter' } })], + })); + // Record EVERY published options snapshot for f1 across the overlap. + const optionsSeen: (unknown[] | null)[] = []; + const unsubscribe = session.state.subscribe((s) => optionsSeen.push(s.filters[0].options)); + const first = session.start(); + await flush(); + const second = session.refresh(); // supersedes the pending first source run + releaseFirst(); + await Promise.all([first, second]); + unsubscribe(); + const fresh = [{ value: 'FRESH', label: 'FRESH' }]; + expect(session.state.value.filters[0].options).toEqual(fresh); + // The STALE run's data never reaches state (its provider was discarded), + // AND — the #359 guard — once the fresher wave published FRESH options no + // later publish from the superseded wave ever reverts them to null: without + // the guard the stale wave's applyFilterProviders would blank every + // consumer to missing-helper/null over the correct FRESH state. + expect(optionsSeen).not.toContainEqual([{ value: 'STALE', label: 'STALE' }]); + const firstFreshAt = optionsSeen.findIndex((o) => JSON.stringify(o) === JSON.stringify(fresh)); + expect(firstFreshAt).toBeGreaterThanOrEqual(0); + expect(optionsSeen.slice(firstFreshAt).every((o) => JSON.stringify(o) === JSON.stringify(fresh))).toBe(true); + }); + + it('destroy cancels a shared in-flight source exactly once, even with two consumers', async () => { + const abortSpy = vi.spyOn(AbortController.prototype, 'abort'); + const { exec } = makeExec((sql) => (sql.includes('source') ? new Promise(() => {}) : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = sharedDoc( + [{ id: 'f1', parameter: 'p1', sourceQueryId: 'src' }, { id: 'f2', parameter: 'p2', sourceQueryId: 'src' }], + ); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [query('src', "SELECT ['V'] AS p1, ['W'] AS p2 /* source */", { dashboard: { role: 'filter' } })], + })); + // Intentionally not awaited: the source responder never resolves, so + // `start()` never settles either — only `destroy()`'s abort matters here. + void session.start(); + await flush(); + session.destroy(); + expect(abortSpy).toHaveBeenCalledTimes(1); + abortSpy.mockRestore(); + }); + + it('publishes filterDiagnostics with severity, not duplicated per shared-source consumer', async () => { + const { exec } = makeExec((sql) => (sql.includes('source') ? { error: 'source down' } : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = sharedDoc( + [{ id: 'f1', parameter: 'p1', sourceQueryId: 'src' }, { id: 'f2', parameter: 'p2', sourceQueryId: 'src' }], + ); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [query('src', 'SELECT 1 /* source */', { dashboard: { role: 'filter' } })], + })); + await session.start(); + const diags = session.state.value.filterDiagnostics; + const failures = diags.filter((d) => d.code === 'filter-query-failed'); + expect(failures.length).toBe(1); // ONE execution → ONE diagnostic, not one per consumer. + expect(failures[0].severity).toBe('error'); + }); + + it('preserves warning severity for an unused-helper diagnostic', async () => { + const { exec } = makeExec((sql) => (sql.includes('source') + ? { columns: [{ name: 'z', type: 'Array(String)' }], rows: [[['V']]] } + : { columns: [{ name: 'n' }], rows: [[1]] })); + // No tile/filter parameter named 'z' — the source's helper column has no consumer. + const document = sharedDoc([{ id: 'f1', parameter: 'unrelated', sourceQueryId: 'src' }], [tile('t', 'qt')]); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [query('qt', 'SELECT 1 AS n'), query('src', 'SELECT 1 /* source */', { dashboard: { role: 'filter' } })], + })); + await session.start(); + const warn = session.state.value.filterDiagnostics.find((d) => d.code === 'filter-helper-unused'); + expect(warn?.severity).toBe('warning'); + }); + + it('marks a consumer missing-helper when the shared source omits its column; a sibling with a returned column stays ready', async () => { + const { exec } = makeExec((sql) => (sql.includes('source') + ? { columns: [{ name: 'p1', type: 'Array(String)' }], rows: [[['V']]] } // only p1, no p2 + : { columns: [{ name: 'n' }], rows: [[1]] })); + const document = sharedDoc( + [{ id: 'f1', parameter: 'p1', sourceQueryId: 'src' }, { id: 'f2', parameter: 'p2', sourceQueryId: 'src' }], + [tile('ta', 'qa'), tile('tb', 'qb')], + ); + const session = createDashboardViewerSession(makeDeps({ + document, exec, + queries: [ + query('qa', 'SELECT {p1:String} AS n'), query('qb', 'SELECT {p2:String} AS n'), + query('src', "SELECT ['V'] AS p1 /* source */", { dashboard: { role: 'filter' } }), + ], + })); + await session.start(); + const f1 = session.state.value.filters.find((f) => f.id === 'f1')!; + const f2 = session.state.value.filters.find((f) => f.id === 'f2')!; + expect(f1.status).toBe('ready'); + expect(f2.status).toBe('missing-helper'); + expect(f2.options).toBeNull(); + }); + + it('marks a filter source-error when its sourceQueryId does not resolve to any query — visible, not silently skipped', async () => { + const { exec, calls } = makeExec(() => ({ columns: [{ name: 'n' }], rows: [[1]] })); + const document = sharedDoc([{ id: 'f1', parameter: 'p', sourceQueryId: 'ghost-src' }], [tile('t', 'qt')]); + const session = createDashboardViewerSession(makeDeps({ + document, exec, queries: [query('qt', 'SELECT {p:String} AS n')], + })); + await session.start(); + expect(session.state.value.filters[0].status).toBe('source-error'); + expect(session.state.value.filterDiagnostics).toEqual( + expect.arrayContaining([expect.objectContaining({ code: 'filter-source-missing', sourceId: 'ghost-src' })]), + ); + expect(calls.some((c) => c.sql.includes('ghost'))).toBe(false); // never executes + }); +}); + describe('filter-bar bridge (controls / getFilterField / applyFilter)', () => { it('exposes controls + a draft-aware field state, and applyFilter sets value AND active explicitly', async () => { const { exec, calls } = makeExec(() => ({ columns: [{ name: 'n' }], rows: [[1]] })); diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index 9f530c4d..de29f7f8 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -2503,6 +2503,85 @@ describe('renderDashboard — shared rich filter bar over the viewer (#188)', () }); }); +// #359: the shared-source filter wave now publishes `optionsRev` (bumped ONLY +// when a curated source's option VALUE CONTENT changes — including a clear to +// null — never on an unchanged republish) and `filterDiagnostics` (its own +// merge diagnostics, separate from the presentation `diagnostics` above). The +// UI folds `optionsRev` into the filter-bar rebuild signature and renders +// each diagnostic's severity as its own `is-*` class. +describe('renderDashboard — filter-source runtime rebuild + diagnostics (#359)', () => { + it('rebuilds the filter bar when curated option CONTENT changes (same length), not on an unchanged republish', async () => { + let call = 0; + const { app } = dashApp({ + responder: (sql) => { + if (sql.includes('opts')) { + call++; + // Same content on the first two runs (initial start + one refresh); + // different content (same length) on the third run. + return { columns: [{ name: 'p', type: 'Array(String)' }], rows: [[call === 3 ? ['a', 'b'] : ['x', 'y']]] }; + } + return {}; + }, + workspace: wsWith({ + queries: [ + q('q1', 'SELECT k, v FROM a WHERE x = {p:String}'), + q('src', "SELECT ['x','y'] AS p -- opts", { dashboard: { role: 'filter' } }), + ], + tiles: [{ id: 't1', queryId: 'q1' }], + filters: [{ id: 'f1', parameter: 'p', sourceQueryId: 'src' }], + }), + }); + await render(app); + const barBefore = qs(app.root, '.dash-filter-host').firstElementChild; + expect(barBefore).not.toBeNull(); + // Refresh #1: the source republishes the SAME option content — no rebuild + // (the pre-#359 boolean-only signature would have missed this distinction + // too, since it only ever asked "empty vs non-empty"). + await (runOnclick(qs(app.root, '.dash-refresh')) as Promise); + expect(qs(app.root, '.dash-filter-host').firstElementChild).toBe(barBefore); + // Refresh #2: the source returns DIFFERENT content, same length — this is + // exactly the case the old boolean-only signature missed; `optionsRev` + // fixes it. + await (runOnclick(qs(app.root, '.dash-refresh')) as Promise); + expect(qs(app.root, '.dash-filter-host').firstElementChild).not.toBe(barBefore); + }); + + it('renders filterDiagnostics with severity-mapped classes, alongside presentation diagnostics', async () => { + const { app } = dashApp({ + responder: (sql) => { + if (sql.includes('optsinfo')) return { columns: [{ name: 'pinfo', type: 'Array(String)' }], rows: [[['x', 'x']]] }; + if (sql.includes('optswarn')) return { columns: [{ name: 'pwarn', type: 'Array(String)' }], rows: [[['a', 'b']]] }; + return {}; + }, + workspace: wsWith({ + queries: [ + q('q1', 'SELECT k, v FROM a WHERE x = {pinfo:String}'), + // A duplicate option value ('x' twice) → an 'info' diagnostic + // (`filter-duplicate-option`) from readFilterOptions. + q('srcInfo', "SELECT ['x','x'] AS pinfo -- optsinfo", { dashboard: { role: 'filter' } }), + // 'pwarn' has no Panel consumer → a 'warning' diagnostic + // (`filter-helper-unused`) from the merge. + q('srcWarn', "SELECT ['a','b'] AS pwarn -- optswarn", { dashboard: { role: 'filter' } }), + ], + tiles: [{ id: 't1', queryId: 'q1' }], + filters: [ + // An unresolvable source query id → an 'error' diagnostic + // (`filter-source-missing`). + { id: 'ferr', parameter: 'perr', sourceQueryId: 'nope' }, + { id: 'fwarn', parameter: 'pwarn', sourceQueryId: 'srcWarn' }, + { id: 'finfo', parameter: 'pinfo', sourceQueryId: 'srcInfo' }, + ], + }), + }); + await render(app); + const rows = qsa(app.root, '.dash-filter-diagnostics .dash-config-diagnostic'); + const bySeverity = (cls: string) => rows.find((r) => r.classList.contains(cls)); + expect(bySeverity('is-error')?.textContent).toBe('Filter references unknown source query "nope"'); + expect(bySeverity('is-warning')?.textContent).toBe('Filter helper "pwarn" has no current Panel consumer.'); + expect(bySeverity('is-info')?.textContent).toBe('Filter helper "pinfo" contains a duplicate value.'); + }); +}); + // #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-execution.test.ts b/tests/unit/filter-execution.test.ts index dae65ce7..7adc6e47 100644 --- a/tests/unit/filter-execution.test.ts +++ b/tests/unit/filter-execution.test.ts @@ -7,9 +7,10 @@ describe('Filter execution', () => { it('owns a lossless, read-only, bounded structured transport', () => { const out = filterExecution('SELECT [1] AS id', { params: { custom: 1 } }); expect(out).toMatchObject({ owned: true, format: 'Filter', rowLimit: FILTER_TOP_LEVEL_ROW_LIMIT, error: null, diagnostics: [] }); - expect(out.params).toMatchObject({ readonly: 2, max_result_bytes: FILTER_RESULT_BYTE_CAP, custom: 1, + expect(out.params).toMatchObject({ max_result_bytes: FILTER_RESULT_BYTE_CAP, custom: 1, 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(out.params).not.toHaveProperty('readonly'); }); it('reports every static SQL contract failure', () => { expect(filterSqlDiagnostics('')).toMatchObject([{ code: 'filter-sql-empty' }]); From f14bd3d6b2468cd30f813b20c4e0fb9dc10f4e0e Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Tue, 21 Jul 2026 14:55:26 +0200 Subject: [PATCH 2/3] =?UTF-8?q?fix(#359):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20real=20E2E,=20missing-helper=20diagnostic,=20retained=20prov?= =?UTF-8?q?ider?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the three P1 review findings on PR #361: 1. E2E was red and misleading. The shared-source case built two standalone comboboxes from hand-split arrays and never touched the viewer. Replaced with a REAL DashboardViewerSession integration: two filter definitions share one Filter source, the harness asserts the source executed exactly once (window.__sharedRequests === 1), both consumers resolve to `ready`, and each filter's curated column populates a real buildFilterOptionField control. Adds the @preact/signals-core import-map entry the unbundled harness needs. 2. Missing helper was silent. When a source succeeds but omits a consumer's column, applyFilterProviders now publishes a `filter-helper-missing` warning (severity warning, sourceId + helperName, naming the source and column) in addition to setting `missing-helper` — the UI renders diagnostics, not per-filter status, so the control is no longer unexplained. 3. Provider not retained per source. FilterSourceRuntime now stores `provider: FilterProvider | null`, updated only for the current generation; the terminal merge reads the COMPLETE set from every source runtime, not just the wave's returns. A future selective wave (#360) can re-merge unaffected sources' helpers instead of clearing or rerunning them. Full gate green (4421 tests, tsc, build); new E2E verified loading + one-request behavior in a local Chromium harness run. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01UDsUDSPoDYa1M1rbpgdG3f --- CHANGELOG.md | 8 +- .../application/dashboard-viewer-session.ts | 62 ++++++++--- tests/e2e/filter-source.html | 101 ++++++++++++------ tests/e2e/filter-source.spec.js | 17 ++- tests/unit/dashboard-viewer-session.test.ts | 8 ++ 5 files changed, 145 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ce9d198..da9a103c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,13 @@ auto-generated per-PR notes; this file is the curated, human-readable history. 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. This is the one-source-runtime boundary #360 builds on. + 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 diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index 60660659..6325f6c5 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -291,8 +291,13 @@ interface FilterRuntime { * 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. No stored `provider` — `runFilterSource` RETURNS it; - * `runFilterWave` collects the returns and merges once. */ + * 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; @@ -300,6 +305,7 @@ interface FilterSourceRuntime { gen: number; abortController: AbortController | null; status: 'idle' | 'loading' | 'ready' | 'error'; + provider: FilterProvider | null; } const cfgType = (panel: unknown): string | undefined => @@ -429,7 +435,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa if (!source) { source = { id: filter.sourceId, query: queryById.get(filter.sourceId), - consumers: [], gen: 0, abortController: null, status: 'idle', + consumers: [], gen: 0, abortController: null, status: 'idle', provider: null, }; filterSources.set(filter.sourceId, source); } @@ -645,12 +651,14 @@ 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). Returns the `FilterProvider` the caller collects into ONE merge; - * `source.status` is set to the TRANSPORT terminal (`ready`/`error`) — - * per-consumer curation status is derived afterward in - * `applyFilterProviders`. Every stale-gen guard mirrors the tile runner: - * a superseded source's provider is discarded (returns `null`) rather than - * publishing a stale result. */ + * 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` @@ -663,6 +671,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa }; if (source.gen !== generation) return null; source.status = 'error'; + source.provider = provider; return provider; } const query = source.query; @@ -673,6 +682,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa }; if (source.gen !== generation) return null; source.status = 'error'; + source.provider = provider; return provider; } if (source.gen !== generation) return null; @@ -700,6 +710,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa provider = { sourceId: source.id, sourceName: queryName(query), ...normalized }; source.status = normalized.helpers.length ? 'ready' : 'error'; } + source.provider = provider; return provider; } @@ -711,7 +722,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa * 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 }[], providers: (FilterProvider | null)[]): void { + 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`), @@ -722,12 +733,22 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // `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; - filterDiagnostics = merged.diagnostics; // published as-is — never deduped. + // 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') { @@ -748,9 +769,17 @@ 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.status = namedByDiagnostic ? 'helper-error' : 'missing-helper'; + 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 @@ -773,9 +802,12 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa for (const consumer of source.consumers) consumer.state.status = 'loading'; } publish(); - const providers = await runPool(plan, VIEWER_TILE_CONCURRENCY, + // 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, providers); + applyFilterProviders(plan); } diff --git a/tests/e2e/filter-source.html b/tests/e2e/filter-source.html index 46f573ac..e55212c5 100644 --- a/tests/e2e/filter-source.html +++ b/tests/e2e/filter-source.html @@ -4,6 +4,16 @@ Filter source harness + +