From 8fe908740fb2e957f0d35be8cce5ec3ebd4e829f Mon Sep 17 00:00:00 2001 From: Joe Date: Sun, 1 Feb 2026 08:29:00 -0600 Subject: [PATCH] Add statistics tracking for syncback operations - 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. --- Cargo.lock | 1 + Cargo.toml | 1 + rbx-dom | 2 +- ...ck_util__ignore_trees_removing-stdout.snap | 4 +- ...ck_util__nested_projects_weird-stdout.snap | 6 +- ...util__ref_properties_duplicate-stdout.snap | 4 +- src/snapshot_middleware/dir.rs | 21 +- src/syncback/mod.rs | 46 ++- src/syncback/property_filter.rs | 56 ++- src/syncback/snapshot.rs | 12 +- src/syncback/stats.rs | 334 ++++++++++++++++++ src/web/api.rs | 88 +++-- tests/rojo_test/syncback_util.rs | 15 +- tests/tests/clean_mode.rs | 35 +- 14 files changed, 541 insertions(+), 84 deletions(-) create mode 100644 src/syncback/stats.rs diff --git a/Cargo.lock b/Cargo.lock index 1749183a..4c5160dd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2264,6 +2264,7 @@ dependencies = [ "rbx_reflection", "rbx_reflection_database", "rbx_xml", + "regex", "reqwest", "ritz", "rmp-serde", diff --git a/Cargo.toml b/Cargo.toml index f7a5aa71..cb51866a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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" diff --git a/rbx-dom b/rbx-dom index 42ab1af0..44bf8aeb 160000 --- a/rbx-dom +++ b/rbx-dom @@ -1 +1 @@ -Subproject commit 42ab1af0e347f7947c1ac792bf35d0c8b65391e0 +Subproject commit 44bf8aeb0c754a304917a622baaf028f5065bb6e diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees_removing-stdout.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees_removing-stdout.snap index 68a2a314..36f02997 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees_removing-stdout.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees_removing-stdout.snap @@ -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 diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__nested_projects_weird-stdout.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__nested_projects_weird-stdout.snap index bbdf4f6b..370ed871 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__nested_projects_weird-stdout.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__nested_projects_weird-stdout.snap @@ -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 diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties_duplicate-stdout.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties_duplicate-stdout.snap index 05d449d0..e9d435da 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties_duplicate-stdout.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties_duplicate-stdout.snap @@ -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 diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index 3527fdad..7966abb2 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -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 = 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() { diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 19f96413..e391c23b 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -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 = 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 { + 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 { + // 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) } diff --git a/src/syncback/property_filter.rs b/src/syncback/property_filter.rs index 0a850722..4adbd906 100644 --- a/src/syncback/property_filter.rs +++ b/src/syncback/property_filter.rs @@ -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 } diff --git a/src/syncback/snapshot.rs b/src/syncback/snapshot.rs index 67cc4d45..c56ee2a5 100644 --- a/src/syncback/snapshot.rs +++ b/src/syncback/snapshot.rs @@ -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 { diff --git a/src/syncback/stats.rs b/src/syncback/stats.rs new file mode 100644 index 00000000..ff67a100 --- /dev/null +++ b/src/syncback/stats.rs @@ -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>, + /// Set of unknown property names encountered (class.property format). + unknown_properties: Mutex>, +} + +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 { + 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 { + 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); + } +} diff --git a/src/web/api.rs b/src/web/api.rs index db8b8a56..09df7570 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -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, + stats: &crate::syncback::SyncbackStats, ) -> bool { let mut current_ref = target_ref; @@ -371,11 +379,9 @@ impl ApiService { if has_duplicate_siblings.contains(¤t_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, + 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)?; diff --git a/tests/rojo_test/syncback_util.rs b/tests/rojo_test/syncback_util.rs index 07e0089b..6b5f1f77 100644 --- a/tests/rojo_test/syncback_util.rs +++ b/tests/rojo_test/syncback_util.rs @@ -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 "/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, "/"); + + settings.bind(|| assert_snapshot!(format!("{name}-stdout"), normalized_stdout)); settings.bind(|| callback(project_path.as_path())) } diff --git a/tests/tests/clean_mode.rs b/tests/tests/clean_mode.rs index eb16b4d1..c1793358 100644 --- a/tests/tests/clean_mode.rs +++ b/tests/tests/clean_mode.rs @@ -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