codeant-ai-for-open-source[bot] commented on code in PR #42155:
URL: https://github.com/apache/superset/pull/42155#discussion_r3607268990
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -2292,6 +2292,96 @@ describe('plugin-chart-table', () => {
});
});
+ test('should not reset pagination when a cell is clicked (#42010)', async
() => {
+ type DataRow = {
+ id: number;
+ name: string;
+ value: number;
+ };
+
+ const columns: Column<DataRow>[] = [
+ {
+ Header: 'ID',
+ accessor: 'id',
+ id: 'id',
+ Cell: ({ value }: CellProps<DataRow>) => <td>{value}</td>,
+ },
+ {
+ Header: 'Name',
+ accessor: 'name',
+ id: 'name',
+ Cell: ({ value }: CellProps<DataRow>) => <td>{value}</td>,
+ },
+ {
+ Header: 'Value',
+ accessor: 'value',
+ id: 'value',
+ Cell: ({ value }: CellProps<DataRow>) => <td>{value}</td>,
+ },
+ ];
+
+ // 30 rows with page size 10 = 3 pages
+ const data: DataRow[] = Array.from({ length: 30 }, (_, i) => ({
+ id: i + 1,
+ name: `User ${i + 1}`,
+ value: (i + 1) * 100,
+ }));
+
+ const { container } = render(
+ <ProviderWrapper>
+ <DataTable<DataRow>
+ columns={columns}
+ data={data}
+ rowCount={data.length}
+ pageSize={10}
+ serverPagination={false}
+ serverPaginationData={{}}
+ onServerPaginationChange={jest.fn()}
+ handleSortByChange={jest.fn()}
+ sortByFromParent={[]}
+ onSearchColChange={jest.fn()}
+ searchOptions={[]}
+ sticky={false}
+ searchInput={false}
+ selectPageSize={false}
+ />
+ </ProviderWrapper>,
+ );
+
+ // Verify we're on page 1 (showing rows 1-10)
+ const getRowTexts = () =>
+ Array.from(container.querySelectorAll('tbody td:nth-child(2)')).map(
+ td => td.textContent,
+ );
+ expect(getRowTexts()).toContain('User 1');
+ expect(getRowTexts()).not.toContain('User 11');
+
+ // Navigate to page 2 by clicking the page 2 link
+ const page2Link = container.querySelector('a[href="#page-1"]')!;
+ expect(page2Link).toBeTruthy();
+ fireEvent.click(page2Link);
+
+ // Wait for page 2 to render (showing rows 11-20)
+ await waitFor(() => {
+ expect(getRowTexts()).toContain('User 11');
+ expect(getRowTexts()).not.toContain('User 1');
+ });
+
+ // Click a cell on page 2
+ const cell = screen.getByText('User 11');
+ fireEvent.click(cell);
+
Review Comment:
**Suggestion:** This regression test does not exercise the real bug path
because it mounts `DataTable` with plain `<td>` cell renderers that have no
click side effects. Clicking `User 11` here only clicks static text, so no
filter/state update occurs and pagination cannot reset, which means the test
can pass even if the original cell-click pagination regression in `TableChart`
still exists. Mount the chart-level component/path that wires cell `onClick`
behavior (or explicitly add equivalent click handlers in the test columns) so
the test validates the actual integration that regressed. [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ TableChart cell-click pagination regression may go undetected.
- ⚠️ Cross-filter table behavior lacks effective regression test.
- ⚠️ Test suite gives false confidence about pagination stability.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the TableChart cell click behavior in
`superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx` around
lines 1059–1237,
where `getColumnConfigs` defines a `Cell` renderer that returns a styled
<td> with an
`onClick` handler calling `toggleFilter(key, value)` when `emitCrossFilters`
is true, and
`DataTable` is wired via `cell.render('Cell', { key: cell.column.id })` in
`DataTable.tsx`
lines 400–408.
2. Note that the new regression test in
`superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx`
lines 96–120
defines `columns: Column<DataRow>[]` where each `Cell: ({ value }:
CellProps<DataRow>) =>
<td>{value}</td>,` (lines 108, 114, 120) returns a plain <td> element with
no `onClick` or
cross-filter side effects.
3. Observe that the test mounts `<DataTable<DataRow> ... />` directly (lines
13–28) with
these columns and then does `const cell = screen.getByText('User 11');
fireEvent.click(cell);` (lines 52–53), so the click targets a bare <td>
without any event
handler wired by TableChart’s `toggleFilter` logic.
4. If the original pagination-reset bug in TableChart is reintroduced (for
example by
reverting the `autoResetPage` logic or the cross-filter handling in
`TableChart.tsx` while
leaving DataTable unchanged), clicks on real chart cells would again reset
the page in the
UI, but this test would still pass because it never exercises TableChart’s
`Cell`
`onClick` path—only static DataTable cells—so no filter/state change occurs
and pagination
cannot reset under this test setup.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e954f1ed61de4af9bdccaff3228a9e1f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e954f1ed61de4af9bdccaff3228a9e1f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
**Line:** 2307:2373
**Comment:**
*Incomplete Implementation: This regression test does not exercise the
real bug path because it mounts `DataTable` with plain `<td>` cell renderers
that have no click side effects. Clicking `User 11` here only clicks static
text, so no filter/state update occurs and pagination cannot reset, which means
the test can pass even if the original cell-click pagination regression in
`TableChart` still exists. Mount the chart-level component/path that wires cell
`onClick` behavior (or explicitly add equivalent click handlers in the test
columns) so the test validates the actual integration that regressed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42155&comment_hash=6334db28361ae05cfe4cfd71cfd36d67be187897ff19e68bb36b4c0204e9bc7e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42155&comment_hash=6334db28361ae05cfe4cfd71cfd36d67be187897ff19e68bb36b4c0204e9bc7e&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
