Review 5558

Review #5558 — Badge long-label containment

freddymeta · collaborator · OPEN · view on GitHub

PR

#5558 fix(Badge): a long label no longer escapes its container by freddymeta (bucket: collaborator — write access, treated as internal)

HEAD REVIEWED

1733f9bf784f7a491409e0b59c1d789b83d8ac03 <- every claim below was verified at this commit

VERSIONS

LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.12

Read sources:

• Review kit source of truth on the M5: loop 1.5.0; audit rubric 1.12.
• Fresh fork-wiki clone at 665b3d1686c92ed6a97d660418705d5b7e659931: Rulings, Review-Presentation, Evidence-Templates, and Measurement-Harness.
• Fresh public-wiki clone at a43e2bbc27c8cd56c747462f6c1a20d0c90e5379: Component-Audit-Rubric, Design-Conventions, API-Conventions, and Theming-Infrastructure.
• Main-branch rubric at d4d4fc58b0bd2e21ad7ab23a492edcb4aafa4bc5: Copilot/path instructions, review gate, sample comment, contribution guide, and owner files.

LANE

LANE: full WHY: Intended responsive visual change; layout/CSS diff; manual frames required; prior CHANGES_REQUESTED review from rubyycheung remains unresolved; one decisive visual pair plus one control pair captured with sensor receipts.

STEP 0

Clear. The exact head is in facebook/astryx; freddymeta is a collaborator with write permission. The final PR diff contains only one changeset, one Badge story, and Badge.tsx. No dependency, lockfile, workflow, install hook, executable script, credential, environment, or build-network path changed. A worktree-local node_modules symlink was added and removed inside the commit history and is absent from the reviewed head.

PROBLEM

WHY 1: The author wants Badge labels that are wider than their available space to stop escaping their container. WHY 2: That escape covers neighboring content in tight places like hover cards and fixed table cells, so a person cannot reliably read either the status badge or the content beside it. WHY 3: Badge exists to make status/category information scannable in dense UI; a badge that bleeds over adjacent content turns that signal into visual noise.

USER-FACING PROBLEM: A person scanning a dense row or hover card sees a long status badge paint over nearby content instead of fitting inside the space the layout gave it.

VERDICT: clear

SOLUTION

Badge now lets the pill be no wider than its container. The label text, rather than the icon or the surrounding layout, becomes the part that gives way: it can shrink and clip with an ellipsis while the badge stays one line tall. The responsibility stays inside Badge, so every caller in a tight cell gets the containment behavior without repeating the CSS recipe.

SOLUTION (2 decisions · ~19 runtime lines of +117/-1)

1. The badge root gets maxWidth: '100%' and minWidth: 0 so the component can shrink inside flex/grid/table constraints — serves the overflow problem.
2. The label is wrapped in an inner span with overflow: hidden, textOverflow: 'ellipsis', and minWidth: 0 so text clips instead of pushing the pill wider — serves the overflow problem, but creates the full-label recovery gap below.

VERDICT: note — containment is the right shape, but the new truncation state needs a recovery path.

ARCHITECTURE

OWNER: Badge owns its one-line status/category pill geometry. TIER 1: none. TIER 2: status/category display; no icon-system or size-system changes. SEAMS: root xstyle/className/style passthrough, label: ReactNode, optional icon, table/flex/grid/narrow/RTL containers. BEHAVIOR UNIT: inline CSS — no state machine, no hook, no Effect; the boundary is testable through rendered geometry.

seam	driven result
fixed-layout table cell	works for containment: overlap with the next cell fell 60.11px → 0px.
constrained block	works: right spill fell 60.11px → 0px.
long unbroken text	works: right spill fell 145.88px → 0px.
flex row with sibling	works: after width shrinks to 132.05px inside a 170px row; no overlap.
grid track	works: overflowX fell 20px → 0px.
RTL via Storybook global	works for containment: left spill fell 60.11px → 0px after preserving the RTL provider direction.
icon + truncation story	after path keeps the icon visible and shrinks the label; title/recovery still absent.

