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

Refactor Reconciler tests and update path handling logic

Browse source 
- Updated test descriptions in `diff.spec.lua` and `diff.stress.spec.lua` to reflect changes in behavior regarding duplicate-named children in both real and virtual DOM contexts.
- Adjusted assertions to ensure that duplicate-named children are correctly removed or added based on the new matching logic.
- Refactored `hydrate.lua` and `matching.lua` to streamline the matching process by removing unnecessary parameters, enhancing clarity and maintainability.
- Updated metadata references in various test files to include the `.model.json5` extension for consistency in path handling.

These changes improve the accuracy of reconciliation tests and enhance the overall robustness of the Reconciler's path handling logic.
This commit is contained in:
Joe 2026-02-17 04:46:54 -06:00
parent 3ad3c21c01
commit bb9d241253
18 changed files with 129 additions and 94 deletions
Download patch file Download diff file Expand all files Collapse all files

71
src/change_processor.rs
View file

@ -485,10 +485,8 @@ impl JobThreadContext {
// this function is called. The index still stores old filesystem
// paths. Remap them: replace the old directory segment with the
// new one so we can find the actual files.
let old_segments = crate::split_ref_path(old_path);
let old_segment = old_segments.last().map(|s| s.as_str()).unwrap_or(old_path);
let new_segments = crate::split_ref_path(new_path);
let new_segment = new_segments.last().map(|s| s.as_str()).unwrap_or(new_path);
let old_segment = old_path.rsplit('/').next().unwrap_or(old_path);
let new_segment = new_path.rsplit('/').next().unwrap_or(new_path);
// Pre-compute slugified forms for fallback comparison.
let slugified_old = if name_needs_slugify(old_segment) {
@ -1202,11 +1200,17 @@ impl JobThreadContext {
// Capture the old path BEFORE rename for Rojo_Ref_* path updates.
// Must be computed before the `tree.get_instance(id)` borrow.
let old_ref_path = if update.changed_name.is_some() {
Some(crate::ref_target_path_from_tree(&tree, id))
} else {
None
};
let old_ref_path =
if update.changed_name.is_some() || update.changed_class_name.is_some() {
Some(crate::ref_target_path_from_tree(&tree, id))
} else {
None
};
// The new filesystem name segment after a rename. Set during
// rename handling and used by the ref path update code to build
// the correct filesystem-name-based ref path.
let mut new_ref_segment: Option<String> = None;
if let Some(instance) = tree.get_instance(id) {
// Track the current source file path — rename and ClassName
@ -1278,6 +1282,8 @@ impl JobThreadContext {
// The init file moved with the directory.
overridden_source_path =
Some(new_dir_path.join(file_name));
new_ref_segment =
Some(slugified_new_name.clone());
let old_meta = grandparent.join(format!(
"{}.meta.json5",
dir_name
@ -1438,6 +1444,8 @@ impl JobThreadContext {
effective_meta_base = old_base;
} else {
overridden_source_path = Some(new_path.clone());
new_ref_segment =
Some(new_file_name.clone());
let old_meta = parent
.join(format!("{}.meta.json5", old_base));
let new_meta = parent.join(format!(
@ -1616,6 +1624,14 @@ impl JobThreadContext {
);
} else {
overridden_source_path = Some(new_path.clone());
// For standalone files, the ref path
// segment changes with the extension.
// For init files, the directory name
// (ref segment) is unchanged.
if !is_init {
new_ref_segment =
Some(new_file_name.clone());
}
}
}
} else {
@ -1717,31 +1733,32 @@ impl JobThreadContext {
//
// NOTE: The tree name hasn't been updated yet (apply_patch_set
// runs after this loop), so we can't use full_path_of for the
// new path. Construct it by replacing the last path segment.
// Use split_ref_path for escape-aware splitting (handles
// instance names containing "/").
// new path. Construct it by replacing the last path segment
// with the NEW filesystem name (set during rename handling).
if let Some(ref old_ref_path) = old_ref_path {
if let Some(ref new_name) = update.changed_name {
let segments = crate::split_ref_path(old_ref_path);
if let Some(ref segment) = new_ref_segment {
// Use the filesystem name computed during rename handling
let segments: Vec<&str> = old_ref_path.split('/').collect();
let new_ref_path = if segments.len() > 1 {
// Rebuild parent path from all segments except the last,
// re-escaping each segment to preserve the original format.
let parent_parts: Vec<_> = segments[..segments.len() - 1]
.iter()
.map(|s| crate::escape_ref_path_segment(s).into_owned())
.collect();
format!(
"{}/{}",
parent_parts.join("/"),
crate::escape_ref_path_segment(new_name)
)
let parent = segments[..segments.len() - 1].join("/");
format!("{}/{}", parent, segment)
} else {
// Root-level instance (no parent in path)
crate::escape_ref_path_segment(new_name).into_owned()
segment.clone()
};
if *old_ref_path != new_ref_path {
self.update_ref_paths_after_rename(old_ref_path, &new_ref_path);
}
} else if update.changed_name.is_some()
|| update.changed_class_name.is_some()
{
// Rename or class change was requested but no filesystem
// rename happened (e.g., ProjectNode, init-file class
// change where directory name stays the same). No ref
// path update needed.
log::trace!(
"Skipping ref path update for {:?}: no filesystem rename",
id
);
}
}
}

17
src/snapshot/tree.rs
View file

@ -10,7 +10,7 @@ use rbx_dom_weak::{
use crate::{multimap::MultiMap, RojoRef};
use super::{InstanceMetadata, InstanceSnapshot};
use super::{InstigatingSource, InstanceMetadata, InstanceSnapshot};
/// An expanded variant of rbx_dom_weak's `WeakDom` that tracks additional
/// metadata per instance that's Rojo-specific.
@ -291,8 +291,19 @@ impl RojoTree {
pub fn filesystem_name_for(&self, id: Ref) -> String {
if let Some(meta) = self.metadata_map.get(&id) {
if let Some(source) = &meta.instigating_source {
if let Some(name) = source.path().file_name().and_then(|f| f.to_str()) {
return name.to_string();
match source {
InstigatingSource::Path(_) => {
if let Some(name) = source.path().file_name().and_then(|f| f.to_str()) {
return name.to_string();
}
}
InstigatingSource::ProjectNode { .. } => {
// Project-sourced instance: use instance name, not the
// project file path (which would be e.g. "default.project.json5").
if let Some(inst) = self.inner.get_by_ref(id) {
return inst.name.clone();
}
}
}
}
}

14
src/snapshot_middleware/project.rs
View file

@ -628,6 +628,17 @@ pub fn syncback_project<'sync>(
}
}
// Project children with $path are expected in the old DOM (loaded
// from their own path) AND may also appear as filesystem children of
// the parent's scanned directory. Exclude them from duplicate
// detection so the project child loop below can match them.
let project_path_child_names: HashSet<String> = node
.children
.iter()
.filter(|(_, child_node)| child_node.path.is_some())
.map(|(name, _)| name.to_lowercase())
.collect();
let mut old_duplicate_names: HashSet<String> = HashSet::new();
{
let mut seen: HashSet<String> = HashSet::new();
@ -636,6 +647,9 @@ pub fn syncback_project<'sync>(
.get_old_instance(*child_ref)
.expect("all children of Instances should be in old DOM");
let lower = child.name().to_lowercase();
if project_path_child_names.contains(&lower) {
continue;
}
if !seen.insert(lower.clone()) {
old_duplicate_names.insert(lower);
}

12
src/syncback/dedup_suffix.rs
View file

@ -58,11 +58,6 @@ pub fn parse_dedup_suffix(stem: &str) -> Option<(&str, u32)> {
None
}
/// Checks whether a filename stem has a dedup suffix.
pub fn has_dedup_suffix(stem: &str) -> bool {
parse_dedup_suffix(stem).is_some()
}
/// Builds a dedup'd filename from a base stem, optional suffix number, and
/// extension.
///
@ -180,13 +175,6 @@ mod tests {
assert_eq!(parse_dedup_suffix("My Script~1"), Some(("My Script", 1)));
}
#[test]
fn has_suffix() {
assert!(has_dedup_suffix("Foo~1"));
assert!(!has_dedup_suffix("Foo"));
assert!(!has_dedup_suffix("Foo~abc"));
}
#[test]
fn build_name_file() {
assert_eq!(