feat(explore): drill-down hierarchy for ECharts charts#41907
Open
tomerkl65 wants to merge 13 commits into
Open
feat(explore): drill-down hierarchy for ECharts charts#41907tomerkl65 wants to merge 13 commits into
tomerkl65 wants to merge 13 commits into
Conversation
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Author
|
This supersedes #40449, which I had to close and recreate — the branch was @michael-s-molina @villebro @EnxDev — following up on the earlier review, this
|
tomerkl65
force-pushed
the
feat/echarts-drilldown-hierarchy
branch
from
July 9, 2026 13:39
beb03aa to
6f70126
Compare
![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 #6392bc
Actionable Suggestions - 3
-
superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts - 2
- No test coverage for retry logic · Line 344-465
- isLoading stuck true on unmount · Line 344-465
-
superset-frontend/src/components/Chart/chartReducers.test.ts - 1
- Incomplete test assertions · Line 85-85
Additional Suggestions - 8
-
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx - 1
-
Schema property missing from type · Line 60-60The Gauge chart's `EchartsGaugeFormData` type does not declare `drilldown_hierarchy` even though this control is now exposed. Other charts in this plugin (Pie, BoxPlot, Funnel, Radar, BoxPlot) all declare it explicitly in their types. Add the property to maintain schema consistency across the plugin.
-
-
superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts - 1
-
No timeout on async query wait · Line 363-363There is no timeout on the waitForAsyncData promise returned by handleChartDataResponse at line 363. If the async job never completes (e.g., WebSocket disconnects without ERROR event, or polling falls behind), the promise hangs indefinitely with no retry trigger. This blocks the drill-down UI with a permanently stuck loading spinner.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts - 1
-
Missing test coverage for onDrillDown · Line 60-60The `onDrillDown` hook is correctly destructured from `hooks` (line 60) and passed to the return object (line 338), following the same pattern as `onContextMenu`. However, the test file (`test/BoxPlot/transformProps.test.ts`) does not assert either hook in the return, so the new passthrough is unverified. Per [6262] (BITO.md adaptive rule), tests should validate behavior they describe. Add an assertion like `expect.objectContaining({ onDrillDown: expect.any(Function) })` to the existing transformProps test or a dedicated hook-passthrough test.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx - 2
-
Inconsistent formattedVal in drill-down handler · Line 265-265The `formattedVal` on line 265 uses `String(values[i])`, but the identical cross-filter path in the same file already uses `formatSeriesName` (line 394) which handles time-column formatting, number formatting, and null values. Inconsistent `formattedVal` will cause the drill-down breadcrumb label to display raw unformatted values (e.g. raw timestamps or unformatted decimals) instead of user-friendly labels.
-
Inconsistent formattedVal in bar-chart and fallback branches · Line 264-264The second drill-down branch (bar chart with category X-axis, line 271–283) and the fallback branch (line 284–291) also use `String(...)` for `formattedVal`. All three branches should use `formatSeriesName` for consistency with the cross-filter path (line 394) and with the parallel `eventHandlers.ts` drill-down handler (lines 186–189).
-
-
superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx - 1
-
Missing control panel tests · Line 114-114This diff adds the `drilldownHierarchySection` control to the Radar chart but lacks corresponding test coverage. Other eCharts plugins in the same repository (Timeseries/Line, Timeseries/Bar, Timeseries/Area, etc.) have `controlPanel.test.ts` files that verify their control panel configurations. Per BITO.md adaptive rule 11730, new features should include comprehensive unit tests covering success paths and edge cases.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx - 1
-
Missing test coverage for drilldown · Line 65-65The test file at test/Timeseries/Area/controlPanel.test.ts does not verify the newly added `drilldownHierarchySection`. Add assertions to confirm the section is present in controlPanelSections and the drilldown_hierarchy control is correctly configured with expected defaults.
-
-
superset-frontend/src/components/Chart/ChartRenderer.tsx - 1
-
Unused triggerQuery in ChartActions · Line 96-96`triggerQuery` is declared in the `ChartActions` interface but never destructured or used within `ChartRenderer`. DrillDownHost passes it via `overlayProps.actions` and calls it directly — the declaration in `ChartActions` is dead code for this file.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/components/Chart/DrillDown/useDrillDownState.test.ts - 1
- Incorrect assertion pattern in test · Line 440-444
-
superset-frontend/src/components/Chart/ChartRenderer.tsx - 1
- Duplicate onDrillDown declaration · Line 148-151
-
superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx - 1
- Hardcoded non-translated separator · Line 101-101
Review Details
-
Files reviewed - 34 · Commit Range:
beb03aa..beb03aa- superset-frontend/packages/superset-ui-chart-controls/src/sections/drilldownHierarchy.tsx
- superset-frontend/packages/superset-ui-chart-controls/src/sections/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/controlPanel.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Step/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.test.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.test.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx
- superset-frontend/src/components/Chart/DrillDown/types.ts
- superset-frontend/src/components/Chart/DrillDown/useDrillDownState.test.ts
- superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts
- superset-frontend/src/components/Chart/chartReducer.ts
- superset-frontend/src/components/Chart/chartReducers.test.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
Contributor
EnxDev's Review Agent — #41907 · HEAD 6f70126request changes — the drill path is wrong for x-axis charts that also have a Dimension: it drills on the series dimension, not the x-axis column the breadcrumb claims. Context: reviewed against the diff only (no linked issue/SIP). The three earlier bot findings on 🔴 Functional
🟡 Should-fix
🔵 Nits
🙌 Praise
|
tomerkl65
force-pushed
the
feat/echarts-drilldown-hierarchy
branch
3 times, most recently
from
July 9, 2026 15:48
967b575 to
d77f08d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41907 +/- ##
==========================================
+ Coverage 65.03% 65.08% +0.04%
==========================================
Files 2744 2749 +5
Lines 153627 153930 +303
Branches 35226 35323 +97
==========================================
+ Hits 99915 100180 +265
- Misses 51805 51843 +38
Partials 1907 1907
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tomerkl65
force-pushed
the
feat/echarts-drilldown-hierarchy
branch
from
July 9, 2026 16:41
d77f08d to
a10f64b
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #93499c
Actionable Suggestions - 3
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx - 1
- Missing drilldown test coverage · Line 62-62
-
superset-frontend/packages/superset-ui-chart-controls/src/sections/drilldownHierarchy.tsx - 1
- Missing unit test coverage · Line 1-69
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts - 1
- Missing test coverage for onDrillDown · Line 744-744
Additional Suggestions - 4
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.test.ts - 2
-
Weak label assertion in drill-down test · Line 72-96The test currently asserts the label argument using `expect.any(String)`, which only checks type. Please replace this with an explicit value assertion (e.g., `'USA'`) and extend the test to validate the filters array’s `formattedVal` field, not just `col` and `op`.
-
Missing test for empty-groupby guard · Line 47-70The combination `onDrillDown != null` with `groupby.length === 0` is not covered by existing tests. Currently all drill-down tests use `groupby: ['country']`, so the guard on line 176 is exercised only in the truthy branch. Adding a test for the empty-groupby fallback ensures the nullish-coalescing fallback on line 205 is exercised and does not regress.
-
-
superset-frontend/src/components/Chart/DrillDown/useDrillDownState.test.ts - 2
-
Inconsistent retry attempt count · Line 591-591The comment incorrectly states '2 retries' but `MAX_ATTEMPTS = 3` means 3 total attempts (attempt 1, attempt 2, attempt 3). Update comment to accurately reflect 3 total attempts.
-
Unverifiable assertion in test · Line 632-636The test comment at line 632-634 claims 'should not leave isLoading stuck' but the test provides no assertion verifying this. After unmount, `result.current` is invalid and `isLoading` cannot be queried. The test only verifies that the cleanup function doesn't throw, not that `isLoading` actually resets to false.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.test.ts - 1
- Unused refs property in fixture · Line 34-34
Review Details
-
Files reviewed - 34 · Commit Range:
d77f08d..d77f08d- superset-frontend/packages/superset-ui-chart-controls/src/sections/drilldownHierarchy.tsx
- superset-frontend/packages/superset-ui-chart-controls/src/sections/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/controlPanel.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Step/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.test.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.test.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx
- superset-frontend/src/components/Chart/DrillDown/types.ts
- superset-frontend/src/components/Chart/DrillDown/useDrillDownState.test.ts
- superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts
- superset-frontend/src/components/Chart/chartReducer.ts
- superset-frontend/src/components/Chart/chartReducers.test.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
tomerkl65
force-pushed
the
feat/echarts-drilldown-hierarchy
branch
2 times, most recently
from
July 9, 2026 17:37
0ef7d19 to
0ed9d9f
Compare
tomerkl65
requested review from
betodealmeida,
dpgaspar,
eschutho,
geido,
kgabryje,
mistercrunch,
nytai,
rusackas,
sadpandajoe,
sha174n and
villebro
as code owners
July 9, 2026 17:37
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Code Review Agent Run #647224
Actionable Suggestions - 1
-
superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx - 1
- Inconsistent test utility imports · Line 19-27
Additional Suggestions - 5
-
superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts - 1
-
CWE-20: Leaf Filters Persist After Navigation · Line 323-323Leaf filters are applied to `effectiveFormData.adhoc_filters` even when `selectedLeaf` is undefined (e.g., after `resetTo()` or when navigating back). The `selectedLeafFilters` state is only cleared in `resetTo`, but the filter accumulation in `effectiveFormData` reads it regardless of whether `selectedLeaf` is set. This could cause stale leaf filters to persist in `effectiveFormData` across navigation levels until the chart is reconfigured. ([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
-
-
superset-frontend/src/components/Chart/ChartRenderer.tsx - 1
-
Unused interface prop · Line 97-97The `triggerQuery?: (value: boolean, key: number | string) => void` prop added at line 97 is defined in `ChartActions` but never destructured or passed to `SuperChart`'s hooks bag (lines 398-426) in this diff. This creates a contract inconsistency where the interface promises functionality that isn't connected.
-
-
superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx - 1
-
Missing i18n for breadcrumb separator · Line 101-101The breadcrumb separator character `›` is user-facing text rendered directly in the UI. Per [6516], all such labels must be wrapped with the translation function (`t()`) to support internationalization. Import `t` from `@apache-superset/core` and change lines 101 and 118 to use `t('›')` instead of the literal character.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts - 1
-
Missing drill-down test coverage · Line 155-155The Funnel plugin lacks test coverage for the onDrillDown integration. Given BITO.md rule 6262 (assert behavior logic in tests, not just rendering), adding tests that verify onDrillDown is correctly passed through and handled would prevent regressions.
-
-
superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx - 1
-
Duplicate inline mock component · Line 188-195Duplicate function definitions: `CaptureRenderer` is defined inline at lines 188-195 and again at lines 235-242 with identical implementation. Extract to module-level constant to reduce duplication and improve maintainability.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/components/Chart/DrillDown/useDrillDownState.test.ts - 1
- Inverted assertion logic in resetTo test · Line 467-471
Review Details
-
Files reviewed - 34 · Commit Range:
2a07da6..2a07da6- superset-frontend/packages/superset-ui-chart-controls/src/sections/drilldownHierarchy.tsx
- superset-frontend/packages/superset-ui-chart-controls/src/sections/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Step/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Gauge/transformProps.test.ts
- superset-frontend/src/components/Chart/Chart.tsx
- superset-frontend/src/components/Chart/ChartRenderer.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.test.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx
- superset-frontend/src/components/Chart/DrillDown/types.ts
- superset-frontend/src/components/Chart/DrillDown/useDrillDownState.test.ts
- superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts
- superset-frontend/src/components/Chart/chartReducer.ts
- superset-frontend/src/components/Chart/chartReducers.test.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
Add a hierarchical drill-down interaction for ECharts charts. The chart author defines an ordered list of columns in a new **Drill-down hierarchy** control (`drilldown_hierarchy`). On a dashboard, left-clicking a data point drills to the next level scoped to the clicked value, shows a breadcrumb for navigating back up, and emits a cross-filter for the full drill path so other charts stay in sync. At the deepest level a click narrows the chart to the selected value. Supported charts: Bar, Line, Area, Scatter, Smooth Line, Step (x-axis based) and Pie, Funnel, Gauge, Radar, Box Plot (groupby based).
- Use spec/helpers/testing-library in DrillDownHost test and dedupe CaptureRenderer - Guard leaf filters so they never persist after navigating back (resetTo) - Wrap breadcrumb separator in t() for i18n - Add Funnel onDrillDown pass-through test coverage
tomerkl65
force-pushed
the
feat/echarts-drilldown-hierarchy
branch
from
July 10, 2026 12:55
2a07da6 to
e342acd
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
- Skip leading metric/series tokens in labelMap when mapping groupby columns in the drill-down click handler (matches getCrossFilterDataMask), fixing misaligned filters on multi-metric charts - Add drilldownHierarchySection to the Box Plot control panel so its onDrillDown plumbing is reachable - Add regression test for the multi-metric labelMap offset
…ps tests - Cast inline-literal ChartProps through unknown (matches Timeseries/Waterfall test convention) to resolve TS2352 in Funnel and Gauge drill-down tests - Apply prettier formatting to Gauge test queriesData
Contributor
There was a problem hiding this comment.
Code Review Agent Run #3f0632
Actionable Suggestions - 2
-
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts - 1
- Missing test for onDrillDown · Line 434-434
-
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts - 1
- Missing test coverage for onDrillDown · Line 259-259
Additional Suggestions - 3
-
superset-frontend/plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx - 1
-
CWE-20: Missing type for drilldown_hierarchy · Line 70-70The `drilldownHierarchySection` exposes a `drilldown_hierarchy` control (string array, default `[]`), but `EchartsPieFormData` in `types.ts` has no corresponding property. TypeScript will not recognize the runtime value. Add the field to the type definition to match other plugin types (e.g., `GaugeChartTransformedProps`).
-
-
superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx - 1
-
Duplicate filter-building logic · Line 183-189The filter-object construction inside `handleResetTo` (lines 183-189) duplicates the logic in `onDrillDown` (lines 105-121) — both build `IN`-op filter objects from `drillStack` entries. Duplication risks divergence if the filter schema is ever extended.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx - 1
-
Missing unit test for drilldown · Line 114-114The new `sections.drilldownHierarchySection` is added to the Radar chart's control panel (line 114) but lacks corresponding test coverage. Other charts in this plugin (Scatter, SmoothLine, Bar, Area, Line, Step) have controlPanel.test.ts files, but Radar only has transformProps.test.ts and utils.test.ts. Per adaptive rule [6262], tests should verify actual business logic — the drilldown behavior requires a unit test.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/components/Chart/DrillDown/useDrillDownState.test.ts - 1
- Retry test under-advances timers · Line 595-601
Review Details
-
Files reviewed - 36 · Commit Range:
54aa3b2..26ccc78- superset-frontend/packages/superset-ui-chart-controls/src/sections/drilldownHierarchy.tsx
- superset-frontend/packages/superset-ui-chart-controls/src/sections/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/controlPanel.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Step/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Funnel/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Gauge/transformProps.test.ts
- superset-frontend/src/components/Chart/Chart.tsx
- superset-frontend/src/components/Chart/ChartRenderer.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.test.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx
- superset-frontend/src/components/Chart/DrillDown/types.ts
- superset-frontend/src/components/Chart/DrillDown/useDrillDownState.test.ts
- superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts
- superset-frontend/src/components/Chart/chartReducer.ts
- superset-frontend/src/components/Chart/chartReducers.test.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
…ter builder - Add onDrillDown pass-through tests for Pie and Radar transformProps - Add Radar controlPanel test asserting the drilldown_hierarchy control - Extract shared toCrossFilterClauses helper in DrillDownHost to remove duplicated filter-building logic between onDrillDown and handleResetTo
Contributor
Code Review Agent Run #41bcb4Actionable 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 |
Contributor
EnxDev's Review Agent — #41907 · HEAD db42179comment — supersedes my prior Verified against the code, not the "fixed" claims:
🟡 Should-fix
🔵 Nits
🙌 Praise
|
- Use formatSeriesName for the timeseries drill breadcrumb/cross-filter label (parity with the groupby drill path) instead of raw String() - Add onDrillDown pass-through test for BoxPlot transformProps
- Replace the hand-rolled div/span/button breadcrumb with the antd Breadcrumb from @superset-ui/core/components (mirrors DrillByModal) - Keep keyboard a11y via role/tabIndex + Enter/Space on navigable segments - Update test to assert role=button/tabindex instead of native button tag
- Extract the timeseries drill-filter building into a pure buildTimeseriesDrillFilters helper (and shared getCategoryAxisValue) so the click behavior is unit-testable - Add tests: category-series click builds a filter keyed to the x-axis column; non-series clicks (e.g. componentType 'xAxis') do not drill; horizontal orientation reads the correct tuple index; time axis drills on the event name; labels formatted via formatSeriesName - EchartsTimeseries now delegates to the helper (no behavior change)
…flag - Add DRILL_DOWN feature flag (default off / dark launch) to config.py and the frontend FeatureFlag enum; sync docs/static/feature-flags.json - Gate the DrillDownHost runtime and the drilldown_hierarchy control visibility on the flag - Enable the flag in DrillDownHost tests and add a flag-off gating test
…ill by / Drill to detail Add a 'Drilling into data' section clarifying the three independent, flag-gated drill interactions (DRILL_TO_DETAIL, DRILL_BY, DRILL_DOWN) and how they differ.
Contributor
|
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). 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 |
Contributor
EnxDev's Review Agent — #41907 · HEAD 60f31c0comment — supersedes my prior comment (db42179). Every prior should-fix is addressed: the timeseries drill handler was extracted to Verified against the code, not the "fixed" claims:
The open bot findings are stale: codeant's four "logic-error" flags (metrics offset, horizontal orientation, falsy-zero, 🟡 Should-fix
🔵 Nits
🙌 Praise
|
| const config: ControlPanelConfig = { | ||
| controlPanelSections: [ | ||
| sections.echartsTimeSeriesQueryWithXAxisSort, | ||
| sections.drilldownHierarchySection, |
Contributor
There was a problem hiding this comment.
Suggestion: Adding this section unconditionally exposes an empty “Drill-down hierarchy” panel when the DrillDown feature flag is off, because only the inner control has visibility gating. Gate the section itself (or conditionally add it) so users don’t see a blank section in disabled environments. [logic error]
Severity Level: Major ⚠️
⚠️ Explore Bar chart shows empty Drill-down hierarchy section.
⚠️ Users see feature label without any usable controls.
⚠️ Same empty section on Area, Step, Pie charts.Steps of Reproduction ✅
1. Disable the DRILL_DOWN feature flag as shown in
`superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx:163-166`, where
`global.featureFlags[FeatureFlag.DrillDown]` is set to `false`.
2. Open Explore for a Bar chart, which uses the control panel config in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx:15-22`;
note `controlPanelSections` includes `sections.drilldownHierarchySection` at line 317
unconditionally.
3. The section `drilldownHierarchySection` is defined in
`superset-frontend/packages/superset-ui-chart-controls/src/sections/drilldownHierarchy.tsx:2-34`
with label `t('Drill-down hierarchy')` and a single control `drilldown_hierarchy` whose
`config.visibility` is `() => isFeatureEnabled(FeatureFlag.DrillDown)` at line 29, so this
control is hidden when the flag is disabled.
4. In Explore, sections are rendered by `renderControlPanelSection` in
`superset-frontend/src/explore/components/ControlPanelsContainer.tsx:593-597` and 629-135;
section visibility uses only `section.visibility` (`isVisible = visibility?.call(...) !==
false` at line 14), which is undefined for `drilldownHierarchySection`, so the section
remains visible with `style: { display: isVisible ? 'block' : 'none' }` at line 135, while
its only row is dropped because `renderedControls.length === 0` at lines 104-107. The
result is a visible "Drill-down hierarchy" header with no controls whenever the DRILL_DOWN
feature flag is off.
(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/Timeseries/Regular/Bar/controlPanel.tsx
**Line:** 317:317
**Comment:**
*Logic Error: Adding this section unconditionally exposes an empty “Drill-down hierarchy” panel when the `DrillDown` feature flag is off, because only the inner control has `visibility` gating. Gate the section itself (or conditionally add it) so users don’t see a blank section in disabled environments.
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
+256
to
+266
| const drillFilters = buildTimeseriesDrillFilters({ | ||
| componentType: props.componentType, | ||
| data: props.data, | ||
| name: props.name, | ||
| xAxisType: xAxis.type, | ||
| xAxisLabel: xAxis.label, | ||
| orientation: formData.orientation, | ||
| dateFormat: formData.dateFormat, | ||
| numberFormat: formData.numberFormat, | ||
| coltype: coltypeMapping?.[xAxis.label], | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: For time-axis clicks, this forwards props.name into drill filter construction, but ECharts click name is often the formatted axis label while the raw filterable value is in the data tuple (the same file already uses data[0] for time drill-to-detail). This can generate drill filters with display strings instead of raw temporal values and return empty/incorrect drilled results. Use the raw axis value from the clicked data point for non-category axes when building drill filters. [logic error]
Severity Level: Critical 🚨
- ❌ Timeseries drill-down on time axes returns empty results.
- ❌ Dashboard cross-filters from drill path mis-scope linked charts.
- ⚠️ Breadcrumb labels no longer reflect actual filtered timestamps.
- ⚠️ Users experience inconsistent behavior between context menu drill modes.Steps of Reproduction ✅
1. Render a Timeseries chart with a time x-axis on a dashboard so that the React component
`EchartsTimeseries` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:48-69`
is used with `xAxis.type === AxisType.Time` and `onDrillDown` provided by the
DrillDownHost (drill-down hierarchy configured in the chart controls).
2. Click a data point in the timeseries; the ECharts click event is handled by
`eventHandlers.click` defined in `EchartsTimeseries.tsx:241-264`, which, when
`hasDrillHierarchy` is true, calls `buildTimeseriesDrillFilters` with `data: props.data`
and `name: props.name` (see lines 256-266 where the drill filter params are constructed).
3. In
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/drillFilters.ts:85-135`,
`buildTimeseriesDrillFilters` handles non-category axes (time axis) via the branch
starting at line 122: it ignores `data` and builds the filter from `name` (`val: name as
string | number`, `formattedVal: format(name as string | number)` on lines 124-131), while
the same Timeseries file uses `data[0]` as the raw timestamp for drill-to-detail in the
context menu handler (`EchartsTimeseries.tsx:75-86` where `val: data[0]` and
`formattedVal: xValueFormatter(data[0])` are used when `xAxis.type === AxisType.Time`).
4. On time axes, the ECharts click event typically carries the raw timestamp in the series
data tuple (`props.data[0]`) and a formatted axis label in `props.name`; because
`buildTimeseriesDrillFilters` uses `name` instead of the raw axis value, the emitted drill
filters (filtering `col: xAxisLabel` by `val: name`) can contain display strings rather
than the underlying temporal values, causing the backend drill-down query to be scoped to
mismatched values and returning empty or incorrect results, while the drill-to-detail
context menu (which uses `data[0]`) still works as expected—demonstrating the
inconsistency.
(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/Timeseries/EchartsTimeseries.tsx
**Line:** 256:266
**Comment:**
*Logic Error: For time-axis clicks, this forwards `props.name` into drill filter construction, but ECharts click `name` is often the formatted axis label while the raw filterable value is in the data tuple (the same file already uses `data[0]` for time drill-to-detail). This can generate drill filters with display strings instead of raw temporal values and return empty/incorrect drilled results. Use the raw axis value from the clicked data point for non-category axes when building drill filters.
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
Contributor
There was a problem hiding this comment.
Code Review Agent Run #927598
Actionable Suggestions - 2
-
superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.test.tsx - 1
- Incomplete tabindex assertion · Line 98-98
-
docs/docs/using-superset/exploring-data.mdx - 1
- CWE-1104: Deprecated Feature Documented Without Notice · Line 332-350
Review Details
-
Files reviewed - 12 · Commit Range:
db42179..7eb64fb- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
- superset-frontend/plugins/plugin-chart-echarts/test/BoxPlot/transformProps.test.ts
- superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.test.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/drillFilters.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/drillFilters.test.ts
- superset-frontend/packages/superset-ui-chart-controls/src/sections/drilldownHierarchy.tsx
- superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx
- superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx
- superset/config.py
- docs/docs/using-superset/exploring-data.mdx
-
Files skipped - 1
- docs/static/feature-flags.json - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- 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
| const root = screen.getByText('country'); | ||
| expect(root).toHaveAttribute('role', 'button'); | ||
| expect(root).toHaveAttribute('tabindex', '0'); | ||
| expect(screen.getByText('USA')).toHaveAttribute('role', 'button'); |
Contributor
There was a problem hiding this comment.
Incomplete tabindex assertion
Test claims all clickable segments are 'focusable via tabIndex' (line 93-94 comment) but only verifies tabindex on the 'country' element, not on 'USA'. Both segments are created by the same clickable() function which sets tabIndex={0} (DrillDownBreadcrumb.tsx:68). Inconsistent assertion weakens accessibility coverage.
Code Review Run #927598
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
+332
to
+350
| ### Drilling into data | ||
|
|
||
| Superset offers three distinct "drill" interactions. They are independent, each gated by its own | ||
| feature flag, and can be enabled in any combination: | ||
|
|
||
| - **Drill to detail** (`DRILL_TO_DETAIL`): right-click a data point and choose **Drill to detail** | ||
| to open a modal listing the individual rows that make up that point. It answers "what records are | ||
| behind this value?" without changing the chart. | ||
| - **Drill by** (`DRILL_BY`): right-click a data point and choose **Drill by** to open a modal that | ||
| re-groups the chart by a different dimension, scoped to the clicked value. It answers "how does | ||
| this value break down by another column?" and leaves the original chart untouched. | ||
| - **Drill-down hierarchy** (`DRILL_DOWN`, off by default): configure an ordered **Drill-down | ||
| hierarchy** in the chart's control panel, then left-click a data point on a dashboard chart to | ||
| descend to the next level of that hierarchy *in place*. A breadcrumb above the chart lets you | ||
| step back up, and each level emits a cross-filter for the accumulated path so the rest of the | ||
| dashboard follows along. It answers "let me navigate this dimension hierarchy by clicking." | ||
|
|
||
| Drill to detail and Drill by are context-menu actions that open modals; the drill-down hierarchy is | ||
| an in-place, click-to-navigate interaction on the dashboard itself. |
Contributor
There was a problem hiding this comment.
CWE-1104: Deprecated Feature Documented Without Notice
The DRILL_TO_DETAIL feature flag is deprecated (config.py line 936, lifecycle: deprecated) and was deprecated in version 5.0 (CHANGELOG/5.0.0.md line 462). The documentation should note this status to avoid misleading users about a deprecated feature. Also, the description for DRILL_DOWN implies it follows the same Behavior enum pattern as DrillToDetail/DrillBy, but Behavior.DrillDown does not exist in the Behavior enum (Base.ts lines 25-36); the drill-down feature uses a separate DrillDownHost.tsx implementation on dashboards. (See also: CWE-1104)
Code Review Run #927598
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Member
|
I added a hold label to this PR because I believe it's a significant enough change to require a SIP. I think we want to look at the need for an additional drilling method since we already have Drill By that allows a lot of flexibility. So to switch to something hierarchical that require configuration should get some level of consensus. I think we'd want to see the UI in screenshots of how the drilling works. I think we'd want to hear the use cases of why we need a new drilling method or what's insufficient about the existing one, and I think we'll need more detail on how one sets up the hierarchy for hierarchical drilling, especially in terms of semantic layer extensions that are now landing. |
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
3 participants
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 a hierarchical drill-down interaction for ECharts charts. The chart author
defines an ordered list of columns in a new Drill-down hierarchy control.
On a dashboard, left-clicking a data point drills to the next level scoped to
the clicked value, shows a breadcrumb for navigating back up, and emits a
cross-filter for the full drill path so other charts stay in sync.
Design decisions:
drilldown_hierarchycontrol — the x-axis control stays asingle (scalar) column, so existing charts and all downstream query
consumers are unaffected. No global
multichange, no feature flag needed.owns the drill stack), so downstream charts are scoped to the entire path,
not just the deepest clicked column.
deepest level, consistently across charts.
Supported charts: Bar, Line, Area, Scatter, Smooth Line, Step (x-axis based)
and Pie, Funnel, Gauge, Radar, Box Plot (groupby based).
TESTING INSTRUCTIONS
and add the deeper levels; save and add to a dashboard.
breadcrumb appears.
ADDITIONAL INFORMATION