The implementation lives in Badge and does not add a new owner or extension mechanism. xstyle remains after base styles, so callers can still override layout deliberately.

VERDICT: clear

IMPACT

Everyone using Badge gets safer containment in narrow, table, flex, and RTL contexts: the badge no longer paints over adjacent content. The new visible state is that long labels are intentionally clipped; for a meaningful status like “Awaiting security review,” a sighted user can now see only the truncated label and has no component-provided path back to the full text. Existing short labels are unchanged in the control frame.

VERDICT: BLOCKS — clipped status text has no recovery path.

API

No public API change: BadgeProps still exposes variant?: BadgeVariant, required label: ReactNode, optional icon?: ReactNode, inherited xstyle/className/style/data-*, and no new export. BaseProps omits native title at packages/core/src/BaseProps.ts:26, so a plain string-label caller cannot pass a root title through today; a custom ReactNode label can still carry its own title.

	change	public?	class	doc'd?	verdict
—	no API change	—	—	—	ok

OSSIFICATION: nothing new ossifies. If the fix chooses an internal title fallback for string/number labels, that can stay server-safe and does not require adding a public prop.

VERDICT: clear

THEMING

No theme target, token, variant, or CSS variable is added or removed. The root target remains astryx-badge with variant; maxWidth: '100%' and minWidth: 0 are large layout constraints allowed by rubric T2b, not tokenized spacing. No raw colors are added in component styling; the story-only outline uses var(--color-border).

VERDICT: clear

BREAKING

BEHAVIOR: Badge still renders the same required label and optional icon; no state/default/condition/event path changed. I also checked label={null}, label={false}, and label="" through Storybook args: the empty badge stayed 16px wide on both main and head without an icon; the remaining nullable-label+icon gap is not part of the documented required-label contract. API: no existing call site stops compiling; no public prop/export/signature/default changed. VISUAL: yes, intentionally — long labels clip instead of overflowing; control label Active is 51.98px before and after; no outer dimension grew. THEME: no theme target/token/var/override changed.

VERDICT: note — intended visual behavior changes, with no breaking API/theme change.

PERFORMANCE & RESOURCES

EFFECTS: zero added/changed/moved Effects. RENDER: one extra static span around the label; no state, subscriptions, memoized return, or render-loop work. LISTENERS/OBSERVERS: none. LAYOUT: CSS-only sizing/clipping; no layout reads, ResizeObserver, getBoundingClientRect, or build-time/runtime measurement in production. BUNDLE: no dependency change and no runtime import added.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: The PR intentionally changes layout/rendered pixels for a core component's overflow state, and the new long-label story does not exist on main.

The decisive pair was opened and inspected. The before badge paints 60.11px into the adjacent cell; the after badge ends inside its 100px cell with an ellipsis and 0px overlap.

Sensor	Before	After
Build	main d4d4fc58b0bd2e21ad7ab23a492edcb4aafa4bc5	head 1733f9bf784f7a491409e0b59c1d789b83d8ac03
Receipt	JSON	JSON
Story	core-badge--default	same
Theme / mode / direction	neutral / light / LTR	same
Viewport / media	420×220 @1; forced colors off; reduced motion off; fine pointer; hover	same
Semantic state	table-long-100; “Awaiting security review”	same
Geometry	badge 156.11×20; adjacent overlap 60.11px	badge 92×20; adjacent overlap 0px
Settled / errors	fonts loaded; 0 animations; no Storybook error	same

Exact main — badge overlaps the next cell	Exact head — badge stays contained

The one needed control stays unchanged: the short Active badge is 51.98×20 before and after.

Exact main — short-label control	Exact head — short-label control

Control receipts: before · after.

