Skip to content

Commit 466187e

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

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: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -527,7 +527,23 @@ function diffChildren(
527527
let oldChild = oldTree.firstChild;
528528
let newChild = newTree.firstChild;
529529
while (oldChild !== null && newChild !== null) {
530+
const isMismatch = !nodeMatching(oldChild, newChild, replayer.mirror, rrnodeMirror);
531+
532+
// If oldChild and newChild don't match due to changes or misalignment, a new DOM node is created for newChild and inserted before oldChild.
533+
if (isMismatch) {
534+
const newNode = createOrGetNode(newChild, replayer.mirror, rrnodeMirror);
535+
536+
// Insert the new node before the current oldChild to preserve correct order.
537+
oldTree.insertBefore(newNode, oldChild);
538+
539+
diff(newNode, newChild, replayer, rrnodeMirror);
540+
541+
newChild = newChild.nextSibling;
542+
continue;
543+
}
544+
530545
diff(oldChild, newChild, replayer, rrnodeMirror);
546+
531547
oldChild = oldChild.nextSibling;
532548
newChild = newChild.nextSibling;
533549
}

packages/rrdom/test/diff.test.ts

Lines changed: 67 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,72 @@ 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 },
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 },
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+
});
1250+
1251+
1252+
11861253
});
11871254

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

0 commit comments

Comments
 (0)