bito-code-review[bot] commented on code in PR #41346:
URL: https://github.com/apache/superset/pull/41346#discussion_r3461657585
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts:
##########
@@ -1363,4 +1365,170 @@ describe('plugin-chart-ag-grid-table', () => {
expect(query.extras?.where || undefined).toBeUndefined();
});
});
+
+ describe('buildQuery - server pagination row limit', () => {
+ const serverPaginationFormData: TableChartFormData = {
+ ...basicFormData,
+ server_pagination: true,
+ };
+
+ test('uses user row limit when it is lower than server page size', () => {
+ const { queries } = buildQuery(
+ {
+ ...serverPaginationFormData,
+ row_limit: 10,
+ server_page_length: 20,
+ slice_id: 101,
+ },
+ {
+ ownState: {
+ currentPage: 0,
+ pageSize: 20,
+ },
+ },
+ );
+
+ expect(queries[0]).toMatchObject({
+ row_limit: 10,
+ row_offset: 0,
+ });
+ });
+
+ test('limits server page size by remaining rows inside user row limit', ()
=> {
+ const { queries } = buildQuery(
+ {
+ ...serverPaginationFormData,
+ row_limit: 120,
+ server_page_length: 50,
+ slice_id: 102,
+ },
+ {
+ ownState: {
+ currentPage: 2,
+ pageSize: 50,
+ },
+ },
+ );
+
+ expect(queries[0]).toMatchObject({
+ row_limit: 20,
+ row_offset: 100,
+ });
+ });
+
+ test('clamps pages beyond the row limit instead of emitting row_limit: 0',
() => {
+ const { queries } = buildQuery(
+ {
+ ...serverPaginationFormData,
+ row_limit: 120,
+ server_page_length: 50,
+ slice_id: 103,
+ },
+ {
+ ownState: {
+ // Page 5 is well past the cap; offset would be 250 > 120, which
+ // previously made row_limit collapse to 0 ("no limit").
+ currentPage: 5,
+ pageSize: 50,
+ },
+ },
+ );
+
+ expect(queries[0].row_limit).not.toBe(0);
+ expect(queries[0]).toMatchObject({
+ row_limit: 20,
+ row_offset: 100,
+ });
+ });
+
+ test('restores the full first-page row limit after a filter change reset',
() => {
+ // Uncached export lets us seed cachedChanges directly; the default
+ // export overrides extras with its own closure.
+ const { queries } = buildQueryUncached(
+ {
+ ...serverPaginationFormData,
+ row_limit: 120,
+ server_page_length: 50,
+ slice_id: 104,
+ },
+ {
+ // User was on the capped last page (row_limit would be 20)...
+ ownState: {
+ currentPage: 2,
+ pageSize: 50,
+ },
+ // ...then an external filter changed, so the cached filters differ
+ // from the current ones and pagination resets to page 0.
+ extras: {
+ cachedChanges: {
+ 104: [{ col: 'state', op: '==', val: 'previous' }],
+ },
+ },
+ },
+ );
+
+ expect(queries[0].row_limit).not.toBe(0);
+ expect(queries[0]).toMatchObject({
+ row_limit: 50,
+ row_offset: 0,
+ });
+ });
+
+ test('persists the user page size, not the capped limit, on filter reset',
() => {
+ const setDataMask = jest.fn();
+ buildQueryUncached(
+ {
+ ...serverPaginationFormData,
+ row_limit: 120,
+ server_page_length: 50,
+ slice_id: 106,
+ },
+ {
+ // On the capped last page, the per-request row_limit is 20.
+ ownState: {
+ currentPage: 2,
+ pageSize: 50,
+ },
+ extras: {
+ cachedChanges: {
+ 106: [{ col: 'state', op: '==', val: 'previous' }],
+ },
+ },
+ hooks: { setDataMask, setCachedChanges: jest.fn() },
+ },
+ );
+
+ // The persisted page size must stay 50, not collapse to the capped 20.
+ expect(setDataMask).toHaveBeenCalledWith(
+ expect.objectContaining({
+ ownState: expect.objectContaining({
+ currentPage: 0,
+ pageSize: 50,
+ }),
+ }),
+ );
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing hook assertion after mock</b></div>
<div id="fix">
The `setCachedChanges` mock is created but never asserted. The
implementation calls `setCachedChanges` unconditionally at buildQuery.ts line
418 with `{ [slice_id]: queryObject.filters }`, so this call always fires. The
test should either verify it or document why verification is unnecessary.
</div>
</div>
<small><i>Code Review Run #e02f2a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -58,7 +58,7 @@ export function getQueryMode(formData: TableChartFormData) {
return hasRawColumns ? QueryMode.Raw : QueryMode.Aggregate;
}
-const buildQuery: BuildQuery<TableChartFormData> = (
+export const buildQuery: BuildQuery<TableChartFormData> = (
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Consolidate duplicate buildQuery logic</b></div>
<div id="fix">
This PR introduces syntactic duplication in buildQuery functions across two
plugin chart implementations. The duplicate code spans multiple blocks (31
lines at lines 61-91 in plugin-chart-ag-grid-table vs lines 57-87 in
plugin-chart-table, etc.) totaling significant duplicated logic. Consider
extracting the common buildQuery implementation into a shared utility module to
improve maintainability and reduce code duplication.
</div>
</div>
<small><i>Code Review Run #e02f2a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
