codeant-ai-for-open-source[bot] commented on code in PR #40763:
URL: https://github.com/apache/superset/pull/40763#discussion_r3355504915
##########
superset-frontend/src/components/CrudThemeProvider.tsx:
##########
@@ -83,7 +91,7 @@ export default function CrudThemeProvider({
};
}, [dashboardTheme, fontUrls]);
- if (!dashboardTheme) {
+ if (!dashboardTheme || hasThemeConfigOverride) {
Review Comment:
**Suggestion:** The new override check only skips rendering the dashboard
`SupersetThemeProvider`, but dashboard `fontUrls` are still processed and
injected by the effect. This means dashboard-level theme side effects still run
even when the SDK override is supposed to fully win. Guard the font injection
path with `hasThemeConfigOverride` (and ideally skip dashboard theme parsing
too) so dashboard theme artifacts are not applied when override mode is active.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Embedded dashboards with custom fonts load dashboard fonts
unnecessarily.
- ⚠️ Extra font imports remain when SDK theme override active.
- ⚠️ Theme override semantics incomplete; dashboard artifacts still applied.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a dashboard with a CRUD theme that includes font URLs in its
JSON (e.g.
`"token": { "fontUrls": [...] }`), which is sent as
`dashboard.theme.json_data` to the
frontend and passed into `CrudThemeProvider` via `DashboardPage` at
`superset-frontend/src/dashboard/containers/DashboardPage.tsx:29-31`.
2. In an embedded-dashboard context, have the embedding code call
`ThemeController.setThemeConfig()` with an SDK theme config that also defines
`token.fontUrls`, which sets `themeConfigOverride = true` in
`ThemeController` at
`superset-frontend/src/theme/ThemeController.ts:526-531` and exposes
`hasThemeConfigOverride` through `SupersetThemeProvider` at
`superset-frontend/src/theme/ThemeProvider.tsx:55-67`.
3. Render the dashboard page so that `CrudThemeProvider` runs: it reads
`hasThemeConfigOverride` from `ThemeContext` at
`superset-frontend/src/components/CrudThemeProvider.tsx:45-50`, parses
`theme.json_data`
into `dashboardTheme` and `fontUrls` in the `useMemo` block at `lines
52-70`, and then
executes the `useEffect` at `lines 77-92` which, when `dashboardTheme` and
`fontUrls` are
both truthy, appends a `<style data-superset-fonts>` element with `@import
url(...)` rules
to `document.head`.
4. Even though the JSX branch at
`superset-frontend/src/components/CrudThemeProvider.tsx:94-96` (`if
(!dashboardTheme ||
hasThemeConfigOverride) { return <>{children}</>; }`) skips wrapping
children in
`dashboardTheme.SupersetThemeProvider`, React still runs the `useEffect`
defined earlier;
as a result, dashboard font URLs are injected alongside the SDK override
fonts already
loaded by `ThemeController.applyTheme()` at
`superset-frontend/src/theme/ThemeController.ts:19-31`, leaving dashboard
theme font side
effects active while an SDK override is supposedly in control.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cc47f4c8036d4daea284644f3d05be32&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=cc47f4c8036d4daea284644f3d05be32&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:94
**Comment:**
*Incomplete Implementation: The new override check only skips rendering
the dashboard `SupersetThemeProvider`, but dashboard `fontUrls` are still
processed and injected by the effect. This means dashboard-level theme side
effects still run even when the SDK override is supposed to fully win. Guard
the font injection path with `hasThemeConfigOverride` (and ideally skip
dashboard theme parsing too) so dashboard theme artifacts are not applied when
override mode is 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%2F40763&comment_hash=2de5be962b4da54361f98bd8960a2b0acf568a7355f5cc35737e3b771cf199e4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40763&comment_hash=2de5be962b4da54361f98bd8960a2b0acf568a7355f5cc35737e3b771cf199e4&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]
