From b3b0bc18d04e3ecc10c064b3ed8b39800a52a581 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sun, 26 Jul 2026 21:16:25 +0200 Subject: [PATCH] refactor(#459): rename two missed callback locals; correct the filterActive rationale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. Two items, one code and one accuracy. `ui/dashboard.ts:752` still had `(filter) => !ordinaryTimeIds.has(filter.id)` and `(filter) => filter.id`. The scripted pass only matched callbacks whose body STARTED with `filter.`, so a chained predicate slipped through. Renamed to `variable`. Every other surviving `filter` local is a documented exception: `schema.ts:229` (schema text search) and `spec-examples.test.js:88` (iterating a legacy dashboard-v1 example's `filters`). The accuracy fix matters more. Both the CHANGELOG and the ADR addendum justified keeping the `filterActive` family as "the WORKBENCH's optional-block activation, not a Dashboard variable at all". The first half is right; the last clause is wrong, and the review caught it. `VariableBarApp` — the SHARED port both the Dashboard and the detached Data view build the bar through — also declares `state.filterActive` and `params.saveFilterActive`, and `ui/dashboard.ts:773-781` satisfies it with a purely local `draftActive` map and a no-op `saveFilterActive`. So one of that port's two callers has no Workbench state and nothing persisted behind the name. The exception itself still holds, on a narrower argument now recorded: what the name denotes is activation of a parameter's optional `/*[ … ]*/` filter block (`variable-bar.ts:539` is literally `app.state.filterActive[p.name] = input.value !== ''`), a live SQL-filter concept that predates the curated Dashboard model and survived its removal — not a surviving curated-filter identifier. But naming a shared port after one caller's persisted field is a leaky abstraction, so #478 (inbox) tracks renaming the port to caller-neutral members while `AppState.filterActive`, `saveFilterActive`, `effectiveFilterActive` and `asb:filterActive` stay put. That is deliberately NOT folded in here, for two reasons the issue records: `params` is a `Pick`, so renaming a member stops it being a Pick and the detached caller can no longer pass the app straight through; and the bar MUTATES the caller's map in place before calling `saveFilterActive()`, so an adapter that copies rather than aliases would silently stop persisting activation in the detached view — with no test today that would fail. #478 requires that write-through test regardless of the rename. npm test 5603 passed, tsc/check:arch/build clean. Part of #459 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BF6uJrfy51KgkeTvfzyWDB --- CHANGELOG.md | 12 ++++++++---- docs/ADR-0003-dashboard-viewing.md | 29 +++++++++++++++++++++++++---- src/ui/dashboard.ts | 2 +- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 388fef02..afb7af2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -125,10 +125,14 @@ auto-generated per-PR notes; this file is the curated, human-readable history. **Saved Dashboard variable values are untouched.** The localStorage key stays `asb:dashFilters`, keeping its historical name deliberately: renaming it would have discarded every committed value on the next load. `asb:filterActive` and - its helpers keep their names too — they are the Workbench's optional-block - activation, not Dashboard variables. Both exceptions are documented where they - live, and a new test pins every persisted key string so a future rename cannot - orphan real data while the suite stays green. + its helpers keep their names too — what they denote is activation of a + parameter's optional `/*[ … ]*/` filter block, a live SQL-filter concept that + predates the curated Dashboard model and outlived it, and that key is persisted + as well. (The shared variable-bar port also exposes that name to its Dashboard + caller, where nothing is persisted behind it; that leaky abstraction is #478, + deliberately not folded into a rename.) Both exceptions are documented where + they live, and a new test pins every persisted key string so a future rename + cannot orphan real data while the suite stays green. - **Dashboard variable option SQL is edited in the main editor, as its own tab** (#457). Clicking a variable in the Dashboards tree switches to Query and opens diff --git a/docs/ADR-0003-dashboard-viewing.md b/docs/ADR-0003-dashboard-viewing.md index 1cead524..208d0b9c 100644 --- a/docs/ADR-0003-dashboard-viewing.md +++ b/docs/ADR-0003-dashboard-viewing.md @@ -513,10 +513,31 @@ unchanged. Three decisions are worth recording. of them silently on the next load, and a read-old/write-new migration is a behaviour change in a change whose whole contract is "no behaviour change". The property name deliberately still matches the key string so the two cannot - drift. `state.filterActive`/`asb:filterActive` (and `saveFilterActive`/ - `effectiveFilterActive`) are the second exception, for a different reason: they - name the WORKBENCH's optional-block activation map — a live concept that is not - a Dashboard variable at all — and are likewise persisted. + drift. + +- **`filterActive` stays, and the reason is narrower than "it's the Workbench's".** + `state.filterActive`/`asb:filterActive`/`saveFilterActive`/ + `effectiveFilterActive` keep their names because what they name is *activation + of a parameter's optional `/*[ … ]*/` filter block* — `variable-bar.ts`'s + `app.state.filterActive[p.name] = input.value !== ''` is exactly that — which is + a live SQL-filter concept that predates the curated Dashboard model and survived + its removal. The key is persisted besides. + + What that argument does **not** cover, and an earlier draft of this addendum + wrongly implied it did: `VariableBarApp` — the SHARED port both the Dashboard and + the detached Data view build the bar through — also declares `state.filterActive` + and `params.saveFilterActive`, and the Dashboard satisfies it with a purely local + `draftActive` map and a **no-op** `saveFilterActive`. So one of the port's two + callers has no Workbench state and nothing persisted behind that name. The + concept is still optional-block activation in both callers, so this is a leaky + abstraction rather than a surviving curated-filter name — but naming a shared + port after one caller's persisted field is worth fixing on its own terms, not + inside a rename. Deferred to #478 deliberately: `params` is a + `Pick` (renaming a member stops it being a Pick, + so the detached caller can no longer pass the app straight through), and the bar + MUTATES the caller's map in place before calling `saveFilterActive()` — an + adapter that copies instead of aliasing would silently stop persisting + activation in the detached view, with no test today that would fail. - **The rename is unfalsifiable by construction, so two guards were added.** A pure rename passes its whole suite whether or not it is correct, and every diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index d3ae241a..e4bbb92b 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -749,7 +749,7 @@ export async function renderDashboard( const ordinaryTimeIds = new Set(session.timeRangeGroups.flatMap((group) => [group.fromVariableId, group.toVariableId])); const ordinaryVariableIds = session.state.value.variableStates - .filter((filter) => !ordinaryTimeIds.has(filter.id)).map((filter) => filter.id); + .filter((variable) => !ordinaryTimeIds.has(variable.id)).map((variable) => variable.id); const clearVariablesBtn = h('button', { class: 'dash-clear-variables', type: 'button', disabled: true, onclick: () => { void session.resetVariables(ordinaryVariableIds); },