codeant-ai-for-open-source[bot] commented on code in PR #36951:
URL: https://github.com/apache/superset/pull/36951#discussion_r3454016187
##########
superset/config.py:
##########
@@ -1008,9 +1019,10 @@ class D3TimeFormat(TypedDict, total=False):
"brandLogoAlt": "Apache Superset",
"brandLogoUrl": APP_ICON,
"brandLogoMargin": "18px 0",
- "brandLogoHref": "/",
+ "brandLogoHref": LOGO_TARGET_PATH or "/",
Review Comment:
**Suggestion:** `THEME_DEFAULT` is built before `superset_config.py`
overrides are imported, so this expression captures the initial
`LOGO_TARGET_PATH` value and will not reflect user overrides made in
`superset_config.py`. As a result, setting only `LOGO_TARGET_PATH` still won't
update the logo link at runtime. Recompute
`THEME_DEFAULT["token"]["brandLogoHref"]` after local config overrides are
loaded (or derive it lazily at bootstrap time) so `LOGO_TARGET_PATH` actually
takes effect. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Brand logo link ignores LOGO_TARGET_PATH overrides.
- ⚠️ Admin-configured logo URL never reflected in UI.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the documented override mechanism in `superset/config.py:17-21`,
which states that
all configuration in this file can be overridden by providing a
`superset_config` module
(`from superset_config import *` at the end of the file).
2. Observe where `LOGO_TARGET_PATH` and `THEME_DEFAULT` are defined:
`LOGO_TARGET_PATH` is
set near `superset/config.py:407-423`, and
`THEME_DEFAULT["token"]["brandLogoHref"]` is
initialized earlier at `superset/config.py:65-75` with `"brandLogoHref":
LOGO_TARGET_PATH
or "/"` (shown in the diff at line 1022), meaning this expression is
evaluated once when
the module is imported.
3. At the bottom of `superset/config.py:2785-2824`, see that
`superset_config` overrides
are loaded *after* `THEME_DEFAULT` has been built (`import superset_config`
and `from
superset_config import *`), so a `superset_config.py` that only sets
`LOGO_TARGET_PATH =
"https://example.com"` will update `config.LOGO_TARGET_PATH` but will not
recompute
`config.THEME_DEFAULT["token"]["brandLogoHref"]`, which remains the value
computed when
`LOGO_TARGET_PATH` was `None` (i.e., "/").
4. Follow the runtime usage of `THEME_DEFAULT`:
`superset/views/base.py:get_theme_bootstrap_data()` at lines 12–21 reads
`config_theme_default = get_config_value("THEME_DEFAULT")` and
`common_bootstrap_payload()` at `superset/views/base.py:20-24` updates the
SPA bootstrap
data with `bootstrap_data.update(get_theme_bootstrap_data())` (lines 14–15
of the
snippet), so the frontend receives the stale `"brandLogoHref": "/"` even when
`LOGO_TARGET_PATH` has been overridden in `superset_config`, confirming that
computing
`"brandLogoHref": LOGO_TARGET_PATH or "/"` at import time ignores later user
overrides.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=308c8caa58f142f3959c34bbda78503c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=308c8caa58f142f3959c34bbda78503c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/config.py
**Line:** 1022:1022
**Comment:**
*Incomplete Implementation: `THEME_DEFAULT` is built before
`superset_config.py` overrides are imported, so this expression captures the
initial `LOGO_TARGET_PATH` value and will not reflect user overrides made in
`superset_config.py`. As a result, setting only `LOGO_TARGET_PATH` still won't
update the logo link at runtime. Recompute
`THEME_DEFAULT["token"]["brandLogoHref"]` after local config overrides are
loaded (or derive it lazily at bootstrap time) so `LOGO_TARGET_PATH` actually
takes effect.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36951&comment_hash=747569e3409730170f597dd74d108a3701e1317e52d56a450587a732d2930cf3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36951&comment_hash=747569e3409730170f597dd74d108a3701e1317e52d56a450587a732d2930cf3&reaction=dislike'>👎</a>
##########
tests/unit_tests/config_test.py:
##########
@@ -312,3 +312,24 @@ def test_full_setting(
assert dttm_col.is_dttm
assert dttm_col.python_date_format == "epoch_s"
assert dttm_col.expression == "CAST(dttm as INTEGER)"
+
+
+def test_theme_default_logo_target_path() -> None:
+ """
+ Test that THEME_DEFAULT properly uses LOGO_TARGET_PATH for brandLogoHref.
+
+ When LOGO_TARGET_PATH is None (default), brandLogoHref should be "/".
+ When LOGO_TARGET_PATH is set, it should be used instead.
+ """
+ from superset import config
+
+ # Verify default behavior: LOGO_TARGET_PATH is None, so brandLogoHref is
"/"
+ assert config.LOGO_TARGET_PATH is None
+ assert config.THEME_DEFAULT["token"]["brandLogoHref"] == "/"
+
+ # Verify the expression logic works correctly for custom values
+ custom_path = "https://example.com";
+ assert (custom_path or "/") == custom_path
Review Comment:
**Suggestion:** This assertion is a tautology and does not validate the
config wiring at all, so the test can pass even when `THEME_DEFAULT` ignores
`LOGO_TARGET_PATH`. Replace this with a real override test (e.g., patch/reload
config with a custom `LOGO_TARGET_PATH` and assert
`THEME_DEFAULT["token"]["brandLogoHref"]` changes accordingly) to catch the
integration bug. [code quality]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Test cannot detect regressions in logo href wiring.
- ⚠️ Gives false confidence about theme configuration coverage.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `test_theme_default_logo_target_path` in
`tests/unit_tests/config_test.py:18-36`, which imports `from superset import
config` and
asserts the default wiring (`config.LOGO_TARGET_PATH is None` and
`config.THEME_DEFAULT["token"]["brandLogoHref"] == "/"`) before reaching the
lines under
review.
2. At lines 331–332 in the diff, the test introduces a local variable and
assertion:
`custom_path = "https://example.com"` and `assert (custom_path or "/") ==
custom_path`;
this expression uses only the local variable and does not reference `config`
or
`THEME_DEFAULT` at all.
3. Evaluate the assertion logic: in Python, a non-empty string is truthy, so
`(custom_path
or "/")` always evaluates to `custom_path` when `custom_path =
"https://example.com"`,
making `assert (custom_path or "/") == custom_path` a tautology that will
always pass
regardless of how `LOGO_TARGET_PATH` or
`THEME_DEFAULT["token"]["brandLogoHref"]` are
wired.
4. Because this assertion never consults `config.LOGO_TARGET_PATH` or
`config.THEME_DEFAULT`, any future regression where `"brandLogoHref"` stops
honoring a
custom `LOGO_TARGET_PATH` set in `superset_config` (as described in
`superset/config.py:17-21` and wired at `superset/config.py:65-75`) would
still see this
test passing, demonstrating that the current test does not validate the
configuration
behavior it claims to cover.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0a8ea5e781f4f7fadab58efd8d0fc52&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a0a8ea5e781f4f7fadab58efd8d0fc52&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/config_test.py
**Line:** 331:332
**Comment:**
*Code Quality: This assertion is a tautology and does not validate the
config wiring at all, so the test can pass even when `THEME_DEFAULT` ignores
`LOGO_TARGET_PATH`. Replace this with a real override test (e.g., patch/reload
config with a custom `LOGO_TARGET_PATH` and assert
`THEME_DEFAULT["token"]["brandLogoHref"]` changes accordingly) to catch the
integration bug.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36951&comment_hash=c2e6feb9f86dd35729db7ca3db583537ea910d6e684a38f952d5f0a27925b762&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36951&comment_hash=c2e6feb9f86dd35729db7ca3db583537ea910d6e684a38f952d5f0a27925b762&reaction=dislike'>👎</a>
--
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]
