You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2026/04/22 01:21:50 UTC
(superset) branch subdirectory-bugs created (now 36a32e7b496)
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a change to branch subdirectory-bugs
in repository https://gitbox.apache.org/repos/asf/superset.git
at 36a32e7b496 fix(frontend): prevent duplicate subdirectory prefix in dashboard fullscreen and saved queries navigation
This branch includes the following new commits:
new 36a32e7b496 fix(frontend): prevent duplicate subdirectory prefix in dashboard fullscreen and saved queries navigation
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: fix(frontend): prevent duplicate subdirectory prefix in dashboard fullscreen and saved queries navigation
Posted by el...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a commit to branch subdirectory-bugs
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 36a32e7b496e171317b29abe287e9efd32a0f3af
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Wed Apr 22 01:21:35 2026 +0000
fix(frontend): prevent duplicate subdirectory prefix in dashboard fullscreen and saved queries navigation
React Router v5 prepends its basename automatically on history.push/replace
and <Link to>. Passing a path already prefixed by applicationRoot() doubled
the subdirectory on deployments using SUPERSET_APP_ROOT.
- Dashboard fullscreen toggle (sc-103933): switch from window.location.pathname
to useLocation().pathname so history.replace receives a router-relative path
- Saved Queries "+ Query" / label links (sc-103661): remove makeUrl() from
history.push and <Link to>; retain makeUrl() for window.open and clipboard
where the full browser-absolute path is required
Adds unit tests (Header.test.tsx, SavedQueryList.test.tsx) and Playwright E2E
specs (fullscreen.spec.ts, saved-queries.spec.ts) covering both fixes.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
.../playwright/tests/dashboard/fullscreen.spec.ts | 117 +++++++++++++++++++++
.../playwright/tests/sqllab/saved-queries.spec.ts | 59 +++++++++++
.../dashboard/components/Header/Header.test.tsx | 48 +++++++++
.../Header/useHeaderActionsDropdownMenu.tsx | 13 ++-
.../pages/SavedQueryList/SavedQueryList.test.tsx | 89 +++++++++++++++-
.../src/pages/SavedQueryList/index.tsx | 13 ++-
6 files changed, 331 insertions(+), 8 deletions(-)
diff --git a/superset-frontend/playwright/tests/dashboard/fullscreen.spec.ts b/superset-frontend/playwright/tests/dashboard/fullscreen.spec.ts
new file mode 100644
index 00000000000..a643c7d222f
--- /dev/null
+++ b/superset-frontend/playwright/tests/dashboard/fullscreen.spec.ts
@@ -0,0 +1,117 @@
+/**
+ * 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.
+ */
+
+/**
+ * Fullscreen toggle E2E tests.
+ *
+ * Regression tests for subdirectory deployments: when Superset runs at a path
+ * prefix (e.g. /pcs), the fullscreen toggle must not duplicate the prefix in
+ * the URL (sc-103933).
+ */
+
+import { testWithAssets as test, expect } from '../../helpers/fixtures';
+import { DashboardPage } from '../../pages/DashboardPage';
+import { apiPostDashboard } from '../../helpers/api/dashboard';
+import { TIMEOUT } from '../../utils/constants';
+
+test('toggling fullscreen adds standalone param without duplicating path segments', async ({
+ page,
+ testAssets,
+}) => {
+ test.setTimeout(TIMEOUT.SLOW_TEST);
+
+ // Create a minimal dashboard to test against
+ const dashboardResponse = await apiPostDashboard(page, {
+ dashboard_title: `test_fullscreen_${Date.now()}`,
+ published: true,
+ });
+ expect(dashboardResponse.status()).toBe(201);
+ const { id: dashboardId } = await dashboardResponse.json();
+ testAssets.trackDashboard(dashboardId);
+
+ const dashboardPage = new DashboardPage(page);
+ await dashboardPage.gotoById(dashboardId);
+ await dashboardPage.waitForLoad();
+
+ // Record the pathname before toggling (should be something like /superset/dashboard/N/)
+ const pathBefore = new URL(page.url()).pathname;
+
+ // Open the three-dot menu and click "Enter fullscreen"
+ await dashboardPage.openHeaderActionsMenu();
+ await page.getByText('Enter fullscreen').click();
+
+ // Wait for the URL to update with standalone param
+ await page.waitForURL(/standalone=1/, { timeout: TIMEOUT.API_RESPONSE });
+
+ const urlAfter = new URL(page.url());
+
+ // The pathname must not have grown — i.e. no segment was duplicated
+ expect(urlAfter.pathname).toBe(pathBefore);
+
+ // standalone=1 must be present
+ expect(urlAfter.searchParams.get('standalone')).toBe('1');
+
+ // The dashboard header must still be visible (not a blank page)
+ await expect(
+ page.locator('[data-test="dashboard-header-container"]'),
+ ).toBeVisible();
+});
+
+test('toggling fullscreen off removes standalone param without duplicating path segments', async ({
+ page,
+ testAssets,
+}) => {
+ test.setTimeout(TIMEOUT.SLOW_TEST);
+
+ const dashboardResponse = await apiPostDashboard(page, {
+ dashboard_title: `test_fullscreen_exit_${Date.now()}`,
+ published: true,
+ });
+ expect(dashboardResponse.status()).toBe(201);
+ const { id: dashboardId } = await dashboardResponse.json();
+ testAssets.trackDashboard(dashboardId);
+
+ const dashboardPage = new DashboardPage(page);
+ // Navigate directly into fullscreen mode
+ await page.goto(`superset/dashboard/${dashboardId}/?standalone=1`);
+ await dashboardPage.waitForLoad();
+
+ const pathBefore = new URL(page.url()).pathname;
+
+ // Open the three-dot menu and click "Exit fullscreen"
+ await dashboardPage.openHeaderActionsMenu();
+ await page.getByText('Exit fullscreen').click();
+
+ // Wait for standalone param to disappear
+ await page.waitForFunction(
+ () => !new URL(window.location.href).searchParams.has('standalone'),
+ { timeout: TIMEOUT.API_RESPONSE },
+ );
+
+ const urlAfter = new URL(page.url());
+
+ // Pathname must be unchanged — no duplicated segment
+ expect(urlAfter.pathname).toBe(pathBefore);
+ expect(urlAfter.searchParams.get('standalone')).toBeNull();
+
+ // Dashboard must still be rendered
+ await expect(
+ page.locator('[data-test="dashboard-header-container"]'),
+ ).toBeVisible();
+});
diff --git a/superset-frontend/playwright/tests/sqllab/saved-queries.spec.ts b/superset-frontend/playwright/tests/sqllab/saved-queries.spec.ts
new file mode 100644
index 00000000000..a618ab274b4
--- /dev/null
+++ b/superset-frontend/playwright/tests/sqllab/saved-queries.spec.ts
@@ -0,0 +1,59 @@
+/**
+ * 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.
+ */
+
+/**
+ * Saved Queries list E2E tests.
+ *
+ * Regression tests for subdirectory deployments: "+ Query" and query label
+ * links must navigate to SQL Lab without duplicating the path prefix (sc-103661).
+ */
+
+import { test, expect } from '@playwright/test';
+import { URL as APP_URLS } from '../../utils/urls';
+import { TIMEOUT } from '../../utils/constants';
+
+test('+ Query button navigates to SQL Lab without duplicating path segments', async ({
+ page,
+}) => {
+ test.setTimeout(TIMEOUT.SLOW_TEST);
+
+ await page.goto(APP_URLS.SAVED_QUERIES_LIST);
+
+ // Wait for the list to load
+ await page.waitForSelector('[data-test="saved_query-list-view"]', {
+ timeout: TIMEOUT.PAGE_LOAD,
+ });
+
+ // Click the "+ Query" button in the submenu (accessible name is "plus Query" due to icon)
+ await page.getByRole('button', { name: /Query/i }).click();
+
+ // Wait for SQL Lab to load
+ await page.waitForURL(/sqllab/, { timeout: TIMEOUT.PAGE_LOAD });
+
+ const url = new URL(page.url());
+
+ // No path segment should appear twice consecutively — catches the double-prefix bug
+ // where React Router prepends the basename a second time (e.g. /superset/superset/sqllab).
+ expect(url.pathname).not.toMatch(/\/(\w+)\/\1\//);
+
+ // SQL Lab editor must be visible (not a blank page)
+ await expect(page.locator('[data-test="sql-editor-tabs"]')).toBeVisible({
+ timeout: TIMEOUT.PAGE_LOAD,
+ });
+});
diff --git a/superset-frontend/src/dashboard/components/Header/Header.test.tsx b/superset-frontend/src/dashboard/components/Header/Header.test.tsx
index 0bdd3f6fcee..71c52c01725 100644
--- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx
+++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx
@@ -31,6 +31,20 @@ import { DASHBOARD_HEADER_ID } from '../../util/constants';
import { UPDATE_COMPONENTS } from '../../actions/dashboardLayout';
import { AutoRefreshStatus } from '../../types/autoRefresh';
+const mockHistoryReplace = jest.fn();
+jest.mock('react-router-dom', () => ({
+ ...jest.requireActual('react-router-dom'),
+ useHistory: () => ({
+ replace: mockHistoryReplace,
+ }),
+ useLocation: jest.fn(() => ({
+ pathname: '/dashboard',
+ search: '?standalone=1',
+ hash: '',
+ state: undefined,
+ })),
+}));
+
const initialState = {
dashboardInfo: {
id: 1,
@@ -223,6 +237,13 @@ beforeAll(() => {
beforeEach(() => {
jest.clearAllMocks();
+ const { useLocation } = jest.requireMock('react-router-dom');
+ useLocation.mockReturnValue({
+ pathname: '/dashboard',
+ search: '?standalone=1',
+ hash: '',
+ state: undefined,
+ });
(useUnsavedChangesPrompt as jest.Mock).mockReturnValue({
showModal: false,
@@ -968,3 +989,30 @@ test('should sync theme ref when navigating between dashboards', async () => {
expect(setUnsavedChanges).toHaveBeenCalledTimes(0);
});
});
+
+test('should not duplicate subdirectory prefix when toggling fullscreen', async () => {
+ const { useLocation } = jest.requireMock('react-router-dom');
+ // Simulate React Router with basename=/pcs: useLocation returns path relative to basename
+ useLocation.mockReturnValue({
+ pathname: '/dashboard',
+ search: '?standalone=1',
+ hash: '',
+ state: undefined,
+ });
+ // Simulate browser URL including the subdirectory prefix
+ window.history.pushState({}, 'Test page', '/pcs/dashboard?standalone=1');
+
+ setup();
+ await openActionsDropdown();
+ userEvent.click(screen.getByText('Exit fullscreen'));
+
+ // history.replace must be called with the Router-relative path, not window.location.pathname.
+ // If the subdirectory prefix (/pcs) were included, React Router would prepend it again,
+ // producing /pcs/pcs/dashboard (the bug). The path must start with /dashboard, not /pcs/.
+ expect(mockHistoryReplace).toHaveBeenCalledWith(
+ expect.not.stringMatching(/^\/pcs\//),
+ );
+ expect(mockHistoryReplace).toHaveBeenCalledWith(
+ expect.stringMatching(/^\/dashboard(\?|$)/),
+ );
+});
diff --git a/superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx b/superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx
index 121759846c8..102a0a70581 100644
--- a/superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx
+++ b/superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx
@@ -19,7 +19,7 @@
import type { Dispatch, ReactElement, SetStateAction } from 'react';
import { useState, useEffect, useCallback, useMemo } from 'react';
import { useSelector } from 'react-redux';
-import { useHistory } from 'react-router-dom';
+import { useHistory, useLocation } from 'react-router-dom';
import { Menu, MenuItem } from '@superset-ui/core/components/Menu';
import { t } from '@apache-superset/core/translation';
import { isEmpty } from 'lodash';
@@ -75,6 +75,7 @@ export const useHeaderActionsMenu = ({
const [isDropdownVisible, setIsDropdownVisible] = useState(false);
const { canExportImage } = usePermissions();
const history = useHistory();
+ const location = useLocation();
const directPathToChild = useSelector(
(state: RootState) => state.dashboardState.directPathToChild,
);
@@ -101,8 +102,11 @@ export const useHeaderActionsMenu = ({
case MenuKeys.ToggleFullscreen: {
const isCurrentlyStandalone =
Number(getUrlParam(URL_PARAMS.standalone)) === 1;
+ // Use location.pathname from React Router (relative to basename) rather than
+ // window.location.pathname to avoid duplicating the subdirectory prefix when
+ // history.replace prepends it again.
const url = getDashboardUrl({
- pathname: window.location.pathname,
+ pathname: location.pathname,
filters: getActiveFilters(),
hash: window.location.hash,
standalone: isCurrentlyStandalone ? null : 1,
@@ -125,6 +129,7 @@ export const useHeaderActionsMenu = ({
showRefreshModal,
manageEmbedded,
history,
+ location,
],
);
@@ -133,6 +138,10 @@ export const useHeaderActionsMenu = ({
[dashboardTitle],
);
+ // window.location.pathname is intentional here: this URL is used for sharing
+ // (email, embed, copy link) and must be a full browser-absolute path that
+ // includes the application root. Do NOT replace with useLocation().pathname —
+ // that would strip the subdirectory prefix and produce a broken share link.
const url = useMemo(
() =>
getDashboardUrl({
diff --git a/superset-frontend/src/pages/SavedQueryList/SavedQueryList.test.tsx b/superset-frontend/src/pages/SavedQueryList/SavedQueryList.test.tsx
index 12cf7e6aba7..3464d41b570 100644
--- a/superset-frontend/src/pages/SavedQueryList/SavedQueryList.test.tsx
+++ b/superset-frontend/src/pages/SavedQueryList/SavedQueryList.test.tsx
@@ -25,11 +25,20 @@ import {
fireEvent,
waitFor,
} from 'spec/helpers/testing-library';
-import { MemoryRouter } from 'react-router-dom';
+import { createMemoryHistory } from 'history';
+import { MemoryRouter, Router } from 'react-router-dom';
import { QueryParamProvider } from 'use-query-params';
import { ReactRouter5Adapter } from 'use-query-params/adapters/react-router-5';
import SavedQueryList from '.';
+jest.mock('src/utils/getBootstrapData', () => ({
+ __esModule: true,
+ default: jest.requireActual('src/utils/getBootstrapData').default,
+ applicationRoot: jest.fn(() => '/superset'),
+ staticAssetsPrefix: jest.requireActual('src/utils/getBootstrapData')
+ .staticAssetsPrefix,
+}));
+
// Increase default timeout
jest.setTimeout(30000);
@@ -102,6 +111,30 @@ const renderList = (props = {}, storeOverrides = {}) =>
},
);
+// Renders with a real memory history so we can spy on push/replace without
+// breaking use-query-params (which needs a real location from router context).
+const renderListWithHistory = (props = {}, storeOverrides = {}) => {
+ const history = createMemoryHistory();
+ const result = render(
+ <Router history={history}>
+ <QueryParamProvider adapter={ReactRouter5Adapter}>
+ <SavedQueryList user={mockUser} {...props} />
+ </QueryParamProvider>
+ </Router>,
+ {
+ useRedux: true,
+ store: configureStore([thunk])({
+ user: {
+ ...mockUser,
+ roles: { Admin: [['can_write', 'SavedQuery']] },
+ },
+ ...storeOverrides,
+ }),
+ },
+ );
+ return { ...result, history };
+};
+
// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('SavedQueryList', () => {
beforeEach(() => {
@@ -212,6 +245,60 @@ describe('SavedQueryList', () => {
});
});
+ test('window.open for "open in new tab" retains the subdirectory prefix', async () => {
+ // When a query row is Cmd/Ctrl-clicked the component calls
+ // window.open(makeUrl(...)) — the full application-root-prefixed URL is needed
+ // because window.open operates in browser URL space, not React Router namespace.
+ // This test guards against accidentally removing makeUrl from the window.open call.
+ const mockWindowOpen = jest
+ .spyOn(window, 'open')
+ .mockImplementation(() => null);
+
+ renderList();
+ await screen.findByTestId('saved_query-list-view');
+ await screen.findByText(mockQueries[0].label);
+
+ const editButtons = await screen.findAllByTestId('edit-action');
+ fireEvent.click(editButtons[0], { metaKey: true });
+
+ expect(mockWindowOpen).toHaveBeenCalledWith(
+ expect.stringContaining('/superset/sqllab'),
+ );
+ mockWindowOpen.mockRestore();
+ });
+
+ test('+ Query button navigates without duplicating subdirectory prefix', async () => {
+ // applicationRoot is mocked to /superset. Without the fix, history.push would
+ // receive /superset/sqllab?new=true, and React Router would prepend /superset
+ // again, producing /superset/superset/sqllab?new=true.
+ const { history } = renderListWithHistory();
+ const pushSpy = jest.spyOn(history, 'push');
+ await screen.findByTestId('saved_query-list-view');
+
+ const queryButton = await screen.findByRole('button', { name: /query/i });
+ fireEvent.click(queryButton);
+
+ expect(pushSpy).toHaveBeenCalledWith('/sqllab?new=true');
+ expect(pushSpy).not.toHaveBeenCalledWith(
+ expect.stringContaining('/superset/sqllab'),
+ );
+ });
+
+ test('query label links do not include subdirectory prefix', async () => {
+ // <Link to> in React Router resolves relative to the basename, so passing
+ // makeUrl('/sqllab?savedQueryId=0') would produce /superset/superset/sqllab.
+ renderListWithHistory();
+ await screen.findByTestId('saved_query-list-view');
+ await screen.findByText(mockQueries[0].label);
+
+ const queryLink = screen.getByRole('link', { name: mockQueries[0].label });
+ expect(queryLink).toHaveAttribute('href', '/sqllab?savedQueryId=0');
+ expect(queryLink).not.toHaveAttribute(
+ 'href',
+ expect.stringContaining('/superset/sqllab'),
+ );
+ });
+
test('shows/hides elements based on permissions', async () => {
// Mock info response without write permission
fetchMock.removeRoute(queriesInfoEndpoint);
diff --git a/superset-frontend/src/pages/SavedQueryList/index.tsx b/superset-frontend/src/pages/SavedQueryList/index.tsx
index 65797e69b96..6692950df10 100644
--- a/superset-frontend/src/pages/SavedQueryList/index.tsx
+++ b/superset-frontend/src/pages/SavedQueryList/index.tsx
@@ -223,7 +223,7 @@ function SavedQueryList({
name: t('Query'),
buttonStyle: 'primary',
onClick: () => {
- history.push(makeUrl('/sqllab?new=true'));
+ history.push('/sqllab?new=true');
},
});
@@ -231,6 +231,10 @@ function SavedQueryList({
// Action methods
const openInSqlLab = (id: number, openInNewWindow: boolean) => {
+ // makeUrl is correct here: clipboard and window.open operate in browser URL
+ // space and need the full application-root-prefixed path. Do NOT use makeUrl
+ // for history.push or <Link to> — React Router prepends the basename itself,
+ // which would double the subdirectory prefix.
copyTextToClipboard(() =>
Promise.resolve(
`${window.location.origin}${makeUrl(`/sqllab?savedQueryId=${id}`)}`,
@@ -245,7 +249,8 @@ function SavedQueryList({
if (openInNewWindow) {
window.open(makeUrl(`/sqllab?savedQueryId=${id}`));
} else {
- history.push(makeUrl(`/sqllab?savedQueryId=${id}`));
+ // Router-relative path (no makeUrl): React Router prepends the basename.
+ history.push(`/sqllab?savedQueryId=${id}`);
}
};
@@ -338,9 +343,7 @@ function SavedQueryList({
row: {
original: { id, label },
},
- }: any) => (
- <Link to={makeUrl(`/sqllab?savedQueryId=${id}`)}>{label}</Link>
- ),
+ }: any) => <Link to={`/sqllab?savedQueryId=${id}`}>{label}</Link>,
id: 'label',
},
{