feat(waterfall): custom sort for X-axis categories#42372
Conversation
dosubot
Bot
added
change:frontend
Code Review Agent Run #392611Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const isMetricSort = | ||
| !!x_axis_sort && | ||
| ensureIsArray(baseQueryObject.metrics).some( | ||
| metric => getMetricLabel(metric) === x_axis_sort, | ||
| ); | ||
| // A sort column that isn't already selected (and isn't a metric) must be | ||
| // added to the query so that ORDER BY can reference it. | ||
| const extraSortColumns = | ||
| x_axis_sort && !isMetricSort && !columns.includes(x_axis_sort) | ||
| ? [x_axis_sort] | ||
| : []; |
There was a problem hiding this comment.
Suggestion: If the user selects metric-based sorting and then changes the metric, x_axis_sort can retain the old metric label; this logic then treats that stale value as a column and injects it into columns, which can produce backend query failures for unknown columns. Validate that the selected sort key is still present in current column/metric choices before adding it to the query, and drop/reset invalid stale values. [logic error]
Severity Level: Major ⚠️
- ❌ Waterfall chart errors after changing metric with sort enabled.
- ⚠️ Users see backend query errors instead of updated chart.Steps of Reproduction ✅
1. Open a Waterfall chart in Explore that uses the Waterfall control panel defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:32-88`, and
set a metric in the `metric` control plus a sort by that metric via the `x_axis_sort`
SelectControl (lines 43-71).
2. Change the `metric` control to a different metric without touching `x_axis_sort`;
`mapStateToProps` for `x_axis_sort` at `controlPanel.tsx:58-69` rebuilds `choices` using
only the **current** metric label, but there is no logic here to clear or validate the
existing `controls.x_axis_sort.value`, so it can retain the old metric label as a stale
string.
3. Trigger a query (Run) so that `buildQuery(formData)` in
`superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/buildQuery.ts:26-65` is
called from
`superset-frontend/src/chartCustomizations/components/DeckglLayerVisibility/buildQuery.ts:27`,
passing `formData.x_axis_sort` containing the stale metric label and
`baseQueryObject.metrics` containing only the new metric.
4. Inside `buildQuery.ts`, `isMetricSort` at lines 39-42 evaluates to false because
`getMetricLabel` on the current metric no longer matches the stale `x_axis_sort` label;
`extraSortColumns` at lines 46-49 therefore treats `x_axis_sort` as a column name and
appends it to `columns`, and `orderby` at lines 53-56 orders by this same string, causing
the backend query to reference a non-existent column and return a query error instead of
rendering the updated chart.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/buildQuery.ts
**Line:** 39:49
**Comment:**
*Logic Error: If the user selects metric-based sorting and then changes the metric, `x_axis_sort` can retain the old metric label; this logic then treats that stale value as a column and injects it into `columns`, which can produce backend query failures for unknown columns. Validate that the selected sort key is still present in current column/metric choices before adding it to the query, and drop/reset invalid stale values.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| description: t( | ||
| 'Whether to sort ascending or descending on the X-axis.', | ||
| ), |
There was a problem hiding this comment.
Suggestion: The user-facing description says the toggle controls sorting on the X-axis, but the implementation applies it to whatever field is selected in “Sort by” (which may be a metric or another column). Update the text to match actual behavior to avoid misleading users configuring sort semantics. [comment mismatch]
Severity Level: Minor 🧹
- ⚠️ Description slightly imprecise but matches effective axis behavior.
- ⚠️ Suggestion mainly cosmetic; functionality already matches intent.Steps of Reproduction ✅
1. In Explore for a Waterfall chart, enable the `x_axis_sort` control defined at
`superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:43-71`,
which makes the `x_axis_sort_asc` checkbox visible due to its `visibility` function at
lines 84-85.
2. Hover over or inspect the `x_axis_sort_asc` control, whose description is defined at
lines 81-83 as `t('Whether to sort ascending or descending on the X-axis.')`.
3. Run the chart and observe in
`superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/buildQuery.ts:53-56` that
the ascending/descending flag is applied to whatever field is selected in `x_axis_sort`
when constructing `orderby`, but in all cases this ultimately determines the left-to-right
order of categories along the X-axis.
4. Since toggling this checkbox does in fact control the ascending/descending order of
categories on the X-axis (regardless of whether the sort key is a metric or column), the
current description is broadly accurate; the suggestion is largely about phrasing nuance
rather than a functional mismatch, so the underlying issue is minor and mostly cosmetic.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx
**Line:** 81:83
**Comment:**
*Comment Mismatch: The user-facing description says the toggle controls sorting on the X-axis, but the implementation applies it to whatever field is selected in “Sort by” (which may be a metric or another column). Update the text to match actual behavior to avoid misleading users configuring sort semantics.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42372 +/- ##
=======================================
Coverage 65.26% 65.26%
=======================================
Files 2794 2794
Lines 157516 157538 +22
Branches 36037 36050 +13
=======================================
+ Hits 102797 102816 +19
- Misses 52741 52744 +3
Partials 1978 1978
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The Waterfall chart hard-coded its category order to the x-axis value ascending, so a narrative order (e.g. Previous -> New -> Churn -> Current) was impossible and there was no sort control at all. Add "Sort by" and "Sort ascending" controls. "Sort by" offers any dataset column or the metric; picking a dedicated sort column gives true custom categorical order, which is the idiomatic Superset way to reproduce a Tableau-style manual sort. Ordering is applied at the query level (before the cumulative running total) via ORDER BY, and a chosen sort column is auto-selected so ORDER BY can reference it. When no sort is set the query is byte-identical to before, so existing charts are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
716adb3 to
5cc4220
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #9040ae
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/types.ts - 1
- Type naming mismatch with formData · Line 54-55
Review Details
-
Files reviewed - 4 · Commit Range:
5cc4220..5cc4220- superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/buildQuery.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/types.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Waterfall/buildQuery.test.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| xAxisSort?: string; | ||
| xAxisSortAsc?: boolean; |
There was a problem hiding this comment.
The type definition uses camelCase (xAxisSort/xAxisSortAsc) but the actual control names and formData keys use snake_case (x_axis_sort/x_axis_sort_asc). This mismatch between EchartsWaterfallFormData and the implementation could mislead developers into expecting formData.xAxisSort when the actual key is formData.x_axis_sort.
Code Review Run #9040ae
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
1 participant
SUMMARY
The Waterfall chart hard-coded its category ordering in
buildQuery:So categories always came back alphabetically by the x-axis value, and there was no sort control in the panel. A narrative movement order — e.g. Previous → New business → Expansion → Churn → Current — was impossible.
This adds two controls to the Query section:
sort_orderthat is 1:1 with the category) yields true custom categorical order — the idiomatic Superset way to reproduce a Tableau-style manual sort.Ordering is applied at the query level (via
ORDER BY), which is important because the waterfall's cumulative running total is computed in row order. A chosen sort column that isn't already selected is auto-added to the query soORDER BYcan reference it, and the x-axis/breakdown columns are kept as secondary sort keys so each category's rows stay contiguous. When no sort is set, the query is byte-identical to before — existing charts are unchanged.Note: sorting by a category-level column gives an exact custom order (the recommended pattern). Sorting by the metric while a breakdown is present orders at the row level, so category placement follows first appearance — defined behavior, but a dedicated sort column is the intended approach.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — new control-panel options; with no sort selected, output is unchanged. See testing instructions to verify custom ordering.
TESTING INSTRUCTIONS
stageand an integerstage_order).Unit tests:
cd superset-frontend && npx jest plugins/plugin-chart-echarts/test/Waterfall/— addedbuildQuerycases cover the default (unchanged)orderby, custom-column sort with auto-selection, descending, and metric-sort (no duplicate column).ADDITIONAL INFORMATION
🤖 Generated with Claude Code