Fix: Infinite loop in DataGrid visual parent walk with unit tests

Refactored DataGrid focus handling to use VisualParentWalker.EnumerateUniqueAncestors, preventing infinite loops on cyclic parent chains. Added a new test project with xUnit, internal visibility for testing, and a unit test verifying correct termination on cycles. Updated solution and dependencies accordingly.

Author: maximilien-noal

Spice86.DataGrid.slnx

@@ -1,3 +1,4 @@
 <Solution>
   <Project Path="src/Spice86.DataGrid/Avalonia.Controls.DataGrid/Avalonia.Controls.DataGrid.csproj" />
+  <Project Path="src/Spice86.DataGrid/Spice86.DataGrid.Tests/Spice86.DataGrid.Tests.csproj" />
 </Solution>

src/Spice86.DataGrid/Avalonia.Controls.DataGrid/Avalonia.Controls.DataGrid.csproj

@@ -21,4 +21,8 @@
   <ItemGroup>
     <PackageReference Include="Avalonia" />
   </ItemGroup>
+
+  <ItemGroup>
+    <InternalsVisibleTo Include="Spice86.DataGrid.Tests" />
+  </ItemGroup>
 </Project>

src/Spice86.DataGrid/Avalonia.Controls.DataGrid/DataGrid.cs

@@ -3996,6 +3996,14 @@ private void DataGrid_KeyUp(object sender, KeyEventArgs e)
         }
 
         //TODO: Make override?
