bito-code-review[bot] commented on code in PR #41907: URL: https://github.com/apache/superset/pull/41907#discussion_r3553340051
########## superset-frontend/packages/superset-ui-chart-controls/src/sections/drilldownHierarchy.tsx: ########## @@ -0,0 +1,69 @@ +/** + * 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 { 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, + controlSetRows: [ + [ + { + name: 'drilldown_hierarchy', + config: { + // Reuse the drag-and-drop column selector used for "Dimensions" + // so authors can reorder levels by dragging. + ...dndGroupByControl, + label: t('Drill-down levels'), + description: t( + 'Ordered list of columns to drill into when a user clicks ' + + "a data point. The chart's primary dimension is shown " + + 'initially. Each click drills to the next level, scoped to the ' + + 'clicked value. Drag rows to reorder levels.', + ), + default: [], + // The drill levels must be plain column references (the drill logic + // matches them by name), so disallow ad-hoc/custom SQL columns. + freeForm: false, + // Configures click behavior only and does not affect the base query, + // so editing it re-renders without marking the chart stale. + renderTrigger: true, + }, + }, + ], + ], +}; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing unit test coverage</b></div> <div id="fix"> Per BITO.md rule 11730, comprehensive unit tests are required for new features covering success paths, error scenarios, validation failures, and edge cases. The `drilldownHierarchySection` control panel section lacks test coverage. </div> </div> <small><i>Code Review Run #93499c</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/Regular/Scatter/controlPanel.tsx: ########## @@ -59,6 +59,7 @@ const { const config: ControlPanelConfig = { controlPanelSections: [ sections.echartsTimeSeriesQueryWithXAxisSort, + sections.drilldownHierarchySection, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing drilldown test coverage</b></div> <div id="fix"> The newly added `drilldownHierarchySection` lacks test coverage in `controlPanel.test.ts`. Other controls (`x_axis_time_format`, `x_axis_number_format`) have explicit tests. According to BITO.md [6262], tests should verify actual business logic, not just component rendering. </div> </div> <small><i>Code Review Run #93499c</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/transformProps.ts: ########## @@ -741,6 +741,7 @@ export default function transformProps( onContextMenu, onLegendStateChanged, onLegendScroll, + onDrillDown, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test coverage for onDrillDown</b></div> <div id="fix"> The `onDrillDown` hook is integrated into `transformProps` (line 744) and returned as a transformed prop (line 1243), following the established pattern from `Pie/transformProps.ts`. However, `EchartsTimeseries.test.tsx` has tests for `onContextMenu` but lacks coverage for `onDrillDown`. Add test cases verifying the drill-down handler receives correct filters and label. </div> </div> <small><i>Code Review Run #93499c</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]
