Copilot commented on code in PR #37059:
URL: https://github.com/apache/superset/pull/37059#discussion_r2691887358
##########
superset-frontend/oxlint.json:
##########
@@ -14,7 +14,7 @@
},
"settings": {
"react": {
- "version": "detect"
+ "version": "17.0"
Review Comment:
This change from "detect" to a hardcoded version "17.0" appears unrelated to
the PR's purpose of migrating APP_NAME to brandAppName. This change should be
removed or explained in the PR description if it's intentional and necessary.
```suggestion
"version": "detect"
```
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -210,14 +210,25 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug
}: PageProps) => {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [readyToRender]);
+ // Update document title when dashboard title changes
useEffect(() => {
if (dashboard_title) {
document.title = dashboard_title;
}
+ }, [dashboard_title]);
+
+ // Restore original title on unmount (run once)
+ useEffect(() => {
+ const originalTitle = document.title;
return () => {
- document.title = 'Superset';
+ document.title =
+ originalTitle ||
+ theme?.brandAppName ||
+ theme?.brandLogoAlt ||
+ 'Superset';
};
- }, [dashboard_title]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, []);
Review Comment:
The new title restoration logic on component unmount lacks test coverage.
Since this repository has comprehensive test coverage for dashboard components,
consider adding tests to verify:
1. Title is set when dashboard_title changes
2. Title is restored to the app name on unmount
3. Fallback chain works correctly (originalTitle → brandAppName →
brandLogoAlt → 'Superset')
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -210,14 +210,25 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug
}: PageProps) => {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [readyToRender]);
+ // Update document title when dashboard title changes
useEffect(() => {
if (dashboard_title) {
document.title = dashboard_title;
}
+ }, [dashboard_title]);
+
+ // Restore original title on unmount (run once)
+ useEffect(() => {
+ const originalTitle = document.title;
return () => {
- document.title = 'Superset';
+ document.title =
+ originalTitle ||
+ theme?.brandAppName ||
+ theme?.brandLogoAlt ||
+ 'Superset';
};
- }, [dashboard_title]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, []);
Review Comment:
The title restoration logic has a flaw. The cleanup function captures
`originalTitle` at mount time, which could be the title from a previous page
navigation. The fallback chain (`originalTitle || theme?.brandAppName || ...`)
will only reach the theme values if `originalTitle` is empty/undefined, but it
will typically have some value from navigation. This means the theme-based
fallback won't work as intended.
Consider capturing the intended app name title at mount instead, or
restructuring to restore to the app name directly without relying on the
mount-time title.
##########
UPDATING.md:
##########
@@ -128,6 +128,28 @@ See `superset/mcp_service/PRODUCTION.md` for deployment
guides.
- [35062](https://github.com/apache/superset/pull/35062): Changed the function
signature of `setupExtensions` to `setupCodeOverrides` with options as
arguments.
### Breaking Changes
+- [#34865](https://github.com/apache/superset/issues/34865): The `APP_NAME`
configuration variable no longer controls the browser window/tab title or other
frontend branding. Application names should now be configured using the theme
system with the `brandAppName` token. The `APP_NAME` config is still used for
backend contexts (MCP service, logs, etc.) and serves as a fallback if
`brandAppName` is not set.
+ - **Migration:**
+ ```python
+ # Before (Superset 5.x)
+ APP_NAME = "My Custom App"
+
+ # After (Superset 6.x) - Option 1: Use theme system (recommended)
+ THEME_DEFAULT = {
+ "token": {
+ "brandAppName": "My Custom App", # Window titles
+ "brandLogoAlt": "My Custom App", # Logo alt text
+ "brandLogoUrl": "/static/assets/images/custom_logo.png"
+ }
+ }
+
+ # After (Superset 6.x) - Option 2: Temporary fallback
+ # Keep APP_NAME for now (will be used as fallback for brandAppName)
+ APP_NAME = "My Custom App"
+ # But you should migrate to THEME_DEFAULT.token.brandAppName
+ ```
+ - **Note:** For dark mode, set the same tokens in `THEME_DARK` configuration.
Review Comment:
The documentation states this is a "Breaking Change", but the PR description
emphasizes "Backward compatibility: Existing `APP_NAME` configurations continue
to work automatically via fallback mechanism." These statements appear
contradictory. If backward compatibility is maintained through the fallback,
it's not technically a breaking change. Consider either moving this to a
non-breaking changes section with a note about migration recommendations, or
clarifying what specifically breaks.
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -292,14 +292,25 @@ function ExploreViewContainer(props) {
const theme = useTheme();
+ // Update document title when slice name changes
useEffect(() => {
if (props.sliceName) {
document.title = props.sliceName;
}
+ }, [props.sliceName]);
+
+ // Restore original title on unmount (run once)
+ useEffect(() => {
+ const originalTitle = document.title;
return () => {
- document.title = 'Superset';
+ document.title =
+ originalTitle ||
+ theme?.brandAppName ||
+ theme?.brandLogoAlt ||
+ 'Superset';
};
- }, [props.sliceName]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, []);
Review Comment:
The new title restoration logic on component unmount lacks test coverage.
Since this repository has comprehensive test files for this component
(ExploreViewContainer.test.tsx exists), consider adding tests to verify:
1. Title is set when sliceName changes
2. Title is restored to the app name on unmount
3. Fallback chain works correctly (originalTitle → brandAppName →
brandLogoAlt → 'Superset')
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -292,14 +292,25 @@ function ExploreViewContainer(props) {
const theme = useTheme();
+ // Update document title when slice name changes
useEffect(() => {
if (props.sliceName) {
document.title = props.sliceName;
}
+ }, [props.sliceName]);
+
+ // Restore original title on unmount (run once)
+ useEffect(() => {
+ const originalTitle = document.title;
return () => {
- document.title = 'Superset';
+ document.title =
+ originalTitle ||
+ theme?.brandAppName ||
+ theme?.brandLogoAlt ||
+ 'Superset';
};
- }, [props.sliceName]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, []);
Review Comment:
The title restoration logic has a flaw. The cleanup function captures
`originalTitle` at mount time, which could be the title from a previous page
navigation. The fallback chain (`originalTitle || theme?.brandAppName || ...`)
will only reach the theme values if `originalTitle` is empty/undefined, but it
will typically have some value from navigation. This means the theme-based
fallback won't work as intended.
Consider capturing the intended app name title at mount instead, or
restructuring to restore to the app name directly without relying on the
mount-time title.
##########
superset/views/base.py:
##########
@@ -567,10 +568,39 @@ 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", {})
+ # Deep copy theme data to avoid mutating cached bootstrap payload
+ theme_data = copy.deepcopy(payload.get("common", {}).get("theme", {}))
default_theme = theme_data.get("default", {})
- theme_tokens = default_theme.get("token", {})
+ 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
+ # Get or create token dict
+ if "token" not in theme_config:
+ theme_config["token"] = {}
+ theme_tokens = theme_config["token"]
+
+ if (
+ not theme_tokens.get("brandAppName")
+ or theme_tokens.get("brandAppName") == "Superset"
+ ):
+ # If brandAppName not set or is default, check if APP_NAME
customized
+ if app_name_from_config != "Superset":
+ # User has customized APP_NAME, use it as brandAppName
+ theme_tokens["brandAppName"] = app_name_from_config
+
+ # Write the modified theme data back to payload
+ if "common" not in payload:
+ payload["common"] = {}
+ payload["common"]["theme"] = theme_data
Review Comment:
The code attempts to avoid mutating cached data by deep copying
`theme_data`, but then modifies `payload["common"]["theme"]` directly on line
599. Since `payload["common"]` is a reference to the cached bootstrap data
(returned from `common_bootstrap_payload()`), this assignment mutates the
cached dictionary structure. While the theme data itself is a copy, the cached
dict's structure is being modified.
To fix this properly, you should also deep copy `payload["common"]` before
modifying it, or restructure to avoid mutation entirely.
--
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]
