Refactor syncback instance handling for improved readability

- Consolidated function calls in `syncback_instance_to_path_with_stats` across multiple files for better clarity and maintainability.
- Streamlined the formatting of function arguments to enhance code readability in `api.rs`, `file_names.rs`, and `project.rs`.
- Updated tests to reflect changes in function call structure while ensuring existing functionality remains intact.
This commit is contained in:
Joe 2026-02-10 03:34:08 -06:00
parent 560fce1352
commit 7a090b104f
4 changed files with 185 additions and 118 deletions

View file

@ -10,10 +10,7 @@ use rbx_dom_weak::{
use serde::{Deserialize, Serialize};
use crate::{
json,
resolution::UnresolvedValue,
snapshot::InstanceSnapshot,
syncback::SyncbackSnapshot,
json, resolution::UnresolvedValue, snapshot::InstanceSnapshot, syncback::SyncbackSnapshot,
RojoRef,
};

View file

@ -690,7 +690,7 @@ pub fn syncback_project<'sync>(
if parent_middleware != Middleware::Project {
let taken_names = taken_names_per_dir
.entry(parent_path.clone())
.or_insert_with(std::collections::HashSet::new);
.or_default();
let (child_snap, _needs_meta, dedup_key) = snapshot.with_base_path(
&parent_path,
new_child.referent(),

View file

@ -74,7 +74,11 @@ pub fn name_for_inst<'a>(
let deduped = deduplicate_name(&base, taken_names);
let needs_meta = needs_slugify || deduped != base;
let dedup_key = deduped.clone();
Ok((Cow::Owned(format!("{deduped}.{extension}")), needs_meta, dedup_key))
Ok((
Cow::Owned(format!("{deduped}.{extension}")),
needs_meta,
dedup_key,
))
}
}
}
@ -703,7 +707,8 @@ mod tests {
let child = dom.get_by_ref(child_ref).unwrap();
let taken = HashSet::new();
let (filename, needs_meta, _dk) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap();
let (filename, needs_meta, _dk) =
name_for_inst(Middleware::Dir, child, None, &taken).unwrap();
assert_eq!(filename.as_ref(), "MyFolder");
assert!(!needs_meta);
}
@ -728,7 +733,8 @@ mod tests {
let child = dom.get_by_ref(child_ref).unwrap();
let taken = HashSet::new();
let (filename, needs_meta, _dk) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap();
let (filename, needs_meta, _dk) =
name_for_inst(Middleware::Dir, child, None, &taken).unwrap();
assert_eq!(filename.as_ref(), "Hey_Bro");
assert!(needs_meta);
}
@ -753,7 +759,8 @@ mod tests {
let child = dom.get_by_ref(child_ref).unwrap();
let taken: HashSet<String> = ["stuff".to_string()].into_iter().collect();
let (filename, needs_meta, _dk) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap();
let (filename, needs_meta, _dk) =
name_for_inst(Middleware::Dir, child, None, &taken).unwrap();
assert_eq!(filename.as_ref(), "Stuff~1");
assert!(needs_meta);
}
@ -805,7 +812,8 @@ mod tests {
let child = dom.get_by_ref(child_ref).unwrap();
let taken = HashSet::new();
let (filename, needs_meta, _dk) = name_for_inst(Middleware::Text, child, None, &taken).unwrap();
let (filename, needs_meta, _dk) =
name_for_inst(Middleware::Text, child, None, &taken).unwrap();
assert_eq!(filename.as_ref(), "Readme.txt");
assert!(!needs_meta);
}
@ -1450,17 +1458,13 @@ mod tests {
new_children: &[(&str, Middleware)],
) -> Vec<(String, String, bool)> {
// Seed from bare slugs (simulates tree-based seeding)
let mut taken: HashSet<String> = disk_entries
.iter()
.map(|e| e.to_lowercase())
.collect();
let mut taken: HashSet<String> = disk_entries.iter().map(|e| e.to_lowercase()).collect();
let mut results = Vec::new();
for &(name, mw) in new_children {
let dom = make_inst(name, "ModuleScript");
let child_ref = dom.root().children()[0];
let child = dom.get_by_ref(child_ref).unwrap();
let (filename, needs_meta, dedup_key) =
name_for_inst(mw, child, None, &taken).unwrap();
let (filename, needs_meta, dedup_key) = name_for_inst(mw, child, None, &taken).unwrap();
taken.insert(dedup_key.to_lowercase());
results.push((name.to_string(), filename.into_owned(), needs_meta));
}
@ -1471,10 +1475,7 @@ mod tests {
fn disk_seed_prevents_overwrite_of_existing_dir() {
// "Utils" directory exists on disk. New instance "Utils" (dir)
// must get "Utils~1".
let r = process_new_children_with_disk_seed(
&["utils"],
&[("Utils", Middleware::Dir)],
);
let r = process_new_children_with_disk_seed(&["utils"], &[("Utils", Middleware::Dir)]);
assert_eq!(r[0].1, "Utils~1");
assert!(r[0].2);
}
@ -1482,10 +1483,7 @@ mod tests {
#[test]
fn disk_seed_prevents_overwrite_of_existing_dir_case_insensitive() {
// "SCRIPTS" directory on disk, new instance "Scripts" (dir) must dedup.
let r = process_new_children_with_disk_seed(
&["scripts"],
&[("Scripts", Middleware::Dir)],
);
let r = process_new_children_with_disk_seed(&["scripts"], &[("Scripts", Middleware::Dir)]);
assert_eq!(r[0].1, "Scripts~1");
assert!(r[0].2);
}
@ -1494,10 +1492,7 @@ mod tests {
fn disk_seed_slug_collision_with_existing_dir() {
// "Hey_Bro" directory on disk. New instance "Hey/Bro" slugifies
// to "Hey_Bro" — must dedup to "Hey_Bro~1".
let r = process_new_children_with_disk_seed(
&["hey_bro"],
&[("Hey/Bro", Middleware::Dir)],
);
let r = process_new_children_with_disk_seed(&["hey_bro"], &[("Hey/Bro", Middleware::Dir)]);
assert_eq!(r[0].1, "Hey_Bro~1");
assert!(r[0].2);
}
@ -1553,7 +1548,10 @@ mod tests {
// child correctly detects the collision and gets ~1.
let r = process_new_children_with_disk_seed(
&[],
&[("A/B", Middleware::ModuleScript), ("A:B", Middleware::ModuleScript)],
&[
("A/B", Middleware::ModuleScript),
("A:B", Middleware::ModuleScript),
],
);
assert_eq!(r[0].1, "A_B.luau");
assert!(r[0].2);
@ -1596,10 +1594,7 @@ mod tests {
fn disk_seed_dedup_suffix_dir_already_on_disk() {
// Pathological: "Foo" AND "Foo~1" directories already on disk.
// New "Foo" dir must skip both and land on "Foo~2".
let r = process_new_children_with_disk_seed(
&["foo", "foo~1"],
&[("Foo", Middleware::Dir)],
);
let r = process_new_children_with_disk_seed(&["foo", "foo~1"], &[("Foo", Middleware::Dir)]);
assert_eq!(r[0].1, "Foo~2");
assert!(r[0].2);
}
@ -1632,17 +1627,13 @@ mod tests {
disk_entries: &[&str],
siblings: &[(&str, Middleware)],
) -> Vec<(String, String, bool)> {
let mut taken: HashSet<String> = disk_entries
.iter()
.map(|e| e.to_lowercase())
.collect();
let mut taken: HashSet<String> = disk_entries.iter().map(|e| e.to_lowercase()).collect();
let mut results = Vec::new();
for &(name, mw) in siblings {
let dom = make_inst(name, "ModuleScript");
let child_ref = dom.root().children()[0];
let child = dom.get_by_ref(child_ref).unwrap();
let (filename, needs_meta, dedup_key) =
name_for_inst(mw, child, None, &taken).unwrap();
let (filename, needs_meta, dedup_key) = name_for_inst(mw, child, None, &taken).unwrap();
taken.insert(dedup_key.to_lowercase());
results.push((name.to_string(), filename.into_owned(), needs_meta));
}
@ -1654,10 +1645,8 @@ mod tests {
// Two new project children: "A/B" and "A:B" both slug to "A_B".
// Before fix: both got "A_B" path (collision!).
// After fix: second gets "A_B~1".
let r = process_project_siblings(
&[],
&[("A/B", Middleware::Dir), ("A:B", Middleware::Dir)],
);
let r =
process_project_siblings(&[], &[("A/B", Middleware::Dir), ("A:B", Middleware::Dir)]);
assert_eq!(r[0].1, "A_B");
assert!(r[0].2);
assert_eq!(r[1].1, "A_B~1");
@ -1670,10 +1659,7 @@ mod tests {
// Dir middleware: bare slug IS the filename → dedup works.
let r = process_project_siblings(
&[],
&[
("Hey_Bro", Middleware::Dir),
("Hey/Bro", Middleware::Dir),
],
&[("Hey_Bro", Middleware::Dir), ("Hey/Bro", Middleware::Dir)],
);
assert_eq!(r[0].1, "Hey_Bro");
assert!(!r[0].2);
@ -1704,10 +1690,7 @@ mod tests {
// Second → "Helper~2" (both disk and first sibling taken).
let r = process_project_siblings(
&["helper"],
&[
("Helper", Middleware::Dir),
("Helper", Middleware::Dir),
],
&[("Helper", Middleware::Dir), ("Helper", Middleware::Dir)],
);
assert_eq!(r[0].1, "Helper~1");
assert!(r[0].2);
@ -1850,9 +1833,8 @@ mod tests {
// Every Windows reserved name as a sibling — all get "_" suffix,
// and they should NOT collide with each other since the base names differ.
let reserved = [
"CON", "PRN", "AUX", "NUL",
"COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9",
"LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9",
"CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7",
"COM8", "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9",
];
let r = process_siblings_dir(&reserved);
let names: HashSet<String> = r.iter().map(|(_, f, _)| f.to_lowercase()).collect();
@ -1877,9 +1859,7 @@ mod tests {
.iter()
.map(|s| format!("foo{s}"))
.collect();
let r = process_siblings_dir(
&siblings.iter().map(|s| s.as_str()).collect::<Vec<_>>(),
);
let r = process_siblings_dir(&siblings.iter().map(|s| s.as_str()).collect::<Vec<_>>());
let filenames: HashSet<String> = r.iter().map(|(_, f, _)| f.to_lowercase()).collect();
assert_eq!(
filenames.len(),
@ -2021,9 +2001,9 @@ mod tests {
let r = process_siblings_dir(&names);
assert_eq!(r[0].1, "Script");
assert!(!r[0].2);
for i in 1..100 {
assert_eq!(r[i].1, format!("Script~{i}"));
assert!(r[i].2);
for (i, entry) in r.iter().enumerate().skip(1) {
assert_eq!(entry.1, format!("Script~{i}"));
assert!(entry.2);
}
// All filenames must be unique
let unique: HashSet<String> = r.iter().map(|(_, f, _)| f.to_lowercase()).collect();
@ -2067,14 +2047,32 @@ mod tests {
// Process a massive batch of problematic names and verify
// every resulting filename is unique (case-insensitive).
let names = [
"Foo", "foo", "FOO", "Foo", "Foo",
"A/B", "A:B", "A*B", "A_B",
"CON", "CON_", "con", "Con",
"test.server", "test_server", "test/server",
"", "", "",
" X", "X", "X ",
"Hello\x00", "Hello_",
"日本語", "日本語",
"Foo",
"foo",
"FOO",
"Foo",
"Foo",
"A/B",
"A:B",
"A*B",
"A_B",
"CON",
"CON_",
"con",
"Con",
"test.server",
"test_server",
"test/server",
"",
"",
"",
" X",
"X",
"X ",
"Hello\x00",
"Hello_",
"日本語",
"日本語",
];
let r = process_siblings_dir(&names);
let lowered: Vec<String> = r.iter().map(|(_, f, _)| f.to_lowercase()).collect();
@ -2091,13 +2089,32 @@ mod tests {
fn invariant_every_filename_passes_validation() {
// Every filename produced by the pipeline must pass validate_file_name.
let names = [
"CON", "PRN", "NUL", "COM1", "LPT9",
"foo.server", "bar.meta", "baz.model",
"A/B", "C:D", "E*F", "G?H",
"<init>", "a|b", "x\\y", "q\"r",
"", " ", ".", "..",
"hello.", "hello ", " hello",
"~1", "~~", "a~b~c",
"CON",
"PRN",
"NUL",
"COM1",
"LPT9",
"foo.server",
"bar.meta",
"baz.model",
"A/B",
"C:D",
"E*F",
"G?H",
"<init>",
"a|b",
"x\\y",
"q\"r",
"",
" ",
".",
"..",
"hello.",
"hello ",
" hello",
"~1",
"~~",
"a~b~c",
];
let r = process_siblings_dir(&names);
for (original, filename, _) in &r {
@ -2113,9 +2130,17 @@ mod tests {
// For new instances: needs_meta is true IFF the filesystem name
// differs from the instance name. This is the contract.
let cases = [
"Clean", "Hello World", "test-123",
"A/B", "CON", "foo.server", " X", "X ", "",
"Foo~1", "hello.",
"Clean",
"Hello World",
"test-123",
"A/B",
"CON",
"foo.server",
" X",
"X ",
"",
"Foo~1",
"hello.",
];
for &name in &cases {
let dom = make_inst(name, "Folder");
@ -2126,12 +2151,14 @@ mod tests {
name_for_inst(Middleware::Dir, child, None, &taken).unwrap();
if needs_meta {
assert_ne!(
filename.as_ref(), name,
filename.as_ref(),
name,
"needs_meta=true but filename matches name for {name:?}"
);
} else {
assert_eq!(
filename.as_ref(), name,
filename.as_ref(),
name,
"needs_meta=false but filename differs from name for {name:?}"
);
}

View file

@ -1078,7 +1078,12 @@ impl ApiService {
// This will use detect_existing_script_format to check existing files
let child_slugs = Self::compute_sibling_slugs(&unique_children);
for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &children_dir, stats, &child_slugs)?;
self.syncback_instance_to_path_with_stats(
child,
&children_dir,
stats,
&child_slugs,
)?;
}
}
}
@ -1104,7 +1109,12 @@ impl ApiService {
let child_slugs = Self::compute_sibling_slugs(&unique_children);
for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, existing_path, stats, &child_slugs)?;
self.syncback_instance_to_path_with_stats(
child,
existing_path,
stats,
&child_slugs,
)?;
}
}
} else {
@ -1735,7 +1745,12 @@ impl ApiService {
self.write_script_meta_json_if_needed(&dir_path, added, meta_name_field)?;
log::info!("Syncback: Updated ModuleScript at {}", init_path.display());
for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?;
self.syncback_instance_to_path_with_stats(
child,
&dir_path,
stats,
&child_slugs,
)?;
}
} else {
let file_path = parent_dir.join(format!("{}.luau", encoded_name));
@ -1792,7 +1807,12 @@ impl ApiService {
self.write_script_meta_json_if_needed(&dir_path, added, meta_name_field)?;
log::info!("Syncback: Updated Script at {}", init_path.display());
for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?;
self.syncback_instance_to_path_with_stats(
child,
&dir_path,
stats,
&child_slugs,
)?;
}
} else {
let file_path =
@ -1849,7 +1869,12 @@ impl ApiService {
self.write_script_meta_json_if_needed(&dir_path, added, meta_name_field)?;
log::info!("Syncback: Updated LocalScript at {}", init_path.display());
for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?;
self.syncback_instance_to_path_with_stats(
child,
&dir_path,
stats,
&child_slugs,
)?;
}
} else {
let file_path = parent_dir.join(format!("{}.local.luau", encoded_name));
@ -1900,7 +1925,12 @@ impl ApiService {
// Recursively process children
for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?;
self.syncback_instance_to_path_with_stats(
child,
&dir_path,
stats,
&child_slugs,
)?;
}
}
@ -1919,7 +1949,12 @@ impl ApiService {
dir_path.display()
);
for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?;
self.syncback_instance_to_path_with_stats(
child,
&dir_path,
stats,
&child_slugs,
)?;
}
} else {
let value = added
@ -1961,7 +1996,12 @@ impl ApiService {
dir_path.display()
);
for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?;
self.syncback_instance_to_path_with_stats(
child,
&dir_path,
stats,
&child_slugs,
)?;
}
} else {
let file_path = parent_dir.join(format!("{}.csv", encoded_name));
@ -2015,7 +2055,12 @@ impl ApiService {
// Recursively process children
for child in &unique_children {
self.syncback_instance_to_path_with_stats(child, &dir_path, stats, &child_slugs)?;
self.syncback_instance_to_path_with_stats(
child,
&dir_path,
stats,
&child_slugs,
)?;
}
} else {
let content = self.serialize_instance_to_model_json(added)?;
@ -6189,10 +6234,7 @@ mod tests {
#[test]
fn batch_dir_with_existing_and_collisions() {
let r = simulate_dir_batch(
&["Utils", "Config"],
&["Utils", "Config", "NewThing"],
);
let r = simulate_dir_batch(&["Utils", "Config"], &["Utils", "Config", "NewThing"]);
assert_eq!(r[0].0, "Utils~1");
assert_eq!(r[1].0, "Config~1");
assert_eq!(r[2].0, "NewThing");
@ -6201,10 +6243,7 @@ mod tests {
#[test]
fn batch_dir_dangerous_then_natural_then_slug() {
let r = simulate_dir_batch(
&[],
&["foo.server", "foo_server", "foo/server"],
);
let r = simulate_dir_batch(&[], &["foo.server", "foo_server", "foo/server"]);
assert_eq!(r[0].0, "foo_server");
assert_eq!(r[1].0, "foo_server~1");
assert_eq!(r[2].0, "foo_server~2");
@ -6234,8 +6273,8 @@ mod tests {
let children: Vec<&str> = vec!["Spam"; 20];
let r = simulate_dir_batch(&[], &children);
assert_eq!(r[0].0, "Spam");
for i in 1..20 {
assert_eq!(r[i].0, format!("Spam~{i}"));
for (i, entry) in r.iter().enumerate().skip(1) {
assert_eq!(entry.0, format!("Spam~{i}"));
}
let unique: HashSet<String> = r.iter().map(|(f, _)| f.to_lowercase()).collect();
assert_eq!(unique.len(), 20);
@ -6339,8 +6378,7 @@ mod tests {
let r = simulate_dir_batch(
&["A_B"],
&[
"A/B", "A:B", "A*B", "A?B", "A<B", "A>B", "A|B",
"A\\B", "A\"B",
"A/B", "A:B", "A*B", "A?B", "A<B", "A>B", "A|B", "A\\B", "A\"B",
],
);
assert_eq!(r[0].0, "A_B~1");
@ -6356,10 +6394,7 @@ mod tests {
#[test]
fn batch_dir_con_prn_nul_then_slugged() {
let r = simulate_dir_batch(
&[],
&["CON", "PRN", "CON/", "PRN/"],
);
let r = simulate_dir_batch(&[], &["CON", "PRN", "CON/", "PRN/"]);
assert_eq!(r[0].0, "CON_");
assert_eq!(r[1].0, "PRN_");
assert_eq!(r[2].0, "CON_~1"); // "CON/" slug "CON_" collides
@ -6368,10 +6403,7 @@ mod tests {
#[test]
fn batch_dir_unicode_plus_forbidden() {
let r = simulate_dir_batch(
&[],
&["カフェ/Bar", "カフェ:Bar", "カフェ_Bar"],
);
let r = simulate_dir_batch(&[], &["カフェ/Bar", "カフェ:Bar", "カフェ_Bar"]);
assert_eq!(r[0].0, "カフェ_Bar");
assert_eq!(r[1].0, "カフェ_Bar~1");
assert_eq!(r[2].0, "カフェ_Bar~2");
@ -6381,13 +6413,24 @@ mod tests {
fn batch_dir_all_unique_filenames_invariant() {
// A massive batch — every result must be unique.
let children = &[
"Foo", "foo", "FOO", "A/B", "A:B", "A_B",
"CON", "CON_", "", "", "test.server", "test_server",
"Foo",
"foo",
"FOO",
"A/B",
"A:B",
"A_B",
"CON",
"CON_",
"",
"",
"test.server",
"test_server",
];
let r = simulate_dir_batch(&[], children);
let unique: HashSet<String> = r.iter().map(|(f, _)| f.to_lowercase()).collect();
assert_eq!(
unique.len(), r.len(),
unique.len(),
r.len(),
"every sibling must get a unique filename"
);
}