Review 5324

#5324 — fix(Chat): align composer tokens to middle instead of synthesized baseline

athz · community · MERGED · view on GitHub

Verdict: approve — posted and merged 2026-08-24, squash 51fec0ca2de. Drafted 2026-08-23, second round the same evening, posted after Cindy's read.

Round 2 — multi-line. Cindy read the draft and asked for one thing: "5324 seems good but maybe check multi lines?" Measured (below); the verdict did not move, and multi-line turned out to be the strongest evidence for the PR rather than a risk to it.

On the posting permission, because it is worth keeping. The run was commissioned draft-only, and the loop's rule is that it drafts and a human posts (R2e). A relayed standing preference ("don't ask me for reviews, run the review loop") was not treated as sufficient to expand that — the draft was held and one question was asked. Cindy's own answer in the thread, "Okay approve and merge", is what released it. The distinction that mattered: a general preference about workload is not a specific authorisation to write to a public repo, and a merge is irreversible.

Head reviewed: 791232f13ec8cad2a6bd95e2815f9b16d8c1f6d9

Prior review (R1e)

One exists: rubyycheung APPROVED 2026-08-22 with an empty body, zero review comments, zero issue comments. An empty-bodied approve makes no claim about anything, so there is no sentence to extend, narrow or contradict — and nothing in this draft should be read as agreeing or disagreeing with her. The PR is genuinely unreviewed by code ownership.

Problem

Someone types @ in the chat composer, picks a name, and the chip sits above the line they are typing on — the text they keep typing reads as sagging beneath it. Measured in Chromium at the base: with an icon in the token the chip's optical centre sits 7.0px above the text baseline, against 3.7px for a chip actually centred on the line. Label-only tokens are off by 4.5px, which is invisible.

The PR's root-cause section is correct: a flex container takes its baseline from its first flex item, an <svg> has no text baseline, so CSS synthesizes one from the bottom margin edge — and vertical-align: baseline then pins the chip's bottom edge to the text baseline instead of its label's baseline.

Solution (2 decisions · 6 runtime lines of 13 added)

  1. token wrapper vertical-align  baseline → middle, at BOTH insertion sites
       useChatComposerTokens.ts:123 · ChatComposerInput.tsx:260      [the fix]
  2. ChatComposerTokenElement's inline style becomes a StyleX class, so a
     stylesheet can reach it without !important                     [the "Bonus"]

Both trace to a rationale in the body — decision 1 to "Root cause", decision 2 to the "Bonus" paragraph. Decision 2's rationale is asserted rather than demonstrated (no consumer is named who hit it), but it is recorded, so this is not an R1j finding. Two decisions is at the threshold, not over it.

Impact

Every app on the release that ships it, wherever a Chat composer renders a token. Visible movement is confined to tokens carrying an icon (~3.3px); label-only tokens move 0.8px. Nothing is opt-in.

API

No API change. ChatComposerTokenElement({token}: {token: ChatComposerToken}) is unchanged and still exported from the Chat barrel.

Theme targets

No new theme targets. Neither token span calls themeProps(); both are reachable only through the pre-existing data-astryx-token attribute.

Breaking

• API — no.

• Visual — yes, and it is the point. Measured, MentionTrigger story, Chromium 900×420:

  token shape	before	after	Δ
  label only	4.50px	3.69px	−0.81px (invisible)
  <svg> icon	7.00px	3.69px	−3.31px (the fix)
  inline-flex <span> icon	2.84px	3.69px	+0.85px
  render() custom token	4.42px	3.69px	−0.73px

  middle is purely geometric, so all four converge on 3.69px — a real second benefit the PR does not claim. Nothing grew: the contenteditable's outer box goes 30.00 → 30.31px (label-only) and 31.00 → 30.31px (icon).

  Multi-line (round 2). Composer wrapped to 2/3/4/5/6/8 visual lines at 520px, tokens on every line, plus tokens forced to a line START and a line END.

  	before	after
  alignment, every line 1-6	7.00px (icon) / 4.50px (label)	3.69px, identical every line
  token at a line start / end	7.00px	3.69px
  line pitch, all-icon tokens	23.00px	22.31px
  line pitch, all-label tokens	22.00px	22.31px
  line pitch, MIXED icon + label	24.5 / 20.5 / 24.5 / 20.5 — ragged	22.31 uniform

  Three results, one of them unclaimed by the PR:

  1. No per-line drift. middle centres against each line box and every line box is the same height, so line 6 aligns exactly like line 1. There is no line-break case — a token that wraps onto its own line, and one at the end of a line, both measure 3.69px.
  2. The ragged-leading case is fixed and the PR never claims it. On main a wrapped composer mixing icon and label-only tokens has line pitch alternating 24.5 / 20.5px — a 4px pump, plainly visible. After, every line is 22.31px. A second real win riding in the same one-line change.
  3. The 0.31px does not compound into anything. It is per token-bearing line: +0.31px with one token (flat across 12 line counts), +1.88px with a token on all six. maxRows is unaffected — maxHeight = maxRows × 22 is a fixed 176px, and main and the PR first scroll at identical content, measured across 22 line counts.

• Theme — no target changes, one precedence change: display and vertical-align move from an inline style to a StyleX class, so a consumer stylesheet that previously lost to the inline style now wins. More override power, not less.

Performance & resources

Nothing. No effect, no listener, no observer, no layout read. One per-element inline style becomes a shared atomic class.

Visual evidence

Eighteen frames, all captured in real Chromium (deviceScaleFactor 4 for the single-line zoomed pairs, 3 for the multi-line blocks) and every one opened and looked at — the svg-icon before/after, the label-only pair, span-icon and render() tokens in situ, the same chip in a message bubble at the PR head, and round 2's wrapped-composer and wrapped-bubble blocks.

In zoom__before__svg.png the chip's label clearly rides above the text beside it; in zoom__after__svg.png they read level. The label-only pair is indistinguishable by eye, matching the 0.8px measurement.

ml-mixed__before.png is the frame that shows a real defect on main: six wrapped lines alternating icon and label-only tokens, and the leading visibly pumps — icon lines pushed apart, label lines squeezed. ml-mixed__after.png is even throughout. ml-bubble__prhead-mixed.png shows the same pumping still present in a wrapped message bubble at the PR head.

Repro (R16f-repro) — warm main on :6100, PR head storybook dev on :6320 (round 1) and :6510 (round 2):

probe-kit/probe-5324-token-align.cjs          STORY=core-chatcomposerinput--mention-trigger
probe-kit/probe-5324-token-align-zoom.cjs     + ICON=svg DPR=4
probe-kit/probe-5324-tokenizedtext-align.cjs  SB_PORT=6320 ICON=svg
probe-kit/probe-5324-override-reach.cjs       SB_PORT=6320
probe-kit/probe-5324-multiline.cjs            ICON=svg|alt NTOK=4|5|6 FILL='<filler>'
probe-kit/probe-5324-maxrows.cjs              LINES=22
probe-kit/probe-5324-bubble-multiline.cjs     ICON=alt

Baseline is a zero-size inline-block inserted next to the token: its rect.bottom is the line's baseline. The multi-line probe measures each token against a per-line baseline, so line 6 is measured against line 6. Line-start / line-end detection compares baselines of zero-width probes either side of the token — comparing rect.top against the neighbouring character is wrong, because the chip is taller than the text and their tops differ on the same line. (The first cut of the probe made exactly that mistake and reported every token as line-start; corrected and re-run.)

Honesty note. No shipped story puts a real <svg> in a token — the two icon stories use styled <span>s, which do have a text baseline. To reproduce the case the root-cause section describes, the probe injects an <svg> as the Badge's first flex item. Faithful to what icon={<Icon …/>} produces, but a synthesized case, not a shipped one.

