Review 4743

#4743 — fix(dropdown-menu): move indicator-icon theme target onto the Icon element

#4743 · athz · contributor Head reviewed 68bc50450f90f5b2334a8f3f83e7968066ba2e15 (merged as 7a08c519fed after a main merge) Verdict approve · posted 2026-08-23

This review's subject was the previous review, not the diff. The loop had requested changes on it the same evening, and that block was wrong.

Problem

A theme author writes the documented way to restyle an icon target — 'dropdown-menu-indicator-icon': {base: {width, height, fontSize, color}}, the exact shape Selector.test.tsx:2428 documents — and the submenu chevron does not move. Measured on main: 16×16 and rgb(115,115,115) before the rule, 16×16 and rgb(115,115,115) after it. The identical declaration works on Selector, MultiSelector and ComplexSelector, so the theme comes out inconsistent across four components with nothing failing to explain why.

Solution (1 decision · 8 runtime lines of 22)

  1. the dropdown-menu-indicator-icon target moves from the caret wrapper
     <span> onto the <Icon> that paints the chevron                 [the fix]

Traces to the stated problem. The test update and the changeset are evidence, not decisions. The spinner branch losing the target is a consequence of decision 1 — the target cannot sit on the glyph and also on a wrapper that has no glyph.

Impact

Nobody's pixels change. Reaches only themes that write a rule against dropdown-menu-indicator-icon, and no shipped theme does — none of the seven in .storybook/preview.tsx reference it. Invisible until a theme opts in; then it works where it previously did nothing. Also reaches ContextMenu, Breadcrumbs and SideNav, which share DropdownMenuSubMenu.

API

No API change.

Theme targets

~ dropdown-menu-indicator-icon  → was: caret wrapper <span> (layout only)
                                  now: the <Icon> span that paints the chevron
                                  (public, already shipped; absent from the
                                   hasSpinner branch)

No new targets.

Ossification

Not new surface — conformance to surface that already shipped (R16f: find the declaration; if a landed sibling already says this should happen, it is repair, and the default is take it). The declaration exists three times over: the wiki principle, #4838's twenty components, and the target's own name. The class has twenty merged members.

Breaking

• API — no.
• Visual — no. Submenu row screenshots at DPR 3 are byte-identical before/after under neutral and y2k. The loading row's frames differ only by animation phase, proved by a control: two captures of the same build also differ, while row pixel dimensions are identical (270×96 neutral, 330×126 y2k).
• Theme — one rule shape changes, only in the loading state. display:none on the target hides the spinner today (carrier 14×14 → 0×0) and does not after. Every sanctioned use is unaffected: size, colour and per-state rules reach nothing in the loading state on either side. Hiding via a target is ruled out by theming principle 5, and all three siblings already behave this way.

Performance & resources

Nothing. No effects, listeners, observers or subscriptions added or removed; one mergeProps call deleted per branch, a props spread moved down one element. Node count unchanged.

Visual evidence

- theme rule reaches the chevron?  before/after  shots-4743/before2__chevron__theme-rule-applied.png
                                                 shots-4743/after2__chevron__theme-rule-applied.png
    before 16x16 rgb(115,115,115) · after 28x28 rgb(216,27,96)
- default render, shipped themes   before/after  shots-4743/{before,after}__{neutral,y2k}__chevron.png
    byte-identical both themes
- loading state, theme rule        before/after  shots-4743/{before,after}__spinnerpaint__*.png
    painted canvas pixels identical: track 204,211,219 / arc 38,38,38
- loading state, display:none      before/after  shots-4743/{before,after}__hide-spinner__*.png

All opened with the read tool. Three probes banked to probe-kit/: theme-target-reach.cjs, spinner-paint-reach.cjs, dropdown-indicator-theme-matrix.cjs. Reproduce against storybook dev on the PR head at SB_PORT=6504; the before side is git checkout HEAD~1 -- on the one file, waiting for HMR, and re-running.

A number deliberately not reported: the spinner <canvas> bounding box read 15/17/18/20px across runs. That is an animating canvas sampled mid-frame, not a measurement — which is why the loading-state claim rests on getImageData() pixel readback and identical row dimensions instead.

Risk class

Low. One component, no API, no visual change, and the one behaviour that shifts is unreachable by any sanctioned theme rule.

Judgement

approve — carried by a single finding, which is about the previous review.

1. Round 3's block asked for a target that names two different kinds of
   element depending on state
   → a theme author writing `.astryx-dropdown-menu-indicator-icon {width}`
     would size an <svg> when idle and a layout <span> when loading, with
     nothing to explain the difference    · DropdownMenuSubMenu.tsx:476-487

   Confirmed three independent ways (R16g):
   - reading — theming principle 1, and Selector.tsx:1540 /
     MultiSelector.tsx:1618 / ComplexSelector.tsx:522 all leave their busy
     Spinner bare
   - measurement — reverting the one file returns the glyph to 16x16 grey;
     and the theme rule reaches the painted spinner in NEITHER build
   - grep — no test, doc, story or shipped theme references the target in
     the loading state

