Copilot commented on code in PR #41119:
URL: https://github.com/apache/superset/pull/41119#discussion_r3422502101
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -294,7 +294,7 @@ export function Menu({
if (theme.brandLogoUrl) {
link = (
<StyledBrandWrapper margin={theme.brandLogoMargin}>
- <StyledBrandLink href={ensureAppRoot(theme.brandLogoHref)}>
+ <StyledBrandLink to={ensureAppRoot(theme.brandLogoHref)}>
<StyledImage
Review Comment:
`StyledBrandLink` is now a `GenericLink` (React Router `<Link>` for internal
URLs), but the `to` prop is still wrapped in `ensureAppRoot(...)`. When
`application_root` is set (subdirectory deployments),
`ensureAppRoot('/dashboard/list/')` will prefix the app root, and then the
router `basename` will prefix it again, producing URLs like
`/superset/superset/dashboard/list/`. For client-side navigation the `to` value
should be basename-relative (and only normalized to an absolute path), not
application-root-prefixed.
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -294,7 +294,7 @@ export function Menu({
if (theme.brandLogoUrl) {
link = (
<StyledBrandWrapper margin={theme.brandLogoMargin}>
- <StyledBrandLink href={ensureAppRoot(theme.brandLogoHref)}>
+ <StyledBrandLink to={ensureAppRoot(theme.brandLogoHref)}>
<StyledImage
Review Comment:
This change introduces client-side navigation for the logo via
`GenericLink`, but there are no tests covering internal `brandLogoHref` values
under a non-empty `application_root` (subdirectory deployment). In that setup
the router `basename` behavior is important, and a regression here would only
show up outside the current test wrapper (which uses `BrowserRouter` without a
basename). Consider adding a test that renders the app/router with a basename
and asserts the logo link resolves to the expected href for an internal path
like `/dashboard/list/`.
--
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]
