Implement VFS Path normalization for improved cross-platform tree synchronization (#1201)

src/change_processor.rs

@@ -1,12 +1,12 @@
-use std::{
-    fs,
-    sync::{Arc, Mutex},
-};
-
 use crossbeam_channel::{select, Receiver, RecvError, Sender};
 use jod_thread::JoinHandle;
 use memofs::{IoResultExt, Vfs, VfsEvent};
 use rbx_dom_weak::types::{Ref, Variant};
+use std::path::PathBuf;
+use std::{
+    fs,
+    sync::{Arc, Mutex},
+};
 
 use crate::{
     message_queue::MessageQueue,
@@ -114,6 +114,49 @@ struct JobThreadContext {
 }
 
 impl JobThreadContext {
+    /// 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
+    /// in the tree.
+    /// It then computes and applies changes for each affected instance ID and
+    /// returns a vector of applied patch sets.
+    fn apply_patches(&self, path: PathBuf) -> Vec<AppliedPatchSet> {
+        let mut tree = self.tree.lock().unwrap();
+        let mut applied_patches = Vec::new();
+
+        // Find the nearest ancestor to this path that has
+        // associated instances in the tree. This helps make sure
+        // that we handle additions correctly, especially if we
+        // receive events for descendants of a large tree being
+        // created all at once.
+        let mut current_path = path.as_path();
+        let affected_ids = loop {
+            let ids = tree.get_ids_at_path(current_path);
+
+            log::trace!("Path {} affects IDs {:?}", current_path.display(), ids);
+
+            if !ids.is_empty() {
+                break ids.to_vec();
+            }
+
+            log::trace!("Trying parent path...");
+            match current_path.parent() {
+                Some(parent) => current_path = parent,
+                None => break Vec::new(),
+            }
+        };
+
+        for id in affected_ids {
+            if let Some(patch) = compute_and_apply_changes(&mut tree, &self.vfs, id) {
+                if !patch.is_empty() {
+                    applied_patches.push(patch);
+                }
+            }
+        }
+
+        applied_patches
+    }
+
     fn handle_vfs_event(&self, event: VfsEvent) {
         log::trace!("Vfs event: {:?}", event);
 
@@ -125,41 +168,16 @@ impl JobThreadContext {
         // For a given VFS event, we might have many changes to different parts
         // of the tree. Calculate and apply all of these changes.
         let applied_patches = match event {
-            VfsEvent::Create(path) | VfsEvent::Remove(path) | VfsEvent::Write(path) => {
-                let mut tree = self.tree.lock().unwrap();
-                let mut applied_patches = Vec::new();
-
-                // Find the nearest ancestor to this path that has
-                // associated instances in the tree. This helps make sure
-                // that we handle additions correctly, especially if we
-                // receive events for descendants of a large tree being
-                // created all at once.
-                let mut current_path = path.as_path();
-                let affected_ids = loop {
-                    let ids = tree.get_ids_at_path(current_path);
-
-                    log::trace!("Path {} affects IDs {:?}", current_path.display(), ids);
-
-                    if !ids.is_empty() {
-                        break ids.to_vec();
-                    }
-
-                    log::trace!("Trying parent path...");
-                    match current_path.parent() {
-                        Some(parent) => current_path = parent,
-                        None => break Vec::new(),
-                    }
-                };
-
-                for id in affected_ids {
-                    if let Some(patch) = compute_and_apply_changes(&mut tree, &self.vfs, id) {
-                        if !patch.is_empty() {
-                            applied_patches.push(patch);
-                        }
-                    }
-                }
-
-                applied_patches
+            VfsEvent::Create(path) | VfsEvent::Write(path) => {
+                self.apply_patches(self.vfs.canonicalize(&path).unwrap())
+            }
+            VfsEvent::Remove(path) => {
+                // MemoFS does not track parent removals yet, so we can canonicalize
+                // the parent path safely and then append the removed path's file name.
+                let parent = path.parent().unwrap();
+                let file_name = path.file_name().unwrap();
+                let parent_normalized = self.vfs.canonicalize(parent).unwrap();
+                self.apply_patches(parent_normalized.join(file_name))
             }
             _ => {
                 log::warn!("Unhandled VFS event: {:?}", event);

src/snapshot_middleware/csv.rs

@@ -42,7 +42,7 @@ pub fn snapshot_csv(
         .metadata(
             InstanceMetadata::new()
                 .instigating_source(path)
-                .relevant_paths(vec![path.to_path_buf()]),
+                .relevant_paths(vec![vfs.canonicalize(path)?]),
         );
 
     AdjacentMetadata::read_and_apply_all(vfs, path, name, &mut snapshot)?;

src/snapshot_middleware/dir.rs

@@ -62,18 +62,19 @@ pub fn snapshot_dir_no_meta(
         }
     }
 
+    let normalized_path = vfs.canonicalize(path)?;
     let relevant_paths = vec![
-        path.to_path_buf(),
+        normalized_path.clone(),
         // TODO: We shouldn't need to know about Lua existing in this
         // middleware. Should we figure out a way for that function to add
         // relevant paths to this middleware?
-        path.join("init.lua"),
-        path.join("init.luau"),
-        path.join("init.server.lua"),
-        path.join("init.server.luau"),
-        path.join("init.client.lua"),
-        path.join("init.client.luau"),
-        path.join("init.csv"),
+        normalized_path.join("init.lua"),
+        normalized_path.join("init.luau"),
+        normalized_path.join("init.server.lua"),
+        normalized_path.join("init.server.luau"),
+        normalized_path.join("init.client.lua"),
+        normalized_path.join("init.client.luau"),
+        normalized_path.join("init.csv"),
     ];
 
     let snapshot = InstanceSnapshot::new()

src/snapshot_middleware/json.rs

@@ -32,7 +32,7 @@ pub fn snapshot_json(
         .metadata(
             InstanceMetadata::new()
                 .instigating_source(path)
-                .relevant_paths(vec![path.to_path_buf()])
+                .relevant_paths(vec![vfs.canonicalize(path)?])
                 .context(context),
         );
 

src/snapshot_middleware/json_model.rs

@@ -59,7 +59,7 @@ pub fn snapshot_json_model(
     snapshot.metadata = snapshot
         .metadata
         .instigating_source(path)
-        .relevant_paths(vec![path.to_path_buf()])
+        .relevant_paths(vec![vfs.canonicalize(path)?])
         .context(context)
         .specified_id(id)
         .schema(schema);

src/snapshot_middleware/lua.rs

@@ -88,7 +88,7 @@ pub fn snapshot_lua(
         .metadata(
             InstanceMetadata::new()
                 .instigating_source(path)
-                .relevant_paths(vec![path.to_path_buf()])
+                .relevant_paths(vec![vfs.canonicalize(path)?])
                 .context(context),
         );
 

src/snapshot_middleware/rbxm.rs

@@ -28,7 +28,7 @@ pub fn snapshot_rbxm(
             .metadata(
                 InstanceMetadata::new()
                     .instigating_source(path)
-                    .relevant_paths(vec![path.to_path_buf()])
+                    .relevant_paths(vec![vfs.canonicalize(path)?])
                     .context(context),
             );
 

src/snapshot_middleware/rbxmx.rs

@@ -31,7 +31,7 @@ pub fn snapshot_rbxmx(
             .metadata(
                 InstanceMetadata::new()
                     .instigating_source(path)
-                    .relevant_paths(vec![path.to_path_buf()])
+                    .relevant_paths(vec![vfs.canonicalize(path)?])
                     .context(context),
             );
 

src/snapshot_middleware/toml.rs

@@ -31,7 +31,7 @@ pub fn snapshot_toml(
         .metadata(
             InstanceMetadata::new()
                 .instigating_source(path)
-                .relevant_paths(vec![path.to_path_buf()])
+                .relevant_paths(vec![vfs.canonicalize(path)?])
                 .context(context),
         );
 

src/snapshot_middleware/txt.rs

@@ -28,7 +28,7 @@ pub fn snapshot_txt(
         .metadata(
             InstanceMetadata::new()
                 .instigating_source(path)
-                .relevant_paths(vec![path.to_path_buf()])
+                .relevant_paths(vec![vfs.canonicalize(path)?])
                 .context(context),
         );
 

src/snapshot_middleware/yaml.rs

@@ -37,7 +37,7 @@ pub fn snapshot_yaml(
         .metadata(
             InstanceMetadata::new()
                 .instigating_source(path)
-                .relevant_paths(vec![path.to_path_buf()])
+                .relevant_paths(vec![vfs.canonicalize(path)?])
                 .context(context),
         );