Skip to content

Review 5454

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

Review #5454 — Table frozen/grouped-row defects

Versions: Review Loop 1.1.0 · Component Audit Rubric 1.10
Verdict: request changes

PR

#5454 fix(Table): three defects surfaced by frozen columns and grouped rows by ernestt (bucket: internal — ENGOWNERS)

HEAD REVIEWED

e8df392c9a8387b9e62bde26f37698444a85e0a5

VERSIONS

LOOP VERSION: 1.0.0 AUDIT RUBRIC: 1.10

PROBLEM

  1. A checked row using the default selection wash becomes two visual states at a frozen-column boundary: selected in scrolling cells, unselected in pinned cells.
  2. A grouped row's label and collapse control scroll away while frozen data columns stay visible.
  3. A partially loaded group shows its loaded count as though it were the final total.

All three are reproducible user problems, but they are independent decisions and can land in either order.

VERDICT: clear

SOLUTION

The row-selection owner now publishes the same color through the sticky-cell overlay contract that it already paints on the row. The grouped-row owner makes the heading cluster sticky at the table's logical start edge. Separately, grouped rows accept a result-set total and format a partial count.

SOLUTION (3 independent decisions · 118 runtime changed lines of 322 total; evidence:runtime 1.73)

  1. Synchronize the selection wash into --table-row-overlay and remove it with the row wash. [fix 1]
  2. Move the group-heading start gutter onto a position: sticky; width: fit-content inner wrapper. [fix 2]
  3. Add getGroupTotal, a fourth renderGroupHeader argument, and localized count of total output. [new API]

The commits and changesets are already split one per decision. The third can be removed without weakening the first two.

VERDICT: note — three separable decisions; the new API is not required by the two fixes

ARCHITECTURE

OWNER: Table's plugin pipeline owns grouping/selection; TableRow + useTableStickyColumns own the row-overlay transport and pinned-cell paint. TIER 1: collection/selection; theming/visual state; container padding/bleed; text/i18n/direction; public render-function composition. TIER 2: none. SEAMS: synthetic selection column; synthetic grouped row replacing pre-rendered cells; public renderGroupHeader; sticky cells; scroll wrapper using the four logical container-padding vars; RTL. BEHAVIOR UNIT: the existing hooks are the right units; no new state machine is introduced.

Driven seams:

seam result
default grouped header fixed at logical start in LTR/RTL, narrow/wide, 200% text, forced colors
selection + sticky start run row and pinned-cell fills match in all 14 shipped theme/mode pairs
custom full-width renderGroupHeader fails: its available width collapses from 1,334px to 180px at wide/no-scroll
logical container bleed heading button holds 4–6px from the scroll wrapper's logical start

The default path is owned correctly, but width: fit-content changes the public render-function seam rather than only the plugin's built-in heading.

VERDICT: BLOCKS — the pin fix breaks the shipped custom-renderer composition seam

IMPACT

The first fix makes a selected row read as one state across a freeze line. The second keeps the section name and collapse control reachable while the reader scans horizontally, including RTL. The third changes nothing until a caller opts into a new total callback.

Landing the header change also newly collapses full-width custom heading layouts to their content width. With the two new performance paths, a builder's unrelated state update makes a 10,000-row table repeat 20,000 overlay calls in Storybook dev and 10,000 data-cell renders; the exact base did neither.

VERDICT: BLOCKS — default users improve, but existing custom renderers can visibly reflow

API

Real proposed call site from the source JSDoc:

useTableGroupedRows({
  data: pageRows,
  groupBy: row => row.team,
  getGroupTotal: key => totalsFromServer.get(key),
  // ...
});
change public? class / existing seam doc'd? verdict
+ getGroupTotal?: (groupKey: string) => number | undefined yes — exported UseTableGroupedRowsConfig useTablePagination.totalItems is a scalar total; existing renderGroupHeader(key, count) can already close over per-group totals source + .doc.mjs BLOCKS — no current consumer and documented use destabilizes rows
~ renderGroupHeader(key, count, collapsed, total) yes — shipped in v0.5.0 additive argument on existing render seam; 3-arg functions remain assignable source + .doc.mjs compatible by type

