codeant-ai-for-open-source[bot] commented on code in PR #37141: URL: https://github.com/apache/superset/pull/37141#discussion_r3651939929
########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,311 @@ +/** + * 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, + }); + } Review Comment: **Suggestion:** This test silently passes when the dashboard list contains no cards because the interaction body is skipped by an ordinary conditional rather than marking the test skipped or failing. An environment with a broken or empty dashboard list can therefore report success without verifying dashboard loading. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Dashboard loading coverage is silently bypassed when fixtures are empty. - ⚠️ CI can report success without validating mobile chart rendering. - ⚠️ Dashboard-list fixture failures become false-positive test results. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the `Mobile Dashboard Interaction` suite in `superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts` with the iPhone 12 viewport configured at lines 110-113. 2. The test navigates to `URL.DASHBOARD_LIST` at lines 137-138 and counts `[data-test="styled-card"]` elements at lines 141-142. 3. If the dashboard fixture is unavailable, the list is empty, or the list request fails and renders no cards, `cardCount` is zero. 4. The conditional at lines 144-168 is skipped, so the test performs no navigation or visibility assertion and still reports success instead of explicitly skipping or failing. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=130022be2c1643bfabcba34b30a74952&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=130022be2c1643bfabcba34b30a74952&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:** 144:168 **Comment:** *Possible Bug: This test silently passes when the dashboard list contains no cards because the interaction body is skipped by an ordinary conditional rather than marking the test skipped or failing. An environment with a broken or empty dashboard list can therefore report success without verifying dashboard loading. 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=ff208e90546bdc2a402bb9985f9afc878a75affd8571211e607c8323c9bb55ac&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=ff208e90546bdc2a402bb9985f9afc878a75affd8571211e607c8323c9bb55ac&reaction=dislike'>👎</a> ########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,311 @@ +/** + * 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)/); + } Review Comment: **Suggestion:** This test also passes without checking anything when the menu button or refresh option is absent because both cases are guarded by conditionals with no failure or skip. A regression that removes the mobile menu or refresh action will therefore be reported as successful. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Mobile dashboard menu regressions are not detected. - ⚠️ Refresh-dashboard action regressions are not detected. - ⚠️ CI can pass without validating the mobile refresh workflow. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run `refresh dashboard works from mobile menu` in `superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts`; it opens `URL.DASHBOARD_LIST` at lines 201-203 and selects the first dashboard card at lines 205-209. 2. After navigation to a dashboard at lines 211-214, the test searches for `[data-test="actions-trigger"]` or `[aria-label="Menu actions trigger"]` at lines 217-219. 3. If the mobile menu is removed, renamed, or not rendered, `menuButton.count()` is zero and the conditional at line 221 skips the entire interaction without an assertion. 4. If the menu exists but does not contain `Refresh dashboard`, the nested conditional at lines 227-235 also skips the refresh validation, so either regression is reported as a passing test. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bf7f0143f9144546b8eddeefb16b150f&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=bf7f0143f9144546b8eddeefb16b150f&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:** 221:235 **Comment:** *Possible Bug: This test also passes without checking anything when the menu button or refresh option is absent because both cases are guarded by conditionals with no failure or skip. A regression that removes the mobile menu or refresh action will therefore be reported as successful. 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=5ead08fb829a535fd5879183457ab65e45649a963a0fc7a9eb14e4ed90ab46fd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=5ead08fb829a535fd5879183457ab65e45649a963a0fc7a9eb14e4ed90ab46fd&reaction=dislike'>👎</a> ########## superset-frontend/src/components/ListView/ListView.tsx: ########## @@ -453,26 +490,32 @@ export function ListView<T extends object = any>({ )} <div data-test={className} className={`superset-list-view ${className} `}> <div className="header"> - {cardViewEnabled && ( + {cardViewEnabled && !forceViewMode && ( <ViewModeToggle mode={viewMode} setMode={setViewMode} /> )} <div className="controls" data-test="filters-select"> - {filterable && ( + {/* When a mobile drawer callback is provided, filters and sort + render inside the drawer instead of inline. Only one + FilterControls instance is ever mounted, so filtersRef and + filterControlsRef always point at the visible instance. */} + {filterable && !setMobileFiltersOpen && ( <FilterControls ref={filterControlsRef} filters={filters} internalFilters={internalFilters} updateFilterValue={applyFilterValue} /> )} - {viewMode === 'card' && cardSortSelectOptions && ( - <CardSortSelect - initialSort={sortBy} - onChange={(value: SortColumn[]) => setSortBy(value)} - options={cardSortSelectOptions} - /> - )} - {filterable && ( + {viewMode === 'card' && + cardSortSelectOptions && + !setMobileFiltersOpen && ( + <CardSortSelect + initialSort={sortBy} + onChange={(value: SortColumn[]) => setSortBy(value)} + options={cardSortSelectOptions} + /> + )} Review Comment: **Suggestion:** The presence of `setMobileFiltersOpen` is used as the sole condition for removing the inline filters and sort controls, so any caller that provides this controlled callback will lose its filters on every viewport, not just mobile. Gate this behavior with the actual mobile-consumption state or keep the callback API scoped to mobile callers. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Desktop list filters disappear when callback is always supplied. - ⚠️ Desktop card sorting controls disappear with the same configuration. - ⚠️ ListView callers must manually gate the callback by viewport. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render `ListView` from `superset-frontend/src/components/ListView/ListView.tsx` with non-empty `filters` and a `setMobileFiltersOpen` callback, which the new props allow at lines 349-354. 2. The component computes `filterable` from `filters.length` at lines 418-419, but the inline `FilterControls` at lines 501-508 is rendered only when `setMobileFiltersOpen` is falsy. 3. Because the callback is used as a mode flag rather than checking the actual mobile state, the inline `FilterControls` and the `CardSortSelect` at lines 509-517 are removed even when the parent is rendering a desktop list. 4. The same callback still causes the drawer to be rendered at lines 707-744, so desktop users can lose the normal controls unless the caller conditionally omits the callback outside mobile consumption mode. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e420aff8a20e4406a366135d5415fa14&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=e420aff8a20e4406a366135d5415fa14&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/ListView.tsx **Line:** 501:517 **Comment:** *Logic Error: The presence of `setMobileFiltersOpen` is used as the sole condition for removing the inline filters and sort controls, so any caller that provides this controlled callback will lose its filters on every viewport, not just mobile. Gate this behavior with the actual mobile-consumption state or keep the callback API scoped to mobile callers. 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=da4caab3804ac7b74e3483f969b2fbc329f41b8c21734a26baaaff1f62ea4507&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=da4caab3804ac7b74e3483f969b2fbc329f41b8c21734a26baaaff1f62ea4507&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]
