Skip to content

Fix edge case where oldChild can become detach causing the recursive … #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: pendo-main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/new-plants-exercise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"rrdom": patch
---

Fix early termination of recursive loop in diffChilren when a node mismatch is found
19 changes: 19 additions & 0 deletions packages/rrdom/src/diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,26 @@ function diffChildren(
let oldChild = oldTree.firstChild;
let newChild = newTree.firstChild;
while (oldChild !== null && newChild !== null) {
// For mismatched node types (e.g., Element vs Text), we create a new node and insert it before the old one.
// This preserves the oldChild reference needed for the recursive diff loop, which would break if we replaced the node directly.
const isMismatch = !sameNodeType(oldChild, newChild);

if (isMismatch) {
const newNode = createOrGetNode(newChild, replayer.mirror, rrnodeMirror);

try {
handleInsertBefore(oldTree, newNode, oldChild);
} catch (e) {
console.warn(e);
}

diff(newNode, newChild, replayer, rrnodeMirror);
newChild = newChild.nextSibling;
continue;
}

diff(oldChild, newChild, replayer, rrnodeMirror);

oldChild = oldChild.nextSibling;
newChild = newChild.nextSibling;
}
Expand Down
64 changes: 64 additions & 0 deletions packages/rrdom/test/diff.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
Mirror as RRNodeMirror,
RRDocument,
RRMediaElement,
RRElement,
printRRDom,
} from '../src';
import {
Expand Down Expand Up @@ -1183,6 +1184,69 @@ describe('diff algorithm for rrdom', () => {
expect(node.childNodes[0].childNodes.length).toEqual(1);
expect(mirror.getId(node.childNodes[0].childNodes[0])).toEqual(2);
});

it('should maintain correct order when duplicate node is found causing nextSibling traversal desync', () => {
const oldList = [
{ tagName: 'STYLE', id: 1 },
{ tagName: 'STYLE', id: 2 },
{ tagName: 'BASE', id: 3 },
{ tagName: 'META', id: 4 },
{ tagName: 'META', id: 5 },
{ tagName: 'META', id: 6 },
{ tagName: 'TITLE', id: 7 },
{ tagName: 'STYLE', id: 8 },
{ tagName: 'STYLE', id: 9 },
{ tagName: 'LINK', id: 10 },
{ tagName: 'STYLE', id: 11 },
{ tagName: 'LINK', id: 12 },
{ tagName: 'NOSCRIPT', id: 13 },
];

// Duplicate STYLE#1 at index 1
const newList = [...oldList];
newList.splice(1, 0, { tagName: 'STYLE', id: 1 });

const mirror = createMirror();
const oldHead = createTree(
{ tagName: 'head', id: 0, children: oldList as unknown as ElementType[] },
undefined,
mirror,
) as Node;
expect(oldHead.childNodes.length).toBe(oldList.length);

const rrdom = new RRDocument();
const newHead = createTree(
{ tagName: 'head', id: 0, children: newList as unknown as ElementType[] },
rrdom
) as RRNode;

// Add test attributes to each node in newHead
Array.from(newHead.childNodes).forEach((node, index) => {
if (node instanceof RRElement) {
node.setAttribute('data-test-id', `${newList[index].id}`);
node.setAttribute('data-test-tag', node.tagName);
}
});

const replayer: ReplayerHandler = {
mirror,
applyCanvas: () => {},
applyInput: () => {},
applyScroll: () => {},
applyStyleSheetMutation: () => {},
};

// Run diff
diff(oldHead, newHead, replayer);

// Extract final IDs from real DOM
const finalIds = Array.from(oldHead.childNodes)
.filter((n): n is Element => n.nodeType === Node.ELEMENT_NODE)
.map((el) => el.getAttribute('data-test-id') || el.getAttribute('id'));

// Assert the real DOM now matches the RR DOM list
expect(finalIds).toEqual(oldList.map(n => String(n.id)));
});
});

describe('diff shadow dom', () => {
Expand Down
Loading