[Fiber] Detach Fragment refs during the mutation phase - #37326
Merged
Conversation
github-actions
Bot
added
the
React Core Team
eps1lon
changed the title
A `ref` callback inlined into a component's render gets a new identity on every render, which is supposed to make React detach the old ref before attaching the new one. These tests pin down what actually happens for a `Fragment` ref, across the three ways such a ref gets detached, and their assertions record today's behavior so the suite stays green. Each place where that behavior is wrong carries a `TODO` naming the value that should have been there instead. The first test renders a `Fragment` and a host `<div>` carrying identically shaped inline ref callbacks in the same tree, so that both travel through a single commit and any difference between them is attributable to the commit phase rather than to scheduling. The host ref runs its cleanup before re-attaching while the Fragment ref re-attaches with nothing in between, so two attaches produce one cleanup and that one runs on unmount. Every attach receives the same `FragmentInstance`, which is what turns the dropped cleanup into an accumulating leak for anything the callback registers on that instance. The second test removes the ref from a Fragment that stays mounted, which takes the `ref === null` branch of `markRef` and then bails out of `commitAttachRef`. Nothing runs at all here, not even on the later deletion, because by then the ref to detach against is already null, so this cleanup is lost outright rather than deferred. The third test wraps the first scenario in `StrictMode`. Its mount assertion holds today, since the double invoke goes through `disappearLayoutEffects` and `reappearLayoutEffects`, which pair their Fragment detach and attach correctly. What it does not do is reach the update, because the double invoke is gated on newly placed fibers. This one collects its own log rather than using `Scheduler.log`, because `setIsStrictModeForDevtools` disables yield values for the duration of the double invoke to keep StrictMode tests quiet, which would hide the very detach and reattach the test is there to observe. Co-Authored-By: Claude Code (Opus 5) <noreply@anthropic.com>
`commitMutationEffectsOnFiber` acts on the `Ref` flag for every ref-bearing tag except `Fragment`, whose case only called `updateFragmentInstanceFiber` before falling through. `updateFragment` does mark the flag whenever the ref identity changes and `commitLayoutEffectOnFiber` does act on it by attaching, so a `Fragment` with an inline ref callback re-attached on every render while the cleanup returned by the previous callback was overwritten in `commitAttachRef` without ever being called. Since `createWorkInProgress` copies `refCleanup` forward the dropped cleanup was unrecoverable, and since the same `FragmentInstance` survives re-renders, event listeners and observers registered in the callback accumulated on it. This change adds the same detach guard the other tags use. It runs before `updateFragmentInstanceFiber` repoints `_fragmentFiber` at the new fiber so that a cleanup which walks children observes the tree it was created against, which matches how host components detach against `current`. It also runs ahead of the child traversal, the order the Fragment case already uses in `commitDeletionEffectsOnFiber` and `disappearLayoutEffects`. The characterization tests added in the previous commit are updated to assert the corrected behavior. Removing a ref from a mounted Fragment now runs its cleanup at the point of removal, and the StrictMode test keeps its mount assertion unchanged, since that path was already correct, and only gains the cleanup on update. Co-Authored-By: Claude Code (Opus 5) <noreply@anthropic.com>
eps1lon
force-pushed
the
sebbie/fragment-ref-cleanup-rerender-0d3cf7
branch
from
August 19, 2026 17:16
3e6055f to
0157f6f
Compare
eps1lon
marked this pull request as ready for review
August 19, 2026 17:22
jackpope
approved these changes
Aug 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
refcallback inlined into a component's render gets a new identity on every render, so React marks theRefflag and re-attaches the ref. For aFragmentthat re-attach happened without the matching detach so event listeners could accumulate if the event listener also changed identity.How did you test this change?