Skip to content

Review 5484

Cindy Zhang edited this page Aug 27, 2026 · 4 revisions

Review #5484 — Spinner flex shrink

Versions: Review Loop unversioned (pre-1.0) · Component Audit Rubric 1.10
Head: 4305365f · Verdict: request changes

Problem. A narrow flex host may compress Spinner's box while its SVG ring keeps the requested geometry.

Solution. flexShrink: 0 keeps the intrinsic visual box intact. It belongs below the theming PR so the independent behavior fix can land first.

Evidence. The fix works across all sizes, flex axes, RTL, zoom, and forced colors. The remaining public ask is a browser regression fixture because the unit suite cannot observe layout.

Review as posted:

Thanks — flexShrink: 0 is the right invariant for Spinner's own box.

The browser-only bug still has no shipped regression: the unit suite cannot observe layout, and the visual gate has no narrow-host story. Could you add that story so the browser check owns the red→green behavior?

[Reviewed by Robohands]


Round 3 — delta re-review at 9a197cc6

Versions: Review Loop 1.6.0 · Component Audit Rubric 1.13
Head: 9a197cc6608198a2c0c747f64033150e51247c76 · compared from the previously approved head 783feafca2872caaf7e7e07bc5f6034ac29d38ee · Verdict: approve (proposed; read-only re-review, nothing was posted, approved or merged on the PR)

The record above is Round 1 (4305365f, request-changes). Round 2 — an approve at 783feafc on 2026-08-27 — was posted but never written up here; its text is quoted in the reconciliation table below rather than back-dated into its own section.

PR

#5484 fix(Spinner): stop a narrow flex host compressing the spinner's box — by freddymeta (bucket: collaborator — not in .github/ENGOWNERS or .github/DESIGNOWNERS, but gh api …/collaborators/freddymeta/permissionwrite; treated as internal: they can merge and follow up, so no Discord clause and no full-review link)

HEAD REVIEWED

9a197cc6608198a2c0c747f64033150e51247c76 <- every claim below was verified at this commit Delta re-reviewed from the previously approved head 783feafca2872caaf7e7e07bc5f6034ac29d38ee.

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: fast is refused on the first condition — main moved under the PR's own file. The delta contains #5214, which rewrote Spinner.tsx to make the ring geometry themeable, so the PR's central claim ("the painted circle is inscribed in the box, so hiding and showing the overflow render byte-identical pixels") was measured against a component that no longer exists. That needs more than one decisive check: the red→green, the byte-identity sweep across the three themed stories #5214 added, and the merge/story-fix repair itself.

PRIOR REVIEW — reconciliation (R1e / R31b)

Both prior reviews are ours (cixzhang), and there is no standing block.

round head state what it said at 9a197cc6
1 4305365f CHANGES_REQUESTED "the visual gate has no narrow-host story. Could you add that story so the browser check owns the red→green behavior?" satisfiedCore/Spinner → NarrowFlexHost is indexed and renders three hosts; driven below
2 783feafc APPROVED "the narrow-host story closes my prior ask, and the root-cause correction holds. This is ready to approve." extended, not contradicted — the root cause still holds; the approve was auto-dismissed by the two new commits (reviewDecision is now empty, mergeStateStatus: BLOCKED)

I am extending round 2, not reversing it. The one thing round 2 could not have checked is the interaction with #5214, which landed on main after 783feafc.

What moved since the approved headgit log 783feafc..9a197cc6: 17 landed main commits pulled in by dab42073 (a cixzhang merge), then 9a197cc6 (a cixzhang story fix). Of those, exactly one touches this component: a5f4b039 = #5214 feat(Spinner): make the ring geometry and colors themeable.

The merge damage, and its repair. dab42073 left apps/storybook/stories/Spinner.stories.tsx unparseable: the ThemedHairlineStroke story's render body lost its closing </VStack> ), };, so NarrowFlexHost was nested inside it. 9a197cc6 restores those three lines and re-indents one comment in Spinner.tsx. Verified mechanically rather than by reading: git diff origin/main 9a197cc6 on the two Spinner files is exactly the PR's own change and nothing else — ThemedHairlineStroke has no diff hunk at all, so main's four themed stories survive the merge byte-for-byte, and the storybook index on the head build lists core-spinner--narrow-flex-host alongside all seven of main's.

