codeant-ai-for-open-source[bot] commented on code in PR #41111:
URL: https://github.com/apache/superset/pull/41111#discussion_r3465269043
##########
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:
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:
```ts
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.
##########
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:
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 `click` handler, and
- the new `queryEventHandlers` for axis-label clicks.
- Because of that, a **single x-axis label click** can hit **both paths**,
which means `setDataMask` may 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:
```typescript
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.
--
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]
