codeant-ai-for-open-source[bot] commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r2837329675
##########
superset-frontend/src/pages/DashboardList/index.tsx:
##########
@@ -146,6 +148,9 @@ const DASHBOARD_COLUMNS_TO_FETCH = [
function DashboardList(props: DashboardListProps) {
const { addDangerToast, addSuccessToast, user } = props;
+ const { md: isNotMobile } = Grid.useBreakpoint();
Review Comment:
**Suggestion:** The breakpoint hook result for `md` can be `undefined` on
initial render (as seen in similar usage on the Home page), which makes
`!isNotMobile` evaluate to true and incorrectly enables mobile-only behavior
(forced card view and mobile filter icon) on desktop until the breakpoints
resolve; adding a default of `true` for `isNotMobile` keeps the initial
behavior desktop-safe and consistent. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Desktop dashboard list briefly shows mobile-only search icon.
- ⚠️ Desktop dashboard list initially forces card view mode.
- ⚠️ Inconsistent layout compared to other ListView-based pages.
- ⚠️ Visual flicker on every `/dashboard/list/` navigation.
```
</details>
```suggestion
const { md: isNotMobile = true } = Grid.useBreakpoint();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Navigate to the Dashboard list route `/dashboard/list/`, which is wired
to the
`DashboardList` component in
`superset-frontend/src/views/routes.tsx:216-218` (`Component:
DashboardList`).
2. On initial render of `DashboardList`
(`superset-frontend/src/pages/DashboardList/index.tsx:149-156`),
`Grid.useBreakpoint()` is
called and destructured as `const { md: isNotMobile } =
Grid.useBreakpoint();`. The Ant
Design `Grid.useBreakpoint` hook returns a partial breakpoint map, so `md`
is `undefined`
until the hook's internal media-query listeners resolve.
3. Because `isNotMobile` is `undefined` on this first render, all usages of
`!isNotMobile`
in `DashboardList` evaluate to `true`, specifically:
- The `leftIcon` prop passed to `SubMenu` at `index.tsx:747-763` renders
the
mobile-only search button on desktop (`!isNotMobile ? <Button ... /> :
undefined`).
- The `forceViewMode` prop passed to `ListView` at `index.tsx:826-859` is
set to
`'card'` (`forceViewMode={!isNotMobile ? 'card' : undefined}`), forcing
card view even
when the default desktop view should allow toggling to table view.
4. Once React re-renders after `Grid.useBreakpoint` resolves `md` to `true`
for a desktop
viewport, `isNotMobile` becomes `true`, `!isNotMobile` flips to `false`, and
the UI
switches back to desktop behavior (mobile search icon disappears and
`forceViewMode`
becomes `undefined`). This produces a brief but repeatable flash of
mobile-only layout on
every desktop navigation to `/dashboard/list/`. The need for a safe default
is
corroborated by the Home page implementation in
`superset-frontend/src/pages/Home/index.tsx:149-152`, which uses `const {
md: isNotMobile
= true } = Grid.useBreakpoint();` to avoid this exact issue.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/DashboardList/index.tsx
**Line:** 151:151
**Comment:**
*Logic Error: The breakpoint hook result for `md` can be `undefined` on
initial render (as seen in similar usage on the Home page), which makes
`!isNotMobile` evaluate to true and incorrectly enables mobile-only behavior
(forced card view and mobile filter icon) on desktop until the breakpoints
resolve; adding a default of `true` for `isNotMobile` keeps the initial
behavior desktop-safe and consistent.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=ebaa361f3d7c6d20ed895147f5c3d5482dfce98b2e53a2b21c7a3025bf568cd7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=ebaa361f3d7c6d20ed895147f5c3d5482dfce98b2e53a2b21c7a3025bf568cd7&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/MobileUnsupported/index.tsx:
##########
@@ -0,0 +1,218 @@
+/**
+ * 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 { useCallback } from 'react';
+import { useHistory, useLocation } from 'react-router-dom';
+import { t } from '@apache-superset/core';
+import { css, useTheme } from '@apache-superset/core/ui';
+import { Button, Grid } from '@superset-ui/core/components';
+import { Icons } from '@superset-ui/core/components/Icons';
+
+const { useBreakpoint } = Grid;
+
+interface MobileUnsupportedProps {
+ /** The original path the user was trying to access */
+ originalPath?: string;
+}
+
+/**
+ * A mobile-friendly page shown when users try to access
+ * features that aren't supported on mobile devices.
+ */
+function MobileUnsupported({ originalPath }: MobileUnsupportedProps) {
+ const theme = useTheme();
+ const history = useHistory();
+ const location = useLocation();
+ const screens = useBreakpoint();
+
+ // Get the original path from props or query params
+ const fromPath =
+ originalPath ||
+ new URLSearchParams(location.search).get('from') ||
+ location.pathname;
Review Comment:
**Suggestion:** The `from` query parameter is used directly as a navigation
target without validation, so a crafted URL like
`/mobile-unsupported?from=https://evil.com` would cause the "Continue anyway"
action to redirect users to an attacker-controlled site, creating an open
redirect vulnerability; restrict this value to same-origin paths (e.g., those
starting with `/`) before using it in `history.push`. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Potential open redirect via unvalidated `from` query parameter.
- ⚠️ Users could be redirected to attacker-controlled external domains.
- ⚠️ Increases phishing risk using legitimate-looking Superset URLs.
```
</details>
```suggestion
const fromQueryParam = new URLSearchParams(location.search).get('from');
const safeFromPath =
fromQueryParam && fromQueryParam.startsWith('/') ? fromQueryParam :
undefined;
const fromPath =
originalPath || safeFromPath || location.pathname;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe the `MobileUnsupported` component definition in
`superset-frontend/src/pages/MobileUnsupported/index.tsx:28-37`, where the
`originalPath`
prop is optional (`originalPath?: string`), indicating the component is
intended to work
even when `originalPath` is not supplied.
2. Note how `fromPath` is computed in
`superset-frontend/src/pages/MobileUnsupported/index.tsx:40-47`: it prefers
`originalPath`, then falls back to `new
URLSearchParams(location.search).get('from')`, and
finally to `location.pathname`, so the `from` query parameter directly
controls the
navigation target whenever `originalPath` is absent.
3. In `superset-frontend/src/components/MobileRouteGuard/index.tsx:71-111`,
unsupported
routes on mobile render `<MobileUnsupported originalPath={location.pathname +
location.search} />`, so in that specific flow the untrusted `from` query
parameter is not
used (the prop takes precedence), and `Continue anyway` navigates back to
the same-origin
URL.
4. Because `MobileUnsupported` is designed to be mountable without
`originalPath` (step 1)
and there is an explicit mobile-supported route pattern for
`/mobile-unsupported` in
`MOBILE_SUPPORTED_ROUTES` at
`superset-frontend/src/components/MobileRouteGuard/index.tsx:30-48`, any
route (e.g.,
`/mobile-unsupported`) that renders `<MobileUnsupported />` without
`originalPath` would
allow a URL like `/mobile-unsupported?from=https://evil.com` to set
`fromPath` to
`https://evil.com`; clicking "Continue anyway" then calls
`history.push(fromPath)`
(index.tsx:57-65), causing an open redirect to the attacker-controlled
domain. The
suggested change prevents this by constraining `from` to same-origin paths.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/MobileUnsupported/index.tsx
**Line:** 44:47
**Comment:**
*Security: The `from` query parameter is used directly as a navigation
target without validation, so a crafted URL like
`/mobile-unsupported?from=https://evil.com` would cause the "Continue anyway"
action to redirect users to an attacker-controlled site, creating an open
redirect vulnerability; restrict this value to same-origin paths (e.g., those
starting with `/`) before using it in `history.push`.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=0b450b6622875ebb74753e36a3567bbc8adfba41c38b183cf0a5b02912941c28&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=0b450b6622875ebb74753e36a3567bbc8adfba41c38b183cf0a5b02912941c28&reaction=dislike'>👎</a>
##########
superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts:
##########
@@ -0,0 +1,344 @@
+/**
+ * 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';
+import { TIMEOUT } from '../../utils/constants';
+
+/**
+ * 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('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
+ if (cardCount > 0) {
+ await expect(cards.first()).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD });
+ } else {
+ // No dashboards - that's OK for the test environment
+ await expect(
+ page
+ .getByText('No dashboards yet')
+ .or(page.locator('[data-test="listview-table"]')),
Review Comment:
**Suggestion:** The fallback branch for when there are no dashboards asserts
the presence of the text "No dashboards yet", but this string does not exist
anywhere in the application and the ListView empty state instead exposes a
generic `data-test="empty-state"` container; this will cause the test to fail
in environments without dashboards even though the UI is behaving correctly.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Mobile dashboard list test fails with zero dashboards.
- ⚠️ CI may break on instances without seeded dashboards.
```
</details>
```suggestion
.locator('[data-test="empty-state"]')
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with this PR deployed and ensure there are no dashboards
in the system
(so the `/dashboard/list/` ListView is empty).
2. Run the Playwright test file
`superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts`, which
navigates to
`dashboard/list/` in the `Mobile Dashboard Viewing` describe block (see
lines 41–43 in
that file).
3. In the test `dashboard list renders in card view on mobile` (lines
46–65), note that
when `cardCount === 0`, the test executes the `else` branch that asserts
`page.getByText('No dashboards
yet').or(page.locator('[data-test="listview-table"]))` is
visible.
4. The actual ListView empty state in
`superset-frontend/src/components/ListView/ListView.tsx` renders an
`<EmptyWrapper
data-test="empty-state">` when `!loading && rows.length === 0` (see lines
36–55 of the
snippet from that file), and does not render the text `'No dashboards yet'`
nor the
`[data-test="listview-table"]` table in card mode, causing the Playwright
expectation to
fail even though the UI is correct.
```
</details>
<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:** 62:62
**Comment:**
*Logic Error: The fallback branch for when there are no dashboards
asserts the presence of the text "No dashboards yet", but this string does not
exist anywhere in the application and the ListView empty state instead exposes
a generic `data-test="empty-state"` container; this will cause the test to fail
in environments without dashboards even though the UI is behaving correctly.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=89998272253bb98eae0f3debef65510fbf3da55aa15d1ff1bbc3fe17e4676bda&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=89998272253bb98eae0f3debef65510fbf3da55aa15d1ff1bbc3fe17e4676bda&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/MobileRouteGuard/index.tsx:
##########
@@ -0,0 +1,114 @@
+/**
+ * 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 { ReactNode, useEffect, useState } from 'react';
+import { useLocation } from 'react-router-dom';
+import { Grid } from '@superset-ui/core/components';
+import MobileUnsupported from 'src/pages/MobileUnsupported';
+
+const { useBreakpoint } = Grid;
+
+/**
+ * Routes that are supported on mobile devices.
+ * All other routes will show the MobileUnsupported page on mobile.
+ */
+export const MOBILE_SUPPORTED_ROUTES: RegExp[] = [
+ // Authentication
+ /^\/login\/?$/,
+ /^\/logout\/?$/,
+ /^\/register\/?/,
+
+ // Welcome / Home page
+ /^\/superset\/welcome\/?$/,
+
+ // Dashboard list and individual dashboards
+ /^\/dashboard\/list\/?$/,
+ /^\/superset\/dashboard\/[^/]+\/?$/,
+
+ // The mobile unsupported page itself
+ /^\/mobile-unsupported\/?$/,
+
+ // User info
+ /^\/user_info\/?$/,
+];
+
+/**
+ * Check if a given path is supported on mobile
+ */
+export function isMobileSupportedRoute(path: string): boolean {
+ // Remove query string and hash for matching
+ const cleanPath = path.split(/[?#]/)[0];
+ return MOBILE_SUPPORTED_ROUTES.some(pattern => pattern.test(cleanPath));
+}
+
+interface MobileRouteGuardProps {
+ children: ReactNode;
+}
+
+/**
+ * A component that wraps route content and redirects to the
+ * MobileUnsupported page when accessing non-mobile-friendly
+ * routes on mobile devices.
+ *
+ * Users can bypass this by clicking "Continue anyway" which
+ * sets a sessionStorage flag.
+ */
+function MobileRouteGuard({ children }: MobileRouteGuardProps) {
+ const screens = useBreakpoint();
+ const location = useLocation();
+ const [bypassEnabled, setBypassEnabled] = useState(() => {
+ try {
+ return sessionStorage.getItem('mobile-bypass') === 'true';
+ } catch {
+ return false;
+ }
+ });
+
+ // Check for bypass flag when location changes
+ useEffect(() => {
+ try {
+ const bypass = sessionStorage.getItem('mobile-bypass') === 'true';
+ setBypassEnabled(bypass);
+ } catch {
+ // Storage access denied, keep current state
+ }
+ }, [location.pathname]);
Review Comment:
**Suggestion:** The bypass flag for mobile blocking is only refreshed when
`location.pathname` changes, but the "Continue anyway" action on the
MobileUnsupported page can push the same path again (same pathname, different
history entry). In that case, the effect that reads
`sessionStorage['mobile-bypass']` does not run, so `bypassEnabled` stays false
and the user remains stuck on the unsupported screen even after opting to
continue. Use the full `location` object as the effect dependency so the bypass
state is re-read whenever navigation occurs, even if the pathname string is
unchanged. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Mobile users cannot immediately bypass unsupported routes.
- ⚠️ "Continue anyway" action appears ineffective on first use.
- ⚠️ MobileUnsupported session bypass logic not honored for same path.
```
</details>
```suggestion
}, [location]);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the Superset frontend and use a mobile viewport (<768px) so that
`MobileRouteGuard` is active for routes, as wired in
`superset-frontend/src/views/App.tsx:74-88` where each route is wrapped with
`<MobileRouteGuard>`.
2. Navigate directly to an unsupported mobile route, for example the chart
list URL used
in tests (`URL.CHART_LIST` in
`superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts:55-56`,
typically
`/chart/list/`). On mobile, `MobileRouteGuard`
(`superset-frontend/src/components/MobileRouteGuard/index.tsx:71-112`)
detects `isMobile
=== true` and `isMobileSupportedRoute(location.pathname) === false`, so it
renders
`<MobileUnsupported originalPath={location.pathname + location.search} />`
instead of the
chart list.
3. On the `MobileUnsupported` page
(`superset-frontend/src/pages/MobileUnsupported/index.tsx:37-215`), click
the "Continue
anyway" link, which triggers `handleContinueAnyway` at lines 57-65. This
function sets
`sessionStorage.setItem('mobile-bypass', 'true')` (lines 59-63) and then
calls
`history.push(fromPath)` where `fromPath` is the original unsupported URL
(lines 43-47),
e.g. `/chart/list/` again.
4. After this push to the same pathname, `MobileRouteGuard` remains mounted
for the same
route; its effect at `index.tsx:82-90` depends only on
`[location.pathname]`, so React
sees the same pathname string and does not re-run the effect to re-read
`sessionStorage['mobile-bypass']`. The `bypassEnabled` state stays `false`,
so the guard
continues to treat the route as non-bypassed and renders `MobileUnsupported`
again,
leaving the user stuck on the unsupported view until they navigate to a
different path and
back (which causes a remount and fresh `useState` initialization).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/MobileRouteGuard/index.tsx
**Line:** 90:90
**Comment:**
*Logic Error: The bypass flag for mobile blocking is only refreshed
when `location.pathname` changes, but the "Continue anyway" action on the
MobileUnsupported page can push the same path again (same pathname, different
history entry). In that case, the effect that reads
`sessionStorage['mobile-bypass']` does not run, so `bypassEnabled` stays false
and the user remains stuck on the unsupported screen even after opting to
continue. Use the full `location` object as the effect dependency so the bypass
state is re-read whenever navigation occurs, even if the pathname string is
unchanged.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=0b3f5005366495804e3170db4191b1b1a0162472bb8e11e85fbee5676f4a0363&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=0b3f5005366495804e3170db4191b1b1a0162472bb8e11e85fbee5676f4a0363&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]
