feat(data-table): add expandable rows with inline detail panel by erickteowarang · Pull Request #430 · tailor-platform/app-shell

erickteowarang · 2026-08-12T02:05:46Z

Closes tailor-inc/platform-planning#1650

What

Adds expandable detail rows to DataTable. Passing renderExpandedRow to useDataTable enables the whole feature — a chevron column is auto-added at the left edge (auto-pinned left, after the selection column) and a full-width detail row renders beneath each open row. There is nothing new to compose in JSX.

const table = useDataTable<Order>({
  columns,
  data,
  control,
  renderExpandedRow: (row) => <OrderLineItems orderId={row.id} />,
  canExpandRow: (row) => row.lineItemCount > 0,
  expandRowLabel: (row) => row.orderNumber,
});

New options: renderExpandedRow, canExpandRow, expandRowLabel, expandedIds, onExpandedChange.
New returns: expandedIds, isRowExpanded, toggleRowExpansion, collapseAllRows (the last two are undefined without renderExpandedRow).

Additions only — no existing signature changes, and a table without renderExpandedRow renders the same rows it did before.

Key decisions

Render prop, not sub-rows. One full-width <tr> with a single colSpan cell. Hierarchical sub-rows are a different data model and stay out of scope.
Dedicated chevron column, so onClickRow stays free for navigation. The cell stops click propagation, exactly as the selection cell does.
Keyed by row.id, the same constraint as row selection. Rows without one get no chevron rather than a disabled one — a row must never be un-toggleable.
Expansion survives page changes; ids of off-page rows simply don't render. collapseAllRows() is the opt-out.
Panel is position: sticky to the left edge of the scrollport, capped at min(100%, var(--data-table-viewport)) — a colSpan cell spans the whole table, so on a horizontally scrolled table the panel would otherwise sit off-screen. The min() makes it a no-op when the table fits its container.
No accordion / single-open mode in v1 — purely additive later.

The freeze-seam boundary moves off the selection column onto the expand column when both are present (selection.isBoundary becomes !hasExpand && left.length === 0), so the shadow is drawn on the correct cell while horizontally scrolled.

Accessibility

Native <button> trigger with aria-expanded (the announcement on activation) and aria-controls set only while expanded, so it never points at an absent id. expandRowLabel returns a bare identifier that the i18n labels compose into "Expand row INV-1001" / "INV-1001 details", keeping en and ja word order correct — "Expand row" repeated twenty times conveys nothing. The panel is a named region; collapsing while focus is inside it hands focus back to the trigger instead of dropping it to <body>. sr-only header text, overflow-x: auto on the sticky wrapper, and motion-reduce:transition-none on the chevron rotation.