PROBLEM

WHY 1: in an ordinary narrow flex row the spinner's box is compressed to a few pixels and the ring is cut off at the box edge. WHY 2: the person waiting has no visible indication that anything is happening — in the story's 120px row the indicator is gone entirely (2.00 × 24 box around a 21px ring). WHY 3: being visible while work is in flight is the whole job of this component; when it silently disappears the person cannot tell a slow request from a dead one, and re-submits or leaves.

USER-FACING PROBLEM: someone uploading attachments in a 140px row sees a 3px crescent where the loading indicator should be, and beside a sibling that keeps its width sees nothing at all.

PROBLEM SEVERITY: broken task — the definition's "cannot … perceive" arm. Why not the class below it. harmful friction would fit if the person merely lost context or repeated work; here the affordance is absent — a 2.00px box, shown in the frame below, with nothing on screen. The upload itself still completes and the role="status"/"Loading" announcement is intact, so a screen-reader user is unaffected; it is the sighted user who cannot perceive the state at all. That is the lowest class the evidence supports, and the evidence is a frame rather than an adjective.

VERDICT: clear

SOLUTION

The spinner's box used to hide its own overflow, and a flex item that hides overflow tells flex layout it may shrink to nothing. So a narrow row was free to squeeze the box while the ring kept drawing at the size it was asked for, and the hidden overflow then cut the ring off at the squeezed edge — invisibly, because a sliced ring still spins. The box now says outright that it will not shrink, and it no longer hides what it draws. A row that is too narrow makes the indicator stick out of the row instead of cutting it down inside it.

SOLUTION (1 decision · 22 runtime lines of 93)

  1. the box is the ring's size and a host may not take that away — overflow: hidden deleted, flex-shrink: 0 added [the fix] · Spinner.tsx:226-263

Two declarations, one invariant, and they must ship together by the author's own argument: deleting the clip restores the automatic minimum size, which a host removes again the moment it sets min-width: 0 on its items, and flex-shrink: 0 is what holds there. I counted this as one decision rather than two for that reason, and say so because an earlier head of this PR shipped only half of it. The other 71 lines are the changeset and the regression story.

BURDEN: low — two CSS declarations. No runtime state, no Effects, no listeners, no timers, no shared coupling, no public surface, no new test matrix. BURDEN MATCH: proportionate — a broken-task problem answered by a deletion plus one declaration.

VERDICT: clear

ARCHITECTURE

OWNER: Spinner's own box — styles.spinner, packages/core/src/Spinner/Spinner.tsx:226 (the changed declaration is :263). TIER 1: none — no shared system, hook, provider or protocol is touched. TIER 2: none. SEAMS: xstyle, className, style (the component has no render prop, slot, portal or custom surface); and the label path, which puts a wrapper div between the sized span and the host. BEHAVIOR UNIT: inline — two static declarations, no state machine, zero Effects. Extracting anything here would be indirection.

seam driven result
unlabelled span, direct flex child (NarrowFlexHost, 3 hosts) works — 20×20, 24×24, 36×36, 0px clipped
labelled path (wrapper div between span and host) works, and never had the bug — driven on both builds with the same 120px host: the wrapper is inline-flex column, so the span is a column item and the row's squeeze does not reach it. Ring 24×24, 0 clipped, identical on main and head (probe-kit/spinner-5484-label-seam.cjs)
xstyle / style / className stylex.props(styles.spinner, …, xstyle) puts xstyle last, so a consumer who genuinely wants a shrinkable spinner can still say so. The escape hatch survives
RTL driven — 0px clipped in both directions, boxes identical

This is the radius question — where does the fix NOT reach — and the answer is that the one place it could have missed, the labelled wrapper, was never affected.

VERDICT: clear

IMPACT

End users, on the next release, in any app that puts a spinner in a flex row that cannot give it its width. Today they see a sliver or nothing; after this they see the whole ring, overflowing the row if the row is too small. That is the fix and it reaches everyone.

Theme authors who shrink a spinner's stroke below the size's default: their ring stops being amputated. See VISUAL EVIDENCE — this is real, it is good, and the PR does not claim it.

What landing this newly exposes. One thing, and it is not this PR's. A theme that makes the box smaller than the size's default svg frame — the shipped ThemedHairlineStroke does, 30px box against a fixed 36px svg — leaves the svg overflowing its own span, and Chromium start-aligns an over-large grid item rather than centring it, so the ring is drawn 3px right and 3px low of the box's centre. That mis-centring is #5214's and is present on main today (span x 16–46, circle x 20–48, measured identically on both builds). Until now the clip hid the consequence by cutting the ring in half; with the clip gone the ring is whole and 2px of it sits outside the box. Newly visible, not newly broken — and visibly better than what it replaces.

VERDICT: note — a pre-existing #5214 mis-centring becomes visible; the change is an improvement at every frame below

API

No API change. No prop, export, type, default or accepted value moves; SpinnerProps is untouched.

OSSIFICATION: nothing new ossifies. flex-shrink: 0 is an internal StyleX declaration, not surface — no theme can select it, no consumer names it, and xstyle still outranks it, so removing it later costs a line. Deleting overflow: hidden removes surface rather than adding it.

VERDICT: clear

THEMING

No theme target added, removed or renamed. No token, no custom property, no themeProps key changes; themeProps('spinner', {size, shade}) and the four public vars are exactly as main has them.

