Restore selection from last inspected when updating component filters

This uses the tracked path to select the nearest one instead of relying on a specific id still existing since it'll be remounted. It might get filtered so it's best to select nearest path anyway. I need to move the test into inspected element since we rely on the inspected element being changed to track the current selected path.
facebook · Aug 12, 2024 · 06a33e0 · 06a33e0
1 parent d511eaf
commit 06a33e0
Show file tree

Hide file tree

Showing 4 changed files with 155 additions and 86 deletions.
diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
@@ -34,6 +34,8 @@ describe('InspectedElement', () => {
   let SettingsContextController;
   let StoreContext;
   let TreeContextController;
+  let TreeStateContext;
+  let TreeDispatcherContext;
 
   let TestUtilsAct;
   let TestRendererAct;
@@ -73,6 +75,10 @@ describe('InspectedElement', () => {
       require('react-devtools-shared/src/devtools/views/context').StoreContext;
     TreeContextController =
       require('react-devtools-shared/src/devtools/views/Components/TreeContext').TreeContextController;
+    TreeStateContext =
+      require('react-devtools-shared/src/devtools/views/Components/TreeContext').TreeStateContext;
+    TreeDispatcherContext =
+      require('react-devtools-shared/src/devtools/views/Components/TreeContext').TreeDispatcherContext;
 
     // Used by inspectElementAtIndex() helper function
     utils.act(() => {
@@ -3016,4 +3022,132 @@ describe('InspectedElement', () => {
       );
     });
   });
+
+  it('should properly handle when components filters are updated', async () => {
+    const Wrapper = ({children}) => children;
+
+    let state;
+    let dispatch;
+    const Capture = () => {
+      dispatch = React.useContext(TreeDispatcherContext);
+      state = React.useContext(TreeStateContext);
+      return null;
+    };
+
+    function Child({logError = false, logWarning = false}) {
+      if (logError === true) {
+        console.error('test-only: error');
+      }
+      if (logWarning === true) {
+        console.warn('test-only: warning');
+      }
+      return null;
+    }
+
+    async function selectNextErrorOrWarning() {
+      await utils.actAsync(
+        () =>
+          dispatch({type: 'SELECT_NEXT_ELEMENT_WITH_ERROR_OR_WARNING_IN_TREE'}),
+        false,
+      );
+    }
+
+    async function selectPreviousErrorOrWarning() {
+      await utils.actAsync(
+        () =>
+          dispatch({
+            type: 'SELECT_PREVIOUS_ELEMENT_WITH_ERROR_OR_WARNING_IN_TREE',
+          }),
+        false,
+      );
+    }
+
+    withErrorsOrWarningsIgnored(['test-only:'], () =>
+      utils.act(() =>
+        render(
+          <React.Fragment>
+            <Wrapper>
+              <Child logWarning={true} />
+            </Wrapper>
+            <Wrapper>
+              <Wrapper>
+                <Child logWarning={true} />
+              </Wrapper>
+            </Wrapper>
+          </React.Fragment>,
+        ),
+      ),
+    );
+
+    utils.act(() =>
+      TestRenderer.create(
+        <Contexts>
+          <Capture />
+        </Contexts>,
+      ),
+    );
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+         ▾ <Wrapper>
+             <Child> ⚠
+         ▾ <Wrapper>
+           ▾ <Wrapper>
+               <Child> ⚠
+    `);
+
+    await selectNextErrorOrWarning();
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+         ▾ <Wrapper>
+      →      <Child> ⚠
+         ▾ <Wrapper>
+           ▾ <Wrapper>
+               <Child> ⚠
+    `);
+
+    await utils.actAsync(() => {
+      store.componentFilters = [utils.createDisplayNameFilter('Wrapper')];
+    }, false);
+
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+      →    <Child> ⚠
+           <Child> ⚠
+    `);
+
+    await selectNextErrorOrWarning();
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+           <Child> ⚠
+      →    <Child> ⚠
+    `);
+
+    await utils.actAsync(() => {
+      store.componentFilters = [];
+    }, false);
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+         ▾ <Wrapper>
+             <Child> ⚠
+         ▾ <Wrapper>
+           ▾ <Wrapper>
+      →        <Child> ⚠
+    `);
+
+    await selectPreviousErrorOrWarning();
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+         ▾ <Wrapper>
+      →      <Child> ⚠
+         ▾ <Wrapper>
+           ▾ <Wrapper>
+               <Child> ⚠
+    `);
+  });
 });
diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js
@@ -2286,91 +2286,6 @@ describe('TreeListContext', () => {
       `);
     });
 
-    it('should properly handle when components filters are updated', () => {
-      const Wrapper = ({children}) => children;
-
-      withErrorsOrWarningsIgnored(['test-only:'], () =>
-        utils.act(() =>
-          render(
-            <React.Fragment>
-              <Wrapper>
-                <Child logWarning={true} />
-              </Wrapper>
-              <Wrapper>
-                <Wrapper>
-                  <Child logWarning={true} />
-                </Wrapper>
-              </Wrapper>
-            </React.Fragment>,
-          ),
-        ),
-      );
-
-      utils.act(() => TestRenderer.create(<Contexts />));
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-           ▾ <Wrapper>
-               <Child> ⚠
-           ▾ <Wrapper>
-             ▾ <Wrapper>
-                 <Child> ⚠
-      `);
-
-      selectNextErrorOrWarning();
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-           ▾ <Wrapper>
-        →      <Child> ⚠
-           ▾ <Wrapper>
-             ▾ <Wrapper>
-                 <Child> ⚠
-      `);
-
-      utils.act(() => {
-        store.componentFilters = [utils.createDisplayNameFilter('Wrapper')];
-      });
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-        →    <Child> ⚠
-             <Child> ⚠
-      `);
-
-      selectNextErrorOrWarning();
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-             <Child> ⚠
-        →    <Child> ⚠
-      `);
-
-      utils.act(() => {
-        store.componentFilters = [];
-      });
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-           ▾ <Wrapper>
-               <Child> ⚠
-           ▾ <Wrapper>
-             ▾ <Wrapper>
-        →        <Child> ⚠
-      `);
-
-      selectPreviousErrorOrWarning();
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-           ▾ <Wrapper>
-        →      <Child> ⚠
-           ▾ <Wrapper>
-             ▾ <Wrapper>
-                 <Child> ⚠
-      `);
-    });
-
     it('should preserve errors for fibers even if they are filtered out of the tree initially', () => {
       const Wrapper = ({children}) => children;
 

diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js
@@ -794,10 +794,23 @@ export default class Agent extends EventEmitter<{
 
   updateComponentFilters: (componentFilters: Array<ComponentFilter>) => void =
     componentFilters => {
-      for (const rendererID in this._rendererInterfaces) {
+      for (const rendererIDString in this._rendererInterfaces) {
+        const rendererID = +rendererIDString;
         const renderer = ((this._rendererInterfaces[
           (rendererID: any)
         ]: any): RendererInterface);
+        if (this._lastSelectedRendererID === rendererID) {
+          // Changing component filters will unmount and remount the DevTools tree.
+          // Track the last selection's path so we can restore the selection.
+          const path = renderer.getPathForElement(this._lastSelectedElementID);
+          if (path !== null) {
+            renderer.setTrackedPath(path);
+            this._persistedSelection = {
+              rendererID,
+              path,
+            };
+          }
+        }
         renderer.updateComponentFilters(componentFilters);
       }
     };

diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js
@@ -1169,6 +1169,13 @@ export function attach(
       if (alternate) {
         fiberToFiberInstanceMap.set(alternate, newRoot);
       }
+
+      // Before the traversals, remember to start tracking
+      // our path in case we have selection to restore.
+      if (trackedPath !== null) {
+        mightBeOnTrackedPath = true;
+      }
+
       currentRootID = newRoot.id;
       setRootPseudoKey(currentRootID, root.current);
       mountFiberRecursively(root.current, false);