bito-code-review[bot] commented on code in PR #37141: URL: https://github.com/apache/superset/pull/37141#discussion_r2837407675
########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,344 @@ +/** + * 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 { test, expect, devices } from '@playwright/test'; +import { TIMEOUT } from '../../utils/constants'; + +/** + * Mobile dashboard viewing tests verify that dashboards can be viewed + * and interacted with on mobile devices. + * + * These tests assume the World Bank's Health sample dashboard exists. + */ + +// Use iPhone 12 viewport for mobile tests +const mobileViewport = devices['iPhone 12']; + +test.describe('Mobile Dashboard Viewing', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + test.beforeEach(async ({ page }) => { + // Navigate to dashboard list to find a dashboard + await page.goto('dashboard/list/'); + await page.waitForLoadState('networkidle'); + }); + + test('dashboard list renders in card view on mobile', async ({ page }) => { + // On mobile, dashboard list should show cards, not table + // Look for card elements + const cards = page.locator('[data-test="styled-card"]'); + + // Should have at least one card if dashboards exist + // (This test may need adjustment based on test data availability) + const cardCount = await cards.count(); + + // Either cards are visible, or the empty state is shown + if (cardCount > 0) { + await expect(cards.first()).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + } else { + // No dashboards - that's OK for the test environment + await expect( + page + .getByText('No dashboards yet') + .or(page.locator('[data-test="listview-table"]')), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect test assertion logic</b></div> <div id="fix"> The test assertion allows table view to incorrectly pass by checking for '[data-test="listview-table"]' when no cards are found. This contradicts the test name 'dashboard list renders in card view on mobile'. To fix, remove the or condition so the test strictly verifies card view behavior. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion await expect( page .getByText('No dashboards yet') ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); ```` </div> </details> </div> <small><i>Code Review Run #e92493</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/DashboardBuilder/DashboardBuilder.test.tsx: ########## @@ -521,3 +536,82 @@ test('should maintain layout when switching between tabs', async () => { expect(gridContainer).toBeInTheDocument(); expect(tabPanels.length).toBeGreaterThan(0); }); + +// Mobile support tests +// Note: The main mobile tests require mocking useBreakpoint to return mobile breakpoints +// which is done at the module level. These tests verify mobile-related component behavior. + +test('should not render filter bar panel on desktop when nativeFiltersEnabled is false', () => { + (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [ + 100, + jest.fn(), + ]); + (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' }); + (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' }); + + jest.spyOn(useNativeFiltersModule, 'useNativeFilters').mockReturnValue({ + showDashboard: true, + missingInitialFilters: [], + dashboardFiltersOpen: true, + toggleDashboardFiltersOpen: jest.fn(), + nativeFiltersEnabled: false, + hasFilters: false, + }); + + const { queryByTestId } = render(<DashboardBuilder />, { + useRedux: true, + store: storeWithState({ + ...mockState, + dashboardLayout: undoableDashboardLayout, + }), + useDnd: true, + useTheme: true, + }); + + // Filter panel should not be present when native filters are disabled + expect(queryByTestId('dashboard-filters-panel')).not.toBeInTheDocument(); +}); + +test('should render dashboard content wrapper', () => { + (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [ + 100, + jest.fn(), + ]); + (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' }); + (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' }); + + const { getByTestId } = render(<DashboardBuilder />, { + useRedux: true, + store: storeWithState({ + ...mockState, + dashboardLayout: undoableDashboardLayout, + }), + useDnd: true, + useTheme: true, + }); + + // Dashboard content wrapper should always be present + expect(getByTestId('dashboard-content-wrapper')).toBeInTheDocument(); +}); + +test('should render header container', () => { + (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [ + 100, + jest.fn(), + ]); + (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' }); + (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' }); + + const { queryByTestId } = render(<DashboardBuilder />, { + useRedux: true, + store: storeWithState({ + ...mockState, + dashboardLayout: undoableDashboardLayout, + }), + useDnd: true, + useTheme: true, + }); + + // Header container should be present + expect(queryByTestId('dashboard-header-container')).toBeInTheDocument(); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect test ID causing test failure</b></div> <div id="fix"> The test expects a 'dashboard-header-container' test ID, but the component uses 'dashboard-header-wrapper'. This will cause the test to fail since the element won't be found. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - }); - // Header container should be present - expect(queryByTestId('dashboard-header-container')).toBeInTheDocument(); + }); - // Header container should be present + expect(queryByTestId('dashboard-header-wrapper')).toBeInTheDocument(); ``` </div> </details> </div> <small><i>Code Review Run #e92493</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/MobileRouteGuard/MobileRouteGuard.test.tsx: ########## @@ -0,0 +1,124 @@ +/** + * 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 MobileRouteGuard, { + isMobileSupportedRoute, + MOBILE_SUPPORTED_ROUTES, +} from './index'; + +// Store the original sessionStorage +const originalSessionStorage = window.sessionStorage; + +// Clean up sessionStorage before each test +beforeEach(() => { + sessionStorage.clear(); + jest.clearAllMocks(); +}); + +afterEach(() => { + // Restore sessionStorage if it was mocked + Object.defineProperty(window, 'sessionStorage', { + value: originalSessionStorage, + writable: true, + }); +}); + +// Unit tests for isMobileSupportedRoute helper function +test('isMobileSupportedRoute returns true for dashboard list', () => { + expect(isMobileSupportedRoute('/dashboard/list/')).toBe(true); + expect(isMobileSupportedRoute('/dashboard/list')).toBe(true); +}); + +test('isMobileSupportedRoute returns true for individual dashboards', () => { + expect(isMobileSupportedRoute('/superset/dashboard/123/')).toBe(true); + expect(isMobileSupportedRoute('/superset/dashboard/my-dashboard/')).toBe( + true, + ); + expect(isMobileSupportedRoute('/superset/dashboard/abc-123/')).toBe(true); +}); + +test('isMobileSupportedRoute returns true for welcome page', () => { + expect(isMobileSupportedRoute('/superset/welcome/')).toBe(true); + expect(isMobileSupportedRoute('/superset/welcome')).toBe(true); +}); + +test('isMobileSupportedRoute returns true for auth routes', () => { + expect(isMobileSupportedRoute('/login/')).toBe(true); + expect(isMobileSupportedRoute('/logout/')).toBe(true); + expect(isMobileSupportedRoute('/register/')).toBe(true); +}); + +test('isMobileSupportedRoute returns false for chart routes', () => { + expect(isMobileSupportedRoute('/chart/list/')).toBe(false); + expect(isMobileSupportedRoute('/explore/')).toBe(false); + expect(isMobileSupportedRoute('/superset/explore/')).toBe(false); +}); + +test('isMobileSupportedRoute returns false for SQL Lab', () => { + expect(isMobileSupportedRoute('/sqllab/')).toBe(false); + expect(isMobileSupportedRoute('/superset/sqllab/')).toBe(false); +}); + +test('isMobileSupportedRoute returns false for database/dataset routes', () => { + expect(isMobileSupportedRoute('/database/list/')).toBe(false); + expect(isMobileSupportedRoute('/dataset/list/')).toBe(false); +}); + +test('isMobileSupportedRoute strips query params and hash', () => { + expect(isMobileSupportedRoute('/dashboard/list/?page=1')).toBe(true); + expect(isMobileSupportedRoute('/dashboard/list/#section')).toBe(true); + expect(isMobileSupportedRoute('/chart/list/?page=1')).toBe(false); +}); + +test('MOBILE_SUPPORTED_ROUTES includes expected patterns', () => { + // Verify the constant is exported and has expected patterns + expect(MOBILE_SUPPORTED_ROUTES).toBeInstanceOf(Array); + expect(MOBILE_SUPPORTED_ROUTES.length).toBeGreaterThan(0); + + // Check some patterns exist + const hasLoginPattern = MOBILE_SUPPORTED_ROUTES.some(p => p.test('/login/')); + const hasDashboardListPattern = MOBILE_SUPPORTED_ROUTES.some(p => + p.test('/dashboard/list/'), + ); + const hasWelcomePattern = MOBILE_SUPPORTED_ROUTES.some(p => + p.test('/superset/welcome/'), + ); + + expect(hasLoginPattern).toBe(true); + expect(hasDashboardListPattern).toBe(true); + expect(hasWelcomePattern).toBe(true); +}); + +// Integration tests for MobileRouteGuard component +// Note: These tests require mocking at the module level which is complex +// The tests below verify the component structure and exports + +test('MobileRouteGuard exports the component as default', () => { + expect(MobileRouteGuard).toBeDefined(); + expect(typeof MobileRouteGuard).toBe('function'); +}); + +test('isMobileSupportedRoute is exported', () => { + expect(isMobileSupportedRoute).toBeDefined(); + expect(typeof isMobileSupportedRoute).toBe('function'); +}); + +test('MOBILE_SUPPORTED_ROUTES is exported', () => { + expect(MOBILE_SUPPORTED_ROUTES).toBeDefined(); + expect(Array.isArray(MOBILE_SUPPORTED_ROUTES)).toBe(true); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing Regex Anchoring Tests</b></div> <div id="fix"> Tests lack edge cases for regex anchoring. Without checks for extended paths, bugs like missing $ anchors in patterns could go undetected. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion }); test('isMobileSupportedRoute does not match extended paths', () => { expect(isMobileSupportedRoute('/dashboard/list/extra')).toBe(false); expect(isMobileSupportedRoute('/login/something')).toBe(false); expect(isMobileSupportedRoute('/superset/welcome/more')).toBe(false); }); ```` </div> </details> </div> <small><i>Code Review Run #e92493</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/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx: ########## @@ -81,6 +81,15 @@ const headerStyles = (theme: SupersetTheme) => css` display: flex; align-items: center; } + + /* Mobile: center the title between left and right panels */ + @media (max-width: 767px) { + .title-panel { + flex: 1; + justify-content: center; + margin-right: 0; + } + } Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Mobile layout broken with leftPanelItems</b></div> <div id="fix"> The mobile responsive design intends to center the title between left and right panels, but the current flex layout doesn't account for the new leftPanelItems prop. When leftPanelItems is provided, the title won't be centered as intended. Switching to CSS grid on mobile ensures proper centering regardless of leftPanelItems presence. </div> </div> <small><i>Code Review Run #e92493</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]
