New feature tk drilldown hierarchy#40449
Open
tomerkl65 wants to merge 17 commits into
Open
Conversation
dosubot
Bot
added
change:frontend
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+120
to
+133
| // Going to an intermediate level — set cross-filter to that level's filter | ||
| const targetLevel = drillStack[depth - 1]; | ||
| if (targetLevel) { | ||
| const filters = targetLevel.filters.map(f => ({ | ||
| col: f.col, | ||
| op: 'IN' as const, | ||
| val: [f.val] as (string | number | boolean)[], | ||
| })); | ||
| rendererProps.actions.updateDataMask(rendererProps.chartId, { | ||
| extraFormData: { filters }, | ||
| filterState: { | ||
| value: filters.length ? [targetLevel.label] : null, | ||
| selectedValues: filters.length ? [targetLevel.label] : null, | ||
| }, |
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Breadcrumb jump-to-depth updates the dashboard data mask using only the target level's filters (e.g., just the region), while the drilled chart's own queries use the accumulated filters from all ancestor levels (e.g., country + region). After multi-level drilling, this causes other charts' cross-filters to diverge from the drilled chart's actual state when navigating via the breadcrumb.
Suggestion: When handling breadcrumb navigation, rebuild the data mask filters from all DrillDownLevel entries up to the selected depth (mirroring effectiveFormData's accumulatedFilters), and set filterState to represent the full path, so the dashboard-wide cross-filter matches the drilled chart's query filters.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx
**Line:** 120:133
**Comment:**
*HIGH: Breadcrumb jump-to-depth updates the dashboard data mask using only the target level's filters (e.g., just the region), while the drilled chart's own queries use the accumulated filters from all ancestor levels (e.g., country + region). After multi-level drilling, this causes other charts' cross-filters to diverge from the drilled chart's actual state when navigating via the breadcrumb.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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
Contributor
|
The PR comments file contains only the header row and no actual comments. There are no actionable suggestions or review comments to analyze in this PR. |
| groupby, | ||
| selectedValues, | ||
| onContextMenu, | ||
| onDrillDown, |
Contributor
There was a problem hiding this comment.
Suggestion: Forwarding onDrillDown here switches BoxPlot clicks into the drill-down branch in allEventHandlers, which currently skips setDataMask updates. That means cross-filter is no longer emitted when drill hierarchy is enabled, so linked dashboard charts stop updating. Update the drill-down click path to also emit cross-filter (as done in Timeseries) before wiring this prop through. [logic error]
Severity Level: Major ⚠️
- ❌ BoxPlot drill-down stops emitting dashboard cross-filters.
- ⚠️ Linked charts remain stale when clicking BoxPlot points.Steps of Reproduction ✅
1. Configure BoxPlot transform props to expose `onDrillDown` by returning it from
`transformProps` at
`superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts:312-324`,
where the object includes `setDataMask`, `emitCrossFilters`, `labelMap`, `groupby`,
`selectedValues`, `onContextMenu`, and `onDrillDown` (line 323).
2. Observe that `EchartsBoxPlot` at
`superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/EchartsBoxPlot.tsx:23-28`
calls `allEventHandlers(props)` and passes the transformed props including `onDrillDown`,
`setDataMask`, `emitCrossFilters`, `groupby`, `labelMap`, and `selectedValues` into the
generic event handler factory.
3. Follow the click handling logic in `allEventHandlers` at
`superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts:152-215`, where
`onDrillDown` is read (lines 155-165), `hasDrillHierarchy` is computed as `!!onDrillDown`
(line 170), and when `hasDrillHierarchy && groupby.length > 0`, `eventHandlers.click` is
set to `drillDownClickHandler` (lines 172-195, 197-200) instead of `clickEventHandler`.
4. Note that `drillDownClickHandler` builds `drillFilters` and calls
`onDrillDown(drillFilters, label)` (lines 175-193) but never calls `clickEventHandler` or
`setDataMask`, while `clickEventHandler` (lines 90-106) is the only place that computes a
cross-filter data mask via `getCrossFilterDataMask(...)(name)` and calls `setDataMask`.
Therefore, once a drill hierarchy is configured and `onDrillDown` is non-null, BoxPlot
clicks invoke drill-down only and no longer emit cross-filters, leaving linked dashboard
charts unfiltered.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts
**Line:** 323:323
**Comment:**
*Logic Error: Forwarding `onDrillDown` here switches BoxPlot clicks into the drill-down branch in `allEventHandlers`, which currently skips `setDataMask` updates. That means cross-filter is no longer emitted when drill hierarchy is enabled, so linked dashboard charts stop updating. Update the drill-down click path to also emit cross-filter (as done in Timeseries) before wiring this prop through.
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| groupby, | ||
| selectedValues: filterState.selectedValues || [], | ||
| onContextMenu, | ||
| onDrillDown, |
Contributor
There was a problem hiding this comment.
Suggestion: Passing onDrillDown into Gauge transformed props activates the drill-only click handler path in allEventHandlers, which does not apply a data mask. As a result, clicks drill but do not broadcast cross-filters to other charts when hierarchy is configured. Add cross-filter emission in the drill-down branch before exposing this hook. [logic error]
Severity Level: Major ⚠️
- ❌ Gauge drill-down no longer updates dashboard cross-filters.
- ⚠️ Other charts ignore Gauge click interactions under drill.Steps of Reproduction ✅
1. In Gauge transform props, observe that hooks are destructured to include `onDrillDown`
at `superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts:229`
(`const { setDataMask = () => {}, onContextMenu, onDrillDown } = hooks;`) and that
`onDrillDown` is returned to the renderer at lines 369-383, specifically line 380
(`onDrillDown,`).
2. See that `EchartsGauge` at
`superset-frontend/plugins/plugin-chart-echarts/src/Gauge/EchartsGauge.tsx:23-27`
constructs `eventHandlers = allEventHandlers(props);`, passing `setDataMask`,
`emitCrossFilters`, `groupby`, `labelMap`, `selectedValues`, `formData`, and the newly
wired `onDrillDown` into `allEventHandlers`.
3. Inspect `allEventHandlers` in
`superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts:152-215`, where
`onDrillDown` is read from `transformedProps` (lines 155-165), `hasDrillHierarchy` is set
based on `!!onDrillDown` (line 170), and `drillDownClickHandler` is created when
`hasDrillHierarchy && groupby.length > 0` (lines 172-195). In this case,
`eventHandlers.click` is set to `drillDownClickHandler` (lines 197-200).
4. Compare with `clickEventHandler` at lines 90-106 in the same file: it uses
`getCrossFilterDataMask(selectedValues, groupby, labelMap)` and calls `setDataMask` when
`emitCrossFilters` is true, emitting cross-filters. Because wiring `onDrillDown` for Gauge
causes `allEventHandlers` to choose `drillDownClickHandler` (which only calls
`onDrillDown` and never `setDataMask`) instead of `clickEventHandler`, Gauge clicks in a
dashboard with drill hierarchy configured will drill but no longer update the data mask,
so cross-filters are not propagated to other charts.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts
**Line:** 380:380
**Comment:**
*Logic Error: Passing `onDrillDown` into Gauge transformed props activates the drill-only click handler path in `allEventHandlers`, which does not apply a data mask. As a result, clicks drill but do not broadcast cross-filters to other charts when hierarchy is configured. Add cross-filter emission in the drill-down branch before exposing this hook.
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| groupby, | ||
| selectedValues, | ||
| onContextMenu, | ||
| onDrillDown, |
Contributor
There was a problem hiding this comment.
Suggestion: This new onDrillDown passthrough makes Radar use the drill-down click path, but that path currently omits cross-filter updates. In dashboards with drill hierarchy enabled, Radar clicks will drill without filtering sibling charts. Align the drill-down handler with Timeseries behavior by emitting cross-filter and drill-down together. [logic error]
Severity Level: Major ⚠️
- ❌ Radar drill-down stops broadcasting cross-filters to dashboards.
- ⚠️ Radar clicks no longer synchronize with linked charts.Steps of Reproduction ✅
1. In Radar transform props, verify that hooks are destructured to include `onDrillDown`
at `superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts:129`
(`const { setDataMask = () => {}, onContextMenu, onDrillDown } = hooks;`) and that
`onDrillDown` is propagated in the returned props at lines 406-420, specifically line 417
(`onDrillDown,`).
2. Confirm that `EchartsRadar` at
`superset-frontend/plugins/plugin-chart-echarts/src/Radar/EchartsRadar.tsx:23-27` calls
`const eventHandlers = allEventHandlers(props);`, which passes Radar's `groupby`,
`labelMap`, `selectedValues`, `setDataMask`, `emitCrossFilters`, `formData`,
`coltypeMapping`, and now `onDrillDown` into the shared `allEventHandlers` function.
3. Inspect `allEventHandlers` in
`superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts:152-215`: it
reads `onDrillDown` (lines 155-165), sets `hasDrillHierarchy = !!onDrillDown` (line 170),
and when `hasDrillHierarchy && groupby.length > 0` constructs `drillDownClickHandler` that
builds `drillFilters` and calls `onDrillDown(drillFilters, label)` (lines 172-193).
4. Note that in this branch, `eventHandlers.click` is assigned to `drillDownClickHandler`
(lines 197-200) and never calls `clickEventHandler`, while `clickEventHandler` (lines
90-106) is the only function that computes a cross-filter `dataMask` and calls
`setDataMask`. Thus, for Radar charts with a configured drill hierarchy (where
`onDrillDown` is provided by the host), clicks now trigger `onDrillDown` only and stop
emitting cross-filters, so sibling charts on the dashboard no longer filter in response to
Radar clicks.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts
**Line:** 417:417
**Comment:**
*Logic Error: This new `onDrillDown` passthrough makes Radar use the drill-down click path, but that path currently omits cross-filter updates. In dashboards with drill hierarchy enabled, Radar clicks will drill without filtering sibling charts. Align the drill-down handler with Timeseries behavior by emitting cross-filter and drill-down together.
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| groupby, | ||
| selectedValues: filterState.selectedValues || [], | ||
| onContextMenu, | ||
| onDrillDown, |
Contributor
There was a problem hiding this comment.
Suggestion: onDrillDown is now included in transformed props, but EchartsSunburst does not read or invoke it in its click handlers. This leaves Sunburst drill-down non-functional despite the new wiring, so the feature appears enabled in data flow but never executes. Wire onDrillDown into Sunburst click handling and build drill filters from the clicked tree path. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Sunburst drill-down never triggers despite onDrillDown wiring.
- ⚠️ Users get cross-filters only, no Sunburst hierarchy drill.Steps of Reproduction ✅
1. In Sunburst transform props, verify that `hooks` are destructured to include
`onDrillDown` at
`superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts:242`
(`const { setDataMask = () => {}, onContextMenu, onDrillDown } = hooks;`) and that
`onDrillDown` is returned to the renderer at lines 396-410, specifically line 407
(`onDrillDown,`).
2. Inspect `EchartsSunburst` at
`superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx:36-50`:
the component destructures `height`, `width`, `echartOptions`, `setDataMask`,
`selectedValues`, `formData`, `onContextMenu`, `refs`, `emitCrossFilters`, and
`coltypeMapping` from `props`, but does not read or destructure `onDrillDown` anywhere, so
the drill-down hook returned from `transformProps` is effectively ignored.
3. Examine Sunburst's click handling in the same file: `getCrossFilterDataMask` (lines
51-93) constructs a data mask from `treePathInfo`, and `handleChange` (lines 97-106) calls
`setDataMask(getCrossFilterDataMask(treePathInfo).dataMask)` when `emitCrossFilters` is
true. The `eventHandlers.click` implementation (lines 108-112) calls
`handleChange(treePathInfo)` but never calls `onDrillDown`, so clicks only update
cross-filters and never invoke the drill-down hook.
4. As a result, when a drill hierarchy is configured and the host provides `onDrillDown`
for Sunburst (demonstrated by transform props returning it at line 407), clicking on
Sunburst segments will continue to perform cross-filtering via `setDataMask` but will
never call `onDrillDown`, meaning the new drill-down feature (drill state and breadcrumb
updates managed by `DrillDownHost`) remains non-functional for Sunburst despite being
wired through transform props.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts
**Line:** 407:407
**Comment:**
*Incomplete Implementation: `onDrillDown` is now included in transformed props, but `EchartsSunburst` does not read or invoke it in its click handlers. This leaves Sunburst drill-down non-functional despite the new wiring, so the feature appears enabled in data flow but never executes. Wire `onDrillDown` into Sunburst click handling and build drill filters from the clicked tree path.
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
| groupby, | ||
| selectedValues: filterState.selectedValues || [], | ||
| onContextMenu, | ||
| onDrillDown, |
Contributor
There was a problem hiding this comment.
Suggestion: This only forwards onDrillDown in transformed props, but Treemap click handling still does not invoke it, so Treemap never actually drills down despite being advertised as supported. Wire onDrillDown into Treemap click events (similar to other plugins) so user clicks can trigger hierarchy navigation. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Treemap charts never initiate drill-down on click.
- ⚠️ Treemap cannot participate in drill hierarchies.Steps of Reproduction ✅
1. Chart hooks, including `onDrillDown`, are exposed by `ChartRenderer` via its `hooks`
bag at `superset-frontend/src/components/Chart/ChartRenderer.tsx:265-271`, and
`DrillDownHost` only supplies a non-undefined `onDrillDown` when a hierarchy is configured
(see `superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx:82-86`).
2. For Treemap, `transformProps` at
`superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:107-129`
destructures `hooks` as `const { setDataMask = () => {}, onContextMenu, onDrillDown } =
hooks;` and forwards `onDrillDown` in the returned props object at
`transformProps.ts:309-323`, specifically `onDrillDown,` on line 320 in the new hunk.
3. The Treemap renderer `EchartsTreemap` at
`superset-frontend/plugins/plugin-chart-echarts/src/Treemap/EchartsTreemap.tsx:34-47`
defines its parameter destructuring as `{ echartOptions, emitCrossFilters, groupby,
height, labelMap, onContextMenu, refs, setDataMask, selectedValues, width, formData,
coltypeMapping }` (no `onDrillDown`), and its `eventHandlers` at
`EchartsTreemap.tsx:112-156` only call `handleChange` to emit cross-filters and
`onContextMenu` for context menus; there is no reference to `onDrillDown`.
4. When a Treemap chart is used in a dashboard with a drilldown hierarchy configured (so
`onDrillDown` is provided through hooks) and the user clicks a node, the click handler at
`EchartsTreemap.tsx:112-116` runs `handleChange` and updates the data mask, but because
`onDrillDown` is never plumbed into `EchartsTreemap` or invoked from its `eventHandlers`,
no drill-down navigation occurs and the DrillDownHost state never changes, meaning Treemap
cannot participate in the advertised hierarchical drilldown feature.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts
**Line:** 320:320
**Comment:**
*Incomplete Implementation: This only forwards `onDrillDown` in transformed props, but Treemap click handling still does not invoke it, so Treemap never actually drills down despite being advertised as supported. Wire `onDrillDown` into Treemap click events (similar to other plugins) so user clicks can trigger hierarchy navigation.
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
| const eventHandlers: EventHandlers = { | ||
| click: | ||
| groupby.length > 0 | ||
| click: drillDownClickHandler |
Contributor
There was a problem hiding this comment.
Suggestion: When onDrillDown is present, the click path bypasses clickEventHandler entirely, so no cross-filter data mask is emitted for Pie/Funnel/Gauge/Radar/BoxPlot clicks. This breaks dashboard synchronization during drill-down. Emit setDataMask(getCrossFilterDataMask(...).dataMask) inside the drill-down click handler (guarded by emitCrossFilters) before/alongside calling onDrillDown. [logic error]
Severity Level: Critical 🚨
- ❌ Pie/Funnel/Gauge/Radar/BoxPlot clicks stop cross-filtering.
- ⚠️ Dashboards lose synchronized filtering during drill-down.Steps of Reproduction ✅
1. Non-timeseries ECharts plugins like Pie, Funnel, Gauge, Radar, and BoxPlot all use the
shared `allEventHandlers` helper: for example `EchartsPie` at
`superset-frontend/plugins/plugin-chart-echarts/src/Pie/EchartsPie.tsx:23-28` computes
`const eventHandlers = allEventHandlers(props);` and passes them to `<Echart>`, as do
`EchartsFunnel`, `EchartsGauge`, `EchartsRadar`, and `EchartsBoxPlot`.
2. Each plugin's `transformProps` forwards drilldown and cross-filter hooks: e.g. Pie's
`transformProps` at
`superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:124-139,259-243`
destructures `const { setDataMask = () => {}, onContextMenu, onDrillDown } = hooks;` and
returns `setDataMask`, `emitCrossFilters`, `labelMap`, `groupby`, `selectedValues`, and
`onDrillDown` in the transformed props (lines 229-243).
3. The shared `allEventHandlers` implementation at
`superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts:152-195` reads
`onDrillDown` and computes `hasDrillHierarchy = !!onDrillDown` at line 170; when
`hasDrillHierarchy && groupby.length > 0`, it defines `drillDownClickHandler` that builds
`drillFilters` from `groupby`/`labelMap` and calls `onDrillDown(drillFilters, label)`
(lines 172-193), but does not call `getCrossFilterDataMask` or `setDataMask`.
4. Because `eventHandlers.click` is defined at `eventHandlers.ts:197-206` as `click:
drillDownClickHandler ? drillDownClickHandler : groupby.length > 0 ?
clickEventHandler(...) : () => {};`, whenever a drill hierarchy is configured (so
`drillDownClickHandler` is truthy), clicks on Pie/Funnel/Gauge/Radar/BoxPlot charts go
exclusively through `drillDownClickHandler`; the `clickEventHandler` branch that emits
cross-filter data masks is bypassed, so `setDataMask` is never invoked on click and other
dashboard charts no longer receive cross-filters during drill-down, contradicting the
intended "emit cross-filter alongside drill-down" behavior described in the PR.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts
**Line:** 198:206
**Comment:**
*Logic Error: When `onDrillDown` is present, the click path bypasses `clickEventHandler` entirely, so no cross-filter data mask is emitted for Pie/Funnel/Gauge/Radar/BoxPlot clicks. This breaks dashboard synchronization during drill-down. Emit `setDataMask(getCrossFilterDataMask(...).dataMask)` inside the drill-down click handler (guarded by `emitCrossFilters`) before/alongside calling `onDrillDown`.
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
Comment on lines
+30
to
+31
| if (Array.isArray(xAxis)) { | ||
| return xAxis.length > 0 && isQueryFormColumn(xAxis[0]); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Base-axis time metadata missing for multi-level x_axis.
- ⚠️ Time normalization skipped, affecting advanced analytics pipelines.Steps of Reproduction ✅
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.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| groupby, | ||
| selectedValues, | ||
| onContextMenu, | ||
| onDrillDown, |
Contributor
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ Funnel clicks stop cross-filtering other dashboard charts.
- ⚠️ Drill-down breadcrumbs and dashboard filters become inconsistent.Steps of Reproduction ✅
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.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| groupby, | ||
| selectedValues, | ||
| onContextMenu, | ||
| onDrillDown, |
Contributor
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ Pie clicks no longer cross-filter linked dashboard charts.
- ⚠️ Drill-down pies show stale filters on sibling charts.Steps of Reproduction ✅
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.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #7ccb16
Actionable Suggestions - 4
-
superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx - 1
- CWE-20: Missing keyboard handler · Line 75-84
-
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts - 1
- onDrillDown not consumed in component · Line 128-128
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx - 1
- Missing drill-down tests · Line 226-300
-
superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts - 1
- Missing translation function wrapper · Line 221-221
Additional Suggestions - 2
-
superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts - 1
-
Missing unit tests for complex hook · Line 1-278This hook has complex business logic (hierarchy derivation, filter accumulation, async request handling) but lacks unit tests. Following the project's testing strategy of preferring unit tests would improve reliability and catch regressions.
-
-
superset-frontend/packages/superset-ui-core/src/query/getXAxis.ts - 1
-
Semantic duplication in array handling · Line 29-33Both isXAxisSet (lines 29-33) and getXAxisColumn (lines 45-50) contain identical array handling patterns. Consider extracting to a shared helper like `getFirstAxisColumn(xAxis)` to reduce duplication and maintenance risk.
-
Review Details
-
Files reviewed - 20 · Commit Range:
7c26b45..94ebe3d- superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/mixins.tsx
- superset-frontend/packages/superset-ui-core/src/chart/types/Base.ts
- superset-frontend/packages/superset-ui-core/src/query/getXAxis.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts
- superset-frontend/src/components/Chart/Chart.tsx
- superset-frontend/src/components/Chart/ChartRenderer.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx
- superset-frontend/src/components/Chart/DrillDown/types.ts
- superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Comment on lines
+75
to
+84
| role="button" | ||
| tabIndex={0} | ||
| onClick={handleClick(0)} | ||
| css={css` | ||
| cursor: pointer; | ||
| color: ${theme.colorPrimary}; | ||
| `} | ||
| > | ||
| {hierarchy[0] ?? ''} | ||
| </span> |
Contributor
There was a problem hiding this comment.
CWE-20: Missing keyboard handler
The breadcrumb segments have role="button" and tabIndex={0} but lack onKeyDown handlers, making them inaccessible via keyboard navigation. Users relying on keyboard cannot activate these interactive elements. (See also: CWE-20)
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx
+++ b/superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx
@@ -74,7 +74,10 @@ export function DrillDownBreadcrumb({
<span
role="button"
tabIndex={0}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') onJumpTo(0);
+ }}
onClick={handleClick(0)}
css={css`
cursor: pointer;
@@ -93,6 +96,9 @@ export function DrillDownBreadcrumb({
<span
role="button"
tabIndex={0}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') onJumpTo(index + 1);
+ }}
onClick={handleClick(index + 1)}
css={css`
cursor: pointer;
color: ${theme.colorPrimary};
`}
>
Code Review Run #7ccb16
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| currencyCodeColumn, | ||
| } = datasource; | ||
| const { setDataMask = () => {}, onContextMenu } = hooks; | ||
| const { setDataMask = () => {}, onContextMenu, onDrillDown } = hooks; |
Contributor
There was a problem hiding this comment.
onDrillDown not consumed in component
The onDrillDown hook is extracted at line 128 and passed in the return object at line 320, but EchartsTreemap.tsx does not receive or consume this prop. The click event handler only emits cross-filters via handleChange — it never calls onDrillDown. This breaks drill-down functionality for the Treemap chart even when drilldown_hierarchy is configured.
Code Review Run #7ccb16
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
+226
to
+300
| 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]), | ||
| }); | ||
| }); | ||
| // Also emit cross-filter so other charts in the dashboard filter too | ||
| if (emitCrossFilters) { | ||
| // Use direct setDataMask (not handleChange which toggles) | ||
| setDataMask({ | ||
| extraFormData: { | ||
| filters: groupby.map((col, idx) => ({ | ||
| col, | ||
| op: 'IN' as const, | ||
| val: [values[idx]] as (string | number | boolean)[], | ||
| })), | ||
| }, | ||
| filterState: { | ||
| value: [values], | ||
| selectedValues: [name], | ||
| }, | ||
| }); | ||
| } | ||
| } 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]), | ||
| }); | ||
| // Also emit cross-filter by x-axis value | ||
| if (emitCrossFilters) { | ||
| setDataMask({ | ||
| extraFormData: { | ||
| filters: [{ | ||
| col: xAxis.label, | ||
| op: 'IN' as const, | ||
| val: [props.data[0]] as (string | number | boolean)[], | ||
| }], | ||
| }, | ||
| filterState: { | ||
| value: [String(props.data[0])], | ||
| selectedValues: [String(props.data[0])], | ||
| }, | ||
| }); | ||
| } | ||
| } 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), | ||
| }); | ||
| } | ||
| if (drillFilters.length > 0) { | ||
| const label = drillFilters | ||
| .map(f => f.formattedVal ?? String(f.val)) | ||
| .join(', '); | ||
| onDrillDown(drillFilters, label); | ||
| } | ||
| }, TIMER_DURATION); | ||
| return; |
Contributor
There was a problem hiding this comment.
Missing drill-down tests
The new onDrillDown click handler (lines 226-300) with multiple branching logic and timer-based execution has no corresponding tests. Per [6262], tests must verify actual business logic — this click handler's complex branching (hasDimensions → categorical X-axis → props.name fallback) with cross-filter side effects requires test coverage to prevent regressions.
Code Review Run #7ccb16
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| }) | ||
| .catch(err => { | ||
| if (!cancelled) { | ||
| setError(err?.message || 'Failed to load drill-down data'); |
Contributor
There was a problem hiding this comment.
Missing translation function wrapper
User-facing error message should use translation function t('Failed to load drill-down data') for i18n support, consistent with DrillByModal.tsx:105 pattern in this same codebase.
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts
+++ b/superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts
@@ -17,6 +17,7 @@
* under the License.
*/
import { useCallback, useEffect, useMemo, useState } from 'react';
+import { t } from '@apache-superset/core/translation';
import {
BinaryQueryObjectFilterClause,
ensureIsArray,
@@ -218,7 +219,7 @@ export function useDrillDownState({
setError(err?.message || 'Failed to load drill-down data');
}
})
- .finally(() => {
+ .finally(() => {
if (!cancelled) {
setIsLoading(false);
}
});
@@ -233,7 +234,7 @@ export function useDrillDownState({
setError(err?.message || 'Failed to load drill-down data');
},
.catch(err => {
- if (!cancelled) {
- setError(err?.message || 'Failed to load drill-down data');
+ if (!cancelled) {
+ setError(err?.message || t('Failed to load drill-down data'));
}
})
.finally(() => {
Code Review Run #7ccb16
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
…d keyboard a11y, i18n error, remove incomplete Treemap/Sunburst wiring
Contributor
Code Review Agent Run #572c17Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Adds hierarchical drill-down support to ECharts-based chart plugins. Users can define a drill-down hierarchy by adding multiple columns to the X-Axis control. The first column is displayed initially; clicking a data point drills into the next level, scoped to the clicked value. A breadcrumb above the chart allows navigating back up.
How it works:
multi: trueinxAxisMixin)getXAxisColumnandisXAxisSetare updated to handle array x_axis (using the first entry for the query)DrillDownHostwrapper component manages the drill state and re-fetches data for each levelEchartsTimeseries,allEventHandlers) detect the hierarchy and callonDrillDowninstead of emitting a cross-filterDrillDownBreadcrumbcomponent renders above the chart showing the navigation pathSupported charts: Bar, Pie, Funnel, Gauge, Radar, BoxPlot, Treemap, Sunburst (all charts declaring
Behavior.DrillBy)BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: X-Axis accepts a single column. No drill-down on click.
After: X-Axis accepts multiple columns. Clicking a bar drills to the next level with a breadcrumb for navigation.
TESTING INSTRUCTIONS
country,region)country › USA)ADDITIONAL INFORMATION