Skip to content

Underline nav shared observer coalesced rebuild#8035

Open
mattcosta7 wants to merge 1 commit into
mainfrom
underline-nav-shared-observer-coalesced-rebuild
Open

Underline nav shared observer coalesced rebuild#8035
mattcosta7 wants to merge 1 commit into
mainfrom
underline-nav-shared-observer-coalesced-rebuild

Conversation

@mattcosta7

@mattcosta7 mattcosta7 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Closes #

Follow-up performance work on top of the merged UnderlineNav rewrite (#7506) and ActionBar overflow handling. Replaces the per-item overflow detection (one IntersectionObserver per item plus per-item offsetTop reads) with a single shared, root-scoped IntersectionObserver per component, and coalesces same-frame registry rebuilds into one microtask-scheduled update.

This is internal-only: no public API changes.

Why

  • Removes forced reflow. The previous approach read ref.current.offsetTop inside useSyncExternalStore's snapshot, forcing a synchronous layout per item. Overflow is now derived from IntersectionObserver entries off the render path; snapshots read a cached boolean.
  • Fewer observers. One observer per registry instead of one per item, lazily created and reused.
  • Minimal re-renders. Only items whose overflow state actually flips re-render; structural registry rebuilds are coalesced via queueMicrotask so a resize that wraps several items triggers a single rebuild.

Changelog

Changed

  • UnderlineNav and ActionBar now detect item overflow using a single shared root-scoped IntersectionObserver per component instead of one observer per item.
  • createDescendantRegistry gains an opt-in overflow option exposing useRegisterOverflowObserver, plus microtask-coalesced rebuilds.

Removed

  • Per-item IntersectionObserver + offsetTop overflow checks in UnderlineNav.Item and ActionBar items.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

No behavioral changes are intended. Existing ActionBar, UnderlineNav, and descendant-registry unit tests pass. Verify overflow behavior by resizing the viewport: items wrap into the overflow menu and the "More" affordance appears/disappears as before, and overflowed items remain non-focusable (tabIndex={-1} / aria-hidden).

Merge checklist

@mattcosta7 mattcosta7 self-assigned this Jun 23, 2026
@changeset-bot

changeset-bot Bot commented Jun 23, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d436177

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 requested a deployment to storybook-preview-8035 June 23, 2026 14:55 Abandoned
@github-actions github-actions Bot temporarily deployed to storybook-preview-8035 June 23, 2026 15:06 Inactive
Base automatically changed from underline-nav-full-css-spike to main June 25, 2026 16:49
@mattcosta7 mattcosta7 force-pushed the underline-nav-shared-observer-coalesced-rebuild branch from 13d8541 to 3d4beb8 Compare June 26, 2026 14:18
@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 Jun 26, 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. If this doesn't work, you can also use the original workflow here. 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.

…nObserver for overflow detection

Replace per-item IntersectionObserver + offsetTop reads with one shared, root-scoped observer per registry and microtask-coalesced rebuilds. Eliminates forced reflow and observer churn during resize. No public API changes.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 optimizes overflow detection for UnderlineNav and ActionBar by moving from per-item overflow work (multiple IntersectionObserver instances plus layout reads) to a single shared, root-scoped IntersectionObserver per component, and by coalescing registry rebuilds to reduce redundant work during resize/wrap events.

Changes:

  • Extend createDescendantRegistry with an opt-in “overflow” mode that provides a shared observer (useRegisterOverflowObserver) and microtask-coalesced rebuild scheduling.
  • Update UnderlineNav and ActionBar items to use the shared overflow observer and pass a clipping root ref to the registry provider.
  • Add a patch changeset describing the internal performance improvement.
Show a summary per file
File Description
packages/react/src/utils/descendant-registry.tsx Adds shared overflow observer support and coalesced rebuild scheduling to the descendant registry utility.
packages/react/src/UnderlineNav/UnderlineNavItemsRegistry.ts Enables the registry’s overflow mode for UnderlineNav items.
packages/react/src/UnderlineNav/UnderlineNavItem.tsx Switches item overflow detection to the registry’s shared observer hook.
packages/react/src/UnderlineNav/UnderlineNav.tsx Passes the clipping root ref into the UnderlineNav registry provider.
packages/react/src/ActionBar/ActionBar.tsx Enables overflow mode for ActionBar registry and switches items to shared observer-based overflow detection.
.changeset/underline-nav-shared-overflow-observer.md Adds a patch changeset for the internal performance optimization.

Review details

  • Files reviewed: 6/6 changed files
  • Comments generated: 3
  • Review effort level: Low

Comment on lines +270 to +274
if (!supportsIntersectionObserver()) return null
if (rootRef && rootRef.current === null) return null

const root = rootRef?.current ?? null
if (observerRef.current && observerRootRef.current === root) return observerRef.current
Comment on lines +162 to +170
const scheduleRebuild = useCallback(() => {
if (pendingRebuildRef.current) return
pendingRebuildRef.current = true
queueMicrotask(() => {
pendingRebuildRef.current = false
// Guard against dispatching into an unmounted provider.
if (isMountedRef.current) rebuildRegistry()
})
}, [])
Comment on lines +105 to +116
/**
* Subscribe an element to the registry's shared, root-scoped IntersectionObserver and derive whether it is currently
* overflowing from the observed entry.
*
* This requires the registry to be created with the `overflow` option so a shared observer is configured. When no
* shared observer is configured the hook is inert and always reports `false`.
*
* @param ref Ref to the element whose overflow state should be tracked.
* @param options.disabled When true, skips observer subscription entirely and always reports `false`. Useful for
* items whose overflow is determined by an ancestor (e.g. ActionBar items inside an overflowing group).
*/
function useRegisterOverflowObserver(ref: RefObject<HTMLElement | null>, options?: {disabled?: boolean}) {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

2 participants