From 2a879cdc95228b1b3b4cdc81cfc04599716b5562 Mon Sep 17 00:00:00 2001 From: "Sebastian \"Sebbie\" Silbermann" Date: Fri, 6 Feb 2026 15:16:23 +0100 Subject: [PATCH 1/2] [DevTools] Fix broken commit tree builder for initial operations (#35710) --- .../src/backend/fiber/renderer.js | 41 ++++++++++++------- .../react-devtools-shared/src/constants.js | 2 +- .../src/devtools/store.js | 40 ------------------ .../views/Profiler/CommitTreeBuilder.js | 4 -- packages/react-devtools-shared/src/utils.js | 7 ---- 5 files changed, 28 insertions(+), 66 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 289bad6f329b..bc1b2a590d59 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/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 flush 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. @@ -2245,11 +2253,10 @@ export function attach( // Which in turn enables fiber props, states, and hooks to be inspected. let i = 0; operations[i++] = rendererID; - if (currentRoot === null) { - // TODO: This is not always safe so this field is probably not needed. + if (root === null) { operations[i++] = -1; } else { - operations[i++] = currentRoot.id; + operations[i++] = root.id; } // Now fill in the string table. @@ -5746,11 +5753,11 @@ export function attach( mountFiberRecursively(root.current, false); + flushPendingEvents(currentRoot); + currentRoot = (null: any); }); - flushPendingEvents(); - needsToFlushComponentLogs = false; } } @@ -5760,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} = @@ -5774,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); } } @@ -5876,7 +5889,7 @@ export function attach( } // We're done here. - flushPendingEvents(); + flushPendingEvents(currentRoot); needsToFlushComponentLogs = false; diff --git a/packages/react-devtools-shared/src/constants.js b/packages/react-devtools-shared/src/constants.js index c965282e60d6..53488cc2f454 100644 --- a/packages/react-devtools-shared/src/constants.js +++ b/packages/react-devtools-shared/src/constants.js @@ -22,7 +22,7 @@ export const TREE_OPERATION_REMOVE = 2; export const TREE_OPERATION_REORDER_CHILDREN = 3; export const TREE_OPERATION_UPDATE_TREE_BASE_DURATION = 4; export const TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS = 5; -export const TREE_OPERATION_REMOVE_ROOT = 6; +// Removed `TREE_OPERATION_REMOVE_ROOT` export const TREE_OPERATION_SET_SUBTREE_MODE = 7; export const SUSPENSE_TREE_OPERATION_ADD = 8; export const SUSPENSE_TREE_OPERATION_REMOVE = 9; diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index 3e2dde5fea19..45ced5d4fdd3 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -16,7 +16,6 @@ import { PROFILING_FLAG_PERFORMANCE_TRACKS_SUPPORT, TREE_OPERATION_ADD, TREE_OPERATION_REMOVE, - TREE_OPERATION_REMOVE_ROOT, TREE_OPERATION_REORDER_CHILDREN, TREE_OPERATION_SET_SUBTREE_MODE, TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS, @@ -1644,45 +1643,6 @@ export default class Store extends EventEmitter<{ break; } - case TREE_OPERATION_REMOVE_ROOT: { - i += 1; - - const id = operations[1]; - - if (__DEBUG__) { - debug(`Remove root ${id}`); - } - - const recursivelyDeleteElements = (elementID: number) => { - const element = this._idToElement.get(elementID); - this._idToElement.delete(elementID); - if (element) { - // Mostly for Flow's sake - for (let index = 0; index < element.children.length; index++) { - recursivelyDeleteElements(element.children[index]); - } - } - }; - - const root = this._idToElement.get(id); - if (root === undefined) { - this._throwAndEmitError( - Error( - `Cannot remove root "${id}": no matching node was found in the Store.`, - ), - ); - - break; - } - - recursivelyDeleteElements(id); - - this._rootIDToCapabilities.delete(id); - this._rootIDToRendererID.delete(id); - this._roots = this._roots.filter(rootID => rootID !== id); - this._weightAcrossRoots -= root.weight; - break; - } case TREE_OPERATION_REORDER_CHILDREN: { const id = operations[i + 1]; const numChildren = operations[i + 2]; diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js b/packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js index 9a2c3fa3056c..02ecc98ec6d1 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js @@ -11,7 +11,6 @@ import { __DEBUG__, TREE_OPERATION_ADD, TREE_OPERATION_REMOVE, - TREE_OPERATION_REMOVE_ROOT, TREE_OPERATION_REORDER_CHILDREN, TREE_OPERATION_SET_SUBTREE_MODE, TREE_OPERATION_UPDATE_TREE_BASE_DURATION, @@ -313,9 +312,6 @@ function updateTree( } break; } - case TREE_OPERATION_REMOVE_ROOT: { - throw Error('Operation REMOVE_ROOT is not supported while profiling.'); - } case TREE_OPERATION_REORDER_CHILDREN: { id = ((operations[i + 1]: any): number); const numChildren = ((operations[i + 2]: any): number); diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index c84ed2d36476..66de8440b65e 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -28,7 +28,6 @@ import { import { TREE_OPERATION_ADD, TREE_OPERATION_REMOVE, - TREE_OPERATION_REMOVE_ROOT, TREE_OPERATION_REORDER_CHILDREN, TREE_OPERATION_SET_SUBTREE_MODE, TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS, @@ -295,12 +294,6 @@ export function printOperationsArray(operations: Array) { } break; } - case TREE_OPERATION_REMOVE_ROOT: { - i += 1; - - logs.push(`Remove root ${rootID}`); - break; - } case TREE_OPERATION_SET_SUBTREE_MODE: { const id = operations[i + 1]; const mode = operations[i + 2]; From 65db1000b944c8a07b5947c06b38eb8364dce4f2 Mon Sep 17 00:00:00 2001 From: "Sebastian \"Sebbie\" Silbermann" Date: Fri, 6 Feb 2026 16:00:43 +0100 Subject: [PATCH 2/2] [test] Move profilingCommitTreeBuilder to versioned renderer (#35711) --- .../profilingCommitTreeBuilder-test.js | 171 ++---------------- 1 file changed, 14 insertions(+), 157 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js b/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js index 968fda10dab5..f08629dd05df 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js @@ -9,10 +9,7 @@ import type Store from 'react-devtools-shared/src/devtools/store'; -import { - getLegacyRenderImplementation, - getModernRenderImplementation, -} from './utils'; +import {getVersionedRenderImplementation} from './utils'; describe('commit tree', () => { let React = require('react'); @@ -32,12 +29,10 @@ describe('commit tree', () => { Scheduler = require('scheduler'); }); - const {render: legacyRender} = getLegacyRenderImplementation(); - const {render: modernRender} = getModernRenderImplementation(); + const {render} = getVersionedRenderImplementation(); // @reactVersion >= 16.9 - // @reactVersion <= 18.2 - it('should be able to rebuild the store tree for each commit (legacy render)', () => { + it('should be able to rebuild the store tree for each commit', () => { const Parent = ({count}) => { Scheduler.unstable_advanceTime(10); return new Array(count) @@ -50,13 +45,13 @@ describe('commit tree', () => { }); utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => legacyRender()); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] ▾ [Memo] `); - utils.act(() => legacyRender()); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] ▾ @@ -64,73 +59,14 @@ describe('commit tree', () => { [Memo] [Memo] `); - utils.act(() => legacyRender()); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] ▾ [Memo] [Memo] `); - utils.act(() => legacyRender()); - expect(store).toMatchInlineSnapshot(` - [root] - - `); - utils.act(() => store.profilerStore.stopProfiling()); - - const rootID = store.roots[0]; - const commitTrees = []; - for (let commitIndex = 0; commitIndex < 4; commitIndex++) { - commitTrees.push( - store.profilerStore.profilingCache.getCommitTree({ - commitIndex, - rootID, - }), - ); - } - - expect(commitTrees[0].nodes.size).toBe(3); // + + - expect(commitTrees[1].nodes.size).toBe(5); // + + x 3 - expect(commitTrees[2].nodes.size).toBe(4); // + + x 2 - expect(commitTrees[3].nodes.size).toBe(2); // + - }); - - // @reactVersion >= 18 - it('should be able to rebuild the store tree for each commit (createRoot)', () => { - const Parent = ({count}) => { - Scheduler.unstable_advanceTime(10); - return new Array(count) - .fill(true) - .map((_, index) => ); - }; - const Child = React.memo(function Child() { - Scheduler.unstable_advanceTime(2); - return null; - }); - - utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => modernRender()); - expect(store).toMatchInlineSnapshot(` - [root] - ▾ - [Memo] - `); - utils.act(() => modernRender()); - expect(store).toMatchInlineSnapshot(` - [root] - ▾ - [Memo] - [Memo] - [Memo] - `); - utils.act(() => modernRender()); - expect(store).toMatchInlineSnapshot(` - [root] - ▾ - [Memo] - [Memo] - `); - utils.act(() => modernRender()); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] @@ -179,10 +115,9 @@ describe('commit tree', () => { }); // @reactVersion >= 16.9 - // @reactVersion <= 18.2 - it('should support Lazy components (legacy render)', async () => { + it('should support Lazy components', async () => { utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => legacyRender()); + utils.act(() => render()); await Promise.resolve(); expect(store).toMatchInlineSnapshot(` [root] @@ -191,7 +126,7 @@ describe('commit tree', () => { [suspense-root] rects={null} `); - utils.act(() => legacyRender()); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] ▾ @@ -200,7 +135,7 @@ describe('commit tree', () => { [suspense-root] rects={null} `); - utils.act(() => legacyRender()); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] @@ -223,55 +158,10 @@ describe('commit tree', () => { expect(commitTrees[2].nodes.size).toBe(2); // + }); - // @reactVersion >= 18.0 - it('should support Lazy components (createRoot)', async () => { - utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => modernRender()); - await Promise.resolve(); - expect(store).toMatchInlineSnapshot(` - [root] - ▾ - - [suspense-root] rects={null} - - `); - utils.act(() => modernRender()); - expect(store).toMatchInlineSnapshot(` - [root] - ▾ - ▾ - - [suspense-root] rects={null} - - `); - utils.act(() => modernRender()); - expect(store).toMatchInlineSnapshot(` - [root] - - `); - utils.act(() => store.profilerStore.stopProfiling()); - - const rootID = store.roots[0]; - const commitTrees = []; - for (let commitIndex = 0; commitIndex < 3; commitIndex++) { - commitTrees.push( - store.profilerStore.profilingCache.getCommitTree({ - commitIndex, - rootID, - }), - ); - } - - expect(commitTrees[0].nodes.size).toBe(3); // + + - expect(commitTrees[1].nodes.size).toBe(4); // + + + - expect(commitTrees[2].nodes.size).toBe(2); // + - }); - // @reactVersion >= 16.9 - // @reactVersion <= 18.2 - it('should support Lazy components that are unmounted before resolving (legacy render)', async () => { + it('should support Lazy components that are unmounted before resolving', async () => { utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => legacyRender()); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] ▾ @@ -279,7 +169,7 @@ describe('commit tree', () => { [suspense-root] rects={null} `); - utils.act(() => legacyRender()); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] @@ -300,38 +190,5 @@ describe('commit tree', () => { expect(commitTrees[0].nodes.size).toBe(3); expect(commitTrees[1].nodes.size).toBe(2); // + }); - - // @reactVersion >= 18.0 - it('should support Lazy components that are unmounted before resolving (createRoot)', async () => { - utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => modernRender()); - expect(store).toMatchInlineSnapshot(` - [root] - ▾ - - [suspense-root] rects={null} - - `); - utils.act(() => modernRender()); - expect(store).toMatchInlineSnapshot(` - [root] - - `); - utils.act(() => store.profilerStore.stopProfiling()); - - const rootID = store.roots[0]; - const commitTrees = []; - for (let commitIndex = 0; commitIndex < 2; commitIndex++) { - commitTrees.push( - store.profilerStore.profilingCache.getCommitTree({ - commitIndex, - rootID, - }), - ); - } - - expect(commitTrees[0].nodes.size).toBe(3); // + + - expect(commitTrees[1].nodes.size).toBe(2); // + - }); }); });