fix: should only recycle off-screen list-item in recursive hydration#1641
fix: should only recycle off-screen list-item in recursive hydration#1641
Conversation
🦋 Changeset detectedLatest commit: 7097fd6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds a changeset for a patch to @lynx-js/react, limits recycleSignMap updates during ListChildren hydration to existing recycle entries, and updates tests to assert only off-screen list items are eligible for recycling during hydration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Web Explorer#4836 Bundle Size — 367.43KiB (0%).7097fd6(current) vs 9e2c497 main#4829(baseline) Bundle metrics
|
| Current #4836 |
Baseline #4829 |
|
|---|---|---|
 Initial JS |
144.23KiB |
144.23KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
 Modules |
220(+0.46%) |
219 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
3.32% |
3.32% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4836 |
Baseline #4829 |
|
|---|---|---|
| JS |
235.43KiB |
235.43KiB |
| Other |
100.16KiB |
100.16KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch fix/should-recycle-onscreen-list... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#4845 Bundle Size — 237.53KiB (+0.01%).7097fd6(current) vs 9e2c497 main#4838(baseline) Bundle metrics
|
| Current #4845 |
Baseline #4838 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
38.63% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
160 |
160 |
| Duplicate Modules |
65 |
65 |
| Duplicate Code |
45.83% |
45.83% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 regression
| Current #4845 |
Baseline #4838 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.77KiB (+0.03%) |
91.74KiB |
Bundle analysis report Branch fix/should-recycle-onscreen-list... Project dashboard
Generated by RelativeCI Documentation Report issue
gaoachao
changed the title
gaoachao
force-pushed
the
fix/should-recycle-onscreen-list-item-only
branch
from
637e4c3 to
01894a5
Compare
gaoachao
changed the title
Signed-off-by: BitterGourd <91231822+gaoachao@users.noreply.github.com>
CodSpeed Performance ReportMerging #1641 will not alter performanceComparing Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/react/runtime/__test__/list.test.jsx (3)
1777-1781: Tighten assertions: check map existence/size and avoid order dependence.Minor: make failures crisper and resilient to insertion-order changes.
Apply this diff:
// Should only update `list-item` in the recycling pool // Should not add the on-screen `list-item` to the recycling pool, +expect(recycleSignMap).toBeInstanceOf(Map); +expect(recycleSignMap.size).toBe(1); -expect([...recycleSignMap.keys()]).toStrictEqual([__GetElementUniqueID(d3.__element_root)]); +expect(new Set(recycleSignMap.keys())).toEqual(new Set([__GetElementUniqueID(d3.__element_root)])); expect(recycleSignMap.get(__GetElementUniqueID(d3.__element_root))).not.toBe(d3);
1948-1952: Prefer order-agnostic key checks and assert pool size.Minor: Map key order is insertion-defined; asserting size plus Set-equality is more robust.
Apply this diff:
// d1 was enqueued as well, so it should be in the recycling pool -expect([...recycleSignMap.keys()]).toStrictEqual([ +expect(recycleSignMap.size).toBe(2); +expect(new Set(recycleSignMap.keys())).toEqual(new Set([ __GetElementUniqueID(d3.__element_root), __GetElementUniqueID(d1.__element_root), -]); +]));
1948-1952: Optional: clean up per-list recycling/sign maps at test end to avoid cross-test residue.Not functionally necessary here, but clearing the specific list’s entries can reduce leakage when many tests run in one process.
Add near the end of this test (before the closing brace):
// Cleanup the per-list bookkeeping to avoid cross-test residue recycleMap.clear(); delete gRecycleMap[listID]; signMap.clear(); delete gSignMap[listID];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/react/runtime/__test__/list.test.jsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
🧬 Code graph analysis (1)
packages/react/runtime/__test__/list.test.jsx (1)
packages/react/runtime/__test__/utils/nativeMethod.ts (1)
__GetElementUniqueID(40-43)
🔇 Additional comments (1)
packages/react/runtime/__test__/list.test.jsx (1)
1777-1781: Good: asserts constrain recycling to off-screen items (addresses prior feedback).This directly verifies the recycling pool doesn’t absorb on-screen items during hydration and follows up on the earlier suggestion to assert
recycleSignMap.
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.112.6 ### Patch Changes - Support nested list. ([#1581](#1581)) - Should only recycle off-screen `list-item` in recursive hydration. ([#1641](#1641)) - fix `fireEvent` type error in testing library ([#1596](#1596)) ## @lynx-js/rspeedy@0.11.1 ### Patch Changes - Disable lazyCompilation by default. ([#1647](#1647)) - Bump Rsbuild v1.5.2 with Rspack v1.5.1. ([#1624](#1624)) - Add `output.dataUriLimit.*` for fine-grained control of asset inlining. ([#1648](#1648)) ```js import { defineConfig } from "@lynx-js/rspeedy"; export default defineConfig({ output: { dataUriLimit: { image: 5000, media: 0, }, }, }); ``` ## @lynx-js/tailwind-preset@0.2.1 ### Patch Changes - Extend the `uiVariants` plugin with additional states: `initial`, `closed`, `indeterminate`, and `invalid`. These can be used with the `ui-*` prefix (e.g., `ui-initial`, `ui-closed`, `ui-indeterminate`, `ui-invalid`). ([#1663](#1663)) ## @lynx-js/web-elements@0.8.5 ### Patch Changes - fix: register attr of width is set to height incorrectly ([#1649](#1649)) - Updated dependencies \[]: - @lynx-js/web-elements-template@0.8.5 ## create-rspeedy@0.11.1 ## upgrade-rspeedy@0.11.1 ## @lynx-js/web-elements-template@0.8.5 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit
Checklist