codeant-ai-for-open-source[bot] commented on code in PR #41907:
URL: https://github.com/apache/superset/pull/41907#discussion_r3559090590
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts:
##########
@@ -164,16 +164,52 @@ export const allEventHandlers = (
selectedValues,
coltypeMapping,
formData,
+ onDrillDown,
} = transformedProps;
+
+ // When a drill-down hierarchy is configured, left-click drills instead of
+ // emitting a cross-filter. The DrillDownHost provides onDrillDown only when
+ // a hierarchy exists.
+ const hasDrillHierarchy = !!onDrillDown;
+
+ const drillDownClickHandler =
+ hasDrillHierarchy && groupby.length > 0
+ ? (e: { name: string }) => {
+ const values = labelMap[e.name];
+ if (!values) return;
+ const drillFilters: BinaryQueryObjectFilterClause[] = [];
+ groupby.forEach((dimension, i) => {
+ drillFilters.push({
+ col: dimension,
+ op: '==',
+ val: values[i],
+ formattedVal: formatSeriesName(values[i], {
+ timeFormatter: getTimeFormatter(formData.dateFormat),
+ numberFormatter: getNumberFormatter(formData.numberFormat),
+ coltype: coltypeMapping?.[getColumnLabel(dimension)],
+ }),
+ });
+ });
Review Comment:
**Suggestion:** The drill-down click path builds filter values with
`values[i]`, but `labelMap` entries can include leading metric/series fields
(the regular cross-filter path already compensates for this with an offset).
This can bind each `groupby` column to the wrong value and emit incorrect drill
filters. Reuse the same offset logic used in `getCrossFilterDataMask` (or a
shared helper) when mapping `groupby` dimensions to `labelMap` values.
[incorrect variable usage]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Drill-down filters bind dimensions to incorrect values.
⚠️ Timeseries drill-down cross-filters other charts inaccurately.
⚠️ Dashboard drill-down exploration yields confusing, inconsistent results.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run a multi-metric ECharts timeseries chart with at least one groupby
dimension so the
backend builds a composite label_map entry (see
`superset/common/query_context_processor.py:149-167` where `label_map`
values come from
splitting flattened column labels that include metrics first, then groupby
columns).
2. On the frontend, observe that
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:280-288`
converts `label_map` into `labelMap`, preserving the array order from the
backend so
`labelMap[seriesKey]` still starts with non-groupby (metric/time-compare)
tokens followed
by groupby values.
3. When drill-down hooks are wired, `Timeseries/transformProps.ts:1236-1243`
passes
`labelMap`, `groupby`, `selectedValues`, and `onDrillDown` into the chart
props, and the
chart uses `allEventHandlers` from
`superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts:155-103`
to
create the click handler.
4. In `getCrossFilterDataMask` at `eventHandlers.ts:42-51`, the regular
cross-filter path
explicitly compensates for prefixed non-groupby fields by computing
`metricsCount =
v.length - groupby.length` and indexing `v[metricsCount + idx]`, proving
that `labelMap`
entries may have leading metric/series fields that must be skipped when
mapping to
`groupby`.
5. However, the drill-down click handler added in this PR at
`eventHandlers.ts:56-73` uses
`const values = labelMap[e.name];` and then `groupby.forEach((dimension, i)
=> { val:
values[i], ... })` (lines 181-192 in the PR hunk) without applying the
`metricsCount`
offset, so for composite labels `values[0]` is a metric/series token rather
than the first
groupby value.
6. Add a drill-down hierarchy to the timeseries chart and click a series
point whose
`labelMap` entry includes a leading metric/series token; the handler will
construct
`drillFilters` with each `groupby` column bound to `values[i]` (wrongly
including
metric/series tokens), and `onDrillDown` will receive misaligned filters and
label,
causing the DrillDownHost to emit an incorrect cross-filter path for other
dashboard
charts.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c22205ec869b4d6ebcf4507575f7f092&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=c22205ec869b4d6ebcf4507575f7f092&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/utils/eventHandlers.ts
**Line:** 181:192
**Comment:**
*Incorrect Variable Usage: The drill-down click path builds filter
values with `values[i]`, but `labelMap` entries can include leading
metric/series fields (the regular cross-filter path already compensates for
this with an offset). This can bind each `groupby` column to the wrong value
and emit incorrect drill filters. Reuse the same offset logic used in
`getCrossFilterDataMask` (or a shared helper) when mapping `groupby` dimensions
to `labelMap` 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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41907&comment_hash=7bd984302c80ef9f9eca1fd3c4d7ea4eee3d953c8d30b6b5c9923bbf487d178d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41907&comment_hash=7bd984302c80ef9f9eca1fd3c4d7ea4eee3d953c8d30b6b5c9923bbf487d178d&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]
