feat: reduce slotIndex to save json serialization time#2486
Conversation
|
📝 WalkthroughWalkthroughNormalized per-node slot-index handling: internal Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
upupming
force-pushed
the
feat/reduce-slotIndex-serialize
branch
from
1002a9a to
f55b288
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/patch/snapshotPatchApply.ts (1)
36-50:⚠️ Potential issue | 🟡 MinorType cast to
numberdoesn't safely handle undefined—add explicit null coalescing or type guard.The param definition in
snapshotPatch.ts(line 34) documents slotIndex as/* number | undefined */, but line 40 ofsnapshotPatchApply.tscasts unsafely:const __slotIndex = snapshotPatch[++i] as number;. All current producers push numeric values (via direct__slotIndexorslotIndex ?? 0), and theSnapshotInstancefield defaults to0, so the risk is limited. However, the cast should reflect reality. Either:
- Use explicit null coalescing:
const __slotIndex = (snapshotPatch[++i] as number | undefined) ?? 0;- Or update the type definition to
/* number */if undefined is impossible.Note: Patch streams with lower
reloadVersionare filtered out inupdateMainThread.ts, so true cross-version replay of old patches is not a supported scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/lifecycle/patch/snapshotPatchApply.ts` around lines 36 - 50, The InsertBefore case reads a possibly-undefined slot index unsafely; change how __slotIndex is derived in snapshotPatchApply.ts (case SnapshotOperation.InsertBefore) to handle number | undefined from snapshotPatch by coercing to 0 when absent (e.g., read as number | undefined and use null coalescing) before assigning to child.__slotIndex and calling parent.insertBefore; this uses the existing snapshotPatch array, the __slotIndex variable, and snapshotInstanceManager/child to locate the fix without altering other logic.packages/react/runtime/__test__/renderToOpcodes.test.jsx (1)
267-303:⚠️ Potential issue | 🟡 MinorRestore the
Math.randomspy after this test.Line 268 installs a global spy with
vi.spyOn(Math, 'random').mockReturnValue(0.5), but the suite'safterEachonly callsvi.clearAllMocks(), which clears call history but does not restore the original implementation. Later tests that callMath.random()will continue receiving0.5instead of random values, affecting their behavior and snapshot validity.Use
mockRestore()in a try/finally block to ensure the spy is cleaned up after this test:Proposed fix
it('should render with attr', () => { - vi.spyOn(Math, 'random').mockReturnValue(0.5); - const random = Math.random(); + const randomSpy = vi.spyOn(Math, 'random').mockReturnValue(0.5); + try { + const random = Math.random(); - function App() { - return ( - <view random={random}> - <text>Hello World</text> - <raw-text text={'Hello World'.toLowerCase()} /> - </view> - ); - } + function App() { + return ( + <view random={random}> + <text>Hello World</text> + <raw-text text={'Hello World'.toLowerCase()} /> + </view> + ); + } - expect(renderToString(<App />)).toMatchInlineSnapshot(` + expect(renderToString(<App />)).toMatchInlineSnapshot(` [ 0, { @@ 1, ] `); + } finally { + randomSpy.mockRestore(); + } });🤖 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 267 - 303, The test "should render with attr" installs a global spy via vi.spyOn(Math, 'random').mockReturnValue(0.5) but never restores the original implementation; fix this by capturing the spy (e.g., const rndSpy = vi.spyOn(Math, 'random').mockReturnValue(0.5)), then wrap the test body in a try/finally and call rndSpy.mockRestore() in the finally block (or use rndSpy.mockRestore() after assertions) so Math.random is restored for subsequent tests.
🧹 Nitpick comments (2)
packages/react/runtime/src/snapshot/types.ts (1)
19-26: Runtime vs. serialized field-name divergence — document the contract.After this rename, runtime instances use
__slotIndex(seeBackgroundSnapshotInstance.__slotIndex: number = 0) while the wire/serialized form usesslotIndex. That divergence is intentional (serialization drops the double-underscore prefix and omits the field when0), and matches the asymmetric reads inbackgroundSnapshot.ts(v.slotIndex ?? 0for serialized vsv.__slotIndexfor runtime). However, it's easy to mis-wire in future changes.Recommend a short JSDoc here pointing out:
slotIndexcorresponds to the runtime__slotIndexproperty.- It is omitted when the value is
0to reduce JSON size.This will make the runtime/wire invariant explicit and prevent future readers from "fixing" one side to match the other.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/types.ts` around lines 19 - 26, Add a short JSDoc to the SerializedSnapshotInstance interface documenting that the serialized field slotIndex maps to the runtime property __slotIndex (see BackgroundSnapshotInstance.__slotIndex) and that slotIndex is intentionally omitted from the wire form when its value is 0 to reduce JSON size; mention the asymmetric reads in backgroundSnapshot.ts (uses v.slotIndex ?? 0 for deserialization and runtime uses v.__slotIndex) so future editors don’t accidentally try to rename/mirror the fields incorrectly.packages/react/runtime/src/snapshot/definition.ts (1)
144-150: Consider movingisSlotV2assignment out of theslot[0]guard.When
slotis empty (e.g., the hardcodednulltext entry at line 105),isSlotV2is leftundefinedrather thanfalse. Downstream checks usethis.__snapshot_def.isSlotV2 ? ...(ternary), soundefinedcurrently behaves the same asfalse, but explicitly assigningfalsewould make the invariant clearer and safer for future readers using strict equality.Also worth double-checking: the destructure
([type]) => ...correctly targets theDynamicPartType(first element of each[type, elementIndex]tuple); theeveryreturnstruefor an empty slot array, so keeping it gated byslot[0]avoids accidentally flagging empty-slot snapshots as SlotV2 — which is the current (correct) behavior. Just noting it as a subtle gotcha.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/definition.ts` around lines 144 - 150, The isSlotV2 property can remain undefined when slot is empty; explicitly initialize it to false and then compute it inside the existing guard to preserve the current empty-array behavior. Concretely, set s.isSlotV2 = false before the if (slot && slot[0]) check, and inside that block assign s.isSlotV2 = slot.every(([type]) => type === DynamicPartType.SlotV2 || type === DynamicPartType.ListSlotV2); keep the guard so empty slot arrays aren’t treated as SlotV2 and note the destructure ([type]) in the every callback is correct for the [type, elementIndex] tuples.
🤖 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/snapshot/snapshot.ts`:
- Line 91: takeElements() creates clones via Object.create which bypasses the
constructor/class-field initializer so cloned instances may lack the required
__slotIndex field or have a wrong value; update the clone creation logic in
takeElements() (and the similar clone path around the other block) to explicitly
set child.__slotIndex = source.__slotIndex (or 0 when source is undefined) after
Object.create, ensuring every cloned element has a defined numeric __slotIndex;
reference the __slotIndex field and the takeElements() clone code path when
making the change.
---
Outside diff comments:
In `@packages/react/runtime/__test__/renderToOpcodes.test.jsx`:
- Around line 267-303: The test "should render with attr" installs a global spy
via vi.spyOn(Math, 'random').mockReturnValue(0.5) but never restores the
original implementation; fix this by capturing the spy (e.g., const rndSpy =
vi.spyOn(Math, 'random').mockReturnValue(0.5)), then wrap the test body in a
try/finally and call rndSpy.mockRestore() in the finally block (or use
rndSpy.mockRestore() after assertions) so Math.random is restored for subsequent
tests.
In `@packages/react/runtime/src/lifecycle/patch/snapshotPatchApply.ts`:
- Around line 36-50: The InsertBefore case reads a possibly-undefined slot index
unsafely; change how __slotIndex is derived in snapshotPatchApply.ts (case
SnapshotOperation.InsertBefore) to handle number | undefined from snapshotPatch
by coercing to 0 when absent (e.g., read as number | undefined and use null
coalescing) before assigning to child.__slotIndex and calling
parent.insertBefore; this uses the existing snapshotPatch array, the __slotIndex
variable, and snapshotInstanceManager/child to locate the fix without altering
other logic.
---
Nitpick comments:
In `@packages/react/runtime/src/snapshot/definition.ts`:
- Around line 144-150: The isSlotV2 property can remain undefined when slot is
empty; explicitly initialize it to false and then compute it inside the existing
guard to preserve the current empty-array behavior. Concretely, set s.isSlotV2 =
false before the if (slot && slot[0]) check, and inside that block assign
s.isSlotV2 = slot.every(([type]) => type === DynamicPartType.SlotV2 || type ===
DynamicPartType.ListSlotV2); keep the guard so empty slot arrays aren’t treated
as SlotV2 and note the destructure ([type]) in the every callback is correct for
the [type, elementIndex] tuples.
In `@packages/react/runtime/src/snapshot/types.ts`:
- Around line 19-26: Add a short JSDoc to the SerializedSnapshotInstance
interface documenting that the serialized field slotIndex maps to the runtime
property __slotIndex (see BackgroundSnapshotInstance.__slotIndex) and that
slotIndex is intentionally omitted from the wire form when its value is 0 to
reduce JSON size; mention the asymmetric reads in backgroundSnapshot.ts (uses
v.slotIndex ?? 0 for deserialization and runtime uses v.__slotIndex) so future
editors don’t accidentally try to rename/mirror the fields incorrectly.
🪄 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: 0e60f9c0-57a2-4647-91ec-a747d7ee1759
📒 Files selected for processing (21)
packages/react/runtime/__test__/delayed-lifecycle-events.test.jsxpackages/react/runtime/__test__/hydrate.test.jsxpackages/react/runtime/__test__/lifecycle/reload.test.jsxpackages/react/runtime/__test__/preact.test.jsxpackages/react/runtime/__test__/renderToOpcodes.test.jsxpackages/react/runtime/__test__/snapshot/dynamicPartType.test.jsxpackages/react/runtime/__test__/snapshot/event.test.jsxpackages/react/runtime/__test__/snapshot/ref.test.jsxpackages/react/runtime/__test__/snapshotPatch.test.jsxpackages/react/runtime/src/lifecycle/patch/snapshotPatchApply.tspackages/react/runtime/src/snapshot/backgroundSnapshot.tspackages/react/runtime/src/snapshot/definition.tspackages/react/runtime/src/snapshot/snapshot.tspackages/react/runtime/src/snapshot/types.tspackages/react/testing-library/src/__tests__/act.test.jsxpackages/react/testing-library/src/__tests__/alog.test.jsxpackages/react/testing-library/src/__tests__/end-to-end.test.jsxpackages/react/testing-library/src/__tests__/list.test.jsxpackages/react/testing-library/src/__tests__/renderComponent.test.jsxpackages/react/testing-library/src/__tests__/setState-jsx.test.jsxpackages/react/testing-library/src/__tests__/text.test.jsx
💤 Files with no reviewable changes (2)
- packages/react/testing-library/src/tests/end-to-end.test.jsx
- packages/react/testing-library/src/tests/act.test.jsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1002a9a52e
ℹ️ 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".
upupming
force-pushed
the
feat/reduce-slotIndex-serialize
branch
from
f55b288 to
b569462
Compare
React External#564 Bundle Size — 583.27KiB (+0.19%).b569462(current) vs cffd0b1 main#558(baseline) Bundle metrics
|
| Current #564 |
Baseline #558 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
30.07% |
98.34% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 regression
| Current #564 |
Baseline #558 |
|
|---|---|---|
| Other |
583.27KiB (+0.19%) |
582.19KiB |
Bundle analysis report Branch feat/reduce-slotIndex-serialize Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9021 Bundle Size — 898.15KiB (0%).b569462(current) vs cffd0b1 main#9015(baseline) Bundle metrics
|
| Current #9021 |
Baseline #9015 |
|
|---|---|---|
| Initial JS |
44.47KiB |
44.47KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
229(-0.43%) |
230 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.22%(+0.04%) |
27.21% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9021 |
Baseline #9015 |
|
|---|---|---|
| JS |
494.3KiB |
494.3KiB |
| Other |
401.63KiB |
401.63KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch feat/reduce-slotIndex-serialize Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#579 Bundle Size — 195.57KiB (+0.07%).b569462(current) vs cffd0b1 main#573(baseline) Bundle metrics
|
| Current #579 |
Baseline #573 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
43.08% |
42.65% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
66 |
66 |
| Duplicate Code |
43.99%(+0.02%) |
43.98% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #579 |
Baseline #573 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
84.34KiB (+0.16%) |
84.2KiB |
Bundle analysis report Branch feat/reduce-slotIndex-serialize Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7446 Bundle Size — 224.41KiB (+0.06%).b569462(current) vs cffd0b1 main#7440(baseline) Bundle metrics
|
| Current #7446 |
Baseline #7440 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
35.01% |
34.74% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
69 |
69 |
| Duplicate Code |
44.5% |
44.5% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #7446 |
Baseline #7440 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
78.65KiB (+0.17%) |
78.52KiB |
Bundle analysis report Branch feat/reduce-slotIndex-serialize Project dashboard
Generated by RelativeCI Documentation Report issue
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react/runtime/src/snapshot/backgroundSnapshot.ts (1)
596-602: Minor: add a brief comment explaining the before/after filter asymmetry.The intentional asymmetry
(v.slotIndex ?? 0) === index(serializedbefore) vs.v.__slotIndex === index(in-memoryafter) is correct — serialized nodes omitslotIndexwhen it is0, whileBackgroundSnapshotInstance.__slotIndexis now always an initialized number. A one-line comment here would save future readers a round-trip tosnapshot.ts'stoJSON().📝 Suggested clarifying comment
if (type === DynamicPartType.SlotV2 || type === DynamicPartType.ListSlotV2) { + // `before` is the serialized form where `slotIndex` is omitted when 0 + // (see `SnapshotInstance.toJSON`); `after.__slotIndex` is always numeric. filteredBeforeChildNodes = beforeChildNodes.filter(v => (v.slotIndex ?? 0) === index); filteredAfterChildNodes = afterChildNodes.filter(v => v.__slotIndex === index); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/backgroundSnapshot.ts` around lines 596 - 602, Add a one-line clarifying comment above the before/after filtering in the DynamicPartType.ListChildren branch (where filteredBeforeChildNodes and filteredAfterChildNodes are assigned) explaining the intentional asymmetry: serialized nodes use (v.slotIndex ?? 0) === index because toJSON omits slotIndex when it is 0, whereas in-memory BackgroundSnapshotInstance.__slotIndex is always initialized as a number so we match with v.__slotIndex === index. Reference DynamicPartType.ListChildren, filteredBeforeChildNodes, filteredAfterChildNodes, and BackgroundSnapshotInstance.__slotIndex (see toJSON in snapshot.ts) in the comment so future readers understand the difference.
🤖 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/snapshot/backgroundSnapshot.ts`:
- Around line 596-602: Add a one-line clarifying comment above the before/after
filtering in the DynamicPartType.ListChildren branch (where
filteredBeforeChildNodes and filteredAfterChildNodes are assigned) explaining
the intentional asymmetry: serialized nodes use (v.slotIndex ?? 0) === index
because toJSON omits slotIndex when it is 0, whereas in-memory
BackgroundSnapshotInstance.__slotIndex is always initialized as a number so we
match with v.__slotIndex === index. Reference DynamicPartType.ListChildren,
filteredBeforeChildNodes, filteredAfterChildNodes, and
BackgroundSnapshotInstance.__slotIndex (see toJSON in snapshot.ts) in the
comment so future readers understand the difference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad115842-01d1-454c-84b8-47353c0a2b48
📒 Files selected for processing (21)
packages/react/runtime/__test__/delayed-lifecycle-events.test.jsxpackages/react/runtime/__test__/hydrate.test.jsxpackages/react/runtime/__test__/lifecycle/reload.test.jsxpackages/react/runtime/__test__/preact.test.jsxpackages/react/runtime/__test__/renderToOpcodes.test.jsxpackages/react/runtime/__test__/snapshot/dynamicPartType.test.jsxpackages/react/runtime/__test__/snapshot/event.test.jsxpackages/react/runtime/__test__/snapshot/ref.test.jsxpackages/react/runtime/__test__/snapshotPatch.test.jsxpackages/react/runtime/src/lifecycle/patch/snapshotPatchApply.tspackages/react/runtime/src/snapshot/backgroundSnapshot.tspackages/react/runtime/src/snapshot/definition.tspackages/react/runtime/src/snapshot/snapshot.tspackages/react/runtime/src/snapshot/types.tspackages/react/testing-library/src/__tests__/act.test.jsxpackages/react/testing-library/src/__tests__/alog.test.jsxpackages/react/testing-library/src/__tests__/end-to-end.test.jsxpackages/react/testing-library/src/__tests__/list.test.jsxpackages/react/testing-library/src/__tests__/renderComponent.test.jsxpackages/react/testing-library/src/__tests__/setState-jsx.test.jsxpackages/react/testing-library/src/__tests__/text.test.jsx
💤 Files with no reviewable changes (2)
- packages/react/testing-library/src/tests/act.test.jsx
- packages/react/testing-library/src/tests/end-to-end.test.jsx
✅ Files skipped from review due to trivial changes (4)
- packages/react/testing-library/src/tests/renderComponent.test.jsx
- packages/react/testing-library/src/tests/text.test.jsx
- packages/react/runtime/test/delayed-lifecycle-events.test.jsx
- packages/react/testing-library/src/tests/alog.test.jsx
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/react/runtime/src/snapshot/definition.ts
- packages/react/runtime/src/lifecycle/patch/snapshotPatchApply.ts
- packages/react/testing-library/src/tests/setState-jsx.test.jsx
- packages/react/runtime/test/preact.test.jsx
- packages/react/runtime/src/snapshot/types.ts
- packages/react/testing-library/src/tests/list.test.jsx
- packages/react/runtime/test/lifecycle/reload.test.jsx
- packages/react/runtime/test/snapshot/dynamicPartType.test.jsx
- packages/react/runtime/test/snapshotPatch.test.jsx
- packages/react/runtime/test/hydrate.test.jsx
- packages/react/runtime/test/renderToOpcodes.test.jsx
- packages/react/runtime/test/snapshot/ref.test.jsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b569462735
ℹ️ 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".
Merging this PR will improve performance by 33.79%
Performance Changes
Comparing 
Footnotes
|
2 participants
Summary by CodeRabbit
Refactor
Tests
Checklist