codeant-ai-for-open-source[bot] commented on code in PR #41111:
URL: https://github.com/apache/superset/pull/41111#discussion_r3465252082
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -234,12 +257,15 @@ export default function EchartsTimeseries({
// Cross-filter by dimension (original behavior)
const { seriesName: name } = props;
handleChange(name);
- } else if (canCrossFilterByXAxis && props.name != null) {
- // Cross-filter by X-axis value when no dimensions (issue #25334).
- // Use `name` (the category-axis value) instead of `data[0]`: for
- // horizontal bars the data tuple is value-first, so `data[0]` would
- // be the metric value rather than the category (issue #41102).
- handleXAxisChange(props.name);
+ } 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);
+ }
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Duplicate cross-filter queries on X-axis label clicks.
- ⚠️ Increased dashboard latency from redundant filter updates.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=953fd8d0d1e1407abaf609bf787120c7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=953fd8d0d1e1407abaf609bf787120c7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41111&comment_hash=1ff0bbb4e25054ff25d9c4359d8c0a00830eb5a9cd213efa5160dbdecf622691&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41111&comment_hash=1ff0bbb4e25054ff25d9c4359d8c0a00830eb5a9cd213efa5160dbdecf622691&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -218,6 +221,26 @@ export default function EchartsTimeseries({
// Determine if X-axis can be used for cross-filtering (categorical axis
without dimensions)
const canCrossFilterByXAxis =
!hasDimensions && xAxis.type === AxisType.Category;
+ const categoryAxisValueIndex =
+ formData.orientation === OrientationType.Horizontal ? 1 : 0;
+ const getCategoryAxisValue = useCallback(
+ (data: unknown, name: unknown) => {
+ if (Array.isArray(data)) {
+ const categoryAxisValue = data[categoryAxisValueIndex];
+ if (
+ typeof categoryAxisValue === 'string' ||
+ typeof categoryAxisValue === 'number'
+ ) {
+ return categoryAxisValue;
+ }
+ }
+ if (typeof name === 'string' || typeof name === 'number') {
+ return name;
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Legend clicks can apply incorrect X-axis filters.
- ⚠️ Users see unexpected filters after legend toggles.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2e406ac63a44a1b8aabf0bebf09b5bf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d2e406ac63a44a1b8aabf0bebf09b5bf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41111&comment_hash=2cb1e0728f94cbf0a0a3a202695f090cdbe755d413a02bfa8ecb63739b78e0da&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41111&comment_hash=2cb1e0728f94cbf0a0a3a202695f090cdbe755d413a02bfa8ecb63739b78e0da&reaction=dislike'>👎</a>
--
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]
