[Fiber] SuspenseList with "hidden" tail row should "catch" suspense (#35042)

Normally if you suspend in a SuspenseList row above a Suspense boundary
in that row, it'll suspend the parent. Which can itself delay the commit
or resuspend a parent boundary. That's because SuspenseList mostly just
coordinates the state of the inner boundaries and isn't a boundary
itself.

However, for tail "hidden" and "collapsed" this is not quite the case
because the rows themselves can avoid being rendered.

In the case of "collapsed" we require at least one Suspense boundary
above to have successfully rendered before committing the list because
the idea of this mode is that you should at least always show some
indicator that things are still loading. Since we'd never try the next
one after that at all, this just works. Expect there was an unrelated
bug that meant that "suspend with delay" on a Retry didn't suspend the
commit. This caused a scenario were it'd allow a commit proceed when it
shouldn't. So I fixed that too. The counter intuitive thing here is that
we won't actually show a previous completed row if the loading state of
the next row is still loading.

For tail "hidden" it's a little different because we don't actually
require any loading indicator at all to be shown while it's loading. If
we attempt a row and it suspends, we can just hide it (and the rest) and
move to commit. Therefore this implements a path where if all the rest
of the tail are new mounts (we wouldn't be required to unmount any
existing boundaries) then we can treat the SuspenseList boundary itself
as "catching" the suspense. This is more coherent semantics since any
future row that we didn't attempt also wouldn't resuspend the parent.

This allows simple cases like `<SuspenseList>{list}</SuspenseList>` to
stream in each row without any indicator and no need for Suspense
boundaries.

packages/react-reconciler/src/ReactFiberBeginWork.js

@@ -3397,6 +3397,13 @@ function updateSuspenseListComponent(
 
   let suspenseContext: SuspenseContext = suspenseStackCursor.current;
 
+  if (workInProgress.flags & DidCapture) {
+    // This is the second pass after having suspended in a row. Proceed directly
+    // to the complete phase.
+    pushSuspenseListContext(workInProgress, suspenseContext);
+    return null;
+  }
+
   const shouldForceFallback = hasSuspenseListContext(
     suspenseContext,
     (ForceSuspenseFallback: SuspenseContext),
@@ -4011,6 +4018,14 @@ function attemptEarlyBailoutIfNoScheduledUpdate(
       break;
     }
     case SuspenseListComponent: {
+      if (workInProgress.flags & DidCapture) {
+        // Second pass caught.
+        return updateSuspenseListComponent(
+          current,
+          workInProgress,
+          renderLanes,
+        );
+      }
       const didSuspendBefore = (current.flags & DidCapture) !== NoFlags;
 
       let hasChildWork = includesSomeLane(

packages/react-reconciler/src/ReactFiberCompleteWork.js

@@ -138,6 +138,7 @@ import {
   popSuspenseListContext,
   popSuspenseHandler,
   pushSuspenseListContext,
+  pushSuspenseListCatch,
   setShallowSuspenseListContext,
   ForceSuspenseFallback,
   setDefaultShallowSuspenseListContext,
@@ -765,6 +766,17 @@ function cutOffTailIfNeeded(
   }
 }
 
+function isOnlyNewMounts(tail: Fiber): boolean {
+  let fiber: null | Fiber = tail;
+  while (fiber !== null) {
+    if (fiber.alternate !== null) {
+      return false;
+    }
+    fiber = fiber.sibling;
+  }
+  return true;
+}
+
 function bubbleProperties(completedWork: Fiber) {
   const didBailout =
     completedWork.alternate !== null &&
@@ -1855,7 +1867,10 @@ function completeWork(
       if (renderState.tail !== null) {
         // We still have tail rows to render.
         // Pop a row.
+        // TODO: Consider storing the first of the new mount tail in the state so
+        // that we don't have to recompute this for every row in the list.
         const next = renderState.tail;
+        const onlyNewMounts = isOnlyNewMounts(next);
         renderState.rendering = next;
         renderState.tail = next.sibling;
         renderState.renderingStartTime = now();
@@ -1874,7 +1889,26 @@ function completeWork(
           suspenseContext =
             setDefaultShallowSuspenseListContext(suspenseContext);
         }
-        pushSuspenseListContext(workInProgress, suspenseContext);
+        if (
+          renderState.tailMode === 'visible' ||
+          renderState.tailMode === 'collapsed' ||
+          !onlyNewMounts ||
+          // TODO: While hydrating, we still let it suspend the parent. Tail mode hidden has broken
+          // hydration anyway right now but this preserves the previous semantics out of caution.
+          // Once proper hydration is implemented, this special case should be removed as it should
+          // never be needed.
+          getIsHydrating()
+        ) {
+          pushSuspenseListContext(workInProgress, suspenseContext);
+        } else {
+          // If we are rendering in 'hidden' (default) tail mode, then we if we suspend in the
+          // tail itself, we can delete it rather than suspend the parent. So we act as a catch in that
+          // case. For 'collapsed' we need to render at least one in suspended state, after which we'll
+          // have cut off the rest to never attempt it so it never hits this case.
+          // If this is an updated node, we cannot delete it from the tail so it's effectively visible.
+          // As a consequence, if it resuspends it actually suspends the parent by taking the other path.
+          pushSuspenseListCatch(workInProgress, suspenseContext);
+        }
         // Do a pass over the next row.
         if (getIsHydrating()) {
           // Re-apply tree fork since we popped the tree fork context in the beginning of this function.

packages/react-reconciler/src/ReactFiberSuspenseContext.js

@@ -48,9 +48,10 @@ export function pushPrimaryTreeSuspenseHandler(handler: Fiber): void {
   // Shallow Suspense context fields, like ForceSuspenseFallback, should only be
   // propagated a single level. For example, when ForceSuspenseFallback is set,
   // it should only force the nearest Suspense boundary into fallback mode.
-  pushSuspenseListContext(
-    handler,
+  push(
+    suspenseStackCursor,
     setDefaultShallowSuspenseListContext(suspenseStackCursor.current),
+    handler,
   );
 
   // Experimental feature: Some Suspense boundaries are marked as having an
@@ -113,7 +114,7 @@ export function pushDehydratedActivitySuspenseHandler(fiber: Fiber): void {
   // Reuse the current value on the stack.
   // TODO: We can avoid needing to push here by by forking popSuspenseHandler
   // into separate functions for Activity, Suspense and Offscreen.
-  pushSuspenseListContext(fiber, suspenseStackCursor.current);
+  push(suspenseStackCursor, suspenseStackCursor.current, fiber);
   push(suspenseHandlerStackCursor, fiber, fiber);
   if (shellBoundary === null) {
     // We can contain any suspense inside the Activity boundary.
@@ -127,7 +128,7 @@ export function pushOffscreenSuspenseHandler(fiber: Fiber): void {
     // Reuse the current value on the stack.
     // TODO: We can avoid needing to push here by by forking popSuspenseHandler
     // into separate functions for Activity, Suspense and Offscreen.
-    pushSuspenseListContext(fiber, suspenseStackCursor.current);
+    push(suspenseStackCursor, suspenseStackCursor.current, fiber);
     push(suspenseHandlerStackCursor, fiber, fiber);
     if (shellBoundary === null) {
       // We're rendering hidden content. If it suspends, we can handle it by
@@ -141,7 +142,7 @@ export function pushOffscreenSuspenseHandler(fiber: Fiber): void {
 }
 
 export function reuseSuspenseHandlerOnStack(fiber: Fiber) {
-  pushSuspenseListContext(fiber, suspenseStackCursor.current);
+  push(suspenseStackCursor, suspenseStackCursor.current, fiber);
   push(suspenseHandlerStackCursor, getSuspenseHandler(), fiber);
 }
 
@@ -155,7 +156,7 @@ export function popSuspenseHandler(fiber: Fiber): void {
     // Popping back into the shell.
     shellBoundary = null;
   }
-  popSuspenseListContext(fiber);
+  pop(suspenseStackCursor, fiber);
 }
 
 // SuspenseList context
@@ -201,9 +202,32 @@ export function pushSuspenseListContext(
   fiber: Fiber,
   newContext: SuspenseContext,
 ): void {
+  // Push the current handler in this case since we're not catching at the SuspenseList
+  // for typical rows.
+  const handlerOnStack = suspenseHandlerStackCursor.current;
+  push(suspenseHandlerStackCursor, handlerOnStack, fiber);
   push(suspenseStackCursor, newContext, fiber);
 }
 
+export function pushSuspenseListCatch(
+  fiber: Fiber,
+  newContext: SuspenseContext,
+): void {
+  // In this case we do want to handle catching suspending on the actual boundary itself.
+  // This is used for rows that are allowed to be hidden anyway.
+  push(suspenseHandlerStackCursor, fiber, fiber);
+  push(suspenseStackCursor, newContext, fiber);
+  if (shellBoundary === null) {
+    // We can contain the effects to hiding the current row.
+    shellBoundary = fiber;
+  }
+}
+
 export function popSuspenseListContext(fiber: Fiber): void {
   pop(suspenseStackCursor, fiber);
+  pop(suspenseHandlerStackCursor, fiber);
+  if (shellBoundary === fiber) {
+    // Popping back into the shell.
+    shellBoundary = null;
+  }
 }

packages/react-reconciler/src/ReactFiberThrow.js

@@ -27,6 +27,7 @@ import {
   ActivityComponent,
   SuspenseComponent,
   OffscreenComponent,
+  SuspenseListComponent,
 } from './ReactWorkTags';
 import {
   DidCapture,
@@ -400,7 +401,8 @@ function throwException(
       if (suspenseBoundary !== null) {
         switch (suspenseBoundary.tag) {
           case ActivityComponent:
-          case SuspenseComponent: {
+          case SuspenseComponent:
+          case SuspenseListComponent: {
             // If this suspense/activity boundary is not already showing a fallback, mark
             // the in-progress render as suspended. We try to perform this logic
             // as soon as soon as possible during the render phase, so the work
@@ -561,6 +563,13 @@ function throwException(
     // Instead of surfacing the error, find the nearest Suspense boundary
     // and render it again without hydration.
     if (hydrationBoundary !== null) {
+      if (__DEV__) {
+        if (hydrationBoundary.tag === SuspenseListComponent) {
+          console.error(
+            'SuspenseList should never catch while hydrating. This is a bug in React.',
+          );
+        }
+      }
       if ((hydrationBoundary.flags & ShouldCapture) === NoFlags) {
         // Set a flag to indicate that we should try rendering the normal
         // children again, not the fallback.

packages/react-reconciler/src/ReactFiberUnwindWork.js

@@ -11,7 +11,10 @@ import type {ReactContext} from 'shared/ReactTypes';
 import type {Fiber, FiberRoot} from './ReactInternalTypes';
 import type {Lanes} from './ReactFiberLane';
 import type {ActivityState} from './ReactFiberActivityComponent';
-import type {SuspenseState} from './ReactFiberSuspenseComponent';
+import type {
+  SuspenseState,
+  SuspenseListRenderState,
+} from './ReactFiberSuspenseComponent';
 import type {Cache} from './ReactFiberCacheComponent';
 import type {TracingMarkerInstance} from './ReactFiberTracingMarkerComponent';
 
@@ -31,7 +34,7 @@ import {
   CacheComponent,
   TracingMarkerComponent,
 } from './ReactWorkTags';
-import {DidCapture, NoFlags, ShouldCapture} from './ReactFiberFlags';
+import {DidCapture, NoFlags, ShouldCapture, Update} from './ReactFiberFlags';
 import {NoMode, ProfileMode} from './ReactTypeOfMode';
 import {
   enableProfilerTimer,
@@ -180,8 +183,27 @@ function unwindWork(
     }
     case SuspenseListComponent: {
       popSuspenseListContext(workInProgress);
-      // SuspenseList doesn't actually catch anything. It should've been
+      // SuspenseList doesn't normally catch anything. It should've been
       // caught by a nested boundary. If not, it should bubble through.
+      const flags = workInProgress.flags;
+      if (flags & ShouldCapture) {
+        workInProgress.flags = (flags & ~ShouldCapture) | DidCapture;
+        // If we caught something on the SuspenseList itself it's because
+        // we want to ignore something. Re-enter the cycle and handle it
+        // in the complete phase.
+        const renderState: null | SuspenseListRenderState =
+          workInProgress.memoizedState;
+        if (renderState !== null) {
+          // Cut off any remaining tail work and don't commit the rendering one.
+          // This assumes that we have already confirmed that none of these are
+          // already mounted.
+          renderState.rendering = null;
+          renderState.tail = null;
+        }
+        // Schedule the commit phase to attach retry listeners.
+        workInProgress.flags |= Update;
+        return workInProgress;
+      }
       return null;
     }
     case HostPortal:

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -1355,7 +1355,7 @@ function finishConcurrentRender(
       throw new Error('Root did not complete. This is a bug in React.');
     }
     case RootSuspendedWithDelay: {
-      if (!includesOnlyTransitions(lanes)) {
+      if (!includesOnlyTransitions(lanes) && !includesOnlyRetries(lanes)) {
         // Commit the placeholder.
         break;
       }

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

@@ -2346,6 +2346,227 @@ describe('ReactSuspenseList', () => {
     );
   });
 
+  // @gate enableSuspenseList
+  it('reveals "hidden" rows one by one without suspense boundaries', async () => {
+    const A = createAsyncText('A');
+    const B = createAsyncText('B');
+    const C = createAsyncText('C');
+
+    function Foo() {
+      return (
+        <SuspenseList revealOrder="forwards" tail="hidden">
+          <div>
+            <A />
+          </div>
+          <B />
+          <C />
+        </SuspenseList>
+      );
+    }
+
+    ReactNoop.render(
+      <Suspense fallback="Loading root">
+        <Foo />
+      </Suspense>,
+    );
+
+    await waitForAll(['Suspend! [A]']);
+
+    // We can commit without any rows at all leaving empty.
+    expect(ReactNoop).toMatchRenderedOutput(null);
+
+    await act(() => A.resolve());
+    assertLog(['A', 'Suspend! [B]']);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <div>
+        <span>A</span>
+      </div>,
+    );
+
+    await act(() => B.resolve());
+    assertLog(['B', 'Suspend! [C]']);
+
+    // Incremental loading is suspended.
+    jest.advanceTimersByTime(500);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <div>
+          <span>A</span>
+        </div>
+        <span>B</span>
+      </>,
+    );
+
+    await act(() => C.resolve());
+    assertLog(['C']);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <div>
+          <span>A</span>
+        </div>
+        <span>B</span>
+        <span>C</span>
+      </>,
+    );
+  });
+
+  // @gate enableSuspenseList
+  it('preserves already mounted rows when a new hidden on is inserted in the tail', async () => {
+    const B = createAsyncText('B');
+    const C = createAsyncText('C');
+
+    let count = 0;
+    function MountCount({children}) {
+      // This component should only mount once.
+      React.useLayoutEffect(() => {
+        count++;
+      }, []);
+      return children;
+    }
+
+    function Foo({insert}) {
+      return (
+        <SuspenseList
+          revealOrder="forwards"
+          tail={insert ? 'hidden' : 'visible'}>
+          <Text text="A" />
+          {insert ? <B /> : null}
+          <MountCount>
+            <Suspense fallback={<Text text="Loading C" />}>
+              <C />
+            </Suspense>
+          </MountCount>
+        </SuspenseList>
+      );
+    }
+
+    await act(() => {
+      ReactNoop.render(<Foo insert={false} />);
+    });
+    assertLog(['A', 'Suspend! [C]', 'Loading C', 'Suspend! [C]']);
+
+    expect(count).toBe(1);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span>A</span>
+        <span>Loading C</span>
+      </>,
+    );
+
+    await act(() => {
+      ReactNoop.render(<Foo insert={true} />);
+    });
+
+    assertLog(['A', 'Suspend! [B]', 'A', 'Suspend! [B]']);
+
+    expect(count).toBe(1);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span>A</span>
+        <span>Loading C</span>
+      </>,
+    );
+
+    await act(async () => {
+      await B.resolve();
+      await C.resolve();
+    });
+
+    assertLog(['A', 'B', 'C']);
+
+    expect(count).toBe(1);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span>A</span>
+        <span>B</span>
+        <span>C</span>
+      </>,
+    );
+  });
+
+  // @gate enableSuspenseList
+  it('reveals "collapsed" rows one by one after the first without boundaries', async () => {
+    const A = createAsyncText('A');
+    const B = createAsyncText('B');
+    const C = createAsyncText('C');
+
+    function Foo() {
+      return (
+        <SuspenseList revealOrder="forwards" tail="collapsed">
+          <A />
+          <Suspense fallback={<Text text="Loading B" />}>
+            <B />
+          </Suspense>
+          <C />
+        </SuspenseList>
+      );
+    }
+
+    await act(async () => {
+      ReactNoop.render(
+        <Suspense fallback="Loading root">
+          <Foo />
+        </Suspense>,
+      );
+      await waitForAll(['Suspend! [A]', 'Suspend! [A]']);
+    });
+
+    // The root is still blocked on the first row.
+    expect(ReactNoop).toMatchRenderedOutput('Loading root');
+
+    await A.resolve();
+
+    await waitForAll(['A', 'Suspend! [B]', 'Loading B']);
+
+    // Incremental loading is suspended.
+    jest.advanceTimersByTime(500);
+
+    // Because we have a Suspense boundary that can commit we can now unblock the rest.
+    // If it wasn't a boundary then we couldn't make progress because it would commit
+    // without any loading state.
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span>A</span>
+        <span>Loading B</span>
+      </>,
+    );
+
+    await act(() => B.resolve());
+    assertLog(['B', 'Suspend! [C]', 'B', 'Suspend! [C]']);
+
+    // Incremental loading is suspended.
+    jest.advanceTimersByTime(500);
+
+    // Surprisingly unsuspending B actually causes the parent to resuspend
+    // because C is now unblocked which resuspends the parent. Preventing the
+    // Retry from committing. That's because we don't want to commit into a
+    // state that doesn't have any loading indicators at all. That's what
+    // "collapsed" is for. To ensure there's always a loading indicator.
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span>A</span>
+        <span>Loading B</span>
+      </>,
+    );
+
+    await act(() => C.resolve());
+    assertLog(['B', 'C']);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span>A</span>
+        <span>B</span>
+        <span>C</span>
+      </>,
+    );
+  });
+
   // @gate enableSuspenseList
   it('eventually resolves a nested forwards suspense list', async () => {
     const B = createAsyncText('B');