From 556d522bb9ff1441a27774a9ddbb56312e725957 Mon Sep 17 00:00:00 2001
From: Simon Cropp <simon.cropp@gmail.com>
Date: Sat, 22 Aug 2026 23:29:33 +1000
Subject: [PATCH] Leave the reader where they were when something else is
 removed

Remove held the selection as an index and zeroed the scroll, so any removal above
the entry on screen silently changed what was on screen - to whatever that index
now landed on, at the top of it. Most removals are of something else: a settle from
a test that has started passing, "Accept all in <solution>" from a header, a sweep
that skipped this entry and took the rest.

The selection follows its key now, the way Sync and EnqueueInline already did, and
keeps its scroll. Only an entry that is itself gone falls back to advancing by
index, which is the one case where the reader has to be moved at all.
---
 .../QueueRemovalTests.cs                      | 54 +++++++++++++++++++
 src/DiffEngineViewer/ViewerSession.cs         | 22 ++++++--
 2 files changed, 73 insertions(+), 3 deletions(-)
 create mode 100644 src/DiffEngineViewer.Tests/QueueRemovalTests.cs

diff --git a/src/DiffEngineViewer.Tests/QueueRemovalTests.cs b/src/DiffEngineViewer.Tests/QueueRemovalTests.cs
new file mode 100644
index 00000000..024b3576
--- /dev/null
+++ b/src/DiffEngineViewer.Tests/QueueRemovalTests.cs
@@ -0,0 +1,54 @@
+/// <summary>
+/// What a removal does to the entry being read. Removals arrive for entries other than that one -
+/// a settle from a test that has started passing, a sweep from a group header, a bulk accept that
+/// skipped this entry - and the reader is in the middle of a comparison while they do.
+/// </summary>
+public class QueueRemovalTests
+{
+    [Test]
+    public async Task Settling_another_entry_leaves_the_one_being_read_where_it_was()
+    {
+        var state = Scrolled(out var reading);
+
+        var settled = ViewerSession.Settle(state, state.Queue[0].Key);
+
+        await Assert.That(settled.Current!.Key).IsEqualTo(reading);
+        await Assert.That(settled.ScrollTop).IsEqualTo(state.ScrollTop);
+    }
+
+    /// <summary>
+    /// The entry on screen going is the one case where the reader has to be moved, and the top of
+    /// the next entry is where they go.
+    /// </summary>
+    [Test]
+    public async Task Settling_the_entry_being_read_moves_on_to_the_next()
+    {
+        var state = Scrolled(out var reading);
+
+        var settled = ViewerSession.Settle(state, reading);
+
+        await Assert.That(settled.Current!.Key).IsNotEqualTo(reading);
+        await Assert.That(settled.ScrollTop).IsEqualTo(0);
+    }
+
+    /// <summary>
+    /// Three entries, the middle one selected and scrolled into - so that a removal above it moves
+    /// every index below, which is the thing being asserted about.
+    /// </summary>
+    static SessionState Scrolled(out string reading)
+    {
+        var state = Fixtures.Inline(
+            Fixtures.Patch("A.cs", 1, null, Fixtures.Long(true)),
+            Fixtures.Patch("B.cs", 2, null, Fixtures.Long(true)),
+            Fixtures.Patch("C.cs", 3, null, Fixtures.Long(true)));
+        reading = state.Queue[1].Key;
+        state = ViewerSession.SelectKey(state, reading);
+        state = ViewerSession.Apply(state, CommandKind.PageDown);
+        if (state.ScrollTop == 0)
+        {
+            throw new("The entry did not scroll, so nothing below asserts anything.");
+        }
+
+        return state;
+    }
+}
diff --git a/src/DiffEngineViewer/ViewerSession.cs b/src/DiffEngineViewer/ViewerSession.cs
index d59969e7..eb734e31 100644
--- a/src/DiffEngineViewer/ViewerSession.cs
+++ b/src/DiffEngineViewer/ViewerSession.cs
@@ -870,16 +870,32 @@ static int IndexOf(IReadOnlyList<QueueEntry> queue, string key)
         return -1;
     }
 
-    static SessionState Remove(SessionState state, IReadOnlyList<QueueEntry> queue, string? message) =>
-        Clamp(state with
+    /// <summary>
+    /// Drops whatever is no longer in the queue and leaves the reader where they were.
+    /// <para>
+    /// Most removals are not of the entry on screen: a settle from a test that has started
+    /// passing, "Accept all in &lt;solution&gt;" from a header, a sweep that skipped this one.
+    /// Holding <see cref="SessionState.Selected" /> as an index across those quietly changed what
+    /// was on screen to whatever the old index now landed on, at the top of it. So the selection
+    /// follows its key, the way <see cref="Sync" /> and <see cref="EnqueueInline" /> do, and only
+    /// an entry that is itself gone falls back to advancing by index.
+    /// </para>
+    /// </summary>
+    static SessionState Remove(SessionState state, IReadOnlyList<QueueEntry> queue, string? message)
+    {
+        var key = state.Current?.Key;
+        var selected = key is null ? -1 : IndexOf(queue, key);
+        return Clamp(state with
         {
             Queue = queue,
-            ScrollTop = 0,
+            Selected = selected < 0 ? state.Selected : selected,
+            ScrollTop = selected < 0 ? 0 : state.ScrollTop,
             Message = message,
             // Nothing left to manage, so the window has no reason to stay open.
             Exit = queue.Count == 0,
             Menu = null
         });
+    }
 
     static SessionState Select(SessionState state, int index)
     {