Measured class: table overlap 60.11px → 0px; constrained block spill 60.11px → 0px; long unbroken spill 145.88px → 0px; grid overflowX 20px → 0px; RTL left spill 60.11px → 0px; auto-sized long label unchanged at 156.11px; short control unchanged at 51.98px. The reproducible probe, manifest, and exact arm diff are banked.

VERDICT: clear — containment works in the measured layout cases; the missing full-label recovery is a separate blocking UX/a11y finding.

A11Y & I18N

pr-a11y ran and passed on the exact head; .github/a11y-baseline.json is not changed. No production user-visible string or AT-facing string is added; the story text is Storybook-only. RTL was driven through the Storybook direction global, and the constrained RTL case stopped spilling after the fix.

The truncation recovery path is missing: in the PR's own Long labels in tight space story, both long badges are truncated (labelClientWidth 76/100px vs labelScrollWidth 140px), while the badge and label have no title, no focusability (tabIndex = -1), and no visible tooltip carrying the full text after hover. A sighted user who cannot infer the hidden part of “Awaiting security review” cannot recover it from the component.

VERDICT: BLOCKS — truncated user-facing status text has no recovery path.

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	note — containment is right; recovery path missing
ARCHITECTURE	clear
IMPACT	BLOCKS — clipped status text has no recovery path
API	clear
THEMING	clear
BREAKING	note — intended visual behavior changes; no API/theme break
PERFORMANCE	clear
VISUAL	clear
A11Y & I18N	BLOCKS — truncated user-facing status text has no recovery path

GOAL: met — the overflow goal is proven: table overlap 60.11px → 0px, constrained spill 60.11px → 0px, long-unbroken spill 145.88px → 0px, and RTL left spill 60.11px → 0px. DISPOSITION: missing full-label recovery → blocks now; this is the same unresolved hold rubyycheung filed in the existing CHANGES_REQUESTED review, and no later push addressed it. ADVICE: proven remedy / bounded direction — prior review names the accepted minimum fallback (title), BaseProps currently omits native title, and the PR's own body says a measured tooltip would be a separate client-component tradeoff; add a server-safe full-label fallback for string/number labels or another recovery path with equivalent evidence. AUTHOR CAN PROCEED: yes — acceptance criteria: when a string/number Badge label is visually truncated, a sighted user can recover the full label at least on hover without making Badge a client component; update the story/comment so it no longer promises a hover behavior that is absent. WORST OUTCOME: “A sighted user who cannot infer the hidden part of ‘Awaiting security review’ cannot recover it from the component.” → request changes

JUDGEMENT NEEDED: none — defect fix against the existing Badge contract, and the prior design-owner review already asked for this exact recovery path.

request changes

1. [BLOCKS] Truncated string labels have no recovery path → a sighted user scanning a tight table sees a clipped status and cannot hover/focus/tap to recover the full label · packages/core/src/Badge/Badge.tsx:195

REVIEW

Thanks, this fixes the spill. I agree with Ruby’s hold though: the status is now clipped, but the rendered badge has no title, tooltip, or other path for a sighted user to recover the full label. Can you add a server-safe full-label fallback for string/number labels and keep the richer only-when-truncated tooltip as its own PR if it needs measurement?

[Reviewed by Robohands]

Public review length: 61 words (request-changes cap: 150).

INLINE

• packages/core/src/Badge/Badge.tsx:195 — Might need a full-label fallback here; truncated text has no title/tooltip.

EVIDENCE I DID NOT SPEND

• apps/storybook/stories/Badge.stories.tsx:96-98 says hover reveals full text, but this does not appear in the rendered story; the main issue is the component behavior at Badge.tsx:195.
• The long explanatory comments in Badge.tsx:49-62 and the 36-line changeset are heavier than usual, but I did not spend review words there because the truncation recovery path blocks first.
• The transient node_modules symlink appears only in commit history and is absent from the final file list/head.

TIME

