Temp Fragment wrapper for hidden, suspended UI gets treeBaseDuration for all children

Author: Brian Vaughn

packages/react-reconciler/src/ReactFiberBeginWork.js

@@ -1236,11 +1236,14 @@ function updateSuspenseComponent(
         );
 
         if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
-          // Fiber treeBaseDuration must be at least as large as the sum of children treeBaseDurations.
-          // Otherwise the profiler's onRender metrics will be off,
-          // and the DevTools Profiler flamegraph will visually break as well.
-          primaryChildFragment.treeBaseDuration =
-            currentPrimaryChild.treeBaseDuration;
+          // treeBaseDuration is the sum of all the child tree base durations.
+          let treeBaseDuration = 0;
+          let hiddenChild = currentPrimaryChild;
+          while (hiddenChild !== null) {
+            treeBaseDuration += hiddenChild.treeBaseDuration;
+            hiddenChild = hiddenChild.sibling;
+          }
+          primaryChildFragment.treeBaseDuration = treeBaseDuration;
         }
 
         primaryChildFragment.effectTag |= Placement;

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

@@ -232,32 +232,37 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) {
 
     describe('profiler durations', () => {
       let App;
-      let Fallback;
-      let Suspending;
       let onRender;
 
       beforeEach(() => {
         // Order of parameters: id, phase, actualDuration, treeBaseDuration
         onRender = jest.fn();
 
-        Fallback = () => {
+        const Fallback = () => {
           ReactTestRenderer.unstable_yield('Fallback');
-          advanceTimeBy(5);
+          advanceTimeBy(10);
           return 'Loading...';
         };
 
-        Suspending = () => {
+        const Suspending = () => {
           ReactTestRenderer.unstable_yield('Suspending');
           advanceTimeBy(2);
           return <AsyncText ms={1000} text="Loaded" fakeRenderDuration={1} />;
         };
 
+        const NonSuspending = () => {
+          ReactTestRenderer.unstable_yield('NonSuspending');
+          advanceTimeBy(5);
+          return 'NonSuspending';
+        };
+
         App = () => {
           ReactTestRenderer.unstable_yield('App');
           return (
             <Profiler id="root" onRender={onRender}>
               <Suspense maxDuration={500} fallback={<Fallback />}>
                 <Suspending />
+                <NonSuspending />
               </Suspense>
             </Profiler>
           );
@@ -266,30 +271,31 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) {
 
       it('properly accounts for base durations when a suspended times out in a sync tree', () => {
         const root = ReactTestRenderer.create(<App />);
-        expect(root.toJSON()).toEqual('Loading...');
+        expect(root.toJSON()).toEqual(['Loading...']);
         expect(onRender).toHaveBeenCalledTimes(2);
 
-        // Initial mount should be 3ms–
-        // 2ms from Suspending, and 1ms from the AsyncText it renders.
-        expect(onRender.mock.calls[0][2]).toBe(3);
-        expect(onRender.mock.calls[0][3]).toBe(3);
+        // Initial mount should be 8ms–
+        // 2ms from Suspending, 1ms from the AsyncText it renders,
+        // And 5ms from NonSuspending.
+        expect(onRender.mock.calls[0][2]).toBe(8);
+        expect(onRender.mock.calls[0][3]).toBe(8);
 
-        // When the fallback UI is displayed, and the origina UI hidden,
-        // the baseDuration should only include the 5ms spent rendering Fallback,
-        // but the treeBaseDuration should include that and the 3ms spent in Suspending.
-        expect(onRender.mock.calls[1][2]).toBe(5);
-        expect(onRender.mock.calls[1][3]).toBe(8);
+        // When the fallback UI is displayed, and the original UI is hidden,
+        // the baseDuration should only include the 10ms spent rendering Fallback,
+        // but the treeBaseDuration should include that and the 8ms spent in the hidden tree.
+        expect(onRender.mock.calls[1][2]).toBe(10);
+        expect(onRender.mock.calls[1][3]).toBe(18);
 
         jest.advanceTimersByTime(1000);
 
-        expect(root.toJSON()).toEqual('Loaded');
+        expect(root.toJSON()).toEqual(['Loaded', 'NonSuspending']);
         expect(onRender).toHaveBeenCalledTimes(3);
 
         // When the suspending data is resolved and our final UI is rendered,
         // the baseDuration should only include the 1ms re-rendering AsyncText,
-        // but the treeBaseDuration should include that and the 2ms spent rendering Suspending.
+        // but the treeBaseDuration should include the full 8ms spent in the tree.
         expect(onRender.mock.calls[2][2]).toBe(1);
-        expect(onRender.mock.calls[2][3]).toBe(3);
+        expect(onRender.mock.calls[2][3]).toBe(8);
       });
 
       it('properly accounts for base durations when a suspended times out in a concurrent tree', () => {
@@ -301,6 +307,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) {
           'App',
           'Suspending',
           'Suspend! [Loaded]',
+          'NonSuspending',
           'Fallback',
         ]);
         expect(root).toMatchRenderedOutput(null);
@@ -312,19 +319,19 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) {
         expect(onRender).toHaveBeenCalledTimes(1);
 
         // Initial mount only shows the "Loading..." Fallback.
-        // The treeBaseDuration then should be 5ms spent rendering Fallback,
-        // but the actualDuration should include that and the 3ms spent in Suspending.
-        expect(onRender.mock.calls[0][2]).toBe(8);
-        expect(onRender.mock.calls[0][3]).toBe(5);
+        // The treeBaseDuration then should be 10ms spent rendering Fallback,
+        // but the actualDuration should also include the 8ms spent rendering the hidden tree.
+        expect(onRender.mock.calls[0][2]).toBe(18);
+        expect(onRender.mock.calls[0][3]).toBe(10);
 
-        expect(root).toFlushAndYield(['Suspending', 'Loaded']);
-        expect(root).toMatchRenderedOutput('Loaded');
+        expect(root).toFlushAndYield(['Suspending', 'Loaded', 'NonSuspending']);
+        expect(root).toMatchRenderedOutput('LoadedNonSuspending');
         expect(onRender).toHaveBeenCalledTimes(2);
 
         // When the suspending data is resolved and our final UI is rendered,
-        // both times should include the 3ms re-rendering Suspending and AsyncText.
-        expect(onRender.mock.calls[1][2]).toBe(3);
-        expect(onRender.mock.calls[1][3]).toBe(3);
+        // both times should include the 8ms re-rendering Suspending and AsyncText.
+        expect(onRender.mock.calls[1][2]).toBe(8);
+        expect(onRender.mock.calls[1][3]).toBe(8);
       });
     });
   });