perf(useIsMacOS): replace useState+useEffect with useSyncExternalStore#7548
Conversation
🦋 Changeset detectedLatest commit: 3b97134 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
04b72c7 to
7223e52
Compare
7223e52 to
3b97134
Compare
There was a problem hiding this comment.
Pull request overview
This PR optimizes the SSR-safe useIsMacOS hook by switching it to React 18’s useSyncExternalStore, removing an unconditional mount-time state update that caused an extra render pass in all consumers (e.g. Tooltip, KeybindingHint, TreeView).
Changes:
- Replaced
useState+useEffectinuseIsMacOSwithuseSyncExternalStoreto avoid a guaranteed second render on mount. - Added a changeset for a patch release describing the performance improvement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/react/src/hooks/useIsMacOS.ts | Refactors platform detection to useSyncExternalStore to avoid unnecessary mount re-renders while staying SSR-safe. |
| .changeset/perf-useismacOS-sync-external-store.md | Adds a patch changeset documenting the perf change for release notes. |
| export function useIsMacOS() { | ||
| const [isMacOS, setIsMacOS] = useState(() => (canUseDOM ? ssrUnsafeIsMacOS() : false)) | ||
|
|
||
| useEffect(() => { | ||
| // eslint-disable-next-line react-hooks/set-state-in-effect | ||
| setIsMacOS(ssrUnsafeIsMacOS()) | ||
| }, []) | ||
|
|
||
| return isMacOS | ||
| return useSyncExternalStore(subscribe, ssrUnsafeIsMacOS, getServerSnapshot) | ||
| } |
There was a problem hiding this comment.
useIsMacOS is a publicly exported hook and its behavior around SSR/hydration is subtle. There are hook unit tests for other hooks under packages/react/src/hooks/__tests__, but none for this one—please add a small useIsMacOS test that at least verifies SSR defaults to false (e.g. via react-dom/server), and client behavior can be controlled via a mocked @primer/behaviors/utils implementation.
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/13848 |
Closes #
useIsMacOSuseduseState+useEffectto detect the platform in an SSR-safe way. TheuseStateinitializer already computed the correct value on the client, but theuseEffectunconditionally calledsetIsMacOSwith the same value, scheduling a second render pass on every mount.Replaced with
useSyncExternalStore, the idiomatic React 18+ API for values that differ between server and client. During hydration, React handles the server/client mismatch internally in a single synchronous pass, no extra render needed.Eliminates a guaranteed double-render on every mount for all consumers: Tooltip, KeybindingHint, and TreeView. The impact scales with the number of children, since the unnecessary re-render cascades to all of them.
Measurements
Measured on the TreeView stress test (1000 tree items) as worst case. 2 runs each, reload-based traces, no CPU/network throttling.
Individual runs
Before (main):
After (PR):
TreeView shows the largest improvement because a single
useIsMacOScall at the root triggers a re-render that cascades across 1000 child items. Tooltip and KeybindingHint will see smaller absolute gains but every instance still saves one full render cycle on mount.Changelog
New
N/A
Changed
useIsMacOSnow usesuseSyncExternalStoreinstead ofuseState+useEffect, eliminating an unnecessary re-render on every mountRemoved
N/A
Rollout strategy
Testing & Reviewing
All 3 consumer test suites pass (89 tests total):
TooltipV2/__tests__/Tooltip.test.tsxKeybindingHint/KeybindingHint.test.tsxTreeView/TreeView.test.tsxTo verify the double render is eliminated:
console.log('render')inside the Tooltip componentMerge checklist