Fix `nextSibling` bugs and refactor `updateNodes()` and its fuzzer #3059

kfule · 2025-11-18T12:43:45Z

This PR fixes nextSibling-related bugs in updateNodes() and updates updateNodesFuzzer to better cover these cases. It also refactors updateNodes() to simplify the logic and improve maintainability.

Description

This PR makes the following changes related to updateNodes().

Fix for bugs related to nextSibling
Fix for updateNodesFuzzer and addition of test cases
Refactoring including adjusting variable declaration positions and names
- This change reduces the bundle size to 8,899 bytes gzipped.

Motivation and Context

This PR fixes the following issues related to the way nextSibling is set, which I noticed while reading the updateNodes() code. (The cases below are simplified versions of those found by the fuzzer.)
Additionally, since updateNodesFuzzer also appeared to have problems, I fixed those issues as well and added additional test cases.

case 1 (flems)

const root = document.body

const e = (k) => m(k, {key: k})       // element vnode
const p = (k) => m(k + "p", {key: k}) // element vnode (another tag)

const o1 = [e("k1"),e("k2"),e("k3"),e("k4")]
const v1 = [        p("k2"),e("k4"),e("k3")]

// create
m.render(root, v1)
console.log(Array.from(root.childNodes).map(n => n.nodeName)) // [ 'K2P', 'K4', 'K3' ] correct

// update
m.render(root, [])
m.render(root, o1)
m.render(root, v1)
console.log(Array.from(root.childNodes).map(n => n.nodeName)) // [ 'K4', 'K3', 'K2P' ] incorrect

case 2 (flems)

const root = document.body

const e = (k) => m(k, {key: k})   // element vnode (with dom)
const f = (k) => m("[", {key: k}) // fragment vnode (without dom)

const o1 = [f("k1"),       e("k2")]
const v1 = [e("k1"),e("a"),f("k2")]

m.render(root, o1)
m.render(root, v1) // Node.insertBefore: Child to insert before is not a child of this node

case 3 (flems)

const root = document.body

const e = (k) => m(k, {key: k})   // element vnode (with dom)
const f = (k) => m("[", {key: k}) // fragment vnode (without dom)

const o1 = [f("k1"),f("k2"),e("k3")]
const v1 = [e("k1"),f("k3"),e("k2")]

m.render(root, o1)
m.render(root, v1) // Node.insertBefore: Child to insert before is not a child of this node

Since these cases occur only with keyed vnodes and involve very rare patterns - such as fragments that do not correspond to DOM nodes or tag changes that cause node deletions and insertions - the bugs likely did not surface very often.

How Has This Been Tested?

npm run test
I have also confirmed the above flems case fix.

As for the fuzzer, I’ve confirmed that it is tested with different patterns, and that even after increasing the number of runs, all tests still pass.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
I have added tests to cover my changes.
All new and existing tests passed.
My change requires a documentation update, and I've opened a pull request to update it already:
I have read https://mithril.js.org/contributing.html.

dead-claudia

In addition to the other comments, can you also put in some deterministic tests to ensure we don't regress in at least the cases you listed?

If they break, I'd rather us see it right away. I don't want to risk the bug unexpectedly reappearing making me roll for initiative after a bad release. 😅 (This may be before your time here, but we got burned hard in a v0.2 release thanks to missing test coverage.)

dead-claudia · 2025-11-20T10:32:30Z

render/render.js

+					var oldIndices = new Array(end - start + 1)
+					for (var i = 0; i < oldIndices.length; i++) oldIndices[i] = -1


Can we do this instead? Or is it slower?

Suggested change

var oldIndices = new Array(end - start + 1)

for (var i = 0; i < oldIndices.length; i++) oldIndices[i] = -1

var oldIndices = new Int32Array(end - start + 1).fill(-1)

It seems like a good idea.
Since this code path isn't tested with npm run perf, I'll take a quick look using dbmonster or some other benchmark.

I tried running dbmonster, but I couldn’t tell if there was any noticeable difference. So I changed it to new Array(end - start + 1).fill(-1), which reduces the bundle size.

dead-claudia · 2025-11-20T10:35:09Z

render/render.js

 		var il = lisTemp.length = a.length
 		for (var i = 0; i < il; i++) lisTemp[i] = a[i]
