From 5894eb887c5c0700a7c1aa14c468fdbef7070314 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 23 Jun 2026 03:22:04 +0200 Subject: [PATCH 1/3] Add shared IntersectionObserver and coalesced rebuilds to descendant registry Reworks `createDescendantRegistry` to support an optional shared IntersectionObserver owned by the Provider (instead of one observer per item) and to coalesce registry rebuilds so multiple registrations in the same tick trigger a single rebuild. Both UnderlineNav and ActionBar opt into these via the new `useRegisterOverflowObserver` hook. --- .../underline-nav-shared-overflow-observer.md | 5 + .../src/UnderlineNav/UnderlineNavItem.tsx | 24 +-- .../UnderlineNav/UnderlineNavItemsRegistry.ts | 11 +- .../react/src/utils/descendant-registry.tsx | 177 +++++++++++++++++- 4 files changed, 190 insertions(+), 27 deletions(-) create mode 100644 .changeset/underline-nav-shared-overflow-observer.md diff --git a/.changeset/underline-nav-shared-overflow-observer.md b/.changeset/underline-nav-shared-overflow-observer.md new file mode 100644 index 00000000000..087b9fc0a1c --- /dev/null +++ b/.changeset/underline-nav-shared-overflow-observer.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Internal: `UnderlineNav` and `ActionBar` overflow detection now shares a single `IntersectionObserver` per component (owned by the descendant registry) instead of creating one observer per item, and the registry coalesces rebuilds so multiple items crossing the overflow boundary in the same frame trigger a single rebuild. This reduces re-render churn and observer allocations during resize. No public API changes. diff --git a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx index e25083c9304..e2104a0233e 100644 --- a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx @@ -1,4 +1,4 @@ -import React, {forwardRef, useRef, useContext, useCallback, useSyncExternalStore} from 'react' +import React, {forwardRef, useRef, useContext} from 'react' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {UnderlineNavContext} from './UnderlineNavContext' import {UnderlineItem} from '../internal/components/UnderlineTabbedInterface' @@ -22,24 +22,10 @@ export const UnderlineNavItem = forwardRef((allProps, forwardedRef) => { const {loadingCounters} = useContext(UnderlineNavContext) - const isOverflowing = useSyncExternalStore( - useCallback( - onChange => { - const observer = new IntersectionObserver(() => onChange(), { - threshold: 1, - }) - if (ref.current) observer.observe(ref.current) - return () => observer.disconnect() - }, - [ref], - ), - // Note: the IntersectionObserver is just being used as a trigger to re-check - // `offsetTop > 0`; this is fast and simpler than checking visibility from - // the observed entry. When an item wraps, it will move to the next row which - // increases its `offsetTop` - () => (ref.current ? ref.current.offsetTop > 0 : false), - () => false, - ) + // Subscribe to the registry's shared IntersectionObserver. The observer is just being used as a trigger to + // re-check `offsetTop > 0`; this is fast and simpler than checking visibility from the observed entry. When an + // item wraps, it moves to the next (clipped) row, which increases its `offsetTop`. + const isOverflowing = UnderlineNavItemsRegistry.useRegisterOverflowObserver(ref) UnderlineNavItemsRegistry.useRegisterDescendant(isOverflowing ? allProps : null) diff --git a/packages/react/src/UnderlineNav/UnderlineNavItemsRegistry.ts b/packages/react/src/UnderlineNav/UnderlineNavItemsRegistry.ts index 5de98d0b5fb..75a3b525863 100644 --- a/packages/react/src/UnderlineNav/UnderlineNavItemsRegistry.ts +++ b/packages/react/src/UnderlineNav/UnderlineNavItemsRegistry.ts @@ -53,5 +53,12 @@ export type UnderlineNavItemProps = { counter?: number | string } & LinkProps -/** Registry of currently-overflowing underline items. If an item is not overflowing, its value will be `null`. */ -export const UnderlineNavItemsRegistry = createDescendantRegistry() +/** + * Registry of currently-overflowing underline items. If an item is not overflowing, its value will be `null`. + * + * Items opt into a single shared IntersectionObserver (threshold 1) via `useRegisterOverflowObserver` instead of + * each item creating its own observer. + */ +export const UnderlineNavItemsRegistry = createDescendantRegistry({ + overflow: {threshold: 1}, +}) diff --git a/packages/react/src/utils/descendant-registry.tsx b/packages/react/src/utils/descendant-registry.tsx index d2e6359a490..9aa862a17e4 100644 --- a/packages/react/src/utils/descendant-registry.tsx +++ b/packages/react/src/utils/descendant-registry.tsx @@ -8,8 +8,10 @@ import { useReducer, useRef, useState, + useSyncExternalStore, type Dispatch, type ReactNode, + type RefObject, } from 'react' import useIsomorphicLayoutEffect from './useIsomorphicLayoutEffect' @@ -25,6 +27,16 @@ interface DescendantRegistryContext { key: number } +/** Subscribe a single observed element to the shared IntersectionObserver. Returns an unsubscribe function. */ +type ObserveFn = (element: Element, onChange: () => void) => () => void + +interface OverflowObserverContext { + /** Subscribe an element. `null` when no shared observer is configured for this registry. */ + observe: ObserveFn | null +} + +const noopObserve: OverflowObserverContext = {observe: null} + /** * Create a "descendant registry" for a component. This allows a parent to store and track an ordered registry of * child components, even if they are deeply nested in the tree. For example, a menu component can use this to track @@ -40,18 +52,40 @@ interface DescendantRegistryContext { * 4. Access the registered data using the value from `useRegistryState`. This will be a map of `string` to `T`, where * the string key is a unique and stable identifier for each component which can be used as a `key` if necessary. * + * For overflow-style components (e.g. UnderlineNav, ActionBar), use the optional `overflow` option to opt every + * descendant into a *single* shared `IntersectionObserver` (instead of one observer per item) via + * `useRegisterOverflowObserver`. This reduces observer allocations and batches resize-driven callbacks. + * * @note Note that this pattern is not SSR compatible. It won't raise errors or hydration mismatches, but the * registry will not be available during SSR. The registry is built during the effect phase, so it will be populated * after hydration on the client. The initial `undefined` value can be used to safely show loading UI during SSR/initial * render if necessary. */ -export function createDescendantRegistry() { +export function createDescendantRegistry(options?: { + /** + * Configure a shared IntersectionObserver owned by the `Provider`. When set, descendants can call + * `useRegisterOverflowObserver` to subscribe to a single observer rather than each creating their own. + */ + overflow?: { + /** + * IntersectionObserver threshold. The observer is only used as a *trigger* to re-check `offsetTop > 0`, + * so the exact value mainly affects reliability at the wrap boundary. + * @default 1 + */ + threshold?: number + } +}) { const Context = createContext>({ register: () => () => {}, updateValue: () => {}, key: -1, }) + const ObserverContext = createContext(noopObserve) + + const overflowThreshold = options?.overflow?.threshold ?? 1 + const overflowEnabled = options?.overflow !== undefined + /** * Instantiate descendant registry state. The initial value will be `undefined`, indicating that the registry hasn't * been built yet. @@ -74,6 +108,46 @@ export function createDescendantRegistry() { return id } + /** + * Subscribe an element to the registry's shared IntersectionObserver and derive whether it is currently overflowing + * (i.e. has wrapped to a clipped row, so `offsetTop > 0`). Falls back to a per-item observer if no shared observer is + * configured for this registry, so the hook is always safe to call. + * + * The IntersectionObserver is only used as a cheap *trigger* to re-check `offsetTop`; the actual overflow detection + * relies on `flex-wrap` + `overflow: hidden` pushing items to a clipped second row, not on viewport intersection. + * That's why there is no `root` option on the observer. + * + * @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, options?: {disabled?: boolean}) { + const disabled = options?.disabled ?? false + const {observe} = useContext(ObserverContext) + + const subscribe = useCallback( + (onChange: () => void) => { + if (disabled) return () => {} + const element = ref.current + if (!element) return () => {} + + // Prefer the provider's shared observer; fall back to a local observer when none is configured. + if (observe) return observe(element, onChange) + + const observer = new IntersectionObserver(() => onChange(), {threshold: overflowThreshold}) + observer.observe(element) + return () => observer.disconnect() + }, + [ref, observe, disabled], + ) + + return useSyncExternalStore( + subscribe, + () => (!disabled && ref.current ? ref.current.offsetTop > 0 : false), + () => false, + ) + } + const unsetValue = Symbol('unset') /** Provide context for registering descendant components. This only needs to wrap `children`. */ @@ -83,6 +157,28 @@ export function createDescendantRegistry() { /** State value to trigger a re-render and force all descendants to re-register. This ensures everything remains ordered. */ const [key, rebuildRegistry] = useReducer(prev => prev + 1, 0) + // Coalesce rebuilds: when several descendants register in the same tick (e.g. multiple items crossing the wrap + // boundary during one resize frame), schedule a single rebuild via microtask instead of one rebuild per + // registration. The microtask drains before paint, so the rebuild still lands within the same frame. + const pendingRebuildRef = useRef(false) + const isMountedRef = useRef(true) + 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() + }) + }, []) + + useEffect(() => { + isMountedRef.current = true + return () => { + isMountedRef.current = false + } + }, []) + // If a rebuild is queued, instantiate a new map. Must be in a layout effect to run before all descendants' effects run to populate it useIsomorphicLayoutEffect(function instantiateNewRegistry() { if (workingRegistryRef.current === 'queued') { @@ -101,9 +197,10 @@ export function createDescendantRegistry() { workingRegistryRef.current.set(id, unsetValue) } else if (workingRegistryRef.current === 'idle') { // When idle, registering a new component causes the whole registry to be rebuilt (because that item could - // be inserted anywhere in the tree, changing the order of items) + // be inserted anywhere in the tree, changing the order of items). Coalesce concurrent registrations so the + // rebuild only happens once per tick. workingRegistryRef.current = 'queued' - rebuildRegistry() + scheduleRebuild() } // Noop if status is `queued` since we will restart the map in the next cycle @@ -121,7 +218,7 @@ export function createDescendantRegistry() { } } }, - [setRegistry], + [setRegistry, scheduleRebuild], ) /** Update a descendant's value in the registry. */ @@ -153,8 +250,76 @@ export function createDescendantRegistry() { const contextValue = useMemo(() => ({register, updateValue, key}), [register, updateValue, key]) - return {children} + return ( + + {overflowEnabled ? {children} : children} + + ) + } + + /** + * Owns a single IntersectionObserver shared by every descendant that calls `useRegisterOverflowObserver`. + * Each observed element maps to a set of change callbacks; one observer notification fans out to all of them. + */ + function OverflowObserverProvider({children}: {children: ReactNode}) { + // Map of observed element -> set of subscriber callbacks. + const subscribersRef = useRef void>>>(new Map()) + const observerRef = useRef(null) + + // Lazily create the observer on first subscribe so SSR / zero-item renders allocate nothing. + const getObserver = useCallback(() => { + if (observerRef.current) return observerRef.current + if (typeof IntersectionObserver === 'undefined') return null + observerRef.current = new IntersectionObserver( + entries => { + for (const entry of entries) { + const callbacks = subscribersRef.current.get(entry.target) + if (!callbacks) continue + for (const cb of callbacks) cb() + } + }, + {threshold: overflowThreshold}, + ) + return observerRef.current + }, []) + + const observe = useCallback( + (element, onChange) => { + const observer = getObserver() + let callbacks = subscribersRef.current.get(element) + if (!callbacks) { + callbacks = new Set() + subscribersRef.current.set(element, callbacks) + observer?.observe(element) + } + callbacks.add(onChange) + + return () => { + const set = subscribersRef.current.get(element) + if (!set) return + set.delete(onChange) + if (set.size === 0) { + subscribersRef.current.delete(element) + observer?.unobserve(element) + } + } + }, + [getObserver], + ) + + useEffect(() => { + const subscribers = subscribersRef.current + return () => { + observerRef.current?.disconnect() + observerRef.current = null + subscribers.clear() + } + }, []) + + const value = useMemo(() => ({observe}), [observe]) + + return {children} } - return {Provider, useRegistryState, useRegisterDescendant} + return {Provider, useRegistryState, useRegisterDescendant, useRegisterOverflowObserver} } From 75c8b500199b7a2c4faa7f46664a47b0f29b2748 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 23 Jun 2026 03:25:00 +0200 Subject: [PATCH 2/3] Adopt shared overflow observer in ActionBar and add registry tests - ActionBar opts into the shared IntersectionObserver via `useRegisterOverflowObserver` (threshold 0.75), with grouped items using the `disabled` option to skip subscription, matching the previous early-return behavior. - Add unit tests asserting coalesced rebuilds (N same-tick registrations -> single rebuild) and shared-observer fan-out (one observer, one callback updating all items), plus observer unobserve/disconnect cleanup. --- packages/react/src/ActionBar/ActionBar.tsx | 42 +-- .../__tests__/descendant-registry.test.tsx | 297 +++++++++++++++++- 2 files changed, 307 insertions(+), 32 deletions(-) diff --git a/packages/react/src/ActionBar/ActionBar.tsx b/packages/react/src/ActionBar/ActionBar.tsx index 8fd85484606..f61dfdd5b03 100644 --- a/packages/react/src/ActionBar/ActionBar.tsx +++ b/packages/react/src/ActionBar/ActionBar.tsx @@ -1,5 +1,5 @@ import {type RefObject, type MouseEventHandler, useContext} from 'react' -import React, {useState, useCallback, useRef, forwardRef, useMemo, useSyncExternalStore} from 'react' +import React, {useState, useCallback, useRef, forwardRef, useMemo} from 'react' import {KebabHorizontalIcon} from '@primer/octicons-react' import {ActionList, type ActionListItemProps} from '../ActionList' @@ -139,7 +139,11 @@ export type ActionBarMenuProps = { returnFocusRef?: React.RefObject } & IconButtonProps -const ActionBarItemsRegistry = createDescendantRegistry() +// Items opt into a single shared IntersectionObserver (threshold 0.75) via `useRegisterOverflowObserver` instead of +// each item creating its own observer. 0.75 is used (rather than 1) because in some scenarios a threshold of 1 doesn't +// trigger correctly - the browser still thinks a tiny bit of the button is not visible since the container height is +// exactly the button height. See `useActionBarItem`. +const ActionBarItemsRegistry = createDescendantRegistry({overflow: {threshold: 0.75}}) const FOCUSABLE_ITEM_SELECTOR = ':is(button, a, input, [tabindex]):not(:disabled):not([data-overflowing]):not([data-more-button-inactive])' @@ -310,33 +314,13 @@ function useActionBarItem(ref: React.RefObject, registryProp const isGroupOverflowing = useContext(ActionBarGroupContext)?.isOverflowing const isInGroup = isGroupOverflowing !== undefined - const subscribeIntersectionObserver = useCallback( - (onChange: () => void) => { - // There's no need to register observers on items inside of a group - // since the entire group overflows at once - if (isInGroup) return () => {} - - // Technically 1 should work as the threshold, but in some scenarios that - // doesn't seem to trigger correctly - probably because the browser still - // thinks a tiny bit of the button is not visible, since the container - // height is exactly the button height. So 75% should be more reliable. - const observer = new IntersectionObserver(() => onChange(), {threshold: 0.75}) - - if (ref.current) observer.observe(ref.current) - return () => observer.disconnect() - }, - [ref, isInGroup], - ) - - const isItemOverflowing = useSyncExternalStore( - subscribeIntersectionObserver, - // Note: the IntersectionObserver is just being used as a trigger to re-check - // `offsetTop > 0`; this is fast and simpler than checking visibility from - // the observed entry. When an item wraps, it will move to the next row which - // increases its `offsetTop` - () => (ref.current ? ref.current.offsetTop > 0 : false), - () => false, - ) + // Subscribe to the registry's shared IntersectionObserver to detect overflow. The observer is just being used as a + // trigger to re-check `offsetTop > 0`; this is fast and simpler than checking visibility from the observed entry. + // When an item wraps, it moves to the next row which increases its `offsetTop`. + // + // There's no need to observe items inside of a group since the entire group overflows at once, so `disabled` skips + // subscription for grouped items (the snapshot returns `false`, matching the previous early-return behavior). + const isItemOverflowing = ActionBarItemsRegistry.useRegisterOverflowObserver(ref, {disabled: isInGroup}) const isOverflowing = isGroupOverflowing || isItemOverflowing diff --git a/packages/react/src/utils/__tests__/descendant-registry.test.tsx b/packages/react/src/utils/__tests__/descendant-registry.test.tsx index f357b8d2b6e..39fa167a191 100644 --- a/packages/react/src/utils/__tests__/descendant-registry.test.tsx +++ b/packages/react/src/utils/__tests__/descendant-registry.test.tsx @@ -1,7 +1,7 @@ -import {describe, expect, it} from 'vitest' +import {describe, expect, it, vi, beforeEach, afterEach} from 'vitest' import type React from 'react' -import {Fragment, useState} from 'react' -import {render} from '@testing-library/react' +import {Fragment, useRef, useState} from 'react' +import {render, act} from '@testing-library/react' import {createDescendantRegistry} from '../descendant-registry' import {userEvent} from '@testing-library/user-event' @@ -181,3 +181,294 @@ describe('createDescendantRegistry', () => { expect(getByTestId('registry-values').textContent).toBe('first,second,third') }) }) + +describe('createDescendantRegistry coalesced rebuilds', () => { + /** + * Builds a registry that counts how many times the Provider rebuilds (re-runs the rebuild reducer) by counting + * renders of an instrumented child placed inside the Provider. Each rebuild bumps the registry `key`, which forces + * descendants to re-render, so a render counter on a registry descendant is a reliable proxy for rebuild count. + */ + function createCountingRegistry() { + const {Provider, useRegistryState, useRegisterDescendant} = createDescendantRegistry() + + const rebuildSpy = vi.fn() + + function RegistryParent({children}: {children: React.ReactNode}) { + const [registryState, setRegistry] = useRegistryState() + return ( + <> +
{Array.from(registryState?.values() ?? []).join(',')}
+ {children} + + ) + } + + /** Registers itself and pings `rebuildSpy` whenever the registry `key` changes (i.e. on each rebuild). */ + function RebuildProbe() { + useRegisterDescendant('probe') + rebuildSpy() + return null + } + + function Item({value}: {value: string}) { + useRegisterDescendant(value) + return null + } + + return {RegistryParent, Item, RebuildProbe, rebuildSpy} + } + + it('coalesces multiple same-tick registrations into a single rebuild', async () => { + const {RegistryParent, Item, RebuildProbe, rebuildSpy} = createCountingRegistry() + + function Test() { + const [extraItems, setExtraItems] = useState(0) + return ( + + + + {Array.from({length: extraItems}, (_, i) => ( + + ))} + + , + ) + } + + const {getByRole, getByTestId} = render() + + // Let the initial mount settle (initial registration + commit + the idle-state rebuild it schedules). + await act(async () => { + await Promise.resolve() + }) + + const rendersBeforeAdd = rebuildSpy.mock.calls.length + + // Add five items in a single state update. Each newly-registered item, while the registry is idle, would + // historically trigger its own synchronous rebuild (up to 5). With coalescing they collapse into one. + await act(async () => { + await userEvent.click(getByRole('button')) + // Flush the coalescing microtask. + await Promise.resolve() + }) + + expect(getByTestId('registry-values').textContent).toContain('extra-4') + + const rebuildsForAdd = rebuildSpy.mock.calls.length - rendersBeforeAdd + // The five registrations should coalesce: far fewer rebuild-driven renders than the number of items added. + // We assert a tight upper bound (<= 2) to allow for React's own re-render of the changed subtree plus the + // single coalesced rebuild, while still failing if each item triggered its own rebuild (which would be >= 5). + expect(rebuildsForAdd).toBeLessThanOrEqual(2) + }) + + it('still registers every item correctly after a coalesced rebuild', async () => { + const {RegistryParent, Item} = createCountingRegistry() + + function Test() { + const [show, setShow] = useState(false) + return ( + + + {show && ( + <> + + + + + )} + + + , + ) + } + + const {getByRole, getByTestId} = render() + expect(getByTestId('registry-values').textContent).toBe('a,e') + + await act(async () => { + await userEvent.click(getByRole('button')) + await Promise.resolve() + }) + + // Order must be preserved across the coalesced rebuild. + expect(getByTestId('registry-values').textContent).toBe('a,b,c,d,e') + }) +}) + +describe('createDescendantRegistry shared IntersectionObserver', () => { + // Capture every IntersectionObserver instance and its observed elements so we can assert a single shared observer + // is used and drive its callback manually. + type FakeObserver = { + callback: IntersectionObserverCallback + observed: Set + observe: ReturnType + unobserve: ReturnType + disconnect: ReturnType + trigger: () => void + } + let observers: FakeObserver[] = [] + + beforeEach(() => { + observers = [] + class MockIntersectionObserver { + callback: IntersectionObserverCallback + observed = new Set() + observe = vi.fn((el: Element) => { + this.observed.add(el) + }) + unobserve = vi.fn((el: Element) => { + this.observed.delete(el) + }) + disconnect = vi.fn(() => { + this.observed.clear() + }) + constructor(callback: IntersectionObserverCallback) { + this.callback = callback + observers.push(this as unknown as FakeObserver) + ;(this as unknown as FakeObserver).trigger = () => { + const entries = Array.from(this.observed).map( + target => ({target}) as IntersectionObserverEntry, + ) + this.callback(entries, this as unknown as IntersectionObserver) + } + } + takeRecords() { + return [] + } + } + vi.stubGlobal('IntersectionObserver', MockIntersectionObserver) + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + /** + * Registry with the shared overflow observer enabled. Items report their overflow state into the DOM so we can + * assert how many were updated by a single observer callback (the fan-out). + */ + function createOverflowRegistry() { + const {Provider, useRegistryState, useRegisterDescendant, useRegisterOverflowObserver} = + createDescendantRegistry({overflow: {threshold: 1}}) + + function RegistryParent({children}: {children: React.ReactNode}) { + const [, setRegistry] = useRegistryState() + return {children} + } + + function Item({value}: {value: string}) { + const ref = useRef(null) + const isOverflowing = useRegisterOverflowObserver(ref) + useRegisterDescendant(isOverflowing ? value : null) + return
+ } + + return {RegistryParent, Item} + } + + it('creates a single shared observer for all items rather than one per item', () => { + const {RegistryParent, Item} = createOverflowRegistry() + + render( + + + + + , + ) + + // Exactly one observer instance, observing all three elements. + expect(observers).toHaveLength(1) + expect(observers[0].observed.size).toBe(3) + }) + + it('fans out a single observer callback to every subscribed item', () => { + const {RegistryParent, Item} = createOverflowRegistry() + + const {getByTestId} = render( + + + + + , + ) + + // Simulate all items wrapping to a clipped row (offsetTop > 0). jsdom reports 0 for offsetTop, so stub it. + const items = ['a', 'b', 'c'].map(v => getByTestId(`item-${v}`)) + for (const el of items) { + Object.defineProperty(el, 'offsetTop', {configurable: true, value: 10}) + } + + // One observer notification should update all three items via fan-out. + act(() => { + observers[0].trigger() + }) + + for (const el of items) { + expect(el).toHaveAttribute('data-overflowing', 'true') + } + }) + + it('unobserves an element from the shared observer when its item unmounts', async () => { + const {RegistryParent, Item} = createOverflowRegistry() + + function Test() { + const [showC, setShowC] = useState(true) + return ( +
+ + + + {showC && } + + +
+ ) + } + + const {getByRole} = render() + expect(observers).toHaveLength(1) + expect(observers[0].observed.size).toBe(3) + + await userEvent.click(getByRole('button')) + + // Still the same single observer, now observing only the two remaining items. + expect(observers).toHaveLength(1) + expect(observers[0].observed.size).toBe(2) + }) + + it('disconnects the shared observer when the provider unmounts', async () => { + const {RegistryParent, Item} = createOverflowRegistry() + + function Test() { + const [mounted, setMounted] = useState(true) + return ( +
+ {mounted && ( + + + + + )} + +
+ ) + } + + const {getByRole} = render() + expect(observers).toHaveLength(1) + const observer = observers[0] + + await userEvent.click(getByRole('button')) + + expect(observer.disconnect).toHaveBeenCalled() + }) +}) From 13d854154d3fcbebb975af01c2f01819f64a3029 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 23 Jun 2026 10:50:56 -0400 Subject: [PATCH 3/3] Derive overflow from root-scoped IntersectionObserver entries instead of offsetTop reads (#8030) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- .../underline-nav-shared-overflow-observer.md | 2 +- packages/react/src/ActionBar/ActionBar.tsx | 21 ++- .../react/src/UnderlineNav/UnderlineNav.tsx | 2 +- .../src/UnderlineNav/UnderlineNavItem.tsx | 5 +- .../UnderlineNav/UnderlineNavItemsRegistry.ts | 6 +- .../__tests__/descendant-registry.test.tsx | 81 +++++++++--- .../react/src/utils/descendant-registry.tsx | 121 ++++++++++++------ 7 files changed, 160 insertions(+), 78 deletions(-) diff --git a/.changeset/underline-nav-shared-overflow-observer.md b/.changeset/underline-nav-shared-overflow-observer.md index 087b9fc0a1c..da950989c4e 100644 --- a/.changeset/underline-nav-shared-overflow-observer.md +++ b/.changeset/underline-nav-shared-overflow-observer.md @@ -2,4 +2,4 @@ "@primer/react": patch --- -Internal: `UnderlineNav` and `ActionBar` overflow detection now shares a single `IntersectionObserver` per component (owned by the descendant registry) instead of creating one observer per item, and the registry coalesces rebuilds so multiple items crossing the overflow boundary in the same frame trigger a single rebuild. This reduces re-render churn and observer allocations during resize. No public API changes. +Internal: `UnderlineNav` and `ActionBar` overflow detection now uses a shared root-scoped `IntersectionObserver` per component, and the registry coalesces same-frame rebuilds. This reduces observer churn during resize with no public API changes. diff --git a/packages/react/src/ActionBar/ActionBar.tsx b/packages/react/src/ActionBar/ActionBar.tsx index f61dfdd5b03..d0827364ca5 100644 --- a/packages/react/src/ActionBar/ActionBar.tsx +++ b/packages/react/src/ActionBar/ActionBar.tsx @@ -139,11 +139,9 @@ export type ActionBarMenuProps = { returnFocusRef?: React.RefObject } & IconButtonProps -// Items opt into a single shared IntersectionObserver (threshold 0.75) via `useRegisterOverflowObserver` instead of -// each item creating its own observer. 0.75 is used (rather than 1) because in some scenarios a threshold of 1 doesn't -// trigger correctly - the browser still thinks a tiny bit of the button is not visible since the container height is -// exactly the button height. See `useActionBarItem`. -const ActionBarItemsRegistry = createDescendantRegistry({overflow: {threshold: 0.75}}) +// Items opt into a single shared IntersectionObserver via `useRegisterOverflowObserver` instead of each item creating +// its own observer. +const ActionBarItemsRegistry = createDescendantRegistry({overflow: {}}) const FOCUSABLE_ITEM_SELECTOR = ':is(button, a, input, [tabindex]):not(:disabled):not([data-overflowing]):not([data-more-button-inactive])' @@ -207,6 +205,7 @@ export const ActionBar: React.FC> = ({ gap = 'condensed', }) => { const [childRegistry, setChildRegistry] = ActionBarItemsRegistry.useRegistryState() + const overflowContainerRef = useRef(null) const overflowItems = useMemo( () => @@ -237,10 +236,12 @@ export const ActionBar: React.FC> = ({ data-size={size} data-has-overflow={overflowItems ? overflowItems.length > 0 : undefined} > -
+
{/* An empty first element allows the real first item to wrap to the next line and get clipped. */}
- {children} + + {children} +
@@ -314,12 +315,8 @@ function useActionBarItem(ref: React.RefObject, registryProp const isGroupOverflowing = useContext(ActionBarGroupContext)?.isOverflowing const isInGroup = isGroupOverflowing !== undefined - // Subscribe to the registry's shared IntersectionObserver to detect overflow. The observer is just being used as a - // trigger to re-check `offsetTop > 0`; this is fast and simpler than checking visibility from the observed entry. - // When an item wraps, it moves to the next row which increases its `offsetTop`. - // // There's no need to observe items inside of a group since the entire group overflows at once, so `disabled` skips - // subscription for grouped items (the snapshot returns `false`, matching the previous early-return behavior). + // subscription for grouped items and always reports `false` for the child item itself. const isItemOverflowing = ActionBarItemsRegistry.useRegisterOverflowObserver(ref, {disabled: isInGroup}) const isOverflowing = isGroupOverflowing || isItemOverflowing diff --git a/packages/react/src/UnderlineNav/UnderlineNav.tsx b/packages/react/src/UnderlineNav/UnderlineNav.tsx index ca085e71689..bf95c79284e 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.tsx @@ -101,7 +101,7 @@ export const UnderlineNav = forwardRef( data-has-overflow={isOverflowing ? 'true' : undefined} > - + {children} diff --git a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx index e2104a0233e..3acf653a25e 100644 --- a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx @@ -22,9 +22,8 @@ export const UnderlineNavItem = forwardRef((allProps, forwardedRef) => { const {loadingCounters} = useContext(UnderlineNavContext) - // Subscribe to the registry's shared IntersectionObserver. The observer is just being used as a trigger to - // re-check `offsetTop > 0`; this is fast and simpler than checking visibility from the observed entry. When an - // item wraps, it moves to the next (clipped) row, which increases its `offsetTop`. + // Observe the wrapping `
  • ` directly so a root-scoped IntersectionObserver can detect when the item is clipped + // onto the hidden next row. const isOverflowing = UnderlineNavItemsRegistry.useRegisterOverflowObserver(ref) UnderlineNavItemsRegistry.useRegisterDescendant(isOverflowing ? allProps : null) diff --git a/packages/react/src/UnderlineNav/UnderlineNavItemsRegistry.ts b/packages/react/src/UnderlineNav/UnderlineNavItemsRegistry.ts index 75a3b525863..f85bcd608c6 100644 --- a/packages/react/src/UnderlineNav/UnderlineNavItemsRegistry.ts +++ b/packages/react/src/UnderlineNav/UnderlineNavItemsRegistry.ts @@ -56,9 +56,9 @@ export type UnderlineNavItemProps = { /** * Registry of currently-overflowing underline items. If an item is not overflowing, its value will be `null`. * - * Items opt into a single shared IntersectionObserver (threshold 1) via `useRegisterOverflowObserver` instead of - * each item creating its own observer. + * Items opt into a single shared IntersectionObserver via `useRegisterOverflowObserver` instead of each item creating + * its own observer. */ export const UnderlineNavItemsRegistry = createDescendantRegistry({ - overflow: {threshold: 1}, + overflow: {}, }) diff --git a/packages/react/src/utils/__tests__/descendant-registry.test.tsx b/packages/react/src/utils/__tests__/descendant-registry.test.tsx index 39fa167a191..227336e6dd8 100644 --- a/packages/react/src/utils/__tests__/descendant-registry.test.tsx +++ b/packages/react/src/utils/__tests__/descendant-registry.test.tsx @@ -233,7 +233,7 @@ describe('createDescendantRegistry coalesced rebuilds', () => { - , + ) } @@ -282,7 +282,7 @@ describe('createDescendantRegistry coalesced rebuilds', () => { - , + ) } @@ -302,13 +302,16 @@ describe('createDescendantRegistry coalesced rebuilds', () => { describe('createDescendantRegistry shared IntersectionObserver', () => { // Capture every IntersectionObserver instance and its observed elements so we can assert a single shared observer // is used and drive its callback manually. + /** Minimal `IntersectionObserverEntry` shape needed to drive overflow updates in tests. */ + type MockEntry = Pick type FakeObserver = { callback: IntersectionObserverCallback + options?: IntersectionObserverInit observed: Set observe: ReturnType unobserve: ReturnType disconnect: ReturnType - trigger: () => void + emit: (entries: MockEntry[]) => void } let observers: FakeObserver[] = [] @@ -316,6 +319,7 @@ describe('createDescendantRegistry shared IntersectionObserver', () => { observers = [] class MockIntersectionObserver { callback: IntersectionObserverCallback + options?: IntersectionObserverInit observed = new Set() observe = vi.fn((el: Element) => { this.observed.add(el) @@ -326,14 +330,15 @@ describe('createDescendantRegistry shared IntersectionObserver', () => { disconnect = vi.fn(() => { this.observed.clear() }) - constructor(callback: IntersectionObserverCallback) { + constructor(callback: IntersectionObserverCallback, options?: IntersectionObserverInit) { this.callback = callback + this.options = options observers.push(this as unknown as FakeObserver) - ;(this as unknown as FakeObserver).trigger = () => { - const entries = Array.from(this.observed).map( - target => ({target}) as IntersectionObserverEntry, + ;(this as unknown as FakeObserver).emit = entries => { + this.callback( + entries.map(entry => entry as IntersectionObserverEntry), + this as unknown as IntersectionObserver, ) - this.callback(entries, this as unknown as IntersectionObserver) } } takeRecords() { @@ -352,12 +357,20 @@ describe('createDescendantRegistry shared IntersectionObserver', () => { * assert how many were updated by a single observer callback (the fan-out). */ function createOverflowRegistry() { - const {Provider, useRegistryState, useRegisterDescendant, useRegisterOverflowObserver} = - createDescendantRegistry({overflow: {threshold: 1}}) + const {Provider, useRegistryState, useRegisterDescendant, useRegisterOverflowObserver} = createDescendantRegistry< + string | null + >({overflow: {}}) function RegistryParent({children}: {children: React.ReactNode}) { const [, setRegistry] = useRegistryState() - return {children} + const rootRef = useRef(null) + return ( +
    + + {children} + +
    + ) } function Item({value}: {value: string}) { @@ -370,10 +383,10 @@ describe('createDescendantRegistry shared IntersectionObserver', () => { return {RegistryParent, Item} } - it('creates a single shared observer for all items rather than one per item', () => { + it('creates a single shared observer for all items with the provided root element', () => { const {RegistryParent, Item} = createOverflowRegistry() - render( + const {getByTestId} = render( @@ -383,6 +396,7 @@ describe('createDescendantRegistry shared IntersectionObserver', () => { // Exactly one observer instance, observing all three elements. expect(observers).toHaveLength(1) + expect(observers[0].options?.root).toBe(getByTestId('observer-root')) expect(observers[0].observed.size).toBe(3) }) @@ -397,15 +411,17 @@ describe('createDescendantRegistry shared IntersectionObserver', () => { , ) - // Simulate all items wrapping to a clipped row (offsetTop > 0). jsdom reports 0 for offsetTop, so stub it. const items = ['a', 'b', 'c'].map(v => getByTestId(`item-${v}`)) - for (const el of items) { - Object.defineProperty(el, 'offsetTop', {configurable: true, value: 10}) - } // One observer notification should update all three items via fan-out. act(() => { - observers[0].trigger() + observers[0].emit( + items.map(target => ({ + target, + isIntersecting: false, + intersectionRatio: 0, + })), + ) }) for (const el of items) { @@ -413,6 +429,34 @@ describe('createDescendantRegistry shared IntersectionObserver', () => { } }) + it('updates an item from the observed entry without reading layout', () => { + const {RegistryParent, Item} = createOverflowRegistry() + + const {getByTestId} = render( + + + , + ) + + const item = getByTestId('item-a') + Object.defineProperty(item, 'offsetTop', { + configurable: true, + get() { + throw new Error('offsetTop should not be read') + }, + }) + + act(() => { + observers[0].emit([{target: item, isIntersecting: false, intersectionRatio: 0}]) + }) + expect(item).toHaveAttribute('data-overflowing', 'true') + + act(() => { + observers[0].emit([{target: item, isIntersecting: true, intersectionRatio: 1}]) + }) + expect(item).toHaveAttribute('data-overflowing', 'false') + }) + it('unobserves an element from the shared observer when its item unmounts', async () => { const {RegistryParent, Item} = createOverflowRegistry() @@ -441,6 +485,7 @@ describe('createDescendantRegistry shared IntersectionObserver', () => { // Still the same single observer, now observing only the two remaining items. expect(observers).toHaveLength(1) expect(observers[0].observed.size).toBe(2) + expect(observers[0].unobserve).toHaveBeenCalled() }) it('disconnects the shared observer when the provider unmounts', async () => { diff --git a/packages/react/src/utils/descendant-registry.tsx b/packages/react/src/utils/descendant-registry.tsx index 9aa862a17e4..2b2b86ab81d 100644 --- a/packages/react/src/utils/descendant-registry.tsx +++ b/packages/react/src/utils/descendant-registry.tsx @@ -19,6 +19,8 @@ export interface ProviderProps { children: ReactNode /** State setter from `useRegistryState`. */ setRegistry: Dispatch | undefined>> + /** Clipping container used as the `IntersectionObserver` root for overflow detection. */ + rootRef?: RefObject } interface DescendantRegistryContext { @@ -28,7 +30,7 @@ interface DescendantRegistryContext { } /** Subscribe a single observed element to the shared IntersectionObserver. Returns an unsubscribe function. */ -type ObserveFn = (element: Element, onChange: () => void) => () => void +type ObserveFn = (element: Element, onOverflowChange: (isOverflowing: boolean) => void) => () => void interface OverflowObserverContext { /** Subscribe an element. `null` when no shared observer is configured for this registry. */ @@ -66,14 +68,7 @@ export function createDescendantRegistry(options?: { * Configure a shared IntersectionObserver owned by the `Provider`. When set, descendants can call * `useRegisterOverflowObserver` to subscribe to a single observer rather than each creating their own. */ - overflow?: { - /** - * IntersectionObserver threshold. The observer is only used as a *trigger* to re-check `offsetTop > 0`, - * so the exact value mainly affects reliability at the wrap boundary. - * @default 1 - */ - threshold?: number - } + overflow?: object }) { const Context = createContext>({ register: () => () => {}, @@ -83,7 +78,6 @@ export function createDescendantRegistry(options?: { const ObserverContext = createContext(noopObserve) - const overflowThreshold = options?.overflow?.threshold ?? 1 const overflowEnabled = options?.overflow !== undefined /** @@ -109,13 +103,11 @@ export function createDescendantRegistry(options?: { } /** - * Subscribe an element to the registry's shared IntersectionObserver and derive whether it is currently overflowing - * (i.e. has wrapped to a clipped row, so `offsetTop > 0`). Falls back to a per-item observer if no shared observer is - * configured for this registry, so the hook is always safe to call. + * Subscribe an element to the registry's shared, root-scoped IntersectionObserver and derive whether it is currently + * overflowing from the observed entry. * - * The IntersectionObserver is only used as a cheap *trigger* to re-check `offsetTop`; the actual overflow detection - * relies on `flex-wrap` + `overflow: hidden` pushing items to a clipped second row, not on viewport intersection. - * That's why there is no `root` option on the observer. + * 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 @@ -124,26 +116,31 @@ export function createDescendantRegistry(options?: { function useRegisterOverflowObserver(ref: RefObject, options?: {disabled?: boolean}) { const disabled = options?.disabled ?? false const {observe} = useContext(ObserverContext) + const isOverflowingRef = useRef(false) const subscribe = useCallback( (onChange: () => void) => { if (disabled) return () => {} const element = ref.current - if (!element) return () => {} - - // Prefer the provider's shared observer; fall back to a local observer when none is configured. - if (observe) return observe(element, onChange) + // The hook only tracks overflow through the provider's shared observer. When no shared observer is configured + // (or the element isn't attached yet) the hook is inert and reports `false`. + if (!element || observe === null) return () => {} + + const updateOverflowState = (isOverflowing: boolean) => { + if (isOverflowing !== isOverflowingRef.current) { + isOverflowingRef.current = isOverflowing + onChange() + } + } - const observer = new IntersectionObserver(() => onChange(), {threshold: overflowThreshold}) - observer.observe(element) - return () => observer.disconnect() + return observe(element, updateOverflowState) }, [ref, observe, disabled], ) return useSyncExternalStore( subscribe, - () => (!disabled && ref.current ? ref.current.offsetTop > 0 : false), + () => (disabled ? false : isOverflowingRef.current), () => false, ) } @@ -151,7 +148,7 @@ export function createDescendantRegistry(options?: { const unsetValue = Symbol('unset') /** Provide context for registering descendant components. This only needs to wrap `children`. */ - function Provider({children, setRegistry}: ProviderProps) { + function Provider({children, setRegistry, rootRef}: ProviderProps) { const workingRegistryRef = useRef | 'queued' | 'idle'>('queued') /** State value to trigger a re-render and force all descendants to re-register. This ensures everything remains ordered. */ @@ -252,7 +249,7 @@ export function createDescendantRegistry(options?: { return ( - {overflowEnabled ? {children} : children} + {overflowEnabled ? {children} : children} ) } @@ -261,57 +258,88 @@ export function createDescendantRegistry(options?: { * Owns a single IntersectionObserver shared by every descendant that calls `useRegisterOverflowObserver`. * Each observed element maps to a set of change callbacks; one observer notification fans out to all of them. */ - function OverflowObserverProvider({children}: {children: ReactNode}) { + function OverflowObserverProvider({children, rootRef}: {children: ReactNode; rootRef?: RefObject}) { // Map of observed element -> set of subscriber callbacks. - const subscribersRef = useRef void>>>(new Map()) + const subscribersRef = useRef void>>>(new Map()) + const observedElementsRef = useRef>(new Set()) const observerRef = useRef(null) + const observerRootRef = useRef(null) // Lazily create the observer on first subscribe so SSR / zero-item renders allocate nothing. const getObserver = useCallback(() => { - if (observerRef.current) return observerRef.current - if (typeof IntersectionObserver === 'undefined') return null + 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 + + observerRef.current?.disconnect() + observedElementsRef.current.clear() + observerRef.current = new IntersectionObserver( entries => { for (const entry of entries) { const callbacks = subscribersRef.current.get(entry.target) if (!callbacks) continue - for (const cb of callbacks) cb() + const isOverflowing = getIsOverflowing(entry) + for (const cb of callbacks) cb(isOverflowing) } }, - {threshold: overflowThreshold}, + {root, threshold: [0, 1]}, ) + observerRootRef.current = root return observerRef.current - }, []) + }, [rootRef]) + + const observeSubscribedElements = useCallback(() => { + const observer = getObserver() + if (!observer) return + + // When the root ref becomes available or changes, re-check every subscribed element so they are all attached to + // the latest shared observer instance. + for (const element of subscribersRef.current.keys()) { + if (!observedElementsRef.current.has(element)) { + observer.observe(element) + observedElementsRef.current.add(element) + } + } + }, [getObserver]) const observe = useCallback( - (element, onChange) => { - const observer = getObserver() + (element, onOverflowChange) => { let callbacks = subscribersRef.current.get(element) if (!callbacks) { callbacks = new Set() subscribersRef.current.set(element, callbacks) - observer?.observe(element) } - callbacks.add(onChange) + callbacks.add(onOverflowChange) + observeSubscribedElements() return () => { const set = subscribersRef.current.get(element) if (!set) return - set.delete(onChange) + set.delete(onOverflowChange) if (set.size === 0) { subscribersRef.current.delete(element) - observer?.unobserve(element) + observedElementsRef.current.delete(element) + observerRef.current?.unobserve(element) } } }, - [getObserver], + [observeSubscribedElements], ) + useIsomorphicLayoutEffect(() => { + observeSubscribedElements() + }) + useEffect(() => { const subscribers = subscribersRef.current + const observedElements = observedElementsRef.current return () => { observerRef.current?.disconnect() observerRef.current = null + observedElements.clear() subscribers.clear() } }, []) @@ -323,3 +351,16 @@ export function createDescendantRegistry(options?: { return {Provider, useRegistryState, useRegisterDescendant, useRegisterOverflowObserver} } + +/** + * Treat any target that is not fully visible within the observer root as overflowing. Wrapped items should be fully + * clipped (`isIntersecting: false`, `intersectionRatio: 0`), but partial ratios also count as overflowing to guard + * against sub-pixel boundary cases. + */ +function getIsOverflowing(entry: Pick) { + return !entry.isIntersecting || entry.intersectionRatio < 1 +} + +function supportsIntersectionObserver() { + return typeof IntersectionObserver !== 'undefined' +}