Review 4259
#4259 feat(lab): add LineClamp — clamp arbitrary content to N lines by HelloOjasMutreja (bucket: contributor)
None. The expected head df979caa1cf1229cf8be6c8913dc87530f1036ca had moved to 48566e359d3e335faec7698f28f3b3ee944ae1ad before gate 1, so the review failed closed.
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full WHY: A visual and interactive full review was requested, but the exact-head precondition failed before evidence collection.
Not looked at; no commit was reviewed.
VERDICT: not looked at — the PR moved before review began
Not looked at; no source or runtime evidence was evaluated.
VERDICT: not looked at — the PR moved before review began
Not looked at; no ownership or composition seam was evaluated.
VERDICT: not looked at — the PR moved before review began
Not looked at; no user or builder impact was evaluated.
VERDICT: not looked at — the PR moved before review began
Not looked at; no API claim was evaluated.
VERDICT: not looked at — the PR moved before review began
Not looked at; no theme target or token claim was evaluated.
VERDICT: not looked at — the PR moved before review began
BEHAVIOR: not looked at API: not looked at VISUAL: not looked at THEME: not looked at
VERDICT: not looked at — the PR moved before review began
EFFECTS: not looked at RENDER: not looked at LISTENERS/OBSERVERS: not looked at LAYOUT: not looked at BUNDLE: not looked at
VERDICT: not looked at — the PR moved before review began
VISUAL CHECK: manual frames required WHY: This was requested as a visual and interactive full review; no frames were captured because the exact-head gate failed first.
VERDICT: not looked at — no exact-head browser evidence was collected
REMEDY SEARCH: not triggered — no proven visual defect
Not looked at; no browser interaction or exact-head CI result was evaluated.
VERDICT: not looked at — the PR moved before review began
| slot | verdict |
|---|---|
| PROBLEM | not looked at |
| SOLUTION | not looked at |
| ARCHITECTURE | not looked at |
| IMPACT | not looked at |
| API | not looked at |
| THEMING | not looked at |
| BREAKING | not looked at |
| PERFORMANCE | not looked at |
| VISUAL | not looked at |
| A11Y & I18N | not looked at |
GOAL: not met — the exact-head review did not run
DISPOSITION: head mismatch → no PR judgment; a fresh review must pin the current head
ADVICE: omitted — there is no reviewed evidence
AUTHOR CAN PROCEED: no review guidance was produced
WORST OUTCOME: none found — no evidence was evaluated
JUDGEMENT NEEDED: none identified because the review did not begin
No PR verdict.
[not written — head changed before review]
None.
- The expected head and live head differed at gate 1.
- No checkout, install, build, browser, test, or PR write occurred.
TIME total 1m
- setup/rules: 1m
- install/build/server: 0m
- browser/a11y evidence: 0m
- focused tests: 0m
- code/history analysis: 0m
- remote CI wait: 0m
- Every substantive claim about the PR, because the exact-head precondition failed.
Not posted. The exact-head gate failed before review evidence was collected.
#4259 feat(lab): add LineClamp — clamp arbitrary content to N lines by HelloOjasMutreja (bucket: contributor)
48566e359d3e335faec7698f28f3b3ee944ae1ad <- every claim below was verified at this commit
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full WHY: This adds a component, public canary API, a theme target, pooled observation, conditional Tooltip behavior, and intended rendered output; the user also requested the visual/interactive full path.
WHY 1: Text.maxLines clamps only Text's own content, so a builder cannot apply the same behavior to a composed block.
WHY 2: A builder showing a compact rich-content preview must discard composition or recreate overflow detection and disclosure.
WHY 3: Reimplemented clamps drift in what they reveal and how people recover the hidden content.
USER-FACING PROBLEM: A builder presenting mixed rich content has no Astryx primitive that keeps the preview compact while preserving its composition. PROBLEM SEVERITY: missing capability — the linked issue demonstrates a design-system need and asks for an Astryx lab path.
VERDICT: clear
The wrapper applies the browser's multi-line clamp to one root around arbitrary children. A shared observer measures whether that root actually overflows. When it does, the existing Tooltip system can expose the full extracted text, keeping measurement and disclosure with their existing owners.
SOLUTION (3 decisions · ~142 runtime lines of 428 total)
- Add a canary-only
LineClampwrapper for arbitrary children, with a selectable semantic root — serves the composed-content gap. - Require
maxLinesand reuseuseTruncationplus CSS line clamping — serves the compact-preview behavior. - Default
hasTooltipon and accept a placement value — serves disclosure of clipped text.
BURDEN: medium — one public lab component, pooled ResizeObserver measurement, two state updates in the reused hook, and Tooltip listeners/layer lifecycle only while overflow exists. BURDEN MATCH: proportionate — the issue demonstrates an existing system need and the implementation reuses the current measurement and Tooltip owners.
VERDICT: clear
OWNER: LineClamp owns the clamp boundary; useTruncation owns overflow measurement; Tooltip owns disclosure.
TIER 1: themeProps, useTruncation, Tooltip, mergeProps; all reused.
TIER 2: none.
SEAMS: arbitrary inline children, short versus overflowing content, hasTooltip={false}, touch, consumer styles, selectable root, and consumer ref.
BEHAVIOR UNIT: inline — the component only composes existing measurement and Tooltip units; it introduces no independent state machine or Effect.
| seam | driven result |
|---|---|
| default overflowing text | clamps to 3 lines and opens the Tooltip |
| mixed inline children | clamps to 2 lines and preserves strong/em/code content |
| short content | remains unclamped and mounts no Tooltip |
hasTooltip={false} |
remains clamped with no title, description, or Tooltip |
| coarse touch pointer | one tap opens one Astryx Tooltip |
| consumer callback ref across rerender | fails — the same element is reported detached/reattached four times |
The ownership is otherwise correct and the tested composition seams hold. The ref seam uses render-time mergeRefs instead of the stable hook current Text now uses.
VERDICT: BLOCKS — the public ref spuriously detaches and reattaches on ordinary rerenders
Only builders opting into the canary lab component are reached. Their users get a compact mixed-content block and optional full-text disclosure; no existing stable consumer changes on install.
VERDICT: clear
<LineClamp maxLines={2} hasTooltip="below" as="p">
Some <strong>mixed</strong> content
</LineClamp>| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
LineClamp(props): ReactNode |
lab barrel + subpath, canary-only | the linked issue plus Text.maxLines
|
en+zh | ok — maintainer accepted the lab shape |
+ |
children: ReactNode |
yes | composition content | en+zh | ok |
+ |
maxLines: number |
yes | Text.maxLines |
en+zh | ok |
+ |
hasTooltip?: boolean | LayerPlacement = true |
yes | Text.hasTruncateTooltip |
en+zh | ok for lab |
+ |
as?: 'div' | 'span' | 'p' = 'div' |
yes | Text.as |
en+zh | ok |
+ |
ref?: React.Ref<HTMLElement> plus BaseProps<HTMLElement>
|
yes | standard root passthrough | JSDoc/source | finding — unstable across rerenders |
OSSIFICATION: Lab is private and canary-only, so this shape is intentionally revisable. The linked issue establishes the component class, and @cixzhang's prior comment explicitly accepted landing the current lab shape and iterating forward; this review extends that decision rather than reopening it.
VERDICT: BLOCKS — the ref contract reports false lifecycle transitions on every rerender
themeProps('line-clamp') is correctly named and sits on the clipping/painting root. The component adds no color, spacing, radius, shadow, state target, or physical-direction styling. Its .doc.mjs does not yet enumerate the new root target; accepted as lab-stage documentation debt and required before promotion.
VERDICT: note — the canary target is not listed in LineClamp.doc.mjs
BEHAVIOR: no existing path changes; all behavior is opt-in through the new component.
API: no — additive surface in private, canary-only @astryxdesign/lab.
VISUAL: no existing pixels change; the new component intentionally renders a clamp and optional Tooltip.
THEME: no existing target changes; line-clamp is additive and canary-only.
VERDICT: clear
EFFECTS: zero explicit Effects in LineClamp. The reused Tooltip brings its existing timeout cleanup, default/controlled-open synchronization, anchor listeners, and layer-dismissal lifecycle only after overflow is detected.
RENDER: the diagnostic rerender called a LineClamp consumer ref 4 times (null/node twice); current Text's focused control calls it 0 times by using useMergedRefs.
LISTENERS/OBSERVERS: one shared ResizeObserver serves all useTruncation instances and cleans up through the callback ref. The unstable merged ref unnecessarily unregisters/registers that measurement on rerender; Tooltip listeners clean up through its existing ref contract.
LAYOUT: each truncation registration synchronously measures a Range; the unstable ref repeats that work on rerender. No render-body layout read was added.
BUNDLE: Tooltip stays lazy; the PR Analysis report measured the new component at 164 B CJS.
VERDICT: BLOCKS — render-time mergeRefs repeats ref lifecycle and truncation measurement
VISUAL CHECK: manual frames required WHY: This is a new rendered component with intended clamping and Tooltip pixels; the stable visual job was skipped on the exact head because lab has no stable visual scope.
| Warm-main control | PR head |
|---|---|
 |
 |
