-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5508
jiunshinn · open · view on GitHub
Verdict: approve (drafted — no public action taken)
HEAD REVIEWED: 581fa468ca2707f64fffbb18d08c319fd0513eef — every claim below was
verified at this commit. Parent 41494fcd41310c819f0501df2e798235c4b41135.
Re-checked immediately before writing this record: head unmoved.
Versions. Review Loop 1.4.0 · Component Audit Rubric 1.12.
Bucket: external contributor. Not in .github/ENGOWNERS or
.github/DESIGNOWNERS; repos/facebook/astryx/collaborators/jiunshinn/permission
returns read; head repo is the fork jiunshinn/astryx. They cannot merge
themselves. R1g applies: merge unless there is something we cannot live with.
LANE: full. Mandated for this run: the change alters source-build behaviour and no named visual endpoint story covered it going in. (It would also have been defensible as fast — one decision, ~6 runtime lines, no new surface — which is worth recording as a calibration point.)
Prior review: none. pulls/5508/reviews and pulls/5508/comments are both
empty; the only issue comments are the Vercel bot and the PR Analysis Report.
Nothing to extend or contradict (R1e).
Main did not invalidate this (R31c). main advanced from the merge-base to
90bf6c9, and #5561
(055e75f65f5) did touch Selector.tsx — but the sizeStyles region this PR
rewrites is byte-identical between the PR's parent and current main, linePad
is still on main at :398, and GitHub reports MERGEABLE.
A builder consuming @astryxdesign/core through the documented source-build
path — Theming Infrastructure, Source build (StyleX interop): "Import
components from @astryxdesign/core … StyleX compiler extracts and deduplicates
styles at build time" — whose Babel configuration reaches next/babel gets
next build failing, not tsc. The error is
Unsupported expression: FunctionExpression, thrown from
transformStyleXCreate and pointed at Selector.tsx:398: a file they do not
own, naming an expression kind rather than anything they wrote. They cannot ship
until they find the undocumented workaround (configFile: false).
Reproduced, not taken on trust: reverting only Selector.tsx to the parent and
running the PR's own test produces exactly that error at exactly that line.
VERDICT: clear
Explain it plainly. StyleX works out the final CSS while the code is being built, by reading through to a plain string. A tiny helper function is fine while it stays in its original form — but some consumers' build tools rewrite function syntax into an older form first, and after that rewrite StyleX can no longer read through it, so it stops the build. Writing the three calculations out where they are used removes the only thing that could be rewritten, so the build now survives whatever a consumer's tooling does to function syntax. The arithmetic is untouched, so the CSS it produces is the same as before.
SOLUTION (1 decision · ~6 runtime lines of 78 added)
1. inline the three size-specific paddingBlock calc() strings, deleting the
module-scope `linePad` helper [the fix] — traces
to the stated problem in the body and in #5464
The other 72 added lines are evidence, not decisions: a 65-line regression test
and a 7-line changeset. Nothing is marked [NO STATED PROBLEM]. One decision is
under the >2 smell threshold; no split to name.
Early 7d hypothesis, written before the slots: everything I find will be a child of "is inlining the whole fix, or does the same class survive elsewhere in the package?" — answered below under ARCHITECTURE and IMPACT, and it came back clean, which is the result that mattered.
VERDICT: clear
OWNER: Selector's own style module — `sizeStyles` in Selector.tsx
TIER 1: the StyleX compile pipeline (@stylexjs/babel-plugin). No Astryx
system is involved; nothing shared is rebuilt locally.
TIER 2: the size axis (--size-element-sm/md/lg) — unchanged
SEAMS: the two documented consumption paths — source build (StyleX
interop) and dist build — plus the rendered composition
BEHAVIOR UNIT: inline, correctly. These are three constant strings; there is no
state machine, gesture or timing protocol to extract, and
extraction is precisely what caused the bug.
Seams driven, not read off source:
| seam | driven result |
|---|---|
source build — consumer Babel (next/babel) + StyleX, production |
fails at the parent, passes at the head. Same file, same command |
| dist build — the repo's own Babel → extracted CSS |
byte-identical: 96 rules / 5,785 bytes / sha256 5dc188dd… on both sides |
| rendered composition — Storybook, neutral theme, LTR | byte-identical frames, 0 of 360,000 pixels differing |
Is this THE fix, or A fix (R3c)? The root cause is that StyleX must
constant-evaluate values inside stylex.create, so any module-scope helper
call is evaluable only until a consumer's Babel lowers it. R3d's sharp test —
find the next place the same failure can happen — was run, not reasoned:
- Every
.ts/.tsxinpackages/core,lab,chartsandrichtextthat authors StyleX (252 files) was compiled through the same consumer pipeline. 229 compiled clean, including every single file inpackages/core/src. ZeroUnsupported expressionfailures anywhere. The 23 that failed did so withCould not resolve the path to the imported file— a limitation of my standalone harness's module resolution, all in lab/charts/richtext, none of them this defect. Probe banked at~/astryx/shots-5508/sweep.mjs. - The nearest-looking neighbours are a different shape, which is what a
genuinely scoped fix leaves you looking at:
Step.tsx:618,838takeinlinePadas a parameter of a dynamic style function, which is the sanctioned StyleX API (CLAUDE.mdSTYLEX-CAPS: "DYNAMIC: Functions in stylex.create for runtime values — YES"), evaluated at runtime rather than called at author time.Dialog.tsx:257carries a comment already stating this constraint out loud — "This literal has no logic — StyleX can't analyze a helper, so the values … are computed at the call site and passed in" — so the repo already knew, and Dialog already avoids it.
So this is the fix, not a patch, and it ships no surface: the payment for being wrong later is deleting three template literals.
The mechanism was already ruled on (R14c). #5464
is the maintainer's own issue and prescribes it: "Inline the three calc()
strings at their call sites." It also asked that the --spacing-5 comment be
kept on the inlined version — the PR keeps it (Selector.tsx:395-397).
VERDICT: clear
-
The builder on the documented source-build path whose Babel reaches
next/babel: their build stops failing on a file they cannot edit. That is the whole population this change reaches, and for them it is the difference between shipping and not. - The end user of any app built with Astryx: nothing at all. Not "probably nothing" — the extracted stylesheet is the same bytes and the rendered frames are the same bytes, so there is no pixel for anyone to notice.
-
The next maintainer: the new comment at
Selector.tsx:398-400is what stops someone re-extracting the helper as a tidy-up and re-shipping the bug.
What does landing this newly expose? Nothing. This is the case where that
question usually bites — fix one member of a family and the unfixed siblings
become visibly wrong — so it was answered by measurement rather than assumption:
the sweep above found no second member with this defect in packages/core.
There is no asymmetry for merging to reveal.
VERDICT: clear
No API change. linePad was module-private — git show <parent>:Selector.tsx | grep -c "export const linePad" → 0, and git grep linePad origin/main -- packages/ returns only its four lines inside Selector.tsx. No prop, export,
type, default, signature or accepted value moves. No usage snippet: there is no
call site to write.
Ossification is still owed for what the diff adds internally, which is where
the real question lives. It adds one thing: Selector.source-build.test.mjs, a
new kind of test for this repo — the first test anywhere in packages/ that
compiles package source through a consumer's Babel preset (git grep -l next/babel returns app configs, packages/build/src/config.js,
scripts/package-source.js, and this file). It is not public surface and
nothing can import it, so it freezes nothing; but it is a pattern the next
person will copy, and the note under PERFORMANCE is about its one soft spot.
VERDICT: clear
No new theme targets, and no existing one moves. The three tokens the helper read are still read by the inlined strings — confirmed in the extracted CSS text rather than inferred from the diff:
padding-block:calc((var(--size-element-sm) - var(--spacing-5) - 2 * var(--border-width)) / 2)
padding-block:calc((var(--size-element-md) - var(--spacing-5) - 2 * var(--border-width)) / 2)
padding-block:calc((var(--size-element-lg) - var(--spacing-5) - 2 * var(--border-width)) / 2)
Rubric §2: T1/T3 no raw colour, spacing, radius or shadow is introduced — the
one hit from
grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(" over the
added lines is the string #5464 in the test's describe title, a false
positive, not a hex colour. T2 no themeable surface is removed: a theme
overriding --size-element-*, --spacing-5 or --border-width still reaches
exactly the same calc(). T4 no new astryx-* class. No new bound is
introduced, so R14f's far-side test has nothing to drive past.
VERDICT: clear
-
API — no. Nothing exported changed;
linePadwas private. -
Visual — no. Established three independent ways, not asserted: identical
extracted CSS (same 96 rules, same 5,785 bytes, same sha256), identical
computed geometry (28/32/36px trigger heights, 3/5/7px
padding-blockin both arms, matching the token arithmetic), and byte-identical PNGs. -
Theme — no. Same three custom properties, same
calc(), same cascade position.
Behaviour walk. The diff adds no conditional, no default, no early return and no state setter, and moves no value a component reads at runtime — it changes how three constant strings are written, and the compiler's output proves the constants are the same. empty / loading / error / disabled / controlled-vs-uncontrolled / boundary: not reachable — the diff introduces no state, no default and no prop to move through them.
Nothing grew. The outer box is the same in both arms (250×28/32/36), so R15d's reflow question does not arise.
VERDICT: clear
- Effects — zero. None added, moved or removed; the diff contains no React code at all.
- Render — nothing. The change is entirely compile-time. The emitted class names and the emitted CSS are identical, so the runtime does the same work.
-
Value ownership / listeners / observers / layout — nothing. No DOM write,
no
document/windowlistener, no observer, no forced reflow. - Bundle. The emitted CSS is byte-identical. The module loses one module-scope arrow and gains three inline template literals; I did not measure the JS delta and am not claiming one.
- Perf test — not applicable. No O(N) work on any flow.
-
One real, permanent cost, stated as a number rather than an adjective: the
new test adds a full Babel + StyleX compile of
Selector.tsxto theuisuite — 262 ms measured on this machine in isolation (vitest run packages/core/src/Selector/Selector.source-build.test.mjs --project ui). Small, real, and paid on every CI run forever. It buys a guard that demonstrably fires, so it is worth it; it is recorded because "cheap" is a measurement, not an adjective (R18g).
VERDICT: clear
VISUAL CHECK: manual frames required
WHY: the diff rewrites three `paddingBlock` declarations inside
`stylex.create` — style-authoring work on a rendering path — and both the
PR and this review claim unchanged pixels. An unchanged-pixels claim on
CSS work requires manual frames. `not applicable` is unavailable because
the changed path renders. `visual gate sufficient` is unavailable too:
`pr-visual` was green on the exact head (`581fa468`), but the gate's green
is not evidence FOR an unchanged-pixels claim, and this is not a
behaviour-only change.
Arms. One worktree, one install, one browser, one storybook dev (port
6508, PID 38902, killed by PID at the end — never by name). after is the clean
head tree. before is the same tree with only Selector.tsx reverted to the
PR's parent — the harness's own prescription for a baseline, rather than a
different checkout.
Both arms have the same git HEAD, so the Build sensor cannot separate them. That gap is closed two ways rather than waved at:
-
before.diffis banked beside the receipts and was read: it contains exactly one hunk —linePadrestored and the new comment removed — andgit status --porcelainshowed that one file modified and nothing else. - An arm-identity sensor added to the probe fetches the module the dev
server is actually serving (
/@fs<abs path>) and assertslinePadpresent forbeforeand absent forafter. Recorded in each geometry receipt asservedHasLinePad: true/false. This is what proves the frame shows the source it claims, which a git SHA cannot do for a working-tree arm.
Screenshot sensor receipt — written beside each PNG by captureWithSensors();
all sensors passed before either image was written, and the nine comparable
sensors are identical across arms (compared programmatically, 0 mismatches):
| sensor | before | after |
|---|---|---|
| build (worktree HEAD) | 581fa468 |
581fa468 |
arm identity (servedHasLinePad) |
true | false |
| story | core-selector--size-variants |
core-selector--size-variants |
| globals | astryxTheme:neutral; colorMode:light; direction:ltr |
same |
| theme attr / colour mode |
neutral / light
|
neutral / light
|
| computed direction | ltr |
ltr |
| viewport / DPR | 900×400 @1 |
900×400 @1 |
| media (forced-colors / reduced-motion / coarse / hover) | false / false / false / true |
same |
target count (.astryx-selector) |
3 |
3 |
| semantic state | 3 × role=combobox, names Small/Medium/Large, aria-expanded=false
|
same |
| subject geometry |
250×28 @ (325,124), inside viewport |
same |
| fonts / running animations |
loaded / 0
|
loaded / 0
|
| page + Storybook errors | none | none |
Two expectations failed on the first attempt and were investigated, not
loosened. expected.state was written from the contract before the page was
opened, and asserted role="combobox" on .astryx-selector; the page returned
div. Reading the source settled it: .astryx-selector is the trigger
container (Selector.tsx:90) — which is exactly the element sizeStyles[size]
is applied to, so it is the correct visual subject — while role="combobox"
lives on the inner trigger button (Selector.tsx:1612). The probe's state reader
was corrected to read the button. It then returned name: null, because Field
associates the label natively as <label htmlFor> (Field/FieldLabel.tsx:103)
rather than through aria-labelledby; the resolver was extended to follow that
association. Both were defects in my reader, not in the expectation, and no
observed value was copied back into expected.
Frames.
| what it shows | frames | path |
|---|---|---|
| Selector sm/md/lg — the only story rendering all three changed values | before/after |
~/astryx/shots-5508/before__size-variants.png · ~/astryx/shots-5508/after__size-variants.png
|
| pixel diff of the pair | after only | ~/astryx/shots-5508/diff__size-variants.png |
Both frames were opened with the read tool and looked at. They show three outlined selector fields labelled Small / Medium / Large, each with its placeholder and a chevron, text vertically centred, heights visibly stepping 28 → 32 → 36px, nothing clipped. They are indistinguishable, which is what the numbers say too: the two PNGs are byte-identical files, 0 of 360,000 pixels differing at threshold 0.
Every visual difference labelled: there are none. The invisible-by-design claim is the strong one here, so it carries what R15e asks of it — what was compared, not merely that something was looked at: extracted stylesheet identity, computed geometry against the token arithmetic, and pixels.
Render matrix — rows dropped with the reason, not silently omitted. RTL, 200% zoom, forced colors, icon-only, hug/fill, 320px and the container cases are not owed here, and the reason is stronger than running them would be: the extracted stylesheet is the same bytes and the component emits the same class names, so the two builds are the same stylesheet applied to the same DOM. No condition can separate them, because there is nothing to separate. Had the CSS differed by one byte, every one of those rows would be owed.
VERDICT: clear
Structural with respect to both — established with the command, once:
grep -nE "aria-|role=|useTranslator|t\('@astryx" <added lines> → 0
grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(" <added lines> → 1 (the string "#5464")
No role, no ARIA attribute, no user-facing or AT-facing string, no new locale
key, no directional glyph, no physical property. §1's browser half (A1, A4, A5,
A8/A9, A13–A15) and §9's direction half (I8–I12) cannot be reached by a diff
that changes three constant style values into three identical constant style
values — and the frames confirm the accessible structure is unchanged: the state
sensor read the same three role="combobox" controls with the same accessible
names and the same aria-expanded in both arms. pr-a11y and pr-rtl both ran
green on this exact head; I did not re-run axe locally, per the harness.
VERDICT: clear
low-risk — all four hold: no new API surface and no new theme target; no behaviour regression (nothing runtime changed, proved by identical output); no performance regression at runtime (the only cost is 262 ms of CI, recorded above); nothing grew (identical outer box).
PROBLEM clear
SOLUTION clear — 1 decision, traces to the stated problem
ARCHITECTURE clear — driven at all three seams; R3d sweep found no second instance
IMPACT clear
API clear — no API change
THEMING clear — no new targets, same three tokens in the emitted calc()
BREAKING clear — API no · Visual no · Theme no
PERFORMANCE clear — zero effects; +262 ms of CI, stated as a number
VISUAL clear — manual frames required, captured, receipted, byte-identical
A11Y & I18N clear — structural, with the grep
Read against the slot bodies, not the labels (R34d): the only sentence anywhere in the slots describing something a person hits that this change causes is the phantom-dependency note under API/PERFORMANCE, and its consequence is a CI test failure with a legible cause — nothing a user or builder is stuck behind. No slot's prose is worse than its line.
JUDGEMENT NEEDED: none — defect fix against the existing contract, and the mechanism was prescribed by the maintainer in #5464 rather than chosen here.
GOAL: met — the PR's stated goal is that Selector's raw source survives a
consumer Babel pipeline. Measured both ways with the same command: the parent
fails with Unsupported expression: FunctionExpression at Selector.tsx:398,
thrown from transformStyleXCreate; the head passes. Not "the tests are green" —
the guard was proved to fire.
DISPOSITION
1. [not blocking] `next` is a phantom dependency of packages/core
→ the next person who bumps or removes `next` in apps/docsite gets a failing
Selector test in a package that does not declare it, with nothing
connecting the two; and the guard silently runs against next
16.3.0-preview.5 rather than the next 15 the reporter and the example apps
use · packages/core/src/Selector/Selector.source-build.test.mjs:41
→ accepted. Who pays: a future contributor changing docsite's dependencies.
Why acceptable: the workspace sanctions this deliberately —
pnpm-workspace.yaml sets `nodeLinker: hoisted` / `shamefullyHoist: true`
and says so in its own comment, "Keeps the flat, Yarn-1-style node_modules
layout that build tools (and some phantom test imports) rely on." Who
decided: that convention, cited, not this reviewer. Surfaced to the author
as an optional inline so it is not swallowed silently.
No orphan findings: that is the only negative finding in the review.
ADVICE: omitted — deliberately. I can state the fact (undeclared next,
resolved from a hoisted preview build) but I cannot cite a landed sibling for
either remedy, and declaring next as a devDependency of a design-system
package is a real cost I have not priced. Under R2l an uncited, unbuilt fix is
authorship, not review, so the inline names the problem and stops.
AUTHOR CAN PROCEED: yes — nothing is required of them. The single note is optional, needs no decision from us, and does not gate the merge.
WORST OUTCOME: the strongest user-impact sentence in the slots is
"next build failing, not tsc … a file they do not own, naming an expression
kind rather than anything they wrote" — and that describes the state this PR
removes, not one it creates. For harm this change introduces: none found.
Compatible with approve.
Verdict: approve. Every slot is clear; no slot said BLOCKS, and R16e is
explicit that a review with no blocking finding cannot be request-changes. R1g's
three exceptions were each checked and none applies: no new permanent surface,
no changed meaning on existing surface, and the mechanism is not wrong — it is
the maintainer's own, independently verified.
Thanks — checked the new test actually fails without the fix, and the extracted stylesheet is unchanged byte for byte. Merging as is; one optional nit inline.
[Reviewed by Robohands]
Inline — packages/core/src/Selector/Selector.source-build.test.mjs:41
( presets: ['next/babel'],):
nextisn't a core dep — this resolves off docsite's hoisted16.3.0-preview.5.
26 words in the summary (cap 30 for approve-with-nits), 11 in the inline (cap
15). No Discord clause: R12e puts it on request-changes, declines and
maintainer-routing, never an approve. No [Full review] link: that is for
authors in ENGOWNERS/DESIGNOWNERS only, and this record exists regardless
under R17.
Not posted. This review was drafted read-only; no public action was taken on the
PR — no comment, no approval, no request-changes, no label change, no push to
facebook/astryx.
Pass 1 — one FAIL. The draft summary read "the extracted CSS is
byte-identical", which sits close enough to a claim about appearance to trip
R15-gate, and the frames could not be published to an assets/pr-5508 branch
because this run was permitted exactly one remote mutation (this wiki push).
R15-gate allows only two fixes and never a third: attach the frame, or delete
the sentence. Rewritten to "the extracted stylesheet is unchanged byte for
byte", which is unambiguously a statement about build output, with the frames
carried in this record for whoever posts. Everything else passed: verdict
correct for the bucket, both caps met, one anchor and it was opened and its text
pasted, no rubric ids or severity headers in the public text, no restatement of
the author's own solution, closing line matches the verdict.
Pass 2 on the rewrite — PASS. Length 26/11. No visual sentence without a
frame. R7c2 run: the diff was grepped for the thing the nit is about and the
author had not already declared next. R34a: the finding is anchored at the
line that causes it (presets: ['next/babel']), not at a line describing it.
R34b: severity is the worst outcome it enables — a legible CI failure — which is
a note. R2m: the comment does not say "nothing needed" while reporting a defect;
nothing blocks and the nit is marked optional. R16g: no blocking finding needing
a second confirmation, and the one positive claim that carries the verdict was
confirmed two independent ways (the failing run's error text matched the reported
error, and the arm-identity sensor plus before.diff proved which source each
arm ran).
# the guard fires (fails at the parent, passes at the head)
cd ~/astryx/worktrees/<wt> && git checkout 41494fc -- packages/core/src/Selector/Selector.tsx
pnpm exec vitest run packages/core/src/Selector/Selector.source-build.test.mjs --project ui
git checkout HEAD -- packages/core/src/Selector/Selector.tsx
# extracted-CSS identity across the two arms
node ~/astryx/shots-5508/css-identity.mjs /tmp/out.txt
# R3d class sweep: the consumer pipeline over every stylex-authoring file
node ~/astryx/shots-5508/sweep.mjs
# frames + sensor receipts (reusable, banked in probe-kit)
ARM=after WT=<worktree> SHA=581fa468 SB_PORT=6508 OUT=~/astryx/shots-5508 \
node ~/astryx/probe-kit/selector-sizevariants-frames.cjs
New reusable probe banked: ~/astryx/probe-kit/selector-sizevariants-frames.cjs.
TIME total 27m
setup 5m worktree at the exact head + clone-install (11s) + one
@astryxdesign/build build + storybook dev (warm main reused: no —
see waste)
reading 7m loop version, brief, critic, loop mechanics, harness,
evidence templates, repo rubric from main, fork wiki, issue #5464
measuring 9m vitest both arms, extracted-CSS identity, 252-file sweep,
2 frames + receipts, pixel diff. 3 re-measures: two forced by
my own state-sensor expectations being wrong about which
element carries role=combobox and how Field wires the label,
one to capture the failure's full error text after the first
run's tail cut it off
writing 6m draft + two critic passes + this record
waste 2m wrote the first sweep probe into /tmp, where @babel/core does
not resolve, and had to move it inside the worktree; and ran
the parent-arm vitest twice because the first invocation
tailed the output and cut off the error message that was the
whole point of the run
Warm main was not reused, deliberately and correctly: main has advanced
past this PR's merge-base and includes #5561,
which touches Selector.tsx. Using it as the baseline would have folded another
PR's changes into the comparison. The right BEFORE was this PR's parent, taken
in the same worktree.
-
The author's specific figure of 5,395 bytes /
sha256 79d42c67…. I proved the identity they claim — the extracted CSS is the same on both sides — but under my own serialisation it is 5,785 bytes /sha256 5dc188dd…. Different serialisation, same conclusion; I am not endorsing their number, only the claim it was offered for. -
SAMPLE_PR_COMMENT.mddoes not exist onmain(git show origin/main:SAMPLE_PR_COMMENT.md→ path does not exist), so it could not be read as part of the repo rubric. -
23 of 252 stylex-authoring files (all in
lab,charts,richtext) could not be compiled by my standalone harness — it failed to resolve their imports, which is a limitation of running Babel outside the build, not a finding. Every file inpackages/core/srccompiled. -
Vercel is red on this PR, and it is not this PR's. Checked rather than
assumed (R14h): it is
FAILUREon all four of jiunshinn's PRs sampled, including #5507 and #5509 which both merged anyway, andSUCCESSon four non-fork PRs sampled (#5563, #5561, #5553, #5546). It is a fork-preview artifact. I did not open the Vercel log to confirm the cause. -
review-requiredisPENDING, which is expected, not a defect:.github/REVIEW_GATE.mdrequires code review on all contributor PRs. - No second browser engine. This Mac runs Chromium only; nothing in this change depends on engine behaviour.