Ossification: #5448, the only motivating in-repo consumer, now pages by whole sections and does not call getGroupTotal. The existing render callback can display a caller-owned total today. The new getter is therefore a permanent second spelling without a live consumer, and removing it later would require deprecation.

VERDICT: BLOCKS — split/drop the new API until a real consumer proves the shape

THEMING

No theme target is added, removed, or moved. --table-row-overlay is component-author plumbing, not a theme surface; it carries the existing --color-accent-muted token through the pinned-cell paint owner. headerInner is structural positioning and correctly receives no target.

The known pre-existing gap remains: selected row state is not reflected through themeProps('table-row'), so themes cannot target selection. This PR does not worsen that contract and is not charged for it.

All seven shipped themes × light/dark resolved identical row and pinned overlay colors after selection.

VERDICT: clear

BREAKING

  • API: additive; existing 3-argument renderers compile and run. New API is opt-in.
  • Visual: yes — a custom renderer using the existing full-width content area changes from 1,334px to 180px at 1,400px viewport.
  • Theme: no target/token/override changes; 14 theme/mode pairs matched.
  • Behaviour: selection/uncheck/flag flip retain existing semantics; grouped collapse/expand retains focus. The new total changes only opted-in default text.

The visual break reaches the already-published renderGroupHeader surface in v0.5.0 and has no breaking changeset.

VERDICT: BLOCKS — existing custom-header layout can change on upgrade

PERFORMANCE & RESOURCES

Effects: no new effect; selection keeps its existing no-deps notify effect. Listeners/observers/layout reads/bundle dependencies: none added.

Measured in one Chromium run and one worktree, exact PR base vs head, at N=10/100/500/2,000/10,000 (the repo's Table Lab exposes 10,000):

path on unrelated owner rerender base head guarded probe
data-cell renders from inline getGroupTotal 0 at every N N at every N (10,000 at max) not attempted
--table-row-overlay set/remove calls on unchecked rows 0 at every N 2N in Storybook dev (20,000 at max) 0 at 10,000

The getter's identity enters the plugin memo dependency list, so the documented inline callback creates a new plugin and invalidates every memoized row. The selection path unconditionally removes an already-absent property on every notification; a one-line current-value guard reduced the 10,000-row probe from 20,000 calls to 0 without changing the visual result.

Repro: review-artifacts/pr-5454/scale-probe.cjs, scale-before.json, scale-after.json, scale-guarded.json.

VERDICT: BLOCKS — both new paths add linear work to unrelated renders

VISUAL EVIDENCE

Containment: the Table scroll wrapper consumes all four logical container-padding vars and bleeds to the container edge; the group cell spans the complete table; the sticky inner is bounded by that cell.

case before after result
wide 1,400px shots/before__wide.png shots/after__wide.png default layout unchanged except intentional partial count
narrow 320px, scrolled 420px shots/before__narrow.png shots/after__narrow.png intentional: collapse button −416px → +4px from start
RTL 700px, scrolled −420px shots/before__rtl.png shots/after__rtl.png intentional: collapse button −416px → +4px from logical start
200% text shots/before__zoom200.png shots/after__zoom200.png intentional pin holds; no clipping observed
forced colors shots/before__forced-colors.png shots/after__forced-colors.png pin holds; checkbox + ARIA preserve selected state when fills normalize
full-width custom renderer shots/before__custom-wide.png shots/after__custom-wide.png unintentional: custom content 1,334px → 180px; trailing action moves 1,154px

Theme matrix: shots/after__{neutral,stone,butter,chocolate,gothic,matcha,y2k}__{light,dark}.png; every selected row/pinned cell pair matched.

VERDICT: BLOCKS — default fix works, public custom composition visibly regresses

A11Y & I18N

Auto-covered: pr-a11y and pr-rtl ran and passed at the reviewed head; no baseline entry was added. The new visible string uses @astryx.tableGroupedRows.partialCount with a non-empty translator description.

Browser-checked: Enter collapses and expands the group; focus remains on the same chevron; aria-expanded updates; the control stays reachable in LTR/RTL, narrow, 200% text, and forced colors. Selection retains its checked checkbox and aria-selected when forced colors normalize the fills.

No new focus owner, announcement, role, or keyboard path is introduced.

VERDICT: clear

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION note — 3 independent decisions
ARCHITECTURE BLOCKS — custom render seam
IMPACT BLOCKS — custom layout reflow
API BLOCKS — unused/destabilizing new surface
THEMING clear
BREAKING BLOCKS — visual
PERFORMANCE BLOCKS — linear work
VISUAL BLOCKS — custom composition
A11Y & I18N clear

GOAL: partly met — default selection and heading defects are fixed across the driven matrix; the pin breaks a documented composition seam, and the total API adds a separate unstable path.

DISPOSITION:

  1. Custom renderer width regression → blocks now; preserve its available-width behavior while keeping the built-in heading/control reachable.
  2. Selection overlay's repeated no-op write → blocks now; on an unrelated owner update the builder pays one extra DOM call per row per notification, and the tested current-value guard makes that flat again.
  3. getGroupTotal surface + row invalidation → blocks now; split/drop the third commit until a real consumer and stable plugin path exist.

ADVICE: bounded direction — retain the custom render slot's full-width contract; skip unchanged overlay writes; treat total-count support as a separate API decision with a consumer and a row-memoization invariant.

AUTHOR CAN PROCEED: yes — acceptance criteria are the three disposition lines, re-run against the banked probes.

WORST OUTCOME: “a custom renderer using the existing full-width content area changes from 1,334px to 180px at 1,400px viewport” → request changes.

request changes

  1. [BLOCKS] width: fit-content shrink-wraps the public custom-header slot → existing full-width heading layouts visibly collapse on upgrade · packages/core/src/Table/plugins/groupedRows/useTableGroupedRows.tsx:173

  2. [BLOCKS] selection rewrites an unchanged overlay on every row notification → large tables pay linear DOM calls on unrelated owner renders · packages/core/src/Table/plugins/selection/useTableSelection.tsx:168

  3. [BLOCKS] inline getGroupTotal becomes plugin identity → every data row re-renders when its owner re-renders · packages/core/src/Table/plugins/groupedRows/useTableGroupedRows.tsx:470

REVIEW

Thanks — the selection wash and default group heading now work across the browser matrix. Three far-side blockers remain. A full-width renderGroupHeader collapses from 1,334px to 180px. With the documented inline getGroupTotal, a builder's unrelated state update on a 10,000-row table re-rendered all 10,000 data cells; selection's unguarded overlay sync made 20,000 no-op calls in Storybook dev. The exact base did neither. The new total API also has no current consumer—#5448 now loads whole sections—and the existing renderer can already show a supplied total. Could we preserve the custom slot's width, skip unchanged overlay writes, and split/drop getGroupTotal until it has a consumer and preserves row memoization?

INLINE (3)

  • packages/core/src/Table/plugins/groupedRows/useTableGroupedRows.tsx:173 — This shrink-wraps public renderGroupHeader content: my full-width case collapsed 1,334px → 180px.
  • packages/core/src/Table/plugins/selection/useTableSelection.tsx:168 — This adds a no-op CSS-variable mutation per row on every owner render; guard unchanged values.
  • packages/core/src/Table/plugins/groupedRows/useTableGroupedRows.tsx:470 — The documented inline getter invalidates every row: 10,000 data rows re-rendered; base stayed at zero.

EVIDENCE I DID NOT SPEND

  • The selected row and pinned cell resolved identical colors in all 14 shipped theme/mode pairs.
  • The default collapse button moved from −416px off-start to +4px in both LTR and RTL.
  • The reviewed head is mergeable and all 17 substantive CI checks ran green; only review-required is pending.

Round 2 — exact-head re-review, 2026-08-26

Versions: Review Loop 1.4.0 · Component Audit Rubric 1.12 Head reviewed: e24ef4c50ab3a5eede328242e9cfdf7f0b9a54d7 (ernestt pushed 17:47:47Z, after our changes-requested at 05:55:24Z) Verdict: approve · drafted, not posted Lane: full — our own CHANGES_REQUESTED was standing, and the diff is Table interaction + layout.

Title changed since round 1: three defects became two. The PR is now +179/−9 across 6 files.

Reconciliation with our own round-1 review (R31b)

All three blockers walked at the exact head. None is a reversal; round 2 extends round 1 with one note that is a consequence of ask 1.

our ask at e24ef4c evidence
preserve the custom slot's width satisfied renderGroupHeader inner box 1,404 → 1,408px; the trailing element sits at x=1345 at rest and x=985 scrolled in both arms; the scrolled frames share one sha256
skip unchanged overlay writes satisfied 3 notifications × 6 rows = 18 row passes → 2 setProperty + 1 removeProperty, exactly the rows that moved
split/drop getGroupTotal satisfied prop, changeset, tests, locale key and doc entry all gone; grep for getGroupTotal|partialCount across packages/ apps/ .changeset/ → 0 hits

Did main invalidate it (R31c)? No. 55 commits since the merge-base cd1a2da, and git log cd1a2da..origin/main is empty for all four touched source files. mergeable: MERGEABLE; 19 CI checks green on this head (one skipped: Anchor review-clear chain).

Problem

A checked row on a table with a frozen first column reads as selected on the scrolling side and unselected under every pinned column. And a grouped table scrolled sideways loses its section headings with the content, leaving unlabelled grey bands and no pointer-reachable collapse control.

Solution — 2 decisions · 27 runtime lines of 179 added

  1. useTableSelection publishes and withdraws --table-row-overlay alongside the row wash, both writes guarded on the current value.
  2. The group heading's contents become a sticky, start-inset box carrying the cell's start gutter; width: fit-content only when the plugin draws the heading.

Both trace to a stated problem. The rest of the diff is evidence: 62 test lines, two patch changesets, a JSDoc pagination section and two .doc.mjs descriptions.

The new finding — the pin is scoped to the built-in heading

Measured, both arms, one build, one Storybook, BEFORE = the two source files reverted to the merge-base in place (before-arm.diff, 5,848 B, 81 changed lines, the exact reverse of the PR's own hunks and nothing else):

case, scrolled 360px base e24ef4c
built-in heading, chevron x −356, off-screen, static +4, visible, sticky
built-in heading, RTL, from the logical start −356, off-screen +4, visible
custom renderGroupHeader, chevron x −356, off-screen −356, off-screen

custom-scrolled__after.png, custom-scrolled__before.png and default-scrolled__before.png are all one sha256 (74ac3326…): with a custom heading, head emits the same bytes as the pre-fix bug.

Not a regression, and not a block. At rest the two custom arms differ by 267 of 248,320 pixels (0.108%) at a maximum channel delta of 2/255 — text antialiasing from the heading becoming a sticky box — with the chevron at x=4, the trailing element at x=1345 and the cell at 1,420px in both. The collapse control is still keyboard-reachable there (focus pulls it into view, scrollLeft 360 → 0, which is what base does for both paths). And it is the direct consequence of round 1's own ask to protect that slot's width.

So: GOAL: partly met, and the note exists to stop the changelog reading as unconditional.

Performance — the guard, driven three ways

Overlay writes over 3 selection notifications × 6 rows = 18 row passes:

arm set remove total
merge-base — no overlay code 0 0 0
round-1 head e8df392 — unguarded 4 14 18 (one per row per notification)
e24ef4c — guarded 2 1 3 (exactly the rows that moved)

unguarded-arm.diff (1,397 B) removes exactly the three guard conditionals. Zero effects added, moved or deleted.

Breaking

API no · Theme no · Visual yes and intended on the two default paths, both patch-changeset'd; the heading cell's outer box is 1,420px in both arms, so nothing grew and nothing reflows around it · Behaviour: six states walked, the only new conditional is !renderGroupHeader and both sides are driven.

Visual evidence

Eight frames, real Chromium, every one opened and looked at, every one carrying a captureWithSensors() receipt with runningAnimations: 0, fonts: loaded, no page or Storybook error, and non-generic state sensors matching across arms. The first capture round recorded 7 and 14 running animations (Table's background-color transitions); rather than argue the pixels were settled, all frames were re-shot with animations frozen.

  • built-in heading scrolled: three unlabelled grey bands become "Design Systems (3)", "Infra (2)", "Growth (1)", each pinned with its chevron — 0.79% of pixels
  • checked row scrolled: frozen checkbox and Name cells rgb(255,255,255) → rgb(241,241,241), matching the scrolled side; unselected rows unchanged on both sides — 5.32%
  • custom heading scrolled: 0.00%, same sha as the pre-fix default

No shipped story combines grouped rows or selection with useTableStickyColumns, so the green pr-visual covers neither endpoint — the PR Analysis Report on this head reads "No visual change across 2 compared shot(s). 46 shot(s) have no baseline yet."

Judgement

approve. Ten slots: seven clear, three note (ARCHITECTURE, IMPACT, VISUAL) — and the three are one thing seen from three sides, so the review says one thing. No slot's prose describes a user who is stuck, cannot reach something, or loses their place.

  1. The heading pin is conditional on the plugin drawing the heading → a consumer passing renderGroupHeader still gets unlabelled bands and a chevron 356px off the start edge when scrolled · useTableGroupedRows.tsx:390
  2. Both changesets credit @ernesttien → gh api users/ernesttien 404s, @ernestt is the account and what the other eight landed entries use, so the release notes lose the credit · .changeset/grouped-rows-pinned-heading.md:21 and .changeset/table-selection-sticky-overlay.md:18

ADVICE: omitted on 1 — a fix that pins the plugin's chevron while leaving the caller's slot full-width is designable but I did not build it (R2l). AUTHOR CAN PROCEED: yes. JUDGEMENT NEEDED: none — defect fix against the existing contract, no API, no theme target, no new capability.

Dropped without raising: the two new em dashes in the .doc.mjs prose, two days after #5413 and #5488 swept them. 34 of 220 core .doc.mjs files still carry one on main and no lint enforces it, so the count killed the finding (#5557 is a third open sweep and will take these).

The review, as drafted — not posted

Thanks — all three fixed at e24ef4c. The pin only applies when the plugin draws the heading, so a custom renderGroupHeader still loses its chevron when scrolled. Both changesets need @ernestt.

Full review

Inlines:

  • packages/core/src/Table/plugins/groupedRows/useTableGroupedRows.tsx:390 — Custom headings skip the shrink-wrap, so the chevron can't pin.
  • .changeset/grouped-rows-pinned-heading.md:21 — @ernesttien 404s. @ernestt is the account, and both changesets have the wrong one.

Posting note. Our round-1 CHANGES_REQUESTED is still standing and only the cixzhang account can clear it — an approve has to be posted from that account or the PR stays blocked however green it is (R31b.4).

Critic

Three passes, all failed-then-fixed except the last.

  • Pass 1 — R2 (47 words against a 30 cap), R2m/R12 (a "nothing to change" framing directly above an inline asking for a change, plus a closing question on an approve), R2h (an inline carrying its own measurement), the GOAL label (met where partly met was right), R38 (runningAnimations: 7 in every receipt, unmentioned). All frames re-shot.
  • Pass 2 — R12b: the trim to 28 words had dropped "when scrolled", which made the posted sentence false at rest. Plus three prose-vs-artifact drifts: 80 changed lines vs 81, an over-stated keyboard row, and a PERFORMANCE column labelled with a head I had cited rather than driven. The unguarded arm was then driven, which turned a cited number into a measured one.
  • Pass 3 — "would she have posted it: yes", with one violation: R14h, "byte-identical to base in both scroll states" when only the scrolled state had been captured. The missing state was captured rather than the sentence softened, and the critic's doubt was better than its guess — the at-rest frames are not byte-identical, they differ by 267 antialiased pixels.

The residual failure class across all three gates was the same: a summary sentence quantifying slightly past the artifact it came from.

What changed before posting

Not posted. This record is the round-2 draft as handed over.

Time

44m total — setup 3m (warm main not reused; an in-worktree BEFORE arm is the right baseline when main is 55 commits past the merge-base), reading 5m, measuring 12m, writing 10m, critic 14m, waste 3m. Waste: a selection probe that read Storybook's own argstable <tbody><tr> instead of the story's table; capturing all frames before asserting runningAnimations; and reading the critic file in chunks that truncated mid-file.

Artifacts: {review-root}/review-artifacts/pr-5454-r2/ — gate-1..4.md, measure-{before,after}.json, probe2-{before,unguarded,after}.json, pixels.json, atrest-diff.json, before-arm.diff, unguarded-arm.diff, shots/ (8 PNGs + receipts).

Clone this wiki locally