Review 5317

#5317 — fix(Table): keep group headers out of the caller's cell renderers

ernestt · OPEN · view on GitHub

Verdict: approve with nits

Head reviewed: 659fd584bf9efa74a9baab59c3223b869cff9840

Problem

BaseTable evaluates every column's renderCell against every row before transformBodyRow can discard a synthetic group header's cells (BaseTable.tsx:185-188). The header Proxy answering '' only rescues a renderer that prints a field; one that keys a lookup — STATUS_META[item.status].dot — throws on '' exactly as on undefined. Switching grouping on over a table with a lookup-keyed cell renderer blanked the page, with no consumer-side opt-out.

Solution

A transformColumns step wraps each column so renderCell short-circuits to null on a header row (useTableGroupedRows.tsx:352-354). Wrappers are cached in a module-level WeakMap keyed on the source column so BaseTable's element-wise column stabilisation keeps passing (BaseTable.tsx:396). transformBodyCell runs before renderCell is evaluated but only transforms cell props, so transformColumns was the only lever available.

Impact

Every app on the release that ships it, but only where grouping is already on, and only as a crash that stops happening. Measured main vs head: the header rows' renderCell calls disappear and every real row's render count is identical. isGroupHeader reaches only the next builder composing their own row-level plugin.

API

~ useTableGroupedRows(): + isGroupHeader: (item: T) => boolean
    (public, required member of UseTableGroupedRowsResult — type IS in the
     barrel, Table/index.ts:109)

Theme targets

No new theme targets.

Ossification

A class, with real members already in the repo. Every plugin that hands a row to a consumer callback sees synthetic headers and cannot tell: useTableRowStatus({getStatus}), useTableRowIndex({getRowKey}), selection's getIsItemSelected, and any consumer transformBodyRow. The only tell today is string-matching the private __group_ key prefix, so this replaces a footgun rather than adding an axis.

Breaking

• API — no. Additive member on a returned object.
• Visual — no. transformBodyRow already replaced a header row's children wholesale with a single <td colSpan={999}>. Table suite: 479 tests, all pass.
• Theme — no.

Performance & resources

• Render — one columns.map per table render, O(columns) not O(rows).
• Listeners — none added. Layout — no reads. Bundle — no new dep.
• Downstream memoization — measured (vitest/jsdom, renderCell calls per row id, inputs reference-stable, 2 groups / 4 rows): main {'':2, a:1,b:1,c:1,d:1} → head {a:1,b:1,c:1,d:1}; identical row behaviour, the '' entries are the header rows the fix removes. The WeakMap holds.
• Perf test — none for groupedRows; pagination, selection and tree each have one.

Risk class

Needs judgement — fails one of three. New API surface: one required member on an exported result type. Behavior regression: none, measured. Performance regression: none, measured.

Judgement

approve with nits

1. The guard covers only the columns present when grouped's transformColumns
   runs, and `grouped` is not in PLUGIN_ORDER
   → a builder who writes plugins={{grouped, rowStatus}} with a lookup-keyed
     getStatus gets the original crash back: grouped-first throws
     `TypeError: Cannot read properties of undefined (reading 'color')`. The
     repo's own table lab assembles in exactly that order
                                              · useBaseTablePlugins.ts:50
                                              · table-lab/page.tsx:282

2. No `*-perf.test.tsx` for groupedRows, and the stability test calls
   transformColumns outside React
   → the fix's safety rests on BaseTable's element-wise column stabilisation,
     code this plugin does not own; if that changes, a grouped table re-renders
     every row on every parent render and no test fails
                                              · useTableGroupedRows.test.tsx:258

Neither blocks. Finding 1 is a strict improvement over main, which crashes in both orders. Finding 2 is a nit by R18c's own test: the added work is O(columns), not O(rows).

The review, as posted

Thanks, good catch — and you're right about the column caching. Two nits inline; the ordering one is worth a follow-up.

• useTableGroupedRows.tsx:353 — Columns injected after this — rowIndex, rowStatus in table-lab — still hit header rows.
• useTableGroupedRows.test.tsx:258 — Calls transform outside React; a -perf.test.tsx would lock the row-memo invariant instead.

What changed before posting

Posted as drafted.