codeant-ai-for-open-source[bot] commented on code in PR #37141: URL: https://github.com/apache/superset/pull/37141#discussion_r3651467971
########## superset-frontend/playwright/generators/docs/mobile-screenshots.spec.ts: ########## @@ -0,0 +1,167 @@ +/** + * 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. + */ + +/** + * Mobile Experience Documentation Screenshot Generator + * + * Captures phone-sized screenshots for the mobile consumption mode docs + * (docs/docs/using-superset/mobile-experience.mdx). Depends on example data + * loaded via `superset load_examples` AND the MOBILE_CONSUMPTION_MODE + * feature flag being enabled in the target environment: + * + * FEATURE_FLAGS = {"MOBILE_CONSUMPTION_MODE": True} + * + * Run locally: + * cd superset-frontend + * PLAYWRIGHT_BASE_URL=http://localhost:8088 PLAYWRIGHT_ADMIN_PASSWORD=admin npm run docs:screenshots + * + * Screenshots are saved under docs/static/img/screenshots/mobile/. + */ + +import path from 'path'; +import { Page, test, expect } from '@playwright/test'; +import { URL } from '../../utils/urls'; + +const MOBILE_SCREENSHOTS_DIR = path.resolve( + __dirname, + '../../../../docs/static/img/screenshots/mobile', +); + +// iPhone 12-class viewport; 2x scale factor for crisp docs images +test.use({ + viewport: { width: 390, height: 844 }, + deviceScaleFactor: 2, + hasTouch: true, +}); Review Comment: **Suggestion:** The screenshot tests do not enable `MOBILE_CONSUMPTION_MODE`; with the documented default-off configuration, the dashboard, navigation, and unsupported-route screenshots will capture the regular desktop behavior or fail their mobile-specific assertions. Configure the test context or runner to enable the flag rather than relying only on a comment describing an external prerequisite. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Documentation screenshot generation fails with default flags. - ❌ Mobile screenshots capture non-mobile behavior. - ⚠️ CI or release docs require manual flag setup. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the documentation screenshot suite through the command documented at `superset-frontend/playwright/generators/docs/mobile-screenshots.spec.ts:30-32` against a Superset instance using the feature flag default, where `MOBILE_CONSUMPTION_MODE` is disabled. 2. Playwright applies only the phone viewport and touch settings from `test.use()` at `superset-frontend/playwright/generators/docs/mobile-screenshots.spec.ts:47-51`; it does not enable the feature flag. 3. The `mobile unsupported route screenshot` test navigates to `URL.SQLLAB` at `superset-frontend/playwright/generators/docs/mobile-screenshots.spec.ts:157-160`, but `MobileRouteGuard` at `superset-frontend/src/components/MobileRouteGuard/index.tsx:39` does not activate when the flag is disabled. 4. The expected text `This view isn't available on mobile` is absent, causing the assertion at `superset-frontend/playwright/generators/docs/mobile-screenshots.spec.ts:159-161` to fail; the dashboard filter test similarly cannot find `data-test="mobile-filters-trigger"` at lines 95-97. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=24282ca1e4364fdaad8051d18484ec70&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=24282ca1e4364fdaad8051d18484ec70&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/generators/docs/mobile-screenshots.spec.ts **Line:** 47:51 **Comment:** *Possible Bug: The screenshot tests do not enable `MOBILE_CONSUMPTION_MODE`; with the documented default-off configuration, the dashboard, navigation, and unsupported-route screenshots will capture the regular desktop behavior or fail their mobile-specific assertions. Configure the test context or runner to enable the flag rather than relying only on a comment describing an external prerequisite. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=3a69ef59c294f4fd868e0c79631b21d304d8bbb928523d4476d7b55cf1107476&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=3a69ef59c294f4fd868e0c79631b21d304d8bbb928523d4476d7b55cf1107476&reaction=dislike'>👎</a> ########## superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts: ########## @@ -0,0 +1,192 @@ +/** + * 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'; + +// NOTE: These tests exercise the mobile consumption experience and require +// the MOBILE_CONSUMPTION_MODE feature flag to be enabled in the target +// environment (FEATURE_FLAGS = {"MOBILE_CONSUMPTION_MODE": True}). +import { URL } from '../../utils/urls'; +import { TIMEOUT } from '../../utils/constants'; + +/** + * Mobile navigation tests verify the MobileRouteGuard behavior + * and mobile-specific navigation patterns. + * + * These tests run with a mobile viewport to trigger mobile-specific behavior. + */ + +// Use iPhone 12 viewport for mobile tests +const mobileViewport = devices['iPhone 12']; + +test.describe('Mobile Navigation', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); Review Comment: **Suggestion:** The mobile tests never enable `MOBILE_CONSUMPTION_MODE`, even though the feature flag defaults to off and all assertions require the mobile route guard and mobile UI to be active. In environments using the default configuration, these tests will navigate to the normal desktop routes and fail. Enable the flag in the test setup or configure this suite with a guaranteed flag-enabled environment. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Mobile navigation suite fails under default configuration. - ❌ Route-guard behavior is not tested reliably. - ⚠️ Desktop behavior can invalidate mobile assertions. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run `superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts` against an environment using the documented default feature configuration, where `MOBILE_CONSUMPTION_MODE` is disabled. 2. The suite setup at `superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts:38-42` configures only the iPhone viewport and user agent; it never enables the feature flag. 3. Navigate to `URL.CHART_LIST` in the test at `superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts:48-53`. 4. `MobileRouteGuard` at `superset-frontend/src/components/MobileRouteGuard/index.tsx:39` receives the desktop/non-mobile result from `useIsMobile()` at `superset-frontend/src/hooks/useIsMobile.ts:48` because the flag is off, so the normal chart list renders instead of `This view isn't available on mobile`; the assertion at lines 54-57 fails. The same missing setup affects the SQL Lab, dashboard, welcome-page, and bypass tests. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a3ca6cefb9dd414d86e6ff03f82e5965&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a3ca6cefb9dd414d86e6ff03f82e5965&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts **Line:** 38:42 **Comment:** *Possible Bug: The mobile tests never enable `MOBILE_CONSUMPTION_MODE`, even though the feature flag defaults to off and all assertions require the mobile route guard and mobile UI to be active. In environments using the default configuration, these tests will navigate to the normal desktop routes and fail. Enable the flag in the test setup or configure this suite with a guaranteed flag-enabled environment. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=512106c3c60b482a7082b26966c4d86f3f88604efed66296b30e88427ef444a9&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=512106c3c60b482a7082b26966c4d86f3f88604efed66296b30e88427ef444a9&reaction=dislike'>👎</a> ########## superset-frontend/spec/helpers/mobileTestUtils.ts: ########## @@ -0,0 +1,150 @@ +/** + * 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. + */ + +/** + * Mobile testing utilities for Jest tests. + * + * Note: We mock 'antd' directly rather than '@superset-ui/core/components' because + * mocking the latter causes circular dependency issues with ActionButton during + * jest.requireActual evaluation. Since Grid is re-exported from antd, mocking + * antd at the source works correctly. + */ + +import { FeatureFlag } from '@superset-ui/core'; + +/** + * Standard mobile breakpoint values (below md breakpoint) + */ +export const mobileBreakpoints = { + xs: true, + sm: true, + md: false, + lg: false, + xl: false, + xxl: false, +}; + +/** + * Standard desktop breakpoint values (at or above md breakpoint) + */ +export const desktopBreakpoints = { + xs: true, + sm: true, + md: true, + lg: true, + xl: true, + xxl: true, +}; + +/** + * Creates a mock for antd Grid.useBreakpoint that returns mobile breakpoints. + * Use this at the top of test files that need to simulate mobile viewport. + * + * @example + * jest.mock('antd', () => mockAntdWithMobileBreakpoint()); + */ +export const mockAntdWithMobileBreakpoint = () => ({ + ...jest.requireActual('antd'), + Grid: { + ...jest.requireActual('antd').Grid, + useBreakpoint: () => mobileBreakpoints, + }, +}); + +/** + * Creates a mock for antd Grid.useBreakpoint that returns desktop breakpoints. + * Use this at the top of test files that need to simulate desktop viewport. + * + * @example + * jest.mock('antd', () => mockAntdWithDesktopBreakpoint()); + */ +export const mockAntdWithDesktopBreakpoint = () => ({ + ...jest.requireActual('antd'), + Grid: { + ...jest.requireActual('antd').Grid, + useBreakpoint: () => desktopBreakpoints, + }, +}); + +/** + * Mocks window.matchMedia so `(max-width: ...)` queries match, simulating + * a mobile viewport for the useIsMobile hook. Returns a cleanup function + * restoring the previous matchMedia. Mobile behavior requires BOTH this + * AND the MOBILE_CONSUMPTION_MODE flag (see enableMobileConsumptionFlag). + */ +export const mockMobileMatchMedia = () => { + const previous = window.matchMedia; + Object.defineProperty(window, 'matchMedia', { + writable: true, + value: jest.fn().mockImplementation((query: string) => ({ + matches: query.includes('max-width'), + media: query, + onchange: null, + addListener: jest.fn(), + removeListener: jest.fn(), + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + dispatchEvent: jest.fn(), Review Comment: **Suggestion:** `mockMobileMatchMedia` reports every media query containing `max-width` as matching, regardless of its breakpoint. Tests using this helper can therefore incorrectly activate styles or behavior for unrelated max-width queries, producing false-positive mobile behavior and masking responsive regressions. Match only the intended mobile breakpoint query. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Jest responsive tests receive inaccurate breakpoint state. - ⚠️ Unrelated components may render mobile-only branches. - ⚠️ Breakpoint regressions can be masked by false positives. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Import and call `mockMobileMatchMedia()` from `superset-frontend/spec/helpers/mobileTestUtils.ts:91` in a Jest test, as instructed by the helper documentation at lines 86-89. 2. The helper replaces `window.matchMedia` at lines 93-104 with an implementation whose `matches` value is computed at line 96. 3. Request `window.matchMedia('(max-width: 575px)')` for an unrelated component breakpoint; line 96 returns `true` solely because the query contains `max-width`, even though the helper documents a mobile breakpoint below `md` rather than every possible maximum width. 4. Any component or hook under test that branches on that query observes mobile styling or behavior unexpectedly, so tests can pass with a false responsive state and fail to detect breakpoint-specific regressions. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=644a5f527e6c469ca21f4ed0396a3847&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=644a5f527e6c469ca21f4ed0396a3847&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/spec/helpers/mobileTestUtils.ts **Line:** 91:103 **Comment:** *Possible Bug: `mockMobileMatchMedia` reports every media query containing `max-width` as matching, regardless of its breakpoint. Tests using this helper can therefore incorrectly activate styles or behavior for unrelated max-width queries, producing false-positive mobile behavior and masking responsive regressions. Match only the intended mobile breakpoint query. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=ad701dc2ffcbee575746a6be2fea30b0478fd0b515b17afa9c360b34d6bd2442&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=ad701dc2ffcbee575746a6be2fea30b0478fd0b515b17afa9c360b34d6bd2442&reaction=dislike'>👎</a> -- 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]
