DevTools: Reset cached indices in Store after elements reordered (facebook#22147)

Author: Brian Vaughn

packages/react-devtools-shared/src/__tests__/treeContext-test.js

@@ -2177,6 +2177,111 @@ describe('TreeListContext', () => {
       `);
     });
 
+    it('should update correctly when elements are re-ordered', () => {
+      const container = document.createElement('div');
+      function ErrorOnce() {
+        const didErroRef = React.useRef(false);
+        if (!didErroRef.current) {
+          didErroRef.current = true;
+          console.error('test-only:one-time-error');
+        }
+        return null;
+      }
+      withErrorsOrWarningsIgnored(['test-only:'], () =>
+        utils.act(() =>
+          legacyRender(
+            <React.Fragment>
+              <Child key="A" />
+              <ErrorOnce key="B" />
+              <Child key="C" />
+              <ErrorOnce key="D" />
+            </React.Fragment>,
+            container,
+          ),
+        ),
+      );
+
+      let renderer;
+      utils.act(() => (renderer = TestRenderer.create(<Contexts />)));
+      expect(state).toMatchInlineSnapshot(`
+        ✕ 2, ⚠ 0
+        [root]
+             <Child key="A">
+             <ErrorOnce key="B"> ✕
+             <Child key="C">
+             <ErrorOnce key="D"> ✕
+      `);
+
+      // Select a child
+      selectNextErrorOrWarning();
+      utils.act(() => renderer.update(<Contexts />));
+      expect(state).toMatchInlineSnapshot(`
+        ✕ 2, ⚠ 0
+        [root]
+             <Child key="A">
+        →    <ErrorOnce key="B"> ✕
+             <Child key="C">
+             <ErrorOnce key="D"> ✕
+      `);
+
+      // Re-order the tree and ensure indices are updated.
+      withErrorsOrWarningsIgnored(['test-only:'], () =>
+        utils.act(() =>
+          legacyRender(
+            <React.Fragment>
+              <ErrorOnce key="B" />
+              <Child key="A" />
+              <ErrorOnce key="D" />
+              <Child key="C" />
+            </React.Fragment>,
+            container,
+          ),
+        ),
+      );
+      expect(state).toMatchInlineSnapshot(`
+        ✕ 2, ⚠ 0
+        [root]
+        →    <ErrorOnce key="B"> ✕
+             <Child key="A">
+             <ErrorOnce key="D"> ✕
+             <Child key="C">
+      `);
+
+      // Select the next child and ensure the index doesn't break.
+      selectNextErrorOrWarning();
+      expect(state).toMatchInlineSnapshot(`
+        ✕ 2, ⚠ 0
+        [root]
+             <ErrorOnce key="B"> ✕
+             <Child key="A">
+        →    <ErrorOnce key="D"> ✕
+             <Child key="C">
+      `);
+
+      // Re-order the tree and ensure indices are updated.
+      withErrorsOrWarningsIgnored(['test-only:'], () =>
+        utils.act(() =>
+          legacyRender(
+            <React.Fragment>
+              <ErrorOnce key="D" />
+              <ErrorOnce key="B" />
+              <Child key="A" />
+              <Child key="C" />
+            </React.Fragment>,
+            container,
+          ),
+        ),
+      );
+      expect(state).toMatchInlineSnapshot(`
+        ✕ 2, ⚠ 0
+        [root]
+        →    <ErrorOnce key="D"> ✕
+             <ErrorOnce key="B"> ✕
+             <Child key="A">
+             <Child key="C">
+      `);
+    });
+
     it('should update select and auto-expand parts components within hidden parts of the tree', () => {
       const Wrapper = ({children}) => children;
 

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

@@ -57,6 +57,8 @@ const LOCAL_STORAGE_COLLAPSE_ROOTS_BY_DEFAULT_KEY =
 const LOCAL_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY =
   'React::DevTools::recordChangeDescriptions';
 
+type ErrorAndWarningTuples = Array<{|id: number, index: number|}>;
+
 type Config = {|
   checkBridgeProtocolCompatibility?: boolean,
   isProfiling?: boolean,
@@ -94,7 +96,7 @@ export default class Store extends EventEmitter<{|
   // Computed whenever _errorsAndWarnings Map changes.
   _cachedErrorCount: number = 0;
   _cachedWarningCount: number = 0;
-  _cachedErrorAndWarningTuples: Array<{|id: number, index: number|}> = [];
+  _cachedErrorAndWarningTuples: ErrorAndWarningTuples | null = null;
 
   // Should new nodes be collapsed by default when added to the tree?
   _collapseNodesByDefault: boolean = true;
@@ -510,7 +512,34 @@ export default class Store extends EventEmitter<{|
 
   // Returns a tuple of [id, index]
   getElementsWithErrorsAndWarnings(): Array<{|id: number, index: number|}> {
-    return this._cachedErrorAndWarningTuples;
+    if (this._cachedErrorAndWarningTuples !== null) {
+      return this._cachedErrorAndWarningTuples;
+    } else {
+      const errorAndWarningTuples: ErrorAndWarningTuples = [];
+
+      this._errorsAndWarnings.forEach((_, id) => {
+        const index = this.getIndexOfElementID(id);
+        if (index !== null) {
+          let low = 0;
+          let high = errorAndWarningTuples.length;
+          while (low < high) {
+            const mid = (low + high) >> 1;
+            if (errorAndWarningTuples[mid].index > index) {
+              high = mid;
+            } else {
+              low = mid + 1;
+            }
+          }
+
+          errorAndWarningTuples.splice(low, 0, {id, index});
+        }
+      });
+
+      // Cache for later (at least until the tree changes again).
+      this._cachedErrorAndWarningTuples = errorAndWarningTuples;
+
+      return errorAndWarningTuples;
+    }
   }
 
   getErrorAndWarningCountForElementID(
@@ -1124,6 +1153,9 @@ export default class Store extends EventEmitter<{|
 
     this._revision++;
 
+    // Any time the tree changes (e.g. elements added, removed, or reordered) cached inidices may be invalid.
+    this._cachedErrorAndWarningTuples = null;
+
     if (haveErrorsOrWarningsChanged) {
       let errorCount = 0;
       let warningCount = 0;
@@ -1135,28 +1167,6 @@ export default class Store extends EventEmitter<{|
 
       this._cachedErrorCount = errorCount;
       this._cachedWarningCount = warningCount;
-
-      const errorAndWarningTuples: Array<{|id: number, index: number|}> = [];
-
-      this._errorsAndWarnings.forEach((_, id) => {
-        const index = this.getIndexOfElementID(id);
-        if (index !== null) {
-          let low = 0;
-          let high = errorAndWarningTuples.length;
-          while (low < high) {
-            const mid = (low + high) >> 1;
-            if (errorAndWarningTuples[mid].index > index) {
-              high = mid;
-            } else {
-              low = mid + 1;
-            }
-          }
-
-          errorAndWarningTuples.splice(low, 0, {id, index});
-        }
-      });
-
-      this._cachedErrorAndWarningTuples = errorAndWarningTuples;
     }
 
     if (haveRootsChanged) {
@@ -1187,18 +1197,6 @@ export default class Store extends EventEmitter<{|
       console.groupEnd();
     }
 
-    const indicesOfCachedErrorsOrWarningsAreStale =
-      !haveErrorsOrWarningsChanged &&
-      (addedElementIDs.length > 0 || removedElementIDs.size > 0);
-    if (indicesOfCachedErrorsOrWarningsAreStale) {
-      this._cachedErrorAndWarningTuples.forEach(entry => {
-        const index = this.getIndexOfElementID(entry.id);
-        if (index !== null) {
-          entry.index = index;
-        }
-      });
-    }
-
     this.emit('mutated', [addedElementIDs, removedElementIDs]);
   };