Uh oh!
There was an error while loading. Please reload this page.
perf(ThemeProvider): Reduce unnecessary renders and effect cascades - #7695
Conversation
🦋 Changeset detectedLatest commit: e11672a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
There was a problem hiding this comment.
Pull request overview
This PR refactors ThemeProvider implementations (both @primer/react and @primer/styled-react) to simplify SSR color-mode handoff handling by removing the ReactDOM-based flushSync workaround and by making context value construction more stable.
Changes:
- Replace the server color-mode passthrough
useRef+ReactDOM.flushSyncworkaround with aserverColorModestate that is cleared after hydration. - Memoize the
ThemeContext.Providervalue viauseMemoto reduce unnecessary context value churn. - Remove the unused
react-domimport from both ThemeProvider implementations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/styled-react/src/components/ThemeProvider.tsx | Aligns styled-react ThemeProvider SSR handoff logic with a state-based server passthrough and memoized context value. |
| packages/react/src/ThemeProvider.tsx | Updates core ThemeProvider SSR handoff logic to remove ReactDOM.flushSync usage and memoizes the context value. |
Comments suppressed due to low confidence (1)
packages/react/src/ThemeProvider.tsx:88
- ThemeProvider has an existing unit test suite, but the
preventSSRMismatch/ server handoff path introduced here isn’t covered. Please add a test that simulates server handoff data (e.g., by mockinguseIdto a stable value and inserting a matching__PRIMER_DATA_<id>__script element) and asserts that the provider initially uses the server-resolved color mode and then switches to client resolution after hydration.
// Lazy initializer reads DOM + parses JSON once instead of every render
const [serverColorMode, setServerColorMode] = React.useState<ColorMode | undefined>(
() => getServerHandoff(uniqueDataId).resolvedServerColorMode,
)
const [colorMode, setColorMode] = useSyncedState(props.colorMode ?? fallbackColorMode ?? defaultColorMode)
const [dayScheme, setDayScheme] = useSyncedState(props.dayScheme ?? fallbackDayScheme ?? defaultDayScheme)
const [nightScheme, setNightScheme] = useSyncedState(props.nightScheme ?? fallbackNightScheme ?? defaultNightScheme)
const systemColorMode = useSystemColorMode()
const resolvedColorMode = serverColorMode ?? resolveColorMode(colorMode, systemColorMode)
const colorScheme = chooseColorScheme(resolvedColorMode, dayScheme, nightScheme)
const {resolvedTheme, resolvedColorScheme} = React.useMemo(
() => applyColorScheme(theme, colorScheme),
[theme, colorScheme],
)
// After hydration, clear the server passthrough so client-side color mode takes over
React.useEffect(
function clearServerPassthrough() {
if (serverColorMode !== undefined) {
setServerColorMode(undefined)
}
},
[serverColorMode],
)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…m color mode - Replace useState + useEffect SSR hydration handoff with useSyncExternalStore - Replace useState + useEffect in useSystemColorMode with useSyncExternalStore - Cache getServerHandoff DOM read + JSON.parse per ID - Remove post-hydration re-render and effect cascades
mattcosta7
commented
Mar 25, 2026
CI in gh/gh-ui wil fail until the current release merges and removes the unused feature flag |
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/17032 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Performance improvements to
ThemeProviderin both@primer/reactand@primer/styled-reactby replacinguseState+useEffectpatterns withuseSyncExternalStore. All changes maintain identical user-facing behavior.Changes
1.
useSyncExternalStorefor SSR hydration handoffBefore:
setTimeout→ReactDOM.flushSync(setColorMode(resolved))→setColorMode(original)— 3 renders, forced sync flush, temporarily corruptedcolorModestate. The previous commit simplified this touseState+useEffect, but still caused a post-hydration re-render.After:
useSyncExternalStorewithgetServerSnapshotreading the<script type="application/json">tag during hydration, andgetSnapshotresolving from client state after. Zero post-hydration re-renders. No effects.Also removes the
ReactDOMimport entirely.2.
useSyncExternalStoreforuseSystemColorModeBefore:
useState(getSystemColorMode)+useEffectsubscribing tomatchMediachanges — potential stale-then-update flicker between init and effect, and a redundantsetSystemColorModecall to catch changes during the gap.After:
useSyncExternalStore(subscribeToSystemColorMode, getSystemColorMode, () => "day")— always current, no effect gap, no state.3. Cached
getServerHandoffDOM read +
JSON.parseis now cached per ID in aMap. The<script>tag content is static (written once during SSR), so subsequent calls return the cached result.4. Memoized context value
The
ThemeContext.Providervalue was a new object literal every render, causing all consumers to re-render even when nothing changed. Now wrapped inuseMemo.Scenario analysis
useSyncExternalStorehandles transition)useEffectsubscribes, potential flickeruseSyncExternalStore— always synchronousmatchMediacallsMediaQueryListallocation)Testing
All existing ThemeProvider tests pass (20 in
@primer/react, 29 in@primer/styled-react).