TIME total 34m setup 8m worktree + fast-install + one Storybook-config build + Storybook dev; warm main reused after refresh: yes reading 11m kit, critic, fresh wiki pages, main rubric files, PR body, prior review, diff measuring 8m one probe, 2 screenshot pairs, layout/RTL/title semantics; 1 re-measure after RTL sensor fix writing 7m draft + R1-R15 self-critique + rewrite + final critique waste 6m parallel test/CI command queue timed out; image contact sheet attempt failed because PIL is unavailable

WHAT I COULD NOT VERIFY

• Safari/WebKit rendering; the M5 harness only supports Chromium for Playwright.
• The read tool returned image metadata instead of inline pixels, so I also inspected the frame files via sensor receipts, DOM geometry, and a pixel-classified text render.

EVIDENCE AND TESTS

• Focused Vitest: Badge 8/8 passed (log).
• check-use-client and the changeset gate passed (log).
• Exact-head CI: build, test, lint, pr-a11y, pr-rtl, and pr-visual pass; review-required remains pending (checks · actions · statuses).
• Chromium frames: passing captureWithSensors receipts for the decisive pair and short-label control.
• Layout probe: fixed table, block, unbroken text, flex, grid, RTL, and icon seams all driven before/after.
• Recovery probe: truncated labels have title = null, tabIndex = -1, and no visible tooltip after hover (log).
• Full reproducibility bundle: assets/pr-5558.

CRITIC

DRAFT: request changes for the missing full-label recovery path. CRITIQUE 1 (R1-R15): failed R1e because it did not explicitly reconcile rubyycheung's existing CHANGES_REQUESTED; failed R15 because the draft referred to visual evidence without listing frame/receipt paths; R2 was close but had removable background. No invented findings under R3/R5/R13/R14. REWRITE: moved prior-review reconciliation into JUDGEMENT and the public first sentence, cut background, attached exact frame/receipt paths, kept one line-anchored inline. CRITIQUE 2 (R1-R15): passes R1/R1g collaborator handling, R1e prior-review reconciliation, R2 word cap (61-word public body before signature; 10-word inline), R3/R4/R5 problem/remedy shape, R12 no rubric ids/internal links/Discord, R13 no inherited debt charged, R14 anchors verified, R15 evidence banked. Remaining posting note: if this is publicly posted with visual frames, the parent must publish the PNGs to a durable public location first.

WHAT CHANGED BEFORE POSTING

Nothing was posted to the PR. The proposed public text is archived exactly as drafted; no GitHub review, comment, approval, request-changes action, merge, or contributor-branch push was made.

---

Round 2 — re-review at current head e116dacfb4f381e7985f6b8b83baad5a851c9d63

PR

#5558 fix(Badge): a long label no longer escapes its container by freddymeta (bucket: collaborator — write access, treated as internal)

HEAD REVIEWED

e116dacfb4f381e7985f6b8b83baad5a851c9d63 — exact current head requested.

VERSIONS

LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: Re-review after standing request-changes; behavior is visible and the request concerns recovery paths across SSR, hover, touch, keyboard, string/number labels, and rich labels.

PRIOR REVIEW RECONCILIATION

Ruby requested a recoverable full label after truncation, ideally tooltip on hover/focus with touch alternative, and said native title was the minimum fallback. Cindy then narrowed our standing ask to a server-safe fallback for string/number labels, leaving richer only-when-truncated tooltip work for its own PR if measurement is needed.

Round 2 satisfies the narrowed ask: string and number labels now render title on the server and after hydration. It deliberately does not solve the broader focus/touch/rich-label tooltip; that remaining work is tracked publicly in #5585.

PROBLEM

WHY 1: The first reviewed head fixed Badge overflow by clipping over-wide labels, but clipped status text needed some path back to the full value. WHY 2: Without recovery, a sighted user scanning dense UI could see only an ellipsis and lose the status/category meaning. WHY 3: Badge exists to make compact statuses scannable; truncation is acceptable only if the full status is still recoverable for the supported path. USER-FACING PROBLEM: A person seeing a truncated string/number Badge label needed a server-safe way to recover the full label without turning Badge into a client component. PROBLEM SEVERITY: harmful friction — the status remains visible in part, but the hidden tail can carry the distinguishing information.

VERDICT: clear

SOLUTION

Badge derives a plain text fallback only when label is a non-empty string or a number. That value is written to the root title, so server-rendered markup and hydrated markup carry the same browser-native hover fallback. Rich ReactNode labels are not flattened, because doing so would guess at what nested markup or icons mean.

SOLUTION (1 decision · ~21 runtime lines of +74/-10 since reviewed head)

1. Add a labelTitle branch for string/number labels and set it on the Badge root; document and test the remaining richer tooltip gap via #5585.

BURDEN: low — pure render-time derivation, no hook, state, Effect, listener, observer, measurement, public API, or client boundary. BURDEN MATCH: proportionate — it is the minimum server-safe fallback the standing review asked for.

VERDICT: clear

ARCHITECTURE

OWNER: Badge owns the fallback for its own clipped label. TIER 1: none. TIER 2: status/category display; no shared primitive or behavior owner changed. SEAMS: SSR, hydration, string labels, number labels, rich labels, pointer hover, keyboard, touch. BEHAVIOR UNIT: inline pure derivation — no independent behavior unit needed.

seam	driven result
SSR/hydration	one-off Vitest rendered and hydrated string, number, and rich labels with no hydration warning; string/number SSR contained title, rich did not
string label	clipped in Chromium and root title="Awaiting security review"
number label	clipped in Chromium and root title="1234567890123456"
rich label	clipped in Chromium and root title=null; this is deliberate and tracked in #5585
pointer hover	the hovered clipped string badge has the full text in the DOM title; no custom tooltip is introduced
keyboard	Badge remains unfocusable (tabIndex=-1); no keyboard recovery path in this PR
touch	coarse-pointer tap does not show any touch-visible recovery UI in this PR

VERDICT: note — the minimum fallback works; the remaining full tooltip behavior is explicitly separate.

IMPACT

String/number Badge labels that are clipped now have a browser-native hover fallback without changing server renderability. Keyboard, touch, and rich-node labels still cannot recover the full clipped label from this PR alone; the author opened #5585 for that client-component/focusable-tooltip tradeoff.

VERDICT: note — accepted follow-up, because the previous Cindy-scoped ask was the server-safe string/number fallback.

API

No public prop, export, default, or accepted value changes. BadgeProps.label remains ReactNode; native title remains omitted from BaseProps, so this is component-owned fallback behavior rather than new caller API.

VERDICT: clear

THEMING

No theme target, token, CSS variable, or component override contract changed in the round-two delta. The new title attribute is not theme surface.

VERDICT: clear

BREAKING

BEHAVIOR: yes, string/number labels now expose a native title; this is additive fallback behavior. Rich labels, keyboard, and touch remain as before this round. API: no existing typed call site stops compiling or changes required props. VISUAL: no pixel change in the round-two code delta; current-head frames match the clipped visual state from round 1. THEME: no theme target/token/override change.

VERDICT: clear

PERFORMANCE & RESOURCES

EFFECTS: zero added/changed/moved Effects. RENDER: one typeof branch and optional string conversion; no state or render-loop work. LISTENERS/OBSERVERS: none. LAYOUT: no measurement or layout reads. BUNDLE: no dependency or import added.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: The acceptance behavior is visible/clipped text plus recovery affordance state; the exact head also changed docs/story prose after a visual bug review.

Actual PR story at current head:

Current LongLabels story

Review harness matrix for string, number, rich, and short labels:

Desktop recovery matrix	Keyboard after Tab	Touch after tap

Sensor summary: assets/pr-5558/round2/sensor-summary.md. Detailed matrix: assets/pr-5558/round2/recovery-detail.json.

