Fix Portal server rendering by deferring the portal mount to the client

.changeset/clean-portals-render.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Portal: Render nothing while server rendering and during hydration instead of accessing the DOM during render, fixing server rendering support. Client-only renders are unchanged and still mount portaled content in the same commit.

packages/react/script/react-compiler.mjs

@@ -30,7 +30,6 @@ const unsupportedPatterns = [
   'src/NavList/NavList.test.tsx',
   'src/Overlay/Overlay.figma.tsx',
   'src/Overlay/Overlay.tsx',
-  'src/Portal/Portal.tsx',
   'src/SelectPanel/SelectPanel.examples.stories.tsx',
   'src/SelectPanel/SelectPanel.test.tsx',
   'src/SelectPanel/SelectPanel.tsx',
@@ -50,7 +49,6 @@ const unsupportedPatterns = [
   'src/hooks/useControllableState.ts',
   'src/hooks/useFocusTrap.ts',
   'src/hooks/useFocusZone.ts',
-  'src/hooks/useMenuInitialFocus.ts',
   'src/hooks/useOnEscapePress.ts',
   'src/hooks/useOnOutsideClick.tsx',
   'src/hooks/useResizeObserver.ts',

packages/react/src/Portal/Portal.test.tsx

@@ -1,11 +1,147 @@
-import {describe, expect, it} from 'vitest'
+import {describe, expect, it, vi} from 'vitest'
 import Portal, {registerPortalRoot, PortalContext} from '../Portal/index'
 
 import {render} from '@testing-library/react'
 import BaseStyles from '../BaseStyles'
-import React from 'react'
+import React, {act} from 'react'
+import {hydrateRoot, type Root} from 'react-dom/client'
+import {renderToString} from 'react-dom/server'
+
+const renderOnServer = (children: React.ReactNode) => {
+  const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
+
+  try {
+    return renderToString(children)
+  } finally {
+    consoleErrorSpy.mockRestore()
+  }
+}
 
 describe('Portal', () => {
+  it('renders nothing during server rendering', () => {
+    // React's server renderer throws when it encounters a portal, so `Portal` has
+    // to render nothing while server rendering.
+    expect(renderOnServer(<Portal>portal content</Portal>)).toEqual('')
+  })
+
+  it('preserves generated IDs during hydration', async () => {
+    const Id = ({name}: {name: string}) => {
+      const id = React.useId()
+      return <div id={id} data-name={name} />
+    }
+    const App = () => (
+      <>
+        <Id name="before" />
+        <Portal>
+          <Id name="portal" />
+        </Portal>
+        <Id name="after" />
+      </>
+    )
+    const container = document.createElement('div')
+    container.innerHTML = renderOnServer(<App />)
+    document.body.appendChild(container)
+
+    const serverIds = Array.from(container.querySelectorAll('[data-name]'), element => element.id)
+    const recoverableErrors: unknown[] = []
+    let root: Root | undefined
+
+    try {
+      await act(async () => {
+        root = hydrateRoot(container, <App />, {
+          onRecoverableError: error => recoverableErrors.push(error),
+        })
+      })
+
+      expect(recoverableErrors).toEqual([])
+      expect(Array.from(container.querySelectorAll('[data-name]'), element => element.id)).toEqual(serverIds)
+      expect(document.querySelector('[data-name="portal"]')).toBeInstanceOf(HTMLElement)
+    } finally {
+      await act(async () => root?.unmount())
+      container.remove()
+    }
+  })
+
+  it('mounts children in the same commit as the Portal', () => {
+    const refPerCommit: Array<HTMLElement | null> = []
+
+    const App = () => {
+      const ref = React.useRef<HTMLDivElement>(null)
+
+      // Intentionally no dependency array, so this runs after every commit and the
+      // length of `refPerCommit` is also the number of commits.
+      React.useLayoutEffect(() => {
+        refPerCommit.push(ref.current)
+      })
+
+      return (
+        <Portal>
+          <div ref={ref}>portal content</div>
+        </Portal>
+      )
+    }
+
+    const {baseElement} = render(<App />)
+
+    // A single entry proves the Portal did not need a second pass, and a populated
+    // ref proves the children mounted in the same commit as the Portal itself.
+    expect(refPerCommit).toEqual([expect.any(HTMLDivElement)])
+
+    baseElement.innerHTML = ''
+  })
+
+  it('removes its host element from the portal root when unmounted', () => {
+    const {baseElement, unmount} = render(<Portal>portal content</Portal>)
+    const generatedRoot = baseElement.querySelector('#__primerPortalRoot__')
+
+    expect(generatedRoot?.querySelectorAll('[data-component="Portal"]')).toHaveLength(1)
+
+    unmount()
+
+    expect(generatedRoot?.querySelectorAll('[data-component="Portal"]')).toHaveLength(0)
+
+    baseElement.innerHTML = ''
+  })
+
+  it('attaches a single host element under StrictMode, which remounts effects', () => {
+    const {baseElement} = render(
+      <React.StrictMode>
+        <Portal>portal content</Portal>
+      </React.StrictMode>,
+    )
+    const generatedRoot = baseElement.querySelector('#__primerPortalRoot__')
+
+    expect(generatedRoot?.querySelectorAll('[data-component="Portal"]')).toHaveLength(1)
+    expect(generatedRoot?.textContent.trim()).toEqual('portal content')
+
+    baseElement.innerHTML = ''
+  })
+
+  it('moves the portal when containerName changes', () => {
+    const {baseElement} = render(
+      <main>
+        <div id="rootA" />
+        <div id="rootB" />
+      </main>,
+    )
+    const rootA = baseElement.querySelector('#rootA')!
+    const rootB = baseElement.querySelector('#rootB')!
+    registerPortalRoot(rootA, 'rootA')
+    registerPortalRoot(rootB, 'rootB')
+
+    const {rerender} = render(<Portal containerName="rootA">portal content</Portal>)
+
+    expect(rootA.textContent.trim()).toEqual('portal content')
+    expect(rootB.textContent.trim()).toEqual('')
+
+    rerender(<Portal containerName="rootB">portal content</Portal>)
+
+    expect(rootA.querySelectorAll('[data-component="Portal"]')).toHaveLength(0)
+    expect(rootB.textContent.trim()).toEqual('portal content')
+
+    baseElement.innerHTML = ''
+  })
+
   it('renders a default portal into document.body (no BaseStyles present)', () => {
     const {baseElement} = render(<Portal>123test123</Portal>)
     const generatedRoot = baseElement.querySelector('#__primerPortalRoot__')
@@ -65,6 +201,24 @@ describe('Portal', () => {
     baseElement.innerHTML = ''
   })
 
+  it('calls onMount once, and does not remount when an inline onMount changes identity', () => {
+    const onMount = vi.fn()
+
+    const {baseElement, rerender} = render(<Portal onMount={() => onMount()}>portal content</Portal>)
+    const portalNode = baseElement.querySelector('[data-component="Portal"]')
+
+    expect(onMount).toHaveBeenCalledTimes(1)
+
+    // A new `onMount` function identity must not detach and re-attach the portal,
+    // which would unmount and remount every portaled DOM node.
+    rerender(<Portal onMount={() => onMount()}>portal content</Portal>)
+
+    expect(onMount).toHaveBeenCalledTimes(1)
+    expect(baseElement.querySelector('[data-component="Portal"]')).toBe(portalNode)
+
+    baseElement.innerHTML = ''
+  })
+
   it('renders into the custom portal root (default root name - imperative)', () => {
     const portalRootJSX = <div id="myPortalRoot"></div>
     let {baseElement} = render(portalRootJSX)

packages/react/src/Portal/Portal.tsx

@@ -1,9 +1,33 @@
-import React, {useContext} from 'react'
+import React, {useContext, useSyncExternalStore} from 'react'
 import {createPortal} from 'react-dom'
 import useLayoutEffect from '../utils/useIsomorphicLayoutEffect'
 import {PortalContext} from './PortalContext'
 import {DEFAULT_PORTAL_CONTAINER_NAME, ensureDefaultPortal, getPortalRoot} from './portalRoot'
 
+const subscribe = () => () => {}
+const getSnapshot = () => true
+const getServerSnapshot = () => false
+
+/**
+ * Returns `true` only when React is rendering on the client, and not as part of
+ * hydrating server-rendered markup.
+ *
+ * `Portal` needs this because React's server renderer throws when it encounters a
+ * portal, and portaled content is never part of the server-rendered markup.
+ *
+ * `useSyncExternalStore` is used rather than a `useEffect`/`useState` "is mounted"
+ * flag because it only defers the portal for the renders that need it: it returns
+ * `false` while server rendering and for the hydration pass (keeping hydration
+ * consistent with the server markup), but `true` from the very first render of a
+ * client-only render. That way client-rendered portals — the overwhelmingly common
+ * case for overlays, dialogs and menus — still mount their children in the same
+ * commit as the `Portal` itself, so refs into portaled content are populated by the
+ * time the parent's layout effects run.
+ */
+function useIsClientRender() {
+  return useSyncExternalStore(subscribe, getSnapshot, getServerSnapshot)
+}
+
 export interface PortalProps {
   /**
    * Called when this portal is added to the DOM
@@ -28,23 +52,37 @@ export const Portal: React.FC<React.PropsWithChildren<PortalProps>> = ({
   containerName: _containerName,
 }) => {
   const {portalContainerName} = useContext(PortalContext)
-  const elementRef = React.useRef<HTMLDivElement | null>(null)
-  // eslint-disable-next-line react-hooks/refs
-  if (!elementRef.current) {
+  const isClientRender = useIsClientRender()
+
+  // `onMount` is read from a ref so that it is not part of the effect below's
+  // dependencies. Depending on it directly would detach and re-attach the portal
+  // — unmounting and remounting every portaled DOM node — whenever a consumer
+  // passes an inline callback.
+  const onMountRef = React.useRef(onMount)
+  useLayoutEffect(() => {
+    onMountRef.current = onMount
+  })
+
+  // The host element is created once per `Portal` instance. A lazy state initializer
+  // is used instead of an effect so that the portal, and its children, mount in the
+  // same commit as the `Portal` itself. `document` is not available while server
+  // rendering, where the element is never needed because nothing is rendered.
+  const [element] = React.useState<HTMLDivElement | null>(() => {
+    if (typeof document === 'undefined') return null
+
     const div = document.createElement('div')
     div.setAttribute('data-component', 'Portal')
     // Portaled content should get their own stacking context so they don't interfere
     // with each other in unexpected ways. One should never find themselves tempted
     // to change the zIndex to a value other than "1".
     div.style.position = 'relative'
     div.style.zIndex = '1'
-    elementRef.current = div
-  }
-
-  // eslint-disable-next-line react-hooks/refs
-  const element = elementRef.current
+    return div
+  })
 
   useLayoutEffect(() => {
+    if (!element || !isClientRender) return
+
     let containerName = _containerName ?? portalContainerName
     if (containerName === undefined) {
       containerName = DEFAULT_PORTAL_CONTAINER_NAME
@@ -58,13 +96,12 @@ export const Portal: React.FC<React.PropsWithChildren<PortalProps>> = ({
       )
     }
     parentElement.appendChild(element)
-    onMount?.()
+    onMountRef.current?.()
 
     return () => {
       parentElement.removeChild(element)
     }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [element, _containerName, portalContainerName])
+  }, [element, isClientRender, _containerName, portalContainerName])
 
-  return createPortal(children, element)
+  return element && isClientRender ? createPortal(children, element) : null
 }

packages/react/src/__tests__/ssr.test.tsx

@@ -0,0 +1,19 @@
+/**
+ * Tests that run in a real server environment, where there is no `document`.
+ * @vitest-environment node
+ */
+import {describe, expect, it} from 'vitest'
+import {renderToString} from 'react-dom/server'
+import Portal from '../Portal'
+
+describe('server rendering', () => {
+  it('has no DOM available', () => {
+    expect(typeof document).toEqual('undefined')
+  })
+
+  it('renders Portal without touching the DOM', () => {
+    // React's server renderer throws when it encounters a portal, so `Portal` must
+    // render nothing, and must not create its host element, while server rendering.
+    expect(renderToString(<Portal>portal content</Portal>)).toEqual('')
+  })
+})

packages/react/src/hooks/useMenuInitialFocus.ts

@@ -70,9 +70,8 @@ export const useMenuInitialFocus = (
         setTimeout(() => firstElement?.focus())
       }
     },
-    // we don't want containerRef in dependencies
-    // because re-renders to containerRef while it's open should not fire initialMenuFocus
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-    [open, openingGesture, anchorRef],
+    // `containerRef` is a ref object, so its identity is stable across re-renders and
+    // including it here cannot re-fire the initial focus while the menu stays open.
+    [open, openingGesture, anchorRef, containerRef],
   )
 }

packages/react/vitest.config.browser.mts

@@ -43,6 +43,7 @@ export default defineConfig({
       '**/*.types.test.ts',
       '**/*.types.test.tsx',
       'src/__tests__/exports.test.ts',
+      'src/__tests__/ssr.test.tsx',
       'src/__tests__/storybook.test.tsx',
     ],
     include: ['src/**/*.test.?(c|m)[jt]s?(x)'],

packages/react/vitest.config.mts

@@ -24,7 +24,7 @@ export default defineConfig({
   },
   test: {
     name: '@primer/react (node)',
-    include: ['src/__tests__/exports.test.ts', 'src/__tests__/storybook.test.tsx'],
+    include: ['src/__tests__/exports.test.ts', 'src/__tests__/ssr.test.tsx', 'src/__tests__/storybook.test.tsx'],
     environment: 'node',
     detectAsyncLeaks: true,
   },

script/check-classname-tests.mjs

@@ -23,6 +23,8 @@ const IGNORED_FILES = [
   'packages/react/src/__tests__/ThemeProvider.test.tsx',
   'packages/react/src/__tests__/deprecated/ActionMenu.test.tsx',
   'packages/react/src/__tests__/Caret.test.tsx',
+  // Asserts server rendering behavior, not rendered markup
+  'packages/react/src/__tests__/ssr.test.tsx',
   'packages/react/src/TreeView/useRovingTabIndex.test.tsx',
 ]