bito-code-review[bot] commented on code in PR #41907: URL: https://github.com/apache/superset/pull/41907#discussion_r3574783200
########## superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.test.tsx: ########## @@ -0,0 +1,138 @@ +/** + * 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 { render, screen, fireEvent } from 'spec/helpers/testing-library'; +import { DrillDownBreadcrumb } from './DrillDownBreadcrumb'; +import { DrillDownLevel } from './types'; + +const hierarchy = ['country', 'region', 'city']; + +const drillStack: DrillDownLevel[] = [ + { + filters: [{ col: 'country', op: '==', val: 'USA' }], + label: 'USA', + }, + { + filters: [{ col: 'region', op: '==', val: 'Texas' }], + label: 'Texas', + }, +]; + +test('breadcrumb returns null when drillStack is empty and no selectedLeaf', () => { + const { container } = render( + <DrillDownBreadcrumb + hierarchy={hierarchy} + drillStack={[]} + onJumpTo={jest.fn()} + />, + ); + + expect(container.firstChild).toBeNull(); +}); + +test('breadcrumb renders hierarchy[0] and drill labels', () => { + render( + <DrillDownBreadcrumb + hierarchy={hierarchy} + drillStack={drillStack} + onJumpTo={jest.fn()} + />, + ); + + // The root column name should be rendered + expect(screen.getByText('country')).toBeInTheDocument(); + // Drill labels should be rendered + expect(screen.getByText('USA')).toBeInTheDocument(); + expect(screen.getByText('Texas')).toBeInTheDocument(); +}); + +test('clicking a breadcrumb segment calls onJumpTo with correct depth', () => { + const onJumpTo = jest.fn(); + + render( + <DrillDownBreadcrumb + hierarchy={hierarchy} + drillStack={drillStack} + onJumpTo={onJumpTo} + />, + ); + + // Click the root segment (hierarchy[0]) — should call onJumpTo(0) + fireEvent.click(screen.getByText('country')); + expect(onJumpTo).toHaveBeenCalledWith(0); + + // Click the first drill label 'USA' — should call onJumpTo(1) + fireEvent.click(screen.getByText('USA')); + expect(onJumpTo).toHaveBeenCalledWith(1); +}); + +test('clickable breadcrumb segments are keyboard accessible', () => { + render( + <DrillDownBreadcrumb + hierarchy={hierarchy} + drillStack={drillStack} + onJumpTo={jest.fn()} + />, + ); + + // Clickable segments expose role="button" and are focusable via tabIndex so + // they remain operable by keyboard (Enter/Space) without a native <button>. + const root = screen.getByText('country'); + expect(root).toHaveAttribute('role', 'button'); + expect(root).toHaveAttribute('tabindex', '0'); + expect(screen.getByText('USA')).toHaveAttribute('role', 'button'); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete tabindex assertion</b></div> <div id="fix"> Test claims all clickable segments are 'focusable via tabIndex' (line 93-94 comment) but only verifies `tabindex` on the 'country' element, not on 'USA'. Both segments are created by the same `clickable()` function which sets `tabIndex={0}` (DrillDownBreadcrumb.tsx:68). Inconsistent assertion weakens accessibility coverage. </div> </div> <small><i>Code Review Run #927598</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 ########## docs/docs/using-superset/exploring-data.mdx: ########## @@ -329,6 +329,26 @@ various options in this section, refer to the Lastly, save your chart as Tutorial Resample and add it to the Tutorial Dashboard. Go to the tutorial dashboard to see the four charts side by side and compare the different outputs. +### Drilling into data + +Superset offers three distinct "drill" interactions. They are independent, each gated by its own +feature flag, and can be enabled in any combination: + +- **Drill to detail** (`DRILL_TO_DETAIL`): right-click a data point and choose **Drill to detail** + to open a modal listing the individual rows that make up that point. It answers "what records are + behind this value?" without changing the chart. +- **Drill by** (`DRILL_BY`): right-click a data point and choose **Drill by** to open a modal that + re-groups the chart by a different dimension, scoped to the clicked value. It answers "how does + this value break down by another column?" and leaves the original chart untouched. +- **Drill-down hierarchy** (`DRILL_DOWN`, off by default): configure an ordered **Drill-down + hierarchy** in the chart's control panel, then left-click a data point on a dashboard chart to + descend to the next level of that hierarchy *in place*. A breadcrumb above the chart lets you + step back up, and each level emits a cross-filter for the accumulated path so the rest of the + dashboard follows along. It answers "let me navigate this dimension hierarchy by clicking." + +Drill to detail and Drill by are context-menu actions that open modals; the drill-down hierarchy is +an in-place, click-to-navigate interaction on the dashboard itself. Review Comment: <div> <div id="suggestion"> <div id="issue"><b>CWE-1104: Deprecated Feature Documented Without Notice</b></div> <div id="fix"> The `DRILL_TO_DETAIL` feature flag is deprecated (config.py line 936, lifecycle: deprecated) and was deprecated in version 5.0 (CHANGELOG/5.0.0.md line 462). The documentation should note this status to avoid misleading users about a deprecated feature. Also, the description for `DRILL_DOWN` implies it follows the same Behavior enum pattern as DrillToDetail/DrillBy, but `Behavior.DrillDown` does not exist in the Behavior enum (Base.ts lines 25-36); the drill-down feature uses a separate DrillDownHost.tsx implementation on dashboards. (See also: [CWE-1104](https://cwe.mitre.org/data/definitions/1104.html)) </div> </div> <small><i>Code Review Run #927598</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]
