bito-code-review[bot] commented on code in PR #37141: URL: https://github.com/apache/superset/pull/37141#discussion_r3655078561
########## superset-frontend/src/pages/MobileUnsupported/index.tsx: ########## @@ -0,0 +1,143 @@ +/** + * 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 { useCallback } from 'react'; +import { useHistory } from 'react-router-dom'; +import { t } from '@apache-superset/core/translation'; +import { css, useTheme } from '@apache-superset/core/theme'; +import { Button } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; + +/** + * A mobile-friendly page shown when users try to access + * features that aren't supported on mobile devices. Growing the window + * past the mobile breakpoint unblocks the route automatically (useIsMobile + * subscribes to the breakpoint), so no manual bypass is offered. + */ +function MobileUnsupported() { + const theme = useTheme(); + const history = useHistory(); + + const handleViewDashboards = useCallback(() => { + history.push('/dashboard/list/'); + }, [history]); + + const handleGoHome = useCallback(() => { + history.push('/welcome/'); + }, [history]); + + return ( + <div + css={css` + display: flex; + flex-direction: column; + align-items: center; + justify-content: center; + min-height: calc(100vh - 60px); + padding: ${theme.sizeUnit * 6}px; + text-align: center; + background: ${theme.colorBgContainer}; + `} + > + {/* Icon */} + <div + css={css` + width: 120px; + height: 120px; + border-radius: 50%; + background: ${theme.colorBgLayout}; + display: flex; + align-items: center; + justify-content: center; + margin-bottom: ${theme.sizeUnit * 6}px; + `} + > + <Icons.DesktopOutlined + iconSize="xxl" + iconColor={theme.colorTextSecondary} + css={css` + font-size: 48px; + `} + /> + </div> + + {/* Title */} + <h1 + css={css` + font-size: ${theme.fontSizeXL}px; + font-weight: ${theme.fontWeightStrong}; + color: ${theme.colorText}; + margin: 0 0 ${theme.sizeUnit * 2}px 0; + `} + > + {t("This view isn't available on mobile")} + </h1> + + {/* Description */} + <p + css={css` + font-size: ${theme.fontSizeSM}px; + color: ${theme.colorTextSecondary}; + margin: 0 0 ${theme.sizeUnit * 8}px 0; + max-width: 300px; + line-height: 1.5; + `} + > + {t( + 'Some features require a larger screen. Try viewing dashboards for the best mobile experience.', + )} + </p> + + {/* Primary action */} + <div> + <Button + buttonStyle="primary" + onClick={handleViewDashboards} + css={css` + width: 280px; + height: 48px; + font-size: ${theme.fontSizeSM}px; + `} + > + {t('View Dashboards')} + </Button> + </div> Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Semantic duplication in CSS</b></div> <div id="fix"> Lines 113-115 and 129-133 contain identical CSS for both buttons (`width: 280px`, `height: 48px`, `font-size`). This duplication increases maintenance burden — if either button's dimensions change, both must be updated. Consider extracting into a constant or reusing a styled button variant. </div> </div> <small><i>Code Review Run #ee4452</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/Header/Header.test.tsx: ########## @@ -186,6 +186,15 @@ const recordError = jest.fn(); const setPaused = jest.fn(); const setPausedByTab = jest.fn(); +// Mock useBreakpoint to return desktop breakpoints (prevents mobile rendering) +jest.mock('antd', () => ({ + ...jest.requireActual('antd'), + Grid: { + ...jest.requireActual('antd').Grid, + useBreakpoint: () => ({ xs: true, sm: true, md: true, lg: true, xl: true }), + }, +})); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Use mobileTestUtils helper for antd mock</b></div> <div id="fix"> This mock duplicates the `mockAntdWithDesktopBreakpoint` helper already exported from `spec/helpers/mobileTestUtils.ts` (lines 85-91), which is the established pattern used by `Home.test.tsx`, `DashboardList.test.tsx`, and other test files. Reuse the helper for consistency. Also note the mock is missing the `xxl` breakpoint that `desktopBreakpoints` in the helper includes — if the component references `xxl` (via antd v5's `Grid.useBreakpoint` behavior), this gap can cause rendering divergence between tests and production. </div> </div> <small><i>Code Review Run #ee4452</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/components/ListView/ListView.test.tsx: ########## @@ -359,3 +359,84 @@ describe('ListView', () => { expect(mockedPropsComprehensive.fetchData).toHaveBeenCalled(); }); }); + +// Mobile support tests +test('respects forceViewMode prop and hides view toggle', () => { + // Omit cardSortSelectOptions to avoid CardSortSelect needing initialSort + const { cardSortSelectOptions, ...propsWithoutSort } = mockedPropsComprehensive; + render( + <QueryParamProvider location={makeMockLocation()}> + <ListView + {...propsWithoutSort} + renderCard={() => <div>Card</div>} + forceViewMode="card" + /> + </QueryParamProvider>, + { store: mockStore() }, + ); + + // View toggle should not be present when forceViewMode is set + expect(screen.queryByLabelText('card-view')).not.toBeInTheDocument(); + expect(screen.queryByLabelText('list-view')).not.toBeInTheDocument(); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Test expects hidden toggle</b></div> <div id="fix"> The test verifies that the view toggle is hidden when forceViewMode is set, but the current ListView implementation renders the toggle regardless of this prop. This mismatch will cause the test to fail. The toggle should not be shown when the view mode is forced to avoid user confusion. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - {cardViewEnabled && ( - <ViewModeToggle mode={viewMode} setMode={setViewMode} /> - )} + {cardViewEnabled && !forceViewMode && ( + <ViewModeToggle mode={viewMode} setMode={setViewMode} /> + )} ``` </div> </details> </div> <small><i>Code Review Run #9148bb</i></small> </div><div> <div id="suggestion"> <div id="issue"><b>Misleading aria-label test selectors</b></div> <div id="fix"> The ViewModeToggle component renders buttons with `aria-pressed` but no `aria-label`. Tests use `queryByLabelText('card-view')` and `queryByLabelText('list-view')` which will find no elements, causing this assertion to always pass vacuously. Either use `queryAllByRole('button')` and filter by `aria-pressed` attribute, or add `aria-label` props to the toggle buttons in `ViewModeToggle`. </div> </div> <small><i>Code Review Run #3c89a6</i></small> </div><div> <div id="suggestion"> <div id="issue"><b>Invalid test selector for view toggle</b></div> <div id="fix"> Test selectors at lines 403-404 use `queryByLabelText('card-view')` and `queryByLabelText('list-view')`, but the toggle buttons in ListView.tsx (lines 289-312) have no aria-label attributes. They only have `aria-pressed` for state and tooltip titles. The tests will return null instead of the expected elements. Use `queryByRole('button', { pressed: true })` or add explicit testids/aria-labels to the buttons. </div> </div> <small><i>Code Review Run #ee4452</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/hooks/useIsMobile.ts: ########## @@ -0,0 +1,71 @@ +/** + * 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 { useEffect, useState } from 'react'; +import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core'; +import { useTheme } from '@apache-superset/core/theme'; + +// Matches antd's screenSMMax token; used only when no theme is in scope. +const FALLBACK_MOBILE_MAX_WIDTH = 767; + +/** + * Whether the mobile consumption-only experience is enabled for this + * deployment. Non-hook variant for use inside styled-component + * interpolations; prefer `useIsMobile` in components. + */ +export function isMobileConsumptionEnabled(): boolean { + return isFeatureEnabled(FeatureFlag.MobileConsumptionMode); +} + +/** + * Returns true when MOBILE_CONSUMPTION_MODE is enabled AND the viewport is + * at or below the theme's `screenSMMax` breakpoint. All mobile-specific + * behavior (route guarding, consumption-only chrome, drawer navigation) + * should key off this hook so the flag remains a single kill switch. + * + * The matchMedia subscription is only installed when the flag is on, and + * state only changes when the match flips, so with the flag off (or on + * desktop) this hook never causes a re-render — consumers are inert. + * + * The initial value is always false (desktop), so the first paint never + * takes the mobile branch by accident. + */ +export function useIsMobile(): boolean { + const enabled = isMobileConsumptionEnabled(); + const theme = useTheme(); + const maxWidth = theme?.screenSMMax ?? FALLBACK_MOBILE_MAX_WIDTH; + const [isSmallScreen, setIsSmallScreen] = useState(false); + + useEffect(() => { + if (!enabled) { + return undefined; + } + const mediaQuery = window.matchMedia(`(max-width: ${maxWidth}px)`); + const update = () => setIsSmallScreen(mediaQuery.matches); + update(); + // Safari < 14 lacks addEventListener on MediaQueryList + if (mediaQuery.addEventListener) { + mediaQuery.addEventListener('change', update); + return () => mediaQuery.removeEventListener('change', update); + } + mediaQuery.addListener(update); + return () => mediaQuery.removeListener(update); + }, [enabled, maxWidth]); + + return enabled && isSmallScreen; +} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing unit tests for new hook</b></div> <div id="fix"> Add unit tests for this new hook per project guidelines. The hook handles stateful viewport tracking with a feature flag kill switch and theme-dependent breakpoint, so tests should cover all execution paths: flag enabled/disabled, media query match/not-match, theme changes mid-lifecycle, and the inert initial state guarantee. </div> </div> <small><i>Code Review Run #3c89a6</i></small> </div><div> <div id="suggestion"> <div id="issue"><b>Missing unit tests for new hook</b></div> <div id="fix"> This new hook exposes a public API but has no unit tests. Rule [11730] requires comprehensive unit tests for new tools covering success paths, error scenarios, validation failures, and edge cases. The test helpers in `spec/helpers/mobileTestUtils.ts` already provide mock infrastructure for `matchMedia` — use them to test the `enabled`/`isSmallScreen` matrix. </div> </div> <small><i>Code Review Run #ee4452</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]
