diff --git a/.changeset/cascade-elimination.md b/.changeset/cascade-elimination.md new file mode 100644 index 00000000000..9b2f6d6ef30 --- /dev/null +++ b/.changeset/cascade-elimination.md @@ -0,0 +1,22 @@ +--- +'@primer/react': patch +--- + +Eliminate nested-update cascades in `useFocus`, `PageLayout.Pane`, and +`AnchoredOverlay`: + +- `useFocus` no longer produces a second re-render after focusing; one + `focus()` call now results in exactly one render instead of two. +- `PageLayout.Pane` (resizable) no longer triggers a forced re-render + before paint on mount. The CSS variable and ARIA attributes are still + updated synchronously in the layout effect; the React state sync is + wrapped in `startTransition` so it runs in the transition lane rather + than as part of the layout-effect commit. +- `AnchoredOverlay` no longer keeps `useAnchoredPosition`'s scroll + listeners and `ResizeObserver` attached while it is closed. After an + open→close cycle, the first scroll/resize event no longer fires a + spurious `setPosition(undefined)` that re-renders the closed overlay. + +Also adds a profiler-based test harness at +`src/utils/testing/profiler.tsx` so future regressions can be pinned with +`counter.updateCount` and `counter.nestedUpdateCount` assertions. diff --git a/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx b/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx index 255c771c6b3..0613dd97493 100644 --- a/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx +++ b/packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx @@ -7,6 +7,7 @@ import {Button} from '../Button' import BaseStyles from '../BaseStyles' import type {AnchorPosition} from '@primer/behaviors' import {implementsClassName} from '../utils/testing' +import {createRenderCounter} from '../utils/testing/profiler' import {FeatureFlags} from '../FeatureFlags' import {registerPortalRoot} from '../Portal' @@ -234,6 +235,58 @@ describe.each([true, false])( }, ) +describe('AnchoredOverlay scroll/resize cascade', () => { + it('does not re-render after close when window scrolls (closed-overlay listeners detached)', async () => { + // Before the `enabled: open` fix, useAnchoredPosition kept its scroll + // listeners attached while the overlay was closed. The first scroll event + // after a close would fire updatePosition, hit the else branch, and call + // setPosition(undefined) — which differs from the stale last-open value, + // forcing a re-render of AnchoredOverlay. Gating `enabled` on `open` tears + // the listeners down on close so no spurious re-render fires. + function Demo() { + const [open, setOpen] = useState(false) + return ( + + setOpen(true)} + onClose={() => setOpen(false)} + renderAnchor={props => } + > +
content
+ + + ) + } + + const [Wrap, counter] = createRenderCounter() + render( + + + , + ) + + // Open then close the overlay so position has a non-undefined last value. + await userEvent.click(document.body.querySelector('button')!) + await userEvent.keyboard('{Escape}') + + // Let any pending close-time effects flush. + await act(async () => {}) + counter.reset() + + // Dispatch a scroll event on window. Under the old behavior this would + // fire the still-attached rAF-throttled handler → setPosition(undefined) + // → one re-render. Under the fix, listeners were torn down on close. + await act(async () => { + window.dispatchEvent(new Event('scroll', {bubbles: true})) + // Flush rAF so any (incorrectly) scheduled updatePosition would run. + await new Promise(resolve => requestAnimationFrame(() => resolve(undefined))) + }) + + expect(counter.updateCount).toBe(0) + }) +}) + describe('AnchoredOverlay feature flag specific behavior', () => { describe('with primer_react_css_anchor_positioning feature flag enabled', () => { it('should render overlay as visible immediately when flag is enabled', () => { diff --git a/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx b/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx index e0050775587..a507313e784 100644 --- a/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx +++ b/packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx @@ -239,10 +239,13 @@ export const AnchoredOverlay: React.FC { @@ -259,6 +260,29 @@ describe('PageLayout', async () => { fireEvent.lostPointerCapture(divider, {pointerId: 1}) expect(pane!.style.willChange).toBe('') }) + it('should not cause a nested-update cascade on mount when resizable', async () => { + // The usePaneWidth layout effect used to call setMaxPaneWidth(initialMax) + // unconditionally on mount, forcing a synchronous re-render before paint + // (a "nested-update" in React Profiler terms). The fix defers that + // setState to a post-paint useEffect, so the layout effect's commit + // should produce zero nested updates. + const [Wrap, counter] = createRenderCounter() + render( + + + + + + + + + + , + ) + // Allow post-paint effects to flush + await act(async () => {}) + expect(counter.nestedUpdateCount).toBe(0) + }) }) describe('PageLayout.Content', () => { diff --git a/packages/react/src/PageLayout/usePaneWidth.ts b/packages/react/src/PageLayout/usePaneWidth.ts index f0c12b57e11..4ace09f7673 100644 --- a/packages/react/src/PageLayout/usePaneWidth.ts +++ b/packages/react/src/PageLayout/usePaneWidth.ts @@ -388,9 +388,15 @@ export function usePaneWidth({ maxWidthDiffRef.current = getMaxWidthDiffFromViewport() const initialMax = getMaxPaneWidthRef.current() maxPaneWidthRef.current = initialMax - setMaxPaneWidth(initialMax) paneRef.current?.style.setProperty('--pane-max-width', `${initialMax}px`) updateAriaValues(handleRef.current, {min: minPaneWidth, max: initialMax, current: currentWidthRef.current}) + // Sync React state via a transition so it doesn't force a synchronous + // re-render before paint (i.e. a Profiler `nested-update`). The DOM and + // ARIA values above are already correct; this state is only consumed by + // the next React render of `aria-valuemax`. Same pattern as `syncAll`. + startTransition(() => { + setMaxPaneWidth(initialMax) + }) // For custom widths that aren't viewport-constrained, max is fixed - no need to listen to resize if (customMaxWidth !== null && !constrainToViewport) return diff --git a/packages/react/src/internal/hooks/__tests__/useFocus.test.tsx b/packages/react/src/internal/hooks/__tests__/useFocus.test.tsx new file mode 100644 index 00000000000..4c764ef55bb --- /dev/null +++ b/packages/react/src/internal/hooks/__tests__/useFocus.test.tsx @@ -0,0 +1,100 @@ +import {act, render, screen} from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import React from 'react' +import {describe, expect, it} from 'vitest' +import {useFocus} from '../useFocus' +import {createRenderCounter} from '../../../utils/testing/profiler' + +describe('useFocus', () => { + it('focuses the element on the next commit', async () => { + function Demo() { + const focus = useFocus() + const [show, setShow] = React.useState(false) + const inputRef = React.useRef(null) + return ( + <> + {show ? : null} + + + ) + } + + render() + await userEvent.click(screen.getByText('show')) + expect(screen.getByTestId('target')).toHaveFocus() + }) + + it('does not cause a cascade — one focus call produces exactly one render', async () => { + // Renders are caused by: initial mount (1), and the single user click that + // sets visibility + calls focus() (2 — both setState calls are batched). + // The previous implementation produced an extra render because the effect + // reset focusTarget to null, triggering a second render. + const [Wrap, counter] = createRenderCounter() + + function Demo() { + const focus = useFocus() + const [show, setShow] = React.useState(false) + const inputRef = React.useRef(null) + return ( + + <> + {show ? : null} + + + + ) + } + + render() + counter.reset() + + await userEvent.click(screen.getByText('show')) + + // Exactly one render from the click — no cascade from useFocus's effect. + expect(counter.updateCount).toBe(1) + }) + + it('host component is not re-rendered by focus() when version is stable', async () => { + // The hook should not cause the host component to render more than what + // the user's own setState triggers. We track function-body executions + // directly (rather than using ) so we count this component + // specifically rather than any wrapper subtree. + let renderCount = 0 + + let triggerFocus: (() => void) | null = null + + function Demo() { + renderCount += 1 + const focus = useFocus() + const ref = React.useRef(null) + triggerFocus = () => focus(ref.current!) + return + } + + render() + expect(renderCount).toBe(1) // initial mount + + await act(async () => { + triggerFocus!() + }) + // The host re-renders once because useFocus bumps its internal version state. + // It must NOT re-render a second time (which the old impl did when resetting state). + expect(renderCount).toBe(2) + }) +}) diff --git a/packages/react/src/internal/hooks/useFocus.ts b/packages/react/src/internal/hooks/useFocus.ts index 5ca0f4598c1..7bb2e2494bf 100644 --- a/packages/react/src/internal/hooks/useFocus.ts +++ b/packages/react/src/internal/hooks/useFocus.ts @@ -1,22 +1,31 @@ -import {type RefObject, useEffect, useState} from 'react' +import {type RefObject, useCallback, useEffect, useRef, useState} from 'react' +/** + * Returns a function that focuses the given element on the next render commit. + * + * The target is stored in a ref (not state) and the effect is gated by a + * version counter, so calling `focus()` produces exactly one render — never + * the two-render cascade you'd get from `setState(target)` followed by + * `setState(null)` inside the effect. + */ export function useFocus() { - const [focusTarget, setFocusTarget] = useState | null>(null) + const targetRef = useRef | null>(null) + const [version, setVersion] = useState(0) useEffect(() => { - if (focusTarget === null) { - return - } + const target = targetRef.current + if (target === null) return + targetRef.current = null - if (focusTarget instanceof HTMLElement) { - focusTarget.focus() + if (target instanceof HTMLElement) { + target.focus() } else { - focusTarget.current?.focus() + target.current?.focus() } + }, [version]) - // eslint-disable-next-line react-hooks/set-state-in-effect - setFocusTarget(null) - }, [focusTarget]) - - return setFocusTarget + return useCallback((target: HTMLElement | RefObject) => { + targetRef.current = target + setVersion(v => v + 1) + }, []) } diff --git a/packages/react/src/utils/testing/profiler.tsx b/packages/react/src/utils/testing/profiler.tsx new file mode 100644 index 00000000000..1dcfdfdfe2f --- /dev/null +++ b/packages/react/src/utils/testing/profiler.tsx @@ -0,0 +1,74 @@ +import React, {Profiler, type ProfilerOnRenderCallback, type ReactElement} from 'react' + +export type RenderCounter = { + /** Total number of times the wrapped subtree has rendered (mount + updates). */ + readonly count: number + /** Number of mount renders. */ + readonly mountCount: number + /** Number of update renders (any cause). */ + readonly updateCount: number + /** + * Number of updates triggered inside the same commit cycle that produced the + * original render — i.e., a `setState` call from inside a `useLayoutEffect` + * that forced a synchronous re-render before paint. This is the canonical + * "nested update" / cascade signal; assert `nestedUpdateCount === 0` to pin + * that a component never forces a double-commit. + */ + readonly nestedUpdateCount: number + /** Reset the counter. */ + reset(): void +} + +/** + * Returns a `[Wrap, counter]` pair. Wrap the component under test with `Wrap` + * to count how many times its subtree renders. Useful for asserting that a + * single user interaction (or a stable prop) does not cause cascading renders. + * + * @example + * const [Wrap, counter] = createRenderCounter() + * render() + * // ... interaction ... + * expect(counter.updateCount).toBe(1) + * expect(counter.nestedUpdateCount).toBe(0) + */ +export function createRenderCounter( + id = 'profiler-root', +): [(props: {children: ReactElement | ReactElement[]}) => ReactElement, RenderCounter] { + const state = {count: 0, mountCount: 0, updateCount: 0, nestedUpdateCount: 0} + + const onRender: ProfilerOnRenderCallback = (_id, phase) => { + state.count += 1 + if (phase === 'mount') state.mountCount += 1 + else state.updateCount += 1 + if (phase === 'nested-update') state.nestedUpdateCount += 1 + } + + const counter: RenderCounter = { + get count() { + return state.count + }, + get mountCount() { + return state.mountCount + }, + get updateCount() { + return state.updateCount + }, + get nestedUpdateCount() { + return state.nestedUpdateCount + }, + reset() { + state.count = 0 + state.mountCount = 0 + state.updateCount = 0 + state.nestedUpdateCount = 0 + }, + } + + const Wrap = ({children}: {children: ReactElement | ReactElement[]}) => ( + + {children} + + ) + + return [Wrap, counter] +}