Skip to content

Review 5324

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

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

athz · community · OPEN · view on GitHub

Verdict: approve — not posted. Drafted 2026-08-23; awaiting Cindy's read. review-required is still pending ("core change, community contribution") and she is a requested reviewer.

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

  • 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

Seven pairs, all captured in real Chromium (deviceScaleFactor 4 for the zoomed pairs) and every one opened and looked at — the svg-icon before/after, the label-only pair, span-icon and render() tokens in situ, and the same chip in a message bubble at the PR head.

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.

Repro (R16f-repro) — warm main on :6100, PR head storybook dev on :6320:

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

Baseline is a zero-size inline-block inserted next to the token: its rect.bottom is the line's baseline.

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

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

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 yet — not posted. Held for Cindy's read.

Clone this wiki locally