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.
This commit is contained in:
Joe 2026-02-14 13:28:29 -06:00
parent ca2a938902
commit faa12dc573
6 changed files with 445 additions and 103 deletions

View file

@ -76,6 +76,7 @@ impl ChangeProcessor {
message_queue: Arc<MessageQueue<AppliedPatchSet>>, message_queue: Arc<MessageQueue<AppliedPatchSet>>,
tree_mutation_receiver: Receiver<PatchSet>, tree_mutation_receiver: Receiver<PatchSet>,
suppressed_paths: Arc<Mutex<std::collections::HashMap<PathBuf, (usize, usize)>>>, suppressed_paths: Arc<Mutex<std::collections::HashMap<PathBuf, (usize, usize)>>>,
ref_path_index: Arc<Mutex<crate::RefPathIndex>>,
project_root: PathBuf, project_root: PathBuf,
critical_error_receiver: Option<Receiver<memofs::WatcherCriticalError>>, critical_error_receiver: Option<Receiver<memofs::WatcherCriticalError>>,
) -> Self { ) -> Self {
@ -92,6 +93,7 @@ impl ChangeProcessor {
pending_recovery: Mutex::new(Vec::new()), pending_recovery: Mutex::new(Vec::new()),
suppressed_paths, suppressed_paths,
project_root, project_root,
ref_path_index,
}; };
let job_thread = jod_thread::Builder::new() 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. /// Root directory of the project, used to display relative paths in logs.
project_root: PathBuf, 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<Mutex<crate::RefPathIndex>>,
} }
impl JobThreadContext { impl JobThreadContext {
@ -390,16 +396,16 @@ impl JobThreadContext {
} }
} }
/// After an instance is renamed, scan all meta/model files on disk for /// After an instance is renamed, update all `Rojo_Ref_*` attributes on
/// `Rojo_Ref_*` attributes whose path references the old name, and update /// disk that reference the old path prefix, replacing it with the new
/// them to the new path. /// prefix.
/// ///
/// The Rojo_Ref_* attributes only exist on the filesystem (in meta/model /// Uses the `RefPathIndex` for O(affected_files) lookup instead of
/// JSON5 files), not in the tree's Attributes property. The tree stores /// scanning the full tree. After updating files, also updates the index
/// Variant::Ref properties directly by instance ID. /// keys and filesystem paths so future renames remain efficient.
fn update_ref_paths_after_rename( fn update_ref_paths_after_rename(
&self, &self,
tree: &mut crate::snapshot::RojoTree, _tree: &mut crate::snapshot::RojoTree,
old_path: &str, old_path: &str,
new_path: &str, new_path: &str,
) { ) {
@ -407,89 +413,74 @@ impl JobThreadContext {
return; return;
} }
let root_id = tree.root().id(); // Look up affected files via the pre-computed index.
let mut files_to_check: Vec<std::path::PathBuf> = Vec::new(); 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). if files_from_index.is_empty() {
// Used to map stale InstigatingSource paths to their actual on-disk return;
// locations after a directory rename. }
// Use split_ref_path for escape-aware splitting (handles instance
// names containing "/" which are escaped as "\/" in ref paths). // 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_segments = crate::split_ref_path(old_path);
let old_segment = old_segments.last().map(|s| s.as_str()).unwrap_or(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_segments = crate::split_ref_path(new_path);
let new_segment = new_segments.last().map(|s| s.as_str()).unwrap_or(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. // Pre-compute slugified forms for fallback comparison.
// We check each instance's instigating source for meta/model files. let slugified_old = if name_needs_slugify(old_segment) {
for inst_with_meta in tree.descendants(root_id) { Some(slugify_name(old_segment))
if let Some(source) = &inst_with_meta.metadata().instigating_source {
let inst_path = source.path();
// 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),
);
replaced = true;
break; // Only replace the first exact match
}
}
}
}
if replaced {
components.iter().collect::<PathBuf>()
} else { } else {
inst_path.to_path_buf() None
} };
let slugified_new = || -> String {
if name_needs_slugify(new_segment) {
slugify_name(new_segment)
} else { } else {
inst_path.to_path_buf() new_segment.to_string()
} }
}; };
// For directory-format instances, check init.meta.json5 let original_paths = files_from_index.clone();
if inst_path.is_dir() { let files_to_check: Vec<PathBuf> = files_from_index
let init_meta = inst_path.join("init.meta.json5"); .into_iter()
if init_meta.exists() { .map(|file_path| {
files_to_check.push(init_meta); 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;
continue;
}
}
}
}
}
result.push(comp);
}
if replaced {
result
} else { } else {
let file_name = inst_path.file_name().and_then(|f| f.to_str()).unwrap_or(""); file_path
// 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);
}
}
}
}
} }
})
.collect();
let mut updated_count = 0; let mut updated_count = 0;
for file_path in &files_to_check { 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 { 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!( log::info!(
"Updated Rojo_Ref_* paths in {} file(s): '{}' -> '{}'", "Updated Rojo_Ref_* paths in {} file(s): '{}' -> '{}'",
updated_count, updated_count,

View file

@ -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::{ use rbx_dom_weak::{
types::{Ref, Variant}, 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<String, HashSet<PathBuf>>,
}
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<PathBuf> {
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)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
@ -338,4 +451,118 @@ mod tests {
fn ref_target_attr_name_value() { fn ref_target_attr_name_value() {
assert_eq!(ref_target_attribute_name("Value"), "Rojo_Target_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);
}
} }

View file

@ -108,6 +108,10 @@ pub struct ServeSession {
/// the appropriate counter, each suppressed VFS event decrements it. /// the appropriate counter, each suppressed VFS event decrements it.
#[allow(dead_code)] #[allow(dead_code)]
suppressed_paths: Arc<Mutex<std::collections::HashMap<std::path::PathBuf, (usize, usize)>>>, suppressed_paths: Arc<Mutex<std::collections::HashMap<std::path::PathBuf, (usize, usize)>>>,
/// Index of meta/model files that contain `Rojo_Ref_*` attributes.
/// Shared between ApiService (writes) and ChangeProcessor (rename updates).
ref_path_index: Arc<Mutex<crate::RefPathIndex>>,
} }
impl ServeSession { impl ServeSession {
@ -153,6 +157,7 @@ impl ServeSession {
let (tree_mutation_sender, tree_mutation_receiver) = crossbeam_channel::unbounded(); let (tree_mutation_sender, tree_mutation_receiver) = crossbeam_channel::unbounded();
let suppressed_paths = Arc::new(Mutex::new(std::collections::HashMap::new())); 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"); log::trace!("Starting ChangeProcessor");
let change_processor = ChangeProcessor::start( let change_processor = ChangeProcessor::start(
@ -161,6 +166,7 @@ impl ServeSession {
Arc::clone(&message_queue), Arc::clone(&message_queue),
tree_mutation_receiver, tree_mutation_receiver,
Arc::clone(&suppressed_paths), Arc::clone(&suppressed_paths),
Arc::clone(&ref_path_index),
root_project.folder_location().to_path_buf(), root_project.folder_location().to_path_buf(),
critical_error_receiver, critical_error_receiver,
); );
@ -175,6 +181,7 @@ impl ServeSession {
tree_mutation_sender, tree_mutation_sender,
vfs, vfs,
suppressed_paths, suppressed_paths,
ref_path_index,
}) })
} }
@ -199,6 +206,12 @@ impl ServeSession {
Arc::clone(&self.suppressed_paths) 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<Mutex<crate::RefPathIndex>> {
Arc::clone(&self.ref_path_index)
}
#[allow(unused)] #[allow(unused)]
pub fn vfs(&self) -> &Vfs { pub fn vfs(&self) -> &Vfs {
&self.vfs &self.vfs

View file

@ -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) // Empty path should resolve to root (RojoTree.get_instance_by_path("") returns root)
let has_ref = patch_set.updated_instances.iter().any(|u| { let has_ref = patch_set.updated_instances.iter().any(|u| {
u.changed_properties.get(&ustr("PrimaryPart")).is_some_and( u.changed_properties
|v| matches!(v, Some(Variant::Ref(r)) if *r == root_id), .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"); assert!(has_ref, "Empty path should resolve to root instance");
} }
@ -531,11 +531,7 @@ fn ref_attr_priority_path_wins_over_target() {
.first() .first()
.copied() .copied()
.unwrap(); .unwrap();
let model_children: Vec<_> = tree let model_children: Vec<_> = tree.get_instance(model_id).unwrap().children().to_vec();
.get_instance(model_id)
.unwrap()
.children()
.to_vec();
let target_id = model_children[0]; // Target let target_id = model_children[0]; // Target
let other_id = model_children[1]; // OtherPart 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" "Rojo_Ref_* should win over Rojo_Target_* -- PrimaryPart should point to Target, not OtherPart"
); );
} }
other => panic!( other => panic!("PrimaryPart should be Some(Variant::Ref), got {:?}", other),
"PrimaryPart should be Some(Variant::Ref), got {:?}",
other
),
} }
} }

View file

@ -22,7 +22,9 @@ use rbx_dom_weak::{
use crate::{ use crate::{
serve_session::ServeSession, serve_session::ServeSession,
snapshot::{InstanceWithMeta, InstigatingSource, PatchSet, PatchUpdate}, snapshot::{
InstanceSnapshot, InstanceWithMeta, InstigatingSource, PatchAdd, PatchSet, PatchUpdate,
},
syncback::{slugify_name, VISIBLE_SERVICES}, syncback::{slugify_name, VISIBLE_SERVICES},
web::{ web::{
interface::{ interface::{
@ -163,6 +165,7 @@ pub async fn call(
pub struct ApiService { pub struct ApiService {
serve_session: Arc<ServeSession>, serve_session: Arc<ServeSession>,
suppressed_paths: Arc<Mutex<HashMap<PathBuf, (usize, usize)>>>, suppressed_paths: Arc<Mutex<HashMap<PathBuf, (usize, usize)>>>,
ref_path_index: Arc<Mutex<crate::RefPathIndex>>,
} }
/// Derives the directory name from a standalone script's filesystem path. /// 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 { impl ApiService {
pub fn new(serve_session: Arc<ServeSession>) -> Self { pub fn new(serve_session: Arc<ServeSession>) -> Self {
let suppressed_paths = serve_session.suppressed_paths(); let suppressed_paths = serve_session.suppressed_paths();
let ref_path_index = serve_session.ref_path_index();
ApiService { ApiService {
serve_session, serve_session,
suppressed_paths, suppressed_paths,
ref_path_index,
} }
} }
@ -646,9 +651,7 @@ impl ApiService {
} }
// Write non-Source properties to meta/model files // Write non-Source properties to meta/model files
if let Err(err) = if let Err(err) = self.syncback_updated_properties(update, &tree, &added_paths) {
self.syncback_updated_properties(update, &tree, &added_paths)
{
log::warn!( log::warn!(
"Failed to persist non-Source properties for instance {:?}: {}", "Failed to persist non-Source properties for instance {:?}: {}",
update.id, update.id,
@ -688,10 +691,27 @@ impl ApiService {
}) })
.collect(); .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<PatchAdd> = 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 tree_mutation_sender
.send(PatchSet { .send(PatchSet {
removed_instances: actually_removed, removed_instances: actually_removed,
added_instances: Vec::new(), added_instances,
updated_instances, updated_instances,
}) })
.unwrap(); .unwrap();
@ -699,6 +719,38 @@ impl ApiService {
msgpack_ok(WriteResponse { session_id }) 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<Variant> = 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. /// Syncback an added instance by creating a file in the filesystem.
/// The file watcher will pick up the change and update Rojo's tree. /// The file watcher will pick up the change and update Rojo's tree.
/// ///
@ -2670,8 +2722,7 @@ impl ApiService {
target_ref, target_ref,
path path
); );
ref_attributes ref_attributes.insert(attr_name, serde_json::Value::String(path.clone()));
.insert(attr_name, serde_json::Value::String(path.clone()));
} else { } else {
log::warn!( log::warn!(
"Cannot persist Ref property '{}' for instance {:?}: \ "Cannot persist Ref property '{}' for instance {:?}: \
@ -2697,6 +2748,17 @@ impl ApiService {
let (properties, mut attributes) = let (properties, mut attributes) =
self.filter_properties_for_meta(class_name.as_str(), &props, skip_prop); 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<String> = ref_attributes
.iter()
.filter_map(|(_, v)| v.as_str().map(|s| s.to_string()))
.collect();
let removed_ref_values: Vec<String> = remove_attributes
.iter()
.filter(|a| a.starts_with(crate::REF_PATH_ATTRIBUTE_PREFIX))
.cloned()
.collect();
// Merge Rojo_Ref_* attributes into the attributes map // Merge Rojo_Ref_* attributes into the attributes map
for (k, v) in ref_attributes { for (k, v) in ref_attributes {
attributes.insert(k, v); attributes.insert(k, v);
@ -2706,6 +2768,9 @@ impl ApiService {
return Ok(()); 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 // Determine which meta file to write based on file structure
if inst_path.is_dir() { if inst_path.is_dir() {
// Directory format: write to init.meta.json5 // Directory format: write to init.meta.json5
@ -2729,6 +2794,7 @@ impl ApiService {
"Syncback: Persisted non-Source properties to {}", "Syncback: Persisted non-Source properties to {}",
meta_path.display() meta_path.display()
); );
written_meta_path = meta_path;
} else if is_script { } else if is_script {
// Standalone script: write to adjacent Name.meta.json5 // Standalone script: write to adjacent Name.meta.json5
// Derive the base name from the filesystem path (not instance.name()) // Derive the base name from the filesystem path (not instance.name())
@ -2762,6 +2828,7 @@ impl ApiService {
"Syncback: Persisted non-Source properties to {}", "Syncback: Persisted non-Source properties to {}",
meta_path.display() meta_path.display()
); );
written_meta_path = meta_path;
} else { } else {
// Non-script standalone file. Only .model.json5/.model.json support // Non-script standalone file. Only .model.json5/.model.json support
// in-place JSON property updates. Other file types (.txt, .csv, .toml, // in-place JSON property updates. Other file types (.txt, .csv, .toml,
@ -2789,6 +2856,7 @@ impl ApiService {
"Syncback: Persisted non-Source properties to {}", "Syncback: Persisted non-Source properties to {}",
inst_path.display() inst_path.display()
); );
written_meta_path = inst_path.to_path_buf();
} else { } else {
// For .txt, .csv, .toml, .yaml, etc. — use adjacent meta file // For .txt, .csv, .toml, .yaml, etc. — use adjacent meta file
let parent_dir = inst_path.parent().context("No parent directory")?; let parent_dir = inst_path.parent().context("No parent directory")?;
@ -2812,6 +2880,26 @@ impl ApiService {
"Syncback: Persisted non-Source properties to {}", "Syncback: Persisted non-Source properties to {}",
meta_path.display() 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);
} }
} }

View file

@ -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. /// 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 /// The `added_paths` mechanism in `syncback_updated_properties` pre-computes
/// syncback_updated_properties runs because additions go through the VFS /// paths for instances from `request.added`, so the Rojo_Ref_PrimaryPart
/// watcher asynchronously. /// attribute is written correctly even though the new Part hasn't been
/// added to the tree by the VFS watcher yet.
#[test] #[test]
fn ref_to_instance_added_in_same_request() { fn ref_to_instance_added_in_same_request() {
run_serve_test("ref_two_way_sync", |session, _| { run_serve_test("ref_two_way_sync", |session, _| {
@ -6755,9 +6756,8 @@ fn ref_to_instance_added_in_same_request() {
// Wait for processing // Wait for processing
thread::sleep(Duration::from_millis(500)); thread::sleep(Duration::from_millis(500));
// The meta file should contain Rojo_Ref_PrimaryPart pointing to the new Part // 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 // The added_paths fallback resolves the path from the AddedInstance data.
// syncback_updated_properties processes the PrimaryPart update
let meta_path = session let meta_path = session
.path() .path()
.join("src/Workspace/TestModel/init.meta.json5"); .join("src/Workspace/TestModel/init.meta.json5");
@ -6766,6 +6766,26 @@ fn ref_to_instance_added_in_same_request() {
"Rojo_Ref_PrimaryPart", "Rojo_Ref_PrimaryPart",
"Workspace/TestModel/NewPart", "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"
);
}); });
} }