Skip to content

Stabilize reactFragments host node handle#434

Closed
everettbu wants to merge 1 commit into
mainfrom
fr-expando-rename
Closed

Stabilize reactFragments host node handle#434
everettbu wants to merge 1 commit into
mainfrom
fr-expando-rename

Conversation

@everettbu

@everettbu everettbu commented Jan 27, 2026

Copy link
Copy Markdown

Mirror of facebook/react#35642
Original author: jackpope


react/react#34935 Introduced unstable_reactFragments handle on DOM nodes to enable caching of Observers.

This has been tested in production and is stable so it can be rolled out with the Fragment Refs feature.

@greptile-apps

greptile-apps Bot commented Jan 27, 2026

Copy link
Copy Markdown

Greptile Summary

This PR stabilizes the reactFragments expando property on host nodes by removing the unstable_ prefix. The rename is applied across the DOM renderer, React Native Fabric renderer, and associated tests.

  • Bug: Incomplete rename in DOM renderer's deleteChildFromFragmentInstance — The null-check on line 3605 of ReactFiberConfigDOM.js still references unstable_reactFragments while the property is now stored as reactFragments. This makes the delete call unreachable, causing fragment handles to never be cleaned up from DOM nodes (memory leak). The equivalent code in ReactFiberConfigFabric.js was renamed correctly.
  • The Fabric renderer changes are clean and consistent.
  • Tests were updated to use the new property name but do not cover the deleteChildFromFragmentInstance deletion path where the bug exists.

Confidence Score: 2/5

  • This PR has an incomplete rename that introduces a memory leak in the DOM renderer's fragment cleanup path.
  • The PR is a simple rename but has one missed occurrence of the old property name in a null-check, making the subsequent delete unreachable. This is a clear bug that causes fragment handles to never be removed from DOM nodes. The Fabric renderer was renamed correctly, confirming this is an oversight rather than intentional.
  • Pay close attention to packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js — line 3605 still uses the old unstable_reactFragments name in a null-check, making the cleanup on line 3606 dead code.

Important Files Changed

Filename Overview
packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js Incomplete rename: deleteChildFromFragmentInstance still checks unstable_reactFragments (line 3605) but deletes from reactFragments (line 3606), making the delete unreachable and causing a memory leak.
packages/react-dom/src/tests/ReactDOMFragmentRefs-test.js Test assertions correctly updated from unstable_reactFragments to reactFragments. No coverage for the deleteChildFromFragmentInstance code path, which is where the bug exists.
packages/react-native-renderer/src/ReactFiberConfigFabric.js Clean, consistent rename of unstable_reactFragments to reactFragments across type definition, add, and delete paths. No issues found.

Last reviewed commit: 1709ee7

@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.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@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.

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@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.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines 3605 to +3606
if (instance.unstable_reactFragments != null) {
instance.unstable_reactFragments.delete(fragmentInstance);
instance.reactFragments.delete(fragmentInstance);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missed rename causes dead code / memory leak

The null-check on line 3605 still references the old property name unstable_reactFragments, but the property is now stored as reactFragments (set in addFragmentHandleToInstance). Since instance.unstable_reactFragments will always be undefined after this rename, the condition is never truthy, so reactFragments.delete(fragmentInstance) on line 3606 will never execute. This means fragment handles will accumulate on DOM nodes and never be cleaned up — a memory leak.

The Fabric renderer (ReactFiberConfigFabric.js:892) correctly renames both the check and the delete. This file should match:

Suggested change
if (instance.unstable_reactFragments != null) {
instance.unstable_reactFragments.delete(fragmentInstance);
instance.reactFragments.delete(fragmentInstance);
if (instance.reactFragments != null) {
instance.reactFragments.delete(fragmentInstance);
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
Line: 3605-3606

Comment:
**Missed rename causes dead code / memory leak**

The null-check on line 3605 still references the old property name `unstable_reactFragments`, but the property is now stored as `reactFragments` (set in `addFragmentHandleToInstance`). Since `instance.unstable_reactFragments` will always be `undefined` after this rename, the condition is never truthy, so `reactFragments.delete(fragmentInstance)` on line 3606 will never execute. This means fragment handles will accumulate on DOM nodes and never be cleaned up — a memory leak.

The Fabric renderer (`ReactFiberConfigFabric.js:892`) correctly renames both the check and the delete. This file should match:

```suggestion
    if (instance.reactFragments != null) {
      instance.reactFragments.delete(fragmentInstance);
```

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 Mar 3, 2026
@everettbu everettbu deleted the fr-expando-rename branch March 3, 2026 21:20
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