Skip to content

Review 5466

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 3 revisions

Review 5466 — Give NumberInput steppers a dedicated icon slot

Author rubyycheung · full loop, 5 gates, 32m · read-only.

Ruling (Cindy, 2026-08-24): namespaced key, not an IconName member — Rulings #11. Enabling change: #5475.

Two corrections posted after the review: the head moved twice mid-run (8770ad5 → 8fac0ee → 7c28c26). The filled-triangle finding is stale — all seven themes are back on <ChevronDown/>. And at 7c28c26, scripts/bundled-number-input-stepper-icons.test.tsx is 507 lines of git show output committed with a .test.tsx extension.

pr-visual red is a harness ordering bug, filed as #5474.

PR

#5466 Give NumberInput steppers a dedicated icon slot by rubyycheung (bucket: internal — DESIGNOWNERS, write access)

HEAD REVIEWED

8fac0ee39dc640e388f0023f91e10f36c22c32d4 <- every claim below verified at this commit

The head moved mid-review (8770ad5 → 8fac0ee, +87/-9 → +261/-9); everything was re-checked. The new commit swaps all seven bundled themes from <ChevronDown/> to a rotated filled Lucide Triangle, adds a theme-registry test, and adds a geometry section to the body. Prior review: none — reviews → 0, comments → 0; bots only.

PROBLEM

A theme author who wants the NumberInput steppers to carry a different caret from the one in menus, selectors and nav can't have it: both stepper buttons resolve chevronDown, and 14 core components share that key, so whatever glyph they register lands in all 14 places. Real, with one named consumer — a downstream integration wanting a filled 12px caret on steppers and its outline chevron everywhere else. Demonstrated, but singular.

VERDICT: clear

SOLUTION

