From 4985a3fedefa6b81ad9ca00b456d833ecf83a9a5 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 06:11:16 -0600 Subject: [PATCH 01/13] work --- .../script_index_pruning_999b5d04.plan.md | 371 ++++++++++++++++++ TODO.txt | 5 + plugin/src/ChangeBatcher/init.lua | 9 + plugin/src/ServeSession.lua | 82 ++++ .../default.project.json5 | 11 + .../src/DeepNest/Middle/DeepScript.luau | 1 + .../src/MixedFolder/SomeModel.model.json5 | 4 + .../src/MixedFolder/SomeScript.luau | 1 + .../src/PureParts/PartA.model.json5 | 4 + .../src/PureParts/PartB.model.json5 | 4 + .../src/TopScript.server.luau | 1 + src/snapshot/patch_apply.rs | 12 + src/snapshot/tree.rs | 34 +- src/web/api.rs | 130 +++--- src/web/interface.rs | 4 + tests/tests/mod.rs | 1 + tests/tests/scripts_only_pruning.rs | 280 +++++++++++++ 17 files changed, 891 insertions(+), 63 deletions(-) create mode 100644 .cursor/plans/script_index_pruning_999b5d04.plan.md create mode 100644 rojo-test/serve-tests/scripts_only_read_pruning/default.project.json5 create mode 100644 rojo-test/serve-tests/scripts_only_read_pruning/src/DeepNest/Middle/DeepScript.luau create mode 100644 rojo-test/serve-tests/scripts_only_read_pruning/src/MixedFolder/SomeModel.model.json5 create mode 100644 rojo-test/serve-tests/scripts_only_read_pruning/src/MixedFolder/SomeScript.luau create mode 100644 rojo-test/serve-tests/scripts_only_read_pruning/src/PureParts/PartA.model.json5 create mode 100644 rojo-test/serve-tests/scripts_only_read_pruning/src/PureParts/PartB.model.json5 create mode 100644 rojo-test/serve-tests/scripts_only_read_pruning/src/TopScript.server.luau create mode 100644 tests/tests/scripts_only_pruning.rs diff --git a/.cursor/plans/script_index_pruning_999b5d04.plan.md b/.cursor/plans/script_index_pruning_999b5d04.plan.md new file mode 100644 index 00000000..51b099cb --- /dev/null +++ b/.cursor/plans/script_index_pruning_999b5d04.plan.md @@ -0,0 +1,371 @@ +--- +name: Script index pruning +overview: "Scripts-only mode: add script index to RojoTree for fast server-side pruning, send syncScriptsOnly to plugin so it skips non-script two-way sync work, and add server-side defense filtering on write. Comprehensive test fixtures for all paths." +todos: + - id: script-index + content: "Add `script_refs: HashSet` to `RojoTree` and maintain it in `insert_instance()`, `remove()`, `new()`" + status: completed + - id: class-change + content: Handle ClassName changes in `apply_update_child()` with `update_script_tracking()` + status: completed + - id: optimize-read + content: Rewrite `handle_api_read` scripts-only path to use script index + ancestor walk + status: completed + - id: server-info + content: Add `syncScriptsOnly` to ServerInfoResponse and populate from project config + status: completed + - id: ws-inject-ancestors + content: Fix filter_subscribe_message_for_scripts to inject missing ancestor instances when a new script appears in a previously-pruned subtree + status: completed + - id: plugin-filter + content: "Plugin: skip non-script instances in ChangeBatcher when syncScriptsOnly is true" + status: completed + - id: plugin-descendant-added + content: "Plugin: connect DescendantAdded on synced services to detect new scripts created in Studio outside the mapped tree" + status: completed + - id: server-write-filter + content: "Server: filter non-script updates in handle_api_write when scripts-only mode" + status: completed + - id: tests + content: "Add test fixtures: read-API pruning, WebSocket ancestor injection, write-API filtering, new-script-in-pruned-subtree" + status: completed + - id: cleanup + content: Remove now-unused `collect_scripts_and_ancestors` helper, share `is_script_class` + status: completed +isProject: false +--- + +# Scripts-Only Mode: Full Pruning (Server + Plugin) + +## Problem + +When `syncScriptsOnly` is enabled: + +1. **Server read path**: `handle_api_read` walks the entire descendant tree (O(n)) just to find scripts -- wasteful for large places with 100K+ non-script instances. +2. **Plugin two-way sync**: Plugin has no knowledge of scripts-only mode. It batches and sends property changes for non-script ancestor containers back to the server, which processes them unnecessarily. +3. **Server write path**: `handle_api_write` processes all incoming updates regardless of scripts-only mode -- no defense-in-depth filtering. + +## Solution + +Three-pronged approach: + +- **Server**: Script index on `RojoTree` for O(scripts * depth) reads, filter writes +- **Plugin**: Receive `syncScriptsOnly` flag, skip non-script instances in `ChangeBatcher` +- **Tests**: Fixtures verifying pruning in read, WebSocket, write, and plugin paths + +--- + +## Part 1: Server-Side Script Index + +### 1a. Add script index to `RojoTree` ([src/snapshot/tree.rs](src/snapshot/tree.rs)) + +- Add `script_refs: HashSet` field to `RojoTree` (alongside `path_to_ids`, `specified_id_to_refs`) +- Initialize in `RojoTree::new()` +- `**insert_instance()`**: after inserting, if `is_script_class(class_name)` then `self.script_refs.insert(referent)` (recursive children handled by the recursive call) +- `**remove()`**: `self.script_refs.remove(&id)` in the existing descendant cleanup loop +- Add `pub fn script_refs(&self) -> &HashSet` accessor +- Add `pub(crate) fn update_script_tracking(&mut self, id: Ref, old_class: &str, new_class: &str)` for ClassName changes +- Add `pub(crate) fn is_script_class(class_name: &str) -> bool` free function (shared by `tree.rs` and `api.rs`) + +### 1b. Handle ClassName changes ([src/snapshot/patch_apply.rs](src/snapshot/patch_apply.rs)) + +In `apply_update_child()` (line 270), when `changed_class_name` is present: + +- Capture old class name before the mutable borrow begins +- After mutable borrow is dropped, call `tree.update_script_tracking(id, &old_class, new_class.as_str())` + +### 1c. Optimize `handle_api_read` ([src/web/api.rs](src/web/api.rs) lines 3103-3138) + +Replace the two-pass full-tree walk with script-index-based ancestor walk: + +```rust +let all_scripts = tree.script_refs(); +let mut included_ids: HashSet = HashSet::new(); +let requested_set: HashSet = requested_ids.iter().copied().collect(); + +for &script_id in all_scripts { + let mut chain = Vec::new(); + let mut current = script_id; + let mut is_descendant = false; + + while let Some(inst) = tree.get_instance(current) { + if included_ids.contains(¤t) { + is_descendant = true; + break; + } + if requested_set.contains(¤t) { + is_descendant = true; + chain.push(current); + break; + } + chain.push(current); + let parent = inst.parent(); + if parent.is_none() { break; } + current = parent; + } + + if is_descendant { + included_ids.extend(chain); + } +} + +for &id in &included_ids { + if let Some(instance) = tree.get_instance(id) { + instances.insert(id, instance_for_scripts_only(instance, &included_ids)); + } +} +``` + +### 1d. Fix WebSocket ancestor injection ([src/web/api.rs](src/web/api.rs) `filter_subscribe_message_for_scripts`) + +**Bug**: When a new script appears on disk inside a previously-pruned subtree, the WebSocket patch contains only the new script in `msg.added`. The function walks up the parent chain and adds ancestors to `included_ids`, but line 3574 only **retains** from `msg.added`. Ancestors that already exist in the tree (but are NOT in the plugin's InstanceMap because they were pruned) are never sent. The plugin receives an orphaned script with an unknown parent. + +**Fix**: After building `included_ids`, inject missing ancestors from the tree into `msg.added`: + +```rust +// After: msg.added.retain(|id, _| included_ids.contains(id)); + +// Inject ancestors that the plugin may not have (they exist in the tree +// but weren't part of this patch). Without this, a new script in a +// previously-pruned subtree arrives with an unknown parent. +for &id in &included_ids { + if !msg.added.contains_key(&id) { + if let Some(tree_inst) = tree.get_instance(id) { + let inst = instance_for_scripts_only(tree_inst, &included_ids); + msg.added.insert(id, inst); + } + } +} +``` + +The Reconciler handles redundant ancestors gracefully (matches them to existing Studio instances via the matching algorithm), so injecting an ancestor the plugin already has is harmless. + +### 1e. Filter writes in `handle_api_write` ([src/web/api.rs](src/web/api.rs) line 447) + +When `sync_scripts_only()` is true, filter incoming `WriteRequest`: + +- `**updated**`: Retain only updates where `tree.get_instance(id)` is a script class +- `**added**`: Allow all (scripts + children of scripts are valid additions) +- `**removed**`: Allow all (plugin should only send script removals, but let server handle gracefully) + +Defense-in-depth -- the plugin should already filter, but the server shouldn't blindly trust client data. + +--- + +## Part 2: Server-to-Plugin Communication + +### 2a. Add `syncScriptsOnly` to ServerInfoResponse ([src/web/interface.rs](src/web/interface.rs) line 260) + +Add new field after `sync_source_only`: + +```rust +#[serde(default)] +pub sync_scripts_only: bool, +``` + +### 2b. Populate from project config ([src/web/api.rs](src/web/api.rs) line 395) + +In `handle_api_rojo()`, add: + +```rust +sync_scripts_only: self.serve_session.sync_scripts_only(), +``` + +The accessor already exists at `serve_session.rs` line 501. + +--- + +## Part 3: Plugin-Side Filtering + +### 3a. Store flag in ServeSession ([plugin/src/ServeSession.lua](plugin/src/ServeSession.lua)) + +In `__computeInitialPatch` where `serverInfo` is received (line 619): + +```lua +self.__syncScriptsOnly = serverInfo.syncScriptsOnly or false +self.__changeBatcher:setSyncScriptsOnly(self.__syncScriptsOnly) +``` + +### 3b. Filter in ChangeBatcher ([plugin/src/ChangeBatcher/init.lua](plugin/src/ChangeBatcher/init.lua)) + +Add `__syncScriptsOnly` field. In `add()` (line 120), early return for non-script instances: + +```lua +function ChangeBatcher:add(instance, propertyName) + if self.__syncScriptsOnly and not instance:IsA("LuaSourceContainer") then + return + end + -- ... existing code ... +end +``` + +Using `IsA("LuaSourceContainer")` is the idiomatic Roblox way to check Script/LocalScript/ModuleScript (it catches all three and any future subclasses). + +Add `setSyncScriptsOnly()` method: + +```lua +function ChangeBatcher:setSyncScriptsOnly(value) + self.__syncScriptsOnly = value +end +``` + +### 3c. Detect new scripts created in Studio ([plugin/src/ServeSession.lua](plugin/src/ServeSession.lua)) + +**Problem**: The plugin only connects `Changed` signals on instances in its InstanceMap. If a user creates a new Script inside a container that was pruned (no scripts), the plugin has zero signals on that container and the new script goes undetected. The plugin doesn't use `DescendantAdded` anywhere today. + +**Fix**: When `syncScriptsOnly` is true, connect `DescendantAdded` on each synced service root. When a new `LuaSourceContainer` appears outside the mapped tree, encode it and its unmapped ancestor chain as additions and send via the write API. + +**Key insight**: `syncback_added_instance` ([src/web/api.rs](src/web/api.rs) line 1097) already has **upsert logic** -- it checks `find_child_by_name` and updates in place if the instance already exists in the tree (returns `Ok(true)` to skip PatchAdd). This means the plugin can send intermediate containers as "additions" and the server won't create duplicates. + +**Plugin implementation** (in `ServeSession.lua`, after initial sync): + +```lua +if self.__syncScriptsOnly then + for id, instance in self.__instanceMap.fromIds do + if instance.Parent == game then + local conn = instance.DescendantAdded:Connect(function(descendant) + if not self.__twoWaySync then return end + if not descendant:IsA("LuaSourceContainer") then return end + if self.__instanceMap.fromInstances[descendant] then return end + + local chain = {} -- unmapped ancestors, ordered parent-first + local current = descendant.Parent + while current and current ~= game do + if self.__instanceMap.fromInstances[current] then + break + end + table.insert(chain, 1, current) + current = current.Parent + end + + local mappedParent = current + local mappedParentId = self.__instanceMap.fromInstances[mappedParent] + if not mappedParentId then return end + + local patch = PatchSet.newEmpty() + local prevParentId = mappedParentId + + -- Intermediate containers: skeleton only (className, name, parent). + -- No properties -- the server already has these instances on disk. + -- Sending properties would trigger syncback_update_existing_instance + -- which writes to disk, risking round-trip identity violations. + for _, intermediate in chain do + local tempId = HttpService:GenerateGUID() + patch.added[tempId] = { + parent = prevParentId, + name = intermediate.Name, + className = intermediate.ClassName, + properties = {}, + children = {}, + } + self.__instanceMap:insert(tempId, intermediate) + prevParentId = tempId + end + + -- The script itself gets full encoding (Source, etc.) + local scriptTempId = HttpService:GenerateGUID() + patch.added[scriptTempId] = encodeInstance(descendant, prevParentId) + self.__instanceMap:insert(scriptTempId, descendant) + + self.__apiContext:write(patch) + end) + table.insert(self.__connections, conn) + end + end +end +``` + +**Round-trip identity**: Intermediate containers are sent as skeletons (className + name only, empty properties). The server's upsert logic (`syncback_added_instance` line 1097) matches them by name to existing tree instances and returns `Ok(true)` to skip file creation. For non-script types with empty properties, the upsert at line 1370 (`if !added.properties.is_empty()`) skips the meta-file write entirely. This ensures the filesystem is NOT modified for containers that already exist. Only the new script file gets written. + +**Server-side handling**: No new server code needed. The existing upsert handles intermediate containers without creating duplicates or modifying existing files. The new script gets created as a new file. The VFS watcher picks up the new file, the tree updates, and subsequent WebSocket patches include the script normally. + +**After the write completes**: The server broadcasts a patch that includes the new script. The plugin receives it and the InstanceMap gets updated with the real server-assigned Ref (the temp ID was for the write request only -- the server's applied patch uses its own Refs). The plugin's InstanceMap may briefly have stale temp IDs, but the next patch from the server will replace them via hydration. + +**Note**: The server side already handles the reverse direction: when a script file is created on disk in a pruned subtree, the VFS watcher picks it up, adds it to the tree and `script_refs`, and the WebSocket path (with the 1d ancestor injection fix) correctly sends it to the plugin. + +--- + +## Part 4: Test Fixtures + +### 4a. New test fixture: `rojo-test/serve-tests/scripts_only_read_pruning/` + +Project with scripts at various depths mixed with non-script instances. Tests that `GET /api/read` only returns scripts and their ancestors -- verifies the script index produces correct results. + +``` +default.project.json5 (syncScriptsOnly: true) +src/ + TopScript.server.luau + DeepNest/ + Middle/ + DeepScript.luau + PureParts/ + PartA.model.json5 + PartB.model.json5 + MixedFolder/ + SomeScript.luau + SomeModel.model.json5 +``` + +**Test assertions:** + +- Read response includes `TopScript`, `DeepScript`, `SomeScript` and their ancestor containers +- Read response does NOT include `PartA`, `PartB`, `SomeModel`, or the `PureParts` folder +- All included non-script ancestors have `ignoreUnknownInstances: true` and empty properties +- Instance count matches expected (scripts + ancestors only) + +### 4b. New tests in `tests/tests/serve.rs` (or `syncback_format_transitions.rs`) + +- `**scripts_only_read_prunes_non_script_subtrees`**: Verify read response excludes entire `PureParts/` subtree +- `**scripts_only_read_includes_deep_ancestors`**: Verify deeply nested scripts pull in full ancestor chain +- `**scripts_only_websocket_injects_ancestors_for_new_script`**: Create a new script file on disk inside a previously-pruned subtree (`PureParts/NewScript.luau`), verify the WebSocket message includes both the new script AND the `PureParts` ancestor container +- `**scripts_only_websocket_prunes_non_script_patches`**: Add a non-script file on disk, verify WebSocket message filters it out +- `**scripts_only_write_rejects_non_script_updates`**: POST an update for a non-script instance, verify it's filtered/ignored +- `**scripts_only_serverinfo_flag**`: Verify `GET /api/rojo` includes `syncScriptsOnly: true` + +### 4c. Existing fixture enhancement + +The existing `rojo-test/serve-tests/syncback_scripts_only/` fixture already has `NonScriptModel.model.json5`. Existing tests focus on syncback write behavior. Add a new test using this fixture that verifies `NonScriptModel` is absent from the read response. + +--- + +## Mode Transitions (Stop Server -> Toggle syncScriptsOnly -> Restart) + +This works with zero plugin-side mode-switching logic. The transitions are inherently safe because: + +1. `App:endSession()` calls `serveSession:stop()` -> `instanceMap:stop()` which **removes ALL mappings** +2. `App:startSession()` creates a **brand new** `ServeSession` with a fresh `InstanceMap` and `ChangeBatcher` +3. The fresh session calls `start()` -> `GET /api/rojo` (receives `syncScriptsOnly` flag) -> `GET /api/read` (receives pruned or full tree depending on mode) -> `hydrate()` + `diff()` + +**Normal -> Scripts-only**: Server returns pruned tree. Fresh InstanceMap gets only scripts + ancestors. Studio's non-script instances are untouched (all nodes have `ignoreUnknownInstances: true`). ChangeBatcher receives `syncScriptsOnly = true` and filters non-script changes. + +**Scripts-only -> Normal**: Server returns full tree. Fresh InstanceMap gets everything. `hydrate()` matches server instances to existing Studio instances by Name+ClassName (Studio already has them -- they were never deleted). ChangeBatcher receives `syncScriptsOnly = false` and sends all changes. + +No special handling needed. The plugin doesn't need to know it was previously in a different mode. + +## What stays the same + +- `**instance_for_scripts_only`**: Still used to build filtered Instance objects. No logic change. +- `**InstanceMap.__connectSignals()`**: Signals still connected for all mapped instances. The ChangeBatcher filtering (3b) is upstream and simpler than skipping signal connections, which could miss edge cases with Parent change detection. + +## atlas.mdc Conformance + +**Round-trip identity**: Preserved. Scripts-only pruning only affects what is SENT over the wire, not what is stored on disk. The filesystem always contains the full tree. Building an rbxl from the filesystem produces the same result regardless of scripts-only mode. The skeleton encoding for intermediate containers (step 3c) ensures no filesystem writes for existing non-script instances. + +**Code quality / DRY**: `is_script_class()` is extracted as a shared `pub(crate)` free function in `tree.rs`, replacing the private duplicate in `api.rs`. The `collect_scripts_and_ancestors` helper is removed (replaced by the script index). No new duplication introduced. + +**Two-way sync parity with CLI syncback**: In scripts-only mode, two-way sync only handles script instances. This is a different operating mode, not a divergence from syncback behavior. When `syncScriptsOnly` is false, all paths are identical to before. + +**Error handling**: All new Rust code uses `anyhow::Context` and `?` propagation per atlas.mdc conventions. No `unwrap()` or `expect()` in library code. Plugin code guards against nil InstanceMap lookups and destroyed instances. + +**Protocol compatibility**: The new `syncScriptsOnly` field on `ServerInfoResponse` uses `#[serde(default)]`, making it backward-compatible. Old plugins ignore the field (defaults to `false`). No protocol version bump needed. + +**No unnecessary changes**: The plan does not modify globIgnorePaths handling (already correct at the snapshot layer), does not rename variables, and does not refactor unrelated code. + +## Complexity comparison + +- **Initial read** (100K instances, 200 scripts, depth 20): ~200K iterations (before) vs ~4,000 (after) +- **Two-way sync** (plugin): Non-script changes silently dropped in `ChangeBatcher.add()` instead of round-tripping to server +- **Server writes**: Non-script updates filtered before filesystem I/O +- **Tree mutation overhead**: O(1) per insert/remove (HashSet add/remove) +- **Memory**: O(scripts) HashSet entries (negligible) + diff --git a/TODO.txt b/TODO.txt index f3086e50..28bd7278 100644 --- a/TODO.txt +++ b/TODO.txt @@ -350,3 +350,8 @@ when in scripts only mode, try to avoid the entire tree where scripts dont exist [yielding after sync] there is some process that is making disconnecting after a one-shot sync completes take FOREVER if the server is yielding (usually from the reconciler) + - once the one-shot sync completes, it should not be waiting for additional things to come from the server. it just needs to kill the connection immediately. and go back to the main page. + +[init scripts] +scripts actually named init or probably even Init cause the sync engine to completely break. it will syncback, but then i cannot even serve the project. +this needs to be slugified when the name exactly matches init, ignoring case [for anything, not just scripts!] diff --git a/plugin/src/ChangeBatcher/init.lua b/plugin/src/ChangeBatcher/init.lua index 00eace8c..8669856c 100644 --- a/plugin/src/ChangeBatcher/init.lua +++ b/plugin/src/ChangeBatcher/init.lua @@ -31,6 +31,7 @@ function ChangeBatcher.new(instanceMap, onChangesFlushed) __onChangesFlushed = onChangesFlushed, __pendingPropertyChanges = {}, __syncSourceOnly = false, + __syncScriptsOnly = false, __paused = false, -- Map of target Instance -> list of {sourceInstance, propertyName} @@ -54,6 +55,10 @@ function ChangeBatcher:setSyncSourceOnly(enabled) self.__syncSourceOnly = enabled end +function ChangeBatcher:setSyncScriptsOnly(enabled) + self.__syncScriptsOnly = enabled +end + -- Pause the batcher to prevent change accumulation during confirmation function ChangeBatcher:pause() self.__paused = true @@ -118,6 +123,10 @@ function ChangeBatcher:__onTargetInstanceInserted(instance) end function ChangeBatcher:add(instance, propertyName) + if self.__syncScriptsOnly and not instance:IsA("LuaSourceContainer") then + return + end + local properties = self.__pendingPropertyChanges[instance] if not properties then diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 7f3f7fce..d45bf13e 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -113,6 +113,7 @@ function ServeSession.new(options) __apiContext = options.apiContext, __twoWaySync = options.twoWaySync, __syncSourceOnly = false, + __syncScriptsOnly = false, __reconciler = reconciler, __instanceMap = instanceMap, __changeBatcher = changeBatcher, @@ -331,6 +332,9 @@ function ServeSession:start() self.__syncSourceOnly = serverInfo.syncSourceOnly or false self.__changeBatcher:setSyncSourceOnly(self.__syncSourceOnly) + self.__syncScriptsOnly = serverInfo.syncScriptsOnly or false + self.__changeBatcher:setSyncScriptsOnly(self.__syncScriptsOnly) + self.__serverInfo = serverInfo return self:__computeInitialPatch(serverInfo):andThen(function(catchUpPatch) self:setLoadingText("Starting sync loop...") @@ -360,6 +364,8 @@ function ServeSession:start() if self.__initialSyncCompleteCallback ~= nil then self.__initialSyncCompleteCallback() end + + self:__connectScriptsOnlyWatchers() end) :catch(function(err) if self.__status ~= Status.Disconnected then @@ -892,6 +898,82 @@ function ServeSession:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo) end end +function ServeSession:__connectScriptsOnlyWatchers() + if not self.__syncScriptsOnly then + return + end + if not self.__twoWaySync then + return + end + + for _, instance in self.__instanceMap.fromIds do + if instance.Parent ~= game then + continue + end + + local conn = instance.DescendantAdded:Connect(function(descendant) + if not self.__twoWaySync then + return + end + if self.__status ~= Status.Connected then + return + end + if not descendant:IsA("LuaSourceContainer") then + return + end + if self.__instanceMap.fromInstances[descendant] then + return + end + + local chain = {} + local current = descendant.Parent + while current and current ~= game do + if self.__instanceMap.fromInstances[current] then + break + end + table.insert(chain, 1, current) + current = current.Parent + end + + local mappedParent = current + if not mappedParent then + return + end + local mappedParentId = self.__instanceMap.fromInstances[mappedParent] + if not mappedParentId then + return + end + + local patch = PatchSet.newEmpty() + local prevParentId = mappedParentId + + for _, intermediate in chain do + local tempId = string.gsub(HttpService:GenerateGUID(false), "-", ""):lower() + patch.added[tempId] = { + parent = prevParentId, + name = intermediate.Name, + className = intermediate.ClassName, + properties = {}, + children = {}, + } + self.__instanceMap:insert(tempId, intermediate) + prevParentId = tempId + end + + local scriptTempId = string.gsub(HttpService:GenerateGUID(false), "-", ""):lower() + patch.added[scriptTempId] = encodeInstance(descendant, prevParentId) + self.__instanceMap:insert(scriptTempId, descendant) + + Log.info( + "Scripts-only: detected new script {} outside mapped tree, sending to server", + descendant:GetFullName() + ) + self.__apiContext:write(patch) + end) + table.insert(self.__connections, conn) + end +end + function ServeSession:__stopInternal(err) self:__setStatus(Status.Disconnected, err) self.__apiContext:disconnect() diff --git a/rojo-test/serve-tests/scripts_only_read_pruning/default.project.json5 b/rojo-test/serve-tests/scripts_only_read_pruning/default.project.json5 new file mode 100644 index 00000000..69a4eb87 --- /dev/null +++ b/rojo-test/serve-tests/scripts_only_read_pruning/default.project.json5 @@ -0,0 +1,11 @@ +{ + "name": "scripts_only_read_pruning", + "syncScriptsOnly": true, + "tree": { + "$className": "DataModel", + "ServerScriptService": { + "$className": "ServerScriptService", + "$path": "src" + } + } +} diff --git a/rojo-test/serve-tests/scripts_only_read_pruning/src/DeepNest/Middle/DeepScript.luau b/rojo-test/serve-tests/scripts_only_read_pruning/src/DeepNest/Middle/DeepScript.luau new file mode 100644 index 00000000..0f670f19 --- /dev/null +++ b/rojo-test/serve-tests/scripts_only_read_pruning/src/DeepNest/Middle/DeepScript.luau @@ -0,0 +1 @@ +return "deep script" diff --git a/rojo-test/serve-tests/scripts_only_read_pruning/src/MixedFolder/SomeModel.model.json5 b/rojo-test/serve-tests/scripts_only_read_pruning/src/MixedFolder/SomeModel.model.json5 new file mode 100644 index 00000000..61edbef6 --- /dev/null +++ b/rojo-test/serve-tests/scripts_only_read_pruning/src/MixedFolder/SomeModel.model.json5 @@ -0,0 +1,4 @@ +{ + "className": "Configuration", + "properties": {} +} diff --git a/rojo-test/serve-tests/scripts_only_read_pruning/src/MixedFolder/SomeScript.luau b/rojo-test/serve-tests/scripts_only_read_pruning/src/MixedFolder/SomeScript.luau new file mode 100644 index 00000000..a813b0cb --- /dev/null +++ b/rojo-test/serve-tests/scripts_only_read_pruning/src/MixedFolder/SomeScript.luau @@ -0,0 +1 @@ +return "mixed folder script" diff --git a/rojo-test/serve-tests/scripts_only_read_pruning/src/PureParts/PartA.model.json5 b/rojo-test/serve-tests/scripts_only_read_pruning/src/PureParts/PartA.model.json5 new file mode 100644 index 00000000..3daa0cf7 --- /dev/null +++ b/rojo-test/serve-tests/scripts_only_read_pruning/src/PureParts/PartA.model.json5 @@ -0,0 +1,4 @@ +{ + "className": "Part", + "properties": {} +} diff --git a/rojo-test/serve-tests/scripts_only_read_pruning/src/PureParts/PartB.model.json5 b/rojo-test/serve-tests/scripts_only_read_pruning/src/PureParts/PartB.model.json5 new file mode 100644 index 00000000..3daa0cf7 --- /dev/null +++ b/rojo-test/serve-tests/scripts_only_read_pruning/src/PureParts/PartB.model.json5 @@ -0,0 +1,4 @@ +{ + "className": "Part", + "properties": {} +} diff --git a/rojo-test/serve-tests/scripts_only_read_pruning/src/TopScript.server.luau b/rojo-test/serve-tests/scripts_only_read_pruning/src/TopScript.server.luau new file mode 100644 index 00000000..6b766386 --- /dev/null +++ b/rojo-test/serve-tests/scripts_only_read_pruning/src/TopScript.server.luau @@ -0,0 +1 @@ +print("top level script") diff --git a/src/snapshot/patch_apply.rs b/src/snapshot/patch_apply.rs index fa236635..46c832ab 100644 --- a/src/snapshot/patch_apply.rs +++ b/src/snapshot/patch_apply.rs @@ -251,6 +251,13 @@ fn apply_update_child(context: &mut PatchApplyContext, tree: &mut RojoTree, patc applied_patch.changed_metadata = Some(metadata); } + // Capture old class name before the mutable borrow so we can update + // the script index afterward if the class changes. + let old_class_name = patch.changed_class_name.and_then(|_| { + tree.get_instance(patch.id) + .map(|inst| inst.class_name().to_string()) + }); + let mut instance = match tree.get_instance_mut(patch.id) { Some(instance) => instance, None => { @@ -307,6 +314,11 @@ fn apply_update_child(context: &mut PatchApplyContext, tree: &mut RojoTree, patc applied_patch.changed_properties.insert(key, property_entry); } + if let (Some(old_class), Some(new_class)) = (&old_class_name, &applied_patch.changed_class_name) + { + tree.update_script_tracking(patch.id, old_class, new_class.as_str()); + } + defer_ref_properties(tree, patch.id, context); context.applied_patch_set.updated.push(applied_patch) diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index fd7922b7..4666dad7 100644 --- a/src/snapshot/tree.rs +++ b/src/snapshot/tree.rs @@ -1,5 +1,5 @@ use std::{ - collections::{HashMap, VecDeque}, + collections::{HashMap, HashSet, VecDeque}, path::{Path, PathBuf}, }; @@ -12,6 +12,11 @@ use crate::{multimap::MultiMap, RojoRef}; use super::{InstanceMetadata, InstanceSnapshot, InstigatingSource}; +#[inline] +pub(crate) fn is_script_class(class_name: &str) -> bool { + matches!(class_name, "Script" | "LocalScript" | "ModuleScript") +} + /// An expanded variant of rbx_dom_weak's `WeakDom` that tracks additional /// metadata per instance that's Rojo-specific. /// @@ -39,6 +44,11 @@ pub struct RojoTree { /// the same RojoRef for multiple different instances. An entry containing /// multiple elements is an error condition that should be raised to the user. specified_id_to_refs: MultiMap, + + /// Refs of all Script, LocalScript, and ModuleScript instances in the tree. + /// Maintained incrementally for fast scripts-only mode filtering in the + /// serve API without walking the entire tree. + script_refs: HashSet, } impl RojoTree { @@ -52,6 +62,7 @@ impl RojoTree { metadata_map: HashMap::new(), path_to_ids: MultiMap::new(), specified_id_to_refs: MultiMap::new(), + script_refs: HashSet::new(), }; let root_ref = tree.inner.root_ref(); @@ -124,6 +135,8 @@ impl RojoTree { _ => Vec::new(), }; + let is_script = is_script_class(snapshot.class_name.as_ref()); + let builder = InstanceBuilder::empty() .with_class(snapshot.class_name) .with_name(snapshot.name.into_owned()) @@ -133,6 +146,10 @@ impl RojoTree { let referent = self.inner.insert(parent_ref, builder); self.insert_metadata(referent, snapshot.metadata); + if is_script { + self.script_refs.insert(referent); + } + for child in snapshot.children { self.insert_instance(referent, child); } @@ -146,6 +163,7 @@ impl RojoTree { while let Some(id) = to_move.pop_front() { self.remove_metadata(id); + self.script_refs.remove(&id); if let Some(instance) = self.inner.get_by_ref(id) { to_move.extend(instance.children().iter().copied()); @@ -231,6 +249,20 @@ impl RojoTree { self.specified_id_to_refs.insert(specified, id); } + pub fn script_refs(&self) -> &HashSet { + &self.script_refs + } + + pub fn update_script_tracking(&mut self, id: Ref, old_class: &str, new_class: &str) { + let was_script = is_script_class(old_class); + let is_script = is_script_class(new_class); + if was_script && !is_script { + self.script_refs.remove(&id); + } else if !was_script && is_script { + self.script_refs.insert(id); + } + } + /// Looks up an instance by its filesystem-name path in the tree. /// /// Each segment is matched against the **filesystem name** of child diff --git a/src/web/api.rs b/src/web/api.rs index eb2e839d..9b18e41e 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -393,6 +393,7 @@ impl ApiService { game_id: self.serve_session.game_id(), root_instance_id, sync_source_only: true, + sync_scripts_only: self.serve_session.sync_scripts_only(), ignore_hidden_services, visible_services, git_metadata, @@ -450,7 +451,7 @@ impl ApiService { let body = request.into_body().collect().await.unwrap().to_bytes(); - let request: WriteRequest = match deserialize_msgpack(&body) { + let mut request: WriteRequest = match deserialize_msgpack(&body) { Ok(request) => request, Err(err) => { return msgpack( @@ -467,6 +468,24 @@ impl ApiService { ); } + if self.serve_session.sync_scripts_only() { + let tree = self.serve_session.tree(); + let before = request.updated.len(); + request.updated.retain(|update| { + tree.get_instance(update.id) + .map(|inst| { + crate::snapshot::is_script_class(inst.class_name().as_str()) + }) + .unwrap_or(false) + }); + let filtered = before - request.updated.len(); + if filtered > 0 { + log::debug!( + "Scripts-only mode: filtered {filtered} non-script update(s) from write request" + ); + } + } + // Process removed instances (syncback: delete files from Rojo filesystem) // Phase 1: Gather paths with the tree lock held. // Phase 2: Delete files without the lock. @@ -3101,39 +3120,43 @@ impl ApiService { let mut instances = HashMap::new(); if scripts_only { - // In scripts-only mode, we need to: - // 1. Find all scripts - // 2. Include their ancestor chains (so the tree structure is valid) - // 3. Mark non-script ancestors with ignoreUnknownInstances: true - // 4. Filter children to only include instances in our set - + // Use the script index to avoid walking the entire tree. + // For each script, walk UP to see if it's a descendant of a + // requested ID, collecting the ancestor chain along the way. let mut included_ids: HashSet = HashSet::new(); + let requested_set: HashSet = requested_ids.iter().copied().collect(); - // First pass: collect all script IDs and their ancestors - for id in &requested_ids { - if let Some(instance) = tree.get_instance(*id) { - collect_scripts_and_ancestors(&tree, instance.id(), &mut included_ids); - for descendant in tree.descendants(*id) { - collect_scripts_and_ancestors(&tree, descendant.id(), &mut included_ids); + for &script_id in tree.script_refs() { + let mut chain = Vec::new(); + let mut current = script_id; + let mut is_descendant = false; + + while let Some(inst) = tree.get_instance(current) { + if included_ids.contains(¤t) { + is_descendant = true; + break; } + if requested_set.contains(¤t) { + is_descendant = true; + chain.push(current); + break; + } + chain.push(current); + let parent = inst.parent(); + if parent.is_none() { + break; + } + current = parent; + } + + if is_descendant { + included_ids.extend(chain); } } - // Second pass: create Instance objects for included IDs - for id in &requested_ids { - if let Some(instance) = tree.get_instance(*id) { - if included_ids.contains(id) { - let inst = instance_for_scripts_only(instance, &included_ids); - instances.insert(*id, inst); - } - - for descendant in tree.descendants(*id) { - let desc_id = descendant.id(); - if included_ids.contains(&desc_id) { - let inst = instance_for_scripts_only(descendant, &included_ids); - instances.insert(desc_id, inst); - } - } + for &id in &included_ids { + if let Some(instance) = tree.get_instance(id) { + instances.insert(id, instance_for_scripts_only(instance, &included_ids)); } } } else { @@ -3532,17 +3555,13 @@ fn parent_requirements(class: &str) -> Option<&str> { }) } -/// Checks if a class name is a script type (Script, LocalScript, ModuleScript). -#[inline] -fn is_script_class(class_name: &str) -> bool { - matches!(class_name, "Script" | "LocalScript" | "ModuleScript") -} +use crate::snapshot::is_script_class; /// Filters a SubscribeMessage to only include scripts and their ancestors. /// Non-script ancestors get ignoreUnknownInstances: true. -fn filter_subscribe_message_for_scripts( - tree: &crate::snapshot::RojoTree, - msg: &mut SubscribeMessage<'_>, +fn filter_subscribe_message_for_scripts<'a>( + tree: &'a crate::snapshot::RojoTree, + msg: &mut SubscribeMessage<'a>, ) { use std::borrow::Cow; @@ -3574,9 +3593,19 @@ fn filter_subscribe_message_for_scripts( // Filter added to only include IDs in our set msg.added.retain(|id, _| included_ids.contains(id)); - // ALL instances get ignoreUnknownInstances: true in scripts-only mode - // This ensures we only sync script content, never delete/modify other instances - // Non-scripts also have their properties cleared - they're just tree structure + // Inject ancestors that the plugin may not have yet. When a new script + // appears in a previously-pruned subtree, its ancestor containers exist + // in the tree but weren't part of this patch. Without injection the + // plugin receives an orphaned script with an unknown parent. + for id in included_ids.iter().copied().collect::>() { + if !msg.added.contains_key(&id) { + if let Some(tree_inst) = tree.get_instance(id) { + let inst = instance_for_scripts_only(tree_inst, &included_ids); + msg.added.insert(id, inst); + } + } + } + for inst in msg.added.values_mut() { inst.metadata = Some(InstanceMetadata { ignore_unknown_instances: true, @@ -3612,29 +3641,6 @@ fn filter_subscribe_message_for_scripts( }); } -/// Recursively collects a script instance and all its ancestors into the set. -/// Only adds to the set if the instance is a script (then adds ancestors too). -fn collect_scripts_and_ancestors( - tree: &crate::snapshot::RojoTree, - id: Ref, - included_ids: &mut HashSet, -) { - if let Some(instance) = tree.get_instance(id) { - if is_script_class(instance.class_name().as_str()) { - // This is a script - add it and all ancestors - let mut current_id = id; - while let Some(inst) = tree.get_instance(current_id) { - included_ids.insert(current_id); - let parent_id = inst.parent(); - if parent_id.is_none() || included_ids.contains(&parent_id) { - break; - } - current_id = parent_id; - } - } - } -} - /// Creates an Instance for scripts-only mode. /// - Scripts get their properties synced normally /// - Non-scripts only provide tree structure (no properties synced) diff --git a/src/web/interface.rs b/src/web/interface.rs index 756e6ea8..88244589 100644 --- a/src/web/interface.rs +++ b/src/web/interface.rs @@ -258,6 +258,10 @@ pub struct ServerInfoResponse { /// Other property changes will be ignored. #[serde(default)] pub sync_source_only: bool, + /// When true, only script instances (and their ancestors for tree structure) + /// are synced. The plugin should skip non-script changes in two-way sync. + #[serde(default)] + pub sync_scripts_only: bool, /// When true, hidden/internal services should be ignored during sync. /// The plugin should not mark these services as "to delete" during forward sync. #[serde(default)] diff --git a/tests/tests/mod.rs b/tests/tests/mod.rs index efcd3121..ea0b91ce 100644 --- a/tests/tests/mod.rs +++ b/tests/tests/mod.rs @@ -5,6 +5,7 @@ mod connected_mode; mod git_sync_defaults; mod live_syncback; mod matching_fixtures; +mod scripts_only_pruning; mod serve; mod syncback; mod syncback_format_transitions; diff --git a/tests/tests/scripts_only_pruning.rs b/tests/tests/scripts_only_pruning.rs new file mode 100644 index 00000000..0a32ce5a --- /dev/null +++ b/tests/tests/scripts_only_pruning.rs @@ -0,0 +1,280 @@ +use std::collections::HashMap; + +use librojo::web_api::{InstanceUpdate, WriteRequest}; + +use crate::rojo_test::serve_util::run_serve_test; + +#[test] +fn scripts_only_read_prunes_non_script_subtrees() { + run_serve_test("scripts_only_read_pruning", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + let read_response = session.get_api_read(root_id).unwrap(); + + let names: Vec<&str> = read_response + .instances + .values() + .map(|inst| inst.name.as_ref()) + .collect(); + + assert!( + names.contains(&"TopScript"), + "TopScript should be included" + ); + assert!( + names.contains(&"DeepScript"), + "DeepScript should be included" + ); + assert!( + names.contains(&"SomeScript"), + "SomeScript should be included" + ); + + assert!( + !names.contains(&"PartA"), + "PartA should be pruned" + ); + assert!( + !names.contains(&"PartB"), + "PartB should be pruned" + ); + assert!( + !names.contains(&"SomeModel"), + "SomeModel should be pruned" + ); + assert!( + !names.contains(&"PureParts"), + "PureParts folder should be pruned (no script descendants)" + ); + }); +} + +#[test] +fn scripts_only_read_includes_deep_ancestors() { + run_serve_test("scripts_only_read_pruning", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + let read_response = session.get_api_read(root_id).unwrap(); + + let names: Vec<&str> = read_response + .instances + .values() + .map(|inst| inst.name.as_ref()) + .collect(); + + assert!( + names.contains(&"DeepNest"), + "DeepNest ancestor should be included for DeepScript" + ); + assert!( + names.contains(&"Middle"), + "Middle ancestor should be included for DeepScript" + ); + assert!( + names.contains(&"DeepScript"), + "DeepScript should be included" + ); + }); +} + +#[test] +fn scripts_only_read_ancestors_have_no_properties() { + run_serve_test("scripts_only_read_pruning", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + let read_response = session.get_api_read(root_id).unwrap(); + + for (_, inst) in &read_response.instances { + let is_script = matches!( + inst.class_name.as_str(), + "Script" | "LocalScript" | "ModuleScript" + ); + if !is_script && inst.name != "scripts_only_read_pruning" { + assert!( + inst.properties.is_empty(), + "Non-script ancestor '{}' ({}) should have empty properties, got {} properties", + inst.name, + inst.class_name, + inst.properties.len() + ); + if let Some(ref meta) = inst.metadata { + assert!( + meta.ignore_unknown_instances, + "Non-script ancestor '{}' should have ignoreUnknownInstances: true", + inst.name + ); + } + } + } + }); +} + +#[test] +fn scripts_only_serverinfo_flag() { + run_serve_test("scripts_only_read_pruning", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + assert!( + info.sync_scripts_only, + "syncScriptsOnly should be true in ServerInfoResponse" + ); + }); +} + +#[test] +fn scripts_only_serverinfo_flag_absent_when_disabled() { + run_serve_test("syncback_scripts_only", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + assert!( + info.sync_scripts_only, + "syncScriptsOnly should be true for syncback_scripts_only fixture" + ); + }); +} + +#[test] +fn scripts_only_write_filters_non_script_updates() { + run_serve_test("scripts_only_read_pruning", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + + let read_response = session.get_api_read(root_id).unwrap(); + + let sss_id = read_response + .instances + .iter() + .find(|(_, inst)| inst.class_name == "ServerScriptService") + .map(|(id, _)| *id) + .expect("ServerScriptService should exist"); + + let write_request = WriteRequest { + session_id: info.session_id, + removed: vec![], + added: HashMap::new(), + updated: vec![InstanceUpdate { + id: sss_id, + changed_name: Some("Renamed".to_string()), + changed_class_name: None, + changed_properties: Default::default(), + changed_metadata: None, + }], + stage_ids: Vec::new(), + }; + + session + .post_api_write(&write_request) + .expect("Write request should succeed"); + + std::thread::sleep(std::time::Duration::from_millis(200)); + + let read_after = session.get_api_read(root_id).unwrap(); + let sss_after = read_after + .instances + .iter() + .find(|(_, inst)| inst.class_name == "ServerScriptService") + .map(|(_, inst)| inst.name.as_ref()) + .expect("ServerScriptService should still exist"); + assert_eq!( + sss_after, "ServerScriptService", + "Non-script update should have been filtered; name should remain unchanged" + ); + }); +} + +#[test] +fn scripts_only_existing_fixture_prunes_non_script_model() { + run_serve_test("syncback_scripts_only", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + let read_response = session.get_api_read(root_id).unwrap(); + + let names: Vec<&str> = read_response + .instances + .values() + .map(|inst| inst.name.as_ref()) + .collect(); + + assert!( + !names.contains(&"NonScriptModel"), + "NonScriptModel (Configuration) should be pruned in scripts-only mode" + ); + }); +} + +#[test] +fn scripts_only_websocket_prunes_non_script_additions() { + use librojo::web_api::SocketPacketType; + + run_serve_test("scripts_only_read_pruning", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + + let read_response = session.get_api_read(info.root_instance_id).unwrap(); + let cursor = read_response.message_cursor; + + let packet = session + .recv_socket_packet(SocketPacketType::Messages, cursor, || { + let pure_parts_dir = session.path().join("src").join("PureParts"); + std::fs::write( + pure_parts_dir.join("AnotherPart.model.json5"), + "{ \"className\": \"Part\", \"properties\": {} }\n", + ) + .expect("Should write non-script file"); + }) + .ok(); + + if let Some(packet) = packet { + let librojo::web_api::SocketPacketBody::Messages(messages_packet) = packet.body; + for msg in &messages_packet.messages { + let has_part = msg + .added + .values() + .any(|inst| inst.name.as_ref() == "AnotherPart"); + assert!( + !has_part, + "WebSocket should not include non-script AnotherPart in scripts-only mode" + ); + } + } + }); +} + +#[test] +fn scripts_only_websocket_injects_ancestors_for_new_script() { + use librojo::web_api::SocketPacketType; + + run_serve_test("scripts_only_read_pruning", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + + let read_response = session.get_api_read(info.root_instance_id).unwrap(); + let cursor = read_response.message_cursor; + + let packet = session + .recv_socket_packet(SocketPacketType::Messages, cursor, || { + let pure_parts_dir = session.path().join("src").join("PureParts"); + std::fs::write( + pure_parts_dir.join("NewScript.luau"), + "return \"new script in pruned subtree\"\n", + ) + .expect("Should write new script file"); + }) + .expect("Should receive WebSocket packet for new script"); + + let librojo::web_api::SocketPacketBody::Messages(messages_packet) = packet.body; + let all_added: Vec<&str> = messages_packet + .messages + .iter() + .flat_map(|msg| msg.added.values()) + .map(|inst| inst.name.as_ref()) + .collect(); + + assert!( + all_added.contains(&"NewScript"), + "WebSocket should include the new script. Got: {:?}", + all_added + ); + assert!( + all_added.contains(&"PureParts"), + "WebSocket should inject the PureParts ancestor. Got: {:?}", + all_added + ); + }); +} From 9820ec653438821064fcf635a0e01a8aeb5772de Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 06:39:13 -0600 Subject: [PATCH 02/13] Enhance script handling and temp ID resolution in ServeSession and Reconciler - Improved error handling in ServeSession by rolling back temp IDs if encoding fails for new scripts. - Added logic in applyPatch to remap echoed additions from the DescendantAdded handler, preventing duplicate instances. - Updated API to resolve intermediate container temp IDs to real tree references, ensuring proper parent-child relationships for new scripts. - Introduced new tests to verify the presence of server info flags and the resolution of temp IDs in scripts-only mode. These changes aim to optimize script management and enhance the overall functionality of the session handling. --- plugin/src/Reconciler/applyPatch.lua | 27 ++++++++ plugin/src/ServeSession.lua | 10 ++- src/web/api.rs | 52 +++++++++++++-- tests/tests/scripts_only_pruning.rs | 95 +++++++++++++++++++++++++++- 4 files changed, 175 insertions(+), 9 deletions(-) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 547cdee0..465fc18c 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -67,6 +67,33 @@ local function applyPatch(instanceMap, patch) end end + -- Detect echoed additions from DescendantAdded handler. + -- When the plugin pre-inserts instances with temp IDs and the server + -- broadcasts the same instances with real Refs, remap the InstanceMap + -- entries rather than creating duplicates via reify. + local remapProgress = true + while remapProgress do + remapProgress = false + for id, virtualInstance in pairs(patch.added) do + local parentInstance = instanceMap.fromIds[virtualInstance.Parent] + if not parentInstance then continue end + for _, child in parentInstance:GetChildren() do + if child.Name == virtualInstance.Name + and child.ClassName == virtualInstance.ClassName then + local existingId = instanceMap.fromInstances[child] + if existingId and existingId ~= id then + Log.trace("Echo remap: {} -> {} for {}", existingId, id, child:GetFullName()) + instanceMap:removeId(existingId) + instanceMap:insert(id, child) + patch.added[id] = nil + remapProgress = true + break + end + end + end + end + end + for id, virtualInstance in pairs(patch.added) do if instanceMap.fromIds[id] ~= nil then -- This instance already exists. We might've already added it in a diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index d45bf13e..7952a8b3 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -961,7 +961,15 @@ function ServeSession:__connectScriptsOnlyWatchers() end local scriptTempId = string.gsub(HttpService:GenerateGUID(false), "-", ""):lower() - patch.added[scriptTempId] = encodeInstance(descendant, prevParentId) + local encoded = encodeInstance(descendant, prevParentId) + if not encoded then + Log.warn("Scripts-only: failed to encode new script {}, rolling back temp IDs", descendant:GetFullName()) + for tempId in patch.added do + self.__instanceMap:removeId(tempId) + end + return + end + patch.added[scriptTempId] = encoded self.__instanceMap:insert(scriptTempId, descendant) Log.info( diff --git a/src/web/api.rs b/src/web/api.rs index 9b18e41e..eef77cdb 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -547,6 +547,50 @@ impl ApiService { if !request.added.is_empty() { let tree = self.serve_session.tree(); + + // Phase 0: Resolve intermediate container temp IDs to real tree Refs. + // The DescendantAdded handler sends containers that already exist in the + // tree as "additions" with temp IDs. Resolve them so child additions + // (the actual new scripts) can find their parents. + let mut temp_to_real: HashMap = HashMap::new(); + let mut progress = true; + while progress { + progress = false; + for (guid, added) in request.added.iter() { + if temp_to_real.contains_key(guid) { + continue; + } + let parent = added.parent.unwrap_or(Ref::none()); + let real_parent = + temp_to_real.get(&parent).copied().unwrap_or(parent); + if tree.get_instance(real_parent).is_none() { + continue; + } + if let Some(existing_ref) = + Self::find_child_by_name(&tree, real_parent, &added.name) + { + temp_to_real.insert(*guid, existing_ref); + progress = true; + } + } + } + if !temp_to_real.is_empty() { + log::debug!( + "Scripts-only temp ID resolution: resolved {} intermediate container(s)", + temp_to_real.len() + ); + for added in request.added.values_mut() { + if let Some(parent) = added.parent { + if let Some(&real) = temp_to_real.get(&parent) { + added.parent = Some(real); + } + } + } + for guid in temp_to_real.keys() { + updated_in_place.insert(*guid); + } + } + // Pre-compute duplicate sibling info in O(N) for efficient path uniqueness checks // This makes all subsequent path checks O(d) instead of O(d × s) let duplicate_siblings_cache = Self::compute_tree_refs_with_duplicate_siblings(&tree); @@ -3633,12 +3677,8 @@ fn filter_subscribe_message_for_scripts<'a>( .unwrap_or(false) }); - // Only remove scripts (plugin will ignore unknown IDs anyway) - msg.removed.retain(|_id| { - // We can't check removed instances in the tree since they're gone, - // so we let all removals through - the plugin will ignore unknown IDs - true - }); + // Removals are left unfiltered: we can't check removed instances in the + // tree (they're already gone), and the plugin ignores unknown IDs. } /// Creates an Instance for scripts-only mode. diff --git a/tests/tests/scripts_only_pruning.rs b/tests/tests/scripts_only_pruning.rs index 0a32ce5a..779b5287 100644 --- a/tests/tests/scripts_only_pruning.rs +++ b/tests/tests/scripts_only_pruning.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; -use librojo::web_api::{InstanceUpdate, WriteRequest}; +use librojo::web_api::{AddedInstance, InstanceUpdate, WriteRequest}; +use rbx_dom_weak::types::{Ref, Variant}; use crate::rojo_test::serve_util::run_serve_test; @@ -121,7 +122,7 @@ fn scripts_only_serverinfo_flag() { } #[test] -fn scripts_only_serverinfo_flag_absent_when_disabled() { +fn scripts_only_serverinfo_flag_present_for_other_fixture() { run_serve_test("syncback_scripts_only", |session, _redactions| { let info = session.get_api_rojo().unwrap(); assert!( @@ -131,6 +132,17 @@ fn scripts_only_serverinfo_flag_absent_when_disabled() { }); } +#[test] +fn scripts_only_serverinfo_flag_false_when_disabled() { + run_serve_test("empty", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + assert!( + !info.sync_scripts_only, + "syncScriptsOnly should default to false when not set in project" + ); + }); +} + #[test] fn scripts_only_write_filters_non_script_updates() { run_serve_test("scripts_only_read_pruning", |session, _redactions| { @@ -278,3 +290,82 @@ fn scripts_only_websocket_injects_ancestors_for_new_script() { ); }); } + +#[test] +fn scripts_only_write_resolves_intermediate_temp_ids() { + run_serve_test("scripts_only_read_pruning", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + let read_response = session.get_api_read(root_id).unwrap(); + + let sss_id = read_response + .instances + .iter() + .find(|(_, inst)| inst.class_name == "ServerScriptService") + .map(|(id, _)| *id) + .expect("ServerScriptService should exist"); + + // PureParts exists on disk but is pruned from the read response. + // Simulate the DescendantAdded handler: send PureParts as an + // intermediate container with a temp ID, and a new script under it. + let temp_folder_ref = Ref::new(); + let temp_script_ref = Ref::new(); + + let folder_added = AddedInstance { + parent: Some(sss_id), + name: "PureParts".to_string(), + class_name: "Folder".to_string(), + properties: HashMap::new(), + children: vec![], + }; + + let mut script_props = HashMap::new(); + script_props.insert( + "Source".to_string(), + Variant::String("return 'temp id test'".to_string()), + ); + let script_added = AddedInstance { + parent: Some(temp_folder_ref), + name: "TempIdScript".to_string(), + class_name: "ModuleScript".to_string(), + properties: script_props, + children: vec![], + }; + + let mut added_map = HashMap::new(); + added_map.insert(temp_folder_ref, folder_added); + added_map.insert(temp_script_ref, script_added); + + let write_request = WriteRequest { + session_id: info.session_id, + removed: vec![], + added: added_map, + updated: vec![], + stage_ids: Vec::new(), + }; + + session + .post_api_write(&write_request) + .expect("Write request should succeed"); + + std::thread::sleep(std::time::Duration::from_millis(500)); + + let read_after = session.get_api_read(root_id).unwrap(); + let names: Vec<&str> = read_after + .instances + .values() + .map(|inst| inst.name.as_ref()) + .collect(); + + assert!( + names.contains(&"TempIdScript"), + "TempIdScript should appear after temp ID resolution. Got: {:?}", + names + ); + assert!( + names.contains(&"PureParts"), + "PureParts should now be included (has script descendant). Got: {:?}", + names + ); + }); +} From dcd48642be1d09fcfa866bd8b4b9f133f000a01c Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 06:44:37 -0600 Subject: [PATCH 03/13] Refactor error handling and improve code readability in ServeSession and applyPatch - Enhanced error logging in ServeSession by formatting warning messages for better clarity. - Improved readability in applyPatch by restructuring conditional statements for clarity. - Simplified assertions in tests for script pruning to maintain consistency and readability. These changes aim to improve code maintainability and enhance the clarity of error messages. --- .cursor/hooks.json | 9 +++++++++ plugin/src/Reconciler/applyPatch.lua | 7 ++++--- plugin/src/ServeSession.lua | 5 ++++- src/web/api.rs | 11 ++++------- tests/tests/scripts_only_pruning.rs | 20 ++++---------------- 5 files changed, 25 insertions(+), 27 deletions(-) create mode 100644 .cursor/hooks.json diff --git a/.cursor/hooks.json b/.cursor/hooks.json new file mode 100644 index 00000000..723daa94 --- /dev/null +++ b/.cursor/hooks.json @@ -0,0 +1,9 @@ +{ + "version": 1, + "hooks": { + "stop": [ + { "command": "cargo fmt" }, + { "command": "stylua plugin/src" } + ] + } +} diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 465fc18c..794fe3a1 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -76,10 +76,11 @@ local function applyPatch(instanceMap, patch) remapProgress = false for id, virtualInstance in pairs(patch.added) do local parentInstance = instanceMap.fromIds[virtualInstance.Parent] - if not parentInstance then continue end + if not parentInstance then + continue + end for _, child in parentInstance:GetChildren() do - if child.Name == virtualInstance.Name - and child.ClassName == virtualInstance.ClassName then + if child.Name == virtualInstance.Name and child.ClassName == virtualInstance.ClassName then local existingId = instanceMap.fromInstances[child] if existingId and existingId ~= id then Log.trace("Echo remap: {} -> {} for {}", existingId, id, child:GetFullName()) diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 7952a8b3..53fd0724 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -963,7 +963,10 @@ function ServeSession:__connectScriptsOnlyWatchers() local scriptTempId = string.gsub(HttpService:GenerateGUID(false), "-", ""):lower() local encoded = encodeInstance(descendant, prevParentId) if not encoded then - Log.warn("Scripts-only: failed to encode new script {}, rolling back temp IDs", descendant:GetFullName()) + Log.warn( + "Scripts-only: failed to encode new script {}, rolling back temp IDs", + descendant:GetFullName() + ) for tempId in patch.added do self.__instanceMap:removeId(tempId) end diff --git a/src/web/api.rs b/src/web/api.rs index eef77cdb..b2f8c10a 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -473,9 +473,7 @@ impl ApiService { let before = request.updated.len(); request.updated.retain(|update| { tree.get_instance(update.id) - .map(|inst| { - crate::snapshot::is_script_class(inst.class_name().as_str()) - }) + .map(|inst| crate::snapshot::is_script_class(inst.class_name().as_str())) .unwrap_or(false) }); let filtered = before - request.updated.len(); @@ -561,8 +559,7 @@ impl ApiService { continue; } let parent = added.parent.unwrap_or(Ref::none()); - let real_parent = - temp_to_real.get(&parent).copied().unwrap_or(parent); + let real_parent = temp_to_real.get(&parent).copied().unwrap_or(parent); if tree.get_instance(real_parent).is_none() { continue; } @@ -3642,10 +3639,10 @@ fn filter_subscribe_message_for_scripts<'a>( // in the tree but weren't part of this patch. Without injection the // plugin receives an orphaned script with an unknown parent. for id in included_ids.iter().copied().collect::>() { - if !msg.added.contains_key(&id) { + if let std::collections::hash_map::Entry::Vacant(e) = msg.added.entry(id) { if let Some(tree_inst) = tree.get_instance(id) { let inst = instance_for_scripts_only(tree_inst, &included_ids); - msg.added.insert(id, inst); + e.insert(inst); } } } diff --git a/tests/tests/scripts_only_pruning.rs b/tests/tests/scripts_only_pruning.rs index 779b5287..1bc87dbd 100644 --- a/tests/tests/scripts_only_pruning.rs +++ b/tests/tests/scripts_only_pruning.rs @@ -18,10 +18,7 @@ fn scripts_only_read_prunes_non_script_subtrees() { .map(|inst| inst.name.as_ref()) .collect(); - assert!( - names.contains(&"TopScript"), - "TopScript should be included" - ); + assert!(names.contains(&"TopScript"), "TopScript should be included"); assert!( names.contains(&"DeepScript"), "DeepScript should be included" @@ -31,18 +28,9 @@ fn scripts_only_read_prunes_non_script_subtrees() { "SomeScript should be included" ); - assert!( - !names.contains(&"PartA"), - "PartA should be pruned" - ); - assert!( - !names.contains(&"PartB"), - "PartB should be pruned" - ); - assert!( - !names.contains(&"SomeModel"), - "SomeModel should be pruned" - ); + assert!(!names.contains(&"PartA"), "PartA should be pruned"); + assert!(!names.contains(&"PartB"), "PartB should be pruned"); + assert!(!names.contains(&"SomeModel"), "SomeModel should be pruned"); assert!( !names.contains(&"PureParts"), "PureParts folder should be pruned (no script descendants)" From 57433a01589c2300b8e06871d0363488c1b2974e Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 07:23:49 -0600 Subject: [PATCH 04/13] Enhance scripts-only mode handling and improve temp ID resolution - Introduced a new mechanism in ServeSession to manage scripts-only mode, ensuring proper handling of temporary IDs during script additions. - Updated the applyPatch function to remap only scripts-only temp IDs, preventing false positives with legitimate additions. - Modified the API to resolve intermediate container temp IDs to real tree references, enhancing the integrity of parent-child relationships for new scripts. - Added tests to verify the functionality of scripts-only mode and the resolution of temp IDs. These changes aim to optimize script management and improve the overall functionality of the session handling. --- TODO.txt | 3 + plugin/src/InstanceMap.lua | 4 + plugin/src/Reconciler/applyPatch.lua | 41 +++--- plugin/src/ServeSession.lua | 50 +++++-- ...to_end__tests__serve__add_folder_info.snap | 1 + ..._to_end__tests__serve__edit_init_info.snap | 1 + .../end_to_end__tests__serve__empty_info.snap | 1 + ...__tests__serve__empty_json_model_info.snap | 1 + ...end__tests__serve__forced_parent_info.snap | 1 + ...__tests__serve__meshpart_with_id_info.snap | 1 + ...sts__serve__move_folder_of_stuff_info.snap | 1 + ...__serve__no_name_default_project_info.snap | 1 + ...d__tests__serve__no_name_project_info.snap | 1 + ...serve__no_name_top_level_project_info.snap | 1 + ...d__tests__serve__pivot_migration_info.snap | 1 + ...nd__tests__serve__ref_properties_info.snap | 1 + ...rve__ref_properties_patch_update_info.snap | 1 + ...ts__serve__ref_properties_remove_info.snap | 1 + ...o_end__tests__serve__remove_file_info.snap | 1 + ...nd_to_end__tests__serve__scripts_info.snap | 1 + ...d__tests__serve__sync_rule_alone_info.snap | 1 + ..._tests__serve__sync_rule_complex_info.snap | 1 + ...s__serve__sync_rule_no_extension_info.snap | 1 + ...serve__sync_rule_no_name_project_info.snap | 1 + src/web/api.rs | 80 +++++----- tests/tests/scripts_only_pruning.rs | 139 ++++++++++++++++++ 26 files changed, 274 insertions(+), 63 deletions(-) diff --git a/TODO.txt b/TODO.txt index 28bd7278..56538631 100644 --- a/TODO.txt +++ b/TODO.txt @@ -355,3 +355,6 @@ there is some process that is making disconnecting after a one-shot sync complet [init scripts] scripts actually named init or probably even Init cause the sync engine to completely break. it will syncback, but then i cannot even serve the project. this needs to be slugified when the name exactly matches init, ignoring case [for anything, not just scripts!] + +[one shot mode change] +new changes that come in / out during the review menu on the plugin is really disorienting. diff --git a/plugin/src/InstanceMap.lua b/plugin/src/InstanceMap.lua index 40b7d761..a5452810 100644 --- a/plugin/src/InstanceMap.lua +++ b/plugin/src/InstanceMap.lua @@ -35,6 +35,10 @@ function InstanceMap.new(onInstanceChanged) -- Callback invoked when a new instance is inserted into the map. -- Used by ChangeBatcher to resolve pending unresolved Ref properties. onInstanceInserted = nil, + + -- Temp IDs generated by DescendantAdded watchers in scripts-only mode. + -- Used by applyPatch to distinguish echo additions from legitimate ones. + scriptsOnlyTempIds = {}, } return setmetatable(self, InstanceMap) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 794fe3a1..61648f77 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -71,24 +71,29 @@ local function applyPatch(instanceMap, patch) -- When the plugin pre-inserts instances with temp IDs and the server -- broadcasts the same instances with real Refs, remap the InstanceMap -- entries rather than creating duplicates via reify. - local remapProgress = true - while remapProgress do - remapProgress = false - for id, virtualInstance in pairs(patch.added) do - local parentInstance = instanceMap.fromIds[virtualInstance.Parent] - if not parentInstance then - continue - end - for _, child in parentInstance:GetChildren() do - if child.Name == virtualInstance.Name and child.ClassName == virtualInstance.ClassName then - local existingId = instanceMap.fromInstances[child] - if existingId and existingId ~= id then - Log.trace("Echo remap: {} -> {} for {}", existingId, id, child:GetFullName()) - instanceMap:removeId(existingId) - instanceMap:insert(id, child) - patch.added[id] = nil - remapProgress = true - break + -- Only remap IDs explicitly tracked as scripts-only temp IDs to avoid + -- false positives with legitimate same-named additions. + if next(instanceMap.scriptsOnlyTempIds) then + local remapProgress = true + while remapProgress do + remapProgress = false + for id, virtualInstance in pairs(patch.added) do + local parentInstance = instanceMap.fromIds[virtualInstance.Parent] + if not parentInstance then + continue + end + for _, child in parentInstance:GetChildren() do + if child.Name == virtualInstance.Name and child.ClassName == virtualInstance.ClassName then + local existingId = instanceMap.fromInstances[child] + if existingId and existingId ~= id and instanceMap.scriptsOnlyTempIds[existingId] then + Log.trace("Echo remap: {} -> {} for {}", existingId, id, child:GetFullName()) + instanceMap.scriptsOnlyTempIds[existingId] = nil + instanceMap:removeId(existingId) + instanceMap:insert(id, child) + patch.added[id] = nil + remapProgress = true + break + end end end end diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 53fd0724..198aaeee 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -906,18 +906,39 @@ function ServeSession:__connectScriptsOnlyWatchers() return end - for _, instance in self.__instanceMap.fromIds do - if instance.Parent ~= game then - continue + local services = {} + if self.__serverInfo and self.__serverInfo.visibleServices then + for _, serviceName in ipairs(self.__serverInfo.visibleServices) do + local ok, service = pcall(game.GetService, game, serviceName) + if ok and service then + table.insert(services, service) + end end + else + for _, instance in self.__instanceMap.fromIds do + if instance.Parent == game then + table.insert(services, instance) + end + end + end - local conn = instance.DescendantAdded:Connect(function(descendant) + for _, service in services do + local conn = service.DescendantAdded:Connect(function(descendant) if not self.__twoWaySync then return end if self.__status ~= Status.Connected then return end + if Settings:get("oneShotSync") then + return + end + if RunService:IsRunning() then + return + end + if self.__changeBatcher:isPaused() then + return + end if not descendant:IsA("LuaSourceContainer") then return end @@ -936,18 +957,26 @@ function ServeSession:__connectScriptsOnlyWatchers() end local mappedParent = current - if not mappedParent then - return - end - local mappedParentId = self.__instanceMap.fromInstances[mappedParent] + local mappedParentId = mappedParent and self.__instanceMap.fromInstances[mappedParent] + if not mappedParentId then - return + local rootId = self.__instanceMap.fromInstances[game] + if not rootId then + return + end + table.insert(chain, 1, service) + mappedParentId = rootId end local patch = PatchSet.newEmpty() local prevParentId = mappedParentId for _, intermediate in chain do + local existingId = self.__instanceMap.fromInstances[intermediate] + if existingId then + prevParentId = existingId + continue + end local tempId = string.gsub(HttpService:GenerateGUID(false), "-", ""):lower() patch.added[tempId] = { parent = prevParentId, @@ -957,6 +986,7 @@ function ServeSession:__connectScriptsOnlyWatchers() children = {}, } self.__instanceMap:insert(tempId, intermediate) + self.__instanceMap.scriptsOnlyTempIds[tempId] = true prevParentId = tempId end @@ -969,11 +999,13 @@ function ServeSession:__connectScriptsOnlyWatchers() ) for tempId in patch.added do self.__instanceMap:removeId(tempId) + self.__instanceMap.scriptsOnlyTempIds[tempId] = nil end return end patch.added[scriptTempId] = encoded self.__instanceMap:insert(scriptTempId, descendant) + self.__instanceMap.scriptsOnlyTempIds[scriptTempId] = true Log.info( "Scripts-only: detected new script {} outside mapped tree, sending to server", diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_folder_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_folder_info.snap index 51c8c263..4e552470 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_folder_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_folder_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__edit_init_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__edit_init_info.snap index 5a921e2c..291b7cbb 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__edit_init_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__edit_init_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_info.snap index 55ee4ef5..fb3aaf51 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_info.snap index 996e79b2..49ba901f 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__forced_parent_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__forced_parent_info.snap index af3f5568..cd845477 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__forced_parent_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__forced_parent_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__meshpart_with_id_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__meshpart_with_id_info.snap index 9b74771e..50df6ea1 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__meshpart_with_id_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__meshpart_with_id_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__move_folder_of_stuff_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__move_folder_of_stuff_info.snap index b73a82f0..e72e21bb 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__move_folder_of_stuff_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__move_folder_of_stuff_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_default_project_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_default_project_info.snap index 180281e1..d6513fcd 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_default_project_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_default_project_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_project_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_project_info.snap index 6c950d3c..bf0f7be4 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_project_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_project_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_top_level_project_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_top_level_project_info.snap index 6277e107..e1bf19c6 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_top_level_project_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_top_level_project_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__pivot_migration_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__pivot_migration_info.snap index 22ecb8b9..c9f02a9c 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__pivot_migration_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__pivot_migration_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_info.snap index b103b930..16694999 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_info.snap index b103b930..16694999 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_remove_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_remove_info.snap index 08d20d27..572f0ffc 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_remove_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_remove_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__remove_file_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__remove_file_info.snap index ce635caa..3680063f 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__remove_file_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__remove_file_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__scripts_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__scripts_info.snap index 55ec17c0..484b57d9 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__scripts_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__scripts_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_alone_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_alone_info.snap index 3807dd72..8d03ce9e 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_alone_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_alone_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_complex_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_complex_info.snap index 5835c4c3..9d640eed 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_complex_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_complex_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_extension_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_extension_info.snap index 11d3cdbb..09428402 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_extension_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_extension_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_name_project_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_name_project_info.snap index 2ff01d5b..6a823ca4 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_name_project_info.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_name_project_info.snap @@ -9,6 +9,7 @@ rootInstanceId: id-2 serverFork: atlas serverVersion: "[server-version]" sessionId: id-1 +syncScriptsOnly: false syncSourceOnly: true visibleServices: - Lighting diff --git a/src/web/api.rs b/src/web/api.rs index b2f8c10a..06f63e24 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -546,45 +546,53 @@ impl ApiService { if !request.added.is_empty() { let tree = self.serve_session.tree(); - // Phase 0: Resolve intermediate container temp IDs to real tree Refs. - // The DescendantAdded handler sends containers that already exist in the - // tree as "additions" with temp IDs. Resolve them so child additions - // (the actual new scripts) can find their parents. - let mut temp_to_real: HashMap = HashMap::new(); - let mut progress = true; - while progress { - progress = false; - for (guid, added) in request.added.iter() { - if temp_to_real.contains_key(guid) { - continue; - } - let parent = added.parent.unwrap_or(Ref::none()); - let real_parent = temp_to_real.get(&parent).copied().unwrap_or(parent); - if tree.get_instance(real_parent).is_none() { - continue; - } - if let Some(existing_ref) = - Self::find_child_by_name(&tree, real_parent, &added.name) - { - temp_to_real.insert(*guid, existing_ref); - progress = true; - } - } - } - if !temp_to_real.is_empty() { - log::debug!( - "Scripts-only temp ID resolution: resolved {} intermediate container(s)", - temp_to_real.len() - ); - for added in request.added.values_mut() { - if let Some(parent) = added.parent { - if let Some(&real) = temp_to_real.get(&parent) { - added.parent = Some(real); + if self.serve_session.sync_scripts_only() { + // Phase 0: Resolve intermediate container temp IDs to real tree Refs. + // The DescendantAdded handler sends containers that already exist in the + // tree as "additions" with temp IDs. Resolve them so child additions + // (the actual new scripts) can find their parents. + let mut temp_to_real: HashMap = HashMap::new(); + let mut progress = true; + while progress { + progress = false; + for (guid, added) in request.added.iter() { + if temp_to_real.contains_key(guid) { + continue; + } + let parent = added.parent.unwrap_or(Ref::none()); + let real_parent = temp_to_real.get(&parent).copied().unwrap_or(parent); + if tree.get_instance(real_parent).is_none() { + continue; + } + if let Some(existing_ref) = + Self::find_child_by_name(&tree, real_parent, &added.name) + { + let class_matches = tree + .get_instance(existing_ref) + .map(|inst| inst.class_name().as_str() == added.class_name) + .unwrap_or(false); + if class_matches { + temp_to_real.insert(*guid, existing_ref); + progress = true; + } } } } - for guid in temp_to_real.keys() { - updated_in_place.insert(*guid); + if !temp_to_real.is_empty() { + log::debug!( + "Scripts-only temp ID resolution: resolved {} intermediate container(s)", + temp_to_real.len() + ); + for added in request.added.values_mut() { + if let Some(parent) = added.parent { + if let Some(&real) = temp_to_real.get(&parent) { + added.parent = Some(real); + } + } + } + for guid in temp_to_real.keys() { + updated_in_place.insert(*guid); + } } } diff --git a/tests/tests/scripts_only_pruning.rs b/tests/tests/scripts_only_pruning.rs index 1bc87dbd..0cc668c6 100644 --- a/tests/tests/scripts_only_pruning.rs +++ b/tests/tests/scripts_only_pruning.rs @@ -357,3 +357,142 @@ fn scripts_only_write_resolves_intermediate_temp_ids() { ); }); } + +#[test] +fn normal_mode_addition_works_without_phase0() { + run_serve_test("add_folder", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + assert!( + !info.sync_scripts_only, + "add_folder fixture should NOT have syncScriptsOnly" + ); + + let root_id = info.root_instance_id; + + let temp_ref = Ref::new(); + let mut props = HashMap::new(); + props.insert( + "Source".to_string(), + Variant::String("print('hello')".to_string()), + ); + let added = AddedInstance { + parent: Some(root_id), + name: "NormalModeScript".to_string(), + class_name: "Script".to_string(), + properties: props, + children: vec![], + }; + let mut added_map = HashMap::new(); + added_map.insert(temp_ref, added); + + let write_request = WriteRequest { + session_id: info.session_id, + removed: vec![], + added: added_map, + updated: vec![], + stage_ids: Vec::new(), + }; + + session + .post_api_write(&write_request) + .expect("Normal mode write should succeed without Phase 0"); + + std::thread::sleep(std::time::Duration::from_millis(500)); + + let read_after = session.get_api_read(root_id).unwrap(); + let names: Vec<&str> = read_after + .instances + .values() + .map(|inst| inst.name.as_ref()) + .collect(); + assert!( + names.contains(&"NormalModeScript"), + "Script should exist after normal-mode addition. Got: {:?}", + names + ); + }); +} + +#[test] +fn scripts_only_phase0_resolves_matching_class() { + run_serve_test("scripts_only_read_pruning", |session, _redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + let read_response = session.get_api_read(root_id).unwrap(); + + let sss_id = read_response + .instances + .iter() + .find(|(_, inst)| inst.class_name == "ServerScriptService") + .map(|(id, _)| *id) + .expect("ServerScriptService should exist"); + + // DeepNest exists in the tree as a Folder. Send an intermediate + // container with matching name AND class. Phase 0 should resolve it. + let temp_folder_ref = Ref::new(); + let temp_script_ref = Ref::new(); + + let folder_added = AddedInstance { + parent: Some(sss_id), + name: "DeepNest".to_string(), + class_name: "Folder".to_string(), + properties: HashMap::new(), + children: vec![], + }; + + let mut script_props = HashMap::new(); + script_props.insert( + "Source".to_string(), + Variant::String("return 'matching class test'".to_string()), + ); + let script_added = AddedInstance { + parent: Some(temp_folder_ref), + name: "MatchingClassScript".to_string(), + class_name: "ModuleScript".to_string(), + properties: script_props, + children: vec![], + }; + + let mut added_map = HashMap::new(); + added_map.insert(temp_folder_ref, folder_added); + added_map.insert(temp_script_ref, script_added); + + let write_request = WriteRequest { + session_id: info.session_id, + removed: vec![], + added: added_map, + updated: vec![], + stage_ids: Vec::new(), + }; + + session + .post_api_write(&write_request) + .expect("Write request should succeed"); + + std::thread::sleep(std::time::Duration::from_millis(500)); + + let read_after = session.get_api_read(root_id).unwrap(); + let names: Vec<&str> = read_after + .instances + .values() + .map(|inst| inst.name.as_ref()) + .collect(); + + assert!( + names.contains(&"MatchingClassScript"), + "Script should appear under resolved DeepNest. Got: {:?}", + names + ); + + // Verify there's still only ONE Folder named DeepNest (resolved, not duplicated) + let deep_nest_count = read_after + .instances + .values() + .filter(|inst| inst.name == "DeepNest" && inst.class_name == "Folder") + .count(); + assert_eq!( + deep_nest_count, 1, + "Phase 0 should resolve to existing DeepNest, not create a duplicate" + ); + }); +} From 9d387d2a529107210a69609a3a12e83c5d4f7962 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 11:07:11 -0600 Subject: [PATCH 05/13] Update TODO list with error handling and sync issues - Added detailed error messages related to Rojo crashes and two-way sync operations in the TODO list. - Included specific instances of removed references during synchronization to aid in debugging. - Highlighted the need to address the Rojo bug and suggested filing an issue for further investigation. These updates aim to enhance error tracking and improve the management of synchronization tasks. --- TODO.txt | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/TODO.txt b/TODO.txt index 56538631..ea038178 100644 --- a/TODO.txt +++ b/TODO.txt @@ -358,3 +358,78 @@ this needs to be slugified when the name exactly matches init, ignoring case [fo [one shot mode change] new changes that come in / out during the review menu on the plugin is really disorienting. + +[error] +[INFO librojo::change_processor] Two-way sync: Removing instance Ref(Some(11172145805454499324864790020248300740)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VisalizationInstancesV2\Visualize\VisualizeV2\init.luau) +[INFO librojo::change_processor] Two-way sync: Removing instance Ref(Some(250896002424020262431971720464633879491)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VFX\ShopAura\BillboardGui\Script.client.luau) +[INFO librojo::change_processor] Two-way sync: Removing instance Ref(Some(180523706273884596638310961780081579965)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VFX) +[INFO librojo::change_processor] Two-way sync: Removing instance Ref(Some(255012798697385670746832788816389180140)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VFX\ShopAura) +[INFO librojo::change_processor] Two-way sync: Removing instance Ref(Some(327927689482872520713513681823365738785)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup) +[INFO librojo::change_processor] Two-way sync: Writing Source to d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\Disco\init.luau +[ERROR atlas] Rojo crashed! You are running Rojo 8.4.1. +[ERROR atlas] This is probably a Rojo bug. +[ERROR atlas] +[ERROR atlas] Please consider filing an issue: https://github.com/UserGeneratedLLC/rojo/issues +[ERROR atlas] +[ERROR atlas] Details: called `Option::unwrap()` on a `None` value +[ERROR atlas] in file src\snapshot\tree.rs on line 471 + 0: backtrace::backtrace::win64::trace + at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\backtrace\win64.rs:85 + backtrace::backtrace::trace_unsynchronized + at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\backtrace\mod.rs:66 + backtrace::backtrace::trace + at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\backtrace\mod.rs:53 + 1: backtrace::capture::Backtrace::create + at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\capture.rs:294 + 2: backtrace::capture::Backtrace::new + at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\capture.rs:259 + 3: atlas::main::closure$0 + at D:\UserGenerated\rojo\src\main.rs:48 + 4: std::panicking::panic_with_hook + at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\panicking.rs:833 + 5: std::panicking::panic_handler::closure$0 + at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\panicking.rs:691 + 6: std::sys::backtrace::__rust_end_short_backtrace + at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\sys\backtrace.rs:176 + 7: std::panicking::panic_handler + at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\panicking.rs:689 + 8: core::panicking::panic_fmt + at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\core\src\panicking.rs:80 + 9: core::panicking::panic + at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\core\src\panicking.rs:150 + 10: core::option::unwrap_failed + at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\core\src\option.rs:2175 + 11: librojo::snapshot::tree::RojoTree::remove_metadata + at D:\UserGenerated\rojo\src\snapshot\tree.rs:471 + librojo::snapshot::tree::RojoTree::remove + at D:\UserGenerated\rojo\src\snapshot\tree.rs:165 + 12: librojo::snapshot::patch_apply::apply_remove_instance + at D:\UserGenerated\rojo\src\snapshot\patch_apply.rs:208 + librojo::snapshot::patch_apply::apply_patch_set + at D:\UserGenerated\rojo\src\snapshot\patch_apply.rs:32 + 13: librojo::change_processor::JobThreadContext::handle_tree_event + at D:\UserGenerated\rojo\src\change_processor.rs:1858 + 14: librojo::change_processor::impl$1::start::closure$0 + at D:\UserGenerated\rojo\src\change_processor.rs:174 + std::sys::backtrace::__rust_begin_short_backtrace,crossbeam_channel::err::RecvError> > > + at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys\backtrace.rs:160 + 15: std::thread::lifecycle::spawn_unchecked::closure$1::closure$0 + at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\thread\lifecycle.rs:92 + core::panic::unwind_safe::impl$25::call_once + at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\panic\unwind_safe.rs:274 + std::panicking::catch_unwind::do_call + at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:581 + std::panicking::catch_unwind + at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:544 + std::panic::catch_unwind + at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:359 + std::thread::lifecycle::spawn_unchecked::closure$1 + at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\thread\lifecycle.rs:90 + core::ops::function::FnOnce::call_once,crossbeam_channel::err::RecvError> > >,tuple$<> > + at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:250 + 16: std::sys::thread::windows::impl$0::new::thread_start + at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\sys\thread\windows.rs:59 + 17: BaseThreadInitThunk + 18: RtlUserThreadStart +Process exited with code 1 + From 9d6b48a53e70a5b586435d655f79a13b76a9386d Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 11:09:13 -0600 Subject: [PATCH 06/13] Refactor TODO list and improve error handling in RojoTree - Removed outdated tasks related to scripts-only mode from the TODO list to streamline focus on current issues. - Enhanced the remove method in RojoTree to prevent unnecessary operations when an ID does not exist. - Improved error handling in the remove_metadata function to gracefully handle missing metadata. These changes aim to optimize task management and enhance the robustness of the RojoTree implementation. --- TODO.txt | 3 --- src/snapshot/tree.rs | 8 +++++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/TODO.txt b/TODO.txt index ea038178..0e3c593f 100644 --- a/TODO.txt +++ b/TODO.txt @@ -345,9 +345,6 @@ we should support right-click to unselect the bubble (atlas/studio/skip) [auto select issue] when you commit changes on git, it's considered unchanged. we should send like the last git hash too, like prior to head, too. -[scripts only mode] -when in scripts only mode, try to avoid the entire tree where scripts dont exist to make syncing from the Roblox plugin faster. prune non-script leaves. - [yielding after sync] there is some process that is making disconnecting after a one-shot sync completes take FOREVER if the server is yielding (usually from the reconciler) - once the one-shot sync completes, it should not be waiting for additional things to come from the server. it just needs to kill the connection immediately. and go back to the main page. diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index 4666dad7..5626cce1 100644 --- a/src/snapshot/tree.rs +++ b/src/snapshot/tree.rs @@ -158,6 +158,10 @@ impl RojoTree { } pub fn remove(&mut self, id: Ref) { + if self.inner.get_by_ref(id).is_none() { + return; + } + let mut to_move = VecDeque::new(); to_move.push_back(id); @@ -468,7 +472,9 @@ impl RojoTree { /// Moves the Rojo metadata from the instance with the given ID from this /// tree into some loose maps. fn remove_metadata(&mut self, id: Ref) { - let metadata = self.metadata_map.remove(&id).unwrap(); + let Some(metadata) = self.metadata_map.remove(&id) else { + return; + }; if let Some(specified) = metadata.specified_id { self.specified_id_to_refs.remove(&specified, id); From ec51c14a41cdfb0e4b68db7cdbed271f0471a4b7 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 11:16:10 -0600 Subject: [PATCH 07/13] Enhance error handling and improve script tracking in RojoTree and ServeSession - Added a test to verify the functionality of script tracking updates in RojoTree, ensuring proper addition and removal of script references. - Improved error handling in ServeSession by clearing temporary script IDs upon disconnection to prevent potential memory leaks. - Updated API logic to filter out empty properties and children during instance addition, enhancing the integrity of the synchronization process. These changes aim to optimize script management and enhance the robustness of the session handling and tree operations. --- TODO.txt | 75 ----------------------------- plugin/src/ServeSession.lua | 1 + src/snapshot/tree.rs | 23 +++++++++ src/web/api.rs | 11 +++-- tests/tests/scripts_only_pruning.rs | 2 +- 5 files changed, 33 insertions(+), 79 deletions(-) diff --git a/TODO.txt b/TODO.txt index 0e3c593f..0db1c29a 100644 --- a/TODO.txt +++ b/TODO.txt @@ -355,78 +355,3 @@ this needs to be slugified when the name exactly matches init, ignoring case [fo [one shot mode change] new changes that come in / out during the review menu on the plugin is really disorienting. - -[error] -[INFO librojo::change_processor] Two-way sync: Removing instance Ref(Some(11172145805454499324864790020248300740)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VisalizationInstancesV2\Visualize\VisualizeV2\init.luau) -[INFO librojo::change_processor] Two-way sync: Removing instance Ref(Some(250896002424020262431971720464633879491)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VFX\ShopAura\BillboardGui\Script.client.luau) -[INFO librojo::change_processor] Two-way sync: Removing instance Ref(Some(180523706273884596638310961780081579965)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VFX) -[INFO librojo::change_processor] Two-way sync: Removing instance Ref(Some(255012798697385670746832788816389180140)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup\VFX\ShopAura) -[INFO librojo::change_processor] Two-way sync: Removing instance Ref(Some(327927689482872520713513681823365738785)) from tree (path: d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\EmperorPenguin\EventVFXSetup) -[INFO librojo::change_processor] Two-way sync: Writing Source to d:\UserGenerated\knockout\src\ServerScriptService\Services\AdminAbuseService\Disco\init.luau -[ERROR atlas] Rojo crashed! You are running Rojo 8.4.1. -[ERROR atlas] This is probably a Rojo bug. -[ERROR atlas] -[ERROR atlas] Please consider filing an issue: https://github.com/UserGeneratedLLC/rojo/issues -[ERROR atlas] -[ERROR atlas] Details: called `Option::unwrap()` on a `None` value -[ERROR atlas] in file src\snapshot\tree.rs on line 471 - 0: backtrace::backtrace::win64::trace - at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\backtrace\win64.rs:85 - backtrace::backtrace::trace_unsynchronized - at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\backtrace\mod.rs:66 - backtrace::backtrace::trace - at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\backtrace\mod.rs:53 - 1: backtrace::capture::Backtrace::create - at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\capture.rs:294 - 2: backtrace::capture::Backtrace::new - at C:\Users\Joe\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\backtrace-0.3.76\src\capture.rs:259 - 3: atlas::main::closure$0 - at D:\UserGenerated\rojo\src\main.rs:48 - 4: std::panicking::panic_with_hook - at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\panicking.rs:833 - 5: std::panicking::panic_handler::closure$0 - at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\panicking.rs:691 - 6: std::sys::backtrace::__rust_end_short_backtrace - at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\sys\backtrace.rs:176 - 7: std::panicking::panic_handler - at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\panicking.rs:689 - 8: core::panicking::panic_fmt - at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\core\src\panicking.rs:80 - 9: core::panicking::panic - at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\core\src\panicking.rs:150 - 10: core::option::unwrap_failed - at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\core\src\option.rs:2175 - 11: librojo::snapshot::tree::RojoTree::remove_metadata - at D:\UserGenerated\rojo\src\snapshot\tree.rs:471 - librojo::snapshot::tree::RojoTree::remove - at D:\UserGenerated\rojo\src\snapshot\tree.rs:165 - 12: librojo::snapshot::patch_apply::apply_remove_instance - at D:\UserGenerated\rojo\src\snapshot\patch_apply.rs:208 - librojo::snapshot::patch_apply::apply_patch_set - at D:\UserGenerated\rojo\src\snapshot\patch_apply.rs:32 - 13: librojo::change_processor::JobThreadContext::handle_tree_event - at D:\UserGenerated\rojo\src\change_processor.rs:1858 - 14: librojo::change_processor::impl$1::start::closure$0 - at D:\UserGenerated\rojo\src\change_processor.rs:174 - std::sys::backtrace::__rust_begin_short_backtrace,crossbeam_channel::err::RecvError> > > - at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys\backtrace.rs:160 - 15: std::thread::lifecycle::spawn_unchecked::closure$1::closure$0 - at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\thread\lifecycle.rs:92 - core::panic::unwind_safe::impl$25::call_once - at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\panic\unwind_safe.rs:274 - std::panicking::catch_unwind::do_call - at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:581 - std::panicking::catch_unwind - at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:544 - std::panic::catch_unwind - at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:359 - std::thread::lifecycle::spawn_unchecked::closure$1 - at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\thread\lifecycle.rs:90 - core::ops::function::FnOnce::call_once,crossbeam_channel::err::RecvError> > >,tuple$<> > - at C:\Users\Joe\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:250 - 16: std::sys::thread::windows::impl$0::new::thread_start - at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library\std\src\sys\thread\windows.rs:59 - 17: BaseThreadInitThunk - 18: RtlUserThreadStart -Process exited with code 1 - diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 198aaeee..c253849c 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -1020,6 +1020,7 @@ end function ServeSession:__stopInternal(err) self:__setStatus(Status.Disconnected, err) self.__apiContext:disconnect() + table.clear(self.__instanceMap.scriptsOnlyTempIds) self.__instanceMap:stop() self.__changeBatcher:stop() diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index 5626cce1..169b94a4 100644 --- a/src/snapshot/tree.rs +++ b/src/snapshot/tree.rs @@ -625,6 +625,29 @@ mod test { use super::RojoTree; + #[test] + fn update_script_tracking_adds_and_removes() { + let snapshot = InstanceSnapshot::new().name("Root").class_name("DataModel"); + let child = InstanceSnapshot::new() + .name("TestFolder") + .class_name("Folder"); + + let mut tree = RojoTree::new(snapshot); + let root = tree.get_root_id(); + let folder_id = tree.insert_instance(root, child); + + assert!(!tree.script_refs().contains(&folder_id)); + + tree.update_script_tracking(folder_id, "Folder", "ModuleScript"); + assert!(tree.script_refs().contains(&folder_id)); + + tree.update_script_tracking(folder_id, "ModuleScript", "Script"); + assert!(tree.script_refs().contains(&folder_id)); + + tree.update_script_tracking(folder_id, "Script", "Folder"); + assert!(!tree.script_refs().contains(&folder_id)); + } + #[test] fn swap_duped_specified_ids() { let custom_ref = RojoRef::new("MyCoolRef".into()); diff --git a/src/web/api.rs b/src/web/api.rs index 06f63e24..33969959 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -23,7 +23,8 @@ use rbx_dom_weak::{ use crate::{ serve_session::ServeSession, snapshot::{ - InstanceSnapshot, InstanceWithMeta, InstigatingSource, PatchAdd, PatchSet, PatchUpdate, + is_script_class, InstanceSnapshot, InstanceWithMeta, InstigatingSource, PatchAdd, PatchSet, + PatchUpdate, }, syncback::{slugify_name, VISIBLE_SERVICES}, web::{ @@ -559,6 +560,9 @@ impl ApiService { if temp_to_real.contains_key(guid) { continue; } + if !added.properties.is_empty() || !added.children.is_empty() { + continue; + } let parent = added.parent.unwrap_or(Ref::none()); let real_parent = temp_to_real.get(&parent).copied().unwrap_or(parent); if tree.get_instance(real_parent).is_none() { @@ -593,6 +597,9 @@ impl ApiService { for guid in temp_to_real.keys() { updated_in_place.insert(*guid); } + request + .added + .retain(|guid, _| !temp_to_real.contains_key(guid)); } } @@ -3604,8 +3611,6 @@ fn parent_requirements(class: &str) -> Option<&str> { }) } -use crate::snapshot::is_script_class; - /// Filters a SubscribeMessage to only include scripts and their ancestors. /// Non-script ancestors get ignoreUnknownInstances: true. fn filter_subscribe_message_for_scripts<'a>( diff --git a/tests/tests/scripts_only_pruning.rs b/tests/tests/scripts_only_pruning.rs index 0cc668c6..f4a93158 100644 --- a/tests/tests/scripts_only_pruning.rs +++ b/tests/tests/scripts_only_pruning.rs @@ -73,7 +73,7 @@ fn scripts_only_read_ancestors_have_no_properties() { let root_id = info.root_instance_id; let read_response = session.get_api_read(root_id).unwrap(); - for (_, inst) in &read_response.instances { + for inst in read_response.instances.values() { let is_script = matches!( inst.class_name.as_str(), "Script" | "LocalScript" | "ModuleScript" From 75727171ef0e8dd8f795b3f80c7c6e4ccfb88395 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 11:17:41 -0600 Subject: [PATCH 08/13] Enhance scripts-only mode in RojoTree with optimizations and fixes - Optimized scripts-only mode by implementing a server-side script index for efficient reads and added a `syncScriptsOnly` flag to streamline synchronization. - Fixed WebSocket ancestor injection to ensure new scripts in pruned subtrees are sent with their full ancestor chain. - Introduced `DescendantAdded` watchers to detect new scripts created in Studio and send them to the server with appropriate containers. These changes aim to improve performance and reliability in scripts-only mode. --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16ea250b..d2bd4235 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,10 @@ Making a new release? Simply add the new header with the version and date undern ## Unreleased +* Optimize scripts-only mode with a server-side script index on `RojoTree` for O(scripts × depth) reads instead of full-tree walks, send `syncScriptsOnly` flag to the plugin so `ChangeBatcher` skips non-script two-way sync work, and filter non-script updates in the write API as defense-in-depth. +* Fix WebSocket ancestor injection in scripts-only mode so new scripts appearing in previously-pruned subtrees are sent with their full ancestor chain. +* Add plugin-side `DescendantAdded` watchers in scripts-only mode to detect new scripts created in Studio outside the mapped tree and send them to the server with skeleton intermediate containers. + ## [8.4.1] (February 19th, 2026) * Fix spurious diffs during VFS events by removing redundant Rojo reference attributes from snapshot comparisons. From a68e0ab7c607b44c3988ae7b6949021ad48835a5 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 11:22:33 -0600 Subject: [PATCH 09/13] Add patch application state management in ServeSession - Introduced a new flag `__applyingPatch` to track the patch application state, preventing `DescendantAdded` events during reconciliation. - Updated the `__applyPatch` method to set and clear the `__applyingPatch` flag appropriately, enhancing control over script updates. - Added checks in the `__connectScriptsOnlyWatchers` method to avoid processing during patch application. These changes aim to improve the reliability of script handling during patch operations and prevent unintended side effects. --- plugin/src/ServeSession.lua | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index c253849c..47b7d477 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -124,6 +124,7 @@ function ServeSession.new(options) __serverInfo = nil, __confirmingPatch = nil, __isConfirming = false, -- Explicit confirmation state flag for defense-in-depth + __applyingPatch = false, -- Set during reconciler patch application to suppress DescendantAdded __connections = connections, __precommitCallbacks = {}, __postcommitCallbacks = {}, @@ -561,8 +562,10 @@ function ServeSession:__applyPatch(patch) end Timer.stop() + self.__applyingPatch = true local patchApplySuccess, unappliedPatch = pcall(self.__reconciler.applyPatch, self.__reconciler, patch) if not patchApplySuccess then + self.__applyingPatch = false if historyRecording then ChangeHistoryService:FinishRecording(historyRecording, Enum.FinishRecordingOperation.Commit) end @@ -596,6 +599,7 @@ function ServeSession:__applyPatch(patch) PatchSet.assign(unappliedPatch, unappliedUpdateRefs) end end + self.__applyingPatch = false if not PatchSet.isEmpty(unappliedPatch) then Log.info( @@ -939,6 +943,9 @@ function ServeSession:__connectScriptsOnlyWatchers() if self.__changeBatcher:isPaused() then return end + if self.__applyingPatch then + return + end if not descendant:IsA("LuaSourceContainer") then return end From d4b390a3ec2422d18e83230ba0b9a024ef30fcdf Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 11:36:15 -0600 Subject: [PATCH 10/13] Refactor script class handling and improve imports in git.rs and ref_properties.rs - Moved the `is_script_class` function to the `snapshot` module for better organization and reusability. - Updated imports in `git.rs` and `ref_properties.rs` to reflect the new location of `is_script_class`, enhancing code clarity and maintainability. These changes aim to streamline script class management and improve the overall structure of the codebase. --- src/git.rs | 11 ++++------- src/syncback/ref_properties.rs | 5 +---- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/git.rs b/src/git.rs index 9c19ad49..0fc8e560 100644 --- a/src/git.rs +++ b/src/git.rs @@ -10,13 +10,10 @@ use std::{ use rbx_dom_weak::types::Ref; use sha1::{Digest, Sha1}; -use crate::{snapshot::RojoTree, web::interface::GitMetadata}; - -const SCRIPT_CLASSES: &[&str] = &["Script", "LocalScript", "ModuleScript"]; - -fn is_script_class(class: &str) -> bool { - SCRIPT_CLASSES.contains(&class) -} +use crate::{ + snapshot::{is_script_class, RojoTree}, + web::interface::GitMetadata, +}; pub fn git_repo_root(project_root: &Path) -> Option { let output = Command::new("git") diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index cc54a3a2..b62ce232 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -11,6 +11,7 @@ use rbx_dom_weak::{ use crate::{ ref_attribute_name, ref_target_attribute_name, + snapshot::is_script_class, syncback::{name_needs_slugify, slugify_name}, REF_ID_ATTRIBUTE_NAME, REF_PATH_ATTRIBUTE_PREFIX, REF_POINTER_ATTRIBUTE_PREFIX, }; @@ -131,10 +132,6 @@ fn tentative_fs_name(inst: &Instance) -> String { format!("{slug}.{extension}") } -fn is_script_class(class: &str) -> bool { - matches!(class, "Script" | "LocalScript" | "ModuleScript") -} - pub struct RefLinks { /// Refs that use path-based linking (path is unique). path_links: HashMap>, From 22e437a055a39d67fefc9d4039f1fdd9cfe362f4 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 12:00:01 -0600 Subject: [PATCH 11/13] Implement fallback error handling and enhance scripts-only mode in ServeSession - Added a fallback mechanism in the `__applyPatch` method to handle errors during patch application, logging warnings for any encountered issues. - Improved the transitive closure logic in the `__connectScriptsOnlyWatchers` method to ensure all necessary temporary IDs are included as skeleton containers, enhancing the integrity of the synchronization process. These changes aim to improve error resilience and ensure proper handling of script references in scripts-only mode. --- plugin/src/ServeSession.lua | 88 ++++++++++++++++++++++++++++--------- 1 file changed, 67 insertions(+), 21 deletions(-) diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 47b7d477..1f2ec64d 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -574,33 +574,39 @@ function ServeSession:__applyPatch(patch) error(unappliedPatch) end - if Settings:get("enableSyncFallback") and not PatchSet.isEmpty(unappliedPatch) then - -- Some changes did not apply, let's try replacing them instead - local addedIdList = PatchSet.addedIdList(unappliedPatch) - local updatedIdList = PatchSet.updatedIdList(unappliedPatch) + local fallbackOk, fallbackErr = pcall(function() + if Settings:get("enableSyncFallback") and not PatchSet.isEmpty(unappliedPatch) then + -- Some changes did not apply, let's try replacing them instead + local addedIdList = PatchSet.addedIdList(unappliedPatch) + local updatedIdList = PatchSet.updatedIdList(unappliedPatch) - Log.debug("ServeSession:__replaceInstances(unappliedPatch.added)") - Timer.start("ServeSession:__replaceInstances(unappliedPatch.added)") - local addSuccess, unappliedAddedRefs = self:__replaceInstances(addedIdList) - Timer.stop() + Log.debug("ServeSession:__replaceInstances(unappliedPatch.added)") + Timer.start("ServeSession:__replaceInstances(unappliedPatch.added)") + local addSuccess, unappliedAddedRefs = self:__replaceInstances(addedIdList) + Timer.stop() - Log.debug("ServeSession:__replaceInstances(unappliedPatch.updated)") - Timer.start("ServeSession:__replaceInstances(unappliedPatch.updated)") - local updateSuccess, unappliedUpdateRefs = self:__replaceInstances(updatedIdList) - Timer.stop() + Log.debug("ServeSession:__replaceInstances(unappliedPatch.updated)") + Timer.start("ServeSession:__replaceInstances(unappliedPatch.updated)") + local updateSuccess, unappliedUpdateRefs = self:__replaceInstances(updatedIdList) + Timer.stop() - -- Update the unapplied patch to reflect which Instances were replaced successfully - if addSuccess then - table.clear(unappliedPatch.added) - PatchSet.assign(unappliedPatch, unappliedAddedRefs) + -- Update the unapplied patch to reflect which Instances were replaced successfully + if addSuccess then + table.clear(unappliedPatch.added) + PatchSet.assign(unappliedPatch, unappliedAddedRefs) + end + if updateSuccess then + table.clear(unappliedPatch.updated) + PatchSet.assign(unappliedPatch, unappliedUpdateRefs) + end end - if updateSuccess then - table.clear(unappliedPatch.updated) - PatchSet.assign(unappliedPatch, unappliedUpdateRefs) - end - end + end) self.__applyingPatch = false + if not fallbackOk then + Log.warn("Sync fallback errored: {}", fallbackErr) + end + if not PatchSet.isEmpty(unappliedPatch) then Log.info( "Could not apply all changes requested by the Rojo server:\n{}", @@ -1014,6 +1020,46 @@ function ServeSession:__connectScriptsOnlyWatchers() self.__instanceMap:insert(scriptTempId, descendant) self.__instanceMap.scriptsOnlyTempIds[scriptTempId] = true + -- Transitive closure: ensure every temp-ID parent referenced by + -- a patch entry is itself included as a skeleton container. When + -- multiple scripts are created in the same pruned subtree (e.g., + -- paste), later handlers find intermediates already in instanceMap + -- with temp IDs from earlier handlers. Without re-including those + -- skeletons, the server's Phase 0 can't resolve the temp parents. + local closureNeeded = true + while closureNeeded do + closureNeeded = false + local toAdd = {} + for _, entry in pairs(patch.added) do + local pid = entry.parent + if + pid + and self.__instanceMap.scriptsOnlyTempIds[pid] + and not patch.added[pid] + and not toAdd[pid] + then + local inst = self.__instanceMap.fromIds[pid] + if inst then + local pp = inst.Parent + local ppId = pp and self.__instanceMap.fromInstances[pp] + if ppId then + toAdd[pid] = { + parent = ppId, + name = inst.Name, + className = inst.ClassName, + properties = {}, + children = {}, + } + closureNeeded = true + end + end + end + end + for id, entry in toAdd do + patch.added[id] = entry + end + end + Log.info( "Scripts-only: detected new script {} outside mapped tree, sending to server", descendant:GetFullName() From 288e679653555fcd2af8dd8de041593d65c69f95 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 12:30:17 -0600 Subject: [PATCH 12/13] Add script reference handling in RojoTree during snapshot insertion - Implemented logic to insert script references into the tree when a snapshot of a script class is processed. - Enhanced the existing snapshot insertion method to ensure proper tracking of script instances. These changes aim to improve the management of script references within the RojoTree structure. --- src/snapshot/tree.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index 169b94a4..a40aeea9 100644 --- a/src/snapshot/tree.rs +++ b/src/snapshot/tree.rs @@ -69,6 +69,10 @@ impl RojoTree { tree.insert_metadata(root_ref, snapshot.metadata); + if is_script_class(snapshot.class_name.as_ref()) { + tree.script_refs.insert(root_ref); + } + for child in snapshot.children { tree.insert_instance(root_ref, child); } From 9abf2be6aead8454c4b337f737a3dad9fb90e945 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 14:26:44 -0600 Subject: [PATCH 13/13] Add wait_until_all_staged function to improve file staging checks - Introduced a new function `wait_until_all_staged` that polls for all specified files to be staged with exponential backoff, enhancing reliability in tests. - Replaced direct assertions for file staging with calls to `wait_until_all_staged` in multiple test cases, improving test robustness and reducing reliance on fixed sleep durations. These changes aim to ensure that all files are properly staged before proceeding with assertions, enhancing the accuracy of the tests. --- tests/tests/git_sync_defaults.rs | 62 ++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/tests/tests/git_sync_defaults.rs b/tests/tests/git_sync_defaults.rs index 5a7e95ec..bf13d305 100644 --- a/tests/tests/git_sync_defaults.rs +++ b/tests/tests/git_sync_defaults.rs @@ -705,6 +705,32 @@ fn batch_script_rel_path(i: usize) -> String { format!("src/{}.luau", batch_script_name(i)) } +/// Poll until every `rel_paths` entry is staged, with exponential backoff. +/// Panics after `timeout` if any file is still unstaged. +fn wait_until_all_staged(dir: &Path, rel_paths: &[String], timeout: Duration) { + let start = std::time::Instant::now(); + let mut interval = Duration::from_millis(200); + loop { + let all_staged = rel_paths.iter().all(|p| git_is_staged(dir, p)); + if all_staged { + return; + } + if start.elapsed() >= timeout { + for p in rel_paths { + if !git_is_staged(dir, p) { + panic!( + "Timed out after {:?} waiting for staging: {} is not staged", + timeout, p + ); + } + } + return; + } + thread::sleep(interval); + interval = (interval * 2).min(Duration::from_secs(2)); + } +} + #[test] fn batch_stage_20_files_no_index_lock_race() { let mut session = TestServeSession::new_with_git("git_sync_defaults", |path| { @@ -741,15 +767,11 @@ fn batch_stage_20_files_no_index_lock_race() { stage_ids, }; session.post_api_write(&write_request).unwrap(); - thread::sleep(Duration::from_millis(1500)); - for i in 1..=BATCH_SCRIPT_COUNT { - assert!( - git_is_staged(session.path(), &batch_script_rel_path(i)), - "{} should be staged", - batch_script_name(i) - ); - } + let rel_paths: Vec = (1..=BATCH_SCRIPT_COUNT) + .map(batch_script_rel_path) + .collect(); + wait_until_all_staged(session.path(), &rel_paths, Duration::from_secs(30)); } #[test] @@ -797,15 +819,14 @@ fn batch_source_write_20_files_all_staged() { stage_ids, }; session.post_api_write(&write_request).unwrap(); - thread::sleep(Duration::from_millis(2000)); + + let rel_paths: Vec = (1..=BATCH_SCRIPT_COUNT) + .map(batch_script_rel_path) + .collect(); + wait_until_all_staged(session.path(), &rel_paths, Duration::from_secs(30)); for i in 1..=BATCH_SCRIPT_COUNT { let rel = batch_script_rel_path(i); - assert!( - git_is_staged(session.path(), &rel), - "{} should be staged after batch Source write", - batch_script_name(i) - ); let content = fs::read_to_string(session.path().join(&rel)).unwrap(); assert!( content.contains(&format!("batch pull {}", i)), @@ -878,16 +899,11 @@ fn batch_mixed_stage_and_source_write() { stage_ids, }; session.post_api_write(&write_request).unwrap(); - thread::sleep(Duration::from_millis(2000)); - // All 20 files should be staged - for i in 1..=BATCH_SCRIPT_COUNT { - assert!( - git_is_staged(session.path(), &batch_script_rel_path(i)), - "{} should be staged in mixed batch", - batch_script_name(i) - ); - } + let rel_paths: Vec = (1..=BATCH_SCRIPT_COUNT) + .map(batch_script_rel_path) + .collect(); + wait_until_all_staged(session.path(), &rel_paths, Duration::from_secs(30)); // Files 11-20 should have the new pull content for i in 11..=BATCH_SCRIPT_COUNT {