codeant-ai-for-open-source[bot] commented on code in PR #40449:
URL: https://github.com/apache/superset/pull/40449#discussion_r3305734363
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/mixins.tsx:
##########
@@ -40,7 +40,7 @@ const getAxisLabel = (
export const xAxisMixin = {
label: (state: ControlPanelState) => getAxisLabel(state?.form_data).label,
- multi: false,
+ multi: true,
Review Comment:
**Suggestion:** Making `x_axis` multi-select at the shared mixin level
affects every chart using this control, but several existing consumers still
treat `x_axis` as a single value (for example scalar equality checks and
single-column type checks). This creates inconsistent behavior in control logic
and drill option filtering when multiple x-axis columns are selected. Restrict
multi-select to drill-down-capable charts (or migrate all scalar consumers
first). [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Advanced analytics controls hidden for multi-level x-axis charts.
- ⚠️ X-axis sort options miscomputed when x_axis holds arrays.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Explore view for any chart that uses the shared `x_axis`
control, e.g. a
bar/timeseries chart whose control panel wiring maps `x_axis` to
`dndXAxisControl`
(`superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx:509`).
2. Note that `dndXAxisControl` spreads `xAxisMixin`
(`superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx:34-37`),
and `xAxisMixin` now sets `multi: true` (`mixins.tsx:41-58`), so
`controls.x_axis.value`
can become an array when multiple x-axis columns are selected.
3. With such a chart, select multiple x-axis columns (e.g. a temporal column
plus a
categorical column) so that `controls.x_axis.value` is an array, then observe
`displayTimeRelatedControls` in `displayTimeRelatedControls.ts:10-20` which
reads `const
xAxisValue = xAxis?.value;` and passes it to
`isAdhocColumn`/`isPhysicalColumn` assuming a
single `QueryFormColumn`; for an array both checks fail and the function
returns `false`,
hiding advanced analytics/forecast sections whose `visibility` is
`displayTimeRelatedControls` (`advancedAnalytics.tsx:34`,
`forecastInterval.tsx:36`).
4. In the same multi-x-axis configuration, observe that other control
utilities also
assume a scalar x-axis value: `isSortable` in `isSortable.ts:8-21` and
`xAxisSortControl`
in `customControls.tsx:24-47` both cast `controls?.x_axis?.value as
QueryFormColumn` and
feed it into `getColumnLabel`/`checkColumnType`; when `value` is an array
these casts
either miscompute options or break, so time-related visibility and sort
controls behave
inconsistently whenever multiple x-axis columns are selected.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=98761c0a05a94a25b3a23e5481e5fe7b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=98761c0a05a94a25b3a23e5481e5fe7b&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/packages/superset-ui-chart-controls/src/shared-controls/mixins.tsx
**Line:** 43:43
**Comment:**
*Api Mismatch: Making `x_axis` multi-select at the shared mixin level
affects every chart using this control, but several existing consumers still
treat `x_axis` as a single value (for example scalar equality checks and
single-column type checks). This creates inconsistent behavior in control logic
and drill option filtering when multiple x-axis columns are selected. Restrict
multi-select to drill-down-capable charts (or migrate all scalar consumers
first).
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%2F40449&comment_hash=06e728382d652278546a7acf3b0da6b980bad79de635a34ac6d59f8682434d43&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40449&comment_hash=06e728382d652278546a7acf3b0da6b980bad79de635a34ac6d59f8682434d43&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts:
##########
@@ -335,6 +335,7 @@ export default function transformProps(
groupby,
selectedValues,
onContextMenu,
+ onDrillDown,
Review Comment:
**Suggestion:** Adding `onDrillDown` to transformed props switches click
handling to the drill-down branch in `allEventHandlers`, which skips
`setDataMask` updates. As a result, clicking a funnel segment no longer emits
cross-filters to other dashboard charts. Keep drill-down, but also trigger the
cross-filter data mask on click so dashboard-level filtering still works.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Funnel clicks stop cross-filtering other dashboard charts.
- ⚠️ Drill-down breadcrumbs and dashboard filters become inconsistent.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a funnel chart via `EchartsFunnel`
(`superset-frontend/plugins/plugin-chart-echarts/src/Funnel/EchartsFunnel.tsx:5-20`),
which calls `allEventHandlers(props)` (`utils/eventHandlers.ts:152-217`) and
passes the
resulting `eventHandlers` into the shared `Echart` component; `props` here
are the
transformed props produced by `Funnel/transformProps.ts`.
2. In `transformProps` for Funnel
(`superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts:88-102,
155-167, 68-82`), the hooks bag is destructured as `const { setDataMask = ()
=> {},
onContextMenu, onDrillDown } = hooks;` and the returned object includes
`setDataMask`,
`emitCrossFilters`, `labelMap`, `groupby`, `selectedValues`,
`onContextMenu`, and
`onDrillDown` (line 79).
3. `allEventHandlers` (`utils/eventHandlers.ts:152-205`) reads `onDrillDown`
from the
transformed props (line 164), computes `const hasDrillHierarchy =
!!onDrillDown;` (line
170), and if `hasDrillHierarchy && groupby.length > 0` builds
`drillDownClickHandler`
(lines 172-193) that assembles `drillFilters` and calls
`onDrillDown(drillFilters, label)`
but never invokes `setDataMask`.
4. In the same function, the `click` handler is set to
`drillDownClickHandler` when
present, otherwise to `clickEventHandler(...)` (lines 197-205), where
`clickEventHandler`
(`eventHandlers.ts:90-106`) is the only code path that calls `setDataMask`
and emits
cross-filters. When DrillDownHost wires `onDrillDown` into the hooks via
`ChartRenderer`
(`ChartRenderer.tsx:16-22` and `DrillDownHost.tsx:23-30`),
`hasDrillHierarchy` becomes
true, `click` is bound to `drillDownClickHandler`, and clicking a funnel
segment no longer
updates the data mask, so no cross-filter is propagated to other charts on
the dashboard.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=94b1445887104027aa36358f5c48a3ad&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=94b1445887104027aa36358f5c48a3ad&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/Funnel/transformProps.ts
**Line:** 338:338
**Comment:**
*Incomplete Implementation: Adding `onDrillDown` to transformed props
switches click handling to the drill-down branch in `allEventHandlers`, which
skips `setDataMask` updates. As a result, clicking a funnel segment no longer
emits cross-filters to other dashboard charts. Keep drill-down, but also
trigger the cross-filter data mask on click so dashboard-level filtering still
works.
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%2F40449&comment_hash=cea0a1532e7cd8098c4124b5232dde879537dde55a7dc30afd9f1b32ff53f6e0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40449&comment_hash=cea0a1532e7cd8098c4124b5232dde879537dde55a7dc30afd9f1b32ff53f6e0&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/query/getXAxis.ts:
##########
@@ -25,8 +25,13 @@ import {
Optional,
} from '@superset-ui/core';
-export const isXAxisSet = (formData: QueryFormData) =>
- isQueryFormColumn(formData.x_axis);
+export const isXAxisSet = (formData: QueryFormData) => {
+ const xAxis = formData.x_axis;
+ if (Array.isArray(xAxis)) {
+ return xAxis.length > 0 && isQueryFormColumn(xAxis[0]);
Review Comment:
**Suggestion:** This now treats array `x_axis` as set, but downstream query
normalization still assumes `formData.x_axis` is a scalar column. In
`normalizeTimeColumn`, comparisons against `formData.x_axis` fail when it is an
array, so base-axis time metadata is not applied. Normalize by unwrapping the
first x-axis column before downstream consumers use it. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Base-axis time metadata missing for multi-level x_axis.
- ⚠️ Time normalization skipped, affecting advanced analytics pipelines.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Build a chart that uses the shared `x_axis` control, then enable a drill
hierarchy so
`formData.x_axis` becomes an array (multi-column) via the `xAxisMixin`
multi-select
(`mixins.tsx:41-58`) and the DnD wiring in `sharedControls.tsx:509`.
2. When this chart executes, `buildQueryContext`
(`superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts:35-61`)
constructs base query objects and checks `if (isXAxisSet(formData))` at line
59; the new
`isXAxisSet` implementation in `getXAxis.ts:28-34` treats an array `x_axis`
as "set" by
checking only `xAxis[0]`.
3. Because `isXAxisSet(formData)` returns `true` for array-valued `x_axis`,
`buildQueryContext` maps each query through `normalizeTimeColumn(formData,
query)`
(`buildQueryContext.ts:59-61`); inside `normalizeTimeColumn`
(`normalizeTimeColumn.ts:40-50`), `axisIdx` is computed by comparing each
`queryObject.columns` entry against `formData.x_axis`, but these comparisons
call
`isPhysicalColumn(formData.x_axis)`/`isAdhocColumn(formData.x_axis)`
assuming a scalar
`QueryFormColumn`, so when `formData.x_axis` is an array `axisIdx` never
matches.
4. With `axisIdx` remaining `-1`, the normalization branch at
`normalizeTimeColumn.ts:51-77` is skipped and the function returns the
original
`queryObject` (lines 80-81), meaning no column is annotated with
`columnType: 'BASE_AXIS'`
or `timeGrain`; downstream consumers that rely on base-axis metadata (e.g.
post-processing
that inspects `getXAxisLabel(formData)` in `sortOperator.ts:5-9`) lose this
information
whenever `formData.x_axis` is an array rather than a single column.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=953d4f8bb5ba41e79d53eae8bed2cf4d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=953d4f8bb5ba41e79d53eae8bed2cf4d&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/packages/superset-ui-core/src/query/getXAxis.ts
**Line:** 30:31
**Comment:**
*Api Mismatch: This now treats array `x_axis` as set, but downstream
query normalization still assumes `formData.x_axis` is a scalar column. In
`normalizeTimeColumn`, comparisons against `formData.x_axis` fail when it is an
array, so base-axis time metadata is not applied. Normalize by unwrapping the
first x-axis column before downstream consumers use 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%2F40449&comment_hash=2d5f6d071d8f1f3a72a02b8028361d0330811ba9dad46698671b5014c3a088be&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40449&comment_hash=2d5f6d071d8f1f3a72a02b8028361d0330811ba9dad46698671b5014c3a088be&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -495,6 +495,7 @@ export default function transformProps(
groupby,
selectedValues,
onContextMenu,
+ onDrillDown,
Review Comment:
**Suggestion:** Passing `onDrillDown` into the pie transformed props makes
`allEventHandlers` use the drill-down click path, which currently bypasses
cross-filter updates (`setDataMask`). This causes pie clicks to stop
propagating cross-filters to other charts when hierarchy is enabled. Emit
cross-filter data alongside drill-down to preserve expected dashboard
interactions. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Pie clicks no longer cross-filter linked dashboard charts.
- ⚠️ Drill-down pies show stale filters on sibling charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a pie chart via `EchartsPie`
(`superset-frontend/plugins/plugin-chart-echarts/src/Pie/EchartsPie.tsx:5-20`),
which
calls `allEventHandlers(props)` and passes the resulting handlers into
`Echart`; as with
Funnel, `props` here are produced by `Pie/transformProps.ts`.
2. In `transformProps` for Pie
(`superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:124-138,
246-260, 488-502`), the hooks bag is destructured as `const { setDataMask =
() => {},
onContextMenu, onDrillDown } = hooks;` (line 259), and the returned
transformed props
include `setDataMask`, `labelMap`, `groupby`, `selectedValues`,
`onContextMenu`,
`onDrillDown`, `refs`, `emitCrossFilters`, and `coltypeMapping` (lines
488-502).
3. `allEventHandlers` in `utils/eventHandlers.ts:152-205` reads
`onDrillDown` (line 164)
and, when it is defined and `groupby.length > 0`, constructs
`drillDownClickHandler`
(lines 172-193) that computes drill filters and calls `onDrillDown` but does
not call
`setDataMask`; it then assigns `click` to `drillDownClickHandler`, falling
back to
`clickEventHandler(...)` only when no drill hierarchy is present (lines
197-205).
4. When DrillDownHost provides `onDrillDown` via the hooks getter in
`ChartRenderer`
(`ChartRenderer.tsx:16-22`, `DrillDownHost.tsx:23-30`), pie charts with a
configured drill
hierarchy end up using `drillDownClickHandler` for left-clicks, so clicks no
longer
trigger `clickEventHandler` (`eventHandlers.ts:90-106`) and thus stop
emitting
cross-filter data masks; as a result, pie segment clicks drill into the next
level but do
not filter other dashboard charts.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bb4432d5657946efb30e8d1fef4e8a6a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bb4432d5657946efb30e8d1fef4e8a6a&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/Pie/transformProps.ts
**Line:** 498:498
**Comment:**
*Incomplete Implementation: Passing `onDrillDown` into the pie
transformed props makes `allEventHandlers` use the drill-down click path, which
currently bypasses cross-filter updates (`setDataMask`). This causes pie clicks
to stop propagating cross-filters to other charts when hierarchy is enabled.
Emit cross-filter data alongside drill-down to preserve expected dashboard
interactions.
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%2F40449&comment_hash=69cbcad93bb54d50c9aae08f3a75f4df80d481433c0a4951082d362c33f324c8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40449&comment_hash=69cbcad93bb54d50c9aae08f3a75f4df80d481433c0a4951082d362c33f324c8&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]
