-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5570
cixzhang · merged · view on GitHub
Verdict: approve with three nits (posted review) · squash-merged as 6efc31b
#5570 Storybook: add the missing Probe Theme all-axes page by cixzhang (bucket: the maintainer)
9c7477e065cbe4f96a77ce11bf8d25ac1c8f7fe4 — the corrected head, after fix round 1.
First head reviewed: 67fe85b630aaa2d9ab3377c98518734211de8775. Current main at the final round was 824a181d, six commits ahead of the merge-base with no conflict; #5573, which landed mid-review and edits the visual-gate config, was diffed and does not touch the tiers, viewport or probe-theme settings this review cites.
LOOP VERSION: 1.4.1 AUDIT RUBRIC: 1.12
The first two legs ran under 1.4.0 and correctly held it through their critic pass; the corrected-head round was a fresh rerun and took the then-current 1.4.1.
LANE: full WHY: a new rendered surface with an intended visual change, and the exact-head visual gate is skipped rather than green, so the visual obligation could not be discharged by CI.
A theme author who wants to see what defineTheme's axes actually do has to hunt across four unrelated story files, and there is nowhere at all to see the probe fixture — the thing the repo uses to prove a theme applied.
The body overstates it on two of six axes: Theme.stories.tsx already ships a Token Inspector story rendering token swatches with resolved values, and CodeTheme.stories.tsx exports 16 stories including one built on defineTheme({syntax: dracula}). The genuine gap is icons, indicators, fonts, and one-page probe coverage.
VERDICT: note — the stated gap is false for tokens and syntax; the remaining gap is real.
Storybook's Theme toolbar already decides which theme the page renders under. This page shows one specimen of each thing a theme can change and deliberately does not pin a theme of its own, so flipping the toolbar re-renders everything under the new theme. The probe theme swaps in loud marked stand-ins, so both a person and a script can tell whether the theme really arrived.
SOLUTION (2 decisions · 317 lines, ~250 specimen markup, 0 runtime lines shipped to consumers)
- Add
Core/Themes/Probe Theme— All Axes, Component Targets, Registries. - Do not wrap in
<Theme>; the toolbar owns the active theme. [the load-bearing one]
Five AxisCards carry six axes — one card holds both registries. The fix round added no third decision; it corrected a string inside decision 1.
VERDICT: clear — the fix is minimal and correctly scoped.
OWNER: apps/storybook — a story file; owns no runtime concept.
TIER 1: the theme system (defineTheme, useTheme, the icon and indicator registries) — reused, not reimplemented.
TIER 2: none.
SEAMS: the Storybook toolbar global; the visual gate's probe tier.
BEHAVIOR UNIT: inline — three render functions, no state machine.
| seam | driven result |
|---|---|
| toolbar global, neutral | works — theme attribute neutral, 0 icon / 0 indicator markers |
| toolbar global, probe | works — theme attribute probe, 9 icon / 3 indicator markers |
probe tier story selection |
reachable — that tier scouts every story, so a page that pinned its own theme would be shot under a theme it forced |
| per-PR visual gate | never reaches it — the job gates on changed components, and a themes page is not a component |
The no-<Theme> decision is correct and load-bearing, and it was verified by driving the toolbar rather than by reading the file's docblock.
VERDICT: clear.
Builders only, and only those who go looking. A theme author opening the page gets, on one screen, what a theme changes, and can flip the toolbar and watch it change. Nobody consuming Astryx sees anything; no component, package or API moves.
Landing it makes the page the most visible on-screen statement of the indicator contract in the repo. After the fix it is accurate about what is an indicator; it remains silent about check, the third registry member.
VERDICT: clear.
No prop, export, accepted value, type, default or theme target changes. A story file is not public surface and the Storybook app is not published, so nothing ossifies. The three new story ids enter the visual-gate baseline on the next scheduled run — the normal cost of any new story, reversible by deleting the file.
VERDICT: clear — no API change.
No new theme targets; the file consumes tokens and defines none. Every custom property it references was confirmed in the browser to resolve under the probe theme, rather than checked by eye — a typo'd var() fails silently. Raw layout numbers on a specimen page fall under the token-integrity exemption list.
VERDICT: clear.
BEHAVIOR: no. Three pieces of local state with sane initials, no async, no effects; the fix touched only a label string. API: no. Nothing existing is touched, and no consumer exists — the Storybook app is private and unpublished. VISUAL: no, for anything that already existed. Within the PR's own new page the fix moves 505 pixels of text, which is the intended change. THEME: no. No target, token or override changes; existing themes resolve identically.
VERDICT: clear.
EFFECTS: zero — no useEffect, no useLayoutEffect, no listener, observer, timer or subscription anywhere in the diff.
RENDER: three useStates set only from user input; one useTheme() per specimen, memoised on theme and effective mode.
BUNDLE: no dependency added; the PR Analysis Report reports no component packages changed.
VERDICT: clear — zero effects, no measurable cost.
VISUAL CHECK: manual frames required WHY: the deliverable is a rendered page, its acceptance evidence is screenshots, the exact-head visual job is skipped rather than green, and the author's own frames are the claim under review.
Exact-head CI, read as counts rather than colours: 19 check-runs / 6 workflow runs — 15 success, 4 skipped, 0 running. The four skips (a11y, RTL, visual, and the review-clear anchor) are all by the "no components changed" condition, not an unapproved workflow. 19 is in the healthy band.
Both theme arms were captured through the sensor harness, which asserts every sensor before writing the image and emits a receipt beside it. Expectations were authored from the contract before the run and were not loosened.
| what it proves | arms | sensor receipt |
|---|---|---|
| all six axes under Neutral and Probe at 1280×980 | before/after pair | matching receipts on build, story, globals, theme, mode, direction, viewport/DPR, media, target count, geometry, settled render, errors, and semantic state |
| the fix is confined to one label | old-head vs corrected-head render | 505 changed pixels of 1,445,120 (0.03%) |
| the body's screenshots predate the fix | published asset vs corrected head | published bytes match the first head exactly; label region cropped from both and inspected |
One sensor failure occurred and is recorded rather than smoothed over: the first capture's semantic-state read used a CSS-class query that matched the checkbox label twice through nested elements and never reached the radio group's label. The expectation was not loosened — the probe was rewritten to read accessible names by ARIA role, after which both arms passed first try with every count unchanged.
All six axes verified measurably distinct at the corrected head:
| axis | neutral | probe |
|---|---|---|
| components | button background rgb(38, 38, 38)
|
rgb(245, 61, 169) |
| tokens | accent light-dark(#262626, #ebebeb)
|
rgb(255, 0, 128) |
| icons | 0 markers | 9 markers |
| indicators | 0 markers | 3 — checkbox checked, radio checked, radio unchecked |
| fonts | Figtree, -apple-system, … |
AstryxProbeFace, monospace |
| syntax | one colour family | three distinct probe colours |
Every difference is intentional and sourced — the recolouring and marker swaps to the body's "unmistakably synthetic" sentence and the probe fixture's own token and registry definitions, the height growth to the probe's font-size and spacing overrides, and the label change to the fix commit. Nothing unintentional was found in the render.
VERDICT: clear — receipts on both arms, six axes verified, the one intended difference isolated to 505 pixels.
The automated axe job is skipped because no component changed, and the baseline file is untouched. Accessible names were checked in a real browser at the corrected head: exactly one checkbox ("Checkbox indicator"), one switch ("Switch control"), one radiogroup ("Radio indicator"), two radios ("First", "Second"); every icon passes a label. Strings are fixture labels on a Storybook specimen, so the translation catalog is correctly untouched and no sibling story localises. The diff writes no physical CSS property, so it is direction-safe by construction. Forced-colors rendering of the fixture was not looked at, and is named rather than implied.
The fix slightly improves this slot: the switch's accessible name now describes what it is.
VERDICT: clear.
| slot | verdict |
|---|---|
| PROBLEM | note — body overstates the gap on two axes |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear — no API change |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear — zero effects |
| VISUAL | clear — receipts both arms, six axes verified |
| A11Y & I18N | clear |
| BODY RECORD | note — the PR's screenshots contradict its code |
The last row is a cross-slot finding no single slot owned, and it was created by the fix: the visual slot verified the page renders correctly, and the problem slot read the body's prose rather than its images. Only comparing the published asset bytes against the corrected head showed the evidence section displaying a label the code no longer contained.
GOAL: met — driven under sensor receipt: the toolbar flips the theme attribute both ways, 9 icon and 3 indicator markers appear under Probe against zero under Neutral, all six axes render distinct values, and every referenced token resolves. DISPOSITION: stale body screenshots → non-blocking nit, raised; gap claim → non-blocking nit, raised; pixel-diff arithmetic → non-blocking nit, raised with its reproduction. None touches shipped behaviour and none, alone or together, justifies holding the PR. ADVICE: omitted, deliberately — the two record nits are text edits needing no direction, and the screenshot remedy is obvious from the finding. AUTHOR CAN PROCEED: yes — nothing blocks. If the body is refreshed, the criterion is that the published probe image renders "Switch control". WORST OUTCOME: "Someone reading the PR later trusts the screenshot over the code and believes the page still labels the switch an indicator." → approve with nits. That names a reader briefly misled about a label, not a user or builder unable to do something; nothing is unreachable, undismissable, unannounced, lost or corrupted, and the code itself is correct.
JUDGEMENT NEEDED: none — no new API, no new concept, no design choice.
The first head labelled a Switch as an indicator inside the card titled "Icon and indicator registries". The indicator contract is exactly three members — check, radio, checkbox — and a search of core returns no indicator usage under Switch, so the switch renders no marker at all. The page was the most visible on-screen statement of that contract, which is what made a label worth raising.
The author pushed one commit relabelling it "Switch control". That fix was verified at the token level rather than read: normalising all whitespace out of both revisions and diffing the token streams shows exactly one token changed in the whole file, indicator" → control". The remainder of the hunk is the formatter collapsing a four-line element now that its attributes fit on one line, and lint is green on that shape.
Two findings from the first round were withdrawn during the loop, and both withdrawals are worth recording:
- Severity. The first draft blocked on all three findings. Its own impact, API and breaking slots said nothing reaches any consumer, which contradicts a block; three record nits do not sum to one.
-
The prescription. The first draft advised swapping the switch for a
Selector, to surface the third registry member. Driving that in a real browser refuted it: a Selector emits one selection mark per option, not one per component, and while closed all of them are present in the DOM but none is visible. On a static reference page whose value is that you can look at it, a Selector contributes nothing a reader can see, and no fixed marker count belongs in an acceptance criterion. The advice was withdrawn rather than repaired — a correct finding with no prescription beats one with wrong advice.
Considered and deliberately not raised: the switch still sits inside the registries card while belonging to neither registry, and check is still absent from the page. Neither survived the test of writing a consequence line — "Switch control" now reads as a deliberate contrast against the two labelled "indicator", and the PR never claimed exhaustive registry coverage.
Thanks — relabel's right, six axes still check out.
Body screenshots are pre-fix ("Switch indicator"); Token Inspector and CodeTheme already cover tokens and syntax; 91.37% should be ~96%.
Refresh and merge?
[Reviewed by Robohands]
None. The one line-anchored finding was fixed in the fix round, and the three remaining nits are about the PR body, which has no line anchors.
- The ninth icon marker is the code block's copy control, proved by subtraction rather than assumption: the Registries story alone renders eight, the All Axes page nine. The count rots silently if that toolbar changes.
- The page does not close a coverage gap in the visual gate — the indicators axis needs only a story rendering a checkbox, radio or selected option, and existing checkbox stories already supply one. The PR never claims otherwise, so this is context rather than a correction.
- The relabel moved 505 pixels, small enough that a pixel-threshold gate would pass it silently. The marker assertion, not the diff, is what makes these axes checkable.
- A class-based label query folds in a duplicate measurement copy and misses a field group's label; reading accessible names by role is the only reliable approach on this page.
TIME total 35m across three legs (initial · rewrite · corrected head) setup 8m two worktrees, cloned installs, Storybook dev on scratch ports (warm main reused: yes, throughout) reading 12m kit in full, critic verdict in full, the theme contract, the probe fixture, the gate, and the Selector render path measuring 11m 13 Chromium runs including 4 sensored captures, 3 pixel comparisons, 2 crops; 3 re-measures writing 4m three presentations waste 3m a probe died on a network-idle wait the harness explicitly warns about; a typecheck ran against an unbuilt workspace; a summary shipped at 48 words against a 30-word cap and a merge decision held on a check a re-poll would have cleared
Of the three re-measures, one was forced by a critic finding and one by a sensor catching a bad probe — both the loop working as intended.
- Forced-colors rendering of the probe fixture — recorded as not looked at rather than implied clear.
- Whether the scheduled probe-tier gate absorbs the three new story ids without baseline churn; it runs on a schedule, not on this PR.
Posted as drafted, verbatim. No full-review link — the brief reserves that for teammates in the owners files, and this is the maintainer's own PR. No Discord clause, for the same reason.
The review reached its final form through two critic passes. The first corrected an over-graded verdict (request changes → approve with nits), an unsupported prescription, missing screenshot receipts, undisclosed image provenance, and two miscounts. The second cut the summary from 48 words to the 30-word cap, refreshed the CI reading once the last check completed, and confirmed the mid-review landing of another PR did not invalidate this one.