gabotorresruiz commented on code in PR #43778:
URL: https://github.com/apache/superset/pull/43778#discussion_r3915715039
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -560,6 +577,78 @@ export class ThemeController {
this.updateTheme(themeToApply);
}
+ /**
+ * Re-reads the persisted system default/dark themes from the server and
+ * re-applies them live, so changes made on the Themes admin page take effect
+ * without a full page reload. The endpoint returns the same resolved theme
+ * slice used to bootstrap the page, so the live result matches a reload.
+ *
+ * Bails before applying whenever an explicit theme-config override is active
+ * (e.g. from the Embedded SDK) — checked both at entry and again after the
+ * fetch resolves — so it does not overwrite an externally-provided theme.
+ *
+ * Non-throwing: the server mutation has already succeeded by the time this
+ * runs, so a failed refresh must not surface a failure to the caller. A
+ * failed fetch or parse logs and leaves the current theme unchanged; if a
+ * fetched slice is valid-shaped but throws while applying, updateTheme's own
+ * recovery path handles the fallback.
+ */
+ public async refreshSystemThemes(): Promise<void> {
+ // An explicit theme-config override takes precedence over system themes.
+ if (this.themeConfigOverride) return;
+
+ // Assign this refresh a monotonically increasing id so out-of-order
+ // responses can be resolved by "newest successfully-applied wins".
+ this.refreshSeq += 1;
+ const seq = this.refreshSeq;
+
+ try {
+ const response = await SupersetClient.get({
+ endpoint: '/api/v1/theme/system',
+ });
+ // Drop this response if a newer refresh has already applied a slice (so
a
+ // slow older request can't clobber it, and a newer request that fails to
+ // fetch can't discard this valid one), or if an embedded theme-config
+ // override took over while this request was in flight.
+ if (seq <= this.appliedRefreshSeq || this.themeConfigOverride) return;
+
+ const themeConfig = response.json?.result as
+ | BootstrapThemeDataConfig
+ | undefined;
+ if (!themeConfig) return;
+
+ // This response wins; record it before mutating so an older in-flight
+ // response can't overwrite it.
+ this.appliedRefreshSeq = seq;
+
+ const {
+ bootstrapDefaultTheme,
+ bootstrapDarkTheme,
+ bootstrapDefaultMode,
+ } = this.parseThemeConfig(themeConfig);
+
+ // Reproduce the constructor's slot assignments so live == reload.
+ this.defaultTheme =
+ bootstrapDefaultTheme || this.builtInDefaultTheme || null;
+ this.darkTheme = bootstrapDarkTheme;
+ this.bootstrapDefaultMode = bootstrapDefaultMode;
+
+ // Dark-theme availability may have changed (set or unset); re-sync the
+ // prefers-color-scheme listener so SYSTEM-mode OS switching stays
correct.
+ this.reconcileMediaQueryListener();
+
Review Comment:
Not a blocker, and this whole flow is really nicely done. One small gap I
could actually reproduce, building on what the Bito run hinted at:
`refreshSystemThemes()` re-syncs the listener and the theme slots but never
refreshes `this.systemMode`, which the constructor sets at line 160 and only
`handleSystemThemeChange` updates afterwards. Since that handler skips the
repaint when `oldSystemMode === newSystemMode`, a stale value swallows exactly
one OS transition.
I verified it live on this branch with `THEME_DARK = None` (so no listener
at boot): loaded the app with the OS in light mode, emulated a flip to dark
while untracked, set a system dark theme from the Themes page (applied live, as
intended), switched to "Match system" (rendered dark correctly, since
`getThemeForMode` resolves the OS preference live), then flipped the OS back to
light: the app stayed dark, and only recovered after a second full flip cycle.
One line right after `reconcileMediaQueryListener()` fixes it:
```ts
this.systemMode = ThemeController.getSystemPreferredMode();
```
I ran the full ThemeController suite with that added and all 101 tests still
pass. A test to lock it in: construct with no dark theme, flip the mocked media
query `matches` to true, `refreshSystemThemes` with a dark slice, then fire the
registered change handler with `matches` back to false and assert a repaint
happened (today it doesn't, because `oldSystemMode` still holds the
construction-time value). Admittedly a narrow edge, but it keeps "live ==
reload" exactly true.
--
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]
