codeant-ai-for-open-source[bot] commented on code in PR #40449: URL: https://github.com/apache/superset/pull/40449#discussion_r3534487782
########## superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx: ########## @@ -0,0 +1,230 @@ +/** + * 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 { ReactElement } from 'react'; +import { + render as rtlRender, + screen, + act, + fireEvent, +} from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ThemeProvider, supersetTheme } from '@apache-superset/core/theme'; +import { QueryFormData } from '@superset-ui/core'; +import { DrillDownHost } from './DrillDownHost'; +import { clearDrillDownState } from './useDrillDownState'; +import type { ChartRendererProps } from '../ChartRenderer'; + +// Enable the drill-down feature flag for all tests in this file. +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + isFeatureEnabled: (flag: string) => flag === 'DRILL_DOWN_HIERARCHY', +})); + +function render(ui: ReactElement) { + return rtlRender(ui, { + wrapper: ({ children }) => ( + <ThemeProvider theme={supersetTheme}>{children}</ThemeProvider> + ), + }); +} + +beforeEach(() => { + clearDrillDownState(); +}); + +jest.mock('src/components/Chart/chartAction', () => ({ + getChartDataRequest: jest.fn(() => + Promise.resolve({ response: {}, json: { result: [{ data: [] }] } }), + ), + handleChartDataResponse: jest.fn(() => + Promise.resolve([{ data: [{ region: 'Texas' }] }]), + ), +})); + +jest.mock('src/explore/exploreUtils', () => ({ + getQuerySettings: jest.fn(() => [false]), +})); + +jest.mock('src/utils/simpleFilterToAdhoc', () => ({ + simpleFilterToAdhoc: jest.fn(filter => ({ + expressionType: 'SIMPLE', + clause: 'WHERE', + operator: filter.op, + subject: filter.col, + comparator: filter.val, + })), +})); + +// A simple mock ChartRenderer component that renders its props for inspection +function MockChartRenderer( + props: ChartRendererProps & { onDrillDown?: unknown }, +) { + return ( + <div data-test="mock-chart-renderer"> + <span data-test="has-on-drill-down"> + {props.onDrillDown ? 'yes' : 'no'} + </span> + <span data-test="form-data-x-axis"> + {JSON.stringify((props.formData as Record<string, unknown>).x_axis)} + </span> + </div> + ); +} + +const baseFormData: QueryFormData = { + datasource: '1__table', + viz_type: 'echarts_timeseries_bar', + slice_id: 42, + x_axis: 'country', + groupby: [], + adhoc_filters: [], +}; + +const baseRendererProps: ChartRendererProps = { + formData: baseFormData, + vizType: 'echarts_timeseries_bar', + chartId: 1, + height: 400, + width: 600, + queriesResponse: [{ data: [{ country: 'USA' }] }], + actions: {} as ChartRendererProps['actions'], +}; + +test('DrillDownHost passes through to ChartRenderer when no hierarchy', () => { + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} Review Comment: **Suggestion:** Replace the `any` cast with a concrete component type for `ChartRendererComponent` so the test uses explicit props typing. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The test file contains a direct `any` cast in new TypeScript code, which matches the custom rule prohibiting `any` types. This is a real violation in the final file state. </details> <details> <summary><b>Rule source 📖 </b></summary> .cursor/rules/dev-standard.mdc (line 16) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3f524f93e2094ba0866a392e4ba321e9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3f524f93e2094ba0866a392e4ba321e9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx **Line:** 112:112 **Comment:** *Custom Rule: Replace the `any` cast with a concrete component type for `ChartRendererComponent` so the test uses explicit props typing. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40449&comment_hash=d1fa622e091b6b303d5529cef6d3b7f3feb875b03e867942a35557bcab19f35d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40449&comment_hash=d1fa622e091b6b303d5529cef6d3b7f3feb875b03e867942a35557bcab19f35d&reaction=dislike'>👎</a> ########## superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx: ########## @@ -0,0 +1,230 @@ +/** + * 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 { ReactElement } from 'react'; +import { + render as rtlRender, + screen, + act, + fireEvent, +} from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ThemeProvider, supersetTheme } from '@apache-superset/core/theme'; +import { QueryFormData } from '@superset-ui/core'; +import { DrillDownHost } from './DrillDownHost'; +import { clearDrillDownState } from './useDrillDownState'; +import type { ChartRendererProps } from '../ChartRenderer'; + +// Enable the drill-down feature flag for all tests in this file. +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + isFeatureEnabled: (flag: string) => flag === 'DRILL_DOWN_HIERARCHY', +})); + +function render(ui: ReactElement) { + return rtlRender(ui, { + wrapper: ({ children }) => ( + <ThemeProvider theme={supersetTheme}>{children}</ThemeProvider> + ), + }); +} + +beforeEach(() => { + clearDrillDownState(); +}); + +jest.mock('src/components/Chart/chartAction', () => ({ + getChartDataRequest: jest.fn(() => + Promise.resolve({ response: {}, json: { result: [{ data: [] }] } }), + ), + handleChartDataResponse: jest.fn(() => + Promise.resolve([{ data: [{ region: 'Texas' }] }]), + ), +})); + +jest.mock('src/explore/exploreUtils', () => ({ + getQuerySettings: jest.fn(() => [false]), +})); + +jest.mock('src/utils/simpleFilterToAdhoc', () => ({ + simpleFilterToAdhoc: jest.fn(filter => ({ + expressionType: 'SIMPLE', + clause: 'WHERE', + operator: filter.op, + subject: filter.col, + comparator: filter.val, + })), +})); + +// A simple mock ChartRenderer component that renders its props for inspection +function MockChartRenderer( + props: ChartRendererProps & { onDrillDown?: unknown }, +) { + return ( + <div data-test="mock-chart-renderer"> + <span data-test="has-on-drill-down"> + {props.onDrillDown ? 'yes' : 'no'} + </span> + <span data-test="form-data-x-axis"> + {JSON.stringify((props.formData as Record<string, unknown>).x_axis)} + </span> + </div> + ); +} + +const baseFormData: QueryFormData = { + datasource: '1__table', + viz_type: 'echarts_timeseries_bar', + slice_id: 42, + x_axis: 'country', + groupby: [], + adhoc_filters: [], +}; + +const baseRendererProps: ChartRendererProps = { + formData: baseFormData, + vizType: 'echarts_timeseries_bar', + chartId: 1, + height: 400, + width: 600, + queriesResponse: [{ data: [{ country: 'USA' }] }], + actions: {} as ChartRendererProps['actions'], +}; + +test('DrillDownHost passes through to ChartRenderer when no hierarchy', () => { + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + />, + ); + + // Should render the chart + expect(screen.getByTestId('mock-chart-renderer')).toBeInTheDocument(); + // No drill-down host wrapper when no hierarchy + expect(screen.queryByTestId('drill-down-host')).not.toBeInTheDocument(); + // onDrillDown should not be provided + expect(screen.getByTestId('has-on-drill-down')).toHaveTextContent('no'); +}); + +test('DrillDownHost shows breadcrumb wrapper when hierarchy exists', () => { + const formDataWithHierarchy: QueryFormData = { + ...baseFormData, + x_axis: 'country', + drilldown_hierarchy: ['country', 'region', 'city'], + }; + + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + formData={formDataWithHierarchy} + />, + ); + + // Should render the drill-down host wrapper + expect(screen.getByTestId('drill-down-host')).toBeInTheDocument(); + // Chart should still render + expect(screen.getByTestId('mock-chart-renderer')).toBeInTheDocument(); +}); + +test('onDrillDown is provided when hierarchy exists', () => { + const formDataWithHierarchy: QueryFormData = { + ...baseFormData, + x_axis: 'country', + drilldown_hierarchy: ['country', 'region', 'city'], + }; + + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + formData={formDataWithHierarchy} + />, + ); + + expect(screen.getByTestId('has-on-drill-down')).toHaveTextContent('yes'); +}); + +test('DrillDownHost renders with drilldown_hierarchy field', () => { + const formDataWithHierarchy: QueryFormData = { + ...baseFormData, + x_axis: 'country', + drilldown_hierarchy: ['country', 'region', 'city'], + }; + + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + formData={formDataWithHierarchy} + />, + ); + + expect(screen.getByTestId('drill-down-host')).toBeInTheDocument(); + expect(screen.getByTestId('has-on-drill-down')).toHaveTextContent('yes'); +}); + +test('returning to the top level clears the cross-filter and re-queries the base', async () => { + const updateDataMask = jest.fn(); + const triggerQuery = jest.fn(); + const formDataWithHierarchy: QueryFormData = { + ...baseFormData, + x_axis: 'country', + drilldown_hierarchy: ['country', 'region', 'city'], + }; + + let capturedOnDrillDown: + | ((filters: unknown, label: string) => void) + | undefined; + function CaptureRenderer( + props: ChartRendererProps & { + onDrillDown?: (filters: unknown, label: string) => void; + }, + ) { + capturedOnDrillDown = props.onDrillDown; + return <div data-test="mock-chart-renderer" />; + } + + render( + <DrillDownHost + ChartRendererComponent={CaptureRenderer as any} Review Comment: **Suggestion:** Remove the `any` cast and type `CaptureRenderer` to match the expected `ChartRendererComponent` signature. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This new test code also uses an explicit `any` cast, so it violates the no-`any` rule. The suggestion accurately identifies a real issue present in the file. </details> <details> <summary><b>Rule source 📖 </b></summary> .cursor/rules/dev-standard.mdc (line 16) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=07fd90bd88174061b4ca34170b9b7ea9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=07fd90bd88174061b4ca34170b9b7ea9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx **Line:** 206:206 **Comment:** *Custom Rule: Remove the `any` cast and type `CaptureRenderer` to match the expected `ChartRendererComponent` signature. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40449&comment_hash=8cf8cca8d5f418ada97265a0f466b75daad4290ae1fb573e419b849724e2297a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40449&comment_hash=8cf8cca8d5f418ada97265a0f466b75daad4290ae1fb573e419b849724e2297a&reaction=dislike'>👎</a> ########## superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx: ########## @@ -0,0 +1,230 @@ +/** + * 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 { ReactElement } from 'react'; +import { + render as rtlRender, + screen, + act, + fireEvent, +} from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ThemeProvider, supersetTheme } from '@apache-superset/core/theme'; +import { QueryFormData } from '@superset-ui/core'; +import { DrillDownHost } from './DrillDownHost'; +import { clearDrillDownState } from './useDrillDownState'; +import type { ChartRendererProps } from '../ChartRenderer'; + +// Enable the drill-down feature flag for all tests in this file. +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + isFeatureEnabled: (flag: string) => flag === 'DRILL_DOWN_HIERARCHY', +})); + +function render(ui: ReactElement) { + return rtlRender(ui, { + wrapper: ({ children }) => ( + <ThemeProvider theme={supersetTheme}>{children}</ThemeProvider> + ), + }); +} + +beforeEach(() => { + clearDrillDownState(); +}); + +jest.mock('src/components/Chart/chartAction', () => ({ + getChartDataRequest: jest.fn(() => + Promise.resolve({ response: {}, json: { result: [{ data: [] }] } }), + ), + handleChartDataResponse: jest.fn(() => + Promise.resolve([{ data: [{ region: 'Texas' }] }]), + ), +})); + +jest.mock('src/explore/exploreUtils', () => ({ + getQuerySettings: jest.fn(() => [false]), +})); + +jest.mock('src/utils/simpleFilterToAdhoc', () => ({ + simpleFilterToAdhoc: jest.fn(filter => ({ + expressionType: 'SIMPLE', + clause: 'WHERE', + operator: filter.op, + subject: filter.col, + comparator: filter.val, + })), +})); + +// A simple mock ChartRenderer component that renders its props for inspection +function MockChartRenderer( + props: ChartRendererProps & { onDrillDown?: unknown }, +) { + return ( + <div data-test="mock-chart-renderer"> + <span data-test="has-on-drill-down"> + {props.onDrillDown ? 'yes' : 'no'} + </span> + <span data-test="form-data-x-axis"> + {JSON.stringify((props.formData as Record<string, unknown>).x_axis)} + </span> + </div> + ); +} + +const baseFormData: QueryFormData = { + datasource: '1__table', + viz_type: 'echarts_timeseries_bar', + slice_id: 42, + x_axis: 'country', + groupby: [], + adhoc_filters: [], +}; + +const baseRendererProps: ChartRendererProps = { + formData: baseFormData, + vizType: 'echarts_timeseries_bar', + chartId: 1, + height: 400, + width: 600, + queriesResponse: [{ data: [{ country: 'USA' }] }], + actions: {} as ChartRendererProps['actions'], +}; + +test('DrillDownHost passes through to ChartRenderer when no hierarchy', () => { + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + />, + ); + + // Should render the chart + expect(screen.getByTestId('mock-chart-renderer')).toBeInTheDocument(); + // No drill-down host wrapper when no hierarchy + expect(screen.queryByTestId('drill-down-host')).not.toBeInTheDocument(); + // onDrillDown should not be provided + expect(screen.getByTestId('has-on-drill-down')).toHaveTextContent('no'); +}); + +test('DrillDownHost shows breadcrumb wrapper when hierarchy exists', () => { + const formDataWithHierarchy: QueryFormData = { + ...baseFormData, + x_axis: 'country', + drilldown_hierarchy: ['country', 'region', 'city'], + }; + + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + formData={formDataWithHierarchy} + />, + ); + + // Should render the drill-down host wrapper + expect(screen.getByTestId('drill-down-host')).toBeInTheDocument(); + // Chart should still render + expect(screen.getByTestId('mock-chart-renderer')).toBeInTheDocument(); +}); + +test('onDrillDown is provided when hierarchy exists', () => { + const formDataWithHierarchy: QueryFormData = { + ...baseFormData, + x_axis: 'country', + drilldown_hierarchy: ['country', 'region', 'city'], + }; + + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + formData={formDataWithHierarchy} + />, + ); + + expect(screen.getByTestId('has-on-drill-down')).toHaveTextContent('yes'); +}); + +test('DrillDownHost renders with drilldown_hierarchy field', () => { + const formDataWithHierarchy: QueryFormData = { + ...baseFormData, + x_axis: 'country', + drilldown_hierarchy: ['country', 'region', 'city'], + }; + + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + formData={formDataWithHierarchy} + />, + ); + + expect(screen.getByTestId('drill-down-host')).toBeInTheDocument(); + expect(screen.getByTestId('has-on-drill-down')).toHaveTextContent('yes'); +}); + +test('returning to the top level clears the cross-filter and re-queries the base', async () => { + const updateDataMask = jest.fn(); + const triggerQuery = jest.fn(); + const formDataWithHierarchy: QueryFormData = { + ...baseFormData, + x_axis: 'country', + drilldown_hierarchy: ['country', 'region', 'city'], + }; + + let capturedOnDrillDown: + | ((filters: unknown, label: string) => void) + | undefined; + function CaptureRenderer( + props: ChartRendererProps & { + onDrillDown?: (filters: unknown, label: string) => void; + }, + ) { + capturedOnDrillDown = props.onDrillDown; + return <div data-test="mock-chart-renderer" />; + } + + render( + <DrillDownHost + ChartRendererComponent={CaptureRenderer as any} + {...baseRendererProps} + formData={formDataWithHierarchy} + actions={{ updateDataMask, triggerQuery } as any} Review Comment: **Suggestion:** Type the `actions` mock object with the appropriate `ChartRendererProps['actions']` shape instead of casting to `any`. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The code uses `as any` in a changed TypeScript test file, which is exactly what the custom rule forbids. This is a verified violation. </details> <details> <summary><b>Rule source 📖 </b></summary> .cursor/rules/dev-standard.mdc (line 16) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7c945b31076547d1806e34d2aba69ddd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7c945b31076547d1806e34d2aba69ddd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/Chart/DrillDown/DrillDownHost.test.tsx **Line:** 209:209 **Comment:** *Custom Rule: Type the `actions` mock object with the appropriate `ChartRendererProps['actions']` shape instead of casting to `any`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40449&comment_hash=86ff9bff281fdd29f47723eede1549c4c7af9baf566181ee626090f4e7957b2f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40449&comment_hash=86ff9bff281fdd29f47723eede1549c4c7af9baf566181ee626090f4e7957b2f&reaction=dislike'>👎</a> -- 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]
