Skip to content

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

Open
ernestt wants to merge 1 commit into
mainfrom
feat/grouped-rows-header-cell-guard
Open

fix(Table): keep group headers out of the caller's cell renderers#5317
ernestt wants to merge 1 commit into
mainfrom
feat/grouped-rows-header-cell-guard

Conversation

@ernestt

@ernestt ernestt commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

What

useTableGroupedRows injects synthetic group-header rows into the flattened data. Each is a Proxy that answers unknown fields with '', and the source comment claims this makes "arbitrary field access from user cell renderers" safe.

It doesn't. The Proxy rescues a renderer that prints a field. It does nothing for the far more common one that keys a lookup off it:

renderCell: item => <StatusDot status={STATUS_META[item.status].dot} />

'' is no more a member of that map than undefined is, so this throws and blanks the page the moment grouping is switched on. There was no way to opt out from the consumer side, because BaseTable evaluates every column's renderCell against every row before transformBodyRow gets to discard a header's cells:

const rawContent = isDefaultRenderer
  ? defaultCellRenderer(item, col.key)
  : (col.renderCell?.(item) ?? null);

I hit this building a table template and had to work around it with a transformColumns wrapper in the consumer — which is what this PR moves into the plugin, where it belongs.

How

transformColumns now wraps each column so renderCell is skipped for header rows. Those cells are discarded moments later by transformBodyRow regardless, so nothing is lost by never producing them. The Proxy stays as a backstop for the default renderer, which reads item[key] directly.

Wrapped columns are cached against their source column in a WeakMap. This matters: BaseTable stabilises the resolved column array with an element-by-element reference check, so allocating a fresh column object per render would have silently defeated MemoizedTableRow and re-rendered every row on every render. There's a test locking that down.

The hook also now returns isGroupHeader, so a consumer guarding their own row-level plugin (click-to-open-detail, row links, per-row menus) has something better to test than whether the row key happens to start with __group_:

transformBodyRow(props, item) {
  if (grouped.isGroupHeader(item)) return props;
  return {...props, htmlProps: {...props.htmlProps, onClick: () => open(item)}};
}

Test plan

Three tests added, each verified to fail without its fix:

  • A column whose renderCell keys a lookup off a field no longer throws when grouping is on; only the three real rows reach the renderer. Without the guard: TypeError: Cannot read properties of undefined.
  • transformColumns returns the same wrapped column objects across calls, and leaves renderCell-less columns untouched. Without the WeakMap: fails.
  • isGroupHeader distinguishes the two synthetic headers from the three real rows.
  • Full Table suite: 479 passed (22 files).
  • tsc --noEmit, eslint, and prettier --check all clean.

Notes

One thing I chose not to change: aria-rowcount / aria-rowindex drift when headers are present, since the rendered rows include them. Fixing it properly needs a count the plugin doesn't own, so it seemed out of scope here.

Made with Cursor

The grouped-rows plugin wraps each synthetic header in a Proxy that answers
unknown fields with '', on the theory that a cell renderer reaching into a
header would then read an empty string rather than throw. That only holds for
a renderer that prints a field. The common shape keys a lookup off one --
STATUS_META[item.status].dot -- and '' is no more a member of that map than
undefined is, so the renderer throws and takes the page down with it. There
was no way for a consumer to opt out: BaseTable evaluates every column's
renderCell against every row before transformBodyRow can discard a header's
cells.

Skip the call instead. Those cells are thrown away moments later regardless,
so nothing is lost by never producing them, and the Proxy stays as a backstop
for the default renderer that reads item[key] directly. Wrapped columns are
cached against their source column because BaseTable compares the resolved
column array element by element to decide whether every row must re-render --
allocating a fresh column per render would have quietly cost that.

Also return isGroupHeader, so a consumer guarding their own row-level plugin
has something better to test than whether the row key starts with __group_.

Co-authored-by: Cursor <cursoragent@cursor.com>
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 22, 2026
@vercel

vercel Bot commented Aug 22, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview Aug 22, 2026 12:57am

Request Review

@github-actions github-actions Bot added the needs:code-review High-risk change (new package/component/API) — needs human code review before merge label Aug 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

Modified Components

Table (@astryxdesign/core) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 1952 -
Complexity N/A Very High (113) -

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.8KB 1.2KB

Accessibility Audit

Status: No accessibility violations detected.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

github-actions Bot added a commit that referenced this pull request Aug 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. needs:code-review High-risk change (new package/component/API) — needs human code review before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant