Fix replaceInstances messing up GetChildren ordering (#1265)

Fixes #1257. replaceInstances now preserves GetChildren ordering.

I also had the unit tests unreliably say that Packages wasn't a member
of ReplicatedStorage so I added WaitForChild and tests run 100% for me
now.

plugin/run-tests.server.lua

@@ -1,8 +1,8 @@
 local ReplicatedStorage = game:GetService("ReplicatedStorage")
 
-local TestEZ = require(ReplicatedStorage.Packages:WaitForChild("TestEZ", 10))
+local TestEZ = require(ReplicatedStorage:WaitForChild("Packages", 10):WaitForChild("TestEZ", 10))
 
-local Rojo = ReplicatedStorage.Rojo
+local Rojo = ReplicatedStorage:WaitForChild("Rojo", 10)
 
 local Settings = require(Rojo.Plugin.Settings)
 Settings:set("logLevel", "Trace")

plugin/src/ServeSession.lua

@@ -18,6 +18,7 @@ local PatchSet = require(script.Parent.PatchSet)
 local Reconciler = require(script.Parent.Reconciler)
 local strict = require(script.Parent.strict)
 local Settings = require(script.Parent.Settings)
+local orderSwaps = require(script.Parent.orderSwaps)
 
 local Status = strict("Session.Status", {
 	NotStarted = "NotStarted",
@@ -320,6 +321,14 @@ function ServeSession:__replaceInstances(idList)
 		return false
 	end
 
+	-- Roblox appends to GetChildren() on every reparent, so the order in which
+	-- we re-parent replacements determines their final sibling order.
+	-- We process ancestors before descendants (so each replacement's
+	-- parent already exists when we re-parent it) and siblings in their original
+	-- GetChildren() order. Because the loop below moves the old instance's
+	-- children into the replacement *before* re-parenting the replacement, this
+	-- rebuilds GetChildren() exactly as it was before the swap.
+	local swaps = {}
 	for id, replacement in replacements do
 		local oldInstance = self.__instanceMap.fromIds[id]
 		if not oldInstance then
@@ -328,6 +337,16 @@ function ServeSession:__replaceInstances(idList)
 			continue
 		end
 
+		table.insert(swaps, {
+			id = id,
+			replacement = replacement,
+			oldInstance = oldInstance,
+		})
+	end
+
+	for _, swap in orderSwaps(swaps) do
+		local id, replacement, oldInstance = swap.id, swap.replacement, swap.oldInstance
+
 		self.__instanceMap:insert(id, replacement)
 		Log.trace("Swapping Instance {} out via api/models/ endpoint", id)
 		local oldParent = oldInstance.Parent

plugin/src/orderSwaps.lua

@@ -0,0 +1,44 @@
+--[[
+	Determines the order in which `ServeSession:__replaceInstances` should swap
+	instances so that sibling order is preserved.
+
+	Roblox appends to `GetChildren()` on every reparent, so the order in which we
+	re-parent replacements determines their final sibling order. To rebuild
+	`GetChildren()` exactly as it was before the swap we must:
+
+	* process ancestors before descendants, so each replacement's parent already
+	  exists when we re-parent the replacement, and
+	* process siblings in their original `GetChildren()` order.
+
+	`swaps` is an array of `{ id, replacement, oldInstance }` entries. This sorts
+	the array in place (annotating each entry with `depth`/`siblingIndex`) and
+	returns it.
+]]
+local function orderSwaps(swaps)
+	for _, swap in swaps do
+		local depth = 0
+		local ancestor = swap.oldInstance.Parent
+		while ancestor ~= nil do
+			depth += 1
+			ancestor = ancestor.Parent
+		end
+		swap.depth = depth
+
+		local siblingIndex = 0
+		if swap.oldInstance.Parent ~= nil then
+			siblingIndex = table.find(swap.oldInstance.Parent:GetChildren(), swap.oldInstance) or 0
+		end
+		swap.siblingIndex = siblingIndex
+	end
+
+	table.sort(swaps, function(a, b)
+		if a.depth ~= b.depth then
+			return a.depth < b.depth
+		end
+		return a.siblingIndex < b.siblingIndex
+	end)
+
+	return swaps
+end
+
+return orderSwaps

plugin/src/orderSwaps.spec.lua

@@ -0,0 +1,57 @@
+return function()
+	local orderSwaps = require(script.Parent.orderSwaps)
+
+	it("orders same-named siblings by their original GetChildren order", function()
+		local parent = Instance.new("Model")
+		local a1 = Instance.new("Part")
+		a1.Name = "a"
+		a1.Parent = parent
+		local a2 = Instance.new("Part")
+		a2.Name = "a"
+		a2.Parent = parent
+		local a3 = Instance.new("Part")
+		a3.Name = "a"
+		a3.Parent = parent
+
+		-- Input deliberately out of sibling order.
+		-- orderSwaps must restore the GetChildren() order.
+		local ordered = orderSwaps({
+			{ id = "3", oldInstance = a3 },
+			{ id = "1", oldInstance = a1 },
+			{ id = "2", oldInstance = a2 },
+		})
+
+		expect(ordered[1].oldInstance).to.equal(a1)
+		expect(ordered[2].oldInstance).to.equal(a2)
+		expect(ordered[3].oldInstance).to.equal(a3)
+	end)
+
+	it("orders ancestors before descendants", function()
+		local root = Instance.new("Model")
+		local child = Instance.new("Folder")
+		child.Parent = root
+		local grandchild = Instance.new("Part")
+		grandchild.Parent = child
+
+		local ordered = orderSwaps({
+			{ id = "grandchild", oldInstance = grandchild },
+			{ id = "child", oldInstance = child },
+			{ id = "root", oldInstance = root },
+		})
+
+		expect(ordered[1].oldInstance).to.equal(root)
+		expect(ordered[2].oldInstance).to.equal(child)
+		expect(ordered[3].oldInstance).to.equal(grandchild)
+	end)
+
+	it("returns a single swap unchanged", function()
+		local part = Instance.new("Part")
+
+		local ordered = orderSwaps({
+			{ id = "1", oldInstance = part },
+		})
+
+		expect(#ordered).to.equal(1)
+		expect(ordered[1].oldInstance).to.equal(part)
+	end)
+end