feat(chart): enable cross-filter on x-axis labels for bar, line, area and scatter charts - #41111
Conversation
… and scatter charts - Add QueryEventHandlers type to support query-based ECharts event registration - Extend Echart.tsx wrapper to support chart.on(event, query, handler) pattern - Add handleXAxisLabelClick handler in EchartsTimeseries.tsx to capture x-axis label clicks and emit cross-filter via existing handleXAxisChange - Add triggerEvent: true to xAxis config in transformProps.ts when axis type is categorical and no dimensions are set Closes apache#25334
Code Review Agent Run #6c2f19Actionable Suggestions - 0Additional Suggestions - 2
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 |
dosubot
Bot
added
change:frontend
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. In To resolve this, you should clear all query listeners for the specific event name before re-binding, or ensure the handler reference is stable. A minimal fix is to use (queryEventHandlers || []).forEach(({ name, query, handler }) => {
chartRef.current?.off(name);
chartRef.current?.on(name, query, handler);
});This ensures that any previously registered handlers for that event name are cleared, preventing duplicate triggers. superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx |
Code Review Agent Run #8b840eActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
| const queryEventHandlers = useMemo( | ||
| () => [ | ||
| { | ||
| name: 'click', | ||
| query: 'xAxis.category', | ||
| handler: handleXAxisLabelClick, | ||
| }, | ||
| ], | ||
| [handleXAxisLabelClick], | ||
| ); |
There was a problem hiding this comment.
Suggestion: The query event is hardcoded to xAxis.category, so horizontal charts (where categorical labels are rendered on yAxis after axis swap) will never emit this handler. As a result, label-click cross-filter silently fails for horizontal bar configurations. Register the query dynamically based on chart orientation/actual categorical axis (e.g., yAxis.category for horizontal mode). [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Horizontal bar chart axis-label clicks emit no cross-filters.
- ⚠️ Generic Chart horizontal layouts lose label-driven filtering.Steps of Reproduction ✅
1. In Explore, create a Bar Chart using the ECharts timeseries bar plugin, which is wired
to `EchartsTimeseries` via `loadChart: () => import('../../EchartsTimeseries')` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/index.ts:8-25`.
2. In the Bar Chart control panel, set orientation to Horizontal (control wired through
`orientation` in `DEFAULT_FORM_DATA` and `OrientationType.Horizontal` usage in
`controlPanel.tsx:54-69`), and configure no groupby dimensions so `groupBy.length === 0`
and the x-axis is categorical, causing `canCrossFilterByXAxis` to be true in
`EchartsTimeseries`
(`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:98-218`).
3. When the chart renders, `transformProps` builds a categorical axis and, after handling
orientation and axis swapping, returns the logical axis metadata `{ label: xAxisLabel,
type: xAxisType }` as `xAxis` in the transformed props (`transformProps.ts:188-196` and
return at `transformProps.ts:73-92`), while `EchartsTimeseries` installs
`queryEventHandlers` with `query: 'xAxis.category'` at `EchartsTimeseries.tsx:340-345` and
passes them to the `Echart` wrapper, which calls `chartRef.current?.on(name, query,
handler)` (`components/Echart.tsx:204-216`).
4. For horizontal charts, the categorical axis is rendered as `yAxis` after the swap in
`transformProps` (`transformProps.ts:79-81`), so axis label clicks are emitted with
`componentType: 'yAxis'`; because the handler is hardcoded to `query: 'xAxis.category'`,
ECharts never dispatches these label-click events to `handleXAxisLabelClick`,
`handleXAxisChange` is never called, and `setDataMask(getXAxisCrossFilterDataMask(...))`
at `EchartsTimeseries.tsx:205-211` is never invoked—clicking categorical labels in
horizontal charts does nothing and no cross-filter is emitted.
(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/Timeseries/EchartsTimeseries.tsx
**Line:** 340:349
**Comment:**
*Incorrect Condition Logic: The query event is hardcoded to `xAxis.category`, so horizontal charts (where categorical labels are rendered on `yAxis` after axis swap) will never emit this handler. As a result, label-click cross-filter silently fails for horizontal bar configurations. Register the query dynamically based on chart orientation/actual categorical axis (e.g., `yAxis.category` for horizontal mode).
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- Type query event handlers with ECharts event payloads instead of any - Register axis-label click handlers against the rendered categorical axis so horizontal bar charts use yAxis.category - Add regression coverage for vertical vs horizontal query event binding - Assert categorical axes enable triggerEvent for label click handling
Code Review Agent Run #bd5bbeActionable Suggestions - 0Review 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 |
Code Review Agent Run #bb691bActionable Suggestions - 0Review 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 |
rusackas
added
the
review:checkpoint
|
| Language | Fuzzy before | Fuzzy after | New |
|---|---|---|---|
fi |
4815 | 4817 | +2 |
it |
1665 | 1667 | +2 |
ko |
1519 | 1521 | +2 |
th |
4815 | 4817 | +2 |
How to fix
1. Install dependencies (if not already set up):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.
3. Resolve the fuzzy entries in the affected language files (fi, it, ko, th):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.
4. Commit your changes to the .po files.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41111 +/- ##
==========================================
- Coverage 64.36% 63.78% -0.59%
==========================================
Files 2653 2653
Lines 144965 145001 +36
Branches 33437 33455 +18
==========================================
- Hits 93305 92483 -822
- Misses 49974 50834 +860
+ Partials 1686 1684 -2
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:
|
| const getCategoryAxisValue = useCallback( | ||
| (data: unknown) => | ||
| Array.isArray(data) ? data[categoryAxisValueIndex] : undefined, | ||
| [categoryAxisValueIndex], | ||
| ); |
There was a problem hiding this comment.
Suggestion: The category extractor assumes event data is always an array, but ECharts often sends point objects like { value: [...] } (for example when color-by-primary-axis or styled points are used). In those cases this returns undefined, so point-click and context-menu cross-filtering silently stop working even though the chart is categorical. Read from both array data and object value payloads (or use the event name) so category extraction works across all ECharts payload shapes. [api mismatch]
Severity Level: Major ⚠️
- ❌ Color-by-primary-axis timeseries charts can't emit cross-filters.
- ❌ Context-menu X-axis cross-filter fails with styled point objects.Steps of Reproduction ✅
1. Open Explore with the "Generic Chart" (Echarts timeseries) viz, which is wired via
`EchartsTimeseriesChartPlugin` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/index.ts` and uses
`transformProps` + `EchartsTimeseries.tsx` for rendering.
2. Configure a categorical X-axis with no groupby (so `hasDimensions` is false and
`xAxis.type === AxisType.Category`), enable cross-filtering, set series type to bar, and
enable `colorByPrimaryAxis` (all handled in `transformProps` at
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`).
3. With `colorByPrimaryAxis` enabled, `transformSeries` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts` replaces
each raw `[x, y]` tuple with an object `{ value: [x, y], itemStyle: … }` via
`applyColorByPrimaryAxis` (see `transformNegativeLabelsPosition` and
`applyColorByPrimaryAxis` around lines ~145–55 and ~179–55 in that file), so ECharts click
events deliver `props.data` as an object instead of an array.
4. Click any bar: `eventHandlers.click` in `EchartsTimeseries.tsx` (around lines 233–247)
calls `getCategoryAxisValue(props.data)` (lines 226–230), which only handles
`Array.isArray(data)` and returns `undefined` for the `{ value: [...] }` object; the
subsequent type guard fails, `handleXAxisChange` is never called, and no X-axis
cross-filter is emitted even though `canCrossFilterByXAxis` is true—confirming the bug
that object-shaped event payloads break categorical cross-filtering.
(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/Timeseries/EchartsTimeseries.tsx
**Line:** 226:230
**Comment:**
*Api Mismatch: The category extractor assumes event `data` is always an array, but ECharts often sends point objects like `{ value: [...] }` (for example when color-by-primary-axis or styled points are used). In those cases this returns `undefined`, so point-click and context-menu cross-filtering silently stop working even though the chart is categorical. Read from both array data and object `value` payloads (or use the event `name`) so category extraction works across all ECharts payload shapes.
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 fixThere was a problem hiding this comment.
Thanks for catching this. I updated the category extractor to read the orientation-specific array value first, and fall back to the ECharts event name when the payload is not an array or does not contain a string/number category value. This keeps the horizontal bar fix while preserving category cross-filtering for other ECharts payload shapes.
Code Review Agent Run #ff2737Actionable Suggestions - 0Review 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 |
| if (typeof name === 'string' || typeof name === 'number') { | ||
| return name; |
There was a problem hiding this comment.
Suggestion: Falling back to name without checking event source can convert non-category interactions (for example legend/component clicks that also carry a string name) into X-axis filters, producing incorrect filter values for the X-axis column. Only use name when the event is known to come from category-axis label clicks (or series points with verified category payload), otherwise ignore it. [logic error]
Severity Level: Major ⚠️
- ❌ Legend clicks can apply incorrect X-axis filters.
- ⚠️ Users see unexpected filters after legend toggles.Steps of Reproduction ✅
1. Use an ECharts timeseries chart with no `groupby` and a categorical X-axis (`xAxis.type
=== AxisType.Category`), so `hasDimensions` is false and `canCrossFilterByXAxis` is true
as defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:221-223`.
Cross-filtering is enabled via `emitCrossFilters` and `setDataMask` from
`CrossFilterTransformedProps` in
`superset-frontend/plugins/plugin-chart-echarts/src/types.ts:156-163`.
2. At render time, `Echart` registers a generic `click` handler for all chart clicks at
`superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:90-93`, wiring
it to `eventHandlers.click` defined in `EchartsTimeseries.tsx:246-271`. The same
`eventHandlers` object also registers legend-related handlers (`legendselectchanged`,
`legendselectall`, `legendinverseselect`) at lines 281-289, indicating legend interactions
are part of this chart's behavior.
3. When the user clicks a legend item representing a metric (for example, legend entry
"Sales"), ECharts emits a `click` event for the legend component with a string `name` (the
series name) alongside legend-select events; this `click` is handled by the generic
`eventHandlers.click` registered in `Echart.tsx:90-93`. Because `hasDimensions` is false
and `canCrossFilterByXAxis` is true, the handler's timer branch in
`EchartsTimeseries.tsx:16-31` takes the `else if (canCrossFilterByXAxis)` path at line 21.
4. Inside that branch, `getCategoryAxisValue(props.data, props.name)` is called at lines
23-26; for a legend click, `props.data` is not an X-axis tuple (often undefined), while
`props.name` is the legend label "Sales". Since `Array.isArray(data)` is false,
`getCategoryAxisValue` falls through to the unscoped fallback at lines 237-238 (`if
(typeof name === 'string' || typeof name === 'number') { return name; }`), returning
`"Sales"` even though it is not an X-axis category value. The handler then calls
`handleXAxisChange(categoryAxisValue)` at line 27, which invokes
`setDataMask(getXAxisCrossFilterDataMask(xAxisValue).dataMask)` at lines 211-217, building
a filter on the X-axis column (`xAxis.label`) with value `"Sales"` via
`getXAxisCrossFilterDataMask` at lines 163-197. This produces an incorrect X-axis filter
derived from a legend interaction rather than a real category, so legend clicks can
unintentionally drive X-axis cross-filters with invalid values.
(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/Timeseries/EchartsTimeseries.tsx
**Line:** 237:238
**Comment:**
*Logic Error: Falling back to `name` without checking event source can convert non-category interactions (for example legend/component clicks that also carry a string `name`) into X-axis filters, producing incorrect filter values for the X-axis column. Only use `name` when the event is known to come from category-axis label clicks (or series points with verified category payload), otherwise ignore it.
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 fixThere was a problem hiding this comment.
i didint change anything on the file, yesterday you approved my commit now seeing new issue?
There was a problem hiding this comment.
I just changed the test file to make test build sucess
There was a problem hiding this comment.
Yes — this can happen even if you didn’t change that file.
What likely happened is:
- A new review pass ran after your commit was already approved.
- The reviewer/tool is now flagging a previously unnoticed edge case in the same logic.
- So it’s not necessarily a regression from your latest change; it may just be a newly detected issue in the existing implementation.
In this case, the concern is valid: the fallback to name can accidentally treat non-x-axis clicks like legend/component clicks as if they were category clicks, which could create wrong cross-filters.
A safer fix is to only accept name when the event is known to come from the category axis click path, and otherwise ignore it.
For example, make the fallback more restrictive:
const getCategoryAxisValue = useCallback(
(data: unknown, name: unknown) => {
if (Array.isArray(data)) {
const categoryAxisValue = data[categoryAxisValueIndex];
if (
typeof categoryAxisValue === 'string' ||
typeof categoryAxisValue === 'number'
) {
return categoryAxisValue;
}
}
return undefined;
},
[categoryAxisValueIndex],
);Since handleXAxisLabelClick already handles the axis-label click case using event.value, this removes the risky fallback while keeping the intended cross-filter behavior.
So in short: you didn’t necessarily “change” the issue; it was just newly surfaced by review.
If you want, I can also help apply the minimal fix for this comment and check the rest of the PR comments too.
| } else if (canCrossFilterByXAxis) { | ||
| // Cross-filter by X-axis value when no dimensions (issue #25334) | ||
| const categoryAxisValue = getCategoryAxisValue( | ||
| props.data, | ||
| props.name, | ||
| ); | ||
| if (categoryAxisValue !== undefined) { | ||
| handleXAxisChange(categoryAxisValue); | ||
| } |
There was a problem hiding this comment.
Suggestion: The generic click handler now triggers X-axis cross-filtering for any click event when there are no dimensions, including axis-label clicks that are also handled by queryEventHandlers. This causes duplicate setDataMask updates for a single user action and can trigger redundant dashboard re-filter/query cycles. Restrict this branch to series data clicks (or skip axis component clicks) so axis-label clicks are handled only once by the query-based handler. [performance]
Severity Level: Major ⚠️
- ❌ Duplicate cross-filter queries on X-axis label clicks.
- ⚠️ Increased dashboard latency from redundant filter updates.Steps of Reproduction ✅
1. Create or open an ECharts timeseries chart (plugin registered at
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/index.ts:34-43`) with no
`groupby` (so `hasDimensions` is false) and a categorical X-axis (`xAxis.type ===
AxisType.Category`), which satisfies `canCrossFilterByXAxis` in
`EchartsTimeseries.tsx:221-223`.
2. Observe that `transformProps` enables axis-level click events for this configuration by
setting `triggerEvent: true` when `xAxisType === AxisType.Category && groupBy.length ===
0` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:13-16`
(shown via `Read`), so X-axis labels emit `click` events.
3. When the chart renders, `Echart` registers both the generic click handler and the
query-based click handler: `chartRef.current?.on(name, handler)` for `eventHandlers` in
`superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:90-93`, and
`chartRef.current?.on(name, query, handler)` for `queryEventHandlers` in
`Echart.tsx:95-97`, using the handlers defined in `EchartsTimeseries.tsx`
(`eventHandlers.click` at lines 246-271 and `queryEventHandlers` at 385-393).
4. Clicking an X-axis category label (e.g., "Product A") causes ECharts to fire a `click`
event on the axis label, which matches both the generic `click` listener and the filtered
`click` listener with query `${categoryAxis}.category`. The generic handler in
`EchartsTimeseries.tsx:246-271` enters the `else if (canCrossFilterByXAxis)` branch at
line 260, computes a category value via `getCategoryAxisValue(props.data, props.name)` at
lines 262-265, and calls `handleXAxisChange(categoryAxisValue)` at line 267, which in turn
calls `setDataMask(getXAxisCrossFilterDataMask(xAxisValue).dataMask)` at lines 211-217.
Independently, the query-based handler `handleXAxisLabelClick` registered in
`queryEventHandlers` at lines 369-393 receives the same axis-label `click` as an
`ECElementEvent` and calls `handleXAxisChange(event.value)` at lines 370-377. As a result,
a single axis-label click triggers `handleXAxisChange` twice, and thus `setDataMask`
twice, causing duplicate cross-filter updates and redundant downstream queries.
(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/Timeseries/EchartsTimeseries.tsx
**Line:** 260:268
**Comment:**
*Performance: The generic `click` handler now triggers X-axis cross-filtering for any click event when there are no dimensions, including axis-label clicks that are also handled by `queryEventHandlers`. This causes duplicate `setDataMask` updates for a single user action and can trigger redundant dashboard re-filter/query cycles. Restrict this branch to series data clicks (or skip axis component clicks) so axis-label clicks are handled only once by the query-based handler.
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 fixThere was a problem hiding this comment.
i didint change anything on the file, yesterday you approved my commit now seeing new issue?
There was a problem hiding this comment.
I just changed the test file to make test build sucess
There was a problem hiding this comment.
Yes — that can happen even if you didn’t change the file.
Why you’re seeing a new issue now
- The code itself didn’t change, but the review context/tooling may have.
- This PR now registers both:
- the generic
clickhandler, and - the new
queryEventHandlersfor axis-label clicks.
- the generic
- Because of that, a single x-axis label click can hit both paths, which means
setDataMaskmay fire twice.
So was the comment correct?
Yes, the flagged issue is valid in the current diff.
Minimal fix
You should make sure the generic click handler only handles series/data clicks, and let axis-label clicks be handled only by the query-based handler.
For example, in EchartsTimeseries.tsx, tighten the generic branch so it skips axis label events:
click: props => {
if (!hasDimensions && !canCrossFilterByXAxis) {
return;
}
if (clickTimer.current) {
clearTimeout(clickTimer.current);
}
clickTimer.current = setTimeout(() => {
if (hasDimensions) {
const { seriesName: name } = props;
handleChange(name);
} else if (canCrossFilterByXAxis && props.componentType === 'series') {
const categoryAxisValue = getCategoryAxisValue(props.data, props.name);
if (categoryAxisValue !== undefined) {
handleXAxisChange(categoryAxisValue);
}
}
}, TIMER_DURATION);
},That keeps:
- series clicks → generic handler
- axis-label clicks → query handler only
Conclusion
So no, this doesn’t mean you changed something yesterday — it means the new cross-filter behavior exposed a duplicate path that the review caught now. If you want, I can also help check the rest of the PR comments and validate whether any others need fixes.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Avoid flagging production-performance or architecture issues in test files; scope these reviews to production code, not test-only changes.
Applied to:
**/test/****/tests/****/*test*.<ext>
💡 To manage or update this instruction, visit: CodeAnt AI Settings
|
LGTM, thanks for seeing this through! Happy it made it across the finish line :D |
|
Bito Automatic Review Skipped – PR Already Merged |
… and scatter charts (apache#41111) Co-authored-by: Abdullah Sahin <you@example.comclear>
sadpandajoe
removed
the
review:checkpoint
3 participants
Closes #25334