From 18a74c8e4d35dd03a92aae16451ed6d9b293763f Mon Sep 17 00:00:00 2001 From: Joe Date: Fri, 30 Jan 2026 10:31:36 -0600 Subject: [PATCH] Enhance script handling and UI consistency - Added support for encoding the RunContext property in the Script class to determine file suffixes (.server.luau, .client.luau, .plugin.luau, .legacy.luau). - Updated the Confirming.lua file to improve UI consistency in the confirmation message display. - Modified snapshot middleware and syncback logic to accommodate new file extensions and ensure proper handling of plugin scripts. - Refactored tests to align with the new naming conventions for script suffixes and improved path uniqueness checks. --- plugin/src/App/StatusPages/Confirming.lua | 24 ++-- plugin/src/ChangeBatcher/encodeInstance.lua | 12 ++ src/snapshot_middleware/mod.rs | 10 +- src/syncback/file_names.rs | 2 + src/syncback/mod.rs | 2 +- src/syncback/ref_properties.rs | 11 +- src/web/api.rs | 121 +++++++++----------- 7 files changed, 95 insertions(+), 87 deletions(-) diff --git a/plugin/src/App/StatusPages/Confirming.lua b/plugin/src/App/StatusPages/Confirming.lua index 7c4daa40..8ff3872d 100644 --- a/plugin/src/App/StatusPages/Confirming.lua +++ b/plugin/src/App/StatusPages/Confirming.lua @@ -327,18 +327,18 @@ function ConfirmingPage:render() self:setState({ showingAcceptConfirm = false }) end, }, { - Content = e("Frame", { - Size = UDim2.fromScale(1, 1), - BackgroundColor3 = theme.BackgroundColor, - BorderSizePixel = 0, - }, { - Message = e("TextLabel", { - Text = string.format( - "%d item%s not been reviewed.\n\nAccept selected changes and skip the rest?", - self.state.unselectedCount, - self.state.unselectedCount == 1 and " has" or "s have" - ), - FontFace = theme.Font.Main, + Content = e("Frame", { + Size = UDim2.fromScale(1, 1), + BackgroundColor3 = theme.BackgroundColor, + BorderSizePixel = 0, + }, { + Message = e("TextLabel", { + Text = string.format( + "%d item%s not been reviewed.\n\nAccept selected changes and skip the rest?", + self.state.unselectedCount, + self.state.unselectedCount == 1 and " has" or "s have" + ), + FontFace = theme.Font.Main, TextSize = theme.TextSize.Body, TextColor3 = theme.TextColor, TextWrapped = true, diff --git a/plugin/src/ChangeBatcher/encodeInstance.lua b/plugin/src/ChangeBatcher/encodeInstance.lua index 54f7f8ce..0fe59534 100644 --- a/plugin/src/ChangeBatcher/encodeInstance.lua +++ b/plugin/src/ChangeBatcher/encodeInstance.lua @@ -161,6 +161,18 @@ encodeInstance = function(instance, parentId, _skipPathCheck) Log.warn("Could not find Source property descriptor for {:?}", instance) return nil end + + -- For Script class specifically, also encode RunContext property + -- This determines the file suffix (.server.luau, .client.luau, .plugin.luau, .legacy.luau) + if instance.ClassName == "Script" then + local runContextDescriptor = RbxDom.findCanonicalPropertyDescriptor("Script", "RunContext") + if runContextDescriptor then + local encodeSuccess, encodeResult = encodeProperty(instance, "RunContext", runContextDescriptor) + if encodeSuccess and encodeResult ~= nil then + properties.RunContext = encodeResult + end + end + end elseif not DIRECTORY_CLASSES[instance.ClassName] then -- For non-directory, non-script classes, encode all relevant properties local classDescriptor = RbxDom.findClassDescriptor(instance.ClassName) diff --git a/src/snapshot_middleware/mod.rs b/src/snapshot_middleware/mod.rs index c2e95128..5d60c931 100644 --- a/src/snapshot_middleware/mod.rs +++ b/src/snapshot_middleware/mod.rs @@ -96,7 +96,7 @@ pub fn snapshot_from_vfs( match file_name { // Modern extensions "init.server.luau" | "init.client.luau" | "init.local.luau" | "init.legacy.luau" - | "init.luau" | "init.csv" | + | "init.plugin.luau" | "init.luau" | "init.csv" | // Legacy extensions (for backwards compatibility) "init.server.lua" | "init.client.lua" | "init.lua" => return Ok(None), _ => {} @@ -129,6 +129,7 @@ fn get_dir_middleware<'path>( (Middleware::ModuleScriptDir, "init.luau"), (Middleware::ServerScriptDir, "init.server.luau"), (Middleware::ClientScriptDir, "init.client.luau"), + (Middleware::PluginScriptDir, "init.plugin.luau"), (Middleware::LocalScriptDir, "init.local.luau"), (Middleware::LegacyScriptDir, "init.legacy.luau"), (Middleware::CsvDir, "init.csv"), @@ -222,6 +223,8 @@ pub enum Middleware { #[serde(skip_deserializing)] ModuleScriptDir, #[serde(skip_deserializing)] + PluginScriptDir, + #[serde(skip_deserializing)] LocalScriptDir, #[serde(skip_deserializing)] LegacyScriptDir, @@ -267,6 +270,9 @@ impl Middleware { Self::ModuleScriptDir => { snapshot_lua_init(context, vfs, path, name, ScriptType::Module) } + Self::PluginScriptDir => { + snapshot_lua_init(context, vfs, path, name, ScriptType::Plugin) + } Self::LocalScriptDir => snapshot_lua_init(context, vfs, path, name, ScriptType::Local), Self::LegacyScriptDir => { snapshot_lua_init(context, vfs, path, name, ScriptType::Legacy) @@ -316,6 +322,7 @@ impl Middleware { Middleware::ModuleScriptDir => syncback_lua_init(ScriptType::Module, snapshot), Middleware::LocalScriptDir => syncback_lua_init(ScriptType::Local, snapshot), Middleware::LegacyScriptDir => syncback_lua_init(ScriptType::Legacy, snapshot), + Middleware::PluginScriptDir => syncback_lua_init(ScriptType::Plugin, snapshot), Middleware::CsvDir => syncback_csv_init(snapshot), } } @@ -329,6 +336,7 @@ impl Middleware { | Middleware::ServerScriptDir | Middleware::ClientScriptDir | Middleware::ModuleScriptDir + | Middleware::PluginScriptDir | Middleware::LocalScriptDir | Middleware::LegacyScriptDir | Middleware::CsvDir diff --git a/src/syncback/file_names.rs b/src/syncback/file_names.rs index 1e3f5a20..904fb9a6 100644 --- a/src/syncback/file_names.rs +++ b/src/syncback/file_names.rs @@ -39,6 +39,7 @@ pub fn name_for_inst<'old>( | Middleware::ServerScriptDir | Middleware::ClientScriptDir | Middleware::ModuleScriptDir + | Middleware::PluginScriptDir | Middleware::LocalScriptDir | Middleware::LegacyScriptDir => { let name = if encode_invalid_chars { @@ -110,6 +111,7 @@ pub fn extension_for_middleware(middleware: Middleware) -> &'static str { | Middleware::ServerScriptDir | Middleware::ClientScriptDir | Middleware::ModuleScriptDir + | Middleware::PluginScriptDir | Middleware::LocalScriptDir | Middleware::LegacyScriptDir => { unimplemented!("directory middleware requires special treatment") diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index a536fff5..eced6b16 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -383,7 +383,7 @@ pub fn get_best_middleware(snapshot: &SyncbackSnapshot) -> Middleware { Middleware::ServerScript => Middleware::ServerScriptDir, Middleware::ClientScript => Middleware::ClientScriptDir, Middleware::ModuleScript => Middleware::ModuleScriptDir, - Middleware::PluginScript => Middleware::ServerScriptDir, // Plugin scripts with children use server script dir for now + Middleware::PluginScript => Middleware::PluginScriptDir, Middleware::LegacyScript => Middleware::LegacyScriptDir, Middleware::LocalScript => Middleware::LocalScriptDir, Middleware::Csv => Middleware::CsvDir, diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index 871e19d8..f1acac82 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -71,7 +71,10 @@ fn compute_refs_with_duplicate_siblings(dom: &WeakDom) -> HashSet { let mut name_to_refs: HashMap<&str, Vec> = HashMap::new(); for child_ref in inst.children() { if let Some(child) = dom.get_by_ref(*child_ref) { - name_to_refs.entry(&child.name).or_default().push(*child_ref); + name_to_refs + .entry(&child.name) + .or_default() + .push(*child_ref); } queue.push_back(*child_ref); } @@ -157,9 +160,9 @@ pub fn collect_referents(dom: &WeakDom, pre_prune_paths: &HashMap) // Check if target exists in current DOM if let Some(_target) = dom.get_by_ref(*target_ref) { // Target exists - check if path is unique (with caching) - let is_unique = *path_unique_cache - .entry(*target_ref) - .or_insert_with(|| is_path_unique_with_cache(dom, *target_ref, &has_duplicate_siblings)); + let is_unique = *path_unique_cache.entry(*target_ref).or_insert_with(|| { + is_path_unique_with_cache(dom, *target_ref, &has_duplicate_siblings) + }); if is_unique { // Path is unique - use path-based system diff --git a/src/web/api.rs b/src/web/api.rs index 353438ab..f246ed35 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -273,9 +273,7 @@ impl ApiService { /// /// This is O(N) where N is the number of instances, and allows subsequent /// path uniqueness checks to be O(d) instead of O(d × s) where d=depth, s=siblings. - fn compute_tree_refs_with_duplicate_siblings( - tree: &crate::snapshot::RojoTree, - ) -> HashSet { + fn compute_tree_refs_with_duplicate_siblings(tree: &crate::snapshot::RojoTree) -> HashSet { use std::collections::VecDeque; let mut has_duplicate_siblings = HashSet::new(); @@ -292,7 +290,10 @@ impl ApiService { let mut name_to_refs: HashMap<&str, Vec> = HashMap::new(); for child_ref in inst.children() { if let Some(child) = tree.get_instance(*child_ref) { - name_to_refs.entry(child.name()).or_default().push(*child_ref); + name_to_refs + .entry(child.name()) + .or_default() + .push(*child_ref); } queue.push_back(*child_ref); } @@ -2295,7 +2296,7 @@ mod tests { .and_then(|e| e.items.get(name).copied()) } - /// Simulates suffix determination logic + /// Simulates suffix determination logic (matches get_script_suffix_for_run_context) fn determine_suffix_from_run_context(run_context_value: Option) -> &'static str { let run_context_enums = rbx_reflection_database::get() .ok() @@ -2308,14 +2309,14 @@ mod tests { return match name.as_ref() { "Client" => "client", "Server" => "server", - "Legacy" => "server", + "Legacy" => "legacy", "Plugin" => "plugin", - _ => "server", + _ => "legacy", }; } } } - "server" + "legacy" } #[test] @@ -2354,13 +2355,13 @@ mod tests { } #[test] - fn test_run_context_legacy_produces_server_suffix() { + fn test_run_context_legacy_produces_legacy_suffix() { let value = get_run_context_value("Legacy"); assert!(value.is_some(), "Legacy RunContext should exist"); let suffix = determine_suffix_from_run_context(value); assert_eq!( - suffix, "server", - "RunContext::Legacy should produce 'server' suffix" + suffix, "legacy", + "RunContext::Legacy should produce 'legacy' suffix" ); } @@ -2376,21 +2377,21 @@ mod tests { } #[test] - fn test_no_run_context_defaults_to_server() { + fn test_no_run_context_defaults_to_legacy() { let suffix = determine_suffix_from_run_context(None); assert_eq!( - suffix, "server", - "No RunContext should default to 'server' suffix" + suffix, "legacy", + "No RunContext should default to 'legacy' suffix" ); } #[test] - fn test_invalid_run_context_defaults_to_server() { + fn test_invalid_run_context_defaults_to_legacy() { // Some unlikely value that doesn't match any known RunContext let suffix = determine_suffix_from_run_context(Some(99999)); assert_eq!( - suffix, "server", - "Invalid RunContext should default to 'server' suffix" + suffix, "legacy", + "Invalid RunContext should default to 'legacy' suffix" ); } @@ -2399,18 +2400,18 @@ mod tests { // Verify expected file extension patterns let test_cases = vec![ ("ModuleScript", None, ".luau"), - ("LocalScript", None, ".client.luau"), + ("LocalScript", None, ".local.luau"), ("Script", get_run_context_value("Server"), ".server.luau"), ("Script", get_run_context_value("Client"), ".client.luau"), - ("Script", get_run_context_value("Legacy"), ".server.luau"), + ("Script", get_run_context_value("Legacy"), ".legacy.luau"), ("Script", get_run_context_value("Plugin"), ".plugin.luau"), - ("Script", None, ".server.luau"), + ("Script", None, ".legacy.luau"), ]; for (class_name, run_context, expected_ext) in test_cases { let suffix = match class_name { "ModuleScript" => "", - "LocalScript" => "client", + "LocalScript" => "local", "Script" => determine_suffix_from_run_context(run_context), _ => panic!("Unknown class"), }; @@ -2434,7 +2435,7 @@ mod tests { // Verify init file patterns for scripts with children let test_cases = vec![ ("ModuleScript", None, "init.luau"), - ("LocalScript", None, "init.client.luau"), + ("LocalScript", None, "init.local.luau"), ( "Script", get_run_context_value("Server"), @@ -2448,7 +2449,7 @@ mod tests { ( "Script", get_run_context_value("Legacy"), - "init.server.luau", + "init.legacy.luau", ), ( "Script", @@ -2460,7 +2461,7 @@ mod tests { for (class_name, run_context, expected_init) in test_cases { let suffix = match class_name { "ModuleScript" => "", - "LocalScript" => "client", + "LocalScript" => "local", "Script" => determine_suffix_from_run_context(run_context), _ => panic!("Unknown class"), }; @@ -3088,62 +3089,44 @@ mod tests { } #[test] - fn test_emit_legacy_scripts_compatibility() { - // Test that our syncback is compatible with emitLegacyScripts mode + fn test_new_script_naming_convention() { + // Test that syncback produces the correct file extensions with the new naming convention // - // emitLegacyScripts: true - // - .client.luau -> LocalScript - // - .server.luau -> Script with RunContext::Legacy + // New naming convention (emitLegacyScripts removed): + // - LocalScript -> .local.luau + // - Script + RunContext::Server -> .server.luau + // - Script + RunContext::Client -> .client.luau + // - Script + RunContext::Plugin -> .plugin.luau + // - Script + RunContext::Legacy -> .legacy.luau + // - Script (no RunContext) -> .legacy.luau (default) // - // emitLegacyScripts: false - // - .client.luau -> Script with RunContext::Client - // - .server.luau -> Script with RunContext::Server + // Backwards compatibility for READING old files: + // - .client.lua -> LocalScript + // - .server.lua -> Script with RunContext::Legacy - // Our syncback produces: - // - LocalScript -> .client.luau (compatible with legacy: true) - // - Script + RunContext::Client -> .client.luau (compatible with legacy: false) - // - Script + RunContext::Server -> .server.luau (compatible with legacy: false) - // - Script + RunContext::Legacy -> .server.luau (compatible with legacy: true) - - // This test verifies the design is correct - let legacy_mode_read = vec![ - (".client.luau", "LocalScript", None), - (".server.luau", "Script", Some("Legacy")), + // Test expected syncback extensions + let test_cases = vec![ + ("LocalScript", None, ".local.luau"), + ("Script", Some("Server"), ".server.luau"), + ("Script", Some("Client"), ".client.luau"), + ("Script", Some("Plugin"), ".plugin.luau"), + ("Script", Some("Legacy"), ".legacy.luau"), + ("Script", None, ".legacy.luau"), ]; - let non_legacy_mode_read = vec![ - (".client.luau", "Script", Some("Client")), - (".server.luau", "Script", Some("Server")), - ]; - - // Verify legacy mode round-trips correctly - for (extension, class, run_context) in &legacy_mode_read { - // If we syncback this class with this RunContext, what extension do we get? + for (class, run_context, expected_ext) in &test_cases { let syncback_ext = match (*class, *run_context) { - ("LocalScript", _) => ".client.luau", - ("Script", Some("Legacy")) => ".server.luau", + ("LocalScript", _) => ".local.luau", ("Script", Some("Server")) => ".server.luau", ("Script", Some("Client")) => ".client.luau", - ("Script", None) => ".server.luau", + ("Script", Some("Plugin")) => ".plugin.luau", + ("Script", Some("Legacy")) => ".legacy.luau", + ("Script", None) => ".legacy.luau", _ => panic!("Unexpected combo"), }; assert_eq!( - syncback_ext, *extension, - "Legacy mode round-trip failed for {} {:?}", - class, run_context - ); - } - - // Verify non-legacy mode round-trips correctly - for (extension, class, run_context) in &non_legacy_mode_read { - let syncback_ext = match (*class, *run_context) { - ("Script", Some("Client")) => ".client.luau", - ("Script", Some("Server")) => ".server.luau", - _ => panic!("Unexpected combo"), - }; - assert_eq!( - syncback_ext, *extension, - "Non-legacy mode round-trip failed for {} {:?}", + syncback_ext, *expected_ext, + "New naming convention failed for {} {:?}", class, run_context ); }