codeant-ai-for-open-source[bot] commented on code in PR #37059:
URL: https://github.com/apache/superset/pull/37059#discussion_r2682798793
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -297,9 +297,9 @@ function ExploreViewContainer(props) {
document.title = props.sliceName;
}
return () => {
- document.title = 'Superset';
+ document.title = theme.brandAppName || theme.brandLogoAlt || 'Superset';
};
- }, [props.sliceName]);
+ }, [props.sliceName, theme.brandAppName, theme.brandLogoAlt]);
Review Comment:
**Suggestion:** The effect dependency list references nested `theme`
properties (`theme.brandAppName`, `theme.brandLogoAlt`) which will evaluate
those property accesses on every render and can throw if `theme` is undefined;
prefer depending on the `theme` object itself so the effect reruns when the
theme changes and you avoid direct property access in the dependency array.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
}, [props.sliceName, theme]);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Depending on the whole `theme` object avoids accessing nested properties
during dependency evaluation (which could throw if `theme` were undefined) and
ensures the effect re-runs when the theme reference changes. It's a reasonable
change; the trade-off of rerunning on any theme property change is acceptable
here because the effect is simply setting document.title.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
**Line:** 302:302
**Comment:**
*Possible Bug: The effect dependency list references nested `theme`
properties (`theme.brandAppName`, `theme.brandLogoAlt`) which will evaluate
those property accesses on every render and can throw if `theme` is undefined;
prefer depending on the `theme` object itself so the effect reruns when the
theme changes and you avoid direct property access in the dependency array.
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.
```
</details>
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -297,9 +297,9 @@ function ExploreViewContainer(props) {
document.title = props.sliceName;
}
return () => {
- document.title = 'Superset';
+ document.title = theme.brandAppName || theme.brandLogoAlt || 'Superset';
Review Comment:
**Suggestion:** Accessing nested properties on `theme` without guarding for
`theme` being undefined can throw a TypeError (cannot read property
'brandAppName' of undefined). Use optional chaining when reading `theme`
properties and make the effect depend on the `theme` object (not its nested
properties) to avoid runtime errors and stale/incorrect dependency evaluations.
[null pointer]
**Severity Level:** Minor ⚠️
```suggestion
document.title = theme?.brandAppName || theme?.brandLogoAlt ||
'Superset';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a small, defensive improvement: guarding with optional chaining
prevents a possible TypeError if `theme` were ever undefined at runtime.
`useTheme()` usually returns a theme object, so this is unlikely to be
required, but the change is harmless and reduces risk with no functional
downside.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
**Line:** 300:300
**Comment:**
*Null Pointer: Accessing nested properties on `theme` without guarding
for `theme` being undefined can throw a TypeError (cannot read property
'brandAppName' of undefined). Use optional chaining when reading `theme`
properties and make the effect depend on the `theme` object (not its nested
properties) to avoid runtime errors and stale/incorrect dependency evaluations.
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.
```
</details>
##########
superset/templates/superset/spa.html:
##########
@@ -24,6 +24,8 @@
{% block title %}
{% if title %}
{{ title }}
+ {% else %}
+ {{ default_title }}
Review Comment:
**Suggestion:** The template uses a manual if/else for `title` vs
`default_title`; prefer Jinja's `default` filter to handle undefined/empty
`default_title` in a concise, reliable way so empty strings or undefined values
fall back to the app config. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
{{ default_title | default(appbuilder.app.config.get('APP_NAME',
'')) }}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Using the `default` filter is a concise, idiomatic Jinja solution that
covers undefined or empty values and can fall back to the app name. It improves
correctness and readability compared to manually checking elsewhere.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/templates/superset/spa.html
**Line:** 28:28
**Comment:**
*Logic Error: The template uses a manual if/else for `title` vs
`default_title`; prefer Jinja's `default` filter to handle undefined/empty
`default_title` in a concise, reliable way so empty strings or undefined values
fall back to the app config.
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.
```
</details>
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -215,9 +215,9 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
document.title = dashboard_title;
}
return () => {
- document.title = 'Superset';
+ document.title = theme.brandAppName || theme.brandLogoAlt || 'Superset';
};
- }, [dashboard_title]);
+ }, [dashboard_title, theme.brandAppName, theme.brandLogoAlt]);
Review Comment:
**Suggestion:** The current effect's cleanup runs on every dependency
change, which causes a brief title reset/flicker between cleanup and the next
effect run; separate concerns into two effects—one that updates title when
`dashboard_title` changes, and one (mounted once) that captures and restores
the original title on unmount—to avoid unnecessary cleanup runs on re-renders.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
useEffect(() => {
if (dashboard_title) {
document.title = dashboard_title;
}
}, [dashboard_title]);
useEffect(() => {
const originalTitle = document.title;
return () => {
document.title = originalTitle || theme?.brandAppName ||
theme?.brandLogoAlt || 'Superset';
};
}, []);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is the correct direction: separate concerns into a short-lived effect
that updates the title when `dashboard_title` changes and a mount-only
effect that captures/restores the original document.title on unmount.
That prevents the cleanup from running on every dependency change (which
causes flicker) and ensures the original title is preserved once the
component unmounts.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/containers/DashboardPage.tsx
**Line:** 215:220
**Comment:**
*Logic Error: The current effect's cleanup runs on every dependency
change, which causes a brief title reset/flicker between cleanup and the next
effect run; separate concerns into two effects—one that updates title when
`dashboard_title` changes, and one (mounted once) that captures and restores
the original title on unmount—to avoid unnecessary cleanup runs on re-renders.
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.
```
</details>
##########
tests/unit_tests/views/test_base_theme_helpers.py:
##########
@@ -530,3 +530,175 @@ def test_ui_admin_disabled_no_config_themes(self,
mock_get_config, mock_app):
assert result["theme"]["enableUiThemeAdministration"] is False
assert result["theme"]["default"] == {}
assert result["theme"]["dark"] == {}
+
+
+class TestBrandAppNameFallback:
+ """Test brandAppName fallback mechanism for APP_NAME migration (issue
#34865)"""
+
+ @patch("superset.views.base.get_spa_payload")
+ @patch("superset.views.base.app")
+ def test_brandappname_uses_theme_value_when_set(
+ self, mock_app, mock_payload
+ ):
+ """Test that explicit brandAppName in theme takes precedence"""
+ from superset.views.base import get_spa_template_context
+
+ mock_app.config = MagicMock()
+ mock_app.config.get.side_effect = lambda k, d=None: {
+ "APP_NAME": "Fallback App Name",
+ }.get(k, d)
Review Comment:
**Suggestion:** Tests set up the patched `app` as a MagicMock and then
configure `mock_app.config.get.side_effect` via a lambda; this is more verbose
and error-prone than using a real dict-like config object. Replace the
MagicMock-based config with a plain mapping (dict) so that
`app.config.get("APP_NAME", "Superset")` behaves identically to runtime and the
test is clearer and less brittle. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
# Use a plain dict for config to mirror Flask's config mapping
behavior
mock_app.config = {"APP_NAME": "Fallback App Name"}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Replacing the MagicMock-based config with a plain dict is a reasonable
improvement: it better mirrors Flask's config mapping (which exposes .get) and
is less brittle than fiddling with side_effect. The change is low-risk and
improves clarity of the test.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/test_base_theme_helpers.py
**Line:** 546:549
**Comment:**
*Possible Bug: Tests set up the patched `app` as a MagicMock and then
configure `mock_app.config.get.side_effect` via a lambda; this is more verbose
and error-prone than using a real dict-like config object. Replace the
MagicMock-based config with a plain mapping (dict) so that
`app.config.get("APP_NAME", "Superset")` behaves identically to runtime and the
test is clearer and less brittle.
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.
```
</details>
##########
superset/views/base.py:
##########
@@ -567,9 +567,28 @@ def get_spa_template_context(
"""
payload = get_spa_payload(extra_bootstrap_data)
- # Extract theme data for template access
- theme_data = get_theme_bootstrap_data().get("theme", {})
+ # Extract theme data from payload (this is what gets sent to frontend)
+ theme_data = payload.get("common", {}).get("theme", {})
default_theme = theme_data.get("default", {})
+ dark_theme = theme_data.get("dark", {})
+
+ # Apply brandAppName fallback to both default and dark themes
+ # Priority: theme brandAppName > APP_NAME config > "Superset" default
+ app_name_from_config = app.config.get("APP_NAME", "Superset")
+ for theme_config in [default_theme, dark_theme]:
+ if not theme_config:
+ continue
+ theme_tokens = theme_config.get("token", {})
Review Comment:
**Suggestion:** When a theme has no "token" key the code creates a local
token dict but never assigns it back to the theme config, so the fallback
`brandAppName` is not persisted into the theme structure; ensure you create and
assign the `token` dict into `theme_config` before mutating it. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
theme_tokens = theme_config.get("token")
if theme_tokens is None:
# Ensure we persist a token dict back into the theme config so
mutations are retained
theme_tokens = {}
theme_config["token"] = theme_tokens
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
If a theme_config lacks a "token" key, theme_config.get("token", {}) returns
a fresh dict that is not attached back to theme_config.
Mutating that local dict will not persist the fallback into the theme
structure, so later reads (e.g., theme_tokens = default_theme.get("token", {}))
won't see the brandAppName.
Assigning the dict back (theme_config["token"] = theme_tokens) or ensuring
token exists before mutation fixes the logic bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 581:581
**Comment:**
*Logic Error: When a theme has no "token" key the code creates a local
token dict but never assigns it back to the theme config, so the fallback
`brandAppName` is not persisted into the theme structure; ensure you create and
assign the `token` dict into `theme_config` before mutating it.
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.
```
</details>
##########
superset/views/base.py:
##########
@@ -567,9 +567,28 @@ def get_spa_template_context(
"""
payload = get_spa_payload(extra_bootstrap_data)
- # Extract theme data for template access
- theme_data = get_theme_bootstrap_data().get("theme", {})
+ # Extract theme data from payload (this is what gets sent to frontend)
+ theme_data = payload.get("common", {}).get("theme", {})
default_theme = theme_data.get("default", {})
+ dark_theme = theme_data.get("dark", {})
Review Comment:
**Suggestion:** The code mutates the theme dicts taken from the cached
bootstrap payload in-place, which will modify the memoized cached object shared
across requests and cause state leakage/race conditions; make deep copies of
the theme dicts before mutating them. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
import copy
theme_data = payload.get("common", {}).get("theme", {})
default_theme = copy.deepcopy(theme_data.get("default", {}) or {})
dark_theme = copy.deepcopy(theme_data.get("dark", {}) or {})
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The payload's "common" value comes from a memoized function
(cached_common_bootstrap_data).
Mutating dicts taken from that cached structure in-place will leak state
across requests and
can cause surprising/racy behavior. Deep-copying the theme dicts before
modifying them fixes
the root cause. The suggestion is accurate and directly addresses a real
shared-state bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 572:573
**Comment:**
*Race Condition: The code mutates the theme dicts taken from the cached
bootstrap payload in-place, which will modify the memoized cached object shared
across requests and cause state leakage/race conditions; make deep copies of
the theme dicts before mutating them.
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.
```
</details>
##########
superset-frontend/packages/superset-core/src/ui/theme/types.ts:
##########
@@ -119,6 +119,7 @@ export interface SupersetSpecificTokens {
// Brand-related
brandIconMaxWidth: number;
+ brandAppName: string;
Review Comment:
**Suggestion:** Breaking change / required property: adding `brandAppName:
string` as a required property will break existing theme objects and consumers
that don't provide this token (TypeScript errors at compile time and runtime
`undefined` where code expects a string). Make the new token optional to
preserve backward compatibility. [type error]
**Severity Level:** Minor ⚠️
```suggestion
brandAppName?: string;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR introduced a new required token (brandAppName: string) which is a
breaking
change for existing theme objects. Making it optional prevents immediate
TypeScript/consumer breakage and is the least invasive compatibility fix.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-core/src/ui/theme/types.ts
**Line:** 122:122
**Comment:**
*Type Error: Breaking change / required property: adding `brandAppName:
string` as a required property will break existing theme objects and consumers
that don't provide this token (TypeScript errors at compile time and runtime
`undefined` where code expects a string). Make the new token optional to
preserve backward compatibility.
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.
```
</details>
##########
superset/views/base.py:
##########
@@ -581,13 +600,17 @@ def get_spa_template_context(
# No custom URL either, use default SVG
spinner_svg = get_default_spinner_svg()
+ # Determine default title using the (potentially updated) brandAppName
+ default_title = theme_tokens.get("brandAppName", "Superset")
Review Comment:
**Suggestion:** `default_title` is derived only from the default theme
token; if the default theme lacks a brand name but the dark theme has one (or
APP_NAME is customized), the title will incorrectly fall back to "Superset";
pick the title from default token, then dark token, then APP_NAME config, then
"Superset". [logic error]
**Severity Level:** Minor ⚠️
```suggestion
default_title = (
theme_tokens.get("brandAppName")
or dark_theme.get("token", {}).get("brandAppName")
or app.config.get("APP_NAME", "Superset")
or "Superset"
)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Reading only the default theme token can miss a valid brand name present in
the dark theme or the APP_NAME config.
The proposed fallback chain (default token -> dark token -> APP_NAME config
-> "Superset") is reasonable and fixes a visible UX inconsistency where the
title falls back to "Superset" even though a brand name exists elsewhere.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 604:604
**Comment:**
*Logic Error: `default_title` is derived only from the default theme
token; if the default theme lacks a brand name but the dark theme has one (or
APP_NAME is customized), the title will incorrectly fall back to "Superset";
pick the title from default token, then dark token, then APP_NAME config, then
"Superset".
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.
```
</details>
--
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]
