[DevTools] Fix broken commit tree builder for initial operations

Author: eps1lon

packages/react-devtools-shared/src/backend/fiber/renderer.js

@@ -1138,7 +1138,7 @@ export function attach(
   // if any passive effects called console.warn / console.error.
   let needsToFlushComponentLogs = false;
 
-  function bruteForceFlushErrorsAndWarnings() {
+  function bruteForceFlushErrorsAndWarnings(root: FiberInstance) {
     // Refresh error/warning count for all mounted unfiltered Fibers.
     let hasChanges = false;
     // eslint-disable-next-line no-for-of-loops/no-for-of-loops
@@ -1156,7 +1156,7 @@ export function attach(
       }
     }
     if (hasChanges) {
-      flushPendingEvents();
+      flushPendingEvents(root);
     }
   }
 
@@ -1183,7 +1183,7 @@ export function attach(
         updateMostRecentlyInspectedElementIfNecessary(devtoolsInstance.id);
       }
     }
-    flushPendingEvents();
+    flushPendingEvents(null);
   }
 
   function clearConsoleLogsHelper(instanceID: number, type: 'error' | 'warn') {
@@ -1211,7 +1211,7 @@ export function attach(
         }
         const changed = recordConsoleLogs(devtoolsInstance, componentLogsEntry);
         if (changed) {
-          flushPendingEvents();
+          flushPendingEvents(null);
           updateMostRecentlyInspectedElementIfNecessary(devtoolsInstance.id);
         }
       }
@@ -1533,6 +1533,8 @@ export function attach(
     if (isProfiling) {
       // Re-mounting a tree while profiling is in progress might break a lot of assumptions.
       // If necessary, we could support this- but it doesn't seem like a necessary use case.
+      // Supporting change of filters while profiling would require a refactor
+      // to after each root instead of at the end.
       throw Error('Cannot modify filter preferences while profiling');
     }
 
@@ -1647,7 +1649,8 @@ export function attach(
       focusedActivityFilter.activityID = focusedActivityID;
     }
 
-    flushPendingEvents();
+    // We're not profiling so it's safe to flush without a specific root.
+    flushPendingEvents(null);
 
     needsToFlushComponentLogs = false;
   }
@@ -2203,7 +2206,12 @@ export function attach(
     }
   }
 
