refactor(#459): rename two missed callback locals; correct the filterActive rationale - #479
Merged
Merged
Conversation
…Active rationale 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<WorkbenchParameterSession, …>`, 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BF6uJrfy51KgkeTvfzyWDB
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ui/dashboard.tsthat the scripted rename pass missed because it only matched callbacks whose body started withfilter.((filter) => !ordinaryTimeIds.has(filter.id)and(filter) => filter.id).filterActivefamily is not purely "the WORKBENCH's optional-block activation" —VariableBarApp, the shared port both the Dashboard and detached Data view build the bar through, also declares it.paramsis aPick<WorkbenchParameterSession, …>so folding the rename into Rename surviving filter terminology to Dashboard variables #459 would break that Pick, and the bar mutates the caller's map in place before callingsaveFilterActive()— an adapter that copies rather than aliases would silently stop persisting activation in the detached view, with no test today that would catch it. Rename shared VariableBar activation port to caller-neutral names #478 tracks the write-through test this requires.This commit was cherry-picked from
refactor/variable-terminology-459(33f5476), which was pushed after PR #474 (the #459 PR) had already merged, so it never made it into a PR.Test plan
npm test— 5603 passed, 100% statement/line coveragetsc --noEmit/check:archclean (vianpm testpretest)Part of #459
🤖 Generated with Claude Code
https://claude.ai/code/session_011sMwR4BHM85MExSBYXzTYj