codeant-ai-for-open-source[bot] commented on code in PR #37975:
URL: https://github.com/apache/superset/pull/37975#discussion_r2929315540
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.test.tsx:
##########
@@ -189,6 +217,14 @@ test('should render the error', async () => {
expect(screen.getByText('Error: Something went wrong')).toBeInTheDocument();
});
+test('should render pagination when results exceed page size', async () => {
+ fetchWithPaginatedData();
+ await waitForRender();
+ // With total_count=100 and page size=50, pagination should render
+ const pagination = document.querySelector('.ant-pagination');
+ expect(pagination).toBeTruthy();
Review Comment:
**Suggestion:** This test validates only that pagination exists, but the bug
fix is about restricting page-size choices to the single valid value. As
written, it can pass even when invalid page sizes (5/15/25/100) are still
available, so regressions in the actual fix won't be caught. Update the test to
open the page-size control and assert that only `50 / page` is available.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ CI misses drill-detail page-size regression coverage.
- ❌ Invalid size options can reappear undetected.
- ⚠️ PR claim validated incompletely by current test.
```
</details>
```suggestion
test('should only expose the valid page size option', async () => {
fetchWithPaginatedData();
await waitForRender();
const pageSizeControl = await screen.findByText(/50\s*\/\s*page/i);
pageSizeControl.click();
expect(screen.queryByText(/5\s*\/\s*page/i)).not.toBeInTheDocument();
expect(screen.queryByText(/15\s*\/\s*page/i)).not.toBeInTheDocument();
expect(screen.queryByText(/25\s*\/\s*page/i)).not.toBeInTheDocument();
expect(screen.queryByText(/100\s*\/\s*page/i)).not.toBeInTheDocument();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Drill to Detail from chart context menu: `useDrillDetailMenuItems`
sets
`setShowModal(true)` at
`superset-frontend/src/components/Chart/useDrillDetailMenuItems/index.tsx:118-124,192-194`,
and `DrillDetailModal` renders `DrillDetailPane` at
`superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:149-153`.
2. In `DrillDetailPane`, the table is rendered with
`defaultPageSize={DEFAULT_PAGE_SIZE}`
but without `pageSizeOptions` at
`superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:314-335`;
Table
defaults include `['5','15','25','50','100']` at
`superset-frontend/packages/superset-ui-core/src/components/Table/index.tsx:288`.
3. Run the added test `should render pagination when results exceed page
size` in
`superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.test.tsx:220-226`;
it
only checks `.ant-pagination` exists.
4. Observe the test passes regardless of whether invalid size options are
still present,
because it never opens the page-size control nor asserts option contents;
regression in
the intended fix is therefore not detected.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.test.tsx
**Line:** 220:225
**Comment:**
*Logic Error: This test validates only that pagination exists, but the
bug fix is about restricting page-size choices to the single valid value. As
written, it can pass even when invalid page sizes (5/15/25/100) are still
available, so regressions in the actual fix won't be caught. Update the test to
open the page-size control and assert that only `50 / page` is available.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37975&comment_hash=bd0293663cf2d4693d6756e4e4061a5df0a4809df79f86a6a98cef49441293d7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37975&comment_hash=bd0293663cf2d4693d6756e4e4061a5df0a4809df79f86a6a98cef49441293d7&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]