-  function flushPendingEvents(): void {
+  /**
+   * Allowed to flush pending events without a specific root when:
+   * - pending operations don't record tree mutations e.g. TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS
+   * - not profiling (the commit tree builder requires the root of the mutations)
+   */
+  function flushPendingEvents(root: FiberInstance | null): void {
     if (shouldBailoutWithPendingOperations()) {
       // If we aren't profiling, we can just bail out here.
       // No use sending an empty update over the bridge.
@@ -2222,7 +2230,7 @@ export function attach(
 
     const operations = new Array<number>(
       // Identify which renderer this update is coming from.
-      1 + // [rendererID]
+      2 + // [rendererID, rootFiberID]
         // How big is the string table?
         1 + // [stringTableLength]
         // Then goes the actual string table.
@@ -2245,6 +2253,11 @@ export function attach(
     // Which in turn enables fiber props, states, and hooks to be inspected.
     let i = 0;
     operations[i++] = rendererID;
+    if (root === null) {
+      operations[i++] = -1;
+    } else {
+      operations[i++] = root.id;
+    }
 
     // Now fill in the string table.
     // [stringTableLength, str1Length, ...str1, str2Length, ...str2, ...]
@@ -5740,11 +5753,11 @@ export function attach(
 
         mountFiberRecursively(root.current, false);
 
+        flushPendingEvents(currentRoot);
+
         currentRoot = (null: any);
       });
 
-      flushPendingEvents();
-
       needsToFlushComponentLogs = false;
     }
   }
@@ -5754,7 +5767,7 @@ export function attach(
     // safe to stop calling it from Fiber.
   }
 
-  function handlePostCommitFiberRoot(root: any) {
+  function handlePostCommitFiberRoot(root: FiberRoot) {
     if (isProfiling && rootSupportsProfiling(root)) {
       if (currentCommitProfilingMetadata !== null) {
         const {effectDuration, passiveEffectDuration} =
@@ -5768,12 +5781,18 @@ export function attach(
     }
 
     if (needsToFlushComponentLogs) {
+      const rootInstance = rootToFiberInstanceMap.get(root);
+      if (rootInstance === undefined) {
+        throw new Error(
+          'Should have a root instance for a committed root. This is a bug in React DevTools.',
+        );
+      }
       // We received new logs after commit. I.e. in a passive effect. We need to
       // traverse the tree to find the affected ones. If we just moved the whole
       // tree traversal from handleCommitFiberRoot to handlePostCommitFiberRoot
       // this wouldn't be needed. For now we just brute force check all instances.
       // This is not that common of a case.
-      bruteForceFlushErrorsAndWarnings();
+      bruteForceFlushErrorsAndWarnings(rootInstance);
     }
   }
 
@@ -5870,7 +5889,7 @@ export function attach(
     }
 
     // We're done here.
-    flushPendingEvents();
+    flushPendingEvents(currentRoot);
 
     needsToFlushComponentLogs = false;
 

packages/react-devtools-shared/src/backend/legacy/renderer.js

@@ -525,7 +525,7 @@ export function attach(
 
     const operations = new Array<number>(
       // Identify which renderer this update is coming from.
-      1 + // [rendererID]
+      2 + // [rendererID, rootFiberID]
         // How big is the string table?
         1 + // [stringTableLength]
         // Then goes the actual string table.
@@ -544,6 +544,7 @@ export function attach(
     // Which in turn enables fiber properations, states, and hooks to be inspected.
     let i = 0;
     operations[i++] = rendererID;
+    operations[i++] = rootID;
 
     // Now fill in the string table.
     // [stringTableLength, str1Length, ...str1, str2Length, ...str2, ...]

packages/react-devtools-shared/src/bridge.js

@@ -67,12 +67,6 @@ export const BRIDGE_PROTOCOL: Array<BridgeProtocol> = [
     minNpmVersion: '4.22.0',
     maxNpmVersion: null,
   },
-  // Version 3 no longer sends a rootFiberID at the start
-  {
-    version: 3,
-    minNpmVersion: '7.1.0',
-    maxNpmVersion: null,
-  },
 ];
 
 export const currentBridgeProtocol: BridgeProtocol =

packages/react-devtools-shared/src/devtools/store.js

@@ -1350,9 +1350,8 @@ export default class Store extends EventEmitter<{
     let haveErrorsOrWarningsChanged = false;
     let hasSuspenseTreeChanged = false;
 
-    // The first value is always rendererID
-    let i = 0;
-    const rendererID = operations[i++];
+    // The first two values are always rendererID and rootID
+    const rendererID = operations[0];
 
     const addedElementIDs: Array<number> = [];
     // This is a mapping of removed ID -> parent ID:
@@ -1362,6 +1361,8 @@ export default class Store extends EventEmitter<{
       new Map();
     let nextActivitySliceID: Element['id'] | null = null;
 
+    let i = 2;
+
     // Reassemble the string table.
     const stringTable: Array<string | null> = [
       null, // ID = 0 corresponds to the null string.

packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js

@@ -171,7 +171,7 @@ function updateTree(
     return clonedNode;
   };
 
-  let i = 1;
+  let i = 2;
   let id: number = ((null: any): number);
 
   // Reassemble the string table.

packages/react-devtools-shared/src/utils.js

@@ -221,11 +221,13 @@ export function utfEncodeString(string: string): Array<number> {
 }
 
 export function printOperationsArray(operations: Array<number>) {
-  // The first value is always rendererID
-  let i = 0;
-  const rendererID = operations[i++];
+  // The first two values are always rendererID and rootID
+  const rendererID = operations[0];
+  const rootID = operations[1];
+
+  const logs = [`operations for renderer:${rendererID} and root:${rootID}`];
 
-  const logs = [`operations for renderer:${rendererID}`];
+  let i = 2;
 
   // Reassemble the string table.
   const stringTable: Array<null | string> = [