diff --git a/plugin/src/Reconciler/diff.lua b/plugin/src/Reconciler/diff.lua index fb4c79d0..798e5e4c 100644 --- a/plugin/src/Reconciler/diff.lua +++ b/plugin/src/Reconciler/diff.lua @@ -15,6 +15,32 @@ local function isEmpty(table) return next(table) == nil end +-- Finds duplicate names among children and returns a set of names that are duplicated +local function findDuplicateNames(children: { Instance } | { string }, virtualInstances: { [string]: any }?): { [string]: boolean } + local nameCounts: { [string]: number } = {} + local duplicates: { [string]: boolean } = {} + + for _, child in children do + local name: string + if typeof(child) == "Instance" then + name = child.Name + elseif virtualInstances then + -- child is a virtual instance ID + local virtualChild = virtualInstances[child] + name = virtualChild and virtualChild.Name or "" + else + continue + end + + nameCounts[name] = (nameCounts[name] or 0) + 1 + if nameCounts[name] > 1 then + duplicates[name] = true + end + end + + return duplicates +end + local function fuzzyEq(a: number, b: number, epsilon: number): boolean return math.abs(a - b) < epsilon end @@ -119,9 +145,86 @@ local function diff(instanceMap, virtualInstances, rootId) updated = {}, } + -- Count of locations with duplicate-named siblings + local skippedDuplicateCount = 0 + + -- Pre-scan to find ALL instances that are in an ambiguous path + -- (i.e., they or any of their ancestors have duplicate-named siblings) + local ambiguousIds: { [string]: boolean } = {} + + local function markSubtreeAmbiguous(id) + if ambiguousIds[id] then + return + end + ambiguousIds[id] = true + + local virtualInstance = virtualInstances[id] + if virtualInstance then + for _, childId in ipairs(virtualInstance.Children) do + markSubtreeAmbiguous(childId) + end + end + end + + -- Recursively scan for duplicates and mark ambiguous subtrees + local function scanForDuplicates(id) + local virtualInstance = virtualInstances[id] + if not virtualInstance then + return + end + + local instance = instanceMap.fromIds[id] + + -- Detect duplicate names among real DOM children (if instance exists) + local realDuplicates: { [string]: boolean } = {} + if instance then + local realChildren = instance:GetChildren() + realDuplicates = findDuplicateNames(realChildren) + + -- Count locations with duplicates + if next(realDuplicates) then + skippedDuplicateCount += 1 + end + end + + -- Detect duplicate names among virtual DOM children + local virtualDuplicates = findDuplicateNames(virtualInstance.Children, virtualInstances) + + -- Merge duplicates from both sides + local allDuplicates: { [string]: boolean } = {} + for name in realDuplicates do + allDuplicates[name] = true + end + for name in virtualDuplicates do + allDuplicates[name] = true + end + + -- Mark all children with duplicate names (and their entire subtrees) as ambiguous + for _, childId in ipairs(virtualInstance.Children) do + local virtualChild = virtualInstances[childId] + if virtualChild and allDuplicates[virtualChild.Name] then + markSubtreeAmbiguous(childId) + end + end + + -- Recurse into non-ambiguous children to find deeper duplicates + for _, childId in ipairs(virtualInstance.Children) do + if not ambiguousIds[childId] then + scanForDuplicates(childId) + end + end + end + + -- First pass: scan entire tree to find all ambiguous paths + scanForDuplicates(rootId) + -- Add a virtual instance and all of its descendants to the patch, marked as - -- being added. + -- being added. Skip ambiguous instances. local function markIdAdded(id) + if ambiguousIds[id] then + return + end + local virtualInstance = virtualInstances[id] patch.added[id] = virtualInstance @@ -132,6 +235,11 @@ local function diff(instanceMap, virtualInstances, rootId) -- Internal recursive kernel for diffing an instance with the given ID. local function diffInternal(id) + -- Skip entirely if this instance is in an ambiguous path + if ambiguousIds[id] then + return true + end + local virtualInstance = virtualInstances[id] local instance = instanceMap.fromIds[id] @@ -155,6 +263,11 @@ local function diff(instanceMap, virtualInstances, rootId) local changedProperties = {} for propertyName, virtualValue in pairs(virtualInstance.Properties) do + -- Skip CanvasPosition on ScrollingFrame (runtime state, not meaningful for sync) + if propertyName == "CanvasPosition" and virtualInstance.ClassName == "ScrollingFrame" then + continue + end + local getProperySuccess, existingValueOrErr = getProperty(instance, propertyName) if getProperySuccess then @@ -222,8 +335,18 @@ local function diff(instanceMap, virtualInstances, rootId) -- corresponding virtual instance should be removed. Any instance that -- does have a corresponding virtual instance is recursively diffed. for _, childInstance in ipairs(instance:GetChildren()) do + -- Skip TouchInterest (auto-created by Roblox when touch events are connected) + if childInstance.ClassName == "TouchInterest" then + continue + end + local childId = instanceMap.fromInstances[childInstance] + -- Skip children in ambiguous paths + if childId and ambiguousIds[childId] then + continue + end + if childId == nil then -- pcall to avoid security permission errors local success, skip = pcall(function() @@ -235,6 +358,14 @@ local function diff(instanceMap, virtualInstances, rootId) continue end + -- Check if this unmapped child has a duplicate name among siblings + -- If so, skip it (we can't reliably determine what to do with it) + local siblings = instance:GetChildren() + local siblingDuplicates = findDuplicateNames(siblings) + if siblingDuplicates[childInstance.Name] then + continue + end + -- This is an existing instance not present in the virtual DOM. -- We can mark it for deletion unless the user has asked us not -- to delete unknown stuff. @@ -253,6 +384,11 @@ local function diff(instanceMap, virtualInstances, rootId) -- Traverse the list of children in the virtual DOM. Any virtual -- instance that has no corresponding real instance should be created. for _, childId in ipairs(virtualInstance.Children) do + -- Skip children in ambiguous paths + if ambiguousIds[childId] then + continue + end + local childInstance = instanceMap.fromIds[childId] if childInstance == nil then @@ -267,6 +403,14 @@ local function diff(instanceMap, virtualInstances, rootId) local diffSuccess, err = diffInternal(rootId) + -- Log count of skipped duplicates + if skippedDuplicateCount > 0 then + Log.warn( + "Skipped {} location(s) with duplicate-named siblings (cannot reliably sync)", + skippedDuplicateCount + ) + end + if not diffSuccess then return false, err end diff --git a/plugin/src/WhitespaceUtil.lua b/plugin/src/WhitespaceUtil.lua index 8d05f054..51baf3fe 100644 --- a/plugin/src/WhitespaceUtil.lua +++ b/plugin/src/WhitespaceUtil.lua @@ -18,7 +18,8 @@ end -- Counts lines that differ between two strings (capped at 100) -- Returns: totalDiff, whitespaceDiff (lines that only differ in whitespace) --- Returns 10000, 10000 if exceeds 100 (frontend displays as "100+") +-- Returns 10000 for totalDiff if exceeds 100 (frontend displays as "100+") +-- whitespaceDiff is 10000 only if ALL checked differences were whitespace-only function WhitespaceUtil.CountLineDifferences(current: string, incoming: string): (number, number) if type(current) ~= "string" or type(incoming) ~= "string" then return 0, 0 @@ -29,6 +30,7 @@ function WhitespaceUtil.CountLineDifferences(current: string, incoming: string): local maxLines = math.max(#currentLines, #incomingLines) local totalDiff, whitespaceDiff = 0, 0 + local hasContentChange = false for i = 1, maxLines do local currentLine = currentLines[i] or "" local incomingLine = incomingLines[i] or "" @@ -37,9 +39,13 @@ function WhitespaceUtil.CountLineDifferences(current: string, incoming: string): totalDiff += 1 if NormalizeLine(currentLine) == NormalizeLine(incomingLine) then whitespaceDiff += 1 + else + hasContentChange = true end if totalDiff > 100 then - return 10000, 10000 + -- Return 10000 for both only if ALL differences were whitespace-only + -- Otherwise return different values so isWhitespaceOnly is false + return 10000, if hasContentChange then 0 else 10000 end end end diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index 639c1232..ea581828 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -134,23 +134,29 @@ pub fn syncback_dir_no_meta<'sync>( let mut children = Vec::new(); let mut removed_children = Vec::new(); - // We have to enforce unique child names for the file system. - let mut child_names = HashSet::with_capacity(new_inst.children().len()); - let mut duplicate_set = HashSet::new(); + // Detect duplicate child names (case-insensitive for file system safety). + // We skip duplicates instead of failing, logging them for the user. + let mut child_name_counts: HashMap = HashMap::new(); for child_ref in new_inst.children() { let child = snapshot.get_new_instance(*child_ref).unwrap(); - if !child_names.insert(child.name.to_lowercase()) { - duplicate_set.insert(child.name.as_str()); - } + let lower_name = child.name.to_lowercase(); + *child_name_counts.entry(lower_name).or_insert(0) += 1; } - if !duplicate_set.is_empty() { - if duplicate_set.len() <= 25 { - anyhow::bail!( - "Instance has children with duplicate name (case may not exactly match):\n {}", - duplicate_set.into_iter().collect::>().join(", ") - ); - } - anyhow::bail!("Instance has more than 25 children with duplicate names"); + + let duplicate_names: HashSet = child_name_counts + .into_iter() + .filter(|(_, count)| *count > 1) + .map(|(name, _)| name) + .collect(); + + if !duplicate_names.is_empty() { + let inst_path = crate::syncback::inst_path(snapshot.new_tree(), snapshot.new); + let duplicate_list: Vec<&str> = duplicate_names.iter().map(|s| s.as_str()).collect(); + log::warn!( + "Skipping children with duplicate names at '{}' (cannot reliably sync): {:?}", + inst_path, + duplicate_list + ); } if let Some(old_inst) = snapshot.old_inst() { @@ -162,6 +168,13 @@ pub fn syncback_dir_no_meta<'sync>( for new_child_ref in new_inst.children() { let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); + + // Skip children with duplicate names - cannot reliably sync them + if duplicate_names.contains(&new_child.name.to_lowercase()) { + old_child_map.remove(new_child.name.as_str()); + continue; + } + // Skip instances of ignored classes if snapshot.should_ignore_class(&new_child.class) { // Also remove from old_child_map so it won't be marked as removed @@ -203,8 +216,12 @@ pub fn syncback_dir_no_meta<'sync>( } } // Any children that are in the old dom but not the new one are removed. - // Filter out instances of ignored classes from removal. + // Filter out instances of ignored classes and duplicates from removal. removed_children.extend(old_child_map.into_values().filter(|inst| { + // Don't remove duplicates - we're skipping them entirely + if duplicate_names.contains(&inst.name().to_lowercase()) { + return false; + } if snapshot.should_ignore_class(inst.class_name().as_str()) { log::debug!( "Not removing instance {} because its class {} is ignored", @@ -220,6 +237,12 @@ pub fn syncback_dir_no_meta<'sync>( // There is no old instance. Just add every child. for new_child_ref in new_inst.children() { let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); + + // Skip children with duplicate names - cannot reliably sync them + if duplicate_names.contains(&new_child.name.to_lowercase()) { + continue; + } + // Skip instances of ignored classes if snapshot.should_ignore_class(&new_child.class) { log::debug!(