Recorded and not raised (R1j): the PR body never says the loading branch drops the target, and no sentence justifies it. Correct anyway — and charging a contributor who had waited seventeen days for a one-clause changeset edit is the failure R1g exists to prevent. The clause was added on merge.

Tests 19/19 in DropdownMenuSubMenu.test.tsx. CI green.

The review, as posted

Thanks for the patience on this, and sorry — my last round was wrong. The target belongs on the Icon, as Selector and MultiSelector already do. Merging.

[Reviewed by Robohands]

Inline, DropdownMenuSubMenu.tsx:477:

Right to drop it here — Spinner has its own target; this span only lays out.

The timeline, and why it is the record that matters

GitHub shows three CHANGES_REQUESTED. Two are 14 seconds apart with empty bodies — one inline question and its verdict flag, submitted as one act. There were two substantive reviews, not three.

when	elapsed	what
Aug 6 01:01	—	opened: divider-margin CSS var + indicator-icon. Supersedes #4667
Aug 6 23:31–23:55	+23h	Round 1. One 🔴 blocker, one design question reserved for the TL, three 🟡 sections, six bullets
Aug 7 07:25	+7h30m	athz force-pushes: drops the divider work entirely, keeps the indicator-icon move. Renames the PR twice
Aug 7 07:31	+7h36m	athz replies explaining the rescope
Aug 7 → Aug 23	16 days	nothing
Aug 23 20:58	+17d	Round 3. CHANGES_REQUESTED on the spinner branch
Aug 24 04:23	+17d	retraction, approve, main merged in, squash-merged

Author turnaround 7½ hours. Ours 16 days. The PR was not looping. It was queueing, and the one time it moved, it moved backwards.

Why this went around — four failures, all ours

1. Round 1 overloaded a contributor, and the PR lost its subject. A 🔴 blocker, plus a design question the reviewer explicitly reserved for herself ("that's the question I want to answer first"), plus three 🟡 sections, on a divider-margin change. The rational response to "there is a blocker, and also a question you are not allowed to answer, and also six other things" is to delete the contested part. That is what athz did — and the contested part was what the PR was for. #4667 was superseded by #4743, #4743 then dropped the divider work, and two of his PRs have produced no divider fix.

2. Sixteen days of silence. He answered in 7½ hours. In return the PR's Storybook preview was garbage-collected (a 404 by review time) and its head carried 8 CI runs where a current PR gets ~19. Both are our latency surfacing as his problem.

3. Round 3 blocked him for following the written rule. The system had already decided this, in three places, and round 3 cited none of them:

• Theming Infrastructure, principle 1: "Target the element that carries the styling… If a decorative glyph is wrapped in an inert/positioning div, the target belongs on the glyph, not the wrapper." And principle 3 names a caret among the concepts that must converge across Selector, MultiSelector and DropdownMenu.
• #4838 (4b1fef5f700) converted twenty components to exactly this shape and shipped a @astryx/no-wrapper-transform lint rule for it. DropdownMenu is the one component it missed.
• The siblings do not put the target on their Spinner. Selector.tsx:1540, MultiSelector.tsx:1618, ComplexSelector.tsx:522 are each a bare {isBusy && <Spinner size="sm" />}; Spinner carries astryx-spinner itself (Spinner.tsx:281,300).

4. The parent decision was never named — and did not need escalating. Both rounds circled "where does this theme target belong", which is a tier question and would be the TL's. It was already answered, in writing and in twenty landed components. The loop's real defect is that it re-derived a settled decision from first principles and then requested changes against it.

athz read the precedent better than the loop did: his changeset mirrors #4838's wording and convention down to the [fix] prefix and the trailing handle.

What we pushed to his branch, and why

Under R13d (refresh yes, content no): a main merge (17 days stale, clean, no conflicts) and one changeset clause noting that the loading branch leaves the target to Spinner's own. Both disclosed on the PR in a comment before the merge. Never force-pushed.

Still open, and not this PR

Round 1's design question was never answered and has now cost two PRs their subject: is a menu divider's vertical rhythm a theme value, or spacing the menu owns? "I'm working through a rule I haven't written into the wiki yet… layout properties encode a component's structure, so they shouldn't become theme seams… Margin is the part I want to be deliberate about." It does not block #4743 and must not be attached to it.

The gap this exposes in the kit

Second instance of the loop re-deriving a settled decision instead of looking it up. #4707 was the first, caught by STEP 7c2 before it posted; this one shipped and cost a contributor 16 days. 7c2 greps the diff for the fix it is about to ask for. It does not grep the SIBLINGS for the convention — which is the check that would have killed both. Proposed as a rule; see the run notes.

Cost: 20 minutes, 11 probe runs, 3 re-measures.