diff --git a/src/core/engine/mod.rs b/src/core/engine/mod.rs index 02119a59..a28462cf 100644 --- a/src/core/engine/mod.rs +++ b/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, diff --git a/src/core/engine/tests.rs b/src/core/engine/tests.rs index 653c305c..c0bdf0dc 100644 --- a/src/core/engine/tests.rs +++ b/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] diff --git a/src/core/engine/windows.rs b/src/core/engine/windows.rs index 9e2329d4..7c29e548 100644 --- a/src/core/engine/windows.rs +++ b/src/core/engine/windows.rs @@ -341,22 +341,14 @@ impl Engine { } } - /// Close the current tab. Returns true if closed. - pub fn close_tab(&mut self) -> bool { - if self.active_group().tabs.len() <= 1 { - // If there is a second group, close this group instead of erroring. - if !self.group_layout.is_single_group() { - self.close_editor_group(); - return true; - } - self.message = "Cannot close last tab".to_string(); - return false; - } - + /// Remove tab `tab_idx` from `group_id`: tears down its windows, cleans + /// up orphaned buffers, and strips it from nav history and the MRU + /// stack. Does **not** decide or touch `active_tab` / global focus — + /// callers own that policy. Returns the `TabId` that was removed. + fn remove_tab_raw(&mut self, group_id: GroupId, tab_idx: usize) -> TabId { // Collect the buffer IDs of windows being closed so we can clean them // up from the buffer manager if nothing else references them. - let active_tab_idx = self.active_group().active_tab; - let window_ids: Vec = self.active_group().tabs[active_tab_idx].window_ids(); + let window_ids: Vec = self.editor_groups[&group_id].tabs[tab_idx].window_ids(); let closed_buffer_ids: Vec = window_ids .iter() .filter_map(|wid| self.windows.get(wid).map(|w| w.buffer_id)) @@ -377,35 +369,24 @@ impl Engine { } } - let closed_group = self.active_group; - let closed_tab_id = self.active_group().tabs[active_tab_idx].id; - self.active_group_mut().tabs.remove(active_tab_idx); + let closed_tab_id = self.editor_groups[&group_id].tabs[tab_idx].id; + self.editor_groups + .get_mut(&group_id) + .unwrap() + .tabs + .remove(tab_idx); // Remove the closed tab from nav history. self.tab_nav_history - .retain(|&(g, t)| !(g == closed_group && t == closed_tab_id)); + .retain(|&(g, t)| !(g == group_id && t == closed_tab_id)); if self.tab_nav_index >= self.tab_nav_history.len() { self.tab_nav_index = self.tab_nav_history.len().saturating_sub(1); } - // Remove the closed tab from MRU and adjust indices + // Remove the closed tab from MRU. No index fixup needed — entries + // are keyed by TabId, so removing one tab never repoints another. self.tab_mru - .retain(|&(g, idx)| !(g == closed_group && idx == active_tab_idx)); - for entry in &mut self.tab_mru { - if entry.0 == closed_group && entry.1 > active_tab_idx { - entry.1 -= 1; - } - } - - // Adjust active tab index - let tabs_len = self.active_group().tabs.len(); - if self.active_group().active_tab >= tabs_len { - self.active_group_mut().active_tab = tabs_len - 1; - } - self.tab_mru_touch(); - // Ensure the new active tab's window state is consistent. - self.repair_active_window(); - self.ensure_active_tab_visible(); + .retain(|&(g, t)| !(g == group_id && t == closed_tab_id)); // Remove any buffers that are no longer referenced by any window. // This prevents orphaned dirty buffers from falsely triggering `:qa` @@ -423,34 +404,147 @@ impl Engine { } } + closed_tab_id + } + + /// Determine which tab should become active in `group_id` after closing + /// `closed_tab_id`. Reads the MRU stack for the most recently used tab + /// that still exists in the group (other than the one being closed) and + /// returns `None` when the MRU has nothing usable, so the caller can + /// fall back to positional adjacency (#673). + pub(crate) fn successor_tab_after_close( + &self, + group_id: GroupId, + closed_tab_id: TabId, + ) -> Option { + let group = self.editor_groups.get(&group_id)?; + self.tab_mru.iter().find_map(|&(g, tid)| { + (g == group_id && tid != closed_tab_id && group.tabs.iter().any(|t| t.id == tid)) + .then_some(tid) + }) + } + + /// Close the current tab. Returns true if closed. + pub fn close_tab(&mut self) -> bool { + if self.active_group().tabs.len() <= 1 { + // If there is a second group, close this group instead of erroring. + if !self.group_layout.is_single_group() { + self.close_editor_group(); + return true; + } + self.message = "Cannot close last tab".to_string(); + return false; + } + + let group_id = self.active_group; + let active_tab_idx = self.active_group().active_tab; + let closed_tab_id = self.active_group().tabs[active_tab_idx].id; + + // Decide the successor before mutating the tab list: MRU-first, + // falling back to whatever adjacency shifts into the closed slot. + let successor = self.successor_tab_after_close(group_id, closed_tab_id); + + self.remove_tab_raw(group_id, active_tab_idx); + + let tabs_len = self.active_group().tabs.len(); + let new_idx = successor + .and_then(|tid| self.active_group().tabs.iter().position(|t| t.id == tid)) + .unwrap_or_else(|| active_tab_idx.min(tabs_len.saturating_sub(1))); + self.active_group_mut().active_tab = new_idx; + + self.tab_mru_touch(); + // Ensure the new active tab's window state is consistent. + self.repair_active_window(); + self.ensure_active_tab_visible(); + true } - /// Close a specific tab by group and index. Used for right-click "Close" on non-active tabs. + /// Close a specific tab by group and index. Used for right-click "Close" + /// on a tab bar entry. + /// + /// Closing a tab that is **not** the group's active tab never changes + /// `active_tab` or steals global focus (#673) — it just removes the tab + /// in place. Closing a group's own active tab picks a successor the same + /// way `close_tab` does, but only moves global focus to `group_id` when + /// that group was already focused. pub fn close_tab_at(&mut self, group_id: GroupId, tab_idx: usize) -> bool { - // Switch to the target group/tab, then close it. - if !self.editor_groups.contains_key(&group_id) { + let Some(group) = self.editor_groups.get(&group_id) else { return false; - } - let tabs_len = self.editor_groups[&group_id].tabs.len(); - if tab_idx >= tabs_len { + }; + if tab_idx >= group.tabs.len() { return false; } - let prev_group = self.active_group; - let prev_tab = self.active_group().active_tab; - self.active_group = group_id; - self.editor_groups.get_mut(&group_id).unwrap().active_tab = tab_idx; - let closed = self.close_tab(); - // If we didn't close (last tab), restore. - if !closed { - self.active_group = prev_group; - if let Some(g) = self.editor_groups.get_mut(&prev_group) { - if prev_tab < g.tabs.len() { - g.active_tab = prev_tab; + + if tab_idx == group.active_tab { + if group_id == self.active_group { + return self.close_tab(); + } + return self.close_active_tab_of_background_group(group_id); + } + + // Closing a background tab: remove it in place. Active tab index, + // MRU order, and global focus are all left untouched aside from the + // index shift needed to keep `active_tab` pointing at the same tab. + self.remove_tab_raw(group_id, tab_idx); + if let Some(g) = self.editor_groups.get_mut(&group_id) { + if tab_idx < g.active_tab { + g.active_tab -= 1; + } + } + if group_id == self.active_group { + self.ensure_active_tab_visible(); + } + true + } + + /// Close the active tab of a group that is not the globally focused + /// group, without moving focus there. Mirrors `close_tab`'s successor + /// policy and last-tab-in-group handling, scoped to `group_id`. + fn close_active_tab_of_background_group(&mut self, group_id: GroupId) -> bool { + let Some(group) = self.editor_groups.get(&group_id) else { + return false; + }; + let active_tab_idx = group.active_tab; + + if group.tabs.len() <= 1 { + if self.group_layout.is_single_group() { + return false; + } + // Remove the whole (unfocused) group; global focus is untouched. + if let Some(g) = self.editor_groups.get(&group_id) { + let window_ids: Vec = + g.tabs.iter().flat_map(|t| t.window_ids()).collect(); + for wid in window_ids { + self.windows.remove(&wid); } } + self.editor_groups.remove(&group_id); + self.group_layout.remove(group_id); + self.tab_mru.retain(|&(g, _)| g != group_id); + self.tab_nav_history.retain(|&(g, _)| g != group_id); + if self.tab_nav_index >= self.tab_nav_history.len() { + self.tab_nav_index = self.tab_nav_history.len().saturating_sub(1); + } + return true; } - closed + + let closed_tab_id = group.tabs[active_tab_idx].id; + let successor = self.successor_tab_after_close(group_id, closed_tab_id); + + self.remove_tab_raw(group_id, active_tab_idx); + + let tabs_len = self.editor_groups[&group_id].tabs.len(); + let new_idx = successor + .and_then(|tid| { + self.editor_groups[&group_id] + .tabs + .iter() + .position(|t| t.id == tid) + }) + .unwrap_or_else(|| active_tab_idx.min(tabs_len.saturating_sub(1))); + self.editor_groups.get_mut(&group_id).unwrap().active_tab = new_idx; + true } /// Close all tabs in the current group except the active one. @@ -471,6 +565,13 @@ impl Engine { // After closing, the active_tab might have shifted. } // Ensure the originally active tab (now the only one) is selected. + // Each `close_tab()` call above already ran the MRU-successor logic + // and its own `tab_mru_touch()` for the *intermediate* tabs this + // loop temporarily made active — those entries are transient and get + // overwritten below. The final `tab_mru_touch()` call MUST stay + // after this `active_tab = 0` assignment; reordering them would + // leave the front of the MRU stack pointing at a tab this function + // never intended to keep active. self.active_group_mut().active_tab = 0; self.tab_mru_touch(); self.repair_active_window(); @@ -488,6 +589,10 @@ impl Engine { self.active_group_mut().active_tab = i; self.close_tab(); } + // As in `close_other_tabs`: this `tab_mru_touch()` must stay after + // pinning `active_tab` back to the original tab, so it overwrites + // whatever transient MRU entries the loop's intermediate closes + // wrote for tabs that were never meant to end up active. self.active_group_mut().active_tab = active_tab_idx; self.tab_mru_touch(); self.repair_active_window(); @@ -505,6 +610,7 @@ impl Engine { self.active_group_mut().active_tab = 0; self.close_tab(); } + // See `close_other_tabs`: pin-then-touch ordering is load-bearing. self.active_group_mut().active_tab = 0; self.tab_mru_touch(); self.repair_active_window(); @@ -541,6 +647,9 @@ impl Engine { self.close_tab(); } // Recalculate the active tab (original one shifted down by removed tabs below it). + // See `close_other_tabs`: this `tab_mru_touch()` must stay after the + // active-tab recalculation above, so it overwrites the transient MRU + // entries the loop's intermediate closes wrote along the way. let remaining = self.active_group().tabs.len(); if self.active_group().active_tab >= remaining { self.active_group_mut().active_tab = remaining.saturating_sub(1); @@ -1426,9 +1535,9 @@ impl Engine { } } - /// Record the current (group, tab_index) as the most recently used tab. + /// Record the current (group, tab_id) as the most recently used tab. pub fn tab_mru_touch(&mut self) { - let entry = (self.active_group, self.active_group().active_tab); + let entry = (self.active_group, self.active_tab().id); self.tab_mru.retain(|e| *e != entry); self.tab_mru.insert(0, entry); } @@ -1556,13 +1665,13 @@ impl Engine { /// Calling again toggles back (Vim behaviour). pub fn goto_last_accessed_tab(&mut self) { // Prune stale entries - self.tab_mru.retain(|&(g, idx)| { + self.tab_mru.retain(|&(g, tid)| { self.editor_groups .get(&g) - .is_some_and(|grp| idx < grp.tabs.len()) + .is_some_and(|grp| grp.tabs.iter().any(|t| t.id == tid)) }); // Ensure current is at index 0 - let current = (self.active_group, self.active_group().active_tab); + let current = (self.active_group, self.active_tab().id); if self.tab_mru.first() != Some(¤t) { self.tab_mru.retain(|e| *e != current); self.tab_mru.insert(0, current); @@ -1570,8 +1679,12 @@ impl Engine { if self.tab_mru.len() < 2 { return; // No previous tab to jump to } - let (group_id, tab_idx) = self.tab_mru[1]; - if self.editor_groups.contains_key(&group_id) { + let (group_id, tab_id) = self.tab_mru[1]; + let tab_idx = self + .editor_groups + .get(&group_id) + .and_then(|g| g.tabs.iter().position(|t| t.id == tab_id)); + if let Some(tab_idx) = tab_idx { self.active_group = group_id; self.active_group_mut().active_tab = tab_idx; self.line_annotations.clear(); @@ -1590,22 +1703,22 @@ impl Engine { self.dismiss_editor_hover(); // Build a clean MRU list: only include entries that still exist - self.tab_mru.retain(|&(g, idx)| { + self.tab_mru.retain(|&(g, tid)| { self.editor_groups .get(&g) - .is_some_and(|grp| idx < grp.tabs.len()) + .is_some_and(|grp| grp.tabs.iter().any(|t| t.id == tid)) }); // Ensure the current tab is at index 0 - let current = (self.active_group, self.active_group().active_tab); + let current = (self.active_group, self.active_tab().id); if self.tab_mru.first() != Some(¤t) { self.tab_mru.retain(|e| *e != current); self.tab_mru.insert(0, current); } // Also add any tabs not yet in MRU (e.g. from before MRU tracking started) for (&gid, group) in &self.editor_groups { - for idx in 0..group.tabs.len() { - if !self.tab_mru.contains(&(gid, idx)) { - self.tab_mru.push((gid, idx)); + for tab in &group.tabs { + if !self.tab_mru.contains(&(gid, tab.id)) { + self.tab_mru.push((gid, tab.id)); } } } @@ -1653,8 +1766,12 @@ impl Engine { return; } let idx = self.tab_switcher_selected; - if let Some(&(group_id, tab_idx)) = self.tab_mru.get(idx) { - if self.editor_groups.contains_key(&group_id) { + if let Some(&(group_id, tab_id)) = self.tab_mru.get(idx) { + let tab_idx = self + .editor_groups + .get(&group_id) + .and_then(|g| g.tabs.iter().position(|t| t.id == tab_id)); + if let Some(tab_idx) = tab_idx { self.active_group = group_id; self.active_group_mut().active_tab = tab_idx; self.tab_mru_touch(); @@ -1732,9 +1849,9 @@ impl Engine { pub fn tab_switcher_items(&self) -> Vec<(String, String, bool)> { self.tab_mru .iter() - .filter_map(|&(gid, tab_idx)| { + .filter_map(|&(gid, tab_id)| { let group = self.editor_groups.get(&gid)?; - let tab = group.tabs.get(tab_idx)?; + let tab = group.tabs.iter().find(|t| t.id == tab_id)?; let win = self.windows.get(&tab.active_window)?; let state = self.buffer_manager.get(win.buffer_id)?; let name = state.display_name(); @@ -2001,6 +2118,12 @@ impl Engine { } /// Move the current tab from the active group to the next group. + /// + /// Any `tab_mru`/`tab_nav_history` entry for this tab still names the + /// old group; since both stacks are keyed by `(GroupId, TabId)` (#673), + /// such an entry simply stops matching anything (the tab isn't in that + /// group anymore) rather than being misread as pointing at whatever tab + /// now occupies its old slot. No explicit fixup is required. pub fn move_tab_to_other_group(&mut self) { if self.group_layout.is_single_group() { return; @@ -2055,6 +2178,9 @@ impl Engine { } /// Move a tab from one group to another at a specific insertion index. + /// + /// See `move_tab_to_other_group` re: `tab_mru` — stale cross-group + /// entries harmlessly stop matching rather than repointing (#673). pub fn move_tab_to_target_group_at( &mut self, src_group: GroupId, @@ -2093,6 +2219,9 @@ impl Engine { } /// Move a tab out of its group into a new split adjacent to `target_group`. + /// + /// See `move_tab_to_other_group` re: `tab_mru` — stale cross-group + /// entries harmlessly stop matching rather than repointing (#673). pub fn move_tab_to_new_split( &mut self, src_group: GroupId, @@ -2171,6 +2300,10 @@ impl Engine { } /// Reorder a tab within its group. + /// + /// No `tab_mru` fixup is needed here: the MRU stack is keyed by `TabId` + /// (#673), and reordering never changes a tab's id — only its position + /// in `tabs`, which the MRU doesn't track. pub fn reorder_tab_in_group(&mut self, group_id: GroupId, from_idx: usize, to_idx: usize) { if let Some(g) = self.editor_groups.get_mut(&group_id) { if from_idx >= g.tabs.len() { diff --git a/src/tui_main/shell_app.rs b/src/tui_main/shell_app.rs index 6e89be6b..e867dbec 100644 --- a/src/tui_main/shell_app.rs +++ b/src/tui_main/shell_app.rs @@ -6637,4 +6637,134 @@ mod tests { driver.screen() ); } + + /// #673 black-box regression: closing a tab must activate the MRU + /// successor, not whatever tab is positionally adjacent to the closed + /// slot. Read through the *rendered* tab bar (not just engine state) so + /// the test proves what the user actually sees. + /// + /// The naive repro ([A, B, C], close C then B => A) passes by adjacency + /// accident even with the bug fully present (see the issue), so this + /// uses a non-adjacent prior tab: tabs [W, X, A, Y, Z], active pinned to + /// A, then open B and C (both far from A) and close them in turn. The + /// MRU-correct successor is A; pure positional adjacency lands on Z. + /// + /// The active tab is distinguished in the tab bar only by background + /// colour (`theme.tab_active_bg` vs `theme.tab_bar_bg` — see + /// `quadraui::tui::tab_bar::draw_tab_bar`'s doc comment), not by any + /// text difference, so this reads `style_at` on the resolved tab-label + /// cell rather than scanning `screen()` for a marker. + #[test] + fn close_tab_after_close_reactivates_mru_tab_not_positional_neighbour() { + let dir = std::env::temp_dir().join(format!( + "vimcode_close_tab_673_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&dir).unwrap(); + let make = |name: &str| -> std::path::PathBuf { + let p = dir.join(name); + std::fs::write(&p, "content").unwrap(); + p + }; + let tab_w = make("tab_w.txt"); + let tab_x = make("tab_x.txt"); + let tab_a = make("tab_a.txt"); + let tab_y = make("tab_y.txt"); + let tab_z = make("tab_z.txt"); + let tab_b = make("tab_b.txt"); + let tab_c = make("tab_c.txt"); + + let mut app = TuiShellApp::new(None); + // Tabs open in order after the pre-existing [No Name] tab: + // [No Name], W, X, A, Y, Z. + app.engine.new_tab(Some(&tab_w)); + app.engine.new_tab(Some(&tab_x)); + app.engine.new_tab(Some(&tab_a)); + app.engine.new_tab(Some(&tab_y)); + app.engine.new_tab(Some(&tab_z)); + + // Pin the active tab to A — not adjacent to where B/C will land. + let a_idx = app + .engine + .active_group() + .tabs + .iter() + .position(|t| { + app.engine + .windows + .get(&t.active_window) + .and_then(|w| app.engine.buffer_manager.get(w.buffer_id)) + .and_then(|s| s.file_path.as_ref()) + == Some(&tab_a) + }) + .expect("tab_a.txt should be open"); + app.engine.goto_tab(a_idx); + + app.engine.new_tab(Some(&tab_b)); + app.engine.new_tab(Some(&tab_c)); + + // Close C then B — the buggy adjacency-only logic lands on Z here; + // the MRU-aware fix lands back on A. + assert!(app.engine.close_tab()); + assert!(app.engine.close_tab()); + + let driver = driver_with_shell(app, config(), 200, 24); + let screen = driver.screen(); + + // Only B and C were closed — X, A, Y, Z all remain in the tab bar + // throughout; this test is about which one is *active*, not which + // ones survive. + assert!( + driver.find_bounds("tab_b.txt").is_none(), + "tab_b.txt should have been closed; screen:\n{screen}" + ); + assert!( + driver.find_bounds("tab_c.txt").is_none(), + "tab_c.txt should have been closed; screen:\n{screen}" + ); + + let a_bounds = driver + .find_bounds("tab_a.txt") + .unwrap_or_else(|| panic!("tab_a.txt should still be open; screen:\n{screen}")); + let x_bounds = driver + .find_bounds("tab_x.txt") + .unwrap_or_else(|| panic!("tab_x.txt should still be open; screen:\n{screen}")); + let z_bounds = driver + .find_bounds("tab_z.txt") + .unwrap_or_else(|| panic!("tab_z.txt should still be open; screen:\n{screen}")); + + let a_style = driver + .style_at(a_bounds.x as u16, a_bounds.y as u16) + .expect("a_bounds should be on-screen"); + // tab_x.txt is never touched after its initial open, so it is + // guaranteed inactive throughout — the reference "inactive" style. + // Comparing A only against Z (the buggy answer) would pass either + // way the bug resolves (whichever one is active simply differs + // from the other) — see #553 on tests that pass regardless of the + // bug. Anchoring on a third, definitely-inactive tab breaks that + // symmetry: A must differ from it (A is active) and Z must match + // it (Z is *not* active, unlike what the adjacency bug would do). + let x_style = driver + .style_at(x_bounds.x as u16, x_bounds.y as u16) + .expect("x_bounds should be on-screen"); + let z_style = driver + .style_at(z_bounds.x as u16, z_bounds.y as u16) + .expect("z_bounds should be on-screen"); + assert_ne!( + a_style.bg, x_style.bg, + "tab_a.txt must be painted with the active-tab background — \ + the MRU successor after closing C then B is A; screen:\n{screen}" + ); + assert_eq!( + z_style.bg, x_style.bg, + "tab_z.txt must NOT be painted as active — that's the wrong \ + answer pure positional adjacency would pick; screen:\n{screen}" + ); + + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/tests/tab_switcher.rs b/tests/tab_switcher.rs index 2d47d1a2..7be66265 100644 --- a/tests/tab_switcher.rs +++ b/tests/tab_switcher.rs @@ -122,7 +122,8 @@ fn tab_mru_order_reflects_access() { // Selected=1 means we'd switch to tab2 (second in MRU) assert_eq!(e.tab_switcher_selected, 1); let entry = e.tab_mru[1]; - assert_eq!(entry.1, 2, "second MRU entry should be tab 2"); + let tab2_id = e.active_group().tabs[2].id; + assert_eq!(entry.1, tab2_id, "second MRU entry should be tab 2"); } #[test] @@ -164,7 +165,7 @@ fn tab_switcher_confirm_then_reopen_has_correct_mru() { // Ctrl+Tab + Return → switch to tab 1 (second MRU entry) e.handle_key("Tab", None, true); press_key(&mut e, "Return"); - let switched_to = e.active_group().active_tab; + let switched_to_id = e.active_tab().id; // Now open again — the tab we just left (tab 2) should be at MRU[1] e.handle_key("Tab", None, true); @@ -173,9 +174,9 @@ fn tab_switcher_confirm_then_reopen_has_correct_mru() { // The second entry should NOT be the same as where we are now assert_ne!( second_entry.1, - e.active_group().active_tab, + e.active_tab().id, "second MRU entry should be the previous tab" ); // It should be the tab we just left - assert_ne!(second_entry.1, switched_to); + assert_ne!(second_entry.1, switched_to_id); }