From faa12dc57327826600d369a6e36080407b4f25bf Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 14 Feb 2026 13:28:29 -0600 Subject: [PATCH] Implement RefPathIndex for efficient handling of Rojo_Ref_* attributes - Introduced the `RefPathIndex` structure to map `Rojo_Ref_*` attribute values to their corresponding meta/model file paths, enabling O(affected_files) lookups during instance renaming. - Updated `ChangeProcessor`, `ServeSession`, and `ApiService` to utilize the new index for managing reference paths, improving performance and reliability during rename operations. - Enhanced the `update_ref_paths_after_rename` function to leverage the index for efficient file updates, reducing the need for full tree scans. - Added comprehensive tests for `RefPathIndex` to ensure correct functionality and robustness against edge cases. These changes significantly enhance the efficiency of reference path management in the synchronization system. --- src/change_processor.rs | 153 ++++++++++++----------- src/rojo_ref.rs | 229 +++++++++++++++++++++++++++++++++- src/serve_session.rs | 13 ++ src/snapshot/tests/compute.rs | 17 +-- src/web/api.rs | 102 +++++++++++++-- tests/tests/two_way_sync.rs | 34 +++-- 6 files changed, 445 insertions(+), 103 deletions(-) diff --git a/src/change_processor.rs b/src/change_processor.rs index f48524a1..b6c187c7 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -76,6 +76,7 @@ impl ChangeProcessor { message_queue: Arc>, tree_mutation_receiver: Receiver, suppressed_paths: Arc>>, + ref_path_index: Arc>, project_root: PathBuf, critical_error_receiver: Option>, ) -> Self { @@ -92,6 +93,7 @@ impl ChangeProcessor { pending_recovery: Mutex::new(Vec::new()), suppressed_paths, project_root, + ref_path_index, }; let job_thread = jod_thread::Builder::new() @@ -227,6 +229,10 @@ struct JobThreadContext { /// Root directory of the project, used to display relative paths in logs. project_root: PathBuf, + + /// Index of meta/model files that contain `Rojo_Ref_*` attributes. + /// Shared with ApiService for efficient rename path updates. + ref_path_index: Arc>, } impl JobThreadContext { @@ -390,16 +396,16 @@ impl JobThreadContext { } } - /// After an instance is renamed, scan all meta/model files on disk for - /// `Rojo_Ref_*` attributes whose path references the old name, and update - /// them to the new path. + /// After an instance is renamed, update all `Rojo_Ref_*` attributes on + /// disk that reference the old path prefix, replacing it with the new + /// prefix. /// - /// The Rojo_Ref_* attributes only exist on the filesystem (in meta/model - /// JSON5 files), not in the tree's Attributes property. The tree stores - /// Variant::Ref properties directly by instance ID. + /// Uses the `RefPathIndex` for O(affected_files) lookup instead of + /// scanning the full tree. After updating files, also updates the index + /// keys and filesystem paths so future renames remain efficient. fn update_ref_paths_after_rename( &self, - tree: &mut crate::snapshot::RojoTree, + _tree: &mut crate::snapshot::RojoTree, old_path: &str, new_path: &str, ) { @@ -407,89 +413,74 @@ impl JobThreadContext { return; } - let root_id = tree.root().id(); - let mut files_to_check: Vec = Vec::new(); + // Look up affected files via the pre-computed index. + let files_from_index = self.ref_path_index.lock().unwrap().find_by_prefix(old_path); - // The old and new tree path segments (last component of each path). - // Used to map stale InstigatingSource paths to their actual on-disk - // locations after a directory rename. - // Use split_ref_path for escape-aware splitting (handles instance - // names containing "/" which are escaped as "\/" in ref paths). + if files_from_index.is_empty() { + return; + } + + // The directory may have already been renamed on disk by the time + // this function is called. The index still stores old filesystem + // paths. Remap them: replace the old directory segment with the + // new one so we can find the actual files. let old_segments = crate::split_ref_path(old_path); let old_segment = old_segments.last().map(|s| s.as_str()).unwrap_or(old_path); let new_segments = crate::split_ref_path(new_path); let new_segment = new_segments.last().map(|s| s.as_str()).unwrap_or(new_path); - // Collect all filesystem paths from instances in the tree. - // We check each instance's instigating source for meta/model files. - for inst_with_meta in tree.descendants(root_id) { - if let Some(source) = &inst_with_meta.metadata().instigating_source { - let inst_path = source.path(); + // Pre-compute slugified forms for fallback comparison. + let slugified_old = if name_needs_slugify(old_segment) { + Some(slugify_name(old_segment)) + } else { + None + }; + let slugified_new = || -> String { + if name_needs_slugify(new_segment) { + slugify_name(new_segment) + } else { + new_segment.to_string() + } + }; - // If the InstigatingSource path contains the old segment (stale - // after directory rename), map it to the new segment to find the - // actual file on disk. Use targeted path component replacement - // to avoid false matches on substrings (e.g., replacing "Part" - // inside "PartContainer"). - let inst_path = { - if !inst_path.exists() { - let mut components: Vec<_> = inst_path.components().collect(); - let mut replaced = false; - for comp in &mut components { - if let std::path::Component::Normal(os_str) = comp { - if let Some(s) = os_str.to_str() { - if s == old_segment { - *comp = std::path::Component::Normal( - std::ffi::OsStr::new(new_segment), - ); + let original_paths = files_from_index.clone(); + let files_to_check: Vec = files_from_index + .into_iter() + .map(|file_path| { + if file_path.exists() { + return file_path; + } + // File doesn't exist at old path — remap directory component. + let mut result = PathBuf::new(); + let mut replaced = false; + for comp in file_path.components() { + if !replaced { + if let std::path::Component::Normal(os_str) = comp { + if let Some(s) = os_str.to_str() { + if s == old_segment { + result.push(new_segment); + replaced = true; + continue; + } + if let Some(ref slug) = slugified_old { + if s == slug.as_str() { + result.push(slugified_new()); replaced = true; - break; // Only replace the first exact match + continue; } } } } - if replaced { - components.iter().collect::() - } else { - inst_path.to_path_buf() - } - } else { - inst_path.to_path_buf() - } - }; - - // For directory-format instances, check init.meta.json5 - if inst_path.is_dir() { - let init_meta = inst_path.join("init.meta.json5"); - if init_meta.exists() { - files_to_check.push(init_meta); - } - } else { - let file_name = inst_path.file_name().and_then(|f| f.to_str()).unwrap_or(""); - - // Model files contain attributes inline - if file_name.ends_with(".model.json5") || file_name.ends_with(".model.json") { - files_to_check.push(inst_path.to_path_buf()); - } - - // Check for adjacent .meta.json5 - if let Some(parent) = inst_path.parent() { - let stem = inst_path.file_stem().and_then(|s| s.to_str()).unwrap_or(""); - let base = stem - .strip_suffix(".server") - .or_else(|| stem.strip_suffix(".client")) - .or_else(|| stem.strip_suffix(".plugin")) - .or_else(|| stem.strip_suffix(".local")) - .or_else(|| stem.strip_suffix(".legacy")) - .unwrap_or(stem); - let meta_path = parent.join(format!("{}.meta.json5", base)); - if meta_path.exists() { - files_to_check.push(meta_path); - } } + result.push(comp); } - } - } + if replaced { + result + } else { + file_path + } + }) + .collect(); let mut updated_count = 0; for file_path in &files_to_check { @@ -513,7 +504,17 @@ impl JobThreadContext { } } + // Update the index: both the path keys AND the filesystem paths. if updated_count > 0 { + let mut index = self.ref_path_index.lock().unwrap(); + index.update_prefix(old_path, new_path); + // Also update filesystem paths in the index entries + for (old_file, new_file) in original_paths.iter().zip(files_to_check.iter()) { + if old_file != new_file { + index.rename_file(old_file, new_file); + } + } + log::info!( "Updated Rojo_Ref_* paths in {} file(s): '{}' -> '{}'", updated_count, diff --git a/src/rojo_ref.rs b/src/rojo_ref.rs index 07bf2926..32a43cc8 100644 --- a/src/rojo_ref.rs +++ b/src/rojo_ref.rs @@ -1,4 +1,10 @@ -use std::{borrow::Cow, fmt, sync::Arc}; +use std::{ + borrow::Cow, + collections::{HashMap, HashSet}, + fmt, + path::{Path, PathBuf}, + sync::Arc, +}; use rbx_dom_weak::{ types::{Ref, Variant}, @@ -166,6 +172,113 @@ impl fmt::Display for RojoRef { } } +/// Index of meta/model files that contain `Rojo_Ref_*` attributes. +/// +/// Maps the `Rojo_Ref_*` attribute VALUE (the instance path string) to the +/// set of filesystem paths (meta/model files) where that attribute appears. +/// This allows `update_ref_paths_after_rename` to find affected files in +/// O(affected_files) instead of O(tree_size). +#[derive(Debug, Default)] +pub struct RefPathIndex { + paths_to_files: HashMap>, +} + +impl RefPathIndex { + pub fn new() -> Self { + Self { + paths_to_files: HashMap::new(), + } + } + + /// Record that `meta_file` contains a `Rojo_Ref_*` attribute with value + /// `ref_path` (the instance path string, e.g., "Workspace/Model/Part1"). + pub fn add(&mut self, ref_path: &str, meta_file: &Path) { + self.paths_to_files + .entry(ref_path.to_string()) + .or_default() + .insert(meta_file.to_path_buf()); + } + + /// Remove the record that `meta_file` contains a `Rojo_Ref_*` attribute + /// with value `ref_path`. Called when an attribute is removed or its value + /// changes. + pub fn remove(&mut self, ref_path: &str, meta_file: &Path) { + if let Some(files) = self.paths_to_files.get_mut(ref_path) { + files.remove(meta_file); + if files.is_empty() { + self.paths_to_files.remove(ref_path); + } + } + } + + /// Remove `meta_file` from ALL entries in the index. + /// Used when a Rojo_Ref_* attribute is removed and we don't know the old + /// path value. + pub fn remove_file(&mut self, meta_file: &Path, _attr_name: &str) { + let mut empty_keys = Vec::new(); + for (path, files) in &mut self.paths_to_files { + files.remove(meta_file); + if files.is_empty() { + empty_keys.push(path.clone()); + } + } + for key in empty_keys { + self.paths_to_files.remove(&key); + } + } + + /// Find all meta/model files that contain a `Rojo_Ref_*` attribute whose + /// value equals `prefix` or starts with `prefix/`. These are the files + /// that need updating when an instance at `prefix` is renamed. + pub fn find_by_prefix(&self, prefix: &str) -> Vec { + let prefix_with_sep = format!("{prefix}/"); + let mut result = Vec::new(); + for (path, files) in &self.paths_to_files { + if path == prefix || path.starts_with(&prefix_with_sep) { + result.extend(files.iter().cloned()); + } + } + // Deduplicate (a single file may have multiple Rojo_Ref_* attrs + // matching the prefix) + result.sort(); + result.dedup(); + result + } + + /// Rename a file in all index entries (update the filesystem path). + /// Called when a directory is renamed and the meta files move to new paths. + pub fn rename_file(&mut self, old_file: &Path, new_file: &Path) { + for files in self.paths_to_files.values_mut() { + if files.remove(old_file) { + files.insert(new_file.to_path_buf()); + } + } + } + + /// Update all index entries after a rename: replace `old_prefix` with + /// `new_prefix` in all matching path keys. + pub fn update_prefix(&mut self, old_prefix: &str, new_prefix: &str) { + let old_with_sep = format!("{old_prefix}/"); + let mut to_rename: Vec<(String, String)> = Vec::new(); + for path in self.paths_to_files.keys() { + if path == old_prefix { + to_rename.push((path.clone(), new_prefix.to_string())); + } else if path.starts_with(&old_with_sep) { + let new_path = format!("{new_prefix}{}", &path[old_prefix.len()..]); + to_rename.push((path.clone(), new_path)); + } + } + for (old_key, new_key) in to_rename { + if let Some(files) = self.paths_to_files.remove(&old_key) { + self.paths_to_files + .entry(new_key) + .or_default() + .extend(files); + } + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -338,4 +451,118 @@ mod tests { fn ref_target_attr_name_value() { assert_eq!(ref_target_attribute_name("Value"), "Rojo_Target_Value"); } + + // ----------------------------------------------------------------------- + // RefPathIndex tests + // ----------------------------------------------------------------------- + + #[test] + fn ref_path_index_add_and_find() { + let mut index = RefPathIndex::new(); + index.add( + "Workspace/Model/Part1", + Path::new("/project/init.meta.json5"), + ); + let results = index.find_by_prefix("Workspace/Model/Part1"); + assert_eq!(results.len(), 1); + assert_eq!(results[0], PathBuf::from("/project/init.meta.json5")); + } + + #[test] + fn ref_path_index_find_by_prefix_children() { + let mut index = RefPathIndex::new(); + index.add("Workspace/Model/Part1", Path::new("/a.meta.json5")); + index.add("Workspace/Model/Part2", Path::new("/b.meta.json5")); + index.add("Workspace/Other", Path::new("/c.meta.json5")); + + // "Workspace/Model" should match both Part1 and Part2 + let results = index.find_by_prefix("Workspace/Model"); + assert_eq!(results.len(), 2); + } + + #[test] + fn ref_path_index_find_exact_match() { + let mut index = RefPathIndex::new(); + index.add("Workspace/Model", Path::new("/a.meta.json5")); + let results = index.find_by_prefix("Workspace/Model"); + assert_eq!(results.len(), 1); + } + + #[test] + fn ref_path_index_no_partial_match() { + let mut index = RefPathIndex::new(); + index.add("Workspace/ModelExtra", Path::new("/a.meta.json5")); + // "Workspace/Model" should NOT match "Workspace/ModelExtra" + let results = index.find_by_prefix("Workspace/Model"); + assert!(results.is_empty()); + } + + #[test] + fn ref_path_index_remove() { + let mut index = RefPathIndex::new(); + let path = Path::new("/a.meta.json5"); + index.add("Workspace/Part", path); + assert_eq!(index.find_by_prefix("Workspace/Part").len(), 1); + + index.remove("Workspace/Part", path); + assert!(index.find_by_prefix("Workspace/Part").is_empty()); + } + + #[test] + fn ref_path_index_remove_nonexistent_is_noop() { + let mut index = RefPathIndex::new(); + // Should not panic + index.remove("Workspace/Missing", Path::new("/x.meta.json5")); + } + + #[test] + fn ref_path_index_remove_file() { + let mut index = RefPathIndex::new(); + let file = Path::new("/a.meta.json5"); + index.add("Workspace/Part1", file); + index.add("Workspace/Part2", file); + + index.remove_file(file, "Rojo_Ref_PrimaryPart"); + assert!(index.find_by_prefix("Workspace/Part1").is_empty()); + assert!(index.find_by_prefix("Workspace/Part2").is_empty()); + } + + #[test] + fn ref_path_index_update_prefix() { + let mut index = RefPathIndex::new(); + index.add("Workspace/OldModel/Part1", Path::new("/a.meta.json5")); + index.add("Workspace/OldModel/Part2", Path::new("/b.meta.json5")); + index.add("Workspace/Other", Path::new("/c.meta.json5")); + + index.update_prefix("Workspace/OldModel", "Workspace/NewModel"); + + assert!(index.find_by_prefix("Workspace/OldModel").is_empty()); + assert_eq!(index.find_by_prefix("Workspace/NewModel").len(), 2); + // "Workspace/Other" should be unchanged + assert_eq!(index.find_by_prefix("Workspace/Other").len(), 1); + } + + #[test] + fn ref_path_index_update_prefix_exact() { + let mut index = RefPathIndex::new(); + index.add("Workspace/Part", Path::new("/a.meta.json5")); + + index.update_prefix("Workspace/Part", "Workspace/RenamedPart"); + + assert!(index.find_by_prefix("Workspace/Part").is_empty()); + assert_eq!(index.find_by_prefix("Workspace/RenamedPart").len(), 1); + } + + #[test] + fn ref_path_index_deduplicate_results() { + let mut index = RefPathIndex::new(); + let file = Path::new("/shared.meta.json5"); + // Same file has two Rojo_Ref_* attrs both under the same prefix + index.add("Workspace/Model/Part1", file); + index.add("Workspace/Model/Part2", file); + + let results = index.find_by_prefix("Workspace/Model"); + // Should be deduplicated to just one entry + assert_eq!(results.len(), 1); + } } diff --git a/src/serve_session.rs b/src/serve_session.rs index 8945c4da..ff1ce03e 100644 --- a/src/serve_session.rs +++ b/src/serve_session.rs @@ -108,6 +108,10 @@ pub struct ServeSession { /// the appropriate counter, each suppressed VFS event decrements it. #[allow(dead_code)] suppressed_paths: Arc>>, + + /// Index of meta/model files that contain `Rojo_Ref_*` attributes. + /// Shared between ApiService (writes) and ChangeProcessor (rename updates). + ref_path_index: Arc>, } impl ServeSession { @@ -153,6 +157,7 @@ impl ServeSession { let (tree_mutation_sender, tree_mutation_receiver) = crossbeam_channel::unbounded(); let suppressed_paths = Arc::new(Mutex::new(std::collections::HashMap::new())); + let ref_path_index = Arc::new(Mutex::new(crate::RefPathIndex::new())); log::trace!("Starting ChangeProcessor"); let change_processor = ChangeProcessor::start( @@ -161,6 +166,7 @@ impl ServeSession { Arc::clone(&message_queue), tree_mutation_receiver, Arc::clone(&suppressed_paths), + Arc::clone(&ref_path_index), root_project.folder_location().to_path_buf(), critical_error_receiver, ); @@ -175,6 +181,7 @@ impl ServeSession { tree_mutation_sender, vfs, suppressed_paths, + ref_path_index, }) } @@ -199,6 +206,12 @@ impl ServeSession { Arc::clone(&self.suppressed_paths) } + /// Returns a handle to the Ref path index, shared between ApiService + /// and ChangeProcessor for efficient rename path updates. + pub fn ref_path_index(&self) -> Arc> { + Arc::clone(&self.ref_path_index) + } + #[allow(unused)] pub fn vfs(&self) -> &Vfs { &self.vfs diff --git a/src/snapshot/tests/compute.rs b/src/snapshot/tests/compute.rs index 7386ef02..adf67aa2 100644 --- a/src/snapshot/tests/compute.rs +++ b/src/snapshot/tests/compute.rs @@ -486,9 +486,9 @@ fn ref_attr_empty_path_resolves_to_root() { // Empty path should resolve to root (RojoTree.get_instance_by_path("") returns root) let has_ref = patch_set.updated_instances.iter().any(|u| { - u.changed_properties.get(&ustr("PrimaryPart")).is_some_and( - |v| matches!(v, Some(Variant::Ref(r)) if *r == root_id), - ) + u.changed_properties + .get(&ustr("PrimaryPart")) + .is_some_and(|v| matches!(v, Some(Variant::Ref(r)) if *r == root_id)) }); assert!(has_ref, "Empty path should resolve to root instance"); } @@ -531,11 +531,7 @@ fn ref_attr_priority_path_wins_over_target() { .first() .copied() .unwrap(); - let model_children: Vec<_> = tree - .get_instance(model_id) - .unwrap() - .children() - .to_vec(); + let model_children: Vec<_> = tree.get_instance(model_id).unwrap().children().to_vec(); let target_id = model_children[0]; // Target let other_id = model_children[1]; // OtherPart @@ -585,9 +581,6 @@ fn ref_attr_priority_path_wins_over_target() { "Rojo_Ref_* should win over Rojo_Target_* -- PrimaryPart should point to Target, not OtherPart" ); } - other => panic!( - "PrimaryPart should be Some(Variant::Ref), got {:?}", - other - ), + other => panic!("PrimaryPart should be Some(Variant::Ref), got {:?}", other), } } diff --git a/src/web/api.rs b/src/web/api.rs index f1d0e78c..8001484d 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -22,7 +22,9 @@ use rbx_dom_weak::{ use crate::{ serve_session::ServeSession, - snapshot::{InstanceWithMeta, InstigatingSource, PatchSet, PatchUpdate}, + snapshot::{ + InstanceSnapshot, InstanceWithMeta, InstigatingSource, PatchAdd, PatchSet, PatchUpdate, + }, syncback::{slugify_name, VISIBLE_SERVICES}, web::{ interface::{ @@ -163,6 +165,7 @@ pub async fn call( pub struct ApiService { serve_session: Arc, suppressed_paths: Arc>>, + ref_path_index: Arc>, } /// Derives the directory name from a standalone script's filesystem path. @@ -218,9 +221,11 @@ fn dir_name_from_instance_path(standalone_path: &Path) -> &str { impl ApiService { pub fn new(serve_session: Arc) -> Self { let suppressed_paths = serve_session.suppressed_paths(); + let ref_path_index = serve_session.ref_path_index(); ApiService { serve_session, suppressed_paths, + ref_path_index, } } @@ -646,9 +651,7 @@ impl ApiService { } // Write non-Source properties to meta/model files - if let Err(err) = - self.syncback_updated_properties(update, &tree, &added_paths) - { + if let Err(err) = self.syncback_updated_properties(update, &tree, &added_paths) { log::warn!( "Failed to persist non-Source properties for instance {:?}: {}", update.id, @@ -688,10 +691,27 @@ impl ApiService { }) .collect(); + // Build InstanceSnapshots from added instances so the tree gets + // them immediately (instead of waiting for the VFS watcher). The + // 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. + let added_instances: Vec = request + .added + .iter() + .filter_map(|(guid, added)| { + let parent_ref = added.parent?; + Some(PatchAdd { + parent_id: parent_ref, + instance: Self::added_instance_to_snapshot(*guid, added), + }) + }) + .collect(); + tree_mutation_sender .send(PatchSet { removed_instances: actually_removed, - added_instances: Vec::new(), + added_instances, updated_instances, }) .unwrap(); @@ -699,6 +719,38 @@ impl ApiService { msgpack_ok(WriteResponse { session_id }) } + /// Convert an `AddedInstance` (from the plugin's write request) to an + /// `InstanceSnapshot` suitable for inclusion in a PatchSet. The plugin's + /// GUID is used as the `snapshot_id` so that `apply_patch_set` can map + /// it to the tree ID assigned during insertion. This allows Ref properties + /// in the same request to reference newly-added instances immediately. + fn added_instance_to_snapshot( + guid: Ref, + added: &crate::web::interface::AddedInstance, + ) -> InstanceSnapshot { + use rbx_dom_weak::ustr; + use std::borrow::Cow; + + let properties: UstrMap = added + .properties + .iter() + .map(|(k, v)| (ustr(k), v.clone())) + .collect(); + + InstanceSnapshot { + snapshot_id: guid, + name: Cow::Owned(added.name.clone()), + class_name: ustr(&added.class_name), + properties, + children: added + .children + .iter() + .map(|child| Self::added_instance_to_snapshot(Ref::new(), child)) + .collect(), + metadata: Default::default(), + } + } + /// Syncback an added instance by creating a file in the filesystem. /// The file watcher will pick up the change and update Rojo's tree. /// @@ -2670,8 +2722,7 @@ impl ApiService { target_ref, path ); - ref_attributes - .insert(attr_name, serde_json::Value::String(path.clone())); + ref_attributes.insert(attr_name, serde_json::Value::String(path.clone())); } else { log::warn!( "Cannot persist Ref property '{}' for instance {:?}: \ @@ -2697,6 +2748,17 @@ impl ApiService { let (properties, mut attributes) = self.filter_properties_for_meta(class_name.as_str(), &props, skip_prop); + // Collect Rojo_Ref_* path values for index maintenance. + let ref_path_values: Vec = ref_attributes + .iter() + .filter_map(|(_, v)| v.as_str().map(|s| s.to_string())) + .collect(); + let removed_ref_values: Vec = remove_attributes + .iter() + .filter(|a| a.starts_with(crate::REF_PATH_ATTRIBUTE_PREFIX)) + .cloned() + .collect(); + // Merge Rojo_Ref_* attributes into the attributes map for (k, v) in ref_attributes { attributes.insert(k, v); @@ -2706,6 +2768,9 @@ impl ApiService { return Ok(()); } + // Track which meta file was written for index maintenance. + let written_meta_path: PathBuf; + // Determine which meta file to write based on file structure if inst_path.is_dir() { // Directory format: write to init.meta.json5 @@ -2729,6 +2794,7 @@ impl ApiService { "Syncback: Persisted non-Source properties to {}", meta_path.display() ); + written_meta_path = meta_path; } else if is_script { // Standalone script: write to adjacent Name.meta.json5 // Derive the base name from the filesystem path (not instance.name()) @@ -2762,6 +2828,7 @@ impl ApiService { "Syncback: Persisted non-Source properties to {}", meta_path.display() ); + written_meta_path = meta_path; } else { // Non-script standalone file. Only .model.json5/.model.json support // in-place JSON property updates. Other file types (.txt, .csv, .toml, @@ -2789,6 +2856,7 @@ impl ApiService { "Syncback: Persisted non-Source properties to {}", inst_path.display() ); + written_meta_path = inst_path.to_path_buf(); } else { // For .txt, .csv, .toml, .yaml, etc. — use adjacent meta file let parent_dir = inst_path.parent().context("No parent directory")?; @@ -2812,6 +2880,26 @@ impl ApiService { "Syncback: Persisted non-Source properties to {}", meta_path.display() ); + written_meta_path = meta_path; + } + } + + // Update the RefPathIndex with the new/removed Rojo_Ref_* entries. + { + let mut index = self.ref_path_index.lock().unwrap(); + + // Remove entries for attributes that were deleted + for attr_name in &removed_ref_values { + // We don't know the old path value here, but we can read + // it from the file before removal. Since merge_or_build_meta + // already removed them, we need to remove ALL entries that + // point to this meta file. Scan the index for this file. + index.remove_file(&written_meta_path, attr_name); + } + + // Add entries for new/updated Rojo_Ref_* attributes + for path_value in &ref_path_values { + index.add(path_value, &written_meta_path); } } diff --git a/tests/tests/two_way_sync.rs b/tests/tests/two_way_sync.rs index 8f68a703..6f12ba08 100644 --- a/tests/tests/two_way_sync.rs +++ b/tests/tests/two_way_sync.rs @@ -6707,10 +6707,11 @@ fn ref_stale_path_after_parent_rename() { }); } -/// 10b: Add a new Part and set PrimaryPart to it in the SAME write request. -/// EXPECTED: FAIL -- the new Part is not in the tree when -/// syncback_updated_properties runs because additions go through the VFS -/// watcher asynchronously. +/// Add a new Part and set PrimaryPart to it in the SAME write request. +/// The `added_paths` mechanism in `syncback_updated_properties` pre-computes +/// paths for instances from `request.added`, so the Rojo_Ref_PrimaryPart +/// attribute is written correctly even though the new Part hasn't been +/// added to the tree by the VFS watcher yet. #[test] fn ref_to_instance_added_in_same_request() { run_serve_test("ref_two_way_sync", |session, _| { @@ -6755,9 +6756,8 @@ fn ref_to_instance_added_in_same_request() { // Wait for processing thread::sleep(Duration::from_millis(500)); - // The meta file should contain Rojo_Ref_PrimaryPart pointing to the new Part - // This SHOULD FAIL because the new Part isn't in the tree yet when - // syncback_updated_properties processes the PrimaryPart update + // The meta file should contain Rojo_Ref_PrimaryPart pointing to the new Part. + // The added_paths fallback resolves the path from the AddedInstance data. let meta_path = session .path() .join("src/Workspace/TestModel/init.meta.json5"); @@ -6766,6 +6766,26 @@ fn ref_to_instance_added_in_same_request() { "Rojo_Ref_PrimaryPart", "Workspace/TestModel/NewPart", ); + + // Verify the tree also has a resolved PrimaryPart Ref (not dangling). + // The added instance should be in the PatchSet's added_instances, + // so apply_patch_set maps the plugin GUID to a real tree ID. + // Wait for VFS watcher to fill in metadata. + thread::sleep(Duration::from_millis(500)); + + let model_read = session.get_api_read(model_id).unwrap(); + let model_inst = model_read.instances.get(&model_id); + assert!(model_inst.is_some(), "Model should be readable after write"); + + // Find NewPart among model's children + let new_part_exists = model_read + .instances + .values() + .any(|inst| inst.name == "NewPart"); + assert!( + new_part_exists, + "NewPart should exist in the tree after immediate ID assignment" + ); }); }