bito-code-review[bot] commented on code in PR #40907:
URL: https://github.com/apache/superset/pull/40907#discussion_r3976127047
##########
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:
<!-- Bito Reply -->
The fix is correct. Adding `serverPagination` to the dependency array
ensures that the `useCallback` hook correctly captures the updated value,
preventing the stale closure issue identified in the review.
**superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx**
```
[serverPagination, serverPaginationData, setDataMask],
```
##########
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:
<!-- Bito Reply -->
It is reasonable to defer adding tests when a local environment is
unavailable. The suggestion to add test coverage for the new `orderby.length
=== 0` guard is valid, as it ensures the logic correctly preserves explicit
orderby settings while applying defaults only when necessary. You can address
this as a follow-up task once you have access to a suitable testing environment.
##########
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:
<!-- Bito Reply -->
That is a reasonable approach. Prioritizing verified code over adding
untested logic is a sound practice, especially when a suitable environment is
unavailable. You can proceed with the current changes and treat the test
coverage as a separate, follow-up task.
--
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]
