From 3395620b6ee56cb52a919943e5f1c72b09d61b81 Mon Sep 17 00:00:00 2001 From: Sam Firke Date: Tue, 28 Apr 2026 07:40:56 -0400 Subject: [PATCH] fix(table chart): fix rerender bug that continuously cleared search box (#39707) --- .../src/DataTable/DataTable.tsx | 21 +- .../test/TableChart.test.tsx | 218 ++++++++++++++++++ .../src/pages/Chart/Chart.test.tsx | 21 +- 3 files changed, 248 insertions(+), 12 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx b/superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx index 74ffcbefb1f..4cec527e8cd 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx @@ -147,7 +147,25 @@ export default typedMemo(function DataTable({ hooks || [], ].flat(); - const columnNames = Object.keys(data?.[0] || {}); + const columnNames = columns.map((column, index) => { + const normalizedColumn = column as typeof column & { + accessor?: string | ((row: D) => unknown); + columnKey?: string; + id?: string; + }; + + const accessorName = + typeof normalizedColumn.accessor === 'string' + ? normalizedColumn.accessor + : undefined; + + return ( + normalizedColumn.columnKey ?? + normalizedColumn.id ?? + accessorName ?? + String(index) + ); + }); const previousColumnNames = usePrevious(columnNames); const resultsSize = serverPagination ? rowCount : data.length; const sortByRef = useRef([]); // cache initial `sortby` so sorting doesn't trigger page reset @@ -237,6 +255,7 @@ export default typedMemo(function DataTable({ getTableSize: defaultGetTableSize, globalFilter: defaultGlobalFilter, sortTypes, + autoResetGlobalFilter: !isEqual(columnNames, previousColumnNames), autoResetSortBy: !isEqual(columnNames, previousColumnNames), manualSortBy: !!serverPagination, ...moreUseTableOptions, diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index 6988f9afc18..a043c67b39f 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -36,6 +36,8 @@ import { SMART_DATE_ID, getTimeFormatterForGranularity, } from '@superset-ui/core'; +import { CellProps, Column, HeaderProps } from 'react-table'; +import DataTable from '../src/DataTable/DataTable'; import TableChart, { sanitizeHeaderId } from '../src/TableChart'; import { GenericDataType } from '@apache-superset/core/common'; import transformProps from '../src/transformProps'; @@ -1980,6 +1982,222 @@ describe('plugin-chart-table', () => { expect(totalCellAfter).toBeInTheDocument(); }); }); + + test('preserves client-side search text across temporal table rerenders', async () => { + const formDataWithSearch = { + ...testData.basic.formData, + include_search: true, + server_pagination: false, + }; + + const renderChart = () => { + const props = transformProps({ + ...testData.basic, + formData: formDataWithSearch, + }); + props.includeSearch = true; + + return ( + + + + ); + }; + + const { rerender } = render(renderChart()); + + const searchInput = screen.getByRole('textbox'); + fireEvent.change(searchInput, { target: { value: 'Michael' } }); + + await waitFor(() => { + expect(searchInput).toHaveValue('Michael'); + expect(screen.getByText('Michael')).toBeInTheDocument(); + }); + + rerender(renderChart()); + + await waitFor(() => { + expect(screen.getByRole('textbox')).toHaveValue('Michael'); + expect(screen.getByText('Michael')).toBeInTheDocument(); + }); + }); + + test('preserves client-side search text when rerendered with empty data', async () => { + const formDataWithSearch = { + ...testData.basic.formData, + include_search: true, + server_pagination: false, + }; + + const renderChart = (data = testData.basic.queriesData[0].data) => { + const props = transformProps({ + ...testData.basic, + formData: formDataWithSearch, + queriesData: [ + { + ...testData.basic.queriesData[0], + data, + }, + ], + }); + props.includeSearch = true; + + return ( + + + + ); + }; + + const { rerender } = render(renderChart()); + + const searchInput = screen.getByRole('textbox'); + fireEvent.change(searchInput, { target: { value: 'Michael' } }); + + await waitFor(() => { + expect(searchInput).toHaveValue('Michael'); + expect(screen.getByText('Michael')).toBeInTheDocument(); + }); + + rerender(renderChart([])); + + await waitFor(() => { + expect(screen.getByRole('textbox')).toHaveValue('Michael'); + expect(screen.getByLabelText('Search 0 records')).toHaveValue( + 'Michael', + ); + }); + }); + + test('preserves client-side search text for function accessor columns', async () => { + type DataRow = { + city: string; + firstName: string; + }; + + const makeColumns = (): Column[] => [ + { + Header: ({ column }: HeaderProps) => ( + First name + ), + Cell: ({ value }: CellProps) => {value}, + id: 'firstName', + accessor: ((row: DataRow) => row.firstName) as never, + }, + { + Header: ({ column }: HeaderProps) => ( + City + ), + Cell: ({ value }: CellProps) => {value}, + id: 'city', + accessor: ((row: DataRow) => row.city) as never, + }, + ]; + + const data: DataRow[] = [ + { firstName: 'Michael', city: 'Paris' }, + { firstName: 'Jordan', city: 'London' }, + ]; + + const renderDataTable = () => ( + + + columns={makeColumns()} + data={data} + rowCount={data.length} + serverPagination={false} + serverPaginationData={{}} + onServerPaginationChange={jest.fn()} + handleSortByChange={jest.fn()} + sortByFromParent={[]} + onSearchColChange={jest.fn()} + searchOptions={[]} + sticky={false} + /> + + ); + + const { rerender } = render(renderDataTable()); + + const searchInput = screen.getByRole('textbox'); + fireEvent.change(searchInput, { target: { value: 'Michael' } }); + + await waitFor(() => { + expect(searchInput).toHaveValue('Michael'); + expect(screen.getByText('Michael')).toBeInTheDocument(); + }); + + rerender(renderDataTable()); + + await waitFor(() => { + expect(screen.getByRole('textbox')).toHaveValue('Michael'); + expect(screen.getByText('Michael')).toBeInTheDocument(); + }); + }); + + test('preserves client-side search text for string accessor columns without ids', async () => { + type DataRow = { + city: string; + firstName: string; + }; + + const makeColumns = (): Column[] => [ + { + Header: ({ column }: HeaderProps) => ( + First name + ), + Cell: ({ value }: CellProps) => {value}, + accessor: 'firstName', + }, + { + Header: ({ column }: HeaderProps) => ( + City + ), + Cell: ({ value }: CellProps) => {value}, + accessor: 'city', + }, + ]; + + const data: DataRow[] = [ + { firstName: 'Michael', city: 'Paris' }, + { firstName: 'Jordan', city: 'London' }, + ]; + + const renderDataTable = () => ( + + + columns={makeColumns()} + data={data} + rowCount={data.length} + serverPagination={false} + serverPaginationData={{}} + onServerPaginationChange={jest.fn()} + handleSortByChange={jest.fn()} + sortByFromParent={[]} + onSearchColChange={jest.fn()} + searchOptions={[]} + sticky={false} + /> + + ); + + const { rerender } = render(renderDataTable()); + + const searchInput = screen.getByRole('textbox'); + fireEvent.change(searchInput, { target: { value: 'Michael' } }); + + await waitFor(() => { + expect(searchInput).toHaveValue('Michael'); + expect(screen.getByText('Michael')).toBeInTheDocument(); + }); + + rerender(renderDataTable()); + + await waitFor(() => { + expect(screen.getByRole('textbox')).toHaveValue('Michael'); + expect(screen.getByText('Michael')).toBeInTheDocument(); + }); + }); }); test('should build columnLabelToNameMap for adhoc columns with custom labels', () => { diff --git a/superset-frontend/src/pages/Chart/Chart.test.tsx b/superset-frontend/src/pages/Chart/Chart.test.tsx index b80ccc32b29..6f15faceb99 100644 --- a/superset-frontend/src/pages/Chart/Chart.test.tsx +++ b/superset-frontend/src/pages/Chart/Chart.test.tsx @@ -405,7 +405,8 @@ describe('ChartPage', () => { rejectFirstRequest = reject; }); - fetchMock.get(exploreApiRoute, () => firstRequestPromise); + const firstRequestHandler = jest.fn(() => firstRequestPromise); + fetchMock.get(exploreApiRoute, firstRequestHandler); render( <> @@ -419,16 +420,16 @@ describe('ChartPage', () => { }, ); - // Wait for the first request to be initiated - await waitFor(() => - expect(fetchMock.callHistory.calls(exploreApiRoute).length).toBe(1), - ); + // Wait for the initial request cycle to begin. Under CI, mount/navigation + // setup can trigger more than one explore fetch before history is cleared. + await waitFor(() => expect(firstRequestHandler).toHaveBeenCalled()); // Set up second request to return immediately fetchMock.clearHistory().removeRoutes(); - fetchMock.get(exploreApiRoute, { + const secondRequestHandler = jest.fn(() => ({ result: { dataset: { id: 1 }, form_data: exploreFormData }, - }); + })); + fetchMock.get(exploreApiRoute, secondRequestHandler); // Navigate to trigger a new request (which should abort the first) fireEvent.click(screen.getByText('Navigate')); @@ -441,10 +442,8 @@ describe('ChartPage', () => { // Wait for the first request to settle before asserting await firstRequestPromise.catch(() => undefined); - // Wait for the second request to complete - await waitFor(() => - expect(fetchMock.callHistory.calls(exploreApiRoute).length).toBe(1), - ); + // Wait for the replacement request to run after navigation. + await waitFor(() => expect(secondRequestHandler).toHaveBeenCalled()); // No error toast should be shown from the aborted first request expect(addDangerToastSpy).not.toHaveBeenCalled();