bito-code-review[bot] commented on code in PR #40449:
URL: https://github.com/apache/superset/pull/40449#discussion_r3305830634
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -125,7 +125,7 @@ export default function transformProps(
currencyFormats = {},
currencyCodeColumn,
} = datasource;
- const { setDataMask = () => {}, onContextMenu } = hooks;
+ const { setDataMask = () => {}, onContextMenu, onDrillDown } = hooks;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>onDrillDown not consumed in component</b></div>
<div id="fix">
The `onDrillDown` hook is extracted at line 128 and passed in the return
object at line 320, but `EchartsTreemap.tsx` does not receive or consume this
prop. The click event handler only emits cross-filters via `handleChange` — it
never calls `onDrillDown`. This breaks drill-down functionality for the Treemap
chart even when `drilldown_hierarchy` is configured.
</div>
</div>
<small><i>Code Review Run #7ccb16</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useCallback } from 'react';
+import { css, useTheme } from '@apache-superset/core/theme';
+import { DrillDownLevel } from './types';
+
+interface DrillDownBreadcrumbProps {
+ /** Ordered list of column names from the chart's drill-down hierarchy */
+ hierarchy: string[];
+ /** The drill levels the user has navigated through */
+ drillStack: DrillDownLevel[];
+ /** Value selected at the deepest level (shown but not clickable) */
+ selectedLeaf?: string;
+ /** Reset the drill-down to the given depth (0 = back to the start) */
+ onJumpTo: (depth: number) => void;
+}
+
+/**
+ * Compact breadcrumb shown above the chart while the user is drilled into
+ * a hierarchy. Clicking any segment jumps back up the stack.
+ *
+ * country › USA › region › Texas
+ */
+export function DrillDownBreadcrumb({
+ hierarchy,
+ drillStack,
+ selectedLeaf,
+ onJumpTo,
+}: DrillDownBreadcrumbProps) {
+ const theme = useTheme();
+
+ const handleClick = useCallback(
+ (depth: number) => (e: React.MouseEvent) => {
+ e.preventDefault();
+ onJumpTo(depth);
+ },
+ [onJumpTo],
+ );
+
+ if (drillStack.length === 0 && !selectedLeaf) {
+ return null;
+ }
+
+ return (
+ <div
+ data-test="drill-down-breadcrumb"
+ css={css`
+ padding: ${theme.sizeUnit}px ${theme.sizeUnit * 2}px;
+ background: ${theme.colorBgContainer};
+ border-bottom: 1px solid ${theme.colorBorderSecondary};
+ font-size: ${theme.fontSizeSM}px;
+ display: flex;
+ align-items: center;
+ gap: ${theme.sizeUnit}px;
+ flex-wrap: wrap;
+ `}
+ >
+ <span
+ role="button"
+ tabIndex={0}
+ onClick={handleClick(0)}
+ css={css`
+ cursor: pointer;
+ color: ${theme.colorPrimary};
+ `}
+ >
+ {hierarchy[0] ?? ''}
+ </span>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Missing keyboard handler</b></div>
<div id="fix">
The breadcrumb segments have role="button" and tabIndex={0} but lack
onKeyDown handlers, making them inaccessible via keyboard navigation. Users
relying on keyboard cannot activate these interactive elements. (See also:
[CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
a/superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx
+++
b/superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.tsx
@@ -74,7 +74,10 @@ export function DrillDownBreadcrumb({
<span
role="button"
tabIndex={0}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') onJumpTo(0);
+ }}
onClick={handleClick(0)}
css={css`
cursor: pointer;
@@ -93,6 +96,9 @@ export function DrillDownBreadcrumb({
<span
role="button"
tabIndex={0}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') onJumpTo(index +
1);
+ }}
onClick={handleClick(index + 1)}
css={css`
cursor: pointer;
color: ${theme.colorPrimary};
`}
>
```
</div>
</details>
</div>
<small><i>Code Review Run #7ccb16</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -218,6 +219,87 @@ export default function EchartsTimeseries({
const eventHandlers: EventHandlers = {
click: props => {
+ // Drill-down takes priority over cross-filter when a hierarchy is
configured.
+ // The DrillDownHost provides onDrillDown only when a hierarchy exists.
+ const hasDrillHierarchy = !!onDrillDown;
+
+ if (hasDrillHierarchy) {
+ if (clickTimer.current) {
+ clearTimeout(clickTimer.current);
+ }
+ clickTimer.current = setTimeout(() => {
+ const { seriesName: name } = props;
+ const drillFilters: BinaryQueryObjectFilterClause[] = [];
+ if (hasDimensions) {
+ const values = labelMap[name] ?? [];
+ groupby.forEach((dimension, i) => {
+ drillFilters.push({
+ col: dimension,
+ op: '==',
+ val: values[i],
+ formattedVal: String(values[i]),
+ });
+ });
+ // Also emit cross-filter so other charts in the dashboard filter
too
+ if (emitCrossFilters) {
+ // Use direct setDataMask (not handleChange which toggles)
+ setDataMask({
+ extraFormData: {
+ filters: groupby.map((col, idx) => ({
+ col,
+ op: 'IN' as const,
+ val: [values[idx]] as (string | number | boolean)[],
+ })),
+ },
+ filterState: {
+ value: [values],
+ selectedValues: [name],
+ },
+ });
+ }
+ } else if (xAxis.type === AxisType.Category && props.data?.[0] !=
null) {
+ // Bar chart with x_axis only (no groupby dimensions)
+ drillFilters.push({
+ col: xAxis.label,
+ op: '==',
+ val: props.data[0],
+ formattedVal: String(props.data[0]),
+ });
+ // Also emit cross-filter by x-axis value
+ if (emitCrossFilters) {
+ setDataMask({
+ extraFormData: {
+ filters: [{
+ col: xAxis.label,
+ op: 'IN' as const,
+ val: [props.data[0]] as (string | number | boolean)[],
+ }],
+ },
+ filterState: {
+ value: [String(props.data[0])],
+ selectedValues: [String(props.data[0])],
+ },
+ });
+ }
+ } else if (props.name) {
+ // Fallback: use the event name (typically the x-axis label)
+ drillFilters.push({
+ col: xAxis.label,
+ op: '==',
+ val: props.name,
+ formattedVal: String(props.name),
+ });
+ }
+ if (drillFilters.length > 0) {
+ const label = drillFilters
+ .map(f => f.formattedVal ?? String(f.val))
+ .join(', ');
+ onDrillDown(drillFilters, label);
+ }
+ }, TIMER_DURATION);
+ return;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing drill-down tests</b></div>
<div id="fix">
The new `onDrillDown` click handler (lines 226-300) with multiple branching
logic and timer-based execution has no corresponding tests. Per [6262], tests
must verify actual business logic — this click handler's complex branching
(hasDimensions → categorical X-axis → props.name fallback) with cross-filter
side effects requires test coverage to prevent regressions.
</div>
</div>
<small><i>Code Review Run #7ccb16</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts:
##########
@@ -0,0 +1,278 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useCallback, useEffect, useMemo, useState } from 'react';
+import {
+ BinaryQueryObjectFilterClause,
+ ensureIsArray,
+ QueryData,
+ QueryFormData,
+} from '@superset-ui/core';
+import { simpleFilterToAdhoc } from 'src/utils/simpleFilterToAdhoc';
+import {
+ getChartDataRequest,
+ handleChartDataResponse,
+} from 'src/components/Chart/chartAction';
+import { getQuerySettings } from 'src/explore/exploreUtils';
+import { DrillDownLevel } from './types';
+
+/**
+ * The form-data field name that stores the ordered list of drill columns.
+ * The chart starts at hierarchy[0] and advances one level per click.
+ */
+const HIERARCHY_FIELD = 'drilldown_hierarchy';
+const HIERARCHY_FIELD_CAMEL = 'drilldownHierarchy';
+
+/**
+ * Default form-data field that holds the chart's grouping dimension.
+ * Most echarts plugins use 'groupby'; Sunburst uses 'columns'. The click
+ * handler can override this on a per-event basis.
+ */
+const DEFAULT_GROUPBY_FIELD = 'groupby';
+const DEFAULT_ADHOC_FILTERS_FIELD = 'adhoc_filters';
+
+interface UseDrillDownStateArgs {
+ formData: QueryFormData;
+ /** Original chart data, shown when the drill stack is empty */
+ baseQueriesResponse?: QueryData[] | null;
+}
+
+interface UseDrillDownStateResult {
+ /** True if the user has drilled at least one level deep */
+ isDrilling: boolean;
+ /** The breadcrumb path showing where the user is in the hierarchy */
+ drillStack: DrillDownLevel[];
+ /** Value selected at the deepest level */
+ selectedLeaf?: string;
+ /** The computed hierarchy of column names */
+ hierarchy: string[];
+ /** form_data adjusted for the current drill level */
+ effectiveFormData: QueryFormData;
+ /** Chart data for the current drill level (or base data when not drilling)
*/
+ effectiveQueriesResponse: QueryData[] | null | undefined;
+ /** True while the next-level data is being fetched */
+ isLoading: boolean;
+ /** Error message if the drill query failed */
+ error?: string;
+ /** Whether the chart has a configured drill-down hierarchy */
+ hasHierarchy: boolean;
+ /** Whether there are more levels to drill into from the current state */
+ hasMoreLevels: boolean;
+ /**
+ * Push a new level onto the drill stack. Called from the chart's click
+ * handler with the filters that identify the clicked data point.
+ */
+ drillDown: (
+ filters: BinaryQueryObjectFilterClause[],
+ label: string,
+ options?: { groupbyFieldName?: string; adhocFilterFieldName?: string },
+ ) => void;
+ /** Truncate the drill stack to the given depth (0 = back to start) */
+ resetTo: (depth: number) => void;
+ /** Reset to base data (depth 0) */
+ reset: () => void;
+}
+
+/**
+ * Hook that manages a chart's drill-down state. Owns the drill stack,
+ * computes the effective form_data for the current level, fetches the
+ * data for that level, and exposes navigation helpers (drillDown / resetTo).
+ *
+ * The hook never mutates the upstream Redux store: closing or refreshing
+ * the dashboard wipes the drill state and restores the original chart.
+ */
+export function useDrillDownState({
+ formData,
+ baseQueriesResponse,
+}: UseDrillDownStateArgs): UseDrillDownStateResult {
+ const [drillStack, setDrillStack] = useState<DrillDownLevel[]>([]);
+ const [selectedLeaf, setSelectedLeaf] = useState<string | undefined>();
+ const [drillData, setDrillData] = useState<QueryData[] | null>(null);
+ const [isLoading, setIsLoading] = useState(false);
+ const [error, setError] = useState<string | undefined>();
+
+ // Reset whenever the underlying form_data (chart configuration) changes —
+ // e.g. the dashboard owner edited the chart and saved.
+ useEffect(() => {
+ setDrillStack([]);
+ setSelectedLeaf(undefined);
+ setDrillData(null);
+ setError(undefined);
+ }, [formData.slice_id, formData.viz_type]);
+
+ const hierarchy = useMemo<string[]>(
+ () => {
+ const fd = formData as Record<string, unknown>;
+
+ // Option 1: x_axis is an array (multi-column, the new UX)
+ const xAxis = fd.x_axis ?? fd.xAxis;
+ if (Array.isArray(xAxis) && xAxis.length > 1) {
+ return xAxis as string[];
+ }
+
+ // Option 2: explicit drilldown_hierarchy field (legacy/separate control)
+ const drillLevels = ensureIsArray(fd[HIERARCHY_FIELD] ??
fd[HIERARCHY_FIELD_CAMEL]) as string[];
+ if (drillLevels.length > 0) {
+ const xAxisStr = (typeof xAxis === 'string' ? xAxis : undefined);
+ if (xAxisStr && !drillLevels.includes(xAxisStr)) {
+ return [xAxisStr, ...drillLevels];
+ }
+ return drillLevels;
+ }
+
+ return [];
+ },
+ [formData],
+ );
+
+ const hasHierarchy = hierarchy.length > 0;
+ const currentDepth = drillStack.length;
+ const hasMoreLevels = hasHierarchy && currentDepth < hierarchy.length - 1;
+
+ const effectiveFormData = useMemo<QueryFormData>(() => {
+ if (currentDepth === 0) {
+ return formData;
+ }
+ const nextColumn = hierarchy[currentDepth];
+
+ // Merge accumulated filters from every level into adhoc_filters.
+ const accumulatedFilters = drillStack.flatMap(level => level.filters);
+ const baseAdhoc = ensureIsArray(
+ (formData as Record<string, unknown>)[DEFAULT_ADHOC_FILTERS_FIELD],
+ );
+
+ const fdRecord = formData as Record<string, unknown>;
+
+ // Determine which field to swap with the next hierarchy level.
+ // Bar/Line/Area charts use x_axis as the primary dimension when groupby
is empty.
+ const groupbyValue = fdRecord[DEFAULT_GROUPBY_FIELD];
+ const hasGroupby = Array.isArray(groupbyValue)
+ ? groupbyValue.length > 0
+ : !!groupbyValue;
+
+ const updated = { ...formData } as Record<string, unknown>;
+
+ if (hasGroupby) {
+ // Chart uses groupby — swap it
+ const isArrayGroupby = Array.isArray(groupbyValue);
+ updated[DEFAULT_GROUPBY_FIELD] = isArrayGroupby
+ ? [nextColumn]
+ : nextColumn;
+ } else if (fdRecord.x_axis !== undefined || fdRecord.xAxis !== undefined) {
+ // Chart uses x_axis (Bar/Line/Timeseries) — swap it
+ if (fdRecord.x_axis !== undefined) {
+ updated.x_axis = nextColumn;
+ }
+ if (fdRecord.xAxis !== undefined) {
+ updated.xAxis = nextColumn;
+ }
+ } else {
+ // Fallback: set groupby
+ updated[DEFAULT_GROUPBY_FIELD] = [nextColumn];
+ }
+
+ updated[DEFAULT_ADHOC_FILTERS_FIELD] = [
+ ...baseAdhoc,
+ ...accumulatedFilters.map(f => simpleFilterToAdhoc(f)),
+ ];
+
+ return updated as QueryFormData;
+ }, [formData, drillStack, currentDepth, hierarchy]);
+
+ // Fetch data whenever the user drills (stack changes and is non-empty).
+ useEffect(() => {
+ if (currentDepth === 0) {
+ setDrillData(null);
+ setError(undefined);
+ return undefined;
+ }
+
+ let cancelled = false;
+ setIsLoading(true);
+ setError(undefined);
+
+ const [useLegacyApi] = getQuerySettings(effectiveFormData);
+ getChartDataRequest({ formData: effectiveFormData })
+ .then(({ response, json }) =>
+ handleChartDataResponse(response, json, useLegacyApi),
+ )
+ .then(queriesResponse => {
+ if (!cancelled) {
+ setDrillData(queriesResponse as QueryData[]);
+ }
+ })
+ .catch(err => {
+ if (!cancelled) {
+ setError(err?.message || 'Failed to load drill-down data');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing translation function wrapper</b></div>
<div id="fix">
User-facing error message should use translation function `t('Failed to load
drill-down data')` for i18n support, consistent with DrillByModal.tsx:105
pattern in this same codebase.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts
+++ b/superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts
@@ -17,6 +17,7 @@
* under the License.
*/
import { useCallback, useEffect, useMemo, useState } from 'react';
+import { t } from '@apache-superset/core/translation';
import {
BinaryQueryObjectFilterClause,
ensureIsArray,
@@ -218,7 +219,7 @@ export function useDrillDownState({
setError(err?.message || 'Failed to load drill-down data');
}
})
- .finally(() => {
+ .finally(() => {
if (!cancelled) {
setIsLoading(false);
}
});
@@ -233,7 +234,7 @@ export function useDrillDownState({
setError(err?.message || 'Failed to load drill-down data');
},
.catch(err => {
- if (!cancelled) {
- setError(err?.message || 'Failed to load drill-down data');
+ if (!cancelled) {
+ setError(err?.message || t('Failed to load drill-down data'));
}
})
.finally(() => {
```
</div>
</details>
</div>
<small><i>Code Review Run #7ccb16</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
