chore(react): bump @lynx-js/internal-preact + fix SlotV2 cross-wrapper InsertBefore#2512
chore(react): bump @lynx-js/internal-preact + fix SlotV2 cross-wrapper InsertBefore#2512
Conversation
🦋 Changeset detectedLatest commit: 4221128 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:
📝 WalkthroughWalkthroughUpgrades Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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! |
Merging this PR will improve performance by 9.1%
Performance Changes
Comparing 
Footnotes
|
React External#746 Bundle Size — 680.27KiB (+0.01%).4221128(current) vs 989f345 main#736(baseline) Bundle metrics
|
| Current #746 |
Baseline #736 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
39.69% |
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 #746 |
Baseline #736 |
|
|---|---|---|
| Other |
680.27KiB (+0.01%) |
680.2KiB |
Bundle analysis report Branch chore/bump-internal-preact-10.29... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9202 Bundle Size — 900.02KiB (0%).4221128(current) vs 989f345 main#9192(baseline) Bundle metrics
|
| Current #9202 |
Baseline #9192 |
|
|---|---|---|
| Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
230(+1.32%) |
227 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.28%(-0.04%) |
27.29% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9202 |
Baseline #9192 |
|
|---|---|---|
| JS |
495.88KiB |
495.88KiB |
| Other |
401.92KiB |
401.92KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch chore/bump-internal-preact-10.29... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#761 Bundle Size — 196.54KiB (+0.04%).4221128(current) vs 989f345 main#751(baseline) Bundle metrics
|
| Current #761 |
Baseline #751 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
43.38% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
66 |
66 |
| Duplicate Code |
44.08%(+0.02%) |
44.07% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #761 |
Baseline #751 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
85.31KiB (+0.08%) |
85.24KiB |
Bundle analysis report Branch chore/bump-internal-preact-10.29... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7629 Bundle Size — 225.38KiB (+0.03%).4221128(current) vs 989f345 main#7619(baseline) Bundle metrics
|
| Current #7629 |
Baseline #7619 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
35.31% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
69 |
69 |
| Duplicate Code |
44.57% |
44.57% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #7629 |
Baseline #7619 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
79.63KiB (+0.09%) |
79.55KiB |
Bundle analysis report Branch chore/bump-internal-preact-10.29... Project dashboard
Generated by RelativeCI Documentation Report issue
…-e604bd5 Includes a diff-time fix that baselines `__slotIndex` on freshly-created DOM nodes, preventing `insert()`'s slot-branch from firing spuriously on first placement and avoiding a stale `insertBefore` reference when a sibling detaches mid-diff.
upupming
force-pushed
the
chore/bump-internal-preact-10.29.1
branch
from
149ec84 to
5854047
Compare
![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
🤖 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/testing-library/src/__tests__/setState-jsx.test.jsx`:
- Around line 174-185: The test calls lynxTestingEnv.switchToMainThread(), runs
assertions (inspecting __root and calling printSnapshotInstanceToString), then
calls lynxTestingEnv.switchToBackgroundThread() but if an assertion throws the
background switch is skipped; fix by wrapping the main-thread inspection block
(the code between lynxTestingEnv.switchToMainThread() and
switchToBackgroundThread()) in a try/finally and ensure
lynxTestingEnv.switchToBackgroundThread() is invoked in the finally block so the
environment is always restored; apply the same try/finally pattern to the other
occurrence mentioned (the block around lines 314-325).
🪄 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: 8c804e5d-d5a1-4439-a01e-8f0fed50b2b0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.changeset/bump-internal-preact-10-29-1.mdpackages/react/package.jsonpackages/react/testing-library/src/__tests__/setState-jsx.test.jsx
✅ Files skipped from review due to trivial changes (2)
- packages/react/package.json
- .changeset/bump-internal-preact-10-29-1.md
Rename setState-jsx.test.jsx → slot-jsx.test.jsx and add three tests exposing the slot-branch null-fallback bug in @lynx-js/internal-preact: single-key cross-slot move emits wrong beforeId, multi-key move drops children from the background snapshot tree, and a 200-step fuzz that catches any layout producing a spurious null beforeId. Also update changeset: bump to minor, shorten description, add diff link.
![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: fb4f48d72b
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/react/testing-library/src/__tests__/slot-jsx.test.jsx (3)
174-185: Thread-switch state can leak on failure.Lines 174/185 (and 314/325) flip
lynxTestingEnvbetween main and background threads without atry/finally. If any interveningexpectthrows, subsequent tests inherit the wrong thread and cascade with confusing failures. Not a blocker, but atry { switchToMainThread(); ... } finally { switchToBackgroundThread(); }— or aafterEachthat normalizes the thread — would make failures localized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/testing-library/src/__tests__/slot-jsx.test.jsx` around lines 174 - 185, The test flips lynxTestingEnv between threads without guaranteeing restoration, so failing assertions can leave the environment on the main thread; wrap the section that calls lynxTestingEnv.switchToMainThread() and the subsequent assertions in a try/finally that calls lynxTestingEnv.switchToBackgroundThread() in the finally block (or add an afterEach that always calls lynxTestingEnv.switchToBackgroundThread()), ensuring the code paths using switchToMainThread()/switchToBackgroundThread() (and the surrounding assertions like expect(...).toMatchInlineSnapshot and printSnapshotInstanceToString(__root)) always restore the background thread even on failure.
10-13: Unused spy reference.
onLifecycleEventCallsis captured on line 11 but never read; same for test 2 which spies onOnLifecycleEventat line 334 without asserting anything. Either assert against it or drop the spy to reduce noise.♻️ Proposed cleanup
- vi.spyOn(lynxTestingEnv.backgroundThread.lynxCoreInject.tt, 'OnLifecycleEvent'); - const onLifecycleEventCalls = lynxTestingEnv.backgroundThread.lynxCoreInject.tt.OnLifecycleEvent.mock.calls; vi.spyOn(lynx.getNativeApp(), 'callLepusMethod'); const callLepusMethodCalls = lynx.getNativeApp().callLepusMethod.mock.calls;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/testing-library/src/__tests__/slot-jsx.test.jsx` around lines 10 - 13, The test creates unused spies and captures (remove or assert them): either delete the vi.spyOn(...) for lynxTestingEnv.backgroundThread.lynxCoreInject.tt.OnLifecycleEvent and the unused const onLifecycleEventCalls (and the similar unused spy later that references OnLifecycleEvent), or replace them with meaningful assertions (e.g., expect(onLifecycleEventCalls.length).toBeGreaterThan(0)) where you actually rely on those calls; also keep or assert callLepusMethodCalls from lynx.getNativeApp().callLepusMethod if it's used, otherwise remove that spy/const as well. Ensure references to OnLifecycleEvent, onLifecycleEventCalls, and callLepusMethodCalls are either asserted against or removed.
484-506: Use structural tree traversal instead of indentation-dependent regex for child counting.The regex
/^ {4}\| \d+\(/gmis tightly coupled to the hardcoded 2-space-per-level indentation inprintSnapshotInstanceToString. If that indentation format ever changes (to 1 space, 3 spaces, tabs, etc.), the regex silently returns0and the assertions pass vacuously.Since
SnapshotInstance.childNodesis available, replace the regex with direct property access:const getViewChildCount = () => { const viewNode = __root.childNodes[0]; // assuming view is first child return viewNode.childNodes.length; };This eliminates the indentation dependency and is more explicit about the tree structure being tested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/testing-library/src/__tests__/slot-jsx.test.jsx` around lines 484 - 506, The test's getViewChildCount uses an indentation-dependent regex on printSnapshotInstanceToString which can break if formatting changes; replace that with a structural traversal using the SnapshotInstance.childNodes tree (starting from __root and locating the view node, e.g. __root.childNodes[0]) and return its childNodes.length instead of relying on the regex so getViewChildCount, printSnapshotInstanceToString and __root use the actual tree structure.
🤖 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/testing-library/src/__tests__/slot-jsx.test.jsx`:
- Around line 396-436: The comments are stale versus the new slot-branch
behavior; update the test text to match the post-fix snapshots (or update
snapshots if you intended to assert the pre-fix behavior). Specifically, either
remove/replace the "Bug: E moves..." and "Background tree: wrong order..." lines
with a one-line explanation that beforeId can be null because SlotV2 uses
slotIndex (referencing callLepusMethodCalls and prettyFormatSnapshotPatch for
the patch expectation), or if you intended to validate the corrected behavior
adjust the inline snapshots and the final tree assertion produced by
printSnapshotInstanceToString(__root) to show [E, A, N, D]. Ensure any comment
mentions SlotV2/slotIndex and reference functions prettyFormatSnapshotPatch and
printSnapshotInstanceToString so readers understand why beforeId is null.
- Around line 600-603: The inline snapshot call toMatchInlineSnapshot() is
empty, so the final-order assertion is a no-op; update the test by replacing the
empty call on the final assertion that uses
printSnapshotInstanceToString(__root) with a populated inline snapshot string
that represents the expected final layout in correct slot order (capture the
output of printSnapshotInstanceToString(__root) after the fuzz run and paste it
into the toMatchInlineSnapshot call). Locate the assertion using
printSnapshotInstanceToString and __root in the slot-jsx.test.jsx test and
insert the concrete snapshot text into toMatchInlineSnapshot to make the
invariant effective.
- Around line 507-510: The inline snapshot assertion is empty and doesn't verify
the expected post-fix slot order; update the test that calls
expect(printSnapshotInstanceToString(__root)).toMatchInlineSnapshot() to include
the actual snapshot output showing the final slot order (the tree string that
reflects [F, H, E, G] order) so the regression fails if the order changes; run
the test locally (or let Jest populate the inline snapshot) and commit the
populated snapshot string for the expect(printSnapshotInstanceToString(__root))
assertion.
---
Nitpick comments:
In `@packages/react/testing-library/src/__tests__/slot-jsx.test.jsx`:
- Around line 174-185: The test flips lynxTestingEnv between threads without
guaranteeing restoration, so failing assertions can leave the environment on the
main thread; wrap the section that calls lynxTestingEnv.switchToMainThread() and
the subsequent assertions in a try/finally that calls
lynxTestingEnv.switchToBackgroundThread() in the finally block (or add an
afterEach that always calls lynxTestingEnv.switchToBackgroundThread()), ensuring
the code paths using switchToMainThread()/switchToBackgroundThread() (and the
surrounding assertions like expect(...).toMatchInlineSnapshot and
printSnapshotInstanceToString(__root)) always restore the background thread even
on failure.
- Around line 10-13: The test creates unused spies and captures (remove or
assert them): either delete the vi.spyOn(...) for
lynxTestingEnv.backgroundThread.lynxCoreInject.tt.OnLifecycleEvent and the
unused const onLifecycleEventCalls (and the similar unused spy later that
references OnLifecycleEvent), or replace them with meaningful assertions (e.g.,
expect(onLifecycleEventCalls.length).toBeGreaterThan(0)) where you actually rely
on those calls; also keep or assert callLepusMethodCalls from
lynx.getNativeApp().callLepusMethod if it's used, otherwise remove that
spy/const as well. Ensure references to OnLifecycleEvent, onLifecycleEventCalls,
and callLepusMethodCalls are either asserted against or removed.
- Around line 484-506: The test's getViewChildCount uses an
indentation-dependent regex on printSnapshotInstanceToString which can break if
formatting changes; replace that with a structural traversal using the
SnapshotInstance.childNodes tree (starting from __root and locating the view
node, e.g. __root.childNodes[0]) and return its childNodes.length instead of
relying on the regex so getViewChildCount, printSnapshotInstanceToString and
__root use the actual tree structure.
🪄 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: c695dfca-1376-4e0a-8e77-445510740e22
📒 Files selected for processing (3)
.changeset/bump-internal-preact-10-29-1.mdpackages/react/testing-library/src/__tests__/setState-jsx.test.jsxpackages/react/testing-library/src/__tests__/slot-jsx.test.jsx
💤 Files with no reviewable changes (1)
- packages/react/testing-library/src/tests/setState-jsx.test.jsx
✅ Files skipped from review due to trivial changes (1)
- .changeset/bump-internal-preact-10-29-1.md
…e behavior All four tests now assert the fixed behavior: E/H/G land in their correct slots with real beforeId references, background and main-thread trees both show the right slot order, and the 200-step fuzz passes without child-count regression. Remove unused Component import, dead OnLifecycleEvent spy, and stale comments that described the old buggy state.
upupming
changed the title
upupming
changed the title
Updates the bump target to the merged version of PR #18 in internal-preact, which includes both the cross-slot keyed move fix and the self-insert guard. Same code as the published 10.29.1-20260424024911-12b794f release. Continues using the pkg.pr.new URL form: the published npm package is named @lynx-js/internal-preact, which can't satisfy @preact/signals's preact peer dep, leading to dual-preact instances and broken hook state in tests. The pkg.pr.new tarball is named preact and resolves cleanly.
upupming
force-pushed
the
chore/bump-internal-preact-10.29.1
branch
from
96cd3e9 to
7dbda08
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dbda08ca7
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react/testing-library/src/__tests__/slot-jsx.test.jsx (2)
536-553: Fuzz test collectspatchesbut never asserts against them — test body doesn't match its name.The test title claims to verify "update patches have no spurious null beforeId when later slot siblings exist", and lines 548–553 replace
callLepusMethodwith a mock that accumulates everydatapayload intopatches. However,patchesis never read afterwards — the only invariants checked arechildCount() === SLOT_COUNT(line 611) and the final tree snapshot (line 621). The child-count check catches gross corruption but wouldn't detect, e.g., a correctly-placed element whoseInsertBeforestill carries a straybeforeId: null— which is the exact scenario the title calls out.Either drop the unused collection to match what the test actually verifies, or add an explicit per-step scan of the emitted
snapshotPatchforInsertBeforeops whosebeforeIdisnullwhile a later keyed sibling exists in the target slot. The latter would make this test a real regression guard for the underlying internal-preact fix rather than relying on indirect symptoms.🧪 Sketch of a direct assertion
let failure = null; for (step = 1; step < STEPS && !failure; step++) { act(() => { setStep(step); }); const count = childCount(); if (count !== SLOT_COUNT) { failure = `step=${step}` + `\n prev=${JSON.stringify(layouts[step - 1])}` + `\n next=${JSON.stringify(layouts[step])}` + `\n want child count=${SLOT_COUNT}, got=${count}` + `\n tree=${printSnapshotInstanceToString(__root)}`; + continue; } + // Walk the most-recent patch and flag any InsertBefore with a null beforeId + // where a later keyed sibling already exists in that slot. + const last = patches[patches.length - 1]; + if (last) { + const ops = prettyFormatSnapshotPatch( + JSON.parse(last).patchList[0].snapshotPatch, + ); + // ... scan ops and set `failure` if a spurious null beforeId is found + } }Also applies to: 600-628
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/testing-library/src/__tests__/slot-jsx.test.jsx` around lines 536 - 553, The test creates and accumulates emitted patches by spying on lynx.getNativeApp().callLepusMethod into the patches array but never asserts on them; update the test (involving patches, callLepusMethod, snapshotPatch, InsertBefore, beforeId, SLOT_COUNT, childCount) to either remove the unused collection or (preferred) add a check after each mutation step that scans the collected patches for snapshotPatch payloads and fails if any InsertBefore operation has beforeId === null while the target slot contains a later keyed sibling—i.e., iterate patches, locate snapshotPatch ops, and assert no InsertBefore with beforeId null when a later sibling key exists in that slot to directly verify the bug described.
338-338: Remove unusedOnLifecycleEventspy.The spy installed on
lynxTestingEnv.backgroundThread.lynxCoreInject.tt.OnLifecycleEventis never referenced or asserted on in this test, so it does nothing beyond adding overhead. The PR explicitly states it removes unused spies elsewhere; this one appears to have been missed.♻️ Proposed fix
- vi.spyOn(lynxTestingEnv.backgroundThread.lynxCoreInject.tt, 'OnLifecycleEvent'); vi.spyOn(lynx.getNativeApp(), 'callLepusMethod');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/testing-library/src/__tests__/slot-jsx.test.jsx` at line 338, Remove the unused spy call on lynxTestingEnv.backgroundThread.lynxCoreInject.tt.OnLifecycleEvent: delete the vi.spyOn(lynxTestingEnv.backgroundThread.lynxCoreInject.tt, 'OnLifecycleEvent'); line from the test (slot-jsx.test.jsx) since it isn't asserted against or used; after removal, run the test suite to ensure no other test relies on that spy and adjust or add any required setup/teardown if failures appear.
🤖 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/testing-library/src/__tests__/slot-jsx.test.jsx`:
- Around line 536-553: The test creates and accumulates emitted patches by
spying on lynx.getNativeApp().callLepusMethod into the patches array but never
asserts on them; update the test (involving patches, callLepusMethod,
snapshotPatch, InsertBefore, beforeId, SLOT_COUNT, childCount) to either remove
the unused collection or (preferred) add a check after each mutation step that
scans the collected patches for snapshotPatch payloads and fails if any
InsertBefore operation has beforeId === null while the target slot contains a
later keyed sibling—i.e., iterate patches, locate snapshotPatch ops, and assert
no InsertBefore with beforeId null when a later sibling key exists in that slot
to directly verify the bug described.
- Line 338: Remove the unused spy call on
lynxTestingEnv.backgroundThread.lynxCoreInject.tt.OnLifecycleEvent: delete the
vi.spyOn(lynxTestingEnv.backgroundThread.lynxCoreInject.tt, 'OnLifecycleEvent');
line from the test (slot-jsx.test.jsx) since it isn't asserted against or used;
after removal, run the test suite to ensure no other test relies on that spy and
adjust or add any required setup/teardown if failures appear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14ee5bec-25f9-4333-8658-580d66c96bf6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.changeset/bump-internal-preact-10-29-1-12b794f.mdpackages/react/package.jsonpackages/react/testing-library/src/__tests__/slot-jsx.test.jsx
✅ Files skipped from review due to trivial changes (2)
- .changeset/bump-internal-preact-10-29-1-12b794f.md
- packages/react/package.json
`SnapshotInstance.insertBefore` for SlotV2 was using `parent.insertBefore(node, ref)` when `newNode.__slotIndex >= existingNode.__slotIndex`, but in SlotV2 each `$N` slot has its own wrapper element. When `existingNode` lives in a different slot wrapper than the target, the underlying DOM API throws `NotFoundError: The child can not be found in the parent.` because `ref` isn't a child of the target wrapper. Widen the existing append fallback to fire whenever the two nodes are in different slots (was: only when `newNode.__slotIndex < existingNode.__slotIndex`). DOM / native auto-detach via `appendChild` still moves the node out of its old wrapper. Add three slot-jsx regression tests: - single-key cross-slot move (E moves slot 2 → 0) - multi-key cross-slot moves (H, G move simultaneously) - three-key cross-slot move that previously threw (F, H + new A, D) - 10000-step / 6-slot fuzz mirroring the internal-preact property test Each cross-slot test snapshots the exact element-PAPI sequence (create / append / insertBefore / remove with element identifiers) so regressions that add redundant DOM ops or accidentally re-create reused keyed nodes show up as a snapshot diff.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47a0efeee8
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react/testing-library/src/__tests__/slot-jsx.test.jsx (1)
15-51: Optional: return arestore()fromspyElementApiand call it per test.
vi.spyOnis captured but never restored. Each test that invokesspyElementApi()grabsoriginal = g[name], which on the 2nd+ invocation is the previous test's wrapper — so spies accumulate and every PAPI call walks an ever‑growing chain of wrappers. The outeropsarray still receives the right trace, so this isn't a correctness bug, but it's brittle and makes trace diffs harder to reason about if anyone widens the helper later.♻️ Suggested refactor
function spyElementApi() { const g = lynxTestingEnv.mainThread.globalThis; + const spies = []; ... const wrap = (name, formatter) => { const original = g[name]; - vi.spyOn(g, name).mockImplementation((...args) => { + const spy = vi.spyOn(g, name).mockImplementation((...args) => { ops.push(formatter(args)); return original.apply(g, args); }); + spies.push(spy); }; ... return { mark() { mark = ops.length; }, trace() { return ops.slice(mark).join('\n'); }, + restore() { for (const s of spies) s.mockRestore(); }, }; }Then
const trace = spyElementApi();at the top of each test gets a matchingtrace.restore()(or useafterEach(() => vi.restoreAllMocks())at the file level ifrestoreMocksisn't already set in the vitest config).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/testing-library/src/__tests__/slot-jsx.test.jsx` around lines 15 - 51, spyElementApi currently spies on global methods via vi.spyOn but never restores them, causing spies to accumulate across tests; update spyElementApi to capture each created spy (or the original functions) inside the wrap closure and return a restore() method that restores each spy (e.g., call spy.mockRestore() or reassign g[name] = original) so tests can call trace = spyElementApi(); ...; trace.restore() (or use restore in afterEach). Ensure you reference the existing symbols spyElementApi, wrap, vi.spyOn, and g[name] when implementing the restore logic.
🤖 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/testing-library/src/__tests__/slot-jsx.test.jsx`:
- Line 721: The fuzz test "fuzz: cross-slot keyed moves keep slot order across
random layouts" is too tight for CI; reduce the STEPS constant from 10000 to
~2000, increase the test timeout from 5000 to 20000, and avoid asserting
slotOrder() on every iteration by only calling the equality checks every N
iterations (e.g., every 50) while still invoking setStep/act(setState) each
loop; apply the same changes to the related loop/test uses referenced by
STEPS/slotOrder()/setStep in the nearby tests.
---
Nitpick comments:
In `@packages/react/testing-library/src/__tests__/slot-jsx.test.jsx`:
- Around line 15-51: spyElementApi currently spies on global methods via
vi.spyOn but never restores them, causing spies to accumulate across tests;
update spyElementApi to capture each created spy (or the original functions)
inside the wrap closure and return a restore() method that restores each spy
(e.g., call spy.mockRestore() or reassign g[name] = original) so tests can call
trace = spyElementApi(); ...; trace.restore() (or use restore in afterEach).
Ensure you reference the existing symbols spyElementApi, wrap, vi.spyOn, and
g[name] when implementing the restore logic.
🪄 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: 7dcd6367-7856-4af7-aa0c-2146c17fd89c
📒 Files selected for processing (2)
packages/react/runtime/src/snapshot/snapshot/snapshot.tspackages/react/testing-library/src/__tests__/slot-jsx.test.jsx
- Replace the misleading "intra-slot ordering with cross-wrapper insert" test (the scenario was structurally unreachable because arrays in JSX become Fragments with their own diff context) with one that records the actual behavior: a keyed VNode moving from a view-level slot into an array slot is recreated, not cross-slot reused. - Add a test for the only structurally reachable cross-slot keyed reuse: two view-level single-VNode slots swapping content. Each slot still ends up with at most one child, so the cross-wrapper `__AppendElement` fallback in `SnapshotInstance.insertBefore` is correct.
upupming
changed the title
2 participants
Summary
Two fixes that together restore correct cross-slot keyed move behavior in
SlotV2snapshots.1. Bump
@lynx-js/internal-preactfor PR #18When a keyed child moved to a different
$Nslot across a re-render, preact'sinsert()slot-branch:40912aae) usednullas theinsertBeforereference when the diff cursor wasn't in the moved node's old slot, appending the moved node past stable siblings — wrong DOM order.995ae4de) when the moved node was exactly at the diff cursor, calledinsertBefore(node, node)which corruptedBackgroundSnapshotInstance's linked list with a self-cycle (eventually OOM on main thread).Both are fixed in PR #18 (merged →
10.29.1-20260424024911-12b794f). We pin via the pkg.pr.new URL because the published npm package is named@lynx-js/internal-preact, which can't satisfy@preact/signals'spreactpeer-dep — that creates a dual-preact graph and breaks hooks (see commit message ofd214d521).2. Fix
SnapshotInstance.insertBeforefor cross-wrapper caseFor SlotV2 each
$Nhas its own wrapper element. The original code only fell back to__AppendElementwhennewNode.__slotIndex < existingNode.__slotIndex; the>direction fell through to__InsertElementBefore(newWrapper, node, ref)which throwsNotFoundError: The child can not be found in the parent.becausereflives in another wrapper.The minimum repro is a 3-key cross-slot rearrange (
[F,H,E,G] → [H,A,D,F]) —Ais new at slot 1, but itsexistingNodecursor isF(still at slot 0 since F's slot update happens later). Widened the check to!==so any cross-wrapper insert falls back to__AppendElement, relying on DOMappendChild's auto-detach to move the node out of its old wrapper.Tests added (
packages/react/testing-library/src/__tests__/slot-jsx.test.jsx)E moves $2 → $0) with patch + tree assertionsH moves $0 → $1, G moves $2 → $3)Each cross-slot test snapshots the exact element-PAPI sequence (
create / append / insertBefore / remove) so any regression that adds a redundant DOM op or accidentally re-creates a reused keyed node shows up as a snapshot diff.Test plan
slot-jsx.test.jsxpasses (8 tests)react/runtime+react/testing-librarysuites pass locally (657 tests)