fix(theme): migrate APP_NAME to brandAppName theme token with backward compatibility#37370
Conversation
…d compatibility This change addresses issue apache#34865 where APP_NAME configuration stopped working for window titles after the Ant Design v5 theme system migration in Superset 6.0. Changes: - Added brandAppName theme token to define application name for window/tab titles - Implemented 3-tier fallback: brandAppName → APP_NAME config → "Superset" - Updated frontend title cleanup in DashboardPage and ExploreViewContainer - Fixed hardcoded "Superset" strings when navigating between pages - Added comprehensive unit tests for fallback mechanism - Documented migration path in UPDATING.md The implementation maintains full backward compatibility with existing APP_NAME configurations while providing a migration path to the new theme-based approach. Fixes apache#34865 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Implemented all recommended fixes from AI code review: **Critical Fixes:** 1. Made brandAppName optional in TypeScript types to avoid breaking existing themes 2. Fixed cached data mutation by deep copying theme data before modifications 3. Fixed token persistence by creating token dict if it doesn't exist 4. Fixed oxlint configuration to use React version "17.0" instead of "detect" **Improvements:** 5. Split useEffect hooks in DashboardPage and ExploreViewContainer to prevent title flicker 6. Added optional chaining for theme access to prevent potential TypeErrors 7. Added Jinja template fallback for default_title **Test Coverage:** 8. Added 2 new test cases for edge cases: - Both default and dark themes being updated - Avoiding mutation of cached payload All existing tests pass and new tests provide better coverage of the fallback logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Auto-formatted test files with ruff to fix pre-commit CI failures. These are formatting-only changes with no functional impact. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review Agent Run #149d8dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // 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; |
There was a problem hiding this comment.
Suggestion: The code captures originalTitle inside an effect that runs after the slice-name effect, so originalTitle may already have been overwritten by the slice-name effect and won't restore the true pre-mount title on unmount. Capture the initial document title at mount using a stable hook (e.g., useMemo) and reference that in the cleanup to reliably restore the original browser title. [logic error]
Severity Level: Critical 🚨
- ❌ Browser title fails to revert after leaving Explore.
- ⚠️ Users see stale tab titles after navigating away.| // 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; | |
| const originalTitle = useMemo(() => document.title, []); | |
| // 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(() => { |
Steps of Reproduction ✅
1. Open a saved chart in Explore so ExploreViewContainer mounts. The component is defined
at superset-frontend/src/explore/components/ExploreViewContainer/index.jsx (function
ExploreViewContainer, lines ~273+).
2. On mount React runs the first effect at lines 294-299 (useEffect for props.sliceName).
If props.sliceName is truthy, that effect sets document.title = props.sliceName (lines
295-298).
3. Immediately after, React runs the next effect at lines 301-312 (the "Restore original
title" effect). That effect executes const originalTitle = document.title at line 303 —
but because the sliceName effect already ran, document.title now holds the slice name, not
the pre-mount title.
4. When the component later unmounts, the cleanup (lines 304-310) restores document.title
to originalTitle (which is the slice name), so the browser tab does not revert to the
pre-mount application title. This reproduces the wrong restored title behavior.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
**Line:** 294:303
**Comment:**
*Logic Error: The code captures `originalTitle` inside an effect that runs after the slice-name effect, so `originalTitle` may already have been overwritten by the slice-name effect and won't restore the true pre-mount title on unmount. Capture the initial document title at mount using a stable hook (e.g., useMemo) and reference that in the cleanup to reliably restore the original browser title.
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.| "token": { | ||
| # Brand | ||
| # Application name for window titles | ||
| "brandAppName": APP_NAME, |
There was a problem hiding this comment.
Suggestion: Backwards-compatibility/config-order issue: brandAppName is evaluated at module import time and will not reflect APP_NAME overrides applied later by a local superset_config.py; allow an environment-based override that can be set externally (and keep a fallback) so deployments that set an app name via env var get the correct value without relying on import order. [possible bug]
Severity Level: Critical 🚨
- ❌ Browser title shows wrong app name on login.
- ❌ Dashboard/tab titles show stale/default name.
- ⚠️ Breaks backward compatibility for APP_NAME overrides.| "brandAppName": APP_NAME, | |
| "brandAppName": os.environ.get("SUPERSET_APP_NAME", APP_NAME or "Superset"), |
Steps of Reproduction ✅
1. Inspect `superset/config.py` where THEME_DEFAULT is declared; the token
`"brandAppName"` is assigned at `superset/config.py:905-906` to the value of `APP_NAME` at
module import time.
2. Provision a local override by creating `superset_config.py` with `APP_NAME = "My Custom
App"` and ensure it is discoverable on PYTHONPATH. The project loads `superset_config`
later in the file (the import block that applies local overrides executes after
THEME_DEFAULT is defined).
3. Start Superset. The module-level `APP_NAME` is updated by the `superset_config` import,
but THEME_DEFAULT["token"]["brandAppName"] remains the earlier bound value because it was
set during module import at `superset/config.py:902-912`.
4. Observe the browser tab/window titles (login page, dashboards) still showing the
old/default name instead of the `APP_NAME` from `superset_config.py`. This reproduces the
import-order issue where the theme token was captured too early. The suggested change
makes the token read from an env override or fallback so deployments that set app name via
env or later config still surface correctly in the theme.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/config.py
**Line:** 906:906
**Comment:**
*Possible Bug: Backwards-compatibility/config-order issue: `brandAppName` is evaluated at module import time and will not reflect `APP_NAME` overrides applied later by a local `superset_config.py`; allow an environment-based override that can be set externally (and keep a fallback) so deployments that set an app name via env var get the correct value without relying on import order.
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #37370 +/- ##
===========================================
+ Coverage 0 66.60% +66.60%
===========================================
Files 0 642 +642
Lines 0 48997 +48997
Branches 0 5491 +5491
===========================================
+ Hits 0 32634 +32634
- Misses 0 15069 +15069
- Partials 0 1294 +1294
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| - [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. |
There was a problem hiding this comment.
Shouldn't this be PR id and not issue id?
Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
f7b08da to
599c367
Compare
sadpandajoe
left a comment
There was a problem hiding this comment.
left a nitpick for UPDATING.md but not a merge blocker
bc32113 to
87ea4f4
Compare
|
Bito Automatic Review Skipped – PR Already Merged |
…d compatibility (apache#37370) Co-authored-by: Rafael Benitez <rebenitez1802@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
…d compatibility (apache#37370) Co-authored-by: Rafael Benitez <rebenitez1802@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
- theming.mdx: document brandAppName theme token (PR #37370) — controls app name in browser title/nav/emails, takes precedence over APP_NAME config - cache.mdx: document SUPERSET_CACHE_WARMUP_USER config key (PR #38449) — controls the user account Selenium WebDriver authenticates as for thumbnail rendering and cache warmup; update selenium → Selenium capitalization - security.mdx: document missing SQL Lab RBAC permissions (PR #36263) — can_estimate_query_cost and can_format_sql must be explicitly granted - sql-templating.mdx: document Jinja support in calculated columns (PR #37791) with examples; add tip that "Format SQL" is Jinja-aware and dialect-specific (PRs #36277, #39393) - creating-your-first-dashboard.mdx: document dashboard tab URLs (#38660), auto-refresh (#37459), "Last queried at" timestamp (#36934), and tab selection when saving charts to dashboards (#36332) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Superset 6.0 moved the navbar brand text and page title to the brandAppName theme token (PR apache/superset#37370). APP_NAME alone is only a fallback after Ant Design v5 migration, so setting just APP_NAME + brandLogoAlt left the UI showing "Superset". Add brandAppName = APP_NAME to both THEME_DEFAULT and THEME_DARK tokens so whitelabel text matches the already-applied logo.
- theming.mdx: document brandAppName theme token (PR #37370) — controls app name in browser title/nav/emails, takes precedence over APP_NAME config - cache.mdx: document SUPERSET_CACHE_WARMUP_USER config key (PR #38449) — controls the user account Selenium WebDriver authenticates as for thumbnail rendering and cache warmup; update selenium → Selenium capitalization - security.mdx: document missing SQL Lab RBAC permissions (PR #36263) — can_estimate_query_cost and can_format_sql must be explicitly granted - sql-templating.mdx: document Jinja support in calculated columns (PR #37791) with examples; add tip that "Format SQL" is Jinja-aware and dialect-specific (PRs #36277, #39393) - creating-your-first-dashboard.mdx: document dashboard tab URLs (#38660), auto-refresh (#37459), "Last queried at" timestamp (#36934), and tab selection when saving charts to dashboards (#36332) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d compatibility (apache#37370) Co-authored-by: Rafael Benitez <rebenitez1802@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
SUMMARY
Original PR: #37059
Fixes #34865 where APP_NAME configuration stopped working for browser window/tab titles after the Ant Design v5 theme migration in Superset 6.0.
Changes:
Backward compatibility: Existing
APP_NAMEconfigurations continue to work automatically via fallback mechanism.TESTING INSTRUCTIONS
superset_config.pyOr test with new theme system:
ADDITIONAL INFORMATION