fix(react/runtime): improve error message of circular reference #1238
Conversation
issue: m-6732040310
🦋 Changeset detectedLatest commit: c2ceb65 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 |
CodSpeed Performance ReportMerging #1238 will not alter performanceComparing Summary
|
Web Explorer#3062 Bundle Size — 304.59KiB (0%).c2ceb65(current) vs ec7228f main#3054(baseline) Bundle metrics
|
| Current #3062 |
Baseline #3054 |
|
|---|---|---|
 Initial JS |
140.65KiB |
140.65KiB |
| Initial CSS |
31.83KiB |
31.83KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
4 |
4 |
| Assets |
5 |
5 |
 Modules |
205(+0.49%) |
204 |
| Duplicate Modules |
17 |
17 |
| Duplicate Code |
4.22% |
4.22% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #3062 |
Baseline #3054 |
|
|---|---|---|
| JS |
221.88KiB |
221.88KiB |
| Other |
50.89KiB |
50.89KiB |
| CSS |
31.83KiB |
31.83KiB |
Bundle analysis report Branch colinaaa:colin/0707/circular Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#3072 Bundle Size — 234.54KiB (+0.04%).c2ceb65(current) vs ec7228f main#3063(baseline) Bundle metrics
|
| Current #3072 |
Baseline #3063 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
37.83% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
160(+1.91%) |
157 |
 Duplicate Modules |
