fix(FloatingPortal): guard addEventListener against null portalNode under concurrent React#4740
Conversation
…nder concurrent React Under React 18+ concurrent rendering, the useEffect that attaches focusin/focusout listeners on portalNode can fire before the ref callback has populated the node, despite the early-return guard at the top of the effect. This causes: TypeError: Cannot read properties of null (reading 'addEventListener') Align the call site with the pattern already used in FloatingFocusManager, where short-circuit evaluation (node && addEventListener(node, ...)) prevents the call when the node is falsy. mergeCleanups already accepts false|null|undefined so no type changes are needed. Fixes mui#4739
ossaidqadri
requested review from
LukasTy,
atomiks,
colmtuite,
flaviendelangle and
michaldudak
as code owners
commit: |
Bundle size
PerformanceTotal duration: 1,545.41 ms 🔺+383.27 ms(+33.0%) | Renders: 53 (+0) | Paint: 2,379.64 ms 🔺+591.75 ms(+33.1%)
…and 7 more — details Check out the code infra dashboard for more information about this PR. |
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent a runtime TypeError in FloatingPortal under React concurrent rendering by avoiding addEventListener(...) calls when portalNode is null.
Changes:
- Adds
portalNode &&short-circuiting to thefocusin/focusoutlistener setup inFloatingPortal. - Adds an inline comment explaining the guard and referencing the reported concurrent rendering issue.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| React.useEffect(() => { | ||
| if (!portalNode || modal) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // Make sure elements inside the portal element are tabbable only when the | ||
| // portal has already been focused, either by tabbing into a focus trap | ||
| // element outside or using the mouse. | ||
| function onFocus(event: FocusEvent) { | ||
| if (portalNode && event.relatedTarget && isOutsideEvent(event)) { | ||
| if (event.type === 'focusin') { | ||
| if (focusInsideDisabledRef.current) { | ||
| enableFocusInside(portalNode); | ||
| focusInsideDisabledRef.current = false; | ||
| } | ||
| } else { | ||
| disableFocusInside(portalNode); | ||
| focusInsideDisabledRef.current = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Listen to the event on the capture phase so they run before the focus | ||
| // trap elements onFocus prop is called. | ||
| // Guard with `portalNode &&` to defend against concurrent React renders | ||
| // where the effect fires before the ref is attached (issue #4739). | ||
| return mergeCleanups( | ||
| addEventListener(portalNode, 'focusin', onFocus, true), | ||
| addEventListener(portalNode, 'focusout', onFocus, true), | ||
| portalNode && addEventListener(portalNode, 'focusin', onFocus, true), | ||
| portalNode && addEventListener(portalNode, 'focusout', onFocus, true), | ||
| ); |
| React.useEffect(() => { | ||
| if (!portalNode || modal) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // Make sure elements inside the portal element are tabbable only when the | ||
| // portal has already been focused, either by tabbing into a focus trap | ||
| // element outside or using the mouse. | ||
| function onFocus(event: FocusEvent) { | ||
| if (portalNode && event.relatedTarget && isOutsideEvent(event)) { | ||
| if (event.type === 'focusin') { | ||
| if (focusInsideDisabledRef.current) { | ||
| enableFocusInside(portalNode); | ||
| focusInsideDisabledRef.current = false; | ||
| } | ||
| } else { | ||
| disableFocusInside(portalNode); | ||
| focusInsideDisabledRef.current = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Listen to the event on the capture phase so they run before the focus | ||
| // trap elements onFocus prop is called. | ||
| // Guard with `portalNode &&` to defend against concurrent React renders | ||
| // where the effect fires before the ref is attached (issue #4739). | ||
| return mergeCleanups( | ||
| addEventListener(portalNode, 'focusin', onFocus, true), | ||
| addEventListener(portalNode, 'focusout', onFocus, true), | ||
| portalNode && addEventListener(portalNode, 'focusin', onFocus, true), | ||
| portalNode && addEventListener(portalNode, 'focusout', onFocus, true), | ||
| ); | ||
| }, [portalNode, modal]); |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The root cause and fix here seems impossible to me? The closure means the variable can't randomly be Codex analysis agrees:
|
Bug
Under React 18+ concurrent rendering (Next.js App Router with
useTransition/client-side navigation),FloatingPortalthrows:Environment
@base-ui/react: 1.4.1Dialog(used as Sheet) andSelectcomponents inside aSuspenseboundaryRoot cause
In
packages/react/src/floating-ui-react/components/FloatingPortal.tsx, theuseEffectthat attachesfocusin/focusoutlisteners has an early-return guard at the top:Under concurrent React, the effect can fire with a
portalNodethat passes the!portalNodeguard at the start but has since becomenull— orportalNodewas briefly truthy during the transition phase and the effect was scheduled before it reset tonull.The
@base-ui/utils/addEventListenerutility has no null guard:Compare this with the guarded pattern used elsewhere in
FloatingFocusManager.tsx:But the
FloatingPortalcall site does NOT use this pattern:Suggested fix
Align the call site with the guarded pattern already used in
FloatingFocusManager:Or add a null guard to
@base-ui/utils/addEventListener:Impact
Non-fatal console error on every client-side navigation to a page with
Dialog/Selectcomponents. Does not break functionality but pollutes the console and will surface as an error in monitoring tools (Sentry, etc.).