From 0e5e8098941e8aa4964f7094c4b48c15c5fb9110 Mon Sep 17 00:00:00 2001 From: Joe Date: Sun, 1 Feb 2026 07:58:06 -0600 Subject: [PATCH] UX update --- ...destructive_syncback_mode_acb8cb54.plan.md | 178 ++++++++++++++++++ .../merge_ui_button_changes_6608afe8.plan.md | 132 +++++++++++++ .../Components/PatchVisualizer/DomLabel.lua | 76 ++++++-- .../App/Components/PatchVisualizer/init.lua | 1 + plugin/src/App/StatusPages/Confirming.lua | 58 +++--- 5 files changed, 397 insertions(+), 48 deletions(-) create mode 100644 .cursor/plans/destructive_syncback_mode_acb8cb54.plan.md create mode 100644 .cursor/plans/merge_ui_button_changes_6608afe8.plan.md diff --git a/.cursor/plans/destructive_syncback_mode_acb8cb54.plan.md b/.cursor/plans/destructive_syncback_mode_acb8cb54.plan.md new file mode 100644 index 00000000..392cee5c --- /dev/null +++ b/.cursor/plans/destructive_syncback_mode_acb8cb54.plan.md @@ -0,0 +1,178 @@ +--- +name: Destructive Syncback Mode +overview: Add a `--clean` flag to syncback that ignores existing file structure and middleware, treating all instances as new to ensure consistent 1:1 output regardless of existing filesystem state. +todos: [] +isProject: false +--- + +# Destructive Syncback Mode + +## Root Cause Analysis + +Your inconsistency stems from three preservation mechanisms in syncback: + +### 1. File Name Preservation (`[src/syncback/file_names.rs](src/syncback/file_names.rs)` lines 19-34) + +When an old instance exists, the **existing filename is always preserved**, even if the instance was renamed: + +```rust +if let Some(old_inst) = old_inst { + if let Some(source) = old_inst.metadata().relevant_paths.first() { + source.file_name() // Returns OLD filename, ignores new instance name + } +} +``` + +### 2. Middleware Preservation (`[src/syncback/mod.rs](src/syncback/mod.rs)` lines 353-357) + +When an old instance exists, its middleware format is preserved: + +```rust +} else if let Some(old_middleware) = old_middleware { + // Use old middleware, even if a clean sync would choose differently + middleware = old_middleware; +} +``` + +This means a `Part.rbxm` stays as `.rbxm` instead of potentially becoming a `Part/` directory with children. + +### 3. Name-Based Child Matching (`[src/snapshot_middleware/dir.rs](src/snapshot_middleware/dir.rs)` lines 196-216) + +Children are matched by name between old and new DOMs. If a child exists in both, it inherits the old structure: + +```rust +if let Some(old_child) = old_child_map.remove(new_child.name.as_str()) { + // Existing child - pass old reference, preserving its structure + children.push(snapshot.with_joined_path(*new_child_ref, Some(old_child.id()))?); +} else { + // New child - gets fresh structure + children.push(snapshot.with_joined_path(*new_child_ref, None)?); +} +``` + +## Solution: Add `--clean` Flag + +Add a `--clean` flag that makes syncback treat all instances as "new" by setting `old` references to `None`, ensuring consistent output regardless of existing filesystem state. + +### Changes Required + +#### 1. Add CLI Flag (`[src/cli/syncback.rs](src/cli/syncback.rs)`) + +Add `--clean` flag to the command definition: + +```rust +/// If provided, syncback will ignore existing file structure and create +/// a fresh project layout. This ensures consistent output regardless of +/// existing filesystem state. +#[clap(long)] +pub clean: bool, +``` + +#### 2. Pass Flag to `syncback_loop` (`[src/cli/syncback.rs](src/cli/syncback.rs)`) + +Modify the `syncback_loop` call to pass the clean flag: + +```rust +let snapshot = syncback_loop( + session_old.vfs(), + &mut dom_old, + dom_new, + session_old.root_project(), + self.clean, // New parameter +)?; +``` + +#### 3. Modify `syncback_loop` (`[src/syncback/mod.rs](src/syncback/mod.rs)`) + +Update function signature and initial snapshot: + +```rust +pub fn syncback_loop( + vfs: &Vfs, + old_tree: &mut RojoTree, + mut new_tree: WeakDom, + project: &Project, + clean_mode: bool, // New parameter +) -> anyhow::Result +``` + +When `clean_mode` is true, set the initial `old` reference to `None`: + +```rust +let mut snapshots = vec![SyncbackSnapshot { + data: syncback_data, + old: if clean_mode { None } else { Some(old_tree.get_root_id()) }, + new: new_tree.root_ref(), + path: project.file_location.clone(), + middleware: Some(Middleware::Project), +}]; +``` + +#### 4. Track Removed Files in Clean Mode (`[src/syncback/mod.rs](src/syncback/mod.rs)`) + +In clean mode, we need to remove all existing files in the target paths. Add logic to collect existing paths for removal: + +```rust +if clean_mode { + // Collect all existing paths under the project tree for removal + for ref_id in descendants(old_tree.inner(), old_tree.get_root_id()) { + if let Some(inst) = old_tree.get_instance(ref_id) { + for path in inst.metadata().relevant_paths.iter() { + if path.is_dir() { + fs_snapshot.remove_dir(path); + } else { + fs_snapshot.remove_file(path); + } + } + } + } +} +``` + +#### 5. Update Child Snapshot Creation (`[src/snapshot_middleware/dir.rs](src/snapshot_middleware/dir.rs)`) + +The `SyncbackSnapshot::with_joined_path` method already handles `old = None` correctly - it will create a fresh child snapshot without old references. + +### Data Flow Diagram + +```mermaid +flowchart TD + subgraph currentBehavior [Current Behavior] + A1[Syncback on Existing Files] --> B1{Old Instance Exists?} + B1 -->|Yes| C1[Preserve Filename] + B1 -->|Yes| D1[Preserve Middleware] + B1 -->|Yes| E1[Match Children by Name] + C1 --> F1[Inconsistent Structure] + D1 --> F1 + E1 --> F1 + end + + subgraph cleanMode [Clean Mode] + A2[Syncback with --clean] --> B2[Set old = None] + B2 --> C2[Generate Fresh Filename] + B2 --> D2[Choose Best Middleware] + B2 --> E2[All Children Are New] + C2 --> F2[Consistent 1:1 Structure] + D2 --> F2 + E2 --> F2 + end +``` + + + +### Usage + +```bash +# Current behavior (preserves existing structure) +rojo syncback -i game.rbxl + +# Destructive mode (ignores existing structure, creates fresh layout) +rojo syncback -i game.rbxl --clean + +# Combine with other flags +rojo syncback -i game.rbxl --clean --list --dry-run +``` + +### Alternative Consideration + +If you also want to support project-level configuration (so you don't need to pass `--clean` every time), we could add a `syncbackRules.cleanMode: true` option in the project file. Let me know if you'd prefer that approach. \ No newline at end of file diff --git a/.cursor/plans/merge_ui_button_changes_6608afe8.plan.md b/.cursor/plans/merge_ui_button_changes_6608afe8.plan.md new file mode 100644 index 00000000..65d4021f --- /dev/null +++ b/.cursor/plans/merge_ui_button_changes_6608afe8.plan.md @@ -0,0 +1,132 @@ +--- +name: Merge UI Button Changes +overview: Rename Push/Pull buttons to Rojo/Studio, remove "all" helper buttons, add double-click cascade functionality with hover-reveal buttons for parent nodes. +todos: + - id: rename-buttons + content: Rename Pull->Studio and Push->Rojo in DomLabel.lua SelectionRadio + status: completed + - id: remove-all-buttons + content: Remove PullAll, SkipAll, PushAll buttons from Confirming.lua + status: completed + - id: add-subtree-callback + content: Create onSubtreeSelectionChange callback in Confirming.lua that sets selection for node and all descendants + status: completed + - id: add-double-click + content: Add double-click handler to SelectionOption buttons with caret indicator + status: completed + - id: hover-reveal-buttons + content: Show selection buttons on hover for parent nodes without patchType + status: completed +isProject: false +--- + +# Merge UI Button Improvements + +## Key Files to Modify + +- `[plugin/src/App/Components/PatchVisualizer/DomLabel.lua](plugin/src/App/Components/PatchVisualizer/DomLabel.lua)` - Selection buttons and hover behavior +- `[plugin/src/App/StatusPages/Confirming.lua](plugin/src/App/StatusPages/Confirming.lua)` - Remove "all" buttons, add subtree selection callback +- `[plugin/src/App/Components/PatchVisualizer/init.lua](plugin/src/App/Components/PatchVisualizer/init.lua)` - Pass patchTree to DomLabel + +## Changes + +### 1. Rename Buttons (DomLabel.lua) + +In `SelectionRadio` function, change button text: + +- "Pull" -> "Studio" (line 113) +- "Push" -> "Rojo" (line 137) + +### 2. Remove "All" Button Helpers (Confirming.lua) + +Remove these elements from the `Buttons` frame (lines 156-184): + +- `PullAll` button +- `SkipAll` button +- `PushAll` button + +Keep the `setAllSelections` function as it will be repurposed for subtree selection. + +### 3. Add Double-Click Cascade with Caret (DomLabel.lua) + +Modify `SelectionOption` component: + +- Add a "▼" caret character after the button text +- Track double-click using `clickCount` in the `Activated` event +- On double-click, call a new `onSubtreeSelectionChange` callback + +Add to `SelectionRadio`: + +- Pass `onSubtreeSelectionChange` prop to each option + +### 4. Add Subtree Selection Callback (Confirming.lua) + +Create new callback `onSubtreeSelectionChange(nodeId, selection)`: + +```lua +self.onSubtreeSelectionChange = function(nodeId, selection) + if not self.props.patchTree then return end + + local node = self.props.patchTree:getNode(nodeId) + if not node then return end + + self:setState(function(state) + local newSelections = table.clone(state.selections) + -- Set this node if it has a patchType + if node.patchType then + newSelections[nodeId] = selection + end + -- Set all descendants + self.props.patchTree:forEach(function(childNode) + if childNode.patchType then + newSelections[childNode.id] = selection + end + end, node) + return { selections = newSelections } + end) +end +``` + +### 5. Show Buttons on Hover for Parents Without Changes (DomLabel.lua) + +For nodes where `patchType == nil` but `hasChildren == true`: + +Add hover state tracking in `DomLabel`: + +```lua +self:setState({ isHovered = false }) +``` + +Add `MouseEnter`/`MouseLeave` events to the main Frame. + +Modify `SelectionRadio` visibility logic: + +- `visible = (props.patchType ~= nil) or (props.hasChildren and self.state.isHovered)` + +The buttons for parent-only nodes will use `onSubtreeSelectionChange` directly (no single-click behavior needed since there's no selection to make for the node itself). + +### 6. Pass Required Props Through Component Tree + +In `PatchVisualizer/init.lua`, pass to DomLabel: + +- `patchTree = patchTree` +- `onSubtreeSelectionChange = self.props.onSubtreeSelectionChange` + +In `Confirming.lua`, pass to PatchVisualizer: + +- `onSubtreeSelectionChange = self.onSubtreeSelectionChange` + +## Component Flow + +```mermaid +flowchart TD + A[Confirming.lua] -->|selections, onSelectionChange, onSubtreeSelectionChange| B[PatchVisualizer] + B -->|node props, selection callbacks| C[DomLabel] + C -->|single click| D[onSelectionChange] + C -->|double click| E[onSubtreeSelectionChange] + D --> F[Update single node selection] + E --> G[Update node + all descendants] +``` + + + diff --git a/plugin/src/App/Components/PatchVisualizer/DomLabel.lua b/plugin/src/App/Components/PatchVisualizer/DomLabel.lua index 6c0f3134..5b1a7d32 100644 --- a/plugin/src/App/Components/PatchVisualizer/DomLabel.lua +++ b/plugin/src/App/Components/PatchVisualizer/DomLabel.lua @@ -61,24 +61,40 @@ local function SelectionOption(props) else Color3.fromHex("7F8C8D")) else theme.BorderedContainer.BackgroundColor local textColor = if isSelected then Color3.new(1, 1, 1) else theme.TextColor + -- Add caret if this button can cascade to children + local displayText = if props.showCaret then props.text .. " ▼" else props.text + local buttonWidth = if props.showCaret then 48 else 36 return e("TextButton", { - Size = UDim2.new(0, 36, 0, 18), + Size = UDim2.new(0, buttonWidth, 0, 18), BackgroundColor3 = bgColor, BackgroundTransparency = props.transparency:map(function(t) return if isSelected then 0.1 + (0.9 * t) else 0.7 + (0.3 * t) end), BorderSizePixel = 0, - Text = props.text, + Text = displayText, FontFace = if isSelected then theme.Font.Bold else theme.Font.Main, TextSize = theme.TextSize.Small, TextColor3 = textColor, TextTransparency = props.transparency, LayoutOrder = props.layoutOrder, ZIndex = 10, - [Roact.Event.Activated] = function() - if props.onClick then - props.onClick() + [Roact.Event.Activated] = function(_rbx, _input, clickCount) + -- For parent-only nodes, any click triggers subtree selection + if props.isParentOnly then + if props.onDoubleClick then + props.onDoubleClick() + end + elseif clickCount == 1 then + -- Double click: apply to subtree + if props.onDoubleClick then + props.onDoubleClick() + end + else + -- Single click: apply to this node only + if props.onClick then + props.onClick() + end end end, }, { @@ -95,8 +111,12 @@ local function SelectionRadio(props) return nil end + local showCaret = props.hasChildren + local frameWidth = if showCaret then 154 else 118 + local isParentOnly = props.isParentOnly + return e("Frame", { - Size = UDim2.new(0, 118, 0, 18), + Size = UDim2.new(0, frameWidth, 0, 18), BackgroundTransparency = 1, Position = UDim2.new(1, -5, 0, 3), AnchorPoint = Vector2.new(1, 0), @@ -109,17 +129,24 @@ local function SelectionRadio(props) SortOrder = Enum.SortOrder.LayoutOrder, Padding = UDim.new(0, 4), }), - Pull = e(SelectionOption, { - text = "Pull", + Studio = e(SelectionOption, { + text = "Studio", optionType = "pull", isSelected = props.selection == "pull", transparency = props.transparency, layoutOrder = 1, + showCaret = showCaret, + isParentOnly = isParentOnly, onClick = function() if props.onSelectionChange then props.onSelectionChange(props.nodeId, "pull") end end, + onDoubleClick = function() + if props.onSubtreeSelectionChange then + props.onSubtreeSelectionChange(props.nodeId, "pull") + end + end, }), Skip = e(SelectionOption, { text = "Skip", @@ -127,23 +154,37 @@ local function SelectionRadio(props) isSelected = props.selection == "ignore", transparency = props.transparency, layoutOrder = 2, + showCaret = showCaret, + isParentOnly = isParentOnly, onClick = function() if props.onSelectionChange then props.onSelectionChange(props.nodeId, "ignore") end end, + onDoubleClick = function() + if props.onSubtreeSelectionChange then + props.onSubtreeSelectionChange(props.nodeId, "ignore") + end + end, }), - Push = e(SelectionOption, { - text = "Push", + Rojo = e(SelectionOption, { + text = "Rojo", optionType = "push", isSelected = props.selection == "push", transparency = props.transparency, layoutOrder = 3, + showCaret = showCaret, + isParentOnly = isParentOnly, onClick = function() if props.onSelectionChange then props.onSelectionChange(props.nodeId, "push") end end, + onDoubleClick = function() + if props.onSubtreeSelectionChange then + props.onSubtreeSelectionChange(props.nodeId, "push") + end + end, }), }) end @@ -183,6 +224,7 @@ function DomLabel:init() self:setState({ renderExpansion = self.expanded, + isHovered = false, }) self.motorStepConnection = self.motor:onStep(function(value) if not self.isMounted then @@ -301,6 +343,12 @@ function DomLabel:render() BackgroundTransparency = 1, Text = "", Size = UDim2.new(1, 0, 1, 0), + [Roact.Event.MouseEnter] = function() + self:setState({ isHovered = true }) + end, + [Roact.Event.MouseLeave] = function() + self:setState({ isHovered = false }) + end, [Roact.Event.Activated] = function(_rbx: Instance, _input: InputObject, clickCount: number) if clickCount == 1 then -- Double click opens the instance in explorer @@ -413,11 +461,17 @@ function DomLabel:render() else nil, }), SelectionRadio = e(SelectionRadio, { - visible = props.patchType ~= nil and props.onSelectionChange ~= nil, + -- Visible for nodes with patchType, or for parent-only nodes on hover + visible = (props.patchType ~= nil and props.onSelectionChange ~= nil) + or (props.patchType == nil and props.hasChildren and self.state.isHovered and props.onSubtreeSelectionChange ~= nil), nodeId = props.nodeId, selection = props.selection, transparency = props.transparency, + hasChildren = props.hasChildren, + -- For parent-only nodes (no patchType), only subtree selection makes sense + isParentOnly = props.patchType == nil, onSelectionChange = props.onSelectionChange, + onSubtreeSelectionChange = props.onSubtreeSelectionChange, }), LineGuides = e("Folder", nil, lineGuides), }) diff --git a/plugin/src/App/Components/PatchVisualizer/init.lua b/plugin/src/App/Components/PatchVisualizer/init.lua index 449339b8..ba247234 100644 --- a/plugin/src/App/Components/PatchVisualizer/init.lua +++ b/plugin/src/App/Components/PatchVisualizer/init.lua @@ -104,6 +104,7 @@ function PatchVisualizer:render() nodeId = node.id, selection = self.props.selections and self.props.selections[node.id], onSelectionChange = self.props.onSelectionChange, + onSubtreeSelectionChange = self.props.onSubtreeSelectionChange, }) if isFinalChild then diff --git a/plugin/src/App/StatusPages/Confirming.lua b/plugin/src/App/StatusPages/Confirming.lua index 75f5708c..ec86fd6a 100644 --- a/plugin/src/App/StatusPages/Confirming.lua +++ b/plugin/src/App/StatusPages/Confirming.lua @@ -48,18 +48,31 @@ function ConfirmingPage:init() end) end - -- Set all selections to a specific value - self.setAllSelections = function(value) + -- Set selection for a node and all its descendants (subtree) + self.onSubtreeSelectionChange = function(nodeId, selection) if not self.props.patchTree then return end - local newSelections = {} - self.props.patchTree:forEach(function(node) + + local node = self.props.patchTree:getNode(nodeId) + if not node then + return + end + + self:setState(function(state) + local newSelections = table.clone(state.selections) + -- Set this node if it has a patchType if node.patchType then - newSelections[node.id] = value + newSelections[nodeId] = selection end + -- Set all descendants + self.props.patchTree:forEach(function(childNode) + if childNode.patchType then + newSelections[childNode.id] = selection + end + end, node) + return { selections = newSelections } end) - self:setState({ selections = newSelections }) end end @@ -123,6 +136,7 @@ function ConfirmingPage:render() patchTree = self.props.patchTree, selections = self.state.selections, onSelectionChange = self.onSelectionChange, + onSubtreeSelectionChange = self.onSubtreeSelectionChange, showStringDiff = function(currentString: string, incomingString: string) self:setState({ @@ -153,41 +167,11 @@ function ConfirmingPage:render() onClick = self.props.onAbort, }), - PullAll = e(TextButton, { - text = "Pull All", - style = "Danger", - transparency = self.props.transparency, - layoutOrder = 2, - onClick = function() - self.setAllSelections("pull") - end, - }), - - SkipAll = e(TextButton, { - text = "Skip All", - style = "Neutral", - transparency = self.props.transparency, - layoutOrder = 3, - onClick = function() - self.setAllSelections("ignore") - end, - }), - - PushAll = e(TextButton, { - text = "Push All", - style = "Success", - transparency = self.props.transparency, - layoutOrder = 4, - onClick = function() - self.setAllSelections("push") - end, - }), - Accept = e(TextButton, { text = "Accept", style = "Primary", transparency = self.props.transparency, - layoutOrder = 5, + layoutOrder = 2, onClick = function() if not self.props.onConfirm then return