fix: should update signMap before __FlushElementTree#1692
Merged
Conversation
🦋 Changeset detectedLatest commit: f34ae01 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 |
Contributor
📝 WalkthroughWalkthroughMoves sign-to-SnapshotInstance mapping earlier in the list componentAtIndex flow (immediately after applyRefQueue) across recycled and normal branches; adds tests asserting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: BitterGourd <91231822+gaoachao@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/react/runtime/src/list.ts (1)
158-163: Consider deduping “set-then-maybe-flush” into a tiny helperWe now repeat “applyRefQueue(); signMap.set(sign, childCtx); (maybe) __FlushElementTree(...)” across branches. A local helper would reduce drift and make the ordering guarantee harder to regress.
Example:
function mapAndMaybeFlush( map: Map<number, SnapshotInstance>, sign: number, ctx: SnapshotInstance, root: FiberElement, opts?: FlushOptions ) { map.set(sign, ctx); if (opts) __FlushElementTree(root, opts); }Also applies to: 197-197
packages/react/runtime/__test__/list.test.jsx (3)
3035-3079: Prefer spies over method reassignment for stabilityUse vi.spyOn for __FlushElementTree and Map#set to avoid mutating globals and instance methods directly.
Apply this diff in-place:
- const mockFlushElementTree = vi.fn(); - const mockSignMapSet = vi.fn(); - - const __originalFlushElementTree = globalThis.__FlushElementTree; - globalThis.__FlushElementTree = (_, options = {}) => { - mockFlushElementTree(options); - }; + const flushSpy = vi.spyOn(globalThis, '__FlushElementTree'); const listID = __GetElementUniqueID(listRef); const signMap = gSignMap[listID]; - const __originalSet = signMap.set; - signMap.set = (...args) => { - mockSignMapSet(...args); - return __originalSet.apply(signMap, args); - }; + const setSpy = vi.spyOn(signMap, 'set'); try { elementTree.triggerComponentAtIndex(listRef, 0); - expect(mockSignMapSet).toHaveBeenCalled(); - expect(mockFlushElementTree).toHaveBeenCalled(); - expect(mockSignMapSet.mock.invocationCallOrder[0]) - .toBeLessThan(mockFlushElementTree.mock.invocationCallOrder[0]); + expect(setSpy).toHaveBeenCalled(); + expect(flushSpy).toHaveBeenCalled(); + expect(setSpy.mock.invocationCallOrder[0]) + .toBeLessThan(flushSpy.mock.invocationCallOrder[0]); } finally { - globalThis.__FlushElementTree = __originalFlushElementTree; - signMap.set = __originalSet; + flushSpy.mockRestore(); + setSpy.mockRestore(); }
3129-3132: Small cleanup: use Map.sizeSimplify pool size assertion.
- const recycleSignMap = recycleMap.get(s1); - expect(Array.from(recycleSignMap.keys()).length).toBe(2); + const recycleSignMap = recycleMap.get(s1); + expect(recycleSignMap.size).toBe(2);
3088-3113: Mirror spy-based approach here as wellAlign with the first test to avoid direct reassignment.
- const mockFlushElementTree = vi.fn(); - const mockSignMapSet = vi.fn(); - - const __originalFlushElementTree = globalThis.__FlushElementTree; - globalThis.__FlushElementTree = (_, options = {}) => { - mockFlushElementTree(options); - }; + const flushSpy = vi.spyOn(globalThis, '__FlushElementTree'); ... - const __originalSet = signMap.set; - signMap.set = (...args) => { - mockSignMapSet(...args); - return __originalSet.apply(signMap, args); - }; + const setSpy = vi.spyOn(signMap, 'set'); ... - expect(mockSignMapSet).toHaveBeenCalled(); - expect(mockFlushElementTree).toHaveBeenCalled(); - expect(mockSignMapSet.mock.invocationCallOrder[0]) - .toBeLessThan(mockFlushElementTree.mock.invocationCallOrder[0]); + expect(setSpy).toHaveBeenCalled(); + expect(flushSpy).toHaveBeenCalled(); + expect(setSpy.mock.invocationCallOrder[0]) + .toBeLessThan(flushSpy.mock.invocationCallOrder[0]); ... - globalThis.__FlushElementTree = __originalFlushElementTree; - signMap.set = __originalSet; + flushSpy.mockRestore(); + setSpy.mockRestore();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react/runtime/__test__/list.test.jsx(1 hunks)packages/react/runtime/src/list.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react/runtime/__test__/list.test.jsx (4)
packages/react/runtime/src/snapshot.ts (1)
SnapshotInstance(278-658)packages/react/runtime/src/pendingListUpdates.ts (1)
__pendingListUpdates(7-44)packages/react/runtime/__test__/utils/nativeMethod.ts (3)
options(23-23)__GetElementUniqueID(40-43)elementTree(25-383)packages/react/runtime/src/list.ts (2)
gSignMap(9-9)gRecycleMap(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (4)
packages/react/runtime/src/list.ts (2)
158-163: Early signMap.set before flush in reuse-path looks correctThis ordering prevents enqueueComponent during flush from reading a stale/empty mapping. The explanatory comment is clear.
197-197: Set mapping before flush in normal path — goodThis aligns ordering across paths and removes a timing hole.
packages/react/runtime/__test__/list.test.jsx (2)
3035-3079: Test captures the ordering contract wellAsserts that signMap.set happens before __FlushElementTree when no reuse occurs. Nice use of invocationCallOrder and proper cleanup in finally.
3081-3141: Good coverage of reuse-path orderingThis validates the recycled path and ensures mapping happens before flush when pulling from the pool.
hzy
reviewed
Sep 9, 2025
hzy
reviewed
Sep 9, 2025
Co-authored-by: Zhiyuan Hong <28915578+hzy@users.noreply.github.com> Signed-off-by: BitterGourd <91231822+gaoachao@users.noreply.github.com>
React Example#4982 Bundle Size — 238.2KiB (0%).f34ae01(current) vs ffc1cd5 main#4967(baseline) Bundle metrics
|
| Current #4982 |
Baseline #4967 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
163 |
163 |
| Duplicate Modules |
67 |
67 |
| Duplicate Code |
46.88% |
46.88% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4982 |
Baseline #4967 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
92.45KiB |
92.45KiB |
Bundle analysis report Branch fix/should-update-signMap-before... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#4975 Bundle Size — 367.22KiB (0%).f34ae01(current) vs ffc1cd5 main#4959(baseline) Bundle metrics
|
| Current #4975 |
Baseline #4959 |
|
|---|---|---|
| Initial JS |
144.22KiB |
144.22KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
 Modules |
219(+0.46%) |
218 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
3.32%(-0.3%) |
3.33% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4975 |
Baseline #4959 |
|
|---|---|---|
| JS |
235.59KiB |
235.59KiB |
| Other |
99.79KiB |
99.79KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch fix/should-update-signMap-before... Project dashboard
Generated by RelativeCI Documentation Report issue
CodSpeed Performance ReportMerging #1692 will not alter performanceComparing 🎉 Hooray!
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.changeset/healthy-maps-clean.md (2)
5-7: Polish copy for clarity and accuracy.Tighten wording and explicitly say “calling” for readability.
Apply this diff:
-Fix "TypeError: cannot read property '0' of undefined" in deferred list-item scenarios. - -Deferred `componentAtIndex` causes nodes that quickly appear/disappear to be enqueued without `__elements`. Update `signMap` before `__FlushElementTree` to resolve the issue. +Fix "TypeError: cannot read property '0' of undefined" in deferred list-item scenarios. + +Deferred `componentAtIndex` can enqueue nodes that quickly appear/disappear without `__elements`. Update `signMap` before calling `__FlushElementTree` to resolve the issue. No public API changes.
8-8: Ensure trailing newline at EOF.Minor formatting nit; some linters require it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/healthy-maps-clean.md(1 hunks)packages/react/runtime/__test__/list.test.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/test/list.test.jsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (1)
.changeset/healthy-maps-clean.md (1)
1-3: Frontmatter looks valid for Changesets.Package and bump type are correct.
hzy
approved these changes
Sep 9, 2025
gaoachao
deleted the
fix/should-update-signMap-before-FlushElementTree
branch
colinaaa
pushed a commit
that referenced
this pull request
Sep 15, 2025
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.113.0 ### Minor Changes - fix: Delay execution of `runOnMainThread()` during initial render ([#1667](#1667)) When called during the initial render, `runOnMainThread()` would execute before the `main-thread:ref` was hydrated, causing it to be incorrectly set to null. This change delays the function's execution to ensure the ref is available and correctly assigned. ### Patch Changes - Fix "TypeError: cannot read property '0' of undefined" in deferred list-item scenarios. ([#1692](#1692)) Deferred `componentAtIndex` causes nodes that quickly appear/disappear to be enqueued without `__elements`. Update `signMap` before `__FlushElementTree` to resolve the issue. - Keep the same `<page/>` element when calling `rerender` in testing library. ([#1656](#1656)) - Bump `swc_core` to `39.0.3`. ([#1721](#1721)) ## @lynx-js/tailwind-preset@0.3.0 ### Minor Changes - Added `group-*`, `peer-*`, and `parent-*` modifiers (ancestor, sibling, and direct-parent scopes) for `uiVariants` plugin. ([#1741](#1741)) Fixed prefix handling in prefixed projects — `ui-*` state markers are not prefixed, while scope markers (`.group`/`.peer`) honor `config('prefix')`. **BREAKING**: Removed slash-based naming modifiers on self (non-standard); slash modifiers remain supported for scoped markers (e.g. `group/menu`, `peer/tab`). Bumped peer dependency to `tailwindcss@^3.4.0` (required for use of internal features). ## @lynx-js/react-webpack-plugin@0.7.0 ### Minor Changes - Remove `@lynx-js/react` from peerDependencies. ([#1711](#1711)) - Add a new required option `workletRuntimePath`. ([#1711](#1711)) ## @lynx-js/rspeedy@0.11.2 ### Patch Changes - Support `server.proxy`. ([#1745](#1745)) - Support `command` and `env` parameters in the function exported by `lynx.config.js`. ([#1669](#1669)) ```js import { defineConfig } from "@lynx-js/rspeedy"; export default defineConfig(({ command, env }) => { const isBuild = command === "build"; const isTest = env === "test"; return { output: { minify: !isTest, }, performance: { buildCache: isBuild, }, }; }); ``` - Support `resolve.dedupe`. ([#1671](#1671)) This is useful when having multiple duplicated packages in the bundle: ```js import { defineConfig } from "@lynx-js/rspeedy"; export default defineConfig({ resolve: { dedupe: ["tslib"], }, }); ``` - Support `resolve.aliasStrategy` for controlling priority between `tsconfig.json` paths and `resolve.alias` ([#1722](#1722)) ```js import { defineConfig } from "@lynx-js/rspeedy"; export default defineConfig({ resolve: { alias: { "@": "./src", }, // 'prefer-tsconfig' (default): tsconfig.json paths take priority // 'prefer-alias': resolve.alias takes priority aliasStrategy: "prefer-alias", }, }); ``` - Bump Rsbuild v1.5.4 with Rspack v1.5.2. ([#1644](#1644)) - Updated dependencies \[[`d7c5da3`](d7c5da3)]: - @lynx-js/chunk-loading-webpack-plugin@0.3.3 - @lynx-js/cache-events-webpack-plugin@0.0.2 ## @lynx-js/react-rsbuild-plugin@0.10.14 ### Patch Changes - Fix using wrong version of `@lynx-js/react/worklet-runtime`. ([#1711](#1711)) - Be compat with `@lynx-js/react` v0.113.0 ([#1667](#1667)) - Disable `builtin:lightningcss-loader` for `environments.web`. ([#1732](#1732)) - Updated dependencies \[[`5ad38e6`](5ad38e6), [`69b3ae0`](69b3ae0), [`69b3ae0`](69b3ae0), [`c2f90bd`](c2f90bd)]: - @lynx-js/template-webpack-plugin@0.8.6 - @lynx-js/react-webpack-plugin@0.7.0 - @lynx-js/react-alias-rsbuild-plugin@0.10.14 - @lynx-js/css-extract-webpack-plugin@0.6.2 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/react-refresh-webpack-plugin@0.3.4 ## @lynx-js/react-alias-rsbuild-plugin@0.10.14 ### Patch Changes - Allow customization of the react$ alias. ([#1653](#1653)) ```js import { defineConfig } from "@lynx-js/rspeedy"; export default defineConfig({ resolve: { alias: { react$: "@lynx-js/react/compat", }, }, }); ``` ## @lynx-js/web-constants@0.16.1 ### Patch Changes - feat: supports lazy bundle. (This feature requires `@lynx-js/lynx-core >= 0.1.3`) ([#1235](#1235)) - Updated dependencies \[]: - @lynx-js/web-worker-rpc@0.16.1 ## @lynx-js/web-core@0.16.1 ### Patch Changes - refactor: improve chunk loading ([#1703](#1703)) - feat: supports lazy bundle. (This feature requires `@lynx-js/lynx-core >= 0.1.3`) ([#1235](#1235)) - Updated dependencies \[[`608f375`](608f375)]: - @lynx-js/web-mainthread-apis@0.16.1 - @lynx-js/web-worker-runtime@0.16.1 - @lynx-js/web-constants@0.16.1 - @lynx-js/web-worker-rpc@0.16.1 ## @lynx-js/web-core-server@0.16.1 ### Patch Changes - refactor: improve chunk loading ([#1703](#1703)) - feat: supports lazy bundle. (This feature requires `@lynx-js/lynx-core >= 0.1.3`) ([#1235](#1235)) ## @lynx-js/web-elements@0.8.6 ### Patch Changes - fix: 1. svg use image tag to render, to differentiate background-image styles ([#1668](#1668)) 1. use blob instead of raw data-uri > Not using data-uri(data:image/svg+xml;utf8,${props.content}) > since it has follow limitations: > > < and > must be encoded to %3C and %3E. > Double quotes must be converted to single quotes. > Colors must use a non-hex format because # will not work inside data-uri. > See: <https://codepen.io/zvuc/pen/BWNLJL> > Instead, we use modern Blob API to create SVG URL that have the same support - Updated dependencies \[[`d618304`](d618304), [`1d97fce`](1d97fce)]: - @lynx-js/web-elements-template@0.8.6 ## @lynx-js/web-elements-template@0.8.6 ### Patch Changes - x-overlay-ng prevent page scroll when visible ([#1499](#1499)) - fix: 1. svg use image tag to render, to differentiate background-image styles ([#1668](#1668)) 1. use blob instead of raw data-uri > Not using data-uri(data:image/svg+xml;utf8,${props.content}) > since it has follow limitations: > > < and > must be encoded to %3C and %3E. > Double quotes must be converted to single quotes. > Colors must use a non-hex format because # will not work inside data-uri. > See: <https://codepen.io/zvuc/pen/BWNLJL> > Instead, we use modern Blob API to create SVG URL that have the same support ## @lynx-js/web-mainthread-apis@0.16.1 ### Patch Changes - feat: supports lazy bundle. (This feature requires `@lynx-js/lynx-core >= 0.1.3`) ([#1235](#1235)) - Updated dependencies \[[`608f375`](608f375)]: - @lynx-js/web-constants@0.16.1 - @lynx-js/web-style-transformer@0.16.1 ## @lynx-js/web-worker-runtime@0.16.1 ### Patch Changes - feat: supports lazy bundle. (This feature requires `@lynx-js/lynx-core >= 0.1.3`) ([#1235](#1235)) - Updated dependencies \[[`608f375`](608f375)]: - @lynx-js/web-mainthread-apis@0.16.1 - @lynx-js/web-constants@0.16.1 - @lynx-js/web-worker-rpc@0.16.1 ## @lynx-js/chunk-loading-webpack-plugin@0.3.3 ### Patch Changes - Fix unmet peer dependency "@rspack/core@'^1.3.10". ([#1660](#1660)) ## @lynx-js/template-webpack-plugin@0.8.6 ### Patch Changes - fix: add appType field for lazy bundle for web ([#1738](#1738)) ## create-rspeedy@0.11.2 ## upgrade-rspeedy@0.11.2 ## @lynx-js/web-style-transformer@0.16.1 ## @lynx-js/web-worker-rpc@0.16.1 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Checklist