SOLUTION  (2 decisions · ~73 runtime lines of 261)
  1. add `numberInputStepperDown` to the core `IconName` union; both stepper
     buttons resolve it   globalIconRegistry.tsx:33, NumberInput.tsx:1026,1048   [the ask]
  2. the stepper affordance becomes a filled triangle instead of a chevron —
     new Core default glyph AND a rotated Lucide `Triangle` in all seven
     bundled themes   defaultIcons.tsx:72, themes/*/src/icons.tsx:53-63   [NO STATED PROBLEM]

Decision 2 is one decision, not eight — the same design call in eight registries. It is independent of decision 1: the themes kept <ChevronDown/> one commit ago. The other ~188 lines are seven generated CLI templates, a 58-line registry test, four doc lists and the changeset.

7a — grep -in "because|why|rationale|previously|regression|wrong" on the body → 3 hits (a summary bullet, the ## Rationale heading, a Sapling-checkout note):

decision the sentence saying the old behaviour was wrong
1 — the union member "Reusing chevronDown coupled the steppers to a semantic shared by unrelated controls." — recorded
2 — the triangle none. Described in four places, justified in none

7d hypothesis: everything will be a child of should a component-specific glyph ossify the core IconName union, or go through the namespaced extension seam that already exists.

VERDICT: note — 2 decisions, the second traces to no stated problem

IMPACT

Every end user on a bundled theme — the stepper arrows change from thin outline chevrons to solid filled triangles. New as of 8fac0ee; one commit ago this was invisible to them. Nothing moves: glyph 12×12, stepper column 16×30, input 454×20, measured identical in both builds. The triangles are markedly heavier — 9.998 × 8.998px painted, by the author's own measurement, against a 1.5px stroke. On Core with no theme it is a smaller solid caret (~5.3 × 3.4px), unchanged by this push, so the same slot renders at about a fifth of the ink without a theme as with one.

A builder with a custom theme — their theme stops compiling on a patch bump. IconRegistry is Record<IconName, ReactNode>, so widening the union adds a required key, and every theme astryx init scaffolds imports that type (import type {IconRegistry} from '@astryxdesign/core/Icon'). Proven at head:

error TS2741: Property 'numberInputStepperDown' is missing in type
'{ close: JSX.Element; ... }' but required in type 'IconRegistry'.

Newly exposed: the other 13 components sharing chevronDown — Banner, Breadcrumbs, Chat, Collapsible, ComplexSelector, DateInput, DropdownMenu, MultiSelector, Selector, SideNav, TabList, TopNav, Table — get a landed precedent for a slot of their own. A Selector beside a NumberInput now shows an outline chevron next to a filled triangle.

VERDICT: note — a patch that changes every themed app's steppers and breaks a scaffolded theme's typecheck

API

No new props, no new exports. One new member on a public union.

registerIcons({numberInputStepperDown: <MyCaret />});
defineTheme({icons: {numberInputStepperDown: <MyCaret />}});
change public? class doc'd? verdict
+ IconName | 'numberInputStepperDown' yes — core barrel + /Icon subpath none — first of 28 names to name a component en+zh+dense [Needs human judgement]
~ IconRegistry gains a required key yes — @astryxdesign/core/Icon — n/a pre-existing, see BREAKING

P1–P5 don't reach this diff: no *Props, no ...rest, no booleans, no callbacks.

Does it earn its permanence?

  1. Class or single case? Single — all 28 existing names are generic visual or functional concepts (close, chevronDown, check, funnel, microphone); none names a component.
  2. What already exists, and why does it not reach? It does reach, and that is the finding. globalIconRegistry.tsx:190-193 — "the seam libraries use to make their own icons themeable: ship the inline SVG as fallback, resolve through this function, and a theme can override the key via registerIcons without the library having to widen the core IconName union." RichTextEditorToolbar.tsx:162-176 runs 15 namespaced richtext:* keys through it, tested at RichTextEditor.test.tsx:1268. The near miss is ours, not the author's: Icon.tsx:204 is icon: IconType | IconName;, so a core component cannot write <Icon icon="numberInput:stepperDown" size="xsm">. getIcon already takes ExtendedIconName; only the prop type is narrow.
  3. Cost of being wrong? A union member can't be withdrawn without breaking every theme that grew the key, and can't be renamed at all.

New concept: yes — a second mechanism for something one mechanism already does.

STEP 5b, both directions
  IconName members naming a component   0 / 28
  components resolving chevronDown      14
  namespaced extension keys shipped     15   (richtext:*)
  diff reaching for getExtendedIcon      0   (7c2 — the finding survives)

VERDICT: [Needs human judgement] — new concept: a second mechanism for a component-scoped themeable glyph

THEMING

No new theme targets, no astryx-* class, no colour, spacing, radius or shadow token — T1–T4b don't reach it. The one style value written is fill: 'currentColor', T2b-exempt.

grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(" <25 changed files>  → 0

The icon registry is themeable surface, so the new key is permanent public theming surface in all but name — judged in API. Seven bundled registries and their seven generated CLI templates byte-identical; theme-layers green.

VERDICT: clear

BREAKING

Behaviour — no. No state, conditional, default, early return or setter. Walked all six states against NumberInput.tsx:1008-1051: empty/zero, loading, error, disabled/readonly, controlled vs uncontrolled, and the canIncrement/canDecrement boundary all render the same tree with a different glyph inside it.

API — yes, for a custom theme. The TS2741 above. Inherent to every IconName addition, not invented here: #4505 added chevronsLeft/chevronsRight identically, [feat] on a patch changeset. R1f checked and honest — the type claim matches the diff and matches precedent, so no bump is asked for. Pre-existing; not the contributor's to fix.

Visual — yes, now on every bundled theme as well as the no-theme path. Frames below. Glyph 12×12, column 16×30, input 454×20 — identical both builds, so nothing grew.

Theme — no. chevronDown untouched at all 14 call sites, and the new test asserts it keeps no fill or transform.

VERDICT: note — a scaffolded theme's typecheck breaks on a patch, precedented and pre-existing

PERFORMANCE & RESOURCES

Effects: zero. grep -nE "useEffect|useLayoutEffect" across the 25 changed files → 0. Same two <Icon> renders, same single getIcon lookup; no new subscription, listener, observer or layout read. The rotation is a static CSS transform. Bundle: one inline SVG path in defaultIcons (~180 bytes) and one more Lucide import per theme — colour, not a finding. Nothing measurably worse.

VERDICT: clear

VISUAL EVIDENCE

Real Chromium, deviceScaleFactor: 8, story core-numberinput--with-number-steppers, PR head on port 6212 against warm main on 6100. Every frame opened with the read tool. Probes banked at {review-root}/probe-kit/stepper-icon-slot.cjs and stepper-icon-slot-hidpi.cjs.

VISUAL EVIDENCE
- bundled theme (neutral)   before/after   shots-5466/v2__before__neutral.png · v2__after__neutral.png
- Core default, no theme    before/after   shots-5466/v2__before__none.png · v2__after__none.png
- neutral, forced-colors: active           shots-5466/fc2__before__neutral.png · fc2__after__neutral.png
difference intentional? source
themed steppers: outline chevron → filled triangle intentional, unsourced described in four places, justified in none (7a). Design Conventions has no line on stepper affordances; the rubric's §5 has no approved stepper representation to reuse
no-theme steppers: outline chevron → small solid caret intentional, unsourced same
arrow direction correct driven, not assumed — below
geometry no change glyph 12×12, column 16×30, input 454×20, both builds

The arrows point the right way, and it is worth saying why I checked. The theme glyph carries rotate(180deg) and the increment button already carries styles.incrementIcon's rotate(180deg) (NumberInput.tsx:186) — which reads like a double rotation and a visible regression. Driven in Chromium: increment composes to a net 0° (svg:180 + SPAN:180), decrement to 180°, and Lucide's Triangle points up natively, so the buttons render ▲ and ▼. Deliberate, and it keeps the slot's name true of what a theme registers.

Credit where it is due: this push fixed a real centring bug the author found unprompted — the body's geometry table records that rotating via the SVG transform attribute displaced the glyph 24px off each 15px button centre, and the CSS transform-origin: center version measures 0px. The new test's not.toHaveProperty('transform') assertions guard exactly that (R14b: the check shows its catch).

For the design owner: the same slot renders at roughly five times the ink under a bundled theme as under the Core fallback, and defaultIcons.tsx's docblock still says the file uses "stroke-based rendering with 1.5px stroke width" with solid fills reserved for "status icons" — a caret is not one, and the exception list was not updated.

Whoever posts publishes these to assets/pr-5466 on the fork.

VERDICT: note — the glyph change is real and correct, and unjustified in the body

A11Y & I18N

§1 automated. pr-a11y green and .github/a11y-baseline.json untouched — no silence bought. The PR-Analysis report's serious color-contrast on Icon is a pre-existing baselined entry. pr-rtl, build and test green. pr-visual is red for a harness reason, not a pixel one — see the top of JUDGEMENT.

§1 browser half. Both glyphs are decorative and stay decorative — svgProps and solidSvgProps carry aria-hidden: true (defaultIcons.tsx:38,53) and the theme props spread iconProps, which does the same. Neither caret enters the a11y tree; the buttons' accessible names (@astryx.numberInput.decrementLabel, untouched) still carry the meaning. A14 driven: forced-colors: active in real Chromium on both builds — the filled triangle paints correctly, frames above. A1/A4/A5/A8-A9/A13/A15 are unreachable by this diff: no element added or removed, no focus target, keyboard path, hover reveal, animation or focus ring changed, and the probe confirms identical element count and box.

§9 strings: zero. grep -nE "useTranslator|t\('@astryx" → 0; packages/core/locales/en.json untouched. The doc prose that changed is metadata, not catalog strings — en, zh and dense were updated together, twice, which is the usual failure here and it did not happen. §9 direction: no logical or physical properties; the rotation is symmetric and direction-independent.

VERDICT: clear

JUDGEMENT

PROBLEM        clear
SOLUTION       note — 2 decisions, one traces to nothing stated
IMPACT         note — every themed app's steppers change; scaffolded themes stop compiling
API            [Needs human judgement] — new concept, second mechanism
THEMING        clear
BREAKING       note — API break for custom themes, precedented
PERFORMANCE    clear
VISUAL         note — glyph change real and correct, unjustified in the body
A11Y & I18N    clear

pr-visual is red, and it is not this PR's pixels. The job log says Error: Theme butter is not built (packages/themes/butter/dist/source.mjs missing) — run pnpm build before the visual gate. The gate resolved Components: Icon,NumberInput,Table and then died before shooting a single frame. It is a harness ordering bug — the visual gate needs the theme dists and this is the first PR in a while to make that matter — not snapshot drift and not the author's code. Worth a separate one-line comment so she isn't chasing it, and worth a gap against the gate; it is not part of this review's verdict. Everything else at head is green.

No slot's body describes something a user would call a bug — nobody is stuck, unable to reach, unable to hear or unable to get out. The arrows point the right way, nothing grew, nothing blocks. The notes do not compound: one design change described-but-unargued, one precedented type break, plus the escalation.

7d, late pass — half held, and it survived a head move. Decision 1's findings are all children of which tier a component-scoped glyph lives in, and the review is written at that level — which is why 8fac0ee rewrote a third of the diff and changed not a word of the argument. Decision 2 refutes the hypothesis: the triangle survives either mechanism and is a separate, unstated design change.

JUDGEMENT NEEDED: API — new concept (a second mechanism for a component-scoped themeable glyph; getExtendedIcon + namespaced keys is the first). Cindy's. The author is a DESIGNOWNER, so her authorship does stand in for the design half — the triangle is her call and I am not second-guessing it — and does not stand in for this. #4327's shape exactly.

What I would decide: hold the union member and take the same outcome through getExtendedIcon('numberInput:stepperDown', <caret/>). The theme author gets the independent mapping either way; the extension route costs no permanent union surface, no required key on every downstream IconRegistry, and no precedent for the other 13 chevron consumers. The tradeoff is one line from us first — widening Icon's prop to IconType | ExtendedIconName — and until that lands the union is the only route that keeps size/color/xstyle. If the downstream timeline can't wait for that line, taking the union member is defensible; it is just not free.

needs human

  1. numberInputStepperDown is the first of IconName's 28 names to hard-code a component, and the union is the expensive tier → whoever writes the next themeable component glyph cites this and the union grows per-component; 13 other components already share chevronDown and can all make the same argument · globalIconRegistry.tsx:33 — | 'numberInputStepperDown'

  2. The stepper affordance becomes a filled triangle in every bundled theme, and the body never says the chevron was wrong → every app on a bundled theme gets new-looking steppers on a patch upgrade, and a Selector beside a NumberInput now shows an outline chevron next to a filled triangle · themes/neutral/src/icons.tsx:63 — numberInputStepperDown: <Triangle {...numberInputStepperDownIconProps} />

REVIEW

Thanks Ruby — steppers wanting a caret of their own is a real gap, and the centring fix was a good catch.

It's the tier I'd push back on, not the idea. None of IconName's 28 names names a component, and widening it makes every downstream IconRegistry grow a required key — a CLI-scaffolded theme stops compiling on a patch. The registry already has the cheaper seam: getExtendedIcon's docblock says it exists so a caller doesn't "have to widen the core IconName union", and the richtext toolbar runs 15 namespaced keys through it.

<Icon icon="numberInput:stepperDown" size="xsm" color="inherit" />

That doesn't typecheck yet (Icon.tsx:204), so that part is ours, not yours.

Would the namespaced key cover your downstream case?

INLINE (0-3, only if genuinely line-anchored)

  • packages/themes/neutral/src/icons.tsx:63 — numberInputStepperDown: <Triangle …/> — Might want a line in the body on why the chevron was wrong; this changes every themed app.
  • scripts/bundled-number-input-stepper-icons.test.tsx:49 — Asserts the props, not a render. Drop incrementIcon and both arrows point down with this still green.
  • .changeset/number-input-stepper-icon-slot.md:15 — @rubycheung — think this wants the second y, otherwise the credit doesn't link.

EVIDENCE I DID NOT SPEND

  • #5318 (ernestt) lets the three Selectors drop the trigger chevron — adjacent ground, different mechanism, no collision.
  • The PR body names the downstream package outright under "Downstream integration". Public repo; a quiet word out of band, not a review comment that repeats it.
  • IconName is hand-mirrored in four prose doc strings plus the CLI table, and this PR adds two more hand-maintained steps to the CLI's icon checklist. R7b drift, pre-existing, not this author's to fix.

TIME

TIME  total 32m
  setup        4m   worktree + clone-install (11s) + build + storybook dev on 6212  (warm main reused: yes)
  reading      4m   brief, critic, harness, PR body, diff, source   (ran parallel with setup)
  measuring    8m   11 probe runs across 2 heads; tsc proof of the downstream break;
                    rotation-composition drive; 2 STEP 5b re-counts; pr-visual log dig
  writing     15m   7 drafts + 5 gates
  waste        3m   upscaled a 12px bitmap with sips for the first "zoom" frames — the blur made the
                    glyph unjudgeable, so I reshot at deviceScaleFactor 8; an element crop whose
                    selector never matched; a citation counted off a truncated grep ("13 richtext
                    keys", actually 15) that gate 2 caught; and three rounds of 5% trims where one
                    real cut was wanted.

Warm main reused: yes (port 6100; no second checkout of main). Re-measures: 2 — one forced by the sips-blurred frames, one forced by the head moving to 8fac0ee and invalidating every themed frame. The second is not waste: the head move was the author's push and re-measuring was the correct response.

WHAT I COULD NOT VERIFY

  • Whether the downstream integration could take the namespaced-key route on its timeline — the question the review ends on, and only the author can answer.
  • Whether the pr-visual harness failure also hides a real snapshot diff. The gate died before shooting, so there is no comparison to read; a rerun after the theme build is the only way to know.

Clone this wiki locally