Fix Suspense throttling mechanism

The throttling mechanism for fallbacks should apply to both their
appearance _and_ disappearance.

This was mostly addressed by #26611. See that PR for additional context.

However, a flaw in the implementation is that we only update the the
timestamp used for throttling when the fallback initially appears. We
don't update it when the real content pops in. If lots of content in
separate Suspense trees loads around the same time, you can still
get jank.

The issue is fixed by updating the throttling timestamp whenever the
visibility of a fallback changes. Not just when it appears.

Author: acdlite

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -55,6 +55,7 @@ import {
   enableLegacyHidden,
   enableHostSingletons,
   diffInCommitPhase,
+  alwaysThrottleRetries,
 } from 'shared/ReactFeatureFlags';
 import {
   FunctionComponent,
@@ -2905,17 +2906,35 @@ function commitMutationEffectsOnFiber(
       recursivelyTraverseMutationEffects(root, finishedWork, lanes);
       commitReconciliationEffects(finishedWork);
 
+      // TODO: We should mark a flag on the Suspense fiber itself, rather than
+      // relying on the Offscreen fiber having a flag also being marked. The
+      // reason is that this offscreen fiber might not be part of the work-in-
+      // progress tree! It could have been reused from a previous render. This
+      // doesn't lead to incorrect behavior because we don't rely on the flag
+      // check alone; we also compare the states explicitly below. But for
+      // modeling purposes, we _should_ be able to rely on the flag check alone.
+      // So this is a bit fragile.
+      //
+      // Also, all this logic could/should move to the passive phase so it
+      // doesn't block paint.
       const offscreenFiber: Fiber = (finishedWork.child: any);
-
       if (offscreenFiber.flags & Visibility) {
-        const newState: OffscreenState | null = offscreenFiber.memoizedState;
-        const isHidden = newState !== null;
-        if (isHidden) {
-          const wasHidden =
-            offscreenFiber.alternate !== null &&
-            offscreenFiber.alternate.memoizedState !== null;
-          if (!wasHidden) {
-            // TODO: Move to passive phase
+        // Throttle the appearance and disappearance of Suspense fallbacks.
+        const isShowingFallback =
+          (finishedWork.memoizedState: SuspenseState | null) !== null;
+        const wasShowingFallback =
+          current !== null &&
+          (current.memoizedState: SuspenseState | null) !== null;
+
+        if (alwaysThrottleRetries) {
+          if (isShowingFallback !== wasShowingFallback) {
+            // A fallback is either appearing or disappearing.
+            markCommitTimeOfFallback();
+          }
+        } else {
+          if (isShowingFallback && !wasShowingFallback) {
+            // Old behavior. Only mark when a fallback appears, not when
+            // it disappears.
             markCommitTimeOfFallback();
           }
         }

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -370,8 +370,10 @@ let workInProgressRootConcurrentErrors: Array<CapturedValue<mixed>> | null =
 let workInProgressRootRecoverableErrors: Array<CapturedValue<mixed>> | null =
   null;
 
-// The most recent time we committed a fallback. This lets us ensure a train
-// model where we don't commit new loading states in too quick succession.
+// The most recent time we either committed a fallback, or when a fallback was
+// filled in with the resolved UI. This lets us throttle the appearance of new
+// content as it streams in, to minimize jank.
+// TODO: Think of a better name for this variable?
 let globalMostRecentFallbackTime: number = 0;
 const FALLBACK_THROTTLE_MS: number = 500;
 

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

@@ -1811,6 +1811,102 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     );
   });
 
+  // @gate enableLegacyCache
+  it('throttles content from appearing if a fallback was filled in recently', async () => {
+    function Foo() {
+      Scheduler.log('Foo');
+      return (
+        <>
+          <Suspense fallback={<Text text="Loading A..." />}>
+            <AsyncText text="A" />
+          </Suspense>
+          <Suspense fallback={<Text text="Loading B..." />}>
+            <AsyncText text="B" />
+          </Suspense>
+        </>
+      );
+    }
+
+    ReactNoop.render(<Foo />);
+    // Start rendering
+    await waitForAll([
+      'Foo',
+      'Suspend! [A]',
+      'Loading A...',
+      'Suspend! [B]',
+      'Loading B...',
+    ]);
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span prop="Loading A..." />
+        <span prop="Loading B..." />
+      </>,
+    );
+
+    // Resolve only A. B will still be loading.
+    await act(async () => {
+      await resolveText('A');
+
+      // If we didn't advance the time here, A would not commit; it would
+      // be throttled because the fallback would have appeared too recently.
+      Scheduler.unstable_advanceTime(10000);
+      jest.advanceTimersByTime(10000);
+      await waitForPaint(['A']);
+      expect(ReactNoop).toMatchRenderedOutput(
+        <>
+          <span prop="A" />
+          <span prop="Loading B..." />
+        </>,
+      );
+    });
+
+    // Advance by a small amount of time. For testing purposes, this is meant
+    // to be just under the throttling interval. It's a heurstic, though, so
+    // if we adjust the heuristic we might have to update this test, too.
+    Scheduler.unstable_advanceTime(400);
+    jest.advanceTimersByTime(400);
+
+    // Now resolve B.
+    await act(async () => {
+      await resolveText('B');
+      await waitForPaint(['B']);
+
+      if (gate(flags => flags.alwaysThrottleRetries)) {
+        // B should not commit yet. Even though it's been a long time since its
+        // fallback was shown, it hasn't been long since A appeared. So B's
+        // appearance is throttled to reduce jank.
+        expect(ReactNoop).toMatchRenderedOutput(
+          <>
+            <span prop="A" />
+            <span prop="Loading B..." />
+          </>,
+        );
+
+        // Advance time a little bit more. Now it commits because enough time
+        // has passed.
+        Scheduler.unstable_advanceTime(100);
+        jest.advanceTimersByTime(100);
+        await waitForAll([]);
+        expect(ReactNoop).toMatchRenderedOutput(
+          <>
+            <span prop="A" />
+            <span prop="B" />
+          </>,
+        );
+      } else {
+        // Old behavior, gated until this rolls out at Meta:
+        //
+        // B appears immediately, without being throttled.
+        expect(ReactNoop).toMatchRenderedOutput(
+          <>
+            <span prop="A" />
+            <span prop="B" />
+          </>,
+        );
+      }
+    });
+  });
+
   // TODO: flip to "warns" when this is implemented again.
   // @gate enableLegacyCache
   it('does not warn when a low priority update suspends inside a high priority update for functional components', async () => {