From 66e3812c8298901f2d564df979c39f39d6055df5 Mon Sep 17 00:00:00 2001 From: JDonaghy Date: Thu, 27 Aug 2026 18:57:46 +0000 Subject: [PATCH 1/2] fix(#673): close-tab activates MRU successor, not positional neighbour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Engine::close_tab picked the next active tab by index arithmetic alone — whatever tab shifted into the closed slot became active — even though tab_mru was maintained on every activation. It was never consulted. - Re-key tab_mru to (GroupId, TabId) instead of a positional index, matching tab_nav_history, so reorder/move can never silently repoint an MRU entry at the wrong tab. Deletes the index-fixup loop that used to run on every close. - Add Engine::successor_tab_after_close(group, closed_tab_id), the one place that reads tab_mru to pick a close successor, falling back to positional adjacency only when the MRU has nothing live. Route close_tab and close_tab_at's active-tab-closing paths through it. - close_tab_at no longer switches focus before closing a background tab: closing a tab that isn't its group's active tab now just removes it in place, leaving active_tab/global focus untouched. Closing the active tab of an unfocused group picks its own MRU-successor without stealing global focus either. Tests (src/core/engine/tests.rs): the non-adjacent repro from the issue (open two tabs on top of a tab that isn't their neighbour, close both, land back on the original tab); background-tab close leaving the active tab alone; a drag/reorder that moves a different tab into the tracked tab's old index slot; and successor selection staying scoped per editor group. A TuiDriver test in src/tui_main/shell_app.rs pins the same non-adjacent repro at the black-box tier, reading the rendered tab bar's active-tab background style (the only way the active tab is distinguished visually) rather than engine state. All five engine tests and the driver test were confirmed to fail against the pre-fix code before the fix landed. Co-Authored-By: Claude Sonnet 5 --- src/core/engine/mod.rs | 10 +- src/core/engine/tests.rs | 193 +++++++++++++++++++++++++++ src/core/engine/windows.rs | 258 +++++++++++++++++++++++++++---------- src/tui_main/shell_app.rs | 130 +++++++++++++++++++ tests/tab_switcher.rs | 9 +- 5 files changed, 522 insertions(+), 78 deletions(-) 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..19f3057e 100644 --- a/src/core/engine/tests.rs +++ b/src/core/engine/tests.rs @@ -16841,6 +16841,199 @@ 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" + ); +} + // ── Git branch picker tests ───────────────────────────────────────────────── #[test] diff --git a/src/core/engine/windows.rs b/src/core/engine/windows.rs index 9e2329d4..d6730907 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. @@ -1426,9 +1520,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 +1650,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 +1664,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 +1688,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 +1751,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 +1834,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 +2103,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 +2163,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 +2204,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 +2285,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); } From 7cc4850b222e2b8c749cce2b9bc90378d28b45c9 Mon Sep 17 00:00:00 2001 From: JDonaghy Date: Thu, 27 Aug 2026 19:59:33 +0000 Subject: [PATCH 2/2] fix(#673): cover close_active_tab_of_background_group with engine tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocking review finding: close_active_tab_of_background_group (the branch close_tab_at takes when tab_idx is the *active* tab of an *unfocused* split group — the real right-click "Close" path) had zero test coverage. Every existing close_tab_at test, including the ones added by this PR, only ever closed a background tab inside the *focused* group. Add two engine-level tests: - test_close_tab_at_active_tab_of_unfocused_group_uses_own_mru_successor: opens a split, replays the non-adjacent MRU repro in the unfocused group, and asserts global focus and the focused group's own active tab never move while the background group's successor comes from its own MRU stack. - test_close_tab_at_last_tab_of_unfocused_group_removes_whole_group: covers the group.tabs.len() <= 1 fallback that tears down the whole background group, and asserts its tab_mru/tab_nav_history entries are pruned. Also address a non-blocking review note: comment the four bulk-close paths (close_other_tabs, close_tabs_to_right, close_tabs_to_left, close_saved_tabs) to make explicit that their final tab_mru_touch() call must stay after the active-tab is pinned back, since it's what overwrites the transient MRU entries their internal close_tab() loop iterations wrote for tabs never meant to end up active. Confirmed both new tests pass against the fixed code; ran the full close_tab*/context_menu test surface plus cargo build/fmt/clippy clean. --- src/core/engine/tests.rs | 115 +++++++++++++++++++++++++++++++++++++ src/core/engine/windows.rs | 15 +++++ 2 files changed, 130 insertions(+) diff --git a/src/core/engine/tests.rs b/src/core/engine/tests.rs index 19f3057e..c0bdf0dc 100644 --- a/src/core/engine/tests.rs +++ b/src/core/engine/tests.rs @@ -17034,6 +17034,121 @@ fn test_close_across_two_groups_picks_correct_successor_per_group() { ); } +#[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 d6730907..7c29e548 100644 --- a/src/core/engine/windows.rs +++ b/src/core/engine/windows.rs @@ -565,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(); @@ -582,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(); @@ -599,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(); @@ -635,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);