From 9820ec653438821064fcf635a0e01a8aeb5772de Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 28 Feb 2026 06:39:13 -0600 Subject: [PATCH] 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 + ); + }); +}