From 316d7ea70e7d82f3f4e2f5ca49f2dafb700299b0 Mon Sep 17 00:00:00 2001 From: Joe Date: Wed, 11 Feb 2026 17:10:35 -0600 Subject: [PATCH] Update logging levels in Rojo plugin and enhance test plans - Promoted sync-related logging from Debug/Trace to Info level across multiple files, ensuring visibility of changes in the Studio output console. - Added a new plan for expanding the connected mode test suite to cover multi-step scenarios and round-trip identity verification. - Introduced a plan to fix stale tree desynchronization by implementing a validation mechanism on plugin connection. - Created a plan to bump sync logging to Info level, detailing todos for various components in the plugin. This update improves the clarity of logs and enhances testing coverage for connected mode scenarios. --- ...bump_sync_logging_to_info_3e39ff81.plan.md | 119 + ...ected_mode_test_expansion_5b0c39a3.plan.md | 489 +++ .../plans/fix_audit_findings_ca70f773.plan.md | 40 +- .../fix_stale_tree_desync_9e74f718.plan.md | 130 + plugin/log/init.lua | 8 +- plugin/src/ApiContext.lua | 10 +- plugin/src/ChangeBatcher/encodeInstance.lua | 5 + .../src/ChangeBatcher/encodePatchUpdate.lua | 5 + plugin/src/ChangeBatcher/encodeProperty.lua | 8 +- plugin/src/ChangeBatcher/init.lua | 3 +- plugin/src/ChangeBatcher/propertyFilter.lua | 16 + plugin/src/Reconciler/applyPatch.lua | 6 +- plugin/src/ServeSession.lua | 41 +- src/change_processor.rs | 3041 +++++++++-------- src/serve_session.rs | 65 + src/web/api.rs | 8 + vscode-rojo | 2 +- 17 files changed, 2443 insertions(+), 1553 deletions(-) create mode 100644 .cursor/plans/bump_sync_logging_to_info_3e39ff81.plan.md create mode 100644 .cursor/plans/connected_mode_test_expansion_5b0c39a3.plan.md create mode 100644 .cursor/plans/fix_stale_tree_desync_9e74f718.plan.md create mode 100644 plugin/src/ChangeBatcher/propertyFilter.lua diff --git a/.cursor/plans/bump_sync_logging_to_info_3e39ff81.plan.md b/.cursor/plans/bump_sync_logging_to_info_3e39ff81.plan.md new file mode 100644 index 00000000..8810a814 --- /dev/null +++ b/.cursor/plans/bump_sync_logging_to_info_3e39ff81.plan.md @@ -0,0 +1,119 @@ +--- +name: Bump sync logging to info +overview: Promote all sync-related change logging in the Rojo Studio plugin from Debug/Trace to Info level, so that every change received, confirmed, sent, and applied is visible in the Studio output console at the default log level. +todos: + - id: log-padding + content: Remove 15-space padding from log tags in plugin/log/init.lua (TRACE_TAG, INFO_TAG, DEBUG_TAG) + status: completed + - id: serve-session + content: Bump Debug/Trace to Info in ServeSession.lua (received, confirmed, applied summaries + per-item decisions). Add combined patch summary after WebSocket message combining. + status: completed + - id: change-batcher + content: Bump batch summary in ChangeBatcher/init.lua line 115 from Debug to Info + status: completed + - id: api-context + content: Bump sending-to-server logs in ApiContext.lua from Debug/Trace to Info (summary + per-item operations) + status: completed + - id: apply-patch + content: Bump patch application logs in Reconciler/applyPatch.lua from Debug to Info (summary + per-update details) + status: completed +isProject: false +--- + +# Bump Sync Change Logging to Info + +## Current State + +The plugin's `Log` module (at `plugin/log/init.lua`) supports levels: Error(0), Warning(1), Info(2), Debug(3), Trace(4). Default log level is `"Info"`, meaning Debug and Trace messages are hidden unless the user manually changes the setting. + +Most sync-flow messages are currently at Debug or Trace, so users can't see what changes are being received/sent/confirmed without digging into settings. + +## Files to Change + +### 0. [plugin/log/init.lua](plugin/log/init.lua) -- Fix excessive log padding + +The log tags have 15 hardcoded spaces prepended for alignment with Studio's timestamp column, but it creates absurdly wide output -- especially for multi-line messages where `addTags` replaces every `\n` with `\n` + the full padded tag. + +```lua +-- Current (lines 15-16) -- repeats tag on every line of multi-line messages: +local function addTags(tag, message) + return tag .. message:gsub("\n", "\n" .. tag) +end + +-- Fix: just prepend the tag once +local function addTags(tag, message) + return tag .. message +end +``` + +```lua +-- Current (lines 19-21) -- 15 spaces of padding before each tag: +local TRACE_TAG = (" "):rep(15) .. "[Rojo-Trace] " +local INFO_TAG = (" "):rep(15) .. "[Rojo-Info] " +local DEBUG_TAG = (" "):rep(15) .. "[Rojo-Debug] " + +-- Fix: remove the padding +local TRACE_TAG = "[Rojo-Trace] " +local INFO_TAG = "[Rojo-Info] " +local DEBUG_TAG = "[Rojo-Debug] " +``` + +Note: `WARN_TAG` on line 22 already has no padding (it goes through `warn()` which has its own formatting). + +### 1. [plugin/src/ServeSession.lua](plugin/src/ServeSession.lua) -- Received / Confirmed / Applied + +**Received from server (WebSocket):** + +- Line 222: `Log.debug` -> `Log.info` -- "Received {} messages from Rojo server" +- After line 228 (after combining messages into `combinedPatch`): add an `Log.info` summary of what's IN the combined patch (removals, additions, updates counts), similar to how `applyPatch.lua` does it. Currently the contents are only visible at Trace via the raw packet dump. + +**Confirmation selections:** + +- Line 699: `Log.debug` -> `Log.info` -- "User selections: {} push, {} pull, {} ignored" +- Line 713: `Log.trace` -> `Log.info` -- "[Push] Update: {}" +- Line 726: `Log.trace` -> `Log.info` -- "[Pull] Update: {}" +- Line 757: `Log.trace` -> `Log.info` -- "[Push] Delete: {}" +- Line 792: `Log.trace` -> `Log.info` -- "[Push] Add: {}.{}" +- Lines 772, 797, 828 are already Info -- no change needed + +**Applied to Studio (summary):** + +- Line 813: `Log.debug` -> `Log.info` -- "Applying to Studio: {} additions, {} removals, {} updates" +- Line 583: `Log.debug` -> `Log.info` -- unapplied patch summary (important for diagnosing sync issues) + +### 2. [plugin/src/ChangeBatcher/init.lua](plugin/src/ChangeBatcher/init.lua) -- Outgoing batched changes + +- Line 115: `Log.debug` -> `Log.info` -- "Two-way sync: {} updates, {} additions, {} removals" +- Lines 70, 73 stay at Trace -- these fire per-property-per-frame and would flood the console if promoted + +### 3. [plugin/src/ApiContext.lua](plugin/src/ApiContext.lua) -- Sent to server + +- Line 202: `Log.debug` -> `Log.info` -- "Sending to server: {} removals, {} additions, {} updates" +- Line 212: `Log.trace` -> `Log.info` -- "[Syncback] Remove ID: {}" +- Line 218: `Log.trace` -> `Log.info` -- "[Syncback] Add {} ({})" +- Line 228: `Log.trace` -> `Log.info` -- "[Syncback] Update ID {} ({} properties)" + +### 4. [plugin/src/Reconciler/applyPatch.lua](plugin/src/Reconciler/applyPatch.lua) -- Applied to Studio (per-item) + +- Line 36: `Log.debug` -> `Log.info` -- "Applying patch to Studio: {} removals, {} additions, {} updates" +- Line 141: `Log.debug` -> `Log.info` -- "[Studio] Update: {} - {}" +- Lines 56, 106 are already Info -- no change needed + +## What stays at Debug/Trace (intentionally) + +- ChangeBatcher per-property detection (lines 70, 73) -- fires every frame, too noisy +- Raw WebSocket packet dump (ApiContext line 299) -- huge payloads, not useful for comparison +- Precommit/postcommit hook registration (ServeSession lines 182, 189, 203, 210) -- lifecycle plumbing, not sync data +- Internal fallback/replace logic (ServeSession lines 561, 566) -- internal recovery mechanism + +## Summary of Sync Flow Visibility After Changes + +``` +Server → Plugin: "Received N messages" + combined patch summary (counts) +Confirmation: "User selections: X push, Y pull, Z ignored" + per-item decisions +Apply to Studio: "Applying patch: N removals, M additions, K updates" + per-item details +Studio → Server: "Two-way sync: X updates, Y additions, Z removals" (batch summary) +Send to Server: "Sending to server: N removals, M additions, K updates" + per-item details +``` + +All of these will now appear in the Studio output console at the default Info log level. \ No newline at end of file diff --git a/.cursor/plans/connected_mode_test_expansion_5b0c39a3.plan.md b/.cursor/plans/connected_mode_test_expansion_5b0c39a3.plan.md new file mode 100644 index 00000000..4cbf5dc4 --- /dev/null +++ b/.cursor/plans/connected_mode_test_expansion_5b0c39a3.plan.md @@ -0,0 +1,489 @@ +--- +name: Connected Mode Test Expansion +overview: Expand the serve test suite to cover multi-step connected mode scenarios -- format transitions, init file operations, directory conversions, slugify/dedup forward-sync, meta name field lifecycle, and sequential change sequences -- with full WebSocket patch verification AND round-trip identity verification. 53 tests total across 8 phases. +todos: + - id: fixtures + content: "Create 4 new test fixtures in rojo-test/serve-tests/: connected_scripts, connected_init, connected_models, connected_slugify" + status: in_progress + - id: harness + content: "Add harness helpers to tests/rojo_test/serve_util.rs: get_message_cursor for chaining multi-step tests, fresh_rebuild_read for round-trip verification" + status: pending + - id: phase1-format-transitions + content: "Implement Phase 1: Format transition tests (standalone<->directory, init type changes) with round-trip verification -- 8 tests" + status: pending + - id: phase2-multi-step + content: "Implement Phase 2: Multi-step connected sequence tests (create/edit/rename/delete lifecycle) with round-trip verification -- 5 tests" + status: pending + - id: phase3-init + content: "Implement Phase 3: Init file operation tests during connected mode with round-trip verification -- 4 tests" + status: pending + - id: phase4-meta + content: "Implement Phase 4: Property/meta file change tests -- 4 tests" + status: pending + - id: phase5-models + content: "Implement Phase 5: Model JSON file change tests -- 5 tests" + status: pending + - id: phase6-cursor + content: "Implement Phase 6: Cursor tracking and reconnection tests -- 3 tests" + status: pending + - id: phase7-mixed + content: "Implement Phase 7: Mixed API + filesystem verification tests -- 2 tests" + status: pending + - id: phase8-slugify-fixture + content: Create connected_slugify fixture with slugified files, dedup ~1 files, clean files, and directory-format slugified instances + status: pending + - id: phase8-slugify-forward + content: "Implement Phase 8a: Slugify forward-sync tests -- editing/creating/deleting slugified files and verifying patches show real names from meta -- 6 tests" + status: pending + - id: phase8-dedup-forward + content: "Implement Phase 8b: Dedup (~N suffix) forward-sync tests -- collision handling, ~1 with and without meta, add/remove colliding files -- 5 tests" + status: pending + - id: phase8-slugdir + content: "Implement Phase 8c: Slugified directory format tests -- init type changes, child adds inside slugified dirs -- 3 tests" + status: pending + - id: phase8-meta-name-lifecycle + content: "Implement Phase 8d: Meta name field lifecycle tests -- create/update/delete meta name, model.json5 name field -- 5 tests" + status: pending + - id: phase8-slug-multistep + content: "Implement Phase 8e: Multi-step slugify scenarios with round-trip verification -- slug rename chains, collision evolution -- 3 tests" + status: pending + - id: run-and-review + content: Run cargo test, cargo insta review, accept snapshots, verify clean pass + status: pending +isProject: false +--- + +# Connected Mode Test Expansion + +## Quality Standard + +This test expansion is held to the same standard as the [slugify branch audit](audit_slugify_branch_e913b7dc.plan.md): + +> **Round-trip identity**: Syncback (or two-way sync) writes a directory tree. Building an rbxl from that directory tree and forward-syncing it back must produce a **bit-identical instance tree** -- same names, same classes, same properties, same hierarchy, same ref targets. Any deviation is a bug. + +Every test in this plan must verify not just "did the WebSocket patch look right" but also "if I killed the server and rebuilt from the filesystem right now, would I get the same tree?" This is verified by starting a fresh `rojo serve` on the same project directory after changes and comparing the resulting tree. + +--- + +## Problem + +The current test suite has a major asymmetry: + +- `**two_way_sync.rs**` (plugin -> filesystem via API): 119 tests, covering stress, format transitions, encoded names, concurrent ops, failure recovery +- `**serve.rs**` (filesystem -> plugin via WebSocket patches): 22 tests, each making only ONE change. No multi-step sequences, no format transitions, no directory conversions, no round-trip verification + +Connected mode means the plugin is connected and receiving patches over time as the filesystem evolves. The `serve.rs` tests only verify a single atomic change, never simulating a session where the tree evolves through multiple states. They also never verify the round-trip invariant -- that the filesystem state after changes would produce an identical tree if rebuilt from scratch. + +--- + +## Architecture of a Connected Mode Test + +Each test follows this pattern in [tests/tests/serve.rs](tests/tests/serve.rs): + +```rust +run_serve_test("fixture_name", |session, mut redactions| { + // 1. Snapshot initial state + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + let read = session.get_api_read(root_id).unwrap(); + assert_yaml_snapshot!("test_initial", read.intern_and_redact(&mut redactions, root_id)); + + // 2. Change 1 -> verify patch + state + let packet = session.recv_socket_packet(SocketPacketType::Messages, 0, || { + fs::write(&path, "new content").unwrap(); + }).unwrap(); + assert_yaml_snapshot!("test_patch1", packet.intern_and_redact(&mut redactions, ())); + let read = session.get_api_read(root_id).unwrap(); + assert_yaml_snapshot!("test_state1", read.intern_and_redact(&mut redactions, root_id)); + + // 3. Change 2 -> verify patch + state (using cursor from patch 1) + let cursor = get_message_cursor(&packet); + let packet = session.recv_socket_packet(SocketPacketType::Messages, cursor, || { + fs::rename(&old, &new).unwrap(); + }).unwrap(); + assert_yaml_snapshot!("test_patch2", packet.intern_and_redact(&mut redactions, ())); + + // 4. Round-trip verification: fresh rebuild must match live tree + let final_read = session.get_api_read(root_id).unwrap(); + let fresh_read = fresh_rebuild_read(session.path()); + assert_trees_match(&final_read, &fresh_read); +}); +``` + +The critical addition over existing serve.rs tests: + +- **Multi-step sequences** with cursor chaining +- **Round-trip verification** at the end -- a fresh `rojo serve` on the same filesystem must produce the same instance tree + +Key files involved: + +- [tests/tests/serve.rs](tests/tests/serve.rs) -- where new tests go +- [tests/rojo_test/serve_util.rs](tests/rojo_test/serve_util.rs) -- test harness +- [rojo-test/serve-tests/](rojo-test/serve-tests/) -- test fixtures +- [rojo-test/serve-test-snapshots/](rojo-test/serve-test-snapshots/) -- insta snapshots + +--- + +## Harness Changes + +### `get_message_cursor` -- cursor extraction for chaining + +[tests/rojo_test/serve_util.rs](tests/rojo_test/serve_util.rs) needs a helper to extract `messageCursor` from a `SocketPacket` for chaining multi-step tests: + +```rust +/// Extract the message cursor from a SocketPacket for use in subsequent subscriptions. +pub fn get_message_cursor(packet: &SocketPacket) -> u32 { + match &packet.body { + SocketPacketBody::Messages(msg) => msg.message_cursor, + _ => panic!("Expected Messages packet"), + } +} +``` + +### `fresh_rebuild_read` -- round-trip identity verification + +This is the core addition for enforcing the round-trip standard. After all changes, start a **fresh** `rojo serve` on the same project directory and read the full tree. If the live DOM and the fresh rebuild disagree, the filesystem state is inconsistent. + +```rust +/// Start a fresh rojo serve on the given project path, read the full tree, +/// and return a normalized representation for comparison. +/// +/// This verifies the round-trip invariant: the filesystem state written by +/// the live session, when read from scratch, must produce the same tree. +pub fn fresh_rebuild_read(project_path: &Path) -> NormalizedTree { + // Find the project file in the directory + let project_file = find_project_file(project_path); + + let port = get_port_number(); + let port_string = port.to_string(); + let working_dir = get_working_dir_path(); + + let mut process = Command::new(ROJO_PATH) + .args(["serve", project_file.to_str().unwrap(), "--port", &port_string]) + .current_dir(working_dir) + .stderr(Stdio::piped()) + .spawn() + .expect("Couldn't start fresh Rojo for round-trip check"); + + let kill_guard = KillOnDrop(process); + + // Wait for fresh server to come online (same backoff as TestServeSession) + let info = wait_for_server(port); + let read = get_read(port, info.root_instance_id); + + // Normalize: strip Refs (non-deterministic), sort children, extract + // (name, class, properties, hierarchy) tuples for structural comparison + normalize_tree(&read) +} +``` + +### `NormalizedTree` -- structural comparison ignoring Refs + +Since two separate `rojo serve` sessions produce different `Ref` values, we need structural comparison. A `NormalizedTree` is a recursive structure of `(name, class_name, sorted_properties, sorted_children)` that can be compared with `==` or snapshotted. + +```rust +#[derive(Debug, PartialEq, Eq, serde::Serialize)] +pub struct NormalizedInstance { + pub name: String, + pub class_name: String, + pub properties: BTreeMap, // property name -> debug representation + pub children: Vec, // sorted by (name, class_name) +} +``` + +### `assert_round_trip` -- convenience assertion + +```rust +/// Assert that the live session tree matches a fresh rebuild from the filesystem. +pub fn assert_round_trip(session: &TestServeSession, root_id: Ref) { + let live_read = session.get_api_read(root_id).unwrap(); + let live_tree = normalize_read_response(&live_read, root_id); + + let fresh_tree = fresh_rebuild_read(session.path()); + + assert_eq!( + live_tree, fresh_tree, + "Round-trip identity violation: live tree and fresh rebuild differ. \ + The filesystem state does not faithfully represent the instance tree." + ); +} +``` + +--- + +## New Test Fixtures + +### `rojo-test/serve-tests/connected_scripts/` + +Fixture with both standalone and directory format scripts, covering all three script types: + +``` +connected_scripts/ + default.project.json5 # name: "connected_scripts", tree: { ReplicatedStorage: { $path: "src" } } + src/ + standalone.luau # ModuleScript (standalone format) + server.server.luau # Script (standalone format) + local.client.luau # LocalScript (standalone format) + DirModule/ + init.luau # ModuleScript (directory format) + ChildA.luau # child ModuleScript + ChildB.luau # child ModuleScript + DirScript/ + init.server.luau # Script (directory format) + Handler.luau # child ModuleScript + DirLocal/ + init.local.luau # LocalScript (directory format) + Helper.luau # child ModuleScript +``` + +### `rojo-test/serve-tests/connected_init/` + +Focused init file fixture for init-specific tests: + +``` +connected_init/ + default.project.json5 # name: "connected_init", tree: { ReplicatedStorage: { $path: "src" } } + src/ + init.luau # Root is a ModuleScript + Child.luau # A child module +``` + +### `rojo-test/serve-tests/connected_models/` + +Model file fixture: + +``` +connected_models/ + default.project.json5 # name: "connected_models", tree: { ReplicatedStorage: { $path: "src" } } + src/ + SimpleModel.model.json5 # { "className": "Configuration", "properties": { "Name": "SimpleModel" } } + DirModel/ + init.meta.json5 # { "className": "Folder" } + Child.luau # child ModuleScript +``` + +--- + +## New Tests -- Organized by Category + +### Phase 1: Format Transitions (filesystem-driven, patch + round-trip verified) + +Highest priority. Tests what the plugin receives when files transform between standalone and directory formats, AND verifies the filesystem is consistent after each transformation. + + +| # | Test Name | What It Tests | Round-Trip | +| --- | ----------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------- | +| 1 | `standalone_to_directory_add_child` | Create `ChildNew.luau` next to `standalone.luau`. The snapshot system should convert `standalone.luau` -> `standalone/init.luau` + `standalone/ChildNew.luau`. Verify the patch shows the correct tree transformation. | Yes | +| 2 | `directory_to_standalone_remove_children` | Remove `ChildA.luau` and `ChildB.luau` from `DirModule/`. With no children left, verify whether `DirModule/init.luau` collapses to `DirModule.luau`. Verify patch. | Yes | +| 3 | `standalone_to_directory_and_back` | Multi-step: (a) add child next to standalone script (standalone->dir), (b) remove child (dir->standalone). Two patches verified, plus round-trip after each step. | Yes | +| 4 | `init_type_change_module_to_server` | Rename `DirModule/init.luau` -> `DirModule/init.server.luau`. Verify patch contains ClassName change from ModuleScript to Script. | Yes | +| 5 | `init_type_change_server_to_local` | Rename `DirScript/init.server.luau` -> `DirScript/init.local.luau`. Verify ClassName change from Script to LocalScript. | Yes | +| 6 | `init_type_change_cycle` | Multi-step: `init.luau` -> `init.server.luau` -> `init.local.luau` -> `init.luau`. Three patches, three state snapshots, round-trip at each step. | Yes | +| 7 | `init_delete_children_survive` | Delete `DirModule/init.luau` while `ChildA.luau` and `ChildB.luau` remain. Verify children survive and parent becomes Folder (or whatever the correct behavior is). | Yes | +| 8 | `init_create_in_existing_folder` | Given a folder with only children (no init file), create `init.luau`. Verify the Folder instance becomes a ModuleScript. | Yes | + + +**Important for tests 1-2**: The snapshot middleware may or may not automatically convert between standalone and directory format when children are added/removed via the filesystem. If it doesn't (i.e., the conversion only happens through the API/change processor), the test should document that and verify the actual behavior. + +### Phase 2: Multi-Step Connected Sequences + +Core connected mode pattern: the tree evolves through multiple states. Each intermediate state is verified. + + +| # | Test Name | What It Tests | Round-Trip | +| --- | ------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------ | +| 9 | `create_edit_rename_delete_sequence` | Create `src/NewScript.luau` -> edit its contents -> rename to `Renamed.luau` -> delete it. Four patches, four state snapshots. Verify each patch contains exactly the expected delta. | Yes (at end) | +| 10 | `directory_lifecycle` | Create `src/NewDir/` -> add `init.luau` + `Child.luau` -> edit `init.luau` -> remove `Child.luau` -> remove directory. Full directory lifecycle with 5 patches. | Yes (at end) | +| 11 | `multi_file_sequential_edits` | Edit `standalone.luau`, then `server.server.luau`, then `local.client.luau`. Verify each patch only contains the changed file (no cross-contamination). | No | +| 12 | `add_multiple_files_sequentially` | Add 3 new `.luau` files one at a time. Verify each patch adds exactly one instance with correct name and class. | Yes (at end) | +| 13 | `remove_multiple_files_sequentially` | Remove 3 files one at a time. Verify each patch removes exactly one instance. | No | + + +### Phase 3: Init File Operations (connected mode) + + +| # | Test Name | What It Tests | Round-Trip | +| --- | ---------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------- | ---------- | +| 14 | `edit_init_then_edit_child` | Edit `DirModule/init.luau`, then edit `DirModule/ChildA.luau`. Two separate patches, each containing only the changed instance's Source property. | No | +| 15 | `replace_init_file_type` | Delete `DirModule/init.luau`, then create `DirModule/init.server.luau`. Verify the ClassName changes from ModuleScript to Script, children preserved. | Yes | +| 16 | `add_init_to_plain_folder` | Start with a directory containing only children (create by removing init from fixture). Add `init.luau`. Verify Folder -> ModuleScript. | Yes | +| 17 | `delete_init_from_directory` | Delete init file from directory with children. Verify parent becomes Folder, children survive. | Yes | + + +### Phase 4: Property/Meta Changes (connected mode) + + +| # | Test Name | What It Tests | Round-Trip | +| --- | ------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------- | ---------- | +| 18 | `adjacent_meta_creation` | Create `standalone.meta.json5` with `{ "properties": { "Disabled": true } }` next to `standalone.luau`. Verify property appears in patch. | Yes | +| 19 | `adjacent_meta_edit` | Edit existing `.meta.json5` to change a property value. Verify update patch. | No | +| 20 | `adjacent_meta_delete` | Delete `.meta.json5`. Verify properties revert to defaults. | Yes | +| 21 | `init_meta_creation` | Create `DirModule/init.meta.json5` with properties. Verify directory instance gains properties. | Yes | + + +### Phase 5: Model File Changes (connected mode) + + +| # | Test Name | What It Tests | Round-Trip | +| --- | ---------------------------- | ------------------------------------------------------------------------------------------ | ---------- | +| 22 | `model_json_edit_properties` | Edit `SimpleModel.model.json5` to change/add properties. Verify property changes in patch. | Yes | +| 23 | `model_json_add_children` | Edit model JSON to add child instances. Verify additions in patch. | Yes | +| 24 | `model_json_remove_children` | Edit model JSON to remove child instances. Verify removals in patch. | No | +| 25 | `model_json_create` | Create new `NewModel.model.json5`. Verify instance addition patch. | Yes | +| 26 | `model_json_delete` | Delete `SimpleModel.model.json5`. Verify instance removal patch. | No | + + +### Phase 6: Cursor Tracking and Reconnection + + +| # | Test Name | What It Tests | Round-Trip | +| --- | ------------------------------- | -------------------------------------------------------------------------------------------------------------------------- | ---------- | +| 27 | `cursor_advances_correctly` | Make 3 filesystem changes. After each, verify the cursor returned in the WebSocket packet increments monotonically. | No | +| 28 | `reconnect_with_old_cursor` | Make 2 changes with cursor advancement. Reconnect with cursor 0. Verify ALL patches are received in the catch-up response. | No | +| 29 | `reconnect_with_partial_cursor` | Make 3 changes. Reconnect with cursor from after change 1. Verify only patches 2+3 are received. | No | + + +### Phase 7: Mixed API + Filesystem Verification + + +| # | Test Name | What It Tests | Round-Trip | +| --- | ---------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ---------- | +| 30 | `api_write_then_filesystem_change` | POST `/api/write` to update Source, then edit a different file on disk. Verify only the filesystem change produces a WebSocket patch (echo suppression working). | Yes | +| 31 | `filesystem_change_then_api_write` | Edit file on disk, receive patch. Then POST `/api/write` on a different instance. Verify the API write produces the correct filesystem change and does NOT echo back as a duplicate patch. | Yes | + + +### Phase 8: Slugify / Dedup / Meta Name (connected mode, round-trip critical) + +This is the most critical phase for round-trip identity. The slugify system replaces forbidden filesystem chars with `_`, deduplicates collisions with `~1`/`~2` suffixes, and stores the real instance name in `.meta.json5` / `.model.json5` `name` fields. If ANY part of this chain is broken in the forward-sync direction (filesystem -> plugin patches), instance names are silently corrupted. + +The existing `two_way_sync.rs` has 17+ tests for slugify/dedup via the API write path (plugin -> filesystem). **None** of those verify the forward-sync direction: what WebSocket patch does the plugin receive when slugified files are changed on the filesystem? This phase fills that gap completely. + +#### New Fixture: `rojo-test/serve-tests/connected_slugify/` + +Richer than `syncback_encoded_names` -- includes dedup collisions and directory-format slugified instances: + +``` +connected_slugify/ + default.project.json5 # name: "connected_slugify", tree: { ReplicatedStorage: { $path: "src" } } + src/ + # Clean-named files (no meta needed) + Normal.luau # ModuleScript "Normal" + CleanScript.server.luau # Script "CleanScript" + + # Slugified files (meta has real name) + What_Module.luau # ModuleScript, meta: "What?Module" + What_Module.meta.json5 # { "name": "What?Module" } + Key_Script.server.luau # Script, meta: "Key:Script" + Key_Script.meta.json5 # { "name": "Key:Script" } + + # Dedup collision: two instances whose slugs both = "Hey_Bro" + Hey_Bro.luau # ModuleScript "Hey_Bro" (natural name, no meta) + Hey_Bro~1.luau # ModuleScript, meta: "Hey/Bro" + Hey_Bro~1.meta.json5 # { "name": "Hey/Bro" } + + # Model JSON with name field + What_Model.model.json5 # { "name": "What?Model", "className": "Configuration" } + + # Slugified directory format + Slug_Dir/ # Directory, meta: "Slug:Dir" + init.luau # ModuleScript init + init.meta.json5 # { "name": "Slug:Dir" } + DirChild.luau # child ModuleScript +``` + +#### Phase 8a: Slugify Forward-Sync (filesystem -> plugin patches) + +Tests that editing/creating/deleting slugified files produces patches with correct **real** instance names (from meta), not filesystem names (slugs). + + +| # | Test Name | What It Tests | Round-Trip | +| --- | ------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------- | +| 32 | `edit_slugified_script_source` | Edit `What_Module.luau` content. Verify WebSocket patch contains `changedProperties.Source` for instance named `What?Module` (NOT `What_Module`). The meta `name` field must be respected during forward-sync. | Yes | +| 33 | `edit_slugified_server_script_source` | Edit `Key_Script.server.luau` content. Verify patch contains Source change for instance named `Key:Script`. Same principle, different script type. | No | +| 34 | `create_new_slugified_file_with_meta` | Create `New_Slug.luau` + `New_Slug.meta.json5` with `{"name": "New/Slug"}`. Verify the patch adds an instance named `New/Slug` (not `New_Slug`). | Yes | +| 35 | `delete_slugified_file_and_meta` | Delete both `What_Module.luau` and `What_Module.meta.json5`. Verify the patch removes exactly one instance (the one named `What?Module`). | No | +| 36 | `edit_only_meta_name_field` | Edit `What_Module.meta.json5` to change `{"name": "What?Module"}` -> `{"name": "What*Module"}`. Verify the patch contains a name change (or re-snapshot). The slug stays the same but the real name changes. This is the "Foo/Bar -> Foo | Bar, both slugify to Foo_Bar" case from the audit. | +| 37 | `delete_meta_name_reverts_to_stem` | Delete `What_Module.meta.json5` (but leave `What_Module.luau`). Verify the patch shows the instance name changing from `What?Module` to `What_Module` (the filename stem becomes the name when no meta exists). | Yes | + + +#### Phase 8b: Dedup (~N Suffix) Forward-Sync + +Tests that `~N` suffixed files are handled correctly: the suffix is just part of the filename stem, the real name comes from meta. Without meta, the full stem (including `~N`) IS the instance name. + + +| # | Test Name | What It Tests | Round-Trip | +| --- | ----------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------- | +| 38 | `edit_dedup_file_with_meta` | Edit `Hey_Bro~1.luau` content. Verify patch shows Source change for instance named `Hey/Bro` (from meta), not `Hey_Bro~1`. | Yes | +| 39 | `dedup_file_without_meta_uses_stem` | Create `Foo~1.luau` with NO meta file. Verify the instance name in the patch is literally `Foo~1` -- the tilde is NOT parsed as a dedup marker during forward-sync. This is the backward compatibility guarantee. | Yes | +| 40 | `add_second_colliding_file` | Start with `Hey_Bro.luau` (name: `Hey_Bro`). Create `Hey_Bro~1.luau` + `Hey_Bro~1.meta.json5` with `{"name": "Hey:Bro"}`. Verify the patch adds a NEW instance named `Hey:Bro` as a sibling, without disturbing `Hey_Bro`. | Yes | +| 41 | `remove_one_of_two_colliding_files` | Delete `Hey_Bro~1.luau` + `Hey_Bro~1.meta.json5`. Verify the patch removes exactly the instance named `Hey/Bro`, leaving `Hey_Bro` untouched. | Yes | +| 42 | `add_third_collision` | With `Hey_Bro.luau` and `Hey_Bro~1.luau` already present, create `Hey_Bro~2.luau` + `Hey_Bro~2.meta.json5` with `{"name": "Hey*Bro"}`. Verify patch adds instance named `Hey*Bro`. All three coexist. | Yes | + + +#### Phase 8c: Slugified Directory Format + +Tests that directory-format instances with slugified names work correctly during connected mode. The real name is in `init.meta.json5`. + + +| # | Test Name | What It Tests | Round-Trip | +| --- | ----------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------- | +| 43 | `edit_init_in_slugified_directory` | Edit `Slug_Dir/init.luau` content. Verify patch shows Source change for instance named `Slug:Dir` (from `init.meta.json5`), not `Slug_Dir`. | Yes | +| 44 | `init_type_change_in_slugified_directory` | Rename `Slug_Dir/init.luau` -> `Slug_Dir/init.server.luau`. Verify ClassName changes from ModuleScript to Script. Instance name must remain `Slug:Dir` (from meta). | Yes | +| 45 | `add_child_to_slugified_directory` | Create `Slug_Dir/NewChild.luau`. Verify patch adds a new ModuleScript child under the instance named `Slug:Dir`. | Yes | + + +#### Phase 8d: Meta Name Field Lifecycle (connected mode) + +Tests the full lifecycle of the meta `name` field -- the bridge between filesystem names and real instance names. Any bug here silently corrupts names. + + +| # | Test Name | What It Tests | Round-Trip | +| --- | --------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ---------- | +| 46 | `add_meta_name_to_clean_file` | `Normal.luau` has no meta (name = `Normal`). Create `Normal.meta.json5` with `{"name": "Nor/mal"}`. Verify the patch shows the instance name changing from `Normal` to `Nor/mal`. | Yes | +| 47 | `update_meta_name_field` | `What_Module.meta.json5` has `{"name": "What?Module"}`. Change to `{"name": "What:Module"}`. Verify the patch shows name change. The slug (`What_Module`) doesn't change, only the real name does. | Yes | +| 48 | `meta_name_with_other_properties` | Create meta file with BOTH `name` AND `$properties`: `{"name": "Real/Name", "$properties": {"Disabled": true}}`. Verify both the name override AND the property appear in the patch. | Yes | +| 49 | `model_json_name_field_edit` | Edit `What_Model.model.json5` to change `"name": "What?Model"` -> `"name": "What:Model"`. Verify name change in patch. The `name` field in `.model.json5` serves the same purpose as in `.meta.json5`. | Yes | +| 50 | `model_json_remove_name_field` | Edit `What_Model.model.json5` to remove the `"name"` field entirely. Verify instance name reverts from `What?Model` to `What_Model` (derived from filename stem). | Yes | + + +#### Phase 8e: Multi-Step Slugify Scenarios (round-trip proofs) + +Multi-step sequences that exercise the slugify system through state transitions, with round-trip verification at each step. These are the highest-value tests for catching subtle bugs. + + +| # | Test Name | What It Tests | Round-Trip | +| --- | --------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------- | +| 51 | `slugify_rename_chain` | Multi-step: (1) Edit `What_Module.luau` content, (2) Update meta name `What?Module` -> `What*Module`, (3) Delete meta (name reverts to `What_Module`), (4) Re-create meta with `{"name": "What/Module"}`. Four patches, four state snapshots. Round-trip verified at each step. | Yes (each step) | +| 52 | `collision_evolution` | Multi-step: (1) Add `Hey_Bro~2.luau` + meta `{"name": "Hey*Bro"}` (third collision), (2) Delete `Hey_Bro~1.luau` + meta (remove `Hey/Bro`), (3) Add `Hey_Bro~1.luau` + meta with different name `{"name": "Hey | Bro"}`(reuse the`~1` slot). Three patches. Verify correct instances exist at each step. | +| 53 | `clean_to_slug_to_clean_via_filesystem` | Multi-step: (1) `Normal.luau` exists (clean name). Add `Normal.meta.json5` with `{"name": "Nor:mal"}` -- name changes to `Nor:mal`. (2) Delete the meta -- name reverts to `Normal`. (3) Rename `Normal.luau` -> `Has_Slash.luau` + create `Has_Slash.meta.json5` with `{"name": "Has/Slash"}` -- completely different slug+name. Round-trip verified at each step. | Yes (each step) | + + +--- + +## Execution Order + +1. **Fixtures**: Create 4 new fixture directories: `connected_scripts`, `connected_init`, `connected_models`, `connected_slugify` +2. **Harness**: Add `get_message_cursor`, `NormalizedTree`/`NormalizedInstance`, `fresh_rebuild_read`, `assert_round_trip` to [tests/rojo_test/serve_util.rs](tests/rojo_test/serve_util.rs) +3. **Phase 1** (format transitions): Highest value, most likely to catch bugs in the snapshot pipeline +4. **Phase 2** (multi-step sequences): Core connected mode pattern, validates cursor chaining +5. **Phase 3** (init file ops): Critical for directory handling correctness +6. **Phases 4-7**: Property/meta, model JSON, cursor tracking, mixed API+filesystem +7. **Phase 8a-e** (slugify/dedup): 22 tests covering the entire forward-sync direction of the name system, with round-trip proofs +8. **Run**: `cargo test`, then `cargo insta review` to accept snapshots +9. **Verify**: All tests pass on a clean run, no snapshot drift + +--- + +## Notes on Test Design + +### Filesystem events are OS-dependent + +Some operations (like rename) may fire different events on macOS (kqueue/FSEvents) vs Windows (ReadDirectoryChangesW) vs Linux (inotify). Tests that depend on exact patch structure may need `#[cfg_attr(target_os = "macos", ignore)]` annotations if event ordering differs. The existing `ref_properties_remove` test already does this. + +### The snapshot system may not auto-convert formats + +When adding a child file next to a standalone script via the filesystem, the snapshot system may NOT automatically convert to directory format (that conversion might only happen through the change processor during API writes). Tests should verify and document the actual behavior rather than assuming conversion happens. + +### Echo suppression is critical for mixed tests + +Phase 7 tests depend on echo suppression working correctly. The API write path suppresses VFS events for paths it wrote, preventing feedback loops. If echo suppression is broken, these tests will see duplicate patches. \ No newline at end of file diff --git a/.cursor/plans/fix_audit_findings_ca70f773.plan.md b/.cursor/plans/fix_audit_findings_ca70f773.plan.md index 8e8c9a32..72bca6c1 100644 --- a/.cursor/plans/fix_audit_findings_ca70f773.plan.md +++ b/.cursor/plans/fix_audit_findings_ca70f773.plan.md @@ -229,22 +229,26 @@ All in [tests/tests/two_way_sync.rs](tests/tests/two_way_sync.rs) and [tests/tes ### Files created -| File | Purpose | -|------|---------| -| `src/syncback/meta.rs` | Shared `upsert_meta_name()` and `remove_meta_name()` -- pure JSON operations without filesystem event suppression. Returns `RemoveNameOutcome` enum so callers handle suppression themselves. | -| `rojo-test/build-tests/tilde_no_meta/` | Build fixture: `Foo~1.luau` with no meta file. Verifies forward sync produces instance name `Foo~1`. | + +| File | Purpose | +| -------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `src/syncback/meta.rs` | Shared `upsert_meta_name()` and `remove_meta_name()` -- pure JSON operations without filesystem event suppression. Returns `RemoveNameOutcome` enum so callers handle suppression themselves. | +| `rojo-test/build-tests/tilde_no_meta/` | Build fixture: `Foo~1.luau` with no meta file. Verifies forward sync produces instance name `Foo~1`. | + ### Files modified -| File | What changed | -|------|-------------| -| `src/syncback/file_names.rs` | Added `KNOWN_SCRIPT_SUFFIXES`, `strip_script_suffix()`, `adjacent_meta_path()`. Added 19 new unit tests (suffix stripping, meta path derivation, tilde collision, stress, idempotency). | -| `src/syncback/mod.rs` | Added `pub mod meta;`. Exported `adjacent_meta_path`. Removed `deny_unknown_fields` from `SyncbackRules`. | -| `src/change_processor.rs` | Rewrote `upsert_meta_name_field` / `remove_meta_name_field` to delegate to shared `syncback::meta` module. **C1 fix**: moved meta name update logic OUTSIDE `if new_path != *path` for both init-file and regular-file rename paths. Introduced `effective_dir_path` / `effective_meta_base` tracking variables. | -| `src/web/api.rs` | **C2 fix**: `syncback_removed_instance` now uses `adjacent_meta_path()` instead of `instance_name` for meta cleanup. **CC2 fix**: init files trigger `remove_dir_all` on the parent directory + grandparent dir-level meta cleanup. Removed unused `instance_name` variable. Fixed misleading comment. | -| `src/snapshot_middleware/project.rs` | **CC1 fix**: collected `new_child_map.drain()` into sorted `Vec` before processing, ensuring deterministic dedup ordering. | -| `tests/tests/build.rs` | Added `tilde_no_meta` to `gen_build_tests!` macro. | -| `tests/tests/two_way_sync.rs` | Added 2 integration tests: `add_instance_with_forbidden_chars_creates_slug_and_meta`, `add_two_colliding_instances_deduplicates`. | + +| File | What changed | +| ------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `src/syncback/file_names.rs` | Added `KNOWN_SCRIPT_SUFFIXES`, `strip_script_suffix()`, `adjacent_meta_path()`. Added 19 new unit tests (suffix stripping, meta path derivation, tilde collision, stress, idempotency). | +| `src/syncback/mod.rs` | Added `pub mod meta;`. Exported `adjacent_meta_path`. Removed `deny_unknown_fields` from `SyncbackRules`. | +| `src/change_processor.rs` | Rewrote `upsert_meta_name_field` / `remove_meta_name_field` to delegate to shared `syncback::meta` module. **C1 fix**: moved meta name update logic OUTSIDE `if new_path != *path` for both init-file and regular-file rename paths. Introduced `effective_dir_path` / `effective_meta_base` tracking variables. | +| `src/web/api.rs` | **C2 fix**: `syncback_removed_instance` now uses `adjacent_meta_path()` instead of `instance_name` for meta cleanup. **CC2 fix**: init files trigger `remove_dir_all` on the parent directory + grandparent dir-level meta cleanup. Removed unused `instance_name` variable. Fixed misleading comment. | +| `src/snapshot_middleware/project.rs` | **CC1 fix**: collected `new_child_map.drain()` into sorted `Vec` before processing, ensuring deterministic dedup ordering. | +| `tests/tests/build.rs` | Added `tilde_no_meta` to `gen_build_tests!` macro. | +| `tests/tests/two_way_sync.rs` | Added 2 integration tests: `add_instance_with_forbidden_chars_creates_slug_and_meta`, `add_two_colliding_instances_deduplicates`. | + ### Test results @@ -255,10 +259,8 @@ All in [tests/tests/two_way_sync.rs](tests/tests/two_way_sync.rs) and [tests/tes ### Key design decisions -1. **`meta.rs` returns outcomes, doesn't suppress** -- The shared `upsert_meta_name` / `remove_meta_name` functions are pure I/O (read JSON, modify, write/delete). Filesystem event suppression stays in the caller (`change_processor.rs`) because it depends on the `JobThreadContext` API. This keeps the shared module framework-agnostic. +1. `**meta.rs` returns outcomes, doesn't suppress** -- The shared `upsert_meta_name` / `remove_meta_name` functions are pure I/O (read JSON, modify, write/delete). Filesystem event suppression stays in the caller (`change_processor.rs`) because it depends on the `JobThreadContext` API. This keeps the shared module framework-agnostic. +2. `**effective_dir_path` / `effective_meta_base` pattern** -- For the C1 fix, rather than duplicating the meta update logic in both branches of the path-change condition, we track which path ended up being used (original or renamed) and run the meta update once afterward. This avoids the "nested inside conditional" bug class entirely. +3. `**adjacent_meta_path` derives from filesystem name** -- The helper strips the file extension and script suffix from the actual filesystem path, never from the instance name. This ensures the meta path always matches the script file regardless of slugification. +4. **Alphabetical sort for project.rs determinism** -- Chose alphabetical sort over referent-ID sort because it's human-predictable and matches what a developer would expect when looking at the filesystem. -2. **`effective_dir_path` / `effective_meta_base` pattern** -- For the C1 fix, rather than duplicating the meta update logic in both branches of the path-change condition, we track which path ended up being used (original or renamed) and run the meta update once afterward. This avoids the "nested inside conditional" bug class entirely. - -3. **`adjacent_meta_path` derives from filesystem name** -- The helper strips the file extension and script suffix from the actual filesystem path, never from the instance name. This ensures the meta path always matches the script file regardless of slugification. - -4. **Alphabetical sort for project.rs determinism** -- Chose alphabetical sort over referent-ID sort because it's human-predictable and matches what a developer would expect when looking at the filesystem. \ No newline at end of file diff --git a/.cursor/plans/fix_stale_tree_desync_9e74f718.plan.md b/.cursor/plans/fix_stale_tree_desync_9e74f718.plan.md new file mode 100644 index 00000000..337b20aa --- /dev/null +++ b/.cursor/plans/fix_stale_tree_desync_9e74f718.plan.md @@ -0,0 +1,130 @@ +--- +name: Fix stale tree desync +overview: Add a tree validation mechanism that re-snapshots the project tree from the real filesystem when a plugin connects, ensuring the in-memory RojoTree never serves stale data even when VFS watcher events are missed. +todos: + - id: validate-method + content: Add `validate_tree()` method to ServeSession that re-snapshots from disk and patches the tree + status: completed + - id: trigger-on-connect + content: Call `validate_tree()` in the WebSocket connection handler before serving data + status: completed + - id: broadcast-corrections + content: Push any corrections from validation to the message queue for all connected plugins + status: completed + - id: test-validation + content: "Test: start serve, externally delete/modify a tracked file, connect plugin, verify tree is corrected" + status: completed +isProject: false +--- + +# Fix Stale Tree Desync on Plugin Connect + +## Problem + +When a plugin connects (especially in one-shot mode), the server's in-memory `RojoTree` can be stale if the VFS file watcher missed external filesystem changes. The tree is built once on startup and only updated via VFS events through `ChangeProcessor`. If events are missed (watcher buffer overflow, rapid batch operations like git, OS-level quirks), the tree silently drifts from reality. The plugin reads this stale tree via `GET /api/read/:id`, computes its diff against Studio, and sees "no changes" even though the filesystem is different. + +Rebooting `rojo serve` forces a fresh read and fixes it, confirming the issue is purely stale in-memory state. + +## Solution: Tree Validation on Plugin Connect + +When a new plugin connection is established (WebSocket handshake), trigger a validation pass that re-snapshots the root project path from the real filesystem, diffs against the current tree, and applies any corrections. This is essentially what `ServeSession::new` does on startup, but incremental and without restart. + +## Key Files + +- `[src/serve_session.rs](src/serve_session.rs)` -- Owns the tree, VFS, and project. Add `validate_tree()` method here. +- `[src/change_processor.rs](src/change_processor.rs)` -- Has `apply_patches()` which re-snapshots a path and patches the tree. Reuse this pattern. +- `[src/web/api.rs](src/web/api.rs)` -- WebSocket connection handler. Call validation before serving the tree. +- `[src/snapshot/patch_compute.rs](src/snapshot/patch_compute.rs)` -- `compute_patch_set` used to diff snapshots. + +## Implementation + +### 1. Add `validate_tree()` to ServeSession + +In `[src/serve_session.rs](src/serve_session.rs)`, add a public method that: + +- Takes the root project's start path (`self.root_project.folder_location()`) +- Locks the VFS and tree +- Calls `snapshot_from_vfs(&instance_context, &vfs, start_path)` to get a fresh snapshot from disk +- Calls `compute_patch_set(snapshot, &tree, root_id)` to diff against the current tree +- If the patch is non-empty, calls `apply_patch_set(&mut tree, patch_set)` and pushes the result to the message queue + +This reuses the same functions that `ServeSession::new` uses for initial tree construction. The VFS reads directly from the filesystem (no cache), so the snapshot will reflect the real state. + +A compile-time constant at the top of `serve_session.rs` controls whether validation runs: + +```rust +/// Set to `false` to skip tree validation on plugin connect (useful for testing). +const VALIDATE_TREE_ON_CONNECT: bool = true; +``` + +The method itself: + +```rust +pub fn validate_tree(&self) -> Vec { + if !VALIDATE_TREE_ON_CONNECT { + log::debug!("Tree validation skipped (VALIDATE_TREE_ON_CONNECT = false)"); + return Vec::new(); + } + + let start = std::time::Instant::now(); + let start_path = self.root_project.folder_location(); + let instance_context = InstanceContext::new(); + + let snapshot = match snapshot_from_vfs(&instance_context, &self.vfs, start_path) { + Ok(s) => s, + Err(e) => { + log::error!("Tree validation snapshot error: {:?}", e); + return Vec::new(); + } + }; + + let mut tree = self.tree.lock().unwrap(); + let root_id = tree.get_root_id(); + let patch_set = compute_patch_set(snapshot, &tree, root_id); + + if patch_set.is_empty() { + log::info!("Tree validation complete (no corrections needed) in {:.1?}", start.elapsed()); + return Vec::new(); + } + + log::info!("Tree validation found stale state, applying corrections"); + let applied = apply_patch_set(&mut tree, patch_set); + log::info!("Tree validation complete in {:.1?}", start.elapsed()); + vec![applied] +} +``` + +Note: The `Vfs` struct wraps the backend with a mutex internally (`self.inner.lock()`), so we access it via `self.vfs` (which is `Arc`) without needing an explicit lock. The exact locking pattern will follow the existing code style. + +### 2. Trigger Validation on WebSocket Connect + +In `[src/web/api.rs](src/web/api.rs)`, the WebSocket handler establishes a new connection. Before entering the message loop (or right after the handshake), call `validate_tree()` and broadcast any corrections: + +```rust +// In the WebSocket handler, after handshake: +let corrections = self.serve_session.validate_tree(); +if !corrections.is_empty() { + self.serve_session.message_queue().push_messages(&corrections); +} +``` + +This ensures the tree is consistent with disk before the plugin starts receiving data. + +### 3. Broadcast Corrections to Already-Connected Plugins + +If multiple plugins are connected, the corrections are pushed to the message queue, which broadcasts to all connected WebSocket clients. This ensures all plugins get the updated state. + +## Edge Cases + +- **Large projects**: Full re-snapshot could add 100-500ms latency on connect for very large projects. This is acceptable for one-shot mode (happens once) and live mode (happens on reconnect). Can be optimized later with targeted path validation if needed. +- **Concurrent access**: The tree lock is held during validation. Other WebSocket handlers will block briefly. This matches the existing pattern in `ChangeProcessor::apply_patches`. +- **No changes**: If the tree is already fresh (common case), `compute_patch_set` returns an empty patch and no work is done. The overhead is one `snapshot_from_vfs` call (directory walk + file stats). +- **VFS consistency**: After validation, VFS events for the re-snapshotted paths will still fire (watches persist). The `compute_and_apply_changes` handler will process them as no-ops since the tree already matches. + +## What This Fixes + +After this change, "no changes appearing" on plugin reconnect should not happen regardless of whether VFS events were missed. The tree is always refreshed from disk on connect, matching the behavior of a fresh `rojo serve` startup. + +## What This Does NOT Fix + +The content "swap" issue (both directions firing during per-item selection) is a separate bug that needs further investigation. The stale tree fix ensures that even if a swap occurs, the developer can immediately reconnect and see the correct diff to recover. \ No newline at end of file diff --git a/plugin/log/init.lua b/plugin/log/init.lua index 882be997..b1974b43 100644 --- a/plugin/log/init.lua +++ b/plugin/log/init.lua @@ -13,12 +13,12 @@ local function getLogLevel() end local function addTags(tag, message) - return tag .. message:gsub("\n", "\n" .. tag) + return tag .. message end -local TRACE_TAG = (" "):rep(15) .. "[Rojo-Trace] " -local INFO_TAG = (" "):rep(15) .. "[Rojo-Info] " -local DEBUG_TAG = (" "):rep(15) .. "[Rojo-Debug] " +local TRACE_TAG = "[Rojo-Trace] " +local INFO_TAG = "[Rojo-Info] " +local DEBUG_TAG = "[Rojo-Debug] " local WARN_TAG = "[Rojo-Warn] " local Log = {} diff --git a/plugin/src/ApiContext.lua b/plugin/src/ApiContext.lua index 4d6de81e..04cf0f63 100644 --- a/plugin/src/ApiContext.lua +++ b/plugin/src/ApiContext.lua @@ -197,9 +197,8 @@ function ApiContext:write(patch) addedCount += 1 end - -- Only log summary at debug level to reduce verbosity if #patch.removed > 0 or addedCount > 0 or #patch.updated > 0 then - Log.debug( + Log.info( "Sending to server: {} removals, {} additions, {} updates", #patch.removed, addedCount, @@ -207,15 +206,14 @@ function ApiContext:write(patch) ) end - -- Log individual operations at trace level (very detailed) for _, removed in ipairs(patch.removed) do - Log.trace("[Syncback] Remove ID: {}", tostring(removed)) + Log.info("[Syncback] Remove ID: {}", tostring(removed)) end for _, addedInstance in pairs(patch.added) do local instanceName = addedInstance.name or addedInstance.Name or "unknown" local instanceClass = addedInstance.className or addedInstance.ClassName or "unknown" - Log.trace("[Syncback] Add {} ({})", instanceName, instanceClass) + Log.info("[Syncback] Add {} ({})", instanceName, instanceClass) end for _, update in ipairs(patch.updated) do @@ -225,7 +223,7 @@ function ApiContext:write(patch) propCount += 1 end end - Log.trace("[Syncback] Update ID {} ({} properties)", update.id, propCount) + Log.info("[Syncback] Update ID {} ({} properties)", update.id, propCount) end local updated = {} diff --git a/plugin/src/ChangeBatcher/encodeInstance.lua b/plugin/src/ChangeBatcher/encodeInstance.lua index fa49024a..acbba93d 100644 --- a/plugin/src/ChangeBatcher/encodeInstance.lua +++ b/plugin/src/ChangeBatcher/encodeInstance.lua @@ -21,6 +21,7 @@ local Log = require(Packages.Log) local RbxDom = require(Packages.RbxDom) local encodeProperty = require(script.Parent.encodeProperty) +local UNENCODABLE_DATA_TYPES = require(script.Parent.propertyFilter) -- Script class names that need Source property local SCRIPT_CLASS_NAMES = { @@ -234,6 +235,10 @@ encodeInstance = function(instance, parentId, _skipPathCheck) -- Get the full PropertyDescriptor for encoding local descriptor = RbxDom.findCanonicalPropertyDescriptor(instance.ClassName, propertyName) if descriptor then + if UNENCODABLE_DATA_TYPES[descriptor.dataType] then + continue + end + local encodeSuccess, encodeResult = encodeProperty(instance, propertyName, descriptor) if encodeSuccess and encodeResult ~= nil then properties[propertyName] = encodeResult diff --git a/plugin/src/ChangeBatcher/encodePatchUpdate.lua b/plugin/src/ChangeBatcher/encodePatchUpdate.lua index 5076fea2..9f5c0441 100644 --- a/plugin/src/ChangeBatcher/encodePatchUpdate.lua +++ b/plugin/src/ChangeBatcher/encodePatchUpdate.lua @@ -3,6 +3,7 @@ local Log = require(Packages.Log) local RbxDom = require(Packages.RbxDom) local encodeProperty = require(script.Parent.encodeProperty) +local UNENCODABLE_DATA_TYPES = require(script.Parent.propertyFilter) return function(instance, instanceId, properties) local update = { @@ -21,6 +22,10 @@ return function(instance, instanceId, properties) continue end + if UNENCODABLE_DATA_TYPES[descriptor.dataType] then + continue + end + local encodeSuccess, encodeResult = encodeProperty(instance, propertyName, descriptor) if not encodeSuccess then diff --git a/plugin/src/ChangeBatcher/encodeProperty.lua b/plugin/src/ChangeBatcher/encodeProperty.lua index 52710e16..942ad410 100644 --- a/plugin/src/ChangeBatcher/encodeProperty.lua +++ b/plugin/src/ChangeBatcher/encodeProperty.lua @@ -11,7 +11,13 @@ return function(instance, propertyName, propertyDescriptor) end local dataType = propertyDescriptor.dataType - local encodeSuccess, encodeResult = RbxDom.EncodedValue.encode(readResult, dataType) + local pcallOk, encodeSuccess, encodeResult = pcall(RbxDom.EncodedValue.encode, readResult, dataType) + + if not pcallOk then + -- Encoder threw an error (e.g., Ref, Region3, SharedString) + Log.warn("Could not sync back property {:?}.{}: {}", instance, propertyName, encodeSuccess) + return false, nil + end if not encodeSuccess then Log.warn("Could not sync back property {:?}.{}: {}", instance, propertyName, encodeResult) diff --git a/plugin/src/ChangeBatcher/init.lua b/plugin/src/ChangeBatcher/init.lua index 226cbb80..a05c697b 100644 --- a/plugin/src/ChangeBatcher/init.lua +++ b/plugin/src/ChangeBatcher/init.lua @@ -107,12 +107,11 @@ function ChangeBatcher:__flush() return nil end - -- Log summary at debug level local addedCount = 0 for _ in pairs(patch.added) do addedCount += 1 end - Log.debug("Two-way sync: {} updates, {} additions, {} removals", #patch.updated, addedCount, #patch.removed) + Log.info("Two-way sync: {} updates, {} additions, {} removals", #patch.updated, addedCount, #patch.removed) return patch end diff --git a/plugin/src/ChangeBatcher/propertyFilter.lua b/plugin/src/ChangeBatcher/propertyFilter.lua new file mode 100644 index 00000000..a70a37c3 --- /dev/null +++ b/plugin/src/ChangeBatcher/propertyFilter.lua @@ -0,0 +1,16 @@ +--[[ + Property type blacklist for encoding, matching the server-side + filter_properties_for_meta behavior in src/web/api.rs. + + Types listed here are skipped during property encoding because they + cannot be serialized to JSON or are meaningless for syncback. +]] + +-- Property data types that cannot be encoded standalone. +-- Matches the Variant::Ref / Variant::UniqueId filter in api.rs. +local UNENCODABLE_DATA_TYPES = { + Ref = true, -- Instance references; need the InstanceMap, not encodable standalone + UniqueId = true, -- Internal Roblox identifiers; not meaningful for syncback +} + +return UNENCODABLE_DATA_TYPES diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 027b668f..547cdee0 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -27,13 +27,12 @@ local function applyPatch(instanceMap, patch) -- to ensure that referents can be mapped to instances correctly. local deferredRefs = {} - -- Log summary of patch being applied (debug level for summary) local addCount = 0 for _ in pairs(patch.added) do addCount += 1 end if #patch.removed > 0 or addCount > 0 or #patch.updated > 0 then - Log.debug( + Log.info( "Applying patch to Studio: {} removals, {} additions, {} updates", #patch.removed, addCount, @@ -124,7 +123,6 @@ local function applyPatch(instanceMap, patch) continue end - -- Log the update (debug level for property details to reduce verbosity) local instancePath = instance:GetFullName() local propList = {} if update.changedName then @@ -138,7 +136,7 @@ local function applyPatch(instanceMap, patch) table.insert(propList, propName) end end - Log.debug("[Studio] Update: {} - {}", instancePath, table.concat(propList, ", ")) + Log.info("[Studio] Update: {} - {}", instancePath, table.concat(propList, ", ")) -- Pause updates on this instance to avoid picking up our changes when -- two-way sync is enabled. diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 407feef3..09e2bd29 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -219,7 +219,7 @@ function ServeSession:__onWebSocketMessage(messagesPacket) return end - Log.debug("Received {} messages from Rojo server", #messagesPacket.messages) + Log.info("Received {} messages from Rojo server", #messagesPacket.messages) -- Combine all messages into a single patch local combinedPatch = PatchSet.newEmpty() @@ -227,6 +227,17 @@ function ServeSession:__onWebSocketMessage(messagesPacket) PatchSet.assign(combinedPatch, message) end + local addedCount = 0 + for _ in pairs(combinedPatch.added) do + addedCount += 1 + end + Log.info( + "Combined patch: {} removals, {} additions, {} updates", + #combinedPatch.removed, + addedCount, + #combinedPatch.updated + ) + -- If we're already waiting for confirmation, merge into the patch being confirmed -- Changes flow into the dialogue even in one-shot mode so user can see them if self.__confirmingPatch ~= nil then @@ -580,7 +591,7 @@ function ServeSession:__applyPatch(patch) end if not PatchSet.isEmpty(unappliedPatch) then - Log.debug( + Log.info( "Could not apply all changes requested by the Rojo server:\n{}", PatchSet.humanSummary(self.__instanceMap, unappliedPatch) ) @@ -696,7 +707,7 @@ function ServeSession:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo) ignoreCount += 1 end end - Log.debug("User selections: {} push, {} pull, {} ignored", pushCount, pullCount, ignoreCount) + Log.info("User selections: {} push, {} pull, {} ignored", pushCount, pullCount, ignoreCount) -- Build partial patches based on selections local pushPatch = PatchSet.newEmpty() -- Items to apply to Studio @@ -710,9 +721,9 @@ function ServeSession:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo) if selection == "push" then -- Apply Rojo changes to Studio - Log.trace("[Push] Update: {}", instancePath) - table.insert(pushPatch.updated, change) - elseif selection == "pull" and self.__twoWaySync then + Log.info("[Push] Update: {}", instancePath) + table.insert(pushPatch.updated, change) + elseif selection == "pull" and self.__twoWaySync then -- Send Studio state back to Rojo if instance then local propertiesToSync = change.changedProperties @@ -723,7 +734,7 @@ function ServeSession:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo) continue end end - Log.trace("[Pull] Update: {}", instancePath) + Log.info("[Pull] Update: {}", instancePath) local update = encodePatchUpdate(instance, change.id, propertiesToSync) if update then table.insert(pullPatch.updated, update) @@ -754,7 +765,7 @@ function ServeSession:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo) if selection == "push" then -- Apply Rojo removal to Studio (delete the instance) - Log.trace("[Push] Delete: {}", instancePath) + Log.info("[Push] Delete: {}", instancePath) table.insert(pushPatch.removed, idOrInstance) elseif selection == "pull" and self.__twoWaySync then -- Syncback: Create file in Rojo from Studio instance @@ -789,7 +800,7 @@ function ServeSession:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo) if selection == "push" then -- Apply Rojo addition to Studio - Log.trace("[Push] Add: {}.{}", parentPath, instanceName) + Log.info("[Push] Add: {}.{}", parentPath, instanceName) pushPatch.added[id] = change elseif selection == "pull" and self.__twoWaySync then -- Don't add in Studio, remove from Rojo @@ -810,12 +821,12 @@ function ServeSession:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo) for _ in pairs(pushPatch.added) do addCount += 1 end - Log.debug( - "Applying to Studio: {} additions, {} removals, {} updates", - addCount, - #pushPatch.removed, - #pushPatch.updated - ) + Log.info( + "Applying to Studio: {} additions, {} removals, {} updates", + addCount, + #pushPatch.removed, + #pushPatch.updated + ) self:__applyPatch(pushPatch) end diff --git a/src/change_processor.rs b/src/change_processor.rs index a71d294b..a1d0b72d 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -1,1501 +1,1540 @@ -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::{Path, PathBuf}; -use std::time::{Duration, Instant}; -use std::{ - fs, - sync::{Arc, Mutex}, -}; - -use crate::{ - message_queue::MessageQueue, - snapshot::{ - apply_patch_set, compute_patch_set, AppliedPatchSet, InstigatingSource, PatchSet, RojoTree, - }, - snapshot_middleware::{snapshot_from_vfs, snapshot_project_node}, - syncback::{deduplicate_name, name_needs_slugify, slugify_name, strip_script_suffix}, -}; - -/// Processes file change events, updates the DOM, and sends those updates -/// through a channel for other stuff to consume. -/// -/// Owns the connection between Rojo's VFS and its DOM by holding onto another -/// thread that processes messages. -/// -/// Consumers of ChangeProcessor, like ServeSession, are intended to communicate -/// with this object via channels. -/// -/// ChangeProcessor expects to be the only writer to the RojoTree and Vfs -/// objects passed to it. -pub struct ChangeProcessor { - /// Controls the runtime of the processor thread. When signaled, the job - /// thread will finish its current work and terminate. - /// - /// This channel should be signaled before dropping ChangeProcessor or we'll - /// hang forever waiting for the message processing loop to terminate. - shutdown_sender: Sender<()>, - - /// A handle to the message processing thread. When dropped, we'll block - /// until it's done. - /// - /// Allowed to be unused because dropping this value has side effects. - #[allow(unused)] - job_thread: JoinHandle>, -} - -impl ChangeProcessor { - /// Spin up the ChangeProcessor, connecting it to the given tree, VFS, and - /// outbound message queue. - pub fn start( - tree: Arc>, - 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(); - let task = JobThreadContext { - tree, - vfs, - message_queue, - pending_recovery: Mutex::new(Vec::new()), - suppressed_paths, - }; - - let job_thread = jod_thread::Builder::new() - .name("ChangeProcessor thread".to_owned()) - .spawn(move || { - log::trace!("ChangeProcessor thread started"); - - loop { - 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?); - }, - recv(shutdown_receiver) -> _ => { - 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(); - }, - } - } - }) - .expect("Could not start ChangeProcessor thread"); - - Self { - shutdown_sender, - job_thread, - } - } -} - -impl Drop for ChangeProcessor { - fn drop(&mut self) { - // Signal the job thread to start spinning down. Without this we'll hang - // forever waiting for the thread to finish its infinite loop. - let _ = self.shutdown_sender.send(()); - - // After this function ends, the job thread will be joined. It might - // block for a small period of time while it processes its last work. - } -} - -/// Contains all of the state needed to synchronize the DOM and VFS. -struct JobThreadContext { - /// A handle to the DOM we're managing. - tree: Arc>, - - /// A handle to the VFS we're managing. - vfs: Arc, - - /// 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 to avoid redundant re-snapshots. Values are `(remove_count, create_write_count)`. - suppressed_paths: Arc>>, -} - -impl JobThreadContext { - /// Find the init file inside a directory-format script folder. - /// Returns the path to the first `init.*.luau` or `init.*.lua` found. - fn find_init_file(dir: &Path) -> Option { - // Check known init file names in priority order - let candidates = [ - "init.luau", - "init.server.luau", - "init.client.luau", - "init.local.luau", - "init.plugin.luau", - "init.legacy.luau", - "init.lua", - "init.server.lua", - "init.client.lua", - "init.local.lua", - ]; - for name in &candidates { - let candidate = dir.join(name); - if candidate.exists() { - return Some(candidate); - } - } - None - } - - /// Canonicalize a path for use as a suppression map key. - fn suppression_key(path: &Path) -> PathBuf { - if let Ok(canonical) = std::fs::canonicalize(path) { - canonical - } else if let Some(parent) = path.parent() { - if let Ok(canonical_parent) = std::fs::canonicalize(parent) { - if let Some(file_name) = path.file_name() { - canonical_parent.join(file_name) - } else { - path.to_path_buf() - } - } else { - path.to_path_buf() - } - } else { - path.to_path_buf() - } - } - - /// Suppress the next Create/Write VFS event for the given path. - /// Prevents re-processing a file-watcher event we triggered ourselves. - fn suppress_path(&self, path: &Path) { - let mut suppressed = self.suppressed_paths.lock().unwrap(); - let key = Self::suppression_key(path); - suppressed.entry(key).or_insert((0, 0)).1 += 1; - } - - /// Remove a Create/Write suppression previously added by [`suppress_path`]. - /// Called when the filesystem operation failed, so that future VFS events - /// for that path are not incorrectly swallowed. - fn unsuppress_path(&self, path: &Path) { - let mut suppressed = self.suppressed_paths.lock().unwrap(); - let key = Self::suppression_key(path); - if let Some(counts) = suppressed.get_mut(&key) { - counts.1 = counts.1.saturating_sub(1); - if counts.0 == 0 && counts.1 == 0 { - suppressed.remove(&key); - } - } - } - - /// Suppress the next VFS event of ANY type for the given path. - /// Used for the **old** path of renames: different platforms deliver - /// different event types for the source of a rename (REMOVE on - /// Linux/Windows, stale CREATE on macOS FSEvents). - fn suppress_path_any(&self, path: &Path) { - let mut suppressed = self.suppressed_paths.lock().unwrap(); - let key = Self::suppression_key(path); - let entry = suppressed.entry(key).or_insert((0, 0)); - entry.0 += 1; - entry.1 += 1; - } - - /// Remove both counters previously added by [`suppress_path_any`]. - fn unsuppress_path_any(&self, path: &Path) { - let mut suppressed = self.suppressed_paths.lock().unwrap(); - let key = Self::suppression_key(path); - if let Some(counts) = suppressed.get_mut(&key) { - counts.0 = counts.0.saturating_sub(1); - counts.1 = counts.1.saturating_sub(1); - if counts.0 == 0 && counts.1 == 0 { - suppressed.remove(&key); - } - } - } - - /// Suppress the next Remove VFS event for the given path. - fn suppress_path_remove(&self, path: &Path) { - let mut suppressed = self.suppressed_paths.lock().unwrap(); - let key = Self::suppression_key(path); - suppressed.entry(key).or_insert((0, 0)).0 += 1; - } - - /// Upsert the `name` field in a `.meta.json5` file, suppressing filesystem - /// events to avoid feedback loops. - fn upsert_meta_name_field(&self, meta_path: &Path, real_name: &str) { - self.suppress_path(meta_path); - if let Err(err) = crate::syncback::meta::upsert_meta_name(meta_path, real_name) { - self.unsuppress_path(meta_path); - log::error!( - "Failed to upsert name in meta file {}: {}", - meta_path.display(), - err - ); - } - } - - /// Upsert the `name` field inside a `.model.json5` / `.model.json` file, - /// suppressing filesystem events. - fn upsert_model_name_field(&self, model_path: &Path, real_name: &str) { - self.suppress_path(model_path); - if let Err(err) = crate::syncback::meta::upsert_model_name(model_path, real_name) { - self.unsuppress_path(model_path); - log::error!( - "Failed to upsert name in model file {}: {}", - model_path.display(), - err - ); - } - } - - /// Remove the `name` field from a `.model.json5` / `.model.json` file, - /// suppressing filesystem events. - fn remove_model_name_field(&self, model_path: &Path) { - use crate::syncback::meta::RemoveNameOutcome; - self.suppress_path(model_path); - match crate::syncback::meta::remove_model_name(model_path) { - Ok(RemoveNameOutcome::NoOp) => { - self.unsuppress_path(model_path); - } - Ok(RemoveNameOutcome::FieldRemoved) => { - // File was rewritten — suppress_path already covers it. - } - Ok(RemoveNameOutcome::FileDeleted) => { - // Model files shouldn't be deleted (they have className etc), - // but handle for completeness. - self.unsuppress_path(model_path); - self.suppress_path_remove(model_path); - } - Err(err) => { - self.unsuppress_path(model_path); - log::error!( - "Failed to remove name from model file {}: {}", - model_path.display(), - err - ); - } - } - } - - /// Remove the `name` field from a `.meta.json5` file, suppressing filesystem - /// events. If the file becomes empty after removal, deletes it entirely. - fn remove_meta_name_field(&self, meta_path: &Path) { - use crate::syncback::meta::RemoveNameOutcome; - // Suppress for the write/delete that may follow - self.suppress_path(meta_path); - match crate::syncback::meta::remove_meta_name(meta_path) { - Ok(RemoveNameOutcome::NoOp) => { - self.unsuppress_path(meta_path); - } - Ok(RemoveNameOutcome::FileDeleted) => { - // File was deleted, not rewritten. Swap: undo the - // pre-emptive Write suppression and add a Remove - // suppression instead so the counts are (1, 0). - self.unsuppress_path(meta_path); - self.suppress_path_remove(meta_path); - } - Ok(RemoveNameOutcome::FieldRemoved) => { - // File was rewritten — suppress_path already covers it. - } - Err(err) => { - self.unsuppress_path(meta_path); - log::error!( - "Failed to remove name from meta file {}: {}", - meta_path.display(), - err - ); - } - } - } - - /// 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 { - 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::info!( - "apply_patches: path {} affects IDs {:?}", - current_path.display(), - ids - ); - - if !ids.is_empty() { - break ids.to_vec(); - } - - 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(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); - } - } - } - - applied_patches - } - - fn handle_vfs_event(&self, event: VfsEvent) { - // 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::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). - // Suppressions are event-type-aware: a Remove suppression only matches - // Remove events, and a Create/Write suppression only matches Create/Write - // events. This prevents a Remove suppression from consuming a Create event - // (which can happen on macOS when FSEvents coalesces delete+recreate). - 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 canonical = std::fs::canonicalize(path).ok(); - let mut suppressed = self.suppressed_paths.lock().unwrap(); - // Determine which key matches: try canonical first (most likely - // to match what suppress_path stored), then fall back to raw. - let matched_key = canonical - .as_ref() - .filter(|c| suppressed.contains_key(c.as_path())) - .cloned() - .or_else(|| { - if suppressed.contains_key(path) { - Some(path.clone()) - } else { - None - } - }); - if let Some(key) = matched_key { - if let Some(counts) = suppressed.get_mut(&key) { - let should_suppress = match &event { - VfsEvent::Remove(_) if counts.0 > 0 => { - counts.0 -= 1; - true - } - VfsEvent::Create(_) | VfsEvent::Write(_) if counts.1 > 0 => { - counts.1 -= 1; - true - } - _ => false, - }; - if counts.0 == 0 && counts.1 == 0 { - suppressed.remove(&key); - } - if should_suppress { - drop(suppressed); - // 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. - self.vfs - .commit_event(&event) - .expect("Error applying VFS change"); - - // 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::Write(path) => { - match self.vfs.canonicalize(&path) { - Ok(canonical_path) => { - log::info!( - "VFS: canonicalize OK for {} -> {}", - path.display(), - canonical_path.display() - ); - self.apply_patches(canonical_path) - } - Err(_) => { - // The path doesn't exist on disk. Two possible causes: - // - // 1. Phantom event from a rename we performed — macOS FSEvents - // can deliver a stale CREATE for the old path. If a pending - // suppression exists for this path, consume it and skip. - // - // 2. The file was deleted between the event firing and us - // processing it. Fall back to parent-directory canonicalize - // (same strategy as the Remove handler) so the tree can - // reconcile the disappearance. - let consumed = { - let key = Self::suppression_key(&path); - let mut suppressed = self.suppressed_paths.lock().unwrap(); - if let Some(counts) = suppressed.get_mut(&key) { - if counts.0 > 0 || counts.1 > 0 { - // Drain whichever counter is available - if counts.1 > 0 { - counts.1 -= 1; - } else { - counts.0 -= 1; - } - if counts.0 == 0 && counts.1 == 0 { - suppressed.remove(&key); - } - true - } else { - false - } - } else { - false - } - }; - - if consumed { - log::info!( - "VFS: phantom Create/Write for non-existent {} — \ - consumed pending suppression (likely stale rename event)", - path.display() - ); - Vec::new() - } else { - // No suppression — the file genuinely vanished. Use the - // parent-directory fallback so the tree can reconcile. - let parent = path.parent().unwrap(); - let file_name = path.file_name().unwrap(); - match self.vfs.canonicalize(parent) { - Ok(parent_normalized) => { - let resolved = parent_normalized.join(file_name); - log::info!( - "VFS: Create/Write for vanished file {} — \ - resolved via parent to {}", - path.display(), - resolved.display() - ); - self.apply_patches(resolved) - } - Err(err) => { - log::info!( - "VFS: Skipping Create/Write for {} — \ - parent no longer exists: {}", - path.display(), - err - ); - Vec::new() - } - } - } - } - } - } - 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. - // However, if the parent was also deleted (e.g., when deleting a directory - // tree), canonicalize will fail - in that case, just skip this event. - let parent = path.parent().unwrap(); - let file_name = path.file_name().unwrap(); - match self.vfs.canonicalize(parent) { - 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() - } - } - } - _ => { - log::warn!("Unhandled VFS event: {:?}", event); - Vec::new() - } - }; - - // 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::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 - // just made. - 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!( - "Processing client patch: {} removed, {} added, {} updated", - patch_set.removed_instances.len(), - patch_set.added_instances.len(), - patch_set.updated_instances.len() - ); - - let applied_patch = { - let mut tree = self.tree.lock().unwrap(); - - // NOTE: We do NOT delete files from disk here. The API handler - // (handle_api_write → remove_instance_at_path) already deleted - // the files before sending this PatchSet. Re-deleting would - // destroy any new file created at the same path — e.g., when an - // instance is renamed from "joe_test" to "joe/test", the new slug - // is also "joe_test", so the API creates a new file at the same - // path that the removal just deleted. If we deleted again here, - // the new file would be destroyed. - // - // The PatchSet removal only needs to update the DOM tree (handled - // by apply_patch_set below). - for &id in &patch_set.removed_instances { - if let Some(instance) = tree.get_instance(id) { - if let Some(instigating_source) = &instance.metadata().instigating_source { - match instigating_source { - InstigatingSource::Path(path) => { - log::info!( - "Two-way sync: Removing instance {:?} from tree (path: {})", - id, - path.display() - ); - } - InstigatingSource::ProjectNode { .. } => { - log::warn!( - "Cannot remove instance {:?}, it's from a project file", - id - ); - } - } - } else { - log::warn!( - "Cannot remove instance {:?}, it is not an instigating source.", - id - ); - } - } else { - log::warn!("Cannot remove instance {:?}, it does not exist.", id); - } - } - - // Collect (Ref, new_instigating_source) for instances whose - // filesystem path changed (rename / ClassName transition). - // Applied after the PatchSet to keep metadata in sync. - let mut metadata_updates: Vec<(Ref, PathBuf)> = Vec::new(); - - for update in &patch_set.updated_instances { - let id = update.id; - - if let Some(instance) = tree.get_instance(id) { - // Track the current source file path — rename and ClassName - // handlers may move the file, so the Source write must target - // the new location instead of the stale instigating_source. - let mut overridden_source_path: Option = None; - - // 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 file_name = - path.file_name().and_then(|f| f.to_str()).unwrap_or(""); - - // For directory-format scripts, the instigating_source - // is the init file (e.g., src/MyModule/init.luau). - // The instance name corresponds to the parent directory, - // not the init file, so we must rename the directory. - let is_init_file = file_name.starts_with("init."); - - if is_init_file { - let dir_path = path.parent().unwrap(); - if let Some(grandparent) = dir_path.parent() { - let dir_name = dir_path - .file_name() - .and_then(|f| f.to_str()) - .unwrap_or(""); - // Slugify the new name for filesystem safety - let slugified_new_name = - if name_needs_slugify(new_name) { - slugify_name(new_name) - } else { - new_name.to_string() - }; - // Directory renames: fs::rename fails safely - // on all platforms if the target is a non-empty - // directory, so no dedup is needed here (unlike - // the file rename path below, where Unix - // fs::rename silently overwrites). - let new_dir_path = - grandparent.join(&slugified_new_name); - - // Track the effective directory path after any rename - let effective_dir_path; - - if new_dir_path != dir_path { - log::info!( - "Two-way sync: Renaming directory {} -> {}", - dir_path.display(), - new_dir_path.display() - ); - self.suppress_path_any(dir_path); - self.suppress_path(&new_dir_path); - if let Err(err) = - fs::rename(dir_path, &new_dir_path) - { - self.unsuppress_path_any(dir_path); - self.unsuppress_path(&new_dir_path); - log::error!( - "Failed to rename directory {:?} to {:?}: {}", - dir_path, - new_dir_path, - err - ); - effective_dir_path = dir_path.to_path_buf(); - } else { - // The init file moved with the directory. - overridden_source_path = - Some(new_dir_path.join(file_name)); - let old_meta = grandparent.join(format!( - "{}.meta.json5", - dir_name - )); - if old_meta.exists() { - let new_meta = - grandparent.join(format!( - "{}.meta.json5", - slugified_new_name - )); - self.suppress_path_any(&old_meta); - self.suppress_path(&new_meta); - if fs::rename(&old_meta, &new_meta) - .is_err() - { - self.unsuppress_path_any(&old_meta); - self.unsuppress_path(&new_meta); - } - } - effective_dir_path = new_dir_path.clone(); - } - } else { - effective_dir_path = dir_path.to_path_buf(); - } - - // Always update the meta name field when the - // instance name changed, even if the directory - // path didn't change (e.g. "Foo/Bar" → "Foo|Bar" - // both slugify to "Foo_Bar"). - let init_meta = - effective_dir_path.join("init.meta.json5"); - if slugified_new_name != *new_name { - self.upsert_meta_name_field( - &init_meta, new_name, - ); - } else { - self.remove_meta_name_field(&init_meta); - } - } - } else if let Some(parent) = path.parent() { - // Derive the suffix from the existing filename - // rather than using replacen with the instance - // name, which may differ from the filesystem - // name (e.g., slugified names). - let extension = path - .extension() - .and_then(|e| e.to_str()) - .unwrap_or(""); - let stem = path - .file_stem() - .and_then(|s| s.to_str()) - .unwrap_or(""); - let known_suffixes = [ - ".server", ".client", ".plugin", ".local", - ".legacy", ".model", - ]; - let script_suffix = known_suffixes - .iter() - .find(|s| stem.ends_with(*s)) - .copied() - .unwrap_or(""); - let old_base = if script_suffix.is_empty() { - stem - } else { - &stem[..stem.len() - script_suffix.len()] - }; - - let slugified_new_name = if name_needs_slugify(new_name) - { - slugify_name(new_name) - } else { - new_name.to_string() - }; - - // Guard against rename collision: if the target - // path already exists and isn't our own file, - // deduplicate the name against siblings. - let deduped_new_name; - { - let candidate_file = if extension.is_empty() { - format!( - "{}{}", - slugified_new_name, script_suffix - ) - } else { - format!( - "{}{}.{}", - slugified_new_name, - script_suffix, - extension - ) - }; - let candidate = parent.join(&candidate_file); - if candidate != *path && candidate.exists() { - let mut taken: std::collections::HashSet< - String, - > = std::collections::HashSet::new(); - if let Ok(entries) = fs::read_dir(parent) { - for entry in entries.flatten() { - let ep = entry.path(); - let slug = if ep.is_dir() { - ep.file_name() - .and_then(|f| f.to_str()) - .unwrap_or("") - .to_lowercase() - } else { - let s = ep - .file_stem() - .and_then(|f| f.to_str()) - .unwrap_or(""); - strip_script_suffix(s) - .to_lowercase() - }; - taken.insert(slug); - } - } - // Free the slot we're vacating - taken.remove(&old_base.to_lowercase()); - deduped_new_name = deduplicate_name( - &slugified_new_name, - &taken, - ); - } else { - deduped_new_name = slugified_new_name.clone(); - } - } - - let new_file_name = if extension.is_empty() { - format!("{}{}", deduped_new_name, script_suffix) - } else { - format!( - "{}{}.{}", - deduped_new_name, script_suffix, extension - ) - }; - let new_path = parent.join(&new_file_name); - - // Track the effective meta base name after any rename - let effective_meta_base: &str; - - if new_path != *path { - log::info!( - "Two-way sync: Renaming {} -> {}", - path.display(), - new_path.display() - ); - self.suppress_path_any(path); - self.suppress_path(&new_path); - if let Err(err) = fs::rename(path, &new_path) { - self.unsuppress_path_any(path); - self.unsuppress_path(&new_path); - log::error!( - "Failed to rename {:?} to {:?}: {}", - path, - new_path, - err - ); - effective_meta_base = old_base; - } else { - overridden_source_path = Some(new_path.clone()); - let old_meta = parent - .join(format!("{}.meta.json5", old_base)); - let new_meta = parent.join(format!( - "{}.meta.json5", - deduped_new_name - )); - if old_meta.exists() { - self.suppress_path_any(&old_meta); - self.suppress_path(&new_meta); - if fs::rename(&old_meta, &new_meta).is_err() - { - self.unsuppress_path_any(&old_meta); - self.unsuppress_path(&new_meta); - } - } - effective_meta_base = &deduped_new_name; - } - } else { - effective_meta_base = old_base; - } - - // Always update the name field when the - // instance name changed, even if the path didn't - // change (e.g. "Foo/Bar" → "Foo|Bar" both slugify - // to "Foo_Bar"). Use deduped_new_name because - // dedup may have appended ~N. - // - // For .model.json5/.model.json files, the name - // field lives INSIDE the model file, not in - // adjacent .meta.json5. - if script_suffix == ".model" { - let model_file = overridden_source_path - .as_deref() - .unwrap_or(path.as_path()); - if deduped_new_name != *new_name { - self.upsert_model_name_field( - model_file, new_name, - ); - } else { - self.remove_model_name_field(model_file); - } - } else { - let current_meta = parent.join(format!( - "{}.meta.json5", - effective_meta_base - )); - if deduped_new_name != *new_name { - self.upsert_meta_name_field( - ¤t_meta, - new_name, - ); - } else { - self.remove_meta_name_field(¤t_meta); - } - } - } - } - } - InstigatingSource::ProjectNode { .. } => { - log::warn!( - "Cannot rename instance {:?} — defined in project file", - id - ); - } - } - } else { - log::warn!("Cannot rename instance {:?} — no instigating source", id); - } - } - - // 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 a rename handler already moved the file, - // use the new path instead of the stale - // instigating_source path. - let effective_path = - overridden_source_path.as_deref().unwrap_or(path.as_path()); - - if old_is_script && new_is_script && effective_path.exists() { - // Script-to-script transition: rename the file extension. - // For directory-format scripts, the path is the directory - // (e.g., src/MyModule/), not the init file inside. We must - // find and rename the init file, not the directory. - // Resolve the actual file to rename. For directories, - // find the init file inside; for files, use directly. - let init_result = if effective_path.is_dir() { - Self::find_init_file(effective_path) - .map(|f| (f.clone(), effective_path.to_path_buf())) - } else { - Some(( - effective_path.to_path_buf(), - effective_path - .parent() - .unwrap_or(effective_path) - .to_path_buf(), - )) - }; - - if let Some((actual_file, file_parent)) = init_result { - let new_suffix = match new_class.as_str() { - "ModuleScript" => "", - "Script" => ".server", - "LocalScript" => ".local", - _ => "", - }; - let is_init = actual_file - .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 { - // Derive the base name from the filesystem - // path, not instance.name() (which may - // differ from the slugified filename). - let stem = actual_file - .file_stem() - .and_then(|s| s.to_str()) - .unwrap_or(""); - let known_suffixes = [ - ".server", ".client", ".plugin", ".local", - ".legacy", - ]; - let base = known_suffixes - .iter() - .find_map(|s| stem.strip_suffix(s)) - .unwrap_or(stem); - if new_suffix.is_empty() { - format!("{}.luau", base) - } else { - format!("{}{}.luau", base, new_suffix) - } - }; - - let new_path = file_parent.join(&new_file_name); - if new_path != actual_file { - log::info!( - "Two-way sync: Changing class {} -> {}, \ - renaming {} -> {}", - old_class, - new_class, - actual_file.display(), - new_path.display() - ); - self.suppress_path_any(&actual_file); - self.suppress_path(&new_path); - if let Err(err) = - fs::rename(&actual_file, &new_path) - { - self.unsuppress_path_any(&actual_file); - self.unsuppress_path(&new_path); - log::error!( - "Failed to rename {:?} to {:?}: {}", - actual_file, - new_path, - err - ); - } else { - overridden_source_path = Some(new_path.clone()); - } - } - } else { - log::warn!( - "Cannot change ClassName for directory {} \ - — no init file found inside", - path.display() - ); - } - } 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() { - log::warn!("Cannot change metadata yet."); - } - - for (key, changed_value) in &update.changed_properties { - if key == "Source" { - // If a rename or ClassName change moved the file - // earlier in this update, write to the new location - // instead of the stale instigating_source path. - let source_path = if let Some(ref overridden) = overridden_source_path { - Some(overridden.clone()) - } else if let Some(instigating_source) = - &instance.metadata().instigating_source - { - match instigating_source { - InstigatingSource::Path(path) => Some(path.clone()), - InstigatingSource::ProjectNode { .. } => { - log::warn!( - "Cannot update instance {:?}, it's from a project file", - id - ); - None - } - } - } else { - log::warn!( - "Cannot update instance {:?}, it is not an instigating source.", - id - ); - None - }; - - if let Some(ref write_path) = source_path { - if let Some(Variant::String(value)) = changed_value { - log::info!( - "Two-way sync: Writing Source to {}", - write_path.display() - ); - self.suppress_path(write_path); - if let Err(err) = fs::write(write_path, value) { - self.unsuppress_path(write_path); - log::error!( - "Failed to write Source to {:?} for instance {:?}: {}", - write_path, - id, - err - ); - } - } else { - log::warn!("Cannot change Source to non-string value."); - } - } - } else { - log::trace!("Skipping non-Source property change: {}", key); - } - } - - // Record metadata update so instigating_source and - // relevant_paths stay in sync after rename / class change. - if let Some(ref new_path) = overridden_source_path { - metadata_updates.push((id, new_path.clone())); - } - } else { - log::warn!("Cannot update instance {:?}, it does not exist.", id); - } - } - - let applied = apply_patch_set(&mut tree, patch_set); - - // Update metadata for instances whose filesystem path changed. - // This keeps instigating_source and path_to_ids in sync so that - // subsequent VFS events (and future renames) target the correct - // instance and path. - for (id, new_instigating_source) in metadata_updates { - if let Some(old_metadata) = tree.get_metadata(id).cloned() { - let mut new_metadata = old_metadata; - new_metadata.instigating_source = - Some(InstigatingSource::Path(new_instigating_source.clone())); - new_metadata.relevant_paths = rebuild_relevant_paths(&new_instigating_source); - tree.update_metadata(id, new_metadata); - } - } - - applied - }; - - if !applied_patch.is_empty() { - self.message_queue.push_messages(&[applied_patch]); - } - } -} - -/// 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"); - - let instigating_source = match &metadata.instigating_source { - Some(path) => path, - None => { - log::error!( - "Instance {:?} did not have an instigating source, but was considered for an update.", - id - ); - log::error!("This is a bug. Please file an issue!"); - return None; - } - }; - - // How we process a file change event depends on what created this - // file/folder in the first place. - match instigating_source { - InstigatingSource::Path(path) => { - // For directory-format scripts, the instigating_source is the - // init file (e.g., init.luau) so that two-way sync writes to the - // correct file. However, snapshot_from_vfs returns None for init - // files because they are handled as part of the parent directory - // snapshot. Detect this case and snapshot the parent directory. - let is_init_file = path - .file_name() - .and_then(|f| f.to_str()) - .map(|f| f.starts_with("init.")) - .unwrap_or(false); - let snapshot_path = if is_init_file { - path.parent().unwrap_or(path.as_path()) - } else { - path.as_path() - }; - - log::info!( - "compute_and_apply_changes: checking path {} for instance {:?}{}", - path.display(), - id, - if is_init_file { - format!( - " (init file, will snapshot parent dir {})", - snapshot_path.display() - ) - } else { - String::new() - } - ); - - 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 {}", - snapshot_path.display() - ); - - let snapshot = match snapshot_from_vfs(&metadata.context, vfs, snapshot_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, snapshot_path) { - Ok(snapshot) => snapshot, - Err(err) => { - log::error!( - "Recovery snapshot error for {}: {:?}", - snapshot_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 if is_init_file && std::fs::metadata(snapshot_path).is_ok() { - // Init file was deleted but the parent directory still - // exists. Re-snapshot the directory — it will become a - // Folder (or whatever the directory middleware produces - // without an init file). - log::info!( - "compute_and_apply_changes: init file {} deleted but parent \ - directory {} still exists. Re-snapshotting directory.", - path.display(), - snapshot_path.display() - ); - - let snapshot = - match snapshot_from_vfs(&metadata.context, vfs, snapshot_path) { - Ok(snapshot) => snapshot, - Err(err) => { - log::error!( - "Directory re-snapshot error for {}: {:?}", - snapshot_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), - }) - } - } - Err(err) => { - log::error!( - "Error processing filesystem change for {}: {:?}", - path.display(), - err - ); - None - } - } - } - - InstigatingSource::ProjectNode { - path, - name, - node, - parent_class, - } => { - // 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, - path, - name, - node, - vfs, - parent_class.as_ref().map(|name| name.as_str()), - ); - - let snapshot = match snapshot_result { - Ok(snapshot) => snapshot, - Err(err) => { - log::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, - }) - } - } -} - -/// Rebuild the `relevant_paths` list for an instance given its new -/// `instigating_source` path. This mirrors the logic in the snapshot -/// middleware so that `path_to_ids` stays correct after a rename. -fn rebuild_relevant_paths(new_path: &Path) -> Vec { - let file_name = new_path.file_name().and_then(|f| f.to_str()).unwrap_or(""); - let is_init = file_name.starts_with("init."); - - if is_init { - // Directory-format script: relevant paths are the directory itself - // plus every possible init file variant inside it. - let dir_path = new_path.parent().unwrap().to_path_buf(); - vec![ - dir_path.clone(), - dir_path.join("init.luau"), - dir_path.join("init.server.luau"), - dir_path.join("init.client.luau"), - dir_path.join("init.plugin.luau"), - dir_path.join("init.local.luau"), - dir_path.join("init.legacy.luau"), - dir_path.join("init.csv"), - dir_path.join("init.lua"), - dir_path.join("init.server.lua"), - dir_path.join("init.client.lua"), - dir_path.join("init.meta.json5"), - ] - } else { - // Standalone file: relevant paths are the file itself and its - // adjacent meta file. - let stem = new_path.file_stem().and_then(|s| s.to_str()).unwrap_or(""); - let base_name = stem - .strip_suffix(".server") - .or_else(|| stem.strip_suffix(".client")) - .or_else(|| stem.strip_suffix(".plugin")) - .or_else(|| stem.strip_suffix(".local")) - .or_else(|| stem.strip_suffix(".legacy")) - .unwrap_or(stem); - vec![ - new_path.to_path_buf(), - new_path.with_file_name(format!("{base_name}.meta.json5")), - ] - } -} +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::fmt; +use std::path::{Path, PathBuf}; +use std::time::{Duration, Instant}; +use std::{ + fs, + sync::{Arc, Mutex}, +}; + +use crate::{ + message_queue::MessageQueue, + snapshot::{ + apply_patch_set, compute_patch_set, AppliedPatchSet, InstigatingSource, PatchSet, RojoTree, + }, + snapshot_middleware::{snapshot_from_vfs, snapshot_project_node}, + syncback::{deduplicate_name, name_needs_slugify, slugify_name, strip_script_suffix}, +}; + +/// Wrapper that displays a path relative to a project root directory. +struct RelPath<'a> { + path: &'a Path, + root: &'a Path, +} + +impl fmt::Display for RelPath<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.path + .strip_prefix(self.root) + .unwrap_or(self.path) + .display() + .fmt(f) + } +} + +/// Returns a display wrapper that shows `path` relative to `root`. +fn rel_path<'a>(path: &'a Path, root: &'a Path) -> RelPath<'a> { + RelPath { path, root } +} + +/// Processes file change events, updates the DOM, and sends those updates +/// through a channel for other stuff to consume. +/// +/// Owns the connection between Rojo's VFS and its DOM by holding onto another +/// thread that processes messages. +/// +/// Consumers of ChangeProcessor, like ServeSession, are intended to communicate +/// with this object via channels. +/// +/// ChangeProcessor expects to be the only writer to the RojoTree and Vfs +/// objects passed to it. +pub struct ChangeProcessor { + /// Controls the runtime of the processor thread. When signaled, the job + /// thread will finish its current work and terminate. + /// + /// This channel should be signaled before dropping ChangeProcessor or we'll + /// hang forever waiting for the message processing loop to terminate. + shutdown_sender: Sender<()>, + + /// A handle to the message processing thread. When dropped, we'll block + /// until it's done. + /// + /// Allowed to be unused because dropping this value has side effects. + #[allow(unused)] + job_thread: JoinHandle>, +} + +impl ChangeProcessor { + /// Spin up the ChangeProcessor, connecting it to the given tree, VFS, and + /// outbound message queue. + pub fn start( + tree: Arc>, + vfs: Arc, + message_queue: Arc>, + tree_mutation_receiver: Receiver, + suppressed_paths: Arc>>, + project_root: PathBuf, + ) -> Self { + let (shutdown_sender, shutdown_receiver) = crossbeam_channel::bounded(1); + let vfs_receiver = vfs.event_receiver(); + let task = JobThreadContext { + tree, + vfs, + message_queue, + pending_recovery: Mutex::new(Vec::new()), + suppressed_paths, + project_root, + }; + + let job_thread = jod_thread::Builder::new() + .name("ChangeProcessor thread".to_owned()) + .spawn(move || { + log::trace!("ChangeProcessor thread started"); + + loop { + 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?); + }, + recv(shutdown_receiver) -> _ => { + 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(); + }, + } + } + }) + .expect("Could not start ChangeProcessor thread"); + + Self { + shutdown_sender, + job_thread, + } + } +} + +impl Drop for ChangeProcessor { + fn drop(&mut self) { + // Signal the job thread to start spinning down. Without this we'll hang + // forever waiting for the thread to finish its infinite loop. + let _ = self.shutdown_sender.send(()); + + // After this function ends, the job thread will be joined. It might + // block for a small period of time while it processes its last work. + } +} + +/// Contains all of the state needed to synchronize the DOM and VFS. +struct JobThreadContext { + /// A handle to the DOM we're managing. + tree: Arc>, + + /// A handle to the VFS we're managing. + vfs: Arc, + + /// 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 to avoid redundant re-snapshots. Values are `(remove_count, create_write_count)`. + suppressed_paths: Arc>>, + + /// Root directory of the project, used to display relative paths in logs. + project_root: PathBuf, +} + +impl JobThreadContext { + /// Returns a display wrapper that shows `path` relative to the project root. + fn display_path<'a>(&'a self, path: &'a Path) -> RelPath<'a> { + rel_path(path, &self.project_root) + } + + /// Find the init file inside a directory-format script folder. + /// Returns the path to the first `init.*.luau` or `init.*.lua` found. + fn find_init_file(dir: &Path) -> Option { + // Check known init file names in priority order + let candidates = [ + "init.luau", + "init.server.luau", + "init.client.luau", + "init.local.luau", + "init.plugin.luau", + "init.legacy.luau", + "init.lua", + "init.server.lua", + "init.client.lua", + "init.local.lua", + ]; + for name in &candidates { + let candidate = dir.join(name); + if candidate.exists() { + return Some(candidate); + } + } + None + } + + /// Canonicalize a path for use as a suppression map key. + fn suppression_key(path: &Path) -> PathBuf { + if let Ok(canonical) = std::fs::canonicalize(path) { + canonical + } else if let Some(parent) = path.parent() { + if let Ok(canonical_parent) = std::fs::canonicalize(parent) { + if let Some(file_name) = path.file_name() { + canonical_parent.join(file_name) + } else { + path.to_path_buf() + } + } else { + path.to_path_buf() + } + } else { + path.to_path_buf() + } + } + + /// Suppress the next Create/Write VFS event for the given path. + /// Prevents re-processing a file-watcher event we triggered ourselves. + fn suppress_path(&self, path: &Path) { + let mut suppressed = self.suppressed_paths.lock().unwrap(); + let key = Self::suppression_key(path); + suppressed.entry(key).or_insert((0, 0)).1 += 1; + } + + /// Remove a Create/Write suppression previously added by [`suppress_path`]. + /// Called when the filesystem operation failed, so that future VFS events + /// for that path are not incorrectly swallowed. + fn unsuppress_path(&self, path: &Path) { + let mut suppressed = self.suppressed_paths.lock().unwrap(); + let key = Self::suppression_key(path); + if let Some(counts) = suppressed.get_mut(&key) { + counts.1 = counts.1.saturating_sub(1); + if counts.0 == 0 && counts.1 == 0 { + suppressed.remove(&key); + } + } + } + + /// Suppress the next VFS event of ANY type for the given path. + /// Used for the **old** path of renames: different platforms deliver + /// different event types for the source of a rename (REMOVE on + /// Linux/Windows, stale CREATE on macOS FSEvents). + fn suppress_path_any(&self, path: &Path) { + let mut suppressed = self.suppressed_paths.lock().unwrap(); + let key = Self::suppression_key(path); + let entry = suppressed.entry(key).or_insert((0, 0)); + entry.0 += 1; + entry.1 += 1; + } + + /// Remove both counters previously added by [`suppress_path_any`]. + fn unsuppress_path_any(&self, path: &Path) { + let mut suppressed = self.suppressed_paths.lock().unwrap(); + let key = Self::suppression_key(path); + if let Some(counts) = suppressed.get_mut(&key) { + counts.0 = counts.0.saturating_sub(1); + counts.1 = counts.1.saturating_sub(1); + if counts.0 == 0 && counts.1 == 0 { + suppressed.remove(&key); + } + } + } + + /// Suppress the next Remove VFS event for the given path. + fn suppress_path_remove(&self, path: &Path) { + let mut suppressed = self.suppressed_paths.lock().unwrap(); + let key = Self::suppression_key(path); + suppressed.entry(key).or_insert((0, 0)).0 += 1; + } + + /// Upsert the `name` field in a `.meta.json5` file, suppressing filesystem + /// events to avoid feedback loops. + fn upsert_meta_name_field(&self, meta_path: &Path, real_name: &str) { + self.suppress_path(meta_path); + if let Err(err) = crate::syncback::meta::upsert_meta_name(meta_path, real_name) { + self.unsuppress_path(meta_path); + log::error!( + "Failed to upsert name in meta file {}: {}", + self.display_path(meta_path), + err + ); + } + } + + /// Upsert the `name` field inside a `.model.json5` / `.model.json` file, + /// suppressing filesystem events. + fn upsert_model_name_field(&self, model_path: &Path, real_name: &str) { + self.suppress_path(model_path); + if let Err(err) = crate::syncback::meta::upsert_model_name(model_path, real_name) { + self.unsuppress_path(model_path); + log::error!( + "Failed to upsert name in model file {}: {}", + self.display_path(model_path), + err + ); + } + } + + /// Remove the `name` field from a `.model.json5` / `.model.json` file, + /// suppressing filesystem events. + fn remove_model_name_field(&self, model_path: &Path) { + use crate::syncback::meta::RemoveNameOutcome; + self.suppress_path(model_path); + match crate::syncback::meta::remove_model_name(model_path) { + Ok(RemoveNameOutcome::NoOp) => { + self.unsuppress_path(model_path); + } + Ok(RemoveNameOutcome::FieldRemoved) => { + // File was rewritten — suppress_path already covers it. + } + Ok(RemoveNameOutcome::FileDeleted) => { + // Model files shouldn't be deleted (they have className etc), + // but handle for completeness. + self.unsuppress_path(model_path); + self.suppress_path_remove(model_path); + } + Err(err) => { + self.unsuppress_path(model_path); + log::error!( + "Failed to remove name from model file {}: {}", + self.display_path(model_path), + err + ); + } + } + } + + /// Remove the `name` field from a `.meta.json5` file, suppressing filesystem + /// events. If the file becomes empty after removal, deletes it entirely. + fn remove_meta_name_field(&self, meta_path: &Path) { + use crate::syncback::meta::RemoveNameOutcome; + // Suppress for the write/delete that may follow + self.suppress_path(meta_path); + match crate::syncback::meta::remove_meta_name(meta_path) { + Ok(RemoveNameOutcome::NoOp) => { + self.unsuppress_path(meta_path); + } + Ok(RemoveNameOutcome::FileDeleted) => { + // File was deleted, not rewritten. Swap: undo the + // pre-emptive Write suppression and add a Remove + // suppression instead so the counts are (1, 0). + self.unsuppress_path(meta_path); + self.suppress_path_remove(meta_path); + } + Ok(RemoveNameOutcome::FieldRemoved) => { + // File was rewritten — suppress_path already covers it. + } + Err(err) => { + self.unsuppress_path(meta_path); + log::error!( + "Failed to remove name from meta file {}: {}", + self.display_path(meta_path), + err + ); + } + } + } + + /// 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 { + 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::info!( + "apply_patches: path {} affects IDs {:?}", + self.display_path(current_path), + ids + ); + + if !ids.is_empty() { + break ids.to_vec(); + } + + log::info!( + "apply_patches: no IDs at {}, trying parent...", + self.display_path(current_path) + ); + 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 {}", + self.display_path(&path) + ); + } + + for id in affected_ids { + if let Some(result) = compute_and_apply_changes(&mut tree, &self.vfs, id, &self.project_root) { + // 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); + } + } + } + + applied_patches + } + + fn handle_vfs_event(&self, event: VfsEvent) { + // 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::info!("VFS event: CREATE {}", self.display_path(path)), + VfsEvent::Write(path) => log::info!("VFS event: WRITE {}", self.display_path(path)), + VfsEvent::Remove(path) => log::info!("VFS event: REMOVE {}", self.display_path(path)), + _ => log::info!("VFS event: OTHER {:?}", event), + } + + // Check if this event should be suppressed (one-shot, from API syncback). + // Suppressions are event-type-aware: a Remove suppression only matches + // Remove events, and a Create/Write suppression only matches Create/Write + // events. This prevents a Remove suppression from consuming a Create event + // (which can happen on macOS when FSEvents coalesces delete+recreate). + 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 canonical = std::fs::canonicalize(path).ok(); + let mut suppressed = self.suppressed_paths.lock().unwrap(); + // Determine which key matches: try canonical first (most likely + // to match what suppress_path stored), then fall back to raw. + let matched_key = canonical + .as_ref() + .filter(|c| suppressed.contains_key(c.as_path())) + .cloned() + .or_else(|| { + if suppressed.contains_key(path) { + Some(path.clone()) + } else { + None + } + }); + if let Some(key) = matched_key { + if let Some(counts) = suppressed.get_mut(&key) { + let should_suppress = match &event { + VfsEvent::Remove(_) if counts.0 > 0 => { + counts.0 -= 1; + true + } + VfsEvent::Create(_) | VfsEvent::Write(_) if counts.1 > 0 => { + counts.1 -= 1; + true + } + _ => false, + }; + if counts.0 == 0 && counts.1 == 0 { + suppressed.remove(&key); + } + if should_suppress { + drop(suppressed); + // 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): {}", + self.display_path(path) + ); + return; + } + } + } + } + + // Update the VFS immediately with the event. + self.vfs + .commit_event(&event) + .expect("Error applying VFS change"); + + // 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::Write(path) => { + match self.vfs.canonicalize(&path) { + Ok(canonical_path) => { + log::info!( + "VFS: canonicalize OK for {} -> {}", + self.display_path(&path), + self.display_path(&canonical_path) + ); + self.apply_patches(canonical_path) + } + Err(_) => { + // The path doesn't exist on disk. Two possible causes: + // + // 1. Phantom event from a rename we performed — macOS FSEvents + // can deliver a stale CREATE for the old path. If a pending + // suppression exists for this path, consume it and skip. + // + // 2. The file was deleted between the event firing and us + // processing it. Fall back to parent-directory canonicalize + // (same strategy as the Remove handler) so the tree can + // reconcile the disappearance. + let consumed = { + let key = Self::suppression_key(&path); + let mut suppressed = self.suppressed_paths.lock().unwrap(); + if let Some(counts) = suppressed.get_mut(&key) { + if counts.0 > 0 || counts.1 > 0 { + // Drain whichever counter is available + if counts.1 > 0 { + counts.1 -= 1; + } else { + counts.0 -= 1; + } + if counts.0 == 0 && counts.1 == 0 { + suppressed.remove(&key); + } + true + } else { + false + } + } else { + false + } + }; + + if consumed { + log::info!( + "VFS: phantom Create/Write for non-existent {} — \ + consumed pending suppression (likely stale rename event)", + self.display_path(&path) + ); + Vec::new() + } else { + // No suppression — the file genuinely vanished. Use the + // parent-directory fallback so the tree can reconcile. + let parent = path.parent().unwrap(); + let file_name = path.file_name().unwrap(); + match self.vfs.canonicalize(parent) { + Ok(parent_normalized) => { + let resolved = parent_normalized.join(file_name); + log::info!( + "VFS: Create/Write for vanished file {} — \ + resolved via parent to {}", + self.display_path(&path), + self.display_path(&resolved) + ); + self.apply_patches(resolved) + } + Err(err) => { + log::info!( + "VFS: Skipping Create/Write for {} — \ + parent no longer exists: {}", + self.display_path(&path), + err + ); + Vec::new() + } + } + } + } + } + } + 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. + // However, if the parent was also deleted (e.g., when deleting a directory + // tree), canonicalize will fail - in that case, just skip this event. + let parent = path.parent().unwrap(); + let file_name = path.file_name().unwrap(); + match self.vfs.canonicalize(parent) { + Ok(parent_normalized) => { + let resolved = parent_normalized.join(file_name); + log::info!("VFS: Remove resolved to {}", self.display_path(&resolved)); + self.apply_patches(resolved) + } + Err(err) => { + log::info!( + "VFS: Skipping remove event for {} — parent no longer exists: {}", + self.display_path(&path), + err + ); + Vec::new() + } + } + } + _ => { + log::warn!("Unhandled VFS event: {:?}", event); + Vec::new() + } + }; + + // 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::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 + // just made. + 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.", + self.display_path(&path) + ); + 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.", + self.display_path(&path) + ); + } + } + } + + fn handle_tree_event(&self, patch_set: PatchSet) { + // Log incoming patch summary at debug level + log::debug!( + "Processing client patch: {} removed, {} added, {} updated", + patch_set.removed_instances.len(), + patch_set.added_instances.len(), + patch_set.updated_instances.len() + ); + + let applied_patch = { + let mut tree = self.tree.lock().unwrap(); + + // NOTE: We do NOT delete files from disk here. The API handler + // (handle_api_write → remove_instance_at_path) already deleted + // the files before sending this PatchSet. Re-deleting would + // destroy any new file created at the same path — e.g., when an + // instance is renamed from "joe_test" to "joe/test", the new slug + // is also "joe_test", so the API creates a new file at the same + // path that the removal just deleted. If we deleted again here, + // the new file would be destroyed. + // + // The PatchSet removal only needs to update the DOM tree (handled + // by apply_patch_set below). + for &id in &patch_set.removed_instances { + if let Some(instance) = tree.get_instance(id) { + if let Some(instigating_source) = &instance.metadata().instigating_source { + match instigating_source { + InstigatingSource::Path(path) => { + log::info!( + "Two-way sync: Removing instance {:?} from tree (path: {})", + id, + self.display_path(path) + ); + } + InstigatingSource::ProjectNode { .. } => { + log::warn!( + "Cannot remove instance {:?}, it's from a project file", + id + ); + } + } + } else { + log::warn!( + "Cannot remove instance {:?}, it is not an instigating source.", + id + ); + } + } else { + log::warn!("Cannot remove instance {:?}, it does not exist.", id); + } + } + + // Collect (Ref, new_instigating_source) for instances whose + // filesystem path changed (rename / ClassName transition). + // Applied after the PatchSet to keep metadata in sync. + let mut metadata_updates: Vec<(Ref, PathBuf)> = Vec::new(); + + for update in &patch_set.updated_instances { + let id = update.id; + + if let Some(instance) = tree.get_instance(id) { + // Track the current source file path — rename and ClassName + // handlers may move the file, so the Source write must target + // the new location instead of the stale instigating_source. + let mut overridden_source_path: Option = None; + + // 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 file_name = + path.file_name().and_then(|f| f.to_str()).unwrap_or(""); + + // For directory-format scripts, the instigating_source + // is the init file (e.g., src/MyModule/init.luau). + // The instance name corresponds to the parent directory, + // not the init file, so we must rename the directory. + let is_init_file = file_name.starts_with("init."); + + if is_init_file { + let dir_path = path.parent().unwrap(); + if let Some(grandparent) = dir_path.parent() { + let dir_name = dir_path + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(""); + // Slugify the new name for filesystem safety + let slugified_new_name = + if name_needs_slugify(new_name) { + slugify_name(new_name) + } else { + new_name.to_string() + }; + // Directory renames: fs::rename fails safely + // on all platforms if the target is a non-empty + // directory, so no dedup is needed here (unlike + // the file rename path below, where Unix + // fs::rename silently overwrites). + let new_dir_path = + grandparent.join(&slugified_new_name); + + // Track the effective directory path after any rename + let effective_dir_path; + + if new_dir_path != dir_path { + log::info!( + "Two-way sync: Renaming directory {} -> {}", + self.display_path(dir_path), + self.display_path(&new_dir_path) + ); + self.suppress_path_any(dir_path); + self.suppress_path(&new_dir_path); + if let Err(err) = + fs::rename(dir_path, &new_dir_path) + { + self.unsuppress_path_any(dir_path); + self.unsuppress_path(&new_dir_path); + log::error!( + "Failed to rename directory {:?} to {:?}: {}", + dir_path, + new_dir_path, + err + ); + effective_dir_path = dir_path.to_path_buf(); + } else { + // The init file moved with the directory. + overridden_source_path = + Some(new_dir_path.join(file_name)); + let old_meta = grandparent.join(format!( + "{}.meta.json5", + dir_name + )); + if old_meta.exists() { + let new_meta = + grandparent.join(format!( + "{}.meta.json5", + slugified_new_name + )); + self.suppress_path_any(&old_meta); + self.suppress_path(&new_meta); + if fs::rename(&old_meta, &new_meta) + .is_err() + { + self.unsuppress_path_any(&old_meta); + self.unsuppress_path(&new_meta); + } + } + effective_dir_path = new_dir_path.clone(); + } + } else { + effective_dir_path = dir_path.to_path_buf(); + } + + // Always update the meta name field when the + // instance name changed, even if the directory + // path didn't change (e.g. "Foo/Bar" → "Foo|Bar" + // both slugify to "Foo_Bar"). + let init_meta = + effective_dir_path.join("init.meta.json5"); + if slugified_new_name != *new_name { + self.upsert_meta_name_field( + &init_meta, new_name, + ); + } else { + self.remove_meta_name_field(&init_meta); + } + } + } else if let Some(parent) = path.parent() { + // Derive the suffix from the existing filename + // rather than using replacen with the instance + // name, which may differ from the filesystem + // name (e.g., slugified names). + let extension = path + .extension() + .and_then(|e| e.to_str()) + .unwrap_or(""); + let stem = path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or(""); + let known_suffixes = [ + ".server", ".client", ".plugin", ".local", + ".legacy", ".model", + ]; + let script_suffix = known_suffixes + .iter() + .find(|s| stem.ends_with(*s)) + .copied() + .unwrap_or(""); + let old_base = if script_suffix.is_empty() { + stem + } else { + &stem[..stem.len() - script_suffix.len()] + }; + + let slugified_new_name = if name_needs_slugify(new_name) + { + slugify_name(new_name) + } else { + new_name.to_string() + }; + + // Guard against rename collision: if the target + // path already exists and isn't our own file, + // deduplicate the name against siblings. + let deduped_new_name; + { + let candidate_file = if extension.is_empty() { + format!( + "{}{}", + slugified_new_name, script_suffix + ) + } else { + format!( + "{}{}.{}", + slugified_new_name, + script_suffix, + extension + ) + }; + let candidate = parent.join(&candidate_file); + if candidate != *path && candidate.exists() { + let mut taken: std::collections::HashSet< + String, + > = std::collections::HashSet::new(); + if let Ok(entries) = fs::read_dir(parent) { + for entry in entries.flatten() { + let ep = entry.path(); + let slug = if ep.is_dir() { + ep.file_name() + .and_then(|f| f.to_str()) + .unwrap_or("") + .to_lowercase() + } else { + let s = ep + .file_stem() + .and_then(|f| f.to_str()) + .unwrap_or(""); + strip_script_suffix(s) + .to_lowercase() + }; + taken.insert(slug); + } + } + // Free the slot we're vacating + taken.remove(&old_base.to_lowercase()); + deduped_new_name = deduplicate_name( + &slugified_new_name, + &taken, + ); + } else { + deduped_new_name = slugified_new_name.clone(); + } + } + + let new_file_name = if extension.is_empty() { + format!("{}{}", deduped_new_name, script_suffix) + } else { + format!( + "{}{}.{}", + deduped_new_name, script_suffix, extension + ) + }; + let new_path = parent.join(&new_file_name); + + // Track the effective meta base name after any rename + let effective_meta_base: &str; + + if new_path != *path { + log::info!( + "Two-way sync: Renaming {} -> {}", + self.display_path(path), + self.display_path(&new_path) + ); + self.suppress_path_any(path); + self.suppress_path(&new_path); + if let Err(err) = fs::rename(path, &new_path) { + self.unsuppress_path_any(path); + self.unsuppress_path(&new_path); + log::error!( + "Failed to rename {:?} to {:?}: {}", + path, + new_path, + err + ); + effective_meta_base = old_base; + } else { + overridden_source_path = Some(new_path.clone()); + let old_meta = parent + .join(format!("{}.meta.json5", old_base)); + let new_meta = parent.join(format!( + "{}.meta.json5", + deduped_new_name + )); + if old_meta.exists() { + self.suppress_path_any(&old_meta); + self.suppress_path(&new_meta); + if fs::rename(&old_meta, &new_meta).is_err() + { + self.unsuppress_path_any(&old_meta); + self.unsuppress_path(&new_meta); + } + } + effective_meta_base = &deduped_new_name; + } + } else { + effective_meta_base = old_base; + } + + // Always update the name field when the + // instance name changed, even if the path didn't + // change (e.g. "Foo/Bar" → "Foo|Bar" both slugify + // to "Foo_Bar"). Use deduped_new_name because + // dedup may have appended ~N. + // + // For .model.json5/.model.json files, the name + // field lives INSIDE the model file, not in + // adjacent .meta.json5. + if script_suffix == ".model" { + let model_file = overridden_source_path + .as_deref() + .unwrap_or(path.as_path()); + if deduped_new_name != *new_name { + self.upsert_model_name_field( + model_file, new_name, + ); + } else { + self.remove_model_name_field(model_file); + } + } else { + let current_meta = parent.join(format!( + "{}.meta.json5", + effective_meta_base + )); + if deduped_new_name != *new_name { + self.upsert_meta_name_field( + ¤t_meta, + new_name, + ); + } else { + self.remove_meta_name_field(¤t_meta); + } + } + } + } + } + InstigatingSource::ProjectNode { .. } => { + log::warn!( + "Cannot rename instance {:?} — defined in project file", + id + ); + } + } + } else { + log::warn!("Cannot rename instance {:?} — no instigating source", id); + } + } + + // 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 a rename handler already moved the file, + // use the new path instead of the stale + // instigating_source path. + let effective_path = + overridden_source_path.as_deref().unwrap_or(path.as_path()); + + if old_is_script && new_is_script && effective_path.exists() { + // Script-to-script transition: rename the file extension. + // For directory-format scripts, the path is the directory + // (e.g., src/MyModule/), not the init file inside. We must + // find and rename the init file, not the directory. + // Resolve the actual file to rename. For directories, + // find the init file inside; for files, use directly. + let init_result = if effective_path.is_dir() { + Self::find_init_file(effective_path) + .map(|f| (f.clone(), effective_path.to_path_buf())) + } else { + Some(( + effective_path.to_path_buf(), + effective_path + .parent() + .unwrap_or(effective_path) + .to_path_buf(), + )) + }; + + if let Some((actual_file, file_parent)) = init_result { + let new_suffix = match new_class.as_str() { + "ModuleScript" => "", + "Script" => ".server", + "LocalScript" => ".local", + _ => "", + }; + let is_init = actual_file + .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 { + // Derive the base name from the filesystem + // path, not instance.name() (which may + // differ from the slugified filename). + let stem = actual_file + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or(""); + let known_suffixes = [ + ".server", ".client", ".plugin", ".local", + ".legacy", + ]; + let base = known_suffixes + .iter() + .find_map(|s| stem.strip_suffix(s)) + .unwrap_or(stem); + if new_suffix.is_empty() { + format!("{}.luau", base) + } else { + format!("{}{}.luau", base, new_suffix) + } + }; + + let new_path = file_parent.join(&new_file_name); + if new_path != actual_file { + log::info!( + "Two-way sync: Changing class {} -> {}, \ + renaming {} -> {}", + old_class, + new_class, + self.display_path(&actual_file), + self.display_path(&new_path) + ); + self.suppress_path_any(&actual_file); + self.suppress_path(&new_path); + if let Err(err) = + fs::rename(&actual_file, &new_path) + { + self.unsuppress_path_any(&actual_file); + self.unsuppress_path(&new_path); + log::error!( + "Failed to rename {:?} to {:?}: {}", + actual_file, + new_path, + err + ); + } else { + overridden_source_path = Some(new_path.clone()); + } + } + } else { + log::warn!( + "Cannot change ClassName for directory {} \ + — no init file found inside", + self.display_path(path) + ); + } + } 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() { + log::warn!("Cannot change metadata yet."); + } + + for (key, changed_value) in &update.changed_properties { + if key == "Source" { + // If a rename or ClassName change moved the file + // earlier in this update, write to the new location + // instead of the stale instigating_source path. + let source_path = if let Some(ref overridden) = overridden_source_path { + Some(overridden.clone()) + } else if let Some(instigating_source) = + &instance.metadata().instigating_source + { + match instigating_source { + InstigatingSource::Path(path) => Some(path.clone()), + InstigatingSource::ProjectNode { .. } => { + log::warn!( + "Cannot update instance {:?}, it's from a project file", + id + ); + None + } + } + } else { + log::warn!( + "Cannot update instance {:?}, it is not an instigating source.", + id + ); + None + }; + + if let Some(ref write_path) = source_path { + if let Some(Variant::String(value)) = changed_value { + log::info!( + "Two-way sync: Writing Source to {}", + self.display_path(write_path) + ); + self.suppress_path(write_path); + if let Err(err) = fs::write(write_path, value) { + self.unsuppress_path(write_path); + log::error!( + "Failed to write Source to {:?} for instance {:?}: {}", + write_path, + id, + err + ); + } + } else { + log::warn!("Cannot change Source to non-string value."); + } + } + } else { + log::trace!("Skipping non-Source property change: {}", key); + } + } + + // Record metadata update so instigating_source and + // relevant_paths stay in sync after rename / class change. + if let Some(ref new_path) = overridden_source_path { + metadata_updates.push((id, new_path.clone())); + } + } else { + log::warn!("Cannot update instance {:?}, it does not exist.", id); + } + } + + let applied = apply_patch_set(&mut tree, patch_set); + + // Update metadata for instances whose filesystem path changed. + // This keeps instigating_source and path_to_ids in sync so that + // subsequent VFS events (and future renames) target the correct + // instance and path. + for (id, new_instigating_source) in metadata_updates { + if let Some(old_metadata) = tree.get_metadata(id).cloned() { + let mut new_metadata = old_metadata; + new_metadata.instigating_source = + Some(InstigatingSource::Path(new_instigating_source.clone())); + new_metadata.relevant_paths = rebuild_relevant_paths(&new_instigating_source); + tree.update_metadata(id, new_metadata); + } + } + + applied + }; + + if !applied_patch.is_empty() { + self.message_queue.push_messages(&[applied_patch]); + } + } +} + +/// 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, + project_root: &Path, +) -> Option { + // Use rel_path(p, project_root) inline for log display. + + let metadata = tree + .get_metadata(id) + .expect("metadata missing for instance present in tree"); + + let instigating_source = match &metadata.instigating_source { + Some(path) => path, + None => { + log::error!( + "Instance {:?} did not have an instigating source, but was considered for an update.", + id + ); + log::error!("This is a bug. Please file an issue!"); + return None; + } + }; + + // How we process a file change event depends on what created this + // file/folder in the first place. + match instigating_source { + InstigatingSource::Path(path) => { + // For directory-format scripts, the instigating_source is the + // init file (e.g., init.luau) so that two-way sync writes to the + // correct file. However, snapshot_from_vfs returns None for init + // files because they are handled as part of the parent directory + // snapshot. Detect this case and snapshot the parent directory. + let is_init_file = path + .file_name() + .and_then(|f| f.to_str()) + .map(|f| f.starts_with("init.")) + .unwrap_or(false); + let snapshot_path = if is_init_file { + path.parent().unwrap_or(path.as_path()) + } else { + path.as_path() + }; + + log::info!( + "compute_and_apply_changes: checking path {} for instance {:?}{}", + rel_path(path, project_root), + id, + if is_init_file { + format!( + " (init file, will snapshot parent dir {})", + rel_path(snapshot_path, project_root) + ) + } else { + String::new() + } + ); + + 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 {}", + rel_path(snapshot_path, project_root) + ); + + let snapshot = match snapshot_from_vfs(&metadata.context, vfs, snapshot_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.", + rel_path(path, project_root) + ); + + let snapshot = + match snapshot_from_vfs(&metadata.context, vfs, snapshot_path) { + Ok(snapshot) => snapshot, + Err(err) => { + log::error!( + "Recovery snapshot error for {}: {:?}", + rel_path(snapshot_path, project_root), + 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 if is_init_file && std::fs::metadata(snapshot_path).is_ok() { + // Init file was deleted but the parent directory still + // exists. Re-snapshot the directory — it will become a + // Folder (or whatever the directory middleware produces + // without an init file). + log::info!( + "compute_and_apply_changes: init file {} deleted but parent \ + directory {} still exists. Re-snapshotting directory.", + rel_path(path, project_root), + rel_path(snapshot_path, project_root) + ); + + let snapshot = + match snapshot_from_vfs(&metadata.context, vfs, snapshot_path) { + Ok(snapshot) => snapshot, + Err(err) => { + log::error!( + "Directory re-snapshot error for {}: {:?}", + rel_path(snapshot_path, project_root), + 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, + rel_path(path, project_root) + ); + + 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), + }) + } + } + Err(err) => { + log::error!( + "Error processing filesystem change for {}: {:?}", + rel_path(path, project_root), + err + ); + None + } + } + } + + InstigatingSource::ProjectNode { + path, + name, + node, + parent_class, + } => { + // 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, + rel_path(path, project_root) + ); + + let snapshot_result = snapshot_project_node( + &metadata.context, + path, + name, + node, + vfs, + parent_class.as_ref().map(|name| name.as_str()), + ); + + let snapshot = match snapshot_result { + Ok(snapshot) => snapshot, + Err(err) => { + log::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, + }) + } + } +} + +/// Rebuild the `relevant_paths` list for an instance given its new +/// `instigating_source` path. This mirrors the logic in the snapshot +/// middleware so that `path_to_ids` stays correct after a rename. +fn rebuild_relevant_paths(new_path: &Path) -> Vec { + let file_name = new_path.file_name().and_then(|f| f.to_str()).unwrap_or(""); + let is_init = file_name.starts_with("init."); + + if is_init { + // Directory-format script: relevant paths are the directory itself + // plus every possible init file variant inside it. + let dir_path = new_path.parent().unwrap().to_path_buf(); + vec![ + dir_path.clone(), + dir_path.join("init.luau"), + dir_path.join("init.server.luau"), + dir_path.join("init.client.luau"), + dir_path.join("init.plugin.luau"), + dir_path.join("init.local.luau"), + dir_path.join("init.legacy.luau"), + dir_path.join("init.csv"), + dir_path.join("init.lua"), + dir_path.join("init.server.lua"), + dir_path.join("init.client.lua"), + dir_path.join("init.meta.json5"), + ] + } else { + // Standalone file: relevant paths are the file itself and its + // adjacent meta file. + let stem = new_path.file_stem().and_then(|s| s.to_str()).unwrap_or(""); + let base_name = stem + .strip_suffix(".server") + .or_else(|| stem.strip_suffix(".client")) + .or_else(|| stem.strip_suffix(".plugin")) + .or_else(|| stem.strip_suffix(".local")) + .or_else(|| stem.strip_suffix(".legacy")) + .unwrap_or(stem); + vec![ + new_path.to_path_buf(), + new_path.with_file_name(format!("{base_name}.meta.json5")), + ] + } +} diff --git a/src/serve_session.rs b/src/serve_session.rs index 12d7cc6e..1ea3bec7 100644 --- a/src/serve_session.rs +++ b/src/serve_session.rs @@ -23,6 +23,9 @@ use crate::{ snapshot_middleware::snapshot_from_vfs, }; +/// Set to `true` to validate on plugin connect (useful for testing, do not enable on production). +const VALIDATE_TREE_ON_CONNECT: bool = false; + /// Contains all of the state for a Rojo serve session. A serve session is used /// when we need to build a Rojo tree and possibly rebuild it when input files /// change. @@ -142,6 +145,7 @@ impl ServeSession { Arc::clone(&message_queue), tree_mutation_receiver, Arc::clone(&suppressed_paths), + root_project.folder_location().to_path_buf(), ); Ok(Self { @@ -259,6 +263,67 @@ impl ServeSession { .map(|rules| rules.ignore_hidden_services()) .unwrap_or(true) } + + /// Re-snapshots the project tree from the real filesystem and patches + /// the in-memory tree to correct any drift caused by missed VFS watcher + /// events. Called on plugin connect to guarantee the tree is fresh. + /// + /// Skipped on freshly-started sessions (< 5 s) where the tree is + /// guaranteed correct and the VFS watcher should handle all changes. + /// + /// Controlled by [`VALIDATE_TREE_ON_CONNECT`]; set that constant to + /// `false` to disable this entirely during testing. + pub fn validate_tree(&self) -> Vec { + if !VALIDATE_TREE_ON_CONNECT { + log::debug!("Tree validation skipped (VALIDATE_TREE_ON_CONNECT = false)"); + return Vec::new(); + } + + // On a freshly-started session the tree was just built from the + // filesystem and cannot be stale. Skip validation to avoid racing + // with the VFS watcher on early file changes. + const MIN_SESSION_AGE: std::time::Duration = std::time::Duration::from_secs(5); + if self.start_time.elapsed() < MIN_SESSION_AGE { + log::debug!( + "Tree validation skipped (session age {:.1?} < {:.1?})", + self.start_time.elapsed(), + MIN_SESSION_AGE + ); + return Vec::new(); + } + + let start = Instant::now(); + let start_path = self.root_project.folder_location(); + let instance_context = InstanceContext::new(); + + let snapshot = match snapshot_from_vfs(&instance_context, &self.vfs, start_path) { + Ok(s) => s, + Err(e) => { + log::error!("Tree validation snapshot error: {:?}", e); + return Vec::new(); + } + }; + + let mut tree = self.tree.lock().unwrap(); + let root_id = tree.get_root_id(); + let patch_set = compute_patch_set(snapshot, &tree, root_id); + + if patch_set.removed_instances.is_empty() + && patch_set.added_instances.is_empty() + && patch_set.updated_instances.is_empty() + { + log::info!( + "Tree validation complete (no corrections needed) in {:.1?}", + start.elapsed() + ); + return Vec::new(); + } + + log::info!("Tree validation found stale state, applying corrections"); + let applied = apply_patch_set(&mut tree, patch_set); + log::info!("Tree validation complete in {:.1?}", start.elapsed()); + vec![applied] + } } #[derive(Debug, Error)] diff --git a/src/web/api.rs b/src/web/api.rs index 033cf723..dec90d28 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -3063,6 +3063,14 @@ async fn handle_websocket_subscription( session_id ); + // Validate the in-memory tree against the real filesystem on every + // plugin connect. This corrects any drift caused by missed VFS watcher + // events so the plugin always sees the true state. + let corrections = serve_session.validate_tree(); + if !corrections.is_empty() { + message_queue.push_messages(&corrections); + } + // Now continuously listen for new messages using select to handle both incoming messages // and WebSocket control messages concurrently let mut cursor = input_cursor; diff --git a/vscode-rojo b/vscode-rojo index a05977d6..f18e6d4c 160000 --- a/vscode-rojo +++ b/vscode-rojo @@ -1 +1 @@ -Subproject commit a05977d6b8107c1ddc5dca7d4d26819ddd6582ba +Subproject commit f18e6d4c523994e0eda7bf18f4947a724717e59d