bito-code-review[bot] commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r3615672059
##########
superset-frontend/src/pages/MobileUnsupported/MobileUnsupported.test.tsx:
##########
@@ -0,0 +1,172 @@
+/**
Review Comment:
<!-- Bito Reply -->
Understood. Since the viewport logic now handles the route unblocking
automatically, the previous test requirement is no longer applicable. I have
noted that this suggestion is resolved by the recent code changes.
##########
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:
<!-- Bito Reply -->
The suggestion provided by the reviewer is incorrect because it replaces a
test ID that is already in use and expected by the existing test suite. You
should ignore this suggestion, as the original test ID is correct for the
component being tested.
**superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx**
```
- // Header container should be present
- expect(queryByTestId('dashboard-header-container')).toBeInTheDocument();
+ // Header container should be present
+ expect(queryByTestId('dashboard-header-wrapper')).toBeInTheDocument();
```
##########
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:
<!-- Bito Reply -->
The suggestion is appropriate for testing the robustness of the route
matching logic. By explicitly testing extended paths that should not match, you
ensure that the route guard correctly anchors its checks and does not
inadvertently allow unauthorized sub-paths.
**superset-frontend/src/components/MobileRouteGuard/MobileRouteGuard.test.tsx**
```
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);
});
```
##########
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:
<!-- Bito Reply -->
The suggestion provided by the reviewer is appropriate to consider. Using
CSS grid on mobile is a robust way to ensure the title remains centered
regardless of the presence of additional elements like leftPanelItems, which
may not be fully accounted for by the current flexbox implementation.
**superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx**
```
@media (max-width: 767px) {
.title-panel {
flex: 1;
justify-content: center;
margin-right: 0;
}
}
```
##########
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:
<!-- Bito Reply -->
The suggestion provided by the reviewer is appropriate for the context. It
correctly addresses the issue by ensuring the test strictly verifies the
expected state, preventing potential false positives where the test might pass
incorrectly if the table view is present. Applying this change improves the
reliability of the test.
**superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts**
```
await expect(
page
.getByText('No dashboards yet')
).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD });
```
##########
superset-frontend/src/components/ListView/ListView.test.tsx:
##########
@@ -350,3 +350,225 @@ 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(
+ <MemoryRouter>
+ <QueryParamProvider adapter={ReactRouter5Adapter}>
+ <ListView
+ {...propsWithoutSort}
+ renderCard={() => <div>Card</div>}
+ forceViewMode="card"
+ />
+ </QueryParamProvider>
+ </MemoryRouter>,
+ { 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();
+});
+
+test('shows card view when forceViewMode is card', () => {
+ // Omit cardSortSelectOptions to avoid CardSortSelect needing initialSort
+ const { cardSortSelectOptions, ...propsWithoutSort } =
+ mockedPropsComprehensive;
+ render(
+ <MemoryRouter>
+ <QueryParamProvider adapter={ReactRouter5Adapter}>
+ <ListView
+ {...propsWithoutSort}
+ renderCard={() => <div data-test="test-card">Card Content</div>}
+ forceViewMode="card"
+ />
+ </QueryParamProvider>
+ </MemoryRouter>,
+ { store: mockStore() },
+ );
+
+ // Should render cards, not table rows
+ expect(screen.getAllByTestId('test-card')).toHaveLength(2);
+});
+
+test('renders mobile filter drawer when mobileFiltersOpen is true', () => {
+ const setMobileFiltersOpen = jest.fn();
+ // Omit cardSortSelectOptions to avoid CardSortSelect needing initialSort
+ const { cardSortSelectOptions, ...propsWithoutSort } =
+ mockedPropsComprehensive;
+ render(
+ <MemoryRouter>
+ <QueryParamProvider adapter={ReactRouter5Adapter}>
+ <ListView
+ {...propsWithoutSort}
+ mobileFiltersOpen
+ setMobileFiltersOpen={setMobileFiltersOpen}
+ mobileFiltersDrawerTitle="Search Dashboards"
+ />
+ </QueryParamProvider>
+ </MemoryRouter>,
+ { store: mockStore() },
+ );
+
+ // Drawer should be visible with custom title
+ expect(screen.getByText('Search Dashboards')).toBeInTheDocument();
+});
+
+test('calls setMobileFiltersOpen(false) when drawer is closed', async () => {
+ const setMobileFiltersOpen = jest.fn();
+ // Omit cardSortSelectOptions to avoid CardSortSelect needing initialSort
+ const { cardSortSelectOptions, ...propsWithoutSort } =
+ mockedPropsComprehensive;
+ render(
+ <MemoryRouter>
+ <QueryParamProvider adapter={ReactRouter5Adapter}>
+ <ListView
+ {...propsWithoutSort}
+ mobileFiltersOpen
+ setMobileFiltersOpen={setMobileFiltersOpen}
+ mobileFiltersDrawerTitle="Search"
+ />
+ </QueryParamProvider>
+ </MemoryRouter>,
+ { store: mockStore() },
+ );
+
+ // Click the close button on the drawer
+ const closeButton = screen.getByLabelText('Close');
+ await userEvent.click(closeButton);
+
+ expect(setMobileFiltersOpen).toHaveBeenCalledWith(false);
+});
+
+test('mobile drawer contains FilterControls', () => {
+ const setMobileFiltersOpen = jest.fn();
+ const { cardSortSelectOptions, ...propsWithoutSort } =
+ mockedPropsComprehensive;
+ render(
+ <MemoryRouter>
+ <QueryParamProvider adapter={ReactRouter5Adapter}>
+ <ListView
+ {...propsWithoutSort}
+ mobileFiltersOpen
+ setMobileFiltersOpen={setMobileFiltersOpen}
+ />
+ </QueryParamProvider>
+ </MemoryRouter>,
+ { store: mockStore() },
+ );
+
+ // The drawer should contain filter controls (comboboxes for select filters)
+ const comboboxes = screen.getAllByRole('combobox');
+ expect(comboboxes.length).toBeGreaterThan(0);
+});
+
+test('mobile drawer contains CardSortSelect when in card view with sort
options', () => {
+ const setMobileFiltersOpen = jest.fn();
+ render(
+ <MemoryRouter>
+ <QueryParamProvider adapter={ReactRouter5Adapter}>
+ <ListView
+ {...mockedPropsComprehensive}
+ renderCard={() => <div>Card</div>}
+ forceViewMode="card"
+ mobileFiltersOpen
+ setMobileFiltersOpen={setMobileFiltersOpen}
+ initialSort={[{ id: 'something' }]}
+ />
+ </QueryParamProvider>
+ </MemoryRouter>,
+ { store: mockStore() },
+ );
+
+ // Sort select should be present (may be multiple - one in drawer, one in
header)
+ const sortSelects = screen.getAllByTestId('card-sort-select');
+ expect(sortSelects.length).toBeGreaterThan(0);
+});
+
+test('uses default drawer title when mobileFiltersDrawerTitle not provided',
() => {
+ const setMobileFiltersOpen = jest.fn();
+ const { cardSortSelectOptions, ...propsWithoutSort } =
+ mockedPropsComprehensive;
+ render(
+ <MemoryRouter>
+ <QueryParamProvider adapter={ReactRouter5Adapter}>
+ <ListView
+ {...propsWithoutSort}
+ mobileFiltersOpen
+ setMobileFiltersOpen={setMobileFiltersOpen}
+ />
+ </QueryParamProvider>
+ </MemoryRouter>,
+ { store: mockStore() },
+ );
+
+ // Default title should be 'Search'
+ expect(screen.getByText('Search')).toBeInTheDocument();
+});
+
+test('does not render drawer when mobileFiltersOpen is false', () => {
+ const setMobileFiltersOpen = jest.fn();
+ const { cardSortSelectOptions, ...propsWithoutSort } =
+ mockedPropsComprehensive;
+ render(
+ <MemoryRouter>
+ <QueryParamProvider adapter={ReactRouter5Adapter}>
+ <ListView
+ {...propsWithoutSort}
+ mobileFiltersOpen={false}
+ setMobileFiltersOpen={setMobileFiltersOpen}
+ mobileFiltersDrawerTitle="Search"
+ />
+ </QueryParamProvider>
+ </MemoryRouter>,
+ { store: mockStore() },
+ );
+
+ // Drawer should not be visible (title not in visible content)
+ // Note: Ant Design drawer might still be in DOM but hidden
+ const drawer = document.querySelector('.ant-drawer-open');
+ expect(drawer).toBeNull();
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion is correct. In Ant Design, the `Drawer` component
remains in the DOM even when closed, so checking for the absence of the
`.ant-drawer-open` class is not a reliable way to verify that the drawer is
hidden. Using a visibility-based assertion, such as checking that the drawer
content is not in the document or is not visible to the user, is the
recommended approach for testing component visibility.
##########
superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx:
##########
@@ -106,6 +111,11 @@ export const useHeaderActionsMenu = ({
case MenuKeys.ManageEmbedded:
manageEmbedded();
break;
+ case 'toggle-favorite':
+ if (saveFaveStar && isStarred !== undefined) {
+ saveFaveStar(dashboardId, isStarred);
+ }
+ break;
Review Comment:
<!-- Bito Reply -->
The suggestion to invert the `isStarred` value when calling `saveFaveStar`
is correct. The current implementation passes the current state, which prevents
the toggle action from functioning as intended. You should update the call to
pass `!isStarred` to ensure the state is correctly toggled.
**superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx**
```
case 'toggle-favorite':
if (saveFaveStar && isStarred !== undefined) {
saveFaveStar(dashboardId, !isStarred);
}
break;
```
--
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]
