Test: reproduce unnecessary remount of a retained nested child

notes/Inserters.md

@@ -96,6 +96,15 @@ An item that leaves the list is removed from the DOM and its per-item lifecycle
 ### Remove (no-op after a steal)
 If the node/group is no longer under this parent (already stolen), `removeFromDynamicList` does nothing — the new host owns it now. This is the "last write wins" safety valve.
 
+### Remove, keeping nested content (un-nesting)
+Removing old content can leave specific items in place: `removeContentMapNodesFromDom` takes a `keepItem` predicate, and passes it down through `removeFromDynamicList` → `NestedGroup.removeFromParent` into every nested item it tears down. The predicate is called with each item's `stableFirstNode` – its identity, the same key as in `contentMap` – not with an inserter object, because the same item can be represented by different inserter objects over time (e.g. fresh `SlottableChildInserter` wrappers on every emission). A kept item – a plain node, or a nested dynamic inserter with its whole span and live subscription – stays in the parent's DOM where it was, while the nested group around it is torn down. It never loses its parent, so it is not unmounted. Like moving a plain element out of a wrapper that's being removed, within the same parent.
+
+The caller must then promptly place every kept item where it belongs – otherwise it would be left in the DOM, untracked. Callers:
+
+- `child <--` taking over a span that contains its new node, even nested (§7).
+- `children <--` reconciliation removing a leaving item that contains nodes / inserters that the same emission keeps (`InserterMoveSpec` §4e).
+- `children.command <--` `ReplaceAll(newNodes, minimizeDiff = true)`, keeping the nodes that stay in the list. It also leaves kept nodes that are already in the right place untouched in the DOM. With `minimizeDiff = false`, `ReplaceAll` skips this work for performance: it removes all current nodes, then appends `newNodes`, re-mounting any overlap.
+
 ### External mutation
 Laminar tolerates a user/third-party removing an inserter's node from the DOM directly: the next reconciliation walks the live span and simply doesn't find it, correcting the count (`InserterExternalMutationSpec`; `ChildrenInserter.updateChildren` count-correction, referencing issue #120). External _insertion_ into a bracketed span (between our sentinels) is reported as an error on teardown/removeAll paths (`removeContentMapNodesFromDom` with a trailing sentinel), because we can't tell an intruder from our own content otherwise. External insertion into an unbracketed (`child <--`) span is silently treated as "the next sibling after our span" — we stop the walk there.
 
@@ -149,7 +158,8 @@ If the previous host genuinely _removed_ the group (set `nestedGroupOpt = js.und
 - **Per-item lifecycle is preserved across a transfer** — the owner is transferred, not rebuilt, so per-item `onMount`/`onUnmount` and internal subscriptions stay live (`InserterMoveSpec` "add-first / steal keeps the item's per-item lifecycle intact").
 - **Ordering is empirical but pinned.** Some teardown/swap orders are not obvious and are deliberately encoded by tests (see `notes/Testing.md`):
   - `child <--` self-replace swaps **unmount-old-then-mount-new**.
-  - A takeover of a _foreign_ span mounts-new-then-unmounts-old.
+  - A `child <--` takeover of a _foreign_ span also unmounts the old content before mounting the new node. The node it keeps (§7) is neither unmounted nor re-mounted.
+  - `children.command <--` `ReplaceAll` unmounts the leaving nodes before mounting the new ones. With `minimizeDiff = false`, all old nodes are considered "leaving", including those that are then re-inserted.
   - `children <--` teardown walks `contentMap` in **insertion order**, not current DOM order (several `#Note` comments; e.g. `InserterMoveSpec:890`, `InserterExternalMutationSpec:148`). This is teardown only — relocation uses DOM order (§5).
   - `Replace` (command) unmounts the old node then mounts the new (`InserterMoveSpec:1718`).
 
@@ -166,7 +176,7 @@ If the previous host genuinely _removed_ the group (set `nestedGroupOpt = js.und
 - Switching TO `child <--` / `text <--` clears prior multi-node content down to (at most) the one node being kept, and drops the trailing sentinel.
 - Switching TO `children.command <--` clears any content left by a _non-command_ inserter (commands can't patch foreign content to a target state), but **preserves** content it built itself across a mere remount, and preserves content built by a _different_ command inserter (`InserterTakeoverSpec` "children.command → children.command (different inserter) keeps the previous content").
 - A takeover that tears down a span reports an externally-inserted intruder (§3).
-- A takeover does **not** blindly tear down everything: `children <--` (a,b) → `child <-- b` keeps b mounted and unmounts only a (`InserterTakeoverSpec`).
+- A takeover does **not** blindly tear down everything: `children <--` (a,b) → `child <-- b` keeps b mounted and unmounts only a (`InserterTakeoverSpec`). This holds even if b is nested inside one of the list's dynamic items, at any depth: those items are torn down around b (§3 "un-nesting").
 
 ### The `setNextInserterType` invariant
 Whenever there is no trailing sentinel, `contentMap` holds **at most one** node. The guard in `setNextInserterType` throws if we ever switch to a trailing-sentinel type while lacking a trailing sentinel but already holding >1 content node (we wouldn't know where the pre-existing content ends). This is structurally unreachable via the public API; `InserterInvariantSpec` pins both the near-miss safety and the white-box guard firing.
@@ -226,10 +236,12 @@ Distinct from the last-write-wins contest above: when a slot could come from an
 
 ## 10. Known deviations from the ideal
 
-### Provably unachievable (not bugs we can fix)
+### Accepted limitations
 
 - **Issue #163 — moving an element between two sibling `child <--` bindings re-mounts in one direction only.** When one element is shown via one of two independent `child <--` bindings toggled by a single signal, whether the toggle re-mounts is _order-dependent_: the binding that GAINS the element must fire before the one that LOSES it for the transfer to be seamless. When the losing binding fires first, the element is detached to `None` (unmount) before re-attachment (mount). This is inherent to synchronous propagation order; a `delaySync` workaround only fixes one direction. Characterized (not "fixed") in `InserterMoveSpec` "CHARACTERIZATION (issue #163)". The `probe(addFirst=false)` remove-first re-mount in the REFERENCE test is the same underlying limitation.
 
+- **Re-wrapping a nested node into a NEW nested inserter can re-mount it.** `children <--` (nested `child <-- b`, a) → (a, NEW nested `child <-- b`): reconciliation removes the leaving item before it reaches the new one, and nothing is known about the new inserter's content until it subscribes, so b can't be kept (§3 "un-nesting"), and is unmounted, then mounted again. If the new inserter comes BEFORE the leaving item in the list, it takes b before the old item is removed, which is seamless. Fixing the general case would require deferring the teardown of leaving items until the end of reconciliation. Characterized in `InserterMoveSpec` "CHARACTERIZATION: re-wrapping a nested node…".
+
 ---
 
 ## 11. Suspicious / uncertain intent (flagged for review)

notes/Testing.md

@@ -41,7 +41,7 @@ Assertion messages should include enough details to pinpoint and diagnose the is
 
 ## Ordering is empirical
 
-Exact teardown / swap order is behaviour worth pinning but not always obvious — e.g. `child <--` swaps **unmount-old-then-mount-new** (self-replace) but a takeover of a foreign span is **mount-new-then-unmount-old**; `children <--` teardown walks the contentMap in **insertion order**, not current DOM order. Write your best guess, run the spec, and encode the order the failure reports, with a one-line // #Note: comment explaining it.
+Exact teardown / swap order is behaviour worth pinning but not always obvious — e.g. `child <--` swaps **unmount-old-then-mount-new** (both self-replace and a takeover of a foreign span); `children <--` teardown walks the contentMap in **insertion order**, not current DOM order. Write your best guess, run the spec, and encode the order the failure reports, with a one-line // #Note: comment explaining it.
 
 ## Misc style
 

src/main/scala/com/raquo/laminar/inserters/ChildInserter.scala

@@ -52,13 +52,13 @@ object ChildInserter {
         .filter(_.ref == ctx.sentinelNode.ref.nextSibling) // Assert that the prev child node was not moved. Note: nextSibling could be null
         .fold {
           // Anything that does exist in the DOM, is not ours. Clean it up first.
-          // It's possible that previous node(s) already contained newChildNode,
-          // so make sure to avoid unmounting it.
+          // The previous content may already contain newChildNode (even nested inside
+          // one of its dynamic items) – if so, it's left in place, not unmounted.
           ctx.clearPreviousInserterContent(
             replaceContentMapWithSingleNode = newChildNode,
             nextInserterType = InserterType.ChildType
           )
-          // Render the new child
+          // Render the new child (or move it into place, if it was retained above)
           DomApi.insertChildAfter(
             parent = ctx.currentParentNode,
             newChild = newChildNode,

src/main/scala/com/raquo/laminar/inserters/ChildrenCommandInserter.scala

@@ -1,6 +1,7 @@
 package com.raquo.laminar.inserters
 
 import com.raquo.airstream.core.EventStream
+import com.raquo.ew.JsSet
 import com.raquo.laminar.domapi.DomApi
 import com.raquo.laminar.modifiers.RenderableNode
 import com.raquo.laminar.nodes.{ChildNode, CommentNode}
@@ -158,25 +159,71 @@ object ChildrenCommandInserter {
         }
 
       case CollectionCommand.RemoveAll =>
-        ctx.removeContentMapNodesFromDom(keepNodeIfPresent = js.undefined)
+        ctx.removeContentMapNodesFromDom(keepItem = InsertContext.keepNoItems)
         ctx.contentMap.clear()
 
-      case CollectionCommand.ReplaceAll(newNodes) =>
-        ctx.removeContentMapNodesFromDom(keepNodeIfPresent = js.undefined)
-        ctx.contentMap.clear()
-        val trailingSentinelRef = ctx.trailingSentinelNodeOpt.get.ref
-        newNodes.foreach { node =>
-          if (
-            DomApi.insertChildBefore(
-              parent = ctx.currentParentNode,
-              newChild = node,
-              referenceChildRef = trailingSentinelRef,
-              slotName = ctx.currentSlotName
-            )
-          ) {
-            ctx.contentMap.set(node.ref, node)
-          }
+      case CollectionCommand.ReplaceAll(newNodes, minimizeDiff) =>
+        if (minimizeDiff) {
+          replaceAllMinimizingDiff(newNodes, ctx)
+        } else {
+          replaceAllNaively(newNodes, ctx)
         }
     }
   }
+
+  private def replaceAllNaively(
+    newNodes: collection.immutable.Seq[ChildNode.Base],
+    ctx: InsertContext
+  ): Unit = {
+    ctx.removeContentMapNodesFromDom(keepItem = InsertContext.keepNoItems)
+    ctx.contentMap.clear()
+    val trailingSentinelRef = ctx.trailingSentinelNodeOpt.get.ref
+    newNodes.foreach { node =>
+      if (
+        DomApi.insertChildBefore(
+          parent = ctx.currentParentNode,
+          newChild = node,
+          referenceChildRef = trailingSentinelRef,
+          slotName = ctx.currentSlotName
+        )
+      ) {
+        ctx.contentMap.set(node.ref, node)
+      }
+    }
+  }
+
+  /** Keep the nodes that are staying in the list, so that they aren't re-mounted.
+    * The old nodes that are leaving are removed (and unmounted) before the new ones mount.
+    */
+  private def replaceAllMinimizingDiff(
+    newNodes: collection.immutable.Seq[ChildNode.Base],
+    ctx: InsertContext
+  ): Unit = {
+    val newNodeRefs = JsSet.empty[dom.Node]
+    newNodes.foreach(node => newNodeRefs.add(node.ref))
+    ctx.removeContentMapNodesFromDom(keepItem = newNodeRefs.has)
+    ctx.contentMap.clear()
+    // Place the nodes in order. Kept nodes that are already in the right place stay put.
+    var afterRef: dom.Node = ctx.sentinelNode.ref
+    newNodes.foreach { node =>
+      val isPlaced = if (afterRef.nextSibling eq node.ref) {
+        // Re-affirm the slot, just like inserting the node would, so that this reconcile
+        // wins over a manual `slot` override, whether or not the node had to move
+        // (consistent last-write-wins semantics).
+        node.applySlot(ctx.currentParentNode, ctx.currentSlotName)
+        true
+      } else {
+        DomApi.insertChildAfter(
+          parent = ctx.currentParentNode,
+          newChild = node,
+          referenceChildRef = afterRef,
+          slotName = ctx.currentSlotName
+        )
+      }
+      if (isPlaced) {
+        ctx.contentMap.set(node.ref, node)
+        afterRef = node.ref
+      }
+    }
+  }
 }

src/main/scala/com/raquo/laminar/inserters/ChildrenInserter.scala

@@ -100,8 +100,14 @@ object ChildrenInserter {
         prevItemRef = prevItemRef.nextSibling
       }) { prevInserter =>
         val nextPrevItemRef = prevInserter.lastNode.nextSibling
+        // Keep the items nested inside prevInserter that the new list re-uses. They're left
+        // just before the cursor, and are placed on their turn (prevInserter itself is never
+        // in nextInsertersMap here).
         // @Note: DOM update
-        prevInserter.removeFromDynamicList(listParentNode)
+        prevInserter.removeFromDynamicList(
+          parent = listParentNode,
+          keepNestedItem = nextInsertersMap.has
+        )
         prevItemRef = nextPrevItemRef
         currentItemCount -= 1
       }

src/main/scala/com/raquo/laminar/inserters/CollectionCommand.scala

@@ -52,11 +52,27 @@ object CollectionCommand {
     @inline override def map[A](project: Nothing => A): CollectionCommand[A] = this
   }
 
-  /** Replace the entire contents of the collection with `newItems` */
-  case class ReplaceAll[+Item](newItems: collection.immutable.Seq[Item]) extends CollectionCommand[Item] {
+  /** Replace the entire contents of the collection with `newItems`.
+    *
+    * @param minimizeDiff
+    *   - `false`: all DOM nodes managed by this `children.command <--` are removed from
+    *     the DOM first, then all the `newItems` are added. This is the fastest option
+    *     when you know that `newItems` does not contain existing nodes (e.g. switching
+    *     to an unrelated list).
+    *   - `true`: `children.command` will run a smarter diffing algorithm that will avoid
+    *     unmounting existing nodes if they are present in `newItems`, preventing a
+    *     re-mount. This is more similar to what `children <--` does (but more basic),
+    *     and involves more overhead, especially for very large lists, but can be
+    *     desirable if you don't want mount/unmount hooks to run unnecessarily in
+    *     this transition.
+    */
+  case class ReplaceAll[+Item](
+    newItems: collection.immutable.Seq[Item],
+    minimizeDiff: Boolean
+  ) extends CollectionCommand[Item] {
 
     @inline override def map[A](project: Item => A): ReplaceAll[A] = {
-      ReplaceAll(newItems.map(project))
+      ReplaceAll(newItems.map(project), minimizeDiff)
     }
   }
 
@@ -105,7 +121,7 @@ object CollectionCommand {
       case RemoveAll =>
         Vector.empty
 
-      case ReplaceAll(newItems) =>
+      case ReplaceAll(newItems, _) =>
         newItems.toVector
     }
   }

src/main/scala/com/raquo/laminar/inserters/InsertContext.scala

@@ -182,14 +182,21 @@ final class InsertContext(
     *
     * @param replaceContentMapWithSingleNode
     *            If specified, will ensure that the resulting contentMap has this node.
-    *            If specified, will prevent this node from being removed from the DOM, but will NOT add it to the DOM.
+    *            If specified, will prevent this node from being removed from the DOM
+    *            (even if it's nested in a dynamic item), but will NOT add it to the DOM.
+    * @param keepItem Items to leave in the DOM – see [[removeContentMapNodesFromDom]].
     */
   def clearPreviousInserterContent(
     replaceContentMapWithSingleNode: js.UndefOr[ChildNode.Base],
-    nextInserterType: js.UndefOr[InserterType]
+    nextInserterType: js.UndefOr[InserterType],
+    keepItem: dom.Node => Boolean = InsertContext.keepNoItems
   ): Unit = {
     // Remove from the DOM any old nodes that shouldn't be retained.
-    removeContentMapNodesFromDom(keepNodeIfPresent = replaceContentMapWithSingleNode)
+    removeContentMapNodesFromDom(
+      keepItem = replaceContentMapWithSingleNode.fold(keepItem) { singleNode =>
+        ref => (ref eq singleNode.ref) || keepItem(ref)
+      }
+    )
 
     // Update the context to match the DOM state
     contentMap.clear()
@@ -213,7 +220,20 @@ final class InsertContext(
   }
 
   /** Walk this context's span forward from [[sentinelNode]], removing every tracked
-    * ([[contentMap]]) node from the DOM except `keepNodeIfPresent`.
+    * ([[contentMap]]) item from the DOM except those matching `keepItem`.
+    *
+    * @param keepItem Called with each item's `stableFirstNode`, the item's identity (the same
+    *                 key as in [[contentMap]]): the node itself for a plain or slotted node,
+    *                 or the leading sentinel for a dynamic inserter.
+    *
+    * `keepItem` is also applied recursively inside the removed dynamic items: a kept item is
+    * left in place under the same parent (a dynamic inserter with its whole span and live
+    * subscription), even if the nested group that contained it is torn down. It never loses
+    * its parent, so it's not unmounted. This lets the caller re-insert it seamlessly.
+    *
+    * #Warning: The caller must promptly place every kept item into the DOM where it belongs,
+    *  and track it there. A kept item is left where it was, untracked by this context – and
+    *  if it came from inside a removed nested group, it's untracked by anyone.
     *
     * Where the walk stops depends on whether we have a [[_trailingSentinelNodeOpt]]:
     *  - With one (a `children <--` or `children.command <--` span), it marks the definite
@@ -231,7 +251,7 @@ final class InsertContext(
     * #Note: this does NOT update [[contentMap]] to match the new DOM.
     */
   def removeContentMapNodesFromDom(
-    keepNodeIfPresent: js.UndefOr[ChildNode.Base]
+    keepItem: dom.Node => Boolean
   ): Unit = {
     val hasTrailingSentinel = _trailingSentinelNodeOpt.nonEmpty
     var maybeRef = sentinelNode.ref.nextSibling
@@ -241,10 +261,10 @@ final class InsertContext(
       if (_trailingSentinelNodeOpt.exists(_.ref == childRef)) {
         // Reached the end of our span. Stop.
         continue = false
-      } else if (keepNodeIfPresent.exists(_.ref == childRef)) {
-        // The node we're keeping. Leave it in place, step over it (a plain content node, so
-        // a single node) and keep clearing whatever old content sits after it.
-        maybeRef = childRef.nextSibling
+      } else if (keepItem(childRef)) {
+        // An item we're keeping. Leave it in place, step over it (in one go, if it's a nested
+        // inserter's span), and keep clearing whatever old content sits after it.
+        maybeRef = contentMap.get(childRef).fold(childRef.nextSibling)(_.lastNode.nextSibling)
       } else {
         contentMap.get(childRef).fold {
           if (hasTrailingSentinel) {
@@ -265,7 +285,7 @@ final class InsertContext(
           // so a multi-node span (e.g. a nested group) is stepped over in one go.
           val nextRef = inserter.lastNode.nextSibling
           // @Note: DOM update
-          inserter.removeFromDynamicList(currentParentNode)
+          inserter.removeFromDynamicList(currentParentNode, keepItem)
           maybeRef = nextRef
         }
       }
@@ -324,6 +344,8 @@ final class InsertContext(
 
 object InsertContext {
 
+  private[laminar] val keepNoItems: dom.Node => Boolean = _ => false
+
   /** Reserve the spot for when we actually insert real nodes later */
   def reserveSpotContext(
     parentNode: ReactiveElement.Base,