perf(ThemeProvider): Reduce unnecessary renders and effect cascades#7695
perf(ThemeProvider): Reduce unnecessary renders and effect cascades#7695mattcosta7 wants to merge 6 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 15a3037 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],
)
…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
|
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 |
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).