EnxDev commented on PR #41907: URL: https://github.com/apache/superset/pull/41907#issuecomment-4952958103
## EnxDev's Review Agent โ apache/superset#41907 ยท HEAD db42179 **comment** โ supersedes my prior [`request changes` (6f70126)](https://github.com/apache/superset/pull/41907#issuecomment-4925733575). The drill-path blocker and all four ๐ด findings are fixed; what's left is test coverage and a maintainer call on flagging/docs. No hard blocker remains. Verified against the code, not the "fixed" claims: - **x_axis + groupby charts now drill the x-axis, not the series dimension.** `useDrillDownState.ts` `xAxisIsSet` branch swaps `x_axis` and preserves `groupby`; `EchartsTimeseries.tsx` builds the filter from `xAxis.label`. Regression test present (`effectiveFormData advances x_axis (not groupby) when the chart has both`). - `chartStatus: isLoading || effectiveQueriesResponse == null ? 'loading' : 'rendered'` โ no more "No results" flash on drill. - Axis-label guard restored (`props.componentType === 'series'`). - `hasHierarchy = hierarchy.length >= 2` โ a single-level hierarchy no longer hijacks the click. Test present. - Explore gate (`source === ChartSource.Dashboard`), store keyed by `chartId` + eviction, `freeForm: false`, `renderTrigger: true`, `handleResetTo` gated on `emitCrossFilters`, and `console.error` no longer dumping form_data โ all addressed. (`triggerQuery` and `updateDataMask` are confirmed bound on the dashboard chart's `actions` via `ChartContainer`, so the reset-to-root re-query is real, not a silent no-op.) ### ๐ก Should-fix - **`plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx`** โ the ~60-line inline drill handler is still ~26% covered (codecov: 17 missing lines on this file). The x-axis category branch, horizontal-bar orientation, the restored axis-label guard, and the non-category branch are all untested โ the same file that carried the three earlier bugs. Two of the fixes I asked for last round shipped without a regression test. Add: category-series click โ filter `col === xAxis.label`; `componentType: 'xAxis'` โ `onDrillDown` not called; first drill render โ renderer receives `chartStatus: 'loading'`. - **No feature flag, no `docs/`, no stated relationship to the existing `DRILL_BY` / drill-to-detail.** A second, parallel drill mechanism on a size/XXL PR is a maintainer decision before merge. - **`EchartsTimeseries.tsx` drill handler** โ the breadcrumb label uses `String(categoryAxisValue)` / `String(props.name)`, while the groupby path (`utils/eventHandlers.ts`) and the cross-filter path both use `formatSeriesName`. Timeseries breadcrumb and cross-filter labels show raw values (unformatted decimals, raw timestamps). Use `formatSeriesName` for parity. For a non-category (time) top level the raw `props.name` is also used as the filter `val`, which can fail to match the underlying column โ low-probability, but worth guarding if time-as-drill-root is meant to be supported. - **`src/components/Chart/chartReducer.ts:226`** โ the "ignore a keyed action when the chart is missing" guard is an unrelated defensive change bundled into this feature PR, and it silently drops every keyed action for a missing chart. Its own comment ties it to drill-down cross-filtering โ i.e. the drill flow dispatches against removed charts. Split it out and fix the root cause rather than masking the symptom. ### ๐ต Nits - `src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx` โ raw `<div>/<span>/<button>`; `@superset-ui/core/components` exports `Breadcrumb`. - BoxPlot `onDrillDown` passthrough is the only transformProps still without a hook test (Pie/Funnel/Gauge/Radar each got one). ### ๐ Praise - The x_axis/groupby fix is the right shape: anchor the drill to `hierarchy[currentDepth]`, advance the x-axis, keep the series `groupby`, and pin exactly that combination with a test. <!-- enxdev-review-agent:db42179 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD db42179._ -- 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]