65(+3.17%) |
63 |
| Duplicate Code |
45.99%(+0.24%) |
45.88% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #3072 |
Baseline #3063 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
88.78KiB (+0.11%) |
88.68KiB |
Bundle analysis report Branch colinaaa:colin/0707/circular Project dashboard
Generated by RelativeCI Documentation Report issue
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Asymmetric Stack Operations Cause Corruption
The renderStack operations are asymmetric. The DIFF hook conditionally pushes vnodes (only for isPossibleOwner), while the DIFFED hook unconditionally pops. This imbalance causes renderStack corruption, leading to stack underflows, incorrect component stack traces, and potentially runtime errors or inaccurate getCurrentVNode() results.
packages/react/runtime/src/debug/component-stack.ts#L141-L155
lynx-stack/packages/react/runtime/src/debug/component-stack.ts
Lines 141 to 155 in d41a8d8
BugBot free trial expires on July 22, 2025
You have used $0.00 of your $0.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
WalkthroughA new debugging utility for tracking component stack traces during rendering was introduced and integrated into the runtime, activated in development mode. Error handling for circular references in object comparison was improved, with enhanced error messages including component stack traces. Related tests and configuration updates were also made. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant App as React App
participant Lynx as Lynx Runtime
participant Stack as Component Stack Utility
Dev->>App: Runs app in development mode
App->>Lynx: Renders components
Lynx->>Stack: setupComponentStack() (if __DEV__)
App->>Lynx: Triggers deep equality check (isDirectOrDeepEqual)
Lynx->>Stack: getCurrentVNode(), getOwnerStack()
Lynx->>App: Throws error with component stack if circular reference detected
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ERROR Cannot resolve version $@rspack/core in overrides. The direct dependencies don't have dependency "@rspack/core". 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react/runtime/src/debug/component-stack.ts (1)
117-126: Optimize string concatenation in hot path.The
getOwnerStackfunction uses repeated string concatenation which could impact performance when building stack traces during error scenarios. Consider using an array and joining at the end.export function getOwnerStack(vnode: PatchedVNode): string { const stack = [vnode]; let next = vnode; while (next._owner != null) { stack.push(next._owner); next = next._owner; } - return stack.reduce((acc, owner) => { - acc += ` in ${getDisplayName(owner)}`; - - const source = owner.__source; - if (source) { - acc += ` (at ${source.fileName}:${source.lineNumber})`; - } - - return (acc += '\n'); - }, ''); + const parts = stack.map(owner => { + let line = ` in ${getDisplayName(owner)}`; + const source = owner.__source; + if (source) { + line += ` (at ${source.fileName}:${source.lineNumber})`; + } + return line; + }); + + return parts.join('\n') + '\n'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/tough-drinks-throw.md(1 hunks)packages/react/runtime/__test__/snapshot/spread.test.jsx(1 hunks)packages/react/runtime/src/debug/component-stack.ts(1 hunks)packages/react/runtime/src/lynx.ts(2 hunks)packages/react/runtime/src/renderToOpcodes/constants.ts(1 hunks)packages/react/runtime/src/utils.ts(1 hunks)packages/react/runtime/vitest.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/react/runtime/vitest.config.ts (1)
Learnt from: PupilTong
PR: lynx-family/lynx-stack#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.
packages/react/runtime/src/lynx.ts (3)
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.
Learnt from: PupilTong
PR: lynx-family/lynx-stack#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: lynx-family/lynx-stack#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.
🧬 Code Graph Analysis (1)
packages/react/runtime/src/lynx.ts (1)
packages/react/runtime/src/debug/component-stack.ts (1)
setupComponentStack(134-174)
🔇 Additional comments (10)
.changeset/tough-drinks-throw.md (1)
1-4: Empty changeset file needs content.The changeset file contains only YAML front matter delimiters without any content. For a bug fix that improves error messages, this should include a description of the change and appropriate versioning metadata.
packages/react/runtime/vitest.config.ts (1)
88-88: Appropriate exclusion of debug instrumentation from coverage.Excluding the component stack debugging module from test coverage is the right approach, as this instrumentation code serves diagnostic purposes and aligns with other debug files already excluded.
packages/react/runtime/src/renderToOpcodes/constants.ts (1)
10-10: Verify the ROOT constant value doesn't conflict with PARENT.The new
ROOTconstant has the same value'__'as the existingPARENTconstant (line 15). While they may serve different purposes (ROOT as a lifecycle hook, PARENT as a VNode property), having identical values could lead to confusion or potential conflicts.Please confirm this duplication is intentional and won't cause issues in the runtime.
packages/react/runtime/src/lynx.ts (2)
9-9: Proper integration of component stack debugging.The import of
setupComponentStackcorrectly brings in the new debugging functionality for tracking component stacks.
39-41: Appropriate conditional setup of component stack tracking.The component stack setup is correctly placed in a development-only conditional block, ensuring the debugging instrumentation is only active when needed. The placement after the main thread setup ensures it runs in both main and background threads during development.
packages/react/runtime/src/utils.ts (2)
7-7: Proper import of component stack utilities.The import of debugging utilities from the component stack module enables the enhanced error messaging functionality.
13-33: Excellent enhancement of circular reference error handling.The implementation effectively addresses the PR objective by:
- Robust error handling: Wrapping JSON.stringify in try-catch to handle circular references gracefully
- Cross-engine compatibility: The regex pattern
/circular|cyclic/icovers error messages from different JavaScript engines (PrimJS, V8, JavaScriptCore) as documented in the comments- Enhanced debugging: In development mode, circular reference errors are augmented with component stack traces, providing valuable context for developers
- Proper error propagation: The error is rethrown after enhancement, maintaining the original error semantics
The approach is well-tested across different engines (as mentioned in past review comments) and provides a significant improvement to developer experience when debugging circular reference issues.
packages/react/runtime/__test__/snapshot/spread.test.jsx (1)
639-647: Well-structured test for circular reference error handling.The test correctly validates that circular references in spread props throw a TypeError with an enhanced error message that includes the component stack trace.
packages/react/runtime/src/debug/component-stack.ts (2)
134-174: Well-implemented component stack tracking with proper lifecycle integration.The implementation correctly patches Preact's lifecycle hooks to maintain component stacks for debugging. The owner tracking logic properly handles the component hierarchy.
142-173: Consider adding error boundaries for stack operations.The stack push/pop operations in the lifecycle hooks could throw errors if the stacks get out of sync. Consider adding try-catch blocks to prevent breaking the render cycle.
Example for one of the hooks:
options[DIFFED] = vnode => { + try { if (isPossibleOwner(vnode)) { ownerStack.pop(); } renderStack.pop(); + } catch (e) { + if (__DEV__) { + console.error('Component stack tracking error:', e); + } + } if (oldDiffed) oldDiffed(vnode); };⛔ Skipped due to learnings
Learnt from: PupilTong PR: lynx-family/lynx-stack#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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Qingyu Wang <[email protected]>
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
3 participants
Summary
Improve the error message when setting circular reference as attributes.
close: m-6732040310
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores