refactor: create element without ref for suspense in main thread#2426
refactor: create element without ref for suspense in main thread#2426
Conversation
🦋 Changeset detectedLatest commit: 1faef9f 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 |
📝 WalkthroughWalkthroughThis PR introduces a patch-level change to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react/runtime/src/lynx/suspense.ts (1)
21-47: Please add regression tests for thread-gated Suspense wrapper refs.Given the split behavior between main/background threads, add targeted tests to lock this contract and prevent ref-related regressions in future refactors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/lynx/suspense.ts` around lines 21 - 47, Add regression tests that assert the thread-gated ref contract around the Suspense wrapper implemented via __createElement: when __MAIN_THREAD__ is false the wrapper should attach refs (verify childrenRef.current becomes a BackgroundSnapshotInstance after rendering and that globalBackgroundSnapshotInstancesToRemove is updated/cleared when newFallback's ref runs), and when __MAIN_THREAD__ is true no ref callbacks should be invoked. Create tests that toggle __MAIN_THREAD__, render the Suspense wrapper that uses newChildren and newFallback (or import the Suspense wrapper behavior), simulate mounting of children then fallback to trigger the ref callbacks, and assert childrenRef mutations and globalBackgroundSnapshotInstancesToRemove changes to lock this behavior against regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react/runtime/src/lynx/suspense.ts`:
- Around line 21-47: Add regression tests that assert the thread-gated ref
contract around the Suspense wrapper implemented via __createElement: when
__MAIN_THREAD__ is false the wrapper should attach refs (verify
childrenRef.current becomes a BackgroundSnapshotInstance after rendering and
that globalBackgroundSnapshotInstancesToRemove is updated/cleared when
newFallback's ref runs), and when __MAIN_THREAD__ is true no ref callbacks
should be invoked. Create tests that toggle __MAIN_THREAD__, render the Suspense
wrapper that uses newChildren and newFallback (or import the Suspense wrapper
behavior), simulate mounting of children then fallback to trigger the ref
callbacks, and assert childrenRef mutations and
globalBackgroundSnapshotInstancesToRemove changes to lock this behavior against
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2a4c4f8-61c3-4962-ab57-a9972058c943
📒 Files selected for processing (2)
.changeset/legal-zebras-sit.mdpackages/react/runtime/src/lynx/suspense.ts
Merging this PR will improve performance by 17.23%
Performance Changes
Comparing Footnotes
|
React External#213 Bundle Size — 591.38KiB (-0.02%).1faef9f(current) vs a822afc main#212(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch p/suspense-mainthread Project dashboard Generated by RelativeCI Documentation Report issue |
Web Explorer#8669 Bundle Size — 728.84KiB (0%).1faef9f(current) vs a822afc main#8668(baseline) Bundle metrics
|
| Current #8669 |
Baseline #8668 |
|
|---|---|---|
43.31KiB |
43.31KiB |
|
2.16KiB |
2.16KiB |
|
0% |
0% |
|
8 |
8 |
|
10 |
10 |
|
149 |
149 |
|
11 |
11 |
|
34.69% |
34.69% |
|
3 |
3 |
|
0 |
0 |
Bundle size by type no changes
| Current #8669 |
Baseline #8668 |
|
|---|---|---|
384.62KiB |
384.62KiB |
|
342.07KiB |
342.07KiB |
|
2.16KiB |
2.16KiB |
Bundle analysis report Branch p/suspense-mainthread Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7094 Bundle Size — 237.68KiB (0%).1faef9f(current) vs a822afc main#7093(baseline) Bundle metrics
|
| Current #7094 |
Baseline #7093 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
38.71% |
|
0 |
0 |
|
4 |
4 |
|
179 |
179 |
|
70 |
70 |
|
46.12% |
46.12% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #7094 |
Baseline #7093 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
91.92KiB |
91.92KiB |
Bundle analysis report Branch p/suspense-mainthread Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#227 Bundle Size — 207.26KiB (0%).1faef9f(current) vs a822afc main#226(baseline) Bundle metrics
Bundle size by type
|
| Current #227 |
Baseline #226 |
|
|---|---|---|
111.23KiB |
111.23KiB |
|
96.03KiB |
96.03KiB |
Bundle analysis report Branch p/suspense-mainthread Project dashboard
Generated by RelativeCI Documentation Report issue
Summary by CodeRabbit
Checklist