Skip to content

[DevTools] Check suspense child node presence in parentSuspense.children before removing#509

Closed
everettbu wants to merge 1 commit into
mainfrom
devtools/explicit-check-suspense-remove
Closed

[DevTools] Check suspense child node presence in parentSuspense.children before removing#509
everettbu wants to merge 1 commit into
mainfrom
devtools/explicit-check-suspense-remove

Conversation

@everettbu

Copy link
Copy Markdown

Mirror of facebook/react#35775
Original author: hoxyq


Currently, this silently removes the last child in the list, which doesn't contain the id.

@greptile-apps

greptile-apps Bot commented Feb 13, 2026

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

Adds a defensive check before removing a suspense child node from its parent's children array in the DevTools store. Previously, if indexOf(id) returned -1 (node not found), splice(-1, 1) would silently remove the last element in the array — a subtle bug that could corrupt the suspense tree state. The fix calls _throwAndEmitError when the child is not found, which is consistent with other guard patterns in the same file.

  • Prevents silent removal of the wrong child node when a suspense node ID is not found in its parent's children array
  • The analogous non-suspense node removal path (line 1619-1620) has the same unguarded pattern and could benefit from a similar fix

Confidence Score: 4/5

  • This PR is safe to merge — it adds a straightforward defensive check that prevents data corruption in an error case.
  • The change is minimal, targeted, and follows established patterns in the codebase. The _throwAndEmitError method always throws (return type empty), so the subsequent splice is correctly unreachable when the guard triggers. No functional behavior changes for the happy path.
  • No files require special attention. The single changed file has a clean, well-scoped modification.

Important Files Changed

Filename Overview
packages/react-devtools-shared/src/devtools/store.js Adds a bounds check before splice to prevent silently removing the last child when a suspense node ID is not found in the parent's children array. The fix is correct and follows existing error handling patterns.

Last reviewed commit: cce34d3

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +1876 to +1882
if (index === -1) {
this._throwAndEmitError(
Error(
`Cannot remove suspense node "${id}" from parent "${parentID}" because it is not a child of the parent.`,
),
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Analogous check missing for non-suspense nodes

This guard is a good fix. Note that the same unguarded indexOf + splice pattern exists for the non-suspense element removal path at line 1619-1620:

const index = parentElement.children.indexOf(id);
parentElement.children.splice(index, 1);

That path has the same silent-last-child-removal bug. It might be worth adding a similar check there for consistency. Not blocking this PR, but flagging for awareness.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-shared/src/devtools/store.js
Line: 1876:1882

Comment:
**Analogous check missing for non-suspense nodes**

This guard is a good fix. Note that the same unguarded `indexOf` + `splice` pattern exists for the non-suspense element removal path at line 1619-1620:

```js
const index = parentElement.children.indexOf(id);
parentElement.children.splice(index, 1);
```

That path has the same silent-last-child-removal bug. It might be worth adding a similar check there for consistency. Not blocking this PR, but flagging for awareness.

How can I resolve this? If you propose a fix, please make it concise.

@everettbu

Copy link
Copy Markdown
Author

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

@everettbu everettbu closed this Feb 13, 2026
@everettbu
everettbu deleted the devtools/explicit-check-suspense-remove branch February 13, 2026 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants