Skip to content

Commit 4833492

Browse files
committed
Fix edge case where oldChild can become detach causing the recursive function to end early
1 parent fd9d274 commit 4833492

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
@@ -525,7 +525,26 @@ function diffChildren(
525525
let oldChild = oldTree.firstChild;
526526
let newChild = newTree.firstChild;
527527
while (oldChild !== null && newChild !== null) {
528+
// For mismatched node types (e.g., Element vs Text), we create a new node and insert it before the old one.
529+
// This preserves the oldChild reference needed for the recursive diff loop, which would break if we replaced the node directly.
530+
const isMismatch = !sameNodeType(oldChild, newChild);
531+
532+
if (isMismatch) {
533+
const newNode = createOrGetNode(newChild, replayer.mirror, rrnodeMirror);
534+
535+
try {
536+
handleInsertBefore(oldTree, newNode, oldChild);
537+
} catch (e) {
538+
console.warn(e);
539+
}
540+
541+
diff(newNode, newChild, replayer, rrnodeMirror);
542+
newChild = newChild.nextSibling;
543+
continue;
544+
}
545+
528546
diff(oldChild, newChild, replayer, rrnodeMirror);
547+
529548
oldChild = oldChild.nextSibling;
530549
newChild = newChild.nextSibling;
531550
}

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 {
@@ -1156,6 +1157,69 @@ describe('diff algorithm for rrdom', () => {
11561157
expect(node.childNodes[0].childNodes.length).toEqual(1);
11571158
expect(mirror.getId(node.childNodes[0].childNodes[0])).toEqual(2);
11581159
});
1160+
1161+
it('should maintain correct order when duplicate node is found causing nextSibling traversal desync', () => {
1162+
const oldList = [
1163+
{ tagName: 'STYLE', id: 1 },
1164+
{ tagName: 'STYLE', id: 2 },
1165+
{ tagName: 'BASE', id: 3 },
1166+
{ tagName: 'META', id: 4 },
1167+
{ tagName: 'META', id: 5 },
1168+
{ tagName: 'META', id: 6 },
1169+
{ tagName: 'TITLE', id: 7 },
1170+
{ tagName: 'STYLE', id: 8 },
1171+
{ tagName: 'STYLE', id: 9 },
1172+
{ tagName: 'LINK', id: 10 },
1173+
{ tagName: 'STYLE', id: 11 },
1174+
{ tagName: 'LINK', id: 12 },
1175+
{ tagName: 'NOSCRIPT', id: 13 },
1176+
];
1177+
1178+
// Duplicate STYLE#1 at index 1
1179+
const newList = [...oldList];
1180+
newList.splice(1, 0, { tagName: 'STYLE', id: 1 });
1181+
1182+
const mirror = createMirror();
1183+
const oldHead = createTree(
1184+
{ tagName: 'head', id: 0, children: oldList as unknown as ElementType[] },
1185+
undefined,
1186+
mirror,
1187+
) as Node;
1188+
expect(oldHead.childNodes.length).toBe(oldList.length);
1189+
1190+
const rrdom = new RRDocument();
1191+
const newHead = createTree(
1192+
{ tagName: 'head', id: 0, children: newList as unknown as ElementType[] },
1193+
rrdom
1194+
) as RRNode;
1195+
1196+
// Add test attributes to each node in newHead
1197+
Array.from(newHead.childNodes).forEach((node, index) => {
1198+
if (node instanceof RRElement) {
1199+
node.setAttribute('data-test-id', `${newList[index].id}`);
1200+
node.setAttribute('data-test-tag', node.tagName);
1201+
}
1202+
});
1203+
1204+
const replayer: ReplayerHandler = {
1205+
mirror,
1206+
applyCanvas: () => {},
1207+
applyInput: () => {},
1208+
applyScroll: () => {},
1209+
applyStyleSheetMutation: () => {},
1210+
};
1211+
1212+
// Run diff
1213+
diff(oldHead, newHead, replayer);
1214+
1215+
// Extract final IDs from real DOM
1216+
const finalIds = Array.from(oldHead.childNodes)
1217+
.filter((n): n is Element => n.nodeType === Node.ELEMENT_NODE)
1218+
.map((el) => el.getAttribute('data-test-id') || el.getAttribute('id'));
1219+
1220+
// Assert the real DOM now matches the RR DOM list
1221+
expect(finalIds).toEqual(oldList.map(n => String(n.id)));
1222+
});
11591223
});
11601224

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

0 commit comments

Comments
 (0)