SENSOR RECEIPT:
| build | story | theme/mode/dir | viewport/media | semantic state | geometry/errors |
|---|---|---|---|---|---|
89abbb3edc1463b114793befc84f6f3f0dbfabe3 |
core-text--multi-line-truncation |
neutral / light / LTR | 900x500@1; fine pointer; no forced colors/reduced motion | clipped 2 lines; no title; 1 open Tooltip; description matches full text |
274x40; fonts loaded; 0 animations/errors |
48566e359d3e335faec7698f28f3b3ee944ae1ad |
lab-lineclamp--default |
neutral / light / LTR | 900x500@1; fine pointer; no forced colors/reduced motion | clipped 3 lines; native title present; 1 open Tooltip; description matches full text |
320x72; fonts loaded; 0 animations/errors |
The intended pixels follow the PR body's stated goals: “clamp the whole block to N lines” and use the “same optional-tooltip-on-truncation pattern as Text.” The unintentional difference is the PR head's second native-tooltip source: current Text/Heading removed this exact title + Tooltip pairing in #5559, while LineClamp.tsx:121 reintroduces it.
Page screenshots cannot contain native title chrome; the state receipt records that source while the frame proves the Astryx Tooltip is open. The same exact-head probe also verified: mixed inline content clamps; short content remains unclamped with no Tooltip; hasTooltip={false} keeps the clamp and removes both disclosure mechanisms; a coarse-pointer tap opens one Astryx Tooltip.
VERDICT: BLOCKS — hovering clipped content exposes both the styled Tooltip and the same native title tooltip
REMEDY SEARCH: not triggered — author owns HOW; acceptance criteria are sufficient
AUTO-COVERED: exact-head pr-a11y completed successfully; .github/a11y-baseline.json is unchanged by this PR. No runtime user- or AT-facing literal is added.
CHECKED BY ME: the Tooltip wires one matching aria-describedby; the full clipped text remains in the DOM; hasTooltip={false} leaves no dangling description; a coarse-pointer tap opens the disclosure. The shipped stories contain no interactive descendants and no component keyboard state.
NOT LOOKED AT: Safari/WebKit was not run; the component's CSS/Tooltip behavior was driven in real Chromium.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | BLOCKS — ref lifecycle breaks at the consumer seam |
| IMPACT | clear |
| API | BLOCKS — callback refs receive false detach/attach transitions |
| THEMING | note — target docs wait for promotion |
| BREAKING | clear |
| PERFORMANCE | BLOCKS — 0 to 4 consumer-ref calls on one rerender |
| VISUAL | BLOCKS — duplicate native and Astryx tooltips |
| A11Y & I18N | clear |
GOAL: partly met — Chromium proves arbitrary mixed content clamps, both disclosure endpoints work, and touch opens the Tooltip; the claimed Text tooltip pattern still renders the duplicate behavior current Text removed.
DISPOSITION: duplicate tooltip -> blocks now; unstable ref/measurement lifecycle -> blocks now; undocumented lab target -> accepted for canary staging under the written lab policy and required before promotion.
ADVICE: proven existing patterns — remove the native title as Text/Heading did in #5559, and use useMergedRefs as Text/Heading did in #5266.
AUTHOR CAN PROCEED: yes — one Tooltip source, zero consumer-ref callbacks on a same-element rerender, and the focused checks above pass.
WORST OUTCOME: “hovering clipped content exposes both the styled Tooltip and the same native title tooltip” -> request changes.
JUDGEMENT NEEDED: none — the maintainer already accepted this component/API as lab-stage surface; these are current-contract regressions, not a new shape decision.
request changes
-
[BLOCKS] The clamp sets
titlewhile also mounting Tooltip -> a pointer user sees the full text twice, with the browser tooltip arriving over the styled one ·packages/lab/src/LineClamp/LineClamp.tsx:121Confirmation: exact-head Chromium measuredtitle+ one open Tooltip; the focused regression inLineClamp.review.test.tsxfails, and current Text's inverse control passes. -
[BLOCKS]
mergeRefs(...)is recreated during render -> a builder's callback ref sees four false detach/attach calls on one ordinary rerender ·packages/lab/src/LineClamp/LineClamp.tsx:114Confirmation: the banked focused regression inLineClamp.review.test.tsxmeasured 4 calls; current Text'suseMergedRefscontrol measured 0.
Thanks — @cixzhang already said the lab shape can iterate, and the clamp/mixed-content paths work. Two current-main regressions need fixing before merge.
Clamped content mounts Astryx's Tooltip and sets the same native
title, so hover shows the full text twice. Text and Heading just removed that pairing in #5559.
mergeRefs(...)is also recreated every render. One rerender called a consumer ref four times (null/node twice), while Text'suseMergedRefscontrol called it zero; that also re-registers truncation measurement.Could you carry over those two current Text patterns? If you'd rather talk it through with someone, we're in Discord.
Current Text control LineClamp at this head
-
packages/lab/src/LineClamp/LineClamp.tsx:121— This recreates the duplicate tooltip Text and Heading just removed in #5559. -
packages/lab/src/LineClamp/LineClamp.tsx:114— Could this useuseMergedRefs? One rerender currently detaches and reattaches the consumer ref twice.
- @cixzhang explicitly allowed the current lab component to land first and iterate.
- The root target is missing from the component docs; the lab policy makes this promotion debt rather than a third blocking ask.
- Exact-head GitHub CI finished: 17 successful, 4 skipped, 1 cancelled setup job;
pr-a11y,pr-rtl, build, test, lint, Storybook, and docsite are green. Vercel deployment remains failed separately.
TIME total 31m
- setup/rules: 4m (exact-head/trust gate, full brief + critic + harness, worktree; warm main reused: yes)
- install/build/server: 2m (fast-install, build support, guarded Storybook)
- browser/a11y evidence: 9m (2 sensor frames, 4 endpoint probes, touch, exact-head CI; 0 re-measures)
- focused tests: 1m (8 authored tests, 2 diagnostic regressions, 2 current-Text controls)
- code/history analysis: 6m (diff, main history, linked issue, conventions, API/theming/ref paths)
- critique + wiki publication: 9m (estimated through serialized publication)
- remote CI wait: 0m (the exact-head runs were complete when checked)
- waste: 6m (1m command invocation correction; 5m clipped-focus arm stopped after a failed sensor)
- Safari/WebKit was not run; Chromium supplied the browser evidence.
- The separate Vercel deployment log; GitHub's required build/test/a11y/RTL jobs are complete and green.
Not posted: this was a read-only review. Gate 3 was revised to embed the visual evidence, bank the focused reproduction, and quote the PR’s stated visual intent.