codeant-ai-for-open-source[bot] commented on code in PR #40777:
URL: https://github.com/apache/superset/pull/40777#discussion_r3358303631
##########
superset-frontend/src/components/CrudThemeProvider.tsx:
##########
@@ -83,7 +91,7 @@ export default function CrudThemeProvider({
};
}, [dashboardTheme, fontUrls]);
- if (!dashboardTheme) {
+ if (!dashboardTheme || hasThemeConfigOverride) {
return <>{children}</>;
Review Comment:
**Suggestion:** The new early return skips rendering the dashboard
`SupersetThemeProvider` when an SDK override is active, but it does not stop or
clean up dashboard font injection, so dashboard `fontUrls` can still leak into
the document and override typography despite the SDK override. Include the
override flag in the font-injection effect logic/dependencies and ensure styles
are removed when the override becomes active. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Embedded dashboards use dashboard fonts instead of SDK fonts.
- ⚠️ SDK theme changes leave stale dashboard font CSS active.
- ⚠️ Branding/typography inconsistent across embedded dashboard views.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a dashboard with a custom theme that includes `fontUrls` in its
JSON config
(e.g., `{ token: { fontUrls: [...] } }`), which is read and normalized in
`superset-frontend/src/components/CrudThemeProvider.tsx:52-70` via
`JSON.parse(theme.json_data)` and `Theme.fromConfig`, returning `{
dashboardTheme,
fontUrls }`.
2. Run the app under the global `SupersetThemeProvider`
(`superset-frontend/src/theme/ThemeProvider.tsx:43-57`) and apply an
Embedded SDK theme
override by calling `ThemeController.setThemeConfig(...)` (see
`superset-frontend/src/theme/ThemeController.ts:67-88`), which sets
`this.themeConfigOverride = true` and causes `hasThemeConfigOverride` in the
context to
become `true` via the effect at `ThemeProvider.tsx:59-75`.
3. Navigate to a dashboard route so that `DashboardPage` renders
(`superset-frontend/src/dashboard/containers/DashboardPage.tsx:11-13`),
which wraps
`DashboardContainer` in `CrudThemeProvider` and passes the dashboard theme
from the API;
inside `CrudThemeProvider` the `useEffect` at
`superset-frontend/src/components/CrudThemeProvider.tsx:77-92` still sees a
non-null
`dashboardTheme` and non-empty `fontUrls` and injects a `<style
data-superset-fonts>` tag
with `@import url(...)` rules into `document.head`, even though
`hasThemeConfigOverride`
is already `true`.
4. Because the early return at `CrudThemeProvider.tsx:94-96` (`if
(!dashboardTheme ||
hasThemeConfigOverride) { return <>{children}</>; }`) skips the dashboard
`SupersetThemeProvider` wrapper but the font-injection effect does not
depend on
`hasThemeConfigOverride` and never checks it, the injected dashboard font
CSS remains
active; when the SDK override is active (either from initial load or toggled
later), the
global SDK theme is used for colors but the stale dashboard `fontUrls`
stylesheet
continues to affect typography, overriding or conflicting with the
SDK-provided fonts.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=08e81653209840e08b882f7b63bea3a8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=08e81653209840e08b882f7b63bea3a8&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-frontend/src/components/CrudThemeProvider.tsx
**Line:** 94:95
**Comment:**
*Logic Error: The new early return skips rendering the dashboard
`SupersetThemeProvider` when an SDK override is active, but it does not stop or
clean up dashboard font injection, so dashboard `fontUrls` can still leak into
the document and override typography despite the SDK override. Include the
override flag in the font-injection effect logic/dependencies and ensure styles
are removed when the override becomes active.
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%2F40777&comment_hash=49c061d119c72369296cc3696e0764e77b30eff2c0957174d7a703cff6a0c3e2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40777&comment_hash=49c061d119c72369296cc3696e0764e77b30eff2c0957174d7a703cff6a0c3e2&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]
