Skip to content

[DevTools] Fix broken commit tree builder for initial operations #35710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
eps1lon merged 3 commits into from
Feb 6, 2026
+28 −66
Merged

[DevTools] Fix broken commit tree builder for initial operations #35710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fec427
[DevTools] Stop sending current root ID in operations
eps1lon Feb 6, 2026
d34860f
Remove unused `TREE_OPERATION_REMOVE_ROOT`
eps1lon Feb 6, 2026
86ed9f4
[DevTools] Fix broken commit tree builder for initial operations
eps1lon Feb 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 27 additions & 14 deletions packages/react-devtools-shared/src/backend/fiber/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -1156,7 +1156,7 @@ export function attach(
}
}
if (hasChanges) {
flushPendingEvents();
flushPendingEvents(root);
}
}

Expand All @@ -1183,7 +1183,7 @@ export function attach(
updateMostRecentlyInspectedElementIfNecessary(devtoolsInstance.id);
}
}
flushPendingEvents();
flushPendingEvents(null);
}

function clearConsoleLogsHelper(instanceID: number, type: 'error' | 'warn') {
Expand Down Expand Up @@ -1211,7 +1211,7 @@ export function attach(
}
const changed = recordConsoleLogs(devtoolsInstance, componentLogsEntry);
if (changed) {
flushPendingEvents();
flushPendingEvents(null);
updateMostRecentlyInspectedElementIfNecessary(devtoolsInstance.id);
}
}
Expand Down Expand Up @@ -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');
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -5746,11 +5753,11 @@ export function attach(

mountFiberRecursively(root.current, false);

flushPendingEvents(currentRoot);

currentRoot = (null: any);
});

flushPendingEvents();

Comment on lines +5756 to -5753
Copy link
Collaborator Author

@eps1lon eps1lon Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix. We were flushing events for initial operations at the end without a root even though we may have been profiling.

needsToFlushComponentLogs = false;
}
}
Expand All @@ -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} =
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -5876,7 +5889,7 @@ export function attach(
}

// We're done here.
flushPendingEvents();
flushPendingEvents(currentRoot);

needsToFlushComponentLogs = false;

Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
40 changes: 0 additions & 40 deletions packages/react-devtools-shared/src/devtools/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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];
Expand Down
4 changes: 0 additions & 4 deletions packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 0 additions & 7 deletions packages/react-devtools-shared/src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -295,12 +294,6 @@ export function printOperationsArray(operations: Array<number>) {
}
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];
Expand Down
Loading