Skip to content

Commit 8c4a9e6

Browse files
committed
Fix edge case where oldChild can become detach causing the recursive function to end early
1 parent 0e48d10 commit 8c4a9e6

File tree

3 files changed

+88
-0
lines changed

3 files changed

+88
-0
lines changed

.changeset/new-plants-exercise.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"rrdom": patch
3+
---
4+
5+
Fix early termination of recursive loop in diffChilren when a node mismatch is found

packages/rrdom/src/diff.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -527,7 +527,26 @@ function diffChildren(
527527
let oldChild = oldTree.firstChild;
528528
let newChild = newTree.firstChild;
529529
while (oldChild !== null && newChild !== null) {
530+
// For mismatched node types (e.g., Element vs Text), we create a new node and insert it before the old one.
531+
// This preserves the oldChild reference needed for the recursive diff loop, which would break if we replaced the node directly.
532+
const isMismatch = !sameNodeType(oldChild, newChild);
533+
534+
if (isMismatch) {
535+
const newNode = createOrGetNode(newChild, replayer.mirror, rrnodeMirror);
536+
537+
try {
538+
handleInsertBefore(oldTree, newNode, oldChild);
539+
} catch (e) {
540+
console.warn(e);
541+
}
542+
543+
diff(newNode, newChild, replayer, rrnodeMirror);
544+
newChild = newChild.nextSibling;
545+
continue;
546+
}
547+
530548
diff(oldChild, newChild, replayer, rrnodeMirror);
549+
531550
oldChild = oldChild.nextSibling;
532551
newChild = newChild.nextSibling;
533552
}

packages/rrdom/test/diff.test.ts

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
Mirror as RRNodeMirror,
1313
RRDocument,
1414
RRMediaElement,
15+
RRElement,
1516
printRRDom,
1617
} from '../src';
1718
import {
@@ -1183,6 +1184,69 @@ describe('diff algorithm for rrdom', () => {
11831184
expect(node.childNodes[0].childNodes.length).toEqual(1);
11841185
expect(mirror.getId(node.childNodes[0].childNodes[0])).toEqual(2);
11851186
});
1187+
1188+
it('should maintain correct order when duplicate node is found causing nextSibling traversal desync', () => {
1189+
const oldList = [
1190+
{ tagName: 'STYLE', id: 1 },
1191+
{ tagName: 'STYLE', id: 2 },
1192+
{ tagName: 'BASE', id: 3 },
1193+
{ tagName: 'META', id: 4 },
1194+
{ tagName: 'META', id: 5 },
1195+
{ tagName: 'META', id: 6 },
1196+
{ tagName: 'TITLE', id: 7 },
1197+
{ tagName: 'STYLE', id: 8 },
1198+
{ tagName: 'STYLE', id: 9 },
1199+
{ tagName: 'LINK', id: 10 },
1200+
{ tagName: 'STYLE', id: 11 },
1201+
{ tagName: 'LINK', id: 12 },
1202+
{ tagName: 'NOSCRIPT', id: 13 },
1203+
];
1204+
1205+
// Duplicate STYLE#1 at index 1
1206+
const newList = [...oldList];
1207+
newList.splice(1, 0, { tagName: 'STYLE', id: 1 });
1208+
1209+
const mirror = createMirror();
1210+
const oldHead = createTree(
1211+
{ tagName: 'head', id: 0, children: oldList as unknown as ElementType[] },
1212+
undefined,
1213+
mirror,
1214+
) as Node;
1215+
expect(oldHead.childNodes.length).toBe(oldList.length);
1216+
1217+
const rrdom = new RRDocument();
1218+
const newHead = createTree(
1219+
{ tagName: 'head', id: 0, children: newList as unknown as ElementType[] },
1220+
rrdom
1221+
) as RRNode;
1222+
1223+
// Add test attributes to each node in newHead
1224+
Array.from(newHead.childNodes).forEach((node, index) => {
1225+
if (node instanceof RRElement) {
1226+
node.setAttribute('data-test-id', `${newList[index].id}`);
1227+
node.setAttribute('data-test-tag', node.tagName);
1228+
}
1229+
});
1230+
1231+
const replayer: ReplayerHandler = {
1232+
mirror,
1233+
applyCanvas: () => {},
1234+
applyInput: () => {},
1235+
applyScroll: () => {},
1236+
applyStyleSheetMutation: () => {},
1237+
};
1238+
1239+
// Run diff
1240+
diff(oldHead, newHead, replayer);
1241+
1242+
// Extract final IDs from real DOM
1243+
const finalIds = Array.from(oldHead.childNodes)
1244+
.filter((n): n is Element => n.nodeType === Node.ELEMENT_NODE)
1245+
.map((el) => el.getAttribute('data-test-id') || el.getAttribute('id'));
1246+
1247+
// Assert the real DOM now matches the RR DOM list
1248+
expect(finalIds).toEqual(oldList.map(n => String(n.id)));
1249+
});
11861250
});
11871251

11881252
describe('diff shadow dom', () => {

0 commit comments

Comments
 (0)