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.

Clone this wiki locally