EnxDev commented on PR #40449: URL: https://github.com/apache/superset/pull/40449#issuecomment-4902061780
## EnxDev's Review Agent โ apache/superset#40449 ยท HEAD 84b64b0 **Request changes** โ supersedes my [prior review](https://github.com/apache/superset/pull/40449#pullrequestreview-4572719388) (HEAD 3de6b12). The risky items from last round are fixed, but the fix removed the only path that configured the hierarchy: as shipped, no control populates `drilldown_hierarchy`, so the feature is unreachable. Treemap/Sunburst are still advertised but not wired, and pre-commit CI is red. **Fixed since last round** (thanks): dropped the global `x_axis` `multi: true`, removed the unrelated `superset/models/core.py` engine-listener change, gated behind the new `DRILL_DOWN_HIERARCHY` flag, fixed the hardcoded `2px` spacing (`theme.sizeUnit / 2`) and the inline `BinaryQueryObjectFilterClause` import. ### ๐ด Functional - **`src/components/Chart/DrillDown/useDrillDownState.ts:2067`** ยท _High_ โ the hook reads the hierarchy from `drilldown_hierarchy` / `drilldownHierarchy` form_data, but **no control in this diff writes that field** (code search: 0 references on master; no control panel file is touched). The prior `xAxisMixin multi:true` approach was reverted and nothing replaced it, so `hasHierarchy` is always false and the entire drill path is dead code for end users. The PR "How it works" still describes `xAxisMixin` / `getXAxisColumn` / `isXAxisSet` changes that aren't in the diff. **regression test:** an explore control test that sets a hierarchy via the UI and asserts `form_data.drilldown_hierarchy` is populated โ impossible today because no control exists. - **`Treemap/transformProps.ts`, `Sunburst/transformProps.ts` (absent)** ยท _High_ โ PR body lists Treemap and Sunburst as supported, but neither `transformProps` is in the diff, so `onDrillDown` is never destructured/forwarded; they won't drill. Sunburst also groups on `columns`, which `effectiveFormData` can't target (it only swaps `groupby`/`x_axis`). Wire them or drop them from the supported list. (persists from prior review) **regression test:** click a Sunburst/Treemap segment with a hierarchy and assert `onDrillDown` fires. - **`src/components/Chart/DrillDown/useDrillDownState.ts:2104`** ยท _Low_ โ `effectiveFormData` checks `hasGroupby` first, so a bar/line chart configured with **both** an `x_axis` and a series `groupby` swaps `groupby` (the series breakdown) instead of `x_axis` when drilling. Confirm the intended target for charts that have both. **regression test:** drill a bar chart with `x_axis` + a non-empty `groupby` and assert `x_axis` is the field swapped. ### ๐ก Should-fix - **`superset/config.py:809`** โ new `DRILL_DOWN_HIERARCHY` flag added to `DEFAULT_FEATURE_FLAGS` and `featureFlags.ts`, but `docs/static/feature-flags.json` isn't synced. Keep them in sync. - **CI** โ `pre-commit (current)` is failing. Resolve before merge (likely lint/format). - **`plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:246`** โ re-implements the drill logic already in `eventHandlers.ts`; the two have diverged (only Timeseries handles the x-axis-only / `props.name` cases). Consolidate to avoid drift. (persists) - **`packages/superset-ui-core/types/storybook-actions.d.ts`** โ new ambient `declare module 'storybook/actions'` is unrelated to drill-down and looks like stray scope from a rebase. Drop it unless it's genuinely required by this PR. - **PR description is stale** โ "How it works" describes the reverted `xAxisMixin`/`getXAxisColumn`/`isXAxisSet` changes; update it to the `drilldown_hierarchy` approach so reviewers/users aren't misled. - No `/docs` update for a new user-facing feature. (persists) ### ๐ต Nits - `DrillDown/DrillDownBreadcrumb.tsx:75` โ navigation still uses hand-rolled `<span role="button">`; prefer the core `Breadcrumb`/`Button` for a11y + consistency. (persists) - PR title `feat: New feature tk drilldown hierarchy` passes pr-lint but reads as a placeholder (double spaces, "tk"); tidy to e.g. `feat(explore): drill-down hierarchy for ECharts charts`. (persists) ### ๐ Praise - `chartReducer.ts:226` โ the guard against stale keyed actions on a removed chart is a clean, well-commented fix with a matching regression test. - Strong test coverage across `useDrillDownState`, `DrillDownHost`, `DrillDownBreadcrumb`, and `eventHandlers`. <!-- enxdev-review-agent:84b64b0 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 84b64b0._ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
