fix: list hydrate error#2434
Conversation
🦋 Changeset detectedLatest commit: 837665e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
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:
📝 WalkthroughWalkthroughThis PR fixes hydration of list nodes when both Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/chatty-fans-flash.md:
- Line 2: The changeset currently marks "@lynx-js/react" as "minor" which
signals a breaking change under the repo's 0.x versioning; update the changeset
entry for the package name "@lynx-js/react" to use "patch" instead of "minor" so
this bugfix is released as a non-breaking patch; save the changeset file after
replacing the token "minor" with "patch" and run the usual changeset
validation/CI to confirm.
In `@packages/react/runtime/src/snapshot/backgroundSnapshot.ts`:
- Line 128: BackgroundSnapshotInstance stores the list key as __item_key but
diffArrayLepus reads node.itemKey, so the fallback key path is never used; fix
by exposing the internal key via a public getter or property named itemKey on
BackgroundSnapshotInstance (or set itemKey from __item_key during construction)
so diffArrayLepus can access it; update references in BackgroundSnapshotInstance
(the __item_key field) and ensure diffArrayLepus continues to read node.itemKey
so key-based list diffing works for background list nodes.
🪄 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: 8f700964-138c-4306-b14e-0540aec79d45
📒 Files selected for processing (5)
.changeset/chatty-fans-flash.mdpackages/react/runtime/src/hydrate.tspackages/react/runtime/src/snapshot/backgroundSnapshot.tspackages/react/runtime/src/snapshot/snapshot.tspackages/react/runtime/src/snapshot/types.ts
Merging this PR will improve performance by 36.76%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | 002-hello-reactLynx-destroyBackground |
929.8 µs | 679.9 µs | +36.76% |
Comparing DwwWxx:p/dingwang.wxx/main/fix_list_hydrate (cd3909e) with main (b1ad1b9)
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 External#365 Bundle Size — 592.3KiB (+0.09%).cd3909e(current) vs b1ad1b9 main#353(baseline) Bundle metrics
|
| Current #365 |
Baseline #353 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
31.07% |
0% |
| 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 #365 |
Baseline #353 |
|
|---|---|---|
| Other |
592.3KiB (+0.09%) |
591.76KiB |
Bundle analysis report Branch DwwWxx:p/dingwang.wxx/main/fix_l... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#380 Bundle Size — 206.52KiB (+0.19%).cd3909e(current) vs b1ad1b9 main#368(baseline) Bundle metrics
|
| Current #380 |
Baseline #368 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
46.04% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
67 |
67 |
| Duplicate Code |
45.8%(+0.02%) |
45.79% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #380 |
Baseline #368 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
95.29KiB (+0.42%) |
94.89KiB |
Bundle analysis report Branch DwwWxx:p/dingwang.wxx/main/fix_l... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#8821 Bundle Size — 748.66KiB (0%).cd3909e(current) vs b1ad1b9 main#8809(baseline) Bundle metrics
|
| Current #8821 |
Baseline #8809 |
|
|---|---|---|
| Initial JS |
44.27KiB |
44.27KiB |
| Initial CSS |
2.16KiB |
2.16KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
10 |
10 |
| Modules |
149 |
149 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
35.02% |
35.02% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8821 |
Baseline #8809 |
|
|---|---|---|
| Other |
401.63KiB |
401.63KiB |
| JS |
344.87KiB |
344.87KiB |
| CSS |
2.16KiB |
2.16KiB |
Bundle analysis report Branch DwwWxx:p/dingwang.wxx/main/fix_l... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7247 Bundle Size — 237.22KiB (+0.17%).cd3909e(current) vs b1ad1b9 main#7235(baseline) Bundle metrics
|
| Current #7247 |
Baseline #7235 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
38.45% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
70 |
70 |
| Duplicate Code |
46.13% |
46.13% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #7247 |
Baseline #7235 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.47KiB (+0.44%) |
91.07KiB |
Bundle analysis report Branch DwwWxx:p/dingwang.wxx/main/fix_l... Project dashboard
Generated by RelativeCI Documentation Report issue
DwwWxx
force-pushed
the
p/dingwang.wxx/main/fix_list_hydrate
branch
from
656698a to
799cf6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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__/hydrate.test.jsx`:
- Line 34: The test calls __pendingListUpdates.clear() with no id which doesn't
remove per-id entries and leaves stale state; update the cleanup to iterate
existing ids and clear each explicitly (e.g., for each id in
__pendingListUpdates.keys() call __pendingListUpdates.clear(id)) or otherwise
reinitialize the global container so all pending list update entries are
removed; reference the __pendingListUpdates.clear method and the
__pendingListUpdates keys to implement the fix.
🪄 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: 45ddcce9-f95f-4934-ba0a-eec0aec2cb8f
📒 Files selected for processing (4)
.changeset/flat-ties-spend.mdpackages/react/runtime/__test__/hydrate.test.jsxpackages/react/runtime/src/hydrate.tspackages/react/runtime/src/snapshot/backgroundSnapshot.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/flat-ties-spend.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/runtime/src/snapshot/backgroundSnapshot.ts
- packages/react/runtime/src/hydrate.ts
DwwWxx
force-pushed
the
p/dingwang.wxx/main/fix_list_hydrate
branch
2 times, most recently
from
bec67dd to
7bce702
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/react/runtime/__test__/hydrate.test.jsx (2)
15-31: Renderundefinedexplicitly in the patch formatter.
JSON.stringify(undefined)becomes an empty field afterjoin(', '), so snapshots likeInsertBefore(-1, 2, )are harder to reason about and can hide formatter bugs.Suggested tweak
- out.push(`${meta.name}(${args.map(a => JSON.stringify(a)).join(', ')})`); + out.push( + `${meta.name}(${args.map(a => a === undefined ? 'undefined' : JSON.stringify(a)).join(', ')})`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/hydrate.test.jsx` around lines 15 - 31, The formatter formatSnapshotPatch produces empty fields for undefined because JSON.stringify(undefined) yields undefined and becomes an empty slot when joined; update the argument serialization in formatSnapshotPatch (the map callback that currently uses JSON.stringify(a)) to explicitly render undefined (e.g., return the string "undefined" when a === undefined) and otherwise use JSON.stringify(a), so InsertBefore(-1, 2, ) becomes InsertBefore(-1, 2, undefined); keep this change localized to formatSnapshotPatch and still use SnapshotOperationParams to determine arg counts.
407-439: UsegetItemKeyOf()here instead of a second scanner.This PR’s regression fix lives in
packages/react/runtime/src/hydrate.ts; duplicating the scan in the test means the suite can stay green even ifgetItemKeyOf()drifts. Import the helper and assertgetItemKeyOf(node, true)/getItemKeyOf(node, false)directly so the new production path is actually covered.Suggested direction
-import { SnapshotOperationParams } from '../src/lifecycle/patch/snapshotPatch'; +import { SnapshotOperationParams } from '../src/lifecycle/patch/snapshotPatch'; +import { getItemKeyOf } from '../src/hydrate'; ... - const getItemKeyFromValues = (node, values) => { - for (let index = 0; index < values?.length; index++) { - const value = values[index]; - if (value && typeof value === 'object' && !Array.isArray(value)) { - if ('item-key' in value) { - return value['item-key'] ?? undefined; - } - } - } - return undefined; - }; mtsList.childNodes.forEach((node, index) => { - const itemKey = getItemKeyFromValues(node, node.__values); + const itemKey = getItemKeyOf(node, true); expect(itemKey).toBeTypeOf('string'); expect(itemKey).toBe(`mts-list-item-${index}`); }); ... btsList.childNodes.forEach((node, index) => { - const itemKey = getItemKeyFromValues(node, node.__values); + const itemKey = getItemKeyOf(node, false); expect(itemKey).toBeTypeOf('string'); expect(itemKey).toBe(`bts-list-item-${index}`); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/hydrate.test.jsx` around lines 407 - 439, Replace the local scanner getItemKeyFromValues with the production helper getItemKeyOf: import getItemKeyOf and remove the duplicated function, then in the mtsList loop call getItemKeyOf(node, true) and in the btsList loop call getItemKeyOf(node, false) and assert the returned string equals the expected `*-list-item-${index}`; this ensures the test exercises the actual getItemKeyOf implementation rather than a second scanner.
🤖 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/hydrate.ts`:
- Around line 82-88: The bucket key for keyless list items collapses everything
to UNREACHABLE_ITEM_KEY_NOT_FOUND causing lost per-type buckets; update the key
construction used around getItemKeyOf (used when building beforeMap/after
buckets in hydrate.ts and consumed by diffArrayLepus) so that when getItemKeyOf
returns null/undefined you fallback to a composite that includes node.type
(e.g., combine UNREACHABLE_ITEM_KEY_NOT_FOUND with node.type) instead of using
UNREACHABLE_ITEM_KEY_NOT_FOUND alone, preserving per-type grouping for unkeyed
items.
---
Nitpick comments:
In `@packages/react/runtime/__test__/hydrate.test.jsx`:
- Around line 15-31: The formatter formatSnapshotPatch produces empty fields for
undefined because JSON.stringify(undefined) yields undefined and becomes an
empty slot when joined; update the argument serialization in formatSnapshotPatch
(the map callback that currently uses JSON.stringify(a)) to explicitly render
undefined (e.g., return the string "undefined" when a === undefined) and
otherwise use JSON.stringify(a), so InsertBefore(-1, 2, ) becomes
InsertBefore(-1, 2, undefined); keep this change localized to
formatSnapshotPatch and still use SnapshotOperationParams to determine arg
counts.
- Around line 407-439: Replace the local scanner getItemKeyFromValues with the
production helper getItemKeyOf: import getItemKeyOf and remove the duplicated
function, then in the mtsList loop call getItemKeyOf(node, true) and in the
btsList loop call getItemKeyOf(node, false) and assert the returned string
equals the expected `*-list-item-${index}`; this ensures the test exercises the
actual getItemKeyOf implementation rather than a second scanner.
🪄 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: 8f8ed800-1be1-497b-ba72-38ecb10babc7
📒 Files selected for processing (4)
.changeset/flat-ties-spend.mdpackages/react/runtime/__test__/hydrate.test.jsxpackages/react/runtime/src/hydrate.tspackages/react/runtime/src/snapshot/backgroundSnapshot.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/flat-ties-spend.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/snapshot/backgroundSnapshot.ts
DwwWxx
force-pushed
the
p/dingwang.wxx/main/fix_list_hydrate
branch
from
7bce702 to
3804e61
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react/runtime/src/hydrate.ts (1)
46-50: Empty stringitem-keywould be coerced toundefined.On line 49, if
obj['item-key']is an empty string (""), the expression"" as string ?? undefinedstill returns""(since??only checks fornull/undefined), but the outer conditionif (value && ...)on line 46 already guards against this.However, the
as string ?? undefinedat the end means that ifobj['item-key']is explicitly set tonull, it returnsundefined. This is likely the intended behavior, but worth verifying thatnullitem-keys should be treated as absent.Minor: Consider simplifying the return statement
if ('item-key' in obj) { - return obj['item-key'] as string ?? undefined; + const key = obj['item-key']; + return typeof key === 'string' ? key : undefined; }This makes the type checking more explicit and handles cases where
item-keymight be a non-string value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/hydrate.ts` around lines 46 - 50, The code currently returns obj['item-key'] with a null-coalescing fallback which can be unclear; explicitly check the type and truthiness of obj['item-key'] so only actual strings are returned and null/non-string values become undefined. Update the branch that reads obj['item-key'] (where value is checked as an object) to: if typeof obj['item-key'] === 'string' then return the string or undefined when empty/falsy as desired, otherwise return undefined; this makes intent explicit and avoids treating non-string or null values as valid keys.
🤖 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/hydrate.ts`:
- Around line 46-50: The code currently returns obj['item-key'] with a
null-coalescing fallback which can be unclear; explicitly check the type and
truthiness of obj['item-key'] so only actual strings are returned and
null/non-string values become undefined. Update the branch that reads
obj['item-key'] (where value is checked as an object) to: if typeof
obj['item-key'] === 'string' then return the string or undefined when
empty/falsy as desired, otherwise return undefined; this makes intent explicit
and avoids treating non-string or null values as valid keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dee80c05-2744-4212-b6e3-b4517ecdc88e
📒 Files selected for processing (4)
.changeset/flat-ties-spend.mdpackages/react/runtime/__test__/hydrate.test.jsxpackages/react/runtime/src/hydrate.tspackages/react/runtime/src/snapshot/backgroundSnapshot.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/flat-ties-spend.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/snapshot/backgroundSnapshot.ts
DwwWxx
force-pushed
the
p/dingwang.wxx/main/fix_list_hydrate
branch
from
3804e61 to
2976fc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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__/hydrate.test.jsx`:
- Around line 440-441: Rename the misspelled variable "patchs" to "patches"
wherever it's declared and used in this test (e.g., the const assignment const
patchs = hydrate(JSON.parse(JSON.stringify(mtsList)), btsList); and the
expect(patchs)... assertion), and update the corresponding assertion/usage at
the other mentioned location (line referenced as 496 in the comment) to use
"patches" as well so all references match.
🪄 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: f16aba44-bf96-4439-b320-c954630459a2
📒 Files selected for processing (4)
.changeset/flat-ties-spend.mdpackages/react/runtime/__test__/hydrate.test.jsxpackages/react/runtime/src/renderToOpcodes/hydrate.tspackages/react/runtime/src/snapshot/backgroundSnapshot.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/flat-ties-spend.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/snapshot/backgroundSnapshot.ts
DwwWxx
force-pushed
the
p/dingwang.wxx/main/fix_list_hydrate
branch
2 times, most recently
from
af6e6e6 to
cd3909e
Compare
DwwWxx
force-pushed
the
p/dingwang.wxx/main/fix_list_hydrate
branch
from
cd3909e to
837665e
Compare
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Checklist