+        // Selects the logical Parent first (so popups walk out of their host), then falls back
+        // to the visual parent. Declared as a static method so it can be passed as a delegate
+        // without capturing 'this'.
+        internal static Visual GetVisualParentForFocusWalk(Visual visual)
+        {
+            return (visual?.Parent as Visual) ?? visual?.GetVisualParent();
+        }
+
         private void DataGrid_LostFocus(object sender, RoutedEventArgs e)
         {
             _focusedObject = null;
@@ -4006,27 +4014,22 @@ private void DataGrid_LostFocus(object sender, RoutedEventArgs e)
                 Visual focusedObject = AvaloniaInternalCompatibilityHelper.GetFocusedElement(this);
                 DataGridColumn editingColumn = null;
 
-                while (focusedObject != null)
+                // Walk up the visual tree using a cycle-safe enumerator to avoid infinite loops
+                // when the visual parent chain contains a cycle (can happen with popups / detached visuals).
+                foreach (Visual ancestor in VisualParentWalker.EnumerateUniqueAncestors<Visual>(focusedObject, GetVisualParentForFocusWalk))
                 {
-                    if (focusedObject == this)
+                    if (ancestor == this)
                     {
                         focusLeftDataGrid = false;
                         break;
                     }
 
-                    // Walk up the visual tree.  If we hit the root, try using the framework element's
-                    // parent.  We do this because Popups behave differently with respect to the visual tree,
-                    // and it could have a parent even if the VisualTreeHelper doesn't find it.
-                    var parent = focusedObject.Parent as Visual;
-                    if (parent == null)
-                    {
-                        parent = focusedObject.GetVisualParent();
-                    }
-                    else
+                    // If the ancestor has a logical Parent (Popups behave differently w.r.t. the visual tree),
+                    // the DataGrid will not receive the routed event for this focus change.
+                    if (ancestor?.Parent as Visual != null)
                     {
                         dataGridWillReceiveRoutedEvent = false;
                     }
-                    focusedObject = parent;
                 }
 
                 if (EditingRow != null && EditingColumnIndex != -1)

src/Spice86.DataGrid/Avalonia.Controls.DataGrid/Utils/VisualParentWalker.cs

@@ -0,0 +1,41 @@
+#nullable enable
+
+using System;
+using System.Collections.Generic;
+
+namespace Avalonia.Controls.Utils
+{
+    /// <summary>
+    /// Walks a parent chain while guarding against cycles, so a cyclic visual-parent graph
+    /// (which can happen with popups / detached visuals) cannot cause an infinite loop.
+    /// </summary>
+    internal static class VisualParentWalker
+    {
+        /// <summary>
+        /// Enumerates <paramref name="start"/> and its ancestors as produced by <paramref name="getParent"/>,
+        /// stopping when the chain ends (null) or when a node is encountered a second time (cycle).
+        /// </summary>
+        public static IEnumerable<T> EnumerateUniqueAncestors<T>(T? start, Func<T, T?> getParent)
+            where T : class
+        {
+            if (start is null)
+            {
+                yield break;
+            }
+
+            HashSet<T> visited = new HashSet<T>(ReferenceEqualityComparer.Instance);
+            T? current = start;
+            while (current is not null)
+            {
+                if (!visited.Add(current))
+                {
+                    // Cycle detected - stop walking.
+                    yield break;
+                }
+
+                yield return current;
+                current = getParent(current);
+            }
+        }
+    }
+}

src/Spice86.DataGrid/Directory.Packages.props

@@ -1,5 +1,8 @@
 <Project>
     <ItemGroup>
         <PackageVersion Include="Avalonia" Version="12.0.2" />
+        <PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.12.0" />
+        <PackageVersion Include="xunit" Version="2.9.1" />
+        <PackageVersion Include="xunit.runner.visualstudio" Version="2.5.11" />
     </ItemGroup>
 </Project>

src/Spice86.DataGrid/Spice86.DataGrid.Tests/Spice86.DataGrid.Tests.csproj

@@ -0,0 +1,21 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+	<TargetFramework>net10.0</TargetFramework>
+	<IsTestProject>true</IsTestProject>
+	<Nullable>enable</Nullable>
+	<WarningsAsErrors />
+  </PropertyGroup>
+
+  <ItemGroup>
+	<PackageReference Include="Microsoft.NET.Test.Sdk" />
+	<PackageReference Include="xunit" />
+	<PackageReference Include="xunit.runner.visualstudio">
+	  <PrivateAssets>all</PrivateAssets>
+	  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
+	</PackageReference>
+  </ItemGroup>
+
+  <ItemGroup>
+	<ProjectReference Include="../Avalonia.Controls.DataGrid/Avalonia.Controls.DataGrid.csproj" />
+  </ItemGroup>
+</Project>

src/Spice86.DataGrid/Spice86.DataGrid.Tests/VisualParentWalkerCycleTests.cs

@@ -0,0 +1,72 @@
+using System;
+using System.Collections.Generic;
+using System.Threading;
+using System.Threading.Tasks;
+using Avalonia.Controls.Utils;
+using Xunit;
+
+namespace Spice86.DataGrid.Tests;
+
+/// <summary>
+/// True logic test for the cycle-safe visual parent walk used by DataGrid_LostFocus.
+///
+/// DataGrid_LostFocus walks up the focused element's parent chain. If that chain contains
+/// a cycle (which can happen with popups / detached visuals), a naive walk loops forever
+/// and freezes the UI. The fix is <see cref="VisualParentWalker.EnumerateUniqueAncestors"/>,
+/// which uses a HashSet to break out of cycles.
+///
+/// - RED  (without fix): EnumerateUniqueAncestors loops forever on a cyclic chain -> test times out -> FAIL
+/// - GREEN (with fix):   EnumerateUniqueAncestors visits each node once then stops -> test PASSES
+/// </summary>
+public class VisualParentWalkerCycleTests
+{
+    private sealed class Node
+    {
+        public Node? Parent;
+    }
+
+    [Fact]
+    public async Task EnumerateUniqueAncestors_OnCyclicChain_TerminatesAndVisitsEachNodeOnce()
+    {
+        // ==================== ARRANGE ====================
+        // Build a cyclic parent chain: a -> b -> c -> a (cycle).
+        Node a = new Node();
+        Node b = new Node();
+        Node c = new Node();
+        a.Parent = b;
+        b.Parent = c;
+        c.Parent = a;
+
+        List<Node> visited = new List<Node>();
+
+        // ==================== ACT ====================
+        // Run the walk on a worker thread with a hard timeout.
+        // Without the HashSet guard, this loops forever and the task never completes.
+        Task task = Task.Run(() =>
+        {
+            foreach (Node node in VisualParentWalker.EnumerateUniqueAncestors<Node>(a, n => n.Parent))
+            {
+                visited.Add(node);
+
+                // Hard safety net so a buggy (cycle-blind) implementation cannot
+                // run away with unbounded memory before the timeout fires.
+                if (visited.Count > 1000)
+                {
+                    break;
+                }
+            }
+        });
+
+        await Task.Delay(TimeSpan.FromSeconds(2));
+        bool completed = task.IsCompletedSuccessfully;
+
+        // ==================== ASSERT ====================
+        Assert.True(completed,
+            "RED: VisualParentWalker.EnumerateUniqueAncestors did not terminate on a cyclic " +
+            "parent chain within 2s. Without the HashSet<T> visited guard, the walk loops " +
+            "forever and freezes the UI in DataGrid_LostFocus.");
+
+        // Each node in the cycle must be visited exactly once, in order.
+        Assert.Equal(new[] { a, b, c }, visited);
+    }
+}