bito-code-review[bot] commented on code in PR #35001: URL: https://github.com/apache/superset/pull/35001#discussion_r2320505530
########## superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.tsx: ########## @@ -0,0 +1,120 @@ +/** + * 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 { memo, ReactElement, RefObject } from 'react'; +import { styled } from '@superset-ui/core'; +import { + LineEditableTabs, + TabsProps as AntdTabsProps, +} from '@superset-ui/core/components/Tabs'; +import HoverMenu from '../../menu/HoverMenu'; +import DragHandle from '../../dnd/DragHandle'; +import DeleteComponentButton from '../../DeleteComponentButton'; + +const StyledTabsContainer = styled.div` + width: 100%; + background-color: ${({ theme }) => theme.colorBgContainer}; + + & .dashboard-component-tabs-content { + height: 100%; + } + + & > .hover-menu:hover { + opacity: 1; + } + + &.dragdroppable-row .dashboard-component-tabs-content { + height: calc(100% - 47px); + } +`; + +export interface TabItem { + key: string; + label: ReactElement; + closeIcon: ReactElement; + children: ReactElement; +} + +export interface TabsComponent { + id: string; +} + +export interface TabsRendererProps { + tabItems: TabItem[]; + editMode: boolean; + renderHoverMenu?: boolean; + tabsDragSourceRef?: RefObject<HTMLDivElement>; + handleDeleteComponent: () => void; + tabsComponent: TabsComponent; + activeKey: string; + tabIds: string[]; + handleClickTab: (index: number) => void; + handleEdit: AntdTabsProps['onEdit']; + tabBarPaddingLeft?: number; +} + +/** + * TabsRenderer component handles the rendering of dashboard tabs + * Extracted from the main Tabs component for better separation of concerns + */ +const TabsRenderer = memo<TabsRendererProps>( + ({ + tabItems, + editMode, + renderHoverMenu = true, + tabsDragSourceRef, + handleDeleteComponent, + tabsComponent, + activeKey, + tabIds, + handleClickTab, + handleEdit, + tabBarPaddingLeft = 0, + }) => ( + <StyledTabsContainer + className="dashboard-component dashboard-component-tabs" + data-test="dashboard-component-tabs" + > + {editMode && renderHoverMenu && tabsDragSourceRef && ( + <HoverMenu innerRef={tabsDragSourceRef} position="left"> + <DragHandle position="left" /> + <DeleteComponentButton onDelete={handleDeleteComponent} /> + </HoverMenu> + )} + + <LineEditableTabs + id={tabsComponent.id} + activeKey={activeKey} + onChange={key => { + if (typeof key === 'string') { + handleClickTab(tabIds.indexOf(key)); + } + }} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing validation for tab key lookup</b></div> <div id="fix"> The `onChange` handler calls `handleClickTab(tabIds.indexOf(key))` without checking if the key exists in `tabIds`. When `indexOf` returns -1 (key not found), this passes an invalid index to `handleClickTab`, potentially causing runtime errors or unexpected behavior in tab navigation. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ```suggestion onChange={key => { if (typeof key === 'string') { const tabIndex = tabIds.indexOf(key); if (tabIndex !== -1) { handleClickTab(tabIndex); } } }} ``` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35001#issuecomment-3251162838>#5a24f7</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/dashboard/components/BuilderComponentPane/index.tsx: ########## @@ -37,81 +39,96 @@ const TABS_KEYS = { LAYOUT_ELEMENTS: 'LAYOUT_ELEMENTS', }; -const BuilderComponentPane = ({ topOffset = 0 }) => ( - <div - data-test="dashboard-builder-sidepane" - css={css` - position: sticky; - right: 0; - top: ${topOffset}px; - height: calc(100vh - ${topOffset}px); - width: ${BUILDER_PANE_WIDTH}px; - `} - > +const BuilderComponentPane = ({ topOffset = 0 }) => { + const theme = useTheme(); + const nativeFiltersBarOpen = useSelector( + (state: any) => state.dashboardState.nativeFiltersBarOpen ?? false, + ); + + const tabBarStyle = useMemo( + () => ({ + paddingLeft: nativeFiltersBarOpen ? 0 : theme.sizeUnit * 4, + }), + [nativeFiltersBarOpen, theme.sizeUnit], + ); + + return ( <div - css={(theme: SupersetTheme) => css` - position: absolute; - height: 100%; + data-test="dashboard-builder-sidepane" + css={css` + position: sticky; + right: 0; + top: ${topOffset}px; + height: calc(100vh - ${topOffset}px); width: ${BUILDER_PANE_WIDTH}px; - box-shadow: -4px 0 4px 0 ${rgba(theme.colorBorder, 0.1)}; - background-color: ${theme.colorBgBase}; `} > - <Tabs - data-test="dashboard-builder-component-pane-tabs-navigation" - id="tabs" + <div css={(theme: SupersetTheme) => css` - line-height: inherit; - margin-top: ${theme.sizeUnit * 2}px; + position: absolute; height: 100%; - - & .ant-tabs-content-holder { + width: ${BUILDER_PANE_WIDTH}px; + box-shadow: -4px 0 4px 0 ${rgba(theme.colorBorder, 0.1)}; + background-color: ${theme.colorBgBase}; + `} + > + <Tabs + data-test="dashboard-builder-component-pane-tabs-navigation" + id="tabs" + tabBarStyle={tabBarStyle} + css={(theme: SupersetTheme) => css` + line-height: inherit; + margin-top: ${theme.sizeUnit * 2}px; height: 100%; - & .ant-tabs-content { + + & .ant-tabs-content-holder { height: 100%; + & .ant-tabs-content { + height: 100%; + } } - } - `} - items={[ - { - key: TABS_KEYS.CHARTS, - label: t('Charts'), - children: ( - <div - css={css` - height: calc(100vh - ${topOffset * 2}px); - `} - > - <SliceAdder /> - </div> - ), - }, - { - key: TABS_KEYS.LAYOUT_ELEMENTS, - label: t('Layout elements'), - children: ( - <> - <NewTabs /> - <NewRow /> - <NewColumn /> - <NewHeader /> - <NewMarkdown /> - <NewDivider /> - {dashboardComponents - .getAll() - .map(({ key: componentKey, metadata }) => ( - <NewDynamicComponent - metadata={metadata} - componentKey={componentKey} - /> - ))} - </> - ), - }, - ]} - /> + `} + items={[ + { + key: TABS_KEYS.CHARTS, + label: t('Charts'), + children: ( + <div + css={css` + height: calc(100vh - ${topOffset * 2}px); + `} + > + <SliceAdder /> + </div> + ), + }, + { + key: TABS_KEYS.LAYOUT_ELEMENTS, + label: t('Layout elements'), + children: ( + <> + <NewTabs /> + <NewRow /> + <NewColumn /> + <NewHeader /> + <NewMarkdown /> + <NewDivider /> + {dashboardComponents + .getAll() + .map(({ key: componentKey, metadata }) => ( + <NewDynamicComponent + metadata={metadata} + componentKey={componentKey} + /> Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing React key prop in map</b></div> <div id="fix"> Missing `key` prop in React map function. Add `key={componentKey}` to `NewDynamicComponent` to prevent React warnings and ensure proper component reconciliation during re-renders. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ```suggestion {dashboardComponents .getAll() .map(({ key: componentKey, metadata }) => ( <NewDynamicComponent key={componentKey} metadata={metadata} componentKey={componentKey} /> ``` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35001#issuecomment-3251162838>#5a24f7</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/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.test.tsx: ########## @@ -0,0 +1,169 @@ +/** + * 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 } from 'spec/helpers/testing-library'; +import TabsRenderer, { TabItem, TabsRendererProps } from './TabsRenderer'; + +const mockTabItems: TabItem[] = [ + { + key: 'tab-1', + label: <div>Tab 1</div>, + closeIcon: <div>×</div>, + children: <div>Tab 1 Content</div>, + }, + { + key: 'tab-2', + label: <div>Tab 2</div>, + closeIcon: <div>×</div>, + children: <div>Tab 2 Content</div>, + }, +]; + +const mockProps: TabsRendererProps = { + tabItems: mockTabItems, + editMode: false, + renderHoverMenu: true, + tabsDragSourceRef: undefined, + handleDeleteComponent: jest.fn(), + tabsComponent: { id: 'test-tabs-id' }, + activeKey: 'tab-1', + tabIds: ['tab-1', 'tab-2'], + handleClickTab: jest.fn(), + handleEdit: jest.fn(), + tabBarPaddingLeft: 16, +}; + +describe('TabsRenderer', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + test('renders tabs container with correct test attributes', () => { + render(<TabsRenderer {...mockProps} />); + + const tabsContainer = screen.getByTestId('dashboard-component-tabs'); + + expect(tabsContainer).toBeInTheDocument(); + expect(tabsContainer).toHaveClass('dashboard-component-tabs'); + }); + + test('renders LineEditableTabs with correct props', () => { + render(<TabsRenderer {...mockProps} />); + + const editableTabs = screen.getByTestId('nav-list'); + expect(editableTabs).toBeInTheDocument(); + }); + + test('applies correct tab bar padding', () => { + const { rerender } = render(<TabsRenderer {...mockProps} />); + + expect(screen.getByTestId('nav-list')).toBeInTheDocument(); + + rerender(<TabsRenderer {...mockProps} tabBarPaddingLeft={0} />); + expect(screen.getByTestId('nav-list')).toBeInTheDocument(); + }); + + test('calls handleClickTab when tab is clicked', () => { + render(<TabsRenderer {...mockProps} />); + expect(mockProps.handleClickTab).not.toHaveBeenCalled(); + }); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete tab click test implementation</b></div> <div id="fix"> Test claims to verify tab click behavior but doesn't actually click any tab. It only checks that the handler wasn't called, which doesn't test the intended functionality. Add actual tab click simulation using `fireEvent.click()` and verify the handler is called with correct arguments. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ```suggestion test('calls handleClickTab when tab is clicked', () => { render(<TabsRenderer {...mockProps} />); const tabElement = screen.getByRole('tab', { name: /tab 1/i }); fireEvent.click(tabElement); expect(mockProps.handleClickTab).toHaveBeenCalledWith(0); }); ``` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35001#issuecomment-3251162838>#5a24f7</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]
