-
Notifications
You must be signed in to change notification settings - Fork 121
fix(react/runtime): fix error in suspense #1569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 5d8f582 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 |
📝 WalkthroughWalkthroughIntroduces a changeset for a patch bump and bug note, adds a Suspense hydration test without initial hydrate, and guards snapshot patch emissions in reconstructInstanceTree with optional chaining to avoid undefined access when the patch collector is absent. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/backgroundSnapshot.ts (1)
520-522
: for..in over possibly undefined extraProps can throw at runtime.extraProps is typed as optional. Iterating with for..in when it’s undefined will throw (“Cannot convert undefined or null to object”). Guard it.
- const extraProps = child.__extraProps; - for (const key in extraProps) { - child.setAttribute(key, extraProps[key]); - } + const extraProps = child.__extraProps; + if (extraProps) { + for (const key in extraProps) { + child.setAttribute(key, extraProps[key]); + } + }
🧹 Nitpick comments (2)
packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
1696-1698
: Rename test to reflect “no initial hydrate” (clarity).“without hydrate” can be read as “no hydration at all”. Consider “without initial hydrate” to avoid ambiguity.
-it('should render fallback and content correctly without hydrate', async () => { +it('should render fallback and content correctly without initial hydrate', async () => {packages/react/runtime/src/backgroundSnapshot.ts (1)
510-526
: Optional: add a DEV-only warning if reconstructInstanceTree runs without a collector.When __globalSnapshotPatch is undefined, we silently skip patch emission. That’s intended in some flows (e.g., pre-hydration), but a DEV guard would help catch unintended suppression elsewhere.
function reconstructInstanceTree(afters: BackgroundSnapshotInstance[], parentId: number, targetId?: number): void { + if (__DEV__ && !__globalSnapshotPatch) { + // This path is expected during specific phases (e.g., before hydration). + // If you see this during regular updates, investigate the patch lifecycle. + console.warn('[reconstructInstanceTree] __globalSnapshotPatch is undefined; patch emission suppressed.'); + } for (const child of afters) { const id = child.__id; __globalSnapshotPatch?.push(SnapshotOperation.CreateElement, child.type, id); const values = child.__values;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/rotten-queens-drum.md
(1 hunks)packages/react/runtime/__test__/lynx/suspense.test.jsx
(1 hunks)packages/react/runtime/src/backgroundSnapshot.ts
(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-07T04:00:59.645Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1454
File: pnpm-workspace.yaml:46-46
Timestamp: 2025-08-07T04:00:59.645Z
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/rotten-queens-drum.md
📚 Learning: 2025-07-22T09:23:07.797Z
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/rotten-queens-drum.md
🧬 Code Graph Analysis (2)
packages/react/runtime/src/backgroundSnapshot.ts (1)
packages/react/runtime/src/lifecycle/patch/snapshotPatch.ts (2)
__globalSnapshotPatch
(53-53)SnapshotOperation
(11-20)
packages/react/runtime/__test__/lynx/suspense.test.jsx (3)
packages/react/runtime/__test__/createSuspender.jsx (1)
createSuspender
(8-26)packages/react/runtime/src/lynx/suspense.ts (1)
Suspense
(14-44)packages/react/runtime/__test__/utils/envManager.ts (1)
globalEnvManager
(86-86)
🔇 Additional comments (2)
.changeset/rotten-queens-drum.md (1)
1-6
: Changeset entry is appropriate and user-facing.Patch bump on @lynx-js/react with a concise bug description matches our changeset policy for end-user fixes.
packages/react/runtime/src/backgroundSnapshot.ts (1)
513-525
: Guarding snapshot patch pushes fixes the Suspense race – LGTM.Switching to optional chaining on CreateElement/InsertBefore in reconstructInstanceTree prevents the undefined-access when the collector isn’t initialized. This aligns with how other mutation sites already guard on __globalSnapshotPatch.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Web Explorer#4423 Bundle Size — 366.68KiB (0%).5d8f582(current) vs d4fd6cb main#4411(baseline) Bundle metrics
Bundle size by type
|
Current #4423 |
Baseline #4411 |
|
---|---|---|
234.68KiB |
234.68KiB |
|
100.16KiB |
100.16KiB |
|
31.84KiB |
31.84KiB |
Bundle analysis report Branch Yradex:fix/1 Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#4430 Bundle Size — 237.09KiB (~+0.01%).5d8f582(current) vs d4fd6cb main#4418(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:fix/1 Project dashboard Generated by RelativeCI Documentation Report issue |
CodSpeed Performance ReportMerging #1569 will not alter performanceComparing Summary
|
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 - fix `withInitDataInState` got wrong state in 2nd or more times `defaultDataProcessor`, now it will keep its own state. ([#1478](#1478)) - change `__CreateElement('raw-text')` to `__CreateRawText('')` to avoid `setNativeProps` not working ([#1570](#1570)) - Fix wrong render result when using expression as `key`. ([#1541](#1541)) See [#1371](#1371) for more details. - fix: `Cannot read properties of undefined` error when using `Suspense` ([#1569](#1569)) - Add `animate` API in Main Thread Script(MTS), so you can now control a CSS animation imperatively ([#1534](#1534)) ```ts import type { MainThread } from "@lynx-js/types"; function startAnimation(ele: MainThread.Element) { "main thread"; const animation = ele.animate([{ opacity: 0 }, { opacity: 1 }], { duration: 3000, }); // Can also be paused // animation.pause() } ``` ## @lynx-js/[email protected] ### Patch Changes - Support caching Lynx native events when chunk splitting is enabled. ([#1370](#1370)) When `performance.chunkSplit.strategy` is not `all-in-one`, Lynx native events are cached until the BTS chunk is fully loaded and are replayed when that chunk is ready. The `firstScreenSyncTiming` flag will no longer change to `jsReady` anymore. - Support exporting `Promise` and function in `lynx.config.ts`. ([#1590](#1590)) - Fix missing `publicPath` using when `rspeedy dev --mode production`. ([#1310](#1310)) - Updated dependencies \[[`aaca8f9`](aaca8f9)]: - @lynx-js/[email protected] - @lynx-js/[email protected] ## [email protected] ### Patch Changes - Add `@lynx-js/preact-devtools` by default. ([#1593](#1593)) ## @lynx-js/[email protected] ### Patch Changes - Bump @clack/prompts to v1.0.0-alpha.4 ([#1559](#1559)) ## @lynx-js/[email protected] ### Patch Changes - Support using multiple times in different environments. ([#1498](#1498)) - Support caching Lynx native events when chunk splitting is enabled. ([#1370](#1370)) When `performance.chunkSplit.strategy` is not `all-in-one`, Lynx native events are cached until the BTS chunk is fully loaded and are replayed when that chunk is ready. The `firstScreenSyncTiming` flag will no longer change to `jsReady` anymore. - Updated dependencies \[[`f0d483c`](f0d483c), [`e4d116b`](e4d116b), [`d33c1d2`](d33c1d2)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - Support using multiple times in different environments. ([#1498](#1498)) - Alias `@lynx-js/preact-devtools` to `false` to reduce an import of empty webpack module. ([#1593](#1593)) ## @lynx-js/[email protected] ### Patch Changes - Support `lynx.createSelectorQuery().select()` and `setNativeProps` API ([#1570](#1570)) ## @lynx-js/[email protected] ### Patch Changes - fix: globalThis is never accessible in MTS ([#1531](#1531)) - Updated dependencies \[]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: fake uidisappear event ([#1539](#1539)) - Updated dependencies \[[`70863fb`](70863fb)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: globalThis is never accessible in MTS ([#1531](#1531)) - Updated dependencies \[[`70863fb`](70863fb)]: - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: globalThis is never accessible in MTS ([#1531](#1531)) - Updated dependencies \[[`70863fb`](70863fb)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - Add new `LynxCacheEventsPlugin`, which will cache Lynx native events until the BTS chunk is fully loaded, and replay them when the BTS chunk is ready. ([#1370](#1370)) - Updated dependencies \[[`aaca8f9`](aaca8f9)]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - Updated dependencies \[[`aaca8f9`](aaca8f9)]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - Updated dependencies \[[`aaca8f9`](aaca8f9)]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - Updated dependencies \[[`aaca8f9`](aaca8f9)]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - Always inline the background script that contains rspack runtime module. ([#1582](#1582)) - Updated dependencies \[[`aaca8f9`](aaca8f9)]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - Add `lynxCacheEventsSetupList` and `lynxCacheEvents` to RuntimeGlobals. It will be used to cache Lynx native events until the BTS chunk is fully loaded, and replay them when the BTS chunk is ready. ([#1370](#1370)) ## [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>
Summary by CodeRabbit
Bug Fixes
Chores
Tests
Checklist