EnxDev commented on code in PR #41907: URL: https://github.com/apache/superset/pull/41907#discussion_r4329610858
########## superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx: ########## @@ -0,0 +1,372 @@ +/** + * 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 { + ComponentType, + useCallback, + useMemo, + useRef, + useState, + useEffect, +} from 'react'; +import { + QueryData, + QueryFormData, + BinaryQueryObjectFilterClause, + FeatureFlag, + isFeatureEnabled, + ensureIsArray, +} from '@superset-ui/core'; +import { useSelector } from 'react-redux'; +import { css } from '@apache-superset/core/theme'; +import { Loading } from '@superset-ui/core/components'; +import { ChartSource } from 'src/types/ChartSource'; +import type { RootState } from 'src/dashboard/types'; +import { selectAsyncModeOverride } from 'src/utils/asyncMode'; +import type { RequestParams } from 'src/components/Chart/chartAction'; +import { useDrillDownState } from './useDrillDownState'; +import { DrillDownBreadcrumb } from './DrillDownBreadcrumb'; +import type { ChartRendererProps } from '../ChartRenderer'; + +/** + * Hook payload contract: chart plugins call `onDrillDown(filters, label)` + * via the chart's hooks bag when the user clicks a data point and a + * drill-down hierarchy is configured. + */ +export type OnDrillDownHook = ( + filters: BinaryQueryObjectFilterClause[], + label: string, +) => void; + +/** + * Build the cross-filter clauses the dashboard data-mask expects from a set of + * drill-down filter clauses. Shared by the drill (onDrillDown) and the + * breadcrumb navigation (handleResetTo) paths so their cross-filter shape can + * never diverge. Special cases mirror the native ECharts cross-filter path: a + * temporal bucket click is passed through as a `TEMPORAL_RANGE`, and a null + * value becomes `IS NULL` (rather than `IN [null]`, which selects nothing). + */ +const toCrossFilterClauses = (filters: BinaryQueryObjectFilterClause[]) => + filters.map(f => { + if (f.op === 'TEMPORAL_RANGE') { + return { col: f.col, op: 'TEMPORAL_RANGE' as const, val: f.val }; + } + if (f.val == null) { + return { col: f.col, op: 'IS NULL' as const }; + } + return { + col: f.col, + op: 'IN' as const, + val: [f.val] as (string | number | boolean)[], + }; + }); + +interface DrillDownHostProps extends ChartRendererProps { + /** The wrapped renderer component */ + ChartRendererComponent: ComponentType< + ChartRendererProps & { onDrillDown?: OnDrillDownHook } + >; +} + +/** + * Wraps `<ChartRenderer>` with drill-down behavior. When the chart's + * form_data declares a `drilldown_hierarchy`, this host: + * + * 1. Tracks how deep the user has drilled (a stack of levels) + * 2. Computes "effective" form_data for the current level (swapping the + * grouping dimension — groupby or x_axis — and adding accumulated filters) + * 3. Re-fetches chart data for that level + * 4. Renders a breadcrumb above the chart for navigating back up + * + * If the chart has no hierarchy, this is a thin pass-through. + */ +export function DrillDownHost({ + ChartRendererComponent, + ...rendererProps +}: DrillDownHostProps) { + const { formData, queriesResponse } = rendererProps; + + // Live cross-filter selection this chart has emitted into the dashboard data + // mask. A drilled chart writes its path here; when the mask is cleared + // (dashboard teardown, or the user removes the cross-filter from the filter + // bar) this goes empty. The hook uses it to discard persisted drill state + // that outlived its data mask instead of replaying it on remount. + const crossFilterValue = useSelector<RootState, unknown>( + state => state.dataMask?.[rendererProps.chartId]?.filterState?.value, + ); + const crossFilterCleared = + !!rendererProps.emitCrossFilters && + ensureIsArray(crossFilterValue).length === 0; + + // Request params for drill queries, mirroring exploreJSON so superseded + // drills abort, hung queries time out, and the per-dashboard async override + // is honored. The hook adds the per-request AbortSignal. + const asyncModeOverride = useSelector(selectAsyncModeOverride); + const dashboardId = useSelector<RootState, number | undefined>( + state => state.dashboardInfo?.id, + ); + const webserverTimeout = useSelector<RootState, number | undefined>( + state => state.common?.conf?.SUPERSET_WEBSERVER_TIMEOUT, + ); + const drillRequestParams = useMemo<RequestParams>(() => { + const params: RequestParams = {}; + if (webserverTimeout) { + params.timeout = webserverTimeout * 1000; + } + if (dashboardId) { + params.dashboard_id = dashboardId; + } + if (asyncModeOverride) { + params.async_mode_override = asyncModeOverride; + } + return params; + }, [webserverTimeout, dashboardId, asyncModeOverride]); + + const { + isDrilling, + drillStack, + selectedLeaf, + hierarchy, + effectiveFormData, + effectiveQueriesResponse, + isLoading, + error, + hasHierarchy, + drillDown, + resetTo, + } = useDrillDownState({ + chartId: rendererProps.chartId, + formData, + baseQueriesResponse: queriesResponse, + crossFilterCleared, + requestParams: drillRequestParams, + }); + + // Drill-down is a dashboard interaction gated behind the DRILL_DOWN feature + // flag. In Explore the control panel and the rendered chart would disagree + // (and clicks there mean something else), so only enable it when rendered + // inside a dashboard. + const drillEnabled = + isFeatureEnabled(FeatureFlag.DrillDown) && + hasHierarchy && + rendererProps.source === ChartSource.Dashboard; Review Comment: `drillEnabled` gates the click handler, but `useDrillDownState` above runs on every chart anyway, and its fetch effect only looks at stack depth. Drill a chart on a dashboard, hit Edit chart, and Explore restores the stack from `drillStateStore` (same `chartId`, same `configKey`, `crossFilterCleared` is false there) and fires the drilled query, retries included, for a result nothing renders. Could the flag and source check go into the hook as an `enabled` arg that skips both the restore and the fetch? ########## superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts: ########## @@ -335,6 +335,7 @@ export default function transformProps( groupby, selectedValues, onContextMenu, + onDrillDown, Review Comment: With more than one metric the boxes are named `group, metric`, but `labelMap` is keyed by `group` alone, so the drill handler finds no entry and bails. A multi-metric box plot with a hierarchy configured never drills, even though Box Plot is on the supported list. Could we key `labelMap` by the series name the boxes actually use, or leave Box Plot out for now? ########## superset-frontend/packages/superset-ui-chart-controls/src/sections/drilldownHierarchy.tsx: ########## @@ -0,0 +1,76 @@ +/** + * 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 { t } from '@apache-superset/core/translation'; +import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core'; +import { ControlPanelSectionConfig } from '../types'; +import { dndGroupByControl } from '../shared-controls/dndControls'; + +/** + * Generic, reusable control panel section that lets the chart author define + * an ordered drill-down hierarchy. When the user clicks a data point on a + * dashboard chart, the host advances to the next level of this hierarchy by + * swapping the chart's dimension and adding a filter for the clicked value. + * A breadcrumb above the chart lets the user step back up. + * + * The list is stored in form_data as `drilldown_hierarchy`: string[] where + * each entry is a column name. Order matters — the chart's own x-axis (or + * groupby) is the top level shown initially, and each entry is a level the + * user reaches by clicking, in order. The x-axis column is prepended + * automatically if it is not already listed. + * + * This control does NOT affect the base (undrilled) query — it only + * configures what happens on click, so `x_axis` stays a single scalar column + * for every existing chart. + */ +export const drilldownHierarchySection: ControlPanelSectionConfig = { + label: t('Drill-down hierarchy'), + expanded: false, Review Comment: With `DRILL_DOWN` off (the default) this section still renders as an empty collapsible panel on all 11 charts, since only the control inside is gated. `ControlPanelSectionConfig` takes `visibility` too and `ControlPanelsContainer` honors it, so it's a one-liner: ```suggestion expanded: false, visibility: () => isFeatureEnabled(FeatureFlag.DrillDown), ``` ########## superset-frontend/src/components/Chart/DrillDown/DrillDownHost.tsx: ########## @@ -0,0 +1,372 @@ +/** + * 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 { + ComponentType, + useCallback, + useMemo, + useRef, + useState, + useEffect, +} from 'react'; +import { + QueryData, + QueryFormData, + BinaryQueryObjectFilterClause, + FeatureFlag, + isFeatureEnabled, + ensureIsArray, +} from '@superset-ui/core'; +import { useSelector } from 'react-redux'; +import { css } from '@apache-superset/core/theme'; +import { Loading } from '@superset-ui/core/components'; +import { ChartSource } from 'src/types/ChartSource'; +import type { RootState } from 'src/dashboard/types'; +import { selectAsyncModeOverride } from 'src/utils/asyncMode'; +import type { RequestParams } from 'src/components/Chart/chartAction'; +import { useDrillDownState } from './useDrillDownState'; +import { DrillDownBreadcrumb } from './DrillDownBreadcrumb'; +import type { ChartRendererProps } from '../ChartRenderer'; + +/** + * Hook payload contract: chart plugins call `onDrillDown(filters, label)` + * via the chart's hooks bag when the user clicks a data point and a + * drill-down hierarchy is configured. + */ +export type OnDrillDownHook = ( + filters: BinaryQueryObjectFilterClause[], + label: string, +) => void; + +/** + * Build the cross-filter clauses the dashboard data-mask expects from a set of + * drill-down filter clauses. Shared by the drill (onDrillDown) and the + * breadcrumb navigation (handleResetTo) paths so their cross-filter shape can + * never diverge. Special cases mirror the native ECharts cross-filter path: a + * temporal bucket click is passed through as a `TEMPORAL_RANGE`, and a null + * value becomes `IS NULL` (rather than `IN [null]`, which selects nothing). + */ +const toCrossFilterClauses = (filters: BinaryQueryObjectFilterClause[]) => + filters.map(f => { + if (f.op === 'TEMPORAL_RANGE') { + return { col: f.col, op: 'TEMPORAL_RANGE' as const, val: f.val }; + } + if (f.val == null) { + return { col: f.col, op: 'IS NULL' as const }; + } + return { + col: f.col, + op: 'IN' as const, + val: [f.val] as (string | number | boolean)[], + }; + }); + +interface DrillDownHostProps extends ChartRendererProps { + /** The wrapped renderer component */ + ChartRendererComponent: ComponentType< + ChartRendererProps & { onDrillDown?: OnDrillDownHook } + >; +} + +/** + * Wraps `<ChartRenderer>` with drill-down behavior. When the chart's + * form_data declares a `drilldown_hierarchy`, this host: + * + * 1. Tracks how deep the user has drilled (a stack of levels) + * 2. Computes "effective" form_data for the current level (swapping the + * grouping dimension — groupby or x_axis — and adding accumulated filters) + * 3. Re-fetches chart data for that level + * 4. Renders a breadcrumb above the chart for navigating back up + * + * If the chart has no hierarchy, this is a thin pass-through. + */ +export function DrillDownHost({ + ChartRendererComponent, + ...rendererProps +}: DrillDownHostProps) { + const { formData, queriesResponse } = rendererProps; + + // Live cross-filter selection this chart has emitted into the dashboard data + // mask. A drilled chart writes its path here; when the mask is cleared + // (dashboard teardown, or the user removes the cross-filter from the filter + // bar) this goes empty. The hook uses it to discard persisted drill state + // that outlived its data mask instead of replaying it on remount. + const crossFilterValue = useSelector<RootState, unknown>( + state => state.dataMask?.[rendererProps.chartId]?.filterState?.value, + ); + const crossFilterCleared = + !!rendererProps.emitCrossFilters && + ensureIsArray(crossFilterValue).length === 0; + + // Request params for drill queries, mirroring exploreJSON so superseded + // drills abort, hung queries time out, and the per-dashboard async override + // is honored. The hook adds the per-request AbortSignal. + const asyncModeOverride = useSelector(selectAsyncModeOverride); + const dashboardId = useSelector<RootState, number | undefined>( + state => state.dashboardInfo?.id, + ); + const webserverTimeout = useSelector<RootState, number | undefined>( + state => state.common?.conf?.SUPERSET_WEBSERVER_TIMEOUT, + ); + const drillRequestParams = useMemo<RequestParams>(() => { + const params: RequestParams = {}; + if (webserverTimeout) { + params.timeout = webserverTimeout * 1000; + } + if (dashboardId) { + params.dashboard_id = dashboardId; + } + if (asyncModeOverride) { + params.async_mode_override = asyncModeOverride; + } + return params; + }, [webserverTimeout, dashboardId, asyncModeOverride]); + + const { + isDrilling, + drillStack, + selectedLeaf, + hierarchy, + effectiveFormData, + effectiveQueriesResponse, + isLoading, + error, + hasHierarchy, + drillDown, + resetTo, + } = useDrillDownState({ + chartId: rendererProps.chartId, + formData, + baseQueriesResponse: queriesResponse, + crossFilterCleared, + requestParams: drillRequestParams, + }); + + // Drill-down is a dashboard interaction gated behind the DRILL_DOWN feature + // flag. In Explore the control panel and the rendered chart would disagree + // (and clicks there mean something else), so only enable it when rendered + // inside a dashboard. + const drillEnabled = + isFeatureEnabled(FeatureFlag.DrillDown) && + hasHierarchy && + rendererProps.source === ChartSource.Dashboard; + + const onDrillDown = useMemo<OnDrillDownHook | undefined>(() => { + // Suspend drilling while a drill query is in error: the overlay has fallen + // back to the base chart, so a click here would append a base-level value + // as the next level and emit contradictory filters (e.g. country=USA plus + // country=Canada). The breadcrumb still allows navigating up, which + // re-queries and clears the error. + if (!drillEnabled || error != null) { + return undefined; + } Review Comment: Returning `undefined` here doesn't pause drilling, it flips the plugins back to their normal click handler. After a failed drill, clicking the fallback base chart emits a regular cross-filter (say `country IN [Canada]`) over the drill-path mask while the breadcrumb still reads `USA › ...`. A no-op keeps them in drill mode and just swallows the click: ```suggestion if (!drillEnabled) { return undefined; } if (error != null) { return () => {}; } ``` Worth a test that a click after a failed drill leaves `setDataMask` alone. ########## superset-frontend/src/components/Chart/DrillDown/useDrillDownState.ts: ########## @@ -0,0 +1,608 @@ +/** + * 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, + useLayoutEffect, + useMemo, + useRef, + useState, +} from 'react'; +import { t } from '@apache-superset/core/translation'; +import { + BinaryQueryObjectFilterClause, + ensureIsArray, + getClientErrorObject, + getColumnLabel, + getXAxisLabel, + isQueryFormColumn, + isXAxisSet, + QueryData, + QueryFormData, +} from '@superset-ui/core'; +import { simpleFilterToAdhoc } from 'src/utils/simpleFilterToAdhoc'; +import { + requestChartDataResolved, + type RequestParams, +} from 'src/components/Chart/chartAction'; +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'; + +/** + * Drill navigation is kept in a module-level store keyed by chart id so it + * survives incidental remounts of the chart component. On a dashboard, an + * unrelated filter change (e.g. removing another chart's cross-filter) can + * cause the grid to re-render and remount the chart, which would otherwise + * reset the local React state and make the breadcrumb vanish mid-drill, + * stranding the user with no way to navigate back up. The store is process + * memory only — a full page reload still starts fresh. + */ +interface StoredDrillState { + drillStack: DrillDownLevel[]; + selectedLeaf?: string; + /** Filters for the value selected at the deepest level, if any. */ + selectedLeafFilters?: BinaryQueryObjectFilterClause[]; + /** + * Identity of the chart configuration (and dashboard slot) this state + * belongs to. A remount that restores from the store compares this against + * the current config so a stale record from a different configuration is + * discarded rather than replayed. + */ + configKey: string; +} +const drillStateStore = new Map<string | number, StoredDrillState>(); + +/** + * Clear persisted drill state. Without arguments clears everything (used by + * tests to isolate cases); with a chart id clears just that chart. + */ +export function clearDrillDownState(chartKey?: string | number): void { + if (chartKey === undefined) { + drillStateStore.clear(); + } else { + drillStateStore.delete(chartKey); + } +} + +interface UseDrillDownStateArgs { + /** Unique chart instance id (dashboard grid assigns one per slot). */ + chartId: string | number; + formData: QueryFormData; + /** Original chart data, shown when the drill stack is empty */ + baseQueriesResponse?: QueryData[] | null; + /** + * True when the chart's owning cross-filter data mask has been cleared + * (dashboard teardown, or the user removed this chart's cross-filter from the + * filter bar). Persisted drill state mirrors that data mask, so when it is + * gone the stored stack is orphaned: on mount it is discarded instead of + * replayed (which would re-fire a drilled query while linked charts sit at + * root). Defaults to false so non-dashboard callers keep plain persistence. + */ + crossFilterCleared?: boolean; + /** + * Base request params (timeout, dashboard_id, async_mode_override) for drill + * queries, mirroring exploreJSON. The hook adds the per-request AbortSignal. + * Without these a superseded synchronous drill keeps running, hung queries + * lack the normal timeout, and per-dashboard async overrides are ignored. + */ + requestParams?: RequestParams; +} + +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; + /** + * 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) => void; + /** Truncate the drill stack to the given depth (0 = back to start) */ + resetTo: (depth: number) => 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({ + chartId, + formData, + baseQueriesResponse, + crossFilterCleared, + requestParams, +}: UseDrillDownStateArgs): UseDrillDownStateResult { + const chartKey = chartId; + + // Identity of the current drill configuration (and dashboard slot). The drill + // stack is anchored to the primary dimension (x_axis or groupby) and the + // hierarchy list, so this key changes whenever the drill would target a + // different set of columns. It is stored alongside persisted state and + // compared on restore. + const configFd = formData as Record<string, unknown>; + const configKey = JSON.stringify([ Review Comment: The comments say this key covers the dashboard slot, but `chartId` is the slice id, which is the same on every dashboard. With cross-filtering off `crossFilterCleared` never flips, so drilling chart 42 and then opening another dashboard that also has chart 42 lands you mid-drill there, despite the docstring saying closing the dashboard wipes the state. Could the dashboard id (already in `drillRequestParams`) go into the key? -- 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]
