feat(react): clean element template registry on remove#2569
Merged
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds explicit removedSubtreeHandleIds to the element-template removeNode opcode: typed protocol tuples, background emission of subtree handle lists, formatter/debug updates, patch handler registry cleanup, and comprehensive test/fixture updates to reflect the new payload shape. ChangesElement Template removeNode Subtree Handle Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
Yradex
force-pushed
the
slice/element-template/04
branch
from
1e1a54a to
1897322
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Web Explorer#9452 Bundle Size — 900.02KiB (0%).edcd696(current) vs 0d51ee8 main#9446(baseline) Bundle metrics
|
| Current #9452 |
Baseline #9446 |
|
|---|---|---|
 Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
 Modules |
229(-0.43%) |
230 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.28% |
27.28% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9452 |
Baseline #9446 |
|
|---|---|---|
| JS |
495.88KiB |
495.88KiB |
| Other |
401.92KiB |
401.92KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch Yradex:slice/element-template/04 Project dashboard
Generated by RelativeCI Documentation Report issue
React Example with Element Template#145 Bundle Size — 198.84KiB (+0.13%).edcd696(current) vs 0d51ee8 main#139(baseline) Bundle metrics
|
| Current #145 |
Baseline #139 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
26.6% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
79(+1.28%) |
78 |
| Duplicate Modules |
23 |
23 |
| Duplicate Code |
40.31%(-0.17%) |
40.38% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 regression
| Current #145 |
Baseline #139 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
53.08KiB (+0.5%) |
52.81KiB |
Bundle analysis report Branch Yradex:slice/element-template/04 Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7880 Bundle Size — 234.22KiB (0%).edcd696(current) vs 0d51ee8 main#7874(baseline) Bundle metrics
|
| Current #7880 |
Baseline #7874 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
188 |
188 |
| Duplicate Modules |
76 |
76 |
| Duplicate Code |
45.08% |
45.08% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7880 |
Baseline #7874 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
88.46KiB |
88.46KiB |
Bundle analysis report Branch Yradex:slice/element-template/04 Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#1010 Bundle Size — 205.14KiB (0%).edcd696(current) vs 0d51ee8 main#1004(baseline) Bundle metrics
|
| Current #1010 |
Baseline #1004 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
183 |
183 |
| Duplicate Modules |
73 |
73 |
| Duplicate Code |
44.6% |
44.6% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #1010 |
Baseline #1004 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
93.91KiB |
93.91KiB |
Bundle analysis report Branch Yradex:slice/element-template/04 Project dashboard
Generated by RelativeCI Documentation Report issue
React External#995 Bundle Size — 685.69KiB (0%).edcd696(current) vs 0d51ee8 main#989(baseline) Bundle metrics
|
| Current #995 |
Baseline #989 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle analysis report Branch Yradex:slice/element-template/04 Project dashboard
Generated by RelativeCI Documentation Report issue
Yradex
force-pushed
the
slice/element-template/04
branch
from
1897322 to
cd6685e
Compare
Yradex
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react/runtime/src/element-template/runtime/patch.ts (1)
88-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegistry cleanup is skipped on error paths, leaking handles permanently
When
resolveHandlefails fortargetIdorchildId(lines 93–96), thecontinuejumps back to the top of thewhileloop and theremovedSubtreeHandleIdscleanup (lines 101–103) is never executed. Since the background side has already committed the removal before dispatching this command, those handle IDs will never be reused — but theirElementTemplateRegistryentries remain, preventing GC of the native subtree forever.The registry cleanup is independent of the native API call: even if
__RemoveNodeFromElementTemplatecannot be invoked, the JS-side strong refs should still be released.🐛 Proposed fix — move cleanup before the native guard
case ElementTemplateUpdateOps.removeNode: { const targetId = stream[i++] as number; const elementSlotIndex = stream[i++] as number; const childId = stream[i++] as number; const removedSubtreeHandleIds = stream[i++] as number[]; const nativeRef = resolveHandle(targetId, 'target'); const childRef = resolveHandle(childId, 'child'); + // Always release strong refs — background has already committed the + // removal and will never reuse these handle IDs, regardless of whether + // the native detach succeeds. + for (const handleId of removedSubtreeHandleIds) { + ElementTemplateRegistry.delete(handleId); + } if (!nativeRef || !childRef) { continue; } __RemoveNodeFromElementTemplate(nativeRef, elementSlotIndex, childRef); - // The native API only detaches from the slot. Releasing ET runtime's - // strong refs after a successful detach lets JS GC reclaim the subtree. - for (const handleId of removedSubtreeHandleIds) { - ElementTemplateRegistry.delete(handleId); - } break; }🤖 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/src/element-template/runtime/patch.ts` around lines 88 - 105, The removeNode branch (ElementTemplateUpdateOps.removeNode) currently skips clearing removedSubtreeHandleIds from ElementTemplateRegistry when resolveHandle(targetId or childId) fails; move the cleanup so that the loop over removedSubtreeHandleIds and ElementTemplateRegistry.delete(handleId) executes regardless of resolveHandle success, i.e., perform the registry cleanup immediately after reading removedSubtreeHandleIds and before the native guard/continue, and then only call __RemoveNodeFromElementTemplate(nativeRef, elementSlotIndex, childRef) if both resolveHandle(targetId, 'target') and resolveHandle(childId, 'child') returned refs.
🤖 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/element-template/runtime/patch.ts`:
- Line 92: The code casts stream[i++] to removedSubtreeHandleIds without
checking it, which can throw if it's not iterable; in
applyElementTemplateUpdateCommands (in patch.ts) add a defensive guard right
after assigning removedSubtreeHandleIds: verify
Array.isArray(removedSubtreeHandleIds) (or at minimum typeof !== 'undefined' and
Symbol.iterator in object) and if not, replace it with an empty array (and
optionally log/debug) before the for (const handleId of removedSubtreeHandleIds)
loop so a malformed stream entry doesn't abort the entire patch loop.
---
Outside diff comments:
In `@packages/react/runtime/src/element-template/runtime/patch.ts`:
- Around line 88-105: The removeNode branch
(ElementTemplateUpdateOps.removeNode) currently skips clearing
removedSubtreeHandleIds from ElementTemplateRegistry when resolveHandle(targetId
or childId) fails; move the cleanup so that the loop over
removedSubtreeHandleIds and ElementTemplateRegistry.delete(handleId) executes
regardless of resolveHandle success, i.e., perform the registry cleanup
immediately after reading removedSubtreeHandleIds and before the native
guard/continue, and then only call __RemoveNodeFromElementTemplate(nativeRef,
elementSlotIndex, childRef) if both resolveHandle(targetId, 'target') and
resolveHandle(childId, 'child') returned refs.
🪄 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: eb19b5c9-8e46-4ead-82f1-b40aa157a619
📒 Files selected for processing (16)
packages/react/runtime/__test__/element-template/debug/alog.test.tspackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate-compiled/children.mixed-operations/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate-compiled/children.removes-missing/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate/children.creates-missing-nodes-recursively/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate/children.missing-slot-record-on-background/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate/coverage.raw-text-key-branches/output.txtpackages/react/runtime/__test__/element-template/runtime/background/hydrate.test.tspackages/react/runtime/__test__/element-template/runtime/background/instance.test.tspackages/react/runtime/__test__/element-template/runtime/background/update/compiled-fixtures.test.tsxpackages/react/runtime/__test__/element-template/runtime/patch/element-template-patch.test.tsxpackages/react/runtime/__test__/element-template/test-utils/debug/updateRunner.test.tsxpackages/react/runtime/__test__/element-template/test-utils/debug/updateRunner.tspackages/react/runtime/src/element-template/background/instance.tspackages/react/runtime/src/element-template/debug/alog.tspackages/react/runtime/src/element-template/protocol/types.tspackages/react/runtime/src/element-template/runtime/patch.ts
Yradex
force-pushed
the
slice/element-template/04
branch
from
cd6685e to
edcd696
Compare
HuJean
approved these changes
May 8, 2026
This was referenced May 8, 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.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Overview
removeNodecarry the exact subtree handle ids that should be released, so the main-thread patch executor can clean the registry after native detach succeeds.__RemoveNodeFromElementTemplatestill receives only the resolved target slot and child ref; registry cleanup remains an internal runtime follow-up.Runtime Contract
The
removeNodecommand now has an explicit tuple shape:The important ordering is:
Key Points
Checklist