I added a test that asserts malformed profiling operations do not throw and that errors/warnings are logged by everettbu · Pull Request #452 · greptileai/react-mirror

everettbu · 2026-02-02T23:21:38Z

Mirror of facebook/react#35674
Original author: apoorvgarewal07

Builds minimal fake profiling data with a valid initial snapshot.
Injects malformed operations (duplicate add, remove of missing node, remove root, and an unsupported op).
Asserts that getCommitTree does not throw and that console.error / console.warn were called.

…ow and that errors/warnings are logged

greptile-apps · 2026-02-02T23:24:10Z

Greptile Overview

Greptile Summary

This PR converts error-throwing behavior to graceful error handling in CommitTreeBuilder.js and adds a test for malformed profiling operations. However, there are critical issues with the implementation:

Critical Issues:

The error handling code doesn't consume all operation parameters before breaking, causing subsequent parameters to be misinterpreted as operation codes and corrupting the operation stream
The test uses invalid operation code 0 which immediately terminates processing, so it never actually tests the malformed operations it's intended to test
REMOVE_ROOT case doesn't consume its required parameter

What Changed:

Replaced throw statements with console.error/console.warn for: duplicate ADD operations, REMOVE of missing nodes, REMOVE_ROOT operations, and unsupported operations
Added test case should tolerate malformed operations without throwing

These bugs must be fixed before the error handling can work correctly. The previous thread comments correctly identified these exact issues.

Confidence Score: 0/5

This PR has critical bugs that will cause incorrect behavior and the test doesn't actually test what it claims to test
The error handling code has fundamental flaws: it doesn't consume all operation parameters before exiting error paths, which corrupts the operation stream parsing. The test uses an invalid operation code that terminates processing immediately, so none of the malformed operations are actually tested. These are not minor issues - they make the error handling non-functional.
Both files require immediate attention - CommitTreeBuilder.js needs to properly consume parameters in error cases, and the test needs valid operation codes

Important Files Changed

Filename	Overview
packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js	replaced `throw` with `console.error`/`console.warn` for malformed operations, but breaks without consuming all parameters, corrupting the operation stream
packages/react-devtools-shared/src/tests/profilingCommitTreeBuilder-test.js	added test for malformed operations, but uses invalid operation code `0` which terminates processing before reaching the actual malformed operations to be tested