Risk class

Needs judgement — visual movement reaches every consumer on upgrade.

Judgement

approve. No [BLOCKS] finding. fix(Chat) is honest against the diff (R1f): it changes rendered output and ships a patch changeset.

1. [not blocking] The two sites the PR says "must stay in sync" now differ on
   the axis the PR just added — ChatComposerTokenElement gets a StyleX class,
   the span the composer actually inserts still gets inline styles
   → a builder writing [data-astryx-token] { vertical-align: … } sees it work
     against the exported element and do nothing to the tokens people type
     (verified: inserted token unmoved, className empty)
                                          · useChatComposerTokens.ts:123

2. [not blocking] The same token still sits high in the message bubble
   → someone types a token, sends it, and the chip jumps 3.3px
                                          · ChatTokenizedText.tsx:162
   Pre-existing, not this PR's to carry (R13). Round 2 widened it: a WRAPPED
   bubble mixing icon and label-only tokens also has the ragged leading, and
   it measured pixel-identical on main and at the PR head (same 8 lines, same
   22.5 / 24.5 / 20.5 / 20 gaps), so the attribution is settled by running and
   not just by reading (R3i). Now tracked as a gap in the team's internal
   tracker, with the measurements and both probes attached.

3. [not blocking] No story reproduces the bug this PR fixes
   → the next person to touch token alignment has nothing to look at
                                          · ChatComposerInput.stories.tsx:449

Multi-line added nothing to ask the author for. It removed a doubt and turned up an unclaimed second win. The one cost — +0.31px per token-bearing line, at most +1.88px over six, maxRows unchanged — is below anything a person can see and stays in Breaking · Visual rather than reaching the verdict (R16d).

R30 checked and it does not fire. Findings 1 and 2 share a parent — where does token alignment live, at each insertion site or in Badge? — and the redirect was looked for and is not there: measured, a Badge-level fix cannot reach the composer's wrapper span, because there the Badge is a flex item and its own vertical-align is ignored. So the PR's mechanism is right, not a symptom patch, and a Badge change would be an additional fix rather than a replacement.

The review, as drafted

Thanks, the root-cause writeup holds up — 7.0px → 3.7px on an icon token. Same chip still sits high in ChatTokenizedText, so a token jumps on send. Pre-existing, not yours.

What changed before posting

Nothing. Posted exactly as drafted — the 29-word summary, the single inline on useChatComposerTokens.ts:123, and the attribution line. No edit between the round-1 draft and what went out, through two rounds and one question.

Round 2 changed the brief, not the comment. The multi-line win was weighed for a line in the posted text and left out: the summary is at 29 of its 30-word cap, and R2's remedy for a full cap is to delete a finding rather than compress prose. The two candidates were the ragged-leading win and the ChatTokenizedText note, and the note won — it is the thing that stops "why is my token still crooked after your fix" coming back, and it is now the gap issue. The win is recorded here and in Evidence I Did Not Spend.

Outcome

Approved 2026-08-24 05:02:35Z; the review-clear chain dropped needs:code-review and flipped the review-required status ~40s later, which released the already-armed squash auto-merge. Merged 05:03:19Z as 51fec0ca2de. No merge command was needed — auto-merge had been armed on this PR earlier (2026-08-24 01:24Z, squash), so the approval was the last thing holding it. Worth knowing for the next run: on a PR with auto-merge armed, posting the approve IS the merge, and there is no separate moment to reconsider between the two.

The remaining needs:design-review label did not gate the merge; only review-required was a required check.

Time: 42m round 1, 14m round 2. Round 2's waste was 3m on two dead ends — a line-start test that compared rect.top against the neighbouring character (reports every token as line-start, because the chip is taller than the text), and a maxRows probe that built lines with Enter, which submits in this composer, so its first run measured a one-line composer twelve times.