EnxDev commented on PR #41754:
URL: https://github.com/apache/superset/pull/41754#issuecomment-4894007582

   Both CodeAnt findings were valid; fixed in ca9d23c30a and 0cb8c56f82 
(254/254 plugin tests passing).
   
   **`AgGridTableChart.tsx` — falsy-zero page clamp** (re: [the clamp 
thread](https://github.com/apache/superset/pull/41754#discussion_r3529558409), 
and @alexandrusoare's question): confirmed. The falsy `rowCount` guard skipped 
the clamp entirely on empty result sets — behavior this branch inherited from 
the pre-existing standalone effect and kept when merging it into the unified 
ownState delta. The guard now checks `rowCount !== undefined` and the clamp is 
a single `Math.max(0, Math.min(currentPage, totalPages - 1))`, so zero rows 
reset to page 0 and a shrunken result set lands on its last remaining page.
   
   One correction to the snippet proposed in that thread: clamping every 
`currentPage >= totalPages` case to `0` would lose the user's position on 
ordinary shrinks — page 5 over 3 remaining pages must land on page 2, which an 
existing regression test in the suite pins. The min/max form covers both cases. 
New test: raw mode + server pagination + `rowCount: 0` asserts the unified 
write carries `currentPage: 0` together with the totals keys, pinning the 
behavior to this effect rather than the pagination footer's own page reset.
   
   **`buildQuery.ts` — stale `ownState.rawSummaryColumns`** (re: [the totals 
thread](https://github.com/apache/superset/pull/41754#discussion_r3529558424)): 
confirmed. For a datasource swap this was a real wedge: the stale name fails 
the totals query server-side, the chart errors before rendering, and the 
priming effect never gets a chance to clear its own state. `buildQuery` now 
intersects `ownState.rawSummaryColumns` with the current `all_columns` labels 
before emitting SUM metrics, so a persisted name that left the selection (or 
belonged to a previous datasource) can never reach the totals query; if nothing 
survives the intersection, no totals query is emitted at all. Two new tests 
cover the partial and empty intersections.
   


-- 
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]

Reply via email to