sadpandajoe commented on code in PR #42602:
URL: https://github.com/apache/superset/pull/42602#discussion_r3698220921
##########
docs/src/components/StorybookWrapper.jsx:
##########
@@ -78,15 +80,39 @@ function getProviders() {
return container || document.body;
};
+ // `themeObject` is a module-level singleton (superset-core/src/theme
+ // index.tsx: `Theme.fromConfig()`), created once with no dark/light
+ // config, so SupersetThemeProvider always rendered whatever that default
+ // algorithm was -- it had no way to know about Docusaurus's theme toggle.
+ // Docusaurus tracks the toggle in React context (useColorMode), so
+ // mirror it onto the singleton via the toggleDarkMode() method Theme
+ // already exposes for exactly this purpose.
+ //
+ // Use useLayoutEffect (not useEffect) so the sync runs before the
+ // browser paints. This component only ever mounts client-side (it's
+ // built inside a BrowserOnly callback), so there's no SSR mismatch
+ // concern -- and running synchronously before paint avoids a brief
+ // flash of the singleton's previous palette when a page loads directly
+ // in dark mode or the toggle fires during route navigation.
+ function ThemeSync({ children }) {
+ const { colorMode } = useColorMode();
+ React.useLayoutEffect(() => {
Review Comment:
Every live example mounts this bridge for the same singleton, so a page with
N demos performs N identical theme recomputations and each one synchronously
notifies all N providers before paint. Could this bridge be mounted once, or
could an identical-mode toggle be a no-op, to avoid quadratic work on
component-heavy pages?
##########
superset-frontend/packages/superset-core/src/theme/Theme.tsx:
##########
@@ -214,9 +230,42 @@ export class Theme {
emotionCache: createCache({ key: 'superset' }),
});
- this.updateProviders = (theme, antdConfig, emotionCache) => {
- setThemeState({ theme, antdConfig, emotionCache });
- };
+ // Register (and, on unmount, deregister) this provider instance's own
+ // listener rather than assigning a single shared callback on every
+ // render, so every concurrently mounted provider for this Theme
+ // instance receives updates, not just the last one to render.
+ //
+ // Use useLayoutEffect (not useEffect) so registration happens in the
+ // same commit phase as any layout effect elsewhere that might call
+ // setConfig/toggleDarkMode on this instance during mount (e.g. the
+ // docs site's dark-mode sync in StorybookWrapper.jsx, which reads the
+ // toggle and pushes it onto the singleton via a layout effect of its
+ // own). Layout effects run bottom-up, so a listener registered here
+ // (this component is nested inside that caller) is guaranteed to be
+ // in place before an ancestor's layout effect can fire and notify it.
+ // If this were a passive effect instead, an ancestor's layout effect
+ // could call toggleDarkMode before this listener exists, dropping that
+ // notification, and the provider would render stale until a later
+ // toggle.
+ // eslint-disable-next-line react-hooks/rules-of-hooks
+ React.useLayoutEffect(() => {
Review Comment:
This still leaves a render-to-subscription window: a descendant layout
effect can mutate the shared `Theme` after this provider snapshots it but
before this listener is registered, so the provider commits the stale palette
and never receives that update. Could registration reconcile against the
current snapshot (or use an external-store subscription) and cover the
descendant-effect ordering?
--
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]
