-
Notifications
You must be signed in to change notification settings - Fork 652
perf(PageLayout): eliminate ~614ms forced reflow from getComputedStyle on mount #7532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Replace getPaneMaxWidthDiff (which calls getComputedStyle, forcing a synchronous layout recalc) with getMaxWidthDiffFromViewport, a pure JS function that derives the same value from window.innerWidth. The CSS variable --pane-max-width-diff only has two values controlled by a single @media (min-width: 1280px) breakpoint, so a simple threshold check is semantically equivalent — no DOM query needed. This eliminates ~614ms of blocking time on mount for pages with large DOM trees (e.g. SplitPageLayout).
🦋 Changeset detectedLatest commit: 76ed144 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR removes a major mount-time performance bottleneck in PageLayout by eliminating a getComputedStyle() call that can force expensive synchronous layout recalculation. It replaces the DOM/CSS variable read with a viewport-width-based lookup derived from the same breakpoint logic already encoded in PageLayout.module.css.
Changes:
- Add
getMaxWidthDiffFromViewport()and use it on mount and when crossing the 1280px breakpoint, avoidinggetComputedStyle()forced reflow. - Export the wide breakpoint
--pane-max-width-diffvalue (959) fromPageLayout.module.cssvia:exportto keep JS/CSS in sync. - Update and extend unit tests to reflect correct wide-breakpoint behavior and cover the new helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/PageLayout/usePaneWidth.ts | Replaces mount and resize-breakpoint diff calculation with a viewport-only lookup helper. |
| packages/react/src/PageLayout/usePaneWidth.test.ts | Updates expectations for wide breakpoint behavior and adds new helper unit tests. |
| packages/react/src/PageLayout/PageLayout.module.css | Exposes the wide diff value (paneMaxWidthDiffWide) via :export for JS consumption. |
| .changeset/pagelayout-remove-reflow.md | Adds a patch changeset describing the performance fix. |
…romViewport tests
Replace vi.spyOn(window, 'innerWidth', 'get').mockReturnValue(...) with
vi.stubGlobal('innerWidth', ...) to prevent spy leaks. The outer describe
block's afterEach calls vi.unstubAllGlobals(), which correctly cleans up
stubGlobal but does not restore spyOn mocks. This makes the tests
consistent with the rest of the file and avoids order-dependent failures.
…nments Add canUseDOM check so the function returns DEFAULT_MAX_WIDTH_DIFF instead of throwing when window is unavailable (SSR, node tests, build-time evaluation). canUseDOM was already imported in the file.
francinelucca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good! some questions/suggestions
| // Initial --pane-max-width should be set on mount (1280 - 959 wide diff = 321) | ||
| expect(refs.paneRef.current?.style.getPropertyValue('--pane-max-width')).toBe('321px') | ||
|
|
||
| // Shrink viewport | ||
| // Shrink viewport (crosses 1280 breakpoint, diff switches to 511) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making sure I understand: this value changed because it's correct from the get-go now?
| export function getPaneMaxWidthDiff(paneElement: HTMLElement | null): number { | ||
| if (!paneElement) return DEFAULT_MAX_WIDTH_DIFF | ||
| const value = parseInt(getComputedStyle(paneElement).getPropertyValue('--pane-max-width-diff'), 10) | ||
| return value > 0 ? value : DEFAULT_MAX_WIDTH_DIFF | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we able to delete this function then? 👀
| --pane-width-medium: 100%; | ||
| --pane-width-large: 100%; | ||
| /* NOTE: This value is exported via :export for use in usePaneWidth.ts */ | ||
| --pane-max-width-diff: 511px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this variable anymore then 👀
| // Viewport changes (no resize event fired, so maxWidthDiffRef stays at 959) | ||
| vi.stubGlobal('innerWidth', 800) | ||
|
|
||
| // getMaxPaneWidth reads window.innerWidth dynamically | ||
| expect(result.current.getMaxPaneWidth()).toBe(289) | ||
| // getMaxPaneWidth reads window.innerWidth dynamically: max(256, 800 - 959) = 256 | ||
| expect(result.current.getMaxPaneWidth()).toBe(256) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we dispatch the resized event here instead so the value updates? otherwise I'm not sure what this part of the test is achieving 😅
Overview
Across all three profiled routes under the Files controller on .com,
SplitPageLayout's internalusePaneWidthhook is the single largest source of client-side latency. The root cause isgetPaneMaxWidthDiff, which callsgetComputedStyle()inside auseLayoutEffecton mount, forcing the browser to synchronously resolve all pending layout work before it can return a CSS variable value.Impact on github.com (production profiling)
getPaneMaxWidthDiffOn the Code View page,
getPaneMaxWidthDiffalone forces the browser to synchronously lay out 430 pending DOM nodes in a single 529ms layout update. On the smaller Code View page, it contributes 124ms with an additional 236ms from other Primer React internals, bringing Primer's combined share to 360ms of 370ms (97%). On the Repo Overview page, Primer React similarly dominates with 379ms of forced reflow — 100% of the reflow budget.Root cause
getPaneMaxWidthDiffcallsgetComputedStyle(paneElement).getPropertyValue('--pane-max-width-diff')inside auseLayoutEffecton mount. This forces a synchronous layout recalculation because:useLayoutEffect), after React has flushed all DOM mutations but before paintgetComputedStylequerySplitPageLayoutis a top-level layout wrapper, so the dirty subtree is typically the entire pageThe fix
The CSS variable
--pane-max-width-diffonly has two possible values, controlled by a single@media (min-width: 1280px)breakpoint:511px959pxThis is fully determinable from
window.innerWidth— nogetComputedStyleneeded. This PR replaces the DOM query with a pure JS viewport width check:The same replacement is applied to the resize handler's breakpoint-crossing path for consistency (previously guarded but still called
getComputedStylewhen crossing the 1280px threshold).Chrome DevTools Performance Trace Results
Profiled using Chrome DevTools traces on the Heavy Content performance story (~5,400 DOM nodes, 1,366 flex containers — representative of a real github.com page).
PageLayout-caused Forced Reflow
main)getPaneMaxWidthDiff)getPaneMaxWidthDiff)Style Recalculation
LCP (4x CPU throttle)
Drag Performance (unchanged, as expected)
Other Metrics
Changelog
New
getMaxWidthDiffFromViewport()— derives the--pane-max-width-diffvalue fromwindow.innerWidthwithout touching the DOMChanged
usePaneWidthnow uses the viewport-based lookup instead ofgetComputedStyle959) fromPageLayout.module.cssvia:exportto keep JS and CSS in syncRemoved
getComputedStylecalls for--pane-max-width-diff(the function is retained for potential future use but is no longer called)Rollout strategy
Testing & Reviewing
usePaneWidthtests pass, with expectations corrected to reflect real-world breakpoint behavior at viewport ≥1280pxgetMaxWidthDiffFromViewportcovering below, at, and above the 1280px breakpointgetComputedStylealways returned the default (511) even at 1280px viewport. The new viewport-based approach correctly returns959at that width, and the test expectations have been updated accordinglyMerge checklist