From ad0e0125add16e0f92d0dbd7c412e6b126180581 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Wed, 22 Jul 2026 14:49:10 -0400 Subject: [PATCH] fix: preserve data table state across prop updates Paginating, sorting or searching a DataTable was undone whenever the consumer re-rendered. Page to 2 and click a row and the table jumps back to page 1 -- clicking a row typically sets state to highlight it, and that re-render was enough, though nothing about the table's data changed. Two independent causes, both required: 1. store.ts -- reset()'s client branch set `state: getTanstackTableState(updatedParams)`, rebuilding the whole TableState from props-time defaults (pageIndex 0, empty sizing, initialState filters and sorting). DataTable keys its effect on [props], a fresh rest-spread object each render, so this ran on every render. 2. DataTableControls.tsx -- the effect was keyed on [onSearchChange, searchValue]. Callers write onSearchChange inline, so it is a new function every render and the effect replayed the handler with an unchanged search value. A handler that writes filter state then handed Tanstack a new columnFilters identity, tripping autoResetPageIndex. Cause 2 is why 976be28 (released as 6.9.2) looked ineffective and was reverted: it addressed only cause 1. reset() no longer touches live table state; initialState now applies once, at store creation. Column pinning is the one slice derived from props, so reset reconciles just that via the extracted getColumnPinningWithActions, shared with getTanstackTableState so the create and update paths cannot drift. The search handler is held in a ref so its effect keys on searchValue alone. Memoizing columns is not an alternative: cell closures capture consumer state, so columns is legitimately new each render and skipping the update would leave stale cells rendering. The update was always correct; only its side effect on state was wrong. Co-Authored-By: Claude Opus 4.8 --- .../DataTable/DataTableControls.tsx | 16 +++- .../DataTable/__tests__/DataTable.spec.tsx | 86 +++++++++++++++++++ src/components/DataTable/store.ts | 46 +++++----- src/components/DataTable/utils.tsx | 35 ++++++-- 4 files changed, 149 insertions(+), 34 deletions(-) diff --git a/src/components/DataTable/DataTableControls.tsx b/src/components/DataTable/DataTableControls.tsx index 0e48ced..3314664 100644 --- a/src/components/DataTable/DataTableControls.tsx +++ b/src/components/DataTable/DataTableControls.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useState } from 'react'; +import React, { useEffect, useRef, useState } from 'react'; import type { RowData, Table } from '@tanstack/table-core'; @@ -22,14 +22,22 @@ export const DataTableControls = ({ const { t } = useTranslation(); + // Held in a ref so that the effect below responds to the search value alone. Call sites normally + // write `onSearchChange` inline, so it is a new function on every render; keying the effect on it + // replayed the handler on every render of the consumer, and a handler that writes filter state + // then reset the table's page via Tanstack's `autoResetPageIndex`. + const onSearchChangeRef = useRef(onSearchChange); + onSearchChangeRef.current = onSearchChange; + useEffect(() => { - if (onSearchChange) { + const handleSearchChange = onSearchChangeRef.current; + if (handleSearchChange) { // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - onSearchChange(searchValue, table); + handleSearchChange(searchValue, table); } else { setGlobalFilter(searchValue || undefined); } - }, [onSearchChange, searchValue]); + }, [searchValue]); return (
diff --git a/src/components/DataTable/__tests__/DataTable.spec.tsx b/src/components/DataTable/__tests__/DataTable.spec.tsx index befd703..0e16373 100644 --- a/src/components/DataTable/__tests__/DataTable.spec.tsx +++ b/src/components/DataTable/__tests__/DataTable.spec.tsx @@ -1,3 +1,5 @@ +import { useState } from 'react'; + import { faker } from '@faker-js/faker'; import type { ColumnDef } from '@tanstack/table-core'; import { fireEvent, render, screen, waitFor } from '@testing-library/react'; @@ -6,6 +8,8 @@ import { describe, expect, it } from 'vitest'; import { DataTable } from '../DataTable.tsx'; +import type { DataTableSearchChangeHandler } from '../types.ts'; + type PaymentStatus = 'failed' | 'pending' | 'processing' | 'success'; type Payment = { @@ -66,3 +70,85 @@ describe('DataTable', () => { await waitFor(() => expect(screen.getAllByTestId('data-table-row').length).toBe(10)); }); }); + +describe('DataTable pagination', () => { + // Deterministic, so a row can be identified by the page it belongs to + const paginatedData: Payment[] = range(25).map((i) => ({ + amount: i, + email: `user-${i + 1}@example.com`, + id: String(i + 1), + status: 'pending' + })); + + const firstRow = () => screen.getAllByTestId('data-table-row')[0]!; + + const goToSecondPage = async () => { + fireEvent.click(screen.getByRole('button', { name: '2' })); + await waitFor(() => expect(firstRow()).toHaveTextContent('user-11@example.com')); + }; + + /** + * Records the clicked row, the way a consumer would in order to highlight it. The point is that + * this is an ordinary `setState` in the component that owns the table: it re-renders the + * consumer, but changes nothing about the table's contents. + */ + const ClickToSelect = ({ onSearchChange }: { onSearchChange?: DataTableSearchChangeHandler }) => { + const [clickedRowId, setClickedRowId] = useState(null); + return ( +
+ {clickedRowId ?? 'none'} + setClickedRowId(payment.id)} + onSearchChange={onSearchChange} + /> +
+ ); + }; + + it('should stay on the current page when a row click re-renders the consumer', async () => { + render(); + await goToSecondPage(); + + fireEvent.click(firstRow()); + await waitFor(() => expect(screen.getByTestId('clicked-row-id')).toHaveTextContent('11')); + + expect(firstRow()).toHaveTextContent('user-11@example.com'); + }); + + // `DataTableControls` runs `onSearchChange` from an effect keyed on the handler itself, so an + // inline handler -- the normal way to write one -- re-runs it on every consumer render, not only + // when the search value changes. A handler that writes filter state then hands Tanstack a new + // `columnFilters` identity, whose `autoResetPageIndex` returns the table to the first page. + it('should stay on the current page when an inline onSearchChange writes filter state', async () => { + render( + { + table.getColumn('status')!.setFilterValue(() => [...statuses]); + }} + /> + ); + await goToSecondPage(); + + fireEvent.click(firstRow()); + await waitFor(() => expect(screen.getByTestId('clicked-row-id')).toHaveTextContent('11')); + + expect(firstRow()).toHaveTextContent('user-11@example.com'); + }); + + // Same reset, seen from the search box: the filter is dropped from the table state while the + // search input keeps its text, so the controls end up contradicting the rows + it('should keep the search applied when a row click re-renders the consumer', async () => { + render(); + const searchBar = screen.getByTestId('data-table-search-bar').querySelector('input')!; + fireEvent.change(searchBar, { target: { value: 'user-11@example.com' } }); + await waitFor(() => expect(screen.getAllByTestId('data-table-row').length).toBe(1)); + + fireEvent.click(firstRow()); + await waitFor(() => expect(screen.getByTestId('clicked-row-id')).toHaveTextContent('11')); + + expect(searchBar).toHaveValue('user-11@example.com'); + expect(screen.getAllByTestId('data-table-row').length).toBe(1); + }); +}); diff --git a/src/components/DataTable/store.ts b/src/components/DataTable/store.ts index de4cd66..baa5b36 100644 --- a/src/components/DataTable/store.ts +++ b/src/components/DataTable/store.ts @@ -13,6 +13,7 @@ import { applyUpdater, calculateColumnSizing, defineMemoizedHandle, + getColumnPinningWithActions, getColumnsWithActions, getTanstackTableState } from './utils.tsx'; @@ -190,30 +191,29 @@ export function createDataTableStore(params: DataTableStoreParams) { if (updatedParams.mode === 'server') { _serverOnPaginationChange = updatedParams.onPaginationChange; _serverOnSortingChange = updatedParams.onSortingChange; - table.setOptions((options) => ({ - ...options, - columns: getColumnsWithActions(updatedParams), - data: updatedParams.data, - meta: { - ...updatedParams.meta, - [ROW_ACTIONS_METADATA_KEY]: updatedParams.rowActions, - [TABLE_NAME_METADATA_KEY]: updatedParams.tableName - }, - pageCount: updatedParams.pageCount - })); - } else { - table.setOptions((options) => ({ - ...options, - columns: getColumnsWithActions(updatedParams), - data: updatedParams.data, - meta: { - ...updatedParams.meta, - [ROW_ACTIONS_METADATA_KEY]: updatedParams.rowActions, - [TABLE_NAME_METADATA_KEY]: updatedParams.tableName - }, - state: getTanstackTableState(updatedParams) - })); } + table.setOptions((options) => ({ + ...options, + columns: getColumnsWithActions(updatedParams), + data: updatedParams.data, + meta: { + ...updatedParams.meta, + [ROW_ACTIONS_METADATA_KEY]: updatedParams.rowActions, + [TABLE_NAME_METADATA_KEY]: updatedParams.tableName + }, + ...(updatedParams.mode === 'server' && { pageCount: updatedParams.pageCount }) + })); + + // `initialState` is applied once, when the store is created. Everything in the live table + // state -- pagination, sorting, column filters, the global filter, column sizing -- is + // owned by the user, not by props, so rebuilding it here would send them back to page one + // (losing their sort and search along with it) every time `data` or `columns` changed + // identity. The only piece derived from props is the actions column's pinning, so + // reconcile just that. + setTableState('columnPinning', (columnPinning) => + getColumnPinningWithActions(columnPinning, updatedParams.rowActions) + ); + updateColumnSizing(); updateStyle(); invalidateHandles(); diff --git a/src/components/DataTable/utils.tsx b/src/components/DataTable/utils.tsx index dd796b6..69171ba 100644 --- a/src/components/DataTable/utils.tsx +++ b/src/components/DataTable/utils.tsx @@ -1,4 +1,12 @@ -import type { ColumnDef, ColumnSizingState, RowData, Table, TableState, Updater } from '@tanstack/table-core'; +import type { + ColumnDef, + ColumnPinningState, + ColumnSizingState, + RowData, + Table, + TableState, + Updater +} from '@tanstack/table-core'; import { sum } from 'lodash-es'; import { ACTIONS_COLUMN_ID, MEMOIZED_HANDLE_ID } from './constants.ts'; @@ -7,6 +15,7 @@ import { DataTableRowActionCell } from './DataTableRowActionCell.tsx'; import type { BaseDataTableStoreParams, DataTableColumnBreakpoints, + DataTableRowAction, DataTableStoreParams, MemoizedHandle } from './types.ts'; @@ -110,6 +119,22 @@ function getColumnsWithActions({ ]; } +/** + * The given column pinning, with the actions column pinned to the right if (and only if) the table + * has row actions. Pinning is the one part of the table state derived from props, so it has to be + * reconcilable on a props update without disturbing the rest of the state. + */ +function getColumnPinningWithActions( + columnPinning: ColumnPinningState, + rowActions: DataTableRowAction[] | undefined +): ColumnPinningState { + const right = (columnPinning.right ?? []).filter((id) => id !== ACTIONS_COLUMN_ID); + return { + ...columnPinning, + right: rowActions ? [...right, ACTIONS_COLUMN_ID] : right + }; +} + function getTanstackTableState({ initialState, rowActions }: DataTableStoreParams): TableState { const { columnFilters = [], columnPinning = {}, sorting = [] } = initialState ?? {}; const state: TableState = { @@ -137,12 +162,7 @@ function getTanstackTableState({ initialState, rowActions }: DataTableStorePa rowSelection: {}, sorting }; - if (rowActions) { - state.columnPinning = { - ...state.columnPinning, - right: [...(state.columnPinning.right ?? []), ACTIONS_COLUMN_ID] - }; - } + state.columnPinning = getColumnPinningWithActions(state.columnPinning, rowActions); return state; } @@ -155,6 +175,7 @@ export { calculateColumnSizing, defineMemoizedHandle, flexRender, + getColumnPinningWithActions, getColumnsWithActions, getTanstackTableState };