Enhance slug deduplication and metadata handling in syncback process

- Introduced a new method `dedup_against_dir` to deduplicate slugified names against existing entries in a parent directory, improving collision handling during file operations.
- Updated the `syncback_added_instance` method to utilize sibling slug sets, ensuring new instances do not overwrite existing files or directories.
- Refactored `strip_middleware_extension` to be publicly accessible for broader use in deduplication logic.
- Improved comments and code organization for better clarity in the slugification and deduplication processes.
This commit is contained in:
Joe 2026-02-10 18:09:53 -06:00
parent f38c1f1311
commit 7d47fe73f6
4 changed files with 238 additions and 53 deletions

View file

@ -15,7 +15,7 @@ use crate::{
apply_patch_set, compute_patch_set, AppliedPatchSet, InstigatingSource, PatchSet, RojoTree, apply_patch_set, compute_patch_set, AppliedPatchSet, InstigatingSource, PatchSet, RojoTree,
}, },
snapshot_middleware::{snapshot_from_vfs, snapshot_project_node}, snapshot_middleware::{snapshot_from_vfs, snapshot_project_node},
syncback::{name_needs_slugify, slugify_name}, syncback::{deduplicate_name, name_needs_slugify, slugify_name},
}; };
/// Processes file change events, updates the DOM, and sends those updates /// Processes file change events, updates the DOM, and sends those updates
@ -311,6 +311,42 @@ impl JobThreadContext {
} }
} }
/// Deduplicate a slugified name against existing entries in a parent
/// directory. Reads the directory, strips entries to bare slugs (file_stem),
/// and uses `deduplicate_name` to find a collision-free name.
fn dedup_against_dir(slug: &str, parent_dir: &Path, exclude: Option<&str>) -> String {
let mut taken = std::collections::HashSet::new();
if let Ok(entries) = fs::read_dir(parent_dir) {
for entry in entries.flatten() {
let name = entry.file_name();
let name_str = name.to_string_lossy();
// Use file_stem as a rough bare-slug approximation.
// For directories this is the dir name itself; for files
// it strips the last extension (e.g., "foo.server" from
// "foo.server.luau"). This is conservative: it may over-
// dedup in rare cases (compound extensions) but never
// under-dedup.
let stem = if entry.path().is_dir() {
name_str.to_string()
} else {
std::path::Path::new(name_str.as_ref())
.file_stem()
.and_then(|s| s.to_str())
.unwrap_or(&name_str)
.to_string()
};
// Exclude the entry being renamed (it will be gone after rename)
if let Some(ex) = exclude {
if name_str == ex {
continue;
}
}
taken.insert(stem.to_lowercase());
}
}
deduplicate_name(slug, &taken)
}
/// Computes and applies patches to the DOM for a given file path. /// Computes and applies patches to the DOM for a given file path.
/// ///
/// This function finds the nearest ancestor to the given path that has associated instances /// This function finds the nearest ancestor to the given path that has associated instances
@ -723,12 +759,22 @@ impl JobThreadContext {
.and_then(|f| f.to_str()) .and_then(|f| f.to_str())
.unwrap_or(""); .unwrap_or("");
// Slugify the new name for filesystem safety // Slugify the new name for filesystem safety
let slugified_new_name = let mut slugified_new_name =
if name_needs_slugify(new_name) { if name_needs_slugify(new_name) {
slugify_name(new_name) slugify_name(new_name)
} else { } else {
new_name.to_string() new_name.to_string()
}; };
// Dedup against siblings to avoid overwriting
// an existing directory at the target path
let candidate = grandparent.join(&slugified_new_name);
if candidate.exists() && candidate != dir_path {
slugified_new_name = Self::dedup_against_dir(
&slugified_new_name,
grandparent,
dir_path.file_name().and_then(|s| s.to_str()),
);
}
let new_dir_path = let new_dir_path =
grandparent.join(&slugified_new_name); grandparent.join(&slugified_new_name);
@ -816,12 +862,33 @@ impl JobThreadContext {
&stem[..stem.len() - script_suffix.len()] &stem[..stem.len() - script_suffix.len()]
}; };
let slugified_new_name = if name_needs_slugify(new_name) let mut slugified_new_name =
{ if name_needs_slugify(new_name) {
slugify_name(new_name) slugify_name(new_name)
} else {
new_name.to_string()
};
// Dedup against siblings to avoid overwriting
// an existing file at the target path
let test_file_name = if extension.is_empty() {
format!("{}{}", slugified_new_name, script_suffix)
} else { } else {
new_name.to_string() format!(
"{}{}.{}",
slugified_new_name, script_suffix, extension
)
}; };
let test_path = parent.join(&test_file_name);
if test_path.exists() && test_path != *path {
let old_filename = path
.file_name()
.and_then(|s| s.to_str());
slugified_new_name = Self::dedup_against_dir(
&slugified_new_name,
parent,
old_filename,
);
}
let new_file_name = if extension.is_empty() { let new_file_name = if extension.is_empty() {
format!("{}{}", slugified_new_name, script_suffix) format!("{}{}", slugified_new_name, script_suffix)
} else { } else {

View file

@ -85,8 +85,9 @@ pub fn name_for_inst<'a>(
} }
/// Strips the middleware extension from a filename to recover the bare slug. /// Strips the middleware extension from a filename to recover the bare slug.
/// Used internally by `name_for_inst` to derive the dedup key for old instances. /// Used by `name_for_inst` to derive the dedup key for old instances, and by
fn strip_middleware_extension(filename: &str, middleware: Middleware) -> String { /// callers that need to seed `taken_names` from filesystem paths.
pub fn strip_middleware_extension(filename: &str, middleware: Middleware) -> String {
// Dir middleware has no extension — filename IS the slug // Dir middleware has no extension — filename IS the slug
match middleware { match middleware {
Middleware::Dir Middleware::Dir

View file

@ -31,7 +31,7 @@ use crate::{
pub use file_names::{ pub use file_names::{
deduplicate_name, extension_for_middleware, name_for_inst, name_needs_slugify, slugify_name, deduplicate_name, extension_for_middleware, name_for_inst, name_needs_slugify, slugify_name,
validate_file_name, strip_middleware_extension, validate_file_name,
}; };
pub use fs_snapshot::FsSnapshot; pub use fs_snapshot::FsSnapshot;
pub use hash::*; pub use hash::*;

View file

@ -564,19 +564,80 @@ 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<Ref, Vec<&crate::web::interface::AddedInstance>> =
HashMap::new();
for added in request.added.values() { for added in request.added.values() {
if let Err(err) = self.syncback_added_instance( if let Some(parent_ref) = added.parent {
added, adds_by_parent.entry(parent_ref).or_default().push(added);
&tree, } else {
&duplicate_siblings_cache, // No parent — process individually (will fail with context)
&stats, adds_by_parent
&converted_parents, .entry(Ref::none())
) { .or_default()
log::warn!( .push(added);
"Failed to syncback added instance '{}': {}", }
added.name, }
err for (parent_ref, siblings) in &adds_by_parent {
); // Pre-seed sibling_slugs from the tree's existing children
// of this parent so new instances dedup against existing ones.
// We derive slugs from actual filesystem paths (via instigating_source)
// to correctly account for dedup suffixes (e.g., Hey_Bro~1).
let mut sibling_slugs: HashSet<String> = if *parent_ref != Ref::none() {
if let Some(parent_inst) = tree.get_instance(*parent_ref) {
use crate::snapshot::InstigatingSource;
use crate::syncback::{name_needs_slugify, strip_middleware_extension};
parent_inst
.children()
.iter()
.filter_map(|r| tree.get_instance(*r))
.map(|inst| {
// Prefer filesystem path for accurate dedup keys
if let Some(InstigatingSource::Path(p)) =
&inst.metadata().instigating_source
{
if let Some(fname) = p.file_name().and_then(|s| s.to_str()) {
if let Some(mw) = inst.metadata().middleware {
return strip_middleware_extension(fname, mw)
.to_lowercase();
}
// No middleware info — use filename as-is
// (directory entries have no extension)
return fname.to_lowercase();
}
}
// Fallback: re-slugify instance name
let name = inst.name();
if name_needs_slugify(name) {
slugify_name(name).to_lowercase()
} else {
name.to_lowercase()
}
})
.collect()
} else {
HashSet::new()
}
} else {
HashSet::new()
};
for added in siblings {
if let Err(err) = self.syncback_added_instance(
added,
&tree,
&duplicate_siblings_cache,
&stats,
&converted_parents,
&mut sibling_slugs,
) {
log::warn!(
"Failed to syncback added instance '{}': {}",
added.name,
err
);
}
} }
} }
} }
@ -768,6 +829,7 @@ impl ApiService {
duplicate_siblings_cache: &HashSet<Ref>, duplicate_siblings_cache: &HashSet<Ref>,
stats: &crate::syncback::SyncbackStats, stats: &crate::syncback::SyncbackStats,
converted_parents: &HashMap<Ref, PathBuf>, converted_parents: &HashMap<Ref, PathBuf>,
sibling_slugs: &mut HashSet<String>,
) -> anyhow::Result<()> { ) -> anyhow::Result<()> {
use anyhow::Context; use anyhow::Context;
@ -899,29 +961,13 @@ impl ApiService {
} }
}; };
// Seed sibling slugs from the tree's existing children of the parent. // Create the instance at the resolved path, accumulating its claimed
// This gives us bare instance-name-level slugs (no extensions) for // slug into sibling_slugs so subsequent siblings in the same batch
// accurate dedup against existing siblings. // see it as taken.
let sibling_slugs: HashSet<String> = { let slug =
use crate::syncback::name_needs_slugify; self.syncback_instance_to_path_with_stats(added, &parent_dir, stats, sibling_slugs)?;
parent_instance sibling_slugs.insert(slug);
.children() Ok(())
.iter()
.filter_map(|r| tree.get_instance(*r))
.map(|inst| {
let name = inst.name();
if name_needs_slugify(name) {
slugify_name(name).to_lowercase()
} else {
name.to_lowercase()
}
})
.collect()
};
// Create the instance at the resolved path
self.syncback_instance_to_path_with_stats(added, &parent_dir, stats, &sibling_slugs)
.map(|_| ())
} }
/// Update an existing instance in place instead of creating new files. /// Update an existing instance in place instead of creating new files.
@ -1086,7 +1132,7 @@ impl ApiService {
if existing_path.is_dir() { if existing_path.is_dir() {
// Update init.meta.json5 if needed // Update init.meta.json5 if needed
if !added.properties.is_empty() { if !added.properties.is_empty() {
self.write_init_meta_json(existing_path, added)?; self.write_init_meta_json(existing_path, added, None)?;
log::info!( log::info!(
"Syncback: Updated existing {} at {}/init.meta.json5", "Syncback: Updated existing {} at {}/init.meta.json5",
class_name, class_name,
@ -1104,7 +1150,7 @@ impl ApiService {
} }
} else { } else {
// It's a standalone file (e.g., .model.json5) // It's a standalone file (e.g., .model.json5)
let content = self.serialize_instance_to_model_json(added)?; let content = self.serialize_instance_to_model_json(added, None)?;
self.suppress_path(existing_path); self.suppress_path(existing_path);
fs::write(existing_path, &content).with_context(|| { fs::write(existing_path, &content).with_context(|| {
format!("Failed to write file: {}", existing_path.display()) format!("Failed to write file: {}", existing_path.display())
@ -1845,8 +1891,8 @@ impl ApiService {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
})?; })?;
// Write init.meta.json5 if has ANY properties (Attributes, Tags, etc.) // Write init.meta.json5 if has ANY properties or needs a name field
let has_metadata = if added.properties.is_empty() { let has_metadata = if added.properties.is_empty() && meta_name_field.is_none() {
false false
} else { } else {
self.write_directory_meta_json(&dir_path, added, meta_name_field)?; self.write_directory_meta_json(&dir_path, added, meta_name_field)?;
@ -1880,7 +1926,7 @@ impl ApiService {
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
})?; })?;
self.write_init_meta_json(&dir_path, added)?; self.write_init_meta_json(&dir_path, added, meta_name_field)?;
log::info!( log::info!(
"Syncback: Created StringValue directory at {}", "Syncback: Created StringValue directory at {}",
dir_path.display() dir_path.display()
@ -1900,6 +1946,23 @@ impl ApiService {
fs::write(&file_path, value.as_bytes()).with_context(|| { fs::write(&file_path, value.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
// Write adjacent meta for name preservation if slugified
if let Some(real_name) = meta_name_field {
let meta = self.build_meta_object(
None,
Some(real_name),
indexmap::IndexMap::new(),
indexmap::IndexMap::new(),
);
let meta_path =
parent_dir.join(format!("{}.meta.json5", encoded_name));
let content = crate::json::to_vec_pretty_sorted(&meta)
.context("Failed to serialize meta")?;
self.suppress_path(&meta_path);
fs::write(&meta_path, &content).with_context(|| {
format!("Failed to write meta: {}", meta_path.display())
})?;
}
log::info!("Syncback: Created StringValue at {}", file_path.display()); log::info!("Syncback: Created StringValue at {}", file_path.display());
} }
} }
@ -1921,6 +1984,8 @@ impl ApiService {
fs::write(&init_path, content.as_bytes()).with_context(|| { fs::write(&init_path, content.as_bytes()).with_context(|| {
format!("Failed to write file: {}", init_path.display()) format!("Failed to write file: {}", init_path.display())
})?; })?;
// Write init.meta.json5 for className and name preservation
self.write_init_meta_json(&dir_path, added, meta_name_field)?;
log::info!( log::info!(
"Syncback: Created LocalizationTable directory at {}", "Syncback: Created LocalizationTable directory at {}",
dir_path.display() dir_path.display()
@ -1932,6 +1997,23 @@ impl ApiService {
fs::write(&file_path, content.as_bytes()).with_context(|| { fs::write(&file_path, content.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
// Write adjacent meta for name preservation if slugified
if let Some(real_name) = meta_name_field {
let meta = self.build_meta_object(
None,
Some(real_name),
indexmap::IndexMap::new(),
indexmap::IndexMap::new(),
);
let meta_path =
parent_dir.join(format!("{}.meta.json5", encoded_name));
let content = crate::json::to_vec_pretty_sorted(&meta)
.context("Failed to serialize meta")?;
self.suppress_path(&meta_path);
fs::write(&meta_path, &content).with_context(|| {
format!("Failed to write meta: {}", meta_path.display())
})?;
}
log::info!( log::info!(
"Syncback: Created LocalizationTable at {}", "Syncback: Created LocalizationTable at {}",
file_path.display() file_path.display()
@ -1968,7 +2050,7 @@ impl ApiService {
})?; })?;
// Write init.meta.json5 with class and properties // Write init.meta.json5 with class and properties
self.write_init_meta_json(&dir_path, added)?; self.write_init_meta_json(&dir_path, added, meta_name_field)?;
log::info!( log::info!(
"Syncback: Updated {} at {}/init.meta.json5", "Syncback: Updated {} at {}/init.meta.json5",
@ -1979,7 +2061,8 @@ impl ApiService {
// Recursively process children // Recursively process children
self.process_children_incremental(&unique_children, &dir_path, stats)?; self.process_children_incremental(&unique_children, &dir_path, stats)?;
} else { } else {
let content = self.serialize_instance_to_model_json(added)?; let content =
self.serialize_instance_to_model_json(added, meta_name_field)?;
// Use the detected file path if available (preserves .model.json // Use the detected file path if available (preserves .model.json
// vs .model.json5), otherwise default to .model.json5 // vs .model.json5), otherwise default to .model.json5
let file_path = match &existing_format { let file_path = match &existing_format {
@ -2004,13 +2087,41 @@ impl ApiService {
/// Process children with incremental slug tracking, so each child's /// Process children with incremental slug tracking, so each child's
/// claimed name is added to the taken set before processing the next. /// claimed name is added to the taken set before processing the next.
/// Pre-seeds the taken set from existing directory entries so new children
/// don't collide with files already on disk.
fn process_children_incremental( fn process_children_incremental(
&self, &self,
children: &[&crate::web::interface::AddedInstance], children: &[&crate::web::interface::AddedInstance],
dir_path: &std::path::Path, dir_path: &std::path::Path,
stats: &crate::syncback::SyncbackStats, stats: &crate::syncback::SyncbackStats,
) -> anyhow::Result<()> { ) -> anyhow::Result<()> {
let mut taken = HashSet::new(); // Seed from existing directory entries (bare stems, lowercased)
let mut taken: HashSet<String> = if dir_path.is_dir() {
fs::read_dir(dir_path)
.ok()
.map(|entries| {
entries
.flatten()
.filter_map(|e| {
let name = e.file_name();
let name_str = name.to_string_lossy();
// For directories, the name IS the slug.
// For files, strip the last extension to approximate the slug.
if e.path().is_dir() {
Some(name_str.to_lowercase())
} else {
std::path::Path::new(name_str.as_ref())
.file_stem()
.and_then(|s| s.to_str())
.map(|s| s.to_lowercase())
}
})
.collect()
})
.unwrap_or_default()
} else {
HashSet::new()
};
for child in children { for child in children {
let slug = self.syncback_instance_to_path_with_stats(child, dir_path, stats, &taken)?; let slug = self.syncback_instance_to_path_with_stats(child, dir_path, stats, &taken)?;
taken.insert(slug); taken.insert(slug);
@ -2091,6 +2202,7 @@ impl ApiService {
&self, &self,
dir_path: &std::path::Path, dir_path: &std::path::Path,
added: &crate::web::interface::AddedInstance, added: &crate::web::interface::AddedInstance,
instance_name: Option<&str>,
) -> anyhow::Result<()> { ) -> anyhow::Result<()> {
use anyhow::Context; use anyhow::Context;
@ -2098,7 +2210,8 @@ impl ApiService {
self.filter_properties_for_meta(&added.class_name, &added.properties, None); self.filter_properties_for_meta(&added.class_name, &added.properties, None);
// For non-Folder classes, we need to include className // For non-Folder classes, we need to include className
let meta = self.build_meta_object(Some(&added.class_name), None, properties, attributes); let meta =
self.build_meta_object(Some(&added.class_name), instance_name, properties, attributes);
let meta_path = dir_path.join("init.meta.json5"); let meta_path = dir_path.join("init.meta.json5");
let content = crate::json::to_vec_pretty_sorted(&meta) let content = crate::json::to_vec_pretty_sorted(&meta)
.context("Failed to serialize init.meta.json5")?; .context("Failed to serialize init.meta.json5")?;
@ -2329,6 +2442,7 @@ impl ApiService {
fn serialize_instance_to_model_json( fn serialize_instance_to_model_json(
&self, &self,
added: &crate::web::interface::AddedInstance, added: &crate::web::interface::AddedInstance,
instance_name: Option<&str>,
) -> anyhow::Result<Vec<u8>> { ) -> anyhow::Result<Vec<u8>> {
use anyhow::Context; use anyhow::Context;
use serde_json::json; use serde_json::json;
@ -2340,6 +2454,9 @@ impl ApiService {
// Build the JSON model structure // Build the JSON model structure
// Format: https://rojo.space/docs/v7/sync-details/#json-models // Format: https://rojo.space/docs/v7/sync-details/#json-models
let mut model = serde_json::Map::new(); let mut model = serde_json::Map::new();
if let Some(name) = instance_name {
model.insert("name".to_string(), json!(name));
}
model.insert("className".to_string(), json!(added.class_name)); model.insert("className".to_string(), json!(added.class_name));
if !properties.is_empty() { if !properties.is_empty() {