Skip to content

fix(menu): Use app prefix in Menu brand icon url #34935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+91 −19
Open

fix(menu): Use app prefix in Menu brand icon url #34935

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8c48ebb
Use app prefix in brand icon url
martyngigg Aug 25, 2025
8adfadb
Remove unused variable
martyngigg Sep 1, 2025
b4cbe56
Lint Menu.test.tsx
martyngigg Sep 1, 2025
d4b2fa7
Add alternative assetUrlIf function
martyngigg Sep 8, 2025
f01771c
Avoid newer URL.canParse to avoid browser compatability issues.
martyngigg Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions superset-frontend/src/features/home/Menu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import fetchMock from 'fetch-mock';
import { render, screen, userEvent } from 'spec/helpers/testing-library';
import setupExtensions from 'src/setup/setupExtensions';
import { getExtensionsRegistry } from '@superset-ui/core';
import * as getBootstrapData from 'src/utils/getBootstrapData';
import { Menu } from './Menu';

const dropdownItems = [
Expand Down Expand Up @@ -238,6 +239,10 @@ const notanonProps = {
};

const useSelectorMock = jest.spyOn(reactRedux, 'useSelector');
const staticAssetsPrefixMock = jest.spyOn(
getBootstrapData,
'staticAssetsPrefix',
);

fetchMock.get(
'glob:*api/v1/database/?q=(filters:!((col:allow_file_upload,opr:upload_is_enabled,value:!t)))',
Expand All @@ -247,6 +252,8 @@ fetchMock.get(
beforeEach(() => {
// setup a DOM element as a render target
useSelectorMock.mockClear();
// By default use empty app root
staticAssetsPrefixMock.mockReturnValue('');
});

test('should render', async () => {
Expand All @@ -272,22 +279,25 @@ test('should render the navigation', async () => {
expect(await screen.findByRole('navigation')).toBeInTheDocument();
});

test('should render the brand', async () => {
useSelectorMock.mockReturnValue({ roles: user.roles });
const {
data: {
brand: { alt, icon },
},
} = mockedProps;
render(<Menu {...mockedProps} />, {
useRedux: true,
useQueryParams: true,
useRouter: true,
useTheme: true,
describe('should render the brand', () => {
it.each(['', '/myapp'])("including app_root '%s'", async app_root => {
staticAssetsPrefixMock.mockReturnValue(app_root);
useSelectorMock.mockReturnValue({ roles: user.roles });
const {
data: {
brand: { alt, icon },
},
} = mockedProps;
render(<Menu {...mockedProps} />, {
useRedux: true,
useQueryParams: true,
useRouter: true,
useTheme: true,
});
expect(await screen.findByAltText(alt)).toBeInTheDocument();
const image = screen.getByAltText(alt);
expect(image).toHaveAttribute('src', `${app_root}${icon}`);
});
expect(await screen.findByAltText(alt)).toBeInTheDocument();
const image = screen.getByAltText(alt);
expect(image).toHaveAttribute('src', icon);
});

test('should render the environment tag', async () => {
Expand Down
7 changes: 4 additions & 3 deletions superset-frontend/src/features/home/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import { useState, useEffect } from 'react';
import { styled, css, useTheme } from '@superset-ui/core';
import { debounce } from 'lodash';
import { assetUrl } from 'src/utils/assetUrl';
import { getUrlParam } from 'src/utils/urlUtils';
import { MainNav, MenuMode } from '@superset-ui/core/components/Menu';
import { Tooltip, Grid, Row, Col, Image } from '@superset-ui/core/components';
Expand Down Expand Up @@ -285,7 +286,7 @@ export function Menu({
>
<Image
preview={false}
src={theme.brandLogoUrl}
src={assetUrl(theme.brandLogoUrl)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this cause issues when the brandLogoUrl is hosted outside Superset?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I have misunderstood how this now works with the new theming?

When assetUrl was originally implemented in #30134 it was assumed that STATIC_ASSETS_PREFIX would be a prefix to all static assets and if using a CDN for example then this would be set as the STATIC_ASSETS_PREFIX and this would then become a prefix to everything.

Similarly we have ensureAppRoot for prefixing non-asset paths.

I think I saw that the default theme.brandLogoUrl value was a relative path much like superset_config[APP_ICON] so I assumed the same rules applied. Maybe assetUrl() could also check the value passed to it is a relative path and only add the prefix then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe assetUrl() could also check the value passed to it is a relative path and only add the prefix then?

Yes, that would be my solution as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone with adding a second assetUrlf that checks for the relative path as in the other places it is used the check is not necessary so it avoids wasting time in those places.

alt={theme.brandLogoAlt || 'Apache Superset'}
/>
</Typography.Link>
Expand All @@ -296,7 +297,7 @@ export function Menu({
// Kept as is for backwards compatibility with the old theme system / superset_config.py
link = (
<GenericLink className="navbar-brand" to={brand.path}>
<Image preview={false} src={brand.icon} alt={brand.alt} />
<Image preview={false} src={assetUrl(brand.icon)} alt={brand.alt} />
</GenericLink>
);
} else {
Expand All @@ -306,7 +307,7 @@ export function Menu({
href={brand.path}
tabIndex={-1}
>
<Image preview={false} src={brand.icon} alt={brand.alt} />
<Image preview={false} src={assetUrl(brand.icon)} alt={brand.alt} />
</Typography.Link>
);
}
Expand Down
Loading