Skip to content

Commit 4f74fe6

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

File tree

3 files changed

+95
-0
lines changed

3 files changed

+95
-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: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,24 @@ 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+
oldTree.insertBefore(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);
529547
oldChild = oldChild.nextSibling;
530548
newChild = newChild.nextSibling;

packages/rrdom/test/diff.test.ts

Lines changed: 72 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,77 @@ 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+
{
1185+
tagName: 'head',
1186+
id: 0,
1187+
children: oldList as unknown as ElementType[],
1188+
},
1189+
undefined,
1190+
mirror,
1191+
) as Node;
1192+
expect(oldHead.childNodes.length).toBe(oldList.length);
1193+
1194+
const rrdom = new RRDocument();
1195+
const newHead = createTree(
1196+
{
1197+
tagName: 'head',
1198+
id: 0,
1199+
children: newList as unknown as ElementType[],
1200+
},
1201+
rrdom,
1202+
) as RRNode;
1203+
1204+
// Add test attributes to each node in newHead
1205+
Array.from(newHead.childNodes).forEach((node, index) => {
1206+
if (node instanceof RRElement) {
1207+
node.setAttribute('data-test-id', `${newList[index].id}`);
1208+
node.setAttribute('data-test-tag', node.tagName);
1209+
}
1210+
});
1211+
1212+
const replayer: ReplayerHandler = {
1213+
mirror,
1214+
applyCanvas: () => {},
1215+
applyInput: () => {},
1216+
applyScroll: () => {},
1217+
applyStyleSheetMutation: () => {},
1218+
};
1219+
1220+
// Run diff
1221+
diff(oldHead, newHead, replayer);
1222+
1223+
// Extract final IDs from real DOM
1224+
const finalIds = Array.from(oldHead.childNodes)
1225+
.filter((n): n is Element => n.nodeType === Node.ELEMENT_NODE)
1226+
.map((el) => el.getAttribute('data-test-id') || el.getAttribute('id'));
1227+
1228+
// Assert the real DOM now matches the RR DOM list
1229+
expect(finalIds).toEqual(oldList.map((n) => String(n.id)));
1230+
});
11591231
});
11601232

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

0 commit comments

Comments
 (0)