Skip to content

fix(#478): make the variable bar's recents port a live read, not a snapshot - #563

Merged
BorisTyshkevich merged 2 commits into
mainfrom
fix/variable-bar-live-recents-478
Jul 29, 2026
Merged

fix(#478): make the variable bar's recents port a live read, not a snapshot#563
BorisTyshkevich merged 2 commits into
mainfrom
fix/variable-bar-live-recents-478

Conversation

@BorisTyshkevich

Copy link
Copy Markdown
Collaborator

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 VariableBarApp cast with an explicit adapter. It correctly aliased the activation map — but it also copied varRecent as a plain data property, and that field has the opposite lifecycle from the others:

  • varValues / filterActive are mutated in place → copying the reference is fine.
  • varRecent is replaced wholesale: workbench-parameter-session.ts's recordBoundParams, clearVarRecent and clearAllVarRecent all assign a new RecentMap through setVarRecent, and app.ts:908 does app.state.varRecent = map. Its own comment says so: "(replaces the old one), hence setVarRecent".

Meanwhile variable-bar.ts reads its recents source at call time, on every dropdown open and keystroke. Before #555 the cast handed it the real App, so that read was live; the adapter froze it at construction.

Symptoms on main today, in a detached Data pane:

  • "Clear recent" updates persisted state, but reopening the field's dropdown still lists the cleared values;
  • a successful detached re-run records a new recent value the open pane never shows.

The fix

Make liveness explicit in the port rather than passing recents as data:

state: { varValues, activeByName }   // aliased objects, mutated in place
getVarRecent(): RecentMap            // live read, called per dropdown open

A callback rather than a getter on state is 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:822 already had varRecent: state.varRecent), so this corrects both callers rather than leaving one behind now that the port expresses the contract.

Also updates the VariableBarApp doc comment, which still claimed a real App and makeApp() satisfy the port directly with no cast — untrue since #555, as both callers now build adapters.

Tests

The previous test only asserted clearVarRecent was called, against a no-op spy — it would still pass with the bug. Six new behavioural tests instead, two per adapter (variable-bar.ts directly, plus the real production adapters via expandDataPane and renderDashboard):

  • replace varRecent wholesale with a map containing a new value → reopen the dropdown → the new value appears;
  • clear a recent through a production-like clearVarRecent that 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-built variableBarApp is 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 build and npm run check:arch pass.

Refs #478, #555.

Checklist

  • npm test passes (the per-file coverage gate is non-negotiable)
  • Tests added/updated in the same change as the code
  • npm run build succeeds (single-file dist/sql.html)
  • Layers kept honest: pure logic in src/core/, network in src/net/ (injected fetch), DOM in src/ui/
  • No new runtime dependency
  • README / CHANGELOG.md ([Unreleased]) updated if behavior or the deployed surface changed
  • Reconciled affected tracked work (roadmap Roadmap to 1.0.0 #68, the issue body, ADR/CHANGELOG) if this change reshaped it

🤖 Generated with Claude Code

https://claude.ai/code/session_01GiubaoqEuBzAyo5C4P8Vqr

BorisTyshkevich and others added 2 commits July 29, 2026 17:27
…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
@BorisTyshkevich
BorisTyshkevich merged commit 0a6f79d into main Jul 29, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant