Skip to content

Eliminate nested-update cascades in useFocus, PageLayout.Pane, and AnchoredOverlay#7864

Open
mattcosta7 wants to merge 1 commit into
mainfrom
cascade-elimination
Open

Eliminate nested-update cascades in useFocus, PageLayout.Pane, and AnchoredOverlay#7864
mattcosta7 wants to merge 1 commit into
mainfrom
cascade-elimination

Conversation

@mattcosta7
Copy link
Copy Markdown
Contributor

@mattcosta7 mattcosta7 commented May 21, 2026

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 / .tsx file under packages/react/src/, including experimental/, deprecated/, next/, internal/, live-region/, and utils/.

Changelog

Changed

1. useFocus — eliminated two-renders-per-call cascade

File: packages/react/src/internal/hooks/useFocus.ts (internal, not publicly exported)

Before: the hook stored the focus target in useState. Calling focus(target) caused:

  1. setFocusTarget(target) → render
  2. Effect ran, called .focus(), then setFocusTarget(null) → another render
  3. Effect ran again with target = null and early-returned

That's two renders for one focus() call.

After: target is held in a useRef, and the effect is gated by a monotonic version counter. Each focus() call now produces exactly one render. The returned function is wrapped in useCallback so its identity is stable.

Test: useFocus.test.tsx contains three assertions, each of which fails against the unchanged code and passes against the fix:

Assertion Old New
counter.updateCount === 1 after one focus() call 2 1
Host function-body executions after one focus() call 3 2
Target element receives focus
2. PageLayout.Pane — eliminated useLayoutEffect → setState cascade on mount

Files: packages/react/src/PageLayout/usePaneWidth.ts, packages/react/src/PageLayout/PageLayout.test.tsx

Before: the resize layout effect called setMaxPaneWidth(initialMax) unconditionally on mount. Initial state was customMaxWidth ?? SSR_DEFAULT_MAX_WIDTH (typically 600); for any viewport-derived case the computed value differed from initial state, forcing a synchronous re-render before paint — a Profiler nested-update.

After: setMaxPaneWidth(initialMax) is wrapped in startTransition, 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 by syncAll elsewhere in the same file for the resize-path setStates.

Test: PageLayout.test.tsx asserts counter.nestedUpdateCount === 0 after mounting a resizable PageLayout.Pane. The original code would produce ≥1 nested update on mount; the new code produces 0.

3. AnchoredOverlay — eliminated closed-overlay scroll/resize cascade

Files: packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx, packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx

Before: AnchoredOverlay passed enabled: !cssAnchorPositioning to useAnchoredPosition, gating only on the CSS feature — meaning useAnchoredPosition's scroll listeners and ResizeObserver stayed attached even when the overlay was closed. After an open→close cycle, the first window resize or ancestor scroll fired updatePosition, which hit the "refs not attached" else branch and called setPosition(undefined). Because position was 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 the enabled flag already had). No work, no setState, no re-render.

The useAnchoredPosition hook itself is unchanged. Its public test that asserts onPositionChange is called with undefined when the overlay is closed-on-mount continues to pass, because that test instantiates the hook directly with always-null refs and enabled: true (the default).

AnchoredOverlay's own forwarding of onPositionChange already filters out undefined (if (onPositionChange && position)), so no consumer of AnchoredOverlay's public API can observe the behavior change.

Test: AnchoredOverlay.test.tsx opens then closes the overlay, dispatches a scroll event on window, flushes rAF, and asserts counter.updateCount === 0. The original code would produce 1 update; the new code produces 0.

Removed

Nothing public. Internally, the unused exported createRenderTracker was inlined into its one consumer (the third useFocus test) and removed from the harness.

Rollout strategy

  • Patch release
  • Minor release
  • Major release
  • None

No public API surface changes. useFocus is an internal hook. AnchoredOverlay's public onPositionChange callback already filtered undefined before forwarding to consumers, so the slight semantic change in when useAnchoredPosition fires onPositionChange(undefined) is unobservable through the public API. PageLayout.Pane's aria-valuemax is 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:

  1. Check out main, revert any one of the three source files, and run the corresponding test — it should fail.
  2. Restore the source file, run the test — it should pass.

The new test harness at packages/react/src/utils/testing/profiler.tsx is 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 redundant useEffect in useRenderForcingRef, 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 in contributor-docs/cascade-audit.md enumerates them.

Out of scope (recommended follow-ups, listed in the audit doc):

  • Apply the same enabled: open pattern to experimental/SelectPanel2's useAnchoredPosition call.
  • Move usePaneWidth's maxPaneWidth from useState to useSyncExternalStore so mount produces 1 render instead of 2 (current fix moves the second render off the critical path; this would eliminate it).
  • Lock render counts on the JS-overflow components (Breadcrumbs, LabelGroup, UnderlineNav) so structural cascades there stop regressing further.

Merge checklist

  • Added/updated tests
  • Added/updated documentation (contributor-docs/cascade-audit.md)
  • Added/updated previews (Storybook) — none required, no UI surface changes
  • Changes are SSR compatible (no new DOM reads during render; usePaneWidth SSR snapshot is unchanged)
  • Tested in Chrome (CI)
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge
  • (GitHub staff only) Integration tests pass at github/github-ui

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 21, 2026

🦋 Changeset detected

Latest commit: 3edf601

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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

@github-actions github-actions Bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label May 21, 2026
@github-actions
Copy link
Copy Markdown
Contributor

⚠️ Action required

👋 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. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

To publish a canary release for integration testing, apply the Canary Release label to this PR.

Copy link
Copy Markdown
Contributor

Copilot AI left a 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 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 useFocus to 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 stop AnchoredOverlay from 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

Comment thread packages/react/src/PageLayout/PageLayout.test.tsx
Comment thread packages/react/src/utils/testing/profiler.tsx
@mattcosta7 mattcosta7 mentioned this pull request May 21, 2026
13 tasks
@siddharthkp siddharthkp added the Canary Release Apply this label when you want CI to create a canary release of the current PR label May 22, 2026
@primer-integration
Copy link
Copy Markdown

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/21358

@primer-integration
Copy link
Copy Markdown

Integration test results from github/github-ui:

Passed  CI   Passed
Passed  VRT   Passed
Passed  Projects   Passed

All checks passed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Canary Release Apply this label when you want CI to create a canary release of the current PR integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants