From 4fea5fb7a82bf8c28e207bd41502fa35f6f4bd0f Mon Sep 17 00:00:00 2001 From: Alexander Lelidis Date: Thu, 22 Oct 2026 08:31:52 -0700 Subject: [PATCH 1/4] Batch descendant registry updates Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../batch-descendant-registry-updates.md | 5 + .../__tests__/descendant-registry.test.tsx | 207 ++++++++++++++++-- .../react/src/utils/descendant-registry.tsx | 56 ++--- 3 files changed, 223 insertions(+), 45 deletions(-) create mode 100644 .changeset/batch-descendant-registry-updates.md diff --git a/.changeset/batch-descendant-registry-updates.md b/.changeset/batch-descendant-registry-updates.md new file mode 100644 index 00000000000..4a736397a0e --- /dev/null +++ b/.changeset/batch-descendant-registry-updates.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +ActionBar, UnderlineNav: Batch descendant registry updates to reduce cascading renders during rapid item changes diff --git a/packages/react/src/utils/__tests__/descendant-registry.test.tsx b/packages/react/src/utils/__tests__/descendant-registry.test.tsx index 3ba46c8dace..3a4fcd704db 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} from 'vitest' import type React from 'react' import {Fragment, useState} from 'react' -import {render} from '@testing-library/react' +import {act, render, waitFor} from '@testing-library/react' import {createDescendantRegistry} from '../descendant-registry' import {userEvent} from '@testing-library/user-event' @@ -9,7 +9,7 @@ import {userEvent} from '@testing-library/user-event' * Creates a fresh registry instance with isolated helper components for each test. This ensures * no state leaks between tests via a shared Context or Provider. */ -function createTestRegistry() { +function createTestRegistry({onRegistryRender}: {onRegistryRender?: () => void} = {}) { const {Provider, useRegistryState, useRegisterDescendant} = createDescendantRegistry() /** @@ -17,6 +17,7 @@ function createTestRegistry() { * State is held here and passed down to the Provider. */ function RegistryParent({children}: {children: React.ReactNode}) { + onRegistryRender?.() const [registryState, setRegistry] = useRegistryState() return ( @@ -37,7 +38,24 @@ function createTestRegistry() { } describe('createDescendantRegistry', () => { - it('registers descendant items inside of other components', () => { + it('publishes the initial registry after descendant effects complete', async () => { + const onRegistryRender = vi.fn() + const {RegistryParent, Item} = createTestRegistry({onRegistryRender}) + + const {getByTestId} = render( + + + , + ) + + expect(getByTestId('registry-values').textContent).toBe('') + expect(onRegistryRender).toHaveBeenCalledTimes(1) + + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a')) + expect(onRegistryRender).toHaveBeenCalledTimes(2) + }) + + it('registers descendant items inside of other components', async () => { const {RegistryParent, Item} = createTestRegistry() function Wrapper({value}: {value: string}) { @@ -52,10 +70,10 @@ describe('createDescendantRegistry', () => { , ) - expect(getByTestId('registry-values').textContent).toBe('a,b,c') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b,c')) }) - it('registers descendant items inside of React fragments', () => { + it('registers descendant items inside of React fragments', async () => { const {RegistryParent, Item} = createTestRegistry() const {getByTestId} = render( @@ -68,7 +86,7 @@ describe('createDescendantRegistry', () => { , ) - expect(getByTestId('registry-values').textContent).toBe('a,b,c') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b,c')) }) it('updates item values on change', async () => { @@ -89,11 +107,11 @@ describe('createDescendantRegistry', () => { } const {getByTestId, getByRole} = render() - expect(getByTestId('registry-values').textContent).toBe('a,middle,b') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,middle,b')) await userEvent.click(getByRole('button')) - expect(getByTestId('registry-values').textContent).toBe('a,c,b') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,c,b')) }) it('registers items added to the middle of children after initial render', async () => { @@ -114,11 +132,11 @@ describe('createDescendantRegistry', () => { } const {getByTestId, getByRole} = render() - expect(getByTestId('registry-values').textContent).toBe('a,b') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b')) await userEvent.click(getByRole('button')) - expect(getByTestId('registry-values').textContent).toBe('a,middle,b') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,middle,b')) }) it('coalesces multiple descendants added in a single update into one correct rebuild', async () => { @@ -145,12 +163,12 @@ describe('createDescendantRegistry', () => { } const {getByTestId, getByRole} = render() - expect(getByTestId('registry-values').textContent).toBe('a,e') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,e')) // Several descendants register in the same tick; the coalesced rebuild must still capture all of them in order. await userEvent.click(getByRole('button')) - expect(getByTestId('registry-values').textContent).toBe('a,b,c,d,e') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b,c,d,e')) }) it('drops items from the registry after they unmount', async () => { @@ -171,11 +189,166 @@ describe('createDescendantRegistry', () => { } const {getByTestId, getByRole} = render() - expect(getByTestId('registry-values').textContent).toBe('a,b,c') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b,c')) await userEvent.click(getByRole('button')) - expect(getByTestId('registry-values').textContent).toBe('a,b') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b')) + }) + + it('batches rapid value updates and unregisters into one registry render', async () => { + const onRegistryRender = vi.fn() + const {RegistryParent, Item} = createTestRegistry({onRegistryRender}) + type TestItem = {id: string; value: string} + let setItems: React.Dispatch> | null = null + + function Items() { + const [items, setItemState] = useState([ + {id: 'a', value: 'a'}, + {id: 'b', value: 'b'}, + {id: 'c', value: 'c'}, + ]) + setItems = setItemState + return items.map(item => ) + } + + const {getByTestId} = render( + + + , + ) + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b,c')) + onRegistryRender.mockClear() + + act(() => + setItems!([ + {id: 'a', value: 'a'}, + {id: 'b', value: 'b-1'}, + {id: 'c', value: 'c'}, + ]), + ) + act(() => + setItems!([ + {id: 'a', value: 'a'}, + {id: 'b', value: 'b-2'}, + {id: 'c', value: 'c'}, + ]), + ) + act(() => + setItems!([ + {id: 'a', value: 'a'}, + {id: 'c', value: 'c-latest'}, + ]), + ) + + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,c-latest')) + expect(onRegistryRender).toHaveBeenCalledTimes(1) + }) + + it('keeps final ordering when descendants rapidly mount, update, and unmount before a rebuild', async () => { + const onRegistryRender = vi.fn() + const {RegistryParent, Item} = createTestRegistry({onRegistryRender}) + type TestItem = {id: string; value: string} + let setItems: React.Dispatch> | null = null + + function Items() { + const [items, setItemState] = useState([ + {id: 'a', value: 'a'}, + {id: 'd', value: 'd'}, + ]) + setItems = setItemState + return items.map(item => ) + } + + const {getByTestId} = render( + + + , + ) + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,d')) + onRegistryRender.mockClear() + + act(() => + setItems!([ + {id: 'a', value: 'a'}, + {id: 'b', value: 'b'}, + {id: 'd', value: 'd'}, + ]), + ) + act(() => + setItems!([ + {id: 'a', value: 'a'}, + {id: 'b', value: 'b-latest'}, + {id: 'c', value: 'c'}, + {id: 'd', value: 'd'}, + ]), + ) + act(() => + setItems!([ + {id: 'a', value: 'a'}, + {id: 'c', value: 'c-updated'}, + {id: 'd', value: 'd'}, + ]), + ) + act(() => + setItems!([ + {id: 'a', value: 'a'}, + {id: 'd', value: 'd'}, + {id: 'c', value: 'c-final'}, + ]), + ) + + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,d,c-final')) + expect(onRegistryRender).toHaveBeenCalledTimes(1) + }) + + it('batches rapid unregister and remount lifecycles without deleting the replacement', async () => { + const onRegistryRender = vi.fn() + const {RegistryParent, Item} = createTestRegistry({onRegistryRender}) + type TestItem = {id: string; value: string} + let setItems: React.Dispatch> | null = null + + function Items() { + const [items, setItemState] = useState([ + {id: 'a', value: 'a'}, + {id: 'b', value: 'b'}, + {id: 'c', value: 'c'}, + ]) + setItems = setItemState + return items.map(item => ) + } + + const {getByTestId} = render( + + + , + ) + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b,c')) + onRegistryRender.mockClear() + + act(() => + setItems!([ + {id: 'a', value: 'a'}, + {id: 'c', value: 'c'}, + ]), + ) + act(() => + setItems!([ + {id: 'a', value: 'a'}, + {id: 'b', value: 'b-remounted'}, + {id: 'c', value: 'c'}, + ]), + ) + act(() => + setItems!([ + {id: 'a', value: 'a'}, + {id: 'b', value: 'b-latest'}, + {id: 'c', value: 'c'}, + ]), + ) + + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b-latest,c')) + expect(onRegistryRender).toHaveBeenCalledTimes(1) }) it('registers deep descendants added to the beginning of the tree after initial render', async () => { @@ -206,10 +379,10 @@ describe('createDescendantRegistry', () => { } const {getByTestId, getByRole} = render() - expect(getByTestId('registry-values').textContent).toBe('second,third') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('second,third')) await userEvent.click(getByRole('button')) - expect(getByTestId('registry-values').textContent).toBe('first,second,third') + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('first,second,third')) }) }) diff --git a/packages/react/src/utils/descendant-registry.tsx b/packages/react/src/utils/descendant-registry.tsx index 2397db27315..8e2fbd92968 100644 --- a/packages/react/src/utils/descendant-registry.tsx +++ b/packages/react/src/utils/descendant-registry.tsx @@ -41,9 +41,9 @@ interface DescendantRegistryContext { * the string key is a unique and stable identifier for each component which can be used as a `key` if necessary. * * @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. + * registry will not be available during SSR. Registrations are collected during the effect phase and published after + * the effects complete, so the registry 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() { const Context = createContext>({ @@ -79,24 +79,27 @@ export function createDescendantRegistry() { /** Provide context for registering descendant components. This only needs to wrap `children`. */ function Provider({children, setRegistry}: ProviderProps) { const workingRegistryRef = useRef | 'queued' | 'idle'>('queued') + const registryRef = useRef(new Map()) /** 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 pendingRegistryUpdateRef = useRef(false) const isMountedRef = useRef(true) - const scheduleRebuild = useCallback(() => { - if (pendingRebuildRef.current) return - pendingRebuildRef.current = true + const scheduleRegistryUpdate = useCallback(() => { + if (pendingRegistryUpdateRef.current) return + pendingRegistryUpdateRef.current = true scheduleMicrotask(() => { - pendingRebuildRef.current = false - // Guard against dispatching into an unmounted provider. - if (isMountedRef.current) rebuildRegistry() + pendingRegistryUpdateRef.current = false + if (!isMountedRef.current) return + + if (workingRegistryRef.current === 'queued') { + rebuildRegistry() + } else if (workingRegistryRef.current === 'idle') { + setRegistry(new Map(registryRef.current)) + } }) - }, []) + }, [setRegistry]) useEffect(() => { isMountedRef.current = true @@ -123,10 +126,9 @@ 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). Coalesce concurrent registrations so the - // rebuild only happens once per tick. + // be inserted anywhere in the tree, changing the order of items). workingRegistryRef.current = 'queued' - scheduleRebuild() + scheduleRegistryUpdate() } // Noop if status is `queued` since we will restart the map in the next cycle @@ -136,15 +138,12 @@ export function createDescendantRegistry() { } else if (workingRegistryRef.current === 'idle') { // No need to rebuild the registry when unregistering, because removing an item doesn't affect the order // the rest of the items - setRegistry(prev => { - const copy = new Map(prev) - copy.delete(id) - return copy - }) + registryRef.current.delete(id) + scheduleRegistryUpdate() } } }, - [setRegistry, scheduleRebuild], + [scheduleRegistryUpdate], ) /** Update a descendant's value in the registry. */ @@ -154,23 +153,24 @@ export function createDescendantRegistry() { workingRegistryRef.current.set(id, value) } else if (workingRegistryRef.current === 'idle') { // No need to rebuild the registry when updating a value; that doesn't affect order - setRegistry(prev => new Map(prev).set(id, value)) + registryRef.current.set(id, value) + scheduleRegistryUpdate() } // Ignore `queued` stage; a rebuild is coming that will capture the new value in the next render }, - [setRegistry], + [scheduleRegistryUpdate], ) - // After all descendants' effects complete, commit the working registry to state + // After all descendants' effects complete, prepare the working registry for batched publication useEffect(function commitWorkingRegistry() { if (workingRegistryRef.current instanceof Map) { // There shouldn't be any unset values left, but still filter them just in case const setEntries = Array.from(workingRegistryRef.current.entries()).filter( (entry): entry is [string, T] => entry[1] !== unsetValue, ) - // eslint-disable-next-line react-you-might-not-need-an-effect/no-pass-data-to-parent - setRegistry(new Map(setEntries)) + registryRef.current = new Map(setEntries) workingRegistryRef.current = 'idle' + scheduleRegistryUpdate() } }) From e852d9a486b19d1a62c08f9a26f4945b461d2d4f Mon Sep 17 00:00:00 2001 From: Alexander Lelidis Date: Thu, 22 Oct 2026 09:44:58 -0700 Subject: [PATCH 2/4] Make registry stress fixtures compiler-safe Capture test state setters from effects instead of mutating outer bindings during render. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../__tests__/descendant-registry.test.tsx | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/packages/react/src/utils/__tests__/descendant-registry.test.tsx b/packages/react/src/utils/__tests__/descendant-registry.test.tsx index 3a4fcd704db..cf85a4b427e 100644 --- a/packages/react/src/utils/__tests__/descendant-registry.test.tsx +++ b/packages/react/src/utils/__tests__/descendant-registry.test.tsx @@ -1,6 +1,6 @@ import {describe, expect, it, vi} from 'vitest' import type React from 'react' -import {Fragment, useState} from 'react' +import {Fragment, useEffect, useState} from 'react' import {act, render, waitFor} from '@testing-library/react' import {createDescendantRegistry} from '../descendant-registry' import {userEvent} from '@testing-library/user-event' @@ -208,7 +208,12 @@ describe('createDescendantRegistry', () => { {id: 'b', value: 'b'}, {id: 'c', value: 'c'}, ]) - setItems = setItemState + useEffect(() => { + setItems = setItemState + return () => { + setItems = null + } + }, []) return items.map(item => ) } @@ -256,7 +261,12 @@ describe('createDescendantRegistry', () => { {id: 'a', value: 'a'}, {id: 'd', value: 'd'}, ]) - setItems = setItemState + useEffect(() => { + setItems = setItemState + return () => { + setItems = null + } + }, []) return items.map(item => ) } @@ -314,7 +324,12 @@ describe('createDescendantRegistry', () => { {id: 'b', value: 'b'}, {id: 'c', value: 'c'}, ]) - setItems = setItemState + useEffect(() => { + setItems = setItemState + return () => { + setItems = null + } + }, []) return items.map(item => ) } From f97853fc8cf72d7e3c5aeb6bebef06a161c42da8 Mon Sep 17 00:00:00 2001 From: Alexander Lelidis Date: Fri, 23 Oct 2026 10:38:40 +0000 Subject: [PATCH 3/4] Preserve registry publication during the effect flush Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23a42dde-c59d-4f5a-97ca-4fa6f7c41164 --- .../src/utils/__tests__/descendant-registry.test.tsx | 7 ++----- packages/react/src/utils/descendant-registry.tsx | 9 +++++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/react/src/utils/__tests__/descendant-registry.test.tsx b/packages/react/src/utils/__tests__/descendant-registry.test.tsx index cf85a4b427e..ba2f451f875 100644 --- a/packages/react/src/utils/__tests__/descendant-registry.test.tsx +++ b/packages/react/src/utils/__tests__/descendant-registry.test.tsx @@ -38,7 +38,7 @@ function createTestRegistry({onRegistryRender}: {onRegistryRender?: () => void} } describe('createDescendantRegistry', () => { - it('publishes the initial registry after descendant effects complete', async () => { + it('publishes the initial registry within the descendant effect flush', () => { const onRegistryRender = vi.fn() const {RegistryParent, Item} = createTestRegistry({onRegistryRender}) @@ -48,10 +48,7 @@ describe('createDescendantRegistry', () => { , ) - expect(getByTestId('registry-values').textContent).toBe('') - expect(onRegistryRender).toHaveBeenCalledTimes(1) - - await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a')) + expect(getByTestId('registry-values').textContent).toBe('a') expect(onRegistryRender).toHaveBeenCalledTimes(2) }) diff --git a/packages/react/src/utils/descendant-registry.tsx b/packages/react/src/utils/descendant-registry.tsx index 8e2fbd92968..c29612b3157 100644 --- a/packages/react/src/utils/descendant-registry.tsx +++ b/packages/react/src/utils/descendant-registry.tsx @@ -41,8 +41,8 @@ interface DescendantRegistryContext { * the string key is a unique and stable identifier for each component which can be used as a `key` if necessary. * * @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. Registrations are collected during the effect phase and published after - * the effects complete, so the registry will be populated after hydration on the client. The initial `undefined` value + * registry will not be available during SSR. Registrations are collected and published during the effect phase, + * so the registry 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() { @@ -161,7 +161,7 @@ export function createDescendantRegistry() { [scheduleRegistryUpdate], ) - // After all descendants' effects complete, prepare the working registry for batched publication + // Publish a rebuilt registry in the same effect flush; idle updates remain batched. useEffect(function commitWorkingRegistry() { if (workingRegistryRef.current instanceof Map) { // There shouldn't be any unset values left, but still filter them just in case @@ -170,7 +170,8 @@ export function createDescendantRegistry() { ) registryRef.current = new Map(setEntries) workingRegistryRef.current = 'idle' - scheduleRegistryUpdate() + // eslint-disable-next-line react-you-might-not-need-an-effect/no-pass-data-to-parent + setRegistry(new Map(setEntries)) } }) From 07a8a3eb66ed5433e02ea0627258f637d675303d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arma=C4=9Fan?= Date: Mon, 26 Oct 2026 08:58:12 +0000 Subject: [PATCH 4/4] Fix descendant registry rebuilds across parent rerenders --- .../__tests__/descendant-registry.test.tsx | 23 +++++++++++++++++++ .../react/src/utils/descendant-registry.tsx | 15 +++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/packages/react/src/utils/__tests__/descendant-registry.test.tsx b/packages/react/src/utils/__tests__/descendant-registry.test.tsx index ba2f451f875..9eab90f70af 100644 --- a/packages/react/src/utils/__tests__/descendant-registry.test.tsx +++ b/packages/react/src/utils/__tests__/descendant-registry.test.tsx @@ -168,6 +168,29 @@ describe('createDescendantRegistry', () => { await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b,c,d,e')) }) + it('preserves all descendants when the parent rerenders before a queued rebuild', async () => { + const {RegistryParent, Item} = createTestRegistry() + + function Test({values}: {values: string[]}) { + return ( + + {values.map(value => ( + + ))} + + ) + } + + const {getByTestId, rerender} = render() + expect(getByTestId('registry-values').textContent).toBe('a,d') + + // Both parent commits must finish before the scheduled rebuild runs. + rerender() + rerender() + + await waitFor(() => expect(getByTestId('registry-values').textContent).toBe('a,b,c,d')) + }) + it('drops items from the registry after they unmount', async () => { const {RegistryParent, Item} = createTestRegistry() diff --git a/packages/react/src/utils/descendant-registry.tsx b/packages/react/src/utils/descendant-registry.tsx index c29612b3157..29dfdbdb327 100644 --- a/packages/react/src/utils/descendant-registry.tsx +++ b/packages/react/src/utils/descendant-registry.tsx @@ -108,12 +108,15 @@ export function createDescendantRegistry() { } }, []) - // 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') { - workingRegistryRef.current = new Map() - } - }) + // Initialize only on mount or an explicit rebuild, before descendant effects populate the map. + useIsomorphicLayoutEffect( + function instantiateNewRegistry() { + if (workingRegistryRef.current === 'queued') { + workingRegistryRef.current = new Map() + } + }, + [key], + ) /** * Register a mounted descendant in the registry.