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..9eab90f70af 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 {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' @@ -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,21 @@ function createTestRegistry() { } describe('createDescendantRegistry', () => { - it('registers descendant items inside of other components', () => { + it('publishes the initial registry within the descendant effect flush', () => { + const onRegistryRender = vi.fn() + const {RegistryParent, Item} = createTestRegistry({onRegistryRender}) + + const {getByTestId} = render( + + + , + ) + + 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 +67,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 +83,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 +104,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 +129,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 +160,35 @@ 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('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 () => { @@ -171,11 +209,181 @@ 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'}, + ]) + useEffect(() => { + setItems = setItemState + return () => { + setItems = null + } + }, []) + 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'}, + ]) + useEffect(() => { + setItems = setItemState + return () => { + setItems = null + } + }, []) + 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'}, + ]) + useEffect(() => { + setItems = setItemState + return () => { + setItems = null + } + }, []) + 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 +414,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..29dfdbdb327 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 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() { 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 @@ -105,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. @@ -123,10 +129,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 +141,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 +156,25 @@ 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 + // 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 const setEntries = Array.from(workingRegistryRef.current.entries()).filter( (entry): entry is [string, T] => entry[1] !== unsetValue, ) + registryRef.current = new Map(setEntries) + workingRegistryRef.current = 'idle' // eslint-disable-next-line react-you-might-not-need-an-effect/no-pass-data-to-parent setRegistry(new Map(setEntries)) - workingRegistryRef.current = 'idle' } })