bito-code-review[bot] commented on code in PR #36275: URL: https://github.com/apache/superset/pull/36275#discussion_r2561425297
########## superset-frontend/plugins/plugin-chart-word-cloud/test/RotationControl.test.tsx: ########## @@ -0,0 +1,47 @@ +/** + * 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 { RotationControl } from '../src/plugin/controls'; +import { render, screen } from 'spec/helpers/testing-library'; + +const setup = (props = {}) => { + const defaultProps = { + name: 'rotation', + value: 'square', + onChange: jest.fn(), + }; + return render(<RotationControl {...defaultProps} {...props} />); +}; + +test('renders rotation control with label', () => { + setup(); + expect(screen.getByText('Word Rotation')).toBeInTheDocument(); +}); + +test('renders select with default value', () => { + setup({ value: 'flat' }); + // Check that the select is rendered (implementation depends on Select component) + expect(screen.getByTestId('rotation')).toBeInTheDocument(); +}); + +test('calls onChange when value changes', () => { Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete onChange test</b></div> <div id="fix"> The test 'calls onChange when value changes' is incomplete with no actual test code or assertions, which means it doesn't verify the `RotationControl` component's onChange behavior. This leaves a gap in test coverage that could allow interaction bugs to go undetected. To fix this, add user interaction simulation using userEvent to change the select value and assert that onChange is called with the expected value, ensuring the component properly handles user inputs. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - import { render, screen } from 'spec/helpers/testing-library'; + import { render, screen, userEvent } from 'spec/helpers/testing-library'; @@ -43,5 +43,10 @@ test('calls onChange when value changes', () => { const onChange = jest.fn(); setup({ onChange }); - // Test onChange is called when select value changes + const user = userEvent.setup(); + const select = screen.getByRole('combobox'); + user.click(select); + const option = screen.getByText('random'); + user.click(option); + expect(onChange).toHaveBeenCalledWith('random'); }); ``` </div> </details> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/3dead35/AGENTS.md#L21";>AGENTS.md:21</a> </li> </ul> </details> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36275#issuecomment-3577590700>#34e1ef</a></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/explore/components/ControlPanelsContainer.tsx: ########## @@ -657,7 +658,29 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { } if (isValidElement(controlItem)) { // When the item is a React element - return controlItem; + // return controlItem; + + const controlName = (controlItem.props as { name: string }) + .name; + const controlState = controlName + ? controls[controlName] + : undefined; + + return cloneElement(controlItem, { + ...(controlItem.props as Record<string, any>), + actions, + controls, + chart, + exploreState, + form_data, + ...(controlState && { + value: controlState.value, + validationErrors: controlState.validationErrors, + default: controlState.default, + onChange: (value: any, errors: any[]) => + setControlValue(controlName, value, errors), + }), + } as any); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>TypeScript `any` type usage violation</b></div> <div id="fix"> The code introduces the use of `any` types in `as Record<string, any>` and `as any` when cloning React elements, which directly violates the critical TypeScript guidelines in the development standards that prohibit `any` types and require proper TypeScript typing. This change in `renderControlPanelSection` clones control elements with additional props like `actions`, `controls`, `chart`, `exploreState`, and `form_data`, but the type assertions bypass TypeScript's safety checks, potentially leading to runtime type errors if prop types don't match. Replacing `any` with `unknown` improves type safety while maintaining functionality, as `unknown` requires explicit type checks before use, aligning with the ongoing modernization toward full TypeScript compliance. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion const controlName = (controlItem.props as { name: string }) .name; const controlState = controlName ? controls[controlName] : undefined; return cloneElement(controlItem, { ...(controlItem.props as Record<string, unknown>), actions, controls, chart, exploreState, form_data, ...(controlState && { value: controlState.value, validationErrors: controlState.validationErrors, default: controlState.default, onChange: (value: unknown, errors: unknown[]) => setControlValue(controlName, value, errors), }), }); ```` </div> </details> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/3dead35/.cursor/rules/dev-standard.mdc#L16";>dev-standard.mdc:16</a> </li> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/3dead35/AGENTS.md#L16";>AGENTS.md:16</a> </li> </ul> </details> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36275#issuecomment-3577590700>#34e1ef</a></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/explore/controlUtils/getControlState.ts: ########## @@ -182,6 +182,14 @@ export function getAllControlsState( state, formData[name], ); + } else if (React.isValidElement(field)) { + const props = field.props as { name: string; [key: string]: any }; + const { name, ...configProps } = props; + controlsState[name] = getControlStateFromControlConfig( + configProps as ControlConfig<any>, + state, + formData[name], + ); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Violation of TypeScript `any` prohibition</b></div> <div id="fix"> The new code handling React elements uses `any` in type assertions, which violates the project's TypeScript standards that explicitly prohibit `any` types. This can lead to unsafe type assumptions and potential runtime errors. Replace `any` with `unknown` to maintain type safety while allowing flexible prop handling. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion } else if (React.isValidElement(field)) { const props = field.props as { name: string; [key: string]: unknown }; const { name, ...configProps } = props; controlsState[name] = getControlStateFromControlConfig( configProps as ControlConfig<unknown>, state, formData[name], ); ```` </div> </details> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/3dead35/.cursor/rules/dev-standard.mdc#L16";>dev-standard.mdc:16</a> </li> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/3dead35/AGENTS.md#L10";>AGENTS.md:10</a> </li> </ul> </details> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36275#issuecomment-3577590700>#34e1ef</a></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-word-cloud/src/plugin/controls/ColorSchemeControl/ColorSchemeLabel.tsx: ########## @@ -0,0 +1,126 @@ +/** + * 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 { css, SupersetTheme } from '@apache-superset/core/ui'; +import { useRef, useState } from 'react'; +import { Tooltip } from '@superset-ui/core/components'; + +type ColorSchemeLabelProps = { + colors: string[]; + id: string; + label: string; +}; + +export default function ColorSchemeLabel(props: ColorSchemeLabelProps) { + const { id, label, colors } = props; + const [showTooltip, setShowTooltip] = useState<boolean>(false); + const labelNameRef = useRef<HTMLElement>(null); + const labelsColorRef = useRef<HTMLElement>(null); + const handleShowTooltip = () => { + const labelNameElement = labelNameRef.current; + const labelsColorElement = labelsColorRef.current; + if ( + labelNameElement && + labelsColorElement && + (labelNameElement.scrollWidth > labelNameElement.offsetWidth || + labelNameElement.scrollHeight > labelNameElement.offsetHeight || + labelsColorElement.scrollWidth > labelsColorElement.offsetWidth || + labelsColorElement.scrollHeight > labelsColorElement.offsetHeight) + ) { + setShowTooltip(true); + } + }; + const handleHideTooltip = () => { + setShowTooltip(false); + }; + + const colorsList = () => + colors.map((color: string, i: number) => ( + <span + data-test="color" + key={`${id}-${i}`} + css={(theme: { sizeUnit: number }) => css` + padding-left: ${theme.sizeUnit / 2}px; + :before { + content: ''; + display: inline-block; + background-color: ${color}; + border: 1px solid ${color === 'white' ? 'black' : color}; + width: 9px; + height: 10px; + } + `} + /> + )); + + const tooltipContent = () => ( + <> + <span>{label}</span> + <div>{colorsList()}</div> + </> + ); + + return ( + <Tooltip + data-testid="tooltip" + overlayClassName="color-scheme-tooltip" + title={tooltipContent} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Tooltip content not rendering due to missing function call</b></div> <div id="fix"> The tooltip's title prop is assigned the `tooltipContent` function directly instead of calling it, preventing the tooltip from rendering its content. This breaks the tooltip functionality when the color scheme label or colors overflow and need to be displayed in full. The `ColorSchemeLabel` component is used in the Select options for color schemes, so this affects users trying to see truncated labels. Fix by invoking the function: `title={tooltipContent()}`. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - title={tooltipContent}, + title={tooltipContent()}, ``` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36275#issuecomment-3577590700>#34e1ef</a></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-word-cloud/test/RotationControl.test.tsx: ########## @@ -0,0 +1,47 @@ +/** + * 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 { RotationControl } from '../src/plugin/controls'; +import { render, screen } from 'spec/helpers/testing-library'; + +const setup = (props = {}) => { + const defaultProps = { + name: 'rotation', + value: 'square', + onChange: jest.fn(), + }; + return render(<RotationControl {...defaultProps} {...props} />); +}; + +test('renders rotation control with label', () => { + setup(); + expect(screen.getByText('Word Rotation')).toBeInTheDocument(); +}); + +test('renders select with default value', () => { + setup({ value: 'flat' }); + // Check that the select is rendered (implementation depends on Select component) + expect(screen.getByTestId('rotation')).toBeInTheDocument(); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Wrong test assertion for select presence</b></div> <div id="fix"> The assertion `screen.getByTestId('rotation')` targets the wrapper div with `data-test="rotation"`, but the Select component itself doesn't have this test ID, causing the test to fail. The `RotationControl` component places the data-test on the outer div, not the Select. To properly verify the select is rendered, use `screen.getByRole('combobox')` instead, which targets the actual Select component and aligns with React Testing Library best practices for accessibility-based queries. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion expect(screen.getByRole('combobox')).toBeInTheDocument(); ```` </div> </details> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/3dead35/AGENTS.md#L21";>AGENTS.md:21</a> </li> </ul> </details> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36275#issuecomment-3577590700>#34e1ef</a></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]
