codeant-ai-for-open-source[bot] commented on code in PR #37141: URL: https://github.com/apache/superset/pull/37141#discussion_r3651467244
########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,326 @@ +/** + * 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}). Review Comment: **Suggestion:** The suite is always collected and navigates to the dashboard list, but the file only documents that `MOBILE_CONSUMPTION_MODE` must be enabled and never enables or skips the tests when it is disabled. In the default configuration described by the PR, these tests will run against the normal table view and fail instead of being skipped. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Default Playwright runs can fail on every mobile test execution. - ⚠️ CI results depend on external feature-flag configuration. - ❌ Dashboard-list regression tests cannot run reliably in their intended mode. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the Playwright suite using the repository's default feature-flag configuration, where the PR description states that `MOBILE_CONSUMPTION_MODE` is disabled by default. 2. Playwright still collects `superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts`; the `test.describe` blocks at lines 38-47 and 109-113 contain no conditional skip or feature-flag setup. 3. Each test's `beforeEach` navigates to `URL.DASHBOARD_LIST` at lines 44-47, so the tests execute against the normal mobile dashboard-list behavior rather than the consumption-mode behavior. 4. The assertion at lines 50-69 expects mobile cards and zero `[data-test="listview-table"]` elements; with the feature disabled, the dashboard list can render its normal table view and cause the test to fail instead of being skipped. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8abf6751aad2465b8ad705ea2ee2f572&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=8abf6751aad2465b8ad705ea2ee2f572&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:** 22:24 **Comment:** *Possible Bug: The suite is always collected and navigates to the dashboard list, but the file only documents that `MOBILE_CONSUMPTION_MODE` must be enabled and never enables or skips the tests when it is disabled. In the default configuration described by the PR, these tests will run against the normal table view and fail instead of being skipped. 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=c01fd32016ea14aa20967b20e51343a1c1908f8cb2302981aa0aca5ea7336ad2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=c01fd32016ea14aa20967b20e51343a1c1908f8cb2302981aa0aca5ea7336ad2&reaction=dislike'>👎</a> ########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,326 @@ +/** + * 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, + }); + + test.beforeEach(async ({ page }) => { + // Navigate to dashboard list to find a dashboard + await page.goto(URL.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; the table + // view must never render on mobile + if (cardCount > 0) { + await expect(cards.first()).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + } else { + await expect(page.locator('[data-test="empty-state"]')).toBeVisible({ + timeout: TIMEOUT.PAGE_LOAD, + }); + } + await expect(page.locator('[data-test="listview-table"]')).toHaveCount(0); + }); + + test('mobile search button appears in dashboard list', async ({ page }) => { + // On mobile, the search/filter button should appear in the header + const searchButton = page + .locator('[aria-label="Search"]') + .or(page.locator('[data-test="mobile-search-button"]')); + + // Search button should be visible on mobile + await expect(searchButton.first()).toBeVisible({ + timeout: TIMEOUT.PAGE_LOAD, + }); + }); + + test('tapping dashboard card opens the dashboard', async ({ page }) => { + // Find a dashboard card + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + if (cardCount > 0) { + // Click the first card + await cards.first().click(); + + // Should navigate to dashboard view + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Dashboard should load (look for dashboard content) + await expect( + page + .locator('[data-test="dashboard-content-wrapper"]') + .or(page.locator('.dashboard')), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + } else { + test.skip(); + } + }); +}); + +test.describe('Mobile Dashboard Interaction', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + // Skip this test suite if no dashboards exist + test.beforeAll(async ({ browser }) => { + const page = await browser.newPage({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + await page.close(); + + if (cardCount === 0) { + test.skip(); + } + }); + + test('dashboard loads and shows charts on mobile', async ({ page }) => { + // Navigate to dashboard list + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + // Click first dashboard + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + if (cardCount > 0) { + await cards.first().click(); + + // Wait for dashboard to load + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Dashboard content should be visible + await expect( + page + .locator('[data-test="dashboard-content-wrapper"]') + .or(page.locator('.dashboard')), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + + // Charts should start loading (look for chart containers) + const chartContainers = page + .locator('[data-test="chart-container"]') + .or(page.locator('.dashboard-chart')); + + // Wait for at least one chart to be visible (with timeout) + await expect(chartContainers.first()).toBeVisible({ + timeout: TIMEOUT.PAGE_LOAD * 2, + }); + } + }); + + test('dashboard header shows hamburger menu on mobile', async ({ page }) => { + // Navigate to dashboard list + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + // Click first dashboard + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + if (cardCount > 0) { + await cards.first().click(); + + // Wait for dashboard + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Look for the hamburger menu / more actions button + const menuButton = page + .locator('[data-test="actions-trigger"]') + .or(page.locator('[aria-label="Menu actions trigger"]')); + + await expect(menuButton.first()).toBeVisible({ + timeout: TIMEOUT.PAGE_LOAD, + }); + } + }); + + test('refresh dashboard works from mobile menu', async ({ page }) => { + // Navigate to dashboard list + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + // Click first dashboard + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + if (cardCount > 0) { + await cards.first().click(); + + // Wait for dashboard + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Open the actions menu + const menuButton = page + .locator('[data-test="actions-trigger"]') + .or(page.locator('[aria-label="Menu actions trigger"]')); + + if ((await menuButton.count()) > 0) { + await menuButton.first().click(); + + // Look for refresh option + const refreshOption = page.getByText('Refresh dashboard'); + + if ((await refreshOption.count()) > 0) { + await refreshOption.click(); + + // Should show success toast or refresh the charts + // This is hard to verify without checking network requests + // Just verify the menu closes and we're still on the dashboard + await page.waitForTimeout(1000); + expect(page.url()).toMatch(/\/dashboard\/(?!list)/); + } + } + } + }); +}); + +test.describe('Mobile Filter Drawer', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + test('filter button appears on dashboards with filters', async ({ page }) => { + // Navigate to dashboard list + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + // Click first dashboard + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + if (cardCount > 0) { + await cards.first().click(); + + // Wait for dashboard + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Give filters time to load + await page.waitForTimeout(2000); + + // Check for filter button (only visible if dashboard has filters) + const filterButton = page + .locator('[data-test="filter-icon"]') + .or( + page + .locator('[aria-label="Filters"]') + .or(page.locator('.mobile-filter-button')), + ); + + const filterCount = await filterButton.count(); + + // The test passes whether filters exist or not + // If filters exist, button should be visible + // If no filters, that's also valid + if (filterCount > 0) { + await expect(filterButton.first()).toBeVisible(); + } Review Comment: **Suggestion:** This test silently passes when no filter button exists because the assertion is guarded by `filterCount > 0`. That means a regression where the mobile filter button is missing is treated as success; the test should either use a dashboard fixture guaranteed to contain filters or explicitly skip when the fixture is unavailable before asserting the button and drawer behavior. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Mobile filter-button regressions remain undetected. - ⚠️ Filter-drawer coverage depends on whichever dashboard appears first. - ⚠️ Dashboard filter usability is not reliably verified. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run `superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts` with a dashboard available in `URL.DASHBOARD_LIST`; the test selects the first card at lines 247-257 and opens its dashboard route. 2. At lines 267-274, the test searches for the filter controls using `[data-test="filter-icon"]`, `[aria-label="Filters"]`, or `.mobile-filter-button`. 3. Remove the mobile filter button, or use a dashboard whose filters are not rendered, so `filterButton.count()` at line 276 returns zero. 4. Because the assertion at lines 281-283 is only executed when `filterCount > 0`, the test completes successfully without verifying that the required filter control exists. The drawer test at lines 287-323 has the same guarded behavior. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=045a469f7beb4259b45f3ec2e81b4bca&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=045a469f7beb4259b45f3ec2e81b4bca&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:** 276:283 **Comment:** *Possible Bug: This test silently passes when no filter button exists because the assertion is guarded by `filterCount > 0`. That means a regression where the mobile filter button is missing is treated as success; the test should either use a dashboard fixture guaranteed to contain filters or explicitly skip when the fixture is unavailable before asserting the button and drawer behavior. 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=79ea7180328d88fb1efd0f8fa36bcf62ae10f967ffeebd60441e12121661ed14&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=79ea7180328d88fb1efd0f8fa36bcf62ae10f967ffeebd60441e12121661ed14&reaction=dislike'>👎</a> ########## superset-frontend/src/components/ListView/utils.ts: ########## @@ -234,10 +236,19 @@ export function useListViewState({ }; const [viewMode, setViewMode] = useState<ViewModeType>( - (query.viewMode as ViewModeType) || + // forceViewMode overrides everything (used for mobile) + forceViewMode || + (query.viewMode as ViewModeType) || (renderCard ? defaultViewMode : 'table'), ); + // Update viewMode when forceViewMode changes (e.g., screen resize) + useEffect(() => { + if (forceViewMode) { + setViewMode(forceViewMode); + } + }, [forceViewMode]); Review Comment: **Suggestion:** When `forceViewMode` is removed, this effect does not restore the prior or default view mode, so resizing from mobile to desktop leaves the list stuck in the forced mode. Handle the transition from a forced value to `undefined` by recomputing the view mode from the query/default state. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Responsive dashboard lists can remain stuck in card view. - ⚠️ Desktop view-mode preference is not restored after resizing. - ⚠️ ListView consumers using forced mobile modes get inconsistent navigation. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render a `ListView` consumer that calls `useListViewState()` with `forceViewMode="card"` while the mobile consumption mode is active; the new option is declared at `superset-frontend/src/components/ListView/utils.ts:188-199` and applied by the state hook at lines 201-212. 2. The initial state at lines 238-243 sets `viewMode` to the forced card mode, and the effect at lines 245-250 also sets it to `card`. 3. Resize the browser from below the mobile breakpoint to desktop so the caller changes `forceViewMode` from `"card"` to `undefined`. 4. The effect runs because its dependency changes, but the `if (forceViewMode)` body is skipped at lines 247-249; no state update restores the query-selected or default mode, so the list remains in card view until another state change or remount. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f7db45a803bd437b869c9558c6181a88&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=f7db45a803bd437b869c9558c6181a88&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/utils.ts **Line:** 245:250 **Comment:** *Logic Error: When `forceViewMode` is removed, this effect does not restore the prior or default view mode, so resizing from mobile to desktop leaves the list stuck in the forced mode. Handle the transition from a forced value to `undefined` by recomputing the view mode from the query/default state. 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=4afbeb43bb514c7cf0135a452ee42853845ab76be5c684f7ad74535f61c4d178&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=4afbeb43bb514c7cf0135a452ee42853845ab76be5c684f7ad74535f61c4d178&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]
