codeant-ai-for-open-source[bot] commented on code in PR #37370:
URL: https://github.com/apache/superset/pull/37370#discussion_r2718727784
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -291,14 +291,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;
Review 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]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Browser title fails to revert after leaving Explore.
- ⚠️ Users see stale tab titles after navigating away.
```
</details>
```suggestion
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(() => {
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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.
```
</details>
##########
superset/config.py:
##########
@@ -902,6 +902,8 @@ class D3TimeFormat(TypedDict, total=False):
THEME_DEFAULT: Theme = {
"token": {
# Brand
+ # Application name for window titles
+ "brandAppName": APP_NAME,
Review 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]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Browser title shows wrong app name on login.
- ❌ Dashboard/tab titles show stale/default name.
- ⚠️ Breaks backward compatibility for APP_NAME overrides.
```
</details>
```suggestion
"brandAppName": os.environ.get("SUPERSET_APP_NAME", APP_NAME or
"Superset"),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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.
```
</details>
--
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]
