bito-code-review[bot] commented on code in PR #40907:
URL: https://github.com/apache/superset/pull/40907#discussion_r3382409751
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1524,10 +1536,13 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
const modifiedOwnState = {
...serverPaginationData,
sortBy,
+ // Changing the sort re-queries the full dataset, so the
+ // previous page offset is meaningless — return to the first page.
+ currentPage: 0,
};
updateTableOwnState(setDataMask, modifiedOwnState);
},
- [serverPagination, serverPaginationData, setDataMask],
+ [serverPaginationData, setDataMask],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing dependency in useCallback</b></div>
<div id="fix">
The `handleSortByChange` callback captures `serverPagination` in the
function body (line 1535: `if (!serverPagination) return`), but this variable
is missing from the dependency array. This creates a stale closure that can
cause the callback to use an outdated `serverPagination` value after prop
changes.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
+++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
@@ -1542,7 +1542,7 @@ export default function TableChart<D extends
DataRecord = DataRecord>(
};
updateTableOwnState(setDataMask, modifiedOwnState);
},
- [serverPaginationData, setDataMask],
+ [serverPagination, serverPaginationData, setDataMask],
);
const handleSearch = (searchText: string) => {
```
</div>
</details>
</div>
<small><i>Code Review Run #0a5875</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:
##########
@@ -131,13 +131,18 @@ const buildQuery: BuildQuery<TableChartFormData> = (
if (queryMode === QueryMode.Aggregate) {
metrics = metrics || [];
- // override orderby with timeseries metric when in aggregation mode
- if (sortByMetric) {
- orderby = [[sortByMetric, !orderDesc]];
- } else if (metrics?.length > 0) {
- // default to ordering by first metric in descending order
- // when no "sort by" metric is set (regardless if "SORT DESC" is set
to true)
- orderby = [[metrics[0], false]];
+ // Fall back to a metric-based default sort only when no explicit orderby
+ // was supplied (e.g. a column sort from the "View as table" results
pane).
+ // An explicit orderby from form data takes precedence.
+ if (orderby.length === 0) {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test for explicit orderby</b></div>
<div id="fix">
The new guard `if (orderby.length === 0)` correctly prevents overriding
explicit orderby from form data. However, no test covers this scenario —
existing tests use `basicFormData` without explicit orderby. Add a test with
`orderby: [['state', true]]` alongside `sortByMetric` to verify the explicit
orderby is preserved.
</div>
</div>
<small><i>Code Review Run #0a5875</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/src/components/GridTable/types.ts:
##########
@@ -59,4 +59,12 @@ export interface TableProps<RecordType> {
usePagination?: boolean;
striped?: boolean;
+
+ /**
+ * Called when the sort state changes, with it translated to a query
+ * `orderby` (`[[columnId, isAscending]]`) so the consumer can re-request
+ * server-sorted data. Only the primary sort column is included, matching the
+ * chart data API's single-column sort constraint.
+ */
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Doc/impl mismatch on multi-column sort</b></div>
<div id="fix">
Update the JSDoc for `onServerSort` to clarify that when multi-column
sorting is used, all sorted columns are passed to the callback in priority
order, matching the implementation.
</div>
</div>
<small><i>Code Review Run #0a5875</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-table/src/buildQuery.ts:
##########
@@ -128,13 +128,18 @@ const buildQuery: BuildQuery<TableChartFormData> = (
if (queryMode === QueryMode.Aggregate) {
metrics = metrics || [];
- // override orderby with timeseries metric when in aggregation mode
- if (sortByMetric) {
- orderby = [[sortByMetric, !orderDesc]];
- } else if (metrics?.length > 0) {
- // default to ordering by first metric in descending order
- // when no "sort by" metric is set (regardless if "SORT DESC" is set
to true)
- orderby = [[metrics[0], false]];
+ // Fall back to a metric-based default sort only when no explicit orderby
+ // was supplied (e.g. a column sort from the "View as table" results
pane).
+ // An explicit orderby from form data takes precedence.
+ if (orderby.length === 0) {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for orderby logic</b></div>
<div id="fix">
The new `if (orderby.length === 0)` guard changes behavior to preserve
explicit orderby, but `buildQuery.test.ts` has no tests validating this logic.
Add tests covering: (a) explicit orderby preserved when sortByMetric exists,
(b) explicit orderby preserved when metrics exist, (c) defaults applied only
when orderby is empty.
</div>
</div>
<small><i>Code Review Run #0a5875</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]
