fix: place cross-slot keyed moves at cursor instead of appending#18
Merged
fix: place cross-slot keyed moves at cursor instead of appending#18
Conversation
…lot mismatch
`insert()`'s slot-branch was using
parentVNode._dom.__slotIndex === oldDom?.__slotIndex ? oldDom : null
to pick the `insertBefore` reference. The `null` fallback (= append to
parent's end) was assumed to self-correct via subsequent slot
processing, but it doesn't: `constructNewChildrenArray` only flags
nodes with `INSERT_VNODE` when their skewed index actually shifts, so
keyed siblings that stay in the same slot do not call back into
`insert()` and never re-insert themselves to rebuild the tail.
Concretely, when a keyed node moves to an earlier slot while later
slots remain stable, the moved node gets appended past the stable tail
and stays there, producing wrong DOM order. Property-based fuzzing
over $N slot layouts hits this within the first iteration; minimized
repro is `[X, A, E, D]` → `[E, A, N, D]` (E moves slot 2 → slot 0,
A and D stable).
Drop the conditional and always pass `oldDom` as the reference
(falling back to `getDomSibling` if it's been detached mid-diff,
mirroring the plain-branch's recovery). Inserting before the cursor is
exactly what the cursor-driven loop expects: the moved node lands at
the current cursor position and the cursor advances one slot.
Tests:
- Add `should place cross-slot keyed move before stable tail siblings`,
the minimized 4-slot deterministic repro.
- Add `fuzz: $N slot placement matches slot order under random
layouts`: 10k iterations of random keyed permutations under a
fixed-size $N slot set, deterministic xorshift32 seed, prints prev
and next layouts on mismatch for easy minimization. Runs in ~90ms.
Local: 1090 passed / 170 skipped / 0 failed in both source and
MINIFY modes.
|
Size Change: +1.61 kB (+0.74%) Total Size: 221 kB 📦 View Changed
ℹ️ View Unchanged
|
commit: |
When a cross-slot keyed node happens to sit exactly at the diff loop cursor (oldDom === parentVNode._dom), inserting it before itself is a no-op in real DOM but corrupts the BackgroundSnapshotInstance linked list by creating a self-cycle (H.__nextSibling = H). Use oldDom.nextSibling as the reference instead: the effective DOM position is unchanged, the BSI linked list stays intact, and the patch carries a valid (non-self) beforeId so the main-thread renderer correctly receives the updated slotIndex for the moved node.
📊 Tachometer Benchmark ResultsSummaryA summary of the benchmark results will show here once they finish. ResultsThe full results of your benchmarks will show here once they finish. |
upupming
changed the title
Collaborator
Author
|
@codex review |
3 tasks
upupming
added a commit
to lynx-family/lynx-stack
that referenced
this pull request
Apr 24, 2026
…r InsertBefore (#2512) ## Summary Two fixes that together restore correct cross-slot keyed move behavior in `SlotV2` snapshots. ### 1. Bump `@lynx-js/internal-preact` for [PR #18](lynx-family/internal-preact#18) When a keyed child moved to a different `$N` slot across a re-render, preact's `insert()` slot-branch: - (commit 1, `40912aae`) used `null` as the `insertBefore` reference when the diff cursor wasn't in the moved node's old slot, appending the moved node past stable siblings — wrong DOM order. - (commit 2, `995ae4de`) when the moved node *was* exactly at the diff cursor, called `insertBefore(node, node)` which corrupted `BackgroundSnapshotInstance`'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`'s `preact` peer-dep — that creates a dual-preact graph and breaks hooks (see commit message of `d214d521`). ### 2. Fix `SnapshotInstance.insertBefore` for cross-wrapper case For SlotV2 each `$N` has its own wrapper element. The original code only fell back to `__AppendElement` when `newNode.__slotIndex < existingNode.__slotIndex`; the `>` direction fell through to `__InsertElementBefore(newWrapper, node, ref)` which throws `NotFoundError: The child can not be found in the parent.` because `ref` lives in another wrapper. The minimum repro is a 3-key cross-slot rearrange (`[F,H,E,G] → [H,A,D,F]`) — `A` is new at slot 1, but its `existingNode` cursor is `F` (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 DOM `appendChild`'s auto-detach to move the node out of its old wrapper. ## Tests added (`packages/react/testing-library/src/__tests__/slot-jsx.test.jsx`) - single-key cross-slot move (`E moves $2 → $0`) with patch + tree assertions - multi-key cross-slot move (`H moves $0 → $1, G moves $2 → $3`) - three-key cross-slot move (the regression that triggered the runtime fix) - two-slot keyed swap - array slot diff-context isolation — documents that arrays-in-slots break cross-slot keyed reuse, so the unreachable edge case stays unreachable - 10000-step / 6-slot fuzz, **keyed and unkeyed variants** end-to-end through the main-thread renderer, mirroring the property-based fuzz in internal-preact 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 - [x] `slot-jsx.test.jsx` passes (8 tests) - [x] full `react/runtime` + `react/testing-library` suites pass locally (657 tests) - [ ] CI green
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
1 participant
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.
Problem
When a keyed node moves to a different
$Nslot across a re-render, preact'sinsert()slot-branch was usingnullas theinsertBeforereference, which appended the node past any stable siblings that followed — wrong DOM order.A second edge case: when the cross-slot node happens to already sit exactly at the diff cursor (
oldDom === parentVNode._dom), callinginsertBefore(node, node)corruptsBackgroundSnapshotInstance's linked list by creating a self-cycle (H.__nextSibling = H), eventually causing OOM on the main thread.Fix
Commit 1 (
40912aae): useoldDom(the diff cursor) as theinsertBeforereference, placing the moved node at the correct slot position rather than appending it.Commit 2 (
995ae4de): when the node is already at the cursor, useoldDom.nextSiblinginstead — same effective DOM position, no self-insert, and the patch carries a validbeforeIdso the main-thread renderer correctly receives the updatedslotIndex.Tests
Browser tests in
test/browser/render.test.jsx: