EnxDev commented on PR #41907: URL: https://github.com/apache/superset/pull/41907#issuecomment-4961845674
## EnxDev's Review Agent โ apache/superset#41907 ยท HEAD 60f31c0 **comment** โ supersedes my prior [comment (db42179)](https://github.com/apache/superset/pull/41907#issuecomment-4952958103). Every prior should-fix is addressed: the timeseries drill handler was extracted to `drillFilters.ts` **with** a real test, the `DRILL_DOWN` flag + config + docs landed, and `formatSeriesName` label parity is in. No blocker remains. Dark-launched (flag off by default). Verified against the code, not the "fixed" claims: - **Timeseries drills the x-axis, not the series dimension.** The inline handler is gone; `buildTimeseriesDrillFilters` builds the filter from `xAxisLabel`, orientation-aware via `getCategoryAxisValue(data, name, orientation)`, with `name != null` so zero-like labels still drill. Covered by `test/Timeseries/drillFilters.test.ts` (category / horizontal / time / no-value / formatting). - **Groupby path applies the metrics offset** โ `eventHandlers.ts` uses `values[metricsCount + i]`, tested in `eventHandlers.test.ts` ("skips leading metric tokens"). - Feature flag in sync across `config.py`, `featureFlags.ts`, `feature-flags.json`; `exploring-data.mdx` explains the relationship to `DRILL_BY` / `DRILL_TO_DETAIL`. - Retry exhaustion, unmount `isLoading` reset, remount persistence, and the reset-to-root re-query are all covered in `useDrillDownState.test.ts`. The open bot findings are stale: codeant's four "logic-error" flags (metrics offset, horizontal orientation, falsy-zero, `values[i]`) reference pre-refactor line numbers and no longer apply; its "BoxPlot has no hierarchy control" is also fixed (`sections.drilldownHierarchySection` was added). The bito test-coverage flags (retry logic, unmount, chartReducer four-field assertion, fake-timer `afterEach`, Radar/Pie passthrough) are addressed. CI is green. ### ๐ก Should-fix - **`plugins/plugin-chart-echarts/src/utils/eventHandlers.ts:~178`** ยท _Low_ โ for a multi-metric Box Plot the series name is `` `${group}, ${metric}` `` but `labelMap` is keyed by `group` only (`BoxPlot/transformProps.ts:165-176`), so `labelMap[e.name]` is `undefined` โ the drill handler returns early and, because drill mode replaces the normal cross-filter click, the click becomes a no-op. Only bites multi-metric BoxPlot + a configured hierarchy (matches the existing cross-filter limitation), but worth gating drill-down to single-metric or keying by the full series name. **regression test:** multi-metric BoxPlot drill click โ `onDrillDown` not called today. - **`src/components/Chart/chartReducer.ts:226`** โ the "ignore a keyed action when the chart is missing" guard is still bundled into this feature PR and silently drops *every* keyed action for a missing chart. Carried over from my last review: its own comment ties it to the drill cross-filter flow dispatching against removed charts โ split it out and fix the root cause rather than mask the symptom. ### ๐ต Nits - `plugins/plugin-chart-echarts/src/utils/eventHandlers.ts` โ `metricsCount = values.length - groupby.length` is recomputed inside the `groupby.forEach`; hoist it above the loop. - `useDrillDownState.ts` โ `console.error('[DrillDown] query failed:', โฆ)`; Superset prefers `logging` from `@apache-superset/core/utils`. - `DrillDownBreadcrumb.tsx` renders navigable segments as `<span role="button">` (the PR description claims a native `<button>`); functionally keyboard-operable via the keydown handler, so fine as-is. ### ๐ Praise - The prior "0 coverage" gap is closed: `drillFilters.ts`, the 640-line `useDrillDownState` spec (remount persistence + retry exhaustion), and the `DrillDownHost` full-path cross-filter test all guard the behavior they claim to. <!-- enxdev-review-agent:60f31c0 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 60f31c0._ -- 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]
