codeant-ai-for-open-source[bot] commented on code in PR #37141: URL: https://github.com/apache/superset/pull/37141#discussion_r3658290795
########## 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', +); Review Comment: **Suggestion:** The screenshot paths are written into a nested directory, but this file never creates `MOBILE_SCREENSHOTS_DIR`. Playwright does not create missing parent directories for screenshots, so the generator fails with an ENOENT error on a clean checkout unless the directory already exists. Create the directory before taking screenshots or ensure it is committed as part of the repository. [resource leak] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Documentation screenshot generation fails on clean checkouts. - ⚠️ All six mobile screenshots depend on the missing directory. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start from a clean checkout where `docs/static/img/screenshots/mobile` does not exist; the generator only computes this path at `superset-frontend/playwright/generators/docs/mobile-screenshots.spec.ts:41-44` and does not create it. 2. Run `npm run docs:screenshots` as documented at `superset-frontend/playwright/generators/docs/mobile-screenshots.spec.ts:30-32`. 3. Complete any screenshot test through `test('mobile dashboard screenshot')` at `superset-frontend/playwright/generators/docs/mobile-screenshots.spec.ts:83-90`. 4. The call to `page.screenshot()` at lines 86-89 attempts to write `mobile_dashboard.jpg` below the missing directory and fails with a filesystem `ENOENT` error; the same issue applies to the other screenshot calls at lines 109-112, 122-124, 132-134, 151-153, and 163-165. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5d4261fe236f449e97f98f658ced1826&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=5d4261fe236f449e97f98f658ced1826&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:** 41:44 **Comment:** *Resource Leak: The screenshot paths are written into a nested directory, but this file never creates `MOBILE_SCREENSHOTS_DIR`. Playwright does not create missing parent directories for screenshots, so the generator fails with an ENOENT error on a clean checkout unless the directory already exists. Create the directory before taking screenshots or ensure it is committed as part of the repository. 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=96b7418818228a01087193ba637bec4c739e7b0976083cde33414b449da17548&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=96b7418818228a01087193ba637bec4c739e7b0976083cde33414b449da17548&reaction=dislike'>👎</a> ########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,327 @@ +/** + * 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 { TIMEOUT } from '../../utils/constants'; +import { URL } from '../../utils/urls'; + +/** + * 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, + }); + Review Comment: **Suggestion:** These tests require `MOBILE_CONSUMPTION_MODE` to be enabled, but the suite neither enables the flag nor skips itself when it is disabled. Since the feature flag is off by default, normal Playwright runs will exercise the existing desktop/list behavior at a mobile viewport and fail assertions such as card-only rendering, mobile controls, and the filter drawer. Configure the project to enable the flag for this suite or conditionally skip the suite when the flag is unavailable. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Default Playwright runs can fail the mobile test suite. - ⚠️ Dashboard card-view assertions become invalid with the flag disabled. - ⚠️ Mobile menu and filter coverage is unavailable without explicit setup. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the mobile Playwright spec with the repository's default feature-flag configuration, where `MOBILE_CONSUMPTION_MODE` is off as stated by the PR description; the suite only configures the iPhone viewport at `superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts:38-43`. 2. The `beforeEach` hook at lines 44-48 opens `URL.DASHBOARD_LIST` without enabling or checking the flag. 3. The first suite then asserts card-only behavior and absence of `[data-test="listview-table"]` at lines 50-69; with the mobile consumption mode disabled, the existing dashboard-list implementation can render its normal view and fail these assertions. 4. The interaction suite similarly expects mobile dashboard controls and charts at lines 139-173 and the filter suite expects mobile filter controls at lines 257-326, but no setup enables the feature flag or skips when it is unavailable. 5. The file comments at lines 22-24 state the prerequisite, but comments do not affect Playwright execution; consequently, a normal default test run can report failures instead of clearly skipping unsupported tests. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ea8ddb7516114c4287320e751045284b&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=ea8ddb7516114c4287320e751045284b&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-dashboard.spec.ts **Line:** 38:43 **Comment:** *Possible Bug: These tests require `MOBILE_CONSUMPTION_MODE` to be enabled, but the suite neither enables the flag nor skips itself when it is disabled. Since the feature flag is off by default, normal Playwright runs will exercise the existing desktop/list behavior at a mobile viewport and fail assertions such as card-only rendering, mobile controls, and the filter drawer. Configure the project to enable the flag for this suite or conditionally skip the suite when the flag is unavailable. 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=246b279cfd33f327624cc1d996c9702d476a3aa509698a70dc8c1d2614839be2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=246b279cfd33f327624cc1d996c9702d476a3aa509698a70dc8c1d2614839be2&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx: ########## @@ -110,6 +111,22 @@ const StyledDiv = styled.div` i.warning { color: ${theme.colorWarning}; } + + /* Mobile consumption mode: show the full chart title without + truncation (controls and links are render-gated in SliceHeader) */ + ${ + isMobileConsumptionEnabled() + ? `@media (max-width: ${theme.screenSMMax}px) { + [data-test='slice-header'] .header-title { + -webkit-line-clamp: unset; + display: block; + white-space: normal; + overflow: visible; + text-overflow: unset; + } + }` + : '' + } Review Comment: **Suggestion:** This title override only applies through `screenSMMax`, while mobile consumption mode applies through the `md` breakpoint at 768px. On viewports between the `sm` and `md` breakpoints, chart titles remain truncated even though the dashboard is rendered in mobile consumption mode. Use the mobile hook's breakpoint or the equivalent 768px media query. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Mobile dashboard chart titles remain truncated. - ⚠️ Users may not identify charts from narrow screens. - ⚠️ Title behavior differs across mobile viewport widths. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable `MOBILE_CONSUMPTION_MODE` and open a dashboard whose charts render through `DashboardWrapper` at `superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx:133`. 2. Resize the browser to a width between the `sm` maximum and the `md` mobile threshold, such as 600px. `useIsMobile()` at `superset-frontend/src/hooks/useIsMobile.ts:48` treats this viewport as mobile. 3. A chart slice header rendered beneath the wrapper exposes the `[data-test='slice-header'] .header-title` selector targeted by the CSS at `DashboardWrapper.tsx:120`. 4. Because the override only applies through `theme.screenSMMax`, the title's `-webkit-line-clamp`, overflow, and truncation styles remain active at 600px; observe that long chart titles are still truncated in mobile consumption mode. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f8224e41d4214a33a037c4812300a11a&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=f8224e41d4214a33a037c4812300a11a&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/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx **Line:** 118:129 **Comment:** *Logic Error: This title override only applies through `screenSMMax`, while mobile consumption mode applies through the `md` breakpoint at 768px. On viewports between the `sm` and `md` breakpoints, chart titles remain truncated even though the dashboard is rendered in mobile consumption mode. Use the mobile hook's breakpoint or the equivalent 768px media 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=cc653343dd0d5d3cc7963ef8233688e282ac2315e7b0b9c3a25bd0de607bf7c7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=cc653343dd0d5d3cc7963ef8233688e282ac2315e7b0b9c3a25bd0de607bf7c7&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx: ########## @@ -101,6 +102,19 @@ const StyledHeader = styled.div<{ filterBarWidth: number }>` z-index: 99; max-width: calc(100vw - ${filterBarWidth}px); + /* Mobile consumption mode: let the dashboard title scroll away and keep + only the tab bar sticky. A pinned title would sit underneath the + higher-z sticky tabs, leaving its bottom edge (kebab button) peeking + out below the tab bar. */ + ${ + isMobileConsumptionEnabled() && + css` + @media (max-width: ${theme.screenSMMax}px) { + position: relative; + } + ` + } Review Comment: **Suggestion:** The mobile mode is activated below the `md` breakpoint (768px), but this media query uses `theme.screenSMMax`, which is the smaller `sm` breakpoint. Between 576px and 767px, `useIsMobile()` treats the viewport as mobile while `StyledHeader` remains sticky, causing the mobile header layout to behave incorrectly. Use the same 768px breakpoint as `useIsMobile()`. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Mobile dashboard header behavior is inconsistent from 576–767px. - ⚠️ Sticky header can overlap the mobile tab bar. - ⚠️ Dashboard title scrolling differs across supported mobile widths. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable `MOBILE_CONSUMPTION_MODE` and open a dashboard through the dashboard route that renders `DashboardBuilder` at `superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:385`. 2. Resize the browser to a width between the theme's `sm` maximum and 767px, such as 600px. At `superset-frontend/src/hooks/useIsMobile.ts:48`, `useIsMobile()` classifies this viewport as mobile because the mobile mode uses the `md` breakpoint. 3. `DashboardBuilder` renders `DashboardHeader` at `superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:541`, but the `StyledHeader` media rule at lines 109-116 does not apply because it checks `theme.screenSMMax`. 4. Observe that the header remains `position: sticky` instead of switching to `position: relative`, so the mobile title/header can remain pinned while the mobile tab layout scrolls. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c7ac57d73c39438590d002d6d65a6126&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=c7ac57d73c39438590d002d6d65a6126&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/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx **Line:** 109:116 **Comment:** *Logic Error: The mobile mode is activated below the `md` breakpoint (768px), but this media query uses `theme.screenSMMax`, which is the smaller `sm` breakpoint. Between 576px and 767px, `useIsMobile()` treats the viewport as mobile while `StyledHeader` remains sticky, causing the mobile header layout to behave incorrectly. Use the same 768px breakpoint as `useIsMobile()`. 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=c1649a0be256a2d7ff1968c8ae709a3b5eea5087014c1eaf513e6bb113054d72&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=c1649a0be256a2d7ff1968c8ae709a3b5eea5087014c1eaf513e6bb113054d72&reaction=dislike'>👎</a> ########## superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx: ########## @@ -82,6 +83,20 @@ const headerStyles = (theme: SupersetTheme) => css` display: flex; align-items: center; } + + /* Mobile consumption mode: center the title between left/right panels */ + ${ + isFeatureEnabled(FeatureFlag.MobileConsumptionMode) && + css` + @media (max-width: ${theme.screenSMMax}px) { + .title-panel { + flex: 1; + justify-content: center; + margin-right: 0; + } + } + ` Review Comment: **Suggestion:** The mobile title styling uses `screenSMMax`, so the title is not centered for mobile-consumption viewports between 576px and 767px even though `useIsMobile` treats those widths as mobile. Align this media query with the `md` breakpoint used by the feature. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Mobile headers are misaligned from 576px to 767px. - ⚠️ Affects supported mobile consumption page headers. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable `MOBILE_CONSUMPTION_MODE` and open a route that renders `PageHeaderWithActions` from `superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx`. 2. Set the browser viewport to a width between 576px and 767px, which is below the mobile `md` threshold used by `useIsMobile()` at `superset-frontend/src/hooks/useIsMobile.ts:48`. 3. `headerStyles()` evaluates the media query at `index.tsx:91`, but `theme.screenSMMax` does not match the full below-768px range, so the `.title-panel` rules at `index.tsx:93-96` are not applied. 4. Observe that the title remains governed by the desktop layout instead of being centered between the left and right panels. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ebcfe129b1534f66ac978c375d0c99b6&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=ebcfe129b1534f66ac978c375d0c99b6&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/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx **Line:** 91:98 **Comment:** *Logic Error: The mobile title styling uses `screenSMMax`, so the title is not centered for mobile-consumption viewports between 576px and 767px even though `useIsMobile` treats those widths as mobile. Align this media query with the `md` breakpoint used by the feature. 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=cfb68b1bee44a2a305529c85c7e052e54be831eae2308169277a8222c16b3882&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=cfb68b1bee44a2a305529c85c7e052e54be831eae2308169277a8222c16b3882&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, + }); + + test.beforeEach(async ({ page }) => { + await page.goto('/'); + }); Review Comment: **Suggestion:** These tests require `MOBILE_CONSUMPTION_MODE` to be enabled, but neither the test configuration nor the suite enables it. Since the feature flag defaults to off, running this file in the default Playwright environment will render the normal chart route instead of the unsupported mobile screen and cause the mobile assertions to fail. Configure the flag for this project/suite or skip the mobile assertions when it is disabled. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Default Playwright runs fail mobile navigation assertions. - ⚠️ Mobile route coverage is invalid without the feature flag. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run `superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts` against the default environment described by the test file, where `MOBILE_CONSUMPTION_MODE` is disabled by default. 2. The `test.beforeEach` hook at `superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts:44-46` only navigates to `/`; it does not enable or inject the feature flag. 3. Navigate to `URL.CHART_LIST` in the test at `mobile-navigation.spec.ts:48`; the application evaluates the mobile guard with the feature disabled, so the normal chart-list route renders instead of `MobileUnsupported`. 4. The assertions at `mobile-navigation.spec.ts:54-65` then fail because the unsupported-screen text and its buttons are absent. The dashboard and welcome tests similarly do not exercise the intended feature-enabled behavior. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e5ce7940f9164669a150489ea4a44891&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=e5ce7940f9164669a150489ea4a44891&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:** 44:46 **Comment:** *Possible Bug: These tests require `MOBILE_CONSUMPTION_MODE` to be enabled, but neither the test configuration nor the suite enables it. Since the feature flag defaults to off, running this file in the default Playwright environment will render the normal chart route instead of the unsupported mobile screen and cause the mobile assertions to fail. Configure the flag for this project/suite or skip the mobile assertions when it is disabled. 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=6f794548df6cc1ded4ac792f142164f818c712745b0cee5abe4567995338786f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=6f794548df6cc1ded4ac792f142164f818c712745b0cee5abe4567995338786f&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, + }); + + test.beforeEach(async ({ page }) => { + await page.goto('/'); + }); + + test('mobile viewport redirects from chart list to MobileUnsupported page', async ({ + page, + }) => { + // Navigate to chart list (not mobile-supported) + await page.goto(URL.CHART_LIST); + + // Should show the MobileUnsupported page + await expect( + page.getByText("This view isn't available on mobile"), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + + // Primary action buttons should be visible + await expect( + page.getByRole('button', { name: 'View Dashboards' }), + ).toBeVisible(); + await expect( + page.getByRole('button', { name: 'Go to Welcome Page' }), + ).toBeVisible(); + }); + + test('mobile viewport allows access to dashboard list', async ({ page }) => { + // Navigate to dashboard list (mobile-supported) + await page.goto(URL.DASHBOARD_LIST); + + // Should NOT show MobileUnsupported page + await expect( + page.getByText("This view isn't available on mobile"), + ).not.toBeVisible({ timeout: TIMEOUT.FORM_LOAD }); + + // Should show dashboard list content (look for dashboard list elements) + await expect( + page + .locator('[data-test="listview-table"]') + .or(page.locator('[data-test="styled-card"]')) + .first(), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + }); + + test('mobile viewport allows access to welcome page', async ({ page }) => { + // Navigate to welcome page (mobile-supported) + await page.goto(URL.WELCOME); + + // Should NOT show MobileUnsupported page + await expect( + page.getByText("This view isn't available on mobile"), + ).not.toBeVisible({ timeout: TIMEOUT.FORM_LOAD }); + + // Should show welcome page content + await expect( + page.getByText('Recents').or(page.getByText('Dashboards')).first(), + ).toBeVisible({ + timeout: TIMEOUT.PAGE_LOAD, + }); + }); + + test('View Dashboards button navigates to dashboard list', async ({ + page, + }) => { + // Navigate to unsupported route + await page.goto(URL.CHART_LIST); + + // Wait for MobileUnsupported page + await expect( + page.getByText("This view isn't available on mobile"), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + + // Click View Dashboards button + await page.getByRole('button', { name: 'View Dashboards' }).click(); + + // Should navigate to dashboard list + await page.waitForURL(url => url.pathname.includes('dashboard/list'), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Dashboard list should be accessible + await expect( + page + .locator('[data-test="listview-table"]') + .or(page.locator('[data-test="styled-card"]')) + .first(), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + }); + + test('Go to Welcome Page button navigates to welcome', async ({ page }) => { + // Navigate to unsupported route + await page.goto(URL.CHART_LIST); + + // Wait for MobileUnsupported page + await expect( + page.getByText("This view isn't available on mobile"), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + + // Click Go to Welcome Page button + await page.getByRole('button', { name: 'Go to Welcome Page' }).click(); + + // Should navigate to welcome page + await page.waitForURL(url => url.pathname.includes('welcome'), { + timeout: TIMEOUT.PAGE_LOAD, + }); + }); + + test('unsupported screen offers no bypass', async ({ page }) => { + // The "Continue anyway" bypass was removed: desktop views are unusable + // at phone width, and growing the viewport unblocks routes automatically + await page.goto(URL.CHART_LIST); + + await expect( + page.getByText("This view isn't available on mobile"), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + + await expect(page.getByText('Continue anyway')).toHaveCount(0); + }); Review Comment: **Suggestion:** The test explicitly asserts that the unsupported screen has no `Continue anyway` bypass, but the feature description specifies a session-scoped `Continue anyway` action. If the described behavior is implemented, this assertion will fail and the test also prevents users from using the documented bypass. Update either the test or the product behavior so they agree. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Mobile route tests fail when the documented bypass exists. - ⚠️ Users cannot rely on the documented unsupported-route bypass. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable `MOBILE_CONSUMPTION_MODE` and navigate to `URL.CHART_LIST` through the test at `superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts:152`. 2. The mobile route guard is invoked through `superset-frontend/src/components/MobileRouteGuard/index.tsx:39`, which displays the unsupported screen for routes not marked mobile-supported. 3. The PR description specifies that this screen provides a session-scoped `Continue anyway` action, but the test at `mobile-navigation.spec.ts:158` asserts that the action has zero matches. 4. If the documented bypass is rendered, the assertion fails; if the assertion is treated as the intended behavior, the implementation no longer matches the documented product requirements. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ada3ccf4101d4a05b22d74ff4e9733b8&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=ada3ccf4101d4a05b22d74ff4e9733b8&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:** 149:159 **Comment:** *Logic Error: The test explicitly asserts that the unsupported screen has no `Continue anyway` bypass, but the feature description specifies a session-scoped `Continue anyway` action. If the described behavior is implemented, this assertion will fail and the test also prevents users from using the documented bypass. Update either the test or the product behavior so they agree. 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=8458f6bc236e1c73ef79aa7c4ce63ab8902a34bfb3a95ee98671682e2df6def1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=8458f6bc236e1c73ef79aa7c4ce63ab8902a34bfb3a95ee98671682e2df6def1&reaction=dislike'>👎</a> ########## superset-frontend/src/components/ListView/CardCollection.tsx: ########## @@ -42,6 +43,18 @@ const CardContainer = styled.div<{ showThumbnails?: boolean }>` ? `${theme.sizeUnit * 8 + 3}px ${theme.sizeUnit * 20}px` : `${theme.sizeUnit * 8 + 1}px ${theme.sizeUnit * 20}px` }; + + /* Full-width cards on mobile (consumption mode) */ + ${ + isMobileConsumptionEnabled() + ? `@media (max-width: ${theme.screenSMMax}px) { + grid-template-columns: 1fr; + grid-gap: ${theme.sizeUnit * 4}px; + padding-left: ${theme.sizeUnit * 4}px; + padding-right: ${theme.sizeUnit * 4}px; + }` + : '' Review Comment: **Suggestion:** The mobile layout is gated by `theme.screenSMMax`, which is the maximum small-screen breakpoint (typically 575px), while the mobile consumption mode is defined as every viewport below the `md` breakpoint (768px). At widths from 576px through 767px, mobile mode is active but these cards remain in the desktop multi-column layout. Use the breakpoint corresponding to the `md` threshold. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Dashboard list cards remain multi-column on narrow tablets. - ⚠️ Mobile consumption layout is inconsistent across supported widths. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable `MOBILE_CONSUMPTION_MODE` and open a list view that renders `CardCollection` from `superset-frontend/src/components/ListView/CardCollection.tsx:71`, such as the mobile dashboard list. 2. Set the viewport between 576px and 767px; `isMobileConsumptionEnabled()` at `CardCollection.tsx:49` is true for the feature-enabled mobile experience. 3. The media query at `CardCollection.tsx:50` uses `theme.screenSMMax`, so it does not apply throughout the below-768px range defined by `useIsMobile()` at `superset-frontend/src/hooks/useIsMobile.ts:48`. 4. Observe that `CardContainer` retains `grid-template-columns: repeat(auto-fit, ...)` from `CardCollection.tsx:39`, producing the desktop-style multi-column layout instead of stacked full-width cards. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=63b07f722d1b4b47b78f199e9a0d08ec&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=63b07f722d1b4b47b78f199e9a0d08ec&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/src/components/ListView/CardCollection.tsx **Line:** 49:56 **Comment:** *Logic Error: The mobile layout is gated by `theme.screenSMMax`, which is the maximum small-screen breakpoint (typically 575px), while the mobile consumption mode is defined as every viewport below the `md` breakpoint (768px). At widths from 576px through 767px, mobile mode is active but these cards remain in the desktop multi-column layout. Use the breakpoint corresponding to the `md` threshold. 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=09d583db4b03faf8f4448834da693fd64d6f0b9e51a68f9c44e4d52a01655416&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=09d583db4b03faf8f4448834da693fd64d6f0b9e51a68f9c44e4d52a01655416&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]
