From a96a0f3903ea0a9d45ff7c30a3fd9efe830c4628 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Fri, 15 Aug 2025 12:14:23 -0400 Subject: [PATCH 1/5] Fix fragmentInstance#compareDocumentPosition nesting and portal cases (#34069) Found a couple of issues while integrating FragmentInstance#compareDocumentPosition into Fabric. 1. Basic checks of nested host instances were inaccurate. For example, checking the first child of the first child of the Fragment would not return CONTAINED_BY. 2. Then fixing that logic exposed issues with Portals. The DOM positioning relied on the assumption that the first and last top-level children were in the same order as the Fiber tree. I added additional checks against the parent's position in the DOM, and special cased a portaled Fragment by getting its DOM parent from the child instance, rather than taking the instance from the Fiber return. This should be accurate in more cases. Though its still a guess and I'm not sure yet I've covered every variation of this. Portals are hard to deal with and we may end up having to push more results towards IMPLEMENTATION_SPECIFIC if accuracy is an issue. --- .../src/client/ReactFiberConfigDOM.js | 72 +++++++-- .../__tests__/ReactDOMFragmentRefs-test.js | 139 +++++++++++++++--- .../src/ReactFiberTreeReflection.js | 52 +++++-- 3 files changed, 219 insertions(+), 44 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 633adc3db68d..03a6104e68db 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -38,9 +38,16 @@ import hasOwnProperty from 'shared/hasOwnProperty'; import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion'; import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols'; import { - isFiberContainedBy, + isFiberContainedByFragment, isFiberFollowing, isFiberPreceding, + isFragmentContainedByFiber, + traverseFragmentInstance, + getFragmentParentHostFiber, + getNextSiblingHostFiber, + getInstanceFromHostFiber, + traverseFragmentInstanceDeeply, + fiberIsPortaledIntoHost, } from 'react-reconciler/src/ReactFiberTreeReflection'; export { @@ -63,13 +70,6 @@ import { markNodeAsHoistable, isOwnedInstance, } from './ReactDOMComponentTree'; -import { - traverseFragmentInstance, - getFragmentParentHostFiber, - getNextSiblingHostFiber, - getInstanceFromHostFiber, - traverseFragmentInstanceDeeply, -} from 'react-reconciler/src/ReactFiberTreeReflection'; export {detachDeletedInstance}; import {hasRole} from './DOMAccessibilityRoles'; @@ -3052,13 +3052,13 @@ FragmentInstance.prototype.compareDocumentPosition = function ( } const children: Array = []; traverseFragmentInstance(this._fragmentFiber, collectChildren, children); + const parentHostInstance = + getInstanceFromHostFiber(parentHostFiber); let result = Node.DOCUMENT_POSITION_DISCONNECTED; if (children.length === 0) { // If the fragment has no children, we can use the parent and // siblings to determine a position. - const parentHostInstance = - getInstanceFromHostFiber(parentHostFiber); const parentResult = parentHostInstance.compareDocumentPosition(otherNode); result = parentResult; if (parentHostInstance === otherNode) { @@ -3095,15 +3095,53 @@ FragmentInstance.prototype.compareDocumentPosition = function ( const lastElement = getInstanceFromHostFiber( children[children.length - 1], ); + + // If the fragment has been portaled into another host instance, we need to + // our best guess is to use the parent of the child instance, rather than + // the fiber tree host parent. + const parentHostInstanceFromDOM = fiberIsPortaledIntoHost(this._fragmentFiber) + ? (getInstanceFromHostFiber(children[0]).parentElement: ?Instance) + : parentHostInstance; + + if (parentHostInstanceFromDOM == null) { + return Node.DOCUMENT_POSITION_DISCONNECTED; + } + + // Check if first and last element are actually in the expected document position + // before relying on them as source of truth for other contained elements + const firstElementIsContained = + parentHostInstanceFromDOM.compareDocumentPosition(firstElement) & + Node.DOCUMENT_POSITION_CONTAINED_BY; + const lastElementIsContained = + parentHostInstanceFromDOM.compareDocumentPosition(lastElement) & + Node.DOCUMENT_POSITION_CONTAINED_BY; const firstResult = firstElement.compareDocumentPosition(otherNode); const lastResult = lastElement.compareDocumentPosition(otherNode); + + const otherNodeIsFirstOrLastChild = + (firstElementIsContained && firstElement === otherNode) || + (lastElementIsContained && lastElement === otherNode); + const otherNodeIsFirstOrLastChildDisconnected = + (!firstElementIsContained && firstElement === otherNode) || + (!lastElementIsContained && lastElement === otherNode); + const otherNodeIsWithinFirstOrLastChild = + firstResult & Node.DOCUMENT_POSITION_CONTAINED_BY || + lastResult & Node.DOCUMENT_POSITION_CONTAINED_BY; + const otherNodeIsBetweenFirstAndLastChildren = + firstElementIsContained && + lastElementIsContained && + firstResult & Node.DOCUMENT_POSITION_FOLLOWING && + lastResult & Node.DOCUMENT_POSITION_PRECEDING; + if ( - (firstResult & Node.DOCUMENT_POSITION_FOLLOWING && - lastResult & Node.DOCUMENT_POSITION_PRECEDING) || - otherNode === firstElement || - otherNode === lastElement + otherNodeIsFirstOrLastChild || + otherNodeIsWithinFirstOrLastChild || + otherNodeIsBetweenFirstAndLastChildren ) { result = Node.DOCUMENT_POSITION_CONTAINED_BY; + } else if (otherNodeIsFirstOrLastChildDisconnected) { + // otherNode has been portaled into another container + result = Node.DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC; } else { result = firstResult; } @@ -3141,7 +3179,9 @@ function validateDocumentPositionWithFiberTree( ): boolean { const otherFiber = getClosestInstanceFromNode(otherNode); if (documentPosition & Node.DOCUMENT_POSITION_CONTAINED_BY) { - return !!otherFiber && isFiberContainedBy(fragmentFiber, otherFiber); + return ( + !!otherFiber && isFiberContainedByFragment(otherFiber, fragmentFiber) + ); } if (documentPosition & Node.DOCUMENT_POSITION_CONTAINS) { if (otherFiber === null) { @@ -3149,7 +3189,7 @@ function validateDocumentPositionWithFiberTree( const ownerDocument = otherNode.ownerDocument; return otherNode === ownerDocument || otherNode === ownerDocument.body; } - return isFiberContainedBy(otherFiber, fragmentFiber); + return isFragmentContainedByFiber(fragmentFiber, otherFiber); } if (documentPosition & Node.DOCUMENT_POSITION_PRECEDING) { return ( diff --git a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js index e7e1d053a7a3..521792d62e95 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js @@ -1197,14 +1197,14 @@ describe('FragmentRefs', () => { function Test() { return ( -
-
+
+
-
-
-
+
+
+
-
+
); } @@ -1289,7 +1289,7 @@ describe('FragmentRefs', () => { }, ); - // containerRef preceds and contains the fragment + // containerRef precedes and contains the fragment expectPosition( fragmentRef.current.compareDocumentPosition(containerRef.current), { @@ -1328,7 +1328,7 @@ describe('FragmentRefs', () => { function Test() { return (
-
+
@@ -1491,6 +1491,77 @@ describe('FragmentRefs', () => { ); }); + // @gate enableFragmentRefs + it('handles nested children', async () => { + const fragmentRef = React.createRef(); + const nestedFragmentRef = React.createRef(); + const childARef = React.createRef(); + const childBRef = React.createRef(); + const childCRef = React.createRef(); + document.body.appendChild(container); + const root = ReactDOMClient.createRoot(container); + + function Child() { + return ( +
+ C +
+ ); + } + + function Test() { + return ( + +
+ A +
+ +
+ B +
+ + + + ); + } + + await act(() => root.render()); + + expectPosition( + fragmentRef.current.compareDocumentPosition(childARef.current), + { + preceding: false, + following: false, + contains: false, + containedBy: true, + disconnected: false, + implementationSpecific: false, + }, + ); + expectPosition( + fragmentRef.current.compareDocumentPosition(childBRef.current), + { + preceding: false, + following: false, + contains: false, + containedBy: true, + disconnected: false, + implementationSpecific: false, + }, + ); + expectPosition( + fragmentRef.current.compareDocumentPosition(childCRef.current), + { + preceding: false, + following: false, + contains: false, + containedBy: true, + disconnected: false, + implementationSpecific: false, + }, + ); + }); + // @gate enableFragmentRefs it('returns disconnected for comparison with an unmounted fragment instance', async () => { const fragmentRef = React.createRef(); @@ -1551,11 +1622,11 @@ describe('FragmentRefs', () => { function Test() { return ( -
- {createPortal(
, document.body)} +
+ {createPortal(
, container)} - {createPortal(
, document.body)} -
+ {createPortal(
, container)} +
); @@ -1600,6 +1671,8 @@ describe('FragmentRefs', () => { const childARef = React.createRef(); const childBRef = React.createRef(); const childCRef = React.createRef(); + const childDRef = React.createRef(); + const childERef = React.createRef(); function Test() { const [c, setC] = React.useState(false); @@ -1612,23 +1685,30 @@ describe('FragmentRefs', () => { {createPortal(
- {c ?
: null} + {c ? ( +
+
+
+ ) : null} , document.body, )} {createPortal(

, document.body)} +

); } await act(() => root.render()); - // Due to effect, order is A->B->C - expect(document.body.innerHTML).toBe( - '
' + + // Due to effect, order is E / A->B->C->D + expect(document.body.outerHTML).toBe( + '' + + '
' + '
' + '

' + - '
', + '
' + + '', ); expectPosition( @@ -1642,7 +1722,6 @@ describe('FragmentRefs', () => { implementationSpecific: false, }, ); - expectPosition( fragmentRef.current.compareDocumentPosition(childARef.current), { @@ -1654,6 +1733,7 @@ describe('FragmentRefs', () => { implementationSpecific: false, }, ); + // Contained by in DOM, but following in React tree expectPosition( fragmentRef.current.compareDocumentPosition(childBRef.current), { @@ -1676,6 +1756,29 @@ describe('FragmentRefs', () => { implementationSpecific: false, }, ); + expectPosition( + fragmentRef.current.compareDocumentPosition(childDRef.current), + { + preceding: false, + following: false, + contains: false, + containedBy: true, + disconnected: false, + implementationSpecific: false, + }, + ); + // Preceding DOM but following in React tree + expectPosition( + fragmentRef.current.compareDocumentPosition(childERef.current), + { + preceding: false, + following: false, + contains: false, + containedBy: false, + disconnected: false, + implementationSpecific: true, + }, + ); }); // @gate enableFragmentRefs diff --git a/packages/react-reconciler/src/ReactFiberTreeReflection.js b/packages/react-reconciler/src/ReactFiberTreeReflection.js index 45da707dfea2..49a357adf826 100644 --- a/packages/react-reconciler/src/ReactFiberTreeReflection.js +++ b/packages/react-reconciler/src/ReactFiberTreeReflection.js @@ -26,6 +26,7 @@ import { ActivityComponent, SuspenseComponent, OffscreenComponent, + Fragment, } from './ReactWorkTags'; import {NoFlags, Placement, Hydrating} from './ReactFiberFlags'; @@ -405,6 +406,21 @@ export function getFragmentParentHostFiber(fiber: Fiber): null | Fiber { return null; } +export function fiberIsPortaledIntoHost(fiber: Fiber): boolean { + let foundPortalParent = false; + let parent = fiber.return; + while (parent !== null) { + if (parent.tag === HostPortal) { + foundPortalParent = true; + } + if (parent.tag === HostRoot || parent.tag === HostComponent) { + break; + } + parent = parent.return; + } + return foundPortalParent; +} + export function getInstanceFromHostFiber(fiber: Fiber): I { switch (fiber.tag) { case HostComponent: @@ -443,22 +459,38 @@ function findNextSibling(child: Fiber): boolean { return true; } -export function isFiberContainedBy( - maybeChild: Fiber, - maybeParent: Fiber, +export function isFiberContainedByFragment( + fiber: Fiber, + fragmentFiber: Fiber, ): boolean { - let parent = maybeParent.return; - if (parent === maybeChild || parent === maybeChild.alternate) { - return true; + let current: Fiber | null = fiber; + while (current !== null) { + if ( + current.tag === Fragment && + (current === fragmentFiber || current.alternate === fragmentFiber) + ) { + return true; + } + current = current.return; } - while (parent !== null && parent !== maybeChild) { + return false; +} + +export function isFragmentContainedByFiber( + fragmentFiber: Fiber, + otherFiber: Fiber, +): boolean { + let current: Fiber | null = fragmentFiber; + const fiberHostParent: Fiber | null = + getFragmentParentHostFiber(fragmentFiber); + while (current !== null) { if ( - (parent.tag === HostComponent || parent.tag === HostRoot) && - (parent.return === maybeChild || parent.return === maybeChild.alternate) + (current.tag === HostComponent || current.tag === HostRoot) && + (current === fiberHostParent || current.alternate === fiberHostParent) ) { return true; } - parent = parent.return; + current = current.return; } return false; } From 2ba7b07ce10448cc37d793a50d5ca0999e63aad8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 15 Aug 2025 13:34:07 -0400 Subject: [PATCH 2/5] [DevTools] Compute a min and max range for the currently selected suspense boundary (#34201) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This computes a min and max range for the whole suspense boundary even when selecting a single component so that each component in a boundary has a consistent range. The start of this range is the earliest start of I/O in that boundary or the end of the previous suspense boundary, whatever is earlier. If the end of the previous boundary would make the range large, then we cap it since it's likely that the other boundary was just an independent render. The end of the range is the latest end of I/O in that boundary. If this is smaller than the end of the previous boundary plus the 300ms throttle, then we extend the end. This visualizes what throttling could potentially do if the previous boundary committed right at its end. Ofc, it might not have committed exactly at that time in this render. So this is just showing a potential throttle that could happen. To see actual throttle, you look in the Performance Track. Screenshot 2025-08-14 at 12 41 43 AM We could come up with some annotation to highlight that this is eligible to be throttled in this case. If the lines don't extend to the edge, then it's likely it was throttled. --- .../src/backend/fiber/renderer.js | 71 +++++++++++++++++++ .../src/backend/legacy/renderer.js | 1 + .../src/backend/types.js | 1 + .../react-devtools-shared/src/backendAPI.js | 2 + .../Components/InspectedElementSuspendedBy.js | 7 +- .../src/frontend/types.js | 2 + 6 files changed, 83 insertions(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 1eee4a3ad8a1..b927b1a58fd7 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -5268,6 +5268,18 @@ export function attach( } } + function getNearestSuspenseNode(instance: DevToolsInstance): SuspenseNode { + while (instance.suspenseNode === null) { + if (instance.parent === null) { + throw new Error( + 'There should always be a SuspenseNode parent on a mounted instance.', + ); + } + instance = instance.parent; + } + return instance.suspenseNode; + } + function getNearestMountedDOMNode(publicInstance: Element): null | Element { let domNode: null | Element = publicInstance; while (domNode && !publicInstanceToDevToolsInstanceMap.has(domNode)) { @@ -5556,6 +5568,56 @@ export function attach( return result; } + const FALLBACK_THROTTLE_MS: number = 300; + + function getSuspendedByRange( + suspenseNode: SuspenseNode, + ): null | [number, number] { + let min = Infinity; + let max = -Infinity; + suspenseNode.suspendedBy.forEach((_, ioInfo) => { + if (ioInfo.end > max) { + max = ioInfo.end; + } + if (ioInfo.start < min) { + min = ioInfo.start; + } + }); + const parentSuspenseNode = suspenseNode.parent; + if (parentSuspenseNode !== null) { + let parentMax = -Infinity; + parentSuspenseNode.suspendedBy.forEach((_, ioInfo) => { + if (ioInfo.end > parentMax) { + parentMax = ioInfo.end; + } + }); + // The parent max is theoretically the earlier the parent could've committed. + // Therefore, the theoretical max that the child could be throttled is that plus 300ms. + const throttleTime = parentMax + FALLBACK_THROTTLE_MS; + if (throttleTime > max) { + // If the theoretical throttle time is later than the earliest reveal then we extend + // the max time to show that this is timespan could possibly get throttled. + max = throttleTime; + } + + // We use the end of the previous boundary as the start time for this boundary unless, + // that's earlier than we'd need to expand to the full fallback throttle range. It + // suggests that the parent was loaded earlier than this one. + let startTime = max - FALLBACK_THROTTLE_MS; + if (parentMax > startTime) { + startTime = parentMax; + } + // If the first fetch of this boundary starts before that, then we use that as the start. + if (startTime < min) { + min = startTime; + } + } + if (min < Infinity && max > -Infinity) { + return [min, max]; + } + return null; + } + function getAwaitStackFromHooks( hooks: HooksTree, asyncInfo: ReactAsyncInfo, @@ -6024,6 +6086,10 @@ export function attach( : fiberInstance.suspendedBy.map(info => serializeAsyncInfo(info, fiberInstance, hooks), ); + const suspendedByRange = getSuspendedByRange( + getNearestSuspenseNode(fiberInstance), + ); + return { id: fiberInstance.id, @@ -6086,6 +6152,7 @@ export function attach( : Array.from(componentLogsEntry.warnings.entries()), suspendedBy: suspendedBy, + suspendedByRange: suspendedByRange, // List of owners owners, @@ -6144,6 +6211,9 @@ export function attach( // Things that Suspended this Server Component (use(), awaits and direct child promises) const suspendedBy = virtualInstance.suspendedBy; + const suspendedByRange = getSuspendedByRange( + getNearestSuspenseNode(virtualInstance), + ); return { id: virtualInstance.id, @@ -6196,6 +6266,7 @@ export function attach( : suspendedBy.map(info => serializeAsyncInfo(info, virtualInstance, null), ), + suspendedByRange: suspendedByRange, // List of owners owners, diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index c2c278393602..82a3a8d1905b 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -858,6 +858,7 @@ export function attach( // Not supported in legacy renderers. suspendedBy: [], + suspendedByRange: null, // List of owners owners, diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 55a1bc6532e2..0d2a86beb5b8 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -300,6 +300,7 @@ export type InspectedElement = { // Things that suspended this Instances suspendedBy: Object, // DehydratedData or Array + suspendedByRange: null | [number, number], // List of owners owners: Array | null, diff --git a/packages/react-devtools-shared/src/backendAPI.js b/packages/react-devtools-shared/src/backendAPI.js index db22606377da..dcaec7dc0341 100644 --- a/packages/react-devtools-shared/src/backendAPI.js +++ b/packages/react-devtools-shared/src/backendAPI.js @@ -270,6 +270,7 @@ export function convertInspectedElementBackendToFrontend( errors, warnings, suspendedBy, + suspendedByRange, nativeTag, } = inspectedElementBackend; @@ -313,6 +314,7 @@ export function convertInspectedElementBackendToFrontend( hydratedSuspendedBy == null // backwards compat ? [] : hydratedSuspendedBy.map(backendToFrontendSerializedAsyncInfo), + suspendedByRange, nativeTag, }; diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js index e5e094955887..3527c23b0669 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js @@ -292,7 +292,7 @@ export default function InspectedElementSuspendedBy({ inspectedElement, store, }: Props): React.Node { - const {suspendedBy} = inspectedElement; + const {suspendedBy, suspendedByRange} = inspectedElement; // Skip the section if nothing suspended this component. if (suspendedBy == null || suspendedBy.length === 0) { @@ -306,6 +306,11 @@ export default function InspectedElementSuspendedBy({ let minTime = Infinity; let maxTime = -Infinity; + if (suspendedByRange !== null) { + // The range of the whole suspense boundary. + minTime = suspendedByRange[0]; + maxTime = suspendedByRange[1]; + } for (let i = 0; i < suspendedBy.length; i++) { const asyncInfo: SerializedAsyncInfo = suspendedBy[i]; if (asyncInfo.awaited.start < minTime) { diff --git a/packages/react-devtools-shared/src/frontend/types.js b/packages/react-devtools-shared/src/frontend/types.js index 4c61a8b1e9d7..7c7487b7bd6d 100644 --- a/packages/react-devtools-shared/src/frontend/types.js +++ b/packages/react-devtools-shared/src/frontend/types.js @@ -279,6 +279,8 @@ export type InspectedElement = { // Things that suspended this Instances suspendedBy: Object, + // Minimum start time to maximum end time + a potential (not actual) throttle, within the nearest boundary. + suspendedByRange: null | [number, number], // List of owners owners: Array | null, From 2d98b45d92ee9adc5ec6dbd2a5a270e0fd2607a4 Mon Sep 17 00:00:00 2001 From: "Sebastian \"Sebbie\" Silbermann" Date: Fri, 15 Aug 2025 19:39:59 +0200 Subject: [PATCH 3/5] [DevTools] Fix Suspense boundaries always being marked as not suspended (#34206) --- .../react-devtools-shared/src/backend/fiber/renderer.js | 8 ++++++++ .../react-devtools-shared/src/backend/legacy/renderer.js | 1 + packages/react-devtools-shared/src/backend/types.js | 2 ++ packages/react-devtools-shared/src/backendAPI.js | 2 ++ .../src/devtools/views/Components/InspectedElement.js | 2 +- .../views/Components/InspectedElementSuspenseToggle.js | 4 +--- packages/react-devtools-shared/src/frontend/types.js | 2 ++ 7 files changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index b927b1a58fd7..5e9bcddbfae5 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -6071,6 +6071,11 @@ export function attach( nativeTag = getNativeTag(fiber.stateNode); } + let isSuspended: boolean | null = null; + if (tag === SuspenseComponent) { + isSuspended = memoizedState !== null; + } + const suspendedBy = fiberInstance.suspenseNode !== null ? // If this is a Suspense boundary, then we include everything in the subtree that might suspend @@ -6121,6 +6126,7 @@ export function attach( forceFallbackForFibers.has(fiber) || (fiber.alternate !== null && forceFallbackForFibers.has(fiber.alternate))), + isSuspended: isSuspended, source, @@ -6209,6 +6215,7 @@ export function attach( const componentLogsEntry = componentInfoToComponentLogsMap.get(componentInfo); + const isSuspended = null; // Things that Suspended this Server Component (use(), awaits and direct child promises) const suspendedBy = virtualInstance.suspendedBy; const suspendedByRange = getSuspendedByRange( @@ -6230,6 +6237,7 @@ export function attach( isErrored: false, canToggleSuspense: supportsTogglingSuspense && hasSuspenseBoundary, + isSuspended: isSuspended, source, diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 82a3a8d1905b..592238934f5d 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -836,6 +836,7 @@ export function attach( // Suspense did not exist in legacy versions canToggleSuspense: false, + isSuspended: null, source: null, diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 0d2a86beb5b8..978b546b8353 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -285,6 +285,8 @@ export type InspectedElement = { // Is this Suspense, and can its value be overridden now? canToggleSuspense: boolean, + // If this Element is suspended. Currently only set on Suspense boundaries. + isSuspended: boolean | null, // Does the component have legacy context attached to it. hasLegacyContext: boolean, diff --git a/packages/react-devtools-shared/src/backendAPI.js b/packages/react-devtools-shared/src/backendAPI.js index dcaec7dc0341..eb1b6f6df3ff 100644 --- a/packages/react-devtools-shared/src/backendAPI.js +++ b/packages/react-devtools-shared/src/backendAPI.js @@ -251,6 +251,7 @@ export function convertInspectedElementBackendToFrontend( canToggleError, isErrored, canToggleSuspense, + isSuspended, hasLegacyContext, id, type, @@ -287,6 +288,7 @@ export function convertInspectedElementBackendToFrontend( canToggleError, isErrored, canToggleSuspense, + isSuspended, hasLegacyContext, id, key, diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js index fd068c0ad185..8f1d1cd7586a 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js @@ -113,7 +113,7 @@ export default function InspectedElementWrapper(_: Props): React.Node { element !== null && element.type === ElementTypeSuspense && inspectedElement != null && - inspectedElement.state != null; + inspectedElement.isSuspended; const canToggleError = !hideToggleErrorAction && diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspenseToggle.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspenseToggle.js index 3b445505cbcc..ebb4b5bcd543 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspenseToggle.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspenseToggle.js @@ -30,15 +30,13 @@ export default function InspectedElementSuspenseToggle({ }: Props): React.Node { const {readOnly} = React.useContext(OptionsContext); - const {id, state, type} = inspectedElement; + const {id, isSuspended, type} = inspectedElement; const canToggleSuspense = !readOnly && inspectedElement.canToggleSuspense; if (type !== ElementTypeSuspense) { return null; } - const isSuspended = state !== null; - const toggleSuspense = (path: any, value: boolean) => { const rendererID = store.getRendererIDForElement(id); if (rendererID !== null) { diff --git a/packages/react-devtools-shared/src/frontend/types.js b/packages/react-devtools-shared/src/frontend/types.js index 7c7487b7bd6d..d7d22b9530c9 100644 --- a/packages/react-devtools-shared/src/frontend/types.js +++ b/packages/react-devtools-shared/src/frontend/types.js @@ -264,6 +264,8 @@ export type InspectedElement = { // Is this Suspense, and can its value be overridden now? canToggleSuspense: boolean, + // If this Element is suspended. Currently only set on Suspense boundaries. + isSuspended: boolean | null, // Does the component have legacy context attached to it. hasLegacyContext: boolean, From 8dba9311e581c005af40ca0986155b85e959ae83 Mon Sep 17 00:00:00 2001 From: "Sebastian \"Sebbie\" Silbermann" Date: Fri, 15 Aug 2025 19:40:35 +0200 Subject: [PATCH 4/5] [DevTools] Handle fallback unmount in Suspense update path (#34199) --- .../src/__tests__/store-test.js | 79 +++++++++++++++++++ .../src/backend/fiber/renderer.js | 25 +++--- 2 files changed, 94 insertions(+), 10 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index 87524ffd045b..a3a428a9d98d 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -2696,4 +2696,83 @@ describe('Store', () => { `); }); + + // @reactVersion >= 18.0 + it('can reconcile Suspense in fallback positions', async () => { + let resolveFallback; + const fallbackPromise = new Promise(resolve => { + resolveFallback = resolve; + }); + let resolveContent; + const contentPromise = new Promise(resolve => { + resolveContent = resolve; + }); + + function Component({children, promise}) { + if (promise) { + React.use(promise); + } + return
{children}
; + } + + await actAsync(() => + render( + + Loading fallback... + + }> + + Loading... + + + }> + + done + + , + ), + ); + + expect(store).toMatchInlineSnapshot(` + [root] + ▾ + ▾ + + [shell] + + + `); + + await actAsync(() => { + resolveFallback(); + }); + + expect(store).toMatchInlineSnapshot(` + [root] + ▾ + ▾ + + [shell] + + + `); + + await actAsync(() => { + resolveContent(); + }); + + expect(store).toMatchInlineSnapshot(` + [root] + ▾ + + [shell] + + `); + }); }); diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 5e9bcddbfae5..439d522801a3 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -4736,26 +4736,30 @@ export function attach( ); shouldMeasureSuspenseNode = false; - if (nextFallbackFiber !== null) { + if (prevFallbackFiber !== null || nextFallbackFiber !== null) { const fallbackStashedSuspenseParent = reconcilingParentSuspenseNode; const fallbackStashedSuspensePrevious = previouslyReconciledSiblingSuspenseNode; const fallbackStashedSuspenseRemaining = remainingReconcilingChildrenSuspenseNodes; // Next, we'll pop back out of the SuspenseNode that we added above and now we'll - // reconcile the fallback, reconciling anything by inserting into the parent SuspenseNode. + // reconcile the fallback, reconciling anything in the context of the parent SuspenseNode. // Since the fallback conceptually blocks the parent. reconcilingParentSuspenseNode = stashedSuspenseParent; previouslyReconciledSiblingSuspenseNode = stashedSuspensePrevious; remainingReconcilingChildrenSuspenseNodes = stashedSuspenseRemaining; try { - updateFlags |= updateVirtualChildrenRecursively( - nextFallbackFiber, - null, - prevFallbackFiber, - traceNearestHostComponentUpdate, - 0, - ); + if (nextFallbackFiber === null) { + unmountRemainingChildren(); + } else { + updateFlags |= updateVirtualChildrenRecursively( + nextFallbackFiber, + null, + prevFallbackFiber, + traceNearestHostComponentUpdate, + 0, + ); + } } finally { reconcilingParentSuspenseNode = fallbackStashedSuspenseParent; previouslyReconciledSiblingSuspenseNode = @@ -4763,7 +4767,8 @@ export function attach( remainingReconcilingChildrenSuspenseNodes = fallbackStashedSuspenseRemaining; } - } else if (nextFiber.memoizedState === null) { + } + if (nextFiber.memoizedState === null) { // Measure this Suspense node in case it changed. We don't update the rect while // we're inside a disconnected subtree nor if we are the Suspense boundary that // is suspended. This lets us keep the rectangle of the displayed content while From 45a6532a088432010c40cbcac1fb6b6f8d56dd69 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Fri, 15 Aug 2025 15:07:42 -0400 Subject: [PATCH 5/5] Add compareDocumentPosition to Fabric FragmentInstance (#34103) Stacked on https://github.com/facebook/react/pull/34069 Same basic semantics as the react-dom for determining document position of a Fragment compared to a given node. It's simpler here because we don't have to deal with inserted nodes or portals. So we can skip a bunch of the validation logic. The logic for handling empty fragments is the same so I've split out `compareDocumentPositionForEmptyFragment` into a shared module. There doesn't seem to be a great place to put shared DOM logic between Fabric and DOM configs at the moment. There may be more of this coming as we add more and more DOM APIs to RN. For testing I've written Fantom tests internally which pass the basic cases on this build. The renderer we have configured for Fabric tests in the repo doesn't support the Element APIs we need like `compareDocumentPosition`. --- .../src/client/ReactFiberConfigDOM.js | 45 +++------- .../src/ReactFiberConfigFabric.js | 90 ++++++++++++++++--- packages/shared/ReactDOMFragmentRefShared.js | 58 ++++++++++++ 3 files changed, 145 insertions(+), 48 deletions(-) create mode 100644 packages/shared/ReactDOMFragmentRefShared.js diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 03a6104e68db..568e892ad81a 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -44,7 +44,6 @@ import { isFragmentContainedByFiber, traverseFragmentInstance, getFragmentParentHostFiber, - getNextSiblingHostFiber, getInstanceFromHostFiber, traverseFragmentInstanceDeeply, fiberIsPortaledIntoHost, @@ -70,6 +69,7 @@ import { markNodeAsHoistable, isOwnedInstance, } from './ReactDOMComponentTree'; +import {compareDocumentPositionForEmptyFragment} from 'shared/ReactDOMFragmentRefShared'; export {detachDeletedInstance}; import {hasRole} from './DOMAccessibilityRoles'; @@ -3055,40 +3055,13 @@ FragmentInstance.prototype.compareDocumentPosition = function ( const parentHostInstance = getInstanceFromHostFiber(parentHostFiber); - let result = Node.DOCUMENT_POSITION_DISCONNECTED; if (children.length === 0) { - // If the fragment has no children, we can use the parent and - // siblings to determine a position. - const parentResult = parentHostInstance.compareDocumentPosition(otherNode); - result = parentResult; - if (parentHostInstance === otherNode) { - result = Node.DOCUMENT_POSITION_CONTAINS; - } else { - if (parentResult & Node.DOCUMENT_POSITION_CONTAINED_BY) { - // otherNode is one of the fragment's siblings. Use the next - // sibling to determine if its preceding or following. - const nextSiblingFiber = getNextSiblingHostFiber(this._fragmentFiber); - if (nextSiblingFiber === null) { - result = Node.DOCUMENT_POSITION_PRECEDING; - } else { - const nextSiblingInstance = - getInstanceFromHostFiber(nextSiblingFiber); - const nextSiblingResult = - nextSiblingInstance.compareDocumentPosition(otherNode); - if ( - nextSiblingResult === 0 || - nextSiblingResult & Node.DOCUMENT_POSITION_FOLLOWING - ) { - result = Node.DOCUMENT_POSITION_FOLLOWING; - } else { - result = Node.DOCUMENT_POSITION_PRECEDING; - } - } - } - } - - result |= Node.DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC; - return result; + return compareDocumentPositionForEmptyFragment( + this._fragmentFiber, + parentHostInstance, + otherNode, + getInstanceFromHostFiber, + ); } const firstElement = getInstanceFromHostFiber(children[0]); @@ -3099,8 +3072,9 @@ FragmentInstance.prototype.compareDocumentPosition = function ( // If the fragment has been portaled into another host instance, we need to // our best guess is to use the parent of the child instance, rather than // the fiber tree host parent. + const firstInstance = getInstanceFromHostFiber(children[0]); const parentHostInstanceFromDOM = fiberIsPortaledIntoHost(this._fragmentFiber) - ? (getInstanceFromHostFiber(children[0]).parentElement: ?Instance) + ? (firstInstance.parentElement: ?Instance) : parentHostInstance; if (parentHostInstanceFromDOM == null) { @@ -3133,6 +3107,7 @@ FragmentInstance.prototype.compareDocumentPosition = function ( firstResult & Node.DOCUMENT_POSITION_FOLLOWING && lastResult & Node.DOCUMENT_POSITION_PRECEDING; + let result = Node.DOCUMENT_POSITION_DISCONNECTED; if ( otherNodeIsFirstOrLastChild || otherNodeIsWithinFirstOrLastChild || diff --git a/packages/react-native-renderer/src/ReactFiberConfigFabric.js b/packages/react-native-renderer/src/ReactFiberConfigFabric.js index 82745e0a9979..3f2f0d192a88 100644 --- a/packages/react-native-renderer/src/ReactFiberConfigFabric.js +++ b/packages/react-native-renderer/src/ReactFiberConfigFabric.js @@ -24,6 +24,7 @@ import { import type {Fiber} from 'react-reconciler/src/ReactInternalTypes'; import {HostText} from 'react-reconciler/src/ReactWorkTags'; import { + getFragmentParentHostFiber, getInstanceFromHostFiber, traverseFragmentInstance, } from 'react-reconciler/src/ReactFiberTreeReflection'; @@ -59,6 +60,7 @@ const { } = nativeFabricUIManager; import {getClosestInstanceFromNode} from './ReactFabricComponentTree'; +import {compareDocumentPositionForEmptyFragment} from 'shared/ReactDOMFragmentRefShared'; import { getInspectorDataForViewTag, @@ -87,7 +89,7 @@ const {get: getViewConfigForType} = ReactNativeViewConfigRegistry; let nextReactTag = 2; type InternalInstanceHandle = Object; -type Node = Object; + export type Type = string; export type Props = Object; export type Instance = { @@ -344,6 +346,15 @@ export function getPublicInstanceFromInternalInstanceHandle( return getPublicInstance(elementInstance); } +function getPublicInstanceFromHostFiber(fiber: Fiber): PublicInstance { + const instance = getInstanceFromHostFiber(fiber); + const publicInstance = getPublicInstance(instance); + if (publicInstance == null) { + throw new Error('Expected to find a host node. This is a bug in React.'); + } + return publicInstance; +} + export function prepareForCommit(containerInfo: Container): null | Object { // Noop return null; @@ -610,6 +621,7 @@ export type FragmentInstanceType = { _observers: null | Set, observeUsing: (observer: IntersectionObserver) => void, unobserveUsing: (observer: IntersectionObserver) => void, + compareDocumentPosition: (otherNode: PublicInstance) => number, }; function FragmentInstance(this: FragmentInstanceType, fragmentFiber: Fiber) { @@ -629,12 +641,8 @@ FragmentInstance.prototype.observeUsing = function ( traverseFragmentInstance(this._fragmentFiber, observeChild, observer); }; function observeChild(child: Fiber, observer: IntersectionObserver) { - const instance = getInstanceFromHostFiber(child); - const publicInstance = getPublicInstance(instance); - if (publicInstance == null) { - throw new Error('Expected to find a host node. This is a bug in React.'); - } - // $FlowFixMe[incompatible-call] Element types are behind a flag in RN + const publicInstance = getPublicInstanceFromHostFiber(child); + // $FlowFixMe[incompatible-call] DOM types expect Element observer.observe(publicInstance); return false; } @@ -656,16 +664,72 @@ FragmentInstance.prototype.unobserveUsing = function ( } }; function unobserveChild(child: Fiber, observer: IntersectionObserver) { - const instance = getInstanceFromHostFiber(child); - const publicInstance = getPublicInstance(instance); - if (publicInstance == null) { - throw new Error('Expected to find a host node. This is a bug in React.'); - } - // $FlowFixMe[incompatible-call] Element types are behind a flag in RN + const publicInstance = getPublicInstanceFromHostFiber(child); + // $FlowFixMe[incompatible-call] DOM types expect Element observer.unobserve(publicInstance); return false; } +// $FlowFixMe[prop-missing] +FragmentInstance.prototype.compareDocumentPosition = function ( + this: FragmentInstanceType, + otherNode: PublicInstance, +): number { + const parentHostFiber = getFragmentParentHostFiber(this._fragmentFiber); + if (parentHostFiber === null) { + return Node.DOCUMENT_POSITION_DISCONNECTED; + } + const parentHostInstance = getPublicInstanceFromHostFiber(parentHostFiber); + const children: Array = []; + traverseFragmentInstance(this._fragmentFiber, collectChildren, children); + if (children.length === 0) { + return compareDocumentPositionForEmptyFragment( + this._fragmentFiber, + parentHostInstance, + otherNode, + getPublicInstanceFromHostFiber, + ); + } + + const firstInstance = getPublicInstanceFromHostFiber(children[0]); + const lastInstance = getPublicInstanceFromHostFiber( + children[children.length - 1], + ); + + // $FlowFixMe[incompatible-use] Fabric PublicInstance is opaque + // $FlowFixMe[prop-missing] + const firstResult = firstInstance.compareDocumentPosition(otherNode); + // $FlowFixMe[incompatible-use] Fabric PublicInstance is opaque + // $FlowFixMe[prop-missing] + const lastResult = lastInstance.compareDocumentPosition(otherNode); + + const otherNodeIsFirstOrLastChild = + firstInstance === otherNode || lastInstance === otherNode; + const otherNodeIsWithinFirstOrLastChild = + firstResult & Node.DOCUMENT_POSITION_CONTAINED_BY || + lastResult & Node.DOCUMENT_POSITION_CONTAINED_BY; + const otherNodeIsBetweenFirstAndLastChildren = + firstResult & Node.DOCUMENT_POSITION_FOLLOWING && + lastResult & Node.DOCUMENT_POSITION_PRECEDING; + let result; + if ( + otherNodeIsFirstOrLastChild || + otherNodeIsWithinFirstOrLastChild || + otherNodeIsBetweenFirstAndLastChildren + ) { + result = Node.DOCUMENT_POSITION_CONTAINED_BY; + } else { + result = firstResult; + } + + return result; +}; + +function collectChildren(child: Fiber, collection: Array): boolean { + collection.push(child); + return false; +} + export function createFragmentInstance( fragmentFiber: Fiber, ): FragmentInstanceType { diff --git a/packages/shared/ReactDOMFragmentRefShared.js b/packages/shared/ReactDOMFragmentRefShared.js new file mode 100644 index 000000000000..c46d67d41140 --- /dev/null +++ b/packages/shared/ReactDOMFragmentRefShared.js @@ -0,0 +1,58 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * Shared logic for Fragment Ref operations for DOM and Fabric configs + * + * @flow + */ + +import type {Fiber} from 'react-reconciler/src/ReactInternalTypes'; + +import {getNextSiblingHostFiber} from 'react-reconciler/src/ReactFiberTreeReflection'; + +export function compareDocumentPositionForEmptyFragment( + fragmentFiber: Fiber, + parentHostInstance: TPublicInstance, + otherNode: TPublicInstance, + getPublicInstance: (fiber: Fiber) => TPublicInstance, +): number { + let result; + // If the fragment has no children, we can use the parent and + // siblings to determine a position. + // $FlowFixMe[incompatible-use] Fabric PublicInstance is opaque + // $FlowFixMe[prop-missing] + const parentResult = parentHostInstance.compareDocumentPosition(otherNode); + result = parentResult; + if (parentHostInstance === otherNode) { + result = Node.DOCUMENT_POSITION_CONTAINS; + } else { + if (parentResult & Node.DOCUMENT_POSITION_CONTAINED_BY) { + // otherNode is one of the fragment's siblings. Use the next + // sibling to determine if its preceding or following. + const nextSiblingFiber = getNextSiblingHostFiber(fragmentFiber); + if (nextSiblingFiber === null) { + result = Node.DOCUMENT_POSITION_PRECEDING; + } else { + const nextSiblingInstance = getPublicInstance(nextSiblingFiber); + const nextSiblingResult = + // $FlowFixMe[incompatible-use] Fabric PublicInstance is opaque + // $FlowFixMe[prop-missing] + nextSiblingInstance.compareDocumentPosition(otherNode); + if ( + nextSiblingResult === 0 || + nextSiblingResult & Node.DOCUMENT_POSITION_FOLLOWING + ) { + result = Node.DOCUMENT_POSITION_FOLLOWING; + } else { + result = Node.DOCUMENT_POSITION_PRECEDING; + } + } + } + } + + result |= Node.DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC; + return result; +}