codeant-ai-for-open-source[bot] commented on code in PR #41119:
URL: https://github.com/apache/superset/pull/41119#discussion_r3427799694
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -292,16 +292,24 @@ export function Menu({
const renderBrand = () => {
let link;
if (theme.brandLogoUrl) {
+ const brandHref = ensureAppRoot(theme.brandLogoHref);
+ const brandImage = (
+ <StyledImage
+ preview={false}
+ src={ensureStaticPrefix(theme.brandLogoUrl)}
+ alt={theme.brandLogoAlt || 'Apache Superset'}
+ height={theme.brandLogoHeight}
+ />
+ );
link = (
<StyledBrandWrapper margin={theme.brandLogoMargin}>
- <StyledBrandLink href={ensureAppRoot(theme.brandLogoHref)}>
- <StyledImage
- preview={false}
- src={ensureStaticPrefix(theme.brandLogoUrl)}
- alt={theme.brandLogoAlt || 'Apache Superset'}
- height={theme.brandLogoHeight}
- />
- </StyledBrandLink>
+ {isUrlExternal(brandHref) || !isFrontendRoute(brandHref) ? (
+ <Typography.Link className="navbar-brand" href={brandHref}>
+ {brandImage}
+ </Typography.Link>
+ ) : (
+ <StyledBrandLink to={brandHref}>{brandImage}</StyledBrandLink>
+ )}
Review Comment:
**Suggestion:** The new branch still falls back to a plain anchor for
root/default logo links, so clicking the logo can still trigger a full page
reload in common configs (for example when `brandLogoHref` is unset and
resolves to `/`). `isFrontendRoute('/')` is false, so the code enters the `<a>`
path instead of SPA navigation. Treat non-external app-root links as internal
(or route them through `GenericLink`) so logo clicks remain client-side. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Header logo still causes full page reload by default.
- ❌ SPA redirect from '/' to '/superset/welcome/' not used on click.
- ⚠️ Undermines PR goal to eliminate hard reload on logo.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the default theme configuration in `superset/config.py` where
`THEME_DEFAULT["token"]["brandLogoHref"]` is set to `'/'` (see
`superset/config.py` lines
6–16 in the `THEME_DEFAULT` block around the brand tokens).
2. Observe the frontend default application root in
`superset-frontend/src/constants.ts`
where `DEFAULT_COMMON_BOOTSTRAP_DATA.application_root` is `'/'` (lines
127–129), and how
`applicationRoot()` is derived from this in
`superset-frontend/src/utils/getBootstrapData.ts` (lines 41–56).
3. In `superset-frontend/src/utils/pathUtils.ts` (lines 44–59),
`ensureAppRoot(path)`
returns the `normalizedPath` unchanged when `path === '/'` and
`applicationRoot()` is
`'/'`, so `ensureAppRoot(theme.brandLogoHref)` in `Menu.tsx` (line 295 of
the PR hunk)
yields `brandHref === '/'` under the default config.
4. In `superset-frontend/src/views/routes.tsx` (lines 123–138),
`frontEndRoutes` is built
from `routes`, which includes `/superset/welcome/` but not `'/'`;
consequently
`isFrontendRoute('/')` returns `false`. In
`superset-frontend/src/views/App.tsx` (lines
82–90), `App` passes this `isFrontendRoute` function into `Menu`. Inside
`superset-frontend/src/features/home/Menu.tsx` `renderBrand()` (PR hunk
lines 295–313),
`isUrlExternal('/')` is `false` and `!isFrontendRoute('/')` is `true`, so
the ternary
chooses the `<Typography.Link className="navbar-brand" href={brandHref}>`
branch instead
of `<StyledBrandLink to={brandHref}>`. When you start the dev server with
default
configuration, load the app, and click the header logo, the browser performs
a full
document navigation to `'/'` (server-side redirecting to
`/superset/welcome/`), rather
than a client-side SPA navigation using React Router's `<Link>` and the
`<Redirect
from="/" to="/superset/welcome/" exact />` route in `App.tsx` (line 115).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b8b08f69a1904886811f586dc4355940&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b8b08f69a1904886811f586dc4355940&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/home/Menu.tsx
**Line:** 295:312
**Comment:**
*Logic Error: The new branch still falls back to a plain anchor for
root/default logo links, so clicking the logo can still trigger a full page
reload in common configs (for example when `brandLogoHref` is unset and
resolves to `/`). `isFrontendRoute('/')` is false, so the code enters the `<a>`
path instead of SPA navigation. Treat non-external app-root links as internal
(or route them through `GenericLink`) so logo clicks remain client-side.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41119&comment_hash=766a3341040517c5e875444f75a9e3ccde9a2514c7ca779e1811f02df2b03ece&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41119&comment_hash=766a3341040517c5e875444f75a9e3ccde9a2514c7ca779e1811f02df2b03ece&reaction=dislike'>👎</a>
--
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]
