A probe theme covering every theming target, and a PR-scoped visual job - #5396
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
requested review from
cvkxx,
ernestt,
kentonquatman and
rubyycheung
August 24, 2026 04:36
…sual job Two gaps the release gate left open. THE PROBE THEME. Real themes style what their designer cared about, so most of the themeable surface was never exercised by any of them — and a newly added target starts life in that unverified set by default. Nothing notices when it stops working, because nothing was styling it. packages/themes/probe is GENERATED from the same target enumeration the CLI uses, so a target added tomorrow is covered the moment its doc lands: 248 targets, 795 selectors, against 25 keys across all seven real themes. Each selector gets a distinct hashed colour, and text/background/border differ, so two sub-targets that collapse onto one element show as one colour instead of two — which a uniform hot-pink theme would hide. Private, never published, and visual:probe-theme:check fails CI when a target is added without covering it. Effect on the gate: declared targets no shot could verify drop 52 -> 8. The remaining 8, and the 49 unexercised real-theme overrides, share one root cause — no story renders them at all. THE PR-SCOPED JOB. pr-visual shoots every story of a touched component in every theme that styles it (deeper than the daily gate, which shoots one representative story), scoped by the same analysis.json pr-a11y uses: a median PR is ~16 shots, ~10s. Over 240 shots it declines and says why — a token change would put hundreds of diffs in front of a reviewer who cannot judge them one by one, and the daily gate reviews that against the whole baseline instead. Non-blocking while it earns trust. A scoped run no longer reports the rest of the baseline as removed, and shots the daily baseline does not reach are reported as unbaselined rather than as additions — both would otherwise appear on every PR.
…ut of the CLI Three things the first cut got wrong, all found by running the build rather than reading it. The package emitted no .d.ts, so Storybook's typecheck failed on the import. It now runs tsc the way every shipped theme does. scripts/check-fully-specified.test.mjs requires every @astryxdesign/theme-* build to run the fully-specified gate, and it is right to: this package emits dist ESM the same way, so it can regress the same way. Worst of the three: generate-cli-themes.mjs discovers themes by scanning packages/themes, so the probe fixture was bundled into the CLI's shipped templates — a 5,000-line generated test fixture offered to users as a selectable theme in `astryx theme add`. The generator now skips private packages, and the bundle drift test grew a guard so it cannot come back. That test's own discovery mirrored the generator's, so it had to learn the same rule.
The review loop's second block: '52 → 8' measured the set-cover tier, not the probe theme — the scout is theme-blind, and buildPlan with probeTheme:'neutral' gives the same number. Worse, nothing anywhere asserted a probe colour ever painted, so a target that had already stopped working would be captured broken and promoted into the baseline as correct, permanently silent. `gate.mjs reach` is the assertion a pixel diff cannot make. Each probe selector has a unique deterministic colour, so 'did this override arrive' is an equality test, not a diff: no baseline, no images, and it names the target instead of a rectangle. paint() is now exported and shared, because two copies of the seed→colour mapping would drift and the check would report the drift as a bug. Three outcomes, not two, because the difference matters: reached — the override painted shadowed — another target on the SAME element won; a fact about the markup missed — nothing probe-coloured won First run over the whole index: 213 reached, 45 shadowed, 50 missed, 8 rendered by no story. Rebased onto v0.5.0 and repinned; the generator picked up main's new targets on its own, 248 → 268, which is the property that makes this worth having.
Contributor
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR No new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
…lds it
<Package>/<Component>/(Default | Theme Sheet | …)
<Package>/Hooks/<hook>
<Package>/Themes/<theme feature>
A reader could not guess where anything lived: six hooks sat beside components
under three different parents, five theme-level features were filed as if they
were components, and one story was under a 'Components' category that exists
nowhere else. Nothing but a reader could notice, which is why it drifted.
The test reads the BUILT index, so it judges what Storybook renders rather than
what the source appears to say. Charts/Chrome/* and Lab/3DChart/* stay as
deliberate sub-grouping.
Titles only — no story moved file, no render changed.
The README now carries the pattern, including the part that is easy to get
backwards: a Theme Sheet must NOT wrap itself in <Theme theme={…}>. Several
existing 'Themed*' stories do, which overrides the global theme, so the toolbar
cannot switch them and the visual gate can never probe them — they are
invisible to exactly the testing they look like they are helping with.
The reach check existed but nothing ran it, so the 50 standing misses would have quietly come back. It is now a leg of the release gate, reported in the job summary with its own artifact. Reported, not enforced. Every one of today's misses traces to a single cause — StyleX emits into @layer priority1-4, which sort AFTER astryx-theme, so wherever a component sets a property the theme override loses. Failing the gate on a known systemic issue teaches everyone to ignore the gate. Enforce it when that is fixed and the count is zero.
cixzhang
added a commit
that referenced
this pull request
Aug 25, 2026
The gate reads each theme's dist/source.mjs — a theme's component map is what defineTheme returns, not a literal in its source — and pr-visual only downloaded the Storybook artifact, so every run since #5396 died on 'Theme butter is not built'. build-storybook already builds them, so it uploads them rather than pr-visual paying for a second full build.
cixzhang
added a commit
that referenced
this pull request
Aug 25, 2026
The gate reads each theme's dist/source.mjs — a theme's component map is what defineTheme returns, not a literal in its source — and pr-visual only downloaded the Storybook artifact, so every run since #5396 died on 'Theme butter is not built'. build-storybook already builds them, so it uploads them rather than pr-visual paying for a second full build.
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.
Both follow-ups from #5372.
The probe theme — "capture new targets when they're added"
Real themes style what their designer cared about, so most of the themeable surface was never exercised by any of them, and a newly added target starts life unverified by default. Nothing notices when it stops working, because nothing was styling it.
packages/themes/probeis generated from the same target enumeration the CLI uses, so a target added tomorrow is covered the moment its doc lands — nobody has to remember:Each selector gets a distinct hashed colour, and text/background/border differ from each other. Two sub-targets that are supposed to be separate elements but actually resolve to the same one show up as a single colour — which a uniform hot-pink theme would hide. Private, never published.
pnpm visual:probe-theme:checkfails CI when a target is added without regenerating.Effect on the gate: declared targets no shot could verify drop 52 → 8. It costs 128 shots, not 496: the probe tier greedily set-covers, so one story rendering twenty targets is one shot.
The PR-scoped job — component regression tests
pr-visualshoots every story of a touched component in every theme that styles it — deeper than the daily gate, which shoots one representative story per component. Scoped by the sameanalysis.jsonpr-a11yalready uses.Over 240 shots it declines and says why. A token or shared-hook change would put hundreds of diffs in front of a reviewer who cannot judge them one by one; the daily gate reviews that against the whole baseline instead. The skip states its reason in the PR comment rather than vanishing. Non-blocking while it earns trust.
Two signals corrected so they don't lie on every PR: a scoped run no longer reports the other 500 baseline shots as
removed, and shots the daily baseline doesn't reach are reported as unbaselined, not as additions.What this surfaced (flagging, not fixing)
69 visual props across 27 components are declared themeable but undocumented —
Stack,Stepper,ButtonGroupand others declarevisualProps: ['direction','gap',…]in their theming targets while documenting no props at all.astryx component Stackshows a theming table namingdirection, but nothing says what values it takes, so a theme author can't use it and the probe theme can't enumerate it. The generator lists every one.The 8 still-uncovered targets and the 49 unexercised real-theme overrides share a single root cause: no story renders them —
Heading type="display-1", a selected TopNav item,Card variant="info", the empty states,hovercard,table-footer. Worth a stories pass; happy to take it.Testing
77 unit tests across the gate (55) and the PR comment formatter (9) plus existing. Verified end to end on the Mac: 642-shot run passes clean, the median-PR path takes 50 shots in ~20s, and the over-budget path declines with
924 shots exceeds the 240-shot budget (5 components touched).