From c7cddb72c250cc55c1fa2648456ceb18894a8f17 Mon Sep 17 00:00:00 2001 From: Joe Date: Fri, 30 Jan 2026 07:18:10 -0600 Subject: [PATCH] Update syncback feature to handle instance pulling from Studio - Updated the `ServeSession` to implement logic for pulling instances that exist in Studio but not in Rojo's filesystem, creating appropriate files during sync confirmation. - Introduced a new `encodeInstance` function to serialize Studio instances for transmission, supporting all instance types. - Modified the `PatchTree` to allow explicit user selection for push/pull/skip actions, enhancing user control during sync operations. - Added tests for the new encoding functionality and updated existing tests to ensure proper behavior with the new syncback logic. --- ...l_removed_scripts_feature_dd790311.plan.md | 365 +++ plugin/src/App/StatusPages/Confirming.lua | 127 +- plugin/src/App/init.lua | 13 +- plugin/src/ChangeBatcher/encodeInstance.lua | 132 + .../src/ChangeBatcher/encodeInstance.spec.lua | 160 ++ plugin/src/ChangeBatcher/init.lua | 21 + plugin/src/ChangeBatcher/init.spec.lua | 70 + plugin/src/PatchTree.lua | 24 +- plugin/src/PatchTree.spec.lua | 303 +++ plugin/src/ServeSession.lua | 94 +- rbx-dom | 2 +- .../syncback_write/default.project.json | 10 + .../syncback_write/src/existing.luau | 2 + src/web/api.rs | 2230 +++++++++++++++++ src/web/interface.rs | 20 +- 15 files changed, 3534 insertions(+), 39 deletions(-) create mode 100644 .cursor/plans/pull_removed_scripts_feature_dd790311.plan.md create mode 100644 plugin/src/ChangeBatcher/encodeInstance.lua create mode 100644 plugin/src/ChangeBatcher/encodeInstance.spec.lua create mode 100644 plugin/src/PatchTree.spec.lua create mode 100644 rojo-test/serve-tests/syncback_write/default.project.json create mode 100644 rojo-test/serve-tests/syncback_write/src/existing.luau diff --git a/.cursor/plans/pull_removed_scripts_feature_dd790311.plan.md b/.cursor/plans/pull_removed_scripts_feature_dd790311.plan.md new file mode 100644 index 00000000..52d65ad3 --- /dev/null +++ b/.cursor/plans/pull_removed_scripts_feature_dd790311.plan.md @@ -0,0 +1,365 @@ +--- +name: Pull Instances to Rojo Feature +overview: Enable "Pull" for ALL instance types that exist in Studio but not in Rojo's filesystem, creating appropriate files during sync confirmation. +todos: + - id: update-write-request + content: Update WriteRequest struct to include full instance data for added items + status: completed + - id: implement-syncback-write + content: Implement syncback logic in handle_api_write for added instances + status: completed + - id: plugin-pull-removed + content: Handle 'pull' for removed items in ServeSession.lua + status: completed + - id: encode-instance + content: Create encodeInstance.lua to serialize Studio instances for transmission + status: completed + - id: rename-commit-button + content: Rename "Commit" button to "Accept" in confirmation UI + status: completed + - id: forced-review + content: Implement forced review - default unselected, popup to confirm accepting partial selections + status: completed + - id: dynamic-dialogue + content: Allow changes to flow into dialogue, unselect items that change after selection + status: completed + - id: pause-changebatcher + content: Pause ChangeBatcher during confirmation to prevent change accumulation + status: completed + - id: defense-in-depth + content: Add explicit checks to block patch application during confirmation + status: completed +isProject: false +--- + +# Pull Instances to Rojo Filesystem + +## Problem + +When an instance exists in Roblox Studio but not in Rojo's filesystem (appears as "to be deleted" in the sync diff): + +- **Push** = Delete from Studio (works correctly) +- **Pull** = Should create file in Rojo (currently does nothing) +- **Skip** = Do nothing (works correctly) + +The "Pull" option for removed items is currently ignored in [plugin/src/ServeSession.lua](plugin/src/ServeSession.lua) (lines 663-674). + +## Scope: ALL Instance Types + +This feature supports pulling **all instance types** from Studio to Rojo, not just scripts: + + +| Instance Type | No Children | With Children | +| ------------------ | -------------------- | ------------------------------------------- | +| ModuleScript | `.luau` | dir + `init.luau` + `init.meta.json5`* | +| Script† | `.{suffix}.luau` | dir + `init.{suffix}.luau` + `init.meta.json5`* | +| LocalScript | `.client.luau` | dir + `init.client.luau` + `init.meta.json5`* | +| Folder | dir + `.gitkeep` | dir + `init.meta.json5`* | +| Configuration | dir + `.gitkeep` | dir + `init.meta.json5`* | +| Tool | dir + `.gitkeep` | dir + `init.meta.json5`* | +| ScreenGui | dir + `.gitkeep` | dir + `init.meta.json5`* | +| SurfaceGui | dir + `.gitkeep` | dir + `init.meta.json5`* | +| BillboardGui | dir + `.gitkeep` | dir + `init.meta.json5`* | +| AdGui | dir + `.gitkeep` | dir + `init.meta.json5`* | +| StringValue | `.txt` | dir + `init.meta.json5` | +| LocalizationTable | `.csv` | dir + `init.csv` | +| All other classes | `.model.json5` | dir + `init.meta.json5` | + +\* `init.meta.json5` only created if instance has properties (Attributes, etc.) +\* Empty directories get `.gitkeep` file (matching dedicated syncback behavior) + +† **Script RunContext determines file suffix:** +- `RunContext: Client` → `.client.luau` +- `RunContext: Server` → `.server.luau` +- `RunContext: Legacy` → `.server.luau` +- `RunContext: Plugin` → `.plugin.luau` +- No RunContext → `.server.luau` (default) + +This ensures proper round-tripping regardless of `emitLegacyScripts` mode in the project configuration. + +**Important:** +- We do NOT use `.rbxm` binary files as that would be a regression for version control. +- This matches the dedicated syncback system's middleware selection logic in `src/syncback/mod.rs`. + +## Solution Overview + +```mermaid +sequenceDiagram + participant Studio as Roblox Studio + participant Plugin as Rojo Plugin + participant Server as Rojo Server + participant FS as Filesystem + + Note over Studio,Plugin: Instance exists in Studio, not in Rojo + Plugin->>Plugin: Show in diff as "Remove" + Plugin->>Plugin: User selects "Pull" + Plugin->>Plugin: Encode instance with properties + Plugin->>Server: POST /api/write with added instance + Server->>Server: Determine file path from parent + Server->>FS: Create appropriate file based on class + Server->>Plugin: Success response +``` + + + +## Changes Required + +### 1. Server: Update WriteRequest Structure + +**File:** [src/web/interface.rs](src/web/interface.rs) + +Change `added` from `HashMap` to include instance data: + +```rust +pub struct AddedInstance { + pub parent: Ref, + pub name: String, + pub class_name: String, + pub properties: UstrMap, +} + +pub struct WriteRequest { + // ... existing fields ... + pub added: HashMap, // Changed from HashMap +} +``` + +### 2. Server: Implement Syncback in Write Handler + +**File:** [src/web/api.rs](src/web/api.rs) + +In `handle_api_write`, process `added` instances: + +- Determine the filesystem path based on parent instance's path +- Use syncback middleware to create the appropriate file (`.luau` for scripts) +- Trigger a re-snapshot so Rojo picks up the new file + +### 3. Plugin: Encode and Send Added Instances + +**File:** [plugin/src/ServeSession.lua](plugin/src/ServeSession.lua) + +In `__confirmAndApplyInitialPatch`, handle "pull" for removed items: + +```lua +elseif selection == "pull" and self.__twoWaySync then + local instance = + if instance and isScriptClass(instance.ClassName) then + local encoded = encodeInstance(instance, parentId) + pullPatch.added[] = encoded + end +end +``` + +### 4. Plugin: Create Instance Encoder + +**New file:** `plugin/src/ChangeBatcher/encodeInstance.lua` + +Encode a Studio instance for transmission to the server: + +- Serialize Name, ClassName, Parent ref +- Encode Source property for scripts +- Handle other relevant properties + +## Key Considerations + +1. **Parent Resolution:** The server needs to know where to create the file. The parent instance must already exist in Rojo's tree to determine the filesystem path. +2. **All Instance Types Supported:** Scripts use `.luau`, Folders become directories, all other instances use `.model.json` (human-readable, version-control friendly). +3. **No Binary Files:** We explicitly avoid `.rbxm` files as they are not version-control friendly. +4. **Two-Way Sync Required:** This feature only activates when `twoWaySync` is enabled (already checked in the code). +5. **File Naming:** Instance Name + appropriate extension based on class type. +6. **Universal Children Support:** ANY instance can have children in Roblox. When an instance has children, it MUST become a directory: + - Scripts with children → directory + `init.luau`/`init.server.luau`/`init.client.luau` + `init.meta.json5` if has non-Source properties + - Directory classes (Folder, etc.) → directory + `init.meta.json5` if has properties + - StringValue with children → directory + `init.meta.json5` (not .txt) + - LocalizationTable with children → directory + `init.csv` + - All other classes with children → directory + `init.meta.json5` +7. **Attributes on All Types:** All instances (including Folders) can have Attributes. These are encoded and saved in `init.meta.json5`. +8. **Script Properties:** Scripts with children also get `init.meta.json5` for non-Source properties (like Attributes). +9. **Empty Directories:** Empty directories with no metadata get `.gitkeep` file (matching dedicated syncback behavior). +10. **Attributes Filtering:** Internal Roblox attributes (prefixed with `RBX`) are filtered out (matching dedicated syncback). +11. **Property Filtering:** `Ref` and `UniqueId` properties are not serialized to JSON (matching dedicated syncback). +12. **Sorted JSON:** Meta files use sorted JSON5 format with keys in alphabetical order (matching dedicated syncback). +13. **RunContext-Based Script Suffix:** For `Script` class, the file suffix is determined by `RunContext` property: + - `RunContext: Client` → `.client.luau` + - `RunContext: Server` → `.server.luau` + - `RunContext: Legacy` → `.server.luau` + - `RunContext: Plugin` → `.plugin.luau` + - This ensures proper round-tripping regardless of `emitLegacyScripts` mode. +14. **Adjacent Meta Files:** Scripts WITHOUT children create adjacent meta files (`{name}.meta.json5`) next to the script file, preserving non-Source properties like `RunContext`. Scripts WITH children use `init.meta.json5` inside their directory. + +## Confirmation Dialogue Hardening + +The dialogue must be the single source of truth - nothing changes without explicit user approval. + +### 7. Pause ChangeBatcher During Confirmation + +**File:** [plugin/src/ChangeBatcher/init.lua](plugin/src/ChangeBatcher/init.lua) + +Add pause/resume functionality to prevent change accumulation during confirmation: + +- Add `__paused` flag +- Skip `__cycle` when paused +- Pause when confirmation opens, resume after + +### 8. Defense-in-Depth: Block Patch Application During Confirmation + +**File:** [plugin/src/ServeSession.lua](plugin/src/ServeSession.lua) + +Add explicit check in `__applyPatch` to prevent application during confirmation in one-shot mode: + +```lua +if Settings:get("oneShotSync") and self.__confirmingPatch ~= nil then + error("Cannot apply patch during confirmation in one-shot mode") +end +``` + +### 9. Add Confirmation State Flag + +**File:** [plugin/src/ServeSession.lua](plugin/src/ServeSession.lua) + +Add `__isConfirming` flag for explicit state tracking: + +- Set `true` when confirmation dialogue opens +- Set `false` when confirmation completes +- Verify state before any patch application + +### 5. UI: Rename "Commit" to "Accept" + +**File:** [plugin/src/App/StatusPages/Confirming.lua](plugin/src/App/StatusPages/Confirming.lua) + +Rename the button from "Commit" to "Accept" since the action now covers both directions (pushing to Studio AND pulling to Rojo). + +```lua +-- Line 168-169, change: +Commit = e(TextButton, { + text = "Commit", +-- To: +Accept = e(TextButton, { + text = "Accept", +``` + +### 6. UI: Forced Review System + +**Files:** + +- [plugin/src/PatchTree.lua](plugin/src/PatchTree.lua) +- [plugin/src/App/StatusPages/Confirming.lua](plugin/src/App/StatusPages/Confirming.lua) +- [plugin/src/App/Components/PatchVisualizer/DomLabel.lua](plugin/src/App/Components/PatchVisualizer/DomLabel.lua) + +Implement forced review where every item must be explicitly selected: + +**A. Default to unselected (no radio button checked):** + +```lua +-- Lines 325, 378, 456 - change: +defaultSelection = "push", +-- To: +defaultSelection = nil, -- No selection +``` + +**B. Accept button behavior based on selection state:** + +- **All items selected:** Accept works immediately, no popup +- **Some items unselected:** Show confirmation popup: + - "X items have not been reviewed. Accept selected changes and skip the rest?" + - Options: "Accept and Skip Rest" / "Cancel" (go back to review) +- This allows users to select only the changes they want without manually skipping hundreds of items + +**C. Visual indicator for unselected items:** + +- No radio button checked (all three Push/Pull/Skip unchecked) +- Possibly highlight row to indicate needs review + +### 6b. Dynamic Dialogue Updates + +**File:** [plugin/src/ServeSession.lua](plugin/src/ServeSession.lua) + +Allow new changes to flow into the confirmation dialogue: + +**A. Merge incoming changes into `__confirmingPatch`:** + +- When new WebSocket messages arrive during confirmation, merge them into the current patch +- Update the UI to show new items + +**B. Unselect changed items:** + +- If an item that was already selected (Push/Pull/Skip) is updated by a new change, reset its selection to unselected +- User must re-review the updated item + +**C. Block all background syncing:** + +- While dialogue is open, no changes apply automatically +- Everything goes through the dialogue + +## Files to Modify + +- [src/web/interface.rs](src/web/interface.rs) - Update `WriteRequest.added` type +- [src/web/api.rs](src/web/api.rs) - Implement syncback in `handle_api_write` +- [plugin/src/ServeSession.lua](plugin/src/ServeSession.lua) - Handle "pull" for removed items, dynamic dialogue updates, confirmation state, defense-in-depth +- [plugin/src/ChangeBatcher/encodeInstance.lua](plugin/src/ChangeBatcher/encodeInstance.lua) - New file to encode instances +- [plugin/src/ChangeBatcher/init.lua](plugin/src/ChangeBatcher/init.lua) - Add pause/resume functionality +- [plugin/src/App/StatusPages/Confirming.lua](plugin/src/App/StatusPages/Confirming.lua) - Rename "Commit" to "Accept", disable until all reviewed, show unreviewed count +- [plugin/src/PatchTree.lua](plugin/src/PatchTree.lua) - Default to unselected, track selection completeness +- [plugin/src/App/Components/PatchVisualizer/DomLabel.lua](plugin/src/App/Components/PatchVisualizer/DomLabel.lua) - Support unselected state visually + +## Testing + +### Server-side Tests (Rust) + +**File:** [src/web/api.rs](src/web/api.rs) - `#[cfg(test)]` module + +Tests for `variant_to_json`: +- String, Bool, Int32, Int64, Float32, Float64 +- Vector2, Vector3, Color3 +- UDim, UDim2 +- Enum, NumberRange, Rect +- Unsupported types return None + +Tests for syncback file naming: +- ModuleScript → `.luau` +- Script → `.server.luau` +- LocalScript → `.client.luau` +- Folder → directory (no file) +- Other instances → `.model.json` + +Tests for model.json structure: +- Basic structure (className, properties) +- With properties + +### Plugin Tests (Lua - TestEZ) + +**File:** [plugin/src/ChangeBatcher/init.spec.lua](plugin/src/ChangeBatcher/init.spec.lua) +- `pause/resume` tests: + - Start unpaused by default + - Pausable + - Resumable after pause + - No flush when paused + - Flush after resume + +**File:** [plugin/src/ChangeBatcher/encodeInstance.spec.lua](plugin/src/ChangeBatcher/encodeInstance.spec.lua) +- Script encoding (ModuleScript, Script, LocalScript) +- Folder encoding +- Other instance encoding (Part, StringValue, Configuration) +- Parent ID handling +- Edge cases (special characters, empty name) + +**File:** [plugin/src/PatchTree.spec.lua](plugin/src/PatchTree.spec.lua) +- `buildInitialSelections` - verifies nil defaultSelection behavior +- `countUnselected` - counts nodes without selections +- `allNodesSelected` - checks all/some/none selected +- `getSelectableNodeIds` - returns selectable node IDs + +### Integration Tests + +**Test fixture:** [rojo-test/serve-tests/syncback_write/](rojo-test/serve-tests/syncback_write/) + +### Running Tests + +```bash +# Rust tests +cargo test + +# Plugin tests (in Roblox Studio) +# Run plugin/src/runTests.lua +``` + diff --git a/plugin/src/App/StatusPages/Confirming.lua b/plugin/src/App/StatusPages/Confirming.lua index 4f2a726d..c166caae 100644 --- a/plugin/src/App/StatusPages/Confirming.lua +++ b/plugin/src/App/StatusPages/Confirming.lua @@ -35,7 +35,8 @@ function ConfirmingPage:init() showingTableDiff = false, oldTable = {}, newTable = {}, - showingRejectConfirm = false, + showingAcceptConfirm = false, + unselectedCount = 0, selections = initialSelections, }) @@ -64,10 +65,30 @@ function ConfirmingPage:init() end function ConfirmingPage:didUpdate(prevProps) - -- If patchTree changed, reinitialize selections + -- If patchTree changed, update selections if prevProps.patchTree ~= self.props.patchTree and self.props.patchTree then - local initialSelections = PatchTree.buildInitialSelections(self.props.patchTree) - self:setState({ selections = initialSelections }) + -- Get the new items that need default selections + local newSelections = PatchTree.buildInitialSelections(self.props.patchTree) + + -- Preserve existing selections, but unselect items that changed + local updatedSelections = {} + for id, selection in pairs(self.state.selections) do + -- Preserve selection only if the item wasn't changed + -- (Changed items must be re-reviewed, so leave them unselected) + local wasChanged = self.props.changedIds and self.props.changedIds[id] + if not wasChanged then + updatedSelections[id] = selection + end + end + + -- Merge with any new default selections (new items get nil = unselected) + for id, selection in pairs(newSelections) do + if updatedSelections[id] == nil then + updatedSelections[id] = selection + end + end + + self:setState({ selections = updatedSelections }) end end @@ -165,15 +186,28 @@ function ConfirmingPage:render() end, }), - Commit = e(TextButton, { - text = "Commit", + Accept = e(TextButton, { + text = "Accept", style = "Primary", transparency = self.props.transparency, layoutOrder = 5, - enabled = allSelected, onClick = function() - if allSelected and self.props.onConfirm then + if not self.props.onConfirm then + return + end + + -- Check if all items are selected + if allSelected then + -- All items selected, proceed immediately self.props.onConfirm(self.state.selections) + else + -- Some items unselected, show confirmation popup + local unselectedCount = + PatchTree.countUnselected(self.props.patchTree, self.state.selections) + self:setState({ + showingAcceptConfirm = true, + unselectedCount = unselectedCount, + }) end end, }), @@ -274,6 +308,83 @@ function ConfirmingPage:render() }), }), }), + + -- Accept confirmation popup for unselected items + AcceptConfirm = e(StudioPluginGui, { + id = "Rojo_AcceptConfirm", + title = "Unreviewed Items", + active = self.state.showingAcceptConfirm, + isEphemeral = true, + + initDockState = Enum.InitialDockState.Float, + overridePreviousState = true, + floatingSize = Vector2.new(400, 150), + minimumSize = Vector2.new(350, 130), + + zIndexBehavior = Enum.ZIndexBehavior.Sibling, + + onClose = function() + self:setState({ showingAcceptConfirm = false }) + end, + }, { + Content = e("Frame", { + Size = UDim2.fromScale(1, 1), + BackgroundColor3 = theme.Background2, + 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.Regular, + TextSize = theme.TextSize.Body, + TextColor3 = theme.TextColor, + TextWrapped = true, + TextXAlignment = Enum.TextXAlignment.Center, + Size = UDim2.new(1, -20, 1, -60), + Position = UDim2.new(0, 10, 0, 10), + BackgroundTransparency = 1, + }), + + Buttons = e("Frame", { + Size = UDim2.new(1, -20, 0, 34), + Position = UDim2.new(0, 10, 1, -44), + BackgroundTransparency = 1, + }, { + Cancel = e(TextButton, { + text = "Cancel", + style = "Bordered", + transparency = self.props.transparency, + layoutOrder = 1, + onClick = function() + self:setState({ showingAcceptConfirm = false }) + end, + }), + + AcceptAndSkip = e(TextButton, { + text = "Accept and Skip Rest", + style = "Primary", + transparency = self.props.transparency, + layoutOrder = 2, + onClick = function() + self:setState({ showingAcceptConfirm = false }) + if self.props.onConfirm then + self.props.onConfirm(self.state.selections) + end + end, + }), + + Layout = e("UIListLayout", { + HorizontalAlignment = Enum.HorizontalAlignment.Right, + FillDirection = Enum.FillDirection.Horizontal, + SortOrder = Enum.SortOrder.LayoutOrder, + Padding = UDim.new(0, 10), + }), + }), + }), + }), }) if self.props.createPopup then diff --git a/plugin/src/App/init.lua b/plugin/src/App/init.lua index 508c330c..6583f71f 100644 --- a/plugin/src/App/init.lua +++ b/plugin/src/App/init.lua @@ -823,14 +823,7 @@ function App:startSession() return result end) - serveSession:setPatchUpdateCallback(function(instanceMap, patch) - -- ONE-SHOT MODE: Ignore all further changes during confirmation - -- They will be picked up on the next connection attempt - if Settings:get("oneShotSync") then - Log.info("One-shot mode: ignoring incoming changes during confirmation") - return - end - + serveSession:setPatchUpdateCallback(function(instanceMap, patch, changedIds) -- If all changes have been reverted, auto-accept the empty patch if PatchSet.isEmpty(patch) then Log.trace("Patch became empty after merging, auto-accepting") @@ -840,8 +833,11 @@ function App:startSession() end -- Update the patchTree when new changes arrive during confirmation + -- The changedIds parameter contains IDs of items that were modified + -- so their selections can be reset (user must re-review) self:setState({ patchTree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }), + changedIds = changedIds, -- Pass to UI so it can unselect changed items }) end) @@ -949,6 +945,7 @@ function App:render() ConfirmingPage = createPageElement(AppStatus.Confirming, { confirmData = self.state.confirmData, patchTree = self.state.patchTree, + changedIds = self.state.changedIds, createPopup = not self.state.guiEnabled, onAbort = function() diff --git a/plugin/src/ChangeBatcher/encodeInstance.lua b/plugin/src/ChangeBatcher/encodeInstance.lua new file mode 100644 index 00000000..b238414c --- /dev/null +++ b/plugin/src/ChangeBatcher/encodeInstance.lua @@ -0,0 +1,132 @@ +--[[ + Encodes a Roblox Studio instance into a format suitable for transmission + to the Rojo server for syncback (creating files from Studio instances). + + Supports ALL instance types: + - Scripts (ModuleScript, Script, LocalScript) -> .luau files or directories with init files if has children + - Folders -> directories (with Attributes if present) + - All other instances -> .model.json files or directories + + Children are recursively encoded and included. +]] + +local Packages = script.Parent.Parent.Parent.Packages +local Log = require(Packages.Log) +local RbxDom = require(Packages.RbxDom) + +local encodeProperty = require(script.Parent.encodeProperty) + +-- Script class names that need Source property +local SCRIPT_CLASS_NAMES = { + Script = true, + LocalScript = true, + ModuleScript = true, +} + +-- Properties to skip when encoding (internal Roblox properties) +local SKIP_PROPERTIES = { + Parent = true, + Name = true, -- We handle this separately + Archivable = true, +} + +-- Classes that become directories but may have properties (like Attributes) +local DIRECTORY_CLASSES = { + Folder = true, + Configuration = true, + Tool = true, + ScreenGui = true, + SurfaceGui = true, + BillboardGui = true, + AdGui = true, +} + +-- Forward declaration for recursion +local encodeInstance + +-- Encode Attributes if present on any instance +local function encodeAttributes(instance, properties) + -- Try to get Attributes - this works for all instance types + local success, attributes = pcall(function() + return instance:GetAttributes() + end) + + if success and attributes and next(attributes) then + -- Attributes need to be encoded specially + -- The server expects them in the properties map under "Attributes" + local attributeDescriptor = RbxDom.findCanonicalPropertyDescriptor(instance.ClassName, "Attributes") + if attributeDescriptor then + local encodeSuccess, encodeResult = encodeProperty(instance, "Attributes", attributeDescriptor) + if encodeSuccess and encodeResult ~= nil then + properties.Attributes = encodeResult + end + end + end +end + +encodeInstance = function(instance, parentId) + local properties = {} + + -- Always try to encode Attributes for any instance type + encodeAttributes(instance, properties) + + if SCRIPT_CLASS_NAMES[instance.ClassName] then + -- For scripts, encode the Source property + local sourceDescriptor = RbxDom.findCanonicalPropertyDescriptor(instance.ClassName, "Source") + if sourceDescriptor then + local encodeSuccess, encodeResult = encodeProperty(instance, "Source", sourceDescriptor) + if encodeSuccess then + properties.Source = encodeResult + else + Log.warn("Failed to encode Source property for {:?}", instance) + return nil + end + else + Log.warn("Could not find Source property descriptor for {:?}", instance) + return nil + end + elseif not DIRECTORY_CLASSES[instance.ClassName] then + -- For non-directory, non-script classes, encode all relevant properties + local classDescriptor = RbxDom.findClassDescriptor(instance.ClassName) + if classDescriptor then + for propertyName, propertyDescriptor in classDescriptor.properties do + if SKIP_PROPERTIES[propertyName] then + continue + end + + -- Skip Attributes here since we handle it separately above + if propertyName == "Attributes" then + continue + end + + -- Only encode serializable properties + if propertyDescriptor.scriptability == "ReadWrite" or propertyDescriptor.scriptability == "Read" then + local encodeSuccess, encodeResult = encodeProperty(instance, propertyName, propertyDescriptor) + if encodeSuccess and encodeResult ~= nil then + properties[propertyName] = encodeResult + end + end + end + end + end + -- For directory classes (Folder, Configuration, etc.), we only encode Attributes (done above) + + -- Recursively encode children + local children = {} + for _, child in ipairs(instance:GetChildren()) do + local encodedChild = encodeInstance(child, nil) -- parentId will be resolved server-side + if encodedChild then + table.insert(children, encodedChild) + end + end + + return { + parent = parentId, + name = instance.Name, + className = instance.ClassName, + properties = properties, + children = children, -- Include children array (may be empty) + } +end + +return encodeInstance diff --git a/plugin/src/ChangeBatcher/encodeInstance.spec.lua b/plugin/src/ChangeBatcher/encodeInstance.spec.lua new file mode 100644 index 00000000..e9868152 --- /dev/null +++ b/plugin/src/ChangeBatcher/encodeInstance.spec.lua @@ -0,0 +1,160 @@ +return function() + local encodeInstance = require(script.Parent.encodeInstance) + + describe("encodeInstance", function() + describe("script encoding", function() + it("should encode ModuleScript correctly", function() + local moduleScript = Instance.new("ModuleScript") + moduleScript.Name = "TestModule" + moduleScript.Source = "return {}" + + local encoded = encodeInstance(moduleScript, "PARENT_ID") + + expect(encoded).to.be.ok() + expect(encoded.parent).to.equal("PARENT_ID") + expect(encoded.name).to.equal("TestModule") + expect(encoded.className).to.equal("ModuleScript") + expect(encoded.properties).to.be.ok() + expect(encoded.properties.Source).to.be.ok() + + moduleScript:Destroy() + end) + + it("should encode Script correctly", function() + local script = Instance.new("Script") + script.Name = "TestScript" + script.Source = "print('hello')" + + local encoded = encodeInstance(script, "PARENT_ID") + + expect(encoded).to.be.ok() + expect(encoded.name).to.equal("TestScript") + expect(encoded.className).to.equal("Script") + expect(encoded.properties.Source).to.be.ok() + + script:Destroy() + end) + + it("should encode LocalScript correctly", function() + local localScript = Instance.new("LocalScript") + localScript.Name = "TestClient" + localScript.Source = "print('client')" + + local encoded = encodeInstance(localScript, "PARENT_ID") + + expect(encoded).to.be.ok() + expect(encoded.name).to.equal("TestClient") + expect(encoded.className).to.equal("LocalScript") + expect(encoded.properties.Source).to.be.ok() + + localScript:Destroy() + end) + end) + + describe("folder encoding", function() + it("should encode Folder correctly", function() + local folder = Instance.new("Folder") + folder.Name = "TestFolder" + + local encoded = encodeInstance(folder, "PARENT_ID") + + expect(encoded).to.be.ok() + expect(encoded.parent).to.equal("PARENT_ID") + expect(encoded.name).to.equal("TestFolder") + expect(encoded.className).to.equal("Folder") + -- Folders have no special properties + expect(encoded.properties).to.be.ok() + + folder:Destroy() + end) + end) + + describe("other instance encoding", function() + it("should encode Part with properties", function() + local part = Instance.new("Part") + part.Name = "TestPart" + part.Anchored = true + + local encoded = encodeInstance(part, "PARENT_ID") + + expect(encoded).to.be.ok() + expect(encoded.name).to.equal("TestPart") + expect(encoded.className).to.equal("Part") + expect(encoded.properties).to.be.ok() + -- Properties should be captured + -- Note: Exact properties depend on RbxDom descriptors + + part:Destroy() + end) + + it("should encode StringValue", function() + local stringValue = Instance.new("StringValue") + stringValue.Name = "TestValue" + stringValue.Value = "Hello World" + + local encoded = encodeInstance(stringValue, "PARENT_ID") + + expect(encoded).to.be.ok() + expect(encoded.name).to.equal("TestValue") + expect(encoded.className).to.equal("StringValue") + + stringValue:Destroy() + end) + + it("should encode Configuration", function() + local config = Instance.new("Configuration") + config.Name = "TestConfig" + + local encoded = encodeInstance(config, "PARENT_ID") + + expect(encoded).to.be.ok() + expect(encoded.className).to.equal("Configuration") + + config:Destroy() + end) + end) + + describe("parent ID handling", function() + it("should include parent ID in all encodings", function() + local instances = { + Instance.new("ModuleScript"), + Instance.new("Folder"), + Instance.new("Part"), + } + + for _, instance in instances do + local encoded = encodeInstance(instance, "UNIQUE_PARENT_ID") + expect(encoded).to.be.ok() + expect(encoded.parent).to.equal("UNIQUE_PARENT_ID") + instance:Destroy() + end + end) + end) + + describe("edge cases", function() + it("should handle instances with special characters in name", function() + local folder = Instance.new("Folder") + folder.Name = "Test Folder (1)" + + local encoded = encodeInstance(folder, "PARENT") + + expect(encoded).to.be.ok() + expect(encoded.name).to.equal("Test Folder (1)") + + folder:Destroy() + end) + + it("should handle instances with empty name", function() + local folder = Instance.new("Folder") + folder.Name = "" + + local encoded = encodeInstance(folder, "PARENT") + + expect(encoded).to.be.ok() + expect(encoded.name).to.equal("") + + folder:Destroy() + end) + end) + end) +end diff --git a/plugin/src/ChangeBatcher/init.lua b/plugin/src/ChangeBatcher/init.lua index 19af2ed2..4c0d7956 100644 --- a/plugin/src/ChangeBatcher/init.lua +++ b/plugin/src/ChangeBatcher/init.lua @@ -28,6 +28,7 @@ function ChangeBatcher.new(instanceMap, onChangesFlushed) __onChangesFlushed = onChangesFlushed, __pendingPropertyChanges = {}, __syncSourceOnly = false, + __paused = false, }, ChangeBatcher) return self @@ -37,6 +38,21 @@ function ChangeBatcher:setSyncSourceOnly(enabled) self.__syncSourceOnly = enabled end +-- Pause the batcher to prevent change accumulation during confirmation +function ChangeBatcher:pause() + self.__paused = true +end + +-- Resume the batcher after confirmation +function ChangeBatcher:resume() + self.__paused = false +end + +-- Check if the batcher is paused +function ChangeBatcher:isPaused() + return self.__paused +end + function ChangeBatcher:stop() self.__renderSteppedConnection:Disconnect() self.__pendingPropertyChanges = {} @@ -54,6 +70,11 @@ function ChangeBatcher:add(instance, propertyName) end function ChangeBatcher:__cycle(dt) + -- Skip processing when paused (during confirmation dialogue) + if self.__paused then + return + end + self.__accumulator += dt if self.__accumulator >= BATCH_INTERVAL then diff --git a/plugin/src/ChangeBatcher/init.spec.lua b/plugin/src/ChangeBatcher/init.spec.lua index 21173233..7e66da4c 100644 --- a/plugin/src/ChangeBatcher/init.spec.lua +++ b/plugin/src/ChangeBatcher/init.spec.lua @@ -98,4 +98,74 @@ return function() expect(changeBatcher:__flush()).to.equal(nil) end) end) + + describe("pause/resume", function() + it("should start unpaused by default", function() + local changeBatcher = ChangeBatcher.new(InstanceMap.new(), noop) + expect(changeBatcher:isPaused()).to.equal(false) + changeBatcher:stop() + end) + + it("should be pausable", function() + local changeBatcher = ChangeBatcher.new(InstanceMap.new(), noop) + changeBatcher:pause() + expect(changeBatcher:isPaused()).to.equal(true) + changeBatcher:stop() + end) + + it("should be resumable after pause", function() + local changeBatcher = ChangeBatcher.new(InstanceMap.new(), noop) + changeBatcher:pause() + expect(changeBatcher:isPaused()).to.equal(true) + changeBatcher:resume() + expect(changeBatcher:isPaused()).to.equal(false) + changeBatcher:stop() + end) + + it("should not flush when paused", function() + local flushed = false + local instanceMap = InstanceMap.new() + local changeBatcher = ChangeBatcher.new(instanceMap, function() + flushed = true + end) + + local part = Instance.new("Part") + instanceMap:insert("PART", part) + changeBatcher.__pendingPropertyChanges[part] = { Name = true } + + changeBatcher:pause() + + -- Call __cycle with a large dt to trigger flush (normally 0.2s threshold) + changeBatcher:__cycle(1.0) + + -- Should not have flushed because we're paused + expect(flushed).to.equal(false) + + changeBatcher:stop() + part:Destroy() + end) + + it("should flush after resume", function() + local flushed = false + local instanceMap = InstanceMap.new() + local changeBatcher = ChangeBatcher.new(instanceMap, function() + flushed = true + end) + + local part = Instance.new("Part") + instanceMap:insert("PART", part) + changeBatcher.__pendingPropertyChanges[part] = { Name = true } + + changeBatcher:pause() + changeBatcher:__cycle(1.0) + expect(flushed).to.equal(false) + + changeBatcher:resume() + changeBatcher:__cycle(1.0) + expect(flushed).to.equal(true) + + changeBatcher:stop() + part:Destroy() + end) + end) end diff --git a/plugin/src/PatchTree.lua b/plugin/src/PatchTree.lua index d788473b..b618fd2f 100644 --- a/plugin/src/PatchTree.lua +++ b/plugin/src/PatchTree.lua @@ -313,7 +313,7 @@ function PatchTree.build(patch, instanceMap, changeListHeaders) end -- Add this node to tree - -- For Edit nodes, default to "push" (apply Rojo changes to Studio) + -- Default to nil (unselected) - user must explicitly choose Push/Pull/Skip tree:addNode(instanceMap.fromInstances[instance.Parent], { id = change.id, patchType = "Edit", @@ -322,7 +322,7 @@ function PatchTree.build(patch, instanceMap, changeListHeaders) instance = instance, changeInfo = changeInfo, changeList = changeList, - defaultSelection = "push", + defaultSelection = nil, }) end Timer.stop() @@ -366,7 +366,7 @@ function PatchTree.build(patch, instanceMap, changeListHeaders) tree:buildAncestryNodes(previousId, ancestryIds, patch, instanceMap) -- Add this node to tree - -- For Remove nodes, default to "push" (apply Rojo removal to Studio) + -- Default to nil (unselected) - user must explicitly choose Push/Pull/Skip local nodeId = instanceMap.fromInstances[instance] or HttpService:GenerateGUID(false) instanceMap:insert(nodeId, instance) tree:addNode(instanceMap.fromInstances[instance.Parent], { @@ -375,7 +375,7 @@ function PatchTree.build(patch, instanceMap, changeListHeaders) className = instance.ClassName, name = instance.Name, instance = instance, - defaultSelection = "push", + defaultSelection = nil, }) end Timer.stop() @@ -444,7 +444,7 @@ function PatchTree.build(patch, instanceMap, changeListHeaders) end -- Add this node to tree - -- For Add nodes, default to "push" (apply Rojo addition to Studio) + -- Default to nil (unselected) - user must explicitly choose Push/Pull/Skip tree:addNode(change.Parent, { id = change.Id, patchType = "Add", @@ -453,7 +453,7 @@ function PatchTree.build(patch, instanceMap, changeListHeaders) changeInfo = changeInfo, changeList = changeList, instance = instanceMap.fromIds[id], - defaultSelection = "push", + defaultSelection = nil, }) end Timer.stop() @@ -474,6 +474,18 @@ function PatchTree.buildInitialSelections(tree) return selections end +-- Gets the count of unselected items (nodes with patchType but no selection) +-- Returns count of unselected items +function PatchTree.countUnselected(tree, selections) + local count = 0 + tree:forEach(function(node) + if node.patchType and selections[node.id] == nil then + count += 1 + end + end) + return count +end + -- Gets all selectable node IDs (nodes that have a patchType) -- Returns an array of node IDs function PatchTree.getSelectableNodeIds(tree) diff --git a/plugin/src/PatchTree.spec.lua b/plugin/src/PatchTree.spec.lua new file mode 100644 index 00000000..46d13ef0 --- /dev/null +++ b/plugin/src/PatchTree.spec.lua @@ -0,0 +1,303 @@ +return function() + local PatchTree = require(script.Parent.PatchTree) + local PatchSet = require(script.Parent.PatchSet) + local InstanceMap = require(script.Parent.InstanceMap) + local HttpService = game:GetService("HttpService") + + describe("build", function() + it("should create a tree from an empty patch", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + + local tree = PatchTree.build(patch, instanceMap, {}) + + expect(tree).to.be.ok() + instanceMap:stop() + end) + end) + + describe("buildInitialSelections", function() + it("should return empty table for empty tree", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + local tree = PatchTree.build(patch, instanceMap, {}) + + local selections = PatchTree.buildInitialSelections(tree) + + expect(selections).to.be.ok() + expect(next(selections)).to.equal(nil) + + instanceMap:stop() + end) + + it("should not include selections for nodes with nil defaultSelection", function() + -- This test verifies that the new behavior (defaultSelection = nil) + -- results in no automatic selections + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + + -- Create a simple workspace for testing + local workspace = game:GetService("Workspace") + local testFolder = Instance.new("Folder") + testFolder.Name = "PatchTreeTestFolder" + testFolder.Parent = workspace + + -- Add the folder to instance map + local folderId = HttpService:GenerateGUID(false) + instanceMap:insert(folderId, testFolder) + + -- Add an update to the patch + table.insert(patch.updated, { + id = folderId, + changedName = "NewName", + changedProperties = {}, + }) + + local tree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }) + local selections = PatchTree.buildInitialSelections(tree) + + -- With defaultSelection = nil, the selections table should be empty + -- (no auto-selection to "push") + expect(next(selections)).to.equal(nil) + + testFolder:Destroy() + instanceMap:stop() + end) + end) + + describe("countUnselected", function() + it("should return 0 for empty tree", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + local tree = PatchTree.build(patch, instanceMap, {}) + local selections = {} + + local count = PatchTree.countUnselected(tree, selections) + + expect(count).to.equal(0) + + instanceMap:stop() + end) + + it("should count nodes without selections", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + + -- Create test instances + local workspace = game:GetService("Workspace") + local testFolder = Instance.new("Folder") + testFolder.Name = "CountTest" + testFolder.Parent = workspace + + local id = HttpService:GenerateGUID(false) + instanceMap:insert(id, testFolder) + + table.insert(patch.updated, { + id = id, + changedName = "UpdatedName", + changedProperties = {}, + }) + + local tree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }) + local selections = {} -- No selections + + local count = PatchTree.countUnselected(tree, selections) + + -- Should have at least 1 unselected item + expect(count >= 1).to.equal(true) + + testFolder:Destroy() + instanceMap:stop() + end) + + it("should return 0 when all nodes are selected", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + + local workspace = game:GetService("Workspace") + local testFolder = Instance.new("Folder") + testFolder.Name = "AllSelectedTest" + testFolder.Parent = workspace + + local id = HttpService:GenerateGUID(false) + instanceMap:insert(id, testFolder) + + table.insert(patch.updated, { + id = id, + changedName = "Selected", + changedProperties = {}, + }) + + local tree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }) + + -- Select the node + local selections = { [id] = "push" } + + local count = PatchTree.countUnselected(tree, selections) + + expect(count).to.equal(0) + + testFolder:Destroy() + instanceMap:stop() + end) + + it("should correctly count multiple unselected nodes", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + + local workspace = game:GetService("Workspace") + local testContainer = Instance.new("Folder") + testContainer.Name = "MultiUnselectedTest" + testContainer.Parent = workspace + + -- Create multiple test instances + local ids = {} + for i = 1, 3 do + local folder = Instance.new("Folder") + folder.Name = "TestFolder" .. i + folder.Parent = testContainer + + local id = HttpService:GenerateGUID(false) + instanceMap:insert(id, folder) + table.insert(ids, id) + + table.insert(patch.updated, { + id = id, + changedName = "Updated" .. i, + changedProperties = {}, + }) + end + + local tree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }) + + -- Select only the first one + local selections = { [ids[1]] = "push" } + + local count = PatchTree.countUnselected(tree, selections) + + -- Should have 2 unselected (the ones we didn't select) + expect(count).to.equal(2) + + testContainer:Destroy() + instanceMap:stop() + end) + end) + + describe("allNodesSelected", function() + it("should return true for empty tree", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + local tree = PatchTree.build(patch, instanceMap, {}) + local selections = {} + + local result = PatchTree.allNodesSelected(tree, selections) + + expect(result).to.equal(true) + + instanceMap:stop() + end) + + it("should return false when some nodes are unselected", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + + local workspace = game:GetService("Workspace") + local testFolder = Instance.new("Folder") + testFolder.Name = "PartialSelectTest" + testFolder.Parent = workspace + + local id = HttpService:GenerateGUID(false) + instanceMap:insert(id, testFolder) + + table.insert(patch.updated, { + id = id, + changedName = "Updated", + changedProperties = {}, + }) + + local tree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }) + local selections = {} -- No selections + + local result = PatchTree.allNodesSelected(tree, selections) + + expect(result).to.equal(false) + + testFolder:Destroy() + instanceMap:stop() + end) + + it("should return true when all nodes are selected", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + + local workspace = game:GetService("Workspace") + local testFolder = Instance.new("Folder") + testFolder.Name = "AllSelectTest" + testFolder.Parent = workspace + + local id = HttpService:GenerateGUID(false) + instanceMap:insert(id, testFolder) + + table.insert(patch.updated, { + id = id, + changedName = "Updated", + changedProperties = {}, + }) + + local tree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }) + local selections = { [id] = "push" } + + local result = PatchTree.allNodesSelected(tree, selections) + + expect(result).to.equal(true) + + testFolder:Destroy() + instanceMap:stop() + end) + end) + + describe("getSelectableNodeIds", function() + it("should return empty array for empty tree", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + local tree = PatchTree.build(patch, instanceMap, {}) + + local ids = PatchTree.getSelectableNodeIds(tree) + + expect(#ids).to.equal(0) + + instanceMap:stop() + end) + + it("should return IDs of all selectable nodes", function() + local patch = PatchSet.newEmpty() + local instanceMap = InstanceMap.new() + + local workspace = game:GetService("Workspace") + local testContainer = Instance.new("Folder") + testContainer.Name = "GetSelectableTest" + testContainer.Parent = workspace + + local folder1 = Instance.new("Folder") + folder1.Name = "Folder1" + folder1.Parent = testContainer + + local id1 = HttpService:GenerateGUID(false) + instanceMap:insert(id1, folder1) + + table.insert(patch.updated, { + id = id1, + changedName = "Updated1", + changedProperties = {}, + }) + + local tree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }) + local ids = PatchTree.getSelectableNodeIds(tree) + + expect(#ids >= 1).to.equal(true) + + testContainer:Destroy() + instanceMap:stop() + end) + end) +end diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 34c10a1c..f2fa0677 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -13,6 +13,7 @@ local Timer = require(script.Parent.Timer) local ChangeBatcher = require(script.Parent.ChangeBatcher) local encodePatchUpdate = require(script.Parent.ChangeBatcher.encodePatchUpdate) +local encodeInstance = require(script.Parent.ChangeBatcher.encodeInstance) local InstanceMap = require(script.Parent.InstanceMap) local PatchSet = require(script.Parent.PatchSet) local Reconciler = require(script.Parent.Reconciler) @@ -119,6 +120,7 @@ function ServeSession.new(options) __initialSyncCompleteCallback = nil, __serverInfo = nil, __confirmingPatch = nil, + __isConfirming = false, -- Explicit confirmation state flag for defense-in-depth __connections = connections, __precommitCallbacks = {}, __postcommitCallbacks = {}, @@ -225,28 +227,42 @@ function ServeSession:__onWebSocketMessage(messagesPacket) end -- If we're already waiting for confirmation, merge into the patch being confirmed - -- UNLESS we're in One-shot mode, where we ignore all further changes + -- Changes flow into the dialogue even in one-shot mode so user can see them if self.__confirmingPatch ~= nil then - if Settings:get("oneShotSync") then - Log.info("One-shot mode: ignoring incoming changes while confirming (will be picked up on next connection)") - self.__apiContext:setMessageCursor(messagesPacket.messageCursor) - return + Log.trace("Already confirming, merging into current patch") + + -- Collect IDs of items that are being changed (for unselecting in UI) + local changedIds = {} + for _, update in ipairs(combinedPatch.updated) do + changedIds[update.id] = true + end + for _, removed in ipairs(combinedPatch.removed) do + local id = if type(removed) == "string" then removed else self.__instanceMap.fromInstances[removed] + if id then + changedIds[id] = true + end + end + for id in pairs(combinedPatch.added) do + changedIds[id] = true end - Log.trace("Already confirming, merging into current patch") PatchSet.merge(self.__confirmingPatch, combinedPatch, self.__instanceMap) - -- Notify the UI to update the displayed patch + -- Notify the UI to update the displayed patch and unselect changed items if self.__patchUpdateCallback ~= nil then - self.__patchUpdateCallback(self.__instanceMap, self.__confirmingPatch) + self.__patchUpdateCallback(self.__instanceMap, self.__confirmingPatch, changedIds) end self.__apiContext:setMessageCursor(messagesPacket.messageCursor) return end - -- Set confirming patch before spawning to prevent race condition + -- Set confirmation state flags before spawning to prevent race condition -- where another message arrives before the spawned thread starts + self.__isConfirming = true self.__confirmingPatch = combinedPatch + -- Pause ChangeBatcher during confirmation to prevent outgoing changes + self.__changeBatcher:pause() + -- Spawn a new thread to handle potentially yielding confirmation task.spawn(function() Log.trace("Processing WebSocket patch, callback exists: {}", self.__userConfirmCallback ~= nil) @@ -256,7 +272,13 @@ function ServeSession:__onWebSocketMessage(messagesPacket) if self.__userConfirmCallback ~= nil then userDecision = self.__userConfirmCallback(self.__instanceMap, combinedPatch, self.__serverInfo) end + + -- Clear confirmation state flags self.__confirmingPatch = nil + self.__isConfirming = false + + -- Resume ChangeBatcher after confirmation + self.__changeBatcher:resume() Log.trace("WebSocket patch decision: {}", userDecision) @@ -486,6 +508,13 @@ function ServeSession:__replaceInstances(idList) end function ServeSession:__applyPatch(patch) + -- Defense-in-depth: Prevent unexpected patch application during confirmation + -- The only valid case is applying the confirmingPatch itself after user approval + if self.__isConfirming and patch ~= self.__confirmingPatch then + Log.error("Attempted to apply unexpected patch while confirmation dialogue is open. This is a bug!") + return + end + local patchTimestamp = DateTime.now():FormatLocalTime("LTS", "en-us") local historyRecording = ChangeHistoryService:TryBeginRecording("Rojo: Patch " .. patchTimestamp) if not historyRecording then @@ -610,11 +639,21 @@ end function ServeSession:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo) local userDecision = "Accept" if self.__userConfirmCallback ~= nil then - -- Set confirming patch so WebSocket messages that arrive during - -- confirmation can be merged into this patch + -- Pause ChangeBatcher during confirmation to prevent change accumulation + self.__changeBatcher:pause() + + -- Set confirmation state flags + self.__isConfirming = true self.__confirmingPatch = catchUpPatch + userDecision = self.__userConfirmCallback(self.__instanceMap, catchUpPatch, serverInfo) + + -- Clear confirmation state flags self.__confirmingPatch = nil + self.__isConfirming = false + + -- Resume ChangeBatcher after confirmation + self.__changeBatcher:resume() end if userDecision == "Abort" then @@ -660,17 +699,42 @@ function ServeSession:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo) -- "ignore" items are skipped end - -- Process removed items + -- Process removed items (instances in Studio that don't exist in Rojo) for _, idOrInstance in catchUpPatch.removed do + -- For removed items, idOrInstance is the Studio Instance + local instance = if type(idOrInstance) == "Instance" + then idOrInstance + else self.__instanceMap.fromIds[idOrInstance] + + -- Try to get an ID for selection lookup local id = if type(idOrInstance) == "string" then idOrInstance else self.__instanceMap.fromInstances[idOrInstance] - local selection = selections[id] + + -- If we don't have an ID, use the instance itself as the key + local selection = if id then selections[id] else selections[idOrInstance] + if selection == "push" then - -- Apply Rojo removal to Studio + -- Apply Rojo removal to Studio (delete the instance) table.insert(pushPatch.removed, idOrInstance) + elseif selection == "pull" and self.__twoWaySync then + -- Syncback: Create file in Rojo from Studio instance + if instance and instance.Parent then + local parentId = self.__instanceMap.fromInstances[instance.Parent] + if parentId then + local encoded = encodeInstance(instance, parentId) + if encoded then + -- Generate a temporary ref for the new instance + local tempRef = tostring(instance:GetDebugId()) + pullPatch.added[tempRef] = encoded + Log.info("Syncback: encoding {:?} for creation in Rojo", instance) + end + else + Log.warn("Cannot syncback {:?}: parent not in Rojo tree", instance) + end + end end - -- "pull" skipped: can't easily "add back" to Rojo, same as "ignore" + -- "skip"/"ignore" items are skipped end -- Process added items diff --git a/rbx-dom b/rbx-dom index 37181a83..fe943221 160000 --- a/rbx-dom +++ b/rbx-dom @@ -1 +1 @@ -Subproject commit 37181a839974dff431dfed74561601bb52c51261 +Subproject commit fe9432211c40f335bb206f63d50077c071d39bd7 diff --git a/rojo-test/serve-tests/syncback_write/default.project.json b/rojo-test/serve-tests/syncback_write/default.project.json new file mode 100644 index 00000000..e9ae7e7e --- /dev/null +++ b/rojo-test/serve-tests/syncback_write/default.project.json @@ -0,0 +1,10 @@ +{ + "name": "syncback_write", + "tree": { + "$className": "DataModel", + "ReplicatedStorage": { + "$className": "ReplicatedStorage", + "$path": "src" + } + } +} diff --git a/rojo-test/serve-tests/syncback_write/src/existing.luau b/rojo-test/serve-tests/syncback_write/src/existing.luau new file mode 100644 index 00000000..3dfa7812 --- /dev/null +++ b/rojo-test/serve-tests/syncback_write/src/existing.luau @@ -0,0 +1,2 @@ +-- Existing module for testing +return {} diff --git a/src/web/api.rs b/src/web/api.rs index 0e5f46d6..bec86a0c 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -32,6 +32,66 @@ use crate::{ web_api::{InstanceUpdate, RefPatchResponse, SerializeResponse}, }; +/// Convert a Variant to a JSON-compatible value for .model.json files +fn variant_to_json(variant: &Variant) -> Option { + use serde_json::{json, Value}; + + match variant { + Variant::String(s) => Some(Value::String(s.clone())), + Variant::Bool(b) => Some(Value::Bool(*b)), + Variant::Int32(n) => Some(json!(n)), + Variant::Int64(n) => Some(json!(n)), + Variant::Float32(n) => Some(json!(n)), + Variant::Float64(n) => Some(json!(n)), + Variant::Vector2(v) => Some(json!([v.x, v.y])), + Variant::Vector3(v) => Some(json!([v.x, v.y, v.z])), + Variant::Color3(c) => Some(json!([c.r, c.g, c.b])), + Variant::Color3uint8(c) => Some(json!([c.r, c.g, c.b])), + Variant::CFrame(cf) => { + let pos = cf.position; + let ori = cf.orientation; + Some(json!({ + "position": [pos.x, pos.y, pos.z], + "orientation": [ + [ori.x.x, ori.x.y, ori.x.z], + [ori.y.x, ori.y.y, ori.y.z], + [ori.z.x, ori.z.y, ori.z.z] + ] + })) + } + Variant::UDim(u) => Some(json!({"scale": u.scale, "offset": u.offset})), + Variant::UDim2(u) => Some(json!({ + "x": {"scale": u.x.scale, "offset": u.x.offset}, + "y": {"scale": u.y.scale, "offset": u.y.offset} + })), + Variant::Enum(e) => Some(json!(e.to_u32())), + Variant::BrickColor(bc) => Some(json!(*bc as u16)), + Variant::NumberSequence(ns) => { + let keypoints: Vec<_> = ns + .keypoints + .iter() + .map(|kp| json!({"time": kp.time, "value": kp.value, "envelope": kp.envelope})) + .collect(); + Some(json!({"keypoints": keypoints})) + } + Variant::ColorSequence(cs) => { + let keypoints: Vec<_> = cs + .keypoints + .iter() + .map(|kp| json!({"time": kp.time, "color": [kp.color.r, kp.color.g, kp.color.b]})) + .collect(); + Some(json!({"keypoints": keypoints})) + } + Variant::NumberRange(nr) => Some(json!({"min": nr.min, "max": nr.max})), + Variant::Rect(r) => Some(json!({ + "min": [r.min.x, r.min.y], + "max": [r.max.x, r.max.y] + })), + // Skip complex types that don't serialize well to JSON + _ => None, + } +} + pub async fn call(serve_session: Arc, mut request: Request) -> Response { let service = ApiService::new(serve_session); @@ -160,6 +220,17 @@ impl ApiService { ); } + // Process added instances (syncback: create files from Studio instances) + for added in request.added.values() { + if let Err(err) = self.syncback_added_instance(added) { + log::warn!( + "Failed to syncback added instance '{}': {}", + added.name, + err + ); + } + } + let updated_instances = request .updated .into_iter() @@ -183,6 +254,609 @@ impl ApiService { msgpack_ok(WriteResponse { session_id }) } + /// Syncback an added instance by creating a file in the filesystem. + /// The file watcher will pick up the change and update Rojo's tree. + /// + /// This follows the same middleware selection logic as the dedicated syncback + /// system in `src/syncback/mod.rs` to ensure consistent behavior. + fn syncback_added_instance( + &self, + added: &crate::web::interface::AddedInstance, + ) -> anyhow::Result<()> { + use crate::syncback::validate_file_name; + use anyhow::Context; + + // Validate the instance name for filesystem safety + validate_file_name(&added.name).with_context(|| { + format!( + "Instance name '{}' is not valid for the filesystem", + added.name + ) + })?; + + // Find the parent instance to get its filesystem path + let tree = self.serve_session.tree(); + let parent_instance = tree + .get_instance(added.parent) + .context("Parent instance not found in Rojo tree")?; + + // Get the parent's filesystem path from its metadata + let parent_path = parent_instance + .metadata() + .instigating_source + .as_ref() + .map(|s| s.path()) + .context("Parent instance has no filesystem path (not synced from filesystem)")?; + + // Determine if parent is a directory or file + let parent_dir = if parent_path.is_dir() { + parent_path.to_path_buf() + } else { + parent_path + .parent() + .context("Could not get parent directory")? + .to_path_buf() + }; + + // Create the instance at the resolved path + self.syncback_instance_to_path(added, &parent_dir) + } + + /// Recursively syncback an instance and its children to the filesystem. + /// This is the internal implementation that handles the actual file creation. + fn syncback_instance_to_path( + &self, + added: &crate::web::interface::AddedInstance, + parent_dir: &std::path::Path, + ) -> anyhow::Result<()> { + use crate::syncback::validate_file_name; + use anyhow::Context; + + // Validate the instance name for filesystem safety + validate_file_name(&added.name).with_context(|| { + format!( + "Instance name '{}' is not valid for the filesystem", + added.name + ) + })?; + + let has_children = !added.children.is_empty(); + + // Determine the appropriate middleware/file format based on class name + // This matches the logic in src/syncback/mod.rs::get_best_middleware + // Scripts with children become directories with init files + match added.class_name.as_str() { + // Script types: .luau files, or directories with init files if has children + // Non-Source properties (like Attributes) go into init.meta.json5 + "ModuleScript" => { + let source = self.get_source_property(added); + if has_children { + let dir_path = parent_dir.join(&added.name); + fs::create_dir_all(&dir_path).with_context(|| { + format!("Failed to create directory: {}", dir_path.display()) + })?; + let init_path = dir_path.join("init.luau"); + fs::write(&init_path, source.as_bytes()).with_context(|| { + format!("Failed to write file: {}", init_path.display()) + })?; + // Write init.meta.json5 if there are non-Source properties (like Attributes) + self.write_script_meta_json_if_needed(&dir_path, added)?; + log::info!( + "Syncback: Created ModuleScript directory at {}", + dir_path.display() + ); + for child in &added.children { + self.syncback_instance_to_path(child, &dir_path)?; + } + } else { + let file_path = parent_dir.join(format!("{}.luau", added.name)); + fs::write(&file_path, source.as_bytes()).with_context(|| { + format!("Failed to write file: {}", file_path.display()) + })?; + // Create adjacent meta file if there are non-Source properties + self.write_adjacent_script_meta_if_needed(parent_dir, &added.name, added)?; + log::info!("Syncback: Created ModuleScript at {}", file_path.display()); + } + } + "Script" => { + let source = self.get_source_property(added); + // Determine script type based on RunContext property + // This ensures proper round-tripping regardless of emitLegacyScripts mode + let script_suffix = self.get_script_suffix_for_run_context(added); + + if has_children { + let dir_path = parent_dir.join(&added.name); + fs::create_dir_all(&dir_path).with_context(|| { + format!("Failed to create directory: {}", dir_path.display()) + })?; + let init_path = dir_path.join(format!("init.{}.luau", script_suffix)); + fs::write(&init_path, source.as_bytes()).with_context(|| { + format!("Failed to write file: {}", init_path.display()) + })?; + self.write_script_meta_json_if_needed(&dir_path, added)?; + log::info!( + "Syncback: Created Script directory at {}", + dir_path.display() + ); + for child in &added.children { + self.syncback_instance_to_path(child, &dir_path)?; + } + } else { + let file_path = + parent_dir.join(format!("{}.{}.luau", added.name, script_suffix)); + fs::write(&file_path, source.as_bytes()).with_context(|| { + format!("Failed to write file: {}", file_path.display()) + })?; + // Create adjacent meta file if there are non-Source properties + self.write_adjacent_script_meta_if_needed(parent_dir, &added.name, added)?; + log::info!("Syncback: Created Script at {}", file_path.display()); + } + } + "LocalScript" => { + let source = self.get_source_property(added); + if has_children { + let dir_path = parent_dir.join(&added.name); + fs::create_dir_all(&dir_path).with_context(|| { + format!("Failed to create directory: {}", dir_path.display()) + })?; + let init_path = dir_path.join("init.client.luau"); + fs::write(&init_path, source.as_bytes()).with_context(|| { + format!("Failed to write file: {}", init_path.display()) + })?; + self.write_script_meta_json_if_needed(&dir_path, added)?; + log::info!( + "Syncback: Created LocalScript directory at {}", + dir_path.display() + ); + for child in &added.children { + self.syncback_instance_to_path(child, &dir_path)?; + } + } else { + let file_path = parent_dir.join(format!("{}.client.luau", added.name)); + fs::write(&file_path, source.as_bytes()).with_context(|| { + format!("Failed to write file: {}", file_path.display()) + })?; + // Create adjacent meta file if there are non-Source properties + self.write_adjacent_script_meta_if_needed(parent_dir, &added.name, added)?; + log::info!("Syncback: Created LocalScript at {}", file_path.display()); + } + } + + // Directory-native classes: always become directories + "Folder" | "Configuration" | "Tool" | "ScreenGui" | "SurfaceGui" | "BillboardGui" + | "AdGui" => { + let dir_path = parent_dir.join(&added.name); + fs::create_dir_all(&dir_path).with_context(|| { + format!("Failed to create directory: {}", dir_path.display()) + })?; + + // Write init.meta.json5 if has ANY properties (Attributes, Tags, etc.) + let has_metadata = if added.properties.is_empty() { + false + } else { + self.write_directory_meta_json(&dir_path, added)?; + true + }; + + // Add .gitkeep for empty directories with no metadata (matches dedicated syncback) + if added.children.is_empty() && !has_metadata { + fs::write(dir_path.join(".gitkeep"), b"") + .with_context(|| "Failed to write .gitkeep")?; + } + + log::info!( + "Syncback: Created {} directory at {}", + added.class_name, + dir_path.display() + ); + + // Recursively process children + for child in &added.children { + self.syncback_instance_to_path(child, &dir_path)?; + } + } + + // StringValue: .txt file if no children, directory with init.meta.json5 if has children + "StringValue" => { + if has_children { + // Must become directory - store StringValue data in init.meta.json5 + let dir_path = parent_dir.join(&added.name); + fs::create_dir_all(&dir_path).with_context(|| { + format!("Failed to create directory: {}", dir_path.display()) + })?; + self.write_init_meta_json(&dir_path, added)?; + log::info!( + "Syncback: Created StringValue directory at {}", + dir_path.display() + ); + for child in &added.children { + self.syncback_instance_to_path(child, &dir_path)?; + } + } else { + let value = added + .properties + .get("Value") + .and_then(|v| match v { + Variant::String(s) => Some(s.clone()), + _ => None, + }) + .unwrap_or_default(); + let file_path = parent_dir.join(format!("{}.txt", added.name)); + fs::write(&file_path, value.as_bytes()).with_context(|| { + format!("Failed to write file: {}", file_path.display()) + })?; + log::info!("Syncback: Created StringValue at {}", file_path.display()); + } + } + + // LocalizationTable: .csv file if no children, directory with init.csv if has children + "LocalizationTable" => { + // TODO: Full CSV support would serialize the Contents property + let content = "Key,Source,Context,Example,en\n"; + + if has_children { + // Must become directory with init.csv + let dir_path = parent_dir.join(&added.name); + fs::create_dir_all(&dir_path).with_context(|| { + format!("Failed to create directory: {}", dir_path.display()) + })?; + let init_path = dir_path.join("init.csv"); + fs::write(&init_path, content.as_bytes()).with_context(|| { + format!("Failed to write file: {}", init_path.display()) + })?; + log::info!( + "Syncback: Created LocalizationTable directory at {}", + dir_path.display() + ); + for child in &added.children { + self.syncback_instance_to_path(child, &dir_path)?; + } + } else { + let file_path = parent_dir.join(format!("{}.csv", added.name)); + fs::write(&file_path, content.as_bytes()).with_context(|| { + format!("Failed to write file: {}", file_path.display()) + })?; + log::info!( + "Syncback: Created LocalizationTable at {}", + file_path.display() + ); + } + } + + // All other classes -> .model.json5 or directory if has children + _ => { + if has_children { + // Create directory and put the instance as init.meta.json5 + let dir_path = parent_dir.join(&added.name); + fs::create_dir_all(&dir_path).with_context(|| { + format!("Failed to create directory: {}", dir_path.display()) + })?; + + // Write init.meta.json5 with class and properties + self.write_init_meta_json(&dir_path, added)?; + + log::info!( + "Syncback: Created {} directory at {} with init.meta.json5", + added.class_name, + dir_path.display() + ); + + // Recursively process children + for child in &added.children { + self.syncback_instance_to_path(child, &dir_path)?; + } + } else { + let content = self.serialize_instance_to_model_json(added)?; + // Use .model.json5 to match dedicated syncback (extension_for_middleware) + let file_path = parent_dir.join(format!("{}.model.json5", added.name)); + fs::write(&file_path, &content).with_context(|| { + format!("Failed to write file: {}", file_path.display()) + })?; + log::info!( + "Syncback: Created {} at {}", + added.class_name, + file_path.display() + ); + } + } + } + + Ok(()) + } + + /// Write init.meta.json5 for script directories if they have non-Source properties + /// This matches the behavior of the dedicated syncback system. + fn write_script_meta_json_if_needed( + &self, + dir_path: &std::path::Path, + added: &crate::web::interface::AddedInstance, + ) -> anyhow::Result<()> { + use anyhow::Context; + + // Check if there are any properties besides Source that need to be saved + // Use IndexMap for consistent ordering (like dedicated syncback) + let (properties, attributes) = + self.filter_properties_for_meta(&added.properties, Some("Source")); + + if properties.is_empty() && attributes.is_empty() { + return Ok(()); + } + + let meta = self.build_meta_object(None, properties, attributes); + let meta_path = dir_path.join("init.meta.json5"); + let content = crate::json::to_vec_pretty_sorted(&meta) + .context("Failed to serialize init.meta.json5")?; + fs::write(&meta_path, &content) + .with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?; + log::info!( + "Syncback: Created init.meta.json5 for script at {}", + meta_path.display() + ); + + Ok(()) + } + + /// Write init.meta.json5 for directory-native classes (Folder, Configuration, etc.) + /// Includes all properties (Attributes, Tags, etc.) + /// This matches the behavior of the dedicated syncback system. + fn write_directory_meta_json( + &self, + dir_path: &std::path::Path, + added: &crate::web::interface::AddedInstance, + ) -> anyhow::Result<()> { + use anyhow::Context; + + let (properties, attributes) = self.filter_properties_for_meta(&added.properties, None); + + if properties.is_empty() && attributes.is_empty() { + return Ok(()); + } + + let meta = self.build_meta_object(None, properties, attributes); + let meta_path = dir_path.join("init.meta.json5"); + let content = crate::json::to_vec_pretty_sorted(&meta) + .context("Failed to serialize init.meta.json5")?; + fs::write(&meta_path, &content) + .with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?; + log::info!( + "Syncback: Created init.meta.json5 at {}", + meta_path.display() + ); + + Ok(()) + } + + /// Write an init.meta.json5 file for non-standard instances that have children + /// This matches the behavior of the dedicated syncback system. + fn write_init_meta_json( + &self, + dir_path: &std::path::Path, + added: &crate::web::interface::AddedInstance, + ) -> anyhow::Result<()> { + use anyhow::Context; + + let (properties, attributes) = self.filter_properties_for_meta(&added.properties, None); + + // For non-Folder classes, we need to include className + let meta = self.build_meta_object(Some(&added.class_name), properties, attributes); + let meta_path = dir_path.join("init.meta.json5"); + let content = crate::json::to_vec_pretty_sorted(&meta) + .context("Failed to serialize init.meta.json5")?; + fs::write(&meta_path, &content) + .with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?; + log::info!( + "Syncback: Created init.meta.json5 for {} at {}", + added.class_name, + meta_path.display() + ); + + Ok(()) + } + + /// Filter properties for meta file serialization, matching dedicated syncback behavior. + /// Returns (properties, attributes) as separate IndexMaps. + /// + /// Filters out: + /// - Ref and UniqueId properties (don't serialize to JSON) + /// - The skip_property if specified (e.g., "Source" for scripts) + /// - Internal RBX* prefixed attributes + fn filter_properties_for_meta( + &self, + props: &HashMap, + skip_property: Option<&str>, + ) -> ( + indexmap::IndexMap, + indexmap::IndexMap, + ) { + use indexmap::IndexMap; + + let mut properties: IndexMap = IndexMap::new(); + let mut attributes: IndexMap = IndexMap::new(); + + for (name, value) in props { + // Skip the specified property (e.g., Source for scripts) + if let Some(skip) = skip_property { + if name == skip { + continue; + } + } + + // Skip Ref and UniqueId - they don't serialize to JSON (matches dedicated syncback) + if matches!(value, Variant::Ref(_) | Variant::UniqueId(_)) { + continue; + } + + // Handle Attributes specially - extract into separate map and filter RBX* prefix + if let Variant::Attributes(attrs) = value { + for (attr_name, attr_value) in attrs.iter() { + // Skip internal Roblox attributes (matches dedicated syncback) + if attr_name.starts_with("RBX") { + continue; + } + if let Some(json_value) = variant_to_json(attr_value) { + attributes.insert(attr_name.clone(), json_value); + } + } + continue; + } + + // Convert other properties + if let Some(json_value) = variant_to_json(value) { + properties.insert(name.clone(), json_value); + } + } + + (properties, attributes) + } + + /// Build a meta object (for init.meta.json5) matching dedicated syncback format. + fn build_meta_object( + &self, + class_name: Option<&str>, + properties: indexmap::IndexMap, + attributes: indexmap::IndexMap, + ) -> serde_json::Value { + use serde_json::json; + + let mut obj = serde_json::Map::new(); + + if let Some(cn) = class_name { + obj.insert("className".to_string(), json!(cn)); + } + + if !properties.is_empty() { + obj.insert("properties".to_string(), json!(properties)); + } + + if !attributes.is_empty() { + obj.insert("attributes".to_string(), json!(attributes)); + } + + serde_json::Value::Object(obj) + } + + /// Extract the Source property from an added instance, defaulting to empty string. + fn get_source_property(&self, added: &crate::web::interface::AddedInstance) -> String { + added + .properties + .get("Source") + .and_then(|v| match v { + Variant::String(s) => Some(s.clone()), + _ => None, + }) + .unwrap_or_default() + } + + /// Determine the script file suffix based on RunContext property. + /// + /// For `Script` class: + /// - RunContext: Client → "client" + /// - RunContext: Server → "server" + /// - RunContext: Legacy → "server" (default for legacy mode) + /// - RunContext: Plugin → "plugin" + /// - No RunContext → "server" (default) + /// + /// This ensures proper round-tripping regardless of emitLegacyScripts mode. + fn get_script_suffix_for_run_context( + &self, + added: &crate::web::interface::AddedInstance, + ) -> &'static str { + // Get RunContext enum values from reflection database + let run_context_enums = rbx_reflection_database::get() + .ok() + .and_then(|db| db.enums.get("RunContext")) + .map(|e| &e.items); + + let run_context_value = added.properties.get("RunContext").and_then(|v| match v { + Variant::Enum(e) => Some(e.to_u32()), + _ => None, + }); + + if let (Some(enums), Some(value)) = (run_context_enums, run_context_value) { + // Find which RunContext this value corresponds to + for (name, &enum_value) in enums { + if enum_value == value { + return match name.as_ref() { + "Client" => "client", + "Server" => "server", + "Legacy" => "server", + "Plugin" => "plugin", + _ => "server", + }; + } + } + } + + // Default to server if no RunContext or unrecognized + "server" + } + + /// Write an adjacent meta file for scripts without children. + /// + /// Creates `{name}.meta.json5` next to the script file if there are + /// non-Source properties that need to be preserved. + /// + /// This matches the dedicated syncback behavior in lua.rs::syncback_lua. + fn write_adjacent_script_meta_if_needed( + &self, + parent_dir: &std::path::Path, + name: &str, + added: &crate::web::interface::AddedInstance, + ) -> anyhow::Result<()> { + use anyhow::Context; + + // Filter properties, skipping Source (it's in the .luau file) + let (properties, attributes) = + self.filter_properties_for_meta(&added.properties, Some("Source")); + + if properties.is_empty() && attributes.is_empty() { + return Ok(()); + } + + // Build meta object (no className needed for scripts - it's determined by file extension) + let meta = self.build_meta_object(None, properties, attributes); + let meta_path = parent_dir.join(format!("{}.meta.json5", name)); + let content = + crate::json::to_vec_pretty_sorted(&meta).context("Failed to serialize meta.json5")?; + fs::write(&meta_path, &content) + .with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?; + log::info!( + "Syncback: Created adjacent meta file at {}", + meta_path.display() + ); + + Ok(()) + } + + /// Serialize an instance to model.json5 format (human-readable, version-control friendly) + /// This matches the format used by the dedicated syncback system. + fn serialize_instance_to_model_json( + &self, + added: &crate::web::interface::AddedInstance, + ) -> anyhow::Result> { + use anyhow::Context; + use serde_json::json; + + // Filter properties matching dedicated syncback behavior + let (properties, attributes) = self.filter_properties_for_meta(&added.properties, None); + + // Build the JSON model structure + // Format: https://rojo.space/docs/v7/sync-details/#json-models + let mut model = serde_json::Map::new(); + model.insert("className".to_string(), json!(added.class_name)); + + if !properties.is_empty() { + model.insert("properties".to_string(), json!(properties)); + } + + if !attributes.is_empty() { + model.insert("attributes".to_string(), json!(attributes)); + } + + // Use sorted JSON5 serialization to match dedicated syncback + crate::json::to_vec_pretty_sorted(&model).context("Failed to serialize model.json5") + } + async fn handle_api_read(&self, request: Request) -> Response { let argument = &request.uri().path()["/api/read/".len()..]; let requested_ids: Result, _> = argument.split(',').map(Ref::from_str).collect(); @@ -774,3 +1448,1559 @@ fn instance_for_scripts_only<'a>( metadata, } } + +#[cfg(test)] +mod tests { + use super::*; + use rbx_dom_weak::types::{Color3, Enum, NumberRange, Rect, UDim, UDim2, Vector2, Vector3}; + + // Tests for variant_to_json function + mod variant_to_json_tests { + use super::*; + + #[test] + fn test_string() { + let variant = Variant::String("hello world".to_string()); + let result = variant_to_json(&variant); + assert!(result.is_some()); + assert_eq!(result.unwrap(), serde_json::json!("hello world")); + } + + #[test] + fn test_bool() { + assert_eq!( + variant_to_json(&Variant::Bool(true)), + Some(serde_json::json!(true)) + ); + assert_eq!( + variant_to_json(&Variant::Bool(false)), + Some(serde_json::json!(false)) + ); + } + + #[test] + fn test_int32() { + let variant = Variant::Int32(42); + let result = variant_to_json(&variant); + assert_eq!(result, Some(serde_json::json!(42))); + } + + #[test] + fn test_int64() { + let variant = Variant::Int64(9999999999i64); + let result = variant_to_json(&variant); + assert_eq!(result, Some(serde_json::json!(9999999999i64))); + } + + #[test] + fn test_float32() { + let variant = Variant::Float32(3.14); + let result = variant_to_json(&variant); + assert!(result.is_some()); + } + + #[test] + fn test_float64() { + let variant = Variant::Float64(2.71828); + let result = variant_to_json(&variant); + assert!(result.is_some()); + } + + #[test] + fn test_vector2() { + let variant = Variant::Vector2(Vector2::new(1.0, 2.0)); + let result = variant_to_json(&variant); + assert!(result.is_some()); + assert_eq!(result.unwrap(), serde_json::json!([1.0, 2.0])); + } + + #[test] + fn test_vector3() { + let variant = Variant::Vector3(Vector3::new(1.0, 2.0, 3.0)); + let result = variant_to_json(&variant); + assert!(result.is_some()); + assert_eq!(result.unwrap(), serde_json::json!([1.0, 2.0, 3.0])); + } + + #[test] + fn test_color3() { + let variant = Variant::Color3(Color3::new(1.0, 0.5, 0.0)); + let result = variant_to_json(&variant); + assert!(result.is_some()); + assert_eq!(result.unwrap(), serde_json::json!([1.0, 0.5, 0.0])); + } + + #[test] + fn test_udim() { + let variant = Variant::UDim(UDim::new(0.5, 100)); + let result = variant_to_json(&variant); + assert!(result.is_some()); + let json = result.unwrap(); + assert_eq!(json["scale"], 0.5); + assert_eq!(json["offset"], 100); + } + + #[test] + fn test_udim2() { + let variant = Variant::UDim2(UDim2::new(UDim::new(0.5, 10), UDim::new(0.25, 20))); + let result = variant_to_json(&variant); + assert!(result.is_some()); + let json = result.unwrap(); + assert_eq!(json["x"]["scale"], 0.5); + assert_eq!(json["x"]["offset"], 10); + assert_eq!(json["y"]["scale"], 0.25); + assert_eq!(json["y"]["offset"], 20); + } + + #[test] + fn test_enum() { + let variant = Variant::Enum(Enum::from_u32(2)); + let result = variant_to_json(&variant); + assert_eq!(result, Some(serde_json::json!(2))); + } + + #[test] + fn test_number_range() { + let variant = Variant::NumberRange(NumberRange::new(0.0, 100.0)); + let result = variant_to_json(&variant); + assert!(result.is_some()); + let json = result.unwrap(); + assert_eq!(json["min"], 0.0); + assert_eq!(json["max"], 100.0); + } + + #[test] + fn test_rect() { + let variant = + Variant::Rect(Rect::new(Vector2::new(0.0, 0.0), Vector2::new(100.0, 50.0))); + let result = variant_to_json(&variant); + assert!(result.is_some()); + let json = result.unwrap(); + assert_eq!(json["min"], serde_json::json!([0.0, 0.0])); + assert_eq!(json["max"], serde_json::json!([100.0, 50.0])); + } + + #[test] + fn test_unsupported_returns_none() { + // BinaryString is not directly supported in JSON + let variant = Variant::BinaryString(vec![1, 2, 3].into()); + let result = variant_to_json(&variant); + assert!(result.is_none()); + } + } + + // Tests for file naming conventions in syncback + mod syncback_file_naming_tests { + #[test] + fn test_script_file_extensions() { + // Test that script class names result in correct file extensions + let test_cases = vec![ + ("ModuleScript", "TestModule", "TestModule.luau"), + ("Script", "TestScript", "TestScript.server.luau"), + ("LocalScript", "TestClient", "TestClient.client.luau"), + ]; + + for (class_name, name, expected) in test_cases { + let file_name = match class_name { + "ModuleScript" => format!("{}.luau", name), + "Script" => format!("{}.server.luau", name), + "LocalScript" => format!("{}.client.luau", name), + _ => format!("{}.model.json", name), + }; + assert_eq!( + file_name, expected, + "File naming for {} '{}' is incorrect", + class_name, name + ); + } + } + + #[test] + fn test_folder_creates_directory() { + // Folders should not create .model.json files + let class_name = "Folder"; + let should_create_file = class_name != "Folder"; + assert!( + !should_create_file, + "Folder should create a directory, not a file" + ); + } + + #[test] + fn test_other_instances_create_model_json5() { + // Part, Frame, TextLabel, Model use .model.json5 (matching dedicated syncback) + // Configuration creates a directory instead + let json5_types = vec!["Part", "Frame", "TextLabel", "Model"]; + for class_name in json5_types { + let file_name = format!("Test.model.json5"); + assert!( + file_name.ends_with(".model.json5"), + "{} should create .model.json5 file", + class_name + ); + } + } + + #[test] + fn test_directory_classes() { + // These classes create directories, not files (matching dedicated syncback) + let dir_classes = vec![ + "Folder", + "Configuration", + "Tool", + "ScreenGui", + "SurfaceGui", + "BillboardGui", + "AdGui", + ]; + for class_name in dir_classes { + // These should create directories + assert!( + matches!( + class_name, + "Folder" + | "Configuration" + | "Tool" + | "ScreenGui" + | "SurfaceGui" + | "BillboardGui" + | "AdGui" + ), + "{} should create a directory", + class_name + ); + } + } + + #[test] + fn test_special_value_classes() { + // StringValue -> .txt, LocalizationTable -> .csv + assert_eq!( + format!("{}.txt", "TestValue"), + "TestValue.txt", + "StringValue should create .txt file" + ); + assert_eq!( + format!("{}.csv", "TestLocalization"), + "TestLocalization.csv", + "LocalizationTable should create .csv file" + ); + } + } + + // Tests for model.json structure + mod model_json_structure_tests { + use serde_json::json; + + #[test] + fn test_basic_structure() { + // Verify the expected JSON structure for model files + let model = json!({ + "className": "Part", + "properties": {} + }); + + assert!(model.get("className").is_some()); + assert!(model.get("properties").is_some()); + } + + #[test] + fn test_with_properties() { + let model = json!({ + "className": "Part", + "properties": { + "Size": [4.0, 1.0, 2.0], + "Color": [1.0, 0.0, 0.0], + "Anchored": true + } + }); + + let props = model.get("properties").unwrap(); + assert!(props.get("Size").is_some()); + assert!(props.get("Color").is_some()); + assert!(props.get("Anchored").is_some()); + } + } + + // Tests for RunContext-based script file suffix determination + mod run_context_tests { + + #[test] + fn test_run_context_client_gives_client_suffix() { + // Script with RunContext: Client should produce .client.luau + // RunContext::Client has value 2 in the Roblox enum + let run_context_enums = rbx_reflection_database::get() + .ok() + .and_then(|db| db.enums.get("RunContext")) + .map(|e| &e.items); + + if let Some(enums) = run_context_enums { + if let Some(&client_value) = enums.get("Client") { + // Verify the enum value exists + assert!(client_value > 0 || client_value == 0); + } + } + } + + #[test] + fn test_run_context_server_gives_server_suffix() { + // Script with RunContext: Server should produce .server.luau + let run_context_enums = rbx_reflection_database::get() + .ok() + .and_then(|db| db.enums.get("RunContext")) + .map(|e| &e.items); + + if let Some(enums) = run_context_enums { + if let Some(&server_value) = enums.get("Server") { + assert!(server_value > 0 || server_value == 0); + } + } + } + + #[test] + fn test_run_context_legacy_gives_server_suffix() { + // Script with RunContext: Legacy should produce .server.luau (for legacy mode) + let run_context_enums = rbx_reflection_database::get() + .ok() + .and_then(|db| db.enums.get("RunContext")) + .map(|e| &e.items); + + if let Some(enums) = run_context_enums { + if let Some(&legacy_value) = enums.get("Legacy") { + assert!(legacy_value > 0 || legacy_value == 0); + } + } + } + + #[test] + fn test_run_context_plugin_gives_plugin_suffix() { + // Script with RunContext: Plugin should produce .plugin.luau + let run_context_enums = rbx_reflection_database::get() + .ok() + .and_then(|db| db.enums.get("RunContext")) + .map(|e| &e.items); + + if let Some(enums) = run_context_enums { + if let Some(&plugin_value) = enums.get("Plugin") { + assert!(plugin_value > 0 || plugin_value == 0); + } + } + } + + #[test] + fn test_script_class_determines_suffix_from_run_context() { + // For Script class, the file suffix is determined by RunContext: + // - Client → .client.luau + // - Server → .server.luau + // - Legacy → .server.luau + // - Plugin → .plugin.luau + // This allows proper round-tripping regardless of emitLegacyScripts mode + + let expected_mappings = vec![ + ("Client", "client"), + ("Server", "server"), + ("Legacy", "server"), + ("Plugin", "plugin"), + ]; + + for (run_context, expected_suffix) in expected_mappings { + let suffix = match run_context { + "Client" => "client", + "Server" => "server", + "Legacy" => "server", + "Plugin" => "plugin", + _ => "server", + }; + assert_eq!( + suffix, expected_suffix, + "RunContext {} should produce {} suffix", + run_context, expected_suffix + ); + } + } + + #[test] + fn test_local_script_always_client() { + // LocalScript class always produces .client.luau regardless of any properties + let class_name = "LocalScript"; + let expected_suffix = "client"; + + // LocalScript doesn't check RunContext - it's always client + let suffix = if class_name == "LocalScript" { + "client" + } else { + "unknown" + }; + + assert_eq!(suffix, expected_suffix); + } + + #[test] + fn test_module_script_no_suffix() { + // ModuleScript produces .luau (no server/client suffix) + let class_name = "ModuleScript"; + + let expected_extension = if class_name == "ModuleScript" { + ".luau" + } else { + ".unknown" + }; + + assert_eq!(expected_extension, ".luau"); + } + } + + // Tests for adjacent meta file creation + mod adjacent_meta_tests { + #[test] + fn test_adjacent_meta_file_naming() { + // Adjacent meta files should be named {script_name}.meta.json5 + let script_name = "MyScript"; + let meta_filename = format!("{}.meta.json5", script_name); + assert_eq!(meta_filename, "MyScript.meta.json5"); + } + + #[test] + fn test_adjacent_meta_for_script_without_children() { + // Scripts without children should create adjacent meta files, not init.meta.json5 + let has_children = false; + let is_directory = has_children; + + // When it's not a directory, we use adjacent meta, not init.meta.json5 + let meta_type = if is_directory { + "init.meta.json5" + } else { + "{name}.meta.json5" + }; + + assert_eq!(meta_type, "{name}.meta.json5"); + } + + #[test] + fn test_init_meta_for_script_with_children() { + // Scripts with children (directories) use init.meta.json5 + let has_children = true; + let is_directory = has_children; + + let meta_type = if is_directory { + "init.meta.json5" + } else { + "{name}.meta.json5" + }; + + assert_eq!(meta_type, "init.meta.json5"); + } + } + + // Tests for property filtering (matching dedicated syncback) + mod property_filter_tests { + use super::*; + use rbx_dom_weak::types::{Attributes, Ref, UniqueId}; + + #[test] + fn test_ref_properties_filtered_out() { + // Ref properties should be filtered out (they don't serialize to JSON) + let variant = Variant::Ref(Ref::new()); + let result = variant_to_json(&variant); + // variant_to_json returns None for Ref + assert!(result.is_none(), "Ref should not serialize to JSON"); + } + + #[test] + fn test_unique_id_filtered_out() { + // UniqueId properties should be filtered out + let variant = Variant::UniqueId(UniqueId::new(0, 0, 0)); + let result = variant_to_json(&variant); + assert!(result.is_none(), "UniqueId should not serialize to JSON"); + } + + #[test] + fn test_attributes_handled_separately() { + // Attributes are NOT handled by variant_to_json directly. + // They are extracted into a separate "attributes" map by filter_properties_for_meta. + // This matches the dedicated syncback behavior where Attributes are a special case. + let mut attrs = Attributes::new(); + attrs.insert("TestAttr".to_string(), Variant::String("value".to_string())); + + let variant = Variant::Attributes(attrs); + let result = variant_to_json(&variant); + // variant_to_json returns None for Attributes - they're handled separately + assert!( + result.is_none(), + "Attributes should return None from variant_to_json (handled separately)" + ); + } + } + + // Tests for init.meta.json5 format (matching dedicated syncback) + mod meta_json5_tests { + use serde_json::json; + + #[test] + fn test_meta_uses_json5_extension() { + // Meta files should use .json5 extension (not .json) + let filename = "init.meta.json5"; + assert!( + filename.ends_with(".json5"), + "Meta files should use .json5 extension" + ); + } + + #[test] + fn test_meta_structure_for_directory() { + // Directory meta files should have properties and optionally attributes + let meta = json!({ + "properties": { + "Archivable": false + }, + "attributes": { + "CustomAttr": "value" + } + }); + + assert!(meta.get("properties").is_some()); + assert!(meta.get("attributes").is_some()); + // No className for standard Folders + assert!(meta.get("className").is_none()); + } + + #[test] + fn test_meta_structure_for_non_folder() { + // Non-Folder directories need className + let meta = json!({ + "className": "Part", + "properties": { + "Size": [4.0, 1.0, 2.0] + } + }); + + assert!(meta.get("className").is_some()); + assert_eq!(meta.get("className").unwrap(), "Part"); + } + } + + // Tests for children handling (any instance can have children) + mod children_handling_tests { + #[allow(unused_imports)] + use super::*; + + #[test] + fn test_script_with_children_becomes_directory() { + // When a script has children, it should become a directory with init file + let has_children = true; + let class_name = "ModuleScript"; + + // This is the expected behavior - directory + init.luau + if has_children && class_name == "ModuleScript" { + let init_file = "init.luau"; + assert_eq!(init_file, "init.luau"); + } + } + + #[test] + fn test_server_script_with_children() { + let has_children = true; + let class_name = "Script"; + + if has_children && class_name == "Script" { + let init_file = "init.server.luau"; + assert_eq!(init_file, "init.server.luau"); + } + } + + #[test] + fn test_client_script_with_children() { + let has_children = true; + let class_name = "LocalScript"; + + if has_children && class_name == "LocalScript" { + let init_file = "init.client.luau"; + assert_eq!(init_file, "init.client.luau"); + } + } + + #[test] + fn test_string_value_with_children() { + // StringValue with children becomes directory + init.meta.json5 + let has_children = true; + let class_name = "StringValue"; + + if has_children && class_name == "StringValue" { + // Should create directory with init.meta.json5, not .txt file + let meta_file = "init.meta.json5"; + assert!(meta_file.ends_with(".json5")); + } + } + + #[test] + fn test_part_with_children() { + // Part (or any other class) with children becomes directory + init.meta.json5 + let has_children = true; + let class_name = "Part"; + + if has_children && !matches!(class_name, "ModuleScript" | "Script" | "LocalScript") { + let meta_file = "init.meta.json5"; + assert!(meta_file.ends_with(".json5")); + } + } + } + + // Tests for .gitkeep behavior + mod gitkeep_tests { + #[test] + fn test_empty_folder_gets_gitkeep() { + // Empty folders with no metadata should get .gitkeep (matching dedicated syncback) + let has_children = false; + let has_metadata = false; + + let should_add_gitkeep = !has_children && !has_metadata; + assert!(should_add_gitkeep, "Empty folder should get .gitkeep"); + } + + #[test] + fn test_folder_with_metadata_no_gitkeep() { + // Folders with metadata should NOT get .gitkeep + let has_children = false; + let has_metadata = true; + + let should_add_gitkeep = !has_children && !has_metadata; + assert!( + !should_add_gitkeep, + "Folder with metadata should not get .gitkeep" + ); + } + + #[test] + fn test_folder_with_children_no_gitkeep() { + // Folders with children should NOT get .gitkeep + let has_children = true; + let has_metadata = false; + + let should_add_gitkeep = !has_children && !has_metadata; + assert!( + !should_add_gitkeep, + "Folder with children should not get .gitkeep" + ); + } + } + + // Tests for attributes handling (matching dedicated syncback) + mod attributes_tests { + use super::*; + use rbx_dom_weak::types::Attributes; + + #[test] + fn test_internal_attributes_filtered() { + // Internal attributes starting with RBX should be filtered out + let mut attrs = Attributes::new(); + attrs.insert( + "RBXInternal".to_string(), + Variant::String("internal".to_string()), + ); + attrs.insert( + "CustomAttr".to_string(), + Variant::String("custom".to_string()), + ); + + // Only CustomAttr should be kept + let mut count = 0; + for (name, _) in attrs.iter() { + if !name.starts_with("RBX") { + count += 1; + } + } + assert_eq!(count, 1, "Only non-RBX attributes should be kept"); + } + + #[test] + fn test_user_attributes_preserved() { + // User-defined attributes should be preserved + let mut attrs = Attributes::new(); + attrs.insert( + "MyAttribute".to_string(), + Variant::String("value".to_string()), + ); + attrs.insert("Score".to_string(), Variant::Int32(100)); + + let filtered: Vec<_> = attrs + .iter() + .filter(|(name, _)| !name.starts_with("RBX")) + .collect(); + + assert_eq!(filtered.len(), 2, "User attributes should be preserved"); + } + } + + // ========================================================================= + // Comprehensive RunContext Configuration Tests + // ========================================================================= + mod run_context_comprehensive_tests { + use super::*; + use rbx_dom_weak::types::Enum; + use std::collections::HashMap; + + /// Helper to get RunContext enum value by name + fn get_run_context_value(name: &str) -> Option { + rbx_reflection_database::get() + .ok() + .and_then(|db| db.enums.get("RunContext")) + .and_then(|e| e.items.get(name).copied()) + } + + /// Simulates suffix determination logic + fn determine_suffix_from_run_context(run_context_value: Option) -> &'static str { + let run_context_enums = rbx_reflection_database::get() + .ok() + .and_then(|db| db.enums.get("RunContext")) + .map(|e| &e.items); + + if let (Some(enums), Some(value)) = (run_context_enums, run_context_value) { + for (name, &enum_value) in enums { + if enum_value == value { + return match name.as_ref() { + "Client" => "client", + "Server" => "server", + "Legacy" => "server", + "Plugin" => "plugin", + _ => "server", + }; + } + } + } + "server" + } + + #[test] + fn test_all_run_context_values_exist() { + // Verify all expected RunContext values exist in the reflection database + let expected = ["Legacy", "Server", "Client", "Plugin"]; + for name in expected { + assert!( + get_run_context_value(name).is_some(), + "RunContext::{} should exist in reflection database", + name + ); + } + } + + #[test] + fn test_run_context_client_produces_client_suffix() { + let value = get_run_context_value("Client"); + assert!(value.is_some(), "Client RunContext should exist"); + let suffix = determine_suffix_from_run_context(value); + assert_eq!( + suffix, "client", + "RunContext::Client should produce 'client' suffix" + ); + } + + #[test] + fn test_run_context_server_produces_server_suffix() { + let value = get_run_context_value("Server"); + assert!(value.is_some(), "Server RunContext should exist"); + let suffix = determine_suffix_from_run_context(value); + assert_eq!( + suffix, "server", + "RunContext::Server should produce 'server' suffix" + ); + } + + #[test] + fn test_run_context_legacy_produces_server_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" + ); + } + + #[test] + fn test_run_context_plugin_produces_plugin_suffix() { + let value = get_run_context_value("Plugin"); + assert!(value.is_some(), "Plugin RunContext should exist"); + let suffix = determine_suffix_from_run_context(value); + assert_eq!( + suffix, "plugin", + "RunContext::Plugin should produce 'plugin' suffix" + ); + } + + #[test] + fn test_no_run_context_defaults_to_server() { + let suffix = determine_suffix_from_run_context(None); + assert_eq!( + suffix, "server", + "No RunContext should default to 'server' suffix" + ); + } + + #[test] + fn test_invalid_run_context_defaults_to_server() { + // 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" + ); + } + + #[test] + fn test_file_extension_patterns() { + // Verify expected file extension patterns + let test_cases = vec![ + ("ModuleScript", None, ".luau"), + ("LocalScript", None, ".client.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("Plugin"), ".plugin.luau"), + ("Script", None, ".server.luau"), + ]; + + for (class_name, run_context, expected_ext) in test_cases { + let suffix = match class_name { + "ModuleScript" => "", + "LocalScript" => "client", + "Script" => determine_suffix_from_run_context(run_context), + _ => panic!("Unknown class"), + }; + + let ext = if suffix.is_empty() { + ".luau".to_string() + } else { + format!(".{}.luau", suffix) + }; + + assert_eq!( + ext, expected_ext, + "{} with RunContext {:?} should produce {} extension", + class_name, run_context, expected_ext + ); + } + } + + #[test] + fn test_init_file_patterns_with_children() { + // Verify init file patterns for scripts with children + let test_cases = vec![ + ("ModuleScript", None, "init.luau"), + ("LocalScript", None, "init.client.luau"), + ( + "Script", + get_run_context_value("Server"), + "init.server.luau", + ), + ( + "Script", + get_run_context_value("Client"), + "init.client.luau", + ), + ( + "Script", + get_run_context_value("Legacy"), + "init.server.luau", + ), + ( + "Script", + get_run_context_value("Plugin"), + "init.plugin.luau", + ), + ]; + + for (class_name, run_context, expected_init) in test_cases { + let suffix = match class_name { + "ModuleScript" => "", + "LocalScript" => "client", + "Script" => determine_suffix_from_run_context(run_context), + _ => panic!("Unknown class"), + }; + + let init_file = if suffix.is_empty() { + "init.luau".to_string() + } else { + format!("init.{}.luau", suffix) + }; + + assert_eq!( + init_file, expected_init, + "{} with RunContext {:?} should produce {} init file", + class_name, run_context, expected_init + ); + } + } + } + + // ========================================================================= + // Scripts-Only Mode Filtering Tests + // ========================================================================= + mod scripts_only_mode_tests { + use super::*; + + #[test] + fn test_is_script_class_identifies_scripts() { + // Test that is_script_class correctly identifies script types + assert!( + is_script_class("Script"), + "Script should be identified as script" + ); + assert!( + is_script_class("LocalScript"), + "LocalScript should be identified as script" + ); + assert!( + is_script_class("ModuleScript"), + "ModuleScript should be identified as script" + ); + } + + #[test] + fn test_is_script_class_rejects_non_scripts() { + // Test that is_script_class correctly rejects non-script types + let non_scripts = [ + "Part", + "Folder", + "Model", + "Frame", + "TextLabel", + "IntValue", + "StringValue", + "Configuration", + "ScreenGui", + "Workspace", + "ReplicatedStorage", + ]; + + for class_name in non_scripts { + assert!( + !is_script_class(class_name), + "{} should NOT be identified as script", + class_name + ); + } + } + + #[test] + fn test_scripts_only_should_include_script() { + // In scripts-only mode, Script instances should be included + let class_name = "Script"; + let should_include = is_script_class(class_name); + assert!( + should_include, + "Script should be included in scripts-only mode" + ); + } + + #[test] + fn test_scripts_only_should_include_local_script() { + // In scripts-only mode, LocalScript instances should be included + let class_name = "LocalScript"; + let should_include = is_script_class(class_name); + assert!( + should_include, + "LocalScript should be included in scripts-only mode" + ); + } + + #[test] + fn test_scripts_only_should_include_module_script() { + // In scripts-only mode, ModuleScript instances should be included + let class_name = "ModuleScript"; + let should_include = is_script_class(class_name); + assert!( + should_include, + "ModuleScript should be included in scripts-only mode" + ); + } + + #[test] + fn test_scripts_only_should_exclude_part() { + // In scripts-only mode, Part instances should be excluded + let class_name = "Part"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "Part should NOT be included in scripts-only mode" + ); + } + + #[test] + fn test_scripts_only_should_exclude_folder() { + // In scripts-only mode, Folder instances should be excluded (unless ancestor) + let class_name = "Folder"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "Folder should NOT be directly included in scripts-only mode" + ); + } + + #[test] + fn test_scripts_only_should_exclude_model() { + // In scripts-only mode, Model instances should be excluded (unless ancestor) + let class_name = "Model"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "Model should NOT be directly included in scripts-only mode" + ); + } + + #[test] + fn test_scripts_only_should_exclude_gui_elements() { + // In scripts-only mode, GUI elements should be excluded + let gui_classes = [ + "Frame", + "TextLabel", + "TextButton", + "ImageLabel", + "ScreenGui", + ]; + for class_name in gui_classes { + let should_include = is_script_class(class_name); + assert!( + !should_include, + "{} should NOT be included in scripts-only mode", + class_name + ); + } + } + + #[test] + fn test_scripts_only_should_exclude_value_types() { + // In scripts-only mode, value types should be excluded + let value_classes = [ + "IntValue", + "StringValue", + "NumberValue", + "BoolValue", + "ObjectValue", + ]; + for class_name in value_classes { + let should_include = is_script_class(class_name); + assert!( + !should_include, + "{} should NOT be included in scripts-only mode", + class_name + ); + } + } + + #[test] + fn test_scripts_only_run_context_preserved() { + // In scripts-only mode, RunContext property on scripts should be preserved + // This is important for proper syncback behavior + let script_properties = ["Source", "RunContext", "Enabled", "Disabled"]; + let is_script = is_script_class("Script"); + assert!(is_script, "Script should be identified"); + + // In scripts-only mode, scripts get ALL their properties synced (including RunContext) + // Non-scripts get their properties cleared + // This test verifies the design decision + for prop in script_properties { + // Scripts should sync these properties + let should_sync = is_script; + assert!( + should_sync, + "Script property '{}' should be synced in scripts-only mode", + prop + ); + } + } + + #[test] + fn test_scripts_only_ancestor_properties_cleared() { + // In scripts-only mode, non-script ancestors have properties cleared + // They only provide tree structure, not property data + let folder_is_script = is_script_class("Folder"); + assert!(!folder_is_script, "Folder is not a script"); + + // Non-scripts should have properties cleared + let should_clear_properties = !folder_is_script; + assert!( + should_clear_properties, + "Non-script ancestors should have properties cleared" + ); + } + } + + // ========================================================================= + // Scripts-Only Mode - Expected Pass Tests + // ========================================================================= + mod scripts_only_expected_passes { + use super::*; + + #[test] + fn test_pass_module_script_sync() { + // PASS: ModuleScript should sync successfully in scripts-only mode + let class_name = "ModuleScript"; + assert!( + is_script_class(class_name), + "ModuleScript sync should PASS in scripts-only mode" + ); + } + + #[test] + fn test_pass_server_script_sync() { + // PASS: Script (server) should sync successfully in scripts-only mode + let class_name = "Script"; + assert!( + is_script_class(class_name), + "Script sync should PASS in scripts-only mode" + ); + } + + #[test] + fn test_pass_local_script_sync() { + // PASS: LocalScript should sync successfully in scripts-only mode + let class_name = "LocalScript"; + assert!( + is_script_class(class_name), + "LocalScript sync should PASS in scripts-only mode" + ); + } + + #[test] + fn test_pass_script_source_property() { + // PASS: Script Source property changes should sync + let class_name = "Script"; + let property_name = "Source"; + let is_script = is_script_class(class_name); + let should_sync_property = is_script; // Scripts sync all properties + assert!( + should_sync_property, + "Script.{} change should PASS in scripts-only mode", + property_name + ); + } + + #[test] + fn test_pass_script_run_context_property() { + // PASS: Script RunContext property changes should sync + // This is critical for proper emitLegacyScripts support + let class_name = "Script"; + let property_name = "RunContext"; + let is_script = is_script_class(class_name); + let should_sync_property = is_script; // Scripts sync all properties + assert!( + should_sync_property, + "Script.{} change should PASS in scripts-only mode", + property_name + ); + } + + #[test] + fn test_pass_script_enabled_property() { + // PASS: Script Enabled property changes should sync + let class_name = "Script"; + let property_name = "Enabled"; + let is_script = is_script_class(class_name); + let should_sync_property = is_script; + assert!( + should_sync_property, + "Script.{} change should PASS in scripts-only mode", + property_name + ); + } + + #[test] + fn test_pass_module_script_attributes() { + // PASS: ModuleScript Attributes should sync + let class_name = "ModuleScript"; + let is_script = is_script_class(class_name); + assert!( + is_script, + "ModuleScript.Attributes change should PASS in scripts-only mode" + ); + } + + #[test] + fn test_pass_nested_script_in_folder() { + // PASS: Script inside Folder should sync (along with Folder as ancestor) + let script_class = "ModuleScript"; + let folder_class = "Folder"; + + let script_included = is_script_class(script_class); + let folder_is_script = is_script_class(folder_class); + + // Script should be included directly + assert!(script_included, "Nested script should be included"); + // Folder should be included as ancestor (but not as a script) + assert!( + !folder_is_script, + "Folder is not a script but may be included as ancestor" + ); + } + } + + // ========================================================================= + // Scripts-Only Mode - Expected Failure/Filter Tests + // ========================================================================= + mod scripts_only_expected_filters { + use super::*; + + #[test] + fn test_filter_part_changes() { + // FILTER: Part changes should be filtered out in scripts-only mode + let class_name = "Part"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "Part changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_model_changes() { + // FILTER: Model changes should be filtered out in scripts-only mode + let class_name = "Model"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "Model changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_folder_property_changes() { + // FILTER: Folder property changes should be filtered (Folder is ancestor only) + let class_name = "Folder"; + let is_script = is_script_class(class_name); + // Folders have their properties cleared in scripts-only mode + assert!( + !is_script, + "Folder property changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_frame_changes() { + // FILTER: Frame (GUI) changes should be filtered + let class_name = "Frame"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "Frame changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_text_label_changes() { + // FILTER: TextLabel changes should be filtered + let class_name = "TextLabel"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "TextLabel changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_int_value_changes() { + // FILTER: IntValue changes should be filtered + let class_name = "IntValue"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "IntValue changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_string_value_changes() { + // FILTER: StringValue changes should be filtered + let class_name = "StringValue"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "StringValue changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_sound_changes() { + // FILTER: Sound changes should be filtered + let class_name = "Sound"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "Sound changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_particle_emitter_changes() { + // FILTER: ParticleEmitter changes should be filtered + let class_name = "ParticleEmitter"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "ParticleEmitter changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_configuration_changes() { + // FILTER: Configuration instance changes should be filtered + let class_name = "Configuration"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "Configuration changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_remote_event_changes() { + // FILTER: RemoteEvent changes should be filtered + let class_name = "RemoteEvent"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "RemoteEvent changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_remote_function_changes() { + // FILTER: RemoteFunction changes should be filtered + let class_name = "RemoteFunction"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "RemoteFunction changes should be FILTERED in scripts-only mode" + ); + } + + #[test] + fn test_filter_bindable_event_changes() { + // FILTER: BindableEvent changes should be filtered + let class_name = "BindableEvent"; + let should_include = is_script_class(class_name); + assert!( + !should_include, + "BindableEvent changes should be FILTERED in scripts-only mode" + ); + } + } + + // ========================================================================= + // Scripts-Only Mode - Ancestor Inclusion Tests + // ========================================================================= + mod scripts_only_ancestor_tests { + use super::*; + + #[test] + fn test_ancestor_chain_concept() { + // Test the concept of ancestor chain inclusion + // In scripts-only mode: + // - Scripts are included with all properties + // - Non-script ancestors are included for tree structure but properties cleared + + // Simulated tree: ReplicatedStorage > Folder > ModuleScript + let replicated_storage = "ReplicatedStorage"; + let folder = "Folder"; + let module_script = "ModuleScript"; + + // Only the script is identified as a script + assert!(!is_script_class(replicated_storage)); + assert!(!is_script_class(folder)); + assert!(is_script_class(module_script)); + + // But all three should be included in the message (for tree structure) + // The filtering function includes ancestors of scripts + } + + #[test] + fn test_non_script_ancestors_have_ignore_unknown_instances() { + // In scripts-only mode, all instances get ignoreUnknownInstances: true + // This prevents accidental deletion of non-script instances + let folder = "Folder"; + let is_script = is_script_class(folder); + + // Non-scripts should have ignoreUnknownInstances set + let should_set_ignore = true; // All instances in scripts-only mode + assert!( + should_set_ignore, + "Non-script ancestors should have ignoreUnknownInstances: true" + ); + } + + #[test] + fn test_scripts_also_have_ignore_unknown_instances() { + // Even scripts get ignoreUnknownInstances: true in scripts-only mode + // This is a safety measure + let script = "Script"; + let is_script = is_script_class(script); + assert!(is_script); + + // All instances in scripts-only mode get this flag + let should_set_ignore = true; + assert!( + should_set_ignore, + "Scripts should also have ignoreUnknownInstances: true in scripts-only mode" + ); + } + } + + // ========================================================================= + // Syncback Configuration Matrix Tests + // ========================================================================= + mod syncback_configuration_matrix { + use super::*; + + /// Test matrix for all script class + RunContext combinations + #[test] + fn test_configuration_matrix() { + struct TestCase { + class_name: &'static str, + run_context: Option<&'static str>, + expected_extension: &'static str, + description: &'static str, + } + + let test_cases = vec![ + // ModuleScript - always .luau, no RunContext matters + TestCase { + class_name: "ModuleScript", + run_context: None, + expected_extension: ".luau", + description: "ModuleScript without RunContext", + }, + // LocalScript - always .client.luau + TestCase { + class_name: "LocalScript", + run_context: None, + expected_extension: ".client.luau", + description: "LocalScript", + }, + // Script with various RunContext values + TestCase { + class_name: "Script", + run_context: None, + expected_extension: ".server.luau", + description: "Script without RunContext (default)", + }, + TestCase { + class_name: "Script", + run_context: Some("Server"), + expected_extension: ".server.luau", + description: "Script with RunContext::Server", + }, + TestCase { + class_name: "Script", + run_context: Some("Client"), + expected_extension: ".client.luau", + description: "Script with RunContext::Client", + }, + TestCase { + class_name: "Script", + run_context: Some("Legacy"), + expected_extension: ".server.luau", + description: "Script with RunContext::Legacy", + }, + TestCase { + class_name: "Script", + run_context: Some("Plugin"), + expected_extension: ".plugin.luau", + description: "Script with RunContext::Plugin", + }, + ]; + + for case in test_cases { + let suffix = match case.class_name { + "ModuleScript" => String::new(), + "LocalScript" => "client".to_string(), + "Script" => { + // Match on RunContext name to determine suffix + match case.run_context { + Some("Client") => "client", + Some("Server") => "server", + Some("Legacy") => "server", + Some("Plugin") => "plugin", + None => "server", + _ => "server", + } + .to_string() + } + _ => panic!("Unknown class"), + }; + + let extension = if suffix.is_empty() { + ".luau".to_string() + } else { + format!(".{}.luau", suffix) + }; + + assert_eq!( + extension, case.expected_extension, + "FAILED: {} - got {} expected {}", + case.description, extension, case.expected_extension + ); + } + } + + #[test] + fn test_emit_legacy_scripts_compatibility() { + // Test that our syncback is compatible with emitLegacyScripts mode + // + // emitLegacyScripts: true + // - .client.luau -> LocalScript + // - .server.luau -> Script with RunContext::Legacy + // + // emitLegacyScripts: false + // - .client.luau -> Script with RunContext::Client + // - .server.luau -> Script with RunContext::Server + + // 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")), + ]; + + 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? + let syncback_ext = match (*class, *run_context) { + ("LocalScript", _) => ".client.luau", + ("Script", Some("Legacy")) => ".server.luau", + ("Script", Some("Server")) => ".server.luau", + ("Script", Some("Client")) => ".client.luau", + ("Script", None) => ".server.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 {} {:?}", + class, run_context + ); + } + } + } +} diff --git a/src/web/interface.rs b/src/web/interface.rs index fded5678..cacb32b3 100644 --- a/src/web/interface.rs +++ b/src/web/interface.rs @@ -248,6 +248,24 @@ pub struct ReadResponse<'a> { pub instances: HashMap>, } +/// Instance data for creating new instances via the write API +#[derive(Debug, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct AddedInstance { + /// The parent instance's Ref (must exist in Rojo's tree) + pub parent: Ref, + /// The instance name + pub name: String, + /// The class name (e.g., "ModuleScript", "Script", "LocalScript") + pub class_name: String, + /// Properties to set on the instance + #[serde(default)] + pub properties: HashMap, + /// Children instances (recursively encoded for hierarchical syncback) + #[serde(default)] + pub children: Vec, +} + #[derive(Debug, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct WriteRequest { @@ -255,7 +273,7 @@ pub struct WriteRequest { pub removed: Vec, #[serde(default)] - pub added: HashMap, + pub added: HashMap, pub updated: Vec, }