Eliminate nested-update cascades in useFocus, PageLayout.Pane, and AnchoredOverlay

.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.

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 (
+        <BaseStyles>
+          <AnchoredOverlay
+            open={open}
+            onOpen={() => setOpen(true)}
+            onClose={() => setOpen(false)}
+            renderAnchor={props => <Button {...props}>Toggle</Button>}
+          >
+            <div>content</div>
+          </AnchoredOverlay>
+        </BaseStyles>
+      )
+    }
+
+    const [Wrap, counter] = createRenderCounter()
+    render(
+      <Wrap>
+        <Demo />
+      </Wrap>,
+    )
+
+    // 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', () => {

packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx

@@ -239,10 +239,13 @@ export const AnchoredOverlay: React.FC<React.PropsWithChildren<AnchoredOverlayPr
       anchorOffset,
       displayInViewport,
       onPositionChange: positionChange,
-      // When native CSS anchor positioning is active, skip JS-based position
-      // computation, scroll listeners, and resize observers since the browser
-      // handles repositioning natively.
-      enabled: !cssAnchorPositioning,
+      // Disable position computation, scroll listeners, and resize observers
+      // when the overlay is closed (no floating element to position) or when
+      // native CSS anchor positioning is active (the browser handles it).
+      // Skipping the listeners while closed avoids a wasted re-render on the
+      // first scroll/resize after a close (where `setPosition(undefined)`
+      // would otherwise clear the stale open-position state).
+      enabled: open && !cssAnchorPositioning,
     },
 
     [overlayElement],

packages/react/src/PageLayout/PageLayout.test.tsx

@@ -6,6 +6,7 @@ import {viewportRanges} from '../hooks/useResponsiveValue'
 import {PageLayout} from './PageLayout'
 import {Placeholder} from '../Placeholder'
 import {implementsClassName} from '../utils/testing'
+import {createRenderCounter} from '../utils/testing/profiler'
 import classes from './PageLayout.module.css'
 
 describe('PageLayout', async () => {
@@ -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(
+        <Wrap>
+          <PageLayout>
+            <PageLayout.Pane resizable>
+              <Placeholder height={320} label="Pane" />
+            </PageLayout.Pane>
+            <PageLayout.Content>
+              <Placeholder height={640} label="Content" />
+            </PageLayout.Content>
+          </PageLayout>
+        </Wrap>,
+      )
+      // Allow post-paint effects to flush
+      await act(async () => {})
+      expect(counter.nestedUpdateCount).toBe(0)
+    })
   })
 
   describe('PageLayout.Content', () => {

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

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<HTMLInputElement>(null)
+      return (
+        <>
+          {show ? <input ref={inputRef} data-testid="target" /> : null}
+          <button
+            type="button"
+            onClick={() => {
+              setShow(true)
+              focus(inputRef)
+            }}
+          >
+            show
+          </button>
+        </>
+      )
+    }
+
+    render(<Demo />)
+    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<HTMLInputElement>(null)
+      return (
+        <Wrap>
+          <>
+            {show ? <input ref={inputRef} data-testid="target" /> : null}
+            <button
+              type="button"
+              onClick={() => {
+                setShow(true)
+                focus(inputRef)
+              }}
+            >
+              show
+            </button>
+          </>
+        </Wrap>
+      )
+    }
+
+    render(<Demo />)
+    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 <Profiler>) 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<HTMLInputElement>(null)
+      triggerFocus = () => focus(ref.current!)
+      return <input ref={ref} data-testid="target" />
+    }
+
+    render(<Demo />)
+    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)
+  })
+})

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<HTMLElement | RefObject<HTMLElement> | null>(null)
+  const targetRef = useRef<HTMLElement | RefObject<HTMLElement> | 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<HTMLElement>) => {
+    targetRef.current = target
+    setVersion(v => v + 1)
+  }, [])
 }

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(<Wrap><MyComponent /></Wrap>)
+ *   // ... 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[]}) => (
+    <Profiler id={id} onRender={onRender}>
+      {children}
+    </Profiler>
+  )
+
+  return [Wrap, counter]
+}