codeant-ai-for-open-source[bot] commented on code in PR #42248:
URL: https://github.com/apache/superset/pull/42248#discussion_r3616053823
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -2482,6 +2482,47 @@ describe('plugin-chart-table', () => {
);
expect(screen.queryByText('Search by')).toBeInTheDocument();
});
+
+ test('dropdown preserves serverPageLength if larger than rowCount and avoids
invalid 0 (#42243)', async () => {
+ const props = transformProps({
+ ...testData.basic,
+ formData: {
+ ...testData.basic.formData,
+ server_pagination: true,
+ },
+ });
+ props.serverPagination = true;
+ props.serverPageLength = 20;
+ props.rowCount = 12;
+ props.data = Array.from({ length: 12 }, (_, i) => ({
+ name: `Row ${i}`,
+ })) as any;
+
+ const { container } = render(
+ <ProviderWrapper>
+ <TableChart {...props} sticky={false} />
+ </ProviderWrapper>,
+ );
+
+ // Initial page size selector text
+ const pageSizeSelector = container.querySelector('.dt-select-page-size');
+ expect(pageSizeSelector).toHaveTextContent('20');
+
+ // Open dropdown to check options
+ const selectTrigger = container.querySelector('.ant-select-selector');
+ if (selectTrigger) {
+ fireEvent.mouseDown(selectTrigger);
+
+ await waitFor(() => {
+ const options = screen.getAllByRole('option');
+ const optionTexts = options.map(opt => opt.textContent);
+ expect(optionTexts).toContain('10');
+ expect(optionTexts).toContain('20');
+ expect(optionTexts).not.toContain('0');
+ expect(optionTexts).not.toContain('All');
+ });
+ }
Review Comment:
**Suggestion:** The assertions that validate dropdown options are wrapped in
a conditional, so if the selector is not found the test silently skips all
critical checks and still passes. Remove the conditional and assert the trigger
exists before interacting so regression failures are not masked. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Regression test may pass without verifying dropdown existence.
- ⚠️ Future server pagination bugs less likely to be caught.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx` and
locate
the regression test added at lines 2486–2525 for issue #42243, which renders
`TableChart`
and then queries `container.querySelector('.ant-select-selector')` into
`selectTrigger`
(line 2512).
2. Observe that all assertions validating the dropdown options (lines
2517–2523) are
wrapped inside `if (selectTrigger) { ... }` (lines 2513–2524), so they
execute only when
`selectTrigger` is non-null.
3. Consider a regression in `DataTable` or `TableChart` where the page-size
dropdown is
not rendered or its structure changes such that `.ant-select-selector` no
longer exists in
the DOM; in that situation,
`container.querySelector('.ant-select-selector')` returns
`null` at line 2512.
4. Run the test suite: because `selectTrigger` is `null`, the `if
(selectTrigger)` guard
prevents `fireEvent.mouseDown` and all `expect` calls from running, meaning
Jest reports
the test as passing even though the dropdown is missing and the regression
this test
intends to catch is not detected.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4be391c0f0584dcb8e4b8668ec4e2504&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=4be391c0f0584dcb8e4b8668ec4e2504&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:** 2512:2524
**Comment:**
*Logic Error: The assertions that validate dropdown options are wrapped
in a conditional, so if the selector is not found the test silently skips all
critical checks and still passes. Remove the conditional and assert the trigger
exists before interacting so regression failures are not masked.
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%2F42248&comment_hash=61adf943343f3874cf2002c0237de7d6735702bd8c55ddf499ca07029fdba9cc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42248&comment_hash=61adf943343f3874cf2002c0237de7d6735702bd8c55ddf499ca07029fdba9cc&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -461,11 +461,20 @@ export default typedMemo(function DataTable<D extends
object>({
resultPageCount = 0;
}
resultCurrentPageSize = serverPageSize;
- const foundPageSizeIndex = pageSizeOptions.findIndex(
- ([option]) => option >= resultCurrentPageSize,
+ const exactMatch = pageSizeOptions.some(
+ ([option]) => option === resultCurrentPageSize,
);
- if (foundPageSizeIndex === -1) {
- resultCurrentPageSize = 0;
+ if (!exactMatch) {
+ const nearestOption = pageSizeOptions.find(
+ ([option]) => option >= resultCurrentPageSize,
+ );
+ if (nearestOption) {
+ resultCurrentPageSize = nearestOption[0];
+ } else if (pageSizeOptions.length > 0) {
+ resultCurrentPageSize = pageSizeOptions[pageSizeOptions.length - 1][0];
+ } else {
+ resultCurrentPageSize = 0;
Review Comment:
**Suggestion:** When fallback logic changes the selected page-size value,
only the displayed selector value is updated; the server pagination
calculations and page-change callback still use the original server page size.
This can desynchronize what users see from what the backend pagination actually
uses. Keep the effective page size consistent across display, page count
calculation, and server callback inputs. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Page-size selector label inconsistent with actual server page size.
- ⚠️ Server pagination callbacks receive different size than UI shows.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render the DataTable component from
`superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx`
with
`serverPagination` enabled and `serverPaginationData?.pageSize` set to a
value that is not
present in `pageSizeOptions` (e.g. 15) while `pageSizeOptions` only contains
other values
(lines 458–459 and 467–476).
2. During rendering, at lines 458–459, `serverPageSize` is set from
`serverPaginationData?.pageSize` (e.g. 15) and `resultPageCount` is computed
using this
`serverPageSize`, so page-count logic is based on 15 rows per page.
3. At lines 463–476, `resultCurrentPageSize` is initially set to
`serverPageSize`, but
because `exactMatch` is false (line 464–466), the fallback block (lines
467–476) runs and
updates only `resultCurrentPageSize` to a different option from
`pageSizeOptions` (e.g.
20), while `serverPageSize` and `resultPageCount` remain based on the
original 15.
4. Later in the same file (outside this diff), the pagination UI uses
`resultCurrentPageSize` for the page-size selector while
`resultOnPageChange` (line
480–481) still calls `onServerPaginationChange(pageNumber, serverPageSize)`,
so the user
sees one page-size value in the selector (e.g. 20) but the server callback
and page-count
calculations still operate with the original size (e.g. 15).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b3ed0ffefe6348ecacbc888ecc570575&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=b3ed0ffefe6348ecacbc888ecc570575&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/src/DataTable/DataTable.tsx
**Line:** 467:476
**Comment:**
*Logic Error: When fallback logic changes the selected page-size value,
only the displayed selector value is updated; the server pagination
calculations and page-change callback still use the original server page size.
This can desynchronize what users see from what the backend pagination actually
uses. Keep the effective page size consistent across display, page count
calculation, and server callback inputs.
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%2F42248&comment_hash=a197a4c018a759f1afc37c72d292a4fb8432a7cc23e2ef65afe00cf634612c7a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42248&comment_hash=a197a4c018a759f1afc37c72d292a4fb8432a7cc23e2ef65afe00cf634612c7a&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]
