test(#473): assert the variable toolbar's real display contract - #556
Merged
Conversation
…phantom class The mobile Dashboard e2e fixture rendered a `has-variables` class on the variable toolbar that `src/ui/dashboard.ts` never sets — the app always builds the toolbar with just `dash-toolbar dash-toolbar-variables` and hides it via an inline `style.display: 'none'` when there are no ordinary variables. Drop the phantom class from the fixture and re-point the two specs (scroll/popover-escape and desktop row-height) at the real class, asserting the toolbar's actual `style.display` instead of a hand-written marker. Sabotage-verified: forcing `style="display: none"` onto the fixture's toolbar turns both re-pointed specs red; reverted after confirming. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiubaoqEuBzAyo5C4P8Vqr
…ract
The hidden branch of ui/dashboard.ts's `hasOrdinaryVariables ? undefined :
{ display: 'none' }` ternary already had a real-app unit test; the shown
branch was asserted nowhere against createApp, only against the e2e
fixture's hand-written markup. Add a dashApp/wsWith render with an ordinary
(non-time-range) variable and assert `.dash-toolbar-variables`'s inline
style.display is '' (no override). Sabotage-verified: flipping the ternary in
src/ui/dashboard.ts turns both this test and the existing hidden-toolbar test
red; reverted after confirming (checksum-verified restore).
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
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.
What & why
Closes #473. The mobile Dashboard e2e fixture rendered a
has-variablesclass on the variable toolbar that the app never sets, and two specs located the toolbar through it — so they passed against hand-written markup rather than the app's own contract.src/styles.csshas no.has-variablesrule and never had a.has-filtersone.Per the owner decision, this re-fixtures to the app's real contract rather than teaching
ui/dashboard.tsto set the class: no app markup or CSS change. The fixture now carries exactly the two classessrc/ui/dashboard.tsbuilds (dash-toolbar dash-toolbar-variables), and both specs assert the real show/hide mechanism — the inlinestyle.display.Closing the gap the issue actually named
#473 warns that "a regression in how the toolbar is actually hidden (or shown) would not be caught". Re-pointing the e2e selectors alone would not have fixed that, because the fixture is hand-written markup — asserting
style.displayon a static<div>proves nothing about the app.src/ui/dashboard.tsbuilds the toolbar withstyle: hasOrdinaryVariables ? undefined : { display: 'none' }. The hidden branch was already covered against a realcreateApprender; the shown branch was not, so inverting that ternary left the whole suite green. This adds the missing unit assertion, so both branches are now falsifiable.Sabotage-verified both ways: flipping the ternary turns the hidden-branch test and the new shown-branch test red; forcing
display: noneonto the fixture turns both re-pointed e2e specs red. Sources restored by writing the saved bytes back, notgit checkout --.Both specs still test their original subjects (a fixed-position combobox popover escaping toolbar-clip bounds, and constant row height while scrolling variables) — the toolbar is a bounding-rect anchor in each, now also carrying the visibility check.
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 — n/a, test-only, no app behaviour change🤖 Generated with Claude Code
https://claude.ai/code/session_01GiubaoqEuBzAyo5C4P8Vqr