The diff writes no colour, spacing, radius or shadow: grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(" over the changed hunk in Spinner.tsx → 0. §2 T1–T4 are unreachable.

What the diff does reach in theme-land is behaviour, not surface, and that is the IMPACT note: a themed hairline stroke now renders its whole arc. Existing targets and overrides all still resolve — driven, all four ThemedGeometry spinners and both ThemedColor spinners are byte-identical between builds.

VERDICT: clear

BREAKING

BEHAVIOR: yes, and it is the fix. Walked the states the diff can reach: a spinner in a shrinking flex host (was clipped, now whole), a spinner in a host that already fitted (unchanged, driven), the labelled path (unchanged, driven), RTL (unchanged, driven). empty / loading / error / disabled / controlled / boundary are not reachable — the diff adds no state, no default, no prop and no conditional. API: no — no call site changes meaning or stops compiling. VISUAL: yes, in two places. (1) The intended one: a compressed spinner is no longer sliced. (2) The unclaimed one: core-spinner--themed-hairline-stroke changes — 25 pixels of 210,800, max channel delta 217. Both are shown below. Every other Spinner story is byte-identical. THEME: no — every existing target and override resolves identically; measured, not asserted.

Far side of the bound (R14f): the invariant box = diameter + 2·stroke means the box is always at least the ring's outer extent (diameter + stroke) for any non-negative stroke, so no theme value can make the box narrower than the ring it contains. Driven at the two extremes the repo ships — calc(2rem + 8px)/0.375rem (52px box) and a 1px hairline (30px box): 0px clipped at both.

Both non-"no" axes reach JUDGEMENT: BEHAVIOR as the GOAL line, VISUAL as dispositions 1 and 2.

VERDICT: note — one intended visual change and one unclaimed improvement, both framed below

PERFORMANCE & RESOURCES

EFFECTS: zero added, zero changed, zero moved. The diff touches only stylex.create; git diff origin/main 9a197cc6 -- packages/core/src/Spinner/Spinner.tsx contains no useEffect, useLayoutEffect, listener, observer, timer or rAF.

RENDER: no reachable change — no state, no new dependency, no work moved into render. LISTENERS/OBSERVERS: none added; none removed. LAYOUT: overflow: hidden is a clip and a scroll container; deleting it removes containment work rather than adding it, and flex-shrink: 0 removes a shrink from the flex resolution. If anything this is very slightly cheaper. I did not measure it — R18g permits that for a diff with no added or changed Effect, and I am making no performance claim in either direction. BUNDLE: one declaration removed, one added, no dependency. CI's report on this head shows @astryxdesign/core unchanged at 4.8 KB CJS / 1.2 KB gzipped.

VERDICT: clear — no added or changed Effect, so no measurement was owed and none is claimed

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: three independent triggers, and the third is the one that matters most on this head.

  1. Pixels are intended to change — a clipped ring becomes whole.
  2. The PR asserts unchanged pixels everywhere else, and that assertion was measured on a component main has since replaced.
  3. CI's visual job ran and is green, and it compared nothing. On 9a197cc6 the Stable visual regression check is SUCCESS and its report reads "110 added · 0 removed" — every frame is classified added, so there is no baseline and no changed-frame comparison in existence. visual-acceptance is still PENDING: freddymeta ran /accept-visual and CI refused it ("only a repository maintainer may accept stable visual changes"). Green here means nothing was compared, not nothing changed — the manual frames below are the only pixel comparison for this head.

Chromium 149 only — Playwright's WebKit and Firefox are both killed on launch by this managed Mac's policy, so a second engine was not measurable.

Arms applied to every frame below, banked and inspected. One mutation is applied identically to both sides of every pair, so it cannot produce a one-sided difference — freeze-both-arms.diff:

.astryx-spinner svg{animation:none !important}

The ring rotates, so two frames taken a microsecond apart differ. Pausing through the Web Animations API is not enough: the component's own rAF re-pins startTime afterwards and the pause is undone. Removing the animation in CSS is deterministic. No other declaration is injected on the claim-2 or claim-3 pairs.

Claim 1 — the fix: a narrow flex host no longer compresses the box or slices the ring

Both frames are the same story on the same build (core-spinner--narrow-flex-host, 9a197cc6). The "before" arm restores main's own state for the span with one further rule, banked as B__clip-restored.diff:

.astryx-spinner{overflow:hidden;flex-shrink:1}

Both halves are needed — main declares overflow: hidden and declares no flex-shrink, so it keeps the initial 1, and the automatic minimum size is only reachable by a shrinkable item. (My first arm restored only the clip, measured 0px clipped everywhere, and was wrong; see TIME · waste.) No unrelated declarations are in either arm diff.

Before — main's two declarations restored After — this PR Diff
Before After Diff
host (the shipped story's own three) box before box after ring clipped before clipped after
md beside a label, 140px row 13.45 × 20 20 × 20 17 3.55px 0
lg beside flex: 1 0 100px, 120px row 2.00 × 24 24 × 24 21 19px 0
xl in a 16px host 6.00 × 36 36 × 36 32 26px 0

These are not the changeset's numbers (16px box, "lost half of its ring") and they do not contradict it: the changeset describes the author's own harness, this table describes the three hosts the shipped story actually renders, which have a 1px border, 4px padding and an 8px gap. Same direction, same mechanism, different rows.

Looked at, not inferred: in the before frame the middle row has no spinner at all — only the grey sibling bar — and the third is a 6px vertical shaving of arc. In the after frame all three are whole rings, and the third overflows its dashed 16px host visibly, which is the stated intent.

SENSOR RECEIPT — core-spinner--narrow-flex-host · build 9a197cc6 (Build sensor asserted against git rev-parse) · theme neutral · colour mode light · direction ltr · viewport 900×700 @1 · forced-colors off, reduced-motion off, pointer fine, hover on · targetCount 3 (written from the story's source before the page was opened) · state status:Loading|status:Loading|status:Loading · geometry visible and inside the viewport · fonts loaded · runningAnimations 0 · zero page and Storybook errors. The two arms match on every sensor; only the arm CSS differs. JSON: B__clip-restored.png.sensors.json, B__head.png.sensors.json. Exact head: https://github.com/facebook/astryx/blob/9a197cc6608198a2c0c747f64033150e51247c76/packages/core/src/Spinner/Spinner.tsx#L226-L263

Claim 2 — nothing else moves, re-measured against the component main now has

Zero clip-arm mutation on this pair: each shared story rendered on a build of origin/main at d80c7889 and on a build of this head at 9a197cc6, element-clipped to the same 620×340 region, byte-compared.

story spinners byte-identical
core-spinner--default 1
core-spinner--sizes 4
core-spinner--shades 2
core-spinner--with-label 2
core-spinner--themed-geometry 4 ✅ 0 differing pixels of 210,800
core-spinner--themed-color 2
core-spinner--themed-hairline-stroke 1 ❌ 25 pixels — see claim 3

The three themed-* rows are the ones that matter for this round: they arrived with #5214, after the head this PR was last approved at, so no earlier round could have checked them.

"Nothing renders differently" is a strong claim, so it gets a frame too — core-spinner--themed-geometry at the head, the story whose four themed rings most plausibly could have moved and did not (0 differing pixels against main):

Themed geometry — identical on both builds

SENSOR RECEIPT — every row above, both sides: build asserted per side (d80c7889 for main, 9a197cc6 for the head) · theme neutral, except themed-geometryspinner-themed-geometry, themed-colorspinner-themed-color, themed-hairline-strokespinner-themed-hairline · colour mode light · direction ltr · viewport 900×700 @1 · forced-colors off, reduced-motion off, pointer fine, hover on · targetCount per the story's own source (the counts in the table) · state = the accessible names joined, e.g. status:Loading...|status:Fetching data for with-label · fonts loaded · runningAnimations 0 · zero page and Storybook errors. Before and after match on every sensor except Build, which is the point of the pair. Fourteen JSON receipts banked as A__<story>__<side>.png.sensors.json.

Claim 3 — the unclaimed improvement: the themed hairline ring stops being amputated

core-spinner--themed-hairline-stroke, 8× magnification of the 56×44 region around the spinner. Same build pair and same freeze as claim 2.

Before — origin/main d80c7889 After — this PR 9a197cc6
Hairline before Hairline after

On main the arc stops dead partway round and resumes as a detached fragment lower down — the right-hand third of the sweep is missing. On this head it is one continuous 135° arc. 25 pixels differ, max channel delta 217, in a 3 × 14 box at (46, 55) — exactly the strip immediately outside the box's right edge (the box is x 16–46).

The mechanism, confirmed a second way (computed geometry, independent of the pixels): the box is 28 + 2×1 = 30px while the svg keeps the size's default frame of 36px, so the svg is the larger box; Chromium start-aligns an over-large grid item instead of centring it, giving spanBox.x == svgBox.x == 16 with the svg 6px wider. The circle therefore lands at x 20–48 against a span of 16–46, and overflow: hidden used to cut the last 2px off. Identical geometry on both builds — the mis-centring is #5214's and is on main today; only the clipping of it changes here.

Labelled intentional or not: intentional, and cited from the component's own contractSpinner.doc.mjs:47 documents --spinner-diameter as "The rendered box is this plus the stroke width on each side, and follows automatically", and :48 documents --spinner-stroke-width as driving the stroke of "both circles the ring is drawn from". A ring drawn to that contract is a whole ring; the sliced one was the defect. The PR does not say this, which is why it is called out — R1j is about not supplying a rationale for an unargued change, and this is the inverse: an unstated change that has a contract behind it, so the ask is one line of changeset text rather than an argument.

SENSOR RECEIPT — as claim 2's row for core-spinner--themed-hairline-stroke: builds d80c7889 / 9a197cc6 · theme spinner-themed-hairline on both · colour mode light · direction ltr · viewport 900×700 @1 · same media · targetCount 1 · state status:Loading · fonts loaded · runningAnimations 0 · no errors. Identical on every sensor except Build. The crops above are lossless 8× nearest-neighbour blow-ups of the receipted PNGs; the un-magnified originals are banked beside them as hairline-full-before.png / hairline-full-after.png.

Class check (R32): which shipped configurations put the box below the size's default frame, i.e. themedDiameter + 2·themedStroke < defaultDiameter + 2·defaultBorder? Enumerated the repo — grep -rn -- "--spinner-stroke-width|--spinner-diameter" packages apps .github — and there is exactly one: themedHairline at apps/storybook/stories/Spinner.stories.tsx:169. ThemedGeometry's four all grow the box (sm 20 > 14, xl 52 > 36) and are byte-identical. No theme package ships a spinner geometry override. One member, and it is one of our own stories.

VERDICT: note — one intended change and one unclaimed improvement, both framed, both sourced; nothing regressed

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect is introduced by this PR. The one visual defect in view (the 3px mis-centred ring in a theme-shrunk box) is pre-existing on main, unchanged by this diff, and not this author's to fix.

A11Y & I18N

Structural-only for both halves, established with the grep rather than by exhaustion:

grep -nE "aria-|role=|useTranslator|t\('@astryx" <the diff's hunks in Spinner.tsx>   → 0
grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(" <same>            → 0

§1 — auto-covered: pr-a11y is SUCCESS on this exact head and .github/a11y-baseline.json is not in the changed-file list, so no silence was bought. Checked by me, driven: the role="status" element and its accessible name are unchanged on both builds — the state sensor read status:Loading on every frame above, and status:Loading... / status:Fetching data on the labelled pair, identical between main and head. A5 focus is not reachable — the spinner holds no focusable element and the diff adds none.

One row deserves naming rather than a tick. A ring that was being silently cropped is now whole — the sighted-user half of "expose state by more than colour or shape" gets better, because on main a themed hairline spinner in this state renders as a broken arc that reads as a rendering glitch rather than as motion.

§9 — the diff adds no string and no directional property. overflow and flex-shrink are direction-agnostic; I8–I12 are unreachable. Driven anyway, because the fix is about which way a box overflows: NarrowFlexHost in RTL gives boxes 20×20, 24×24, 36×36 and 0px clipped, identical to LTR. CI's pr-rtl is also SUCCESS on this head, which is the floor rather than the check.

VERDICT: clear

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION clear
ARCHITECTURE clear
IMPACT note — a pre-existing #5214 mis-centring becomes visible; the change is an improvement
API clear
THEMING clear
BREAKING note — one intended visual change, one unclaimed improvement
PERFORMANCE clear
VISUAL note — one intended change and one unclaimed improvement, both sourced
A11Y & I18N clear

Read against the bodies, not the labels. Two things only JUDGEMENT can see here, and I checked both. Wrong-slot filing: IMPACT, BREAKING and VISUAL all carry one fact — the hairline story's pixels move — and it is owned by VISUAL, which has the frames; the other two point at it rather than each raising a separate note. Compounding: the three notes do not add to anything worse, because they are the same event, and its direction is better at every frame. No slot body contains a sentence describing a user who is stuck, cannot reach something, hears nothing, or loses their place — the one sentence in that shape ("the indicator is gone entirely") describes main, which is what this PR removes.

GOAL: met — the stated problem is "a narrow flex host compresses the box and clips the ring". Measured at this head on the shipped story: 3.55px / 19px / 26px of ring clipped before, 0px after, in all three hosts, with the boxes restored to 20×20, 24×24 and 36×36. The frames show it. That is the outcome changing, not the code changing.

DISPOSITION:

  1. The changeset and PR body claim the clip "clipped nothing" and that pixels are identical at every size and shade.not blocking; the one nit in the review. Untrue at this head since #5214, and the sentence ships to the changelog. Cheap to correct, and the correction is in the PR's favour.
  2. core-spinner--themed-hairline-stroke changes by 25 pixels.accepted, and it is an improvement, not a cost. Who pays: nobody — a theme author who shrinks the stroke gets a whole ring where main gives them a broken one. Sourced from Spinner.doc.mjs:47-48, not from taste. This is the visual break BREAKING records, dispositioned here rather than left in a slot.
  3. The ring is mis-centred by 3px in a theme-shrunk box.out of scope, not accepted on anyone's behalf. Pre-existing on main, measured identically on both builds, #5214's to answer for (R13 — never charge a contributor for inherited debt). Raised to Cindy in this handoff as an adjacent defect; no promise is made to the author, so nothing is owed under R2g.
  4. The visual gate reports "110 added · 0 removed" and compared nothing.not a finding against this PR; a merge fact. visual-acceptance is PENDING and only a repository maintainer can clear it.
  5. needs:design-review is on the PR.standing merge gate, untouched by this verdict. The engineering verdict is approve; the label still gates the merge and this review does not clear it.

ADVICE: omitted — nothing is asked for beyond one sentence of changeset text, and the author owns that wording.

AUTHOR CAN PROCEED: yes — nothing blocks. If they want the nit: the changeset's "it clipped nothing / same pixels at every size and shade" sentence is inaccurate for a theme that shrinks the box, and the honest version is that the themed hairline ring stops being cropped.

WORST OUTCOME: "in the story's 120px row the indicator is gone entirely (2.00 × 24 box around a 21px ring)" → that sentence describes origin/main, not this head. At this head the equivalent sentence is "2px of a hairline ring sits outside its box in one themed story, where main cut the ring in half." Neither the word "cannot" nor any loss of function attaches to the head, and the frames show the head is better in both places. → compatible with approve.

JUDGEMENT NEEDED: none — defect fix against the existing contract. No API moves, no new concept, and the one visual difference is cited from Spinner.doc.mjs:47-48 rather than chosen. (The repo's needs:design-review label and the pending visual-acceptance gate are separate, standing merge gates and are preserved.)

approve

  1. [not blocking] The changeset says the clip "clipped nothing" and that hiding or showing the overflow "renders the same pixels at every size and shade" → whoever reads the changelog is told nothing moved, when a themed hairline spinner visibly stops being cropped — 25 px, max channel delta 217 · .changeset/spinner-box-flex-shrink.md:7

REVIEW

Thanks — re-approving at the new head; the main merge dismissed my earlier approve. Rechecked in Chromium against the themed ring: the fix holds and the hairline story improves.

[Reviewed by Robohands]

INLINE (0–3, only when genuinely line-anchored)

  • .changeset/spinner-box-flex-shrink.md:7 — Not quite true since #5214: the themed hairline ring does change, for the better.

EVIDENCE I DID NOT SPEND

  • packages/core/src/Spinner/Spinner.test.tsx — 47 tests pass at this head (33 at the approved head; #5214 added the rest). No unit test covers the fix, correctly: the property is only observable through layout.
  • 91 other overflow: 'hidden' declarations in packages/core/src. Some are on flex items and inherit the same zero-minimum-size footgun. That is a system observation, not this PR's finding, and one instance never justifies a system change (R6).
  • The labelled Spinner never had this bug — the wrapper is a column flex container, so a row's squeeze never reaches the sized span. Driven on both builds; it makes no difference to the verdict.

TIME

TIME  total 26m
  setup       3m   worktree at the exact head + clone-seeded install (2.3s) + build dist + storybook dev on a scratch port  (warm main reused: yes — already at d80c7889 = origin/main, no rebuild)
  reading     5m   brief, critic, harness, presentation, version file; PR history, both prior reviews, the wiki record, the 783feafc..9a197cc6 delta and the merged Spinner
  measuring   9m   4 probe runs + 1 re-measure of the whole 7-story sweep, forced by the instrument
  writing     6m   presentation, critic pass 1, rewrite
  wiki        3m   fresh clone, Round 3 page + index row, two locked pushes (the second scrubbed operator paths out of the committed receipts), URL and image verification, teardown
  waste       4m   the first sweep froze the rings through the Web Animations API; the component's own rAF re-pins `startTime` afterwards, so every frame was captured at a different rotation and all 7 stories reported a false pixel mismatch. Killing the animation in CSS instead is deterministic. Same run, the first "before" arm restored only `overflow: hidden` and not the initial `flex-shrink: 1`, so it measured 0px clipped and hid the bug it existed to show. Also listed a `core-spinner--inline-in-text` story that does not exist.

  remote CI wait  0m   CI had already settled green by the time the presentation was written; the poll returned on its first iteration

Re-measures: 1 (the full A+B sweep, re-run after both instrument faults were fixed). Warm main reused: yes. Probes banked: probe-kit/spinner-5484-flex-floor.cjs, probe-kit/spinner-5484-label-seam.cjs.

WHAT I COULD NOT VERIFY

  • Anything outside Chromium 149. WebKit and Firefox are killed on launch by this Mac's managed policy, so the start-alignment of an over-large grid item is verified in one engine only. It is the engine CI's visual gate uses.
  • Whether CI's visual gate would have caught the hairline change had it possessed a baseline. It reports all 110 frames as added, so the question is unanswerable from this head.

Clone this wiki locally