greptile-apps

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-02T23:24:13Z

        if (nodes.has(id)) {
-          throw new Error(
-            `Commit tree already contains fiber "${id}". This is a bug in React DevTools.`,
+          // Duplicate add detected; log and skip instead of throwing to avoid crashing the UI.
+          console.error(
+            `Commit tree already contains fiber "${id}". Skipping duplicate add.`,
          );
+          break;


using break here without advancing i past all ADD operation parameters will cause subsequent parameters to be misinterpreted as operation codes. For non-root nodes, there are still 5+ parameters to skip (parentID, ownerID, displayNameStringID, keyStringID, name prop). This will corrupt the operation stream parsing.

need to advance i to skip remaining parameters before breaking, similar to how the normal ADD case increments i throughout lines 234-248

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js Line: 202:207 Comment: using `break` here without advancing `i` past all ADD operation parameters will cause subsequent parameters to be misinterpreted as operation codes. For non-root nodes, there are still 5+ parameters to skip (parentID, ownerID, displayNameStringID, keyStringID, name prop). This will corrupt the operation stream parsing. need to advance `i` to skip remaining parameters before breaking, similar to how the normal ADD case increments `i` throughout lines 234-248 How can I resolve this? If you propose a fix, please make it concise.

greptile-apps

_{2 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-03T09:31:41Z

+          console.error(
+            `Commit tree already contains fiber "${id}". Skipping duplicate add.`,
          );
+          break;


The break statement here exits without consuming all ADD operation parameters, corrupting the operation stream. When a duplicate is detected, i has only advanced by 3 positions (operation + id + type). For non-root nodes, 5 additional parameters remain (parentID, ownerID, displayNameStringID, keyStringID, name prop). These will be misinterpreted as operation codes on the next loop iteration.

Need to advance i past all parameters before breaking:

Suggested change

break;

// Duplicate add detected; log and skip instead of throwing to avoid crashing the UI.

console.error(

`Commit tree already contains fiber "${id}". Skipping duplicate add.`,

);

// Skip remaining ADD parameters to avoid corrupting the operation stream

if (type === ElementTypeRoot) {

i += 4; // isStrictModeCompliant, Profiling flag, supportsStrictMode, hasOwnerMetadata

} else {

i += 5; // parentID, ownerID, displayNameStringID, keyStringID, name prop

}

break;

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js Line: 207:207 Comment: The `break` statement here exits without consuming all ADD operation parameters, corrupting the operation stream. When a duplicate is detected, `i` has only advanced by 3 positions (operation + id + type). For non-root nodes, 5 additional parameters remain (parentID, ownerID, displayNameStringID, keyStringID, name prop). These will be misinterpreted as operation codes on the next loop iteration. Need to advance `i` past all parameters before breaking: ```suggestion // Duplicate add detected; log and skip instead of throwing to avoid crashing the UI. console.error( `Commit tree already contains fiber "${id}". Skipping duplicate add.`, ); // Skip remaining ADD parameters to avoid corrupting the operation stream if (type === ElementTypeRoot) { i += 4; // isStrictModeCompliant, Profiling flag, supportsStrictMode, hasOwnerMetadata } else { i += 5; // parentID, ownerID, displayNameStringID, keyStringID, name prop } break; ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-02-03T09:31:42Z

      case TREE_OPERATION_REMOVE_ROOT: {
-        throw Error('Operation REMOVE_ROOT is not supported while profiling.');
+        // Removing roots is not supported while profiling; log and ignore.
+        console.warn('Operation REMOVE_ROOT is not supported while profiling. Ignoring.');
+        break;


REMOVE_ROOT operation has 1 parameter (root ID) that needs to be consumed. Breaking without advancing i will cause the root ID to be misinterpreted as an operation code.

Suggested change

case TREE_OPERATION_REMOVE_ROOT: {

throw Error('Operation REMOVE_ROOT is not supported while profiling.');

// Removing roots is not supported while profiling; log and ignore.

console.warn('Operation REMOVE_ROOT is not supported while profiling. Ignoring.');

break;

case TREE_OPERATION_REMOVE_ROOT: {

i++; // Skip root ID parameter

// Removing roots is not supported while profiling; log and ignore.

console.warn('Operation REMOVE_ROOT is not supported while profiling. Ignoring.');

break;

}

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js Line: 317:320 Comment: REMOVE_ROOT operation has 1 parameter (root ID) that needs to be consumed. Breaking without advancing `i` will cause the root ID to be misinterpreted as an operation code. ```suggestion case TREE_OPERATION_REMOVE_ROOT: { i++; // Skip root ID parameter // Removing roots is not supported while profiling; log and ignore. console.warn('Operation REMOVE_ROOT is not supported while profiling. Ignoring.'); break; } ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-02-03T09:31:43Z

+    const ops0 = [0, rootID, 0];
+    const ops1 = [
+      0, rootID, 0,
+      1, 2, ElementTypeFunction, rootID, 0, 0, 0,
+      2, 1, 999,
+      6,
+      99,
+    ];


The test doesn't actually validate the malformed operations it intends to test. Operation 0 in both ops0 and at the start of ops1 hits the default case (line 499-503 in CommitTreeBuilder.js), which sets i = operations.length and stops processing immediately. The subsequent malformed operations (duplicate ADD at position 1, REMOVE of missing node at position 2, REMOVE_ROOT at position 6, and unsupported op 99) are never executed.

To properly test these scenarios, remove the leading 0 operations or use valid initial operations like TREE_OPERATION_UPDATE_TREE_BASE_DURATION.

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js Line: 369:376 Comment: The test doesn't actually validate the malformed operations it intends to test. Operation `0` in both `ops0` and at the start of `ops1` hits the default case (line 499-503 in CommitTreeBuilder.js), which sets `i = operations.length` and stops processing immediately. The subsequent malformed operations (duplicate ADD at position 1, REMOVE of missing node at position 2, REMOVE_ROOT at position 6, and unsupported op 99) are never executed. To properly test these scenarios, remove the leading `0` operations or use valid initial operations like `TREE_OPERATION_UPDATE_TREE_BASE_DURATION`. How can I resolve this? If you propose a fix, please make it concise.

greptile-apps

_{2 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-03T18:32:04Z

        if (nodes.has(id)) {
-          throw new Error(
-            `Commit tree already contains fiber "${id}". This is a bug in React DevTools.`,
+          // Duplicate add detected; log and skip instead of throwing to avoid crashing the UI.
+          console.error(
+            `Commit tree already contains fiber "${id}". Skipping duplicate add.`,
          );
+          break;


using break without consuming all ADD parameters corrupts the operation stream. At this point, i has only advanced by 3 (operation + id + type). For non-root nodes, 5 more parameters remain: parentID, ownerID, displayNameStringID, keyStringID, and name prop (lines 234-248). These unread parameters will be misinterpreted as operation codes on the next iteration.

need to advance i past all remaining parameters before breaking:

Suggested change

if (nodes.has(id)) {

throw new Error(

`Commit tree already contains fiber "${id}". This is a bug in React DevTools.`,

// Duplicate add detected; log and skip instead of throwing to avoid crashing the UI.

console.error(

`Commit tree already contains fiber "${id}". Skipping duplicate add.`,

);

break;

if (nodes.has(id)) {

// Duplicate add detected; log and skip instead of throwing to avoid crashing the UI.

console.error(

`Commit tree already contains fiber "${id}". Skipping duplicate add.`,

);

// Skip remaining parameters for non-root nodes

if (type !== ElementTypeRoot) {

i += 5; // parentID, ownerID, displayNameStringID, keyStringID, name prop

} else {

i += 4; // isStrictModeCompliant, Profiling flag, supportsStrictMode flag, hasOwnerMetadata flag

}

break;

}

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js Line: 202:207 Comment: using `break` without consuming all ADD parameters corrupts the operation stream. At this point, `i` has only advanced by 3 (operation + id + type). For non-root nodes, 5 more parameters remain: parentID, ownerID, displayNameStringID, keyStringID, and name prop (lines 234-248). These unread parameters will be misinterpreted as operation codes on the next iteration. need to advance `i` past all remaining parameters before breaking: ```suggestion if (nodes.has(id)) { // Duplicate add detected; log and skip instead of throwing to avoid crashing the UI. console.error( `Commit tree already contains fiber "${id}". Skipping duplicate add.`, ); // Skip remaining parameters for non-root nodes if (type !== ElementTypeRoot) { i += 5; // parentID, ownerID, displayNameStringID, keyStringID, name prop } else { i += 4; // isStrictModeCompliant, Profiling flag, supportsStrictMode flag, hasOwnerMetadata flag } break; } ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-02-03T18:32:05Z

      case TREE_OPERATION_REMOVE_ROOT: {
-        throw Error('Operation REMOVE_ROOT is not supported while profiling.');
+        // Removing roots is not supported while profiling; log and ignore.
+        console.warn('Operation REMOVE_ROOT is not supported while profiling. Ignoring.');
+        break;


REMOVE_ROOT operation has 1 parameter (root ID) that must be consumed. Breaking without advancing i causes the root ID to be misinterpreted as an operation code on the next iteration.

Suggested change

case TREE_OPERATION_REMOVE_ROOT: {

throw Error('Operation REMOVE_ROOT is not supported while profiling.');

// Removing roots is not supported while profiling; log and ignore.

console.warn('Operation REMOVE_ROOT is not supported while profiling. Ignoring.');

break;

case TREE_OPERATION_REMOVE_ROOT: {

// Removing roots is not supported while profiling; log and ignore.

console.warn('Operation REMOVE_ROOT is not supported while profiling. Ignoring.');

i++; // Skip root ID parameter

break;

}

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js Line: 317:320 Comment: REMOVE_ROOT operation has 1 parameter (root ID) that must be consumed. Breaking without advancing `i` causes the root ID to be misinterpreted as an operation code on the next iteration. ```suggestion case TREE_OPERATION_REMOVE_ROOT: { // Removing roots is not supported while profiling; log and ignore. console.warn('Operation REMOVE_ROOT is not supported while profiling. Ignoring.'); i++; // Skip root ID parameter break; } ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-02-03T18:32:06Z

+    const ops0 = [0, rootID, 0];
+    const ops1 = [
+      0, rootID, 0,
+      1, 2, ElementTypeFunction, rootID, 0, 0, 0,
+      2, 1, 999,
+      6,
+      99,
+    ];


the test doesn't actually validate the malformed operations it intends to test. Operation 0 is not a valid operation code (valid codes are 1-13 per constants.js:20-32). When operation 0 is encountered in both ops0 and at the start of ops1, it hits the default case (CommitTreeBuilder.js:499-503), which sets i = operations.length and immediately stops processing. The subsequent malformed operations in ops1 (duplicate ADD at position 1, REMOVE of missing node at position 2, REMOVE_ROOT at position 6, unsupported op 99) are never reached.

to properly test error handling, use valid initial operations:

Suggested change

const ops0 = [0, rootID, 0];

const ops1 = [

0, rootID, 0,

1, 2, ElementTypeFunction, rootID, 0, 0, 0,

2, 1, 999,

6,

99,

];

const ops0 = [4, rootID, 0]; // TREE_OPERATION_UPDATE_TREE_BASE_DURATION

const ops1 = [

4, rootID, 0, // TREE_OPERATION_UPDATE_TREE_BASE_DURATION

1, 2, ElementTypeFunction, rootID, 0, 0, 0, // duplicate ADD (node 2 exists in snapshot)

2, 1, 999, // REMOVE missing node 999

6, // REMOVE_ROOT

99, // unsupported operation

];

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js Line: 369:376 Comment: the test doesn't actually validate the malformed operations it intends to test. Operation `0` is not a valid operation code (valid codes are 1-13 per constants.js:20-32). When operation `0` is encountered in both `ops0` and at the start of `ops1`, it hits the default case (CommitTreeBuilder.js:499-503), which sets `i = operations.length` and immediately stops processing. The subsequent malformed operations in `ops1` (duplicate ADD at position 1, REMOVE of missing node at position 2, REMOVE_ROOT at position 6, unsupported op 99) are never reached. to properly test error handling, use valid initial operations: ```suggestion const ops0 = [4, rootID, 0]; // TREE_OPERATION_UPDATE_TREE_BASE_DURATION const ops1 = [ 4, rootID, 0, // TREE_OPERATION_UPDATE_TREE_BASE_DURATION 1, 2, ElementTypeFunction, rootID, 0, 0, 0, // duplicate ADD (node 2 exists in snapshot) 2, 1, 999, // REMOVE missing node 999 6, // REMOVE_ROOT 99, // unsupported operation ]; ``` How can I resolve this? If you propose a fix, please make it concise.

everettbu · 2026-02-10T09:34:56Z

Upstream PR was closed or merged. Code is synced via branch mirror.

apoorvgarewal07 added 2 commits February 3, 2026 01:15

I added a test that asserts malformed profiling operations do not thr…

61141cd

…ow and that errors/warnings are logged

Merge branch 'main' of https://github.com/facebook/react

98c13a1

everettbu added the CLA Signed label Feb 2, 2026

greptile-apps Bot reviewed Feb 2, 2026

View reviewed changes

Merge branch 'main' into main

6162e4a

greptile-apps Bot reviewed Feb 3, 2026

View reviewed changes

Merge branch 'main' into main

4fe4b75

greptile-apps Bot reviewed Feb 3, 2026

View reviewed changes

everettbu closed this Feb 10, 2026

everettbu deleted the main-35674 branch February 10, 2026 09:34

everettbu restored the main-35674 branch February 11, 2026 04:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

I added a test that asserts malformed profiling operations do not throw and that errors/warnings are logged#452

I added a test that asserts malformed profiling operations do not throw and that errors/warnings are logged#452
everettbu wants to merge 4 commits into
mainfrom
main-35674

everettbu commented Feb 2, 2026

Uh oh!

greptile-apps Bot commented Feb 2, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot left a comment

Uh oh!

greptile-apps Bot Feb 2, 2026

Uh oh!

greptile-apps Bot left a comment

Uh oh!

greptile-apps Bot Feb 3, 2026

Uh oh!

greptile-apps Bot Feb 3, 2026

Uh oh!

greptile-apps Bot Feb 3, 2026

Uh oh!

greptile-apps Bot left a comment

Uh oh!

greptile-apps Bot Feb 3, 2026

Uh oh!

greptile-apps Bot Feb 3, 2026

Uh oh!

greptile-apps Bot Feb 3, 2026

Uh oh!

everettbu commented Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-          break;
+          // Duplicate add detected; log and skip instead of throwing to avoid crashing the UI.
+          console.error(
+            `Commit tree already contains fiber "${id}". Skipping duplicate add.`,
+          );
+          // Skip remaining ADD parameters to avoid corrupting the operation stream
+          if (type === ElementTypeRoot) {
+            i += 4; // isStrictModeCompliant, Profiling flag, supportsStrictMode, hasOwnerMetadata
+          } else {
+            i += 5; // parentID, ownerID, displayNameStringID, keyStringID, name prop
+          }
+          break;

Uh oh!

Conversation

everettbu commented Feb 2, 2026

Uh oh!

greptile-apps Bot commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Greptile Summary

Confidence Score: 0/5

Important Files Changed

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

everettbu commented Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greptile-apps Bot commented Feb 2, 2026 •

edited

Loading