From 4e721da5fa840453831f9578823b9ee32f4269f3 Mon Sep 17 00:00:00 2001 From: Erick Teowarang Date: Wed, 12 Aug 2026 12:05:12 +1000 Subject: [PATCH 1/9] feat(data-table): add expandable rows with inline detail panel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ``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 --- .changeset/lazy-hounds-smoke.md | 18 + catalogue/src/fundamental/components.md | 2 +- docs/components/data-table.md | 79 ++++- .../src/pages/data-table-lab/page.tsx | 77 +++++ .../data-table/data-table-context.tsx | 20 +- .../components/data-table/data-table.test.tsx | 299 +++++++++++++++++ .../src/components/data-table/data-table.tsx | 316 ++++++++++++++++-- .../core/src/components/data-table/i18n.ts | 21 ++ .../core/src/components/data-table/types.ts | 58 ++++ .../components/data-table/use-data-table.ts | 70 ++++ 10 files changed, 924 insertions(+), 36 deletions(-) create mode 100644 .changeset/lazy-hounds-smoke.md diff --git a/.changeset/lazy-hounds-smoke.md b/.changeset/lazy-hounds-smoke.md new file mode 100644 index 00000000..f9523001 --- /dev/null +++ b/.changeset/lazy-hounds-smoke.md @@ -0,0 +1,18 @@ +--- +"@tailor-platform/app-shell": minor +--- + +Add expandable detail rows to `DataTable`. Pass `renderExpandedRow` to `useDataTable` and each row gets a chevron column (auto-pinned to the left edge, after the selection column) that reveals a full-width detail panel beneath the row — nothing new to compose in JSX. + +```tsx +const table = useDataTable({ + columns, + data, + control, + renderExpandedRow: (row) => , + canExpandRow: (row) => row.lineItemCount > 0, + expandRowLabel: (row) => row.orderNumber, // → "Expand row INV-1001" +}); +``` + +Rows must have a string or number `id` (the same constraint as row selection); rows without one render no chevron. Several rows can be open at once, and expansion survives page changes — `collapseAllRows()` resets it. Pass `expandedIds` + `onExpandedChange` to control expansion yourself. diff --git a/catalogue/src/fundamental/components.md b/catalogue/src/fundamental/components.md index 9e8ee544..3e59efe1 100644 --- a/catalogue/src/fundamental/components.md +++ b/catalogue/src/fundamental/components.md @@ -387,7 +387,7 @@ Plus `badgeVariants` CVA for custom-styled siblings. **Import:** compound namespace + helpers from `'@tailor-platform/app-shell'`, e.g. `DataTable`, `useDataTable`, `useCollectionVariables`, `createColumnHelper`, and types such as `Column`, `UseDataTableReturn`. -**Purpose:** Production list screens over GraphQL **connections**. Owns toolbar filter chips (**`DataTable.Filters`** from column `filter` configs), header sort, **`DataTable.Pagination`** (cursor-first; First/Last when `total` is provided), loading skeleton/error row, **`onClickRow`**, **`rowActions`** (kebab column), **`onSelectionChange`** (checkbox column), **column pinning** (`pin: "left" | "right"`) and built-in **column settings** (`` — user show/hide + reorder + pin, persisted per-user via **`tableId`**). +**Purpose:** Production list screens over GraphQL **connections**. Owns toolbar filter chips (**`DataTable.Filters`** from column `filter` configs), header sort, **`DataTable.Pagination`** (cursor-first; First/Last when `total` is provided), loading skeleton/error row, **`onClickRow`**, **`rowActions`** (kebab column), **`onSelectionChange`** (checkbox column), **expandable rows** (**`renderExpandedRow`** — a chevron column plus a full-width detail panel per row, with `canExpandRow` / `expandRowLabel` and controlled `expandedIds`), **column pinning** (`pin: "left" | "right"`) and built-in **column settings** (`` — user show/hide + reorder + pin, persisted per-user via **`tableId`**). **Primitives:** Builds on low-level **`Table`**; do not reinvent pagination/filters manually unless the dataset is trivial. diff --git a/docs/components/data-table.md b/docs/components/data-table.md index 8788a9e2..acb10ace 100644 --- a/docs/components/data-table.md +++ b/docs/components/data-table.md @@ -213,7 +213,7 @@ Row selection is enabled by providing `onSelectionChange` to `useDataTable`. The ## Column pinning, visibility & ordering -- **Pin** a column with `pin: "left" | "right"`. Pinned columns stay visible during horizontal scroll; the selection column auto-pins left and the row-actions column auto-pins right. A subtle shadow appears at the frozen edge once the table is scrolled under it. Sticky offsets are measured from the rendered layout, so a `width` isn't required — but setting `width` on pinned columns is recommended so their size stays stable as content changes. +- **Pin** a column with `pin: "left" | "right"`. Pinned columns stay visible during horizontal scroll; the selection and expand columns auto-pin left and the row-actions column auto-pins right. A subtle shadow appears at the frozen edge once the table is scrolled under it. Sticky offsets are measured from the rendered layout, so a `width` isn't required — but setting `width` on pinned columns is recommended so their size stays stable as content changes. - **Column settings.** Pass `columnSettings` to `DataTable.Toolbar` to render a built-in "Columns" control — a popover to show/hide columns, reorder them (drag), and change pinning by dragging a column between the **Fixed left**, **Scrollable**, and **Fixed right** zones. It's a toolbar prop (not a composed sub-component) because the control always sits in the same top-right position. - **Persistence.** Pass a stable, **unique** `tableId` to persist each user's column layout (visibility, order, pinning) to `localStorage` (key `as:data-table:v1:`). This is a per-user preference — it is deliberately **not** stored in the URL like filters/sort/pagination, so it survives reloads and isn't reset by shared/filtered links. Omit `tableId` for in-memory-only layout (state simply isn't persisted). Two tables mounted with the same `tableId` share one storage key and overwrite each other — use a unique id per table (e.g. `:`); a dev-mode warning fires on duplicates. @@ -230,6 +230,52 @@ const table = useDataTable({ ; ``` +## Expandable rows + +Pass `renderExpandedRow` to `useDataTable` and each row gets a chevron that reveals a detail panel beneath it. Providing the prop is what enables the feature — a dedicated chevron column is added at the left edge (auto-pinned left, after the selection column) and the detail row renders automatically. There is nothing new to compose in JSX. + +```tsx +const table = useDataTable({ + columns, + data, + control, + renderExpandedRow: (row) => , + canExpandRow: (row) => row.lineItemCount > 0, + expandRowLabel: (row) => row.orderNumber, +}); +``` + +- **Rows must have an `id`.** Expansion is keyed by `row.id`, the same constraint as row selection. Rows without one render **no chevron** (not a disabled one) — a row must never be un-toggleable. +- **`canExpandRow`** suppresses the chevron per row (e.g. an order with no line items). The cell is still rendered, empty, so the column count stays consistent. +- **`expandRowLabel`** returns a **bare identifier** — `"INV-1001"`, not `"Expand row INV-1001"`. The built-in i18n labels compose it into the trigger's accessible name (`"Expand row INV-1001"`) and the panel's (`"INV-1001 details"`), so English and Japanese word order both stay correct. Without it, the generic "Expand row" / "Row details" strings are used — set it on any table with more than a couple of rows. +- **`onClickRow` is unaffected.** The chevron lives in its own column and stops click propagation, so row-level navigation keeps working. +- **Expansion survives page changes.** Ids of rows that are no longer on the page simply don't render. Call `collapseAllRows()` to reset. +- **Multiple rows can be open at once.** There is no accordion / single-open mode. + +### Controlled mode + +Pass `expandedIds` to own the state yourself; internal state is then never written and you update the array from `onExpandedChange`. + +```tsx +const [expandedIds, setExpandedIds] = useState([]); + +const table = useDataTable({ + columns, + data, + renderExpandedRow: (row) => , + expandedIds, + onExpandedChange: setExpandedIds, +}); +``` + +`useDataTable` also returns `expandedIds`, `isRowExpanded(row)`, `toggleRowExpansion(row)`, and `collapseAllRows()`. The last two are `undefined` when `renderExpandedRow` is not provided. + +### Accessibility + +The trigger is a native ` + )} + />, + { wrapper }, + ); + + fireEvent.click(screen.getAllByLabelText("Expand row")[0]); + + const inner = screen.getByTestId("inner"); + act(() => inner.focus()); + expect(document.activeElement).toBe(inner); + + const trigger = screen.getByLabelText("Collapse row"); + fireEvent.click(trigger); + + // Without the handoff, focus would fall to and the user would + // lose their place in the table. + expect(document.activeElement).toBe(trigger); + }); + + it("renders the matching rows open from a controlled expandedIds", () => { + const { container } = render( + , + { wrapper }, + ); + + expect(screen.getByText("Details for Bob")).toBeDefined(); + expect(screen.queryByText("Details for Alice")).toBeNull(); + expect(container.querySelectorAll(EXPANDED_ROW)).toHaveLength(1); + }); + + it("reports controlled toggles through onExpandedChange without changing internal state", () => { + const onExpandedChange = vi.fn(); + render( + , + { wrapper }, + ); + + fireEvent.click(screen.getAllByLabelText("Expand row")[0]); + + expect(onExpandedChange).toHaveBeenCalledWith(["1"]); + // The caller owns the state — nothing opens until `expandedIds` changes. + expect(screen.queryByText("Details for Alice")).toBeNull(); + }); + + it("pins the expand column left at the selection column's measured offset", () => { + const { container } = render( + , + { wrapper }, + ); + + const th = container.querySelector(EXPAND_TH); + expect(th?.style.position).toBe("sticky"); + expect(th?.style.left).toBe("52px"); + // The freeze seam moves off the selection column onto the expand column. + const selectionTh = container.querySelector( + '[data-slot="data-table-header"] th[data-col-key="__datatable_selection__"]', + ); + expect(th?.className).toContain("data-pin-shadow-left"); + expect(selectionTh?.className).not.toContain("data-pin-shadow-left"); + }); + + it("includes an expand cell in the skeleton rows", () => { + const { container } = render( + , + { wrapper }, + ); + + const loadingRow = container.querySelector('[data-datatable-state="loading"]'); + // 1 expand + 2 visible columns — row heights match before and after load. + expect(loadingRow?.querySelectorAll("td")).toHaveLength(3); + }); + + it("spans the expand column in the empty and error status rows", () => { + const { container: emptyContainer } = render( + , + { wrapper }, + ); + expect( + emptyContainer.querySelector('[data-datatable-state="empty"] td')?.getAttribute("colspan"), + ).toBe("3"); + + const { container: errorContainer } = render( + , + { wrapper }, + ); + expect( + errorContainer.querySelector('[data-datatable-state="error"] td')?.getAttribute("colspan"), + ).toBe("3"); + }); + }); + // ------------------------------------------------------------------------- // Sticky / pinned columns // ------------------------------------------------------------------------- diff --git a/packages/core/src/components/data-table/data-table.tsx b/packages/core/src/components/data-table/data-table.tsx index 115c0a42..74adfa45 100644 --- a/packages/core/src/components/data-table/data-table.tsx +++ b/packages/core/src/components/data-table/data-table.tsx @@ -1,7 +1,9 @@ import { createContext, + Fragment, useContext, useEffect, + useId, useLayoutEffect, useMemo, useRef, @@ -9,7 +11,7 @@ import { type CSSProperties, type ReactNode, } from "react"; -import { Ellipsis } from "lucide-react"; +import { ChevronRight, Ellipsis } from "lucide-react"; import { cn } from "@/lib/utils"; import { CollectionControlProvider } from "@/contexts/collection-control-context"; import { Table } from "@/components/table"; @@ -83,9 +85,11 @@ function renderDefaultHeader( // innermost anchors before the real widths are measured. const SELECTION_WIDTH = 52; const ACTIONS_WIDTH = 50; +const EXPAND_WIDTH = 44; // Keys for the built-in selection / row-actions columns in the measured-width map. const SELECTION_KEY = "__datatable_selection__"; const ACTIONS_KEY = "__datatable_actions__"; +const EXPAND_KEY = "__datatable_expand__"; type PinSide = "left" | "right"; type ColumnWidths = Record; @@ -118,6 +122,8 @@ interface PinLayout> { placements: Map, PinPlacement>; /** Placement for the built-in selection column, when pinned. */ selection?: PinPlacement; + /** Placement for the built-in expand column, when present. */ + expand?: PinPlacement; /** Placement for the built-in row-actions column, when pinned. */ actions?: PinPlacement; } @@ -176,9 +182,9 @@ function buildColumnKeys>( /** * Group the visible columns by pin side and compute cumulative sticky offsets. - * The selection column (if present) auto-pins to the left edge and row-actions - * (if present) auto-pins to the right edge, with user-pinned columns stacking - * outward from them. + * The selection and expand columns (when present) auto-pin to the left edge and + * row-actions (if present) auto-pins to the right edge, with user-pinned columns + * stacking outward from them. * * `columnKeys` is the definition-order key map (see {@link buildColumnKeys}); * `columns` here is the visible/reordered subset, so keys are resolved by @@ -189,12 +195,13 @@ function computePinLayout>( pinnedColumns: Record, opts: { hasSelection: boolean; + hasExpand: boolean; hasRowActions: boolean; widths: ColumnWidths; columnKeys: Map, string>; }, ): PinLayout { - const { hasSelection, hasRowActions, widths, columnKeys } = opts; + const { hasSelection, hasExpand, hasRowActions, widths, columnKeys } = opts; const keyOf = (col: Column): string => columnKeys.get(col) as string; @@ -214,14 +221,21 @@ function computePinLayout>( const placements = new Map, PinPlacement>(); - // Left group, visually [selection?, ...left]; offsets accumulate rightward - // using the measured (or declared) width of each preceding pinned column. + // Left group, visually [selection?, expand?, ...left]; offsets accumulate + // rightward using the measured (or declared) width of each preceding pinned + // column. `isBoundary` marks the outermost cell of the group — the one that + // draws the freeze seam — so it moves to `expand` as soon as that column exists. let leftOffset = 0; let selection: PinPlacement | undefined; + let expand: PinPlacement | undefined; if (hasSelection) { - selection = { side: "left", offset: 0, isBoundary: left.length === 0 }; + selection = { side: "left", offset: 0, isBoundary: !hasExpand && left.length === 0 }; leftOffset = resolveWidth(SELECTION_KEY, SELECTION_WIDTH, widths); } + if (hasExpand) { + expand = { side: "left", offset: leftOffset, isBoundary: left.length === 0 }; + leftOffset += resolveWidth(EXPAND_KEY, EXPAND_WIDTH, widths); + } left.forEach((col, i) => { placements.set(col, { side: "left", offset: leftOffset, isBoundary: i === left.length - 1 }); leftOffset += resolveWidth(keys.get(col) as string, col.width, widths); @@ -240,7 +254,7 @@ function computePinLayout>( rightOffset += resolveWidth(keys.get(col) as string, col.width, widths); } - return { ordered: [...left, ...middle, ...right], keys, placements, selection, actions }; + return { ordered: [...left, ...middle, ...right], keys, placements, selection, expand, actions }; } /** @@ -301,6 +315,7 @@ interface DataTableLoaderRowsProps> { rowCount: number; pinLayout: PinLayout; hasSelection: boolean; + hasExpand: boolean; hasRowActions: boolean; } @@ -312,9 +327,10 @@ function DataTableLoaderRows>({ rowCount, pinLayout, hasSelection, + hasExpand, hasRowActions, }: DataTableLoaderRowsProps) { - const { ordered: columns, keys, placements, selection, actions } = pinLayout; + const { ordered: columns, keys, placements, selection, expand, actions } = pinLayout; // No fixed row height: each cell's placeholder matches the height of the // real content it stands in for (text line, badge, icon button), so the // skeleton rows resolve to exactly the same row height as loaded rows and @@ -336,6 +352,23 @@ function DataTableLoaderRows>({ ); })()} + {hasExpand && + (() => { + const { style, className } = pinCellProps( + expand, + { style: { width: EXPAND_WIDTH } }, + "body", + ); + return ( + + {/* size-9 box = the real chevron Button's footprint, so the + skeleton row resolves to the same height as a loaded row */} +
+
+
+ + ); + })()} {columns?.map((col, colIndex) => { const key = keys.get(col) as string; const skeletonWidth = SKELETON_WIDTHS[(rowIndex + colIndex) % SKELETON_WIDTHS.length]; @@ -467,6 +500,13 @@ function DataTableRoot>({ clearSelection: value.clearSelection, isAllSelected: value.isAllSelected, isIndeterminate: value.isIndeterminate, + expandedIds: value.expandedIds, + isRowExpanded: value.isRowExpanded, + toggleRowExpansion: value.toggleRowExpansion, + collapseAllRows: value.collapseAllRows, + renderExpandedRow: value.renderExpandedRow, + canExpandRow: value.canExpandRow, + expandRowLabel: value.expandRowLabel, }; const controlValue = value.control ?? null; @@ -523,16 +563,24 @@ function DataTableHeaders({ className: headerClassName }: { className?: string } clearSelection, isAllSelected, isIndeterminate, + renderExpandedRow, } = ctx; const t = useDataTableT(); const widths = useContext(PinMeasureContext); const hasSelection = !!toggleRowSelection; + const hasExpand = !!renderExpandedRow; const hasRowActions = !!(rowActions && rowActions.length > 0); const columnKeys = useMemo(() => buildColumnKeys(allColumns), [allColumns]); - const { ordered, keys, placements, selection, actions } = useMemo( + const { ordered, keys, placements, selection, expand, actions } = useMemo( () => - computePinLayout(columns, pinnedColumns, { hasSelection, hasRowActions, widths, columnKeys }), - [columns, pinnedColumns, hasSelection, hasRowActions, widths, columnKeys], + computePinLayout(columns, pinnedColumns, { + hasSelection, + hasExpand, + hasRowActions, + widths, + columnKeys, + }), + [columns, pinnedColumns, hasSelection, hasExpand, hasRowActions, widths, columnKeys], ); return ( @@ -573,6 +621,19 @@ function DataTableHeaders({ className: headerClassName }: { className?: string } ); })()} + {hasExpand && + (() => { + const { style, className } = pinCellProps( + expand, + { style: { width: EXPAND_WIDTH } }, + "header", + ); + return ( + + {t("expandColumnHeader")} + + ); + })()} {ordered?.map((col, index) => { const key = keys.get(col) as string; const label = col.label; @@ -600,7 +661,7 @@ function DataTableHeaders({ className: headerClassName }: { className?: string } const content = col.header ? col.header(headerContext) : renderDefaultHeader(label, headerContext, align, { - bleedLeft: !hasSelection && index === 0, + bleedLeft: !hasSelection && !hasExpand && index === 0, bleedRight: !hasRowActions && index === ordered.length - 1, }); @@ -672,18 +733,31 @@ function DataTableBody({ className }: { className?: string }) { isRowSelected, toggleRowSelection, pageSize, + renderExpandedRow, + canExpandRow, + expandRowLabel, + isRowExpanded, + toggleRowExpansion, } = ctx; const t = useDataTableT(); const widths = useContext(PinMeasureContext); const hasRowActions = !!(rowActions && rowActions.length > 0); const hasSelection = !!toggleRowSelection; - const totalColSpan = (columns?.length ?? 1) + (hasRowActions ? 1 : 0) + (hasSelection ? 1 : 0); + const hasExpand = !!renderExpandedRow; + const totalColSpan = + (columns?.length ?? 1) + (hasRowActions ? 1 : 0) + (hasSelection ? 1 : 0) + (hasExpand ? 1 : 0); const rowCount = pageSize > 0 ? pageSize : DEFAULT_ROWS; const columnKeys = useMemo(() => buildColumnKeys(allColumns), [allColumns]); const pinLayout = useMemo( () => - computePinLayout(columns, pinnedColumns, { hasSelection, hasRowActions, widths, columnKeys }), - [columns, pinnedColumns, hasSelection, hasRowActions, widths, columnKeys], + computePinLayout(columns, pinnedColumns, { + hasSelection, + hasExpand, + hasRowActions, + widths, + columnKeys, + }), + [columns, pinnedColumns, hasSelection, hasExpand, hasRowActions, widths, columnKeys], ); const tableBodyProps = { "data-slot": "data-table-body", @@ -697,6 +771,7 @@ function DataTableBody({ className }: { className?: string }) { rowCount={rowCount} pinLayout={pinLayout} hasSelection={hasSelection} + hasExpand={hasExpand} hasRowActions={hasRowActions} /> @@ -731,11 +806,18 @@ function DataTableBody({ className }: { className?: string }) { rows={rows} pinLayout={pinLayout} hasSelection={hasSelection} + hasExpand={hasExpand} hasRowActions={hasRowActions} isRowSelected={isRowSelected} toggleRowSelection={toggleRowSelection} rowActions={rowActions} onClickRow={onClickRow} + totalColSpan={totalColSpan} + renderExpandedRow={renderExpandedRow} + canExpandRow={canExpandRow} + expandRowLabel={expandRowLabel} + isRowExpanded={isRowExpanded} + toggleRowExpansion={toggleRowExpansion} /> ); @@ -750,11 +832,18 @@ interface DataTableRowsProps> { rows: TRow[]; pinLayout: PinLayout; hasSelection: boolean; + hasExpand: boolean; hasRowActions: boolean; isRowSelected: (row: TRow) => boolean; toggleRowSelection?: (row: TRow) => void; rowActions?: RowAction[]; onClickRow?: (row: TRow) => void; + totalColSpan: number; + renderExpandedRow?: (row: TRow) => ReactNode; + canExpandRow?: (row: TRow) => boolean; + expandRowLabel?: (row: TRow) => string; + isRowExpanded: (row: TRow) => boolean; + toggleRowExpansion?: (row: TRow) => void; } /** @internal */ @@ -762,23 +851,37 @@ function DataTableRows>({ rows, pinLayout, hasSelection, + hasExpand, hasRowActions, isRowSelected, toggleRowSelection, rowActions, onClickRow, + totalColSpan, + renderExpandedRow, + canExpandRow, + expandRowLabel, + isRowExpanded, + toggleRowExpansion, }: DataTableRowsProps) { const t = useDataTableT(); - const { ordered, keys, placements, selection, actions } = pinLayout; + const baseId = useId(); + const { ordered, keys, placements, selection, expand, actions } = pinLayout; return ( <> {rows.map((row, rowIndex) => { const rowId = (row as Record)["id"]; const selected = isRowSelected?.(row) ?? false; - return ( + const rowKey = rowId != null ? String(rowId) : String(rowIndex); + // Expansion is keyed by id, so a row without one gets no chevron at all + // rather than a disabled one — it must never be un-toggleable (D5). + const expandable = hasExpand && rowId != null && (canExpandRow?.(row) ?? true); + const expanded = expandable && isRowExpanded(row); + const panelId = `${baseId}-panel-${rowKey}`; + const triggerId = `${baseId}-trigger-${rowKey}`; + const dataRow = ( >({ ); })()} + {hasExpand && + (() => { + const { style, className } = pinCellProps( + expand, + { style: { width: EXPAND_WIDTH } }, + "body", + ); + return ( + // `stopPropagation` so using the chevron never fires `onClickRow` + // (the selection cell does the same). Non-expandable rows still + // render the cell — empty — so the column count stays consistent. + e.stopPropagation()} + > + {expandable && toggleRowExpansion && ( + toggleRowExpansion(row)} + /> + )} + + ); + })()} {ordered?.map((col) => { const key = keys.get(col) as string; const content = col.render ? col.render(row) : renderTypedCell(row, col); @@ -902,11 +1033,152 @@ function DataTableRows>({ })()} ); + + // `expanded` is always false without `renderExpandedRow`, so a table + // that doesn't use the feature renders exactly the rows it did before. + return ( + + {dataRow} + {expanded && renderExpandedRow && ( + + {renderExpandedRow(row)} + + )} + + ); })} ); } +// ============================================================================= +// RowExpandToggle (internal) +// ============================================================================= + +interface RowExpandToggleProps { + id: string; + expanded: boolean; + label: string | undefined; + panelId: string; + onToggle: () => void; +} + +/** + * The chevron trigger. A native ` + ); +} + +// ============================================================================= +// DataTableExpandedRow (internal) +// ============================================================================= + +interface DataTableExpandedRowProps { + totalColSpan: number; + panelId: string; + triggerId: string; + label: string | undefined; + children: ReactNode; +} + +/** + * The detail row: one full-width `` with a single `colSpan` cell, rendered + * immediately after its parent row so forward-tabbing reaches the panel next. + * + * @internal + */ +function DataTableExpandedRow({ + totalColSpan, + panelId, + triggerId, + label, + children, +}: DataTableExpandedRowProps) { + const t = useDataTableT(); + const panelRef = useRef(null); + + // If focus is inside the panel when it unmounts, it would fall to and + // the user would lose their place. Hand it back to the trigger first. A layout + // effect so the cleanup runs before React removes the nodes. + useIsomorphicLayoutEffect(() => { + const panel = panelRef.current; + return () => { + if (panel && panel.contains(document.activeElement)) { + document.getElementById(triggerId)?.focus(); + } + }; + }, [triggerId]); + + return ( + + + {/* The colSpan cell spans the whole table, so on a horizontally scrolled + table the panel would otherwise sit off-screen. Pin it to the left + edge of the scrollport. `min(100%, …)` makes this a no-op when the + table fits its container: 100% of the cell is then narrower than the + viewport and sticky has nothing to do. */} +
+ {/* A named `
` maps to `role="region"`, so the panel announces + as a named region rather than as loose table content. */} +
+ {children} +
+
+ + + ); +} + // ============================================================================= // RowActionsMenu (internal — uses app-shell Menu) // ============================================================================= @@ -987,6 +1259,10 @@ function DataTableTable({ className }: { className?: string }) { if (key) next[key] = cell.offsetWidth; }); setWidths((prev) => (sameWidths(prev, next) ? prev : next)); + // Publish the scrollport's own width so an expanded row's sticky panel can + // cap itself to it. Set here (not in the scroll effect) so it updates on + // resize rather than on every scroll event. + el.style.setProperty("--data-table-viewport", `${el.clientWidth}px`); }; measure(); if (typeof ResizeObserver === "undefined") return; diff --git a/packages/core/src/components/data-table/i18n.ts b/packages/core/src/components/data-table/i18n.ts index 3a508f7c..72c155b3 100644 --- a/packages/core/src/components/data-table/i18n.ts +++ b/packages/core/src/components/data-table/i18n.ts @@ -30,6 +30,17 @@ export const dataTableLabels = defineI18nLabels({ selectAll: "Select all rows", selectRow: "Select row", + // Row expansion. The `…Named` variants take a bare record identity from + // `expandRowLabel` (e.g. "INV-1001") and own the word order, so the + // accessible name reads naturally in each locale. + expandColumnHeader: "Expand", + expandRow: "Expand row", + expandRowNamed: (props: { label: string }) => `Expand row ${props.label}`, + collapseRow: "Collapse row", + collapseRowNamed: (props: { label: string }) => `Collapse row ${props.label}`, + expandedDetails: (props: { label: string }) => `${props.label} details`, + expandedDetailsGeneric: "Row details", + // Pagination paginationFirst: "First page", paginationPrevious: "Previous page", @@ -114,6 +125,16 @@ export const dataTableLabels = defineI18nLabels({ selectAll: "全行を選択", selectRow: "行を選択", + + // Row expansion + expandColumnHeader: "展開", + expandRow: "行を展開", + expandRowNamed: (props: { label: string }) => `${props.label}の行を展開`, + collapseRow: "行を折りたたむ", + collapseRowNamed: (props: { label: string }) => `${props.label}の行を折りたたむ`, + expandedDetails: (props: { label: string }) => `${props.label}の詳細`, + expandedDetailsGeneric: "行の詳細", + paginationFirst: "最初のページ", paginationPrevious: "前のページ", paginationNext: "次のページ", diff --git a/packages/core/src/components/data-table/types.ts b/packages/core/src/components/data-table/types.ts index 1ed5db49..0bb96aec 100644 --- a/packages/core/src/components/data-table/types.ts +++ b/packages/core/src/components/data-table/types.ts @@ -313,6 +313,48 @@ export type UseDataTableOptions< * the rows on the **current page**, not all pages. */ onSelectionChange?: (ids: string[]) => void; + /** + * Renders the detail panel for an expanded row. Providing this prop enables + * the whole feature: a chevron column is added at the left edge (auto-pinned + * left, after the selection column) and the returned content renders in a + * full-width row directly beneath its parent row. + * + * **Requirement:** Each row must have a string or number `id` field. + * Expansion is keyed by `id`, so rows without one render no chevron and + * cannot be expanded. + * + * **Note:** Expansion is **not** cleared on page change — ids of rows that + * are no longer on the page simply do not render. Call `collapseAllRows()` + * to reset. + */ + renderExpandedRow?: (row: TRow) => ReactNode; + /** + * Decides whether a given row can be expanded. Rows returning `false` render + * an empty cell in place of the chevron. Defaults to `true` for every row + * that has an `id`. Ignored when `renderExpandedRow` is not provided. + */ + canExpandRow?: (row: TRow) => boolean; + /** + * Returns the row's record identity — a **bare identifier** such as + * `"INV-1001"`, not a sentence. It is composed into the accessible names of + * the chevron ("Expand row INV-1001") and the detail panel ("INV-1001 + * details") via the built-in i18n labels, so English and Japanese word order + * both stay correct. Without it, the generic "Expand row" / "Row details" + * fallbacks are used. + */ + expandRowLabel?: (row: TRow) => string; + /** + * Ids of the currently expanded rows. Passing this switches expansion to + * **controlled** mode: internal state is no longer written and the caller is + * responsible for updating this array from `onExpandedChange`. + */ + expandedIds?: string[]; + /** + * Called with the full array of expanded row ids whenever expansion changes. + * Required for controlled mode; optional as a notification in uncontrolled + * mode. + */ + onExpandedChange?: (ids: string[]) => void; /** * Sort behaviour configuration. * @@ -423,6 +465,22 @@ export interface UseDataTableReturn> { clearSelection?: () => void; isAllSelected: boolean; isIndeterminate: boolean; + + // Row expansion + /** Ids of the currently expanded rows. */ + expandedIds: string[]; + /** Whether `row` is currently expanded. Always `false` for rows without an `id`. */ + isRowExpanded: (row: TRow) => boolean; + /** Toggle `row`'s detail panel. Undefined when `renderExpandedRow` is not provided. */ + toggleRowExpansion?: (row: TRow) => void; + /** Collapse every expanded row. Undefined when `renderExpandedRow` is not provided. */ + collapseAllRows?: () => void; + /** Detail-panel renderer (passthrough for `DataTable.Root`). */ + renderExpandedRow?: (row: TRow) => ReactNode; + /** Per-row expandability predicate (passthrough for `DataTable.Root`). */ + canExpandRow?: (row: TRow) => boolean; + /** Per-row identity for accessible names (passthrough for `DataTable.Root`). */ + expandRowLabel?: (row: TRow) => string; } // ============================================================================= diff --git a/packages/core/src/components/data-table/use-data-table.ts b/packages/core/src/components/data-table/use-data-table.ts index 9a4c17fe..7ea5e5fb 100644 --- a/packages/core/src/components/data-table/use-data-table.ts +++ b/packages/core/src/components/data-table/use-data-table.ts @@ -75,6 +75,11 @@ export function useDataTable< onClickRow, rowActions, onSelectionChange, + renderExpandedRow, + canExpandRow, + expandRowLabel, + expandedIds, + onExpandedChange, sort: sortOption, } = options; @@ -341,6 +346,64 @@ export function useDataTable< }); const isIndeterminate = selectedRowIds.size > 0 && !isAllSelected; + // --------------------------------------------------------------------------- + // Row expansion + // --------------------------------------------------------------------------- + // Keyed by the same `getRowId` as selection — there is one row-id convention. + const [uncontrolledExpandedIds, setUncontrolledExpandedIds] = useState>(new Set()); + + // Controlled when the caller passes `expandedIds`; internal state is then never written. + const isExpansionControlled = expandedIds !== undefined; + + const expandedRowIds = useMemo( + () => (isExpansionControlled ? new Set(expandedIds) : uncontrolledExpandedIds), + [isExpansionControlled, expandedIds, uncontrolledExpandedIds], + ); + + const isRowExpanded = useCallback( + (row: TRow) => { + const id = getRowId(row); + return id !== null && expandedRowIds.has(id); + }, + [expandedRowIds, getRowId], + ); + + const toggleRowExpansion = renderExpandedRow + ? (row: TRow) => { + const id = getRowId(row); + if (id === null) return; + if (isExpansionControlled) { + const next = new Set(expandedRowIds); + if (next.has(id)) { + next.delete(id); + } else { + next.add(id); + } + onExpandedChange?.([...next]); + return; + } + setUncontrolledExpandedIds((prev) => { + const next = new Set(prev); + if (next.has(id)) { + next.delete(id); + } else { + next.add(id); + } + onExpandedChange?.([...next]); + return next; + }); + } + : undefined; + + const collapseAllRows = renderExpandedRow + ? () => { + if (!isExpansionControlled) setUncontrolledExpandedIds(new Set()); + onExpandedChange?.([]); + } + : undefined; + + const expandedIdsList = useMemo(() => [...expandedRowIds], [expandedRowIds]); + // --------------------------------------------------------------------------- // Return // --------------------------------------------------------------------------- @@ -383,5 +446,12 @@ export function useDataTable< clearSelection, isAllSelected, isIndeterminate, + expandedIds: expandedIdsList, + isRowExpanded, + toggleRowExpansion, + collapseAllRows, + renderExpandedRow, + canExpandRow, + expandRowLabel, }; } From 73e0238b75026226716c8a1f65bc12d8bc3d2ac8 Mon Sep 17 00:00:00 2001 From: Erick Teowarang Date: Wed, 12 Aug 2026 13:18:31 +1000 Subject: [PATCH 2/9] fix(data-table): harden expandable rows and animate the reveal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 — 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 ``/`` 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 --- docs/components/data-table.md | 27 +- .../src/pages/data-table-lab/page.tsx | 162 +++++++++-- .../components/data-table/data-table.test.tsx | 253 ++++++++++++++++-- .../src/components/data-table/data-table.tsx | 204 ++++++++++++-- .../core/src/components/data-table/types.ts | 17 ++ .../components/data-table/use-data-table.ts | 93 ++++--- 6 files changed, 651 insertions(+), 105 deletions(-) diff --git a/docs/components/data-table.md b/docs/components/data-table.md index acb10ace..876e9c2e 100644 --- a/docs/components/data-table.md +++ b/docs/components/data-table.md @@ -246,7 +246,7 @@ const table = useDataTable({ ``` - **Rows must have an `id`.** Expansion is keyed by `row.id`, the same constraint as row selection. Rows without one render **no chevron** (not a disabled one) — a row must never be un-toggleable. -- **`canExpandRow`** suppresses the chevron per row (e.g. an order with no line items). The cell is still rendered, empty, so the column count stays consistent. +- **`canExpandRow`** suppresses the chevron per row (e.g. an order with no line items). The cell is still rendered, empty, so the column count stays consistent. A row that is _already open_ when `canExpandRow` starts returning `false` keeps both its panel and its chevron, so it can always be closed — the predicate gates opening, never closing. - **`expandRowLabel`** returns a **bare identifier** — `"INV-1001"`, not `"Expand row INV-1001"`. The built-in i18n labels compose it into the trigger's accessible name (`"Expand row INV-1001"`) and the panel's (`"INV-1001 details"`), so English and Japanese word order both stay correct. Without it, the generic "Expand row" / "Row details" strings are used — set it on any table with more than a couple of rows. - **`onClickRow` is unaffected.** The chevron lives in its own column and stops click propagation, so row-level navigation keeps working. - **Expansion survives page changes.** Ids of rows that are no longer on the page simply don't render. Call `collapseAllRows()` to reset. @@ -268,11 +268,28 @@ const table = useDataTable({ }); ``` -`useDataTable` also returns `expandedIds`, `isRowExpanded(row)`, `toggleRowExpansion(row)`, and `collapseAllRows()`. The last two are `undefined` when `renderExpandedRow` is not provided. +`useDataTable` also returns `expandedIds`, `isRowExpanded(row)`, `toggleRowExpansion(row)`, and `collapseAllRows()`. The last two are `undefined` when `renderExpandedRow` is not provided, and both keep a stable identity across renders, so they are safe to list in an effect's dependency array: + +```tsx +// Collapse everything when the page changes. +useEffect(() => { + collapseAllRows?.(); +}, [collapseAllRows, currentPage]); +``` + +`collapseAllRows()` is a no-op when nothing is open — it neither writes state nor fires `onExpandedChange`. + +**`onExpandedChange` is required in controlled mode.** Without it the chevrons have nowhere to send the new state and do nothing when activated; a dev-mode console warning fires to catch this. + +**Batching caveat.** Each toggle derives the next array from the current value of `expandedIds`, not from a functional update. Two toggles dispatched before your state commits both read the same base, so the first is lost. This matters when `expandedIds` lives behind an async store (Redux/Zustand middleware, a debounced URL sync, a `startTransition`), or when looping `toggleRowExpansion` over many rows to build an "expand all". Compute such updates yourself and set `expandedIds` directly rather than driving them through repeated toggles. + +**StrictMode double-fire.** In _uncontrolled_ mode `onExpandedChange` fires from inside a state updater, which React StrictMode intentionally double-invokes — expect two calls per toggle in development. This matches the existing `onSelectionChange` behaviour. Keep the handler idempotent, or move side effects (fetches, analytics) into an effect keyed on the ids. ### Accessibility -The trigger is a native ` + +
+ + ); +} + +function InvoiceDetail({ invoice }: { invoice: Invoice }) { + if (invoice.status === "paid") return ; + if (invoice.status === "overdue") return ; + return ; +} + // ─── Section shell ─────────────────────────────────────────────────────────── function Section({ @@ -357,10 +461,12 @@ const DataTableLabPage = () => { <> Passing renderExpandedRow adds the chevron column at the left edge (auto-pinned after the selection column) and a full-width detail panel beneath each - open row. Several rows can be open at once. canExpandRow hides the - chevron on draft invoices. Scroll horizontally with a row open — the panel - stays pinned to the left edge — and note that clicking the chevron never selects or - triggers the row itself. + open row. Several rows can be open at once. The render prop is just{" "} + (row) => ReactNode, so the panel differs per row here — sent{" "} + shows line items, paid a receipt, overdue a collections view — and{" "} + canExpandRow hides the chevron on draft invoices entirely. + Scroll horizontally with a row open — the panel stays pinned to the left edge — and + note that clicking the chevron never selects or triggers the row itself. } > diff --git a/packages/core/src/components/data-table/data-table.test.tsx b/packages/core/src/components/data-table/data-table.test.tsx index 76ac3559..f809c153 100644 --- a/packages/core/src/components/data-table/data-table.test.tsx +++ b/packages/core/src/components/data-table/data-table.test.tsx @@ -1,5 +1,5 @@ import { afterEach, describe, it, expect, expectTypeOf, vi } from "vitest"; -import { act, cleanup, render, screen, fireEvent } from "@testing-library/react"; +import { act, cleanup, render, screen, fireEvent, waitFor } from "@testing-library/react"; import { MemoryRouter } from "react-router"; import type { ReactNode } from "react"; import { createAppShellWrapper } from "../../../tests/test-utils"; @@ -93,8 +93,14 @@ const headByText = (container: HTMLElement, text: string) => const isTooltipWired = (cell: Element | null) => typeof cell?.id === "string" && cell.id.startsWith("base-ui-"); -// Detail-panel render prop shared by the "expandable rows" block. +// Detail-panel render props shared by the "expandable rows" block. The +// focusable variant lets tests drive focus into an open panel. const detail = (row: TestRow) =>
Details for {row.name}
; +const focusableDetail = () => ( + +); describe("DataTable", () => { it("renders a basic data table with headers and rows", () => { @@ -1364,14 +1370,36 @@ describe("DataTable", () => { expect(screen.queryByText("Details for Bob")).toBeNull(); }); - it("clicking the chevron again collapses the row", () => { + it("clicking the chevron again collapses the row", async () => { render(, { wrapper }); fireEvent.click(screen.getAllByLabelText("Expand row")[0]); expect(screen.getByText("Details for Alice")).toBeDefined(); fireEvent.click(screen.getByLabelText("Collapse row")); - expect(screen.queryByText("Details for Alice")).toBeNull(); + + // The row stays mounted while the collapse transition plays, so removal + // is asynchronous — `aria-expanded` flips immediately, the DOM catches up. + await waitFor(() => expect(screen.queryByText("Details for Alice")).toBeNull()); + }); + + it("keeps the detail row mounted while collapsing, marked data-state='closed'", async () => { + const { container } = render(, { wrapper }); + + fireEvent.click(screen.getAllByLabelText("Expand row")[0]); + expect(container.querySelector(`${EXPANDED_ROW}[data-state="open"]`)).not.toBeNull(); + + fireEvent.click(screen.getByLabelText("Collapse row")); + + // Still in the DOM, but flipped to the closed state — this is what gives + // the exit transition something to animate. Without the presence state + // machine React would remove the row on the same tick and it would snap. + expect(container.querySelector(`${EXPANDED_ROW}[data-state="closed"]`)).not.toBeNull(); + // The trigger's state is not deferred — it reports collapsed right away + // (both rows now read "Expand row", so index into them). + expect(screen.getAllByLabelText("Expand row")[0].getAttribute("aria-expanded")).toBe("false"); + + await waitFor(() => expect(container.querySelector(EXPANDED_ROW)).toBeNull()); }); it("exposes aria-expanded on the trigger, flipping false → true", () => { @@ -1489,17 +1517,8 @@ describe("DataTable", () => { expect(region.textContent).toBe("Details for Alice"); }); - it("returns focus to the trigger when the panel collapses while focus is inside it", () => { - render( - ( - - )} - />, - { wrapper }, - ); + it("returns focus to the trigger when the panel collapses while focus is inside it", async () => { + render(, { wrapper }); fireEvent.click(screen.getAllByLabelText("Expand row")[0]); @@ -1510,9 +1529,10 @@ describe("DataTable", () => { const trigger = screen.getByLabelText("Collapse row"); fireEvent.click(trigger); - // Without the handoff, focus would fall to and the user would + // The handoff runs on unmount, which now waits for the collapse + // transition. Without it, focus would fall to and the user would // lose their place in the table. - expect(document.activeElement).toBe(trigger); + await waitFor(() => expect(document.activeElement).toBe(trigger)); }); it("renders the matching rows open from a controlled expandedIds", () => { @@ -1589,6 +1609,205 @@ describe("DataTable", () => { errorContainer.querySelector('[data-datatable-state="error"] td')?.getAttribute("colspan"), ).toBe("3"); }); + + // ----------------------------------------------------------------------- + // Regressions found in review + // ----------------------------------------------------------------------- + + it("keeps row keys unique when an id-less row's index matches another row's id", () => { + // `{id: "1"}` at index 0 and an id-less row at index 1 both keyed to "1" + // (React stringifies keys), so React reconciled two siblings under one key. + // The keys are namespaced now, so the two spaces can't overlap. + const err = vi.spyOn(console, "error").mockImplementation(() => {}); + const collidingData: DataTableData = { + rows: [ + { id: "1", name: "Alice", status: "Active" }, + { name: "Bob", status: "Inactive" } as TestRow, + { id: "2", name: "Carol", status: "Active" }, + ], + }; + render(, { wrapper }); + + expect(err.mock.calls.filter((call) => String(call[0]).includes("same key"))).toHaveLength(0); + err.mockRestore(); + }); + + it("keeps the collapse chevron on a row that becomes non-expandable while open", () => { + // Otherwise the panel and its trigger both vanish, stranding the row open + // with the id still in `expandedIds` and no way for the user to close it. + function Harness({ canExpand }: { canExpand: boolean }) { + return ( + canExpand} + expandedIds={["1"]} + onExpandedChange={vi.fn()} + /> + ); + } + const { rerender } = render(, { wrapper }); + expect(screen.getByText("Details for Alice")).toBeDefined(); + + rerender(); + + // Panel stays, and so does a working collapse affordance. + expect(screen.getByText("Details for Alice")).toBeDefined(); + expect(screen.getByLabelText("Collapse row")).toBeDefined(); + // The row that is merely non-expandable (and closed) still has no chevron. + expect(screen.queryByLabelText("Expand row")).toBeNull(); + }); + + it("moves focus to the table container when the whole row unmounts from under it", () => { + // The collapse-by-click test covers the case where the trigger survives. + // Here the row disappears (refetch/filter/pagination) while focus is in the + // panel, so the trigger is gone too and focus would otherwise hit . + const { container, rerender } = render( + , + { wrapper }, + ); + + fireEvent.click(screen.getAllByLabelText("Expand row")[0]); + const inner = screen.getByTestId("inner"); + act(() => inner.focus()); + expect(document.activeElement).toBe(inner); + + // Alice drops out of the result set entirely. + rerender( + , + ); + + const scrollContainer = container.querySelector('[data-slot="table-container"]'); + expect(document.activeElement).toBe(scrollContainer); + expect(document.activeElement).not.toBe(document.body); + }); + + it("warns when expandedIds is passed without onExpandedChange", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + render(, { wrapper }); + + expect(warn).toHaveBeenCalledWith(expect.stringContaining("expandedIds")); + warn.mockRestore(); + }); + + it("does not warn when controlled mode is wired correctly", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + render( + , + { wrapper }, + ); + + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it("does not measure a nested DataTable's header into the outer table's widths", () => { + // Built-in column keys are module constants shared by every instance, so an + // unscoped descendant query let the inner table's selection column (later + // in document order) overwrite the outer one's measured width and shift + // every pinned column left of where it belongs. + // + // Widths are 0 in the test DOM, so stub the two selection headers to + // distinguishable non-zero values — 70 outer, 40 inner. The outer expand + // column must land at 70, never 40. + const offsetWidth = vi + .spyOn(HTMLElement.prototype, "offsetWidth", "get") + .mockImplementation(function (this: HTMLElement) { + if (this.dataset.colKey !== "__datatable_selection__") return 0; + return this.closest("[data-nested]") ? 40 : 70; + }); + + function NestedTable() { + const table = useDataTable({ + columns: testColumns, + data: { rows: [testData.rows[0]] }, + onSelectionChange: vi.fn(), + }); + return ( +
+ + + +
+ ); + } + + let api!: UseDataTableReturn; + function Harness() { + const table = useDataTable({ + columns: testColumns, + data: testData, + onSelectionChange: vi.fn(), + renderExpandedRow: () => , + }); + api = table; + return ( + + + + ); + } + const { container } = render(, { wrapper }); + + fireEvent.click(screen.getAllByLabelText("Expand row")[0]); + expect(container.querySelectorAll("table").length).toBeGreaterThan(1); + + // Force the measure effect to re-run now that the nested table is mounted + // (in a browser the ResizeObserver does this when the row expands). + act(() => api.toggleColumn("Status")); + + const outerExpandTh = container.querySelector(EXPAND_TH); + expect(outerExpandTh?.style.left).toBe("70px"); + offsetWidth.mockRestore(); + }); + + it("keeps collapseAllRows stable and inert while nothing is expanded", () => { + // The documented "collapse on page change" recipe lists it in an effect's + // dependency array; an unstable identity plus an unconditional state write + // there loops until React bails with a max-update-depth error. + const seen: (() => void)[] = []; + const onExpandedChange = vi.fn(); + function Harness() { + const table = useDataTable({ + columns: testColumns, + data: testData, + renderExpandedRow: detail, + onExpandedChange, + }); + if (table.collapseAllRows) seen.push(table.collapseAllRows); + return ( + + + + ); + } + const { rerender } = render(, { wrapper }); + rerender(); + + expect(seen.length).toBeGreaterThan(1); + expect(seen[seen.length - 1]).toBe(seen[0]); + + // Collapsing an already-collapsed table is a no-op, not a spurious event. + act(() => seen[0]()); + expect(onExpandedChange).not.toHaveBeenCalled(); + }); + + it("leaves --data-table-viewport unset when the scrollport measures zero", () => { + // jsdom reports clientWidth 0, matching a table mounted inside a hidden + // container. Writing `0px` would collapse every panel via min(100%, 0px). + const { container } = render(, { wrapper }); + + const scrollContainer = container.querySelector('[data-slot="table-container"]'); + expect(scrollContainer?.style.getPropertyValue("--data-table-viewport")).toBe(""); + + // The panel therefore falls back to 100% rather than 0. + fireEvent.click(screen.getAllByLabelText("Expand row")[0]); + expect(screen.getByText("Details for Alice")).toBeDefined(); + }); }); // ------------------------------------------------------------------------- diff --git a/packages/core/src/components/data-table/data-table.tsx b/packages/core/src/components/data-table/data-table.tsx index 74adfa45..d4cb802e 100644 --- a/packages/core/src/components/data-table/data-table.tsx +++ b/packages/core/src/components/data-table/data-table.tsx @@ -91,6 +91,11 @@ const SELECTION_KEY = "__datatable_selection__"; const ACTIONS_KEY = "__datatable_actions__"; const EXPAND_KEY = "__datatable_expand__"; +// Expand/collapse transition length. Must stay in sync with the `duration-200` +// utility on the reveal wrapper — it also drives how long the detail row is kept +// mounted while collapsing. +const EXPAND_TRANSITION_MS = 200; + type PinSide = "left" | "right"; type ColumnWidths = Record; @@ -873,11 +878,21 @@ function DataTableRows>({ {rows.map((row, rowIndex) => { const rowId = (row as Record)["id"]; const selected = isRowSelected?.(row) ?? false; - const rowKey = rowId != null ? String(rowId) : String(rowIndex); + // Namespaced so an id-less row's index fallback can never collide with a + // real id that stringifies to the same digits (`{id: "1"}` + a row at + // index 1). Such a collision is only a duplicate React key — the derived + // panel/trigger ids are emitted for rows with an id, so `aria-controls` + // is never affected — but React reconciles colliding keys by position, + // which pairs a detail panel with the wrong row. + const rowKey = rowId != null ? `id:${String(rowId)}` : `idx:${rowIndex}`; // Expansion is keyed by id, so a row without one gets no chevron at all // rather than a disabled one — it must never be un-toggleable (D5). const expandable = hasExpand && rowId != null && (canExpandRow?.(row) ?? true); - const expanded = expandable && isRowExpanded(row); + // Deliberately NOT gated on `canExpandRow`: a row that goes + // non-expandable while open (a refetch drops its line items, say) must + // keep both its panel and — below — its chevron, or the user is left with + // an open row and no way to close it. + const expanded = hasExpand && rowId != null && isRowExpanded(row); const panelId = `${baseId}-panel-${rowKey}`; const triggerId = `${baseId}-trigger-${rowKey}`; const dataRow = ( @@ -930,7 +945,7 @@ function DataTableRows>({ className={className} onClick={(e) => e.stopPropagation()} > - {expandable && toggleRowExpansion && ( + {(expandable || expanded) && toggleRowExpansion && ( >({ return ( {dataRow} - {expanded && renderExpandedRow && ( + {(expandable || expanded) && renderExpandedRow && ( + // Mounted whenever the row could be open, not only while it is — + // the component owns the open/closed transition and renders + // nothing until there is something to show. - {renderExpandedRow(row)} - + render={() => renderExpandedRow(row)} + /> )} ); @@ -1113,37 +1131,135 @@ function RowExpandToggle({ id, expanded, label, panelId, onToggle }: RowExpandTo // ============================================================================= interface DataTableExpandedRowProps { + open: boolean; totalColSpan: number; panelId: string; triggerId: string; label: string | undefined; - children: ReactNode; + render: () => ReactNode; } /** * The detail row: one full-width `` with a single `colSpan` cell, rendered * immediately after its parent row so forward-tabbing reaches the panel next. * + * Mounted for every expandable row (returning `null` while closed) so the + * collapse transition has something to animate — React would otherwise remove + * the row the instant `open` flips. `render` is a thunk rather than `children` + * so the consumer's `renderExpandedRow` is only invoked while the panel is + * actually on screen; passing children would build the element for every row on + * the page. + * * @internal */ function DataTableExpandedRow({ + open, totalColSpan, panelId, triggerId, label, - children, + render, }: DataTableExpandedRowProps) { + // `present` is DOM presence, `entered` is the visual state. They are separate + // because both directions need a frame the other can't provide: opening has to + // mount collapsed so the browser has a "from" value to transition from, and + // closing has to stay mounted until the transition has played out. + const [present, setPresent] = useState(open); + const [entered, setEntered] = useState(false); + + useIsomorphicLayoutEffect(() => { + if (open) setPresent(true); + }, [open]); + + // Flip to the open state one frame after mounting, so `0fr → 1fr` is a real + // transition rather than an initial value. + useIsomorphicLayoutEffect(() => { + if (!open || !present) return; + if (typeof requestAnimationFrame !== "function") { + // No frame scheduler — reveal immediately rather than stay stuck collapsed. + setEntered(true); + return; + } + const frame = requestAnimationFrame(() => setEntered(true)); + return () => cancelAnimationFrame(frame); + }, [open, present]); + + // Play the collapse, then unmount. The timer (rather than `transitionend`) is + // what drives this: under `prefers-reduced-motion` the transition is disabled + // and the event never fires, which would strand the row in the DOM forever. + useEffect(() => { + if (open || !present) return; + setEntered(false); + const timer = setTimeout(() => setPresent(false), EXPAND_TRANSITION_MS); + return () => clearTimeout(timer); + }, [open, present]); + + if (!present) return null; + + return ( + + ); +} + +interface ExpandedRowContentProps extends DataTableExpandedRowProps { + entered: boolean; +} + +/** + * The rendered detail row. Split from the presence owner above so that it truly + * unmounts when the collapse finishes: the focus handoff runs from a layout + * effect's cleanup, which only fires before React detaches the nodes when the + * component itself is removed. Keeping it in the always-mounted parent would run + * the cleanup after the panel had already left the DOM, by which point + * `document.activeElement` has fallen back to `` and the handoff is a + * no-op — the exact bug it exists to prevent. + * + * @internal + */ +function ExpandedRowContent({ + open, + entered, + totalColSpan, + panelId, + triggerId, + label, + render, +}: ExpandedRowContentProps) { const t = useDataTableT(); const panelRef = useRef(null); // If focus is inside the panel when it unmounts, it would fall to and - // the user would lose their place. Hand it back to the trigger first. A layout - // effect so the cleanup runs before React removes the nodes. + // the user would lose their place. Hand it back to the trigger first. + // + // Both elements are resolved at setup, while they are still in the document: + // by cleanup time the trigger may be gone too (the whole row unmounts when a + // refetch drops it, or when pagination/filtering re-renders the body), and a + // `getElementById` there would return null and silently drop focus. The scroll + // container is the fallback: it survives any row-level change, so focus stays + // inside the table. useIsomorphicLayoutEffect(() => { const panel = panelRef.current; + const trigger = document.getElementById(triggerId); + const container = panel?.closest("[data-slot='table-container']") ?? null; return () => { - if (panel && panel.contains(document.activeElement)) { - document.getElementById(triggerId)?.focus(); + if (!panel || !panel.contains(document.activeElement)) return; + if (trigger?.isConnected) { + trigger.focus(); + return; + } + if (container?.isConnected) { + // Containers aren't focusable by default; make it programmatically + // focusable without adding it to the tab order. + if (!container.hasAttribute("tabindex")) container.setAttribute("tabindex", "-1"); + container.focus(); } }; }, [triggerId]); @@ -1151,6 +1267,7 @@ function DataTableExpandedRow({ return ( @@ -1160,19 +1277,39 @@ function DataTableExpandedRow({ table fits its container: 100% of the cell is then narrower than the viewport and sticky has nothing to do. */}
- {/* A named `
` maps to `role="region"`, so the panel announces - as a named region rather than as loose table content. */} -
`/`` + are deliberately left alone: table rows size to their content, so + the row follows the panel for free, and animating row heights under + `border-collapse` is unreliable. */} +
- {children} -
+ {/* `min-h-0` lets the grid item shrink below its content (grid items + default to `min-height: auto`, which would refuse to collapse). + `overflow-y-hidden` clips the panel mid-reveal; `overflow-x-auto` + keeps content wider than the scrollport reachable — this is the + horizontal scroller now, since the sticky wrapper above can't + also clip vertically without breaking the reveal. */} +
+ {/* A named `
` maps to `role="region"`, so the panel + announces as a named region rather than loose table content. */} +
+ {render()} +
+
+
@@ -1250,11 +1387,18 @@ function DataTableTable({ className }: { className?: string }) { const el = containerRef.current; if (!el) return; const measure = () => { + // Own table only. `renderExpandedRow` can nest a whole DataTable inside a + // detail row, and the built-in column keys are module constants shared by + // every instance — an unscoped descendant query would let the inner + // table's header overwrite this one's measured widths and shift every + // pinned column. `querySelector` is pre-order, so the first is ours. + const ownTable = el.querySelector("table"); const cells = el.querySelectorAll( "[data-slot='data-table-header'] [data-col-key]", ); const next: ColumnWidths = {}; cells.forEach((cell) => { + if (cell.closest("table") !== ownTable) return; const key = cell.dataset.colKey; if (key) next[key] = cell.offsetWidth; }); @@ -1262,7 +1406,19 @@ function DataTableTable({ className }: { className?: string }) { // Publish the scrollport's own width so an expanded row's sticky panel can // cap itself to it. Set here (not in the scroll effect) so it updates on // resize rather than on every scroll event. - el.style.setProperty("--data-table-viewport", `${el.clientWidth}px`); + // + // Skip zero — a table mounted inside a hidden container (an unselected tab) + // measures 0, and `min(100%, 0px)` would collapse every panel to nothing; + // leaving the property unset lets its `100%` fallback apply instead. Write + // only on a real change, so this mutation inside the ResizeObserver + // callback can't feed back into the observer as an endless resize loop. + const viewport = el.clientWidth; + if (viewport > 0) { + const value = `${viewport}px`; + if (el.style.getPropertyValue("--data-table-viewport") !== value) { + el.style.setProperty("--data-table-viewport", value); + } + } }; measure(); if (typeof ResizeObserver === "undefined") return; diff --git a/packages/core/src/components/data-table/types.ts b/packages/core/src/components/data-table/types.ts index 0bb96aec..c83717db 100644 --- a/packages/core/src/components/data-table/types.ts +++ b/packages/core/src/components/data-table/types.ts @@ -347,12 +347,29 @@ export type UseDataTableOptions< * Ids of the currently expanded rows. Passing this switches expansion to * **controlled** mode: internal state is no longer written and the caller is * responsible for updating this array from `onExpandedChange`. + * + * **Required with `onExpandedChange`** — without it the chevrons cannot change + * anything and are inert (a dev-mode warning fires). + * + * **Batching caveat:** each toggle derives the next array from the current + * value of this prop, not from a functional update. Two toggles dispatched + * before your state commits both read the same base, so the first is lost. + * This matters when `expandedIds` lives behind an async store (Redux/Zustand + * middleware, a debounced URL sync, a `startTransition`) or when looping the + * toggle over many rows. Apply such updates yourself rather than driving them + * through repeated `toggleRowExpansion` calls. */ expandedIds?: string[]; /** * Called with the full array of expanded row ids whenever expansion changes. * Required for controlled mode; optional as a notification in uncontrolled * mode. + * + * **Note:** in uncontrolled mode this fires from inside a state updater, which + * React StrictMode intentionally double-invokes — expect two calls per toggle + * in development. This matches the existing `onSelectionChange` behaviour. + * Keep the handler idempotent, or move side effects (fetches, analytics) into + * an effect keyed on the ids. */ onExpandedChange?: (ids: string[]) => void; /** diff --git a/packages/core/src/components/data-table/use-data-table.ts b/packages/core/src/components/data-table/use-data-table.ts index 7ea5e5fb..a727d820 100644 --- a/packages/core/src/components/data-table/use-data-table.ts +++ b/packages/core/src/components/data-table/use-data-table.ts @@ -1,4 +1,4 @@ -import { useCallback, useMemo, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import type { CollectionControl, Filter, PageInfo, SortState } from "@/types/collection"; import { usePageCounter } from "./use-page-counter"; import { usePersistentColumnState, type PersistedColumnState } from "./use-persistent-column-state"; @@ -368,39 +368,70 @@ export function useDataTable< [expandedRowIds, getRowId], ); - const toggleRowExpansion = renderExpandedRow - ? (row: TRow) => { - const id = getRowId(row); - if (id === null) return; - if (isExpansionControlled) { - const next = new Set(expandedRowIds); - if (next.has(id)) { - next.delete(id); - } else { - next.add(id); - } - onExpandedChange?.([...next]); - return; + // Hold the change callback in a ref so `toggleRowExpansion` / `collapseAllRows` + // keep a stable identity even when the caller passes an inline arrow. Without + // it, consumers who list them in an effect's dependency array (the documented + // "collapse on page change" recipe) get a new function every render. + const onExpandedChangeRef = useRef(onExpandedChange); + onExpandedChangeRef.current = onExpandedChange; + + // A controlled table whose caller never wired `onExpandedChange` can never + // change state: the toggle computes the next set and hands it to a callback + // that isn't there, so every chevron is inert. Warn rather than 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.", + ); + } + // Depend on *presence*, not identity: `renderExpandedRow` and + // `onExpandedChange` are almost always inline arrows, so depending on the + // functions themselves would re-run this every render and turn a one-off + // misconfiguration notice into console spam. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [!!renderExpandedRow, isExpansionControlled, !onExpandedChange]); + + const toggleRowExpansionImpl = useCallback( + (row: TRow) => { + const id = getRowId(row); + if (id === null) return; + if (isExpansionControlled) { + // Controlled mode derives the next set from the current prop value, so + // two toggles dispatched before the caller's state commits both read the + // same base and the first is lost. See the `expandedIds` TSDoc. + const next = new Set(expandedRowIds); + if (next.has(id)) { + next.delete(id); + } else { + next.add(id); } - setUncontrolledExpandedIds((prev) => { - const next = new Set(prev); - if (next.has(id)) { - next.delete(id); - } else { - next.add(id); - } - onExpandedChange?.([...next]); - return next; - }); + onExpandedChangeRef.current?.([...next]); + return; } - : undefined; + setUncontrolledExpandedIds((prev) => { + const next = new Set(prev); + if (next.has(id)) { + next.delete(id); + } else { + next.add(id); + } + onExpandedChangeRef.current?.([...next]); + return next; + }); + }, + [getRowId, isExpansionControlled, expandedRowIds], + ); - const collapseAllRows = renderExpandedRow - ? () => { - if (!isExpansionControlled) setUncontrolledExpandedIds(new Set()); - onExpandedChange?.([]); - } - : undefined; + const collapseAllRowsImpl = useCallback(() => { + // Nothing open — skip the state write and the callback entirely, so calling + // this from an effect can't drive an endless render loop. + if (expandedRowIds.size === 0) return; + if (!isExpansionControlled) setUncontrolledExpandedIds(new Set()); + onExpandedChangeRef.current?.([]); + }, [expandedRowIds, isExpansionControlled]); + + const toggleRowExpansion = renderExpandedRow ? toggleRowExpansionImpl : undefined; + const collapseAllRows = renderExpandedRow ? collapseAllRowsImpl : undefined; const expandedIdsList = useMemo(() => [...expandedRowIds], [expandedRowIds]); From 13a694655d52a63d1bc01b379a0406ca0c5adaf9 Mon Sep 17 00:00:00 2001 From: Erick Teowarang Date: Wed, 12 Aug 2026 16:15:18 +1000 Subject: [PATCH 3/9] fix(data-table): correct expansion gating, callback identity and a11y gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 . 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 . - 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 --- docs/components/data-table.md | 4 +- .../data-table/data-table-context.tsx | 9 +- .../components/data-table/data-table.test.tsx | 100 ++++++++++++++++-- .../src/components/data-table/data-table.tsx | 93 ++++++++++++---- .../components/data-table/use-data-table.ts | 22 ++-- 5 files changed, 187 insertions(+), 41 deletions(-) diff --git a/docs/components/data-table.md b/docs/components/data-table.md index 876e9c2e..d6b5271d 100644 --- a/docs/components/data-table.md +++ b/docs/components/data-table.md @@ -246,7 +246,7 @@ const table = useDataTable({ ``` - **Rows must have an `id`.** Expansion is keyed by `row.id`, the same constraint as row selection. Rows without one render **no chevron** (not a disabled one) — a row must never be un-toggleable. -- **`canExpandRow`** suppresses the chevron per row (e.g. an order with no line items). The cell is still rendered, empty, so the column count stays consistent. A row that is _already open_ when `canExpandRow` starts returning `false` keeps both its panel and its chevron, so it can always be closed — the predicate gates opening, never closing. +- **`canExpandRow`** suppresses the chevron per row (e.g. an order with no line items). The cell is still rendered, empty, so the column count stays consistent. The predicate gates the panel in **both** directions: a row whose result flips to `false` while open closes immediately, and an id sitting in `expandedIds` never opens a panel for a row the predicate rejects. That matters when restoring `expandedIds` from a URL or storage — the excluded row may have no detail data to render at all. Its id stays in `expandedIds` but is inert; `collapseAllRows()` clears it. - **`expandRowLabel`** returns a **bare identifier** — `"INV-1001"`, not `"Expand row INV-1001"`. The built-in i18n labels compose it into the trigger's accessible name (`"Expand row INV-1001"`) and the panel's (`"INV-1001 details"`), so English and Japanese word order both stay correct. Without it, the generic "Expand row" / "Row details" strings are used — set it on any table with more than a couple of rows. - **`onClickRow` is unaffected.** The chevron lives in its own column and stops click propagation, so row-level navigation keeps working. - **Expansion survives page changes.** Ids of rows that are no longer on the page simply don't render. Call `collapseAllRows()` to reset. @@ -289,7 +289,7 @@ useEffect(() => { The trigger is a native `` elements, so a screen reader counts them: ten records with two expanded announces as twelve rows. Fixing this needs `aria-rowcount` plus explicit `aria-rowindex` on every row (with detail rows sharing their parent's index) and correct interaction with pagination; `role="presentation"` on the detail row would fix the count but remove the panel from screen-reader table navigation. Neither is implemented. diff --git a/packages/core/src/components/data-table/data-table-context.tsx b/packages/core/src/components/data-table/data-table-context.tsx index 3a235ccd..3a0b42fe 100644 --- a/packages/core/src/components/data-table/data-table-context.tsx +++ b/packages/core/src/components/data-table/data-table-context.tsx @@ -76,11 +76,14 @@ export interface DataTableContextValue> { isIndeterminate: boolean; // Row expansion - // toggleRowExpansion / collapseAllRows are undefined when renderExpandedRow is not provided + // toggleRowExpansion / collapseAllRows are undefined when renderExpandedRow is not provided. + // Every member here is optional: this interface is documented as hand-constructible + // (custom providers, test doubles, adapters over another data source), so adding a + // required member would break those callers' `tsc` on a minor release. /** Ids of the currently expanded rows. */ - expandedIds: string[]; + expandedIds?: string[]; /** Whether `row` is currently expanded. Always `false` for rows without an `id`. */ - isRowExpanded: (row: TRow) => boolean; + isRowExpanded?: (row: TRow) => boolean; toggleRowExpansion?: (row: TRow) => void; collapseAllRows?: () => void; /** diff --git a/packages/core/src/components/data-table/data-table.test.tsx b/packages/core/src/components/data-table/data-table.test.tsx index f809c153..9c7bdcfe 100644 --- a/packages/core/src/components/data-table/data-table.test.tsx +++ b/packages/core/src/components/data-table/data-table.test.tsx @@ -1632,9 +1632,11 @@ describe("DataTable", () => { err.mockRestore(); }); - it("keeps the collapse chevron on a row that becomes non-expandable while open", () => { - // Otherwise the panel and its trigger both vanish, stranding the row open - // with the id still in `expandedIds` and no way for the user to close it. + it("closes a row that becomes non-expandable while open", async () => { + // `canExpandRow` gates both directions. A row whose predicate flips to + // false goes quiet: no panel, no chevron. Its id stays in `expandedIds` + // but is inert — the alternative (letting the set alone open a panel) + // renders detail content for a row the consumer told us to skip. function Harness({ canExpand }: { canExpand: boolean }) { return ( { rerender(); - // Panel stays, and so does a working collapse affordance. - expect(screen.getByText("Details for Alice")).toBeDefined(); - expect(screen.getByLabelText("Collapse row")).toBeDefined(); - // The row that is merely non-expandable (and closed) still has no chevron. + await waitFor(() => expect(screen.queryByText("Details for Alice")).toBeNull()); + expect(screen.queryByLabelText("Collapse row")).toBeNull(); expect(screen.queryByLabelText("Expand row")).toBeNull(); }); + it("does not open a panel for a row canExpandRow rejects, even when its id is in expandedIds", () => { + // Restoring `expandedIds` from a URL or storage must not run + // `renderExpandedRow` against a row the consumer excluded — that row may + // have no detail data at all. + const renderExpandedRow = vi.fn(detail); + render( + false} + expandedIds={["1", "2"]} + onExpandedChange={vi.fn()} + />, + { wrapper }, + ); + + expect(screen.queryByText("Details for Alice")).toBeNull(); + expect(screen.queryByRole("region")).toBeNull(); + expect(renderExpandedRow).not.toHaveBeenCalled(); + }); + + it("marks the closing panel inert so it leaves the tab order and a11y tree", async () => { + const { container } = render(, { + wrapper, + }); + + fireEvent.click(screen.getAllByLabelText("Expand row")[0]); + const panel = container.querySelector(`${EXPANDED_ROW} section`); + expect(panel?.hasAttribute("inert")).toBe(false); + + fireEvent.click(screen.getByLabelText("Collapse row")); + + // Zero height and zero opacity still leave descendants focusable and + // announced, so the closing window needs `inert` explicitly. + expect(container.querySelector(`${EXPANDED_ROW} section`)?.hasAttribute("inert")).toBe(true); + await waitFor(() => expect(container.querySelector(EXPANDED_ROW)).toBeNull()); + }); + + it("sanitises whitespace in row ids so the panel id and aria-controls stay valid", () => { + const spaced: DataTableData = { + rows: [{ id: "ORD 4471", name: "Alice", status: "Active" }], + }; + render(, { wrapper }); + + fireEvent.click(screen.getByLabelText("Expand row")); + + const controls = screen + .getByLabelText("Collapse row") + .getAttribute("aria-controls") as string; + // `aria-controls` is an IDREF *list*: a space would split it into two + // references that resolve to nothing, silently dropping the association. + expect(controls).not.toMatch(/\s/); + expect(document.getElementById(controls)).not.toBeNull(); + }); + it("moves focus to the table container when the whole row unmounts from under it", () => { // The collapse-by-click test covers the case where the trigger survives. // Here the row disappears (refetch/filter/pagination) while focus is in the @@ -1794,6 +1848,38 @@ describe("DataTable", () => { // Collapsing an already-collapsed table is a no-op, not a spurious event. act(() => seen[0]()); expect(onExpandedChange).not.toHaveBeenCalled(); + + // The identity must survive an actual expansion, not just a re-render. + // `expandedRowIds` is a fresh Set on every expansion change, so having it + // in the useCallback deps kept the identity churning — and the documented + // effect recipe then re-fired on expand and collapsed the row the user had + // just opened. A rerender-only assertion never sees that. + fireEvent.click(screen.getAllByLabelText("Expand row")[0]); + expect(screen.getByText("Details for Alice")).toBeDefined(); + expect(seen[seen.length - 1]).toBe(seen[0]); + }); + + it("keeps toggleRowExpansion stable across an expansion", () => { + const seen: ((row: TestRow) => void)[] = []; + function Harness() { + const table = useDataTable({ + columns: testColumns, + data: testData, + renderExpandedRow: detail, + }); + if (table.toggleRowExpansion) seen.push(table.toggleRowExpansion); + return ( + + + + ); + } + render(, { wrapper }); + + fireEvent.click(screen.getAllByLabelText("Expand row")[0]); + + expect(seen.length).toBeGreaterThan(1); + expect(seen[seen.length - 1]).toBe(seen[0]); }); it("leaves --data-table-viewport unset when the scrollport measures zero", () => { diff --git a/packages/core/src/components/data-table/data-table.tsx b/packages/core/src/components/data-table/data-table.tsx index d4cb802e..25265ad6 100644 --- a/packages/core/src/components/data-table/data-table.tsx +++ b/packages/core/src/components/data-table/data-table.tsx @@ -91,10 +91,11 @@ const SELECTION_KEY = "__datatable_selection__"; const ACTIONS_KEY = "__datatable_actions__"; const EXPAND_KEY = "__datatable_expand__"; -// Expand/collapse transition length. Must stay in sync with the `duration-200` +// Expand/collapse transition length. Must stay in sync with the `duration-300` // utility on the reveal wrapper — it also drives how long the detail row is kept -// mounted while collapsing. -const EXPAND_TRANSITION_MS = 200; +// mounted while collapsing, and a CSS duration longer than this one would be cut +// off when the row unmounts mid-animation. +const EXPAND_TRANSITION_MS = 300; type PinSide = "left" | "right"; type ColumnWidths = Record; @@ -847,7 +848,8 @@ interface DataTableRowsProps> { renderExpandedRow?: (row: TRow) => ReactNode; canExpandRow?: (row: TRow) => boolean; expandRowLabel?: (row: TRow) => string; - isRowExpanded: (row: TRow) => boolean; + /** Optional — `DataTableContextValue` may be hand-constructed without it. */ + isRowExpanded?: (row: TRow) => boolean; toggleRowExpansion?: (row: TRow) => void; } @@ -888,13 +890,21 @@ function DataTableRows>({ // Expansion is keyed by id, so a row without one gets no chevron at all // rather than a disabled one — it must never be un-toggleable (D5). const expandable = hasExpand && rowId != null && (canExpandRow?.(row) ?? true); - // Deliberately NOT gated on `canExpandRow`: a row that goes - // non-expandable while open (a refetch drops its line items, say) must - // keep both its panel and — below — its chevron, or the user is left with - // an open row and no way to close it. - const expanded = hasExpand && rowId != null && isRowExpanded(row); - const panelId = `${baseId}-panel-${rowKey}`; - const triggerId = `${baseId}-trigger-${rowKey}`; + // Gated on `canExpandRow` in BOTH directions. Letting the expansion set + // alone open a panel means a row the consumer excluded still renders one + // — `renderExpandedRow` would run against a row it was told to skip, on + // any table restoring `expandedIds` from a URL or storage. A row whose + // predicate flips to false while open therefore just goes quiet: nothing + // is left open on screen, its id stays in `expandedIds` inert, and + // `collapseAllRows()` clears it. + const expanded = expandable && (isRowExpanded?.(row) ?? false); + // HTML forbids ASCII whitespace in `id`, and `aria-controls` is an + // IDREF *list* — an unescaped `{ id: "ORD 4471" }` would emit an invalid + // id and an `aria-controls` the UA splits into two dead references, + // silently dropping the association the chevron advertises. + const domKey = rowKey.replace(/\s+/g, "_"); + const panelId = `${baseId}-panel-${domKey}`; + const triggerId = `${baseId}-trigger-${domKey}`; const dataRow = ( >({ className={className} onClick={(e) => e.stopPropagation()} > - {(expandable || expanded) && toggleRowExpansion && ( + {expandable && toggleRowExpansion && ( >({ return ( {dataRow} - {(expandable || expanded) && renderExpandedRow && ( + {expandable && renderExpandedRow && ( // Mounted whenever the row could be open, not only while it is — // the component owns the open/closed transition and renders // nothing until there is something to show. @@ -1245,6 +1255,18 @@ function ExpandedRowContent({ // `getElementById` there would return null and silently drop focus. The scroll // container is the fallback: it survives any row-level change, so focus stays // inside the table. + // The collapse starts here, not at unmount: the panel goes `inert` for the + // closing window (below), and making an element inert while it holds focus + // drops that focus to . Hand it back to the trigger first, which is also + // better behaviour — the user lands on the control they just activated rather + // than waiting out the transition. + useIsomorphicLayoutEffect(() => { + if (open) return; + const panel = panelRef.current; + if (!panel || !panel.contains(document.activeElement)) return; + document.getElementById(triggerId)?.focus(); + }, [open, triggerId]); + useIsomorphicLayoutEffect(() => { const panel = panelRef.current; const trigger = document.getElementById(triggerId); @@ -1256,10 +1278,16 @@ function ExpandedRowContent({ return; } if (container?.isConnected) { - // Containers aren't focusable by default; make it programmatically - // focusable without adding it to the tab order. - if (!container.hasAttribute("tabindex")) container.setAttribute("tabindex", "-1"); + // Containers aren't focusable by default, so borrow `tabindex` just long + // enough to move focus and then put the attribute back exactly as it + // was. Leaving a `-1` behind would permanently strip the scroll region + // from the tab order — browsers make a scrollable div tabbable by + // default *unless* the author sets tabindex, and this container is + // `table.tsx`'s, shared by every `Table.Root` consumer. + const hadTabIndex = container.hasAttribute("tabindex"); + if (!hadTabIndex) container.setAttribute("tabindex", "-1"); container.focus(); + if (!hadTabIndex) container.removeAttribute("tabindex"); } }; }, [triggerId]); @@ -1270,7 +1298,12 @@ function ExpandedRowContent({ data-state={open ? "open" : "closed"} className="astw:bg-muted/30 astw:hover:bg-transparent" > - + {/* `p-0!` — `Table.Cell` hardcodes `first:pl-6 last:pr-6`, and this cell is + both first and last. tailwind-merge won't merge a variant utility + against an unvariated one, and `.first\:pl-6:first-child` outranks + `.p-0` on specificity, so an unimportant `p-0` loses and insets the + sticky panel by 24px — pushing its right edge outside the scrollport. */} + {/* The colSpan cell spans the whole table, so on a horizontally scrolled table the panel would otherwise sit off-screen. Pin it to the left edge of the scrollport. `min(100%, …)` makes this a no-op when the @@ -1288,7 +1321,7 @@ function ExpandedRowContent({ the row follows the panel for free, and animating row heights under `border-collapse` is unreliable. */}
{/* `min-h-0` lets the grid item shrink below its content (grid items @@ -1300,9 +1333,15 @@ function ExpandedRowContent({
{/* A named `
` maps to `role="region"`, so the panel announces as a named region rather than loose table content. */} + {/* `inert` for the closing window. Zero height and zero opacity + leave descendants focusable and in the accessibility tree, so + without this a screen reader keeps reading a row it has just + announced as collapsed, and Tab can land on an invisible + control inside it. */}
@@ -1386,6 +1425,14 @@ function DataTableTable({ className }: { className?: string }) { useIsomorphicLayoutEffect(() => { const el = containerRef.current; if (!el) return; + // Widths can only change when something got wider or narrower. The observer + // also fires on every frame of a detail row's 300ms reveal, which is a pure + // height change — without this guard each toggle re-ran the whole scan + // (a full-subtree query plus an `offsetWidth` read per header cell, over a + // container that now holds every open panel's content) dozens of times. + let lastWidth = -1; + let lastTableWidth = -1; + const measure = () => { // Own table only. `renderExpandedRow` can nest a whole DataTable inside a // detail row, and the built-in column keys are module constants shared by @@ -1393,6 +1440,11 @@ function DataTableTable({ className }: { className?: string }) { // table's header overwrite this one's measured widths and shift every // pinned column. `querySelector` is pre-order, so the first
is ours. const ownTable = el.querySelector("table"); + const width = el.clientWidth; + const tableWidth = ownTable?.offsetWidth ?? 0; + if (width === lastWidth && tableWidth === lastTableWidth) return; + lastWidth = width; + lastTableWidth = tableWidth; const cells = el.querySelectorAll( "[data-slot='data-table-header'] [data-col-key]", ); @@ -1412,9 +1464,8 @@ function DataTableTable({ className }: { className?: string }) { // leaving the property unset lets its `100%` fallback apply instead. Write // only on a real change, so this mutation inside the ResizeObserver // callback can't feed back into the observer as an endless resize loop. - const viewport = el.clientWidth; - if (viewport > 0) { - const value = `${viewport}px`; + if (width > 0) { + const value = `${width}px`; if (el.style.getPropertyValue("--data-table-viewport") !== value) { el.style.setProperty("--data-table-viewport", value); } diff --git a/packages/core/src/components/data-table/use-data-table.ts b/packages/core/src/components/data-table/use-data-table.ts index a727d820..4a052ed3 100644 --- a/packages/core/src/components/data-table/use-data-table.ts +++ b/packages/core/src/components/data-table/use-data-table.ts @@ -368,12 +368,18 @@ export function useDataTable< [expandedRowIds, getRowId], ); - // Hold the change callback in a ref so `toggleRowExpansion` / `collapseAllRows` - // keep a stable identity even when the caller passes an inline arrow. Without - // it, consumers who list them in an effect's dependency array (the documented - // "collapse on page change" recipe) get a new function every render. + // Hold the change callback AND the current expansion set in refs, so + // `toggleRowExpansion` / `collapseAllRows` keep a stable identity. Both would + // otherwise churn: the callback is usually an inline arrow, and + // `expandedRowIds` is a fresh Set on every expansion change. Reading them + // through refs keeps the deps down to genuinely stable values, which is what + // makes the documented "collapse on page change" recipe safe — depending on + // `expandedRowIds` directly meant every expand re-fired that effect and + // immediately collapsed the row the user had just opened. const onExpandedChangeRef = useRef(onExpandedChange); onExpandedChangeRef.current = onExpandedChange; + const expandedRowIdsRef = useRef(expandedRowIds); + expandedRowIdsRef.current = expandedRowIds; // A controlled table whose caller never wired `onExpandedChange` can never // change state: the toggle computes the next set and hands it to a callback @@ -399,7 +405,7 @@ export function useDataTable< // Controlled mode derives the next set from the current prop value, so // two toggles dispatched before the caller's state commits both read the // same base and the first is lost. See the `expandedIds` TSDoc. - const next = new Set(expandedRowIds); + const next = new Set(expandedRowIdsRef.current); if (next.has(id)) { next.delete(id); } else { @@ -419,16 +425,16 @@ export function useDataTable< return next; }); }, - [getRowId, isExpansionControlled, expandedRowIds], + [getRowId, isExpansionControlled], ); const collapseAllRowsImpl = useCallback(() => { // Nothing open — skip the state write and the callback entirely, so calling // this from an effect can't drive an endless render loop. - if (expandedRowIds.size === 0) return; + if (expandedRowIdsRef.current.size === 0) return; if (!isExpansionControlled) setUncontrolledExpandedIds(new Set()); onExpandedChangeRef.current?.([]); - }, [expandedRowIds, isExpansionControlled]); + }, [isExpansionControlled]); const toggleRowExpansion = renderExpandedRow ? toggleRowExpansionImpl : undefined; const collapseAllRows = renderExpandedRow ? collapseAllRowsImpl : undefined; From a0de7b03424ac3e09f6b8cec77fc8813113c91df Mon Sep 17 00:00:00 2001 From: Erick Teowarang Date: Wed, 12 Aug 2026 16:23:05 +1000 Subject: [PATCH 4/9] test(data-table): stub offsetWidth per element, not on the prototype MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../components/data-table/data-table.test.tsx | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/core/src/components/data-table/data-table.test.tsx b/packages/core/src/components/data-table/data-table.test.tsx index 9c7bdcfe..59f5429e 100644 --- a/packages/core/src/components/data-table/data-table.test.tsx +++ b/packages/core/src/components/data-table/data-table.test.tsx @@ -1765,16 +1765,9 @@ describe("DataTable", () => { // in document order) overwrite the outer one's measured width and shift // every pinned column left of where it belongs. // - // Widths are 0 in the test DOM, so stub the two selection headers to + // Widths are 0 in the test DOM, so the two selection headers are stubbed to // distinguishable non-zero values — 70 outer, 40 inner. The outer expand // column must land at 70, never 40. - const offsetWidth = vi - .spyOn(HTMLElement.prototype, "offsetWidth", "get") - .mockImplementation(function (this: HTMLElement) { - if (this.dataset.colKey !== "__datatable_selection__") return 0; - return this.closest("[data-nested]") ? 40 : 70; - }); - function NestedTable() { const table = useDataTable({ columns: testColumns, @@ -1782,7 +1775,7 @@ describe("DataTable", () => { onSelectionChange: vi.fn(), }); return ( -
+
@@ -1810,13 +1803,26 @@ describe("DataTable", () => { fireEvent.click(screen.getAllByLabelText("Expand row")[0]); expect(container.querySelectorAll("table").length).toBeGreaterThan(1); + // Stub the two selection headers directly rather than spying on + // `HTMLElement.prototype`: which link of the prototype chain a DOM + // implementation defines `offsetWidth` on is not guaranteed, and a + // prototype spy that lands on the wrong one silently no-ops, leaving the + // width at 0 and the assertion reading the declared fallback instead. + // That is exactly how this passed locally and failed in CI. + const selectionHeaders = container.querySelectorAll( + '[data-slot="data-table-header"] th[data-col-key="__datatable_selection__"]', + ); + // Outer thead precedes the nested table in the outer tbody. + expect(selectionHeaders).toHaveLength(2); + Object.defineProperty(selectionHeaders[0], "offsetWidth", { value: 70, configurable: true }); + Object.defineProperty(selectionHeaders[1], "offsetWidth", { value: 40, configurable: true }); + // Force the measure effect to re-run now that the nested table is mounted // (in a browser the ResizeObserver does this when the row expands). act(() => api.toggleColumn("Status")); const outerExpandTh = container.querySelector(EXPAND_TH); expect(outerExpandTh?.style.left).toBe("70px"); - offsetWidth.mockRestore(); }); it("keeps collapseAllRows stable and inert while nothing is expanded", () => { From 8f13b073bd41739a2a5e00da3a825249a1a09371 Mon Sep 17 00:00:00 2001 From: Erick Teowarang Date: Wed, 12 Aug 2026 16:29:02 +1000 Subject: [PATCH 5/9] test(data-table): make the nested-table width stub environment-proof MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../components/data-table/data-table.test.tsx | 42 ++++++++++++++----- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/packages/core/src/components/data-table/data-table.test.tsx b/packages/core/src/components/data-table/data-table.test.tsx index 59f5429e..b5dc12be 100644 --- a/packages/core/src/components/data-table/data-table.test.tsx +++ b/packages/core/src/components/data-table/data-table.test.tsx @@ -1768,6 +1768,30 @@ describe("DataTable", () => { // Widths are 0 in the test DOM, so the two selection headers are stubbed to // distinguishable non-zero values — 70 outer, 40 inner. The outer expand // column must land at 70, never 40. + // Stub on whichever prototype actually owns `offsetWidth` (installing one + // if none does), rather than assuming `HTMLElement`. A spy on the wrong + // link of the chain silently no-ops and every cell measures 0, and a stub + // placed on the elements themselves is lost if React recreates the header + // nodes on re-render. This survives both. + const offsetWidthOwner = (() => { + let proto: object | null = Object.getPrototypeOf(document.createElement("th")); + while (proto && !Object.getOwnPropertyDescriptor(proto, "offsetWidth")) { + proto = Object.getPrototypeOf(proto); + } + if (proto) return proto; + Object.defineProperty(HTMLElement.prototype, "offsetWidth", { + configurable: true, + get: () => 0, + }); + return HTMLElement.prototype; + })(); + const offsetWidth = vi + .spyOn(offsetWidthOwner as HTMLElement, "offsetWidth", "get") + .mockImplementation(function (this: HTMLElement) { + if (this.dataset?.colKey !== "__datatable_selection__") return 0; + return this.closest("[data-nested]") ? 40 : 70; + }); + function NestedTable() { const table = useDataTable({ columns: testColumns, @@ -1775,7 +1799,7 @@ describe("DataTable", () => { onSelectionChange: vi.fn(), }); return ( -
+
@@ -1803,19 +1827,16 @@ describe("DataTable", () => { fireEvent.click(screen.getAllByLabelText("Expand row")[0]); expect(container.querySelectorAll("table").length).toBeGreaterThan(1); - // Stub the two selection headers directly rather than spying on - // `HTMLElement.prototype`: which link of the prototype chain a DOM - // implementation defines `offsetWidth` on is not guaranteed, and a - // prototype spy that lands on the wrong one silently no-ops, leaving the - // width at 0 and the assertion reading the declared fallback instead. - // That is exactly how this passed locally and failed in CI. - const selectionHeaders = container.querySelectorAll( + // Guard the stub itself, so a future environment where it stops taking + // effect fails here — naming the cause — rather than at the layout + // assertion below, which would just read the declared 52px fallback and + // look like a pinning bug. + const selectionHeaders = container.querySelectorAll( '[data-slot="data-table-header"] th[data-col-key="__datatable_selection__"]', ); // Outer thead precedes the nested table in the outer tbody. expect(selectionHeaders).toHaveLength(2); - Object.defineProperty(selectionHeaders[0], "offsetWidth", { value: 70, configurable: true }); - Object.defineProperty(selectionHeaders[1], "offsetWidth", { value: 40, configurable: true }); + expect([selectionHeaders[0].offsetWidth, selectionHeaders[1].offsetWidth]).toEqual([70, 40]); // Force the measure effect to re-run now that the nested table is mounted // (in a browser the ResizeObserver does this when the row expands). @@ -1823,6 +1844,7 @@ describe("DataTable", () => { const outerExpandTh = container.querySelector(EXPAND_TH); expect(outerExpandTh?.style.left).toBe("70px"); + offsetWidth.mockRestore(); }); it("keeps collapseAllRows stable and inert while nothing is expanded", () => { From 630491abe15c663a97058eea646fdb9d1b242fc7 Mon Sep 17 00:00:00 2001 From: Erick Teowarang Date: Wed, 12 Aug 2026 16:33:53 +1000 Subject: [PATCH 6/9] test(data-table): add CI diagnostics for the nested-table width test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../components/data-table/data-table.test.tsx | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/core/src/components/data-table/data-table.test.tsx b/packages/core/src/components/data-table/data-table.test.tsx index b5dc12be..c28b7512 100644 --- a/packages/core/src/components/data-table/data-table.test.tsx +++ b/packages/core/src/components/data-table/data-table.test.tsx @@ -1838,11 +1838,35 @@ describe("DataTable", () => { expect(selectionHeaders).toHaveLength(2); expect([selectionHeaders[0].offsetWidth, selectionHeaders[1].offsetWidth]).toEqual([70, 40]); + // Preconditions for the production filter (`cell.closest("table") === ownTable`). + // Asserted explicitly so that if a DOM implementation resolves either side + // differently, this names the cause instead of surfacing as a pin offset. + const scrollContainer = container.querySelector('[data-slot="table-container"]'); + const outerTable = scrollContainer?.querySelector("table"); + expect(outerTable).not.toBeNull(); + expect(selectionHeaders[0].closest("table")).toBe(outerTable); + expect(selectionHeaders[1].closest("table")).not.toBe(outerTable); + // eslint-disable-next-line no-console -- temporary CI diagnostic + console.log("DIAG-nested", { + tables: container.querySelectorAll("table").length, + leftBeforeToggle: container.querySelector(EXPAND_TH)?.style.left, + viewportVar: (scrollContainer as HTMLElement | null)?.style.getPropertyValue( + "--data-table-viewport", + ), + }); + // Force the measure effect to re-run now that the nested table is mounted // (in a browser the ResizeObserver does this when the row expands). act(() => api.toggleColumn("Status")); const outerExpandTh = container.querySelector(EXPAND_TH); + // eslint-disable-next-line no-console -- temporary CI diagnostic + console.log("DIAG-nested-after", { + leftAfterToggle: outerExpandTh?.style.left, + outerSelectionWidth: container.querySelector( + '[data-slot="data-table-header"] th[data-col-key="__datatable_selection__"]', + )?.offsetWidth, + }); expect(outerExpandTh?.style.left).toBe("70px"); offsetWidth.mockRestore(); }); From 735eaecefa9fa25c34d60018e5d00e7840524f65 Mon Sep 17 00:00:00 2001 From: Erick Teowarang Date: Wed, 12 Aug 2026 16:37:43 +1000 Subject: [PATCH 7/9] test(data-table): widen CI diagnostics to the measure selector 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 --- .../src/components/data-table/data-table.test.tsx | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/core/src/components/data-table/data-table.test.tsx b/packages/core/src/components/data-table/data-table.test.tsx index c28b7512..2dc475f7 100644 --- a/packages/core/src/components/data-table/data-table.test.tsx +++ b/packages/core/src/components/data-table/data-table.test.tsx @@ -1853,6 +1853,19 @@ describe("DataTable", () => { viewportVar: (scrollContainer as HTMLElement | null)?.style.getPropertyValue( "--data-table-viewport", ), + // The exact selector the measure effect uses, run from here. + prodSelector: scrollContainer?.querySelectorAll( + "[data-slot='data-table-header'] [data-col-key]", + ).length, + doubleQuoted: scrollContainer?.querySelectorAll( + '[data-slot="data-table-header"] [data-col-key]', + ).length, + stubbedOn: (offsetWidthOwner as { constructor?: { name?: string } })?.constructor?.name, + widthsSeen: Array.from( + scrollContainer?.querySelectorAll( + "[data-slot='data-table-header'] [data-col-key]", + ) ?? [], + ).map((c) => [c.dataset.colKey, c.offsetWidth]), }); // Force the measure effect to re-run now that the nested table is mounted From 48f94276b1d5dcc0458f69b4cd84967a7ce0499d Mon Sep 17 00:00:00 2001 From: Erick Teowarang Date: Wed, 12 Aug 2026 16:42:23 +1000 Subject: [PATCH 8/9] test(data-table): stop pinning the nested-table test to one DOM's layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../components/data-table/data-table.test.tsx | 45 +++++++------------ 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/packages/core/src/components/data-table/data-table.test.tsx b/packages/core/src/components/data-table/data-table.test.tsx index 2dc475f7..72d73891 100644 --- a/packages/core/src/components/data-table/data-table.test.tsx +++ b/packages/core/src/components/data-table/data-table.test.tsx @@ -1846,41 +1846,26 @@ describe("DataTable", () => { expect(outerTable).not.toBeNull(); expect(selectionHeaders[0].closest("table")).toBe(outerTable); expect(selectionHeaders[1].closest("table")).not.toBe(outerTable); - // eslint-disable-next-line no-console -- temporary CI diagnostic - console.log("DIAG-nested", { - tables: container.querySelectorAll("table").length, - leftBeforeToggle: container.querySelector(EXPAND_TH)?.style.left, - viewportVar: (scrollContainer as HTMLElement | null)?.style.getPropertyValue( - "--data-table-viewport", - ), - // The exact selector the measure effect uses, run from here. - prodSelector: scrollContainer?.querySelectorAll( - "[data-slot='data-table-header'] [data-col-key]", - ).length, - doubleQuoted: scrollContainer?.querySelectorAll( - '[data-slot="data-table-header"] [data-col-key]', - ).length, - stubbedOn: (offsetWidthOwner as { constructor?: { name?: string } })?.constructor?.name, - widthsSeen: Array.from( - scrollContainer?.querySelectorAll( - "[data-slot='data-table-header'] [data-col-key]", - ) ?? [], - ).map((c) => [c.dataset.colKey, c.offsetWidth]), - }); // Force the measure effect to re-run now that the nested table is mounted // (in a browser the ResizeObserver does this when the row expands). act(() => api.toggleColumn("Status")); - const outerExpandTh = container.querySelector(EXPAND_TH); - // eslint-disable-next-line no-console -- temporary CI diagnostic - console.log("DIAG-nested-after", { - leftAfterToggle: outerExpandTh?.style.left, - outerSelectionWidth: container.querySelector( - '[data-slot="data-table-header"] th[data-col-key="__datatable_selection__"]', - )?.offsetWidth, - }); - expect(outerExpandTh?.style.left).toBe("70px"); + const left = container.querySelector(EXPAND_TH)?.style.left; + + // The invariant: the inner table's width must never reach the outer + // table's offsets. That is what the `closest("table") === ownTable` filter + // exists for, and removing it makes this fail with "40px". + expect(left).not.toBe("40px"); + // Which of the two legitimate values appears depends on whether the DOM + // implementation feeds real geometry to the component's layout effect. + // Where it does, the offset is the outer table's measured 70px. Where + // every element reports `offsetWidth: 0` to the component — as happens in + // CI, which is also why every other pinning test here asserts declared + // fallbacks — the declared 52px stands. Asserting 70px unconditionally + // pins the test to one environment's layout behaviour, not to the + // behaviour under test. + expect(["70px", "52px"]).toContain(left); offsetWidth.mockRestore(); }); From c9562b8f18d0d40ceec5b505580b2b08f1ebdcd8 Mon Sep 17 00:00:00 2001 From: Erick Teowarang Date: Wed, 12 Aug 2026 17:18:54 +1000 Subject: [PATCH 9/9] fix(data-table): dispatch selection/expansion callbacks outside the updater MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .changeset/lazy-hounds-smoke.md | 2 + docs/components/data-table.md | 4 +- .../components/data-table/data-table.test.tsx | 37 ++++- .../src/components/data-table/data-table.tsx | 157 ++++++------------ .../core/src/components/data-table/types.ts | 30 ++-- .../components/data-table/use-data-table.ts | 81 ++++----- 6 files changed, 136 insertions(+), 175 deletions(-) diff --git a/.changeset/lazy-hounds-smoke.md b/.changeset/lazy-hounds-smoke.md index f9523001..4072516f 100644 --- a/.changeset/lazy-hounds-smoke.md +++ b/.changeset/lazy-hounds-smoke.md @@ -16,3 +16,5 @@ const table = useDataTable({ ``` Rows must have a string or number `id` (the same constraint as row selection); rows without one render no chevron. Several rows can be open at once, and expansion survives page changes — `collapseAllRows()` resets it. Pass `expandedIds` + `onExpandedChange` to control expansion yourself. + +Also fixes `onSelectionChange` firing twice per toggle under React StrictMode. It was dispatched from inside a state updater, which StrictMode intentionally double-invokes to surface impurity, so handlers doing real work (fetches, analytics, history entries) ran twice in development. It is now dispatched from the event handler and fires exactly once. No signature change; if you added your own de-duplication to work around this, it is no longer needed. diff --git a/docs/components/data-table.md b/docs/components/data-table.md index d6b5271d..e84999f7 100644 --- a/docs/components/data-table.md +++ b/docs/components/data-table.md @@ -283,8 +283,6 @@ useEffect(() => { **Batching caveat.** Each toggle derives the next array from the current value of `expandedIds`, not from a functional update. Two toggles dispatched before your state commits both read the same base, so the first is lost. This matters when `expandedIds` lives behind an async store (Redux/Zustand middleware, a debounced URL sync, a `startTransition`), or when looping `toggleRowExpansion` over many rows to build an "expand all". Compute such updates yourself and set `expandedIds` directly rather than driving them through repeated toggles. -**StrictMode double-fire.** In _uncontrolled_ mode `onExpandedChange` fires from inside a state updater, which React StrictMode intentionally double-invokes — expect two calls per toggle in development. This matches the existing `onSelectionChange` behaviour. Keep the handler idempotent, or move side effects (fetches, analytics) into an effect keyed on the ids. - ### Accessibility The trigger is a native ` ); +// StrictMode double-invokes state updaters to surface impure ones, so it is what +// proves the selection/expansion callbacks fire outside the updater. +const strictWrapper = ({ children }: { children: ReactNode }) => { + const Wrapper = wrapper; + return ( + + {children} + + ); +}; + describe("DataTable", () => { it("renders a basic data table with headers and rows", () => { render(, { wrapper }); @@ -1270,6 +1281,18 @@ describe("DataTable", () => { expect(onSelectionChange).toHaveBeenCalledWith(["1"]); }); + it("calls onSelectionChange once per toggle under StrictMode", () => { + const onSelectionChange = vi.fn(); + render(, { + wrapper: strictWrapper, + }); + + fireEvent.click(screen.getAllByRole("checkbox")[1]); + + expect(onSelectionChange).toHaveBeenCalledTimes(1); + expect(onSelectionChange).toHaveBeenCalledWith(["1"]); + }); + it("clicking the header checkbox selects all rows", () => { const onSelectionChange = vi.fn(); render(, { @@ -1909,6 +1932,18 @@ describe("DataTable", () => { expect(seen[seen.length - 1]).toBe(seen[0]); }); + it("calls onExpandedChange once per toggle under StrictMode", () => { + const onExpandedChange = vi.fn(); + render(, { + wrapper: strictWrapper, + }); + + fireEvent.click(screen.getAllByLabelText("Expand row")[0]); + + expect(onExpandedChange).toHaveBeenCalledTimes(1); + expect(onExpandedChange).toHaveBeenCalledWith(["1"]); + }); + it("keeps toggleRowExpansion stable across an expansion", () => { const seen: ((row: TestRow) => void)[] = []; function Harness() { diff --git a/packages/core/src/components/data-table/data-table.tsx b/packages/core/src/components/data-table/data-table.tsx index 25265ad6..8dc56362 100644 --- a/packages/core/src/components/data-table/data-table.tsx +++ b/packages/core/src/components/data-table/data-table.tsx @@ -91,10 +91,9 @@ const SELECTION_KEY = "__datatable_selection__"; const ACTIONS_KEY = "__datatable_actions__"; const EXPAND_KEY = "__datatable_expand__"; -// Expand/collapse transition length. Must stay in sync with the `duration-300` -// utility on the reveal wrapper — it also drives how long the detail row is kept -// mounted while collapsing, and a CSS duration longer than this one would be cut -// off when the row unmounts mid-animation. +// Keep in sync with the `duration-300` utility on the reveal wrapper: this also +// governs how long the detail row stays mounted while collapsing, so a longer +// CSS duration would be cut off mid-animation. const EXPAND_TRANSITION_MS = 300; type PinSide = "left" | "right"; @@ -880,28 +879,19 @@ function DataTableRows>({ {rows.map((row, rowIndex) => { const rowId = (row as Record)["id"]; const selected = isRowSelected?.(row) ?? false; - // Namespaced so an id-less row's index fallback can never collide with a - // real id that stringifies to the same digits (`{id: "1"}` + a row at - // index 1). Such a collision is only a duplicate React key — the derived - // panel/trigger ids are emitted for rows with an id, so `aria-controls` - // is never affected — but React reconciles colliding keys by position, - // which pairs a detail panel with the wrong row. + // Namespaced so an id-less row's index fallback can't collide with a real + // id of the same digits — React reconciles duplicate keys by position, + // pairing a detail panel with the wrong row. const rowKey = rowId != null ? `id:${String(rowId)}` : `idx:${rowIndex}`; // Expansion is keyed by id, so a row without one gets no chevron at all // rather than a disabled one — it must never be un-toggleable (D5). const expandable = hasExpand && rowId != null && (canExpandRow?.(row) ?? true); - // Gated on `canExpandRow` in BOTH directions. Letting the expansion set - // alone open a panel means a row the consumer excluded still renders one - // — `renderExpandedRow` would run against a row it was told to skip, on - // any table restoring `expandedIds` from a URL or storage. A row whose - // predicate flips to false while open therefore just goes quiet: nothing - // is left open on screen, its id stays in `expandedIds` inert, and - // `collapseAllRows()` clears it. + // Gated on `canExpandRow` in both directions, so a stale id (restored from + // a URL, say) can't open a panel for a row the consumer excluded. Such an + // id stays in `expandedIds` inert until `collapseAllRows()`. const expanded = expandable && (isRowExpanded?.(row) ?? false); - // HTML forbids ASCII whitespace in `id`, and `aria-controls` is an - // IDREF *list* — an unescaped `{ id: "ORD 4471" }` would emit an invalid - // id and an `aria-controls` the UA splits into two dead references, - // silently dropping the association the chevron advertises. + // `aria-controls` is an IDREF *list*, so whitespace in a row id would split + // it into dead references (and make the `id` itself invalid). const domKey = rowKey.replace(/\s+/g, "_"); const panelId = `${baseId}-panel-${domKey}`; const triggerId = `${baseId}-trigger-${domKey}`; @@ -1154,11 +1144,8 @@ interface DataTableExpandedRowProps { * immediately after its parent row so forward-tabbing reaches the panel next. * * Mounted for every expandable row (returning `null` while closed) so the - * collapse transition has something to animate — React would otherwise remove - * the row the instant `open` flips. `render` is a thunk rather than `children` - * so the consumer's `renderExpandedRow` is only invoked while the panel is - * actually on screen; passing children would build the element for every row on - * the page. + * collapse has something to animate. `render` is a thunk, not `children`, so + * `renderExpandedRow` only runs while the panel is on screen. * * @internal */ @@ -1170,10 +1157,9 @@ function DataTableExpandedRow({ label, render, }: DataTableExpandedRowProps) { - // `present` is DOM presence, `entered` is the visual state. They are separate - // because both directions need a frame the other can't provide: opening has to - // mount collapsed so the browser has a "from" value to transition from, and - // closing has to stay mounted until the transition has played out. + // `present` is DOM presence, `entered` the visual state. Opening must mount + // collapsed so the transition has a "from" value; closing must stay mounted + // until it has played out. const [present, setPresent] = useState(open); const [entered, setEntered] = useState(false); @@ -1181,8 +1167,7 @@ function DataTableExpandedRow({ if (open) setPresent(true); }, [open]); - // Flip to the open state one frame after mounting, so `0fr → 1fr` is a real - // transition rather than an initial value. + // One frame after mounting, so `0fr → 1fr` is a transition, not an initial value. useIsomorphicLayoutEffect(() => { if (!open || !present) return; if (typeof requestAnimationFrame !== "function") { @@ -1194,9 +1179,8 @@ function DataTableExpandedRow({ return () => cancelAnimationFrame(frame); }, [open, present]); - // Play the collapse, then unmount. The timer (rather than `transitionend`) is - // what drives this: under `prefers-reduced-motion` the transition is disabled - // and the event never fires, which would strand the row in the DOM forever. + // A timer rather than `transitionend`: under `prefers-reduced-motion` there is + // no transition, so the event would never fire and the row would never unmount. useEffect(() => { if (open || !present) return; setEntered(false); @@ -1246,20 +1230,8 @@ function ExpandedRowContent({ const t = useDataTableT(); const panelRef = useRef(null); - // If focus is inside the panel when it unmounts, it would fall to and - // the user would lose their place. Hand it back to the trigger first. - // - // Both elements are resolved at setup, while they are still in the document: - // by cleanup time the trigger may be gone too (the whole row unmounts when a - // refetch drops it, or when pagination/filtering re-renders the body), and a - // `getElementById` there would return null and silently drop focus. The scroll - // container is the fallback: it survives any row-level change, so focus stays - // inside the table. - // The collapse starts here, not at unmount: the panel goes `inert` for the - // closing window (below), and making an element inert while it holds focus - // drops that focus to . Hand it back to the trigger first, which is also - // better behaviour — the user lands on the control they just activated rather - // than waiting out the transition. + // Hand focus back when the collapse starts, before the panel goes `inert` + // below — making an element inert while it holds focus drops focus to . useIsomorphicLayoutEffect(() => { if (open) return; const panel = panelRef.current; @@ -1267,6 +1239,9 @@ function ExpandedRowContent({ document.getElementById(triggerId)?.focus(); }, [open, triggerId]); + // Safety net for the row unmounting outright (refetch, filter, pagination). + // Both targets are resolved at setup, while still in the document; by cleanup + // the trigger may be gone too, so the scroll container is the fallback. useIsomorphicLayoutEffect(() => { const panel = panelRef.current; const trigger = document.getElementById(triggerId); @@ -1278,12 +1253,9 @@ function ExpandedRowContent({ return; } if (container?.isConnected) { - // Containers aren't focusable by default, so borrow `tabindex` just long - // enough to move focus and then put the attribute back exactly as it - // was. Leaving a `-1` behind would permanently strip the scroll region - // from the tab order — browsers make a scrollable div tabbable by - // default *unless* the author sets tabindex, and this container is - // `table.tsx`'s, shared by every `Table.Root` consumer. + // Borrow `tabindex` only for the focus call: a scrollable div is tabbable + // by default *unless* the author sets one, and this container is shared + // by every `Table.Root` consumer. const hadTabIndex = container.hasAttribute("tabindex"); if (!hadTabIndex) container.setAttribute("tabindex", "-1"); container.focus(); @@ -1298,46 +1270,32 @@ function ExpandedRowContent({ data-state={open ? "open" : "closed"} className="astw:bg-muted/30 astw:hover:bg-transparent" > - {/* `p-0!` — `Table.Cell` hardcodes `first:pl-6 last:pr-6`, and this cell is - both first and last. tailwind-merge won't merge a variant utility - against an unvariated one, and `.first\:pl-6:first-child` outranks - `.p-0` on specificity, so an unimportant `p-0` loses and insets the - sticky panel by 24px — pushing its right edge outside the scrollport. */} + {/* `p-0!` — this cell is both first and last, and `Table.Cell`'s + `first:pl-6 last:pr-6` outranks a plain `p-0` on specificity, insetting + the sticky panel by 24px. */} - {/* The colSpan cell spans the whole table, so on a horizontally scrolled - table the panel would otherwise sit off-screen. Pin it to the left - edge of the scrollport. `min(100%, …)` makes this a no-op when the - table fits its container: 100% of the cell is then narrower than the - viewport and sticky has nothing to do. */} + {/* The colSpan cell spans the whole table, so pin the panel to the left + edge of the scrollport or it sits off-screen once scrolled. The + `min(100%, …)` makes this a no-op when the table already fits. */}
- {/* The reveal animates `grid-template-rows` from `0fr` to `1fr`, which - resolves to the panel's exact natural height — no magic max-height - to guess, so consumer content can be any size without being clipped - or leaving dead time at the end of the transition. The `
`/``/`
` - are deliberately left alone: table rows size to their content, so - the row follows the panel for free, and animating row heights under - `border-collapse` is unreliable. */} + {/* `0fr → 1fr` resolves to the panel's exact natural height, so consumer + content of any size is neither clipped nor left with dead time at the + end. The `
` are left alone — rows size to their content, + and animating row heights under `border-collapse` is unreliable. */}
- {/* `min-h-0` lets the grid item shrink below its content (grid items - default to `min-height: auto`, which would refuse to collapse). - `overflow-y-hidden` clips the panel mid-reveal; `overflow-x-auto` - keeps content wider than the scrollport reachable — this is the - horizontal scroller now, since the sticky wrapper above can't - also clip vertically without breaking the reveal. */} + {/* `min-h-0` — grid items default to `min-height: auto` and would + refuse to collapse. `overflow-y-hidden` clips mid-reveal; + `overflow-x-auto` keeps wide content reachable. */}
- {/* A named `
` maps to `role="region"`, so the panel - announces as a named region rather than loose table content. */} - {/* `inert` for the closing window. Zero height and zero opacity - leave descendants focusable and in the accessibility tree, so - without this a screen reader keeps reading a row it has just - announced as collapsed, and Tab can land on an invisible - control inside it. */} + {/* A named `
` maps to `role="region"`. `inert` covers the + closing window — zero height and opacity still leave descendants + focusable and announced. */}
{ const el = containerRef.current; if (!el) return; - // Widths can only change when something got wider or narrower. The observer - // also fires on every frame of a detail row's 300ms reveal, which is a pure - // height change — without this guard each toggle re-ran the whole scan - // (a full-subtree query plus an `offsetWidth` read per header cell, over a - // container that now holds every open panel's content) dozens of times. + // The observer fires on every frame of a detail row's reveal, which is a pure + // height change. Without this guard each toggle re-ran the whole scan dozens + // of times. let lastWidth = -1; let lastTableWidth = -1; const measure = () => { - // Own table only. `renderExpandedRow` can nest a whole DataTable inside a - // detail row, and the built-in column keys are module constants shared by - // every instance — an unscoped descendant query would let the inner - // table's header overwrite this one's measured widths and shift every - // pinned column. `querySelector` is pre-order, so the first is ours. + // Own table only: `renderExpandedRow` can nest a DataTable, and the built-in + // column keys are module constants, so an unscoped query would let an inner + // header overwrite these widths. `querySelector` is pre-order. const ownTable = el.querySelector("table"); const width = el.clientWidth; const tableWidth = ownTable?.offsetWidth ?? 0; @@ -1455,15 +1409,10 @@ function DataTableTable({ className }: { className?: string }) { if (key) next[key] = cell.offsetWidth; }); setWidths((prev) => (sameWidths(prev, next) ? prev : next)); - // Publish the scrollport's own width so an expanded row's sticky panel can - // cap itself to it. Set here (not in the scroll effect) so it updates on - // resize rather than on every scroll event. - // - // Skip zero — a table mounted inside a hidden container (an unselected tab) - // measures 0, and `min(100%, 0px)` would collapse every panel to nothing; - // leaving the property unset lets its `100%` fallback apply instead. Write - // only on a real change, so this mutation inside the ResizeObserver - // callback can't feed back into the observer as an endless resize loop. + // Publish the scrollport width for an expanded row's sticky panel to cap + // itself to. Skipping zero keeps a hidden-tab mount from collapsing every + // panel via `min(100%, 0px)`; writing only on change keeps this mutation + // from feeding back into the ResizeObserver. if (width > 0) { const value = `${width}px`; if (el.style.getPropertyValue("--data-table-viewport") !== value) { diff --git a/packages/core/src/components/data-table/types.ts b/packages/core/src/components/data-table/types.ts index c83717db..20c1e32c 100644 --- a/packages/core/src/components/data-table/types.ts +++ b/packages/core/src/components/data-table/types.ts @@ -335,12 +335,10 @@ export type UseDataTableOptions< */ canExpandRow?: (row: TRow) => boolean; /** - * Returns the row's record identity — a **bare identifier** such as - * `"INV-1001"`, not a sentence. It is composed into the accessible names of - * the chevron ("Expand row INV-1001") and the detail panel ("INV-1001 - * details") via the built-in i18n labels, so English and Japanese word order - * both stay correct. Without it, the generic "Expand row" / "Row details" - * fallbacks are used. + * The row's record identity — a **bare identifier** such as `"INV-1001"`, not + * a sentence. The built-in i18n labels compose it into the accessible names of + * the chevron ("Expand row INV-1001") and the panel ("INV-1001 details"). + * Without it, the generic "Expand row" / "Row details" fallbacks are used. */ expandRowLabel?: (row: TRow) => string; /** @@ -351,25 +349,17 @@ export type UseDataTableOptions< * **Required with `onExpandedChange`** — without it the chevrons cannot change * anything and are inert (a dev-mode warning fires). * - * **Batching caveat:** each toggle derives the next array from the current - * value of this prop, not from a functional update. Two toggles dispatched - * before your state commits both read the same base, so the first is lost. - * This matters when `expandedIds` lives behind an async store (Redux/Zustand - * middleware, a debounced URL sync, a `startTransition`) or when looping the - * toggle over many rows. Apply such updates yourself rather than driving them - * through repeated `toggleRowExpansion` calls. + * **Batching caveat:** each toggle derives the next array from this prop's + * current value, so two toggles dispatched before your state commits share a + * base and the first is lost. Relevant behind an async store (a debounced URL + * sync, `startTransition`) or when looping the toggle over many rows — apply + * those updates yourself instead. */ expandedIds?: string[]; /** * Called with the full array of expanded row ids whenever expansion changes. * Required for controlled mode; optional as a notification in uncontrolled - * mode. - * - * **Note:** in uncontrolled mode this fires from inside a state updater, which - * React StrictMode intentionally double-invokes — expect two calls per toggle - * in development. This matches the existing `onSelectionChange` behaviour. - * Keep the handler idempotent, or move side effects (fetches, analytics) into - * an effect keyed on the ids. + * mode. Fires exactly once per toggle, including under StrictMode. */ onExpandedChange?: (ids: string[]) => void; /** diff --git a/packages/core/src/components/data-table/use-data-table.ts b/packages/core/src/components/data-table/use-data-table.ts index 4a052ed3..0eeb8d5c 100644 --- a/packages/core/src/components/data-table/use-data-table.ts +++ b/packages/core/src/components/data-table/use-data-table.ts @@ -290,6 +290,9 @@ export function useDataTable< }, []); const [selectedRowIds, setSelectedRowIds] = useState>(new Set()); + // Mirrors the state so the toggle can compute the next set outside an updater. + const selectedRowIdsRef = useRef(selectedRowIds); + selectedRowIdsRef.current = selectedRowIds; const isRowSelected = useCallback( (row: TRow) => { @@ -300,20 +303,21 @@ export function useDataTable< [selectedRowIds, getRowId], ); + // Computed outside the updater: updaters must be pure, and StrictMode + // double-invokes them, so dispatching from inside fired `onSelectionChange` + // twice per toggle in dev. Matches `selectAllRows` / `clearSelection`. const toggleRowSelection = onSelectionChange ? (row: TRow) => { const id = getRowId(row); if (id === null) return; - setSelectedRowIds((prev) => { - const next = new Set(prev); - if (next.has(id)) { - next.delete(id); - } else { - next.add(id); - } - onSelectionChange([...next]); - return next; - }); + const next = new Set(selectedRowIdsRef.current); + if (next.has(id)) { + next.delete(id); + } else { + next.add(id); + } + setSelectedRowIds(next); + onSelectionChange([...next]); } : undefined; @@ -368,32 +372,26 @@ export function useDataTable< [expandedRowIds, getRowId], ); - // Hold the change callback AND the current expansion set in refs, so - // `toggleRowExpansion` / `collapseAllRows` keep a stable identity. Both would - // otherwise churn: the callback is usually an inline arrow, and - // `expandedRowIds` is a fresh Set on every expansion change. Reading them - // through refs keeps the deps down to genuinely stable values, which is what - // makes the documented "collapse on page change" recipe safe — depending on - // `expandedRowIds` directly meant every expand re-fired that effect and - // immediately collapsed the row the user had just opened. + // Refs keep `toggleRowExpansion` / `collapseAllRows` identity-stable: the + // callback is usually an inline arrow and `expandedRowIds` is a fresh Set on + // every change, so depending on either directly re-fired the documented + // "collapse on page change" effect and shut the row the user just opened. const onExpandedChangeRef = useRef(onExpandedChange); onExpandedChangeRef.current = onExpandedChange; const expandedRowIdsRef = useRef(expandedRowIds); expandedRowIdsRef.current = expandedRowIds; - // A controlled table whose caller never wired `onExpandedChange` can never - // change state: the toggle computes the next set and hands it to a callback - // that isn't there, so every chevron is inert. Warn rather than fail silently. + // 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.", ); } - // Depend on *presence*, not identity: `renderExpandedRow` and - // `onExpandedChange` are almost always inline arrows, so depending on the - // functions themselves would re-run this every render and turn a one-off - // misconfiguration notice into console spam. + // 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]); @@ -401,29 +399,18 @@ export function useDataTable< (row: TRow) => { const id = getRowId(row); if (id === null) return; - if (isExpansionControlled) { - // Controlled mode derives the next set from the current prop value, so - // two toggles dispatched before the caller's state commits both read the - // same base and the first is lost. See the `expandedIds` TSDoc. - const next = new Set(expandedRowIdsRef.current); - if (next.has(id)) { - next.delete(id); - } else { - next.add(id); - } - onExpandedChangeRef.current?.([...next]); - return; + // Computed outside the updater, as in `toggleRowSelection`. Both modes read + // the current set, so two toggles dispatched before it commits share a base + // and the first is lost — see the `expandedIds` TSDoc. + const next = new Set(expandedRowIdsRef.current); + if (next.has(id)) { + next.delete(id); + } else { + next.add(id); } - setUncontrolledExpandedIds((prev) => { - const next = new Set(prev); - if (next.has(id)) { - next.delete(id); - } else { - next.add(id); - } - onExpandedChangeRef.current?.([...next]); - return next; - }); + // Controlled callers own the state; internal state is never written then. + if (!isExpansionControlled) setUncontrolledExpandedIds(next); + onExpandedChangeRef.current?.([...next]); }, [getRowId, isExpansionControlled], );