feat(react/runtime): support using a host element as direct child of Suspense #1455
Conversation
…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 -->
🦋 Changeset detectedLatest commit: 3f74f7d 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 custom Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
1635-1636: Missing closing brace for describe blockThe file ends without closing the
describe('suspense', () => {block that starts at line 38. Add a closing brace to fix the syntax error.} }); +});
🧹 Nitpick comments (2)
packages/react/runtime/src/lynx/suspense.ts (1)
21-22: Consider defining a proper type for the wrapper element.Instead of using
@ts-expect-error, consider properly typing the wrapper element to improve type safety and maintainability.You could define a type declaration for the wrapper element:
declare module 'preact' { namespace JSX { interface IntrinsicElements { wrapper: HTMLAttributes<HTMLElement>; } } }Also applies to: 30-31
packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
455-463: Track the TODO for wrapper cleanupThe TODO comment indicates that wrapper elements (
-2and--3) created bycreateElementin suspense should be removed fromsnapshotInstanceManager. Consider creating an issue to track this cleanup task to prevent potential memory leaks.Would you like me to create an issue to track this cleanup task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/fast-coats-swim.md(1 hunks)packages/react/runtime/__test__/lynx/suspense.test.jsx(1 hunks)packages/react/runtime/src/backgroundSnapshot.ts(5 hunks)packages/react/runtime/src/index.ts(1 hunks)packages/react/runtime/src/lynx/suspense.ts(1 hunks)packages/react/testing-library/src/__tests__/lazy-bundle/index.test.jsx(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 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.291Z
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.230Z
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.
📚 Learning: in the lynx-family/lynx-stack repository, the file `packages/react/testing-library/src/vitest.config...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.139Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/react/testing-library/src/vitest.config.js` is source code for the testing library that gets exported for users, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/react/testing-library/src/__tests__/lazy-bundle/index.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/src/index.ts
📚 Learning: in the lynx-family/lynx-stack repository, the file `packages/rspeedy/create-rspeedy/template-react-v...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.139Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/rspeedy/create-rspeedy/template-react-vitest-rltl-js/vitest.config.js` is a template file for scaffolding new Rspeedy projects, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/react/testing-library/src/__tests__/lazy-bundle/index.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/src/index.ts
📚 Learning: in the lynx-family/lynx-stack repository, the webpack patch (patches/[email protected]) was crea...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1454
File: pnpm-workspace.yaml:46-46
Timestamp: 2025-08-07T04:00:59.627Z
Learning: In the lynx-family/lynx-stack repository, the webpack patch (patches/webpack5.101.0.patch) was created to fix issues with webpack5.99.9 but only takes effect on webpack5.100.0 and later versions. The patchedDependencies entry should use "webpack@^5.100.0" to ensure the patch applies to the correct version range.
Applied to files:
.changeset/fast-coats-swim.mdpackages/react/runtime/src/index.ts
📚 Learning: in the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Applied to files:
.changeset/fast-coats-swim.md
📚 Learning: in the lynx-family/lynx-stack repository, ci checks require changesets when files matching the patte...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
Applied to files:
.changeset/fast-coats-swim.md
📚 Learning: the component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork ...
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.291Z
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.
Applied to files:
packages/react/runtime/src/lynx/suspense.tspackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/src/index.tspackages/react/runtime/src/backgroundSnapshot.ts
🔇 Additional comments (16)
.changeset/fast-coats-swim.md (1)
1-5: LGTM!The changeset correctly documents this feature addition as a patch update.
packages/react/runtime/src/index.ts (1)
36-36: LGTM!The switch to the custom Suspense implementation is clean and maintains the same export interface.
packages/react/testing-library/src/__tests__/lazy-bundle/index.test.jsx (1)
37-43: LGTM!The snapshot updates correctly reflect the new wrapper element structure introduced by the custom Suspense component.
Also applies to: 52-59
packages/react/runtime/src/lynx/suspense.ts (1)
34-38: Consider thread safety for the global removal list manipulation.The splice operation on
globalBackgroundSnapshotInstancesToRemovecould potentially have race conditions if multiple Suspense boundaries interact simultaneously. Additionally, settingchildrenRef.current = undefinedimmediately after might cause issues if the ref is accessed elsewhere before garbage collection.Consider adding defensive checks:
if (bsi && childrenRef.current) { const i = globalBackgroundSnapshotInstancesToRemove.indexOf(childrenRef.current.__id); if (i !== -1) { globalBackgroundSnapshotInstancesToRemove.splice(i, 1); } - childrenRef.current = undefined; + // Defer cleanup to avoid potential access issues + queueMicrotask(() => { + childrenRef.current = undefined; + }); }⛔ Skipped due to 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.291Z 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/backgroundSnapshot.ts (2)
510-526: ReconstructInstanceTree is a private helper – no export neededThe
reconstructInstanceTreefunction is only invoked withinbackgroundSnapshot.ts(via the lazy/Suspense appendChild paths) and isn’t imported elsewhere. It’s intentionally scoped to this module, so there’s no need to add an export. No changes required.Likely an incorrect or invalid review comment.
516-517: Potential data loss when clearing values.Setting
child.__values = undefinedbefore callingsetAttributemight cause issues if the setAttribute method fails or if there are side effects that depend on the previous values.Consider preserving the original values in case of errors:
if (values) { - child.__values = undefined; + const originalValues = child.__values; + child.__values = undefined; child.setAttribute('values', values); + // Values are now set via setAttribute }Likely an incorrect or invalid review comment.
packages/react/runtime/__test__/lynx/suspense.test.jsx (10)
1-36: Test setup looks good!The imports, lifecycle hooks, and environment setup are well-structured for testing the custom Suspense implementation.
38-172: Well-structured test for basic Suspense functionality!The test comprehensively covers the complete lifecycle including hydration, fallback rendering, and content resolution. The inline snapshots effectively verify the wrapper element structure.
174-417: Excellent test coverage for the host element feature!This test thoroughly validates the PR's main feature of supporting host elements as direct children of Suspense. The snapshot patch assertions correctly verify the PreventDestroy operations and element preservation.
601-785: Comprehensive unmount/remount test coverage!The test effectively validates that Suspense components can be cleanly unmounted and remounted without residual state issues.
787-934: Good edge case coverage for missing fallback!The test properly validates that Suspense handles the absence of a fallback prop correctly, maintaining an empty state until resolution.
936-1151: Excellent test for parallel Suspense boundaries!The test effectively validates that multiple Suspense components operate independently without interference.
1153-1303: Well-designed test for nested Suspense boundaries!The test correctly validates that the nearest Suspense boundary captures the suspended state, maintaining proper hierarchical behavior.
1305-1373: Good test for pre-resolved promises!The test correctly validates that fallback content is not displayed when the promise is already resolved.
1375-1463: Comprehensive error boundary interaction test!The test effectively validates that real errors (not promises) thrown inside Suspense are properly caught by error boundaries.
1465-1634: Well-designed test for children updates!The test properly validates that Suspense handles children updates correctly after initial resolution without re-triggering the fallback state.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging #1455 will not alter performanceComparing Summary
|
React Example#3831 Bundle Size — 235.26KiB (+0.05%).3f74f7d(current) vs 3f6cbae main#3828(baseline) Bundle metrics
|
| Current #3831 |
Baseline #3828 |
|
|---|---|---|
 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 #3831 |
Baseline #3828 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
89.5KiB (+0.14%) |
89.38KiB |
Bundle analysis report Branch Yradex:suspense/element Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#3826 Bundle Size — 342.64KiB (0%).3f74f7d(current) vs 3f6cbae main#3823(baseline) Bundle metrics
|
| Current #3826 |
Baseline #3823 |
|
|---|---|---|
| Initial JS |
142.33KiB |
142.33KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| 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 #3826 |
Baseline #3823 |
|
|---|---|---|
| JS |
227.85KiB |
227.85KiB |
| Other |
82.95KiB |
82.95KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch Yradex:suspense/element Project dashboard
Generated by RelativeCI Documentation Report issue
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/[email protected] ### Patch Changes - Supports `recyclable` attribute in `<list-item>` to control whether the list item is recyclable. The `recyclable` attribute depends on Lynx Engine 3.4 or later. ([#1388](#1388)) ```jsx <list-item recyclable={false} /> ``` - feat: Support using a host element as direct child of Suspense ([#1455](#1455)) - Add profile in production build: ([#1336](#1336)) 1. `diff:__COMPONENT_NAME__`: how long ReactLynx diff took. 2. `render:__COMPONENT_NAME__`: how long your render function took. 3. `setState`: an instant trace event, indicate when your setState was called. NOTE: `__COMPONENT_NAME__` may be unreadable when minified, setting `displayName` may help. - Add `onBackgroundSnapshotInstanceUpdateId` event on dev for Preact Devtools to keep the correct snapshotInstanceId info. ([#1173](#1173)) - fix: Prevent error when spreading component props onto an element ([#1459](#1459)) - fix: Correctly check for the existence of background functions in MTS ([#1416](#1416)) ```ts function handleTap() { "main thread"; // The following check always returned false before this fix if (myHandleTap) { runOnBackground(myHandleTap)(); } } ``` ## @lynx-js/[email protected] ### Patch Changes - Remove the experimental `provider` option. ([#1432](#1432)) - Add `output.filename.wasm` and `output.filename.assets` options. ([#1449](#1449)) - fix deno compatibility ([#1412](#1412)) - Should call the `api.onCloseBuild` hook after the build finished. ([#1446](#1446)) - Bump Rsbuild v1.4.15. ([#1423](#1423)) - Support using function in `output.filename.*`. ([#1449](#1449)) ## [email protected] ### Patch Changes - Support ESLint for ReactLynx templates ([#1274](#1274)) ## @lynx-js/[email protected] ### Patch Changes - Updated dependencies \[[`c8ce6aa`](c8ce6aa)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - Fix the `Package subpath './compat' is not defined by "exports"` error. ([#1460](#1460)) ## @lynx-js/[email protected] ### Patch Changes - Fix `GlobalEventEmitter` type definition, the `emit(eventName: string, data: unknown)` function should recevie an array typed `data` and pass as param list of listeners. ([#1479](#1479)) ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. - feat: support path() for `createQuerySelector` ([#1456](#1456)) - Added `getPathInfo` API to `NativeApp` and its cross-thread handler for retrieving the path from a DOM node to the root. - Implemented endpoint and handler registration in both background and UI threads. - Implemented `nativeApp.getPathInfo()` - Updated dependencies \[]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. - feat: support path() for `createQuerySelector` ([#1456](#1456)) - Added `getPathInfo` API to `NativeApp` and its cross-thread handler for retrieving the path from a DOM node to the root. - Implemented endpoint and handler registration in both background and UI threads. - Implemented `nativeApp.getPathInfo()` - fix: when `onNativeModulesCall` is delayed in mounting, the NativeModules execution result may be undefined. ([#1457](#1457)) - fix: `onNativeModulesCall` && `onNapiModulesCall` use getter to get. ([#1466](#1466)) - Updated dependencies \[[`29434ae`](29434ae), [`fb7096b`](fb7096b)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. ## @lynx-js/[email protected] ### Patch Changes - feat: add autocomplete attribute support for x-input component ([#1444](#1444)) Implements autocomplete attribute forwarding from the x-input custom element to the internal HTML input element in the shadow DOM. This enables standard browser autocomplete functionality for x-input elements. - Add referrerpolicy attribute support to x-image web component ([#1420](#1420)) - Updated dependencies \[]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. - Updated dependencies \[[`29434ae`](29434ae), [`fb7096b`](fb7096b)]: - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - feat: support path() for `createQuerySelector` ([#1456](#1456)) - Added `getPathInfo` API to `NativeApp` and its cross-thread handler for retrieving the path from a DOM node to the root. - Implemented endpoint and handler registration in both background and UI threads. - Implemented `nativeApp.getPathInfo()` - Updated dependencies \[[`29434ae`](29434ae), [`fb7096b`](fb7096b)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## [email protected] ## @lynx-js/[email protected] ## @lynx-js/[email protected] ## @lynx-js/[email protected] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
<!-- Thank you for submitting a pull request! We appreciate the time and effort you have invested in making these changes. Please ensure that you provide enough information to allow others to review your pull request. Upon submission, your pull request will be automatically assigned with reviewers. If you want to learn more about contributing to this project, please visit: https://github.com/lynx-family/lynx-stack/blob/main/CONTRIBUTING.md. --> <!-- The AI summary below will be auto-generated - feel free to replace it with your own. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded coverage for lazy-loaded components under Suspense across two implementations, exercising mount/unmount, delayed teardown, and reattachment flows. Adds detailed snapshot and DOM assertions to detect duplicate nodes and verify consistent rendering across transitions. * **Chores** * Added a placeholder changeset entry to track release metadata. No impact on functionality or runtime. <!-- end of auto-generated comment: release notes by coderabbit.ai --> This is a bug that #1455 fixed. To avoid future regression, we added it to our test mannually. ## Checklist <!--- Check and mark with an "x" --> - [x] Tests updated (or not required). - [ ] Documentation updated (or not required). - [ ] Changeset added, and when a BREAKING CHANGE occurs, it needs to be clearly marked (or not required).
This reverts commit cd52ab1.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Checklist