Copilot commented on code in PR #37523:
URL: https://github.com/apache/superset/pull/37523#discussion_r2737959268
##########
superset/app.py:
##########
@@ -78,6 +78,9 @@ def create_app(
for theme_key in ("THEME_DEFAULT", "THEME_DARK"):
theme = app.config[theme_key]
token = theme.get("token", {})
+ # Update brandSpinnerUrl if it points to /static/
+ if token.get("brandSpinnerUrl", "").startswith("/static/"):
+ token["brandSpinnerUrl"] =
f"{app_root}{token['brandSpinnerUrl']}"
Review 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.
##########
superset/app.py:
##########
@@ -78,6 +78,9 @@ def create_app(
for theme_key in ("THEME_DEFAULT", "THEME_DARK"):
theme = app.config[theme_key]
token = theme.get("token", {})
+ # Update brandSpinnerUrl if 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']}"
Review 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/`.
```suggestion
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}"
```
--
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]
