Copilot commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r2824414024
##########
superset-frontend/src/pages/DashboardList/index.tsx:
##########
@@ -730,7 +735,24 @@ function DashboardList(props: DashboardListProps) {
}
return (
<>
- <SubMenu name={t('Dashboards')} buttons={subMenuButtons} />
+ <SubMenu
+ name={t('Dashboards')}
+ buttons={subMenuButtons}
+ leftIcon={
+ !isNotMobile ? (
+ <Button
+ buttonStyle="link"
+ onClick={() => setMobileFiltersOpen(true)}
+ css={css`
+ padding: 0;
+ margin-right: ${theme.sizeUnit * 2}px;
+ `}
+ >
+ <Icons.SearchOutlined iconSize="l" />
+ </Button>
Review Comment:
The mobile search button is icon-only but doesn’t have an accessible name.
Add an `aria-label` (and/or a tooltip) so screen readers can announce what this
control does.
```suggestion
<Tooltip title={t('Search')}>
<Button
buttonStyle="link"
onClick={() => setMobileFiltersOpen(true)}
css={css`
padding: 0;
margin-right: ${theme.sizeUnit * 2}px;
`}
aria-label={t('Search')}
>
<Icons.SearchOutlined iconSize="l" />
</Button>
</Tooltip>
```
##########
superset/static/service-worker.js:
##########
@@ -1,27 +1,1471 @@
-/**
- * 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.
+/*
+ * ATTENTION: An "eval-source-map" devtool has been used.
+ * This devtool is neither made for production nor for readable output files.
+ * It uses "eval()" calls to create a separate source file with attached
SourceMaps in the browser devtools.
+ * If you are trying to read the output file, select a different devtool
(https://webpack.js.org/configuration/devtool/)
+ * or disable the default devtool with "devtool: false".
+ * If you are looking for production-ready output files, see mode:
"production" (https://webpack.js.org/configuration/mode/).
*/
+/******/ (() => { // webpackBootstrap
+/******/ "use strict";
+/******/ var __webpack_modules__ = ({
Review Comment:
This file appears to have been replaced by a webpack dev build output
("eval-source-map", HMR/runtime, dev-server client, etc.). Shipping a service
worker bundle that uses eval/HMR is not production-safe and will likely break
caching/offline behavior and violate CSP/security expectations. Revert this to
the intended minimal/production service worker source (or ensure the build
emits a production-mode artifact) and avoid committing dev-server output into
`superset/static/`.
##########
superset-frontend/src/pages/MobileUnsupported/index.tsx:
##########
@@ -0,0 +1,215 @@
+/**
+ * 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;
+
+ const handleViewDashboards = useCallback(() => {
+ history.push('/dashboard/list/');
+ }, [history]);
+
+ const handleGoHome = useCallback(() => {
+ history.push('/superset/welcome/');
+ }, [history]);
+
+ const handleContinueAnyway = useCallback(() => {
+ // Store preference in sessionStorage so we don't keep redirecting
+ sessionStorage.setItem('mobile-bypass', 'true');
Review Comment:
`sessionStorage.setItem` can throw (storage disabled/quota). Wrap this in
try/catch (or use a shared safe-storage helper) so the "Continue anyway" flow
still works without crashing.
```suggestion
try {
if (typeof window !== 'undefined' && window.sessionStorage) {
window.sessionStorage.setItem('mobile-bypass', 'true');
}
} catch {
// Ignore storage errors so the "Continue anyway" flow still works
}
```
##########
superset-frontend/src/components/MobileRouteGuard/index.tsx:
##########
@@ -0,0 +1,104 @@
+/**
+ * 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(false);
+
+ // Check for bypass flag on mount and when location changes
+ useEffect(() => {
+ const bypass = sessionStorage.getItem('mobile-bypass') === 'true';
+ setBypassEnabled(bypass);
+ }, [location.pathname]);
Review Comment:
Direct `sessionStorage` access can throw in some environments (privacy mode,
disabled storage). Other parts of the codebase wrap storage access in
try/catch. Consider initializing `bypassEnabled` from storage via a safe helper
(try/catch) in the `useState` initializer and re-checking on route changes, to
avoid both potential crashes and a first-render flicker where bypass users
briefly see the unsupported page.
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -549,6 +615,33 @@ export function ListView<T extends object = any>({
)}
</div>
</div>
+
+ {/* Mobile filter drawer */}
+ {filterable && setMobileFiltersOpen && (
+ <Drawer
+ title={mobileFiltersDrawerTitle || t('Search')}
+ placement="left"
+ onClose={() => setMobileFiltersOpen(false)}
+ open={mobileFiltersOpen}
+ width={300}
+ >
+ <MobileFilterDrawerContent>
+ <FilterControls
+ ref={filterControlsRef}
+ filters={filters}
+ internalFilters={internalFilters}
+ updateFilterValue={applyFilterValue}
+ />
+ {viewMode === 'card' && cardSortSelectOptions && (
+ <CardSortSelect
+ initialSort={sortBy}
+ onChange={(value: SortColumn[]) => setSortBy(value)}
+ options={cardSortSelectOptions}
+ />
+ )}
+ </MobileFilterDrawerContent>
Review Comment:
When `setMobileFiltersOpen` is provided, `FilterControls` is rendered in the
header *and* again inside the `Drawer` (sharing the same `ref`). Because AntD
`Drawer` content is typically mounted even when closed, this can cause
duplicated DOM, confusing refs, and extra work. Consider rendering only one
`FilterControls` instance (e.g., move it into the drawer for mobile only using
a breakpoint, or set `destroyOnClose`/conditionally render drawer content only
when `mobileFiltersOpen` is true).
```suggestion
destroyOnClose
>
{mobileFiltersOpen && (
<MobileFilterDrawerContent>
<FilterControls
ref={filterControlsRef}
filters={filters}
internalFilters={internalFilters}
updateFilterValue={applyFilterValue}
/>
{viewMode === 'card' && cardSortSelectOptions && (
<CardSortSelect
initialSort={sortBy}
onChange={(value: SortColumn[]) => setSortBy(value)}
options={cardSortSelectOptions}
/>
)}
</MobileFilterDrawerContent>
)}
```
##########
superset-frontend/src/pages/MobileUnsupported/index.tsx:
##########
@@ -0,0 +1,215 @@
+/**
+ * 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;
+
+ const handleViewDashboards = useCallback(() => {
+ history.push('/dashboard/list/');
+ }, [history]);
+
+ const handleGoHome = useCallback(() => {
+ history.push('/superset/welcome/');
+ }, [history]);
+
+ const handleContinueAnyway = useCallback(() => {
+ // Store preference in sessionStorage so we don't keep redirecting
+ sessionStorage.setItem('mobile-bypass', 'true');
+ history.push(fromPath);
+ }, [history, fromPath]);
+
+ // If we're not on mobile anymore (user rotated/resized), redirect back
+ // This is optional - could remove if we want to keep them on this page
Review Comment:
The comment says the page will "redirect back" when not on mobile anymore,
but the implementation only shows a hint and does not redirect. Either
implement the redirect behavior or update/remove the comment to match what the
code actually does.
```suggestion
// Determine if we're at or above the 'md' breakpoint (i.e. not on mobile)
```
##########
superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx:
##########
@@ -187,6 +201,46 @@ export const useHeaderActionsMenu = ({
const menuItems: MenuItem[] = [];
+ // Mobile-only: show dashboard info items in menu
+ if (isMobile && !editMode) {
+ // Favorite toggle
+ if (saveFaveStar) {
+ menuItems.push({
+ key: 'toggle-favorite',
+ label: isStarred ? t('Remove from favorites') : t('Add to
favorites'),
+ });
+ }
+
+ // Published status
+ menuItems.push({
+ key: 'status-info',
+ label: isPublished ? t('Status: Published') : t('Status: Draft'),
+ disabled: true,
+ });
+
+ // Owner info
+ const ownerNames =
+ dashboardInfo?.owners?.length > 0
+ ? dashboardInfo.owners.map(getOwnerName).join(', ')
+ : t('None');
+ menuItems.push({
+ key: 'owner-info',
+ label: `${t('Owner')}: ${ownerNames}`,
+ disabled: true,
+ });
+
+ // Last modified
+ const modifiedBy =
+ getOwnerName(dashboardInfo?.changed_by) || t('Not available');
+ menuItems.push({
+ key: 'modified-info',
+ label: `${t('Modified')} ${dashboardInfo?.changed_on_delta_humanized
|| ''} ${t('by')} ${modifiedBy}`,
Review Comment:
The "Modified … by …" label is built by concatenating multiple translated
fragments and optional values, which is hard to translate correctly and can
introduce awkward spacing/word order in non-English locales. Prefer a single
`t()` string with interpolation placeholders (and handle the missing delta case
explicitly) so translators can control grammar and formatting.
```suggestion
const changedOnDelta = dashboardInfo?.changed_on_delta_humanized;
const modifiedLabel = changedOnDelta
? t('Modified %(delta)s by %(modifiedBy)s', {
delta: changedOnDelta,
modifiedBy,
})
: t('Modified by %(modifiedBy)s', {
modifiedBy,
});
menuItems.push({
key: 'modified-info',
label: modifiedLabel,
```
--
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]