VERDICT: clear — the current-head visual state is stable; the no-keyboard/no-touch/no-rich recovery is a known follow-up, not a hidden visual regression.

A11Y & I18N

pr-a11y and pr-rtl ran green on exact head. The new production fallback adds no localized string. Browser-driven checks: string/number clipped labels have title; rich clipped labels do not; all badges remain non-focusable; coarse-pointer tap does not reveal a recovery UI. That matches the intended split between this PR and #5585.

VERDICT: note — keyboard/touch/rich recovery remains absent and tracked.

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	clear
ARCHITECTURE	note — minimum fallback only
IMPACT	note — broader recovery tracked in #5585
API	clear
THEMING	clear
BREAKING	clear
PERFORMANCE	clear
VISUAL	clear
A11Y & I18N	note — keyboard/touch/rich recovery remains absent and tracked

GOAL: met — the standing Cindy-scoped ask was a server-safe full-label fallback for string/number labels; SSR/hydration and Chromium both prove that path. DISPOSITION: focus/touch/rich-label recovery → follow-up #5585; acceptable intermediate because the current PR keeps Badge server-renderable and solves the minimum fallback requested after round 1. ADVICE: omitted — the follow-up issue already names the client tooltip/focus/touch tradeoff. AUTHOR CAN PROCEED: yes — no open blocker remains for this scoped PR; posting approval would clear the standing review-required state once GitHub records it. WORST OUTCOME: “Keyboard, touch, and rich-node labels still cannot recover the full clipped label from this PR alone” → approve with tracked follow-up, not request-changes, because that gap is explicitly out of this narrowed server-safe fallback round.

JUDGEMENT NEEDED: none — defect fix against existing Badge behavior; no public API/design decision remains in this PR.

approve

REVIEW

Thanks, this covers the server-safe string/number fallback I asked for. I’d take this and leave focus/touch/rich-label recovery to #5585.

[Reviewed by Robohands]

Public review length: 21 words including attribution.

INLINE

None.

EVIDENCE I DID NOT SPEND

• The new source/test comments are heavier than usual, but I did not spend public review words there because the prior hold is addressed and the comments explain a deliberate SSR tradeoff.
• title is set even when a string label fits; the author records that in #5585, and it is not user-breaking.
• The author cites BaseTable; the actual title precedent is BaseTable header content at BaseTable.tsx:461-464 and Table.test.tsx:1203-1205.

TIME

TIME total 57m setup 17m worktree + fast install + build package + Storybook dev; warm main reused conceptually, but no before rebuild was needed for round-two delta reading 18m kit, critic, main rubrics, public wiki pages, PR/GitHub history, prior Review-5558 history measuring 14m focused tests, SSR/hydration test, Chromium recovery matrix, touch/keyboard probes, screenshots writing 8m presentation + critic self-grade + wiki update waste 7m first pnpm -F @astryxdesign/core test -- Badge.test.tsx invocation expanded into broad workspace CLI tests; reran with direct Vitest filter

WHAT I COULD NOT VERIFY

• Native browser tooltip pixels themselves are not capturable in headless Chromium; I verified the DOM title fallback and user paths around it.
• Safari/WebKit behavior; this review's Playwright harness supports Chromium only.

TESTS AND CHECKS

• vitest run packages/core/src/Badge/Badge.test.tsx: 12 passed.
• One-off SSR/hydration Vitest: string, number, and rich labels, 3 passed.
• git diff --check on changed files: passed.
• check:changesets: passed.
• Exact-head CI from GitHub: check-scope, lint, docsite-test, check-components, test, build-storybook, build-sandbox, theme-layers, pr-a11y, pr-visual, pr-rtl, build, CLA, and Vercel all green; review-required pending only because code review is outstanding.

WHAT CHANGED BEFORE POSTING

Nothing was posted to the PR. The proposed public text is archived exactly as drafted; no GitHub review, comment, approval, request-changes action, merge, reaction, metadata edit, or contributor-branch push was made.