Fix Portal server rendering by deferring the portal mount to the client - #8276
Fix Portal server rendering by deferring the portal mount to the client#8276mattcosta7 with Copilot wants to merge 7 commits into
Conversation
🦋 Changeset detectedLatest commit: 1e2f20d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
Detect server rendering with useSyncExternalStore instead of deferring the host element into an effect. The portal stays client-only, but client-only renders mount portaled children in the same commit as the Portal itself, so refs into portaled content are populated when the parent's layout effects run. This removes the need for the ref-to-state workarounds in ActionMenu, Dialog, Overlay, SelectPanel, useMenuInitialFocus and useOpenAndCloseFocus, which are reverted. Also read onMount from a ref so it can leave the effect deps without a lint suppression, and drop the stale containerRef suppression in useMenuInitialFocus. Both files now compile under the React Compiler. Adds a node-environment SSR test that runs with no DOM available.
|
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
|
Integration test results from github/github-ui PR:
CI check runs linting, type checking, and unit tests. Check the workflow logs for specific failures. Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance. |
There was a problem hiding this comment.
Pull request overview
This PR updates Portal to avoid crashing or throwing during server rendering by deferring portal creation/mounting until React is doing a client render (including special handling for hydration), and adds targeted tests/config updates to cover SSR behavior.
Changes:
- Update
Portalto useuseSyncExternalStoreto distinguish server render/hydration vs client render, returningnulluntil client render is confirmed. - Expand
Portaltest coverage for SSR, hydration, StrictMode, cleanup, andonMountstability; add a dedicated node-environment SSR test. - Enable React Compiler for
Portal.tsxanduseMenuInitialFocus.tsby removing them from the unsupported list and addressing hook-deps suppression.
Show a summary per file
| File | Description |
|---|---|
| script/check-classname-tests.mjs | Ignores the new SSR-focused test from className markup requirements. |
| packages/react/vitest.config.mts | Adds ssr.test.tsx to the node test project include list. |
| packages/react/vitest.config.browser.mts | Excludes ssr.test.tsx from the browser (Playwright) test project. |
| packages/react/src/Portal/Portal.tsx | Defers portal rendering/mounting using useSyncExternalStore; refactors onMount handling. |
| packages/react/src/Portal/Portal.test.tsx | Adds SSR/hydration/StrictMode/cleanup/container move/onMount identity regression tests. |
| packages/react/src/hooks/useMenuInitialFocus.ts | Removes exhaustive-deps suppression by including containerRef in deps with rationale. |
| packages/react/src/tests/ssr.test.tsx | New node-environment SSR regression test ensuring Portal renders null with no DOM. |
| packages/react/script/react-compiler.mjs | Removes Portal.tsx and useMenuInitialFocus.ts from the unsupported patterns list. |
| .changeset/clean-portals-render.md | Adds a patch changeset describing the SSR/hydration Portal behavior change. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
Closes #3721
Portalcalleddocument.createElementduring render, so importing and rendering any component built on it (Dialog,ActionMenu,Overlay,SelectPanel, tooltips, …) crashed immediately in a server environment wheredocumentdoes not exist.Simply guarding with
typeof document === 'undefined'is not enough: in a server render that does have a DOM (a jsdom-based SSR smoke test, for example) the guard passes andrenderToStringthen throwsPortals are not currently supported by the server renderer. So the portal has to be skipped based on whether we are server rendering, not on whether a DOM happens to be present.Portalnow usesuseSyncExternalStoreto detect a client render, and only creates its host element and callscreatePortalonce that is true. The host element is created in a lazyuseStateinitializer rather than in an effect, which is the key detail for consumers.Behavior
documentis never touched; rendersnullnullvia the server snapshot; no thrownull, matching the server; React's store-consistency check then synchronously re-renders and mounts the portal. No hydration mismatch, and generated IDs are preservedPortal, so refs are populated for parent layout effectsThat last row is the important one. An earlier iteration of this PR mounted in two passes via
useLayoutEffect+setState, which delayed portaled children by one commit. That broke every consumer that reads a ref in its first layout effect, and required ref→state workarounds acrossActionMenu,Dialog,Overlay,SelectPanel, anduseOpenAndCloseFocus. All of those workarounds have been reverted — no consumer changes are needed.Changelog
Changed
Portalrenders nothing while server rendering and during hydration, instead of accessing the DOM during render.onMountis read through a latest-ref, so passing an inline arrow no longer risks re-firing the mount effect.Portal.tsxanduseMenuInitialFocus.tsby removing theirreact-hooksESLint suppressions (the compiler bails on any file that disables those rules). Both are verified as actually compiled.Rollout strategy
Bug fix only. Client-only rendering — the path essentially all current consumers are on — is behaviorally identical.
Testing & Reviewing
Portal.test.tsxcovers 16 cases; new ones worth reviewing:hydrateRootand asserts no recoverable errors and that generated IDs are preserved.containerNamechange — cover the effect cleanup and dependency-array changes.src/__tests__/ssr.test.tsxis a new node-environment test (the browser project always has adocument, so it cannot simulate this). It assertstypeof document === 'undefined'and thatrenderToString(<Portal>)returns''. It is added to the node project'sincludeallowlist and excluded from the browser project.Full suite, type-check, lint, and format are green.
Known follow-up
Fixing
Portalis necessary but not sufficient to server-render consumers end to end. I verified this by attemptingrenderToString(<Dialog>)in the node environment:Portalis no longer the blocker, but it still throws fromuseFocusTrap.ts, which readsdocument.activeElementduring render — the same class of bug, in a second location.I deliberately left that out of this PR. Moving that capture into a layout effect changes focus-restoration timing, which is a real behavioral risk in focus management and deserves its own PR and review. Once it lands, a consumer-level SSR test becomes possible and would be a much stronger regression guard.