Fix failing test added in previous commit

The way we detect a "nested update" is if there's synchronous work
remaining at the end of the commit phase.

Currently this check happens before we synchronously flush the passive
effects. I moved it to after the effects are fired, so that it detects
whether synchronous work was scheduled in that phase.

Author: acdlite

packages/react-dom/src/__tests__/ReactUpdates-test.js

@@ -1686,7 +1686,6 @@ describe('ReactUpdates', () => {
     });
   }
 
-  // @gate experimental || www
   it('prevents infinite update loop triggered by synchronous updates in useEffect', () => {
     // Ignore flushSync warning
     spyOnDev(console, 'error');
@@ -1710,7 +1709,6 @@ describe('ReactUpdates', () => {
     }
 
     const container = document.createElement('div');
-    document.body.appendChild(container);
     const root = ReactDOM.createRoot(container);
     expect(() => {
       ReactDOM.flushSync(() => {

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

@@ -1897,6 +1897,15 @@ function commitRootImpl(root, renderPriorityLevel) {
   remainingLanes = root.pendingLanes;
 
   // Check if there's remaining work on this root
+  // TODO: This is part of the `componentDidCatch` implementation. Its purpose
+  // is to detect whether something might have called setState inside
+  // `componentDidCatch`. The mechanism is known to be flawed because `setState`
+  // inside `componentDidCatch` is itself flawed — that's why we recommend
+  // `getDerivedStateFromError` instead. However, it could be improved by
+  // checking if remainingLanes includes Sync work, instead of whether there's
+  // any work remaining at all (which would also include stuff like Suspense
+  // retries or transitions). It's been like this for a while, though, so fixing
+  // it probably isn't that urgent.
   if (remainingLanes === NoLanes) {
     // If there's no remaining work, we can clear the set of already failed
     // error boundaries.
@@ -1909,23 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) {
     }
   }
 
-  if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
-    if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
-      markNestedUpdateScheduled();
-    }
-
-    // Count the number of times the root synchronously re-renders without
-    // finishing. If there are too many, it indicates an infinite update loop.
-    if (root === rootWithNestedUpdates) {
-      nestedUpdateCount++;
-    } else {
-      nestedUpdateCount = 0;
-      rootWithNestedUpdates = root;
-    }
-  } else {
-    nestedUpdateCount = 0;
-  }
-
   onCommitRootDevTools(finishedWork.stateNode, renderPriorityLevel);
 
   if (enableUpdaterTracking) {
@@ -1964,6 +1956,25 @@ function commitRootImpl(root, renderPriorityLevel) {
     flushPassiveEffects();
   }
 
+  // Read this again, since a passive effect might have updated it
+  remainingLanes = root.pendingLanes;
+  if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
+    if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
+      markNestedUpdateScheduled();
+    }
+
+    // Count the number of times the root synchronously re-renders without
+    // finishing. If there are too many, it indicates an infinite update loop.
+    if (root === rootWithNestedUpdates) {
+      nestedUpdateCount++;
+    } else {
+      nestedUpdateCount = 0;
+      rootWithNestedUpdates = root;
+    }
+  } else {
+    nestedUpdateCount = 0;
+  }
+
   // If layout work was scheduled, flush it now.
   flushSyncCallbacks();
 

packages/react-reconciler/src/ReactFiberWorkLoop.old.js

@@ -1897,6 +1897,15 @@ function commitRootImpl(root, renderPriorityLevel) {
   remainingLanes = root.pendingLanes;
 
   // Check if there's remaining work on this root
+  // TODO: This is part of the `componentDidCatch` implementation. Its purpose
+  // is to detect whether something might have called setState inside
+  // `componentDidCatch`. The mechanism is known to be flawed because `setState`
+  // inside `componentDidCatch` is itself flawed — that's why we recommend
+  // `getDerivedStateFromError` instead. However, it could be improved by
+  // checking if remainingLanes includes Sync work, instead of whether there's
+  // any work remaining at all (which would also include stuff like Suspense
+  // retries or transitions). It's been like this for a while, though, so fixing
+  // it probably isn't that urgent.
   if (remainingLanes === NoLanes) {
     // If there's no remaining work, we can clear the set of already failed
     // error boundaries.
@@ -1909,23 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) {
     }
   }
 
-  if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
-    if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
-      markNestedUpdateScheduled();
-    }
-
-    // Count the number of times the root synchronously re-renders without
-    // finishing. If there are too many, it indicates an infinite update loop.
-    if (root === rootWithNestedUpdates) {
-      nestedUpdateCount++;
-    } else {
-      nestedUpdateCount = 0;
-      rootWithNestedUpdates = root;
-    }
-  } else {
-    nestedUpdateCount = 0;
-  }
-
   onCommitRootDevTools(finishedWork.stateNode, renderPriorityLevel);
 
   if (enableUpdaterTracking) {
@@ -1964,6 +1956,25 @@ function commitRootImpl(root, renderPriorityLevel) {
     flushPassiveEffects();
   }
 
+  // Read this again, since a passive effect might have updated it
+  remainingLanes = root.pendingLanes;
+  if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
+    if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
+      markNestedUpdateScheduled();
+    }
+
+    // Count the number of times the root synchronously re-renders without
+    // finishing. If there are too many, it indicates an infinite update loop.
+    if (root === rootWithNestedUpdates) {
+      nestedUpdateCount++;
+    } else {
+      nestedUpdateCount = 0;
+      rootWithNestedUpdates = root;
+    }
+  } else {
+    nestedUpdateCount = 0;
+  }
+
   // If layout work was scheduled, flush it now.
   flushSyncCallbacks();