From e6a58cb04739e1cc561cca7725b4ff9def10e40b Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 14 May 2026 12:25:48 -0700 Subject: [PATCH] fix(subdirectory): navbar logo double-prefix on /superset/ deployment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA on the local /superset/ stack found the navbar brand anchor and logo image rendering with a doubled `/superset/superset/...` prefix, despite round-2's backend `Superset.route_base = ""` fix. Two collaborating defects in the frontend URL helpers: 1. `ensureStaticPrefix(brand.icon)` blindly prepended `staticAssetsPrefix()` over a value the backend had already prefixed via `url_for('static', ...)`, producing `/superset/superset/static/.../superset-logo-horiz.png` and a broken image (visible as alt text in the navbar). 2. `` in `Menu.tsx`'s SPA-route branch handed an already-rooted path to `react-router-dom `, which resolves `to` against the SPA's `` (src/views/App.tsx), producing a doubled anchor `href`. Fixes: - `ensureAppRoot` in `src/utils/pathUtils.ts`: idempotent against an already-rooted path, segment-boundary aware. Cherry-picked from upstream `86ba63b072` (PR #39534). - `ensureStaticPrefix` in `src/utils/assetUrl.ts`: mirror the same idempotence pattern against `staticAssetsPrefix()`. - New helper `stripAppRoot` in `src/utils/pathUtils.ts` (re-exported via `navigationUtils.ts`): returns a path with its leading application-root segment removed. Wired into `Menu.tsx`'s SPA-route brand link so React Router's basename resolves cleanly. Regression coverage: - pathUtils.test.ts: 8 new cases (idempotence + stripAppRoot under empty/single/nested/segment-boundary roots, plus absolute pass-through). - assetUrl.test.ts: 6 new cases (mirror coverage for ensureStaticPrefix). - Menu.test.tsx: 3 new cases — SPA-route brand link strips the root before handing to GenericLink, non-SPA branch renders single-prefix, nested-root variant. 69/69 touched-suite Jest tests green. Out of scope (queued): - DashboardList row hrefs and the broader `Dashboard.url` hard-coded `/superset/...` literals (separate workstream documented in PROJECT.md). - The four follow-on files in PR #39534 (Header/useHeaderActionsDropdownMenu.tsx, related tests) — those address the fullscreen-toggle bug, not the navbar logo, and trip a pre-existing toHaveStyleRule typecheck failure in this worktree. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/features/home/Menu.test.tsx | 145 ++++++++++++++++++ superset-frontend/src/features/home/Menu.tsx | 8 +- superset-frontend/src/utils/assetUrl.test.ts | 39 +++++ superset-frontend/src/utils/assetUrl.ts | 17 +- .../src/utils/navigationUtils.ts | 4 +- superset-frontend/src/utils/pathUtils.test.ts | 69 +++++++++ superset-frontend/src/utils/pathUtils.ts | 36 ++++- 7 files changed, 310 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/features/home/Menu.test.tsx b/superset-frontend/src/features/home/Menu.test.tsx index 6cbacebcb40..81babfb00da 100644 --- a/superset-frontend/src/features/home/Menu.test.tsx +++ b/superset-frontend/src/features/home/Menu.test.tsx @@ -25,6 +25,35 @@ import * as CoreTheme from '@apache-superset/core/theme'; import { Menu } from './Menu'; import * as getBootstrapData from 'src/utils/getBootstrapData'; +// Capture what `` receives so the SPA-route regression +// tests can assert on the value handed to react-router-dom (which applies its +// own basename in production via `` in +// src/views/App.tsx). The test harness's `` has no basename, +// so asserting on the rendered `` wouldn't catch the double-prefix. +let observedGenericLinkTo: unknown = null; +jest.mock('src/components/GenericLink', () => ({ + __esModule: true, + GenericLink: ({ + to, + children, + ...rest + }: { + to: unknown; + children: React.ReactNode; + [k: string]: unknown; + }) => { + observedGenericLinkTo = to; + return ( + )} + > + {children} + + ); + }, +})); + jest.mock('@apache-superset/core/theme', () => ({ ...jest.requireActual('@apache-superset/core/theme'), useTheme: jest.fn(), @@ -796,3 +825,119 @@ test('brand link falls back to brand.path when theme brandLogoUrl is absent', as // ensureAppRoot must have been applied: /welcome/ → /superset/welcome/ expect(brandLink).toHaveAttribute('href', '/superset/welcome/'); }); + +// Regression: the real backend emits `brand.path` and `brand.icon` already +// carrying the app root (because they pass through `url_for`). The frontend +// must not double-prefix them — neither via +// `ensureAppRoot`/`ensureStaticPrefix` nor via React Router's `basename` +// re-prepend. +// +// In production the SPA-route branch goes through ` -> +// react-router-dom `, and the Router's `basename={applicationRoot()}` +// (src/views/App.tsx) re-prepends the app root to the rendered `href`. The +// test harness's `` has no basename, so asserting on the +// rendered `` wouldn't catch the bug. Instead we mock `GenericLink` +// at module load (top of this file) and assert the path *handed to it* +// already has the root stripped — the value the production Router will then +// safely re-prepend. + +test('brand link hands a root-stripped path to GenericLink when brand.path arrives already rooted (SPA route)', async () => { + observedGenericLinkTo = null; + applicationRootMock.mockReturnValue('/superset'); + staticAssetsPrefixMock.mockReturnValue('/superset'); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + const propsWithRootedBrand = { + ...mockedProps, + isFrontendRoute: () => true, + data: { + ...mockedProps.data, + brand: { + ...mockedProps.data.brand, + path: '/superset/welcome/', + icon: '/superset/static/assets/images/superset-logo-horiz.png', + }, + }, + }; + + render(, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + // Wait for the mocked GenericLink to render. + await screen.findByRole('link', { + name: new RegExp(propsWithRootedBrand.data.brand.alt, 'i'), + }); + expect(observedGenericLinkTo).toBe('/welcome/'); +}); + +test('brand link is single-prefix when brand.path arrives already rooted (non-SPA route)', async () => { + applicationRootMock.mockReturnValue('/superset'); + staticAssetsPrefixMock.mockReturnValue('/superset'); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + const propsWithRootedBrand = { + ...mockedProps, + isFrontendRoute: () => false, + data: { + ...mockedProps.data, + brand: { + ...mockedProps.data.brand, + path: '/superset/welcome/', + icon: '/superset/static/assets/images/superset-logo-horiz.png', + }, + }, + }; + + render(, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + const brandLink = await screen.findByRole('link', { + name: new RegExp(propsWithRootedBrand.data.brand.alt, 'i'), + }); + expect(brandLink).toHaveAttribute('href', '/superset/welcome/'); + const brandImg = brandLink.querySelector('img'); + expect(brandImg).toHaveAttribute( + 'src', + '/superset/static/assets/images/superset-logo-horiz.png', + ); +}); + +test('brand link strips a nested application root before handing to GenericLink', async () => { + observedGenericLinkTo = null; + applicationRootMock.mockReturnValue('/preset/superset'); + staticAssetsPrefixMock.mockReturnValue('/preset/superset'); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + const propsWithRootedBrand = { + ...mockedProps, + isFrontendRoute: () => true, + data: { + ...mockedProps.data, + brand: { + ...mockedProps.data.brand, + path: '/preset/superset/welcome/', + icon: '/preset/superset/static/assets/images/superset-logo-horiz.png', + }, + }, + }; + + render(, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + await screen.findByRole('link', { + name: new RegExp(propsWithRootedBrand.data.brand.alt, 'i'), + }); + expect(observedGenericLinkTo).toBe('/welcome/'); +}); diff --git a/superset-frontend/src/features/home/Menu.tsx b/superset-frontend/src/features/home/Menu.tsx index afc180acad2..b1ae1bdc562 100644 --- a/superset-frontend/src/features/home/Menu.tsx +++ b/superset-frontend/src/features/home/Menu.tsx @@ -20,7 +20,7 @@ import { useState, useEffect } from 'react'; import { styled, css, useTheme } from '@apache-superset/core/theme'; import { t } from '@apache-superset/core/translation'; import { ensureStaticPrefix } from 'src/utils/assetUrl'; -import { ensureAppRoot } from 'src/utils/navigationUtils'; +import { ensureAppRoot, stripAppRoot } from 'src/utils/navigationUtils'; import { getUrlParam } from 'src/utils/urlUtils'; import { MainNav, MenuItem } from '@superset-ui/core/components/Menu'; import { Tooltip, Grid, Row, Col, Image } from '@superset-ui/core/components'; @@ -308,8 +308,12 @@ export function Menu({ // --------------------------------------------------------------------------------- // TODO: deprecate this once Theme is fully rolled out // Kept as is for backwards compatibility with the old theme system / superset_config.py + // + // `` re-prepends the app root to the + // `to` prop, so handing it an already-rooted `brand.path` would render a + // doubled `/superset/superset/...` href. Strip the root first. link = ( - + { expect(ensureStaticPrefix(absoluteResourcePath)).toBe(absoluteResourcePath); }); }); + +describe('ensureStaticPrefix should be idempotent', () => { + test('does not double-prefix a path that already starts with the static-assets prefix', () => { + staticAssetsPrefixMock.mockReturnValue('/superset'); + const alreadyRooted = + '/superset/static/assets/images/superset-logo-horiz.png'; + expect(ensureStaticPrefix(alreadyRooted)).toBe(alreadyRooted); + }); + + test('still prefixes a relative path that does not yet carry the prefix', () => { + staticAssetsPrefixMock.mockReturnValue('/superset'); + expect(ensureStaticPrefix('/static/assets/x.png')).toBe( + '/superset/static/assets/x.png', + ); + }); + + test('treats segment boundaries — `/supersetfoo` is not the same as `/superset`', () => { + staticAssetsPrefixMock.mockReturnValue('/superset'); + expect(ensureStaticPrefix('/supersetfoo/img.png')).toBe( + '/superset/supersetfoo/img.png', + ); + }); + + test('handles nested application roots', () => { + staticAssetsPrefixMock.mockReturnValue('/preset/superset'); + const alreadyRooted = '/preset/superset/static/x.png'; + expect(ensureStaticPrefix(alreadyRooted)).toBe(alreadyRooted); + }); + + test('returns the prefix itself unchanged when passed in bare', () => { + staticAssetsPrefixMock.mockReturnValue('/superset'); + expect(ensureStaticPrefix('/superset')).toBe('/superset'); + }); + + test('is a no-op when the static-assets prefix is empty (root deployment)', () => { + staticAssetsPrefixMock.mockReturnValue(''); + expect(ensureStaticPrefix('/static/x.png')).toBe('/static/x.png'); + }); +}); diff --git a/superset-frontend/src/utils/assetUrl.ts b/superset-frontend/src/utils/assetUrl.ts index a7fc2553b50..72a07eff5bd 100644 --- a/superset-frontend/src/utils/assetUrl.ts +++ b/superset-frontend/src/utils/assetUrl.ts @@ -30,10 +30,21 @@ export function assetUrl(path: string): string { /** * Returns the path prepended with the staticAssetsPrefix if the string is a relative path else it returns * the string as is. + * + * Idempotent — a path that already begins with the static-assets prefix at a + * segment boundary is returned unchanged, mirroring the dedupe pattern used by + * `ensureAppRoot` in pathUtils.ts and `SupersetClient.getUrl`. + * * @param url_or_path A url or relative path to a resource */ export function ensureStaticPrefix(url_or_path: string): string { - if (url_or_path.startsWith('/')) return assetUrl(url_or_path); - - return url_or_path; + if (!url_or_path.startsWith('/')) return url_or_path; + const prefix = staticAssetsPrefix(); + if ( + prefix && + (url_or_path === prefix || url_or_path.startsWith(`${prefix}/`)) + ) { + return url_or_path; + } + return assetUrl(url_or_path); } diff --git a/superset-frontend/src/utils/navigationUtils.ts b/superset-frontend/src/utils/navigationUtils.ts index 448ddedf08c..76faeed118e 100644 --- a/superset-frontend/src/utils/navigationUtils.ts +++ b/superset-frontend/src/utils/navigationUtils.ts @@ -21,13 +21,13 @@ import { type AnchorHTMLAttributes, type ReactElement, } from 'react'; -import { ensureAppRoot, makeUrl } from './pathUtils'; +import { ensureAppRoot, makeUrl, stripAppRoot } from './pathUtils'; // Re-export so callers that legitimately need a raw prefixed path (native // fetch, navigator.sendBeacon, image src, third-party `href` props) have a // single sanctioned import location. The static-invariant scan disallows // importing from `pathUtils` directly outside this module. -export { ensureAppRoot, makeUrl }; +export { ensureAppRoot, makeUrl, stripAppRoot }; // `navigateTo` and `navigateWithState` are declared first so the focused // helpers below can call them without tripping oxlint's no-use-before-define diff --git a/superset-frontend/src/utils/pathUtils.test.ts b/superset-frontend/src/utils/pathUtils.test.ts index fd546ff01d5..377b92193b3 100644 --- a/superset-frontend/src/utils/pathUtils.test.ts +++ b/superset-frontend/src/utils/pathUtils.test.ts @@ -229,3 +229,72 @@ test('ensureAppRoot should prefix unknown schemes instead of passing through', a // Unknown / custom schemes are treated as relative paths expect(ensureAppRoot('foo:bar')).toBe('/superset/foo:bar'); }); + +test('ensureAppRoot should be idempotent — not double-prefix an already-prefixed path', async () => { + const { ensureAppRoot } = await loadPathUtils('/superset/'); + + const once = ensureAppRoot('/sqllab'); + const twice = ensureAppRoot(once); + expect(twice).toBe(once); // /superset/sqllab, NOT /superset/superset/sqllab +}); + +test('makeUrl should be idempotent with subdirectory prefix', async () => { + const { makeUrl } = await loadPathUtils('/superset/'); + + const once = makeUrl('/sqllab?new=true'); + const twice = makeUrl(once); + expect(twice).toBe(once); // /superset/sqllab?new=true, NOT /superset/superset/sqllab?new=true +}); + +test('stripAppRoot is a no-op when application root is empty', async () => { + document.body.innerHTML = ''; + jest.resetModules(); + const { stripAppRoot } = await import('./pathUtils'); + + expect(stripAppRoot('/welcome/')).toBe('/welcome/'); + expect(stripAppRoot('/sqllab')).toBe('/sqllab'); +}); + +test('stripAppRoot removes a leading application root from an already-rooted path', async () => { + const { stripAppRoot } = await loadPathUtils('/superset/'); + + expect(stripAppRoot('/superset/welcome/')).toBe('/welcome/'); + expect(stripAppRoot('/superset/dashboard/list/')).toBe('/dashboard/list/'); +}); + +test('stripAppRoot is idempotent — a path without the root is returned unchanged', async () => { + const { stripAppRoot } = await loadPathUtils('/superset/'); + + expect(stripAppRoot('/welcome/')).toBe('/welcome/'); + expect(stripAppRoot(stripAppRoot('/superset/welcome/'))).toBe('/welcome/'); +}); + +test('stripAppRoot returns "/" when given the bare application root', async () => { + const { stripAppRoot } = await loadPathUtils('/superset/'); + + expect(stripAppRoot('/superset')).toBe('/'); +}); + +test('stripAppRoot respects segment boundaries — `/supersetfoo` is not the root', async () => { + const { stripAppRoot } = await loadPathUtils('/superset/'); + + expect(stripAppRoot('/supersetfoo/welcome/')).toBe('/supersetfoo/welcome/'); +}); + +test('stripAppRoot handles nested application roots', async () => { + const { stripAppRoot } = await loadPathUtils('/preset/superset/'); + + expect(stripAppRoot('/preset/superset/welcome/')).toBe('/welcome/'); + expect(stripAppRoot('/welcome/')).toBe('/welcome/'); +}); + +test('stripAppRoot passes absolute and protocol-relative URLs through', async () => { + const { stripAppRoot } = await loadPathUtils('/superset/'); + + expect(stripAppRoot('https://example.com/superset/welcome/')).toBe( + 'https://example.com/superset/welcome/', + ); + expect(stripAppRoot('//cdn.example.com/superset/x.png')).toBe( + '//cdn.example.com/superset/x.png', + ); +}); diff --git a/superset-frontend/src/utils/pathUtils.ts b/superset-frontend/src/utils/pathUtils.ts index 870d8aa95ab..322dc18ed04 100644 --- a/superset-frontend/src/utils/pathUtils.ts +++ b/superset-frontend/src/utils/pathUtils.ts @@ -41,7 +41,15 @@ export function ensureAppRoot(path: string): string { if (SAFE_ABSOLUTE_URL_RE.test(path) || path.startsWith('//')) { return path; } - return `${applicationRoot()}${path.startsWith('/') ? path : `/${path}`}`; + const root = applicationRoot(); + const normalizedPath = path.startsWith('/') ? path : `/${path}`; + if ( + root && + (normalizedPath === root || normalizedPath.startsWith(`${root}/`)) + ) { + return normalizedPath; + } + return `${root}${normalizedPath}`; } /** @@ -61,3 +69,29 @@ export function ensureAppRoot(path: string): string { export function makeUrl(path: string): string { return ensureAppRoot(path); } + +/** + * Returns the path with a leading application-root segment removed. Useful when + * handing an already-rooted path to a consumer that re-prepends the root — + * e.g. react-router-dom's Link resolves its `to` prop against the Router's + * `basename`, so passing an already-rooted path would result in a doubled + * prefix in the rendered anchor href. + * + * Idempotent: stripping a path that does not start with the application root + * returns it unchanged. Absolute URLs and protocol-relative URLs pass through. + * + * @param path - The path to strip + * @returns The path without a leading application-root segment + */ +export function stripAppRoot(path: string): string { + if (SAFE_ABSOLUTE_URL_RE.test(path) || path.startsWith('//')) { + return path; + } + const root = applicationRoot(); + if (!root) return path; + if (path === root) return '/'; + if (path.startsWith(`${root}/`)) { + return path.slice(root.length); + } + return path; +}