feat(react): clean element template state on destroy#2579
Conversation
🦋 Changeset detectedLatest commit: 2c76078 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThe PR refactors element-template runtime destruction by introducing explicit background and main-thread destroy functions with deterministic cleanup ordering, adding timer-based tracking for delayed removed-subtree teardown, decoupling hydration-listener reset from commit-state clearing, and updating call sites and tests to verify the new cleanup behavior. ChangesElement Template Destruction Refactor
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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/__test__/element-template/native/mts-destroy.test.ts (1)
52-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
g.lynx.getNativein the performance-hooks test to avoid cross-test leakage.This test mutates
g.lynx.getNativebut only restoresg.lynx.performance. That can pollute later tests and create order-dependent failures.Suggested fix
it('does not throw when performance hooks are missing', () => { const g = globalThis as typeof globalThis & { lynx: typeof lynx & { performance: Partial<typeof lynx.performance>; }; }; + const originalGetNative = g.lynx.getNative; const originalPerformance = g.lynx.performance; const removeEventListener = vi.fn(); - g.lynx.getNative = () => ({ addEventListener: vi.fn(), removeEventListener }); - g.lynx.performance = {}; - - expect(() => onMtsDestruction()).not.toThrow(); - expect(removeEventListener).toHaveBeenCalledWith('__DestroyLifetime', onMtsDestruction); - - g.lynx.performance = originalPerformance; + try { + g.lynx.getNative = () => ({ addEventListener: vi.fn(), removeEventListener }); + g.lynx.performance = {}; + + expect(() => onMtsDestruction()).not.toThrow(); + expect(removeEventListener).toHaveBeenCalledWith('__DestroyLifetime', onMtsDestruction); + } finally { + g.lynx.getNative = originalGetNative; + g.lynx.performance = originalPerformance; + } });🤖 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__/element-template/native/mts-destroy.test.ts` around lines 52 - 68, The test mutates g.lynx.getNative but never restores it, risking cross-test leakage; capture the original g.lynx.getNative before you overwrite it (e.g., store in a variable like originalGetNative), then restore g.lynx.getNative = originalGetNative at the end of the test (after the expect assertions). Keep references to the existing symbols onMtsDestruction and g.lynx.performance as-is and only add saving/restoring of g.lynx.getNative to prevent order-dependent failures.
🤖 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.
Outside diff comments:
In `@packages/react/runtime/__test__/element-template/native/mts-destroy.test.ts`:
- Around line 52-68: The test mutates g.lynx.getNative but never restores it,
risking cross-test leakage; capture the original g.lynx.getNative before you
overwrite it (e.g., store in a variable like originalGetNative), then restore
g.lynx.getNative = originalGetNative at the end of the test (after the expect
assertions). Keep references to the existing symbols onMtsDestruction and
g.lynx.performance as-is and only add saving/restoring of g.lynx.getNative to
prevent order-dependent failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4aefdfd3-d056-4a54-9a7d-68d583687d40
📒 Files selected for processing (10)
.changeset/empty-et-destroy-cleanup.mdpackages/react/runtime/__test__/element-template/native/callDestroyLifetimeFun.test.tspackages/react/runtime/__test__/element-template/native/mts-destroy.test.tspackages/react/runtime/__test__/element-template/runtime/background/commit-hook.test.tspackages/react/runtime/__test__/element-template/runtime/hydration/hydration-listener.test.tspackages/react/runtime/src/element-template/background/commit-hook.tspackages/react/runtime/src/element-template/background/destroy.tspackages/react/runtime/src/element-template/background/hydration-listener.tspackages/react/runtime/src/element-template/native/callDestroyLifetimeFun.tspackages/react/runtime/src/element-template/native/mts-destroy.ts
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 16.22%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | 002-hello-reactLynx-destroyBackground |
912.4 µs | 863.4 µs | +5.67% |
| ❌ | 008-many-use-state-destroyBackground |
8 ms | 9.5 ms | -16.22% |
Comparing Yradex:slice/element-template/06 (2c76078) with main (dcd0b98)2
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. ↩
-
No successful run was found on
main(736ba38) during the generation of this report, so dcd0b98 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
Yradex
force-pushed
the
slice/element-template/06
branch
from
1950cda to
2c76078
Compare
React Example#7915 Bundle Size — 235.77KiB (0%).2c76078(current) vs dcd0b98 main#7895(baseline) Bundle metrics
|
| Current #7915 |
Baseline #7895 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
197 |
197 |
| Duplicate Modules |
80 |
80 |
| Duplicate Code |
44.85% |
44.85% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7915 |
Baseline #7895 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
90.01KiB |
90.01KiB |
Bundle analysis report Branch Yradex:slice/element-template/06 Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#1045 Bundle Size — 206.69KiB (0%).2c76078(current) vs dcd0b98 main#1025(baseline) Bundle metrics
|
| Current #1045 |
Baseline #1025 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
192 |
192 |
| Duplicate Modules |
77 |
77 |
| Duplicate Code |
44.36% |
44.36% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #1045 |
Baseline #1025 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
95.46KiB |
95.46KiB |
Bundle analysis report Branch Yradex:slice/element-template/06 Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9487 Bundle Size — 900.02KiB (0%).2c76078(current) vs dcd0b98 main#9467(baseline) Bundle metrics
|
| Current #9487 |
Baseline #9467 |
|
|---|---|---|
| Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
229 |
229 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.28% |
27.28% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9487 |
Baseline #9467 |
|
|---|---|---|
| JS |
495.88KiB |
495.88KiB |
| Other |
401.92KiB |
401.92KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch Yradex:slice/element-template/06 Project dashboard
Generated by RelativeCI Documentation Report issue
React External#1030 Bundle Size — 690.27KiB (0%).2c76078(current) vs dcd0b98 main#1010(baseline) Bundle metrics
|
| Current #1030 |
Baseline #1010 |
|
|---|---|---|
| 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/06 Project dashboard
Generated by RelativeCI Documentation Report issue
React Example with Element Template#180 Bundle Size — 198.02KiB (-0.41%).2c76078(current) vs dcd0b98 main#160(baseline) Bundle metrics
|
| Current #180 |
Baseline #160 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
26.7% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
81(+3.85%) |
78 |
| Duplicate Modules |
23 |
23 |
| Duplicate Code |
40.27%(-0.15%) |
40.33% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 improvement
| Current #180 |
Baseline #160 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
52.26KiB (-1.54%) |
53.08KiB |
Bundle analysis report Branch Yradex:slice/element-template/06 Project dashboard
Generated by RelativeCI Documentation Report issue
2 participants
Summary by CodeRabbit
Tests
Chores
Overview
This PR makes Element Template destroy cleanup release the runtime-owned state on both background and main-thread paths.
Before this change, destroy paths only reset listeners. That left two ET-owned retention points outside the normal removed-subtree cleanup flow: delayed background cleanup timers could still run after runtime teardown, and the main-thread ElementTemplate registry could keep strong references after the page lifetime ended.
Key Points
Checklist