diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bd2e277..60375111 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,59 @@ 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. 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. 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 + 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 +90,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..d22a72f6 100644 --- a/README.md +++ b/README.md @@ -533,10 +533,36 @@ 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. 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: 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 +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..298da28e 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,113 @@ 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 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. + + **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. + 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..bea4e1c4 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), ); @@ -345,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 new file mode 100644 index 00000000..c384a012 --- /dev/null +++ b/src/core/variable-selection.ts @@ -0,0 +1,110 @@ +// 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). 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. + * + * 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, 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. + */ +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 survivors = committedUnique.filter((v) => optionValues.has(v)); + return { + 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 a2a799f6..1dd34342 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 } 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'; @@ -147,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 @@ -384,6 +394,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 +421,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 +632,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 +654,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 @@ -612,8 +683,9 @@ export function createDashboardViewerSession(deps: DashboardViewerDeps): Dashboa optionsError: localError, options: null, optionsRev: 0, + optionsTruncated: false, }; - 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 +761,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 +961,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[], incomplete: boolean): string | null { const previous = filter.state.options; const changed = previous === null || previous.length !== options.length @@ -887,7 +978,25 @@ 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 + // 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); + // `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 what was already committed. + filter.state.value = commitValue(reconciled.value); + if (reconciled.deactivate) filter.state.active = false; + return filter.def.parameter; } /** @@ -906,8 +1015,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 +1024,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 +1034,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 +1052,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, 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 // 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 +1165,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 +1174,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 +1255,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 +1269,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..1a9790ad 100644 --- a/src/styles.css +++ b/src/styles.css @@ -2061,11 +2061,72 @@ 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 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; + 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 +2195,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..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'; @@ -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,17 @@ 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', + // 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 }), }; } const onCommit = (name: string): void => { @@ -900,6 +918,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 +957,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 +2142,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 @@ -2169,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 0fc1d083..06062782 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,22 @@ 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; +} + +/** 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 @@ -113,6 +136,24 @@ 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[]; + /** 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; + /** `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 @@ -147,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. */ @@ -223,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 @@ -346,26 +387,82 @@ 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], + 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. + // 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, incomplete) => { + field.setOptions(next, incomplete); + 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 +610,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); }; @@ -569,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 new file mode 100644 index 00000000..292d2e9e --- /dev/null +++ b/src/ui/multi-select-field.ts @@ -0,0 +1,353 @@ +// 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 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; + /** `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; + 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, 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[], 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 + * 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; + 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; + + 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 => { + // 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]); + return opt ? opt.label : selected[0]; + } + 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 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 (inertReason() === null) 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()); + /** + * 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 = 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. + 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, 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 + // 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 + // 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; + // 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(), + 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..d40be324 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 ──────────────────────────────────────────────────────────────── @@ -309,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); @@ -1944,6 +1945,226 @@ 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('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. + 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(); + // 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'); + // 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 () => { + 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..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'; @@ -3516,6 +3517,147 @@ 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'); + }); + + 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(); + }); + + 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/filter-bar.test.ts b/tests/unit/filter-bar.test.ts index 583fbcb0..11b25f42 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,141 @@ 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 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.' } }, + }); + 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..932ef6e8 --- /dev/null +++ b/tests/unit/multi-select-field.test.ts @@ -0,0 +1,551 @@ +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); + }); +}); + +// 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 }); + 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('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(); + 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..59c25d30 --- /dev/null +++ b/tests/unit/variable-selection.test.ts @@ -0,0 +1,112 @@ +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('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: ['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', () => { + 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 }); + }); +});