feat(react/runtime): support using a host element as direct child of Suspense #1308
Conversation
🦋 Changeset detectedLatest commit: 41d6a93 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 change introduces a new internal implementation of the Suspense component in the Lynx React runtime, enabling a host element to be a direct child of Suspense. It updates background snapshot logic to support tree reconstruction, adds comprehensive Suspense tests covering lifecycle and edge cases, and adjusts test snapshots to reflect new wrapper elements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Yradex
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging #1308 will not alter performanceComparing Summary
|
React Example#3717 Bundle Size — 235.26KiB (+0.05%).41d6a93(current) vs 29434ae main#3712(baseline) Bundle metrics
|
| Current #3717 |
Baseline #3712 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
38.01% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
159(+1.92%) |
156 |
 Duplicate Modules |
64(+1.59%) |
63 |
| Duplicate Code |
45.81%(-0.28%) |
45.94% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #3717 |
Baseline #3712 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
89.5KiB (+0.14%) |
89.38KiB |
Bundle analysis report Branch Yradex:suspense/element-in-suspe... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#3707 Bundle Size — 342.52KiB (0%).41d6a93(current) vs 29434ae main#3702(baseline) Bundle metrics
|
| Current #3707 |
Baseline #3702 |
|
|---|---|---|
| Initial JS |
142.05KiB |
142.05KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
65.76% |
| Chunks |
7 |
7 |
| Assets |
7 |
7 |
| Modules |
211 |
211 |
| Duplicate Modules |
17 |
17 |
| Duplicate Code |
3.96% |
3.96% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #3707 |
Baseline #3702 |
|
|---|---|---|
| JS |
227.57KiB |
227.57KiB |
| Other |
83.11KiB |
83.11KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch Yradex:suspense/element-in-suspe... Project dashboard
Generated by RelativeCI Documentation Report issue
Yradex
force-pushed
the
suspense/element-in-suspense
branch
2 times, most recently
from
31134dc to
674fa03
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/react/runtime/src/snapshot.ts (1)
579-582: Potential issue with marking list items as deletedSetting
child.__id = 0for list items could cause issues if the instance is accessed before actual destruction. Consider using a different approach to mark deletion state.Consider adding a dedicated flag instead:
this.__removeChild(child); - // mark this child as deleted - child.__id = 0; + // mark this child as deleted + child.__markedForDeletion = true; return;
🧹 Nitpick comments (6)
packages/react/runtime/src/list.ts (1)
147-151: Guard fix looks good; consider tightening the flowThe new
if (__id) … else …guard preventsunRenderElements()from running on instances without an element – 👍.
You could fold the subsequentif (!__id) … else if …into the first branch to avoid checking__idtwice, but functionally it’s fine.packages/react/runtime/__test__/preact.test.jsx (1)
17-18: Consider moving cleanup to a shared helperMultiple test files now replicate
destroyRemovedSnapshotInstances()calls.
Extract a small helper or add it to a globalafterEachhook to avoid duplication and guarantee coverage across new tests.packages/react/runtime/__test__/snapshotPatch.test.jsx (1)
710-730: Consider adding a success case test for PreventDestroy.While the error case is well-tested, consider adding a test that verifies the successful execution of PreventDestroy on a valid snapshot instance.
describe('PreventDestroy', () => { beforeEach(() => { initGlobalSnapshotPatch(); }); it('error', async function() { initGlobalSnapshotPatch(); const patch = takeGlobalSnapshotPatch(); patch.push(SnapshotOperation.PreventDestroy, 100); snapshotPatchApply(patch); expect(_ReportError).toHaveBeenCalledTimes(1); expect(_ReportError.mock.calls[0]).toMatchInlineSnapshot(` [ [Error: snapshotPatchApply failed: ctx not found, snapshot type: 'null'], { "errorCode": 1101, }, ] `); }); + + it('success', async function() { + const bsi1 = new BackgroundSnapshotInstance(snapshot1); + let patch = takeGlobalSnapshotPatch(); + snapshotPatchApply(patch); + + const si1 = snapshotInstanceManager.values.get(bsi1.__id); + const preventDestroySpy = vi.spyOn(si1, 'preventDestroy'); + + patch = takeGlobalSnapshotPatch(); + patch.push(SnapshotOperation.PreventDestroy, bsi1.__id); + snapshotPatchApply(patch); + + expect(preventDestroySpy).toHaveBeenCalledTimes(1); + expect(_ReportError).not.toHaveBeenCalled(); + }); });packages/react/runtime/src/lynx/suspense.ts (1)
22-23: Consider defining a proper type for the wrapper element.Instead of suppressing TypeScript errors with
@ts-expect-error, consider properly typing the wrapper element or using a type assertion.+ type WrapperProps = { + ref?: (instance: BackgroundSnapshotInstance) => void; + children?: VNode | VNode[]; + }; + // @ts-expect-error wrapper is a valid element type - const newChildren = __createElement('wrapper', { + const newChildren = __createElement('wrapper' as any, { ref: (bsi: BackgroundSnapshotInstance) => { if (bsi) { childrenRef.current = bsi; } }, - }, children); + } as WrapperProps, children);Also applies to: 31-32
packages/react/runtime/src/snapshot.ts (1)
644-650: Consider optimization for frequent operationsThe current implementation has O(n) complexity for both search and removal. If this method is called frequently or the array grows large, consider using a Set for better performance.
If performance becomes a concern:
-export const snapshotInstancesToDestroy: number[] = []; +export const snapshotInstancesToDestroy = new Set<number>(); export function destroyRemovedSnapshotInstances(): void { - for (const id of snapshotInstancesToDestroy) { + for (const id of snapshotInstancesToDestroy) { snapshotInstanceManager.values.get(id)?.tearDown(); } - snapshotInstancesToDestroy.length = 0; + snapshotInstancesToDestroy.clear(); } // In removeChild: - snapshotInstancesToDestroy.push(child.__id); + snapshotInstancesToDestroy.add(child.__id); // In preventDestroy: preventDestroy(): void { - const index = snapshotInstancesToDestroy.indexOf(this.__id); - if (index !== -1) { - snapshotInstancesToDestroy.splice(index, 1); - } + snapshotInstancesToDestroy.delete(this.__id); }packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
450-459: Address TODO: Clean up wrapper elements from snapshotInstanceManagerThe comment indicates that wrapper elements created by
createElementin suspense (-2and-3) remain insnapshotInstanceManager. This could lead to memory leaks over time.Would you like me to help create a cleanup mechanism for these wrapper elements or open an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.changeset/fast-coats-swim.md(1 hunks)packages/react/runtime/__test__/basic.test.jsx(2 hunks)packages/react/runtime/__test__/debug/formatPatch.test.ts(2 hunks)packages/react/runtime/__test__/lynx/suspense.test.jsx(1 hunks)packages/react/runtime/__test__/preact.test.jsx(2 hunks)packages/react/runtime/__test__/renderToOpcodes.test.jsx(2 hunks)packages/react/runtime/__test__/snapshotPatch.test.jsx(1 hunks)packages/react/runtime/src/backgroundSnapshot.ts(2 hunks)packages/react/runtime/src/index.ts(1 hunks)packages/react/runtime/src/internal.ts(1 hunks)packages/react/runtime/src/lifecycle/patch/snapshotPatch.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/snapshotPatchApply.ts(1 hunks)packages/react/runtime/src/lifecycle/patch/updateMainThread.ts(2 hunks)packages/react/runtime/src/list.ts(1 hunks)packages/react/runtime/src/lynx/suspense.ts(1 hunks)packages/react/runtime/src/snapshot.ts(4 hunks)packages/react/testing-library/src/__tests__/lazy-bundle/index.test.jsx(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.263Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-core-server/src/createLynxView.ts:0-0
Timestamp: 2025-07-16T06:26:22.177Z
Learning: In the lynx-stack SSR implementation, each createLynxView instance is used to render once and then discarded. There's no reuse of the same instance for multiple renders, so event arrays and other state don't need to be cleared between renders.
packages/react/runtime/src/index.ts (1)
Learnt from: colinaaa
PR: #1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.263Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
packages/react/runtime/__test__/basic.test.jsx (1)
Learnt from: colinaaa
PR: #1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.263Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
packages/react/runtime/__test__/preact.test.jsx (1)
Learnt from: colinaaa
PR: #1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.263Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
packages/react/runtime/src/internal.ts (4)
Learnt from: colinaaa
PR: #1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.263Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
Learnt from: PupilTong
PR: #1029
File: packages/web-platform/web-core-server/src/createLynxView.ts:0-0
Timestamp: 2025-07-16T06:26:22.177Z
Learning: In the lynx-stack SSR implementation, each createLynxView instance is used to render once and then discarded. There's no reuse of the same instance for multiple renders, so event arrays and other state don't need to be cleared between renders.
Learnt from: PupilTong
PR: #1029
File: packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts:95-99
Timestamp: 2025-07-16T06:28:26.421Z
Learning: In the lynx-stack codebase, CSS selectors in SSR hydration are generated by their own packages, ensuring a predictable format that makes simple string manipulation safe and preferable over regex for performance reasons.
Learnt from: PupilTong
PR: #1292
File: packages/web-platform/web-core-server/src/createLynxView.ts:144-151
Timestamp: 2025-07-15T10:00:56.154Z
Learning: In the lynx-stack codebase, PupilTong prefers the "let it crash" approach over defensive null safety checks when the condition should never occur in normal operation. This applies to cases like the element.getAttribute(lynxUniqueIdAttribute)! call in SSR event capture where the attribute is expected to always be present.
packages/react/runtime/__test__/renderToOpcodes.test.jsx (1)
Learnt from: colinaaa
PR: #1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.263Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
packages/react/runtime/src/list.ts (1)
Learnt from: colinaaa
PR: #1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.263Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
packages/react/runtime/src/lynx/suspense.ts (1)
Learnt from: colinaaa
PR: #1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.263Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
Learnt from: colinaaa
PR: #1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.263Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
🧬 Code Graph Analysis (6)
packages/react/runtime/src/lifecycle/patch/updateMainThread.ts (1)
packages/react/runtime/src/snapshot.ts (1)
destroyRemovedSnapshotInstances(185-190)
packages/react/runtime/__test__/basic.test.jsx (1)
packages/react/runtime/src/snapshot.ts (2)
snapshotInstanceManager(123-134)destroyRemovedSnapshotInstances(185-190)
packages/react/runtime/__test__/preact.test.jsx (1)
packages/react/runtime/src/snapshot.ts (1)
destroyRemovedSnapshotInstances(185-190)
packages/react/runtime/__test__/debug/formatPatch.test.ts (1)
packages/react/runtime/src/lifecycle/patch/snapshotPatch.ts (1)
SnapshotOperation(11-21)
packages/react/runtime/__test__/renderToOpcodes.test.jsx (1)
packages/react/runtime/src/snapshot.ts (1)
destroyRemovedSnapshotInstances(185-190)
packages/react/runtime/src/backgroundSnapshot.ts (1)
packages/react/runtime/src/lifecycle/patch/snapshotPatch.ts (2)
__globalSnapshotPatch(58-58)SnapshotOperation(11-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright (Default) / check
🔇 Additional comments (18)
packages/react/runtime/src/lifecycle/patch/updateMainThread.ts (1)
14-14: LGTM: Proper integration of deferred snapshot cleanup.The addition of
destroyRemovedSnapshotInstances()before__FlushElementTreeensures that stale snapshot instances are cleaned up at the appropriate time in the render cycle, which is essential for the new Suspense implementation's snapshot lifecycle management.Also applies to: 54-54
packages/react/runtime/src/internal.ts (1)
13-13: Verify Suspense API compatibility with Preact/ReactPlease ensure that switching from
preact/compat’s Suspense to our internal implementation maintains the same public API:
- File under review:
packages/react/runtime/src/internal.tsat the import on line 13- import { Suspense } from 'preact/compat'; + import { Suspense } from './lynx/suspense.js';- Confirm that our internal
Suspense(inpackages/react/runtime/src/lynx/suspense.ts):
- Exports a
FunctionComponentnamedSuspense- Accepts the same props as Preact/React’s Suspense (especially
childrenandfallback)- Has the same
fallbacktype (e.g. optional vs. required, accepts arrays or strings)- Behaves identically when rendering and unmounting (snapshot lifecycle)
No changes to the API signature or behavior should be introduced.
packages/react/runtime/__test__/debug/formatPatch.test.ts (1)
25-26: LGTM: Comprehensive test coverage for PreventDestroy operation.The test properly covers the new
PreventDestroysnapshot operation with both input format (operation code + ID) and expected formatted output, ensuring the debug formatting function works correctly.Also applies to: 45-45
packages/react/runtime/src/index.ts (1)
36-36: LGTM: Consistent switch to internal Suspense implementation.The import change properly exposes the new internal Suspense component through the main export, ensuring all consumers get the enhanced functionality for supporting host elements as direct children.
.changeset/fast-coats-swim.md (1)
1-6: LGTM: Accurate changeset entry.The changeset correctly categorizes this as a patch version update for
@lynx-js/reactand accurately describes the new feature allowing host elements as direct children of Suspense.packages/react/runtime/__test__/renderToOpcodes.test.jsx (1)
686-687: Great to flush pending destructionsCalling
destroyRemovedSnapshotInstances()here makes the assertion deterministic.
Just make sure the same call is present in the suite’safterEach, otherwise later tests may still clear without teardown.packages/react/runtime/__test__/preact.test.jsx (1)
177-178: Good explicit cleanup, but still clear the queue for following testsSame remark: ensure this call (or a global hook) precedes any direct
snapshotInstanceManager.clear()to preservetearDown()semantics.packages/react/testing-library/src/__tests__/lazy-bundle/index.test.jsx (1)
37-44: Snapshot update reflects new Suspense wrapperSnapshots now show the internal
<wrapper>introduced by the custom Suspense implementation – looks correct.Also applies to: 52-59
packages/react/runtime/src/backgroundSnapshot.ts (2)
41-44: LGTM! The conditional logic aligns with the Suspense implementation.The exclusion of 'div' elements from the snapshot patch is appropriate for the new Suspense implementation, which uses wrapper elements that should not be created in the main thread.
78-80: No usages of BackgroundSnapshotInstance.appendChild foundA full search of
.ts/.tsx/.js/.jsxfiles revealed no calls toappendChildonBackgroundSnapshotInstanceoutside its own definition. Disabling this method has no current impact on the codebase.packages/react/runtime/src/lifecycle/patch/snapshotPatch.ts (1)
17-17: LGTM! The PreventDestroy operation is properly integrated.The new operation follows the established pattern with sequential numbering and appropriate parameter definition.
Also applies to: 35-38
packages/react/runtime/src/lifecycle/patch/snapshotPatchApply.ts (1)
92-101: LGTM! Consistent implementation with proper error handling.The PreventDestroy case follows the established pattern for snapshot operations with appropriate error handling when the context is not found.
packages/react/runtime/src/lynx/suspense.ts (2)
18-19: LGTM! Appropriate thread-aware createElement selection.The conditional selection of createElement based on the thread context ensures proper behavior in both main and background threads.
33-42: No race condition in the fallback ref callback
The fallback ref handler explicitly checkschildrenRef.currentand only runs when it’s been set, so it safely no-ops if the fallback mounts before the children. Preact’s Suspense update sequence (as exercised by the existingsuspense.test.jsx) ensures that after hydration the child wrapper ref fires before the next fallback mount, and the test suite confirms that thePreventDestroypatch is applied at the correct time.packages/react/runtime/src/snapshot.ts (1)
407-416: Good improvement to cleanup logicThe use of
deleteoperator for property removal and the cleanup fromsnapshotInstanceManager.valuesduring traversal ensures proper memory cleanup.packages/react/runtime/__test__/lynx/suspense.test.jsx (3)
214-412: Excellent test coverage for the main featureThis test case thoroughly validates the PR's main objective of supporting host elements as direct children of Suspense. The verification of the
PreventDestroyoperation in the snapshot patch is particularly important for ensuring the deferred destruction mechanism works correctly.
1-1629: Comprehensive and well-structured test suiteThis test file provides excellent coverage for the new Suspense implementation. The tests are well-organized, clearly documented, and thoroughly validate the integration with the snapshot lifecycle management system, including the new PreventDestroy operation and deferred destruction mechanism.
41-41: Verify Promise.withResolvers() CompatibilityEnsure your test environment supports the modern
Promise.withResolvers()API (ES2024) or include a polyfill:• Browser support starts at Chrome 119, Edge 119, Firefox 121, Safari 17.4, Opera 105 (no support in IE).
• Node.js support begins in v22.0.0; older versions will throw “Promise.withResolvers is undefined.”If your CI or local setup targets Node < 22 or browsers below these versions, add a polyfill (e.g., core-js or a small custom shim) to avoid test failures.
Yradex
force-pushed
the
suspense/element-in-suspense
branch
5 times, most recently
from
b259641 to
b702631
Compare
Yradex
force-pushed
the
suspense/element-in-suspense
branch
from
b702631 to
231a08f
Compare
…Suspense (lynx-family#1308) ## Summary Support using a host element as direct child of Suspense. ## Checklist - [x] Tests updated (or not required). - [ ] Documentation updated (or not required). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved Suspense component now allows direct use of host elements (such as DOM elements) as children. * **Bug Fixes** * Enhanced lifecycle management to ensure proper cleanup and restoration of elements during Suspense operations. * Introduced explicit tracking and reconstruction of removed nodes to improve rendering consistency. * **Tests** * Added comprehensive tests covering Suspense behaviors, including fallback rendering, cleanup, nested and parallel Suspense, error boundaries, and resolved promises. * **Style** * Updated test snapshots to reflect new wrapper structure in rendered output. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary
Support using a host element as direct child of Suspense.
Checklist
Summary by CodeRabbit