From 938a27baf52862b49bdee60f3a0750dc41dc26e9 Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Mon, 20 Mar 2023 17:50:42 -0400
Subject: [PATCH 1/2] Bug: Extra render pass when reverting to client render

I noticed while working on a PR that when an error happens during
hydration, and we revert to client rendering, React actually does _two_
additional render passes instead of just one. We didn't notice it
earlier because none of our tests happened to assert on how many renders
it took to recover, only on the final output.

It's possible this extra render pass had other consequences that I'm
not aware of, like messing with some assumption in the recoverable
errors logic.

This adds a test to demonstrate the issue. (One problem is that we
don't have much test coverage of this scenario in the first place, which
likely would have caught this earlier.)
---
 ...DOMServerPartialHydration-test.internal.js | 28 +++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
index 7d98dbd87dc78..8875fa4c0aaea 100644
--- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
+++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
@@ -19,6 +19,7 @@ let ReactFeatureFlags;
 let Suspense;
 let SuspenseList;
 let Offscreen;
+let useSyncExternalStore;
 let act;
 let IdleEventPriority;
 let waitForAll;
@@ -113,6 +114,7 @@ describe('ReactDOMServerPartialHydration', () => {
     Scheduler = require('scheduler');
     Suspense = React.Suspense;
     Offscreen = React.unstable_Offscreen;
+    useSyncExternalStore = React.useSyncExternalStore;
     if (gate(flags => flags.enableSuspenseList)) {
       SuspenseList = React.SuspenseList;
     }
@@ -480,6 +482,26 @@ describe('ReactDOMServerPartialHydration', () => {
   });
 
   it('recovers with client render when server rendered additional nodes at suspense root', async () => {
+    function CheckIfHydrating({children}) {
+      // This is a trick to check whether we're hydrating or not, since React
+      // doesn't expose that information currently except
+      // via useSyncExternalStore.
+      let serverOrClient = '(unknown)';
+      useSyncExternalStore(
+        () => {},
+        () => {
+          serverOrClient = 'Client rendered';
+          return null;
+        },
+        () => {
+          serverOrClient = 'Server rendered';
+          return null;
+        },
+      );
+      Scheduler.log(serverOrClient);
+      return null;
+    }
+
     const ref = React.createRef();
     function App({hasB}) {
       return (
@@ -487,6 +509,7 @@ describe('ReactDOMServerPartialHydration', () => {
           <Suspense fallback="Loading...">
             <span ref={ref}>A</span>
             {hasB ? <span>B</span> : null}
+            <CheckIfHydrating />
           </Suspense>
           <div>Sibling</div>
         </div>
@@ -494,6 +517,7 @@ describe('ReactDOMServerPartialHydration', () => {
     }
 
     const finalHTML = ReactDOMServer.renderToString(<App hasB={true} />);
+    assertLog(['Server rendered']);
 
     const container = document.createElement('div');
     container.innerHTML = finalHTML;
@@ -514,12 +538,12 @@ describe('ReactDOMServerPartialHydration', () => {
       });
     }).toErrorDev('Did not expect server HTML to contain a <span> in <div>');
 
-    jest.runAllTimers();
-
     expect(container.innerHTML).toContain('<span>A</span>');
     expect(container.innerHTML).not.toContain('<span>B</span>');
 
     assertLog([
+      'Server rendered',
+      'Client rendered',
       'There was an error while hydrating this Suspense boundary. ' +
         'Switched to client rendering.',
     ]);

From 31fd817726b3de9b77bb3a97e819d8813282d705 Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Mon, 20 Mar 2023 17:58:47 -0400
Subject: [PATCH 2/2] Fix: No extra render pass when reverting to client render

This fixes the bug described by the previous commit.
---
 packages/react-reconciler/src/ReactFiberCompleteWork.js | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js
index 73a9a455a617f..d67c78103ff00 100644
--- a/packages/react-reconciler/src/ReactFiberCompleteWork.js
+++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js
@@ -85,8 +85,6 @@ import {
   StaticMask,
   MutationMask,
   Passive,
-  Incomplete,
-  ShouldCapture,
   ForceClientRender,
   SuspenseyCommit,
   ScheduleRetry,
@@ -839,7 +837,7 @@ function completeDehydratedSuspenseBoundary(
   ) {
     warnIfUnhydratedTailNodes(workInProgress);
     resetHydrationState();
-    workInProgress.flags |= ForceClientRender | Incomplete | ShouldCapture;
+    workInProgress.flags |= ForceClientRender | DidCapture;
 
     return false;
   }
@@ -1284,7 +1282,7 @@ function completeWork(
             nextState,
           );
         if (!fallthroughToNormalSuspensePath) {
-          if (workInProgress.flags & ShouldCapture) {
+          if (workInProgress.flags & ForceClientRender) {
             // Special case. There were remaining unhydrated nodes. We treat
             // this as a mismatch. Revert to client rendering.
             return workInProgress;