fix(#478): make the variable bar's recents port a live read, not a snapshot - #563
Merged
Merged
Conversation
…ot a snapshot The VariableBarApp adapters built by results.ts (detached Data, #555) and dashboard.ts (pre-#555) copied varRecent into state as a plain data property, captured once at adapter-construction time. Unlike activeByName, varRecent is replaced wholesale on every record/clear (workbench-parameter-session.ts), so the copy went stale immediately: Clear recent persisted but an open dropdown kept listing cleared values, and a re-run's newly recorded value never showed until the pane reopened. VariableBarApp now exposes getVarRecent(): RecentMap, a live-read callback both adapters implement over their own varRecent reference, read at call time. Updated the single consumer in variable-bar.ts, both production adapters, the port's doc comment, and every other port construction (variable-bar.test.ts's asBarApp, tests/e2e/time-range.html). Added behavioural regression tests (wholesale-replacement-then-reopen, and Clear-recent through a production-like clearVarRecent) in variable-bar.test.ts, results.test.ts, and dashboard.test.ts — each verified to fail against the reverted snapshot form before being restored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiubaoqEuBzAyo5C4P8Vqr
This was referenced Jul 29, 2026
lesandie
pushed a commit
to lesandie/altinity-sql-browser
that referenced
this pull request
Aug 2, 2026
The e2e job was gated to tags, schedule and manual dispatch, so it reported `skipped` on every pull request and every push to `main` while the overall run still reported success. A check that never runs is not a check. The cost was concrete: two `tile-open-workbench` specs have been failing deterministically on chromium, firefox AND webkit, and nothing surfaced it. Today's `bug`/`low` batch (Altinity#554-Altinity#558, Altinity#561-Altinity#563) all merged "green"; two of those PRs changed or added e2e specs that CI never executed. The failures only appeared when the workflow was dispatched by hand (6 failed / 568 passed on `main`), and a second dispatch at f68861c showed the identical 6 failures, so they pre-date that batch. Filed as Altinity#565. Pull requests now run **Chromium only**, path-gated on anything that can move rendered geometry (`src/**` — which is where `styles.css` lives — plus `tests/e2e/**`, `playwright.config.js`, `build/**`, `schemas/**`, the manifests and the workflows). Both Altinity#565 failures reproduce on Chromium, so one engine is enough to have caught them, and it is cheap enough to sit on every relevant PR. Tags, nightly and manual dispatch keep the full three-engine matrix: engine-specific breakage is real here, and that is what a release has to clear. The `gate` job needed no change — it already fails on any `failure` among its needs and tolerates a legitimately skipped job. Altinity#565's two specs are quarantined with `test.fail()` so this gate is meaningful from the first run rather than red on arrival. Deliberately not `skip`/`fixme`: the specs keep executing, so when the underlying bug is fixed they report "expected to fail, but passed" and the quarantine has to be removed. Their assertions encode Altinity#535's intended widen semantics and must not be rewritten to match the broken output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiubaoqEuBzAyo5C4P8Vqr
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
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.
What & why
Fixes a user-visible regression introduced by PR #555 (issue #478), found in review of that PR.
#555 replaced detached Data's
app as VariableBarAppcast with an explicit adapter. It correctly aliased the activation map — but it also copiedvarRecentas a plain data property, and that field has the opposite lifecycle from the others:varValues/filterActiveare mutated in place → copying the reference is fine.varRecentis replaced wholesale:workbench-parameter-session.ts'srecordBoundParams,clearVarRecentandclearAllVarRecentall assign a newRecentMapthroughsetVarRecent, andapp.ts:908doesapp.state.varRecent = map. Its own comment says so: "(replaces the old one), hencesetVarRecent".Meanwhile
variable-bar.tsreads its recents source at call time, on every dropdown open and keystroke. Before #555 the cast handed it the realApp, so that read was live; the adapter froze it at construction.Symptoms on
maintoday, in a detached Data pane:The fix
Make liveness explicit in the port rather than passing recents as data:
A callback rather than a getter on
stateis deliberate — it makes the live-read contract explicit and impossible for a future adapter to satisfy with a snapshot.The Dashboard adapter had the identical defect and is fixed too. That one pre-dates #555 (
git show 1b212cb:src/ui/dashboard.ts:822already hadvarRecent: state.varRecent), so this corrects both callers rather than leaving one behind now that the port expresses the contract.Also updates the
VariableBarAppdoc comment, which still claimed a realAppandmakeApp()satisfy the port directly with no cast — untrue since #555, as both callers now build adapters.Tests
The previous test only asserted
clearVarRecentwas called, against a no-op spy — it would still pass with the bug. Six new behavioural tests instead, two per adapter (variable-bar.tsdirectly, plus the real production adapters viaexpandDataPaneandrenderDashboard):varRecentwholesale with a map containing a new value → reopen the dropdown → the new value appears;clearVarRecentthat actually replaces the map → reopen → the value is gone.Every one was sabotage-checked: reverting the adapter to the snapshot form turns each red, restored by writing the saved bytes back (never
git checkout --).tests/e2e/time-range.html's hand-builtvariableBarAppis updated to the new port shape — an App-shape change has to reach the e2e fixtures, and only e2e would have caught it.npm test→ 201 files / 6665 tests.npm run buildandnpm run check:archpass.Refs #478, #555.
Checklist
npm testpasses (the per-file coverage gate is non-negotiable)npm run buildsucceeds (single-filedist/sql.html)src/core/, network insrc/net/(injected fetch), DOM insrc/ui/CHANGELOG.md([Unreleased]) updated if behavior or the deployed surface changed🤖 Generated with Claude Code
https://claude.ai/code/session_01GiubaoqEuBzAyo5C4P8Vqr