From aabbfb23b2b621f7786e07b8e92a05b4f46244f6 Mon Sep 17 00:00:00 2001 From: Joe Date: Tue, 17 Feb 2026 05:00:24 -0600 Subject: [PATCH] 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. --- .../src/ChangeBatcher/encodeInstance.spec.lua | 7 +-- src/change_processor.rs | 7 +-- src/snapshot/tree.rs | 2 +- src/web/api.rs | 61 +++++++++++++------ tests/tests/two_way_sync.rs | 30 +++++++-- 5 files changed, 75 insertions(+), 32 deletions(-) diff --git a/plugin/src/ChangeBatcher/encodeInstance.spec.lua b/plugin/src/ChangeBatcher/encodeInstance.spec.lua index fbb35db0..596f16d6 100644 --- a/plugin/src/ChangeBatcher/encodeInstance.spec.lua +++ b/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) diff --git a/src/change_processor.rs b/src/change_processor.rs index 796c2351..8c5cfc5a 100644 --- a/src/change_processor.rs +++ b/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 diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index 8966b635..a8085cf1 100644 --- a/src/snapshot/tree.rs +++ b/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. diff --git a/src/web/api.rs b/src/web/api.rs index 566ff0ed..0eabb23a 100644 --- a/src/web/api.rs +++ b/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 = 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> = - 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 = 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, sibling_slugs: &mut HashSet, - ) -> anyhow::Result<()> { + ) -> anyhow::Result { 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. diff --git a/tests/tests/two_way_sync.rs b/tests/tests/two_way_sync.rs index c4f1f1c9..3d68fb50 100644 --- a/tests/tests/two_way_sync.rs +++ b/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).