codeant-ai-for-open-source[bot] commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r3656386287
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -739,6 +776,36 @@ const DashboardBuilder = () => {
`}
/>
)}
+ {/* Mobile filters drawer */}
+ {!isNotMobile && nativeFiltersEnabled && (
+ <Drawer
+ title={t('Filters')}
+ placement="left"
+ onClose={() => setMobileFiltersOpen(false)}
+ open={mobileFiltersOpen}
+ width="85vw"
+ styles={{
+ body: {
+ padding: 0,
+ display: 'flex',
+ flexDirection: 'column',
+ },
+ }}
+ >
+ <FilterBar
+ orientation={FilterBarOrientation.Vertical}
+ verticalConfig={{
+ filtersOpen: true,
+ toggleFiltersBar: () => {},
Review Comment:
**Suggestion:** The mobile filter bar receives a no-op `toggleFiltersBar`,
so any close/collapse control rendered by `FilterBar` cannot dismiss the mobile
drawer and leaves the filter UI stuck open. Wire this callback to
`setMobileFiltersOpen(false)` (or the drawer close handler). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Mobile dashboard filters cannot collapse through their own control.
- ⚠️ Users must use the drawer-level close button instead.
- ⚠️ Affects native-filter dashboards in mobile consumption mode.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable mobile consumption mode and open a dashboard with native filters
at a viewport
narrower than the mobile breakpoint; `DashboardBuilder` computes
`isNotMobile` at
`superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:389`
and
enables the mobile path.
2. Open the dashboard filter drawer through `DashboardHeader` at
`superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:541-548`;
this sets `mobileFiltersOpen` to `true`.
3. The drawer renders a vertical `FilterBar` at
`superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:779-806`,
with `filtersOpen: true` and `toggleFiltersBar: () => {}` at line 799.
4. Use the filter bar's own collapse or close control; `FilterBar` invokes
the supplied
toggle callback, but the no-op function does not change `mobileFiltersOpen`,
so the filter
panel remains open. The drawer's separate close action at lines 784-785
still works,
making this an internal-control usability failure rather than a completely
undismissable
drawer.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=35751032e1bb412f86c09b163f862169&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=35751032e1bb412f86c09b163f862169&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:** 799:799
**Comment:**
*Logic Error: The mobile filter bar receives a no-op
`toggleFiltersBar`, so any close/collapse control rendered by `FilterBar`
cannot dismiss the mobile drawer and leaves the filter UI stuck open. Wire this
callback to `setMobileFiltersOpen(false)` (or the drawer close handler).
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=13a5d7186d8bad9fb92239eedd0a30b8d6e3daf36d41bc4690f5e91b313790b9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=13a5d7186d8bad9fb92239eedd0a30b8d6e3daf36d41bc4690f5e91b313790b9&reaction=dislike'>👎</a>
##########
superset-frontend/spec/helpers/mobileTestUtils.ts:
##########
@@ -0,0 +1,169 @@
+/**
+ * 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.
+ *
+ * Note: FeatureFlag is imported from the '@superset-ui/core/utils' submodule
+ * rather than the '@superset-ui/core' package root. The package root barrel
+ * transitively pulls in the theme module, which imports 'antd'. Consuming
+ * test files call `jest.mock('antd', () => mockAntdWithDesktopBreakpoint())`
+ * before importing this file, so if loading this file triggered an 'antd'
+ * require before `mockAntdWithDesktopBreakpoint` were defined, the mock
+ * factory would throw.
+ */
+
+import { FeatureFlag } from '@superset-ui/core/utils';
+
+/**
+ * Standard mobile breakpoint values (below md breakpoint)
+ */
+export const mobileBreakpoints = {
+ xs: true,
+ sm: true,
+ md: false,
+ lg: false,
+ xl: false,
+ xxl: false,
+};
Review Comment:
**Suggestion:** The mocked Ant Design breakpoint state is incorrect for a
375px mobile viewport: `sm` starts at 576px, so it must be false on mobile.
Returning `sm: true` can make components take the small-screen layout path
during tests and allow mobile behavior to pass incorrectly compared with a real
browser. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Mobile Jest tests may exercise the wrong responsive layout.
- ⚠️ `screens.sm` behavior can diverge from 375px browser behavior.
- ⚠️ Incorrect mocks can allow mobile regression tests to pass falsely.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A Jest test imports `mockAntdWithMobileBreakpoint()` from
`superset-frontend/spec/helpers/mobileTestUtils.ts:70` and applies it as the
documented
`antd` module mock.
2. A component under test calls the mocked `Grid.useBreakpoint()` and
receives the object
defined at `superset-frontend/spec/helpers/mobileTestUtils.ts:42-49`,
including `sm: true`
and `md: false`.
3. In a real 375px browser viewport, Ant Design's `sm` breakpoint (576px
minimum) is not
active, while the mock incorrectly reports it as active.
4. Any responsive component or test assertion branching on `screens.sm` can
therefore
select the small-screen layout in Jest even though that branch would not
execute at the
documented mobile viewport; changing `sm` to `false` makes the mock
consistent with Ant
Design's breakpoint semantics.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1b793a0e81e3472996d6dde3c53097b9&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=1b793a0e81e3472996d6dde3c53097b9&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:** 42:49
**Comment:**
*Possible Bug: The mocked Ant Design breakpoint state is incorrect for
a 375px mobile viewport: `sm` starts at 576px, so it must be false on mobile.
Returning `sm: true` can make components take the small-screen layout path
during tests and allow mobile behavior to pass incorrectly compared with a real
browser.
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=302fbe5e236bc537d66e73c9eec2ec3c7a9c36de32c47c86f07d884e0e873f00&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=302fbe5e236bc537d66e73c9eec2ec3c7a9c36de32c47c86f07d884e0e873f00&reaction=dislike'>👎</a>
##########
superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts:
##########
@@ -0,0 +1,323 @@
+/**
+ * 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();
+ }
Review Comment:
**Suggestion:** The `browser.newPage()` call creates a fresh browser context
and does not inherit the authenticated `page` fixture's storage state. In an
authenticated Superset test environment this page will typically land on the
login page, find zero dashboard cards, and cause the entire interaction suite
to be skipped even when dashboards exist. Create the page through the
authenticated context/fixture instead of using the raw browser fixture.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Authenticated mobile interaction tests are silently skipped.
- ⚠️ CI can report success without exercising dashboard behavior.
- ⚠️ Dashboard loading, menu, refresh, and filter coverage is lost.
```
</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`, which
enters the
worker-level `test.beforeAll` at lines 116-132.
2. The hook calls `browser.newPage()` at line 117 instead of using the
authenticated
`page` fixture or a context initialized with the project's storage state.
3. Navigate that fresh context to `URL.DASHBOARD_LIST` at line 122; an
environment
requiring authentication redirects it to the login page rather than the
dashboard list.
4. The selector at line 125 finds zero dashboard cards, so line 131 calls
`test.skip()`,
causing all dashboard interaction tests to be skipped even though
authenticated dashboard
data exists.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=061f9937a7204e1594a578a8ebe270e1&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=061f9937a7204e1594a578a8ebe270e1&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:** 116:132
**Comment:**
*Possible Bug: The `browser.newPage()` call creates a fresh browser
context and does not inherit the authenticated `page` fixture's storage state.
In an authenticated Superset test environment this page will typically land on
the login page, find zero dashboard cards, and cause the entire interaction
suite to be skipped even when dashboards exist. Create the page through the
authenticated context/fixture instead of using the raw browser fixture.
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=33e47241d2809acef7d6cc90304ca7f326ac4cf51a7065a07715d2ceff5d6929&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=33e47241d2809acef7d6cc90304ca7f326ac4cf51a7065a07715d2ceff5d6929&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}).
Review Comment:
**Suggestion:** These tests are unconditional, but the feature flag they
exercise is documented as disabled by default and no setup in this file enables
or skips the tests when it is disabled. In the default configuration the mobile
guard will not activate, so the mobile assertions will fail rather than being
valid tests. Enable the flag for this project or conditionally skip the suite
when the flag is unavailable. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Default CI fails mobile navigation assertions.
- ⚠️ Route-guard coverage depends on undocumented external setup.
- ⚠️ Chart-list and SQL Lab tests cannot run with flag disabled.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts`
without
externally enabling `MOBILE_CONSUMPTION_MODE`, matching the feature's
documented
default-off configuration.
2. The mobile tests set only the iPhone 12 viewport at lines 35-36 and do
not enable or
conditionally skip based on the feature flag.
3. Open `URL.CHART_LIST` at lines 52 and 107; `MobileRouteGuard` at
`superset-frontend/src/components/MobileRouteGuard/index.tsx:39` evaluates
`useIsMobile()`
from `superset-frontend/src/hooks/useIsMobile.ts:48`, which remains false
when the flag is
disabled.
4. The chart-list route renders normally instead of the mobile unsupported
screen, so
assertions for `This view isn't available on mobile` at lines 54-56 and
110-112 fail; the
dashboard-list and welcome-page mobile assertions can likewise test behavior
that is
inactive.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=57709e272534413994bb386368dd7d51&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=57709e272534413994bb386368dd7d51&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:** 22:24
**Comment:**
*Possible Bug: These tests are unconditional, but the feature flag they
exercise is documented as disabled by default and no setup in this file enables
or skips the tests when it is disabled. In the default configuration the mobile
guard will not activate, so the mobile assertions will fail rather than being
valid tests. Enable the flag for this project 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=32cd1153b84e554a8b3d855649c8b050862098f8d15bb3119c669f2289729bb3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=32cd1153b84e554a8b3d855649c8b050862098f8d15bb3119c669f2289729bb3&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]
