Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughrenderToString now renders directly into a provided SnapshotInstance and emits opcodes only under SSR. renderMainThread passes the root into renderToString and clears children on errors. The runtime opcode applier was removed and tests/snapshots updated to the new behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
🦋 Changeset detectedLatest commit: 1b160b5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
7e06ee2 to
0d8a5a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/react/runtime/src/renderToOpcodes/index.ts (2)
64-64: Consider removing commented opcode code or adding a clear migration note.The file contains multiple commented-out opcode operations with Chinese comments ("保留以保持接口兼容" - kept for interface compatibility). This pattern suggests a transitional state. Consider either:
- Removing the dead code entirely if the transition is complete
- Adding a clear TODO/migration note explaining the deprecation timeline
Leaving extensive commented code reduces readability and maintainability.
Also applies to: 175-176, 301-302, 326-327, 336-337, 347-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/renderToOpcodes/index.ts` at line 64, The file renderToOpcodes/index.ts contains multiple blocks of commented-out opcode operations around the opcodes array and other sections (e.g., where const opcodes = [] is declared and subsequent opcode handling blocks); either remove these dead commented opcode lines if the migration is complete, or replace each commented block with a concise TODO/migration note that explains why the code is retained, the deprecation timeline, and the owner (e.g., "TODO: remove after vX.Y or when feature Z is complete - owner `@team`"). Update every occurrence of the commented opcode blocks (including the initialization of opcodes and the other commented opcode locations) to follow the chosen approach to improve readability and maintainability.
11-11: File-wide@ts-nocheckdisables TypeScript safety.The
//@ts-nocheck`` directive disables all TypeScript checking, hiding potential type errors. Given the coding guidelines specify "Use TypeScript in strict mode," consider gradually enabling type checking and fixing type issues rather than disabling the compiler entirely.The multiple
null as unknown as stringcasts (lines 65, 171, 333) suggest underlying type design issues that should be addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/renderToOpcodes/index.ts` at line 11, Remove the file-wide "// `@ts-nocheck`" and re-enable TypeScript checking (and ensure strict mode is on in tsconfig) for packages/react/runtime/src/renderToOpcodes/index.ts; then fix the three occurrences of the unsafe "null as unknown as string" casts by giving correct types or defaults (use proper union types like string | null | undefined, initialize with a safe default string when appropriate, or make the variable optional) and update any related function/method signatures and return types (e.g., in renderToOpcodes and any helper functions that reference those variables) so the casts are unnecessary; address type mismatches revealed by the compiler rather than suppressing errors.packages/react/runtime/src/lifecycle/render.ts (1)
9-9: Remove commented-out imports.These commented imports add noise without providing value. If the opcode-based rendering path is being removed, delete the imports entirely rather than leaving them as comments.
♻️ Proposed fix
-// import { isValidElement } from 'preact'; - import { profileEnd, profileStart } from '../debug/profile.js'; -// import { renderOpcodesInto } from '../opcodes.js'; import { render as renderToString } from '../renderToOpcodes/index.js'; import { __root } from '../root.js'; -// import { SnapshotInstance } from '../snapshot/snapshot.js';Also applies to: 12-12, 15-15
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/lifecycle/render.ts` at line 9, Remove the dead commented-out import lines (e.g. "// import { isValidElement } from 'preact';" and the other commented import lines present near the top of render.ts) so the file no longer contains commented imports; simply delete those commented import statements to reduce noise and keep only active imports used by the opcode-based rendering path or its replacement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/runtime/src/lifecycle/render.ts`:
- Around line 46-50: The SSR path assigns empty opcodes because
renderOpcodesInto was removed and renderToString currently builds a
SnapshotInstance tree but is invoked without the destination root, so its output
is discarded; update the SSR branch so renderToString is invoked with __root as
the into parameter (pass __root as the third arg) so the same SnapshotInstance
tree built via insertBefore() is rendered into __root (use __root.__jsx as the
input), and then set __root.__opcodes only after renderToString returns the
opcodes/tree; adjust the code around renderToString, renderOpcodesInto, __root,
and __root.__opcodes to ensure the SnapshotInstance produced by renderToString
is the one attached to __root (SnapshotInstance).
In `@packages/react/runtime/src/renderToOpcodes/index.ts`:
- Line 6: Typo in the module docstring: the phrase "Implements rendering to //
opcodes." contains an errant '//' — open the module header for renderToOpcodes
(the top comment in packages/react/runtime/src/renderToOpcodes/index.ts) and
remove the extra slashes so the comment reads "Implements rendering to opcodes."
(or otherwise correct grammar), preserving the rest of the documentation
formatting.
- Around line 274-280: The code uses into.firstChild (on SnapshotInstance) which
is not a public API and will crash at runtime; change the loop that clears
children to use the public childNodes array (e.g. while (into.childNodes.length
> 0) into.removeChild(into.childNodes[0])) or otherwise use into.__firstChild
only if you intentionally accept private access; update the clearing logic
around the call site of _renderToString(rendered, context, isSvgMode,
selectValue, vnode, opcodes, into) and ensure you reference the public methods
removeChild and childNodes on SnapshotInstance instead of firstChild so the
SnapshotInstance contract is respected.
---
Nitpick comments:
In `@packages/react/runtime/src/lifecycle/render.ts`:
- Line 9: Remove the dead commented-out import lines (e.g. "// import {
isValidElement } from 'preact';" and the other commented import lines present
near the top of render.ts) so the file no longer contains commented imports;
simply delete those commented import statements to reduce noise and keep only
active imports used by the opcode-based rendering path or its replacement.
In `@packages/react/runtime/src/renderToOpcodes/index.ts`:
- Line 64: The file renderToOpcodes/index.ts contains multiple blocks of
commented-out opcode operations around the opcodes array and other sections
(e.g., where const opcodes = [] is declared and subsequent opcode handling
blocks); either remove these dead commented opcode lines if the migration is
complete, or replace each commented block with a concise TODO/migration note
that explains why the code is retained, the deprecation timeline, and the owner
(e.g., "TODO: remove after vX.Y or when feature Z is complete - owner `@team`").
Update every occurrence of the commented opcode blocks (including the
initialization of opcodes and the other commented opcode locations) to follow
the chosen approach to improve readability and maintainability.
- Line 11: Remove the file-wide "// `@ts-nocheck`" and re-enable TypeScript
checking (and ensure strict mode is on in tsconfig) for
packages/react/runtime/src/renderToOpcodes/index.ts; then fix the three
occurrences of the unsafe "null as unknown as string" casts by giving correct
types or defaults (use proper union types like string | null | undefined,
initialize with a safe default string when appropriate, or make the variable
optional) and update any related function/method signatures and return types
(e.g., in renderToOpcodes and any helper functions that reference those
variables) so the casts are unnecessary; address type mismatches revealed by the
compiler rather than suppressing errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ce68638-6078-4617-a433-1010d3d09ead
📒 Files selected for processing (2)
packages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/renderToOpcodes/index.ts
8b52387 to
4d05ee6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react/runtime/src/lifecycle/render.ts (1)
23-50:⚠️ Potential issue | 🔴 CriticalMain-thread render output is discarded, leaving SSR opcodes empty.
At Line 23,
renderToStringis called without theintoroot, while Line 47 disables opcode application. Combined, the produced tree is detached and__root.__opcodesat Line 49 receives an empty array. This is a functional regression in render flow.Suggested fix
- opcodes = renderToString(__root.__jsx, undefined); + opcodes = renderToString(__root.__jsx, undefined, __root as any); @@ - // renderOpcodesInto(opcodes, __root as any); + // renderOpcodesInto(opcodes, __root as any); if (__ENABLE_SSR__) { __root.__opcodes = opcodes; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/lifecycle/render.ts` around lines 23 - 50, The main-thread call to renderToString is producing a detached tree because it’s called as renderToString(__root.__jsx, undefined) and the subsequent opcode application is disabled, which results in __root.__opcodes being empty when __ENABLE_SSR__ is true; update the call to renderToString to pass the __root (or proper into root) so opcodes are generated against the actual root (e.g., renderToString(__root.__jsx, __root)) or alternatively re-enable/apply the opcodes via renderOpcodesInto(opcodes, __root) before assigning __root.__opcodes, ensuring renderToString, __root.__jsx, renderOpcodesInto, __root.__opcodes and the __ENABLE_SSR__ guard are used consistently so the SSR opcodes are populated.
🧹 Nitpick comments (1)
packages/react/runtime/src/lifecycle/render.ts (1)
9-15: Remove commented-out imports/logic after this refactor lands.The commented blocks at Line 9-15, Line 33-41, and Line 47 are now dead code paths. Prefer deleting them (or gating with a real feature flag) to keep this lifecycle path readable and less error-prone.
Also applies to: 33-41, 47-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/lifecycle/render.ts` around lines 9 - 15, The file contains multiple commented-out imports and dead logic (e.g., commented imports for isValidElement, renderOpcodesInto, SnapshotInstance and the commented logic around them) left after the refactor; remove these commented lines (or wrap them behind a real feature flag if they must remain) to clean up render lifecycle code: delete the commented imports for isValidElement, renderOpcodesInto, SnapshotInstance, and any other commented logic around renderToString/profileStart/profileEnd/__root so only active imports and code remain (ensure no side-effecting references to those symbols like renderOpcodesInto or SnapshotInstance remain elsewhere in the module).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react/runtime/src/lifecycle/render.ts`:
- Around line 23-50: The main-thread call to renderToString is producing a
detached tree because it’s called as renderToString(__root.__jsx, undefined) and
the subsequent opcode application is disabled, which results in __root.__opcodes
being empty when __ENABLE_SSR__ is true; update the call to renderToString to
pass the __root (or proper into root) so opcodes are generated against the
actual root (e.g., renderToString(__root.__jsx, __root)) or alternatively
re-enable/apply the opcodes via renderOpcodesInto(opcodes, __root) before
assigning __root.__opcodes, ensuring renderToString, __root.__jsx,
renderOpcodesInto, __root.__opcodes and the __ENABLE_SSR__ guard are used
consistently so the SSR opcodes are populated.
---
Nitpick comments:
In `@packages/react/runtime/src/lifecycle/render.ts`:
- Around line 9-15: The file contains multiple commented-out imports and dead
logic (e.g., commented imports for isValidElement, renderOpcodesInto,
SnapshotInstance and the commented logic around them) left after the refactor;
remove these commented lines (or wrap them behind a real feature flag if they
must remain) to clean up render lifecycle code: delete the commented imports for
isValidElement, renderOpcodesInto, SnapshotInstance, and any other commented
logic around renderToString/profileStart/profileEnd/__root so only active
imports and code remain (ensure no side-effecting references to those symbols
like renderOpcodesInto or SnapshotInstance remain elsewhere in the module).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d774672-8681-41f2-acab-b6683c5c5b9f
📒 Files selected for processing (2)
packages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/renderToOpcodes/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/renderToOpcodes/index.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will degrade performance by 63.77%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | 002-hello-reactLynx__main-thread-renderOpcodes |
1,753.9 µs | 21.6 µs | ×81 |
| ⚡ | 003-hello-list__main-thread-renderOpcodes |
2,462.7 µs | 22.7 µs | ×110 |
| ❌ | 003-hello-list__main-thread-renderMainThread |
17.4 ms | 19.2 ms | -9.3% |
| ❌ | 002-hello-reactLynx__main-thread-renderMainThread |
2.2 ms | 3.8 ms | -41.68% |
| ❌ | 006-static-raw-text__main-thread-renderMainThread |
1.8 ms | 4.9 ms | -63.77% |
| ⚡ | 006-static-raw-text__main-thread-renderOpcodes |
3,454.8 µs | 21.7 µs | ×160 |
| ❌ | 007-four-layer-views__main-thread-renderMainThread |
185.6 ms | 386.4 ms | -51.98% |
| ⚡ | transform 1000 view elements |
45.2 ms | 40.4 ms | +11.92% |
| ❌ | basic-performance-text-200 |
11.1 ms | 11.9 ms | -6.39% |
| ⚡ | basic-performance-div-10000 |
244.5 ms | 226.3 ms | +8.01% |
| ⚡ | 007-four-layer-views__main-thread-renderOpcodes |
219,303.6 µs | 22.3 µs | ×9,800 |
Comparing p/refactor-render (1b160b5) with main (55602e2)
Footnotes
-
21 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
React Example#7092 Bundle Size — 237.68KiB (-0.05%).1b160b5(current) vs 55602e2 main#7082(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch p/refactor-render Project dashboard Generated by RelativeCI Documentation Report issue |
React MTF Example#225 Bundle Size — 207.26KiB (-0.06%).1b160b5(current) vs 55602e2 main#215(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch p/refactor-render Project dashboard Generated by RelativeCI Documentation Report issue |
Web Explorer#8667 Bundle Size — 728.84KiB (0%).1b160b5(current) vs 55602e2 main#8657(baseline) Bundle metrics
Bundle size by type
|
| Current #8667 |
Baseline #8657 |
|
|---|---|---|
384.62KiB |
384.62KiB |
|
342.07KiB |
342.07KiB |
|
2.16KiB |
2.16KiB |
Bundle analysis report Branch p/refactor-render Project dashboard
Generated by RelativeCI Documentation Report issue
e500080 to
27ecc14
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react/runtime/src/lifecycle/render.ts (1)
12-12: Remove commented-out code.The commented-out import and function call are dead code now that the single-pass rendering is implemented. Leaving commented-out code increases maintenance burden and can confuse future readers about the intended architecture.
🧹 Proposed cleanup
-// import { renderOpcodesInto } from '../opcodes.js'; import { render as renderToString } from '../renderToOpcodes/index.js';- - // renderOpcodesInto(opcodes, __root as any); if (__ENABLE_SSR__) {Also applies to: 46-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/lifecycle/render.ts` at line 12, Remove the dead commented import and any commented call sites for renderOpcodesInto in render.ts (specifically the commented line "// import { renderOpcodesInto } from '../opcodes.js';" and the commented occurrences around lines 46-47); delete these commented lines so the file contains only active single-pass rendering code and no leftover references to renderOpcodesInto.
🤖 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/lifecycle/render.ts`:
- Line 12: Remove the dead commented import and any commented call sites for
renderOpcodesInto in render.ts (specifically the commented line "// import {
renderOpcodesInto } from '../opcodes.js';" and the commented occurrences around
lines 46-47); delete these commented lines so the file contains only active
single-pass rendering code and no leftover references to renderOpcodesInto.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cab27b47-8ed7-42d9-8f1f-9445f5c39300
📒 Files selected for processing (2)
packages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/renderToOpcodes/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/renderToOpcodes/index.ts
27ecc14 to
88b9c7f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react/runtime/src/renderToOpcodes/index.ts (1)
264-264:⚠️ Potential issue | 🔴 Critical
lastChildandfirstChildare private properties - this will cause runtime errors.According to the
SnapshotInstanceclass (see context snippet 3),__lastChildand__firstChildare private properties. OnlynextSiblinghas a public getter. Accessinginto.lastChildandinto.firstChildwill returnundefined, breaking the suspend/re-render logic.🔧 Suggested fix using childNodes
- const lastSnapshotChild = into.lastChild ?? null; + const childNodes = into.childNodes; + const lastSnapshotChild = childNodes.length > 0 ? childNodes[childNodes.length - 1] : null;- let nodeToRemove = lastSnapshotChild - ? lastSnapshotChild.nextSibling - : into.firstChild; + const currentChildren = into.childNodes; + const startIndex = lastSnapshotChild + ? currentChildren.indexOf(lastSnapshotChild) + 1 + : 0; + // Remove children added after lastSnapshotChild + for (let i = currentChildren.length - 1; i >= startIndex; i--) { + into.removeChild(currentChildren[i]); + }Run the following script to verify public getters on SnapshotInstance:
#!/bin/bash # Check what public getters exist on SnapshotInstance rg -n "^\s+get\s+\w+\(" packages/react/runtime/src/snapshot/snapshot.ts -A 2Also applies to: 277-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/renderToOpcodes/index.ts` at line 264, The code is accessing private SnapshotInstance properties via into.lastChild and into.firstChild which are undefined at runtime; replace those accesses with the public childNodes array (e.g., compute lastSnapshotChild as into.childNodes?.[into.childNodes.length - 1] ?? null and firstSnapshotChild as into.childNodes?.[0] ?? null) in renderToOpcodes (the lines currently using into.lastChild / into.firstChild) so suspend/re-render logic uses the public API; alternatively use the public nextSibling getter when iterating siblings (SnapshotInstance.nextSibling) where appropriate.
🧹 Nitpick comments (2)
packages/react/runtime/src/lifecycle/render.ts (1)
12-12: Remove commented-out code rather than leaving it in place.The commented-out import (line 12) and function call (line 47) should be removed entirely since
renderOpcodesIntois no longer part of the rendering pipeline. Commented-out code adds noise and can cause confusion about whether it should be restored.♻️ Proposed cleanup
-// import { renderOpcodesInto } from '../opcodes.js'; import { render as renderToString } from '../renderToOpcodes/index.js';- - // renderOpcodesInto(opcodes, __root as any); if (__ENABLE_SSR__) {Also applies to: 46-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/lifecycle/render.ts` at line 12, Remove the leftover commented-out import and call for renderOpcodesInto: delete the commented import line referencing renderOpcodesInto (the commented "// import { renderOpcodesInto } from '../opcodes.js';") and the commented function call lines near where renderOpcodesInto was used (the commented call around lines 46-47). Ensure there are no other commented references to renderOpcodesInto elsewhere in lifecycle/render.ts, run the build/tests to confirm nothing else depends on it, and commit the cleaned file.packages/react/runtime/src/renderToOpcodes/index.ts (1)
11-11: Consider removing@ts-nocheckand adding targeted@ts-ignorecomments instead.The
@ts-nocheckdirective suppresses all TypeScript errors in this file, which masks legitimate type issues like thefirstChild/lastChildaccess problems. Consider enabling TypeScript checking and using targeted@ts-ignorecomments only where necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/renderToOpcodes/index.ts` at line 11, Remove the top-level "// `@ts-nocheck`" in renderToOpcodes/index.ts, enable TypeScript checking for the file, and replace it with targeted fixes: add explicit null/undefined guards or optional chaining where DOM-like properties such as firstChild and lastChild are accessed (e.g., wrap uses of firstChild/lastChild with if (node && node.firstChild) or use node.firstChild?.), and add minimal, localized "// `@ts-ignore`" comments only for lines where a third-party typing is incorrect; run type-checking (tsc) and fix any remaining type errors in functions/methods that reference firstChild/lastChild to ensure correct types are asserted or narrowed rather than suppressing the whole file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react/runtime/src/renderToOpcodes/index.ts`:
- Line 264: The code is accessing private SnapshotInstance properties via
into.lastChild and into.firstChild which are undefined at runtime; replace those
accesses with the public childNodes array (e.g., compute lastSnapshotChild as
into.childNodes?.[into.childNodes.length - 1] ?? null and firstSnapshotChild as
into.childNodes?.[0] ?? null) in renderToOpcodes (the lines currently using
into.lastChild / into.firstChild) so suspend/re-render logic uses the public
API; alternatively use the public nextSibling getter when iterating siblings
(SnapshotInstance.nextSibling) where appropriate.
---
Nitpick comments:
In `@packages/react/runtime/src/lifecycle/render.ts`:
- Line 12: Remove the leftover commented-out import and call for
renderOpcodesInto: delete the commented import line referencing
renderOpcodesInto (the commented "// import { renderOpcodesInto } from
'../opcodes.js';") and the commented function call lines near where
renderOpcodesInto was used (the commented call around lines 46-47). Ensure there
are no other commented references to renderOpcodesInto elsewhere in
lifecycle/render.ts, run the build/tests to confirm nothing else depends on it,
and commit the cleaned file.
In `@packages/react/runtime/src/renderToOpcodes/index.ts`:
- Line 11: Remove the top-level "// `@ts-nocheck`" in renderToOpcodes/index.ts,
enable TypeScript checking for the file, and replace it with targeted fixes: add
explicit null/undefined guards or optional chaining where DOM-like properties
such as firstChild and lastChild are accessed (e.g., wrap uses of
firstChild/lastChild with if (node && node.firstChild) or use
node.firstChild?.), and add minimal, localized "// `@ts-ignore`" comments only for
lines where a third-party typing is incorrect; run type-checking (tsc) and fix
any remaining type errors in functions/methods that reference
firstChild/lastChild to ensure correct types are asserted or narrowed rather
than suppressing the whole file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4cd2f85f-0e35-4ca0-bd8e-c546839284b0
📒 Files selected for processing (2)
packages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/renderToOpcodes/index.ts
8620429 to
f91f23d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/react/runtime/src/lifecycle/render.ts (1)
16-23:⚠️ Potential issue | 🟠 MajorRoll back
__rootwhen a render fails.
renderToString(..., __root)now mutates the liveSnapshotInstancetree while rendering. If anything throws after a fewinsertBefore()/setAttribute()calls, the catch only reports the error and dropsopcodes, so the partially rendered subtree stays attached to__root. The old two-phase flow avoided this because the live tree was only touched after render generation succeeded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/lifecycle/render.ts` around lines 16 - 23, The try/catch in render.ts leaves __root partially mutated when renderToString(__root) throws; change the render flow so mutations are rolled back on error by rendering against a safe copy or snapshot and restoring __root to its original state on catch; specifically, before calling renderToString(__root as SnapshotInstance) capture the original subtree (children/attributes) of __root or use a shallow clone of __root, call renderToString on that copy, and in the catch handler restore the original children/attributes (revert any insertBefore()/setAttribute() effects) so __root is unchanged when opcodes is set to [] and lynx.reportError(e) is called.packages/react/runtime/__test__/renderToOpcodes.test.jsx (1)
1461-1495:⚠️ Potential issue | 🟠 MajorError-path expectation no longer matches the test contract.
The test name says “render an empty page”, but Line 1477–Line 1494 now snapshots a populated page with multiple
<view />nodes. This can mask stale-UI retention in the error path. Please either restore the empty-page invariant or explicitly rename/re-scope this test to the intended behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/renderToOpcodes.test.jsx` around lines 1461 - 1495, The test's expectation no longer matches its name: the "should not throw if error - instead it will render an empty page" case now asserts a populated snapshot; update the test to restore the empty-page invariant or rename/scope the test to reflect the new behavior. Specifically, either change the __root.__jsx setup (the App component and rendered tree) and the assertion on __root.__element_root so the snapshot is an empty page when renderPage() is called after App throws, or rename the it(...) description to describe the populated output and adjust the inline snapshot accordingly; make edits around the App function, the __root.__jsx assignment, and the renderPage() assertion to keep behavior and test name consistent.
🧹 Nitpick comments (1)
packages/react/runtime/__test__/renderToOpcodes.test.jsx (1)
10-11: Remove obsolete commented-out migration leftovers.The commented import and repeated
// renderOpcodesInto(opcodes, scratch);lines add noise now that the suite has switched to directrenderToStringBasecalls.Also applies to: 881-882, 909-910, 940-941, 973-974, 1029-1030, 1088-1089, 1109-1110, 1160-1161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/renderToOpcodes.test.jsx` around lines 10 - 11, Remove the obsolete commented migration leftovers: delete the commented import "// import { renderOpcodesInto } from '../src/opcodes';" and all commented "// renderOpcodesInto(opcodes, scratch);" occurrences so the test file only uses renderToStringBase (imported as renderToOpcodes) and isn't cluttered by legacy commented calls; search for those exact commented strings (they appear repeatedly) and remove them across the test where present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/runtime/__test__/renderToOpcodes.test.jsx`:
- Around line 1119-1128: The commented-out "should render empty array"
regression test must be re-enabled and adapted to the new API: replace the old
renderOpcodesInto call with renderToStringBase(..., scratch) so the test for
empty input still runs; locate the disabled test referencing renderOpcodesInto
and scratch and update it to call renderToStringBase([], scratch) (or the
correct argument signature for renderToStringBase) and assert the same snapshot
against scratch.__element_root, keeping the original test name and expectations.
In `@packages/react/runtime/__test__/snapshot/workletRef.test.jsx`:
- Around line 594-605: The test is directly reassigning lynx.reportError which
can leak a mock; replace the direct assignment with a Vitest spy by calling
vi.spyOn(lynx, 'reportError') before the test block and ensure you restore it
after (e.g., call mockRestore() or rely on vi.restoreAllMocks in afterEach);
update the section that sets reportError and the final restoration to use the
spy's API instead of direct reassignment so lynx.reportError is properly cleaned
up across tests.
In `@packages/react/runtime/src/renderToOpcodes/index.ts`:
- Around line 346-352: The code pushes __OpBegin with the original vnode, but
then replaces vnode with new SnapshotInstance(type) when vnode.__parent is set;
to fix, detect reused/hoisted host nodes (vnode.__parent truthy) and clone
(create the SnapshotInstance) before emitting/pushing __OpBegin so the opcode
array records the clone; update the branch around __ENABLE_SSR__ so __OpBegin
always receives the SnapshotInstance (use SnapshotInstance when vnode.__parent
is true, otherwise keep original vnode) and ensure any subsequent logic uses the
same vnode reference.
---
Outside diff comments:
In `@packages/react/runtime/__test__/renderToOpcodes.test.jsx`:
- Around line 1461-1495: The test's expectation no longer matches its name: the
"should not throw if error - instead it will render an empty page" case now
asserts a populated snapshot; update the test to restore the empty-page
invariant or rename/scope the test to reflect the new behavior. Specifically,
either change the __root.__jsx setup (the App component and rendered tree) and
the assertion on __root.__element_root so the snapshot is an empty page when
renderPage() is called after App throws, or rename the it(...) description to
describe the populated output and adjust the inline snapshot accordingly; make
edits around the App function, the __root.__jsx assignment, and the renderPage()
assertion to keep behavior and test name consistent.
In `@packages/react/runtime/src/lifecycle/render.ts`:
- Around line 16-23: The try/catch in render.ts leaves __root partially mutated
when renderToString(__root) throws; change the render flow so mutations are
rolled back on error by rendering against a safe copy or snapshot and restoring
__root to its original state on catch; specifically, before calling
renderToString(__root as SnapshotInstance) capture the original subtree
(children/attributes) of __root or use a shallow clone of __root, call
renderToString on that copy, and in the catch handler restore the original
children/attributes (revert any insertBefore()/setAttribute() effects) so __root
is unchanged when opcodes is set to [] and lynx.reportError(e) is called.
---
Nitpick comments:
In `@packages/react/runtime/__test__/renderToOpcodes.test.jsx`:
- Around line 10-11: Remove the obsolete commented migration leftovers: delete
the commented import "// import { renderOpcodesInto } from '../src/opcodes';"
and all commented "// renderOpcodesInto(opcodes, scratch);" occurrences so the
test file only uses renderToStringBase (imported as renderToOpcodes) and isn't
cluttered by legacy commented calls; search for those exact commented strings
(they appear repeatedly) and remove them across the test where present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd223500-20dc-4291-835f-87b058e6adc0
📒 Files selected for processing (11)
.changeset/afraid-places-act.mdpackages/react/runtime/__test__/lifecycle/reload.test.jsxpackages/react/runtime/__test__/lifecycle/updateData.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/__test__/renderToOpcodes.test.jsxpackages/react/runtime/__test__/snapshot/workletRef.test.jsxpackages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/opcodes.tspackages/react/runtime/src/renderToOpcodes/index.tspackages/react/testing-library/src/__tests__/alog.test.jsxpackages/react/testing-library/src/__tests__/list.test.jsx
✅ Files skipped from review due to trivial changes (2)
- .changeset/afraid-places-act.md
- packages/react/runtime/test/lifecycle/updateData.test.jsx
f91f23d to
4db9507
Compare
There was a problem hiding this comment.
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/renderToOpcodes/index.ts (1)
45-76:⚠️ Potential issue | 🟠 MajorRollback the target tree when a root render throws.
renderToString()now mutatesintobefore the whole render succeeds. If a top-level host subtree throws, those partially inserted nodes stay attached to the target container instead of preserving the previous all-or-nothing behavior.🩹 Suggested rollback on error
export function renderToString(vnode: any, context: any, into: SnapshotInstance): any[] { + const lastChildBeforeRender = into.__lastChild; // Performance optimization: `renderToString` is synchronous and we // therefore don't execute any effects. To do that we pass an empty // array to `options._commit` (`__c`). But we can go one step further // and avoid a lot of dirty checks and allocations by setting // `options._skipEffects` (`__s`) too. @@ try { _renderToString( vnode, context || EMPTY_OBJ, false, undefined, parent, opcodes, 0, into, ); + } catch (e) { + let nodeToRemove = lastChildBeforeRender + ? lastChildBeforeRender.__nextSibling + : into.__firstChild; + while (nodeToRemove) { + const next = nodeToRemove.__nextSibling; + into.removeChild(nodeToRemove); + nodeToRemove = next; + } + throw e; } finally { // options._commit, we don't schedule any effects in this library right now, // so we can pass an empty queue to this hook. if (options[COMMIT]) options[COMMIT](vnode, EMPTY_ARR);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/renderToOpcodes/index.ts` around lines 45 - 76, renderToString currently mutates the target SnapshotInstance `into` before the full render succeeds, so if `_renderToString` throws you must rollback those partial mutations; before calling `_renderToString` capture the prior state of `into` (e.g., its children/list or root node reference) and any related container metadata, then wrap the `_renderToString` call in try/catch and on error restore `into` to that captured state (removing any partially inserted nodes and restoring previous children), rethrow the error, and ensure existing cleanup (resetting options[SKIP_EFFECTS], restoring beforeDiff/beforeDiff2/afterDiff/renderHook/ummountHook and clearing `opcodes`) still runs in finally; reference symbols: renderToString, _renderToString, into, opcodes, parent, options[SKIP_EFFECTS], beforeDiff/beforeDiff2/afterDiff/renderHook/ummountHook.
♻️ Duplicate comments (3)
packages/react/runtime/src/renderToOpcodes/index.ts (1)
336-343:⚠️ Potential issue | 🟠 MajorEmit
__OpBeginafter cloning reused host nodes.When
vnode.__parentis already set, the opcode array still records the old instance while the clone is the node that actually gets inserted. SSR hydration then replays stale node metadata for reused JSX.🩹 Preserve the inserted instance in opcodes
- if (__ENABLE_SSR__) { - opcodes.push(__OpBegin, vnode); - } // already inserted if (vnode.__parent) { vnode = new SnapshotInstance(type); } + if (__ENABLE_SSR__) { + opcodes.push(__OpBegin, vnode); + } into.insertBefore(vnode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/renderToOpcodes/index.ts` around lines 336 - 343, The opcode push records the original vnode but when vnode.__parent is truthy we replace vnode with a new SnapshotInstance(type) and then insert it, so move the SSR opcode emission to after the clone step: check __ENABLE_SSR__ and call opcodes.push(__OpBegin, vnode) only after handling vnode.__parent (i.e., after creating the new SnapshotInstance) so the emitted opcode references the actual node inserted by into.insertBefore; keep the existing symbols (__OpBegin, vnode.__parent, SnapshotInstance, into.insertBefore, opcodes) intact.packages/react/runtime/__test__/renderToOpcodes.test.jsx (1)
1119-1128:⚠️ Potential issue | 🟡 MinorPlease re-enable the empty-array regression test.
This PR changes the render pipeline’s no-op path as well, so commenting the case out removes coverage for one of the simplest rollback/cleanup edges.
🧪 Re-enable the case against the new API
- // it('should render empty array', () => { - // scratch.ensureElements(); - - // renderOpcodesInto([], scratch); - // expect(scratch.__element_root).toMatchInlineSnapshot(` - // <page - // cssId="default-entry-from-native:0" - // /> - // `); - // }); + it('should render empty array', () => { + scratch.ensureElements(); + renderToStringBase([], undefined, scratch); + expect(scratch.__element_root).toMatchInlineSnapshot(` + <page + cssId="default-entry-from-native:0" + /> + `); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/renderToOpcodes.test.jsx` around lines 1119 - 1128, Uncomment and re-enable the empty-array regression test in packages/react/runtime/__test__/renderToOpcodes.test.jsx: restore the commented block that calls scratch.ensureElements(), renderOpcodesInto([], scratch), and the expect on scratch.__element_root; update the inline snapshot if the new API changes the expected DOM shape but keep the assertion on scratch.__element_root to verify the no-op cleanup/rollback path (symbols to touch: renderOpcodesInto, scratch.ensureElements, scratch.__element_root).packages/react/runtime/__test__/snapshot/workletRef.test.jsx (1)
594-605:⚠️ Potential issue | 🟡 MinorUse a Vitest spy for
lynx.reportError.This direct property reassignment survives assertion failures, and
vi.restoreAllMocks()will not put the original method back. A failure in this test can leak the mock into later cases.🧪 Safer mock setup
- const reportError = lynx.reportError; - lynx.reportError = vi.fn(); + const reportError = vi.spyOn(lynx, 'reportError').mockImplementation(() => {}); // main thread render { __root.__jsx = createCompMT1('number'); renderPage(); } - expect(lynx.reportError).toHaveBeenCalledTimes(1); - expect(lynx.reportError).toHaveBeenCalledWith( + expect(reportError).toHaveBeenCalledTimes(1); + expect(reportError).toHaveBeenCalledWith( new Error('MainThreadRef: main-thread:ref must be of type MainThreadRef or main-thread function.'), ); - lynx.reportError = reportError;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/snapshot/workletRef.test.jsx` around lines 594 - 605, Replace the direct reassignment of lynx.reportError with a Vitest spy: create a spy using vi.spyOn(lynx, 'reportError') and mock its implementation for the test, run the render and assertions against the spy, and then restore the original method (either call spy.mockRestore() at the end of the test or rely on vi.restoreAllMocks() in the test teardown). This targets the lynx.reportError usage in the snapshot test to ensure a failing assertion does not leak a persistent mock into later tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/runtime/__test__/renderToOpcodes.test.jsx`:
- Around line 18-20: The wrapper renderToString currently calls
renderToStringBase(element, null, __root) which mutates the shared live snapshot
tree; change the wrapper(s) (renderToString and the similar helper at lines
27-31) to stop passing the shared __root (use null / a fresh root) so tests
don't render into the shared __root, and ensure tests either rebuild the page or
explicitly clear snapshotInstanceManager between cases to fully isolate
snapshots (referencing renderToStringBase, renderToString, __root, and
snapshotInstanceManager).
---
Outside diff comments:
In `@packages/react/runtime/src/renderToOpcodes/index.ts`:
- Around line 45-76: renderToString currently mutates the target
SnapshotInstance `into` before the full render succeeds, so if `_renderToString`
throws you must rollback those partial mutations; before calling
`_renderToString` capture the prior state of `into` (e.g., its children/list or
root node reference) and any related container metadata, then wrap the
`_renderToString` call in try/catch and on error restore `into` to that captured
state (removing any partially inserted nodes and restoring previous children),
rethrow the error, and ensure existing cleanup (resetting options[SKIP_EFFECTS],
restoring beforeDiff/beforeDiff2/afterDiff/renderHook/ummountHook and clearing
`opcodes`) still runs in finally; reference symbols: renderToString,
_renderToString, into, opcodes, parent, options[SKIP_EFFECTS],
beforeDiff/beforeDiff2/afterDiff/renderHook/ummountHook.
---
Duplicate comments:
In `@packages/react/runtime/__test__/renderToOpcodes.test.jsx`:
- Around line 1119-1128: Uncomment and re-enable the empty-array regression test
in packages/react/runtime/__test__/renderToOpcodes.test.jsx: restore the
commented block that calls scratch.ensureElements(), renderOpcodesInto([],
scratch), and the expect on scratch.__element_root; update the inline snapshot
if the new API changes the expected DOM shape but keep the assertion on
scratch.__element_root to verify the no-op cleanup/rollback path (symbols to
touch: renderOpcodesInto, scratch.ensureElements, scratch.__element_root).
In `@packages/react/runtime/__test__/snapshot/workletRef.test.jsx`:
- Around line 594-605: Replace the direct reassignment of lynx.reportError with
a Vitest spy: create a spy using vi.spyOn(lynx, 'reportError') and mock its
implementation for the test, run the render and assertions against the spy, and
then restore the original method (either call spy.mockRestore() at the end of
the test or rely on vi.restoreAllMocks() in the test teardown). This targets the
lynx.reportError usage in the snapshot test to ensure a failing assertion does
not leak a persistent mock into later tests.
In `@packages/react/runtime/src/renderToOpcodes/index.ts`:
- Around line 336-343: The opcode push records the original vnode but when
vnode.__parent is truthy we replace vnode with a new SnapshotInstance(type) and
then insert it, so move the SSR opcode emission to after the clone step: check
__ENABLE_SSR__ and call opcodes.push(__OpBegin, vnode) only after handling
vnode.__parent (i.e., after creating the new SnapshotInstance) so the emitted
opcode references the actual node inserted by into.insertBefore; keep the
existing symbols (__OpBegin, vnode.__parent, SnapshotInstance,
into.insertBefore, opcodes) intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46ac48cf-d655-4c85-a91d-b2a38b8894e8
📒 Files selected for processing (11)
.changeset/afraid-places-act.mdpackages/react/runtime/__test__/lifecycle/reload.test.jsxpackages/react/runtime/__test__/lifecycle/updateData.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/__test__/renderToOpcodes.test.jsxpackages/react/runtime/__test__/snapshot/workletRef.test.jsxpackages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/opcodes.tspackages/react/runtime/src/renderToOpcodes/index.tspackages/react/testing-library/src/__tests__/alog.test.jsxpackages/react/testing-library/src/__tests__/list.test.jsx
✅ Files skipped from review due to trivial changes (4)
- .changeset/afraid-places-act.md
- packages/react/runtime/test/lifecycle/reload.test.jsx
- packages/react/runtime/test/lifecycle/updateData.test.jsx
- packages/react/testing-library/src/tests/alog.test.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/testing-library/src/tests/list.test.jsx
- packages/react/runtime/src/lifecycle/render.ts
React External#211 Bundle Size — 591.5KiB (+0.01%).1b160b5(current) vs 55602e2 main#201(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch p/refactor-render Project dashboard Generated by RelativeCI Documentation Report issue |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4db9507271
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9e1b1aa to
e216403
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react/runtime/src/opcodes.ts (1)
98-153: Delete the retiredrenderOpcodesIntobody instead of commenting it out.Keeping the full implementation commented here makes it harder to see which path is authoritative and invites drift with the new direct-render flow. If this API is intentionally gone, remove it outright and let any leftover callers fail loudly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/opcodes.ts` around lines 98 - 153, Remove the entire commented-out implementation of renderOpcodesInto (including the commented switch handling OpcodeBegin, OpcodeEnd, OpcodeAttr, OpcodeText and the SnapshotInstance logic) instead of leaving it in the file; if the renderOpcodesInto API is retired, delete the export/declaration for renderOpcodesInto entirely so callers fail loudly and no stale commented code referencing SnapshotInstance or opcodes remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/runtime/src/renderToOpcodes/index.ts`:
- Around line 285-315: Before removing and retrying the subtree, save and then
restore the parent snapshot's slot cursor so SnapshotInstance.insertBefore()
won't consume the wrong slot on retry: capture the current
into.__current_slot_index (e.g. const prevSlot = into.__current_slot_index ?? 0)
before the loop that removes children and after the cleanup but before calling
component.setState/renderClassComponent/_renderToString restore
into.__current_slot_index = prevSlot; reference the retry block around
component.setState, renderClassComponent, and _renderToString and ensure this
restore runs for multi-slot snapshots so subsequent
SnapshotInstance.insertBefore() uses the original slot index.
---
Nitpick comments:
In `@packages/react/runtime/src/opcodes.ts`:
- Around line 98-153: Remove the entire commented-out implementation of
renderOpcodesInto (including the commented switch handling OpcodeBegin,
OpcodeEnd, OpcodeAttr, OpcodeText and the SnapshotInstance logic) instead of
leaving it in the file; if the renderOpcodesInto API is retired, delete the
export/declaration for renderOpcodesInto entirely so callers fail loudly and no
stale commented code referencing SnapshotInstance or opcodes remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80569bba-f2ac-4fa9-b59e-54776af2f4a9
📒 Files selected for processing (12)
.changeset/afraid-places-act.mdpackages/react/runtime/__test__/lifecycle/reload.test.jsxpackages/react/runtime/__test__/lifecycle/updateData.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/__test__/renderToOpcodes.test.jsxpackages/react/runtime/__test__/snapshot/workletRef.test.jsxpackages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/opcodes.tspackages/react/runtime/src/renderToOpcodes/index.tspackages/react/runtime/src/snapshot/snapshot.tspackages/react/testing-library/src/__tests__/alog.test.jsxpackages/react/testing-library/src/__tests__/list.test.jsx
✅ Files skipped from review due to trivial changes (5)
- .changeset/afraid-places-act.md
- packages/react/runtime/test/snapshot/workletRef.test.jsx
- packages/react/testing-library/src/tests/list.test.jsx
- packages/react/runtime/test/lifecycle/updateData.test.jsx
- packages/react/runtime/test/lifecycle/reload.test.jsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react/testing-library/src/tests/alog.test.jsx
- packages/react/runtime/test/lynx/suspense.test.jsx
- packages/react/runtime/src/lifecycle/render.ts
e216403 to
f55b8c4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react/runtime/src/opcodes.ts (1)
98-153: Delete the commented-outrenderOpcodesIntobody.The runtime no longer uses this path, so keeping the full retired implementation inline just adds noise and makes future changes harder to reason about. Please remove it rather than preserving it as commented code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/opcodes.ts` around lines 98 - 153, Remove the entire commented-out implementation of renderOpcodesInto (including the commented switch handling OpcodeBegin, OpcodeEnd, OpcodeAttr, OpcodeText and any inner references to SnapshotInstance, CHILDREN, __ENABLE_SSR__, etc.); leave only the function declaration or delete the empty stub entirely so the retired implementation is not kept inline in opcodes.ts, ensuring no leftover commented logic or references to OpcodeBegin/OpcodeEnd/OpcodeAttr/OpcodeText remain.
🤖 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/opcodes.ts`:
- Around line 98-153: Remove the entire commented-out implementation of
renderOpcodesInto (including the commented switch handling OpcodeBegin,
OpcodeEnd, OpcodeAttr, OpcodeText and any inner references to SnapshotInstance,
CHILDREN, __ENABLE_SSR__, etc.); leave only the function declaration or delete
the empty stub entirely so the retired implementation is not kept inline in
opcodes.ts, ensuring no leftover commented logic or references to
OpcodeBegin/OpcodeEnd/OpcodeAttr/OpcodeText remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aadd97f8-d091-434d-97c6-70deaee8fa61
📒 Files selected for processing (12)
.changeset/afraid-places-act.mdpackages/react/runtime/__test__/lifecycle/reload.test.jsxpackages/react/runtime/__test__/lifecycle/updateData.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/__test__/renderToOpcodes.test.jsxpackages/react/runtime/__test__/snapshot/workletRef.test.jsxpackages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/opcodes.tspackages/react/runtime/src/renderToOpcodes/index.tspackages/react/runtime/src/snapshot/snapshot.tspackages/react/testing-library/src/__tests__/alog.test.jsxpackages/react/testing-library/src/__tests__/list.test.jsx
✅ Files skipped from review due to trivial changes (4)
- .changeset/afraid-places-act.md
- packages/react/testing-library/src/tests/list.test.jsx
- packages/react/runtime/test/lifecycle/reload.test.jsx
- packages/react/runtime/src/renderToOpcodes/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/react/runtime/src/snapshot/snapshot.ts
- packages/react/runtime/test/lifecycle/updateData.test.jsx
- packages/react/runtime/test/lynx/suspense.test.jsx
- packages/react/testing-library/src/tests/alog.test.jsx
- packages/react/runtime/test/renderToOpcodes.test.jsx
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/react/runtime/__test__/snapshot/workletRef.test.jsx (1)
594-605:⚠️ Potential issue | 🟡 MinorUse a spy for
lynx.reportError.
vi.restoreAllMocks()won't undo the manual overwrite here, so any early failure leaves the mock installed for later tests.🛠 Suggested fix
- const reportError = lynx.reportError; - lynx.reportError = vi.fn(); + const reportError = vi.spyOn(lynx, 'reportError').mockImplementation(() => {}); // main thread render { __root.__jsx = createCompMT1('number'); renderPage(); } - expect(lynx.reportError).toHaveBeenCalledTimes(1); - expect(lynx.reportError).toHaveBeenCalledWith( + expect(reportError).toHaveBeenCalledTimes(1); + expect(reportError).toHaveBeenCalledWith( new Error('MainThreadRef: main-thread:ref must be of type MainThreadRef or main-thread function.'), ); - lynx.reportError = reportError;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/snapshot/workletRef.test.jsx` around lines 594 - 605, Replace the manual overwrite of lynx.reportError with a proper spy so it is automatically restored: instead of assigning lynx.reportError = vi.fn() in the test, call vi.spyOn(lynx, 'reportError') (or vi.spyOn(lynx, 'reportError').mockImplementation(() => {})) and later restore with mockRestore() or rely on vi.restoreAllMocks() in your test teardown; update the assertions to use the spy and remove the manual reassignment (references: lynx.reportError in the test block and vi.restoreAllMocks()/afterEach teardown).packages/react/runtime/src/renderToOpcodes/index.ts (1)
269-295:⚠️ Potential issue | 🟠 MajorRestore the saved slot cursor before retrying this subtree.
Line 294 derives
__current_slot_indexfromchildNodes.length, which is not the original slot position once a parent has skipped or empty logical slots. A suspend/retry into the same parent can still re-enter at the wrong slot.🛠 Suggested fix
- let lastChild = into.__lastChild; + const lastChild = into.__lastChild; + const prevSlotIndex = into.__current_slot_index; // Recurse into children before invoking the after-diff hook try { _renderToString( rendered, context, @@ while (nodeToRemove) { const next = nodeToRemove.__nextSibling; into.removeChild(nodeToRemove); nodeToRemove = next; } - into.__current_slot_index = into.childNodes.length; + into.__current_slot_index = prevSlotIndex; if (e && typeof e === 'object' && e.then && component && /* _childDidSuspend */ component.__c) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/renderToOpcodes/index.ts` around lines 269 - 295, The saved slot cursor (into.__lastChild / lastChild) must be restored instead of recomputing into.__current_slot_index from childNodes.length before retrying the suspended subtree: capture the original slot index (use lastChild to compute the original position) and set into.__current_slot_index back to that saved value prior to rethrow/retrying when handling the suspension in the catch block (affecting _renderToString retry path where component.__c and the suspend condition are checked); update the code around lastChild, into.__lastChild and into.__current_slot_index so the retry resumes at the original logical slot rather than at childNodes.length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react/runtime/__test__/snapshot/workletRef.test.jsx`:
- Around line 594-605: Replace the manual overwrite of lynx.reportError with a
proper spy so it is automatically restored: instead of assigning
lynx.reportError = vi.fn() in the test, call vi.spyOn(lynx, 'reportError') (or
vi.spyOn(lynx, 'reportError').mockImplementation(() => {})) and later restore
with mockRestore() or rely on vi.restoreAllMocks() in your test teardown; update
the assertions to use the spy and remove the manual reassignment (references:
lynx.reportError in the test block and vi.restoreAllMocks()/afterEach teardown).
In `@packages/react/runtime/src/renderToOpcodes/index.ts`:
- Around line 269-295: The saved slot cursor (into.__lastChild / lastChild) must
be restored instead of recomputing into.__current_slot_index from
childNodes.length before retrying the suspended subtree: capture the original
slot index (use lastChild to compute the original position) and set
into.__current_slot_index back to that saved value prior to rethrow/retrying
when handling the suspension in the catch block (affecting _renderToString retry
path where component.__c and the suspend condition are checked); update the code
around lastChild, into.__lastChild and into.__current_slot_index so the retry
resumes at the original logical slot rather than at childNodes.length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5910ad5c-ce36-40f8-80f4-340d82bf15b4
📒 Files selected for processing (12)
.changeset/afraid-places-act.mdpackages/react/runtime/__test__/lifecycle/reload.test.jsxpackages/react/runtime/__test__/lifecycle/updateData.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/__test__/renderToOpcodes.test.jsxpackages/react/runtime/__test__/snapshot/workletRef.test.jsxpackages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/opcodes.tspackages/react/runtime/src/renderToOpcodes/index.tspackages/react/runtime/src/snapshot/snapshot.tspackages/react/testing-library/src/__tests__/alog.test.jsxpackages/react/testing-library/src/__tests__/list.test.jsx
✅ Files skipped from review due to trivial changes (3)
- packages/react/runtime/src/snapshot/snapshot.ts
- packages/react/testing-library/src/tests/list.test.jsx
- packages/react/runtime/test/lifecycle/reload.test.jsx
🚧 Files skipped from review as they are similar to previous changes (5)
- .changeset/afraid-places-act.md
- packages/react/runtime/test/lifecycle/updateData.test.jsx
- packages/react/runtime/src/opcodes.ts
- packages/react/testing-library/src/tests/alog.test.jsx
- packages/react/runtime/test/renderToOpcodes.test.jsx
f55b8c4 to
6d1cce3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react/runtime/src/snapshot/snapshot.ts (1)
430-439: Type inconsistency in default parameter.The parameter type is
SnapshotInstancebut the default valuethis.__firstChildcan benull. While the loop body handlesnullcorrectly via thewhile (nodeToRemove)check, the type signature is misleading. Consider updating the signature to explicitly allownull:📝 Suggested fix
- removeChildren(start: SnapshotInstance = this.__firstChild): void { + removeChildren(start: SnapshotInstance | null = this.__firstChild): void { let nodeToRemove = start;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/snapshot.ts` around lines 430 - 439, The parameter type for removeChildren is incorrect because the default this.__firstChild may be null; update the signature of removeChildren(start: SnapshotInstance = this.__firstChild) to explicitly allow null (e.g., SnapshotInstance | null) so the type matches the runtime behavior and the while (nodeToRemove) check is correctly reflected in the type system; adjust any related type annotations in SnapshotInstance references if needed.packages/react/runtime/src/renderToOpcodes/index.ts (1)
406-415: Text node helper encapsulates SSR-conditional opcode emission.The
renderToTextNodehelper correctly:
- Creates a text
SnapshotInstancewithnulltype- Sets the text value via
setAttribute(0, text)- Inserts into the parent tree
- Conditionally emits
__OpTextonly under__ENABLE_SSR__, storing the[textNode, text]tuple to preserve the reference for hydration📝 Minor: Add type annotation for `opcodes` parameter
-function renderToTextNode(into: SnapshotInstance, text: string | number, opcodes: Opcode[]) { +function renderToTextNode(into: SnapshotInstance, text: string | number, opcodes: any[]) {
Opcodetype doesn't appear to be defined/imported. Since the file uses@ts-nocheck, this works at runtime, but the annotation is misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/renderToOpcodes/index.ts` around lines 406 - 415, The opcodes parameter in renderToTextNode is annotated with an undefined Opcode type; fix by replacing the unknown type with an available type (e.g., import/define Opcode where opcodes is declared) or use a clearly-available fallback like Array<any> or unknown[] to avoid a misleading annotation; update the signature in renderToTextNode and any related callers that reference opcodes (and ensure the emitted tuple [textNode, text] pushed with __OpText remains compatible), referencing renderToTextNode, SnapshotInstance, __OpText and the module-level opcode type definition to locate where to add/import the correct type.
🤖 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/renderToOpcodes/index.ts`:
- Around line 406-415: The opcodes parameter in renderToTextNode is annotated
with an undefined Opcode type; fix by replacing the unknown type with an
available type (e.g., import/define Opcode where opcodes is declared) or use a
clearly-available fallback like Array<any> or unknown[] to avoid a misleading
annotation; update the signature in renderToTextNode and any related callers
that reference opcodes (and ensure the emitted tuple [textNode, text] pushed
with __OpText remains compatible), referencing renderToTextNode,
SnapshotInstance, __OpText and the module-level opcode type definition to locate
where to add/import the correct type.
In `@packages/react/runtime/src/snapshot/snapshot.ts`:
- Around line 430-439: The parameter type for removeChildren is incorrect
because the default this.__firstChild may be null; update the signature of
removeChildren(start: SnapshotInstance = this.__firstChild) to explicitly allow
null (e.g., SnapshotInstance | null) so the type matches the runtime behavior
and the while (nodeToRemove) check is correctly reflected in the type system;
adjust any related type annotations in SnapshotInstance references if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c654d74d-a922-40e1-ac67-077620ff6c6c
📒 Files selected for processing (12)
.changeset/afraid-places-act.mdpackages/react/runtime/__test__/lifecycle/reload.test.jsxpackages/react/runtime/__test__/lifecycle/updateData.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/__test__/renderToOpcodes.test.jsxpackages/react/runtime/__test__/snapshot/workletRef.test.jsxpackages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/opcodes.tspackages/react/runtime/src/renderToOpcodes/index.tspackages/react/runtime/src/snapshot/snapshot.tspackages/react/testing-library/src/__tests__/alog.test.jsxpackages/react/testing-library/src/__tests__/list.test.jsx
✅ Files skipped from review due to trivial changes (3)
- .changeset/afraid-places-act.md
- packages/react/testing-library/src/tests/alog.test.jsx
- packages/react/runtime/test/lifecycle/reload.test.jsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/react/runtime/test/snapshot/workletRef.test.jsx
- packages/react/runtime/test/lifecycle/updateData.test.jsx
- packages/react/runtime/src/opcodes.ts
- packages/react/runtime/test/lynx/suspense.test.jsx
- packages/react/runtime/test/renderToOpcodes.test.jsx
6d1cce3 to
1460050
Compare
1460050 to
b42a391
Compare
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/react@0.118.0 ### Minor Changes - refactor: create SnapshotInstance in renderToString directly ([#2393](#2393)) - Add `removeCall` for shake function calls. Its initial default value matches the hooks that were previously in `removeCallParams`, and `removeCallParams` now defaults to empty. ([#2423](#2423)) `removeCall` removes matched runtime hook calls entirely, replacing them with `undefined` in expression positions and dropping them in statement positions. `removeCallParams` keeps the existing behavior of preserving the call while stripping its arguments. ### Patch Changes - Improve `shake.removeCall` and `shake.removeCallParams` in the React transform so they also match aliased runtime imports such as `import { useEffect as myUseEffect } ...` and member calls such as `ReactLynxRuntime.useEffect(...)` from default or namespace runtime imports. ([#2437](#2437)) - Create element without ref for suspense in main thread. ([#2426](#2426)) - refactor: make `useEffect`, `useLayoutEffect` and `useImperativeHandle` no-op on the main thread ([#2424](#2424)) ## @lynx-js/react-rsbuild-plugin@0.15.0 ### Minor Changes - Add `removeCall` for shake function calls. Its initial default value matches the hooks that were previously in `removeCallParams`, and `removeCallParams` now defaults to empty. ([#2423](#2423)) `removeCall` removes matched runtime hook calls entirely, replacing them with `undefined` in expression positions and dropping them in statement positions. `removeCallParams` keeps the existing behavior of preserving the call while stripping its arguments. ### Patch Changes - Support `@lynx-js/react` 0.118.0. ([#2432](#2432)) - Updated dependencies \[[`1f4f117`](1f4f117)]: - @lynx-js/react-webpack-plugin@0.9.0 - @lynx-js/react-alias-rsbuild-plugin@0.15.0 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/react-refresh-webpack-plugin@0.3.5 - @lynx-js/css-extract-webpack-plugin@0.7.0 - @lynx-js/template-webpack-plugin@0.10.8 ## @lynx-js/react-webpack-plugin@0.9.0 ### Minor Changes - Add `removeCall` for shake function calls. Its initial default value matches the hooks that were previously in `removeCallParams`, and `removeCallParams` now defaults to empty. ([#2423](#2423)) `removeCall` removes matched runtime hook calls entirely, replacing them with `undefined` in expression positions and dropping them in statement positions. `removeCallParams` keeps the existing behavior of preserving the call while stripping its arguments. ## @lynx-js/rspeedy@0.14.1 ### Patch Changes - Updated dependencies \[]: - @lynx-js/web-rsbuild-server-middleware@0.20.1 ## create-rspeedy@0.14.1 ### Patch Changes - Fix the error when installing Lynx DevTool skill. ([#2427](#2427)) ## @lynx-js/lynx-bundle-rslib-config@0.3.1 ### Patch Changes - Updated dependencies \[[`156d64d`](156d64d), [`59d11b2`](59d11b2)]: - @lynx-js/css-serializer@0.1.5 ## @lynx-js/config-rsbuild-plugin@0.0.2 ### Patch Changes - Support `@lynx-js/rspeedy` 0.14.0. ([#2431](#2431)) ## @lynx-js/css-serializer@0.1.5 ### Patch Changes - feat: add support for @media, @supports, and @layer at-rules ([#2330](#2330)) Add support for additional CSS at-rules in the CSS serializer: - `@media` - for media queries - `@supports` - for feature queries - `@layer` - for cascade layers (both named and anonymous) The parser now handles these at-rules with proper recursive parsing support for nested at-rules. - feat: support custom property declaration in keyframe rule ([#2429](#2429)) ## @lynx-js/web-core@0.20.1 ### Patch Changes - Added support for the `global-bind` event handling modifier in the web platform runtime. ([#2438](#2438)) This mechanism enables seamless cross-element event communication without requiring a formal DOM tree relationship, allowing decoupled elements to observe and respond to standard events occurring anywhere within the component tree. ### Usage Global bindings allow an observer element to react to events triggered on another target element. #### 1. Define the Global Subscription Attach `global-bindTap` (or any equivalent standard event alias) to your observer element: ```jsx <view id="observer" global-bindTap={(event) => { // This will trigger whenever 'tap' is caught by a globally bound event. console.log("Global tap handled!", event); }} /> ``` #### 2. Trigger the Event anywhere The event will be triggered via normal user interaction (such as `tap`) on any other constituent elements: ```jsx <view id="target" bindTap={(event) => { // Note: To successfully propagate globally, ensure the event bubbles. }} /> ``` - feat(web-core): add support for configurable rem unit transform ([#2403](#2403)) - **Description**: Added a new configuration option `transformREM` (also exposed as `transform_rem` on the Rust layer) to the Web Core renderer. When enabled, it recursively converts static `rem` unit values in your styles into dynamic CSS custom properties (`calc(VALUE * var(--rem-unit))`) during template decoding and evaluation. This enables developers to implement responsive font scaling and layout sizing dynamically on the client side simply by modifying the root CSS variable `--rem-unit`. - **Usage**: You can enable this feature when working with `LynxView` by setting `transformREM` to `true`, or directly as an HTML attribute `transform-rem`: ```html <lynx-view url="https://example.com/template.js" transform-rem="true" ></lynx-view> ``` ```javascript const lynxView = document.createElement("lynx-view"); lynxView.transformREM = true; ``` With this enabled, a CSS declaration like `font-size: 1.5rem;` is transparently evaluated as `font-size: calc(1.5 * var(--rem-unit));` by the runtime engine. - Updated dependencies \[[`156d64d`](156d64d), [`59d11b2`](59d11b2)]: - @lynx-js/css-serializer@0.1.5 - @lynx-js/web-worker-rpc@0.20.1 ## @lynx-js/template-webpack-plugin@0.10.8 ### Patch Changes - Updated dependencies \[[`156d64d`](156d64d), [`5151fcf`](5151fcf), [`b630df2`](b630df2), [`59d11b2`](59d11b2)]: - @lynx-js/css-serializer@0.1.5 - @lynx-js/web-core@0.20.1 ## @lynx-js/react-umd@0.118.0 ## @lynx-js/react-alias-rsbuild-plugin@0.15.0 ## upgrade-rspeedy@0.14.1 ## @lynx-js/web-rsbuild-server-middleware@0.20.1 ## @lynx-js/web-worker-rpc@0.20.1 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
Checklist