Update ChangeBatcher tests to reflect encoding behavior for duplicate-named children

- Renamed test case to clarify that children with duplicate names are now encoded rather than skipped.
- Adjusted assertions to ensure all children are included in the encoded output, aligning with the updated encoding logic.
- Refactored related test cases for improved clarity and maintainability.

These changes enhance the accuracy of the ChangeBatcher tests, ensuring they correctly validate the new encoding behavior.

plugin/src/ChangeBatcher/encodeInstance.spec.lua

@@ -232,7 +232,7 @@ return function()
 				root:Destroy()
 			end)
 
-			it("should skip children with duplicate names", function()
+			it("should encode children with duplicate names", function()
 				local parent = Instance.new("Folder")
 				parent.Name = "Parent"
 
@@ -254,9 +254,8 @@ return function()
 				local encoded = encodeInstance(parent, "PARENT_ID")
 
 				expect(encoded).to.be.ok()
-				-- Should only have 1 child (UniqueName), duplicates skipped
-				expect(#encoded.children).to.equal(1)
-				expect(encoded.children[1].name).to.equal("UniqueName")
+				-- All children are encoded (server handles duplicates via dedup suffixes)
+				expect(#encoded.children).to.equal(3)
 
 				parent:Destroy()
 			end)

src/change_processor.rs

@@ -1444,8 +1444,7 @@ impl JobThreadContext {
                                                     effective_meta_base = old_base;
                                                 } else {
                                                     overridden_source_path = Some(new_path.clone());
-                                                    new_ref_segment =
-                                                        Some(new_file_name.clone());
+                                                    new_ref_segment = Some(new_file_name.clone());
                                                     let old_meta = parent
                                                         .join(format!("{}.meta.json5", old_base));
                                                     let new_meta = parent.join(format!(
@@ -1748,9 +1747,7 @@ impl JobThreadContext {
                         if *old_ref_path != new_ref_path {
                             self.update_ref_paths_after_rename(old_ref_path, &new_ref_path);
                         }
-                    } else if update.changed_name.is_some()
-                        || update.changed_class_name.is_some()
-                    {
+                    } else if update.changed_name.is_some() || update.changed_class_name.is_some() {
                         // Rename or class change was requested but no filesystem
                         // rename happened (e.g., ProjectNode, init-file class
                         // change where directory name stays the same). No ref

src/snapshot/tree.rs

@@ -10,7 +10,7 @@ use rbx_dom_weak::{
 
 use crate::{multimap::MultiMap, RojoRef};
 
-use super::{InstigatingSource, InstanceMetadata, InstanceSnapshot};
+use super::{InstanceMetadata, InstanceSnapshot, InstigatingSource};
 
 /// An expanded variant of rbx_dom_weak's `WeakDom` that tracks additional
 /// metadata per instance that's Rojo-specific.

src/web/api.rs

@@ -413,6 +413,11 @@ impl ApiService {
         // Create a stats tracker for this syncback operation
         let stats = crate::syncback::SyncbackStats::new();
 
+        // Track GUIDs that were updated in place (instance already existed
+        // in the tree). These must be excluded from the PatchAdd list to
+        // avoid creating duplicate instances in the tree.
+        let mut updated_in_place: HashSet<Ref> = HashSet::new();
+
         if !request.added.is_empty() {
             let tree = self.serve_session.tree();
             // Pre-compute duplicate sibling info in O(N) for efficient path uniqueness checks
@@ -523,14 +528,21 @@ impl ApiService {
             // Group added instances by parent so siblings share a dedup set.
             // This prevents two instances in the same batch from claiming the
             // same slug when added to the same parent.
-            let mut adds_by_parent: HashMap<Ref, Vec<&crate::web::interface::AddedInstance>> =
-                HashMap::new();
-            for added in request.added.values() {
+            let mut adds_by_parent: HashMap<
+                Ref,
+                Vec<(Ref, &crate::web::interface::AddedInstance)>,
+            > = HashMap::new();
+            for (guid, added) in &request.added {
                 if let Some(parent_ref) = added.parent {
-                    adds_by_parent.entry(parent_ref).or_default().push(added);
+                    adds_by_parent
+                        .entry(parent_ref)
+                        .or_default()
+                        .push((*guid, added));
                 } else {
-                    // No parent — process individually (will fail with context)
-                    adds_by_parent.entry(Ref::none()).or_default().push(added);
+                    adds_by_parent
+                        .entry(Ref::none())
+                        .or_default()
+                        .push((*guid, added));
                 }
             }
             for (parent_ref, siblings) in &adds_by_parent {
@@ -576,8 +588,8 @@ impl ApiService {
                 } else {
                     HashSet::new()
                 };
-                for added in siblings {
-                    if let Err(err) = self.syncback_added_instance(
+                for (guid, added) in siblings {
+                    match self.syncback_added_instance(
                         added,
                         &tree,
                         &duplicate_siblings_cache,
@@ -585,11 +597,17 @@ impl ApiService {
                         &converted_parents,
                         &mut sibling_slugs,
                     ) {
-                        log::warn!(
-                            "Failed to syncback added instance '{}': {}",
-                            added.name,
-                            err
-                        );
+                        Ok(true) => {
+                            updated_in_place.insert(*guid);
+                        }
+                        Ok(false) => {}
+                        Err(err) => {
+                            log::warn!(
+                                "Failed to syncback added instance '{}': {}",
+                                added.name,
+                                err
+                            );
+                        }
                     }
                 }
             }
@@ -696,9 +714,15 @@ impl ApiService {
         // plugin's GUID is used as the snapshot_id so that apply_patch_set's
         // snapshot_id_to_instance_id map rewrites Ref properties in
         // updated_instances to point to the correct tree IDs.
+        //
+        // Instances that were updated in place (already existed in the tree)
+        // are excluded — adding them would create duplicates. Tree
+        // reconciliation will pick up any property changes from the
+        // filesystem write.
         let added_instances: Vec<PatchAdd> = request
             .added
             .iter()
+            .filter(|(guid, _)| !updated_in_place.contains(guid))
             .filter_map(|(guid, added)| {
                 let parent_ref = added.parent?;
                 Some(PatchAdd {
@@ -911,7 +935,7 @@ impl ApiService {
         stats: &crate::syncback::SyncbackStats,
         converted_parents: &HashMap<Ref, PathBuf>,
         sibling_slugs: &mut HashSet<String>,
-    ) -> anyhow::Result<()> {
+    ) -> anyhow::Result<bool> {
         use anyhow::Context;
 
         // Get the parent Ref (required for top-level added instances)
@@ -947,8 +971,11 @@ impl ApiService {
                         added.name,
                         existing_path.display()
                     );
-                    // Update the existing instance instead of creating new files
-                    return self.syncback_update_existing_instance(added, existing_path, stats);
+                    // Update the existing instance instead of creating new files.
+                    // Return true to signal caller to skip PatchAdd (instance
+                    // already exists in tree, no need to add a duplicate).
+                    self.syncback_update_existing_instance(added, existing_path, stats)?;
+                    return Ok(true);
                 }
             }
         }
@@ -1048,7 +1075,7 @@ impl ApiService {
         let slug =
             self.syncback_instance_to_path_with_stats(added, &parent_dir, stats, sibling_slugs)?;
         sibling_slugs.insert(slug);
-        Ok(())
+        Ok(false)
     }
 
     /// Update an existing instance in place instead of creating new files.

tests/tests/two_way_sync.rs

@@ -6156,7 +6156,11 @@ fn ref_set_object_value() {
         let model_path = session
             .path()
             .join("src/Workspace/TestObjectValue.model.json5");
-        poll_meta_has_ref_attr(&model_path, "Rojo_Ref_Value", "Workspace/TestModel/Part1.model.json5");
+        poll_meta_has_ref_attr(
+            &model_path,
+            "Rojo_Ref_Value",
+            "Workspace/TestModel/Part1.model.json5",
+        );
     });
 }
 
@@ -6467,7 +6471,11 @@ fn ref_on_model_json5_instance() {
         let model_path = session
             .path()
             .join("src/Workspace/TestObjectValue.model.json5");
-        poll_meta_has_ref_attr(&model_path, "Rojo_Ref_Value", "Workspace/TestModel/Part1.model.json5");
+        poll_meta_has_ref_attr(
+            &model_path,
+            "Rojo_Ref_Value",
+            "Workspace/TestModel/Part1.model.json5",
+        );
     });
 }
 
@@ -6829,7 +6837,11 @@ fn ref_ambiguous_path_no_crash() {
         let meta_path = session
             .path()
             .join("src/Workspace/DupParent/init.meta.json5");
-        poll_meta_has_ref_attr(&meta_path, "Rojo_Ref_PrimaryPart", "Workspace/Target.model.json5");
+        poll_meta_has_ref_attr(
+            &meta_path,
+            "Rojo_Ref_PrimaryPart",
+            "Workspace/Target.model.json5",
+        );
     });
 }
 
@@ -6990,7 +7002,11 @@ fn ref_partial_removal_preserves_remaining_index_entry() {
             "Workspace/TestModel/Part1.model.json5",
         );
         // Verify second ref is there too
-        assert_meta_has_ref_attr(&meta_path, "Rojo_Ref_Adornee", "Workspace/TestModel/Part2.model.json5");
+        assert_meta_has_ref_attr(
+            &meta_path,
+            "Rojo_Ref_Adornee",
+            "Workspace/TestModel/Part2.model.json5",
+        );
 
         // Step 2: Remove PrimaryPart (set to nil), keep Adornee
         let mut props2 = UstrMap::default();
@@ -7010,7 +7026,11 @@ fn ref_partial_removal_preserves_remaining_index_entry() {
         thread::sleep(Duration::from_millis(300));
         assert_meta_no_ref_attr(&meta_path, "Rojo_Ref_PrimaryPart");
         // Adornee should still be there
-        assert_meta_has_ref_attr(&meta_path, "Rojo_Ref_Adornee", "Workspace/TestModel/Part2.model.json5");
+        assert_meta_has_ref_attr(
+            &meta_path,
+            "Rojo_Ref_Adornee",
+            "Workspace/TestModel/Part2.model.json5",
+        );
 
         // Step 3: Rename Part2. The RefPathIndex should still have the entry
         // for Adornee (not overbroad-removed when PrimaryPart was deleted).