fix: add subdirectory deployment support for brandSpinnerUrl #37523
Conversation
Code Review Agent Run #f98b39Actionable 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 AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #37523 +/- ##
==========================================
+ Coverage 60.48% 66.58% +6.09%
==========================================
Files 1931 643 -1288
Lines 76236 49051 -27185
Branches 8568 5501 -3067
==========================================
- Hits 46114 32661 -13453
+ Misses 28017 15095 -12922
+ Partials 2105 1295 -810
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:
|
There was a problem hiding this comment.
Pull request overview
This PR extends subdirectory deployment support so that the theme token brandSpinnerUrl is treated consistently with brandLogoUrl and APP_ICON, preventing broken spinner images when Superset runs under a non-root path.
Changes:
- In
create_app, whenSUPERSET_APP_ROOT(orsuperset_app_root) is set andAPP_ICONis a/static/path, update the theme tokens to also prefixbrandSpinnerUrlwith the application root when it points to/static/.
| if token.get("brandSpinnerUrl", "").startswith("/static/"): | ||
| token["brandSpinnerUrl"] = f"{app_root}{token['brandSpinnerUrl']}" | ||
| # Update brandLogoUrl if it points to /static/ | ||
| if token.get("brandLogoUrl", "").startswith("/static/"): | ||
| token["brandLogoUrl"] = f"{app_root}{token['brandLogoUrl']}" |
There was a problem hiding this comment.
brandSpinnerUrl defaults to None in THEME_DEFAULT (see superset/config.py), so token.get("brandSpinnerUrl", "").startswith("/static/") will attempt to call .startswith on None when running under a subdirectory, causing an AttributeError during app startup for the default configuration. To avoid this, normalize the value before calling .startswith (for example, by using an intermediate variable and falling back to an empty string when the token value is None or falsy) and then updating the token only when that normalized string starts with /static/.
| if token.get("brandSpinnerUrl", "").startswith("/static/"): | |
| token["brandSpinnerUrl"] = f"{app_root}{token['brandSpinnerUrl']}" | |
| # Update brandLogoUrl if it points to /static/ | |
| if token.get("brandLogoUrl", "").startswith("/static/"): | |
| token["brandLogoUrl"] = f"{app_root}{token['brandLogoUrl']}" | |
| brand_spinner_url = token.get("brandSpinnerUrl") or "" | |
| if brand_spinner_url.startswith("/static/"): | |
| token["brandSpinnerUrl"] = f"{app_root}{brand_spinner_url}" | |
| # Update brandLogoUrl if it points to /static/ | |
| brand_logo_url = token.get("brandLogoUrl") or "" | |
| if brand_logo_url.startswith("/static/"): | |
| token["brandLogoUrl"] = f"{app_root}{brand_logo_url}" |
| # Update brandSpinnerUrl if it points to /static/ | ||
| if token.get("brandSpinnerUrl", "").startswith("/static/"): | ||
| token["brandSpinnerUrl"] = f"{app_root}{token['brandSpinnerUrl']}" |
There was a problem hiding this comment.
The new subdirectory handling for brandSpinnerUrl is not covered by tests, even though superset/app.py already has integration tests around subdirectory deployments (see tests/integration_tests/test_subdirectory_deployments.py). Please add tests that (1) verify brandSpinnerUrl values starting with /static/ are correctly prefixed with SUPERSET_APP_ROOT, and (2) ensure the default None value for brandSpinnerUrl does not cause errors when running under a subdirectory.
1 participant
SUMMARY
This PR fixes theme token handling for subdirectory deployments by ensuring
brandSpinnerUrlgets properly prefixed with the application root path, similar to howbrandLogoUrlandAPP_ICONare already handled.Problem
When Superset is deployed under a subdirectory (e.g.,
/superset/,/analytics/), the loading spinner image (brandSpinnerUrl) would break because its URL wasn't being prefixed with the application root. This caused 404 errors for the spinner image in non-root deployments.Root Cause
The application initialization code already handled
brandLogoUrlandAPP_ICONfor subdirectory deployments, but missedbrandSpinnerUrl, which is another theme token that references static assets.Solution
Extend the existing subdirectory handling logic to include
brandSpinnerUrltoken, applying the same pattern used forbrandLogoUrl.TESTING INSTRUCTIONS
ADDITIONAL INFORMATION