-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5402
cixzhang (ours) · MERGED 98ea5ba · view on GitHub
Verdict: merge — self-gated, six passes, clean at 6.
Head reviewed: c59d312a40b, against parent 51fec0ca2de (main with #5324).
This is the other half of #5324. That review named the bubble twice as a non-blocking finding and never named the consequence of merging without it — which is the case that produced the brief's "what does landing this newly expose?" rule.
You type @, pick a name, and the chip sits level with what you are typing. You send,
and the same chip lifts off the line in the bubble, riding above the words either side
of it. One message old: before #5324 merged that morning both halves were wrong the same
way, so nobody could see it.
Root cause: Badge is inline-flex with a fixed height; a flex box takes its baseline
from its first flex item; an icon has none, so the synthesized baseline pins the chip's
bottom to the line. ChatTokenizedText.tsx:162 put that Badge into the text flow with
nothing aligning it.
Real round trip (type @cin, click the suggestion, press Enter), Chromium 820×900:
| token | composer | bubble before | bubble after |
|---|---|---|---|
| with an icon | 3.69px above baseline | 8.00px | 3.69px |
| label only | 3.69px | 4.50px | 3.69px |
custom render()
|
3.69px | 4.43px | 3.69px |
1. ChatTokenizedText wraps EVERY token — badge and custom render() alike — in
the inline-flex / vertical-align: middle box the composer already uses
The rest is evidence: a patch changeset, an IconAndCustomTokens story, and one line
in the file's existing SYNC: list.
Where the fix belongs — three homes, priced. The class is a fixed-height inline-flex chip in a text flow, and core has two members.
| home | reach | cost |
|---|---|---|
Badge (what #5324's review suggested) |
bubble only | inert where a Badge is a flex child (most places), moves every consumer's badge in a table cell or paragraph, and misses Citation, which is not a Badge |
| a shared chip-in-text style | both members | still 2 of 3 chat sites — useChatComposerTokens.ts:122-123 writes span.style imperatively and cannot take a StyleX style; and it moves Citation, which has no stated problem here |
| the site that puts the chip into text ← taken | the reported defect, both token kinds | one span per token; the shared definition stays deferred |
Citation is the evidence that decided it: measured at the identical 8.00px with an icon, and it is not a Badge.
End users of every app on the next release that renders chat tokens in a bubble. The name you sent stops jumping when the message lands. Visible only on tokens carrying an icon; label-only and custom tokens move under a pixel and nobody sees those. Builders: nothing — no API, no prop, no theme target.
Newly exposed: nothing on screen. Citation is rendered only by Markdown and Chat
never renders one (a user message renders ChatTokenizedText, an assistant message
renders Markdown), so no sentence gains an asymmetry. Citation's own chip is still
broken, not newly visible — filed separately.
No API change. ChatTokenizedText props, type and barrel export all unchanged.
No new theme targets. The wrapper carries StyleX atomic classes and no themeProps().
- API — no.
- Visual — yes, the point. An icon-token bubble gets shorter, 67.57 → 65.31px; a label-only bubble grows 0.81px, 44.50 → 45.31px. Rendered with neighbours in a message list: the bubble reflows by that 0.81px, nothing else moves, no scrollbar at 820×900. Composer untouched at 3.69px both sides.
-
Theme — one DOM-shape change. The Badge is no longer a direct child of the
tokenized-text root, so
.astryx-chat-tokenized-text > .astryx-badgestops matching:outline-style: solidat the parent commit,noneat this head; the descendant form applies at both. The composer has had this wrapper since before #5324, so cross-surface styling already used the descendant form.
No effect, state, subscription, dep array, listener, observer, layout read or dependency. +1 DOM node per token rendered in a bubble; worst case in the repo is 1-3 tokens. Not benchmarked, and not called negligible.
- round trip, icon token, 820×900 before/after before-icon__1-composer.png ·
before-icon__2-bubble.png · after-icon__2-bubble.png
- icon + custom token, 500px before/after story-before__icon-custom.png · story-after__icon-custom.png
- label only (sub-perceptual) before/after story-before__label.png · story-after__label.png
- 200% text zoom, fixed-height chip before/after matrix-before__zoom200.png · matrix-after__zoom200.png
- narrow 260px, line wraps before/after matrix-before__narrow260.png · matrix-after__narrow260.png
- chip wider than a 150px bubble numbers only identical both sides: 297.56px, overflows 163.56px
- RTL numbers only 3.69px, both tokens
All opened with the read tool; five published on the fork's assets/pr-5402 branch and
embedded in the PR body. Dropped rows, named: dark mode and forced-colors (no colour
changes), a second engine (vertical-align is not engine-specific), other consumers
(there are none). Reproduce with probe-kit/probe-chat-token-send-jump.cjs (drives the
real send) and probe-kit/probe-chat-bubble-token-align.cjs (FONT=32, WIDTH=260),
both against storybook dev on :6550.
Not the unattended class: a label-only bubble grew 0.81px, so this is a visual change reviewed with frames.
merge — nothing blocking.
1. [not blocking] `.astryx-chat-tokenized-text > .astryx-badge` stops matching
→ a consumer who styled bubble tokens with a direct-child selector loses that
rule on upgrade · ChatTokenizedText.tsx:164
2. [not blocking] +1 DOM node per token, unbenchmarked
→ we pay one span per token, inseparable from the fix · ChatTokenizedText.tsx:164
3. [not blocking] Citation's label chip has the same defect, 8.00px with an icon
→ someone reading an answer sees the source chip riding above the sentence it
cites; unchanged by this PR · Citation.tsx:73
No unit test added: jsdom does no layout, so an assertion on a style string passes whether or not the chip is aligned. The browser probe is the test; the new story is the durable artifact.
GitHub refuses a self-approval, so nothing was posted as a review. One 29-word note went on the PR:
A token stops jumping when you send it: composer and bubble both sit 3.69px above the baseline. The badge is now wrapped, so
.astryx-chat-tokenized-text > .astryx-badgemust become descendant.[Reviewed by Robohands]
Six passes; artifacts {review-root}/review-artifacts/pr-5402/gate-1.md … gate-6.md.
Gate 1 → 2 was the substantive round and it was right on all nine points. The draft's PROBLEM table had no after column — the one number the PR exists to produce. Its JUDGEMENT asserted "no performance or resource regression" three lines under its own PERFORMANCE slot saying "I did not benchmark it": the verdict laundering its own hedge. Its evidence was four frames at one viewport rather than a matrix — the 200% zoom and narrow-wrap rows were measured only because gate 1 demanded them. And it never asked whether a Citation chip and a token can share a line (they cannot, and saying so with a grep is worth more than the footnote it replaced).
Gates 3 → 6 were all one paragraph — the note destined for the public PR. It arrived
at 86 words against a 30-word cap, opening with "Gate pass:", the loop's own shorthand.
Round 5 cut it to what the run believed was 35 words and was 41. What ended it was
printing wc -w instead of asserting a count.
Two lessons worth carrying:
- A required fix answered by adding a paragraph makes the document longer. Gate 2 grew 152 words under an explicit instruction to trim. Each fix should name the slot it lands in and the words it displaces.
- Text destined for the PR is graded at the caps the moment it is written, not when it is posted. Three passes went on prose that was drafted as an artifact line.