fix(react): clear removed snapshot prop refs#2590
Conversation
🦋 Changeset detectedLatest commit: 3ebe048 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds helpers to find removed snapshot nodes and clears compiler-transient ChangesTransient child prop cleanup on snapshot removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will degrade performance by 21.72%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | 002-hello-reactLynx-destroyBackground |
670.3 µs | 916.9 µs | -26.9% |
| ❌ | 008-many-use-state-destroyBackground |
8 ms | 9.5 ms | -16.17% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing Yradex:wt/run-on-background-clear-prop-refs (3ebe048) with main (66f6ecf)
Footnotes
-
26 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
React External#1321 Bundle Size — 695.33KiB (+0.33%).3ebe048(current) vs 66f6ecf main#1314(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/run-on-background-clea... Project dashboard Generated by RelativeCI Documentation Report issue |
React Example with Element Template#473 Bundle Size — 199.83KiB (0%).3ebe048(current) vs 66f6ecf main#466(baseline) Bundle metrics
Bundle size by type
|
| Current #473 |
Baseline #466 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
54.08KiB |
54.08KiB |
Bundle analysis report Branch Yradex:wt/run-on-background-clea... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9781 Bundle Size — 901.38KiB (0%).3ebe048(current) vs 66f6ecf main#9775(baseline) Bundle metrics
Bundle size by type
|
| Current #9781 |
Baseline #9775 |
|
|---|---|---|
497.1KiB |
497.1KiB |
|
402.06KiB |
402.06KiB |
|
2.22KiB |
2.22KiB |
Bundle analysis report Branch Yradex:wt/run-on-background-clea... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#1340 Bundle Size — 208.1KiB (+0.31%).3ebe048(current) vs 66f6ecf main#1333(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/run-on-background-clea... Project dashboard Generated by RelativeCI Documentation Report issue |
React Example#8207 Bundle Size — 237.15KiB (+0.27%).3ebe048(current) vs 66f6ecf main#8200(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/run-on-background-clea... Project dashboard Generated by RelativeCI Documentation Report issue |
d81fa3c to
2354303
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/runtime/src/snapshot/snapshot/snapshot.ts`:
- Around line 74-83: clearTransientChildPropRefs currently retraverses the
removed subtree via traverseSnapshotInstance, causing O(n²) behavior when
callers repeatedly call clearTransientChildPropRefs(v, v); refactor to accept a
precomputed WeakSet of removed snapshots (e.g., add an optional parameter
removedSnapshots: WeakSet<object>) or an overload so callers can compute
removedSnapshots once using traverseSnapshotInstance(removedChild, ...) and pass
it in; update clearTransientChildPropRefs(owner, removedChild,
removedSnapshots?) to use the passed set if present (fall back to current
traversal only if not provided) and change the current call sites that do
clearTransientChildPropRefs(v, v) to compute the removedSnapshots once and pass
it in, preserving existing behavior when not provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bec8ee41-a6e0-4765-a7a9-26704fb7ebee
📒 Files selected for processing (3)
.changeset/clear-snapshot-prop-refs.mdpackages/react/runtime/__test__/snapshot/renderToOpcodes.test.jsxpackages/react/runtime/src/snapshot/snapshot/snapshot.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react/runtime/__test__/snapshot/renderToOpcodes.test.jsx (1)
111-122: 💤 Low valueConsider testing deeper nesting levels.
The test verifies 2-level nesting (
[[child]]), which is good. Since the PR description mentions recursive handling of array shapes, consider adding a test case with 3+ levels of nesting (e.g.,[[[child]]]) to more thoroughly verify the recursive cleanup logic handles arbitrary depth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/runtime/__test__/snapshot/renderToOpcodes.test.jsx` around lines 111 - 122, Add a parallel test that exercises deeper nesting (e.g., use createElement('view', { $0: [[[child]]] }) and run renderToString(vnode, new SnapshotInstance('root'))), then call vnode.removeChild(child) and assert that vnode.props.$0 becomes the same shape with the child replaced by undefined (e.g., [[[undefined]]]) and vnode.childNodes is empty; mimic the existing test block ("should clear removed children from nested transient prop arrays") but with three or more nested array levels to verify recursive cleanup in removeChild and the transient-props handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/react/runtime/__test__/snapshot/renderToOpcodes.test.jsx`:
- Around line 111-122: Add a parallel test that exercises deeper nesting (e.g.,
use createElement('view', { $0: [[[child]]] }) and run renderToString(vnode, new
SnapshotInstance('root'))), then call vnode.removeChild(child) and assert that
vnode.props.$0 becomes the same shape with the child replaced by undefined
(e.g., [[[undefined]]]) and vnode.childNodes is empty; mimic the existing test
block ("should clear removed children from nested transient prop arrays") but
with three or more nested array levels to verify recursive cleanup in
removeChild and the transient-props handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37d18710-3f8c-4aa0-96f4-8d5b5ba0c8db
📒 Files selected for processing (2)
packages/react/runtime/__test__/snapshot/renderToOpcodes.test.jsxpackages/react/runtime/src/snapshot/snapshot/snapshot.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/snapshot/snapshot/snapshot.ts
Summary by CodeRabbit
Bug Fixes
Tests
Summary
This fixes a main-thread snapshot retention path where removed child subtrees could still be strongly referenced by compiler-generated transient child props. Snapshot JSX uses
$*props as staging references for named children; whenSnapshotInstance.removeChild()detached a child, the structural tree and native elements were cleaned up, but those$*props could still point at the removed snapshot subtree.The practical failure mode is that a deleted list holder or list item subtree can remain reachable from the root props chain after it is removed. That keeps old snapshot instances alive even though they are no longer part of the rendered tree.
Key points
Clear transient
$*owner props during removal. Direct refs such as{ $0: removedChild }are deleted fromprops, so the owner no longer keeps a hard reference to the removed subtree afterremoveChild().Handle compiled array child shapes recursively. When a transient prop stores children as arrays, only removed snapshot entries are cleared, including nested arrays:
Clean transient props inside the removed subtree as it is torn down. This breaks references held by the deleted subtree itself, including component/list structures that had their own compiled child staging props.
Runtime Contract
This only affects compiler-owned transient child props whose keys start with
$. Normal user props, public runtime APIs, serialized snapshot shape, native patch operations, and list update protocols are unchanged.The cleanup is tied to
SnapshotInstance.removeChild(): removed snapshot instances and descendants are collected into a removal set, then$*prop values are cleared if they directly reference any removed snapshot or contain one inside an array. Existing array identity is preserved, and non-removed entries keep their current identity and order.Checklist