hakowork/rojo
3
0
Fork 0
mirror of https://github.com/rojo-rbx/rojo.git synced 2026-10-23 05:51:39 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Add statistics tracking for syncback operations

Browse source 
- Introduced a new `SyncbackStats` module to track and report issues during syncback, including duplicate names, unknown classes, and properties.
- Updated the `syncback_loop` and related functions to utilize the stats tracker, logging summaries of issues encountered.
- Enhanced property filtering and instance syncing to record statistics on unknown classes and properties.
- Modified tests to ensure consistent output by normalizing temporary paths in snapshots.
- Added `regex` dependency for path normalization in tests.
This commit is contained in:
Joe 2026-02-01 08:29:00 -06:00
parent 58399bb863
commit 8fe908740f
14 changed files with 541 additions and 84 deletions
Download patch file Download diff file Expand all files Collapse all files

1
Cargo.lock generated
View file

@ -2264,6 +2264,7 @@ dependencies = [
"rbx_reflection",
"rbx_reflection_database",
"rbx_xml",
"regex",
"reqwest",
"ritz",
"rmp-serde",

1
Cargo.toml
View file

@ -119,6 +119,7 @@ criterion = "0.3.6"
insta = { version = "1.46.2", features = ["redactions", "yaml"] }
paste = "1.0.15"
pretty_assertions = "1.4.1"
regex = "1"
serde_yaml = "0.8.26"
tempfile = "3.24.0"
walkdir = "2.5.0"

2
rbx-dom

@ -1 +1 @@
Subproject commit 42ab1af0e347f7947c1ac792bf35d0c8b65391e0
Subproject commit 44bf8aeb0c754a304917a622baaf028f5065bb6e

4
rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees_removing-stdout.snap
View file

@ -1,5 +1,5 @@
---
source: tests/rojo_test/syncback_util.rs
expression: "String::from_utf8_lossy(&output.stdout)"
expression: normalized_stdout
---
Removing C:/Users/Joe/AppData/Local/Temp/.tmpGY4Eb8/ignore_trees_removing/src
Removing src

6
rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__nested_projects_weird-stdout.snap
View file

@ -1,8 +1,8 @@
---
source: tests/rojo_test/syncback_util.rs
expression: "String::from_utf8_lossy(&output.stdout)"
expression: normalized_stdout
---
Writing src/modules/ClientModule.luau
Writing src/modules/ServerModule.luau
Removing C:/Users/Joe/AppData/Local/Temp/.tmpLKd1Ar/nested_projects_weird/client-only.project.json5
Removing C:/Users/Joe/AppData/Local/Temp/.tmpLKd1Ar/nested_projects_weird/server-only.project.json5
Removing client-only.project.json5
Removing server-only.project.json5

4
rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties_duplicate-stdout.snap
View file

@ -1,9 +1,9 @@
---
source: tests/rojo_test/syncback_util.rs
expression: "String::from_utf8_lossy(&output.stdout)"
expression: normalized_stdout
---
Writing src/Pointer_1.model.json5
Writing src/Pointer_2.model.json5
Writing src/Target/init.meta.json5
Writing src/Target
Removing C:/Users/Joe/AppData/Local/Temp/.tmpA4y8vf/ref_properties_duplicate/src/Target.model.json5
Removing src/Target.model.json5

21
src/snapshot_middleware/dir.rs
View file

@ -139,7 +139,7 @@ pub fn syncback_dir_no_meta<'sync>(
let mut removed_children = Vec::new();
// Detect duplicate child names (case-insensitive for file system safety).
// We skip duplicates instead of failing, logging them for the user.
// We skip duplicates instead of failing, tracking them in stats.
let mut child_name_counts: HashMap<String, usize> = HashMap::new();
for child_ref in new_inst.children() {
let child = snapshot.get_new_instance(*child_ref).unwrap();
@ -153,14 +153,21 @@ pub fn syncback_dir_no_meta<'sync>(
.map(|(name, _)| name)
.collect();
if !duplicate_names.is_empty() && snapshot.warn_duplicate_names() {
// Record duplicate names in stats tracker
if !duplicate_names.is_empty() {
let inst_path = crate::syncback::inst_path(snapshot.new_tree(), snapshot.new);
// Count total instances being skipped (sum of all duplicates)
let mut total_skipped = 0;
for child_ref in new_inst.children() {
let child = snapshot.get_new_instance(*child_ref).unwrap();
if duplicate_names.contains(&child.name.to_lowercase()) {
total_skipped += 1;
}
}
let duplicate_list: Vec<&str> = duplicate_names.iter().map(|s| s.as_str()).collect();
log::warn!(
"Skipping children with duplicate names at '{}' (cannot reliably sync): {:?}",
inst_path,
duplicate_list
);
snapshot
.stats()
.record_duplicate_names_batch(&inst_path, &duplicate_list, total_skipped);
}
if let Some(old_inst) = snapshot.old_inst() {

46
src/syncback/mod.rs
View file

@ -4,6 +4,7 @@ mod hash;
mod property_filter;
mod ref_properties;
mod snapshot;
mod stats;
use anyhow::Context;
use indexmap::IndexMap;
@ -35,6 +36,7 @@ pub use property_filter::{
filter_properties, filter_properties_preallocated, should_property_serialize,
};
pub use snapshot::{inst_path, SyncbackData, SyncbackSnapshot};
pub use stats::SyncbackStats;
/// The name of an enviroment variable to use to override the behavior of
/// syncback on model files.
@ -72,10 +74,27 @@ static GIT_IGNORE_GLOB: OnceLock<Glob> = OnceLock::new();
pub fn syncback_loop(
vfs: &Vfs,
old_tree: &mut RojoTree,
mut new_tree: WeakDom,
new_tree: WeakDom,
project: &Project,
incremental: bool,
) -> anyhow::Result<FsSnapshot> {
syncback_loop_with_stats(vfs, old_tree, new_tree, project, incremental, None)
}
/// Runs the syncback loop with an optional external stats tracker.
/// If no stats are provided, an internal one is created and logged at the end.
pub fn syncback_loop_with_stats(
vfs: &Vfs,
old_tree: &mut RojoTree,
mut new_tree: WeakDom,
project: &Project,
incremental: bool,
external_stats: Option<&SyncbackStats>,
) -> anyhow::Result<FsSnapshot> {
// Create internal stats if not provided externally
let internal_stats = SyncbackStats::new();
let stats = external_stats.unwrap_or(&internal_stats);
let ignore_patterns = project
.syncback_rules
.as_ref()
@ -223,6 +242,7 @@ pub fn syncback_loop(
new_tree: &new_tree,
project,
incremental,
stats,
};
// Always start with old reference for the Project middleware.
@ -304,10 +324,8 @@ pub fn syncback_loop(
extension_for_middleware(new_middleware)
));
let new_snapshot = snapshot.with_new_path(path, snapshot.new, snapshot.old);
log::warn!(
"Could not syncback {inst_path} as a Directory because: {err}.\n\
It will instead be synced back as a {new_middleware:?}."
);
// Record the fallback in stats instead of warning directly
stats.record_rbxm_fallback(&inst_path, &err.to_string());
let new_syncback_result = new_middleware
.syncback(&new_snapshot)
.with_context(|| format!("Failed to syncback {inst_path}"));
@ -428,6 +446,7 @@ pub fn syncback_loop(
// Skip any path that is inside another path we're removing,
// since remove_dir_all will handle the contents.
let paths_vec: Vec<_> = paths_to_remove.iter().collect();
let normalized_project_path = normalize_path(project_path);
for old_path in &paths_vec {
// Check if this path is inside another path we're removing
let is_nested = paths_vec
@ -438,15 +457,26 @@ pub fn syncback_loop(
continue;
}
log::debug!("Removing orphaned path: {}", old_path.display());
// Convert absolute path to relative path for FsSnapshot.
// FsSnapshot expects relative paths and joins them with base_path in write_to_vfs.
let relative_path = old_path
.strip_prefix(&normalized_project_path)
.unwrap_or(old_path);
log::debug!("Removing orphaned path: {}", relative_path.display());
if old_path.is_dir() {
fs_snapshot.remove_dir(old_path);
fs_snapshot.remove_dir(relative_path);
} else {
fs_snapshot.remove_file(old_path);
fs_snapshot.remove_file(relative_path);
}
}
}
// Log stats summary if using internal stats (not provided externally)
if external_stats.is_none() {
stats.log_summary();
}
Ok(fs_snapshot)
}

56
src/syncback/property_filter.rs
View file

@ -3,6 +3,8 @@ use rbx_reflection::{PropertyKind, PropertySerialization, Scriptability};
use crate::{variant_eq::variant_eq, Project};
use super::SyncbackStats;
/// Returns a map of properties from `inst` that are both allowed under the
/// user-provided settings, are not their default value, and serialize.
pub fn filter_properties<'inst>(
@ -21,6 +23,18 @@ pub fn filter_properties_preallocated<'inst>(
project: &Project,
inst: &'inst Instance,
allocation: &mut Vec<(Ustr, &'inst Variant)>,
) {
filter_properties_with_stats(project, inst, allocation, None);
}
/// Fills `allocation` with a list of properties from `inst` that are
/// user-provided settings, are not their default value, and serialize.
/// Optionally tracks unknown classes and properties via the stats tracker.
pub fn filter_properties_with_stats<'inst>(
project: &Project,
inst: &'inst Instance,
allocation: &mut Vec<(Ustr, &'inst Variant)>,
stats: Option<&SyncbackStats>,
) {
let sync_unscriptable = project
.syncback_rules
@ -28,17 +42,22 @@ pub fn filter_properties_preallocated<'inst>(
.and_then(|s| s.sync_unscriptable)
.unwrap_or(true);
let class_data = rbx_reflection_database::get()
.unwrap()
.classes
.get(inst.class.as_str());
let database = rbx_reflection_database::get().unwrap();
let class_data = database.classes.get(inst.class.as_str());
// Track unknown class if not found
if class_data.is_none() {
if let Some(stats) = stats {
stats.record_unknown_class(&inst.class);
}
}
let predicate = |prop_name: &Ustr, prop_value: &Variant| {
// We don't want to serialize Ref or UniqueId properties in JSON files
if matches!(prop_value, Variant::Ref(_) | Variant::UniqueId(_)) {
return true;
}
if !should_property_serialize(&inst.class, prop_name) {
if !should_property_serialize_with_stats(&inst.class, prop_name, stats) {
return true;
}
if !sync_unscriptable {
@ -80,13 +99,30 @@ pub fn filter_properties_preallocated<'inst>(
/// Checks if a property should serialize based on the reflection database.
/// Returns false for properties with DoesNotSerialize serialization, true otherwise.
pub fn should_property_serialize(class_name: &str, prop_name: &str) -> bool {
should_property_serialize_with_stats(class_name, prop_name, None)
}
/// Checks if a property should serialize based on the reflection database.
/// Returns false for properties with DoesNotSerialize serialization, true otherwise.
/// Optionally tracks unknown properties via the stats tracker.
pub fn should_property_serialize_with_stats(
class_name: &str,
prop_name: &str,
stats: Option<&SyncbackStats>,
) -> bool {
let database = rbx_reflection_database::get().unwrap();
let mut current_class_name = class_name;
loop {
let class_data = match database.classes.get(current_class_name) {
Some(data) => data,
None => return true,
None => {
// Unknown class - track it if stats provided
if let Some(stats) = stats {
stats.record_unknown_class(current_class_name);
}
return true;
}
};
if let Some(data) = class_data.properties.get(prop_name) {
log::trace!("found {class_name}.{prop_name} on {current_class_name}");
@ -94,7 +130,7 @@ pub fn should_property_serialize(class_name: &str, prop_name: &str) -> bool {
// It's not really clear if this can ever happen but I want to
// support it just in case!
PropertyKind::Alias { alias_for } => {
should_property_serialize(current_class_name, alias_for)
should_property_serialize_with_stats(current_class_name, alias_for, stats)
}
// Migrations and aliases are happily handled for us by parsers
// so we don't really need to handle them.
@ -109,5 +145,11 @@ pub fn should_property_serialize(class_name: &str, prop_name: &str) -> bool {
break;
}
}
// Property not found in class hierarchy - track it if stats provided
if let Some(stats) = stats {
stats.record_unknown_property(class_name, prop_name);
}
true
}

12
src/syncback/snapshot.rs
View file

@ -13,7 +13,9 @@ use rbx_dom_weak::{
Instance, Ustr, UstrMap, WeakDom,
};
use super::{get_best_middleware, name_for_inst, property_filter::filter_properties};
use super::{
get_best_middleware, name_for_inst, property_filter::filter_properties, SyncbackStats,
};
#[derive(Clone, Copy)]
pub struct SyncbackData<'sync> {
@ -24,6 +26,8 @@ pub struct SyncbackData<'sync> {
/// When true, preserves existing file structure and middleware formats.
/// When false (clean mode), all child instances are treated as new.
pub(super) incremental: bool,
/// Statistics tracker for syncback issues.
pub(super) stats: &'sync SyncbackStats,
}
pub struct SyncbackSnapshot<'sync> {
@ -327,6 +331,12 @@ impl<'sync> SyncbackSnapshot<'sync> {
.map(|rules| rules.warn_duplicate_names())
.unwrap_or(false)
}
/// Returns a reference to the syncback statistics tracker.
#[inline]
pub fn stats(&self) -> &'sync SyncbackStats {
self.data.stats
}
}
pub fn filter_out_property(inst: &Instance, prop_name: &str) -> bool {

334
src/syncback/stats.rs Normal file
View file

@ -0,0 +1,334 @@
//! Statistics tracking for syncback operations.
//!
//! This module provides a way to track and report various issues that occur
//! during syncback, including:
//! - Instances with duplicate names (indistinguishable paths)
//! - Instances that fell back to rbxm/rbxmx format
//! - Unknown classes not in the reflection database
//! - Unknown properties not in the reflection database
use std::collections::HashSet;
use std::sync::atomic::{AtomicUsize, Ordering};
use std::sync::Mutex;
/// Statistics collected during a syncback operation.
///
/// This struct is designed to be used in a single-threaded context during
/// the main syncback loop, but uses interior mutability for flexibility.
#[derive(Default)]
pub struct SyncbackStats {
/// Count of instances skipped due to duplicate/indistinguishable names.
duplicate_name_count: AtomicUsize,
/// Count of instances that fell back to rbxm/rbxmx format.
rbxm_fallback_count: AtomicUsize,
/// Count of instances with unknown classes.
unknown_class_count: AtomicUsize,
/// Count of properties with unknown definitions.
unknown_property_count: AtomicUsize,
/// Set of unknown class names encountered (for reporting).
unknown_classes: Mutex<HashSet<String>>,
/// Set of unknown property names encountered (class.property format).
unknown_properties: Mutex<HashSet<String>>,
}
impl SyncbackStats {
/// Creates a new empty stats tracker.
pub fn new() -> Self {
Self::default()
}
/// Records that an instance was skipped due to having a duplicate name.
///
/// When debug logging is enabled, logs the individual instance path.
pub fn record_duplicate_name(&self, inst_path: &str, name: &str) {
self.duplicate_name_count.fetch_add(1, Ordering::Relaxed);
// Only log individual instances at debug level to avoid spam
if log::log_enabled!(log::Level::Debug) {
log::debug!(
"Skipping instance due to duplicate name: '{}' at '{}'",
name,
inst_path
);
}
}
/// Records that multiple instances were skipped due to duplicate names at a path.
///
/// This is more efficient when multiple duplicates are detected at once.
pub fn record_duplicate_names_batch(
&self,
inst_path: &str,
duplicate_names: &[&str],
total_skipped: usize,
) {
self.duplicate_name_count
.fetch_add(total_skipped, Ordering::Relaxed);
// Only log at debug level
if log::log_enabled!(log::Level::Debug) {
log::debug!(
"Skipping {} instance(s) with duplicate names at '{}': {:?}",
total_skipped,
inst_path,
duplicate_names
);
}
}
/// Records that an instance fell back to rbxm/rbxmx format.
pub fn record_rbxm_fallback(&self, inst_path: &str, reason: &str) {
self.rbxm_fallback_count.fetch_add(1, Ordering::Relaxed);
// Only log individual fallbacks at debug level
if log::log_enabled!(log::Level::Debug) {
log::debug!(
"Instance '{}' fell back to binary model format: {}",
inst_path,
reason
);
}
}
/// Records that an unknown class was encountered.
pub fn record_unknown_class(&self, class_name: &str) {
self.unknown_class_count.fetch_add(1, Ordering::Relaxed);
if let Ok(mut classes) = self.unknown_classes.lock() {
if classes.insert(class_name.to_string()) {
// Only log when we see a new unknown class
if log::log_enabled!(log::Level::Debug) {
log::debug!(
"Encountered unknown class not in reflection database: '{}'",
class_name
);
}
}
}
}
/// Records that an unknown property was encountered.
pub fn record_unknown_property(&self, class_name: &str, property_name: &str) {
self.unknown_property_count.fetch_add(1, Ordering::Relaxed);
let key = format!("{}.{}", class_name, property_name);
if let Ok(mut properties) = self.unknown_properties.lock() {
if properties.insert(key.clone()) {
// Only log when we see a new unknown property
if log::log_enabled!(log::Level::Debug) {
log::debug!(
"Encountered unknown property not in reflection database: '{}'",
key
);
}
}
}
}
/// Returns the count of instances skipped due to duplicate names.
pub fn duplicate_name_count(&self) -> usize {
self.duplicate_name_count.load(Ordering::Relaxed)
}
/// Returns the count of instances that fell back to rbxm format.
pub fn rbxm_fallback_count(&self) -> usize {
self.rbxm_fallback_count.load(Ordering::Relaxed)
}
/// Returns the count of unknown classes encountered.
pub fn unknown_class_count(&self) -> usize {
self.unknown_class_count.load(Ordering::Relaxed)
}
/// Returns the count of unknown properties encountered.
pub fn unknown_property_count(&self) -> usize {
self.unknown_property_count.load(Ordering::Relaxed)
}
/// Returns the unique unknown class names.
pub fn unknown_classes(&self) -> Vec<String> {
self.unknown_classes
.lock()
.map(|g| g.iter().cloned().collect())
.unwrap_or_default()
}
/// Returns the unique unknown properties (as "Class.Property").
pub fn unknown_properties(&self) -> Vec<String> {
self.unknown_properties
.lock()
.map(|g| g.iter().cloned().collect())
.unwrap_or_default()
}
/// Returns true if any issues were recorded.
pub fn has_issues(&self) -> bool {
self.duplicate_name_count() > 0
|| self.rbxm_fallback_count() > 0
|| self.unknown_class_count() > 0
|| self.unknown_property_count() > 0
}
/// Returns true if there are unknown classes or properties that should be
/// reported for reflection database updates.
pub fn has_unknown_types(&self) -> bool {
self.unknown_class_count() > 0 || self.unknown_property_count() > 0
}
/// Logs a summary of all recorded issues as warnings.
///
/// This should be called at the end of a syncback operation.
pub fn log_summary(&self) {
let duplicate_count = self.duplicate_name_count();
let rbxm_count = self.rbxm_fallback_count();
let unknown_class_count = self.unknown_class_count();
let unknown_prop_count = self.unknown_property_count();
if !self.has_issues() {
return;
}
log::warn!("Syncback completed with issues:");
if duplicate_count > 0 {
log::warn!(
" - {} instance(s) could not be synced due to duplicate/indistinguishable names",
duplicate_count
);
}
if rbxm_count > 0 {
log::warn!(
" - {} instance(s) fell back to binary model format (rbxm/rbxmx)",
rbxm_count
);
}
if unknown_class_count > 0 {
let classes = self.unknown_classes();
log::warn!(
" - {} instance(s) have unknown classes not in reflection database",
unknown_class_count
);
if log::log_enabled!(log::Level::Info) && !classes.is_empty() {
log::info!(" Unknown classes: {:?}", classes);
}
}
if unknown_prop_count > 0 {
let properties = self.unknown_properties();
log::warn!(
" - {} property reference(s) to unknown properties not in reflection database",
unknown_prop_count
);
if log::log_enabled!(log::Level::Info) && !properties.is_empty() {
// Only show first 20 to avoid spam
let display: Vec<_> = properties.iter().take(20).collect();
let remaining = properties.len().saturating_sub(20);
if remaining > 0 {
log::info!(
" Unknown properties (showing 20 of {}): {:?}",
properties.len(),
display
);
} else {
log::info!(" Unknown properties: {:?}", display);
}
}
}
// Helpful hint about debug logging
if duplicate_count > 0 || rbxm_count > 0 {
log::info!(
" Enable debug logging (RUST_LOG=debug) to see individual instance details"
);
}
}
/// Merges stats from another SyncbackStats instance into this one.
pub fn merge(&self, other: &SyncbackStats) {
self.duplicate_name_count.fetch_add(
other.duplicate_name_count.load(Ordering::Relaxed),
Ordering::Relaxed,
);
self.rbxm_fallback_count.fetch_add(
other.rbxm_fallback_count.load(Ordering::Relaxed),
Ordering::Relaxed,
);
self.unknown_class_count.fetch_add(
other.unknown_class_count.load(Ordering::Relaxed),
Ordering::Relaxed,
);
self.unknown_property_count.fetch_add(
other.unknown_property_count.load(Ordering::Relaxed),
Ordering::Relaxed,
);
if let (Ok(mut self_classes), Ok(other_classes)) =
(self.unknown_classes.lock(), other.unknown_classes.lock())
{
self_classes.extend(other_classes.iter().cloned());
}
if let (Ok(mut self_props), Ok(other_props)) = (
self.unknown_properties.lock(),
other.unknown_properties.lock(),
) {
self_props.extend(other_props.iter().cloned());
}
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_stats_tracking() {
let stats = SyncbackStats::new();
assert!(!stats.has_issues());
stats.record_duplicate_name("Root/Folder", "Script");
stats.record_duplicate_name("Root/Folder", "Script");
assert_eq!(stats.duplicate_name_count(), 2);
stats.record_rbxm_fallback("Root/Model", "cannot represent as directory");
assert_eq!(stats.rbxm_fallback_count(), 1);
stats.record_unknown_class("MyCustomClass");
stats.record_unknown_class("MyCustomClass"); // duplicate
assert_eq!(stats.unknown_class_count(), 2);
assert_eq!(stats.unknown_classes().len(), 1); // unique
stats.record_unknown_property("Part", "CustomProp");
assert_eq!(stats.unknown_property_count(), 1);
assert!(stats.has_issues());
assert!(stats.has_unknown_types());
}
#[test]
fn test_batch_recording() {
let stats = SyncbackStats::new();
stats.record_duplicate_names_batch("Root/Folder", &["Script", "Model"], 4);
assert_eq!(stats.duplicate_name_count(), 4);
}
#[test]
fn test_merge() {
let stats1 = SyncbackStats::new();
let stats2 = SyncbackStats::new();
stats1.record_duplicate_name("path1", "name1");
stats2.record_duplicate_name("path2", "name2");
stats2.record_unknown_class("Class1");
stats1.merge(&stats2);
assert_eq!(stats1.duplicate_name_count(), 2);
assert_eq!(stats1.unknown_classes().len(), 1);
}
}

88
src/web/api.rs
View file

@ -245,6 +245,9 @@ impl ApiService {
// Process added instances (syncback: create files from Studio instances)
// We hold the tree lock during the entire processing to ensure the cache
// remains consistent with the tree state being checked.
// Create a stats tracker for this syncback operation
let stats = crate::syncback::SyncbackStats::new();
if !request.added.is_empty() {
let tree = self.serve_session.tree();
// Pre-compute duplicate sibling info in O(N) for efficient path uniqueness checks
@ -253,7 +256,7 @@ impl ApiService {
for added in request.added.values() {
if let Err(err) =
self.syncback_added_instance(added, &tree, &duplicate_siblings_cache)
self.syncback_added_instance(added, &tree, &duplicate_siblings_cache, &stats)
{
log::warn!(
"Failed to syncback added instance '{}': {}",
@ -264,6 +267,9 @@ impl ApiService {
}
}
// Log summary of any syncback issues
stats.log_summary();
// Log property updates being synced back
for update in &request.updated {
let prop_names: Vec<&str> = update
@ -359,10 +365,12 @@ impl ApiService {
/// Checks if a path in the Rojo tree is unique using pre-computed duplicate sibling info.
/// Returns true if the path is unique, false if duplicates exist at any level.
/// This is O(d) where d is the depth of the instance.
/// Records any duplicate path issues via the stats tracker.
fn is_tree_path_unique_with_cache(
tree: &crate::snapshot::RojoTree,
target_ref: Ref,
has_duplicate_siblings: &HashSet<Ref>,
stats: &crate::syncback::SyncbackStats,
) -> bool {
let mut current_ref = target_ref;
@ -371,11 +379,9 @@ impl ApiService {
if has_duplicate_siblings.contains(&current_ref) {
if let Some(current) = tree.get_instance(current_ref) {
if let Some(parent) = tree.get_instance(current.parent()) {
log::warn!(
"Syncback: Path to '{}' is not unique - duplicate-named siblings at '{}'",
current.name(),
parent.name()
);
// Build the path for the stats tracker
let inst_path = format!("{}/{}", parent.name(), current.name());
stats.record_duplicate_name(&inst_path, current.name());
}
}
return false;
@ -407,6 +413,7 @@ impl ApiService {
added: &crate::web::interface::AddedInstance,
tree: &crate::snapshot::RojoTree,
duplicate_siblings_cache: &HashSet<Ref>,
stats: &crate::syncback::SyncbackStats,
) -> anyhow::Result<()> {
use crate::syncback::validate_file_name;
use anyhow::Context;
@ -431,7 +438,8 @@ impl ApiService {
// Check if the parent path is unique (no duplicate-named siblings at any level)
// Uses pre-computed cache for O(d) instead of O(d × s) complexity
if !Self::is_tree_path_unique_with_cache(tree, parent_ref, duplicate_siblings_cache) {
if !Self::is_tree_path_unique_with_cache(tree, parent_ref, duplicate_siblings_cache, stats)
{
anyhow::bail!(
"Cannot sync instance '{}' - parent path contains duplicate-named siblings",
added.name
@ -457,7 +465,7 @@ impl ApiService {
};
// Create the instance at the resolved path
self.syncback_instance_to_path(added, &parent_dir)
self.syncback_instance_to_path_with_stats(added, &parent_dir, stats)
}
/// Syncback a removed instance by deleting its file(s) from the filesystem.
@ -551,10 +559,12 @@ impl ApiService {
}
/// Check for duplicate names among children and return the set of unique children
/// that should be synced. Logs warnings for skipped duplicates.
/// that should be synced. Records skipped instances via stats tracker.
fn filter_duplicate_children<'a>(
&self,
children: &'a [crate::web::interface::AddedInstance],
parent_path: &str,
stats: &crate::syncback::SyncbackStats,
) -> Vec<&'a crate::web::interface::AddedInstance> {
use std::collections::HashMap;
@ -571,24 +581,16 @@ impl ApiService {
.map(|(&name, _)| name)
.collect();
// Log skipped duplicates
let mut skipped_count = 0;
for child in children {
if duplicates.contains(child.name.as_str()) {
log::warn!(
"Syncback: Skipped instance '{}' ({}) - has duplicate-named siblings (cannot reliably sync)",
child.name,
child.class_name
);
skipped_count += 1;
// Record skipped duplicates in stats
if !duplicates.is_empty() {
let mut skipped_count = 0;
for child in children {
if duplicates.contains(child.name.as_str()) {
skipped_count += 1;
}
}
}
if skipped_count > 0 {
log::warn!(
"Syncback: Skipped {} instance(s) with duplicate-named siblings",
skipped_count
);
let duplicate_list: Vec<&str> = duplicates.iter().copied().collect();
stats.record_duplicate_names_batch(parent_path, &duplicate_list, skipped_count);
}
// Return only non-duplicate children
@ -600,10 +602,25 @@ impl ApiService {
/// Recursively syncback an instance and its children to the filesystem.
/// This is the internal implementation that handles the actual file creation.
#[allow(dead_code)]
fn syncback_instance_to_path(
&self,
added: &crate::web::interface::AddedInstance,
parent_dir: &std::path::Path,
) -> anyhow::Result<()> {
// Use a default stats tracker for backwards compatibility
let stats = crate::syncback::SyncbackStats::new();
self.syncback_instance_to_path_with_stats(added, parent_dir, &stats)
}
/// Recursively syncback an instance and its children to the filesystem.
/// This is the internal implementation that handles the actual file creation.
/// Uses the provided stats tracker for recording issues.
fn syncback_instance_to_path_with_stats(
&self,
added: &crate::web::interface::AddedInstance,
parent_dir: &std::path::Path,
stats: &crate::syncback::SyncbackStats,
) -> anyhow::Result<()> {
use crate::syncback::validate_file_name;
use anyhow::Context;
@ -616,8 +633,11 @@ impl ApiService {
)
})?;
// Build path string for stats
let inst_path = format!("{}/{}", parent_dir.display(), added.name);
// Filter out children with duplicate names (cannot reliably sync)
let unique_children = self.filter_duplicate_children(&added.children);
let unique_children = self.filter_duplicate_children(&added.children, &inst_path, stats);
let has_children = !unique_children.is_empty();
// Determine the appropriate middleware/file format based on class name
@ -644,7 +664,7 @@ impl ApiService {
dir_path.display()
);
for child in &unique_children {
self.syncback_instance_to_path(child, &dir_path)?;
self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?;
}
} else {
let file_path = parent_dir.join(format!("{}.luau", added.name));
@ -677,7 +697,7 @@ impl ApiService {
dir_path.display()
);
for child in &unique_children {
self.syncback_instance_to_path(child, &dir_path)?;
self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?;
}
} else {
let file_path =
@ -707,7 +727,7 @@ impl ApiService {
dir_path.display()
);
for child in &unique_children {
self.syncback_instance_to_path(child, &dir_path)?;
self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?;
}
} else {
let file_path = parent_dir.join(format!("{}.local.luau", added.name));
@ -751,7 +771,7 @@ impl ApiService {
// Recursively process children
for child in &unique_children {
self.syncback_instance_to_path(child, &dir_path)?;
self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?;
}
}
@ -769,7 +789,7 @@ impl ApiService {
dir_path.display()
);
for child in &unique_children {
self.syncback_instance_to_path(child, &dir_path)?;
self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?;
}
} else {
let value = added
@ -808,7 +828,7 @@ impl ApiService {
dir_path.display()
);
for child in &unique_children {
self.syncback_instance_to_path(child, &dir_path)?;
self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?;
}
} else {
let file_path = parent_dir.join(format!("{}.csv", added.name));
@ -842,7 +862,7 @@ impl ApiService {
// Recursively process children
for child in &unique_children {
self.syncback_instance_to_path(child, &dir_path)?;
self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?;
}
} else {
let content = self.serialize_instance_to_model_json(added)?;

15
tests/rojo_test/syncback_util.rs
View file

@ -1,6 +1,7 @@
use std::{io::Write as _, path::Path, process::Command};
use insta::{assert_snapshot, assert_yaml_snapshot};
use regex::Regex;
use tempfile::tempdir;
use crate::rojo_test::io_util::SYNCBACK_TESTS_PATH;
@ -115,12 +116,14 @@ fn run_syncback_test_impl(name: &str, incremental: bool, callback: impl FnOnce(&
settings.set_snapshot_path(snapshot_path);
settings.set_sort_maps(true);
settings.bind(|| {
assert_snapshot!(
format!("{name}-stdout"),
String::from_utf8_lossy(&output.stdout)
)
});
// Normalize temp directory paths in the output to make snapshots deterministic.
// Paths like "C:/Users/Joe/AppData/Local/Temp/.tmpXXXXXX/test_name/..."
// become "<TEMP>/test_name/..."
let stdout = String::from_utf8_lossy(&output.stdout);
let temp_path_regex = Regex::new(r"[A-Za-z]:/[^\s]*/\.tmp[^/]*/").expect("Invalid regex");
let normalized_stdout = temp_path_regex.replace_all(&stdout, "<TEMP>/");
settings.bind(|| assert_snapshot!(format!("{name}-stdout"), normalized_stdout));
settings.bind(|| callback(project_path.as_path()))
}

35
tests/tests/clean_mode.rs
View file

@ -45,12 +45,15 @@ fn run_syncback(project_path: &Path, input_path: &Path, incremental: bool) -> bo
output.status.success()
}
/// Test that clean mode removes orphaned files that don't exist in the input
/// Test that clean mode removes orphaned files that don't exist in the input.
/// Uses the sync_rules test case which has custom extensions (.modulescript, .text)
/// that get replaced with standard extensions (.luau, .txt) in clean mode.
#[test]
fn clean_mode_removes_orphaned_files() {
let _ = env_logger::try_init();
// Use the sync_rules test case and add an orphaned file
// Use the sync_rules test case - it has files with custom extensions
// that become orphaned when replaced with standard extensions
let source_path = Path::new(SYNCBACK_TESTS_PATH)
.join("sync_rules")
.join("input-project");
@ -64,14 +67,14 @@ fn clean_mode_removes_orphaned_files() {
fs_err::create_dir(&project_path).expect("Couldn't create project directory");
copy_recursive(&source_path, &project_path).expect("Couldn't copy project");
// Create an orphaned file that doesn't match anything in the input
let orphaned_file = project_path.join("src").join("OrphanedFile.luau");
fs_err::write(&orphaned_file, "-- This is orphaned").expect("Couldn't create orphaned file");
// The original files with custom extensions should exist
let modulescript_file = project_path.join("src").join("module.modulescript");
let text_file = project_path.join("src").join("text.text");
assert!(
orphaned_file.exists(),
"Orphaned file should exist before syncback"
modulescript_file.exists(),
"module.modulescript should exist before syncback"
);
assert!(text_file.exists(), "text.text should exist before syncback");
// Run syncback in clean mode (default)
assert!(
@ -79,19 +82,25 @@ fn clean_mode_removes_orphaned_files() {
"Syncback should succeed"
);
// The orphaned file should be removed in clean mode
// (the input doesn't have "OrphanedFile" script)
// The old custom extension files should be removed in clean mode
// (they are orphaned because new files with standard extensions replace them)
assert!(
!orphaned_file.exists(),
"Orphaned file should be removed in clean mode"
!modulescript_file.exists(),
"module.modulescript should be removed in clean mode"
);
assert!(
!text_file.exists(),
"text.text should be removed in clean mode"
);
// The expected files should exist
// The new files with standard extensions should exist
let module_file = project_path.join("src").join("module.luau");
let txt_file = project_path.join("src").join("text.txt");
assert!(
module_file.exists(),
"module.luau should be created by syncback"
);
assert!(txt_file.exists(), "text.txt should be created by syncback");
}
/// Test that clean mode doesn't remove files that are being written to