Run VirtualThought's size-release cleanup only on unmount - #4975
Merged
Conversation
The effect that calls `onResize({ height: null })` was written as a
componentWillUnmount, with a comment asserting its dependencies "should be
memoized and not change for the life of the component". That is not true of
`onResize`: TreeNode memoizes it on `cliff`, which changes whenever a thought's
position in the tree changes. React runs the cleanup before every such re-run,
so a thought released its tracked size while it was still mounted.
Instrumenting the cleanup and adding a sibling (which changes the preceding
thought's cliff) shows the cleanup firing on a live thought, followed by
`removeSize` and an immediate `setSize` for the same key:
CLEANUP-FIRED |_fHZM6U06I_ej stillInDOM= true
removeSize |_fHZM6U06I_ej
setSize |_fHZM6U06I_ej 20
The re-add happens because `updateSize`'s dependencies are a superset, so its
effect body restores the entry in the same passive-effect flush. The visible
behavior is therefore unchanged today. What this removes is the churn — an
in-place `delete` on the `sizes` state object followed by a rebuild — and the
trap of a cleanup that claims to be unmount-only while running mid-life.
Route the call through `useFreshCallback` so the effect's dependency is stable
and the cleanup runs only on unmount, while still invoking the latest
`onResize`. Re-running the instrumented trace confirms the mid-life fire is gone
and all thoughts still release their size on teardown.
Co-Authored-By: Claude Opus 5 (unknown context) <noreply@anthropic.com>
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
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.
Follow-up to #4969, which fixed
useDelayedAutofocuslatching anunmountedref in a cleanup that React re-runs on every dependency change. This is the same mistake inVirtualThought, found by auditing the codebase for it.Problem
VirtualThoughtreleases its tracked size by callingonResize({ height: null })in an effect cleanup, annotated:onResizeis not stable.TreeNodememoizes it asuseCallback(props => setSize({ ...props, cliff }), [cliff, setSize]), andcliffchanges whenever a thought's position in the tree changes. React runs the cleanup before every such re-run, so a thought releases its size while still mounted.Instrumenting the cleanup and adding a sibling (which changes the preceding thought's cliff):
Impact
No user-visible bug today.
updateSize's dependencies are a superset of this effect's, so its body re-adds the entry in the same passive-effect flush — the trace above shows theremoveSize/setSizepair. What this PR removes is the churn (an in-placedeleteon thesizesstate object, then a rebuild) and a live trap in the size-tracking path: any future change that breaks the superset relationship, or that makes the removal observable, turns this into a layout bug.Solution
Route the call through the existing
useFreshCallbackso the effect's dependency is stable and the cleanup runs only on unmount, while still invoking the latestonResize. The comment now describes why the indirection is needed instead of asserting a stability that does not hold.Verification
stillInDOM= false).yarn lint:tsceslint src/components/VirtualThought.tsxNo regression test is included — the current behavior is self-healing, so there is nothing externally observable to assert without instrumenting internals.
🤖 Generated with Claude Code