codeant-ai-for-open-source[bot] commented on code in PR #41907:
URL: https://github.com/apache/superset/pull/41907#discussion_r3551970854
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -244,6 +245,61 @@ export default function EchartsTimeseries({
const eventHandlers: EventHandlers = {
click: props => {
+ // Drill-down takes priority over cross-filter when a hierarchy is
configured.
+ // The DrillDownHost provides onDrillDown only when a hierarchy exists.
+ const hasDrillHierarchy = !!onDrillDown;
+
+ if (hasDrillHierarchy) {
+ if (clickTimer.current) {
+ clearTimeout(clickTimer.current);
+ }
+ clickTimer.current = setTimeout(() => {
+ const { seriesName: name } = props;
+ const drillFilters: BinaryQueryObjectFilterClause[] = [];
+ if (hasDimensions) {
+ const values = labelMap[name] ?? [];
+ groupby.forEach((dimension, i) => {
+ drillFilters.push({
+ col: dimension,
+ op: '==',
+ val: values[i],
+ formattedVal: String(values[i]),
+ });
+ });
+ // Cross-filter is emitted by the DrillDownHost with the full
+ // accumulated drill path; here we only report the click upward.
+ } else if (
+ xAxis.type === AxisType.Category &&
+ props.data?.[0] != null
+ ) {
+ // Bar chart with x_axis only (no groupby dimensions)
+ drillFilters.push({
+ col: xAxis.label,
+ op: '==',
+ val: props.data[0],
+ formattedVal: String(props.data[0]),
+ });
Review Comment:
**Suggestion:** The category drill branch hardcodes `props.data[0]`, but for
horizontal charts the category axis value is at a different index (already
handled elsewhere via `getCategoryAxisValue`). This causes drill-down to filter
on the measure instead of the category for horizontal bars. Reuse the
orientation-aware category extraction here. [incorrect variable usage]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Horizontal bar drill-down filters by metric, not category.
- ⚠️ Dashboard cross-filters inconsistent for horizontal bar charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a timeseries chart with no groupby (hasDimensions is false in
EchartsTimeseries.tsx:102) using a Bar series type configured with
orientation ===
OrientationType.Horizontal in formData (transformProps sets isHorizontal
from orientation
at
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:102
and
builds horizontal bar series accordingly).
2. Ensure the X-axis is categorical so xAxis.type === AxisType.Category in
EchartsTimeseries
(superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:223-224)
and configure a drilldown_hierarchy so DrillDownHost provides onDrillDown
(DrillDownHost.tsx:38-71) to the chart renderer and down to
EchartsTimeseries (props
onDrillDown at EchartsTimeseries.tsx:64).
3. In this horizontal bar configuration, ECharts data points are shaped such
that the
category label is at index 1; EchartsTimeseries already accounts for this via
categoryAxisValueIndex = formData.orientation === OrientationType.Horizontal
? 1 : 0 and
getCategoryAxisValue(data, name) using data[categoryAxisValueIndex]
(EchartsTimeseries.tsx:225-235), which is reused by cross-filter logic
elsewhere (click
cross-filter at 318-323 and context menu cross-filter at 410-415).
4. When you left-click a bar with a drilldown hierarchy active, the click
handler’s
drill-down branch for category axes (EchartsTimeseries.tsx:271-281) fires
and builds
drillFilters with val: props.data[0]; for horizontal bars props.data[0]
holds the metric
value, not the category, so onDrillDown receives filters scoped to the
measure rather than
the category, and DrillDownHost (DrillDownHost.tsx:42-63) emits a
cross-filter that
mis-scopes the drilled chart and other dashboard charts.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8de288e52932447182cd718e7e472c74&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=8de288e52932447182cd718e7e472c74&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:** 271:281
**Comment:**
*Incorrect Variable Usage: The category drill branch hardcodes
`props.data[0]`, but for horizontal charts the category axis value is at a
different index (already handled elsewhere via `getCategoryAxisValue`). This
causes drill-down to filter on the measure instead of the category for
horizontal bars. Reuse the orientation-aware category extraction here.
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=f90f4438f352d1802fa28f5a180dc2efd294c5fbcf16a8ed887e1fd221891456&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41907&comment_hash=f90f4438f352d1802fa28f5a180dc2efd294c5fbcf16a8ed887e1fd221891456&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -244,6 +245,61 @@ export default function EchartsTimeseries({
const eventHandlers: EventHandlers = {
click: props => {
+ // Drill-down takes priority over cross-filter when a hierarchy is
configured.
+ // The DrillDownHost provides onDrillDown only when a hierarchy exists.
+ const hasDrillHierarchy = !!onDrillDown;
+
+ if (hasDrillHierarchy) {
+ if (clickTimer.current) {
+ clearTimeout(clickTimer.current);
+ }
+ clickTimer.current = setTimeout(() => {
+ const { seriesName: name } = props;
+ const drillFilters: BinaryQueryObjectFilterClause[] = [];
+ if (hasDimensions) {
+ const values = labelMap[name] ?? [];
+ groupby.forEach((dimension, i) => {
+ drillFilters.push({
+ col: dimension,
+ op: '==',
+ val: values[i],
+ formattedVal: String(values[i]),
+ });
+ });
Review Comment:
**Suggestion:** The drill filter mapping uses `values[i]` directly, but
timeseries `labelMap` entries can include metric values before dimension values
(the same file already compensates with a metrics offset in cross-filter
logic). On multi-metric grouped charts this will drill on the wrong values and
produce incorrect filters. Compute the dimension index with the metrics offset
before reading from `values`. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Multi-metric timeseries drill-down filters on wrong values.
- ⚠️ Cross-filtered sibling charts show inconsistent scoped data.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a mixed timeseries chart with multiple metrics and at least one
groupby
dimension, then enable a drilldown hierarchy in the chart formData so
DrillDownHost wires
onDrillDown
(superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx:38-71)
into the plugin.
2. The Timeseries transform builds labelMap from the backend label_map for
each series
(superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:79-92),
where entries contain metric values followed by dimension values;
cross-filter logic
elsewhere already compensates by computing metricsCount = v.length -
groupby.length and
indexing dimensions at metricsCount + idx (getCrossFilterDataMask at
EchartsTimeseries.tsx:135-139, and drillByFilters in the context menu at
EchartsTimeseries.tsx:382-388).
3. On the dashboard, left-click a multi-metric grouped series to drill; the
click handler
in EchartsTimeseries
(superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:246-299)
sees hasDrillHierarchy and hasDimensions, reads const values =
labelMap[name] ?? [] and
then for each groupby dimension uses val: values[i] (lines 260-265), which
for
multi-metric series resolves to metric or time-offset entries instead of the
actual
dimension values.
4. The resulting drillFilters array is passed to onDrillDown
(EchartsTimeseries.tsx:293-297), and DrillDownHost converts each into
pathFilters and
emits a cross-filter (DrillDownHost.tsx:42-63) based on these wrong values,
causing the
drilled chart and other dashboard charts to be filtered on
metric/time-offset tokens
rather than the intended dimension values.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8a7a00f340cb4f5e877552cc1c4870a9&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=8a7a00f340cb4f5e877552cc1c4870a9&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:**
*Logic Error: The drill filter mapping uses `values[i]` directly, but
timeseries `labelMap` entries can include metric values before dimension values
(the same file already compensates with a metrics offset in cross-filter
logic). On multi-metric grouped charts this will drill on the wrong values and
produce incorrect filters. Compute the dimension index with the metrics offset
before reading from `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=c5d5754cf522c57e23f44ebc2992c7030d0ee5f2114a5955e65a9aa45077b84d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41907&comment_hash=c5d5754cf522c57e23f44ebc2992c7030d0ee5f2114a5955e65a9aa45077b84d&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -244,6 +245,61 @@ export default function EchartsTimeseries({
const eventHandlers: EventHandlers = {
click: props => {
+ // Drill-down takes priority over cross-filter when a hierarchy is
configured.
+ // The DrillDownHost provides onDrillDown only when a hierarchy exists.
+ const hasDrillHierarchy = !!onDrillDown;
+
+ if (hasDrillHierarchy) {
+ if (clickTimer.current) {
+ clearTimeout(clickTimer.current);
+ }
+ clickTimer.current = setTimeout(() => {
+ const { seriesName: name } = props;
+ const drillFilters: BinaryQueryObjectFilterClause[] = [];
+ if (hasDimensions) {
+ const values = labelMap[name] ?? [];
+ groupby.forEach((dimension, i) => {
+ drillFilters.push({
+ col: dimension,
+ op: '==',
+ val: values[i],
+ formattedVal: String(values[i]),
+ });
+ });
+ // Cross-filter is emitted by the DrillDownHost with the full
+ // accumulated drill path; here we only report the click upward.
+ } else if (
+ xAxis.type === AxisType.Category &&
+ props.data?.[0] != null
+ ) {
+ // Bar chart with x_axis only (no groupby dimensions)
+ drillFilters.push({
+ col: xAxis.label,
+ op: '==',
+ val: props.data[0],
+ formattedVal: String(props.data[0]),
+ });
+ // Cross-filter is emitted by the DrillDownHost with the full
+ // accumulated drill path; here we only report the click upward.
+ } else if (props.name) {
+ // Fallback: use the event name (typically the x-axis label)
+ drillFilters.push({
+ col: xAxis.label,
+ op: '==',
+ val: props.name,
+ formattedVal: String(props.name),
+ });
Review Comment:
**Suggestion:** The fallback condition checks `props.name` by truthiness, so
valid category names like `0` are treated as absent and no drill filter is
emitted. Use a null/undefined check instead of a truthy check so zero-like
labels still drill correctly. [falsy zero check]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Drill-down ignores clicks on zero-valued categories.
- ⚠️ Dashboard cross-filters inconsistent for numeric-zero labels.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a timeseries-based ECharts chart (Bar, Line, etc.) such that
the clicked
element’s category or label is the numeric value 0 (for example, a dimension
containing
values 0, 1, 2) and enable a drilldown hierarchy so DrillDownHost wires
onDrillDown into
the chart (DrillDownHost.tsx:38-71).
2. Ensure the click lands in the drill-down branch where the category-axis
shortcut does
not apply — e.g., either xAxis.type is not AxisType.Category or props.data
is unset for
the event — so the click handler in EchartsTimeseries falls through to the
fallback
condition
(superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:284-291).
3. For the data point whose label is 0, ECharts supplies props.name === 0;
the fallback
branch checks else if (props.name) (line 284), which uses a truthiness
check, so 0 is
treated as falsy and the drillFilters push logic is skipped entirely,
leaving drillFilters
empty for this valid label.
4. Because drillFilters.length === 0, the handler does not call onDrillDown
(the call at
EchartsTimeseries.tsx:293-297 is guarded), meaning DrillDownHost never sees
the drill
event for label 0, the breadcrumb stack is not updated, and no cross-filters
are emitted,
so clicks on zero-valued categories silently fail while other labels drill
correctly.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=19d6e423485e463ba6b70b8aeb1ada72&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=19d6e423485e463ba6b70b8aeb1ada72&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:** 284:291
**Comment:**
*Falsy Zero Check: The fallback condition checks `props.name` by
truthiness, so valid category names like `0` are treated as absent and no drill
filter is emitted. Use a null/undefined check instead of a truthy check so
zero-like labels still drill correctly.
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=43723b7a75c73317f73c89ac83c7cdf65377eaea78b554a31938e13d4cb0e5fd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41907&comment_hash=43723b7a75c73317f73c89ac83c7cdf65377eaea78b554a31938e13d4cb0e5fd&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]
