Eliminate nested-update cascades in useFocus, PageLayout.Pane, and AnchoredOverlay#7864
Eliminate nested-update cascades in useFocus, PageLayout.Pane, and AnchoredOverlay#7864mattcosta7 wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 3edf601 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 |
|
There was a problem hiding this comment.
Pull request overview
This PR reduces unnecessary render/commit cascades in @primer/react by adjusting how focus targeting, pane width initialization, and anchored overlay positioning listeners are managed, and by adding a small Profiler-based test helper to lock these behaviors in place.
Changes:
- Add a
createRenderCounter()Profiler harness for asserting update and nested-update commit counts in tests. - Update
useFocusto avoid a second render caused by clearing state in an effect (ref + version counter). - Avoid a mount-time layout-effect nested update in
usePaneWidth, and stopAnchoredOverlayfrom keeping JS positioning listeners attached while closed.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/utils/testing/profiler.tsx | Adds a small Profiler-based render counter utility for tests. |
| packages/react/src/PageLayout/usePaneWidth.ts | Defers the mount-time setMaxPaneWidth state sync via startTransition to avoid nested-update cascades. |
| packages/react/src/PageLayout/PageLayout.test.tsx | Adds a regression test asserting no nested-update cascade on mount for resizable panes. |
| packages/react/src/internal/hooks/useFocus.ts | Reworks focus targeting to avoid a two-render cascade by using a ref + version state. |
| packages/react/src/internal/hooks/tests/useFocus.test.tsx | Adds tests that pin render counts and verify focus behavior. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx | Gates useAnchoredPosition with open to detach listeners while closed. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx | Adds a regression test ensuring scroll after close doesn’t cause a re-render. |
| .changeset/cascade-elimination.md | Adds a patch changeset describing the cascade eliminations and new test harness. |
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 2
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/21358 |
Closes #
This PR eliminates three measurable nested-update / render cascades in
@primer/react, adds a small<Profiler>-based test harness so cascades can be pinned in tests, and documents the audit that uncovered them.A "cascade" here means: one logical user action (or mount) causes React to commit more renders than it strictly needs. The audit covered every
.ts/.tsxfile underpackages/react/src/, includingexperimental/,deprecated/,next/,internal/,live-region/, andutils/.Changelog
Changed
1.
useFocus— eliminated two-renders-per-call cascadeFile:
packages/react/src/internal/hooks/useFocus.ts(internal, not publicly exported)Before: the hook stored the focus target in
useState. Callingfocus(target)caused:setFocusTarget(target)→ render.focus(), thensetFocusTarget(null)→ another renderThat's two renders for one
focus()call.After: target is held in a
useRef, and the effect is gated by a monotonicversioncounter. Eachfocus()call now produces exactly one render. The returned function is wrapped inuseCallbackso its identity is stable.Test:
useFocus.test.tsxcontains three assertions, each of which fails against the unchanged code and passes against the fix:counter.updateCount === 1after onefocus()callfocus()call2.
PageLayout.Pane— eliminateduseLayoutEffect → setStatecascade on mountFiles:
packages/react/src/PageLayout/usePaneWidth.ts,packages/react/src/PageLayout/PageLayout.test.tsxBefore: the resize layout effect called
setMaxPaneWidth(initialMax)unconditionally on mount. Initial state wascustomMaxWidth ?? SSR_DEFAULT_MAX_WIDTH(typically600); for any viewport-derived case the computed value differed from initial state, forcing a synchronous re-render before paint — a Profilernested-update.After:
setMaxPaneWidth(initialMax)is wrapped instartTransition, pushing the update into the transition lane so it's not processed in the same commit cycle as the layout effect. The CSS variable (--pane-max-width) and ARIA values are still updated imperatively in the layout effect, so consumers (including screen readers) see the correct value before paint. This matches the pattern already used bysyncAllelsewhere in the same file for the resize-path setStates.Test:
PageLayout.test.tsxassertscounter.nestedUpdateCount === 0after mounting a resizablePageLayout.Pane. The original code would produce ≥1 nested update on mount; the new code produces 0.3.
AnchoredOverlay— eliminated closed-overlay scroll/resize cascadeFiles:
packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx,packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsxBefore:
AnchoredOverlaypassedenabled: !cssAnchorPositioningtouseAnchoredPosition, gating only on the CSS feature — meaninguseAnchoredPosition's scroll listeners andResizeObserverstayed attached even when the overlay was closed. After an open→close cycle, the first window resize or ancestor scroll firedupdatePosition, which hit the "refs not attached" else branch and calledsetPosition(undefined). Becausepositionwas still the last-open value, React committed an update and the closed overlay's parent re-rendered.After:
enabled: open && !cssAnchorPositioning. When the overlay is closed the hook tears down its listeners (via cleanup paths theenabledflag already had). No work, no setState, no re-render.The
useAnchoredPositionhook itself is unchanged. Its public test that assertsonPositionChangeis called withundefinedwhen the overlay is closed-on-mount continues to pass, because that test instantiates the hook directly with always-null refs andenabled: true(the default).AnchoredOverlay's own forwarding ofonPositionChangealready filters outundefined(if (onPositionChange && position)), so no consumer ofAnchoredOverlay's public API can observe the behavior change.Test:
AnchoredOverlay.test.tsxopens then closes the overlay, dispatches ascrollevent onwindow, flushes rAF, and assertscounter.updateCount === 0. The original code would produce 1 update; the new code produces 0.Removed
Nothing public. Internally, the unused exported
createRenderTrackerwas inlined into its one consumer (the thirduseFocustest) and removed from the harness.Rollout strategy
No public API surface changes.
useFocusis an internal hook.AnchoredOverlay's publiconPositionChangecallback already filteredundefinedbefore forwarding to consumers, so the slight semantic change in whenuseAnchoredPositionfiresonPositionChange(undefined)is unobservable through the public API.PageLayout.Pane'saria-valuemaxis set on the DOM imperatively before paint, so screen-reader behavior is unchanged.Testing & Reviewing
Each shipped change has a profiler test designed to fail against the unchanged code. Manual verification path:
main, revert any one of the three source files, and run the corresponding test — it should fail.The new test harness at
packages/react/src/utils/testing/profiler.tsxis small and intentionally exposes only what the existing fixes consume, so its surface area is reviewable in isolation.Discipline applied to this PR: the audit also surfaced several "wasted work but no render-count change" candidates (depless ref-sync effects, an effect-merge opportunity in
Checkbox, a redundantuseEffectinuseRenderForcingRef, etc.). Every change of that shape was reverted because a profiler test could not distinguish the pre-change behavior from the post-change behavior. The "Investigated and reverted" table incontributor-docs/cascade-audit.mdenumerates them.Out of scope (recommended follow-ups, listed in the audit doc):
enabled: openpattern toexperimental/SelectPanel2'suseAnchoredPositioncall.usePaneWidth'smaxPaneWidthfromuseStatetouseSyncExternalStoreso mount produces 1 render instead of 2 (current fix moves the second render off the critical path; this would eliminate it).Breadcrumbs,LabelGroup,UnderlineNav) so structural cascades there stop regressing further.Merge checklist
contributor-docs/cascade-audit.md)usePaneWidthSSR snapshot is unchanged)github/github-ui