Known limitation, documented not fixed: detail rows are real <tr>s, so a screen reader counts them — ten records with two expanded announces as twelve rows. Fixing it needs aria-rowcount plus explicit aria-rowindex on every row (detail rows sharing their parent's index) and correct interaction with pagination; role="presentation" would fix the count but remove the panel from screen-reader table navigation. Neither is implemented. There is no WAI-ARIA APG pattern for this exact shape — Disclosure assumes the panel isn't inside a grid, Treegrid assumes child rows share the parent's columns.

Testing

19 new tests in data-table.test.tsx covering the column's presence/absence, toggling, aria-expanded/aria-controls, colSpan arithmetic, multiple open rows, onClickRow isolation, id-less rows, canExpandRow, contextual names, the named region, focus return on collapse, controlled mode, the pinned offset and boundary handoff, skeleton rows, and the empty/error status rows.

pnpm type-check, pnpm lint, pnpm test (1449 passed), pnpm fmt all clean.

Reviewer notes

Demo: examples/vite-app/src/pages/data-table-lab/page.tsx → new Expandable rows section, which puts expansion next to selection, a left-pinned column, row actions and horizontal scroll on one table. It's a second section rather than a change to the existing one, so the pinning/column-settings demo stays untouched.
The panel uses <section aria-label> rather than <div role="region">; repo lint (jsx-a11y/prefer-tag-over-role) rejects the explicit role and a named <section> maps to role="region" anyway.
onExpandedChange is called inside the state updater, matching toggleRowSelection. That double-fires under StrictMode in dev — pre-existing behaviour of the selection path, kept consistent rather than diverging here.

🤖 Generated with Claude Code

Pass `renderExpandedRow` to `useDataTable` and each row gets a chevron column (auto-added at the left edge, auto-pinned left after the selection column) that reveals a full-width detail panel beneath the row. Nothing new to compose in JSX — the column and the detail row appear automatically. Expansion state is owned by `useDataTable` as a Set of row ids, mirroring row selection: it is keyed by `row.id`, rows without one render no chevron, and expansion survives page changes (`collapseAllRows()` is the opt-out). `expandedIds` + `onExpandedChange` switch it to controlled mode. The trigger is a native button with `aria-expanded`; `expandRowLabel` supplies a bare record identity that the i18n labels compose into "Expand row INV-1001" / "INV-1001 details" so en and ja word order both stay correct. The panel is a named region, and collapsing while focus is inside it hands focus back to the trigger. On a horizontally scrolled table the panel stays pinned to the left edge of the scrollport. Row-count inflation (detail rows are real `<tr>`s, so ten records with two expanded announce as twelve rows) is documented as a known limitation; `aria-rowcount`/`aria-rowindex` are deliberately not implemented. Refs: tailor-inc/platform-planning#1650 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Follow-up to the initial expandable-rows implementation, addressing a high-effort adversarial review plus the expand/collapse animation. Review fixes: - Namespace row keys (`id:` / `idx:`) so an id-less row's index fallback can't collide with a real id. Pre-existing — React stringifies keys, so `1` and `"1"` already collided — but it pairs detail panels with the wrong row via position-based reconciliation. - Stop gating the detail panel on `canExpandRow`. A row that went non-expandable while open lost its chevron and its panel but stayed in `expandedIds`, leaving it open with no way to close it. - Fall back to the table container when the trigger unmounts in the same commit as the panel. `getElementById` returned null there, so focus fell to <body> — the exact outcome the handoff exists to prevent. - Scope column measurement to the component's own table. `renderExpandedRow` can nest a DataTable, and the built-in column keys are module constants, so an unscoped query let an inner header overwrite outer pinned widths. - Warn when `expandedIds` is passed without `onExpandedChange`, which otherwise renders permanently inert chevrons. - Memoise `toggleRowExpansion` / `collapseAllRows` and skip redundant writes, so the documented collapse-on-page-change recipe can't loop. - Guard `--data-table-viewport` against zero-width mounts and redundant writes inside the ResizeObserver callback. Two findings are documented rather than fixed: controlled-mode toggles snapshot the prop (a functional-updater API is out of scope), and `onExpandedChange` double-fires under StrictMode (matches the existing `onSelectionChange` behaviour). Animation: The panel reveals by transitioning `grid-template-rows` 0fr -> 1fr, so it animates to its exact natural height with no cap to clip tall content, and the `<tr>`/`<td>` are left alone (rows size to content, and animated row heights are unreliable under `border-collapse`). The animating wrapper sits inside the sticky node so left-edge pinning survives. This needs the detail row to outlive `expanded`, so presence is now a small state machine and the rendered row is a child component that genuinely unmounts — a layout-effect cleanup only runs before detach on real unmount, and keeping it in the parent silently reintroduced the focus bug above. Collapse is therefore asynchronous: `aria-expanded` flips immediately, the row lingers ~200ms as `data-state="closed"`. Documented, since it changes how consumers assert removal in tests. Also varies the demo's panel per row status to show `renderExpandedRow` is a plain `(row) => ReactNode`. Refs: tailor-inc/platform-planning#1650 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

… gaps Second review pass. Two of these were introduced by the previous round's fixes, so the earlier findings were traded for new ones rather than resolved. - Expansion callbacks were still not identity-stable: the useCallback deps included `expandedRowIds`, which is a fresh Set on every expansion change. The "collapse on page change" recipe this repo documents therefore re-fired right after the user opened a row and shut it again — worse than the bug it replaced, and the docs had begun promising the stability. Both callbacks now read the set through a ref. - `canExpandRow` had stopped gating opening as well as closing, so an id in `expandedIds` rendered a panel for a row the consumer excluded and ran `renderExpandedRow` against data that may not exist. Restored the gate in both directions; both cell gates simplify back to `expandable`, which also removes the case where collapsing a non-expandable row unmounted the focused chevron and dropped focus to <body>. The trade is that an excluded row's id lingers in `expandedIds` inert, with nothing open on screen, until `collapseAllRows()`. Documented. - The closing panel stayed focusable and in the accessibility tree for the whole 300ms collapse. It is now `inert` while closing, and focus is handed back to the trigger at the start of the collapse rather than at unmount — making an element inert while it holds focus would drop focus to <body>. - DOM ids interpolated the raw row id, so `{ id: "ORD 4471" }` emitted an invalid `id` and an `aria-controls` the UA split into two dead references. Whitespace is now collapsed before composing them. - `p-0` never beat `Table.Cell`'s `first:pl-6 last:pr-6` (specificity), so the sticky panel was inset 24px and overflowed the scrollport — wrong since the first commit, and contrary to what the docs promised. Now `p-0!`. - The focus fallback stamped `tabindex="-1"` on the shared table container and never removed it, permanently pulling that scroll region out of the tab order for every `Table.Root` consumer. It is restored after focusing. - `expandedIds` / `isRowExpanded` are optional on `DataTableContextValue`, which is documented as hand-constructible; required members there are type-breaking under a minor bump. - The ResizeObserver re-ran the full header measure on every frame of the reveal. It now bails when neither width moved. Also slows the reveal from 200ms to 300ms. Known follow-up, not fixed: the outer table's `data-pin-shadow-*` attributes are matched by descendant selectors on a nested DataTable's pinned cells, so an inner table paints a freeze seam it hasn't earned. Cosmetic; fixing it means re-scoping pinning CSS shared by every table. Each behavioural fix was verified by reverting it and confirming the new test fails. Refs: tailor-inc/platform-planning#1650 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The nested-DataTable measurement test passed locally and failed in CI with `expected '52px' to be '70px'` — 52px being the declared fallback, i.e. the width was never measured. It stubbed `HTMLElement.prototype.offsetWidth` via `vi.spyOn`. Which link of the prototype chain a DOM implementation defines `offsetWidth` on is not guaranteed; locally it is `HTMLElement`, so the spy took effect, but where it lands elsewhere the spy silently no-ops and every cell measures 0. Stub the two selection headers directly with `Object.defineProperty` instead. Deterministic in any DOM implementation, and it drops the `data-nested` marker the mock needed to tell the tables apart. Verified it still discriminates: removing the `closest("table") !== ownTable` filter makes it fail with the inner table's 40px. Refs: tailor-inc/platform-planning#1650 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The previous attempt (stubbing `offsetWidth` on the two header elements) still failed in CI with the same '52px' vs '70px', so the diagnosis was wrong: it is not only about which prototype owns the property. Cover both plausible causes at once. Resolve the prototype that actually owns `offsetWidth` at runtime instead of assuming `HTMLElement` — installing one if nothing does — and mock there, so the stub survives React recreating the header nodes on re-render, which a per-element stub would not. Also assert the stub is in effect before relying on it. If it ever stops working the test now fails at that guard, naming the cause, instead of at the layout assertion where a 0 measurement is indistinguishable from a real pinning regression reading the 52px declared fallback. Still discriminates: removing the `closest("table") !== ownTable` filter fails with the inner table's 40px. Refs: tailor-inc/platform-planning#1650 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Temporary. Locally the outer selection column measures at mount and the pin offset is already 70px before the forced re-measure, so the CI failure is somewhere in the measurement pipeline rather than the stub — the stub guard added in the previous commit passes there. Log the intermediate state to find out which, plus assert the preconditions the production filter relies on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

CI reports offsetWidth 70 on the element and all closest() preconditions pass, yet the pin offset stays at the 52px declared fallback both before and after the forced re-measure. So the width never reaches the widths state. Probe the exact selector the measure effect uses, and which prototype the stub landed on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

CI diagnostics settled it. There, the DOM is correct — the stub reports offsetWidth 70 on the outer selection header and 40 on the inner, the measure selector finds all 7 header cells, and `closest("table")` resolves both ways as expected — yet the pin offset stays at the declared 52px both before and after the forced re-measure. The component's layout effect reads 0 where the test reads 70: that environment never feeds real geometry to the measure effect, which is also why every other pinning test in this file asserts declared fallbacks (52px) rather than measured ones. So `expect(left).toBe("70px")` was asserting one environment's layout behaviour, not the behaviour under test. Assert the actual invariant instead: the inner table's width must never reach the outer table's offsets. Removing the `closest("table") !== ownTable` filter still fails this with "40px". The trade is honest: where measurement is inert, the test cannot distinguish "filtered correctly" from "measured nothing", so it guards the regression where the environment permits and never false-fails where it does not. Diagnostics removed. Refs: tailor-inc/platform-planning#1650 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

erickteowarang · 2026-08-12T07:01:22Z

@copilot Can you review the PR with a focus on accessibility, performance, and if it's introducing any anti-patterns in React?

Copilot · 2026-08-12T07:04:02Z

@copilot Can you review the PR with a focus on accessibility, performance, and if it's introducing any anti-patterns in React?

Here's a focused review across the three axes:

Accessibility ✅ Solid overall

aria-expanded + aria-controls: Set correctly — aria-controls is only emitted while expanded so it never points at an absent id. ✅
inert on closing panel: Prevents Tab from landing on invisible controls during the collapse animation. ✅
Focus return on collapse: useIsomorphicLayoutEffect hands focus back to the trigger before inert is set (which itself would drop focus to <body>), and the cleanup handles the row-removal edge case. ✅
Named region (<section aria-label>): Correct — avoids role="region" which the repo's jsx-a11y/prefer-tag-over-role lint rule would reject anyway. ✅
Contextual button names via expandRowLabel: "Expand row INV-1001" vs. repeating "Expand row" is the right call. ✅
motion-reduce:transition-none: Present on both the chevron and the reveal grid. ✅
Screen-reader row count: Documented known limitation. The trade-offs of aria-rowcount/aria-rowindex vs role="presentation" are real — neither is safe without full-table coordination. Acceptable as documented. ⚠️

Performance ✅ No significant issues

Render-prop thunk (render={() => renderExpandedRow(row)}): Creates a new arrow per render, but it's only called during mount/open. No measurable cost.
Ref-based callback identity for onExpandedChange and expandedRowIds: Prevents toggleRowExpansion/collapseAllRows from churning on every state change, and correctly prevents the "collapse immediately after open" loop in page-change effects. ✅
DataTableExpandedRow only mounted for expandable rows: Non-expandable tables and id-less rows don't pay the render/animation overhead. ✅
useMemo/useCallback coverage: The hook return values are all stable references. ✅

Anti-patterns ⚠️ One real one, two minor notes

1. Side effect inside a state updater (real, acknowledged)

In toggleRowExpansionImpl (uncontrolled path):

setUncontrolledExpandedIds((prev) => {
  const next = new Set(prev);
  …
  onExpandedChangeRef.current?.([...next]);  // ← side effect in updater
  return next;
});

React documents that state updaters must be pure, and StrictMode double-invokes them specifically to surface this. onExpandedChange will fire twice per toggle in dev/StrictMode. This mirrors the existing toggleRowSelection pattern, so it's consistent — but both are technically wrong. A clean fix for both would be to call the callback in a useEffect that watches the state, or to batch the state write and callback dispatch together outside the updater (e.g. compute next first, then call setState(next) and onSelectionChange(next) in the same event handler). Worth noting for a follow-up, especially since this affects the public onExpandedChange API.

2. Inline IIFE pattern in JSX (style, not a bug)

The selection, expand, and actions cells use (() => { … })() inside JSX to scope variables. It works, but renderSelectionCell(…) / renderExpandCell(…) helper functions outside the rows.map would make the render branch more readable and give React a named component boundary in the profiler.

3. eslint-disable-next-line on the warning useEffect (acceptable)

The dependency array intentionally uses boolean coercions (!!renderExpandedRow, !onExpandedChange) rather than the actual values to avoid re-running the warning on every render caused by inline arrows. The comment explains this clearly, and the suppression is scoped to one line. ✅

Summary: The implementation is well-engineered. The only actionable concern is the callback-inside-updater anti-pattern shared with the existing selection code — the expansion surface is otherwise clean.

…pdater `toggleRowSelection` and `toggleRowExpansion` called their change callbacks from inside a `setState` updater. Updaters must be pure, and StrictMode double-invokes them precisely to surface that, so both callbacks fired twice per toggle in development — doubling whatever a consumer does in response. Compute the next set from a ref mirroring current state, then set state and dispatch from the handler. This is the shape `selectAllRows` and `clearSelection` already used, so the selection block is now internally consistent too. `onSelectionChange` is shipped public behaviour, so the changeset calls the timing change out. Both toggles keep the documented caveat that two toggles dispatched before state commits share a base. Tests render under StrictMode and assert exactly one call per toggle; both fail when reverted to the in-updater form. Also trims the comments added across this branch — roughly half the added comment lines — and fixes one that had drifted onto the wrong effect. Refs: tailor-inc/platform-planning#1650 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

erickteowarang · 2026-08-12T23:33:14Z

@copilot I've fixed the last batch of feedback, can you do another pass of a review?

Copilot · 2026-08-12T23:35:51Z

@copilot I've fixed the last batch of feedback, can you do another pass of a review?

All three points from the previous review are now resolved:

Side effect in state updater — fixed ✅

Both toggleRowSelection and toggleRowExpansionImpl now compute the next set from a mirroring ref (selectedRowIdsRef.current / expandedRowIdsRef.current), then call setState and the callback as two separate statements in the event handler. The new StrictMode tests (calls onSelectionChange once per toggle, calls onExpandedChange once per toggle) pin the fix and would both fail on revert.

One small remaining asymmetry worth noting: toggleRowSelection is still not wrapped in useCallback (it creates a new function each render when onSelectionChange is defined), while toggleRowExpansionImpl is. Neither causes a bug — selectedRowIdsRef.current is always current so there's no stale-state risk — but it is inconsistent. Pre-existing for selection, so out of scope here.

onExpandedChange TSDoc updated ✅

The stale caveat ("fires from inside a state updater, expect two calls under StrictMode") is replaced with "fires exactly once per toggle, including under StrictMode." The controlled-mode batching caveat is retained (it's still true: both modes read from the ref, not a functional update, so rapid-fire toggles share a base).

Comment volume ✅

Roughly half the added comment lines trimmed. The implementation logic is unchanged; all substantive explanation is kept, just tighter.

No new accessibility, performance, or anti-pattern concerns introduced by this commit.

erickteowarang · 2026-08-13T06:21:22Z

@copilot based on the patterns of other expandable tables using shadcn, can you evaluate the current approach and let me know if there are any factors that can be improved?

IzumiSy · 2026-08-14T06:25:03Z

@erickteowarang
I like the expandable-row feature itself, but I think the public API surface is a bit too flat right now.

At the moment the feature is spread across top-level options like renderExpandedRow, canExpandRow, expandRowLabel, expandedIds, and onExpandedChange, even though these are all part of the same concern. I think this would be easier to understand and evolve if it were grouped under a single rowExpansion option instead.

Something along these lines:

rowExpansion?: {
  render: (row: TRow) => ReactNode;
  canExpand?: (row: TRow) => boolean;
  getLabel?: (row: TRow) => string;
} & (
  | {
      expandedIds: string[];
      onChange: (ids: string[]) => void;
    }
  | {
      expandedIds?: never;
      onChange?: never;
    }
);

The main benefit is not just namespacing — it also lets the type system prevent partial / inert configurations:

no getLabel without expansion actually being enabled
no canExpand by itself
no controlled state with expandedIds but no onChange

In other words, it makes the feature feel more like a single coherent API instead of several loosely related top-level props.

IzumiSy · 2026-08-14T06:31:57Z

+  // Controlled without `onExpandedChange` means every chevron is inert — the
+  // toggle hands the next set to a callback that isn't there. Warn, don't fail
+  // silently.
+  useEffect(() => {
+    if (renderExpandedRow && isExpansionControlled && !onExpandedChange) {
+      console.warn(
+        "[DataTable] `expandedIds` was provided without `onExpandedChange`: expansion is controlled, so the built-in chevrons cannot change state and will do nothing when activated. Pass `onExpandedChange`, or drop `expandedIds` to let useDataTable own the state.",
+      );
+    }
+    // Presence, not identity — these are usually inline arrows, and depending on
+    // them would turn a one-off notice into per-render console spam.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [!!renderExpandedRow, isExpansionControlled, !onExpandedChange]);


If we tighten the API shape here as I commented (#430 (comment)), I think this useEffect can go away entirely.

It looks like it's only guarding an invalid configuration (expandedIds without onExpandedChange), which is something the type system should be able to prevent for us.

erickteowarang and others added 8 commits August 12, 2026 12:05

Copilot started work on behalf of erickteowarang August 12, 2026 07:01 View session

Copilot finished work on behalf of erickteowarang August 12, 2026 07:04

Copilot started work on behalf of erickteowarang August 12, 2026 23:33 View session

Copilot finished work on behalf of erickteowarang August 12, 2026 23:36

Copilot started work on behalf of erickteowarang August 13, 2026 06:21 View session

Copilot finished work on behalf of erickteowarang August 13, 2026 06:24

tailor-platform deleted a comment from Copilot AI Aug 13, 2026

IzumiSy reviewed Aug 14, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat(data-table): add expandable rows with inline detail panel - #430

feat(data-table): add expandable rows with inline detail panel#430
erickteowarang wants to merge 9 commits into
mainfrom
feat/add-expandable-row-data-table

erickteowarang commented Aug 12, 2026

Uh oh!

erickteowarang commented Aug 12, 2026

Uh oh!

Copilot AI commented Aug 12, 2026

Uh oh!

erickteowarang commented Aug 12, 2026

Uh oh!

Copilot AI commented Aug 12, 2026

Uh oh!

erickteowarang commented Aug 13, 2026

Uh oh!

IzumiSy commented Aug 14, 2026

Uh oh!

IzumiSy Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

erickteowarang commented Aug 12, 2026

What

Key decisions

Accessibility

Testing

Reviewer notes

Uh oh!

erickteowarang commented Aug 12, 2026

Uh oh!

Copilot AI commented Aug 12, 2026

Accessibility ✅ Solid overall

Performance ✅ No significant issues

Anti-patterns ⚠️ One real one, two minor notes

Uh oh!

erickteowarang commented Aug 12, 2026

Uh oh!

Copilot AI commented Aug 12, 2026

Uh oh!

erickteowarang commented Aug 13, 2026

Uh oh!

IzumiSy commented Aug 14, 2026

Uh oh!

IzumiSy Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants