EnxDev commented on PR #41907:
URL: https://github.com/apache/superset/pull/41907#issuecomment-4925733575
## EnxDev's Review Agent — apache/superset#41907 · HEAD 6f70126
**request 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 `EchartsTimeseries.tsx` (metrics offset,
`getCategoryAxisValue`, `props.name != null`) are genuinely fixed in this
revision — I verified each against the code, not the claim — but none of them
has a regression test.
### 🔴 Functional
-
**`plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:258`** ·
_High_ — `hasDimensions` is `groupby.length > 0`, and
Bar/Line/Area/Scatter/Step have **both** `x_axis` and `groupby` (Dimensions).
With `x_axis: country`, `groupby: [gender]`, `drilldown_hierarchy: [region,
city]` → `hierarchy = [country, region, city]`, but the click builds filters
from `groupby` (`gender`), so it drills on the series dimension.
`useDrillDownState.ts:266` then takes the `hasGroupby` branch and swaps
`groupby → [region]` while `x_axis` stays `country`. Result: the chart never
advances along the hierarchy, the breadcrumb reads `country › M`, and the
emitted cross-filter scopes other charts by `gender`. Anchor the drill to
`hierarchy[currentDepth]` — when `x_axis` is set, read the category via
`getCategoryAxisValue` and swap `x_axis`, not `groupby`. **regression test:**
Bar chart with `x_axis: 'country'` + `groupby: ['gender']`, hierarchy
`['country','region']`; click a bar �
�� expect the filter col to be `country` and `effectiveFormData.x_axis ===
'region'`.
- **`src/components/Chart/DrillDown/DrillDownHost.tsx:139`** · _Medium_ —
`chartStatus: isLoading ? 'loading' : 'rendered'`, but on the render right
after `drillDown()` the fetch effect hasn't run: `isLoading === false` and
`drillData === null`. `ChartRenderer` gets `queriesResponse: null` +
`chartStatus: 'rendered'` → `resultsReady` false → `queriesData` undefined →
`SuperChart` paints the "No results were returned for this query" empty state
for a frame, then blanks while loading. Every drill click flashes "No results".
Fix: `chartStatus: isLoading || effectiveQueriesResponse == null ? 'loading' :
'rendered'`. **regression test:** call `onDrillDown`, assert the renderer
receives `chartStatus: 'loading'` before the fetch resolves.
-
**`plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:276`** ·
_Medium_ — the category branch checks only `xAxis.type === AxisType.Category`
and drops master's `props.componentType === 'series'` guard (kept on line 260
of the cross-filter path). ECharts fires `click` for axis labels too, so
clicking an x-axis tick now drills. Restore the guard. **regression test:**
click event with `componentType: 'xAxis'` → `onDrillDown` not called.
- **`src/components/Chart/DrillDown/useDrillDownState.ts:422`** · _Low_ —
when the author lists exactly the chart's own dimension (`drilldown_hierarchy:
['country']` with `x_axis: 'country'`), `hierarchy.length === 1` and
`drillStack.length >= hierarchy.length - 1` is true on the first click, so it
takes the leaf branch and never pushes. `isDrilling` stays false →
`overlayProps` is `{}` → the chart never filters. But `onDrillDown` is still
provided, so normal cross-filter click behavior is replaced and the emitted
cross-filter can't be toggled off. The control's own docstring invites this
("prepended automatically if it is not already listed"). Gate `hasHierarchy` on
`hierarchy.length >= 2`. **regression test:** single-entry hierarchy equal to
`x_axis` → `hasHierarchy === false`.
### 🟡 Should-fix
- **`src/components/Chart/Chart.tsx:359`** — `ChartContainer` feeds
`Chart.tsx` from **both** the dashboard and Explore (`ExploreChartPanel`), so
drill-down and its click-behavior override are live in Explore, where the PR
says this is a dashboard feature. The control panel and the rendered chart then
disagree, and `configKey` is only `slice_id__viz_type`, so editing controls
doesn't reset the drill. Gate on `source === ChartSource.Dashboard`.
- **`src/components/Chart/DrillDown/useDrillDownState.ts:311`** —
`JSON.stringify(effectiveFormData)` runs on every render for **every chart in
the app**, hierarchy or not (at depth 0 `effectiveFormData === formData`).
Guard with `hasHierarchy && currentDepth > 0`.
- **`src/components/Chart/DrillDown/useDrillDownState.ts:73`** — the
module-level `drillStateStore` is keyed by `slice_id`, never evicted, and
shared across dashboards and Explore for the same slice. Key by `chartId`, or
scope it to the dashboard id.
- **`src/components/Chart/DrillDown/useDrillDownState.ts:218`** —
`ensureIsArray(...) as string[]` is an unchecked cast; the control spreads
`sharedControls.groupby` (`dndGroupByControl`, `freeForm: true`), so entries
can be `AdhocColumn` objects. Those never match
`drillLevels.includes(xAxisStr)`, get assigned raw into `x_axis`/`groupby`, and
when one lands at `hierarchy[0]` the breadcrumb renders an object as a React
child. Set `freeForm: false` on the control or normalize with `getColumnLabel`.
-
**`packages/superset-ui-chart-controls/src/sections/drilldownHierarchy.tsx:61`**
— `renderTrigger: false` contradicts its own comment ("no re-render/re-query
is needed"). With `false`, every hierarchy edit marks the chart stale and
re-runs the base query. Use `renderTrigger: true`.
- **`src/components/Chart/DrillDown/DrillDownHost.tsx:149`** —
`handleResetTo` dispatches `updateDataMask` regardless of `emitCrossFilters`,
while `onDrillDown` gates on it. With cross-filters off, drilling emits nothing
but returning to root writes an empty `filterState` and forces
`triggerQuery(true, chartId)` — a full re-query — even though nothing was ever
emitted.
- **`src/components/Chart/chartReducer.ts:230`** — an unrelated defensive
fix bundled into a feature PR, and it silently drops every keyed action for a
missing chart. The comment ties it to drill-down cross-filtering, which
suggests the drill flow dispatches against removed charts. Split it out and fix
the root cause rather than the symptom.
- **Test coverage** — `EchartsTimeseries.tsx` gained 67 lines of drill logic
with zero tests; `eventHandlers.test.ts` only exercises the groupby path in
`utils/eventHandlers.ts`. The x-axis branch, horizontal orientation,
zero-valued labels, and the multi-metric offset are all untested — exactly the
three bugs already reported once on this file. The `x_axis` + `groupby` case
above would have been caught by any test of that combination.
- **No feature flag, no docs.** Superset already ships `DRILL_BY` (default
on) and drill-to-detail. A second, parallel drill mechanism with no flag, no
`docs/` update, and no stated relationship to `DrillBy` needs a maintainer
decision before merge.
- **`src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx:94`** — raw
`<div>`/`<span>`/`<button>` where `@superset-ui/core/components` exports
`Breadcrumb`. `DrillDownHost.test.tsx` also imports `render` from
`@testing-library/react` instead of `spec/helpers/testing-library`, and uses
`as any` four times.
### 🔵 Nits
- `src/components/Chart/DrillDown/useDrillDownState.ts:388` —
`console.error` logs the full `effectiveFormData` on every failed drill.
- `src/components/Chart/ChartRenderer.tsx:149,181` — inline
`import('@superset-ui/core').BinaryQueryObjectFilterClause`; use a top-level
type import.
- `src/components/Chart/DrillDown/DrillDownHost.tsx:191` — the
breadcrumb-height effect depends only on `drillStack.length`, so it misses
`selectedLeaf`/`error` changes and wrapping, leaving `adjustedHeight` stale.
- `src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx:100` —
`key={index}`.
- `src/components/Chart/DrillDown/DrillDownBreadcrumb.test.tsx` — the last
test's comment ("only have been called for the root click test") describes
state from another, isolated test.
### 🙌 Praise
- `src/components/Chart/DrillDown/useDrillDownState.ts:302` — the
`effectiveFormDataRef` + serialized-key guard against identity-only `formData`
churn is a subtle bug to have found, and the comment names the exact failure
mode (in-flight request cancelled, chart spinning to the 60s timeout).
- `src/components/Chart/DrillDown/useDrillDownState.ts:73` — persisting
drill state synchronously on mutate rather than in an effect, with a remount
test to lock it in.
<!-- enxdev-review-agent:6f70126 -->
_Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 6f70126._
--
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]
