Copilot commented on code in PR #40777:
URL: https://github.com/apache/superset/pull/40777#discussion_r3364300651
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -1015,32 +1030,81 @@ export class ThemeController {
*/
private async fetchSystemDefaultTheme(): Promise<AnyThemeConfig | null> {
try {
- // Try to fetch theme marked as system default (is_system_default=true)
- const defaultResponse = await fetch(
-
'/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))',
- );
- if (defaultResponse.ok) {
- const data = await defaultResponse.json();
- if (data.result?.length > 0) {
- const themeConfig = JSON.parse(data.result[0].json_data);
+ // Try to use SupersetClient first if it has been configured
+ try {
+ const response = await SupersetClient.get({
Review Comment:
The note above `fetchSystemDefaultTheme()` says this method uses raw
`fetch()` instead of `SupersetClient`, but the updated implementation now tries
`SupersetClient.get()` first. Updating the note will prevent future confusion
when debugging embedded/guest-token flows.
##########
superset/models/core.py:
##########
@@ -146,6 +146,22 @@ class Theme(AuditMixinNullable, ImportExportMixin, Model):
export_fields = ["theme_name", "json_data"]
+# Event listeners to clear the memoized bootstrap data cache when a theme is
modified
[email protected]_for(Theme, "after_insert")
[email protected]_for(Theme, "after_update")
[email protected]_for(Theme, "after_delete")
+def clear_bootstrap_cache(
+ mapper: sqla.orm.Mapper, connection: sqla.engine.Connection, target: Theme
+) -> None:
Review Comment:
`clear_bootstrap_cache` doesn't use its SQLAlchemy event parameters, which
will trigger `pylint`'s `unused-argument` on changed Python files. Rename the
parameters to `_mapper/_connection/_target` (or disable `unused-argument`) to
keep the listener signature but satisfy linting.
##########
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:
When `hasThemeConfigOverride` is active, the component returns children
without the dashboard ThemeProvider, but the font `@import` effect can still
run and inject dashboard-provided fonts. That can still influence the rendered
UI and undermines the intent of skipping the dashboard theme. Gate the effect
on `hasThemeConfigOverride` and include it in the dependency array so existing
injected styles are cleaned up if the override becomes active after initial
render.
--
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]
