[Suspense] Fix bad loading state not being delayed (#15891)

Fixes a bug where a bad loading state is initially suspended, but a
subsequent update with the same expiration time causes it to
commit immediately.

Author: acdlite

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -323,6 +323,9 @@ export function computeExpirationForFiber(
 
   // If we're in the middle of rendering a tree, do not update at the same
   // expiration time that is already rendering.
+  // TODO: We shouldn't have to do this if the update is on a different root.
+  // Refactor computeExpirationForFiber + scheduleUpdate so we have access to
+  // the root when we check for this condition.
   if (workInProgressRoot !== null && expirationTime === renderExpirationTime) {
     // This is a trick to move this update into a separate batch
     expirationTime -= 1;
@@ -806,8 +809,10 @@ function renderRoot(
     return null;
   }
 
-  if (root.finishedExpirationTime === expirationTime) {
+  if (isSync && root.finishedExpirationTime === expirationTime) {
     // There's already a pending commit at this expiration time.
+    // TODO: This is poorly factored. This case only exists for the
+    // batch.commit() API.
     return commitRoot.bind(null, root);
   }
 

packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js

@@ -2294,4 +2294,51 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       expect(ReactNoop.getChildren()).toEqual([span('D')]);
     });
   });
+
+  it("suspended commit remains suspended even if there's another update at same expiration", async () => {
+    // Regression test
+    function App({text}) {
+      return (
+        <Suspense fallback="Outer fallback">
+          <AsyncText ms={2000} text={text} />
+        </Suspense>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+    await ReactNoop.act(async () => {
+      root.render(<App text="Initial" />);
+    });
+
+    // Resolve initial render
+    await ReactNoop.act(async () => {
+      Scheduler.advanceTime(2000);
+      await advanceTimers(2000);
+    });
+    expect(Scheduler).toHaveYielded([
+      'Suspend! [Initial]',
+      'Promise resolved [Initial]',
+      'Initial',
+    ]);
+    expect(root).toMatchRenderedOutput(<span prop="Initial" />);
+
+    // Suspend B. Since showing a fallback would hide content that's already
+    // visible, it should suspend for a bit without committing.
+    await ReactNoop.act(async () => {
+      root.render(<App text="First update" />);
+
+      expect(Scheduler).toFlushAndYield(['Suspend! [First update]']);
+      // Should not display a fallback
+      expect(root).toMatchRenderedOutput(<span prop="Initial" />);
+    });
+
+    // Suspend A. This should also suspend for a JND.
+    await ReactNoop.act(async () => {
+      root.render(<App text="Second update" />);
+
+      expect(Scheduler).toFlushAndYield(['Suspend! [Second update]']);
+      // Should not display a fallback
+      expect(root).toMatchRenderedOutput(<span prop="Initial" />);
+    });
+  });
 });