From 562e3460fa2bebdc269e07c2cc1cbefdfd78fe6e Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sun, 26 Jul 2026 18:20:14 +0200 Subject: [PATCH 1/4] feat(#468): restore the searchable multi-select for Array(scalar T) variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Array(T)` is the type a multi-select exists for, but #447 phase 1 removed the curated model's multiselect control as an owner decision and phase 2 classified every container type as having no inferred control. A `user : Array(String)` variable with a working option query stored its SQL, ran it fine in its own `Variable: user` tab, and still rendered a free-text box — its option SQL was never batched and no list ever appeared. Restores the #189/PR-#364 control on the inferred-variable model: search over labels AND values, tri-state Select visible scoped to the filtered subset, Clear/Cancel/Apply with draft-until-Apply, close-before-commit, focus return, and refresh reconciliation by bound value. Design: - ONE pure predicate, `multiSelectElementType`, is read by both the option batch and `fieldControlKind`, so a type whose option SQL ran can never be one the bar refuses a select for. `Tuple`/`Map`/`Nested`/`Array(Array(T))` are unchanged. - `fieldControlKind` classifies the TYPE (`'multi'`); `filter-bar.ts` pairs that with the spec, because only the bar can see whether option SQL was configured. - Selections are real `string[]` end to end and bind through the existing typed serializer. They never enter `FilterBarApp.state.varValues`, which stays `Record` — TS property assignability is covariant even for mutable properties, so keeping that type narrow IS the enforcement, not a comment. - An empty selection reduces to UNSET rather than `[]`: `emptyValue()` treats a present `[]` as a real value, so binding one would run panels as `IN []` — nothing returned, but looking filtered. - Reconciliation returns names; `refresh` runs one coalesced wave after both the option request and the tile pool settle. Fixes a latent bug: every `Array(T)` variable with valid option SQL was marked `status: 'error'` by a branch commented "unreachable", invisible only because the select it applied to never rendered. Verified live against github.demo.altinity.cloud: 13 options load, search narrows to 5, Apply binds `param_user=['btyshkevich@altinity.com',...]` as a real ClickHouse literal, and the selection survives a reload. Reconciles #447's non-goals/control matrix/phase-1 gate, ADR-0003's phase-2 addendum, CHANGELOG, README and roadmap #68. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BF6uJrfy51KgkeTvfzyWDB --- CHANGELOG.md | 45 +- README.md | 27 +- docs/ADR-0003-dashboard-viewing.md | 76 +++- src/core/param-pipeline.ts | 24 +- src/core/param-type.ts | 24 ++ src/core/variable-options.ts | 32 +- src/core/variable-selection.ts | 98 +++++ .../application/dashboard-viewer-session.ts | 161 +++++-- src/styles.css | 87 +++- src/ui/dashboard.ts | 34 +- src/ui/filter-bar.ts | 116 ++++- src/ui/multi-select-field.ts | 288 +++++++++++++ tests/e2e/multi-select.html | 27 ++ tests/e2e/multi-select.spec.js | 133 ++++++ tests/unit/dashboard-viewer-session.test.ts | 181 ++++++++ tests/unit/dashboard.test.ts | 41 ++ tests/unit/filter-bar.test.ts | 143 ++++++- tests/unit/multi-select-field.test.ts | 400 ++++++++++++++++++ tests/unit/param-pipeline.test.ts | 29 +- tests/unit/param-type.test.ts | 57 +++ tests/unit/variable-options.test.ts | 24 +- tests/unit/variable-selection.test.ts | 101 +++++ 22 files changed, 2044 insertions(+), 104 deletions(-) create mode 100644 src/core/variable-selection.ts create mode 100644 src/ui/multi-select-field.ts create mode 100644 tests/e2e/multi-select.html create mode 100644 tests/e2e/multi-select.spec.js create mode 100644 tests/unit/multi-select-field.test.ts create mode 100644 tests/unit/variable-selection.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bd2e277..460c7826 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,46 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] ### Added +- **An `Array(T)` Dashboard variable with option SQL is a searchable + multi-select** (#468). `Array(T)` is the type multi-select exists for, but #447 + phase 1 removed the curated model's multiselect control as an owner decision and + phase 2 classified every container as having no inferred control — so a variable + like `user : Array(String)`, with a perfectly good option query configured, + rendered a free-text box and never ran that query. The #189/PR-#364 control is + back, now driven by the variable's own Dashboard-local option SQL. + + The closed trigger reads **`Not set`**, the single option's **label**, or + **`N selected`**. The popover offers a labelled search over both labels and + values, a tri-state **Select visible** scoped to whatever the search currently + shows (hidden rows are never touched), per-option checkboxes, and **Clear / + Cancel / Apply**. Nothing commits until Apply, which canonicalizes by option + order, fires at most once, and re-runs only the panels declaring that variable — + a no-op Apply issues nothing. Cancel, Escape and clicking outside discard the + draft and return focus to the trigger. + + Selections stay **real arrays** end to end — through viewer state, the + per-Dashboard store, and the existing typed `Array(T)` serializer, which binds + `param_user=['ada','bo']` with quotes, backslashes, Unicode and big integers all + escaped, never a joined `"ada,bo"`. An **empty** selection is unset (the panels + wait) rather than a literal `[]`, which would return nothing while looking + filtered. When a refresh drops a selected value the survivors stay bound and the + affected panels re-run in **one** coalesced wave; a pure reorder or a label-only + change re-runs nothing; an empty intersection returns the variable to unset. New + options are never auto-selected. Selections survive a reload. + + Eligibility is one pure predicate (`multiSelectElementType`) shared by the option + batch and the control dispatch, so a type whose option SQL ran can never be one + the bar refuses to render a select for. `Tuple`, `Map`, `Nested` and nested + `Array(Array(…))` are unchanged: no option query, no select, and the same + free-text field plus marker as before. An `Array(scalar T)` with **no** option SQL + also keeps that field, but its marker now names the fix ("add option SQL to pick + from a list") instead of calling a controllable type uncontrollable. + + **Fixes a latent bug:** every `Array(T)` variable with valid option SQL was + silently marked `status: 'error'` by a branch commented "unreachable via + `optionBatchVariables`' own rule" — which was wrong, and invisible only because + the select it would have applied to never rendered. + - **Dashboard variables can offer a list of values, and every list on a Dashboard loads in one request** (#447, phase 2). A variable (inferred from the `{name:Type}` placeholders in its panels' SQL) may carry optional @@ -37,7 +77,10 @@ auto-generated per-PR notes; this file is the curated, human-readable history. Cascading option queries are rejected outright, and `Array`/`Tuple`/`Map`/ `Nested` variables are marked as having no inferred control — they keep their - text input, since a literal typed there still binds. + text input, since a literal typed there still binds. (**Narrowed by #468 + above**, in this same unreleased set: an `Array` of a scalar WITH option SQL now + renders a multi-select. The marker survives for every other container, and for + an `Array(scalar T)` nobody has configured.) ### Changed - **Dashboard variable option SQL is edited in the main editor, as its own tab** diff --git a/README.md b/README.md index 7e518461..0d2325d9 100644 --- a/README.md +++ b/README.md @@ -533,10 +533,29 @@ icon. Option SQL must be one embeddable read query returning exactly two `String` columns — value, then visible label, by POSITION rather than by column name — and -may not reference Dashboard variables itself. A variable with option SQL renders a -strict single-select; one without keeps the direct input inferred from its declared -type. Every configured variable on a Dashboard is compiled into a single -`UNION ALL` request per refresh, so ten of them still cost one round trip. +may not reference Dashboard variables itself. Every configured variable on a +Dashboard is compiled into a single `UNION ALL` request per refresh, so ten of them +still cost one round trip. + +Which control a configured variable renders follows from its declared type: + +- a **scalar** gets a strict single-select over those options; +- an **`Array` of a scalar** (`Array(String)`, `Array(UInt64)`, …) gets a + **searchable multi-select**: a closed trigger reading `Not set` / the single + option's label / `N selected`, and a popover with a search box, a tri-state + **Select visible** scoped to whatever the search currently shows, per-option + checkboxes, and **Clear / Cancel / Apply**. Nothing commits until Apply, which + re-runs only the panels declaring that variable; Cancel, Escape and clicking + outside discard the draft. The selection binds as a real ClickHouse array + literal, so quotes, backslashes, Unicode and big integers are all escaped by the + same typed serializer everything else uses. When a refresh drops a value you had + selected, the rest of your selection survives; if every selected value is gone, + the variable returns to unset. Selections persist across a reload. + +A variable with **no** option SQL keeps the direct input inferred from its declared +type. `Tuple`, `Map`, `Nested` and nested `Array(Array(…))` have no inferable +control at all and always keep a free-text field, where a hand-typed literal such +as `['a','b']` still binds. ## Local install diff --git a/docs/ADR-0003-dashboard-viewing.md b/docs/ADR-0003-dashboard-viewing.md index ab477530..1949cb20 100644 --- a/docs/ADR-0003-dashboard-viewing.md +++ b/docs/ADR-0003-dashboard-viewing.md @@ -2,9 +2,10 @@ - **Status:** Accepted; detached-snapshot decision superseded by #407 on 2026-07-23; surface lifecycle amended by #425 and surface NAVIGATION amended by - #426, both 2026-07-25 (see the addenda) -- **Date:** 2026-07-18; revised 2026-07-23, 2026-07-25 -- **Context tracking:** roadmap #68; #288, #302, #406, #407, #425 + #426, both 2026-07-25; the #447 phase-2 compound-type exclusion narrowed by #468 + on 2026-07-26 (see the addenda) +- **Date:** 2026-07-18; revised 2026-07-23, 2026-07-25, 2026-07-26 +- **Context tracking:** roadmap #68; #288, #302, #406, #407, #425, #447, #457, #468 ## Context @@ -325,6 +326,9 @@ are worth recording because both contradict something the issue's own text impli already validates. Only the compound-type case is new — and it *adorns* the input rather than replacing it, because `param-serialize` binds an array literal typed there and removing the field would leave those panels permanently unfillable. + **Narrowed by the addendum below:** an `Array` of a scalar WITH option SQL now + renders a multi-select instead; the adornment survives for every other container + and for an `Array(scalar T)` nobody has configured. ## Addendum (#457, 2026-07-26): a variable's option SQL is a main-editor document @@ -374,6 +378,72 @@ the application already has. Four decisions replace it. the Dashboard, not on that poke. The tree's orphan-delete still fires it for real, because the tree is visible while a Dashboard is. +## Addendum (#468, 2026-07-26): an `Array(scalar T)` variable binds a selection + +#447 phase 1 removed the curated filter model wholesale, and its non-goals listed +"multiselect or Array-valued variable controls". Phase 2 then classified every +container type as having no inferred control. That left the one type multi-select +exists for — `Array(T)` — as a free-text box, even with a working option list +configured. This addendum records the deliberate reversal for the narrow case, and +restores the #189/PR-#364 control on top of the inferred-variable model. + +- **One predicate decides eligibility, and both consumers read it.** + `multiSelectElementType` (`core/param-type.ts`) answers "is this an `Array` of a + single scalar, and what is the element type" for `core/variable-options.ts`'s + batch filter AND for `fieldControlKind`'s control choice. A type whose option SQL + ran can therefore never be one the bar refuses to render a select for, which is + the same invariant `filter-bar.ts` already stated for the container verdict. + `Tuple`/`Map`/`Nested` have no flat element list; `Array(Array(T))` is rejected by + `param-serialize` outright. All four keep the adorned text field. + +- **`fieldControlKind` classifies the TYPE; the bar pairs it with the spec.** The + pure function has no way to know whether option SQL was configured, and giving it + one would mean passing UI state into a parameter-analysis helper. `'multi'` means + "this type can be multi-selected"; `filter-bar.ts` combines that with + `spec.options !== null`, and an `Array(scalar T)` with no options falls back to + the same adorned input as before — with wording that names the fix ("add option + SQL") instead of calling a controllable type uncontrollable. + +- **A selection is a real `string[]`, never a stringified literal.** + `param-serialize.ts` already builds the ClickHouse literal from a JS array, with + escaping, big integers and empty-string elements covered and tested. Committing a + pre-serialized string instead would put literal construction in the UI, and would + make the committed value unparseable back into a selection for the popover to + re-open on. `dashboard-filter-store.ts` never lost its `string | string[]` + support, so persistence needed no change at all. + +- **The string boundary is `state.varValues`, and it is enforced by its TYPE.** + Arrays travel: viewer session → `ViewerFilterState.value` (already `unknown`) → + `VariableFieldSpec.selection` → the control → `onCommitVariableSelection` → back. + They never enter `FilterBarApp.state.varValues`, which stays + `Record` because the Workbench var-strip owns and persists that + same bag under `asb:varValues`. Widening it would NOT have been a safeguard — + TypeScript's property assignability is covariant even for mutable properties, so + a widened type would still accept the real `AppState` while letting an array + through. Keeping it narrow is the enforcement. + +- **An empty selection is unset, not `[]`.** `param-pipeline`'s `emptyValue()` + treats a present `[]` as a genuine value, so binding one would run every panel as + `… IN []` — returning nothing while LOOKING filtered — where a variable's unset + contract is that its panels wait. `commitValue` reduces it to `UNSET_VALUE`, so + there is exactly one unset form. This deliberately narrows #189, which could + express an "active empty array"; with no defaults and no dormant values, no + control here can author one. + +- **Reconciliation returns names; `refresh` runs one wave.** `applyOptions` reports + which variables a fresh option list actually changed the bound SET for (a pure + reorder or a label-only change reports nothing), `runOptionBatch` collects them, + and `refresh` runs a single `commitAndRerun` over the union AFTER both the option + request and the tile pool have settled. Re-running inside `runOptionBatch` would + supersede tiles mid-refresh and make the outcome classifier judge tiles that are + already re-running. One coalesced wave is structural, not a flag to remember. + +- **A latent bug fell out.** `dashboard-viewer-session.ts` marked every `Array(T)` + variable with valid option SQL as `status: 'error'` via a branch commented + "unreachable" — true only for the types that genuinely cannot be option-backed. + Admitting `Array(scalar T)` into the batch made the comment honest, and the + message now says what is actually wrong. + ## Alternatives considered - **Durable detached snapshots:** rejected because they silently diverge from diff --git a/src/core/param-pipeline.ts b/src/core/param-pipeline.ts index d7e78a74..9076423f 100644 --- a/src/core/param-pipeline.ts +++ b/src/core/param-pipeline.ts @@ -30,7 +30,8 @@ import { splitStatements as _splitStatements, isRowReturning as _isRowReturning import { scanParamDeclarations } from './param-scan.js'; import type { ParamDeclaration } from './param-scan.js'; import { - parseParamType, conflictingTypes, enumValues, isCompoundParamType, typeLexKind, + parseParamType, conflictingTypes, enumValues, isCompoundParamType, multiSelectElementType, + typeLexKind, } from './param-type.js'; import type { ParsedParamType } from './param-type.js'; import { serializeParamValue as _serializeParamValue } from './param-serialize.js'; @@ -693,10 +694,11 @@ export function fieldControls(analysis: ParameterAnalysis): FieldControl[] { } /** `fieldControlKind`'s return shape — which control a `fieldControls` entry - * renders, and (for `'enum'`) the member list to offer. `'unsupported'` is - * reachable only under the scalar-controls policy (see `fieldControlKind`). */ + * renders, and (for `'enum'`) the member list to offer. `'multi'` and + * `'unsupported'` are reachable only under the scalar-controls policy (see + * `fieldControlKind`). */ export interface FieldControlKindResult { - kind: 'enum' | 'date' | 'text' | 'unsupported'; + kind: 'enum' | 'date' | 'text' | 'multi' | 'unsupported'; enumOptions: string[] | null; } @@ -713,7 +715,14 @@ export interface FieldControlKindOptions { * text, because ClickHouse's Bool accept-set is not enumerable — * `yes`/`no`/`on`/`off`/`1`/`0` all work — so the list is a hint, not a * constraint; - * - a COMPOUND type (`Array`/`Tuple`/`Map`/`Nested`) resolves to + * - an `Array` of a SCALAR resolves to `'multi'`: several option rows + * combine into one bound array, which is exactly what the restored #189 + * multi-select does. This says the TYPE can be multi-selected, not that + * this variable has an option list to select from — the bar pairs the + * verdict with the option spec one layer up, because only the bar knows + * whether option SQL was configured; + * - every OTHER compound type (`Tuple`/`Map`/`Nested`, and a nested + * `Array(Array(T))` the serializer rejects outright) resolves to * `'unsupported'`: there is no single-scalar control for a container, and * saying so is better than rendering a box that cannot produce a valid * value. The value pipeline itself still handles these types fine, which is @@ -750,6 +759,11 @@ export function fieldControlKind( if (options.scalarControls) { // Checked AFTER enum/date so the priority order stays single-sourced: a // declaration is only ever compound when neither of those claimed it. + // `multi` is tried FIRST among the container shapes — it is the narrower + // rule, and `multiSelectElementType` is the one predicate the option batch + // (`core/variable-options.js`) filters on, so a type that gets a select can + // never be one whose option SQL was skipped. + if (multiSelectElementType(field.type)) return { kind: 'multi', enumOptions: null }; if (isCompoundParamType(field.type)) return { kind: 'unsupported', enumOptions: null }; if (typeLexKind(field.type) === 'bool') return { kind: 'enum', enumOptions: BOOL_CONTROL_OPTIONS }; } diff --git a/src/core/param-type.ts b/src/core/param-type.ts index 54ec652b..815655f8 100644 --- a/src/core/param-type.ts +++ b/src/core/param-type.ts @@ -228,6 +228,30 @@ export function isCompoundParamType(type: string | ParsedParamType): boolean { return COMPOUND_BASE.test(base); } +/** + * The ELEMENT type when `type` is an `Array` of a single SCALAR — the one + * container shape a Dashboard can offer a multi-select for — else `null`. + * + * This is the single eligibility decision behind the restored `Array(T)` + * multi-select: `core/variable-options.js` consults it to admit a variable's + * option SQL into the batch, and `param-pipeline.js`'s `fieldControlKind` + * consults it to choose the control. One predicate, so "we ran the option SQL" + * and "we rendered a select" can never disagree. + * + * `null` for a scalar (nothing to multi-select), for `Tuple`/`Map`/`Nested` + * (no element list to pick from), and for a nested `Array(Array(T))` — the last + * because `param-serialize.js` rejects nested array VALUES and nested `Array` + * DECLARATIONS outright, so a control that produced one could never bind. + * + * Wrappers are already unwrapped by `parseParamType`, so `Nullable(Array( + * LowCardinality(String)))` yields the `String` element. Pure. + */ +export function multiSelectElementType(type: string | ParsedParamType): ParsedParamType | null { + const t = typeof type === 'string' ? parseParamType(type) : type; + if (!t.isArray || !t.elem) return null; + return t.elem.isArray || isCompoundParamType(t.elem) ? null : t.elem; +} + /** * The lexical family of a parsed (or raw) type, deciding how the typed * serializer emits an array *element* of that type: diff --git a/src/core/variable-options.ts b/src/core/variable-options.ts index 26eac2cf..2b29ab2c 100644 --- a/src/core/variable-options.ts +++ b/src/core/variable-options.ts @@ -34,7 +34,7 @@ import { detectSqlFormat, detectSqlOutfile, sqlString, stripTrailingTrivia } fro import { scanParamDeclarations } from './param-scan.js'; import { analysisView } from './param-pipeline.js'; import { hasOptionalBlocks } from './optional-blocks.js'; -import { isCompoundParamType } from './param-type.js'; +import { isCompoundParamType, multiSelectElementType } from './param-type.js'; import type { DashboardVariable } from './dashboard-variables.types.js'; import type { VariableOption, VariableOptionBatch, VariableOptionBranch, VariableOptionDiagnostic, @@ -212,11 +212,30 @@ export function optionSqlDiagnostics(sql?: string | null): VariableOptionDiagnos const isRunnableOptionSql = (sql: string | null): boolean => sql !== null && optionSqlDiagnostics(sql).length === 0; +/** + * Whether a variable's declared TYPE can be backed by an option list at all. + * + * A scalar takes the single-select; an `Array` of a scalar takes the restored + * multi-select, where the SAME two-String-column option rows are the pool a user + * picks several values from — the array-ness is about how selections are + * COMBINED into one bound value, never about the row shape, so nothing in the + * compiler or the reader varies with it. + * + * Every other container (`Tuple`/`Map`/`Nested`, and a nested + * `Array(Array(T))`) still renders no select: a flat value/label list cannot + * supply one of those, so running its option SQL would be work for a control + * that never appears — and a broken one could fail the combined query and take + * every OTHER variable's options down with it. Same rule, same reason, as + * conflicted and orphaned. + */ +const optionEligibleType = (type: string): boolean => + !isCompoundParamType(type) || multiSelectElementType(type) !== null; + /** * The variables that belong in a refresh's option batch: inferred, * type-consistent (`status === 'active'`, which excludes both a CONFLICTED name - * and an ORPHANED configuration), configured with non-empty option SQL, and - * locally acceptable. + * and an ORPHANED configuration), of an option-backable type, configured with + * non-empty option SQL, and locally acceptable. * * Order is the caller's — `inferDashboardVariables`' inference order — and every * consumer follows it, so the compiled branch order is the Variables order. @@ -225,12 +244,7 @@ export const optionBatchVariables = ( variables: readonly DashboardVariable[], ): DashboardVariable[] => variables.filter( (variable) => variable.status === 'active' - // A CONTAINER-typed variable renders no option select (a two-String-column - // list cannot supply an `Array`/`Tuple`/`Map`/`Nested` value), so running its - // option SQL would be work for a control that never appears — and, worse, a - // broken one could fail the combined query and take every OTHER variable's - // options down with it. Same rule, same reason, as conflicted and orphaned. - && !isCompoundParamType(variable.type ?? '') + && optionEligibleType(variable.type ?? '') && isRunnableOptionSql(variable.sql), ); diff --git a/src/core/variable-selection.ts b/src/core/variable-selection.ts new file mode 100644 index 00000000..dc768b1c --- /dev/null +++ b/src/core/variable-selection.ts @@ -0,0 +1,98 @@ +// Pure value-side helpers for an option-backed Dashboard variable whose declared +// type is `Array(scalar T)` — the multi-select restored from #189/PR #364 onto +// the inferred-Variables model (#447). +// +// Only the three value helpers came back. #189's other half — `resolveFilterSelection` +// / `gatherExecutableConsumers`, the `selection.mode` override, explicit target +// lists — was the CURATED filter model's contract resolver and has no meaning +// here: a variable is inferred from panel SQL, binds by exact name to every +// panel that declares it, and its multi-ness is read straight off its declared +// type by `multiSelectElementType` (`param-type.ts`). There is no persisted +// selection mode to resolve, so this module is named for what survived rather +// than reviving `filter-selection.ts`. +// +// Empty string (`''`) is a VALID option value and a valid selection element +// throughout — never a sentinel for "nothing selected". Activation is carried by +// the field's own `active` flag, and the Dashboard's single unset form is a +// scalar `''` (see `dashboard-viewer-session.ts`'s `UNSET_VALUE`). + +/** + * Structural equality for two selections: same length, same values, in the same + * ORDER — order matters because both sides are already canonicalized against + * one option list, so a difference in order IS a difference in the committed + * value. + * + * Deliberately narrower than #189's original, which took `unknown` and had + * array-vs-string and scalar-vs-scalar arms. Those existed for the error-mode + * raw-string fallback commit, which this model has no path to — every caller + * here holds a `string[]`. Pure. + */ +export function sameSelection(a: readonly string[], b: readonly string[]): boolean { + return a.length === b.length && a.every((v, i) => v === b[i]); +} + +/** + * Canonicalize a set of selection values against the authoritative option list: + * dedupe, drop any value with no matching option (a stale bound value an option + * refresh removed), and order the survivors by OPTION order — never `values`' + * own order, because the option list is authoritative for display order. + * + * Never introduces a value that was not already in `values`: this is a + * filter/reorder, never an auto-select. Pure. + */ +export function canonicalizeSelection( + values: readonly string[], + options: readonly { value: string }[], +): string[] { + const wanted = new Set(values); + const seen = new Set(); + const out: string[] = []; + for (const opt of options) { + if (wanted.has(opt.value) && !seen.has(opt.value)) { + seen.add(opt.value); + out.push(opt.value); + } + } + return out; +} + +/** `reconcileSelection`'s return shape — see its doc comment for exactly what + * `deactivate`/`waveNeeded` mean. */ +export interface SelectionReconciliation { + value: string[]; + deactivate: boolean; + waveNeeded: boolean; +} + +/** + * Reconcile a previously COMMITTED selection against a fresh option list (the + * batch re-ran on a Dashboard refresh). Intersects `committed` with the values + * still present in `options`, then canonicalizes the survivors by the NEW option + * order — never re-introducing a value that is not in `committed`, because + * auto-select is never this function's job. + * + * - `deactivate` — true iff `committed` was non-empty and the intersection is + * empty: every previously selected value is gone, so the variable has nothing + * left to contribute and the caller returns it to unset rather than binding an + * empty selection. + * - `waveNeeded` — true iff the SET of values changed (some committed value was + * dropped because it is no longer a valid option); only then must the caller + * re-run the panels that declare this variable. A pure REORDER or a + * label-only change is `waveNeeded: false` even though `value`'s array order + * may differ from `committed`'s. + * + * Pure. + */ +export function reconcileSelection( + committed: readonly string[], + options: readonly { value: string }[], +): SelectionReconciliation { + const optionValues = new Set(options.map((o) => o.value)); + const committedUnique = Array.from(new Set(committed)); + const survivorsUnique = committedUnique.filter((v) => optionValues.has(v)); + return { + value: canonicalizeSelection(survivorsUnique, options), + deactivate: committedUnique.length > 0 && survivorsUnique.length === 0, + waveNeeded: survivorsUnique.length !== committedUnique.length, + }; +} diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index a2a799f6..f41a7a88 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -47,6 +47,8 @@ import { compileVariableOptionBatch, optionSqlDiagnostics, readVariableOptionBatch, } from '../../core/variable-options.js'; import type { VariableOption } from '../../core/variable-options.js'; +import { reconcileSelection, sameSelection } from '../../core/variable-selection.js'; +import { multiSelectElementType } from '../../core/param-type.js'; import { resolveAuthoredTimeRangeGroups } from '../../core/time-range.js'; import type { DashboardTimeRangeGroup } from '../../core/time-range.js'; import type { Diagnostic } from '../../core/diagnostics.js'; @@ -384,6 +386,12 @@ interface TileRuntime { interface FilterRuntime { def: { id: string; parameter: string }; state: ViewerFilterState; + /** Whether this variable binds a SELECTION (`Array(scalar T)` with a running + * option batch) rather than one scalar. Decided once, at construction, from + * the same pure predicate the bar renders its control on. Session-internal: + * the published `ViewerFilterState` carries no such flag, because a consumer + * can read the shape off `value` itself. */ + multiple: boolean; } /** The compiled option batch for this session, or `null` when no variable is @@ -405,13 +413,44 @@ const toValueString = (value: unknown): string => /** The UNSET value every variable starts at (#447): there are no persisted * defaults, so "no value yet" is the empty string, exactly like a cleared - * control. A variable's value is always a SCALAR — the multiselect/array - * machinery went with the curated filters. */ + * control. This is the ONE unset form, for a multi-select variable too — see + * `commitValue`. */ const UNSET_VALUE = ''; +/** + * The committed form of a proposed value. + * + * A multi-select variable's value is a real `string[]` end to end — the typed + * serializer builds the ClickHouse literal from it (`param-serialize.ts`), so + * escaping, big integers and empty-string elements are all already handled. Two + * rules are applied here, at the one place every write funnels through: + * + * - an EMPTY selection reduces to `UNSET_VALUE`. `param-pipeline`'s + * `emptyValue()` treats a present `[]` as a genuine value, so binding one + * would make every panel run `… IN []` — returning nothing while LOOKING + * filtered — where a Dashboard variable's unset contract is that its panels + * wait. This deliberately narrows #189, which could express an "active empty + * array"; under the inferred-variable model there are no defaults and no + * dormant values, so no control can author one; + * - a non-empty array is COPIED, so a caller's array can never be mutated out + * from under committed state. + */ +const commitValue = (value: unknown): unknown => + (Array.isArray(value) ? (value.length ? value.slice() : UNSET_VALUE) : value); + +/** Whether a proposed value counts as active: a selection by its length, a + * scalar by being non-empty. */ +const valueImpliesActive = (value: unknown): boolean => + (Array.isArray(value) ? value.length > 0 : value != null && value !== ''); + /** Local copy of `effectiveFilterActive` (state.ts is off-limits to this * layer): a param with an explicit activation entry uses it; otherwise a - * non-empty value counts as active. */ + * non-empty value counts as active. + * + * The value-derived pass is only ever a fallback for a name with NO entry in + * the `active` map, and `activeMap()` supplies one for every filter — so the + * array case cannot arise here in production and is deliberately not branched + * on. `commitValue` has already reduced an empty selection to `''` anyway. */ function effectiveActive( values: Record, active: Record, ): Record { @@ -585,10 +624,18 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa const issues = optionSqlDiagnostics(variable.sql); localOptionErrors.set(variable.name, issues.length ? issues.map((issue) => issue.message).join(' ') - // Unreachable via `optionBatchVariables`' own rule (a non-null `sql` is - // either acceptable, and so batched, or produces at least one diagnostic), - // kept as an honest message rather than an empty string. - : 'This variable’s option SQL cannot be used.'); + // Reached when the SQL itself is fine but the variable's TYPE cannot be + // option-backed — a `Tuple`/`Map`/`Nested`/nested-`Array` variable someone + // configured anyway. Its control is the plain input plus the + // no-inferred-control marker, so this message is the record that its stored + // SQL is deliberately not running rather than silently ignored. + // + // Before the `Array(scalar T)` multi-select was restored this branch also + // caught every well-formed `Array(T)` configuration, and was commented as + // unreachable — which was wrong, and set `status: 'error'` on a variable + // whose select simply never rendered. Admitting those into the batch is + // what made the comment true of the remaining containers only. + : 'This variable’s option SQL cannot be used: its type has no option list.'); } const configuredNames = new Set([...batchedNames, ...localOptionErrors.keys()]); const filters: FilterRuntime[] = bindable.map((variable) => { @@ -599,10 +646,26 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa const seed = deps.initialFilters ? deps.initialFilters[name] : undefined; const configured = configuredNames.has(name); const localError = localOptionErrors.get(name) ?? null; + // Whether this variable binds a SELECTION rather than one scalar — fixed + // here, at construction, like every other control decision, and from the + // same pure predicate `fieldControlKind` renders the multi-select on, so the + // session and the bar can never disagree about a variable's shape. + const multiple = batchedNames.has(name) && multiSelectElementType(variable.type ?? '') !== null; + // The store is untrusted, and a variable's type or its option SQL can change + // under an already-persisted value. A seed of the WRONG SHAPE for what this + // variable now binds would reach `serializeParamValue` as a `structural` + // error and block every panel that declares it, so it degrades to unset + // instead of being carried forward. + const seeded = seed !== undefined && Array.isArray(seed.value) === multiple + ? (seed.value ?? UNSET_VALUE) : UNSET_VALUE; const state: ViewerFilterState = { id: name, parameter: name, label: name, - active: seed !== undefined ? !!seed.active : false, - value: seed !== undefined ? (seed.value ?? UNSET_VALUE) : UNSET_VALUE, + // A selection carries its own activation: an array seed that survived the + // shape check is active iff it has elements. `commitValue` has already + // reduced an empty one to `''`, so this can never leave an `Array(T)` + // parameter active with a scalar `''` bound. + active: multiple ? valueImpliesActive(seeded) : (seed !== undefined && !!seed.active), + value: commitValue(seeded), // A batched variable is 'loading' from the very first publish: its control // exists but cannot offer a choice until the batch returns. One whose SQL // was rejected locally is already in its terminal state — no request will @@ -613,7 +676,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa options: null, optionsRev: 0, }; - return { def: { id: name, parameter: name }, state }; + return { def: { id: name, parameter: name }, state, multiple }; }); const filterById = new Map(filters.map((filter) => [filter.def.id, filter])); @@ -689,14 +752,25 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // Starts as one shared empty array so a Dashboard with no configured variable // never allocates. const NO_FILTER_DIAGNOSTICS: Diagnostic[] = []; + /** Shared empty return for every `runOptionBatch` path that reconciles + * nothing — the common case, so it never allocates. */ + const NO_RECONCILED: string[] = []; let filterDiagnostics: Diagnostic[] = NO_FILTER_DIAGNOSTICS; // Stale-wave guard for the options request, reserved BEFORE the token preflight // can yield — exactly like a tile's generation. Without that ordering a // superseded wave could still be the last one to publish its rows. let optionsGen = 0; - const rawValues = (): Record => - Object.fromEntries(filters.map((filter) => [filter.def.parameter, toValueString(filter.state.value)])); + // `unknown`, not `string`: a multi-select variable's committed value is a real + // `string[]` and must reach `serializeParamValue` as one — stringifying it here + // would hand the pipeline `"a,b"`, which binds as a single scalar. Everything + // downstream (`prepareBatch`, `prepareParameterizedBatch`) is already + // `unknown`-typed, so only this coercion had to go. + const rawValues = (): Record => + Object.fromEntries(filters.map((filter) => [ + filter.def.parameter, + Array.isArray(filter.state.value) ? filter.state.value.slice() : toValueString(filter.state.value), + ])); const activeMap = (): Record => Object.fromEntries(filters.map((filter) => [filter.def.parameter, filter.state.active])); @@ -878,8 +952,16 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa /** Apply one variable's fresh option list, bumping `optionsRev` only when the * CONTENT actually changed — so a consumer can distinguish a real refresh from * an unchanged republish (a same-length list with different members included, - * which a bare length or emptiness check would miss). */ - function applyOptions(filter: FilterRuntime, options: VariableOption[]): void { + * which a bare length or emptiness check would miss). + * + * Returns this variable's parameter name when reconciling its committed + * SELECTION against the fresh list actually changed the bound SET; the caller + * collects those names and runs ONE wave over their union. `null` otherwise — + * including for every scalar variable, whose off-list committed value is + * deliberately kept and shown verbatim (`filter-option-field.ts`'s documented + * leniency): a value that is still bound into panels is not something an + * option refresh gets to silently drop. */ + function applyOptions(filter: FilterRuntime, options: VariableOption[]): string | null { const previous = filter.state.options; const changed = previous === null || previous.length !== options.length @@ -888,6 +970,19 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa filter.state.status = 'ready'; filter.state.optionsError = null; if (changed) filter.state.optionsRev += 1; + if (!Array.isArray(filter.state.value)) return null; + const reconciled = reconcileSelection(filter.state.value, options); + if (!reconciled.waveNeeded) { + // A pure REORDER, or a label-only change: adopt the new option order so the + // trigger reads in list order, but nothing new is bound, so no panel re-runs. + if (!sameSelection(reconciled.value, filter.state.value)) filter.state.value = reconciled.value; + return null; + } + // A selected value is gone from the list. Never auto-select a replacement: + // `reconcileSelection` only ever filters and reorders what was already there. + filter.state.value = commitValue(reconciled.value); + if (reconciled.deactivate) filter.state.active = false; + return filter.def.parameter; } /** @@ -906,8 +1001,8 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa * opening ONE variable in its own main-editor tab and running it there (#457) * is the diagnostic path. */ - async function runOptionBatch(generation: number): Promise { - if (optionBatch === null) return; + async function runOptionBatch(generation: number): Promise { + if (optionBatch === null) return NO_RECONCILED; const result = newResult('Table', optionBatch.rowLimit); await deps.exec.executeRead(result, { sql: optionBatch.sql, @@ -915,7 +1010,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa rowLimit: optionBatch.rowLimit, params: { readonly: 2, max_result_bytes: VARIABLE_OPTION_BYTE_CAP }, }); - if (optionsGen !== generation || destroyed) return; // superseded + if (optionsGen !== generation || destroyed) return NO_RECONCILED; // superseded const failure = result.error != null || result.cancelled ? (result.error || 'Cancelled') : null; @@ -925,14 +1020,14 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // to look rather than left with a raw server error. markOptionsFailed(`Variable options could not be loaded: ${failure} ` + '— use Test in a variable’s editor to find the option SQL at fault.'); - return; + return NO_RECONCILED; } const read = readVariableOptionBatch( { columns: result.columns, rows: result.rows }, optionBatch.names, ); if (read.error !== null) { markOptionsFailed(read.error.message, read.error.code); - return; + return NO_RECONCILED; } // A variable whose list was cut off at the cap is reported once, as a warning // rather than an error: the options it DID return are usable, and the only @@ -943,15 +1038,24 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa message: `Only the first ${VARIABLE_OPTION_CAP.toLocaleString()} options are shown for ` + `${[...read.truncated].join(', ')}. Narrow the option SQL to see the rest.`, }]; + const reconciled: string[] = []; for (const filter of filters) { const options = read.byName.get(filter.def.id); if (options === undefined) continue; // not in this batch - applyOptions(filter, options); + const name = applyOptions(filter, options); + if (name !== null) reconciled.push(name); } // Publish as soon as the options land. Without this they would be invisible // until the caller's own post-wave publish — i.e. until the SLOWEST tile // finished, which inverts the whole point of running the two concurrently. publish(); + // The names are RETURNED, never re-run here: launching a wave while the tile + // pool is still in flight would supersede tiles mid-refresh and make the + // outcome classifier judge tiles that are already re-running. The single + // caller (`refresh`) runs ONE coalesced wave over the union once both halves + // have settled — which is what makes "at most one reconciled wave" structural + // rather than a flag someone has to remember to check. + return reconciled; } /** Publish a batch-level options failure: one Dashboard diagnostic, and every @@ -1047,7 +1151,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // returns, and gating the whole grid behind one extra round trip would delay // every panel for a list nothing is waiting on. Both are inside the `running` // window, so the refresh control stays busy until the options have landed too. - await Promise.all([ + const [reconciled] = await Promise.all([ runOptionBatch(optionsGeneration), runPool(runnable, VIEWER_TILE_CONCURRENCY, (runtime) => runTile(runtime, batch.get(runtime.tile.id)!, generations.get(runtime.tile.id)!)), @@ -1056,6 +1160,12 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa // published diagnostic and must not overwrite the last known-good tile // timestamp — the panels did refresh successfully. recordRefreshOutcome(runnable, waveMs); + // A refresh that dropped a selected value from some multi-select variable + // re-runs the panels that declare it — ONE coalesced wave over the union of + // every reconciled name, because `commitAndRerun` reserves generations across + // all of their targets before issuing a single `runAffectedWave` (the same + // coalescing clear-all uses). Runs only after both halves above have settled. + if (reconciled.length && !destroyed) await commitAndRerun(reconciled); publish(false, destroyed ? null : deps.now()); } @@ -1131,9 +1241,10 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa if (destroyed) return; const filter = filterById.get(filterId); if (!filter) return; - // A non-empty, non-nullish scalar counts as a value, so it activates. - filter.state.value = value; - filter.state.active = value != null && value !== ''; + // A non-empty value counts as a value, so it activates — by length for a + // selection, by emptiness for a scalar. + filter.state.value = commitValue(value); + filter.state.active = valueImpliesActive(value); publish(); await commitAndRerun([filter.def.parameter]); } @@ -1144,7 +1255,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa if (!filter) return; // The filter bar owns activation for optional fields, so value and active // are set independently (unlike setFilter's value-implies-active). - filter.state.value = value; + filter.state.value = commitValue(value); filter.state.active = active; publish(); await commitAndRerun([filter.def.parameter]); diff --git a/src/styles.css b/src/styles.css index cbf54b7e..3848b7bb 100644 --- a/src/styles.css +++ b/src/styles.css @@ -2061,11 +2061,69 @@ body.detached-tab .graph-overlay-panel { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; clip: rect(0, 0, 0, 0); white-space: nowrap; border: 0; } -/* The shared anchored-dialog backdrop (popover.ts's default `overlayClassName`). - #447 deleted the curated multiselect filter control this `.ms-` prefix came - from, along with every other `.ms-*` rule; this one survives because the - primitive still defaults to the name and the time-range popover mounts it. */ +/* Searchable multiselect Dashboard variable control (#189, multi-select-field.ts) + — a dedicated dialog popover rather than the single-select combobox + (.var-combo/.var-combo-list) forced into multiselect ARIA roles. The trigger + reuses .var-input's sizing/border so it sits flush with every other filter + field; the popover is its own `position:fixed` panel, the same + escape-the-scrolling-strip trick as .var-combo-list/.file-menu. + #447 deleted this block with the curated filter model; it came back with the + control, minus the per-field source-status rules (.is-waiting/.is-stale) that + belonged to the Filter-source machine rather than to the control. */ +.ms-field { grid-column: 2; display: inline-flex; } +.ms-trigger { + display: inline-flex; align-items: center; + width: calc(var(--var-input-ch, 16) * 1ch); max-width: 220px; + overflow: hidden; text-overflow: ellipsis; white-space: nowrap; + text-align: left; cursor: pointer; +} +.ms-trigger:hover { border-color: var(--accent); } +/* The batch-level failure affordance. The trigger carries `.var-input` too, so + `.var-input.is-error` above already paints it — only the affordance differs: + it stays `aria-disabled`, never `disabled`, so the reason in `title` remains + reachable by keyboard and screen reader, and the cursor says it won't open. */ +.ms-trigger.is-error { cursor: default; } +.ms-popover { + position: fixed; z-index: 70; width: 260px; max-width: calc(100vw - 24px); + display: flex; flex-direction: column; gap: 6px; padding: 8px; + background: var(--bg-editor); border: 1px solid var(--border); + border-radius: var(--r-md); box-shadow: var(--shadow-popover); + font-size: var(--text-body); font-family: var(--mono); +} +/* The shared anchored-dialog backdrop (popover.ts's default `overlayClassName`), + mounted by this control and by the time-range popover alike. */ .ms-overlay { position: fixed; inset: 0; z-index: 60; } +.ms-search { + height: 24px; padding: 0 8px; background: var(--bg-input); color: var(--fg); + border: 1px solid var(--border); border-radius: var(--r-sm); font: inherit; +} +.ms-search:focus { + outline: none; border-color: var(--accent); + box-shadow: var(--ring); +} +.ms-select-all { + display: flex; align-items: center; gap: 6px; cursor: pointer; + padding: 2px 4px; color: var(--fg-mute); font-size: var(--text-label); + border-bottom: 1px solid var(--border-faint); +} +.ms-select-all-cb, .ms-option input[type="checkbox"] { accent-color: var(--accent); flex-shrink: 0; cursor: pointer; } +.ms-options { display: flex; flex-direction: column; max-height: 220px; overflow-y: auto; gap: 1px; } +.ms-option { + display: flex; align-items: center; gap: 6px; padding: 5px 4px; + border-radius: var(--r-sm); cursor: pointer; color: var(--fg); +} +.ms-option:hover { background: var(--bg-hover); } +.ms-option[hidden] { display: none; } +.ms-option-label { overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } +.ms-footer { display: flex; justify-content: flex-end; gap: 6px; padding-top: 4px; border-top: 1px solid var(--border-faint); } +.ms-btn { + height: 24px; padding: 0 10px; border-radius: var(--r-sm); cursor: pointer; + font: inherit; font-size: var(--text-label); + border: 1px solid var(--border); background: transparent; color: var(--fg); +} +.ms-btn:hover { background: var(--bg-hover); } +.ms-btn-clear { margin-right: auto; } +.ms-live { /* sr-only announcer; .sr-only above carries the positioning */ } /* Compound time-range control (#335, time-range-field.ts) — the SECOND consumer of the #364 dialog-popover pattern (openAnchoredDialog). The trigger reuses .var-input's sizing/border; the popover is @@ -2134,21 +2192,22 @@ body.detached-tab .graph-overlay-panel { border: 1px solid var(--border); background: transparent; color: var(--fg); } .trf-btn:hover { background: var(--bg-hover); } -/* Primary-action state model for the time-range popover's Apply (#386 - follow-up; #447 dropped the `.ms-btn-primary` half — the curated multiselect - filter popover it also covered is gone). This block follows the generic - `.trf-btn` hover rule above so neutral hover chrome can never wash an enabled - Apply button into a disabled-looking surface. */ -.trf-btn-primary { border: none; background: var(--accent); color: #fff; font-weight: var(--fw-semibold); } -.trf-btn-primary:hover:not(:disabled) { background: var(--accent-dim); } -.trf-btn-primary:focus-visible { +/* Primary-action state model for BOTH filter popovers' Apply (#386 follow-up): + the multiselect's and the time-range control's. One selector list, never two + parallel blocks — the two buttons are the same affordance and must never + drift apart. This block follows the generic `.trf-btn`/`.ms-btn` hover rules + above so neutral hover chrome can never wash an enabled Apply button into a + disabled-looking surface. */ +.ms-btn-primary, .trf-btn-primary { border: none; background: var(--accent); color: #fff; font-weight: var(--fw-semibold); } +.ms-btn-primary:hover:not(:disabled), .trf-btn-primary:hover:not(:disabled) { background: var(--accent-dim); } +.ms-btn-primary:focus-visible, .trf-btn-primary:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; box-shadow: 0 0 0 2px var(--bg-editor); } -.trf-btn-primary:active:not(:disabled) { +.ms-btn-primary:active:not(:disabled), .trf-btn-primary:active:not(:disabled) { background: var(--accent-dim); transform: translateY(1px); } -.trf-btn-primary:disabled { +.ms-btn-primary:disabled, .trf-btn-primary:disabled { border: 1px solid var(--border); background: var(--bg-input); color: var(--fg-faint); cursor: not-allowed; opacity: 1; } diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index 8bfc60b0..0a26ce0d 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -879,7 +879,14 @@ export async function renderDashboard( // direct input; `[]` means option-backed with nothing to offer yet. const variables: Record = {}; for (const f of sview.filters) { - draftValues[f.parameter] = valueString(f.value); + // An `Array(scalar T)` variable's committed value is a real `string[]`. + // It must NOT be flattened into the shared scalar draft bag — `String()` + // would turn `['a','b']` into `"a,b"`, a value nothing can round-trip — + // so it travels on the spec instead and its draft slot stays unset. That + // bag is `Record` precisely so the Workbench var-strip, + // which shares its shape, can never be handed an array. + const selection = Array.isArray(f.value) ? f.value.filter((v): v is string => typeof v === 'string') : null; + draftValues[f.parameter] = selection === null ? valueString(f.value) : ''; draftActive[f.parameter] = f.active; idByParam.set(f.parameter, f.id); variables[f.parameter] = { @@ -888,6 +895,7 @@ export async function renderDashboard( // whose option SQL was rejected locally has a specific problem, and the // batch failure is only its reason when it was actually in that batch. optionsError: f.optionsError, + ...(selection === null ? {} : { selection }), }; } const onCommit = (name: string): void => { @@ -900,6 +908,14 @@ export async function renderDashboard( const id = idByParam.get(name); if (id) void session.applyFilter(id, value, active); }; + // The multi-select's Apply. Same "complete, deliberate action" semantics as + // the single-select above; the only difference is that the value is a real + // array, which `applyFilter` already accepts (`value: unknown`) and the + // session reduces to unset when it is empty. + const onCommitVariableSelection = (name: string, values: string[], active: boolean): void => { + const id = idByParam.get(name); + if (id) void session.applyFilter(id, values, active); + }; const getField = (name: string, mode: ValidationMode) => session.getFilterField(name, mode, draftValues, draftActive); // #335: assemble the time-range option — one entry per resolved group, // reading each bound's committed value/active straight off `sview.filters` @@ -931,6 +947,7 @@ export async function renderDashboard( const bar = buildFilterBar( filterBarApp, session.controls, onCommit, getField, { document: doc, timeRange, onApplyTimeRange, variables, onCommitVariable, + onCommitVariableSelection, onKeyboardOwnerChange: keyboardOwnerChannel(app) }, ); timeFilterHost.replaceChildren(bar.timeEl); @@ -2115,11 +2132,20 @@ export async function renderDashboard( let lastLabelWaveNowMs: number | null = session.state.value.waveWallNowMs; // #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. - // #447: a variable's committed value is a SCALAR string — the array shape the - // #189 multiselect persisted is gone, so there is nothing to narrow here. + // A multi-select variable's committed value is a real `string[]` and is + // persisted as one — `dashboard-filter-store.ts` has round-tripped arrays + // since #189 (`value: string | string[]`, with an array-aware coerce that + // drops non-string elements rather than stringifying them), so a selection + // survives a reload without ever becoming the joined `"a,b"` that + // `valueString`'s `String()` fallback would produce. const persistBagOf = (filters: readonly ViewerFilterState[]): DashboardFilterBag => { const bag: DashboardFilterBag = {}; - for (const f of filters) bag[f.id] = { value: valueString(f.value), active: f.active }; + for (const f of filters) { + bag[f.id] = { + value: Array.isArray(f.value) ? f.value.map(valueString) : valueString(f.value), + active: f.active, + }; + } return bag; }; // #303: a SEPARATE signature from `barSig` above — that one also flips when diff --git a/src/ui/filter-bar.ts b/src/ui/filter-bar.ts index 0fc1d083..3fecc5d6 100644 --- a/src/ui/filter-bar.ts +++ b/src/ui/filter-bar.ts @@ -13,10 +13,16 @@ // either a strict single-select combobox (`filter-option-field.ts`) or a // multiselect dialog (`multi-select-field.ts`), with a per-field source status // affordance. A Dashboard's variables are now inferred from `{name:Type}` -// placeholders in panel-owned queries and every field is a plain direct input, -// so only the plain branch below survives — including the compound `#335` -// time-range control, which is a presentation of two plain bounds, not a -// curated field. +// placeholders in panel-owned queries, so the per-field SOURCE STATUS machine +// and everything that fed it are gone for good. +// +// The two option-backed CONTROLS came back, now driven by a variable's own +// Dashboard-local option SQL rather than by a Filter-role query: a scalar +// variable gets the strict single-select, and an `Array(scalar T)` variable +// gets the searchable multiselect (restored from #189). Which one — or neither +// — is one decision, `fieldControlKind`'s: it classifies the TYPE, and this +// module pairs that verdict with whether option SQL was actually configured, +// because only the bar can see the spec. import { h } from './dom.js'; import { fieldControlKind } from '../core/param-pipeline.js'; @@ -33,6 +39,7 @@ import { wireComboInput } from './combobox.js'; import type { ComboField } from './combobox.js'; import { buildTimeRangeField } from './time-range-field.js'; import { buildFilterOptionField } from './filter-option-field.js'; +import { buildMultiSelectField } from './multi-select-field.js'; import { Icon } from './icons.js'; import type { KeyboardOwner } from './app.types.js'; import type { VariableOption } from '../core/variable-options.types.js'; @@ -101,6 +108,11 @@ export interface BuildFilterBarOptions { * complete, deliberate action), where `onCommit` only names the parameter and * lets the caller read the shared draft bag. */ onCommitVariable?(name: string, value: string, active: boolean): void; + /** Fires when a MULTI-select Apply commits. Separate from `onCommitVariable` + * because the value is a real `string[]`: `FilterBarApp.state.varValues` is + * deliberately still `Record` (the Workbench var-strip owns + * and persists that same bag), so an array never round-trips through it. */ + onCommitVariableSelection?(name: string, values: string[], active: boolean): void; } /** #447 phase 2: how ONE Dashboard variable's control differs from a plain @@ -113,6 +125,11 @@ export interface VariableFieldSpec { options: readonly VariableOption[] | null; /** Non-null when the option batch failed: the select renders unavailable. */ optionsError?: string | null; + /** The committed SELECTION for a variable rendered as a multi-select + * (`Array(scalar T)` with options). Empty means unset. Carried here rather + * than in `app.state.varValues` on purpose — see + * `onCommitVariableSelection`. Ignored by every other control branch. */ + selection?: readonly string[]; } /** A per-field execution-status update (`status`/`stale`/`waitingFor` mirror @@ -346,26 +363,80 @@ export function buildFilterBar( const specOf = (name: string): VariableFieldSpec | undefined => (variables ? variables[name] : undefined); - /** The marker shown beside a variable whose declared type is a CONTAINER - * (`Array`/`Tuple`/`Map`/`Nested`): there is no inferred single-scalar control - * for it, and no option list can back it. + /** The marker shown beside a variable the Dashboard cannot infer a control for. + * + * Two reasons reach it, and they say different things: + * - a CONTAINER with no flat element list (`Tuple`/`Map`/`Nested`, or a + * nested `Array(Array(T))` the serializer rejects outright) can never have + * a control inferred, whatever the author does; + * - an `Array(scalar T)` with no option SQL has no LIST to pick from yet. + * Its type is perfectly controllable — configuring option SQL turns it + * into the multiselect — so saying "container type" would be misleading + * advice. It names the fix instead. * - * It ADORNS the plain field rather than replacing it. Removing the input - * outright would make an existing Dashboard strictly less capable — a + * Either way it ADORNS the plain field rather than replacing it. Removing the + * input outright would make an existing Dashboard strictly less capable — a * container-typed variable already rendered a free-text field, and * `param-serialize.ts` binds an array literal typed into it perfectly well, so * taking it away would leave those panels permanently `unfilled` with no way to * fill them. The marker says the Dashboard cannot infer a control; it does not * claim the value is unusable. */ - const unsupportedMarker = (p: FieldControl, type: string): HTMLElement => + const unsupportedMarker = (p: FieldControl, type: string, listable = false): HTMLElement => h('span', { class: 'var-unsupported', role: 'img', - 'aria-label': `${p.name} has no inferred control: ${type} is a container type — type a literal value`, - title: `A Dashboard cannot infer a control for ${type}, which is a container type. ` - + 'Type a literal value directly.', + 'aria-label': listable + ? `${p.name} has no option list: add option SQL to pick from a list, or type a literal value` + : `${p.name} has no inferred control: ${type} is a container type — type a literal value`, + title: listable + ? `A Dashboard has no option list for ${type}. Add option SQL to this variable to pick ` + + 'values from a list, or type a literal value directly.' + : `A Dashboard cannot infer a control for ${type}, which is a container type. ` + + 'Type a literal value directly.', }, Icon.eyeOff(), type); + /** The searchable multiselect over one `Array(scalar T)` variable's batched + * option rows (#189, restored). Its committed value is a real `string[]`, so + * — unlike every other branch here — it never touches `app.state.varValues`: + * that bag is `Record` and is shared with the Workbench's own + * variables strip. The selection arrives on the spec and leaves on + * `onCommitVariableSelection`. */ + const buildMultiField = (p: FieldControl, spec: VariableFieldSpec): HTMLElement => { + const field = buildMultiSelectField({ + document, + name: p.name, + options: spec.options ?? [], + selected: spec.selection ?? [], + active: !!app.state.filterActive[p.name], + title: p.name + ': ' + p.type, + // Apply is a complete, deliberate action, so it bypasses the keystroke + // debounce entirely — same reasoning as the single-select's own commit. + // Activation travels with the value: a non-empty selection is active, and + // Clear-then-Apply returns the variable to unset. + onApply: (values, active) => { + app.state.filterActive[p.name] = active; + options.onCommitVariableSelection?.(p.name, values, active); + }, + onKeyboardOwnerChange: options.onKeyboardOwnerChange, + }); + if (spec.optionsError != null) field.setUnavailable(spec.optionsError); + handles.set(p.name, { + el: field.el, + // Like the single-select: no transient per-field status of its own, its + // only failure mode being the batch's, which arrives through `setOptions`. + updateStatus: () => {}, + setOptions: (next, error) => { + field.setOptions(next); + field.setUnavailable(error); + }, + isOpen: field.isOpen, + focusTrigger: field.focusTrigger, + dispose: field.dispose, + }); + return h('label', { class: 'var-field' }, + h('span', { class: 'var-name' }, p.name), field.el); + }; + /** The strict single-select over one variable's batched option rows. */ const buildOptionField = (p: FieldControl, spec: VariableFieldSpec): HTMLElement => { const field = buildFilterOptionField({ @@ -513,17 +584,28 @@ export function buildFilterBar( // #447 phase 2 adds two variable-only branches ahead of the plain one; with no // `variables` map every param still reaches `buildParamField` unchanged. const buildField = (p: FieldControl): HTMLElement => { - // The container verdict comes from the SAME shared decision `buildParamField` + // The type verdict comes from the SAME shared decision `buildParamField` // consults, rather than being passed in by the caller — one decision point, so // the control a Dashboard renders and the policy that classified its type can - // never disagree. It wins over an option list: a two-String-column list cannot - // supply a container value, so offering a select for one would be a lie. - if (fieldControlKind(p, null, { scalarControls: !!variables }).kind === 'unsupported') { + // never disagree. `multiSelectElementType`, which produces `'multi'` here, is + // also the predicate `core/variable-options.js` filters the option batch on, + // so a type that gets a select can never be one whose option SQL was skipped. + const kind = fieldControlKind(p, null, { scalarControls: !!variables }).kind; + // A container with no flat element list: no control is inferable at all. + if (kind === 'unsupported') { const field = buildParamField(p); field.appendChild(unsupportedMarker(p, p.type)); return field; } const spec = specOf(p.name); + if (kind === 'multi') { + // The type CAN be multi-selected; whether there is anything to select from + // is the spec's answer, which only this layer can see. + if (spec && spec.options !== null) return buildMultiField(p, spec); + const field = buildParamField(p); + field.appendChild(unsupportedMarker(p, p.type, true)); + return field; + } if (spec && spec.options !== null) return buildOptionField(p, spec); return buildParamField(p); }; diff --git a/src/ui/multi-select-field.ts b/src/ui/multi-select-field.ts new file mode 100644 index 00000000..3efd52ad --- /dev/null +++ b/src/ui/multi-select-field.ts @@ -0,0 +1,288 @@ +// The searchable multiselect control for an option-backed Dashboard variable +// whose declared type is `Array(scalar T)` — a full ARIA `dialog` popover +// (search + tri-state "select visible" + a native-labeled checklist + +// Clear/Cancel/Apply) rather than forcing the single-select combobox primitive +// (combobox.ts) into multiselect semantics it was never built for. +// +// Restored from #189/PR #364, which #447 phase 1 deleted with the curated filter +// model. What came back is the CONTROL; what did not is everything that belonged +// to curated filters — a `label` separate from the name (a variable has only its +// exact name), `required` (every inferred variable is optional-by-blank, so the +// inactive trigger always reads `Not set`), the Filter-source status machine +// (`idle`/`loading`/`waiting`/`waitingFor`/`stale` — option SQL cannot reference +// a variable, so nothing ever waits on an upstream control), and the error-mode +// plain-text fallback with its `onFallbackCommit`. The one failure this model has +// is the BATCH-level one, which arrives through `setUnavailable` exactly as it +// does on the single-select sibling. +// +// Two existing primitives are borrowed rather than reinvented: +// - `popover.ts`'s `openAnchoredDialog` (#335 — itself extracted FROM this +// control's own `openPopover`) owns the generic dialog chrome: a fresh +// overlay + panel mounted on open and torn down completely on close, the +// ARIA `dialog`/`aria-modal`/`aria-expanded` lifecycle, Escape and backdrop +// close, the Tab focus trap, `fixedAnchor` placement, and focus return; +// - `filter-option-field.ts`'s `setUnavailable` is the model for the +// batch-failure affordance (`aria-disabled` + `aria-invalid` + the reason in +// `title`, never `disabled`). +// +// State ownership: the committed `selected`/`active` are frozen at construction +// — a caller wanting a later committed-value change reflected rebuilds the +// field, the same convention `buildFilterBar` uses everywhere else. `options` +// and the unavailable reason mutate in place (`setOptions`/`setUnavailable`), +// because the option batch lands asynchronously and a rebuild would discard an +// unrelated sibling's in-progress typing. +// +// The OPEN popover owns its own draft `Set` (copied from `selected` at +// open time) plus all its DOM and listeners, local to `openPopover()` — none of +// it survives the matching `close()`, so repeated opens leak nothing. + +import { h } from './dom.js'; +import { openAnchoredDialog } from './popover.js'; +import { idSafe } from './combobox.js'; +import { canonicalizeSelection, sameSelection } from '../core/variable-selection.js'; +import type { VariableOption } from '../core/variable-options.types.js'; +import type { KeyboardOwner } from './app.types.js'; + +/** `buildMultiSelectField`'s options bag. */ +export interface MultiSelectFieldOpts { + /** Injected document realm — defaults to the ambient global. */ + document?: Document; + /** The variable's exact name: its id-safe DOM ids, its accessible names, and + * its visible label are all built from this. A Dashboard variable has no + * label separate from its name (#447). */ + name: string; + /** The batched option rows. May be empty — zero rows is a legal result. */ + options: readonly VariableOption[]; + /** Committed selection. May contain values absent from `options` (a dormant + * value a refresh dropped); never mutated by this module. */ + selected: readonly string[]; + active: boolean; + /** The trigger's resting `title` — `name: Type`, from the bar. */ + title?: string; + onApply(next: string[], active: boolean): void; + onKeyboardOwnerChange?: (owner: KeyboardOwner | null) => void; +} + +/** `buildMultiSelectField`'s return value. */ +export interface MultiSelectFieldHandle { + el: HTMLElement; + /** Swap the option list in place when a refresh's batch lands. An OPEN + * popover is closed as a Cancel first — its draft was built against the + * previous generation and must never be applied against this one. */ + setOptions(next: readonly VariableOption[]): void; + /** The batch-level failure affordance: `null` clears it. */ + setUnavailable(reason: string | null): void; + /** Whether the popover is currently open — a caller reads this to decide + * whether an options refresh needs to announce that it cancelled a draft. */ + isOpen(): boolean; + /** Focuses the trigger — used by a caller that just rebuilt the bar a + * still-open popover was force-closed out from under, to land focus on the + * fresh field rather than ``. */ + focusTrigger(): void; + /** Removes this control's listeners and closes the popover if open (a + * dispose-while-open is a Cancel: no `onApply`). */ + dispose(): void; +} + +export function buildMultiSelectField(opts: MultiSelectFieldOpts): MultiSelectFieldHandle { + const d = opts.document || document; + const { name, selected, active } = opts; + const baseTitle = opts.title ?? name; + const suffix = idSafe(name); + + let options: readonly VariableOption[] = opts.options; + let unavailable: string | null = null; + // The currently-open popover's own close() — non-null iff the popover is open + // (`isOpen()` reads this directly rather than tracking a second flag). + let closeCurrent: ((closeOpts?: { skipFocus?: boolean }) => void) | null = null; + + const trigger = h('button', { + type: 'button', id: 'ms-trigger-' + suffix, class: 'ms-trigger var-input', + 'aria-haspopup': 'dialog', 'aria-expanded': 'false', + }) as HTMLButtonElement; + // Same wrapper convention as `buildFilterOptionField`'s `.var-combo`: the + // grid-column:2 sizing anchor and the status "wrapper" are one node. + const control = h('div', { class: 'ms-field' }, trigger); + + /** `Not set` when nothing is committed; the single option's LABEL when exactly + * one is (a value with no matching option still shows raw, so a dormant + * selection never reads as blank); `N selected` beyond that. */ + const triggerText = (): string => { + if (!active || selected.length === 0) return 'Not set'; + if (selected.length === 1) { + const opt = options.find((o) => o.value === selected[0]); + return opt ? opt.label : selected[0]; + } + return `${selected.length} selected`; + }; + + const render = (): void => { + const text = triggerText(); + trigger.textContent = text; + trigger.title = unavailable ?? baseTitle; + trigger.setAttribute('aria-label', `${name} filter, ${selected.length} selected`); + }; + + const onTriggerClick = (): void => { if (!unavailable) openPopover(); }; + trigger.addEventListener('click', onTriggerClick); + + // Mount a fresh popover. The generic dialog chrome lives in + // `openAnchoredDialog`; this builds only the multiselect content + draft and + // wires the Apply/close ordering on top of it. + function openPopover(): void { + if (closeCurrent) return; // already open — never stack a second popover + const draft = new Set(selected); + let searchText = ''; + + const liveEl = h('div', { class: 'sr-only ms-live', 'aria-live': 'polite' }); + const searchInput = h('input', { + type: 'text', class: 'ms-search', placeholder: `Search ${name} options`, + 'aria-label': `Search ${name} options`, + }) as HTMLInputElement; + const selectAllCb = h('input', { type: 'checkbox', class: 'ms-select-all-cb' }) as HTMLInputElement; + const selectAllRow = h('label', { class: 'ms-select-all' }, selectAllCb, h('span', {}, 'Select visible')); + + const rows = options.map((opt) => { + const cb = h('input', { type: 'checkbox', checked: draft.has(opt.value) }) as HTMLInputElement; + cb.addEventListener('change', () => { + if (cb.checked) draft.add(opt.value); else draft.delete(opt.value); + syncSelectAll(); + }); + const li = h('label', { class: 'ms-option' }, cb, h('span', { class: 'ms-option-label' }, opt.label)); + return { opt, li, cb }; + }); + const listEl = h('div', { class: 'ms-options' }, ...rows.map((r) => r.li)); + + // Tri-state "select visible": unchecked when no visible row is in the draft, + // checked when every visible row is, indeterminate when some are — and the + // accessible label always names the ACTION a click performs next. Native + // indeterminate→click sets `checked = true`, so setting `.checked` to + // `allSelected` here (not `selected > 0`) is what makes a later click + // reliably SELECT — never re-clear — a mixed selection. + function syncSelectAll(): void { + const visibleRows = rows.filter((r) => !r.li.hidden); + const total = visibleRows.length; + const picked = visibleRows.filter((r) => draft.has(r.opt.value)).length; + const allSelected = total > 0 && picked === total; + const noneSelected = picked === 0; + selectAllCb.checked = allSelected; + selectAllCb.indeterminate = !allSelected && !noneSelected; + selectAllCb.setAttribute('aria-label', + allSelected ? `Clear all ${total} visible options` : `Select all ${total} visible options`); + } + // Local case-insensitive substring filter over label AND value — hidden + // (filtered-out) rows are never touched by select-visible below. + function applyFilter(): void { + const q = searchText.trim().toLowerCase(); + let visible = 0; + for (const row of rows) { + const match = !q || row.opt.label.toLowerCase().includes(q) || row.opt.value.toLowerCase().includes(q); + row.li.hidden = !match; + if (match) visible++; + } + liveEl.textContent = `${visible} of ${rows.length} options`; + syncSelectAll(); + } + searchInput.addEventListener('input', () => { searchText = searchInput.value; applyFilter(); }); + selectAllCb.addEventListener('change', () => { + const checked = selectAllCb.checked; + for (const row of rows) { + if (row.li.hidden) continue; // hidden values are never touched + row.cb.checked = checked; + if (checked) draft.add(row.opt.value); else draft.delete(row.opt.value); + } + syncSelectAll(); + }); + + const clearBtn = h('button', { type: 'button', class: 'ms-btn ms-btn-clear' }, 'Clear') as HTMLButtonElement; + const cancelBtn = h('button', { type: 'button', class: 'ms-btn' }, 'Cancel') as HTMLButtonElement; + const applyBtn = h('button', { type: 'button', class: 'ms-btn ms-btn-primary' }, 'Apply') as HTMLButtonElement; + // Clear empties the WHOLE draft, not just the visible subset. + clearBtn.addEventListener('click', () => { + draft.clear(); + for (const row of rows) row.cb.checked = false; + syncSelectAll(); + }); + cancelBtn.addEventListener('click', () => handle.close()); + applyBtn.addEventListener('click', () => { + const canonical = canonicalizeSelection([...draft], options); + const prevCanonical = canonicalizeSelection(selected, options); + const activeNext = canonical.length > 0; + // A no-op Apply (same canonical selection AND same active flag) closes + // silently — `onApply` fires exactly once otherwise. + const changed = !(sameSelection(canonical, prevCanonical) && activeNext === active); + // Close BEFORE calling `onApply` — the shared `openAnchoredDialog` + // contract (#335, originally this control's own merge-gate finding for + // #189). `onApply` routes into `session.applyFilter`, which mutates state + // and `publish()`es SYNCHRONOUSLY; a subscriber (`dashboard.ts`'s + // `rebuildFilterBar`) can run inside this very call stack, and it must + // observe this popover as ALREADY closed — never mistake an ordinary + // commit for a force-cancelled outgoing popover, which is what used to + // fire a false "options were refreshed" announcement. + handle.close(); + if (changed) opts.onApply(canonical, activeNext); + }); + const footer = h('div', { class: 'ms-footer' }, clearBtn, cancelBtn, applyBtn); + + // A `display:contents` wrapper: `openAnchoredDialog` appends ONE content + // element, but `.ms-popover` is a flex column whose direct children carry + // the layout — the contents wrapper generates no box, so they participate in + // the dialog's flex context exactly as if they were direct children. + const content = h('div', { style: { display: 'contents' } }, + searchInput, liveEl, selectAllRow, listEl, footer); + + const handle = openAnchoredDialog({ + document: d, + trigger, + ariaLabel: `${name} options`, + content, + dialogClassName: 'ms-popover', + overlayClassName: 'ms-overlay', + minWidthFromTrigger: true, + initialFocus: () => searchInput, // focus moves into the dialog on open + onClose: () => { closeCurrent = null; }, + onKeyboardOwnerChange: opts.onKeyboardOwnerChange, + }); + closeCurrent = (closeOpts) => handle.close(closeOpts); + + applyFilter(); // seeds the live-region count and the select-visible tri-state + } + + render(); + + return { + el: control, + isOpen: () => closeCurrent !== null, + setOptions: (next) => { + options = next; + // A draft built against the PREVIOUS generation can never be applied + // against this one, so an open popover is cancelled outright. `skipFocus` + // is deliberately NOT passed: the trigger survives an options swap (unlike + // the deleted error-mode input swap), so focus belongs back on it. + closeCurrent?.(); + // Re-render the committed selection's LABEL: the same value may now carry + // a different label. Never re-commits — a refresh is not a user choice. + render(); + }, + setUnavailable: (reason) => { + unavailable = reason; + // `aria-disabled` + `aria-invalid`, never `disabled`: a disabled button is + // not focusable, so its `title` — the only place the reason is written — + // becomes unreachable by keyboard and screen reader, and disabling a + // focused control silently drops focus to ``. `onTriggerClick` + // refuses to open while a reason is set, which is the real gate. + control.classList.toggle('is-error', reason !== null); + trigger.classList.toggle('is-error', reason !== null); + if (reason === null) trigger.removeAttribute('aria-invalid'); + else trigger.setAttribute('aria-invalid', 'true'); + trigger.setAttribute('aria-disabled', reason === null ? 'false' : 'true'); + if (reason !== null) closeCurrent?.(); + render(); + }, + focusTrigger: () => trigger.focus(), + dispose: () => { + closeCurrent?.(); // dispose-while-open is a Cancel: no writes + trigger.removeEventListener('click', onTriggerClick); + }, + }; +} diff --git a/tests/e2e/multi-select.html b/tests/e2e/multi-select.html new file mode 100644 index 00000000..e3391693 --- /dev/null +++ b/tests/e2e/multi-select.html @@ -0,0 +1,27 @@ + + + + + Multi-select control harness + + + + +
+ + + + diff --git a/tests/e2e/multi-select.spec.js b/tests/e2e/multi-select.spec.js new file mode 100644 index 00000000..29baadef --- /dev/null +++ b/tests/e2e/multi-select.spec.js @@ -0,0 +1,133 @@ +import { test, expect } from '@playwright/test'; + +// Real-browser regressions for the restored Array(T) multi-select. happy-dom has +// no :focus-visible, no computed box, and no native Tab traversal, so these two +// concerns can only be checked in a real engine. +// +// The #439 busy/`reclaimFocus` describe from the original #189 suite is gone with +// the feature it drove: this control has no per-field busy state (a variable's +// only failure is the batch's, and `setUnavailable` closes the popover outright +// rather than making it noninteractive). `popover.test.ts` still covers +// `reclaimFocus` at the primitive level for the time-range consumer. + +const open = async (page) => { + await page.getByRole('button', { name: 'city filter, 0 selected' }).click(); + await expect(page.getByRole('dialog', { name: 'city options' })).toBeVisible(); +}; + +test.describe('Multi-select keyboard traversal', () => { + test.beforeEach(async ({ page }) => { + await page.goto('/tests/e2e/multi-select.html'); + await page.waitForFunction(() => window.__ready === true); + }); + + // The shared anchored-dialog focus trap must own every Tab/Shift+Tab + // transition rather than delegate middle-of-list traversal to the browser + // (WebKit's default "Tab highlights every item" preference is OFF, so native + // traversal there can skip checkboxes/buttons entirely). This proves the real + // multi-select consumer sequence in every engine, not just the primitive in + // isolation (see popover.test.ts for the unit-level coverage). + test('Tab traverses the real control sequence in DOM order, reaches Apply, and wraps both ways', async ({ page }) => { + await open(page); + const dialog = page.getByRole('dialog', { name: 'city options' }); + + const search = page.getByPlaceholder('Search city options'); + const selectVisible = page.locator('.ms-select-all-cb'); + const optionCb = page.locator('.ms-option input[type="checkbox"]').first(); + const clear = page.getByRole('button', { name: 'Clear', exact: true }); + const cancel = page.getByRole('button', { name: 'Cancel', exact: true }); + const apply = page.getByRole('button', { name: 'Apply', exact: true }); + + // Initial focus lands on Search. + await expect(search).toBeFocused(); + + // Ordinary Tab visits every declared control, in order, ending on Apply. + for (const next of [selectVisible, optionCb, clear, cancel, apply]) { + await page.keyboard.press('Tab'); + await expect(next).toBeFocused(); + const inDialog = await dialog.evaluate((d) => d.contains(document.activeElement)); + expect(inDialog).toBe(true); + } + + // Apply reached via ordinary Tab visibly matches :focus-visible. + expect(await apply.evaluate((el) => el.matches(':focus-visible'))).toBe(true); + expect(await apply.evaluate((el) => getComputedStyle(el).outlineStyle)).not.toBe('none'); + + // One more Tab wraps forward to Search; Shift+Tab wraps back to Apply. + await page.keyboard.press('Tab'); + await expect(search).toBeFocused(); + await page.keyboard.press('Shift+Tab'); + await expect(apply).toBeFocused(); + }); + + // Escape is a Cancel on every engine, and focus must return to the trigger + // rather than fall through to behind the dismissed modal. + test('Escape closes as a Cancel and returns focus to the trigger', async ({ page }) => { + await open(page); + await page.locator('.ms-option input[type="checkbox"]').first().check(); + await page.keyboard.press('Escape'); + await expect(page.getByRole('dialog', { name: 'city options' })).toHaveCount(0); + await expect(page.getByRole('button', { name: 'city filter, 0 selected' })).toBeFocused(); + // The draft was discarded — the trigger still reads unset. + await expect(page.locator('.ms-trigger')).toHaveText('Not set'); + }); + + // A row hidden by the search filter must be genuinely unfocusable, not merely + // visually gone — otherwise Tab lands on an invisible checkbox. + test('a search-hidden option row is skipped by Tab', async ({ page }) => { + await open(page); + await page.getByPlaceholder('Search city options').fill('zzz'); + await expect(page.locator('.ms-option')).toBeHidden(); + await page.keyboard.press('Tab'); // Select visible + await page.keyboard.press('Tab'); // would be the option row, if it were focusable + await expect(page.getByRole('button', { name: 'Clear', exact: true })).toBeFocused(); + }); +}); + +test.describe('Multi-select Apply action states (#386)', () => { + test.beforeEach(async ({ page }) => { + await page.goto('/tests/e2e/multi-select.html'); + await page.waitForFunction(() => window.__ready === true); + }); + + // The multiselect's Apply and the time-range popover's share one selector list + // in styles.css; this pins that the shared rule actually resolves for the + // `.ms-` half, in both themes. + for (const theme of ['dark', 'light']) { + test(`keeps enabled, hover, disabled, focus, and pressed Apply states distinct in ${theme} theme`, async ({ page }) => { + await page.locator('body').evaluate((el, nextTheme) => { el.dataset.theme = nextTheme; }, theme); + await open(page); + + const apply = page.getByRole('button', { name: 'Apply', exact: true }); + const disabled = page.getByRole('button', { name: 'Disabled Apply', exact: true }); + const styles = (target) => target.evaluate((el) => { + const css = getComputedStyle(el); + return { background: css.backgroundColor, color: css.color, outline: css.outlineStyle, transform: css.transform }; + }); + + const enabled = await styles(apply); + expect(enabled.background).not.toBe('rgba(0, 0, 0, 0)'); + await apply.hover(); + const hovered = await styles(apply); + expect(hovered.background).not.toBe(enabled.background); + expect(await styles(disabled)).not.toEqual(hovered); + + // Enter focus through keyboard navigation so :focus-visible is the state + // under test; programmatic focus intentionally does not promise that + // modality in browsers. The shared focus trap owns every transition + // deterministically, so the bounded loop is only a defensive bound. + for (let i = 0; i < 10 && !(await apply.evaluate((el) => el === document.activeElement)); i++) { + await page.keyboard.press('Tab'); + } + await expect(apply).toBeFocused(); + expect(await apply.evaluate((el) => el.matches(':focus-visible'))).toBe(true); + expect((await styles(apply)).outline).not.toBe('none'); + + const box = await apply.boundingBox(); + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await page.mouse.down(); + expect((await styles(apply)).transform).not.toBe('none'); + await page.mouse.up(); + }); + } +}); diff --git a/tests/unit/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index 30d66a99..47b450c6 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -1944,6 +1944,187 @@ describe('batched option execution (#447 phase 2)', () => { expect(session.state.value.filterDiagnostics).toEqual([]); }); + // An `Array(scalar T)` variable binds a SELECTION: several option rows combine + // into one array, which `param-serialize` turns into a ClickHouse literal. + describe('Array(scalar T) variables bind a selection', () => { + const MULTI_SQL = 'SELECT 1 WHERE u IN {user:Array(String)}'; + /** A session whose one panel declares `user : Array(String)`. */ + const multiSession = (responder: Responder, initialFilters?: Record) => { + const { exec, calls } = makeExec(responder); + const session = createDashboardViewerSession(makeDeps({ + document: doc({ tiles: [tile('t1', 'q1')], variableConfigs: { user: { sql: 'SELECT a, b FROM users' } } }), + exec, + queries: [query('q1', MULTI_SQL)], + ...(initialFilters ? { initialFilters } : {}), + })); + return { session, calls, optionCalls: () => calls.filter((c) => isOptionCall(c.sql)) }; + }; + const usersRespond = (...triples: [string, string, string][]): Responder => + (sql) => (isOptionCall(sql) ? optionRows(...triples) : { columns: [{ name: 'n' }], rows: [[1]] }); + + it('runs its option SQL and offers the list — it is no longer excluded as a container', async () => { + const { session, optionCalls } = multiSession(usersRespond(['user', 'ada', 'Ada'], ['user', 'bo', 'Bo'])); + await session.start(); + expect(optionCalls()).toHaveLength(1); + const f = session.state.value.filters[0]; + expect(f.configured).toBe(true); + expect(f.status).toBe('ready'); + expect(f.optionsError).toBeNull(); + expect(f.options).toEqual([{ value: 'ada', label: 'Ada' }, { value: 'bo', label: 'Bo' }]); + }); + + it('binds a committed selection as a real ClickHouse array literal', async () => { + const { session, calls } = multiSession(usersRespond(['user', 'ada', 'Ada'], ['user', 'bo', 'Bo'])); + await session.start(); + await session.applyFilter('user', ['ada', 'bo'], true); + const tileCall = calls.filter((c) => !isOptionCall(c.sql)).at(-1)!; + // Escaped and bracketed by the shared typed serializer — never joined. + expect(tileCall.params?.param_user).toBe("['ada','bo']"); + expect(session.state.value.filters[0].value).toEqual(['ada', 'bo']); + expect(session.state.value.filters[0].active).toBe(true); + }); + + it('reduces an EMPTY selection to unset rather than binding a literal []', async () => { + // A present `[]` is a real value to `emptyValue()`, so binding it would run + // every panel as `IN []` — nothing returned, but LOOKING filtered — where + // an unset variable's panels must wait instead. + const { session } = multiSession(usersRespond(['user', 'ada', 'Ada'])); + await session.start(); + await session.applyFilter('user', [], false); + expect(session.state.value.filters[0].value).toBe(''); + expect(session.state.value.filters[0].active).toBe(false); + }); + + it('setFilter derives activation from the selection length', async () => { + const { session } = multiSession(usersRespond(['user', 'ada', 'Ada'])); + await session.start(); + await session.setFilter('user', ['ada']); + expect(session.state.value.filters[0].active).toBe(true); + await session.setFilter('user', []); + expect(session.state.value.filters[0].value).toBe(''); + expect(session.state.value.filters[0].active).toBe(false); + }); + + it('copies a committed selection, so a caller cannot mutate bound state', async () => { + const { session } = multiSession(usersRespond(['user', 'ada', 'Ada'])); + await session.start(); + const mine = ['ada']; + await session.applyFilter('user', mine, true); + mine.push('bo'); + expect(session.state.value.filters[0].value).toEqual(['ada']); + }); + + it('restores a persisted selection, and derives its activation from it', async () => { + const { session } = multiSession( + usersRespond(['user', 'ada', 'Ada'], ['user', 'bo', 'Bo']), + { user: { value: ['ada', 'bo'], active: true } }, + ); + await session.start(); + expect(session.state.value.filters[0].value).toEqual(['ada', 'bo']); + expect(session.state.value.filters[0].active).toBe(true); + }); + + it('degrades a persisted SCALAR seed on a selection variable to unset', async () => { + // The wrong shape would reach the serializer as a `structural` error and + // block every panel declaring the name. + const { session } = multiSession( + usersRespond(['user', 'ada', 'Ada']), + { user: { value: 'ada', active: true } }, + ); + await session.start(); + expect(session.state.value.filters[0].value).toBe(''); + expect(session.state.value.filters[0].active).toBe(false); + }); + + it('keeps a surviving selection, reordered to the new list, without re-running panels', async () => { + let call = 0; + const { session, calls } = multiSession((sql) => { + if (!isOptionCall(sql)) return { columns: [{ name: 'n' }], rows: [[1]] }; + call++; + // Same members, new ORDER — a reorder binds the same set. + return call === 1 + ? optionRows(['user', 'ada', 'Ada'], ['user', 'bo', 'Bo']) + : optionRows(['user', 'bo', 'Bo'], ['user', 'ada', 'Ada']); + }); + await session.start(); + await session.applyFilter('user', ['ada', 'bo'], true); + const before = calls.filter((c) => !isOptionCall(c.sql)).length; + await session.refresh(); + expect(session.state.value.filters[0].value).toEqual(['bo', 'ada']); + expect(session.state.value.filters[0].active).toBe(true); + // One refresh wave for the tile, and no EXTRA reconciliation wave. + expect(calls.filter((c) => !isOptionCall(c.sql)).length).toBe(before + 1); + }); + + it('drops a selected value the refresh removed, and re-runs the affected panels ONCE', async () => { + let call = 0; + const { session, calls } = multiSession((sql) => { + if (!isOptionCall(sql)) return { columns: [{ name: 'n' }], rows: [[1]] }; + call++; + return call === 1 + ? optionRows(['user', 'ada', 'Ada'], ['user', 'bo', 'Bo']) + : optionRows(['user', 'ada', 'Ada']); + }); + await session.start(); + await session.applyFilter('user', ['ada', 'bo'], true); + const before = calls.filter((c) => !isOptionCall(c.sql)).length; + await session.refresh(); + expect(session.state.value.filters[0].value).toEqual(['ada']); + expect(session.state.value.filters[0].active).toBe(true); + // The refresh's own wave PLUS exactly one reconciled wave. + expect(calls.filter((c) => !isOptionCall(c.sql)).length).toBe(before + 2); + }); + + it('deactivates when EVERY selected value disappeared', async () => { + let call = 0; + const { session } = multiSession((sql) => { + if (!isOptionCall(sql)) return { columns: [{ name: 'n' }], rows: [[1]] }; + call++; + return call === 1 ? optionRows(['user', 'ada', 'Ada']) : optionRows(['user', 'zed', 'Zed']); + }); + await session.start(); + await session.applyFilter('user', ['ada'], true); + await session.refresh(); + expect(session.state.value.filters[0].value).toBe(''); + expect(session.state.value.filters[0].active).toBe(false); + }); + + it('never reconciles a SCALAR variable off its committed value', async () => { + // A scalar's off-list value is shown verbatim and stays bound — an option + // refresh does not get to silently drop what the panels are already using. + let call = 0; + const { session } = optionSession( + { country: { sql: 'SELECT a, b FROM countries' } }, + (sql) => { + if (!isOptionCall(sql)) return { columns: [{ name: 'n' }], rows: [[1]] }; + call++; + return call === 1 ? optionRows(['country', 'de', 'Germany']) : optionRows(['country', 'fr', 'France']); + }, + ); + await session.start(); + await session.applyFilter('country', 'de', true); + await session.refresh(); + expect(session.state.value.filters[0].value).toBe('de'); + expect(session.state.value.filters[0].active).toBe(true); + }); + }); + + it('explains a configured variable whose TYPE has no option list', async () => { + // A `Map`/`Tuple`/`Nested`/nested-`Array` variable someone configured anyway: + // its SQL is fine, but nothing can render a list for it, so it is kept out of + // the batch and its control says the configuration is deliberately not running. + const { session, optionCalls } = optionSession( + { tags: { sql: 'SELECT a, b FROM t' } }, + (sql) => (isOptionCall(sql) ? optionRows() : { columns: [{ name: 'n' }], rows: [[1]] }), + 'SELECT 1 WHERE m = {tags:Map(String, String)}', + ); + await session.start(); + expect(optionCalls()).toHaveLength(0); + const f = session.state.value.filters.find((x) => x.parameter === 'tags')!; + expect(f.status).toBe('error'); + expect(f.optionsError).toContain('no option list'); + }); + it('does not re-run the batch for a single-tile refresh', async () => { const { session, optionCalls } = optionSession( { country: { sql: 'SELECT a, b FROM countries' } }, diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index 3353419e..0950a1fe 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -3516,6 +3516,47 @@ describe('renderDashboard — filter-source runtime rebuild + diagnostics (#359) const note = qs(app.root, '.var-unsupported'); expect(note.textContent).toContain('Array(String)'); }); + + it('renders an Array(String) variable WITH option SQL as the multi-select, and binds its selection', async () => { + const { app, calls } = dashApp({ + responder: (sql) => (sql.includes('__variable_name') + ? { + columns: [ + { name: '__variable_name', type: 'String' }, + { name: 'v', type: 'String' }, + { name: 'l', type: 'String' }, + ], + rows: [['user', 'ada', 'Ada'], ['user', 'bo', 'Bo']], + } + : { columns: [{ name: 'n', type: 'UInt8' }], rows: [[1]] }), + workspace: wsWith({ + queries: [q('q1', 'SELECT 1 WHERE u IN {user:Array(String)}')], + tiles: [{ id: 't1', queryId: 'q1' }], + variableConfigs: { user: { sql: 'SELECT a, b FROM users' } }, + }), + }); + await render(app); + const panelRuns = () => calls.filter((c) => !c.sql.includes('__variable_name')); + // Unset, so the panel waits — and the control is the multiselect, not a text + // box with the no-inferred-control marker. + expect(panelRuns()).toHaveLength(0); + expect(app.root!.querySelector('.var-unsupported')).toBeNull(); + const trigger = qs(app.root, '.ms-trigger'); + expect(trigger.textContent).toBe('Not set'); + + trigger.click(); + const boxes = [...document.querySelectorAll('.ms-option input[type="checkbox"]')]; + expect(boxes).toHaveLength(2); + for (const cb of boxes) { cb.checked = true; cb.dispatchEvent(new Event('change')); } + (document.querySelector('.ms-btn-primary') as HTMLButtonElement).click(); + await flush(); + + // The Apply reached the session and ran the panel bound to a real ClickHouse + // array literal — never the joined string `ada,bo`. + expect(panelRuns()).toHaveLength(1); + expect(panelRuns()[0].params.param_user).toBe("['ada','bo']"); + expect(qs(app.root, '.ms-trigger').textContent).toBe('2 selected'); + }); }); // #303: the isolated per-dashboard filter store (`asb:dashFilters`) — the diff --git a/tests/unit/filter-bar.test.ts b/tests/unit/filter-bar.test.ts index 583fbcb0..0b858e6d 100644 --- a/tests/unit/filter-bar.test.ts +++ b/tests/unit/filter-bar.test.ts @@ -617,26 +617,30 @@ describe('buildFilterBar — Dashboard variable controls (#447 phase 2)', () => // container-typed variable already had a free-text field, and param-serialize // binds an array literal typed into it — taking it away leaves those panels // permanently unfilled with no way to fill them. - const { bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { + const { bar } = build('SELECT * FROM t WHERE x IN {tags:Map(String, String)}', { variables: { tags: { options: null } }, }); const field = fieldFor(bar, 'tags'); expect(field.querySelector('.var-input')).not.toBeNull(); const note = field.querySelector('.var-unsupported')!; - expect(note.textContent).toContain('Array(String)'); + expect(note.textContent).toContain('Map(String, String)'); expect(note.getAttribute('aria-label')).toContain('no inferred control'); expect(note.getAttribute('title')).toContain('container type'); }); it('the unsupported verdict wins over a configured option list', () => { - // If the value cannot bind, the fact that someone configured options for it - // does not make it usable. - const { bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { - variables: { tags: { options: OPTIONS } }, - }); - expect(fieldFor(bar, 'tags').querySelector('.filter-select')).toBeNull(); - expect(fieldFor(bar, 'tags').querySelector('.var-unsupported')).not.toBeNull(); - expect(fieldFor(bar, 'tags').querySelector('.var-input')).not.toBeNull(); + // A container with no flat element list cannot be supplied by a value/label + // list, so configuring options for one does not make it usable. Unchanged + // for every container EXCEPT `Array(scalar T)` — see the multi-select tests. + for (const type of ['Map(String, String)', 'Tuple(String, String)', 'Array(Array(String))']) { + const { bar } = build(`SELECT * FROM t WHERE x IN {tags:${type}}`, { + variables: { tags: { options: OPTIONS } }, + }); + expect(fieldFor(bar, 'tags').querySelector('.filter-select')).toBeNull(); + expect(fieldFor(bar, 'tags').querySelector('.ms-trigger')).toBeNull(); + expect(fieldFor(bar, 'tags').querySelector('.var-unsupported')).not.toBeNull(); + expect(fieldFor(bar, 'tags').querySelector('.var-input')).not.toBeNull(); + } }); it('reports unsupported for a container even with no entry in the variables map', () => { @@ -646,9 +650,124 @@ describe('buildFilterBar — Dashboard variable controls (#447 phase 2)', () => }); it('never reports unsupported without the variables map — the workbench keeps its text field', () => { - const { bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}'); + for (const type of ['Array(String)', 'Map(String, String)']) { + const { bar } = build(`SELECT * FROM t WHERE x IN {tags:${type}}`); + expect(fieldFor(bar, 'tags').querySelector('.var-unsupported')).toBeNull(); + expect(fieldFor(bar, 'tags').querySelector('.ms-trigger')).toBeNull(); + expect(fieldFor(bar, 'tags').querySelector('.var-input')).not.toBeNull(); + } + }); + + it('marks an Array(scalar) with NO option SQL as having no option list, and keeps its input', () => { + // Its type IS controllable — configuring option SQL turns it into the + // multi-select — so the marker names that fix instead of calling the type + // uncontrollable. The free-text input stays either way: a hand-typed + // `['a','b']` still binds. + const { bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { + variables: { tags: { options: null } }, + }); + const field = fieldFor(bar, 'tags'); + expect(field.querySelector('.var-input')).not.toBeNull(); + expect(field.querySelector('.ms-trigger')).toBeNull(); + const note = field.querySelector('.var-unsupported')!; + expect(note.textContent).toContain('Array(String)'); + expect(note.getAttribute('aria-label')).toContain('no option list'); + expect(note.getAttribute('title')).toContain('Add option SQL'); + // Never the misleading container wording. + expect(note.getAttribute('title')).not.toContain('container type'); + }); + + it('renders an Array(scalar) WITH options as the multi-select, not a text field', () => { + for (const type of ['Array(String)', 'Array(UInt64)', 'Array(LowCardinality(String))']) { + const { bar } = build(`SELECT * FROM t WHERE x IN {tags:${type}}`, { + variables: { tags: { options: OPTIONS } }, + }); + const field = fieldFor(bar, 'tags'); + const trigger = field.querySelector('.ms-trigger')!; + expect(trigger).not.toBeNull(); + expect(trigger.getAttribute('aria-haspopup')).toBe('dialog'); + expect(trigger.textContent).toBe('Not set'); + // It REPLACES the plain input and the single-select, rather than adorning. + expect(field.querySelector('.var-unsupported')).toBeNull(); + expect(field.querySelector('.filter-select')).toBeNull(); + expect(field.querySelector('input.var-input')).toBeNull(); + } + }); + + it('renders the multi-select even when the option list came back empty', () => { + // `[]` means option-backed with no rows — meaningfully different from the + // `null` that means "no option SQL configured". + const { bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { + variables: { tags: { options: [] } }, + }); + expect(fieldFor(bar, 'tags').querySelector('.ms-trigger')).not.toBeNull(); expect(fieldFor(bar, 'tags').querySelector('.var-unsupported')).toBeNull(); - expect(fieldFor(bar, 'tags').querySelector('.var-input')).not.toBeNull(); + }); + + it('takes the committed selection from the spec, and leaves varValues untouched', () => { + // An array must never enter `state.varValues` — that bag is the shared + // `Record` the Workbench var-strip also owns and persists, + // so a selection travels on the spec instead. The bar reads `filterActive` + // for activation, which is a plain boolean and stays in the shared bag. + const { app, bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { + variables: { tags: { options: OPTIONS, selection: ['de', 'fr'] } }, + }); + expect(app.state.varValues.tags).toBeUndefined(); + // Inactive at build time, so the trigger reads unset however long the + // selection is — activation is authoritative, never a value sentinel. + expect(fieldFor(bar, 'tags').querySelector('.ms-trigger')!.textContent).toBe('Not set'); + // The draft it opens with IS that selection, though. + fieldFor(bar, 'tags').querySelector('.ms-trigger')!.click(); + const checked = [...document.querySelectorAll('.ms-option input[type="checkbox"]')] + .map((cb) => cb.checked); + expect(checked).toEqual([true, true]); + }); + + it('commits a multi-select Apply through onCommitVariableSelection, not the scalar bag', () => { + const onCommitVariableSelection = vi.fn(); + const { app, bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { + variables: { tags: { options: OPTIONS } }, + onCommitVariableSelection, + }); + const trigger = fieldFor(bar, 'tags').querySelector('.ms-trigger')!; + trigger.click(); + const dialog = document.querySelector('.ms-popover')!; + const boxes = dialog.querySelectorAll('.ms-option input[type="checkbox"]'); + boxes[0].checked = true; + boxes[0].dispatchEvent(new Event('change')); + (dialog.querySelector('.ms-btn-primary') as HTMLButtonElement).click(); + expect(onCommitVariableSelection).toHaveBeenCalledWith('tags', ['de'], true); + expect(app.state.filterActive.tags).toBe(true); + // The array never touches the shared scalar bag. + expect(app.state.varValues.tags).toBeUndefined(); + }); + + it('routes setVariableOptions and dispose through the multi-select handle', () => { + const { bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { + variables: { tags: { options: OPTIONS } }, + }); + const trigger = fieldFor(bar, 'tags').querySelector('.ms-trigger')!; + trigger.click(); + expect(bar.openPopoverKey()).toBe('tags'); + // A fresh generation cancels the draft it could not be applied against. + bar.setVariableOptions({ tags: { options: [{ value: 'es', label: 'Spain' }], error: null } }); + expect(bar.openPopoverKey()).toBeNull(); + trigger.click(); + expect([...document.querySelectorAll('.ms-option-label')].map((n) => n.textContent)).toEqual(['Spain']); + }); + + it('renders a multi-select unavailable when the batch failed', () => { + const { bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { + variables: { tags: { options: OPTIONS, optionsError: 'Variable options could not be loaded.' } }, + }); + const trigger = fieldFor(bar, 'tags').querySelector('.ms-trigger')!; + expect(trigger.getAttribute('aria-disabled')).toBe('true'); + expect(trigger.getAttribute('aria-invalid')).toBe('true'); + expect(trigger.title).toContain('could not be loaded'); + // Never actually `disabled` — the reason must stay reachable. + expect(trigger.disabled).toBe(false); + trigger.click(); + expect(document.querySelector('.ms-popover')).toBeNull(); }); it('offers true/false suggestions for a Bool variable, and only under the map', () => { diff --git a/tests/unit/multi-select-field.test.ts b/tests/unit/multi-select-field.test.ts new file mode 100644 index 00000000..33be7e21 --- /dev/null +++ b/tests/unit/multi-select-field.test.ts @@ -0,0 +1,400 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { buildMultiSelectField } from '../../src/ui/multi-select-field.js'; +import type { MultiSelectFieldOpts, MultiSelectFieldHandle } from '../../src/ui/multi-select-field.js'; + +// The searchable multiselect for an `Array(scalar T)` Dashboard variable (#189, +// restored onto the inferred-Variables model). Same local-factory + spy + real +// DOM convention as `filter-option-field.test.ts` — no fake app, no snapshots: +// every assertion is on a class, an ARIA attribute, or textContent, because +// those ARE the contract a screen reader and the CSS both read. + +const OPTIONS = [ + { value: 'de', label: 'Germany' }, + { value: 'fr', label: 'France' }, + { value: 'es', label: 'Spain' }, +]; + +const fields: MultiSelectFieldHandle[] = []; +afterEach(() => { for (const f of fields.splice(0)) f.dispose(); }); + +function build(over: Partial = {}) { + const onApply = vi.fn(); + const field = buildMultiSelectField({ + document, name: 'country', options: OPTIONS, selected: [], active: false, onApply, ...over, + }); + fields.push(field); + document.body.replaceChildren(field.el); + return { field, onApply, trigger: field.el.querySelector('.ms-trigger')! }; +} + +const dialog = (): HTMLElement => document.querySelector('.ms-popover')!; +const search = (): HTMLInputElement => dialog().querySelector('.ms-search')!; +const selectAll = (): HTMLInputElement => dialog().querySelector('.ms-select-all-cb')!; +const rows = (): HTMLElement[] => [...dialog().querySelectorAll('.ms-option')]; +const boxes = (): HTMLInputElement[] => + [...dialog().querySelectorAll('.ms-option input[type="checkbox"]')]; +const visibleLabels = (): string[] => rows().filter((r) => !r.hidden) + .map((r) => r.querySelector('.ms-option-label')!.textContent ?? ''); +const live = (): string => dialog().querySelector('.ms-live')!.textContent ?? ''; +const btn = (cls: string): HTMLButtonElement => dialog().querySelector(cls)!; +const check = (cb: HTMLInputElement, next: boolean): void => { + cb.checked = next; + cb.dispatchEvent(new Event('change')); +}; +const type = (text: string): void => { + search().value = text; + search().dispatchEvent(new Event('input')); +}; + +describe('trigger text and accessible name', () => { + it('reads Not set when nothing is committed', () => { + expect(build().trigger.textContent).toBe('Not set'); + }); + + it('reads Not set when a selection exists but the variable is inactive', () => { + // Activation is authoritative — a dormant value is never presented as bound. + expect(build({ selected: ['de', 'fr'], active: false }).trigger.textContent).toBe('Not set'); + }); + + it('reads the single option LABEL when exactly one is selected', () => { + expect(build({ selected: ['de'], active: true }).trigger.textContent).toBe('Germany'); + }); + + it('falls back to the raw value when the single selection has no matching option', () => { + // A dormant value a refresh dropped must not read as blank. + expect(build({ selected: ['gone'], active: true }).trigger.textContent).toBe('gone'); + }); + + it('counts beyond one', () => { + expect(build({ selected: ['de', 'fr'], active: true }).trigger.textContent).toBe('2 selected'); + }); + + it('names itself, and its selected count, for assistive tech', () => { + const { trigger } = build({ selected: ['de', 'fr'], active: true }); + expect(trigger.getAttribute('aria-label')).toBe('country filter, 2 selected'); + expect(trigger.getAttribute('aria-haspopup')).toBe('dialog'); + expect(trigger.getAttribute('aria-expanded')).toBe('false'); + expect(trigger.id).toBe('ms-trigger-country'); + }); + + it('carries the caller title, or the bare name without one', () => { + expect(build({ title: 'country: Array(String)' }).trigger.title).toBe('country: Array(String)'); + expect(build().trigger.title).toBe('country'); + }); + + it('defaults to the ambient document when none is injected', () => { + const field = buildMultiSelectField({ name: 'c', options: [], selected: [], active: false, onApply: () => {} }); + fields.push(field); + expect(field.el.querySelector('.ms-trigger')).not.toBeNull(); + }); +}); + +describe('opening', () => { + it('mounts a named modal dialog and focuses the search box', () => { + const { trigger } = build(); + trigger.click(); + expect(dialog().getAttribute('role')).toBe('dialog'); + expect(dialog().getAttribute('aria-modal')).toBe('true'); + expect(dialog().getAttribute('aria-label')).toBe('country options'); + expect(trigger.getAttribute('aria-expanded')).toBe('true'); + expect(document.activeElement).toBe(search()); + expect(search().getAttribute('aria-label')).toBe('Search country options'); + expect(search().placeholder).toBe('Search country options'); + }); + + it('seeds the draft from the committed selection', () => { + build({ selected: ['de', 'es'], active: true }).trigger.click(); + expect(boxes().map((cb) => cb.checked)).toEqual([true, false, true]); + }); + + it('never stacks a second popover', () => { + const { trigger } = build(); + trigger.click(); + trigger.click(); + expect(document.querySelectorAll('.ms-popover')).toHaveLength(1); + }); + + it('reports whether it is open', () => { + const { field, trigger } = build(); + expect(field.isOpen()).toBe(false); + trigger.click(); + expect(field.isOpen()).toBe(true); + btn('.ms-btn:not(.ms-btn-clear):not(.ms-btn-primary)').click(); + expect(field.isOpen()).toBe(false); + }); +}); + +describe('search', () => { + it('filters on label, case-insensitively, and reports the count', () => { + build().trigger.click(); + type('ger'); + expect(visibleLabels()).toEqual(['Germany']); + expect(live()).toBe('1 of 3 options'); + }); + + it('filters on VALUE too, not just the visible label', () => { + build().trigger.click(); + type('es'); + // 'es' matches Spain by value, and France by its label ('France' has no + // 'es'… but 'Spain' does not either) — value matching is what finds it. + expect(visibleLabels()).toEqual(['Spain']); + }); + + it('shows everything for blank or whitespace-only text', () => { + build().trigger.click(); + type('ger'); + type(' '); + expect(visibleLabels()).toHaveLength(3); + expect(live()).toBe('3 of 3 options'); + }); + + it('can match nothing', () => { + build().trigger.click(); + type('zzz'); + expect(visibleLabels()).toEqual([]); + expect(live()).toBe('0 of 3 options'); + }); +}); + +describe('select visible', () => { + it('starts unchecked, goes indeterminate, then checked', () => { + build().trigger.click(); + expect(selectAll().checked).toBe(false); + expect(selectAll().indeterminate).toBe(false); + expect(selectAll().getAttribute('aria-label')).toBe('Select all 3 visible options'); + + check(boxes()[0], true); + expect(selectAll().checked).toBe(false); + expect(selectAll().indeterminate).toBe(true); + + check(boxes()[1], true); + check(boxes()[2], true); + expect(selectAll().checked).toBe(true); + expect(selectAll().indeterminate).toBe(false); + expect(selectAll().getAttribute('aria-label')).toBe('Clear all 3 visible options'); + }); + + it('unchecking a box leaves the draft', () => { + build({ selected: ['de'], active: true }).trigger.click(); + check(boxes()[0], false); + expect(selectAll().indeterminate).toBe(false); + expect(selectAll().checked).toBe(false); + }); + + it('affects ONLY the filtered subset, leaving hidden selections alone', () => { + const { onApply, trigger } = build(); + trigger.click(); + check(boxes()[0], true); // Germany, which the filter below hides + type('ran'); // France only + expect(visibleLabels()).toEqual(['France']); + check(selectAll(), true); + btn('.ms-btn-primary').click(); + // Germany survived even though it was hidden when Select visible ran. + expect(onApply).toHaveBeenCalledWith(['de', 'fr'], true); + }); + + it('clears only the filtered subset', () => { + const { onApply, trigger } = build({ selected: ['de', 'fr'], active: true }); + trigger.click(); + type('ran'); + check(selectAll(), false); + btn('.ms-btn-primary').click(); + expect(onApply).toHaveBeenCalledWith(['de'], true); + }); + + it('reports zero visible options as neither checked nor mixed', () => { + build().trigger.click(); + type('zzz'); + expect(selectAll().checked).toBe(false); + expect(selectAll().indeterminate).toBe(false); + expect(selectAll().getAttribute('aria-label')).toBe('Select all 0 visible options'); + }); +}); + +describe('Clear / Cancel / Apply', () => { + it('Clear empties the WHOLE draft, not just the visible subset', () => { + const { trigger } = build({ selected: ['de', 'fr', 'es'], active: true }); + trigger.click(); + type('ran'); // only France visible + btn('.ms-btn-clear').click(); + type(''); + expect(boxes().map((cb) => cb.checked)).toEqual([false, false, false]); + }); + + it('Clear then Apply returns the variable to unset', () => { + const { onApply, trigger } = build({ selected: ['de'], active: true }); + trigger.click(); + btn('.ms-btn-clear').click(); + btn('.ms-btn-primary').click(); + expect(onApply).toHaveBeenCalledWith([], false); + }); + + it('Apply canonicalizes by OPTION order, not click order', () => { + const { onApply, trigger } = build(); + trigger.click(); + check(boxes()[2], true); // Spain first + check(boxes()[0], true); // then Germany + btn('.ms-btn-primary').click(); + expect(onApply).toHaveBeenCalledWith(['de', 'es'], true); + }); + + it('Apply closes BEFORE it commits', () => { + // The shared popover contract: a subscriber rebuilding the bar inside the + // commit's synchronous publish must observe this popover as already closed. + const { field, trigger } = build(); + let openAtCommit: boolean | null = null; + fields.pop(); + const f = buildMultiSelectField({ + document, name: 'country', options: OPTIONS, selected: [], active: false, + onApply: () => { openAtCommit = f.isOpen(); }, + }); + fields.push(f); + document.body.replaceChildren(f.el); + f.el.querySelector('.ms-trigger')!.click(); + check(boxes()[0], true); + btn('.ms-btn-primary').click(); + expect(openAtCommit).toBe(false); + expect(field.isOpen()).toBe(false); + expect(trigger).toBeDefined(); + }); + + it('a no-op Apply closes silently', () => { + const { field, onApply, trigger } = build({ selected: ['de'], active: true }); + trigger.click(); + btn('.ms-btn-primary').click(); + expect(onApply).not.toHaveBeenCalled(); + expect(field.isOpen()).toBe(false); + }); + + it('an Apply that only changes activation still commits', () => { + // Same canonical values, different active flag — the value did change. + const { onApply, trigger } = build({ selected: ['de'], active: false }); + trigger.click(); + btn('.ms-btn-primary').click(); + expect(onApply).toHaveBeenCalledWith(['de'], true); + }); + + it('Cancel discards the draft and writes nothing', () => { + const { field, onApply, trigger } = build(); + trigger.click(); + check(boxes()[0], true); + btn('.ms-btn:not(.ms-btn-clear):not(.ms-btn-primary)').click(); + expect(onApply).not.toHaveBeenCalled(); + expect(field.isOpen()).toBe(false); + // Re-opening starts from committed truth, not the discarded draft. + trigger.click(); + expect(boxes().map((cb) => cb.checked)).toEqual([false, false, false]); + }); + + it('Escape discards the draft', () => { + const { field, onApply, trigger } = build(); + trigger.click(); + check(boxes()[0], true); + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); + expect(field.isOpen()).toBe(false); + expect(onApply).not.toHaveBeenCalled(); + }); + + it('drops a selected value the option list does not offer', () => { + // Apply canonicalizes against the authoritative list, so a dormant value + // cannot be silently re-committed. + const { onApply, trigger } = build({ selected: ['de', 'gone'], active: true }); + trigger.click(); + check(boxes()[1], true); + btn('.ms-btn-primary').click(); + expect(onApply).toHaveBeenCalledWith(['de', 'fr'], true); + }); +}); + +describe('setOptions', () => { + it('swaps the list and re-renders the committed label', () => { + const { field, trigger } = build({ selected: ['de'], active: true }); + expect(trigger.textContent).toBe('Germany'); + field.setOptions([{ value: 'de', label: 'Deutschland' }]); + expect(trigger.textContent).toBe('Deutschland'); + trigger.click(); + expect(visibleLabels()).toEqual(['Deutschland']); + }); + + it('cancels an open popover — its draft was built against the old generation', () => { + const { field, onApply, trigger } = build(); + trigger.click(); + check(boxes()[0], true); + field.setOptions([{ value: 'es', label: 'Spain' }]); + expect(field.isOpen()).toBe(false); + expect(onApply).not.toHaveBeenCalled(); + }); + + it('never re-commits — an options refresh is not a user choice', () => { + const { field, onApply } = build({ selected: ['de'], active: true }); + field.setOptions([]); + expect(onApply).not.toHaveBeenCalled(); + }); +}); + +describe('setUnavailable', () => { + it('marks the trigger without disabling it, and refuses to open', () => { + const { field, trigger } = build(); + field.setUnavailable('Variable options could not be loaded.'); + expect(trigger.getAttribute('aria-disabled')).toBe('true'); + expect(trigger.getAttribute('aria-invalid')).toBe('true'); + expect(trigger.classList.contains('is-error')).toBe(true); + expect(field.el.classList.contains('is-error')).toBe(true); + expect(trigger.title).toBe('Variable options could not be loaded.'); + // Never `disabled` — that would make the reason unreachable and drop focus. + expect(trigger.disabled).toBe(false); + trigger.click(); + expect(document.querySelector('.ms-popover')).toBeNull(); + }); + + it('closes an already-open popover', () => { + const { field, trigger } = build(); + trigger.click(); + field.setUnavailable('boom'); + expect(field.isOpen()).toBe(false); + }); + + it('restores on null, back to the resting title', () => { + const { field, trigger } = build({ title: 'country: Array(String)' }); + field.setUnavailable('boom'); + field.setUnavailable(null); + expect(trigger.getAttribute('aria-disabled')).toBe('false'); + expect(trigger.hasAttribute('aria-invalid')).toBe(false); + expect(trigger.classList.contains('is-error')).toBe(false); + expect(trigger.title).toBe('country: Array(String)'); + trigger.click(); + expect(document.querySelector('.ms-popover')).not.toBeNull(); + }); + + it('leaves the committed selection alone', () => { + // It is still bound into every panel that declares the name; a failed list + // is no reason to silently change what those panels show. + const { field, trigger } = build({ selected: ['de'], active: true }); + field.setUnavailable('boom'); + expect(trigger.textContent).toBe('Germany'); + }); +}); + +describe('focusTrigger and dispose', () => { + it('focuses the trigger', () => { + const { field, trigger } = build(); + field.focusTrigger(); + expect(document.activeElement).toBe(trigger); + }); + + it('dispose while open is a silent Cancel', () => { + const { field, onApply, trigger } = build(); + trigger.click(); + check(boxes()[0], true); + field.dispose(); + expect(document.querySelector('.ms-popover')).toBeNull(); + expect(onApply).not.toHaveBeenCalled(); + }); + + it('dispose unwires the trigger', () => { + const { field, trigger } = build(); + field.dispose(); + trigger.click(); + expect(document.querySelector('.ms-popover')).toBeNull(); + }); +}); diff --git a/tests/unit/param-pipeline.test.ts b/tests/unit/param-pipeline.test.ts index a779eb87..fa952291 100644 --- a/tests/unit/param-pipeline.test.ts +++ b/tests/unit/param-pipeline.test.ts @@ -730,13 +730,30 @@ describe('fieldControlKind (shared control priority — review F1/F8)', () => { .toEqual(['true', 'false']); }); - it('reports a compound type as unsupported', () => { - for (const type of ['Array(String)', 'Tuple(String, String)', 'Map(String, String)', 'Nested(a String)']) { + it('reports an Array of a scalar as multi', () => { + // The narrower container rule, tried first: several option rows combine + // into one bound array. This is a statement about the TYPE — whether the + // variable actually HAS an option list is the filter bar's pairing. + for (const type of ['Array(String)', 'Array(UInt64)', 'Array(LowCardinality(String))']) { + expect(fieldControlKind({ type }, null, scalar)).toEqual({ kind: 'multi', enumOptions: null }); + } + expect(fieldControlKind({ type: 'Nullable(Array(String))' }, null, scalar).kind).toBe('multi'); + }); + + it('reports every other compound type as unsupported', () => { + for (const type of ['Tuple(String, String)', 'Map(String, String)', 'Nested(a String)']) { expect(fieldControlKind({ type }, null, scalar)).toEqual({ kind: 'unsupported', enumOptions: null }); } - // Wrapped, and nested one level deeper. - expect(fieldControlKind({ type: 'Nullable(Array(String))' }, null, scalar).kind).toBe('unsupported'); + // A nested array has no serializable form, so it stays unsupported rather + // than becoming a multi-select that could never bind. expect(fieldControlKind({ type: 'Array(Array(UInt8))' }, null, scalar).kind).toBe('unsupported'); + expect(fieldControlKind({ type: 'Array(Tuple(String, UInt8))' }, null, scalar).kind).toBe('unsupported'); + }); + + it('never reports multi or unsupported without the policy', () => { + // The workbench var-strip passes no options bag and keeps its text field. + expect(fieldControlKind({ type: 'Array(String)' })).toEqual({ kind: 'text', enumOptions: null }); + expect(fieldControlKind({ type: 'Map(String, String)' })).toEqual({ kind: 'text', enumOptions: null }); }); it('leaves the enum/date priority ahead of the policy', () => { @@ -747,11 +764,13 @@ describe('fieldControlKind (shared control priority — review F1/F8)', () => { expect(fieldControlKind({ type: 'String' }, ['x'], scalar)).toEqual({ kind: 'enum', enumOptions: ['x'] }); }); - it('keeps a conflicted compound field on text, never unsupported', () => { + it('keeps a conflicted compound field on text, never multi or unsupported', () => { // A conflict means no single authoritative declaration to judge, so the // #173 degrade-to-text rule still wins outright. expect(fieldControlKind({ type: 'Array(String)', conflict: ['Array(String)', 'String'] }, null, scalar)) .toEqual({ kind: 'text', enumOptions: null }); + expect(fieldControlKind({ type: 'Map(String, String)', conflict: ['Map(String, String)', 'String'] }, null, scalar)) + .toEqual({ kind: 'text', enumOptions: null }); }); it('leaves ordinary scalars on their usual controls', () => { diff --git a/tests/unit/param-type.test.ts b/tests/unit/param-type.test.ts index e1df5cbc..0b9b484d 100644 --- a/tests/unit/param-type.test.ts +++ b/tests/unit/param-type.test.ts @@ -8,6 +8,7 @@ import { isSupportedTimeRangeParamType, dateTimeTimeZone, isCompoundParamType, + multiSelectElementType, } from '../../src/core/param-type.js'; // #447 phase 2: named so a single-scalar surface can say "no control for this" @@ -52,6 +53,62 @@ describe('isCompoundParamType', () => { }); }); +// The single eligibility decision behind the restored Array(T) multi-select: +// the option batch and the control dispatch both filter on THIS, so a type that +// gets a select can never be one whose option SQL was skipped. +describe('multiSelectElementType', () => { + it('yields the element type for an Array of a scalar', () => { + expect(multiSelectElementType('Array(String)')?.base).toBe('String'); + expect(multiSelectElementType('Array(UInt64)')?.base).toBe('UInt64'); + expect(multiSelectElementType('Array(Int32)')?.base).toBe('Int32'); + expect(multiSelectElementType("Array(Enum8('a' = 1))")?.base).toBe('Enum8'); + }); + + it('sees through the value-transparent wrappers, on the array AND its element', () => { + // `parseParamType` unwraps Nullable/LowCardinality recursively, so the + // element handed back is already the EFFECTIVE scalar the serializer lexes. + expect(multiSelectElementType('Nullable(Array(String))')?.base).toBe('String'); + expect(multiSelectElementType('Array(LowCardinality(String))')?.base).toBe('String'); + expect(multiSelectElementType('Array(Nullable(UInt64))')?.base).toBe('UInt64'); + }); + + it('is null for a scalar — there is nothing to multi-select', () => { + for (const type of ['String', 'UInt64', 'Date', 'DateTime64(3)', 'Bool', "Enum8('a' = 1)"]) { + expect(multiSelectElementType(type)).toBeNull(); + } + expect(multiSelectElementType('')).toBeNull(); + }); + + it('is null for a container with no flat element list', () => { + for (const type of ['Tuple(String, UInt8)', 'Map(String, UInt64)', 'Nested(a String)']) { + expect(multiSelectElementType(type)).toBeNull(); + } + }); + + it('is null for a nested array, which the serializer rejects outright', () => { + // `param-serialize.js` refuses both nested array VALUES and nested `Array` + // DECLARATIONS, so a control that produced one could never bind. + expect(multiSelectElementType('Array(Array(String))')).toBeNull(); + expect(multiSelectElementType('Array(Nullable(Array(String)))')).toBeNull(); + }); + + it('is null for an Array of a non-array container', () => { + expect(multiSelectElementType('Array(Tuple(String, UInt8))')).toBeNull(); + expect(multiSelectElementType('Array(Map(String, UInt64))')).toBeNull(); + }); + + it('is null for an Array the shared parser cannot read', () => { + // Degrades to an opaque scalar whose `base` is the whole text, so `isArray` + // is false and there is no `elem` to offer. + expect(multiSelectElementType('Array(')).toBeNull(); + }); + + it('accepts an already-parsed type', () => { + expect(multiSelectElementType(parseParamType('Array(UInt8)'))?.base).toBe('UInt8'); + expect(multiSelectElementType(parseParamType('String'))).toBeNull(); + }); +}); + describe('isSupportedTimeRangeParamType', () => { it('accepts supported scalar date/time declarations and valid wrappers', () => { for (const type of [ diff --git a/tests/unit/variable-options.test.ts b/tests/unit/variable-options.test.ts index 45ff282e..7ee5cbcd 100644 --- a/tests/unit/variable-options.test.ts +++ b/tests/unit/variable-options.test.ts @@ -168,11 +168,25 @@ describe('optionBatchVariables', () => { expect(optionBatchVariables([variable({ name: 'bad', sql: 'SELECT a, b FROM t; SELECT 1, 2' })])).toEqual([]); }); - it('excludes a CONTAINER-typed variable, which can never render an option select', () => { - // A two-String-column list cannot supply an Array/Tuple/Map/Nested value, so - // running its option SQL is work for a control that never appears — and a - // broken one could fail the combined query and take other variables down. - for (const type of ['Array(String)', 'Tuple(String, String)', 'Map(String, String)']) { + it('INCLUDES an Array-of-scalar variable — it backs the multi-select', () => { + // The option rows are the same two String columns; the array-ness is about + // how several of them combine into one bound value, not about the row shape. + for (const type of ['Array(String)', 'Array(UInt64)', 'Array(LowCardinality(String))', 'Nullable(Array(Int32))']) { + expect(optionBatchVariables([ + variable({ name: 'tags', type, types: [type], sql: 'SELECT a, b FROM t' }), + ])).toHaveLength(1); + } + }); + + it('excludes a container with no flat element list, which can never render a select', () => { + // A two-String-column list cannot supply a Tuple/Map/Nested value, nor a + // nested array (which `param-serialize` rejects outright), so running its + // option SQL is work for a control that never appears — and a broken one + // could fail the combined query and take other variables down. + for (const type of [ + 'Tuple(String, String)', 'Map(String, String)', 'Nested(a String)', + 'Array(Array(String))', 'Array(Tuple(String, UInt8))', + ]) { expect(optionBatchVariables([ variable({ name: 'tags', type, types: [type], sql: 'SELECT a, b FROM t' }), ])).toEqual([]); diff --git a/tests/unit/variable-selection.test.ts b/tests/unit/variable-selection.test.ts new file mode 100644 index 00000000..05e094aa --- /dev/null +++ b/tests/unit/variable-selection.test.ts @@ -0,0 +1,101 @@ +import { describe, it, expect } from 'vitest'; +import { + sameSelection, canonicalizeSelection, reconcileSelection, +} from '../../src/core/variable-selection.js'; + +// Empty string is a VALID option value throughout — never a sentinel for "no +// selection" (that is an empty array, reduced to unset by the session). +const opts = (...values: string[]): { value: string }[] => values.map((value) => ({ value })); + +describe('sameSelection', () => { + it('compares element-wise, in order', () => { + expect(sameSelection([], [])).toBe(true); + expect(sameSelection(['a', 'b'], ['a', 'b'])).toBe(true); + expect(sameSelection(['a', 'b'], ['b', 'a'])).toBe(false); + expect(sameSelection(['a'], ['a', 'b'])).toBe(false); + expect(sameSelection(['a', 'b'], ['a'])).toBe(false); + expect(sameSelection(['a'], ['b'])).toBe(false); + }); + + it('treats an empty-string element as an ordinary value', () => { + expect(sameSelection([''], [''])).toBe(true); + expect(sameSelection([''], [])).toBe(false); + }); +}); + +describe('canonicalizeSelection', () => { + it('orders by OPTION order, not by the values it was given', () => { + // The option list is authoritative for display order. + expect(canonicalizeSelection(['c', 'a'], opts('a', 'b', 'c'))).toEqual(['a', 'c']); + }); + + it('dedupes', () => { + expect(canonicalizeSelection(['a', 'a', 'b'], opts('a', 'b'))).toEqual(['a', 'b']); + }); + + it('drops a value with no matching option', () => { + // A dormant value a refresh removed from the list. + expect(canonicalizeSelection(['a', 'gone'], opts('a', 'b'))).toEqual(['a']); + }); + + it('never introduces a value that was not asked for', () => { + // A filter/reorder, never an auto-select. + expect(canonicalizeSelection([], opts('a', 'b'))).toEqual([]); + expect(canonicalizeSelection(['a'], opts('a', 'b', 'c'))).toEqual(['a']); + }); + + it('keeps an empty-string option value', () => { + expect(canonicalizeSelection([''], opts('', 'a'))).toEqual(['']); + }); + + it('returns empty against an empty option list', () => { + expect(canonicalizeSelection(['a'], [])).toEqual([]); + }); +}); + +describe('reconcileSelection', () => { + it('keeps survivors, in the NEW option order, with no wave for a pure reorder', () => { + const r = reconcileSelection(['a', 'b'], opts('b', 'a')); + expect(r).toEqual({ value: ['b', 'a'], deactivate: false, waveNeeded: false }); + }); + + it('needs no wave when the list is unchanged', () => { + expect(reconcileSelection(['a'], opts('a', 'b'))) + .toEqual({ value: ['a'], deactivate: false, waveNeeded: false }); + }); + + it('needs a wave when a selected value disappeared', () => { + // The bound SET changed, so the panels that declare this variable must re-run. + expect(reconcileSelection(['a', 'gone'], opts('a', 'b'))) + .toEqual({ value: ['a'], deactivate: false, waveNeeded: true }); + }); + + it('deactivates when EVERY selected value disappeared', () => { + expect(reconcileSelection(['gone', 'also-gone'], opts('a'))) + .toEqual({ value: [], deactivate: true, waveNeeded: true }); + }); + + it('does not deactivate when there was nothing selected to begin with', () => { + // Nothing was contributing, so nothing was lost. + expect(reconcileSelection([], opts('a'))) + .toEqual({ value: [], deactivate: false, waveNeeded: false }); + expect(reconcileSelection([], [])) + .toEqual({ value: [], deactivate: false, waveNeeded: false }); + }); + + it('deactivates when the fresh list is empty and something was selected', () => { + expect(reconcileSelection(['a'], [])) + .toEqual({ value: [], deactivate: true, waveNeeded: true }); + }); + + it('never auto-selects a newly introduced option', () => { + expect(reconcileSelection(['a'], opts('a', 'brand-new')).value).toEqual(['a']); + }); + + it('counts a DUPLICATE committed value once, so dedupe alone is not a wave', () => { + // `['a','a']` and `['a']` bind identically; collapsing them must not re-run + // panels that would receive the exact same array. + expect(reconcileSelection(['a', 'a'], opts('a'))) + .toEqual({ value: ['a'], deactivate: false, waveNeeded: false }); + }); +}); From 249f7a0aa518d2805518f5292f6ba2b4704dc5a7 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sun, 26 Jul 2026 18:41:05 +0200 Subject: [PATCH 2/4] fix(#468): close three selection-destroying paths found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three reproduced before fixing. 1. Apply during the option request cleared a restored selection. `renderDashboard` mounts the surface BEFORE awaiting `session.start()`, and a configured variable publishes with `options: null` (rendered as `[]`) and no error — so the control was fully operable for the whole request. A no-change Apply canonicalized both the draft and the committed selection against the empty list and committed `([], false)`. Verified: trigger read "2 selected", popover opened, `onApply` fired with `[]`. The variable's `loading` status now reaches the control, which renders inert (`aria-disabled`/`aria-busy`, "Loading options…") until `setOptions` — the only thing that clears it — or a batch failure ends the wait. This restores the one piece of #189's status machine that had a real reason to exist; I had dropped it with the Filter-source cascade states it shipped alongside. 2. An option-ORDER change rewrote the bound array with no rerun. Reconciliation re-canonicalized survivors into the new option order while reporting `waveNeeded: false`, so the persisted/session value silently diverged from the one that produced the displayed results. `{name:Array(T)}` promises nothing about membership semantics — panel SQL may read the array positionally — so `reconcileSelection` now preserves committed order and only ever FILTERS. The user's own Apply still canonicalizes; that is a deliberate action against a list they can see. 3. A server-capped option list deleted valid selections. A value living past row 1,000 was treated as removed: dropped, panels rerun, shortened array persisted. Reconciliation is now skipped entirely for a truncated result (the warning still publishes), matching the single-select, which already keeps an off-list value verbatim. The truncation SIGNAL was also unsound, which #461 sharpens: it derived from the KEPT count, so a branch whose 1,001 rows collapsed under the cap through dedup/blank filtering looked complete. It now counts RAW rows against the branch LIMIT, which is what actually says the server cut the result off. Verified live: the trigger goes "Loading options… (disabled)" -> "5 selected", a click during that window is refused, and the restored selection binds intact as ['default','demo','altinity','updater','dmitrii']. 5591 tests, 100% statements + lines; 15 multi-select e2e across all three engines. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BF6uJrfy51KgkeTvfzyWDB --- src/core/variable-options.ts | 19 ++++-- src/core/variable-selection.ts | 44 +++++++----- .../application/dashboard-viewer-session.ts | 24 ++++--- src/styles.css | 13 ++-- src/ui/dashboard.ts | 6 ++ src/ui/filter-bar.ts | 8 +++ src/ui/multi-select-field.ts | 54 ++++++++++----- tests/unit/dashboard-viewer-session.test.ts | 36 +++++++++- tests/unit/dashboard.test.ts | 53 +++++++++++++++ tests/unit/filter-bar.test.ts | 17 +++++ tests/unit/multi-select-field.test.ts | 67 +++++++++++++++++++ tests/unit/variable-selection.test.ts | 15 ++++- 12 files changed, 301 insertions(+), 55 deletions(-) diff --git a/src/core/variable-options.ts b/src/core/variable-options.ts index 2b29ab2c..bea4e1c4 100644 --- a/src/core/variable-options.ts +++ b/src/core/variable-options.ts @@ -359,14 +359,25 @@ export function readVariableOptionBatch( }; } const seen = new Map>(); - for (const name of requested) seen.set(name, new Set()); + const rawCount = new Map(); + for (const name of requested) { seen.set(name, new Set()); rawCount.set(name, 0); } for (const row of response.rows ?? []) { const name = cell(row[0]); const options = byName.get(name); - // `undefined` for a name outside the requested set; `seen` is keyed by the - // same set, so it resolves whenever `options` does. + // `undefined` for a name outside the requested set; `seen` and `rawCount` are + // keyed by the same set, so they resolve whenever `options` does. if (options === undefined) continue; - if (options.length >= VARIABLE_OPTION_CAP) { truncated.add(name); continue; } + // Count RAW rows, before the blank/duplicate filters below. Each branch is + // sent `LIMIT VARIABLE_OPTION_CAP + 1`, so receiving that many rows means the + // SERVER cut the result off and there may be values we never saw. Deriving + // the flag from the KEPT count instead would miss exactly that case whenever + // duplicates or blanks collapsed the branch back under the cap (#461): a + // query returning 1,001 rows of 500 distinct values is still an incomplete + // list, and `applyOptions` must not prune a committed selection against one. + const raw = rawCount.get(name)! + 1; + rawCount.set(name, raw); + if (raw > VARIABLE_OPTION_CAP) truncated.add(name); + if (options.length >= VARIABLE_OPTION_CAP) continue; const value = cell(row[1]); if (value === '') continue; const values = seen.get(name)!; diff --git a/src/core/variable-selection.ts b/src/core/variable-selection.ts index dc768b1c..c384a012 100644 --- a/src/core/variable-selection.ts +++ b/src/core/variable-selection.ts @@ -66,20 +66,30 @@ export interface SelectionReconciliation { /** * Reconcile a previously COMMITTED selection against a fresh option list (the - * batch re-ran on a Dashboard refresh). Intersects `committed` with the values - * still present in `options`, then canonicalizes the survivors by the NEW option - * order — never re-introducing a value that is not in `committed`, because - * auto-select is never this function's job. + * batch re-ran on a Dashboard refresh). Drops the committed values that are no + * longer offered, and does nothing else — it never re-introduces a value that is + * not in `committed` (auto-select is not this function's job), and it never + * REORDERS. * - * - `deactivate` — true iff `committed` was non-empty and the intersection is - * empty: every previously selected value is gone, so the variable has nothing - * left to contribute and the caller returns it to unset rather than binding an - * empty selection. + * Order is preserved deliberately. The committed array is what + * `serializeParamValue` turns into an ordered ClickHouse literal, and panel SQL + * is free to read it order-sensitively (`arrayElement`, `arrayZip`, a positional + * join) — `{name:Array(T)}` promises nothing about membership semantics. An + * automatic refresh that re-canonicalized survivors into the new OPTION order + * would therefore change the value bound into panels whose displayed results came + * from the old order, and persist that difference, while reporting no wave. The + * user's own Apply still canonicalizes (`canonicalizeSelection`): that is a + * deliberate action taken against a list they are looking at. + * + * - `deactivate` — true iff `committed` was non-empty and nothing survived: the + * variable has nothing left to contribute, so the caller returns it to unset + * rather than binding an empty selection. * - `waveNeeded` — true iff the SET of values changed (some committed value was - * dropped because it is no longer a valid option); only then must the caller - * re-run the panels that declare this variable. A pure REORDER or a - * label-only change is `waveNeeded: false` even though `value`'s array order - * may differ from `committed`'s. + * dropped, or a duplicate collapsed into a distinct value that binds + * differently); only then must the caller re-run the panels declaring this + * variable. A label-only or option-ORDER-only change needs no wave, and now + * also leaves `value` byte-identical to `committed`, so the caller has nothing + * to adopt. * * Pure. */ @@ -89,10 +99,12 @@ export function reconcileSelection( ): SelectionReconciliation { const optionValues = new Set(options.map((o) => o.value)); const committedUnique = Array.from(new Set(committed)); - const survivorsUnique = committedUnique.filter((v) => optionValues.has(v)); + const survivors = committedUnique.filter((v) => optionValues.has(v)); return { - value: canonicalizeSelection(survivorsUnique, options), - deactivate: committedUnique.length > 0 && survivorsUnique.length === 0, - waveNeeded: survivorsUnique.length !== committedUnique.length, + value: survivors, + deactivate: committedUnique.length > 0 && survivors.length === 0, + // Compare against the DEDUPED committed list: `['a','a']` and `['a']` bind + // identically, so collapsing them is not a reason to re-run anything. + waveNeeded: survivors.length !== committedUnique.length, }; } diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index f41a7a88..560c5559 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -47,7 +47,7 @@ import { compileVariableOptionBatch, optionSqlDiagnostics, readVariableOptionBatch, } from '../../core/variable-options.js'; import type { VariableOption } from '../../core/variable-options.js'; -import { reconcileSelection, sameSelection } from '../../core/variable-selection.js'; +import { reconcileSelection } from '../../core/variable-selection.js'; import { multiSelectElementType } from '../../core/param-type.js'; import { resolveAuthoredTimeRangeGroups } from '../../core/time-range.js'; import type { DashboardTimeRangeGroup } from '../../core/time-range.js'; @@ -961,7 +961,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa * deliberately kept and shown verbatim (`filter-option-field.ts`'s documented * leniency): a value that is still bound into panels is not something an * option refresh gets to silently drop. */ - function applyOptions(filter: FilterRuntime, options: VariableOption[]): string | null { + function applyOptions(filter: FilterRuntime, options: VariableOption[], incomplete: boolean): string | null { const previous = filter.state.options; const changed = previous === null || previous.length !== options.length @@ -971,15 +971,19 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa filter.state.optionsError = null; if (changed) filter.state.optionsRev += 1; if (!Array.isArray(filter.state.value)) return null; + // A list the server cut off at the cap is not evidence that anything was + // removed: a selected value could simply live past row 1,000. Pruning against + // it would silently delete a valid selection, re-run the panels, and persist + // the shortened array. The single-select already keeps an off-list committed + // value and shows it verbatim; a selection gets the same benefit of the doubt. + // The truncation WARNING still publishes, so the incompleteness is not hidden. + if (incomplete) return null; const reconciled = reconcileSelection(filter.state.value, options); - if (!reconciled.waveNeeded) { - // A pure REORDER, or a label-only change: adopt the new option order so the - // trigger reads in list order, but nothing new is bound, so no panel re-runs. - if (!sameSelection(reconciled.value, filter.state.value)) filter.state.value = reconciled.value; - return null; - } + // `reconcileSelection` never reorders, so a no-wave outcome means there is + // nothing to adopt: the committed value already IS the reconciled one. + if (!reconciled.waveNeeded) return null; // A selected value is gone from the list. Never auto-select a replacement: - // `reconcileSelection` only ever filters and reorders what was already there. + // `reconcileSelection` only ever filters what was already committed. filter.state.value = commitValue(reconciled.value); if (reconciled.deactivate) filter.state.active = false; return filter.def.parameter; @@ -1042,7 +1046,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa for (const filter of filters) { const options = read.byName.get(filter.def.id); if (options === undefined) continue; // not in this batch - const name = applyOptions(filter, options); + const name = applyOptions(filter, options, read.truncated.has(filter.def.id)); if (name !== null) reconciled.push(name); } // Publish as soon as the options land. Without this they would be invisible diff --git a/src/styles.css b/src/styles.css index 3848b7bb..1a9790ad 100644 --- a/src/styles.css +++ b/src/styles.css @@ -2078,11 +2078,14 @@ body.detached-tab .graph-overlay-panel { text-align: left; cursor: pointer; } .ms-trigger:hover { border-color: var(--accent); } -/* The batch-level failure affordance. The trigger carries `.var-input` too, so - `.var-input.is-error` above already paints it — only the affordance differs: - it stays `aria-disabled`, never `disabled`, so the reason in `title` remains - reachable by keyboard and screen reader, and the cursor says it won't open. */ -.ms-trigger.is-error { cursor: default; } +/* The two inert states. The trigger carries `.var-input` too, so + `.var-input.is-error` above already paints the failure — only the affordance + differs: both stay `aria-disabled`, never `disabled`, so the reason in `title` + remains reachable by keyboard and screen reader, and the cursor says the + popover will not open. `.is-loading` is the in-flight option batch, dimmed and + italic like every other pending read in the app. */ +.ms-trigger.is-error, .ms-trigger.is-loading { cursor: default; } +.ms-trigger.is-loading { opacity: .55; font-style: italic; } .ms-popover { position: fixed; z-index: 70; width: 260px; max-width: calc(100vw - 24px); display: flex; flex-direction: column; gap: 6px; padding: 8px; diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index 0a26ce0d..c8d9aea1 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -895,6 +895,12 @@ export async function renderDashboard( // whose option SQL was rejected locally has a specific problem, and the // batch failure is only its reason when it was actually in that batch. optionsError: f.optionsError, + // The batch has not answered for this variable yet. `renderDashboard` + // mounts the whole surface BEFORE awaiting `session.start()`, so a + // configured variable is interactive for the entire option request — + // long enough to open a multi-select and Apply against a list that has + // not arrived, which would clear a restored selection. + loading: f.status === 'loading', ...(selection === null ? {} : { selection }), }; } diff --git a/src/ui/filter-bar.ts b/src/ui/filter-bar.ts index 3fecc5d6..8877c93c 100644 --- a/src/ui/filter-bar.ts +++ b/src/ui/filter-bar.ts @@ -130,6 +130,13 @@ export interface VariableFieldSpec { * than in `app.state.varValues` on purpose — see * `onCommitVariableSelection`. Ignored by every other control branch. */ selection?: readonly string[]; + /** The option batch has not answered for this variable yet (its published + * status is still `loading`). A multi-select renders inert until it has, so a + * no-change Apply against a not-yet-arrived list cannot clear a restored + * selection. Ignored by every other control branch — the single-select + * commits one value the user just picked from what IS shown, so an empty + * list simply offers nothing to pick. */ + loading?: boolean; } /** A per-field execution-status update (`status`/`stale`/`waitingFor` mirror @@ -408,6 +415,7 @@ export function buildFilterBar( options: spec.options ?? [], selected: spec.selection ?? [], active: !!app.state.filterActive[p.name], + loading: !!spec.loading, title: p.name + ': ' + p.type, // Apply is a complete, deliberate action, so it bypasses the keystroke // debounce entirely — same reasoning as the single-select's own commit. diff --git a/src/ui/multi-select-field.ts b/src/ui/multi-select-field.ts index 3efd52ad..bbb82f3d 100644 --- a/src/ui/multi-select-field.ts +++ b/src/ui/multi-select-field.ts @@ -57,6 +57,11 @@ export interface MultiSelectFieldOpts { * value a refresh dropped); never mutated by this module. */ selected: readonly string[]; active: boolean; + /** The option batch has not answered for this variable yet. The control is + * rendered but MUST NOT be operable: `options` is still empty, so an Apply + * would canonicalize a restored selection against nothing and commit a clear. + * Cleared by the first `setOptions`. */ + loading?: boolean; /** The trigger's resting `title` — `name: Type`, from the bar. */ title?: string; onApply(next: string[], active: boolean): void; @@ -92,6 +97,7 @@ export function buildMultiSelectField(opts: MultiSelectFieldOpts): MultiSelectFi let options: readonly VariableOption[] = opts.options; let unavailable: string | null = null; + let loading = !!opts.loading; // The currently-open popover's own close() — non-null iff the popover is open // (`isOpen()` reads this directly rather than tracking a second flag). let closeCurrent: ((closeOpts?: { skipFocus?: boolean }) => void) | null = null; @@ -108,6 +114,10 @@ export function buildMultiSelectField(opts: MultiSelectFieldOpts): MultiSelectFi * one is (a value with no matching option still shows raw, so a dormant * selection never reads as blank); `N selected` beyond that. */ const triggerText = (): string => { + // While the batch is in flight the committed selection cannot be labelled + // (the labels live in the answer), and the control is not operable, so it + // says what it is doing rather than showing a count it cannot act on. + if (loading) return 'Loading options…'; if (!active || selected.length === 0) return 'Not set'; if (selected.length === 1) { const opt = options.find((o) => o.value === selected[0]); @@ -116,14 +126,31 @@ export function buildMultiSelectField(opts: MultiSelectFieldOpts): MultiSelectFi return `${selected.length} selected`; }; + /** Whether the trigger refuses to open — and WHY, for `title`. Loading and a + * batch failure are both "you cannot pick right now"; only the reason differs, + * so they share the one inert state rather than growing a status machine. */ + const inertReason = (): string | null => + (unavailable ?? (loading ? 'Loading this variable’s options…' : null)); + const render = (): void => { - const text = triggerText(); - trigger.textContent = text; - trigger.title = unavailable ?? baseTitle; + const reason = inertReason(); + trigger.textContent = triggerText(); + trigger.title = reason ?? baseTitle; trigger.setAttribute('aria-label', `${name} filter, ${selected.length} selected`); + control.classList.toggle('is-error', unavailable !== null); + trigger.classList.toggle('is-error', unavailable !== null); + trigger.classList.toggle('is-loading', loading && unavailable === null); + if (unavailable === null) trigger.removeAttribute('aria-invalid'); + else trigger.setAttribute('aria-invalid', 'true'); + // `aria-disabled`, never `disabled`: a disabled button is not focusable, so + // the reason in `title` becomes unreachable by keyboard and screen reader, + // and disabling a focused control drops focus to ``. `onTriggerClick` + // is the real gate. + trigger.setAttribute('aria-disabled', reason === null ? 'false' : 'true'); + trigger.setAttribute('aria-busy', String(loading)); }; - const onTriggerClick = (): void => { if (!unavailable) openPopover(); }; + const onTriggerClick = (): void => { if (inertReason() === null) openPopover(); }; trigger.addEventListener('click', onTriggerClick); // Mount a fresh popover. The generic dialog chrome lives in @@ -255,6 +282,11 @@ export function buildMultiSelectField(opts: MultiSelectFieldOpts): MultiSelectFi isOpen: () => closeCurrent !== null, setOptions: (next) => { options = next; + // The batch has answered for this variable, so the control becomes + // operable — this is the ONLY thing that clears `loading`, which is what + // guarantees an Apply can never canonicalize against a list that simply + // had not arrived. + loading = false; // A draft built against the PREVIOUS generation can never be applied // against this one, so an open popover is cancelled outright. `skipFocus` // is deliberately NOT passed: the trigger survives an options swap (unlike @@ -266,17 +298,9 @@ export function buildMultiSelectField(opts: MultiSelectFieldOpts): MultiSelectFi }, setUnavailable: (reason) => { unavailable = reason; - // `aria-disabled` + `aria-invalid`, never `disabled`: a disabled button is - // not focusable, so its `title` — the only place the reason is written — - // becomes unreachable by keyboard and screen reader, and disabling a - // focused control silently drops focus to ``. `onTriggerClick` - // refuses to open while a reason is set, which is the real gate. - control.classList.toggle('is-error', reason !== null); - trigger.classList.toggle('is-error', reason !== null); - if (reason === null) trigger.removeAttribute('aria-invalid'); - else trigger.setAttribute('aria-invalid', 'true'); - trigger.setAttribute('aria-disabled', reason === null ? 'false' : 'true'); - if (reason !== null) closeCurrent?.(); + // A batch failure also ends the wait: there will be no options for this + // wave, and leaving `loading` set would keep claiming one is coming. + if (reason !== null) { loading = false; closeCurrent?.(); } render(); }, focusTrigger: () => trigger.focus(), diff --git a/tests/unit/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index 47b450c6..b65ba771 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -8,6 +8,7 @@ import type { import type { DashboardDocumentV2, DashboardTileV1, SavedQueryV2, } from '../../src/generated/json-schema.types.js'; +import { VARIABLE_OPTION_CAP } from '../../src/core/variable-options.js'; // ── Fixtures ──────────────────────────────────────────────────────────────── @@ -2036,12 +2037,12 @@ describe('batched option execution (#447 phase 2)', () => { expect(session.state.value.filters[0].active).toBe(false); }); - it('keeps a surviving selection, reordered to the new list, without re-running panels', async () => { + it('leaves a surviving selection completely alone when only the option ORDER changed', async () => { let call = 0; const { session, calls } = multiSession((sql) => { if (!isOptionCall(sql)) return { columns: [{ name: 'n' }], rows: [[1]] }; call++; - // Same members, new ORDER — a reorder binds the same set. + // Same members, new ORDER. return call === 1 ? optionRows(['user', 'ada', 'Ada'], ['user', 'bo', 'Bo']) : optionRows(['user', 'bo', 'Bo'], ['user', 'ada', 'Ada']); @@ -2050,12 +2051,41 @@ describe('batched option execution (#447 phase 2)', () => { await session.applyFilter('user', ['ada', 'bo'], true); const before = calls.filter((c) => !isOptionCall(c.sql)).length; await session.refresh(); - expect(session.state.value.filters[0].value).toEqual(['bo', 'ada']); + // The bound literal is UNCHANGED. Adopting the new option order would make + // the persisted value differ from the one that produced the results on + // screen — silently, since this path deliberately runs no wave. + expect(session.state.value.filters[0].value).toEqual(['ada', 'bo']); expect(session.state.value.filters[0].active).toBe(true); // One refresh wave for the tile, and no EXTRA reconciliation wave. expect(calls.filter((c) => !isOptionCall(c.sql)).length).toBe(before + 1); }); + it('never prunes a selection against a list the server CUT OFF at the cap', async () => { + // A value can simply live past row 1,000. Pruning against a truncated list + // would delete a valid selection, re-run the panels, and persist the + // shortened array — the single-select keeps an off-list value verbatim, and + // a selection gets the same benefit of the doubt. The warning still fires. + const capped = (extra: [string, string, string][]) => { + const rows: [string, string, string][] = []; + for (let i = 0; i < VARIABLE_OPTION_CAP + 1; i++) rows.push(['user', `u${i}`, `U${i}`]); + return optionRows(...rows, ...extra); + }; + const { session, calls } = multiSession( + (sql) => (isOptionCall(sql) ? capped([]) : { columns: [{ name: 'n' }], rows: [[1]] }), + { user: { value: ['way-past-the-cap'], active: true } }, + ); + await session.start(); + const before = calls.filter((c) => !isOptionCall(c.sql)).length; + await session.refresh(); + expect(session.state.value.filters[0].value).toEqual(['way-past-the-cap']); + expect(session.state.value.filters[0].active).toBe(true); + // No reconciliation wave — nothing was decided about the selection. + expect(calls.filter((c) => !isOptionCall(c.sql)).length).toBe(before + 1); + // The incompleteness is reported, not hidden. + expect(session.state.value.filterDiagnostics.map((d) => d.code)) + .toContain('variable-options-truncated'); + }); + it('drops a selected value the refresh removed, and re-runs the affected panels ONCE', async () => { let call = 0; const { session, calls } = multiSession((sql) => { diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index 0950a1fe..5b845e22 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -3557,6 +3557,59 @@ describe('renderDashboard — filter-source runtime rebuild + diagnostics (#359) expect(panelRuns()[0].params.param_user).toBe("['ada','bo']"); expect(qs(app.root, '.ms-trigger').textContent).toBe('2 selected'); }); + + it('cannot clear a restored selection by Applying before the option batch answers', async () => { + // `renderDashboard` mounts the whole surface BEFORE awaiting `session.start()`, + // so a configured variable is on screen for the entire option request — long + // enough to open a multi-select and press Apply. With no loading guard the + // draft and the restored selection both canonicalize against the empty list + // that has not arrived, and the no-change Apply commits a CLEAR. + let resolveOptions!: (value: ExecResp) => void; + const pendingOptions = new Promise((resolve) => { resolveOptions = resolve; }); + // A persisted selection to restore, through the REAL default store that + // `renderDashboard` reads `KEYS.dashFilters` from (never the ambient one). + const stored = new Map([[KEYS.dashFilters, JSON.stringify({ + d: { user: { value: ['ada', 'bo'], active: true } }, + })]]); + vi.stubGlobal('localStorage', { + getItem: (k: string) => stored.get(k) ?? null, + setItem: (k: string, v: unknown) => { stored.set(k, String(v)); }, + }); + const { app } = dashApp({ + responder: (sql) => (sql.includes('__variable_name') + ? pendingOptions + : { columns: [{ name: 'n', type: 'UInt8' }], rows: [[1]] }), + workspace: wsWith({ + queries: [q('q1', 'SELECT 1 WHERE u IN {user:Array(String)}')], + tiles: [{ id: 't1', queryId: 'q1' }], + variableConfigs: { user: { sql: 'SELECT a, b FROM users' } }, + }), + }); + const rendering = render(app); + // Flush microtasks up to (but not past) the in-flight option request. + await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); + + const trigger = qs(app.root, '.ms-trigger'); + expect(trigger.textContent).toBe('Loading options…'); + expect(trigger.getAttribute('aria-disabled')).toBe('true'); + trigger.click(); + expect(document.querySelector('.ms-popover')).toBeNull(); + + resolveOptions({ + columns: [ + { name: '__variable_name', type: 'String' }, + { name: 'v', type: 'String' }, + { name: 'l', type: 'String' }, + ], + rows: [['user', 'ada', 'Ada'], ['user', 'bo', 'Bo']], + }); + await rendering; + await flush(); + // The restored selection is intact, and the control is operable now. + expect(qs(app.root, '.ms-trigger').textContent).toBe('2 selected'); + expect(qs(app.root, '.ms-trigger').getAttribute('aria-disabled')).toBe('false'); + vi.unstubAllGlobals(); + }); }); // #303: the isolated per-dashboard filter store (`asb:dashFilters`) — the diff --git a/tests/unit/filter-bar.test.ts b/tests/unit/filter-bar.test.ts index 0b858e6d..11b25f42 100644 --- a/tests/unit/filter-bar.test.ts +++ b/tests/unit/filter-bar.test.ts @@ -756,6 +756,23 @@ describe('buildFilterBar — Dashboard variable controls (#447 phase 2)', () => expect([...document.querySelectorAll('.ms-option-label')].map((n) => n.textContent)).toEqual(['Spain']); }); + it('renders a multi-select inert while its option batch is still loading', () => { + const onCommitVariableSelection = vi.fn(); + const { bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { + variables: { tags: { options: [], loading: true, selection: ['de'] } }, + onCommitVariableSelection, + }); + const trigger = fieldFor(bar, 'tags').querySelector('.ms-trigger')!; + expect(trigger.textContent).toBe('Loading options…'); + expect(trigger.getAttribute('aria-disabled')).toBe('true'); + trigger.click(); + expect(document.querySelector('.ms-popover')).toBeNull(); + // The options landing through the normal fold makes it operable. + bar.setVariableOptions({ tags: { options: OPTIONS, error: null } }); + expect(fieldFor(bar, 'tags').querySelector('.ms-trigger')!.getAttribute('aria-disabled')).toBe('false'); + expect(onCommitVariableSelection).not.toHaveBeenCalled(); + }); + it('renders a multi-select unavailable when the batch failed', () => { const { bar } = build('SELECT * FROM t WHERE x IN {tags:Array(String)}', { variables: { tags: { options: OPTIONS, optionsError: 'Variable options could not be loaded.' } }, diff --git a/tests/unit/multi-select-field.test.ts b/tests/unit/multi-select-field.test.ts index 33be7e21..ccb08f15 100644 --- a/tests/unit/multi-select-field.test.ts +++ b/tests/unit/multi-select-field.test.ts @@ -332,6 +332,73 @@ describe('setOptions', () => { }); }); +describe('loading', () => { + // The surface is mounted BEFORE `session.start()` resolves, so a configured + // variable is on screen for the whole option request. Left operable, a + // no-change Apply would canonicalize a restored selection against the empty + // list that has not arrived yet — and commit a clear. + it('refuses to open while the option batch is in flight', () => { + const { field, trigger } = build({ selected: ['de', 'fr'], active: true, options: [], loading: true }); + expect(trigger.textContent).toBe('Loading options…'); + expect(trigger.getAttribute('aria-disabled')).toBe('true'); + expect(trigger.getAttribute('aria-busy')).toBe('true'); + expect(trigger.classList.contains('is-loading')).toBe(true); + expect(trigger.title).toContain('Loading'); + // Not `disabled` — the reason must stay reachable, and focus must not drop. + expect(trigger.disabled).toBe(false); + trigger.click(); + expect(document.querySelector('.ms-popover')).toBeNull(); + expect(field.isOpen()).toBe(false); + }); + + it('cannot clear a restored selection before its options arrive', () => { + // The exact reviewer scenario, end to end. + const { field, onApply, trigger } = build({ + selected: ['de', 'fr'], active: true, options: [], loading: true, + }); + trigger.click(); + (document.querySelector('.ms-btn-primary') as HTMLButtonElement | null)?.click(); + expect(onApply).not.toHaveBeenCalled(); + field.setOptions(OPTIONS); + // Once the list lands the control is operable and the selection is intact. + expect(trigger.textContent).toBe('2 selected'); + trigger.click(); + expect(boxes().map((cb) => cb.checked)).toEqual([true, true, false]); + }); + + it('setOptions is what makes it operable, and restores the real trigger text', () => { + const { field, trigger } = build({ selected: ['de'], active: true, options: [], loading: true }); + field.setOptions(OPTIONS); + expect(trigger.getAttribute('aria-disabled')).toBe('false'); + expect(trigger.getAttribute('aria-busy')).toBe('false'); + expect(trigger.classList.contains('is-loading')).toBe(false); + expect(trigger.textContent).toBe('Germany'); + trigger.click(); + expect(document.querySelector('.ms-popover')).not.toBeNull(); + }); + + it('a batch failure also ends the wait, and reports the failure instead', () => { + // Leaving `loading` set would keep promising a list that is not coming. + const { trigger } = build({ selected: ['de'], active: true, options: [], loading: true }); + fields[fields.length - 1].setUnavailable('Variable options could not be loaded.'); + expect(trigger.classList.contains('is-loading')).toBe(false); + expect(trigger.classList.contains('is-error')).toBe(true); + expect(trigger.getAttribute('aria-busy')).toBe('false'); + expect(trigger.title).toBe('Variable options could not be loaded.'); + // Still inert, for the other reason. + trigger.click(); + expect(document.querySelector('.ms-popover')).toBeNull(); + }); + + it('is off by default, so a control built with its options is operable at once', () => { + const { trigger } = build(); + expect(trigger.getAttribute('aria-busy')).toBe('false'); + expect(trigger.getAttribute('aria-disabled')).toBe('false'); + trigger.click(); + expect(document.querySelector('.ms-popover')).not.toBeNull(); + }); +}); + describe('setUnavailable', () => { it('marks the trigger without disabling it, and refuses to open', () => { const { field, trigger } = build(); diff --git a/tests/unit/variable-selection.test.ts b/tests/unit/variable-selection.test.ts index 05e094aa..59c25d30 100644 --- a/tests/unit/variable-selection.test.ts +++ b/tests/unit/variable-selection.test.ts @@ -54,9 +54,20 @@ describe('canonicalizeSelection', () => { }); describe('reconcileSelection', () => { - it('keeps survivors, in the NEW option order, with no wave for a pure reorder', () => { + it('PRESERVES the committed order when the option order changes', () => { + // The committed array becomes an ORDERED ClickHouse literal, and panel SQL is + // free to read it order-sensitively — `{name:Array(T)}` promises nothing about + // membership semantics. Re-canonicalizing here would change the value bound + // into panels whose displayed results came from the old order, and persist + // that difference, while reporting no wave. const r = reconcileSelection(['a', 'b'], opts('b', 'a')); - expect(r).toEqual({ value: ['b', 'a'], deactivate: false, waveNeeded: false }); + expect(r).toEqual({ value: ['a', 'b'], deactivate: false, waveNeeded: false }); + }); + + it('preserves committed order among the SURVIVORS too', () => { + const r = reconcileSelection(['z', 'a', 'b'], opts('a', 'b', 'c')); + expect(r.value).toEqual(['a', 'b']); + expect(r.waveNeeded).toBe(true); }); it('needs no wave when the list is unchanged', () => { From 65677ebe72eeadfc50621ce7eb6554c3f931b0a8 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sun, 26 Jul 2026 18:42:17 +0200 Subject: [PATCH 3/4] docs(#468): record the corrected selection-safety semantics CHANGELOG, ADR-0003 addendum and README updated for the three review fixes: the loading guard, order-preserving reconciliation, and no pruning against a truncated option list. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BF6uJrfy51KgkeTvfzyWDB --- CHANGELOG.md | 17 +++++++++++++---- README.md | 11 ++++++++--- docs/ADR-0003-dashboard-viewing.md | 28 ++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 460c7826..9d5ec748 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,10 +32,19 @@ auto-generated per-PR notes; this file is the curated, human-readable history. `param_user=['ada','bo']` with quotes, backslashes, Unicode and big integers all escaped, never a joined `"ada,bo"`. An **empty** selection is unset (the panels wait) rather than a literal `[]`, which would return nothing while looking - filtered. When a refresh drops a selected value the survivors stay bound and the - affected panels re-run in **one** coalesced wave; a pure reorder or a label-only - change re-runs nothing; an empty intersection returns the variable to unset. New - options are never auto-selected. Selections survive a reload. + filtered. Selections survive a reload. + + A committed selection is **never silently changed**. While the option batch is + in flight the control is inert (`Loading options…`) — the Dashboard is mounted + before the request finishes, so an Apply against a list that had not arrived + would otherwise clear a restored selection. An automatic refresh only ever + **removes** values that are genuinely gone, in **one** coalesced wave, and + preserves the committed ORDER: the array binds as an ordered ClickHouse literal + and panel SQL may read it positionally, so re-sorting it to match a new option + order would change what panels bind without re-running them. A label-only or + option-order-only change therefore does nothing at all, and if the option list + came back **truncated** at the 1,000 cap, nothing is pruned — a selected value + may simply live past the cap. New options are never auto-selected. Eligibility is one pure predicate (`multiSelectElementType`) shared by the option batch and the control dispatch, so a type whose option SQL ran can never be one diff --git a/README.md b/README.md index 0d2325d9..76fb77ba 100644 --- a/README.md +++ b/README.md @@ -548,9 +548,14 @@ Which control a configured variable renders follows from its declared type: re-runs only the panels declaring that variable; Cancel, Escape and clicking outside discard the draft. The selection binds as a real ClickHouse array literal, so quotes, backslashes, Unicode and big integers are all escaped by the - same typed serializer everything else uses. When a refresh drops a value you had - selected, the rest of your selection survives; if every selected value is gone, - the variable returns to unset. Selections persist across a reload. + same typed serializer everything else uses. Selections persist across a reload. + + Your selection is never changed behind your back: the control stays inert + (*Loading options…*) until its list arrives, a refresh only ever **removes** + values that are genuinely gone — keeping the order you committed, since the + array binds positionally — and if the option list came back truncated at the + 1,000 cap, nothing is removed at all. If every selected value disappears, the + variable returns to unset. A variable with **no** option SQL keeps the direct input inferred from its declared type. `Tuple`, `Map`, `Nested` and nested `Array(Array(…))` have no inferable diff --git a/docs/ADR-0003-dashboard-viewing.md b/docs/ADR-0003-dashboard-viewing.md index 1949cb20..72057891 100644 --- a/docs/ADR-0003-dashboard-viewing.md +++ b/docs/ADR-0003-dashboard-viewing.md @@ -438,6 +438,34 @@ restores the #189/PR-#364 control on top of the inferred-variable model. supersede tiles mid-refresh and make the outcome classifier judge tiles that are already re-running. One coalesced wave is structural, not a flag to remember. +- **A committed selection is never silently changed** — three separate ways it + could have been, all found in review and all closed: + + - *Applying before the options arrive.* `renderDashboard` mounts the surface + BEFORE awaiting `session.start()`, and a configured variable publishes with + `options: null` and no error, so the control was operable for the entire + request; a no-change Apply canonicalized a restored selection against the + empty list and committed a clear. The variable's `loading` status now reaches + the control, which stays inert until `setOptions` (the only thing that clears + it) or a batch failure. This is the one piece of #189's status machine with a + reason to exist that survived the trim. + - *Re-canonicalizing on refresh.* `reconcileSelection` preserves the COMMITTED + order and only filters. The array binds as an ORDERED literal and panel SQL + may read it positionally (`arrayElement`, a positional join) — `{name:Array(T)}` + promises nothing about membership semantics — so adopting a new option order + would change what panels bind while reporting no wave, and persist the + difference. The user's own Apply still canonicalizes: that is a deliberate + action taken against a list they are looking at. + - *Pruning against a capped list.* A value can live past the 1,000-option cap, + so a truncated result is not evidence that anything was removed; reconciliation + is skipped entirely for one (the warning still publishes). The truncation + SIGNAL was itself unsound — derived from the KEPT count, it missed a branch + whose 1,001 rows collapsed under the cap through dedup or blank filtering + (#461) — and now counts RAW rows against the branch `LIMIT`, which is what + actually says the server cut the result off. The single-select already kept an + off-list committed value verbatim; a selection gets the same benefit of the + doubt. + - **A latent bug fell out.** `dashboard-viewer-session.ts` marked every `Array(T)` variable with valid option SQL as `status: 'error'` via a branch commented "unreachable" — true only for the types that genuinely cannot be option-backed. From c7d41e756f9647e4fbcfb16cd4978fff9de8a6d7 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sun, 26 Jul 2026 18:57:54 +0200 Subject: [PATCH 4/4] fix(#468): Apply must also preserve off-list values from a truncated list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reproduced: with `selected:['u5000']` against a capped list, the trigger showed `u5000` correctly, but a NO-CHANGE Apply committed `([], false)`, and picking one visible value committed `['u1']` — dropping u5000 silently. The previous fix stopped the SESSION pruning against an incomplete list, but left the control canonicalizing against the same partial list on Apply, which undid the preservation one layer up. Both ends need the same fact, so `optionsTruncated` is now published on `ViewerFilterState` and threaded through the spec and `setVariableOptions` to the control. Apply now keeps draft values the list does not offer, appended in committed order, when and only when the list is a prefix. Such a value is invisible — no row exists for it — so the user cannot have deselected it, and a list known to be cut off cannot call it stale. Clear still removes them: emptying the whole draft is the explicit "remove everything". With a COMPLETE list the rule does not apply — the value is genuinely gone and the session has already reconciled it out. Also added `optionsTruncated` to the options signature in dashboard.ts: it changes how the control COMMITS, so a flip must reach it even when the option content it accompanies is byte-identical. Tests: 8 control-level cases (verbatim display, no-change Apply, single pick, several off-list values in order, Clear, complete-list contrast, setOptions toggling the flag, default), the session publishing the flag both ways, and the reviewer's requested shell-level case — a real 1,001-row response through renderDashboard with an off-list persisted value and a no-change Apply. 5601 tests, 100% statements + lines. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BF6uJrfy51KgkeTvfzyWDB --- CHANGELOG.md | 10 ++- README.md | 6 +- docs/ADR-0003-dashboard-viewing.md | 13 +++ .../application/dashboard-viewer-session.ts | 10 +++ src/ui/dashboard.ts | 18 +++- src/ui/filter-bar.ts | 28 +++++-- src/ui/multi-select-field.ts | 51 +++++++++-- tests/unit/dashboard-viewer-session.test.ts | 14 +++- tests/unit/dashboard.test.ts | 48 +++++++++++ tests/unit/multi-select-field.test.ts | 84 +++++++++++++++++++ 10 files changed, 261 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d5ec748..60375111 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,9 +42,13 @@ auto-generated per-PR notes; this file is the curated, human-readable history. preserves the committed ORDER: the array binds as an ordered ClickHouse literal and panel SQL may read it positionally, so re-sorting it to match a new option order would change what panels bind without re-running them. A label-only or - option-order-only change therefore does nothing at all, and if the option list - came back **truncated** at the 1,000 cap, nothing is pruned — a selected value - may simply live past the cap. New options are never auto-selected. + option-order-only change therefore does nothing at all. If the option list came + back **truncated** at the 1,000 cap, nothing is pruned at either end — a + selected value may simply live past the cap, so the refresh leaves it alone + *and* **Apply keeps it** rather than dropping it for being absent from a list + that is known to be a prefix. (**Clear** still removes everything, and with a + complete list an absent value is genuinely gone and does get dropped.) New + options are never auto-selected. Eligibility is one pure predicate (`multiSelectElementType`) shared by the option batch and the control dispatch, so a type whose option SQL ran can never be one diff --git a/README.md b/README.md index 76fb77ba..d22a72f6 100644 --- a/README.md +++ b/README.md @@ -554,8 +554,10 @@ Which control a configured variable renders follows from its declared type: (*Loading options…*) until its list arrives, a refresh only ever **removes** values that are genuinely gone — keeping the order you committed, since the array binds positionally — and if the option list came back truncated at the - 1,000 cap, nothing is removed at all. If every selected value disappears, the - variable returns to unset. + 1,000 cap, nothing is removed at all: a selected value may simply live past the + cap, so it survives both the refresh *and* your next Apply. **Clear** removes + everything, including values the list is too short to show. If every selected + value disappears from a complete list, the variable returns to unset. A variable with **no** option SQL keeps the direct input inferred from its declared type. `Tuple`, `Map`, `Nested` and nested `Array(Array(…))` have no inferable diff --git a/docs/ADR-0003-dashboard-viewing.md b/docs/ADR-0003-dashboard-viewing.md index 72057891..298da28e 100644 --- a/docs/ADR-0003-dashboard-viewing.md +++ b/docs/ADR-0003-dashboard-viewing.md @@ -466,6 +466,19 @@ restores the #189/PR-#364 control on top of the inferred-variable model. off-list committed value verbatim; a selection gets the same benefit of the doubt. + **Incompleteness is published, not private.** The session declining to prune + is undone if the CONTROL's own Apply then canonicalizes the same value away + against the same partial list — `canonicalizeSelection` drops everything the + list does not offer, so a no-change Apply committed `([], false)` and a + single visible pick silently dropped the rest. So `optionsTruncated` rides on + `ViewerFilterState` down to the control, whose Apply keeps draft values the + list does not contain, appended in committed order. They are invisible — no + row exists for them — so the user cannot have deselected one, and the list is + known to be a prefix, so it cannot be called stale. **Clear** still removes + them: it empties the whole draft, which is the explicit "remove everything". + With a COMPLETE list the rule does not apply at all — an off-list value has + genuinely gone, and the session has already reconciled it out. + - **A latent bug fell out.** `dashboard-viewer-session.ts` marked every `Array(T)` variable with valid option SQL as `status: 'error'` via a branch commented "unreachable" — true only for the types that genuinely cannot be option-backed. diff --git a/src/dashboard/application/dashboard-viewer-session.ts b/src/dashboard/application/dashboard-viewer-session.ts index 560c5559..1dd34342 100644 --- a/src/dashboard/application/dashboard-viewer-session.ts +++ b/src/dashboard/application/dashboard-viewer-session.ts @@ -149,6 +149,14 @@ export interface ViewerFilterState { /** Bumped only when this variable's option CONTENT actually changes, so a * consumer can tell a genuine refresh from an unchanged republish. */ optionsRev: number; + /** The server cut this variable's option branch off at the cap, so the list is + * a PREFIX and a committed value may legitimately live past its end. + * + * Published rather than kept private because "incomplete" has to reach the + * CONTROL, not just this layer: the session declining to prune an off-list + * value is undone if the control's own Apply then canonicalizes it away + * against the same partial list. Both ends need the same fact. */ + optionsTruncated: boolean; } /** The Dashboard's per-render layout view (#291) — a discriminated union over @@ -675,6 +683,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa optionsError: localError, options: null, optionsRev: 0, + optionsTruncated: false, }; return { def: { id: name, parameter: name }, state, multiple }; }); @@ -969,6 +978,7 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa filter.state.options = options; filter.state.status = 'ready'; filter.state.optionsError = null; + filter.state.optionsTruncated = incomplete; if (changed) filter.state.optionsRev += 1; if (!Array.isArray(filter.state.value)) return null; // A list the server cut off at the cap is not evidence that anything was diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index c8d9aea1..c73b2791 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -54,7 +54,7 @@ import { queryFavorite } from '../core/saved-query.js'; import { selectOutputColumns } from '../core/select-columns.js'; import { renderKpiCards, KPI_STREAM_ARIA } from './kpi-panel.js'; import { buildFilterBar, FILTER_DEBOUNCE_MS } from './filter-bar.js'; -import type { VariableFieldSpec } from './filter-bar.js'; +import type { VariableFieldSpec, VariableOptionsUpdate } from './filter-bar.js'; import type { FilterBarApp, FilterBarHandle } from './filter-bar.js'; import { pushRecentRange } from '../core/time-range.js'; import { formatChartTimeLabel, formatChartTimeRange } from '../core/time-range.js'; @@ -901,6 +901,10 @@ export async function renderDashboard( // long enough to open a multi-select and Apply against a list that has // not arrived, which would clear a restored selection. loading: f.status === 'loading', + // The list is a PREFIX: a committed value may be valid and simply live + // past the cap. The session already declines to prune one; the control + // must decline too, or its own Apply undoes that one layer up. + optionsIncomplete: f.optionsTruncated, ...(selection === null ? {} : { selection }), }; } @@ -2201,13 +2205,19 @@ export async function renderDashboard( // taken the newest options along with it, so this only runs when the bar // survived — and only when option content or the batch verdict actually // moved, so an unchanged republish touches nothing. + // `optionsTruncated` is part of the signature, not just the payload: it + // changes how the control COMMITS (whether an off-list value is preserved), + // so a flip must reach it even in the contrived case where the option + // content it accompanies is byte-identical. const optionsSig = JSON.stringify(sview.filters.map((f) => - [f.id, f.configured, f.optionsRev, f.status, f.optionsError])); + [f.id, f.configured, f.optionsRev, f.status, f.optionsError, f.optionsTruncated])); if (!rebuilt && optionsSig !== lastOptionsSig) { - const states: Record = {}; + const states: Record = {}; for (const f of sview.filters) { if (!f.configured) continue; - states[f.parameter] = { options: f.options ?? [], error: f.optionsError }; + states[f.parameter] = { + options: f.options ?? [], error: f.optionsError, incomplete: f.optionsTruncated, + }; } currentFilterBar?.setVariableOptions(states); } diff --git a/src/ui/filter-bar.ts b/src/ui/filter-bar.ts index 8877c93c..06062782 100644 --- a/src/ui/filter-bar.ts +++ b/src/ui/filter-bar.ts @@ -115,6 +115,17 @@ export interface BuildFilterBarOptions { onCommitVariableSelection?(name: string, values: string[], active: boolean): void; } +/** One variable's fresh option rows, pushed into an already-built select by + * `FilterBarHandle.setVariableOptions` when a refresh's batch lands. */ +export interface VariableOptionsUpdate { + options: readonly VariableOption[]; + error: string | null; + /** The server cut this variable's branch off at the cap, so `options` is a + * PREFIX. Only a multi-select reads it — to keep committed values the list + * does not contain, rather than canonicalizing them away. */ + incomplete?: boolean; +} + /** #447 phase 2: how ONE Dashboard variable's control differs from a plain * direct-input field. */ export interface VariableFieldSpec { @@ -137,6 +148,12 @@ export interface VariableFieldSpec { * commits one value the user just picked from what IS shown, so an empty * list simply offers nothing to pick. */ loading?: boolean; + /** `options` is a PREFIX — the server cut this variable's branch off at the + * cap. A multi-select then preserves committed values the list does not + * contain instead of canonicalizing them away, matching the session's own + * refusal to reconcile against an incomplete list. Ignored by every other + * control branch. */ + optionsIncomplete?: boolean; } /** A per-field execution-status update (`status`/`stale`/`waitingFor` mirror @@ -171,7 +188,7 @@ interface FieldHandle { * options IN PLACE, so a refresh's option batch landing mid-session never * rebuilds the bar — which would blow away in-progress typing in every other * field and silently cancel any open popover. */ - setOptions?(next: readonly VariableOption[], error: string | null): void; + setOptions?(next: readonly VariableOption[], error: string | null, incomplete?: boolean): void; /** Present on the popover-bearing controls (today: time-range); every fold * over the map that needs one uses optional chaining so a handle without * them is simply skipped. */ @@ -247,7 +264,7 @@ export interface FilterBarHandle { * inherently typing-ending; an options batch lands asynchronously and could * arrive while the user is mid-keystroke in an unrelated field, so rebuilding * on it would discard that input and silently cancel any open popover. */ - setVariableOptions(states: Record): void; + setVariableOptions(states: Record): void; /** #189, #189-F2b, GENERALIZED (#335): the opaque KEY of a popover-bearing * control built by THIS bar instance that currently has its popover open, * or `null` when none does (including a bar that built no such control at @@ -416,6 +433,7 @@ export function buildFilterBar( selected: spec.selection ?? [], active: !!app.state.filterActive[p.name], loading: !!spec.loading, + incomplete: !!spec.optionsIncomplete, title: p.name + ': ' + p.type, // Apply is a complete, deliberate action, so it bypasses the keystroke // debounce entirely — same reasoning as the single-select's own commit. @@ -433,8 +451,8 @@ export function buildFilterBar( // Like the single-select: no transient per-field status of its own, its // only failure mode being the batch's, which arrives through `setOptions`. updateStatus: () => {}, - setOptions: (next, error) => { - field.setOptions(next); + setOptions: (next, error, incomplete) => { + field.setOptions(next, incomplete); field.setUnavailable(error); }, isOpen: field.isOpen, @@ -659,7 +677,7 @@ export function buildFilterBar( setVariableOptions: (states) => { for (const [key, handle] of handles) { const s = states[key]; - if (s) handle.setOptions?.(s.options, s.error); + if (s) handle.setOptions?.(s.options, s.error, s.incomplete); } }, // #189-F2b, GENERALIZED (#335): read by the caller BEFORE disposing this diff --git a/src/ui/multi-select-field.ts b/src/ui/multi-select-field.ts index bbb82f3d..292d2e9e 100644 --- a/src/ui/multi-select-field.ts +++ b/src/ui/multi-select-field.ts @@ -62,6 +62,11 @@ export interface MultiSelectFieldOpts { * would canonicalize a restored selection against nothing and commit a clear. * Cleared by the first `setOptions`. */ loading?: boolean; + /** `options` is a PREFIX — the server cut this variable's branch off at the + * cap. A committed value missing from it may be perfectly valid and simply + * live past the end, so Apply preserves such values instead of canonicalizing + * them away. See `applyBtn`'s handler. */ + incomplete?: boolean; /** The trigger's resting `title` — `name: Type`, from the bar. */ title?: string; onApply(next: string[], active: boolean): void; @@ -71,10 +76,11 @@ export interface MultiSelectFieldOpts { /** `buildMultiSelectField`'s return value. */ export interface MultiSelectFieldHandle { el: HTMLElement; - /** Swap the option list in place when a refresh's batch lands. An OPEN + /** Swap the option list in place when a refresh's batch lands, and say whether + * that list is a PREFIX (the server cut the branch off at the cap). An OPEN * popover is closed as a Cancel first — its draft was built against the * previous generation and must never be applied against this one. */ - setOptions(next: readonly VariableOption[]): void; + setOptions(next: readonly VariableOption[], incomplete?: boolean): void; /** The batch-level failure affordance: `null` clears it. */ setUnavailable(reason: string | null): void; /** Whether the popover is currently open — a caller reads this to decide @@ -98,6 +104,7 @@ export function buildMultiSelectField(opts: MultiSelectFieldOpts): MultiSelectFi let options: readonly VariableOption[] = opts.options; let unavailable: string | null = null; let loading = !!opts.loading; + let incomplete = !!opts.incomplete; // The currently-open popover's own close() — non-null iff the popover is open // (`isOpen()` reads this directly rather than tracking a second flag). let closeCurrent: ((closeOpts?: { skipFocus?: boolean }) => void) | null = null; @@ -231,9 +238,42 @@ export function buildMultiSelectField(opts: MultiSelectFieldOpts): MultiSelectFi syncSelectAll(); }); cancelBtn.addEventListener('click', () => handle.close()); + /** + * The values a set of selections COMMITS to. + * + * Whatever the option list offers is canonicalized by option order — that is + * the list the user is looking at and manipulating. + * + * When the list is a PREFIX (`incomplete`), values it does not contain are + * KEPT, appended in their committed order. Such a value is invisible: there + * is no row for it, so the user cannot have deselected it, and — because the + * list is known to be cut off — it may be perfectly valid rather than stale. + * Dropping it would silently delete a filter the user never touched, which is + * exactly what the session refuses to do when it declines to reconcile + * against a truncated list. Both ends have to agree or the preservation is + * undone here. + * + * Clear still removes them: it empties the whole draft, off-list values + * included, which is the explicit "remove everything" action. + * + * With a COMPLETE list this is plain canonicalization — an off-list value has + * genuinely gone away, and the session has already reconciled it out. + */ + const commitOf = (values: readonly string[]): string[] => { + const visible = canonicalizeSelection(values, options); + if (!incomplete) return visible; + const offered = new Set(options.map((o) => o.value)); + const seen = new Set(visible); + const kept: string[] = []; + for (const v of values) { + if (!offered.has(v) && !seen.has(v)) { seen.add(v); kept.push(v); } + } + return [...visible, ...kept]; + }; + applyBtn.addEventListener('click', () => { - const canonical = canonicalizeSelection([...draft], options); - const prevCanonical = canonicalizeSelection(selected, options); + const canonical = commitOf([...draft]); + const prevCanonical = commitOf(selected); const activeNext = canonical.length > 0; // A no-op Apply (same canonical selection AND same active flag) closes // silently — `onApply` fires exactly once otherwise. @@ -280,8 +320,9 @@ export function buildMultiSelectField(opts: MultiSelectFieldOpts): MultiSelectFi return { el: control, isOpen: () => closeCurrent !== null, - setOptions: (next) => { + setOptions: (next, nextIncomplete = false) => { options = next; + incomplete = nextIncomplete; // The batch has answered for this variable, so the control becomes // operable — this is the ONLY thing that clears `loading`, which is what // guarantees an Apply can never canonicalize against a list that simply diff --git a/tests/unit/dashboard-viewer-session.test.ts b/tests/unit/dashboard-viewer-session.test.ts index b65ba771..d40be324 100644 --- a/tests/unit/dashboard-viewer-session.test.ts +++ b/tests/unit/dashboard-viewer-session.test.ts @@ -310,8 +310,8 @@ describe('inferred variables (#447)', () => { // direct input and has nothing to load ('idle'). Both start with // `options: null`; only a completed batch replaces that. expect(state.filters).toEqual([ - { id: 'region', parameter: 'region', label: 'region', active: false, value: '', status: 'loading', configured: true, optionsError: null, options: null, optionsRev: 0 }, - { id: 'top', parameter: 'top', label: 'top', active: false, value: '', status: 'idle', configured: false, optionsError: null, options: null, optionsRev: 0 }, + { id: 'region', parameter: 'region', label: 'region', active: false, value: '', status: 'loading', configured: true, optionsError: null, options: null, optionsRev: 0, optionsTruncated: false }, + { id: 'top', parameter: 'top', label: 'top', active: false, value: '', status: 'idle', configured: false, optionsError: null, options: null, optionsRev: 0, optionsTruncated: false }, ]); expect(state.resettableFilterIds).toEqual([]); expect(state.activeFilterCount).toBe(0); @@ -2084,6 +2084,16 @@ describe('batched option execution (#447 phase 2)', () => { // The incompleteness is reported, not hidden. expect(session.state.value.filterDiagnostics.map((d) => d.code)) .toContain('variable-options-truncated'); + // And PUBLISHED per variable, so the control can apply the same rule — the + // session's preservation is undone if the control's Apply then + // canonicalizes the off-list value away against the same partial list. + expect(session.state.value.filters[0].optionsTruncated).toBe(true); + }); + + it('publishes optionsTruncated false for a complete list', async () => { + const { session } = multiSession(usersRespond(['user', 'ada', 'Ada'])); + await session.start(); + expect(session.state.value.filters[0].optionsTruncated).toBe(false); }); it('drops a selected value the refresh removed, and re-runs the affected panels ONCE', async () => { diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index 5b845e22..e6d2730f 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -4,6 +4,7 @@ import { activeDashboardView, dashboardViewSelection, partitionKpiBands, } from '../../src/core/dashboard.js'; import { KEYS } from '../../src/state.js'; +import { VARIABLE_OPTION_CAP } from '../../src/core/variable-options.js'; import * as storage from '../../src/core/storage.js'; import { CHART_ROW_CAPS } from '../../src/core/chart-data.js'; import { renderDashboard } from '../../src/ui/dashboard.js'; @@ -3610,6 +3611,53 @@ describe('renderDashboard — filter-source runtime rebuild + diagnostics (#359) expect(qs(app.root, '.ms-trigger').getAttribute('aria-disabled')).toBe('false'); vi.unstubAllGlobals(); }); + + it('a no-change Apply against a TRUNCATED list keeps the off-list selection', () => { + // End to end: the server caps the option branch, so a committed value can be + // valid and simply live past the cap. The session declines to prune it — and + // the control must decline too, or its own Apply canonicalizes it away + // against the same partial list and undoes that one layer up. + const stored = new Map([[KEYS.dashFilters, JSON.stringify({ + d: { user: { value: ['way-past-the-cap'], active: true } }, + })]]); + vi.stubGlobal('localStorage', { + getItem: (k: string) => stored.get(k) ?? null, + setItem: (k: string, v: unknown) => { stored.set(k, String(v)); }, + }); + const rows: unknown[][] = []; + for (let i = 0; i < VARIABLE_OPTION_CAP + 1; i++) rows.push(['user', `u${i}`, `U${i}`]); + const { app, calls } = dashApp({ + responder: (sql) => (sql.includes('__variable_name') + ? { + columns: [ + { name: '__variable_name', type: 'String' }, + { name: 'v', type: 'String' }, + { name: 'l', type: 'String' }, + ], + rows, + } + : { columns: [{ name: 'n', type: 'UInt8' }], rows: [[1]] }), + workspace: wsWith({ + queries: [q('q1', 'SELECT 1 WHERE u IN {user:Array(String)}')], + tiles: [{ id: 't1', queryId: 'q1' }], + variableConfigs: { user: { sql: 'SELECT a, b FROM users' } }, + }), + }); + return render(app).then(async () => { + const panelRuns = () => calls.filter((c) => !c.sql.includes('__variable_name')); + const before = panelRuns().length; + // Shown verbatim: there is no option row for it. + expect(qs(app.root, '.ms-trigger').textContent).toBe('way-past-the-cap'); + qs(app.root, '.ms-trigger').click(); + (document.querySelector('.ms-btn-primary') as HTMLButtonElement).click(); + await flush(); + // Nothing committed, nothing re-run, and the binding is untouched. + expect(qs(app.root, '.ms-trigger').textContent).toBe('way-past-the-cap'); + expect(panelRuns()).toHaveLength(before); + expect(panelRuns().at(-1)!.params.param_user).toBe("['way-past-the-cap']"); + vi.unstubAllGlobals(); + }); + }); }); // #303: the isolated per-dashboard filter store (`asb:dashFilters`) — the diff --git a/tests/unit/multi-select-field.test.ts b/tests/unit/multi-select-field.test.ts index ccb08f15..932ef6e8 100644 --- a/tests/unit/multi-select-field.test.ts +++ b/tests/unit/multi-select-field.test.ts @@ -306,6 +306,90 @@ describe('Clear / Cancel / Apply', () => { }); }); +// The server caps each option branch, so a returned list can be a PREFIX and a +// committed value may be valid yet absent from it. The session already declines +// to prune one; without the same rule here, the control's own Apply would undo +// that preservation one layer up. +describe('an INCOMPLETE option list', () => { + const partial = { options: OPTIONS, selected: ['gone-past-the-cap'], active: true, incomplete: true }; + + it('shows an off-list committed value verbatim', () => { + expect(build(partial).trigger.textContent).toBe('gone-past-the-cap'); + }); + + it('a no-change Apply commits nothing', () => { + const { onApply, trigger } = build(partial); + trigger.click(); + btn('.ms-btn-primary').click(); + expect(onApply).not.toHaveBeenCalled(); + }); + + it('picking a visible value KEEPS the off-list one', () => { + const { onApply, trigger } = build(partial); + trigger.click(); + check(boxes()[1], true); + btn('.ms-btn-primary').click(); + // Visible picks canonicalize by option order; the invisible value the user + // could not have deselected is appended, in committed order. + expect(onApply).toHaveBeenCalledWith(['fr', 'gone-past-the-cap'], true); + }); + + it('keeps SEVERAL off-list values, in committed order', () => { + const { onApply, trigger } = build({ + ...partial, selected: ['zz-past', 'de', 'aa-past'], + }); + trigger.click(); + btn('.ms-btn-primary').click(); + expect(onApply).not.toHaveBeenCalled(); // still a no-op + trigger.click(); // Apply closed it; re-open to make a real change + check(boxes()[1], true); + btn('.ms-btn-primary').click(); + expect(onApply).toHaveBeenCalledWith(['de', 'fr', 'zz-past', 'aa-past'], true); + }); + + it('Clear still removes them — it is the explicit "remove everything"', () => { + const { onApply, trigger } = build(partial); + trigger.click(); + btn('.ms-btn-clear').click(); + btn('.ms-btn-primary').click(); + expect(onApply).toHaveBeenCalledWith([], false); + }); + + it('does NOT preserve off-list values once the list is complete', () => { + // A complete list means the value genuinely went away, and the session has + // already reconciled it out — keeping it here would resurrect it. + const { onApply, trigger } = build({ ...partial, incomplete: false }); + trigger.click(); + check(boxes()[1], true); + btn('.ms-btn-primary').click(); + expect(onApply).toHaveBeenCalledWith(['fr'], true); + }); + + it('setOptions can turn the flag on and off with the list', () => { + const { field, onApply, trigger } = build({ + options: OPTIONS, selected: ['past-the-cap'], active: true, incomplete: false, + }); + // A later refresh comes back truncated: the value is preserved from then on. + field.setOptions(OPTIONS, true); + trigger.click(); + btn('.ms-btn-primary').click(); + expect(onApply).not.toHaveBeenCalled(); + // And a complete refresh drops it again. + field.setOptions(OPTIONS, false); + trigger.click(); + btn('.ms-btn-primary').click(); + expect(onApply).toHaveBeenCalledWith([], false); + }); + + it('defaults to complete when setOptions omits the flag', () => { + const { field, onApply, trigger } = build(partial); + field.setOptions(OPTIONS); + trigger.click(); + btn('.ms-btn-primary').click(); + expect(onApply).toHaveBeenCalledWith([], false); + }); +}); + describe('setOptions', () => { it('swaps the list and re-renders the committed label', () => { const { field, trigger } = build({ selected: ['de'], active: true });