[DevTools] Fix null ref crash in ContextMenu when items list is empty

[everettbu]

Mirror of facebook/react#35929
Original author: fresh3nough

---

Summary

Fixes #35923

When the ContextMenu component renders with an empty items list (or when the portal container is missing), the component returns null and the portal <div ref={ref}> is never mounted. However, the useLayoutEffect still fires and calls repositionToFit(ref.current, ...) where ref.current is null, causing:

TypeError: Cannot read properties of null (reading 'ownerDocument')

This happens in the Timeline profiler view when the user right-clicks on the canvas before hovering over any specific event, resulting in an empty context menu items list being passed to the ContextMenu component.

How did you test this change?

Added contextMenu-test.js with two test cases:

1. Verifies the component does not crash when items is empty (reproduces the exact error from the issue)
2. Verifies the component renders correctly when items are provided

Steps to reproduce the original bug:

1. Open React DevTools
2. Record a profiling session
3. Switch to the Timeline tab
4. Right-click on the canvas area (not on a specific event)

With this fix, the right-click on an empty area no longer crashes.

[greptile-apps]

Greptile Summary

Fixes a TypeError: Cannot read properties of null (reading 'ownerDocument') crash in the DevTools ContextMenu component when the context menu is triggered with an empty items list (e.g., right-clicking the Timeline canvas without hovering over a specific event).

• The root cause was useLayoutEffect running unconditionally and calling repositionToFit(ref.current, ...) even when the component returned null early (empty items or missing portal container), leaving ref.current as null
• The fix extracts the early-return condition into a hideMenu variable, guards the useLayoutEffect body with it, and adds it to the dependency array
• Also replaces the unstable ref = createRef() default parameter with an internal useRef, which is more correct for a per-instance mutable ref that persists across renders
• The ref prop is cleanly removed from ContextMenu's API since it was only used internally; ContextMenuContainer still accepts ref for its useImperativeHandle

Confidence Score: 5/5

• This PR is safe to merge — it's a targeted, well-scoped null-guard fix for a real crash.
• The fix is minimal and correct. The null ref guard in useLayoutEffect directly addresses the reported TypeError. Replacing createRef() with useRef is a strict improvement. The ref prop removal from ContextMenu is clean — ContextMenuContainer still retains its own ref handling via useImperativeHandle. No behavioral regressions or new edge cases are introduced.
• No files require special attention.

Important Files Changed

Filename	Overview
packages/react-devtools-shared/src/devtools/ContextMenu/ContextMenu.js	Guards useLayoutEffect against null ref when menu is hidden (empty items or missing portal). Replaces unstable createRef() default with useRef, adds hideMenu dependency.
packages/react-devtools-shared/src/devtools/ContextMenu/ContextMenuContainer.js	Removes the now-unnecessary ref prop forwarding to ContextMenu, since the inner component manages its own ref internally.

_{Last reviewed commit: b20e929}

[greptile-apps]

_{2 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

packages/react-devtools-shared/src/devtools/ContextMenu/ContextMenu.js, line 111
Incomplete useLayoutEffect dependency array

The effect's closure captures hide, ownerDocument, and position, but only hideMenu is listed as a dependency. This means if position or hide change while the menu is visible, the effect won't re-run — event listeners and repositioning will use stale values.

In practice this is safe here because ContextMenuContainer unmounts the entire ContextMenu when hiding (via shouldShow toggling), so the component never actually re-renders with new values while mounted. But this is fragile — if the parent component's behavior changes, this could become a subtle bug. Consider adding an // eslint-disable-next-line react-hooks/exhaustive-deps comment to document the intentional omission, or adding the full dependency list.

[everettbu]

Upstream PR was closed or merged. Code is synced via branch mirror.