Refactor code for improved readability and consistency

- Simplified function calls and removed unnecessary line breaks in `change_processor.rs`, `rojo_ref.rs`, `syncback/meta.rs`, `syncback/mod.rs`, `syncback/ref_properties.rs`, and `web/api.rs` to enhance code clarity.
- Updated test assertions in `two_way_sync.rs` for better readability by consolidating multiple lines into single assertions.

These changes improve the overall maintainability and readability of the codebase.
This commit is contained in:
Joe 2026-02-18 04:21:50 -06:00
parent b8c3520d25
commit 6f8e5a5cf2
7 changed files with 32 additions and 88 deletions

View file

@ -497,7 +497,10 @@ impl JobThreadContext {
self.suppress_path(file_path); self.suppress_path(file_path);
match crate::syncback::meta::update_ref_paths_in_file( match crate::syncback::meta::update_ref_paths_in_file(
file_path, old_path, new_path, &source_abs, file_path,
old_path,
new_path,
&source_abs,
) { ) {
Ok(true) => { Ok(true) => {
updated_count += 1; updated_count += 1;
@ -1200,9 +1203,7 @@ impl JobThreadContext {
crate::ref_target_path_from_tree(&tree, parent_ref); crate::ref_target_path_from_tree(&tree, parent_ref);
let old_prefix = format!("{}/{}", parent_path, old_ref_segment); let old_prefix = format!("{}/{}", parent_path, old_ref_segment);
let new_prefix = format!("{}/{}", parent_path, new_ref_segment); let new_prefix = format!("{}/{}", parent_path, new_ref_segment);
self.update_ref_paths_after_rename( self.update_ref_paths_after_rename(&old_prefix, &new_prefix, &tree);
&old_prefix, &new_prefix, &tree,
);
} }
// Fix 1: Update the renamed survivor's in-memory // Fix 1: Update the renamed survivor's in-memory
@ -1812,9 +1813,7 @@ impl JobThreadContext {
segment.clone() segment.clone()
}; };
if *old_ref_path != new_ref_path { if *old_ref_path != new_ref_path {
self.update_ref_paths_after_rename( self.update_ref_paths_after_rename(old_ref_path, &new_ref_path, &tree);
old_ref_path, &new_ref_path, &tree,
);
} }
} 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 or class change was requested but no filesystem

View file

@ -271,9 +271,7 @@ pub fn resolve_ref_path_to_absolute(path: &str, source_abs: &str) -> Option<Stri
} else if let Some(rest) = path.strip_prefix("../") { } else if let Some(rest) = path.strip_prefix("../") {
let mut p: Vec<&str> = source_abs.split('/').collect(); let mut p: Vec<&str> = source_abs.split('/').collect();
p.pop(); p.pop();
if p.pop().is_none() { p.pop()?;
return None;
}
(p, rest) (p, rest)
} else { } else {
return Some(path.to_string()); return Some(path.to_string());
@ -281,9 +279,7 @@ pub fn resolve_ref_path_to_absolute(path: &str, source_abs: &str) -> Option<Stri
for segment in rest.split('/') { for segment in rest.split('/') {
if segment == ".." { if segment == ".." {
if parts.pop().is_none() { parts.pop()?;
return None;
}
} else if !segment.is_empty() { } else if !segment.is_empty() {
parts.push(segment); parts.push(segment);
} }
@ -401,11 +397,7 @@ impl RefPathIndex {
/// Relative paths (prefixed with `@self/`, `./`, `../`) are resolved to /// Relative paths (prefixed with `@self/`, `./`, `../`) are resolved to
/// absolute using `tree` so the index always stores absolute target paths. /// absolute using `tree` so the index always stores absolute target paths.
/// This ensures prefix-based lookup works correctly for rename updates. /// This ensures prefix-based lookup works correctly for rename updates.
pub fn populate_from_dir( pub fn populate_from_dir(&mut self, root: &Path, tree: &crate::snapshot::RojoTree) {
&mut self,
root: &Path,
tree: &crate::snapshot::RojoTree,
) {
let mut count = 0usize; let mut count = 0usize;
let mut dirs_to_visit = vec![root.to_path_buf()]; let mut dirs_to_visit = vec![root.to_path_buf()];
while let Some(dir) = dirs_to_visit.pop() { while let Some(dir) = dirs_to_visit.pop() {
@ -893,10 +885,7 @@ mod tests {
#[test] #[test]
fn relative_self_slugified_child() { fn relative_self_slugified_child() {
assert_eq!( assert_eq!(
compute_relative_ref_path( compute_relative_ref_path("Workspace/Model", "Workspace/Model/Hey_Bro.server.luau"),
"Workspace/Model",
"Workspace/Model/Hey_Bro.server.luau"
),
"@self/Hey_Bro.server.luau" "@self/Hey_Bro.server.luau"
); );
} }
@ -1290,10 +1279,7 @@ mod tests {
#[test] #[test]
fn relative_edge_service_self_ref() { fn relative_edge_service_self_ref() {
assert_eq!( assert_eq!(compute_relative_ref_path("Workspace", "Workspace"), "@self");
compute_relative_ref_path("Workspace", "Workspace"),
"@self"
);
} }
#[test] #[test]
@ -1424,10 +1410,7 @@ mod tests {
#[test] #[test]
fn resolve_abs_bare_path_legacy() { fn resolve_abs_bare_path_legacy() {
assert_eq!( assert_eq!(
resolve_ref_path_to_absolute( resolve_ref_path_to_absolute("Workspace/Model/Handle.model.json5", "anything/here"),
"Workspace/Model/Handle.model.json5",
"anything/here"
),
Some("Workspace/Model/Handle.model.json5".to_string()) Some("Workspace/Model/Handle.model.json5".to_string())
); );
} }
@ -1442,10 +1425,7 @@ mod tests {
#[test] #[test]
fn resolve_abs_dotdot_above_root_returns_none() { fn resolve_abs_dotdot_above_root_returns_none() {
assert_eq!( assert_eq!(resolve_ref_path_to_absolute("../../X", "Workspace"), None);
resolve_ref_path_to_absolute("../../X", "Workspace"),
None
);
} }
#[test] #[test]

View file

@ -135,16 +135,12 @@ pub fn update_ref_paths_in_file(
let Some(path_str) = value.as_str() else { let Some(path_str) = value.as_str() else {
continue; continue;
}; };
let Some(resolved) = let Some(resolved) = crate::resolve_ref_path_to_absolute(path_str, source_abs) else {
crate::resolve_ref_path_to_absolute(path_str, source_abs)
else {
continue; continue;
}; };
if resolved == old_prefix || resolved.starts_with(&old_prefix_slash) { if resolved == old_prefix || resolved.starts_with(&old_prefix_slash) {
let new_abs = let new_abs = format!("{new_prefix}{}", &resolved[old_prefix.len()..]);
format!("{new_prefix}{}", &resolved[old_prefix.len()..]); let new_relative = crate::compute_relative_ref_path(source_abs, &new_abs);
let new_relative =
crate::compute_relative_ref_path(source_abs, &new_abs);
*value = serde_json::Value::String(new_relative); *value = serde_json::Value::String(new_relative);
updated = true; updated = true;
} }

View file

@ -175,8 +175,7 @@ pub fn syncback_loop_with_stats(
log::debug!("Collecting referents for new DOM..."); log::debug!("Collecting referents for new DOM...");
let mut deferred_referents = collect_referents(&new_tree, &pre_prune_paths, None); let mut deferred_referents = collect_referents(&new_tree, &pre_prune_paths, None);
let placeholder_map = let placeholder_map = std::mem::take(&mut deferred_referents.placeholder_to_source_and_target);
std::mem::take(&mut deferred_referents.placeholder_to_source_and_target);
// Remove any properties that are manually blocked from syncback via the // Remove any properties that are manually blocked from syncback via the
// project file. // project file.

View file

@ -494,8 +494,7 @@ mod tests {
let links = collect_referents(&dom2, &HashMap::new(), None); let links = collect_referents(&dom2, &HashMap::new(), None);
let placeholders: Vec<&String> = let placeholders: Vec<&String> = links.placeholder_to_source_and_target.keys().collect();
links.placeholder_to_source_and_target.keys().collect();
assert!( assert!(
placeholders.len() >= 2, placeholders.len() >= 2,

View file

@ -2693,10 +2693,8 @@ impl ApiService {
if target_ref.is_none() { if target_ref.is_none() {
remove_attributes.push(attr_name); remove_attributes.push(attr_name);
} else if tree.get_instance(*target_ref).is_some() { } else if tree.get_instance(*target_ref).is_some() {
let source_abs = let source_abs = crate::ref_target_path_from_tree(tree, update.id);
crate::ref_target_path_from_tree(tree, update.id); let target_abs = crate::ref_target_path_from_tree(tree, *target_ref);
let target_abs =
crate::ref_target_path_from_tree(tree, *target_ref);
if !Self::is_ref_path_unique(tree, *target_ref) { if !Self::is_ref_path_unique(tree, *target_ref) {
log::warn!( log::warn!(
@ -2709,15 +2707,11 @@ impl ApiService {
); );
} }
let relative = let relative = crate::compute_relative_ref_path(&source_abs, &target_abs);
crate::compute_relative_ref_path(&source_abs, &target_abs); ref_attributes.insert(attr_name, serde_json::Value::String(relative));
ref_attributes
.insert(attr_name, serde_json::Value::String(relative));
} else if let Some(target_abs) = added_paths.get(target_ref) { } else if let Some(target_abs) = added_paths.get(target_ref) {
let source_abs = let source_abs = crate::ref_target_path_from_tree(tree, update.id);
crate::ref_target_path_from_tree(tree, update.id); let relative = crate::compute_relative_ref_path(&source_abs, target_abs);
let relative =
crate::compute_relative_ref_path(&source_abs, target_abs);
log::info!( log::info!(
"Ref property '{}' for instance {:?}: target {:?} is a \ "Ref property '{}' for instance {:?}: target {:?} is a \
just-added instance, computed relative path '{}'", just-added instance, computed relative path '{}'",
@ -2726,8 +2720,7 @@ impl ApiService {
target_ref, target_ref,
relative relative
); );
ref_attributes ref_attributes.insert(attr_name, serde_json::Value::String(relative));
.insert(attr_name, serde_json::Value::String(relative));
} else { } else {
log::warn!( log::warn!(
"Cannot persist Ref property '{}' for instance {:?}: \ "Cannot persist Ref property '{}' for instance {:?}: \
@ -2895,10 +2888,8 @@ impl ApiService {
if key.starts_with(crate::REF_PATH_ATTRIBUTE_PREFIX) { if key.starts_with(crate::REF_PATH_ATTRIBUTE_PREFIX) {
if let Some(path_str) = value.as_str() { if let Some(path_str) = value.as_str() {
let resolved = let resolved =
crate::resolve_ref_path_to_absolute( crate::resolve_ref_path_to_absolute(path_str, &source_abs)
path_str, &source_abs, .unwrap_or_else(|| path_str.to_string());
)
.unwrap_or_else(|| path_str.to_string());
index.add(&resolved, &written_meta_path); index.add(&resolved, &written_meta_path);
} }
} }

View file

@ -6423,11 +6423,7 @@ fn ref_with_name_change() {
poll_file_exists(&new_meta, "Meta file should exist at new location"); poll_file_exists(&new_meta, "Meta file should exist at new location");
// The ref path should be updated to reflect the renamed parent // The ref path should be updated to reflect the renamed parent
assert_meta_has_ref_attr( assert_meta_has_ref_attr(&new_meta, "Rojo_Ref_PrimaryPart", "@self/Part1.model.json5");
&new_meta,
"Rojo_Ref_PrimaryPart",
"@self/Part1.model.json5",
);
}); });
} }
@ -6859,11 +6855,7 @@ fn ref_ambiguous_path_no_crash() {
let meta_path = session let meta_path = session
.path() .path()
.join("src/Workspace/DupParent/init.meta.json5"); .join("src/Workspace/DupParent/init.meta.json5");
poll_meta_has_ref_attr( poll_meta_has_ref_attr(&meta_path, "Rojo_Ref_PrimaryPart", "./Target.model.json5");
&meta_path,
"Rojo_Ref_PrimaryPart",
"./Target.model.json5",
);
}); });
} }
@ -7024,11 +7016,7 @@ fn ref_partial_removal_preserves_remaining_index_entry() {
"@self/Part1.model.json5", "@self/Part1.model.json5",
); );
// Verify second ref is there too // Verify second ref is there too
assert_meta_has_ref_attr( assert_meta_has_ref_attr(&meta_path, "Rojo_Ref_Adornee", "@self/Part2.model.json5");
&meta_path,
"Rojo_Ref_Adornee",
"@self/Part2.model.json5",
);
// Step 2: Remove PrimaryPart (set to nil), keep Adornee // Step 2: Remove PrimaryPart (set to nil), keep Adornee
let mut props2 = UstrMap::default(); let mut props2 = UstrMap::default();
@ -7048,11 +7036,7 @@ fn ref_partial_removal_preserves_remaining_index_entry() {
thread::sleep(Duration::from_millis(300)); thread::sleep(Duration::from_millis(300));
assert_meta_no_ref_attr(&meta_path, "Rojo_Ref_PrimaryPart"); assert_meta_no_ref_attr(&meta_path, "Rojo_Ref_PrimaryPart");
// Adornee should still be there // Adornee should still be there
assert_meta_has_ref_attr( assert_meta_has_ref_attr(&meta_path, "Rojo_Ref_Adornee", "@self/Part2.model.json5");
&meta_path,
"Rojo_Ref_Adornee",
"@self/Part2.model.json5",
);
// Step 3: Rename Part2. The RefPathIndex should still have the entry // Step 3: Rename Part2. The RefPathIndex should still have the entry
// for Adornee (not overbroad-removed when PrimaryPart was deleted). // for Adornee (not overbroad-removed when PrimaryPart was deleted).
@ -7350,11 +7334,7 @@ fn ref_same_batch_add_module_target_includes_extension() {
let meta_path = session let meta_path = session
.path() .path()
.join("src/Workspace/TestModel/init.meta.json5"); .join("src/Workspace/TestModel/init.meta.json5");
poll_meta_has_ref_attr( poll_meta_has_ref_attr(&meta_path, "Rojo_Ref_PrimaryPart", "./ModuleTarget.luau");
&meta_path,
"Rojo_Ref_PrimaryPart",
"./ModuleTarget.luau",
);
}); });
} }