From 06a33e0515f1bd7a9c1edddce476fa77b3d6b4a1 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 11 Aug 2024 21:41:36 -0400 Subject: [PATCH] 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. --- .../src/__tests__/inspectedElement-test.js | 134 ++++++++++++++++++ .../src/__tests__/treeContext-test.js | 85 ----------- .../src/backend/agent.js | 15 +- .../src/backend/fiber/renderer.js | 7 + 4 files changed, 155 insertions(+), 86 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index 6496c020a0ee4..a4cc492c83efc 100644 --- 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( + + + + + + + + + + , + ), + ), + ); + + utils.act(() => + TestRenderer.create( + + + , + ), + ); + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ▾ + ⚠ + ▾ + ▾ + ⚠ + `); + + await selectNextErrorOrWarning(); + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ▾ + → ⚠ + ▾ + ▾ + ⚠ + `); + + await utils.actAsync(() => { + store.componentFilters = [utils.createDisplayNameFilter('Wrapper')]; + }, false); + + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + → ⚠ + ⚠ + `); + + await selectNextErrorOrWarning(); + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ⚠ + → ⚠ + `); + + await utils.actAsync(() => { + store.componentFilters = []; + }, false); + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ▾ + ⚠ + ▾ + ▾ + → ⚠ + `); + + await selectPreviousErrorOrWarning(); + expect(state).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ▾ + → ⚠ + ▾ + ▾ + ⚠ + `); + }); }); diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js index 8bd5a8e17fa0b..fa2031c6b5c8d 100644 --- 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( - - - - - - - - - - , - ), - ), - ); - - utils.act(() => TestRenderer.create()); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - ▾ - ⚠ - ▾ - ▾ - ⚠ - `); - - selectNextErrorOrWarning(); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - ▾ - → ⚠ - ▾ - ▾ - ⚠ - `); - - utils.act(() => { - store.componentFilters = [utils.createDisplayNameFilter('Wrapper')]; - }); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - → ⚠ - ⚠ - `); - - selectNextErrorOrWarning(); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - ⚠ - → ⚠ - `); - - utils.act(() => { - store.componentFilters = []; - }); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - ▾ - ⚠ - ▾ - ▾ - → ⚠ - `); - - selectPreviousErrorOrWarning(); - expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 - [root] - ▾ - → ⚠ - ▾ - ▾ - ⚠ - `); - }); - 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 index a09a05f9fd7ce..38f6b2b1addda 100644 --- 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) => 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 index 5cbdb68ab8632..d1bb0429e7c48 100644 --- 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);