-		for (var i = 0; i < il; ++i) {
+		for (var i = 0; i < il; i++) {


Can you revert this change? I'd rather keep the Git blame cleaner

reverted it

dead-claudia · 2025-11-20T10:41:46Z

render/tests/test-updateNodesFuzzer.js

+				// FIXME: This does not take into account the "swaps and list reversals" heuristic in updateNodes().
+				// if (root.appendChild.callCount + root.insertBefore.callCount !== test.expected.creations + test.expected.moves) console.log(test, {aC: root.appendChild.callCount, iB: root.insertBefore.callCount}, [].map.call(root.childNodes, function(n){return n.nodeName.toLowerCase()}))


Could you elaborate on what the specific issue is with this?

This feels a bit too generic. Given the nature of the issue it's checking, and the fact it's just auditing for perf opportunities, it might take years to get fixed, assuming it gets fixed at all.

You're absolutely right. I'd like to leave a more detailed comment.

dead-claudia · 2025-11-20T10:42:06Z

render/tests/test-updateNodesFuzzer.js

+				// ditto
+				// o(root.appendChild.callCount + root.insertBefore.callCount).equals(test.expected.creations + test.expected.moves)("moves")


See above comment

kfule · 2025-11-20T14:21:24Z

@dead-claudia Thank you for your review comments.
I’ve made the corresponding changes for each of your points. I’ve also added deterministic tests.
Could you please take another look?

Because the DOM has not yet been arranged in `vnodes` order, `nextSibling` can point to the wrong DOM node. Since `insertBefore` uses `nextSibling`, nodes may be inserted in the wrong position when deletions and insertions occur. This commit fixes that by performing updates based on `old`, which reflects the actual DOM order.

Previously, `getNextSibling()` scanned all the way to the end of the array. If the array contained vnodes that did not correspond to DOM nodes, or if newly inserted vnodes were present within the scan range, the scan could overrun and capture an incorrect nextSibling. This commit fixes that by limiting the scan range appropriately.

…ses that do not account for the heuristics in `updateNodes()`. Previously, due to a bug that caused the same data to be reused, the fuzzer was not exercising the code adequately. It also included test cases that ignored the heuristics in `updateNodes()`, which had a very low probability of being triggered but, once they were, produced a large burst of false-positive test failures from that same data. The commented-out sections are test cases for the number of append and insert operations related to the longest increasing subsequence algorithm, so it should be safe to leave them commented out for now.

…ons and insertions), as well as cases with fragments that do not correspond to DOM nodes.

…ock. By specializing this logic for the unkeyed path, this change reduces the number of conditional branches and also shrinks the bundle size.

…optimize `getKeyMap()`. In v1, `getKeyMap()` was called inside the loop, but in v2 this is no longer necessary. Since key normalization has been improved in v2, much of `getKeyMap()`’s logic has become redundant, so this change inlines and simplifies it to reduce overhead.

These changes aim to improve code readability and slightly reduce the bundle size.

kfule requested a review from a team as a code owner November 18, 2025 12:43

dead-claudia requested changes Nov 20, 2025

View reviewed changes

kfule force-pushed the fix-keyed-update branch from 7179aca to e1ae772 Compare November 20, 2025 14:13

kfule requested a review from dead-claudia November 20, 2025 14:21

kfule added 9 commits November 22, 2025 00:19

updateNodesFuzzer: Added test cases for tag changes (involving deleti…

52830d2

…ons and insertions), as well as cases with fragments that do not correspond to DOM nodes.

[refactor]updateNodes: Move index increment logic into the unkeyed bl…

d346272

…ock. By specializing this logic for the unkeyed path, this change reduces the number of conditional branches and also shrinks the bundle size.

[refactor]updateNodes: Adjust variable declaration positions and names.

2ea5beb

These changes aim to improve code readability and slightly reduce the bundle size.

updateNodesFuzzer: Add detailed comments for FIXME

f1b8d72

Add deterministic tests for nextSibling handling

153bb7a

dead-claudia force-pushed the fix-keyed-update branch from e1ae772 to 153bb7a Compare November 22, 2025 08:19

dead-claudia approved these changes Nov 22, 2025

View reviewed changes

dead-claudia merged commit c50e6be into MithrilJS:main Nov 22, 2025
7 checks passed

JAForbes mentioned this pull request Nov 22, 2025

Release - v2.3.9 #3058

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix `nextSibling` bugs and refactor `updateNodes()` and its fuzzer #3059

Fix `nextSibling` bugs and refactor `updateNodes()` and its fuzzer #3059

Uh oh!

kfule commented Nov 18, 2025 •

edited

Loading

Uh oh!

dead-claudia left a comment

Uh oh!

dead-claudia Nov 20, 2025

Uh oh!

kfule Nov 20, 2025

Uh oh!

kfule Nov 20, 2025

Uh oh!

dead-claudia Nov 20, 2025

Uh oh!

kfule Nov 20, 2025

Uh oh!

dead-claudia Nov 20, 2025

Uh oh!

kfule Nov 20, 2025

Uh oh!

dead-claudia Nov 20, 2025

Uh oh!

kfule commented Nov 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		var oldIndices = new Array(end - start + 1)
		for (var i = 0; i < oldIndices.length; i++) oldIndices[i] = -1

	var oldIndices = new Array(end - start + 1)
	for (var i = 0; i < oldIndices.length; i++) oldIndices[i] = -1
	var oldIndices = new Int32Array(end - start + 1).fill(-1)

		// FIXME: This does not take into account the "swaps and list reversals" heuristic in updateNodes().
		// if (root.appendChild.callCount + root.insertBefore.callCount !== test.expected.creations + test.expected.moves) console.log(test, {aC: root.appendChild.callCount, iB: root.insertBefore.callCount}, [].map.call(root.childNodes, function(n){return n.nodeName.toLowerCase()}))

		// ditto
		// o(root.appendChild.callCount + root.insertBefore.callCount).equals(test.expected.creations + test.expected.moves)("moves")

Uh oh!

Fix nextSibling bugs and refactor updateNodes() and its fuzzer #3059

Fix nextSibling bugs and refactor updateNodes() and its fuzzer #3059

Uh oh!

Conversation

kfule commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist

Uh oh!

dead-claudia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfule commented Nov 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix `nextSibling` bugs and refactor `updateNodes()` and its fuzzer #3059

Fix `nextSibling` bugs and refactor `updateNodes()` and its fuzzer #3059

kfule commented Nov 18, 2025 •

edited

Loading