Skip to content

Review 5508

Cindy Zhang edited this page Aug 26, 2026 · 2 revisions

#5508 — fix(Selector): preserve StyleX evaluation in source builds

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.


Problem

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

Solution

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

Architecture

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/.tsx in packages/core, lab, charts and richtext that authors StyleX (252 files) was compiled through the same consumer pipeline. 229 compiled clean, including every single file in packages/core/src. Zero Unsupported expression failures anywhere. The 23 that failed did so with Could 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 {review-root}/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,838 take inlinePad as a parameter of a dynamic style function, which is the sanctioned StyleX API (CLAUDE.md STYLEX-CAPS: "DYNAMIC: Functions in stylex.create for runtime values — YES"), evaluated at runtime rather than called at author time. Dialog.tsx:257 carries 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

Impact

  • 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-400 is 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

API

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

Theme targets

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

Breaking

  • API — no. Nothing exported changed; linePad was 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-block in 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

Performance & resources

  • 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/window listener, 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.tsx to the ui suite — 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 evidence

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:

  1. before.diff is banked beside the receipts and was read: it contains exactly one hunk — linePad restored and the new comment removed — and git status --porcelain showed that one file modified and nothing else.
  2. An arm-identity sensor added to the probe fetches the module the dev server is actually serving (/@fs<abs path>) and asserts linePad present for before and absent for after. Recorded in each geometry receipt as servedHasLinePad: 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 {review-root}/shots-5508/before__size-variants.png · {review-root}/shots-5508/after__size-variants.png
pixel diff of the pair after only {review-root}/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

A11y & i18n

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

Risk class

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).

Judgement

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.

The review, as drafted

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'],):

next isn't a core dep — this resolves off docsite's hoisted 16.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.

What changed before posting

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.

Critic passes

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).

Reproductions

# the guard fires (fails at the parent, passes at the head)
cd {review-root}/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 <review-assets>/css-identity.mjs <output>

# R3d class sweep: the consumer pipeline over every stylex-authoring file
node {review-root}/shots-5508/sweep.mjs

# frames + sensor receipts (reusable, banked in probe-kit)
ARM=after WT=<worktree> SHA=581fa468 SB_PORT=6508 OUT={review-root}/shots-5508 \
  node {review-root}/probe-kit/selector-sizevariants-frames.cjs

New reusable probe banked: {review-root}/probe-kit/selector-sizevariants-frames.cjs.

Time

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.

What I could not verify

  • 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.md does not exist on main (git show origin/main:SAMPLE_PR_COMMENT.mdpath 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 in packages/core/src compiled.
  • Vercel is red on this PR, and it is not this PR's. Checked rather than assumed (R14h): it is FAILURE on all four of jiunshinn's PRs sampled, including #5507 and #5509 which both merged anyway, and SUCCESS on 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-required is PENDING, which is expected, not a defect: .github/REVIEW_GATE.md requires code review on all contributor PRs.
  • No second browser engine. This Mac runs Chromium only; nothing in this change depends on engine behaviour.

Clone this wiki locally