#673: Closing a tab activates its neighbour, not the last-used tab — tab_mru is maintained but never consulted (and is index-based, so reorder corrupts it)

src/core/engine/mod.rs

@@ -2686,9 +2686,11 @@ pub struct Engine {
     pub tab_switcher_open: bool,
     /// Index of the currently highlighted item in the MRU list.
     pub tab_switcher_selected: usize,
-    /// MRU-ordered list of (group_id, tab_index) pairs.
-    /// Most recently used is at index 0.
-    pub tab_mru: Vec<(GroupId, usize)>,
+    /// MRU-ordered list of (group_id, tab_id) pairs.
+    /// Most recently used is at index 0. Keyed by `TabId` (not a positional
+    /// index) so reordering or moving tabs between groups can never silently
+    /// repoint an entry at the wrong tab (#673).
+    pub tab_mru: Vec<(GroupId, TabId)>,
 
     /// Back/forward tab navigation history.
     /// Each entry is (GroupId, TabId) at the time of the switch.
@@ -3668,7 +3670,7 @@ impl Engine {
             cwd,
             tab_switcher_open: false,
             tab_switcher_selected: 0,
-            tab_mru: vec![(GroupId(0), 0)],
+            tab_mru: vec![(GroupId(0), TabId(1))],
             tab_nav_history: vec![(GroupId(0), TabId(1))],
             tab_nav_index: 0,
             tab_nav_navigating: false,

src/core/engine/tests.rs

@@ -16841,6 +16841,314 @@ fn test_tab_nav_cross_group() {
     assert!(reached_group2, "forward nav should cross groups");
 }
 
+// ── #673: close-tab MRU successor ───────────────────────────────────────────
+//
+// `Engine::close_tab` used to pick the next active tab by index arithmetic
+// alone: whatever tab shifted into the closed slot became active. The
+// `tab_mru` stack was maintained on every activation but never consulted.
+// These tests pin the fix: the successor comes from `tab_mru` (falling back
+// to positional adjacency only when the MRU has nothing usable), and
+// `tab_mru` itself is keyed by `TabId` rather than a positional index so
+// reordering/moving tabs can't silently repoint an entry at the wrong tab.
+
+#[test]
+fn test_close_tab_picks_mru_successor_not_positional_neighbour() {
+    // The naive repro ([A, B, C], close C then B => A) passes by adjacency
+    // accident even with the bug fully present — see the issue's own
+    // warning about this. This uses a *non-adjacent* prior tab so the
+    // defect actually shows:
+    //
+    //   tabs: [X, A, Y, Z]     active = A
+    //   open B  -> [X, A, Y, Z, B]      active = B
+    //   open C  -> [X, A, Y, Z, B, C]   active = C
+    //   close C -> active = B           (adjacency: idx 5 clamped to 4)
+    //   close B -> MRU-correct: A       (buggy adjacency: idx 4 clamped to
+    //                                    3, landing on Z instead)
+    let mut engine = Engine::new(); // X, idx 0
+    engine.new_tab(None); // A, idx 1
+    engine.new_tab(None); // Y, idx 2
+    engine.new_tab(None); // Z, idx 3
+    engine.goto_tab(1); // back to A
+    let a_id = engine.active_tab().id;
+    assert_eq!(engine.active_group().active_tab, 1);
+
+    engine.new_tab(None); // B, idx 4, active
+    engine.new_tab(None); // C, idx 5, active
+    assert_eq!(engine.active_group().tabs.len(), 6);
+    assert_eq!(engine.active_group().active_tab, 5);
+
+    assert!(engine.close_tab(), "closing C should succeed"); // close C
+    assert_eq!(engine.active_group().tabs.len(), 5);
+
+    assert!(engine.close_tab(), "closing B should succeed"); // close B
+    assert_eq!(engine.active_group().tabs.len(), 4);
+    assert_eq!(
+        engine.active_tab().id,
+        a_id,
+        "closing B should reactivate A (the MRU successor), not whichever \
+         tab happens to shift into the closed slot"
+    );
+}
+
+#[test]
+fn test_close_tab_at_background_tab_does_not_change_active_tab() {
+    // Right-click "Close" on a tab that is not the group's active tab must
+    // not steal focus. `close_tab_at` used to switch to the target
+    // group/tab *before* closing it, which activated the closed tab's
+    // neighbour as a side effect and polluted the MRU/nav-history stacks on
+    // the way out.
+    let mut engine = Engine::new();
+    engine.new_tab(None); // idx 1
+    engine.new_tab(None); // idx 2
+    engine.new_tab(None); // idx 3, active
+    engine.goto_tab(1); // make idx 1 active
+    let active_id = engine.active_tab().id;
+    let group = engine.active_group;
+
+    let closed = engine.close_tab_at(group, 3); // background tab, idx 3
+    assert!(closed);
+    assert_eq!(engine.active_group().tabs.len(), 3);
+    assert_eq!(
+        engine.active_tab().id,
+        active_id,
+        "closing a background tab must not change the active tab"
+    );
+    assert_eq!(
+        engine.active_group().active_tab,
+        1,
+        "active index is unaffected when the removed tab was positioned after it"
+    );
+}
+
+#[test]
+fn test_close_tab_at_background_tab_shifts_active_index_when_before_it() {
+    // A background tab positioned *before* the active tab still must not
+    // change which tab is active — but the active tab's numeric index has
+    // to shift down by one to keep pointing at the same tab.
+    let mut engine = Engine::new();
+    engine.new_tab(None); // idx 1
+    engine.new_tab(None); // idx 2, active
+    let active_id = engine.active_tab().id;
+    let group = engine.active_group;
+
+    let closed = engine.close_tab_at(group, 0); // background tab, idx 0
+    assert!(closed);
+    assert_eq!(engine.active_tab().id, active_id);
+    assert_eq!(engine.active_group().active_tab, 1);
+}
+
+#[test]
+fn test_reorder_tab_does_not_corrupt_close_successor() {
+    // #673 defect 3: `tab_mru` used to store a positional `(GroupId,
+    // usize)` index, so dragging a tab elsewhere in the bar silently
+    // repointed every MRU entry sitting at or after the moved slot. This
+    // reproduces exactly that shape: record that A is the desired successor
+    // while A sits at index 1, then reorder the tab list so a *different*
+    // tab (Y) ends up at index 1. If `tab_mru` were still index-keyed, the
+    // successor lookup would resolve to Y; keyed by `TabId`, it must still
+    // resolve to A.
+    let mut engine = Engine::new(); // W, idx 0
+    engine.new_tab(None); // X, idx 1
+    engine.new_tab(None); // Y, idx 2
+    engine.new_tab(None); // Z, idx 3
+
+    engine.goto_tab(1); // active = X, MRU front = X
+    let x_id = engine.active_tab().id;
+    engine.goto_tab(3); // active = Z, MRU: [Z, X, ...]
+    let z_id = engine.active_tab().id;
+
+    let group = engine.active_group;
+    // Drag W (idx 0) to the end. After this, X sits at idx 0 (a completely
+    // different index than when its MRU entry was recorded) and Y — a tab
+    // that was never the intended successor — moves into X's *old* slot
+    // (idx 1). (Dragging also focuses the moved tab, W, per normal
+    // drag-and-drop UX — unrelated to the MRU logic under test here.)
+    engine.reorder_tab_in_group(group, 0, 3);
+    assert_eq!(
+        engine.active_group().tabs.iter().position(|t| t.id == x_id),
+        Some(0),
+        "X should now be at index 0 after the drag"
+    );
+
+    // Navigate to Z (now at idx 2, not its pre-drag idx 3) and close it.
+    // Z's MRU successor, recorded while X sat at idx 1, is X — even though
+    // the tab now sitting at idx 1 is Y, not X.
+    let z_idx = engine
+        .active_group()
+        .tabs
+        .iter()
+        .position(|t| t.id == z_id)
+        .unwrap();
+    engine.goto_tab(z_idx);
+    engine.close_tab(); // close Z
+    assert_eq!(
+        engine.active_tab().id,
+        x_id,
+        "the close successor must resolve X by TabId even though a drag \
+         moved a different tab into X's old positional slot"
+    );
+}
+
+#[test]
+fn test_close_across_two_groups_picks_correct_successor_per_group() {
+    use crate::core::window::SplitDirection;
+
+    let mut engine = Engine::new();
+    // Group 1: the same non-adjacent repro as above.
+    engine.new_tab(None); // A, idx 1
+    engine.new_tab(None); // Y, idx 2
+    engine.new_tab(None); // Z, idx 3
+    engine.goto_tab(1); // active = A
+    let a_id = engine.active_tab().id;
+    let group1 = engine.active_group;
+
+    // Split into a second group (starts with one tab, P).
+    engine.open_editor_group(SplitDirection::Vertical);
+    let group2 = engine.active_group;
+    assert_ne!(group1, group2);
+    let p_id = engine.active_tab().id;
+    engine.new_tab(None); // Q, active in group 2
+    let q_id = engine.active_tab().id;
+
+    // Back in group 1: replay the B/C open-then-close sequence. Group 2's
+    // MRU entries must not leak into group 1's successor decision.
+    engine.active_group = group1;
+    engine.new_tab(None); // B
+    engine.new_tab(None); // C
+    engine.close_tab(); // close C -> B
+    engine.close_tab(); // close B -> A
+    assert_eq!(
+        engine.active_tab().id,
+        a_id,
+        "group 1's own MRU should resolve the successor, unaffected by group 2"
+    );
+
+    // Group 2: closing Q must fall back to P via group 2's own MRU.
+    engine.active_group = group2;
+    assert_eq!(engine.active_tab().id, q_id);
+    engine.close_tab();
+    assert_eq!(
+        engine.active_tab().id,
+        p_id,
+        "group 2's successor must come from its own MRU, not group 1's"
+    );
+}
+
+#[test]
+fn test_close_tab_at_active_tab_of_unfocused_group_uses_own_mru_successor() {
+    // Right-click "Close" on the *active* tab of a background (unfocused)
+    // split group must route through `close_active_tab_of_background_group`,
+    // not `close_tab` (`close_tab` only ever closes `self.active_group`'s own
+    // active tab). It must (a) never move global focus or touch the focused
+    // group's own active tab, and (b) pick its successor from that
+    // background group's own MRU stack. This reuses the same non-adjacent
+    // repro as `test_close_tab_picks_mru_successor_not_positional_neighbour`
+    // so the MRU -- not adjacency -- is what's actually discriminating.
+    use crate::core::window::SplitDirection;
+
+    let mut engine = Engine::new(); // group1: X, idx 0
+    engine.new_tab(None); // A, idx 1
+    engine.new_tab(None); // Y, idx 2
+    engine.new_tab(None); // Z, idx 3
+    engine.goto_tab(1); // active = A
+    let a_id = engine.active_tab().id;
+    let group1 = engine.active_group;
+
+    engine.new_tab(None); // B, idx 4, active
+    engine.new_tab(None); // C, idx 5, active
+    assert_eq!(engine.active_group().tabs.len(), 6);
+
+    // Split off a second group and focus it. group1 is now the background
+    // group; nothing from here on touches group1's MRU (tab_mru_touch only
+    // ever records `self.active_group`).
+    engine.open_editor_group(SplitDirection::Vertical);
+    let group2 = engine.active_group;
+    assert_ne!(group1, group2);
+    let focused_active_before = engine.active_tab().id; // group2's only tab, P
+
+    // Right-click close on group1's active tab (C) while group2 is focused.
+    let closed = engine.close_tab_at(group1, 5);
+    assert!(closed);
+    assert_eq!(
+        engine.active_group, group2,
+        "closing a background group's active tab must not move global focus"
+    );
+    assert_eq!(
+        engine.active_tab().id,
+        focused_active_before,
+        "the focused group's own active tab must be untouched"
+    );
+    assert_eq!(engine.editor_groups[&group1].tabs.len(), 5);
+    let g1_active_idx = engine.editor_groups[&group1].active_tab;
+
+    // Close again: group1's active tab is now B. The MRU-correct successor
+    // is A (active before B/C were opened); positional adjacency alone
+    // would instead land on Z.
+    let closed2 = engine.close_tab_at(group1, g1_active_idx);
+    assert!(closed2);
+    assert_eq!(
+        engine.active_group, group2,
+        "second background close must also not move global focus"
+    );
+    assert_eq!(engine.active_tab().id, focused_active_before);
+    let g1 = &engine.editor_groups[&group1];
+    assert_eq!(g1.tabs.len(), 4);
+    assert_eq!(
+        g1.tabs[g1.active_tab].id, a_id,
+        "background group's active-tab close must pick its own MRU \
+         successor (A), not positional adjacency (which would land on Z)"
+    );
+}
+
+#[test]
+fn test_close_tab_at_last_tab_of_unfocused_group_removes_whole_group() {
+    // Right-click "Close" on the active tab of a background group that is
+    // down to its last tab must remove the whole group -- mirroring
+    // `close_editor_group`'s single-tab-group fallback -- without touching
+    // global focus, and must prune that group's tab_mru/tab_nav_history
+    // entries so a stale group id can never surface as a successor later.
+    use crate::core::window::SplitDirection;
+
+    let mut engine = Engine::new();
+    let group1 = engine.active_group;
+    // Touch nav history / MRU for group1 before splitting so we can assert
+    // they're pruned once the group is gone.
+    engine.goto_tab(0);
+    assert!(engine.tab_nav_history.iter().any(|&(g, _)| g == group1));
+    assert!(engine.tab_mru.iter().any(|&(g, _)| g == group1));
+
+    engine.open_editor_group(SplitDirection::Vertical);
+    let group2 = engine.active_group;
+    assert_ne!(group1, group2);
+    let focused_active_before = engine.active_tab().id;
+
+    assert_eq!(engine.editor_groups[&group1].tabs.len(), 1);
+    let closed = engine.close_tab_at(group1, 0);
+    assert!(closed);
+
+    assert!(
+        !engine.editor_groups.contains_key(&group1),
+        "closing the last tab of a background group must remove the whole group"
+    );
+    assert_eq!(
+        engine.active_group, group2,
+        "removing a background group must not move global focus"
+    );
+    assert_eq!(
+        engine.active_tab().id,
+        focused_active_before,
+        "the focused group's own active tab must be untouched"
+    );
+    assert!(
+        engine.tab_mru.iter().all(|&(g, _)| g != group1),
+        "tab_mru must be pruned of the removed group's entries"
+    );
+    assert!(
+        engine.tab_nav_history.iter().all(|&(g, _)| g != group1),
+        "tab_nav_history must be pruned of the removed group's entries"
+    );
+}
+
 // ── Git branch picker tests ─────────────────────────────────────────────────
 
 #[test]