codeant-ai-for-open-source[bot] commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r3696260003
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -527,7 +546,15 @@ const DashboardBuilder = () => {
const headerContent = useMemo(
() => (
<>
- {!hideDashboardHeader && <DashboardHeader />}
+ {!hideDashboardHeader && (
+ <DashboardHeader
+ onOpenMobileFilters={
+ !isNotMobile && nativeFiltersEnabled && hasFilters
+ ? () => setMobileFiltersOpen(true)
+ : undefined
+ }
Review Comment:
**Suggestion:** The mobile filter trigger is only supplied when `hasFilters`
is true, but `useNativeFilters` defines that value from `filterValues` alone.
Native filters are also enabled and rendered when a dashboard contains only
chart customizations, and the vertical filter bar explicitly displays controls
for either filters or customizations. Such dashboards will render customization
controls in the drawer but provide no mobile control that can open it. Use the
same filters-or-customizations condition for the trigger as the filter bar.
[incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Mobile users cannot open customization-only dashboard controls.
- ⚠️ Affected dashboards expose controls without an accessible trigger.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b2a5207a13574e57b0b6d13e9d18bd14&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=b2a5207a13574e57b0b6d13e9d18bd14&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:** 551:555
**Comment:**
*Incorrect Condition Logic: The mobile filter trigger is only supplied
when `hasFilters` is true, but `useNativeFilters` defines that value from
`filterValues` alone. Native filters are also enabled and rendered when a
dashboard contains only chart customizations, and the vertical filter bar
explicitly displays controls for either filters or customizations. Such
dashboards will render customization controls in the drawer but provide no
mobile control that can open it. Use the same filters-or-customizations
condition for the trigger as the filter bar.
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=9c3f1b106c54fccb916c0567e70e0b22f9cd485f810b7bb5762a470b62d22801&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=9c3f1b106c54fccb916c0567e70e0b22f9cd485f810b7bb5762a470b62d22801&reaction=dislike'>👎</a>
##########
superset-frontend/src/hooks/useIsMobile.ts:
##########
@@ -0,0 +1,71 @@
+/**
+ * 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 { useEffect, useState } from 'react';
+import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import { useTheme } from '@apache-superset/core/theme';
+
+// Matches antd's screenSMMax token; used only when no theme is in scope.
+const FALLBACK_MOBILE_MAX_WIDTH = 767;
+
+/**
+ * Whether the mobile consumption-only experience is enabled for this
+ * deployment. Non-hook variant for use inside styled-component
+ * interpolations; prefer `useIsMobile` in components.
+ */
+export function isMobileConsumptionEnabled(): boolean {
+ return isFeatureEnabled(FeatureFlag.MobileConsumptionMode);
+}
+
+/**
+ * Returns true when MOBILE_CONSUMPTION_MODE is enabled AND the viewport is
+ * at or below the theme's `screenSMMax` breakpoint. All mobile-specific
+ * behavior (route guarding, consumption-only chrome, drawer navigation)
+ * should key off this hook so the flag remains a single kill switch.
+ *
+ * The matchMedia subscription is only installed when the flag is on, and
+ * state only changes when the match flips, so with the flag off (or on
+ * desktop) this hook never causes a re-render — consumers are inert.
+ *
+ * The initial value is always false (desktop), so the first paint never
+ * takes the mobile branch by accident.
+ */
+export function useIsMobile(): boolean {
+ const enabled = isMobileConsumptionEnabled();
+ const theme = useTheme();
+ const maxWidth = theme?.screenSMMax ?? FALLBACK_MOBILE_MAX_WIDTH;
+ const [isSmallScreen, setIsSmallScreen] = useState(false);
+
+ useEffect(() => {
+ if (!enabled) {
+ return undefined;
+ }
+ const mediaQuery = window.matchMedia(`(max-width: ${maxWidth}px)`);
+ const update = () => setIsSmallScreen(mediaQuery.matches);
Review Comment:
**Suggestion:** The hook always returns a desktop value on its first render
and only evaluates `matchMedia` in an effect. `MobileRouteGuard` therefore
renders the original route component for one commit on every mobile load before
replacing it with the unsupported screen, allowing unsupported pages to mount
and run their effects or API requests and causing a visible desktop-to-mobile
flash. Initialize the media-query state synchronously when `window.matchMedia`
is available, while retaining the effect for subscription updates. [stale
reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unsupported mobile routes briefly mount desktop content.
- ⚠️ Route mount effects can issue unnecessary API requests.
- ⚠️ Users may observe a desktop-to-unsupported-screen flash.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2641d4141d14324b80ae3a87c620a2a&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=d2641d4141d14324b80ae3a87c620a2a&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/hooks/useIsMobile.ts
**Line:** 50:59
**Comment:**
*Stale Reference: The hook always returns a desktop value on its first
render and only evaluates `matchMedia` in an effect. `MobileRouteGuard`
therefore renders the original route component for one commit on every mobile
load before replacing it with the unsupported screen, allowing unsupported
pages to mount and run their effects or API requests and causing a visible
desktop-to-mobile flash. Initialize the media-query state synchronously when
`window.matchMedia` is available, while retaining the effect for subscription
updates.
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=3e63673b8ab50898ebef3c86328266a9a8c4fef1c1e3ae99adbb5b4e63a054ce&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=3e63673b8ab50898ebef3c86328266a9a8c4fef1c1e3ae99adbb5b4e63a054ce&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -644,6 +654,64 @@ const RightMenu = ({
handleLogout,
]);
+ // Build mobile menu items - consumption only (no create/admin actions)
+ const mobileMenuItems = useMemo(() => {
+ const items: MenuItem[] = [];
+
+ // Add Dashboards link at top (from main menu)
+ // Match on the FAB-internal `name`, which is stable across locales
+ // (`label` is translated and would break in non-English deployments)
+ const dashboardsMenu = menu?.find(item => item.name === 'Dashboards');
+ if (dashboardsMenu) {
+ const dashboardUrl = dashboardsMenu.url || '/dashboard/list/';
+ items.push({
+ key: 'dashboards',
+ label: isFrontendRoute(dashboardUrl) ? (
+ <Link to={dashboardUrl}>{t('Dashboards')}</Link>
+ ) : (
+ <Typography.Link href={dashboardUrl}>
+ {t('Dashboards')}
+ </Typography.Link>
+ ),
+ icon: <Icons.DashboardOutlined />,
+ });
+ }
+
+ // Add theme menu (flatten children directly)
+ menuItems.forEach(item => {
+ if (!item || !('key' in item)) return;
+
+ // Only include theme-sub-menu and language picker
+ if (item.key === 'theme-sub-menu' || item.key === 'language-picker') {
+ items.push({ type: 'divider', key: `divider-before-${item.key}` });
Review Comment:
**Suggestion:** The language picker created by `useLanguageMenuItems` uses
the key `language-submenu`, not `language-picker`. Therefore the mobile menu
never includes the language picker when language selection is enabled, even
though the desktop menu contains it. Match the actual menu key or use the menu
item's identity rather than this stale key. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Mobile users cannot change language from the navigation drawer.
- ⚠️ Desktop language selection remains available.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=edfc141ccda94ac88d807dc197678506&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=edfc141ccda94ac88d807dc197678506&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/features/home/RightMenu.tsx
**Line:** 684:686
**Comment:**
*Api Mismatch: The language picker created by `useLanguageMenuItems`
uses the key `language-submenu`, not `language-picker`. Therefore the mobile
menu never includes the language picker when language selection is enabled,
even though the desktop menu contains it. Match the actual menu key or use the
menu item's identity rather than this stale key.
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=7794381b6e4198a8b38c14587a7a5bb7d1424632517ab93726eca88c36b5d85f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37141&comment_hash=7794381b6e4198a8b38c14587a7a5bb7d1424632517ab93726eca88c36b5d85f&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]
