From 1b0d089d197f13902b6fd40ca211a8598ba8cbf3 Mon Sep 17 00:00:00 2001
From: Evan Almloff <evanalmloff@gmail.com>
Date: Wed, 31 Jul 2024 00:16:42 +0200
Subject: [PATCH] Fix list diffing with root dynamic nodes (#2749)

---
 packages/core/src/diff/node.rs           | 26 ++++++--
 packages/core/tests/diff_unkeyed_list.rs | 81 ++++++++++++++++++++++++
 2 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/packages/core/src/diff/node.rs b/packages/core/src/diff/node.rs
index f68cff1aa..06925f5a8 100644
--- a/packages/core/src/diff/node.rs
+++ b/packages/core/src/diff/node.rs
@@ -154,9 +154,11 @@ impl VNode {
 
     pub(crate) fn find_first_element(&self, dom: &VirtualDom) -> ElementId {
         let mount = &dom.mounts[self.mount.get().0];
-        match self.get_dynamic_root_node_and_id(0) {
+        let first = match self.get_dynamic_root_node_and_id(0) {
             // This node is static, just get the root id
-            None | Some((_, Placeholder(_) | Text(_))) => mount.root_ids[0],
+            None => mount.root_ids[0],
+            // If it is dynamic and shallow, grab the id from the mounted dynamic nodes
+            Some((idx, Placeholder(_) | Text(_))) => ElementId(mount.mounted_dynamic_nodes[idx]),
             // The node is a fragment, so we need to find the first element in the fragment
             Some((_, Fragment(children))) => {
                 let child = children.first().unwrap();
@@ -170,15 +172,22 @@ impl VNode {
                     .root_node()
                     .find_first_element(dom)
             }
-        }
+        };
+
+        // The first element should never be the default element id (the root element)
+        debug_assert_ne!(first, ElementId::default());
+
+        first
     }
 
     pub(crate) fn find_last_element(&self, dom: &VirtualDom) -> ElementId {
         let mount = &dom.mounts[self.mount.get().0];
         let last_root_index = self.template.roots.len() - 1;
-        match self.get_dynamic_root_node_and_id(last_root_index) {
+        let last = match self.get_dynamic_root_node_and_id(last_root_index) {
             // This node is static, just get the root id
-            None | Some((_, Placeholder(_) | Text(_))) => mount.root_ids[last_root_index],
+            None => mount.root_ids[last_root_index],
+            // If it is dynamic and shallow, grab the id from the mounted dynamic nodes
+            Some((idx, Placeholder(_) | Text(_))) => ElementId(mount.mounted_dynamic_nodes[idx]),
             // The node is a fragment, so we need to find the first element in the fragment
             Some((_, Fragment(children))) => {
                 let child = children.first().unwrap();
@@ -192,7 +201,12 @@ impl VNode {
                     .root_node()
                     .find_last_element(dom)
             }
-        }
+        };
+
+        // The last element should never be the default element id (the root element)
+        debug_assert_ne!(last, ElementId::default());
+
+        last
     }
 
     /// Diff the two text nodes
diff --git a/packages/core/tests/diff_unkeyed_list.rs b/packages/core/tests/diff_unkeyed_list.rs
index 99cf00bab..31a2ef78c 100644
--- a/packages/core/tests/diff_unkeyed_list.rs
+++ b/packages/core/tests/diff_unkeyed_list.rs
@@ -390,3 +390,84 @@ fn remove_many() {
         )
     }
 }
+
+#[test]
+fn replace_and_add_items() {
+    let mut dom = VirtualDom::new(|| {
+        let items = (0..generation()).map(|_| {
+            if generation() % 2 == 0 {
+                VNode::empty()
+            } else {
+                rsx! {
+                    li {
+                        "Fizz"
+                    }
+                }
+            }
+        });
+
+        rsx! {
+            ul {
+                {items}
+            }
+        }
+    });
+
+    // The list starts empty with a placeholder
+    {
+        let edits = dom.rebuild_to_vec().sanitize();
+        assert_eq!(
+            edits.edits,
+            [
+                LoadTemplate { name: "template", index: 0, id: ElementId(1,) },
+                AssignId { path: &[0], id: ElementId(2,) },
+                AppendChildren { id: ElementId(0), m: 1 },
+            ]
+        );
+    }
+
+    // Rerendering adds an a static template
+    {
+        dom.mark_dirty(ScopeId::APP);
+        let edits = dom.render_immediate_to_vec().sanitize();
+        assert_eq!(
+            edits.edits,
+            [
+                LoadTemplate { name: "template", index: 0, id: ElementId(3,) },
+                ReplaceWith { id: ElementId(2,), m: 1 },
+            ]
+        );
+    }
+
+    // Rerendering replaces the old node with a placeholder and adds a new placeholder
+    {
+        dom.mark_dirty(ScopeId::APP);
+        let edits = dom.render_immediate_to_vec().sanitize();
+        assert_eq!(
+            edits.edits,
+            [
+                CreatePlaceholder { id: ElementId(2,) },
+                InsertAfter { id: ElementId(3,), m: 1 },
+                CreatePlaceholder { id: ElementId(4,) },
+                ReplaceWith { id: ElementId(3,), m: 1 },
+            ]
+        );
+    }
+
+    // Rerendering replaces both placeholders with the static nodes and add a new static node
+    {
+        dom.mark_dirty(ScopeId::APP);
+        let edits = dom.render_immediate_to_vec().sanitize();
+        assert_eq!(
+            edits.edits,
+            [
+                LoadTemplate { name: "template", index: 0, id: ElementId(3,) },
+                InsertAfter { id: ElementId(2,), m: 1 },
+                LoadTemplate { name: "template", index: 0, id: ElementId(5,) },
+                ReplaceWith { id: ElementId(4,), m: 1 },
+                LoadTemplate { name: "template", index: 0, id: ElementId(4,) },
+                ReplaceWith { id: ElementId(2,), m: 1 },
+            ]
+        );
+    }
+}