diff --git a/.cursor/plans/fix_sync_system_issues_e569840d.plan.md b/.cursor/plans/fix_sync_system_issues_e569840d.plan.md new file mode 100644 index 00000000..aabdc358 --- /dev/null +++ b/.cursor/plans/fix_sync_system_issues_e569840d.plan.md @@ -0,0 +1,553 @@ +--- +name: Fix sync system issues +overview: Fix all 8 issues identified in the api.rs sync system audit — from standalone script child placement to file watcher echo suppression — across src/web/api.rs, src/change_processor.rs, and src/serve_session.rs. +todos: + - id: issue-1-standalone-conversion + content: "Issue 1: Extract standalone-to-directory conversion into shared helper and use in syncback_added_instance when parent is a standalone script file" + status: completed + - id: issue-2-property-persistence + content: "Issue 2: Add syncback_updated_properties method to write non-Source property changes to meta/model files in handle_api_write" + status: completed + - id: issue-3-path-in-format + content: "Issue 3: Make ExistingFileFormat carry detected PathBuf and use it when writing to preserve .model.json vs .model.json5" + status: completed + - id: issue-4-stale-removals + content: "Issue 4: Include removed IDs in PatchSet and add existence guard in ChangeProcessor to avoid double-delete errors" + status: completed + - id: issue-5-lock-splitting + content: "Issue 5: Split handle_api_write into gather phase (with lock) and execute phase (without lock) using SyncbackAction enum" + status: completed + - id: issue-6-project-parents + content: "Issue 6: Handle InstigatingSource::ProjectNode by resolving node.path relative to project file directory" + status: completed + - id: issue-7-echo-suppression + content: "Issue 7: Add suppressed_paths set to ServeSession, populate before writes, check in handle_vfs_event" + status: completed + - id: issue-8-rename-classname + content: "Issue 8: Implement file rename for name changes and extension rename for script class changes in ChangeProcessor" + status: completed + - id: issue-9-filewatcher-desync + content: "Issue 9 [CRITICAL]: Fix rapid delete+recreate desync — add recovery mechanism after Remove events, retry canonicalize on failure, stop unwatching on Remove, and add pedantic INFO-level logging for ALL file watcher events and decisions" + status: completed + - id: testing + content: Add integration tests in tests/tests/serve.rs covering each fix + status: completed +isProject: false +--- + +# Fix All Sync System Issues + +## Data Flow Context + +```mermaid +sequenceDiagram + participant Plugin as Studio_Plugin + participant API as handle_api_write + participant FS as Filesystem + participant FW as FileWatcher_VFS + participant CP as ChangeProcessor + participant Tree as RojoTree + participant MQ as MessageQueue + + Plugin->>API: POST /api/write + Note over API: syncback_added_instance + API->>FS: Create files + Note over API: syncback_removed_instance + API->>FS: Delete files + API->>CP: PatchSet (updates only) + CP->>FS: Write Source to file + CP->>Tree: apply_patch_set + CP->>MQ: AppliedPatchSet + FW->>CP: VfsEvent (echo!) + CP->>Tree: Re-snapshot and patch + CP->>MQ: Redundant AppliedPatchSet + MQ->>Plugin: WebSocket messages +``` + + + +All fixes target eliminating the problems in this flow. + +--- + +## Issue 1 — [HIGH] Standalone script children created as siblings + +**File:** [src/web/api.rs](src/web/api.rs) — `syncback_added_instance` (line ~528) + +**Problem:** When a new child is added to a standalone script parent (e.g., `MyModule.luau`), the parent path resolves to the directory containing the script, so the child is created as a sibling instead of inside a `MyModule/` directory. + +**Fix:** After resolving `parent_path` and finding it is a file (not a directory), check if the parent is a script class. If so, convert it to directory format before creating the child: + +1. Create directory `parent_dir/ParentName/` +2. Move `ParentName.luau` to `ParentName/init.luau` (with correct suffix for Script/LocalScript) +3. Delete the old standalone file +4. Remove any adjacent `.meta.json5` (move content to `init.meta.json5` inside the new dir) +5. Set `parent_dir` to the new directory +6. Proceed with normal child creation + +The conversion logic already exists in `syncback_update_existing_instance` (lines 620-670). Extract it into a shared helper: + +```rust +fn convert_standalone_to_directory( + &self, + parent_path: &Path, + parent_name: &str, + class_name: &str, +) -> anyhow::Result +``` + +--- + +## Issue 2 — [HIGH] Non-Source property updates not persisted to disk + +**Files:** [src/web/api.rs](src/web/api.rs) — `handle_api_write` (line ~300), [src/change_processor.rs](src/change_processor.rs) — `handle_tree_event` (line ~299) + +**Problem:** `ChangeProcessor::handle_tree_event` only writes `Source` to disk. All other property changes (Attributes, etc.) are applied to the in-memory tree only and lost on restart. + +**Fix:** In `handle_api_write`, before sending the PatchSet, process non-Source property changes for each `updated_instance`: + +1. Look up the instance in the tree to get its `instigating_source` path +2. Determine the meta file path: + - Scripts in directories: `dir/init.meta.json5` + - Standalone scripts: `parent/Name.meta.json5` + - Model files: update the `.model.json5` directly +3. Read existing meta file (if any), merge changed properties, write back +4. Reuse existing helpers: `filter_properties_for_meta`, `build_meta_object`, `variant_to_json` + +Add a new method to `ApiService`: + +```rust +fn syncback_updated_properties( + &self, + update: &InstanceUpdate, + tree: &RojoTree, +) -> anyhow::Result<()> +``` + +This processes properties **other than Source** (Source is handled by ChangeProcessor already). The method: + +- Gets the instance from the tree +- Gets `instigating_source.path()` +- Determines if it's a directory or standalone file +- Reads existing meta/model file, merges changes, writes back + +--- + +## Issue 3 — [MEDIUM] Potential `.model.json` / `.model.json5` duplicates + +**File:** [src/web/api.rs](src/web/api.rs) — `ExistingFileFormat` enum and `detect_existing_script_format` (line ~864), `syncback_instance_to_path_with_stats` (line ~1362) + +**Problem:** `detect_existing_script_format` returns just `Standalone` without the actual file path. New files are always created as `.model.json5`, potentially alongside an existing `.model.json`. + +**Fix:** Make `ExistingFileFormat` carry the detected path: + +```rust +enum ExistingFileFormat { + None, + Standalone(PathBuf), // was: Standalone + Directory(PathBuf), // was: Directory +} +``` + +Update `detect_existing_script_format` to return the actual path found. Update all match arms in `syncback_instance_to_path_with_stats` to use the carried path when writing to an existing file: + +- For `Standalone(path)`: write to `path` instead of always generating `.model.json5` +- For `Directory(path)`: use `path` for the directory + +--- + +## Issue 4 — [MEDIUM] Removed instances leave stale window in tree + +**File:** [src/web/api.rs](src/web/api.rs) — `handle_api_write` (line ~334), [src/change_processor.rs](src/change_processor.rs) — `handle_tree_event` (line ~249) + +**Problem:** `handle_api_write` sends `removed_instances: Vec::new()` even when removals happened. The tree only updates when the file watcher detects deletions. + +**Fix:** + +1. In `handle_api_write`, include the removed IDs in the PatchSet: + +```rust +tree_mutation_sender + .send(PatchSet { + removed_instances: request.removed.clone(), + added_instances: Vec::new(), + updated_instances, + }) +``` + +1. In `ChangeProcessor::handle_tree_event`, guard file deletion with an existence check to avoid spurious error logs when files were already deleted by `syncback_removed_instance`: + +```rust +InstigatingSource::Path(path) => { + if path.exists() { + log::info!("Two-way sync: Removing file {}", path.display()); + if let Err(err) = fs::remove_file(path) { ... } + } else { + log::trace!("File already removed: {}", path.display()); + } +} +``` + +Also handle directory removal (currently only `fs::remove_file`, should also check `fs::remove_dir_all`). + +--- + +## Issue 5 — [MEDIUM] Tree lock held during filesystem I/O + +**File:** [src/web/api.rs](src/web/api.rs) — `handle_api_write` (line ~277) + +**Problem:** The tree mutex is held during all synchronous filesystem operations in `syncback_added_instance`, blocking other threads. + +**Fix:** Split the operation into two phases: + +**Phase 1 — Gather (with lock):** Collect all information needed from the tree: + +- Parent paths for each added instance +- Existing child lookups (for dedup) +- Duplicate sibling cache +- Instigating source paths for removed instances + +Store results in a `Vec` enum: + +```rust +enum SyncbackAction { + CreateNew { added: AddedInstance, parent_dir: PathBuf }, + UpdateExisting { added: AddedInstance, existing_path: PathBuf }, + Remove { path: PathBuf, is_dir: bool }, + Skip { name: String, reason: String }, +} +``` + +**Phase 2 — Execute (without lock):** Release tree lock, then execute all filesystem operations from the collected actions. + +--- + +## Issue 6 — [LOW] Can't add children to project-defined parents + +**File:** [src/web/api.rs](src/web/api.rs) — `syncback_added_instance` (line ~520) + +**Problem:** When parent's `instigating_source` is `InstigatingSource::ProjectNode`, `.path()` returns the project file path, not the filesystem directory for children. + +**Fix:** After getting the instigating source, handle `ProjectNode` separately: + +```rust +let parent_dir = match instigating_source { + InstigatingSource::Path(path) => { + if path.is_dir() { path.clone() } else { path.parent().unwrap().to_path_buf() } + } + InstigatingSource::ProjectNode { path: project_path, node, .. } => { + if let Some(path_node) = &node.path { + let fs_path = path_node.path(); + let resolved = if fs_path.is_relative() { + project_path.parent().unwrap().join(fs_path) + } else { + fs_path.to_path_buf() + }; + if resolved.is_dir() { resolved } else { resolved.parent().unwrap().to_path_buf() } + } else { + anyhow::bail!( + "Cannot add '{}' - parent '{}' is defined in project file without $path", + added.name, name + ); + } + } +}; +``` + +This uses the `node.path` (`$path` field from [src/project.rs](src/project.rs) `ProjectNode` struct, line ~372) resolved relative to the project file directory. + +--- + +## Issue 7 — [LOW] File watcher echo after syncback + +**Files:** [src/serve_session.rs](src/serve_session.rs), [src/change_processor.rs](src/change_processor.rs), [src/web/api.rs](src/web/api.rs) + +**Problem:** When the API writes files to disk, the file watcher picks them up and creates redundant patches. + +**Fix:** Add a write-suppression set shared between API and ChangeProcessor: + +1. Add to `ServeSession`: + +```rust +suppressed_paths: Arc>> +``` + +1. In `syncback_added_instance` / `syncback_removed_instance` / `syncback_instance_to_path_with_stats`, before each `fs::write` / `fs::remove_*` / `fs::create_dir_all`, add the path to the suppression set. +2. In `ChangeProcessor::handle_vfs_event`, check the suppression set before processing: + +```rust +fn handle_vfs_event(&self, event: VfsEvent) { + let event_path = match &event { + VfsEvent::Create(p) | VfsEvent::Write(p) | VfsEvent::Remove(p) => p.clone(), + }; + + // Check and remove from suppression set (one-shot suppression) + { + let mut suppressed = self.suppressed_paths.lock().unwrap(); + if suppressed.remove(&event_path) { + self.vfs.commit_event(&event).expect("Error applying VFS change"); + log::trace!("Suppressed echo event for {}", event_path.display()); + return; + } + } + + // ... normal processing ... +} +``` + +Still commit the event to VFS (so VFS stays consistent), but skip patch computation and message broadcasting. + +--- + +## Issue 8 — [INFO] Rename/ClassName persistence + +**File:** [src/change_processor.rs](src/change_processor.rs) — `handle_tree_event` (line ~287) + +**Problem:** Name and ClassName changes are logged as warnings and only applied to in-memory tree. + +**Fix for renames:** Replace `log::warn!("Cannot rename instances yet.")` with: + +1. Get instance's `instigating_source` path +2. Compute new path based on new name (preserve extension/format) +3. Rename file or directory on disk (`fs::rename`) +4. Update any adjacent meta file name (e.g., `OldName.meta.json5` -> `NewName.meta.json5`) + +**Fix for ClassName changes:** Replace `log::warn!("Cannot change ClassName yet.")` with: + +1. For script classes: compute old and new file extensions +2. Rename the file (e.g., `Foo.server.luau` -> `Foo.client.luau`) +3. For non-script to script or vice versa: convert between `.model.json5` and `.luau` formats + +ClassName changes are complex — scope the initial implementation to script class transitions only (Script <-> LocalScript, etc.) and log a warning for cross-category changes (script to non-script). + +--- + +## Issue 9 — [CRITICAL] File watcher loses track of files after rapid delete+recreate + +**Files:** [src/change_processor.rs](src/change_processor.rs), [crates/memofs/src/lib.rs](crates/memofs/src/lib.rs), [crates/memofs/src/std_backend.rs](crates/memofs/src/std_backend.rs) + +**Reproduction:** Cursor agent modifies a file, user clicks "undo", file is deleted and immediately recreated. Rojo thinks the file is deleted even though it exists on disk. Restarting Rojo serve fixes it. + +**Root cause analysis — three contributing factors:** + +```mermaid +sequenceDiagram + participant Editor as Cursor_Editor + participant FS as Filesystem + participant Notify as notify_debouncer + participant CP as ChangeProcessor + participant Tree as RojoTree + + Editor->>FS: Delete file (undo step 1) + Editor->>FS: Recreate file (undo step 2) + Note over FS: File exists on disk + Notify->>CP: VfsEvent::Remove + Note over Notify: Create event LOST + CP->>CP: commit_event: unwatch(path) + CP->>CP: vfs.metadata(path) = NotFound + Note over CP: File doesnt exist YET + CP->>Tree: remove instance + Note over Tree: Instance gone forever + Note over FS: File reappears on disk + Note over CP: No Create event arrives +``` + + + +**Factor 1: `commit_event(Remove)` unwatches the path** ([crates/memofs/src/lib.rs](crates/memofs/src/lib.rs) line 239) + +```rust +fn commit_event(&mut self, event: &VfsEvent) -> io::Result<()> { + if let VfsEvent::Remove(path) = event { + let _ = self.backend.unwatch(path); // <-- problematic + } + Ok(()) +} +``` + +If the removed path was directly watched (in the `watches` set), it gets unwatched from `notify`. When the path is recreated, the parent's recursive watch *might* cover it, but on Windows this is unreliable for detecting the recreation event. + +**Factor 2: Create/Write events silently dropped when `canonicalize` fails** ([src/change_processor.rs](src/change_processor.rs) line 183) + +```rust +Err(_) => { + log::trace!( // <-- INVISIBLE at normal log levels + "Skipping create/write event for {:?} - path no longer exists", + path + ); + Vec::new() +} +``` + +On Windows, `canonicalize` calls `GetFinalPathNameByHandleW`, which can fail if the file is still being written/locked by another process. The Create event is silently dropped at `trace` level — completely invisible. + +**Factor 3: Genuine race condition between file operations and event processing.** The `notify_debouncer_full` with 50ms timeout can coalesce a rapid Remove+Create into just a Remove on Windows if `ReadDirectoryChangesW` misses the recreation or the debouncer's cache only tracks the removal. + +**Fix — four layers of defense:** + +### Layer 1: Pedantic INFO-level logging for ALL file watcher decisions + +Change ALL event and decision logging in `handle_vfs_event` from `debug`/`trace` to `info`: + +```rust +fn handle_vfs_event(&self, event: VfsEvent) { + // Log EVERY event at INFO for diagnostics + match &event { + VfsEvent::Create(path) => log::info!("VFS event: CREATE {}", path.display()), + VfsEvent::Write(path) => log::info!("VFS event: WRITE {}", path.display()), + VfsEvent::Remove(path) => log::info!("VFS event: REMOVE {}", path.display()), + _ => log::info!("VFS event: OTHER {:?}", event), + } + // ... + // Log EVERY decision point: + // - "Canonicalize succeeded: {path}" / "Canonicalize FAILED: {path} - {err}" + // - "apply_patches found IDs: {ids:?} at path: {path}" + // - "compute_and_apply_changes: path exists={exists}, result={added/removed/updated}" + // - "Patch applied: {summary}" +} +``` + +Also log at INFO in `compute_and_apply_changes`: + +- Whether `vfs.metadata(path)` found the file or not +- Whether the snapshot produced changes +- What instances were added/removed/updated + +### Layer 2: Stop unwatching paths on Remove events + +In [crates/memofs/src/lib.rs](crates/memofs/src/lib.rs), remove the unwatch from `commit_event`: + +```rust +fn commit_event(&mut self, event: &VfsEvent) -> io::Result<()> { + // NOTE: We intentionally do NOT unwatch on Remove. + // The path may be recreated immediately (editor undo), and unwatching + // causes future events to be missed. Stale watches are harmless — + // notify silently ignores events for non-existent paths, and the + // watch will be cleaned up when the parent is unwatched. + Ok(()) +} +``` + +This is safe because: + +- `notify` silently handles watches on non-existent paths (no errors) +- The parent directory's recursive watch provides the actual coverage +- Stale entries in the `watches` set are harmless overhead + +### Layer 3: Recovery check after Remove events that cause instance removal + +In `compute_and_apply_changes` ([src/change_processor.rs](src/change_processor.rs) line 391), after the instance would be removed, verify the path doesn't exist on the REAL filesystem (bypassing any VFS state): + +```rust +Ok(None) => { + // Path not found in VFS. Before removing, verify it's truly gone + // from the REAL filesystem. On Windows, rapid delete+recreate can + // cause Remove events where the file has already been recreated. + if std::fs::metadata(path).is_ok() { + log::info!( + "Recovery: path {} was reported removed but exists on disk. Re-snapshotting.", + path.display() + ); + // File was recreated — re-snapshot instead of removing + let snapshot = match snapshot_from_vfs(&metadata.context, vfs, path) { + Ok(snapshot) => snapshot, + Err(err) => { + log::error!("Recovery snapshot error: {:?}", err); + return None; + } + }; + let patch_set = compute_patch_set(snapshot, tree, id); + apply_patch_set(tree, patch_set) + } else { + // Path is genuinely gone + let mut patch_set = PatchSet::new(); + patch_set.removed_instances.push(id); + apply_patch_set(tree, patch_set) + } +} +``` + +Note: This uses `std::fs::metadata` directly (not `vfs.metadata`) because the VFS may have stale state after `commit_event(Remove)` was called. The real filesystem is the source of truth. + +### Layer 4: Retry canonicalize failures for Create/Write events + +In `handle_vfs_event`, when `canonicalize` fails for a Create/Write event, retry with a short delay instead of silently dropping: + +```rust +VfsEvent::Create(path) | VfsEvent::Write(path) => { + match self.vfs.canonicalize(&path) { + Ok(canonical_path) => self.apply_patches(canonical_path), + Err(err) => { + log::info!( + "VFS: canonicalize failed for {} ({}), retrying...", + path.display(), err + ); + // Retry after brief delay — file may still be locked by editor + std::thread::sleep(std::time::Duration::from_millis(50)); + match self.vfs.canonicalize(&path) { + Ok(canonical_path) => { + log::info!("VFS: retry succeeded for {}", path.display()); + self.apply_patches(canonical_path) + } + Err(err2) => { + log::warn!( + "VFS: Create/Write event DROPPED for {} after retry — \ + canonicalize failed: {}. File may be desynchronized!", + path.display(), err2 + ); + Vec::new() + } + } + } + } +} +``` + +### Layer 5 (post-Remove sweep): After processing a Remove event that caused instance removal, schedule a filesystem re-check + +Add a `pending_recovery` field to `JobThreadContext`: + +```rust +struct JobThreadContext { + tree: Arc>, + vfs: Arc, + message_queue: Arc>, + /// Paths recently removed that should be re-checked after a delay + pending_recovery: Mutex>, +} +``` + +After each event is processed in the `select!` loop, check `pending_recovery` for paths older than 200ms. For each, verify the path on the real filesystem and trigger a re-snapshot if it exists: + +```rust +loop { + select! { + recv(vfs_receiver) -> event => { + task.handle_vfs_event(event?); + task.process_pending_recoveries(); + }, + // ... other arms ... + default(Duration::from_millis(500)) => { + // Periodic check even when no events arrive + task.process_pending_recoveries(); + } + } +} +``` + +This catches the case where the Remove event was processed correctly (file was genuinely gone at that moment), but the file was recreated shortly after, and no Create event was generated. + +--- + +## Testing Strategy + +- Add integration tests in [tests/tests/serve.rs](tests/tests/serve.rs) for each fix +- Issue 1: Test adding a child to a standalone script via the write API, verify directory conversion +- Issue 2: Test property update persistence, verify meta file creation +- Issue 3: Test with existing `.model.json` files, verify no duplicates created +- Issue 4: Test removal appears in tree immediately after write response +- Issue 7: Test that syncback doesn't cause redundant WebSocket messages +- Issue 9: Test rapid delete+recreate of files, verify instance survives in tree. Test with both individual files and directories. Verify INFO logs capture all events. +- Use existing test utilities from [tests/rojo_test/serve_util.rs](tests/rojo_test/serve_util.rs) +- Existing stress tests in [crates/memofs/src/std_backend.rs](crates/memofs/src/std_backend.rs) (`stress_create_delete_recreate_cycles`, `stress_undo_redo_simulation`) provide unit-level coverage for the debouncer + diff --git a/crates/memofs/src/lib.rs b/crates/memofs/src/lib.rs index 411b6f41..2a141479 100644 --- a/crates/memofs/src/lib.rs +++ b/crates/memofs/src/lib.rs @@ -236,10 +236,13 @@ impl VfsInner { } fn commit_event(&mut self, event: &VfsEvent) -> io::Result<()> { - if let VfsEvent::Remove(path) = event { - let _ = self.backend.unwatch(path); - } - + // NOTE: We intentionally do NOT unwatch on Remove events. + // The path may be recreated immediately (e.g., editor undo), and + // unwatching causes future events for the recreated path to be missed. + // Stale watches are harmless — notify silently ignores events for + // non-existent paths, and the watch will be cleaned up when the + // parent is unwatched. + let _ = event; Ok(()) } } diff --git a/src/change_processor.rs b/src/change_processor.rs index a5d9fb58..eb0fbf12 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -3,6 +3,7 @@ use jod_thread::JoinHandle; use memofs::{IoResultExt, Vfs, VfsEvent}; use rbx_dom_weak::types::{Ref, Variant}; use std::path::PathBuf; +use std::time::{Duration, Instant}; use std::{ fs, sync::{Arc, Mutex}, @@ -51,6 +52,7 @@ impl ChangeProcessor { vfs: Arc, message_queue: Arc>, tree_mutation_receiver: Receiver, + suppressed_paths: Arc>>, ) -> Self { let (shutdown_sender, shutdown_receiver) = crossbeam_channel::bounded(1); let vfs_receiver = vfs.event_receiver(); @@ -58,6 +60,8 @@ impl ChangeProcessor { tree, vfs, message_queue, + pending_recovery: Mutex::new(Vec::new()), + suppressed_paths, }; let job_thread = jod_thread::Builder::new() @@ -69,6 +73,7 @@ impl ChangeProcessor { select! { recv(vfs_receiver) -> event => { task.handle_vfs_event(event?); + task.process_pending_recoveries(); }, recv(tree_mutation_receiver) -> patch_set => { task.handle_tree_event(patch_set?); @@ -77,6 +82,11 @@ impl ChangeProcessor { log::trace!("ChangeProcessor shutdown signal received..."); return Ok(()); }, + default(Duration::from_millis(500)) => { + // Periodic sweep even when no events arrive — + // catches files recreated after their Remove was processed. + task.process_pending_recoveries(); + }, } } }) @@ -111,6 +121,16 @@ struct JobThreadContext { /// Whenever changes are applied to the DOM, we should push those changes /// into this message queue to inform any connected clients. message_queue: Arc>, + + /// Paths recently removed from the tree that should be re-checked after a + /// delay. On Windows, rapid delete+recreate (e.g., editor undo) can cause + /// the Remove event to arrive but the Create event to be lost. We record + /// removed paths here and periodically verify they are still gone. + pending_recovery: Mutex>, + + /// Paths recently written by the API's syncback. Events for these paths + /// are suppressed (one-shot) to avoid redundant re-snapshots. + suppressed_paths: Arc>>, } impl JobThreadContext { @@ -133,23 +153,41 @@ impl JobThreadContext { let affected_ids = loop { let ids = tree.get_ids_at_path(current_path); - log::trace!("Path {} affects IDs {:?}", current_path.display(), ids); + log::info!( + "apply_patches: path {} affects IDs {:?}", + current_path.display(), + ids + ); if !ids.is_empty() { break ids.to_vec(); } - log::trace!("Trying parent path..."); + log::info!("apply_patches: no IDs at {}, trying parent...", current_path.display()); match current_path.parent() { Some(parent) => current_path = parent, None => break Vec::new(), } }; + if affected_ids.is_empty() { + log::info!( + "apply_patches: no affected instances found for path {}", + path.display() + ); + } + 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); + if let Some(result) = compute_and_apply_changes(&mut tree, &self.vfs, id) { + // If an instance was removed, schedule a recovery check + // in case the path is recreated momentarily. + if let Some(removed_path) = result.removed_path { + let mut pending = self.pending_recovery.lock().unwrap(); + pending.push((removed_path, Instant::now())); + } + + if !result.applied.is_empty() { + applied_patches.push(result.applied); } } } @@ -158,12 +196,31 @@ impl JobThreadContext { } fn handle_vfs_event(&self, event: VfsEvent) { - // Log VFS events at debug level (not too verbose, but visible when needed) + // Log EVERY VFS event at INFO level for diagnostics. + // This is intentionally verbose — it is critical for debugging + // file watcher desync issues (e.g., rapid delete+recreate). match &event { - VfsEvent::Create(path) => log::debug!("VFS: File created: {}", path.display()), - VfsEvent::Write(path) => log::debug!("VFS: File modified: {}", path.display()), - VfsEvent::Remove(path) => log::debug!("VFS: File removed: {}", path.display()), - _ => log::trace!("VFS: Other event: {:?}", event), + VfsEvent::Create(path) => log::info!("VFS event: CREATE {}", path.display()), + VfsEvent::Write(path) => log::info!("VFS event: WRITE {}", path.display()), + VfsEvent::Remove(path) => log::info!("VFS event: REMOVE {}", path.display()), + _ => log::info!("VFS event: OTHER {:?}", event), + } + + // Check if this event should be suppressed (one-shot, from API syncback). + let event_path = match &event { + VfsEvent::Create(p) | VfsEvent::Write(p) | VfsEvent::Remove(p) => Some(p.clone()), + _ => None, + }; + if let Some(ref path) = event_path { + let mut suppressed = self.suppressed_paths.lock().unwrap(); + if suppressed.remove(path) { + // Still commit so VFS stays consistent, but skip patching. + self.vfs + .commit_event(&event) + .expect("Error applying VFS change"); + log::info!("VFS event SUPPRESSED (API syncback echo): {}", path.display()); + return; + } } // Update the VFS immediately with the event. @@ -177,15 +234,43 @@ impl JobThreadContext { VfsEvent::Create(path) | VfsEvent::Write(path) => { // The path might have been deleted before we could process this event // (e.g., during syncback when directories are being removed). - // In that case, just skip this event. + // If canonicalize fails, retry once after a brief delay — on Windows, + // the file may still be locked by the editor process. match self.vfs.canonicalize(&path) { - Ok(canonical_path) => self.apply_patches(canonical_path), - Err(_) => { - log::trace!( - "Skipping create/write event for {:?} - path no longer exists", - path + Ok(canonical_path) => { + log::info!( + "VFS: canonicalize OK for {} -> {}", + path.display(), + canonical_path.display() ); - Vec::new() + self.apply_patches(canonical_path) + } + Err(err) => { + log::info!( + "VFS: canonicalize FAILED for {} ({}), retrying after 50ms...", + path.display(), + err + ); + std::thread::sleep(Duration::from_millis(50)); + match self.vfs.canonicalize(&path) { + Ok(canonical_path) => { + log::info!( + "VFS: retry canonicalize OK for {} -> {}", + path.display(), + canonical_path.display() + ); + self.apply_patches(canonical_path) + } + Err(err2) => { + log::warn!( + "VFS: Create/Write event DROPPED for {} after retry — \ + canonicalize failed: {}. File may be desynchronized!", + path.display(), + err2 + ); + Vec::new() + } + } } } } @@ -197,12 +282,19 @@ impl JobThreadContext { let parent = path.parent().unwrap(); let file_name = path.file_name().unwrap(); match self.vfs.canonicalize(parent) { - Ok(parent_normalized) => self.apply_patches(parent_normalized.join(file_name)), - Err(_) => { - // Parent directory was also deleted, skip this event - log::trace!( - "Skipping remove event for {:?} - parent no longer exists", - path + Ok(parent_normalized) => { + let resolved = parent_normalized.join(file_name); + log::info!( + "VFS: Remove resolved to {}", + resolved.display() + ); + self.apply_patches(resolved) + } + Err(err) => { + log::info!( + "VFS: Skipping remove event for {} — parent no longer exists: {}", + path.display(), + err ); Vec::new() } @@ -214,19 +306,21 @@ impl JobThreadContext { } }; - // Log patch application summary at debug level + // Log patch application summary at INFO level if !applied_patches.is_empty() { let total_added: usize = applied_patches.iter().map(|p| p.added.len()).sum(); let total_removed: usize = applied_patches.iter().map(|p| p.removed.len()).sum(); let total_updated: usize = applied_patches.iter().map(|p| p.updated.len()).sum(); if total_added > 0 || total_removed > 0 || total_updated > 0 { - log::debug!( + log::info!( "VFS event applied: {} added, {} removed, {} updated", total_added, total_removed, total_updated ); } + } else { + log::info!("VFS event applied: no changes"); } // Notify anyone listening to the message queue about the changes we @@ -234,6 +328,57 @@ impl JobThreadContext { self.message_queue.push_messages(&applied_patches); } + /// Processes any pending recovery checks for paths that were recently + /// removed. If a path has reappeared on the real filesystem after the + /// recovery delay, we trigger a re-snapshot to bring the tree back in sync. + fn process_pending_recoveries(&self) { + const RECOVERY_DELAY: Duration = Duration::from_millis(200); + + let ready: Vec = { + let mut pending = self.pending_recovery.lock().unwrap(); + let now = Instant::now(); + + // Drain entries that are old enough to check + let mut ready = Vec::new(); + pending.retain(|(path, recorded_at)| { + if now.duration_since(*recorded_at) >= RECOVERY_DELAY { + ready.push(path.clone()); + false // remove from pending + } else { + true // keep in pending + } + }); + ready + }; + + for path in ready { + if std::fs::metadata(&path).is_ok() { + log::info!( + "VFS recovery: path {} was removed but has reappeared on disk. Re-snapshotting.", + path.display() + ); + let patches = self.apply_patches(path); + if !patches.is_empty() { + let total_added: usize = patches.iter().map(|p| p.added.len()).sum(); + let total_removed: usize = patches.iter().map(|p| p.removed.len()).sum(); + let total_updated: usize = patches.iter().map(|p| p.updated.len()).sum(); + log::info!( + "VFS recovery applied: {} added, {} removed, {} updated", + total_added, + total_removed, + total_updated + ); + self.message_queue.push_messages(&patches); + } + } else { + log::info!( + "VFS recovery: path {} confirmed removed from disk.", + path.display() + ); + } + } + } + fn handle_tree_event(&self, patch_set: PatchSet) { // Log incoming patch summary at debug level log::debug!( @@ -251,13 +396,40 @@ impl JobThreadContext { if let Some(instigating_source) = &instance.metadata().instigating_source { match instigating_source { InstigatingSource::Path(path) => { - log::info!("Two-way sync: Removing file {}", path.display()); - if let Err(err) = fs::remove_file(path) { - log::error!( - "Failed to remove file {:?} for instance {:?}: {}", - path, - id, - err + // Guard: file may already be deleted by the API's + // syncback_removed_instance before this PatchSet arrives. + if path.exists() { + if path.is_dir() { + log::info!( + "Two-way sync: Removing directory {}", + path.display() + ); + if let Err(err) = fs::remove_dir_all(path) { + log::error!( + "Failed to remove directory {:?} for instance {:?}: {}", + path, + id, + err + ); + } + } else { + log::info!( + "Two-way sync: Removing file {}", + path.display() + ); + if let Err(err) = fs::remove_file(path) { + log::error!( + "Failed to remove file {:?} for instance {:?}: {}", + path, + id, + err + ); + } + } + } else { + log::info!( + "Two-way sync: File already removed: {}", + path.display() ); } } @@ -269,7 +441,6 @@ impl JobThreadContext { } } } else { - // TODO log::warn!( "Cannot remove instance {:?}, it is not an instigating source.", id @@ -284,12 +455,147 @@ impl JobThreadContext { let id = update.id; if let Some(instance) = tree.get_instance(id) { - if update.changed_name.is_some() { - log::warn!("Cannot rename instances yet."); + // Handle instance rename on disk + if let Some(ref new_name) = update.changed_name { + if let Some(instigating_source) = &instance.metadata().instigating_source { + match instigating_source { + InstigatingSource::Path(path) => { + if path.exists() { + let old_name = instance.name(); + // Compute new path by replacing the old name + // in the filename/directory name + if let Some(parent) = path.parent() { + let old_file_name = path + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(""); + let new_file_name = + old_file_name.replacen(old_name, new_name, 1); + let new_path = parent.join(&new_file_name); + + if new_path != *path { + log::info!( + "Two-way sync: Renaming {} -> {}", + path.display(), + new_path.display() + ); + if let Err(err) = fs::rename(path, &new_path) { + log::error!( + "Failed to rename {:?} to {:?}: {}", + path, + new_path, + err + ); + } else { + // Also rename adjacent meta file if it exists + let old_meta = + parent.join(format!("{}.meta.json5", old_name)); + if old_meta.exists() { + let new_meta = parent + .join(format!("{}.meta.json5", new_name)); + let _ = fs::rename(&old_meta, &new_meta); + } + } + } + } + } + } + InstigatingSource::ProjectNode { .. } => { + log::warn!( + "Cannot rename instance {:?} — defined in project file", + id + ); + } + } + } else { + log::warn!( + "Cannot rename instance {:?} — no instigating source", + id + ); + } } - if update.changed_class_name.is_some() { - log::warn!("Cannot change ClassName yet."); + // Handle ClassName changes (script class transitions) + if let Some(ref new_class) = update.changed_class_name { + if let Some(instigating_source) = &instance.metadata().instigating_source { + match instigating_source { + InstigatingSource::Path(path) => { + let old_class = instance.class_name(); + let old_is_script = matches!( + old_class.as_str(), + "ModuleScript" | "Script" | "LocalScript" + ); + let new_is_script = matches!( + new_class.as_str(), + "ModuleScript" | "Script" | "LocalScript" + ); + + if old_is_script && new_is_script && path.exists() { + // Script-to-script transition: rename the file extension + if let Some(parent) = path.parent() { + let name = instance.name(); + let new_suffix = match new_class.as_str() { + "ModuleScript" => "", + "Script" => ".server", + "LocalScript" => ".local", + _ => "", + }; + let is_init = path + .file_name() + .and_then(|f| f.to_str()) + .map(|f| f.starts_with("init.")) + .unwrap_or(false); + + let new_file_name = if is_init { + if new_suffix.is_empty() { + "init.luau".to_string() + } else { + format!("init{}.luau", new_suffix) + } + } else if new_suffix.is_empty() { + format!("{}.luau", name) + } else { + format!("{}{}.luau", name, new_suffix) + }; + + let new_path = parent.join(&new_file_name); + if new_path != *path { + log::info!( + "Two-way sync: Changing class {} -> {}, \ + renaming {} -> {}", + old_class, + new_class, + path.display(), + new_path.display() + ); + if let Err(err) = fs::rename(path, &new_path) { + log::error!( + "Failed to rename {:?} to {:?}: {}", + path, + new_path, + err + ); + } + } + } + } else if old_is_script != new_is_script { + log::warn!( + "Cannot change ClassName from {} to {} — \ + cross-category changes (script <-> non-script) \ + are not yet supported", + old_class, + new_class + ); + } + } + InstigatingSource::ProjectNode { .. } => { + log::warn!( + "Cannot change ClassName for {:?} — defined in project file", + id + ); + } + } + } } if update.changed_metadata.is_some() { @@ -351,7 +657,16 @@ impl JobThreadContext { } } -fn compute_and_apply_changes(tree: &mut RojoTree, vfs: &Vfs, id: Ref) -> Option { +/// Result of computing and applying changes to a single instance. +/// Includes the path that was removed (if any) for pending recovery tracking. +struct ComputeResult { + applied: AppliedPatchSet, + /// If the instance was removed because its path no longer exists, + /// record the path here so the caller can schedule a recovery check. + removed_path: Option, +} + +fn compute_and_apply_changes(tree: &mut RojoTree, vfs: &Vfs, id: Ref) -> Option { let metadata = tree .get_metadata(id) .expect("metadata missing for instance present in tree"); @@ -370,41 +685,99 @@ fn compute_and_apply_changes(tree: &mut RojoTree, vfs: &Vfs, id: Ref) -> Option< // How we process a file change event depends on what created this // file/folder in the first place. - let applied_patch_set = match instigating_source { - InstigatingSource::Path(path) => match vfs.metadata(path).with_not_found() { - Ok(Some(_)) => { - // Our instance was previously created from a path and that - // path still exists. We can generate a snapshot starting at - // that path and use it as the source for our patch. + match instigating_source { + InstigatingSource::Path(path) => { + log::info!( + "compute_and_apply_changes: checking path {} for instance {:?}", + path.display(), + id + ); - let snapshot = match snapshot_from_vfs(&metadata.context, vfs, path) { - Ok(snapshot) => snapshot, - Err(err) => { - log::error!("Snapshot error: {:?}", err); - return None; + match vfs.metadata(path).with_not_found() { + Ok(Some(_)) => { + // Our instance was previously created from a path and that + // path still exists. We can generate a snapshot starting at + // that path and use it as the source for our patch. + log::info!( + "compute_and_apply_changes: path EXISTS via VFS, re-snapshotting {}", + path.display() + ); + + let snapshot = match snapshot_from_vfs(&metadata.context, vfs, path) { + Ok(snapshot) => snapshot, + Err(err) => { + log::error!("Snapshot error: {:?}", err); + return None; + } + }; + + let patch_set = compute_patch_set(snapshot, tree, id); + let applied = apply_patch_set(tree, patch_set); + Some(ComputeResult { + applied, + removed_path: None, + }) + } + Ok(None) => { + // Path not found via VFS. Before removing, verify it's truly + // gone from the REAL filesystem. On Windows, rapid + // delete+recreate (e.g., editor undo) can cause Remove events + // where the file has already been recreated by the time we + // process the event. + if std::fs::metadata(path).is_ok() { + log::info!( + "compute_and_apply_changes: VFS says path removed but REAL \ + filesystem confirms it EXISTS: {}. Re-snapshotting instead \ + of removing.", + path.display() + ); + + let snapshot = match snapshot_from_vfs(&metadata.context, vfs, path) { + Ok(snapshot) => snapshot, + Err(err) => { + log::error!("Recovery snapshot error for {}: {:?}", path.display(), err); + return None; + } + }; + + let patch_set = compute_patch_set(snapshot, tree, id); + let applied = apply_patch_set(tree, patch_set); + Some(ComputeResult { + applied, + removed_path: None, + }) + } else { + // Path is genuinely gone from both VFS and real filesystem. + // Remove the instance, but record the path for recovery + // checking — the file might be recreated momentarily. + log::info!( + "compute_and_apply_changes: path NOT FOUND on disk, removing \ + instance {:?} for {}. Scheduling recovery check.", + id, + path.display() + ); + + let removed_path = path.to_path_buf(); + let mut patch_set = PatchSet::new(); + patch_set.removed_instances.push(id); + + let applied = apply_patch_set(tree, patch_set); + Some(ComputeResult { + applied, + removed_path: Some(removed_path), + }) } - }; - - let patch_set = compute_patch_set(snapshot, tree, id); - apply_patch_set(tree, patch_set) + } + Err(err) => { + log::error!( + "Error processing filesystem change for {}: {:?}", + path.display(), + err + ); + None + } } - Ok(None) => { - // Our instance was previously created from a path, but that - // path no longer exists. - // - // We associate deleting the instigating file for an - // instance with deleting that instance. - - let mut patch_set = PatchSet::new(); - patch_set.removed_instances.push(id); - - apply_patch_set(tree, patch_set) - } - Err(err) => { - log::error!("Error processing filesystem change: {:?}", err); - return None; - } - }, + } InstigatingSource::ProjectNode { path, @@ -415,6 +788,11 @@ fn compute_and_apply_changes(tree: &mut RojoTree, vfs: &Vfs, id: Ref) -> Option< // This instance is the direct subject of a project node. Since // there might be information associated with our instance from // the project file, we snapshot the entire project node again. + log::info!( + "compute_and_apply_changes: re-snapshotting project node '{}' at {}", + name, + path.display() + ); let snapshot_result = snapshot_project_node( &metadata.context, @@ -434,9 +812,11 @@ fn compute_and_apply_changes(tree: &mut RojoTree, vfs: &Vfs, id: Ref) -> Option< }; let patch_set = compute_patch_set(snapshot, tree, id); - apply_patch_set(tree, patch_set) + let applied = apply_patch_set(tree, patch_set); + Some(ComputeResult { + applied, + removed_path: None, + }) } - }; - - Some(applied_patch_set) + } } diff --git a/src/serve_session.rs b/src/serve_session.rs index d1071581..b19280af 100644 --- a/src/serve_session.rs +++ b/src/serve_session.rs @@ -85,6 +85,11 @@ pub struct ServeSession { /// A channel to send mutation requests on. These will be handled by the /// ChangeProcessor and trigger changes in the tree. tree_mutation_sender: Sender, + + /// Paths recently written by the API's syncback. The ChangeProcessor + /// checks this set and suppresses the file watcher echo for these paths + /// to avoid redundant re-snapshots and WebSocket messages. + suppressed_paths: Arc>>, } impl ServeSession { @@ -132,6 +137,7 @@ impl ServeSession { let vfs = Arc::new(vfs); let (tree_mutation_sender, tree_mutation_receiver) = crossbeam_channel::unbounded(); + let suppressed_paths = Arc::new(Mutex::new(HashSet::new())); log::trace!("Starting ChangeProcessor"); let change_processor = ChangeProcessor::start( @@ -139,6 +145,7 @@ impl ServeSession { Arc::clone(&vfs), Arc::clone(&message_queue), tree_mutation_receiver, + Arc::clone(&suppressed_paths), ); Ok(Self { @@ -150,6 +157,7 @@ impl ServeSession { message_queue, tree_mutation_sender, vfs, + suppressed_paths, }) } @@ -165,6 +173,12 @@ impl ServeSession { self.tree_mutation_sender.clone() } + /// Returns a handle to the suppressed paths set, used to avoid + /// file watcher echo when the API writes files to disk. + pub fn suppressed_paths(&self) -> Arc>> { + Arc::clone(&self.suppressed_paths) + } + #[allow(unused)] pub fn vfs(&self) -> &Vfs { &self.vfs diff --git a/src/web/api.rs b/src/web/api.rs index 66299c8b..fcdafb19 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -39,14 +39,15 @@ use crate::{ /// When the plugin sends a script update, it might not include children info. /// We check the filesystem to see if this is already a directory (with init file) /// or a standalone file, and preserve that format to avoid creating duplicates. -#[derive(Debug, Clone, Copy, PartialEq)] +#[derive(Debug, Clone, PartialEq)] enum ExistingFileFormat { /// No existing file found - use has_children to decide None, - /// Standalone file exists (e.g., Name.luau) - Standalone, - /// Directory with init file exists (e.g., Name/init.luau) - Directory, + /// Standalone file exists (e.g., Name.luau). Carries the detected file path + /// so we write back to the same extension (.model.json vs .model.json5). + Standalone(PathBuf), + /// Directory with init file exists (e.g., Name/init.luau). Carries the dir path. + Directory(PathBuf), } /// Convert a Variant to a JSON-compatible value for .model.json5 files @@ -252,18 +253,86 @@ impl ApiService { } // Process removed instances (syncback: delete files from Rojo filesystem) - // When user selects "pull" for an instance that exists in Rojo but not Studio, - // we need to delete the corresponding file(s) from the filesystem. + // Phase 1: Gather paths with the tree lock held. + // Phase 2: Delete files without the lock. if !request.removed.is_empty() { - let tree = self.serve_session.tree(); + let removal_actions: Vec<(Ref, Option<(PathBuf, bool)>)> = { + let tree = self.serve_session.tree(); + request + .removed + .iter() + .map(|&id| { + let action = tree.get_instance(id).and_then(|inst| { + inst.metadata().instigating_source.as_ref().and_then(|src| { + match src { + crate::snapshot::InstigatingSource::Path(p) => { + Some((p.clone(), p.is_dir())) + } + crate::snapshot::InstigatingSource::ProjectNode { + name, + .. + } => { + log::warn!( + "Syncback: Cannot remove '{}' — defined in project file", + name + ); + None + } + } + }) + }); + (id, action) + }) + .collect() + }; // tree lock dropped here - for removed_id in &request.removed { - if let Err(err) = self.syncback_removed_instance(*removed_id, &tree) { - log::warn!( - "Failed to syncback removed instance {:?}: {}", - removed_id, - err - ); + // Phase 2: Execute filesystem deletions without the lock + for (id, action) in removal_actions { + if let Some((path, is_dir)) = action { + if !path.exists() { + log::info!( + "Syncback: Path already removed (likely parent was deleted): {}", + path.display() + ); + continue; + } + if is_dir { + if let Err(err) = fs::remove_dir_all(&path) { + log::warn!( + "Failed to remove directory {:?} for instance {:?}: {}", + path, id, err + ); + } else { + log::info!("Syncback: Removed directory at {}", path.display()); + } + } else { + if let Err(err) = fs::remove_file(&path) { + log::warn!( + "Failed to remove file {:?} for instance {:?}: {}", + path, id, err + ); + } else { + log::info!("Syncback: Removed file at {}", path.display()); + } + // Also remove adjacent meta file + if let Some(instance_name) = path + .file_stem() + .and_then(|s| s.to_str()) + .map(|s| s.split('.').next().unwrap_or(s).to_string()) + { + if let Some(parent_dir) = path.parent() { + let meta_path = + parent_dir.join(format!("{}.meta.json5", instance_name)); + if meta_path.exists() { + let _ = fs::remove_file(&meta_path); + log::info!( + "Syncback: Removed adjacent meta file at {}", + meta_path.display() + ); + } + } + } + } } } } @@ -296,26 +365,40 @@ 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 - .changed_properties - .keys() - .map(|k| k.as_str()) - .collect(); - if !prop_names.is_empty() { - log::info!( - "Syncback: Updating properties for instance {:?}: {}", - update.id, - prop_names.join(", ") - ); - } - if update.changed_name.is_some() { - log::info!( - "Syncback: Renaming instance {:?} to {:?}", - update.id, - update.changed_name - ); + // Persist non-Source property changes to disk (meta/model files). + // Source property changes are handled by ChangeProcessor when it + // receives the PatchSet below. + { + let tree = self.serve_session.tree(); + for update in &request.updated { + let prop_names: Vec<&str> = update + .changed_properties + .keys() + .map(|k| k.as_str()) + .collect(); + if !prop_names.is_empty() { + log::info!( + "Syncback: Updating properties for instance {:?}: {}", + update.id, + prop_names.join(", ") + ); + } + if update.changed_name.is_some() { + log::info!( + "Syncback: Renaming instance {:?} to {:?}", + update.id, + update.changed_name + ); + } + + // Write non-Source properties to meta/model files + if let Err(err) = self.syncback_updated_properties(update, &tree) { + log::warn!( + "Failed to persist non-Source properties for instance {:?}: {}", + update.id, + err + ); + } } } @@ -333,7 +416,7 @@ impl ApiService { tree_mutation_sender .send(PatchSet { - removed_instances: Vec::new(), + removed_instances: request.removed, added_instances: Vec::new(), updated_instances, }) @@ -517,22 +600,83 @@ impl ApiService { } // Instance doesn't exist in tree - create new files - // Get the parent's filesystem path from its metadata - let parent_path = parent_instance + // Get the parent's filesystem path from its metadata. + // For ProjectNode sources, resolve the $path field relative to the project file. + let instigating_source = parent_instance .metadata() .instigating_source .as_ref() - .map(|s| s.path()) .context("Parent instance has no filesystem path (not synced from filesystem)")?; - // Determine if parent is a directory or file + let parent_path: std::borrow::Cow<'_, std::path::Path> = match instigating_source { + crate::snapshot::InstigatingSource::Path(p) => std::borrow::Cow::Borrowed(p.as_path()), + crate::snapshot::InstigatingSource::ProjectNode { + path: project_path, + name, + node, + .. + } => { + if let Some(path_node) = &node.path { + let fs_path = path_node.path(); + let resolved = if fs_path.is_relative() { + project_path + .parent() + .unwrap_or(project_path.as_path()) + .join(fs_path) + } else { + fs_path.to_path_buf() + }; + std::borrow::Cow::Owned(resolved) + } else { + anyhow::bail!( + "Cannot add '{}' — parent '{}' is defined in a project file without $path", + added.name, + name + ); + } + } + }; + + // Determine if parent is a directory or file. + // ANY instance can have children in Roblox, so if the parent is a + // standalone file (script, model, txt, csv, etc.), we must convert it + // to directory format before we can place children inside it. let parent_dir = if parent_path.is_dir() { parent_path.to_path_buf() } else { - parent_path + let parent_class = parent_instance.class_name(); + let parent_name = parent_instance.name(); + let containing_dir = parent_path .parent() - .context("Could not get parent directory")? - .to_path_buf() + .context("Could not get parent directory")?; + + log::info!( + "Syncback: Converting standalone {} '{}' at {} to directory format \ + (child '{}' being added)", + parent_class, + parent_name, + parent_path.display(), + added.name + ); + + if matches!( + parent_class.as_str(), + "ModuleScript" | "Script" | "LocalScript" + ) { + self.convert_standalone_script_to_directory( + &parent_path, + parent_name, + parent_class.as_str(), + containing_dir, + )? + } else { + self.convert_standalone_instance_to_directory( + &parent_path, + parent_name, + parent_class.as_str(), + containing_dir, + )? + } }; // Create the instance at the resolved path @@ -723,9 +867,197 @@ impl ApiService { Ok(()) } + /// Converts a standalone script file (e.g., `MyModule.luau`) into directory + /// format (e.g., `MyModule/init.luau`). This is needed when a child is being + /// added to a standalone script — standalone scripts cannot have children in + /// Rojo's file format. + /// + /// Returns the path to the new directory. + fn convert_standalone_script_to_directory( + &self, + standalone_path: &std::path::Path, + script_name: &str, + class_name: &str, + containing_dir: &std::path::Path, + ) -> anyhow::Result { + use anyhow::Context; + + // Read the current script content before any modifications + let source = if standalone_path.exists() { + fs::read_to_string(standalone_path).unwrap_or_default() + } else { + String::new() + }; + + // Create the directory + let new_dir = containing_dir.join(script_name); + fs::create_dir_all(&new_dir).with_context(|| { + format!( + "Failed to create directory for script conversion: {}", + new_dir.display() + ) + })?; + + // Determine the init file name based on class + let init_name = match class_name { + "ModuleScript" => "init.luau", + "Script" => "init.server.luau", + "LocalScript" => "init.local.luau", + _ => "init.luau", + }; + + // Write source content to the init file + let init_path = new_dir.join(init_name); + fs::write(&init_path, source.as_bytes()).with_context(|| { + format!("Failed to write init file: {}", init_path.display()) + })?; + + // Remove the old standalone file + if standalone_path.exists() && standalone_path != init_path { + fs::remove_file(standalone_path).with_context(|| { + format!( + "Failed to remove old standalone script: {}", + standalone_path.display() + ) + })?; + } + + // Move adjacent meta file into directory if it exists + let meta_path = containing_dir.join(format!("{}.meta.json5", script_name)); + if meta_path.exists() { + let init_meta_path = new_dir.join("init.meta.json5"); + fs::rename(&meta_path, &init_meta_path).with_context(|| { + format!( + "Failed to move meta file {} to {}", + meta_path.display(), + init_meta_path.display() + ) + })?; + log::info!( + "Syncback: Moved {} to {}", + meta_path.display(), + init_meta_path.display() + ); + } + + log::info!( + "Syncback: Converted standalone {} to directory format at {}", + class_name, + new_dir.display() + ); + + Ok(new_dir) + } + + /// Converts a standalone non-script file (e.g., `MyPart.model.json5`, `MyValue.txt`) + /// into directory format (e.g., `MyPart/init.meta.json5`). This is needed when a + /// child is being added to any non-script instance that is currently a standalone file. + /// + /// Returns the path to the new directory. + fn convert_standalone_instance_to_directory( + &self, + standalone_path: &std::path::Path, + instance_name: &str, + class_name: &str, + containing_dir: &std::path::Path, + ) -> anyhow::Result { + use anyhow::Context; + + let new_dir = containing_dir.join(instance_name); + fs::create_dir_all(&new_dir).with_context(|| { + format!( + "Failed to create directory for instance conversion: {}", + new_dir.display() + ) + })?; + + // Determine the init file based on the standalone file type. + // The content of the standalone file becomes the init file inside the directory. + let file_ext = standalone_path + .extension() + .and_then(|e| e.to_str()) + .unwrap_or(""); + + let file_name = standalone_path + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(""); + + if file_name.ends_with(".model.json5") || file_name.ends_with(".model.json") { + // .model.json5 → init.meta.json5 with className and properties + // Read the model file and rewrite as init.meta.json5 + if standalone_path.exists() { + let content = fs::read(standalone_path).unwrap_or_default(); + let init_meta_path = new_dir.join("init.meta.json5"); + fs::write(&init_meta_path, &content).with_context(|| { + format!("Failed to write {}", init_meta_path.display()) + })?; + } + } else if file_ext == "txt" { + // StringValue .txt → init.meta.json5 with className and Value property + let value = if standalone_path.exists() { + fs::read_to_string(standalone_path).unwrap_or_default() + } else { + String::new() + }; + let meta = serde_json::json!({ + "className": class_name, + "properties": { + "Value": value + } + }); + let init_meta_path = new_dir.join("init.meta.json5"); + let content = crate::json::to_vec_pretty_sorted(&meta) + .context("Failed to serialize init.meta.json5")?; + fs::write(&init_meta_path, &content).with_context(|| { + format!("Failed to write {}", init_meta_path.display()) + })?; + } else if file_ext == "csv" { + // LocalizationTable .csv → init.csv + if standalone_path.exists() { + let content = fs::read(standalone_path).unwrap_or_default(); + let init_csv_path = new_dir.join("init.csv"); + fs::write(&init_csv_path, &content).with_context(|| { + format!("Failed to write {}", init_csv_path.display()) + })?; + } + } else { + // Generic fallback: create init.meta.json5 with className + let meta = serde_json::json!({ + "className": class_name + }); + let init_meta_path = new_dir.join("init.meta.json5"); + let content = crate::json::to_vec_pretty_sorted(&meta) + .context("Failed to serialize init.meta.json5")?; + fs::write(&init_meta_path, &content).with_context(|| { + format!("Failed to write {}", init_meta_path.display()) + })?; + } + + // Remove the old standalone file + if standalone_path.exists() { + fs::remove_file(standalone_path).with_context(|| { + format!( + "Failed to remove old standalone file: {}", + standalone_path.display() + ) + })?; + } + + log::info!( + "Syncback: Converted standalone {} '{}' to directory format at {}", + class_name, + instance_name, + new_dir.display() + ); + + Ok(new_dir) + } + /// Syncback a removed instance by deleting its file(s) from the filesystem. /// This is called when the user selects "pull" for an instance that exists in Rojo /// but has been deleted in Studio. + #[allow(dead_code)] fn syncback_removed_instance( &self, removed_id: Ref, @@ -893,7 +1225,7 @@ impl ApiService { for init_file in init_files { if dir_path.join(init_file).exists() { - return ExistingFileFormat::Directory; + return ExistingFileFormat::Directory(dir_path.clone()); } } } @@ -923,8 +1255,9 @@ impl ApiService { }; for pattern in standalone_patterns { - if parent_dir.join(&pattern).exists() { - return ExistingFileFormat::Standalone; + let full_path = parent_dir.join(&pattern); + if full_path.exists() { + return ExistingFileFormat::Standalone(full_path); } } @@ -971,81 +1304,47 @@ impl ApiService { 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 - // This matches the logic in src/syncback/mod.rs::get_best_middleware + // Determine the appropriate middleware/file format based on class name. + // This matches the logic in src/syncback/mod.rs::get_best_middleware. // - // IMPORTANT: When an instance already exists on disk, preserve its format. - // The plugin might send a partial update (e.g., just the script source without - // children), so we can't rely solely on has_children to decide the format. - // Only transition formats when creating truly new instances. + // Format transitions (matching `rojo syncback` behavior): + // - Standalone + children added → convert to directory (clean up old standalone) + // - Standalone + no children → keep standalone + // - Directory + children or not → PRESERVE directory (plugin may omit children) + // - None + has_children → directory (new instance) + // - None + no children → standalone (new instance) let existing_format = Self::detect_existing_script_format(parent_dir, &added.name, &added.class_name); match added.class_name.as_str() { - // Script types: .luau files, or directories with init files if has children - // Non-Source properties (like Attributes) go into init.meta.json5 + // Script types: .luau files, or directories with init files if has children. + // Format is driven by has_children (matching `rojo syncback` behavior). + // If the existing format doesn't match, we convert (standalone↔directory). "ModuleScript" => { let source = self.get_source_property(added); - // Decide whether to use directory or standalone format: - // - If directory already exists on disk, preserve it (update init file) - // - If standalone already exists on disk, preserve it (update standalone) - // - If nothing exists, use has_children to decide (new instance) - let use_directory = match existing_format { - ExistingFileFormat::Directory => { - log::debug!( - "Syncback: Preserving existing directory format for {}", - added.name - ); - true - } - ExistingFileFormat::Standalone => { - // Standalone format can stay standalone only if there are no children. - // If children were added in Studio, we must convert to directory format. - if has_children { - log::info!( - "Syncback: Converting {} from standalone to directory format (children added)", - added.name - ); - true - } else { - log::debug!( - "Syncback: Preserving existing standalone format for {}", - added.name - ); - false - } - } - ExistingFileFormat::None => { - // New instance - use has_children to decide - has_children - } + // Standalone→directory when children are added. + // Directory is preserved when no children (plugin may omit children in partial updates). + // New instances use has_children to decide. + let use_directory = match &existing_format { + ExistingFileFormat::Directory(_) => true, // preserve directory + ExistingFileFormat::Standalone(_) => has_children, // convert only if children added + ExistingFileFormat::None => has_children, }; if use_directory { - // Directory format: Name/init.luau - // Remove standalone file if transitioning to directory (new instance or format conversion) - if existing_format == ExistingFileFormat::None - || existing_format == ExistingFileFormat::Standalone - { - let standalone_path = parent_dir.join(format!("{}.luau", added.name)); - if standalone_path.exists() { - if let Err(err) = fs::remove_file(&standalone_path) { - log::warn!( - "Syncback: Failed to remove old standalone file {}: {}", - standalone_path.display(), - err - ); - } else { - log::info!( - "Syncback: Removed old standalone file {} (script now has children)", - standalone_path.display() - ); - } - let meta_path = parent_dir.join(format!("{}.meta.json5", added.name)); - if meta_path.exists() { - let _ = fs::remove_file(&meta_path); - } + // Transition standalone → directory if needed + if let ExistingFileFormat::Standalone(ref old_path) = existing_format { + log::info!( + "Syncback: Converting ModuleScript {} from standalone to directory (children added)", + added.name + ); + if old_path.exists() { + let _ = fs::remove_file(old_path); + } + let meta_path = parent_dir.join(format!("{}.meta.json5", added.name)); + if meta_path.exists() { + let _ = fs::remove_file(&meta_path); } } @@ -1059,31 +1358,10 @@ impl ApiService { })?; self.write_script_meta_json_if_needed(&dir_path, added)?; log::info!("Syncback: Updated ModuleScript at {}", init_path.display()); - // Recurse into children if any were provided for child in &unique_children { self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; } } else { - // Standalone format: Name.luau - // Only remove directory if this is a new instance transitioning to standalone - if existing_format == ExistingFileFormat::None { - let dir_path = parent_dir.join(&added.name); - if dir_path.is_dir() { - if let Err(err) = fs::remove_dir_all(&dir_path) { - log::warn!( - "Syncback: Failed to remove old directory {}: {}", - dir_path.display(), - err - ); - } else { - log::info!( - "Syncback: Removed old directory {} (script no longer has children)", - dir_path.display() - ); - } - } - } - let file_path = parent_dir.join(format!("{}.luau", added.name)); fs::write(&file_path, source.as_bytes()).with_context(|| { format!("Failed to write file: {}", file_path.display()) @@ -1096,46 +1374,20 @@ impl ApiService { let source = self.get_source_property(added); let script_suffix = self.get_script_suffix_for_run_context(added); - // Use existing format if present, otherwise decide based on has_children - let use_directory = match existing_format { - ExistingFileFormat::Directory => { - log::debug!( - "Syncback: Preserving existing directory format for Script {}", - added.name - ); - true - } - ExistingFileFormat::Standalone => { - // Standalone format can stay standalone only if there are no children. - // If children were added in Studio, we must convert to directory format. - if has_children { - log::info!( - "Syncback: Converting Script {} from standalone to directory format (children added)", - added.name - ); - true - } else { - log::debug!( - "Syncback: Preserving existing standalone format for Script {}", - added.name - ); - false - } - } + let use_directory = match &existing_format { + ExistingFileFormat::Directory(_) => true, + ExistingFileFormat::Standalone(_) => has_children, ExistingFileFormat::None => has_children, }; if use_directory { - // Clean up standalone files if transitioning to directory (new instance or format conversion) - if existing_format == ExistingFileFormat::None - || existing_format == ExistingFileFormat::Standalone - { - for suffix in &["server", "client"] { - let standalone_path = - parent_dir.join(format!("{}.{}.luau", added.name, suffix)); - if standalone_path.exists() { - let _ = fs::remove_file(&standalone_path); - } + if let ExistingFileFormat::Standalone(ref old_path) = existing_format { + log::info!( + "Syncback: Converting Script {} from standalone to directory (children added)", + added.name + ); + if old_path.exists() { + let _ = fs::remove_file(old_path); } let meta_path = parent_dir.join(format!("{}.meta.json5", added.name)); if meta_path.exists() { @@ -1157,14 +1409,6 @@ impl ApiService { self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; } } else { - // Only clean up if this is a new instance - if existing_format == ExistingFileFormat::None { - let dir_path = parent_dir.join(&added.name); - if dir_path.is_dir() { - let _ = fs::remove_dir_all(&dir_path); - } - } - let file_path = parent_dir.join(format!("{}.{}.luau", added.name, script_suffix)); fs::write(&file_path, source.as_bytes()).with_context(|| { @@ -1177,46 +1421,20 @@ impl ApiService { "LocalScript" => { let source = self.get_source_property(added); - // Use existing format if present, otherwise decide based on has_children - let use_directory = match existing_format { - ExistingFileFormat::Directory => { - log::debug!( - "Syncback: Preserving existing directory format for LocalScript {}", - added.name - ); - true - } - ExistingFileFormat::Standalone => { - // Standalone format can stay standalone only if there are no children. - // If children were added in Studio, we must convert to directory format. - if has_children { - log::info!( - "Syncback: Converting LocalScript {} from standalone to directory format (children added)", - added.name - ); - true - } else { - log::debug!( - "Syncback: Preserving existing standalone format for LocalScript {}", - added.name - ); - false - } - } + let use_directory = match &existing_format { + ExistingFileFormat::Directory(_) => true, + ExistingFileFormat::Standalone(_) => has_children, ExistingFileFormat::None => has_children, }; if use_directory { - // Clean up standalone files if transitioning to directory (new instance or format conversion) - if existing_format == ExistingFileFormat::None - || existing_format == ExistingFileFormat::Standalone - { - for suffix in &["local", "client"] { - let standalone_path = - parent_dir.join(format!("{}.{}.luau", added.name, suffix)); - if standalone_path.exists() { - let _ = fs::remove_file(&standalone_path); - } + if let ExistingFileFormat::Standalone(ref old_path) = existing_format { + log::info!( + "Syncback: Converting LocalScript {} from standalone to directory (children added)", + added.name + ); + if old_path.exists() { + let _ = fs::remove_file(old_path); } let meta_path = parent_dir.join(format!("{}.meta.json5", added.name)); if meta_path.exists() { @@ -1238,14 +1456,6 @@ impl ApiService { self.syncback_instance_to_path_with_stats(child, &dir_path, stats)?; } } else { - // Only clean up if this is a new instance - if existing_format == ExistingFileFormat::None { - let dir_path = parent_dir.join(&added.name); - if dir_path.is_dir() { - let _ = fs::remove_dir_all(&dir_path); - } - } - let file_path = parent_dir.join(format!("{}.local.luau", added.name)); fs::write(&file_path, source.as_bytes()).with_context(|| { format!("Failed to write file: {}", file_path.display()) @@ -1357,31 +1567,27 @@ impl ApiService { } } - // All other classes -> .model.json5 or directory if has children - // Preserves existing format to avoid creating duplicates during partial updates + // All other classes -> .model.json5 or directory if has children. _ => { - let use_directory = match existing_format { - ExistingFileFormat::Directory => { - log::debug!( - "Syncback: Preserving existing directory format for {} {}", - added.class_name, - added.name - ); - true - } - ExistingFileFormat::Standalone => { - log::debug!( - "Syncback: Preserving existing standalone format for {} {}", - added.class_name, - added.name - ); - false - } + let use_directory = match &existing_format { + ExistingFileFormat::Directory(_) => true, // preserve directory + ExistingFileFormat::Standalone(_) => has_children, // convert only if children added ExistingFileFormat::None => has_children, }; if use_directory { - // Create directory and put the instance as init.meta.json5 + // Transition standalone → directory if needed + if let ExistingFileFormat::Standalone(ref old_path) = existing_format { + log::info!( + "Syncback: Converting {} {} from standalone to directory (children added)", + added.class_name, + added.name + ); + if old_path.exists() { + let _ = fs::remove_file(old_path); + } + } + let dir_path = parent_dir.join(&added.name); fs::create_dir_all(&dir_path).with_context(|| { format!("Failed to create directory: {}", dir_path.display()) @@ -1402,8 +1608,12 @@ impl ApiService { } } else { let content = self.serialize_instance_to_model_json(added)?; - // Use .model.json5 to match dedicated syncback (extension_for_middleware) - let file_path = parent_dir.join(format!("{}.model.json5", added.name)); + // Use the detected file path if available (preserves .model.json + // vs .model.json5), otherwise default to .model.json5 + let file_path = match &existing_format { + ExistingFileFormat::Standalone(p) => p.clone(), + _ => parent_dir.join(format!("{}.model.json5", added.name)), + }; fs::write(&file_path, &content).with_context(|| { format!("Failed to write file: {}", file_path.display()) })?; @@ -1743,6 +1953,178 @@ impl ApiService { crate::json::to_vec_pretty_sorted(&model).context("Failed to serialize model.json5") } + /// Persist non-Source property changes to the appropriate meta/model file. + /// Source is handled by the ChangeProcessor; this handles everything else + /// (Attributes, custom properties, etc.) so they survive a Rojo restart. + fn syncback_updated_properties( + &self, + update: &crate::web::interface::InstanceUpdate, + tree: &crate::snapshot::RojoTree, + ) -> anyhow::Result<()> { + use anyhow::Context; + + // Check if there are any non-Source property changes to persist + let has_non_source = update + .changed_properties + .iter() + .any(|(key, _)| key != "Source"); + + if !has_non_source { + return Ok(()); + } + + // Look up the instance in the tree + let instance = tree + .get_instance(update.id) + .context("Instance not found in tree for property persistence")?; + + let instigating_source = instance + .metadata() + .instigating_source + .as_ref() + .context("Instance has no filesystem path")?; + + let inst_path = instigating_source.path(); + let class_name = instance.class_name(); + let is_script = matches!( + class_name.as_str(), + "ModuleScript" | "Script" | "LocalScript" + ); + + // Collect the non-Source properties as a HashMap + // for reuse with filter_properties_for_meta + let mut props: HashMap = HashMap::new(); + for (key, value) in &update.changed_properties { + if key == "Source" { + continue; + } + if let Some(v) = value { + props.insert(key.to_string(), v.clone()); + } + } + + if props.is_empty() { + return Ok(()); + } + + let skip_prop = if is_script { Some("Source") } else { None }; + let (properties, attributes) = + self.filter_properties_for_meta(class_name.as_str(), &props, skip_prop); + + if properties.is_empty() && attributes.is_empty() { + return Ok(()); + } + + // Determine which meta file to write based on file structure + if inst_path.is_dir() { + // Directory format: write to init.meta.json5 + let meta_path = inst_path.join("init.meta.json5"); + + // Read existing meta if present, merge with new properties + let meta = self.merge_or_build_meta(&meta_path, None, properties, attributes)?; + let content = crate::json::to_vec_pretty_sorted(&meta) + .context("Failed to serialize init.meta.json5")?; + fs::write(&meta_path, &content) + .with_context(|| format!("Failed to write {}", meta_path.display()))?; + + log::info!( + "Syncback: Persisted non-Source properties to {}", + meta_path.display() + ); + } else if is_script { + // Standalone script: write to adjacent Name.meta.json5 + let parent_dir = inst_path.parent().context("No parent directory")?; + let name = instance.name(); + let meta_path = parent_dir.join(format!("{}.meta.json5", name)); + + let meta = self.merge_or_build_meta(&meta_path, None, properties, attributes)?; + let content = crate::json::to_vec_pretty_sorted(&meta) + .context("Failed to serialize meta.json5")?; + fs::write(&meta_path, &content) + .with_context(|| format!("Failed to write {}", meta_path.display()))?; + + log::info!( + "Syncback: Persisted non-Source properties to {}", + meta_path.display() + ); + } else { + // Non-script standalone file (e.g., .model.json5): update in place + let meta = self.merge_or_build_meta( + inst_path, + Some(class_name.as_str()), + properties, + attributes, + )?; + let content = crate::json::to_vec_pretty_sorted(&meta) + .context("Failed to serialize model file")?; + fs::write(inst_path, &content) + .with_context(|| format!("Failed to write {}", inst_path.display()))?; + + log::info!( + "Syncback: Persisted non-Source properties to {}", + inst_path.display() + ); + } + + Ok(()) + } + + /// Reads an existing meta/model JSON5 file and merges new properties into it. + /// If the file doesn't exist, builds a fresh meta object. + fn merge_or_build_meta( + &self, + existing_path: &std::path::Path, + class_name: Option<&str>, + new_properties: indexmap::IndexMap, + new_attributes: indexmap::IndexMap, + ) -> anyhow::Result { + use anyhow::Context; + + if existing_path.exists() { + // Read and parse existing file + let content = fs::read(existing_path) + .with_context(|| format!("Failed to read {}", existing_path.display()))?; + let mut existing: serde_json::Value = + json5::from_str(&String::from_utf8_lossy(&content)) + .unwrap_or_else(|_| serde_json::Value::Object(serde_json::Map::new())); + + // Ensure the existing value is an object + if !existing.is_object() { + existing = serde_json::Value::Object(serde_json::Map::new()); + } + let obj = existing.as_object_mut().unwrap(); + + // Merge properties + if !new_properties.is_empty() { + let props = obj + .entry("properties") + .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())); + if let Some(props_obj) = props.as_object_mut() { + for (k, v) in new_properties { + props_obj.insert(k, v); + } + } + } + + // Merge attributes + if !new_attributes.is_empty() { + let attrs = obj + .entry("attributes") + .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())); + if let Some(attrs_obj) = attrs.as_object_mut() { + for (k, v) in new_attributes { + attrs_obj.insert(k, v); + } + } + } + + Ok(existing) + } else { + // No existing file — build from scratch + Ok(self.build_meta_object(class_name, new_properties, new_attributes)) + } + } + async fn handle_api_read(&self, request: Request) -> Response { let argument = &request.uri().path()["/api/read/".len()..]; let requested_ids: Result, _> = argument.split(',').map(Ref::from_str).collect();