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)

[JDonaghy]

Summary

Closing a tab activates its positional neighbour, not the tab you were last looking at. There is a tab_mru stack in the engine, it is maintained on every activation — and it is never consulted to choose the successor.

Reported by the user:

If I am looking at a tab, then open 2 tabs and then later close both, I want to see the tab that was at the top before I opened the new ones. That's not what happens.

Root cause

Engine::close_tab (src/core/engine/windows.rs:345) picks the next active tab by index arithmetic alone:

self.active_group_mut().tabs.remove(active_tab_idx);
...
// 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();

active_tab still holds the closed tab's index, so whatever tab shifted into that slot becomes active (the tab to the right), clamped to the last tab when the rightmost was closed. tab_mru_touch() is called after the decision and merely records whatever adjacency chose — the MRU stack is written but never read here.

⚠ The naive repro passes by accident, and a test written against it will pass with the bug fully present. With tabs [A, B, C] and A never revisited, closing C then B lands on A by pure adjacency — the right answer for the wrong reason. The defect only shows when the tab you were on is not adjacent to the new ones:

tabs: [X, A, Y, Z]     active = A (idx 1)
open B  -> [X, A, Y, Z, B]      active = B
open C  -> [X, A, Y, Z, B, C]   active = C
close C -> active = B           (idx 5 clamped to 4)
close B -> active = Z           (idx 4 clamped to 3)   <-- expected A

This is the #553 failure mode waiting to happen: a green test that never discriminated. Write the test with a non-adjacent prior tab, and verify it fails against current develop before you fix anything.

Second defect — closing a background tab steals focus

The user called this out explicitly: "you can close a tab without activating it." Today you cannot. close_tab_at(group_id, tab_idx) (windows.rs:430) is documented as "Used for right-click 'Close' on non-active tabs" and implemented as switch to the target group/tab, then close it. So closing a background tab:

• changes which tab is active as a side effect,
• pushes the closed tab onto the MRU and nav history on its way out,
• and therefore corrupts the very stack that should have decided the successor.

Closing a non-active tab must leave the active tab alone and simply remove the closed tab from the stack.

Third defect — the MRU stack is index-based and silently corrupted by reorder

tab_mru stores (GroupId, usize) — a positional index. tab_nav_history, right next to it, correctly stores (GroupId, TabId). The index form forces the fixup loop at windows.rs:392-396 on close, and it is not fixed up at all by the functions that move tabs around:

• reorder_tab_in_group (windows.rs:2174) — tabs.remove(from) + tabs.insert(to), no tab_mru touch. This is the tab drag-and-drop path (Consolidate editor-group tab drag-drop onto quadraui drop primitives (full TabGroupController adoption deferred — see quadraui#395) #515).
• move_tab_to_other_group (windows.rs:2004) — moves a tab between groups, no tab_mru touch.
• move_tab_to_target_group_at (:2058), move_tab_to_new_split (:2096) — same.

So today, dragging any tab silently re-points every MRU entry at a different tab. Fixing the close-successor without fixing this just makes the wrong answer arrive more confidently.

Recommended fix

1. Re-key tab_mru to (GroupId, TabId), matching tab_nav_history. This deletes the index-fixup loop at :392-396 and makes the reorder paths correct by construction rather than by remembering to patch them.
2. Add one well-named successor function — e.g. Engine::successor_tab_after_close(group, closed_tab_id) -> Option<(GroupId, TabId)> — that reads the MRU and falls back to adjacency only when the MRU has nothing live. Route every close path through it: close_tab, close_tab_at, close_other_tabs (:457), close_tabs_to_right (:480), close_tabs_to_left (:497), close_saved_tabs (:514), close_all_tabs (:799).
3. Make close_tab_at not activate its target. Remove the closed tab from tab_mru and tab_nav_history and leave active_tab untouched unless the tab being closed is the active one.

Keep the policy in that one function. quadraui#596 (WorkspaceController, open-N-view-one) and quadraui#597 (preview tier, lifted from vimcode) are absorbing exactly this kind of tab-lifecycle policy — vimcode#658 is the adoption issue. A single well-named engine function is liftable into WorkspaceController later; the same rule smeared across seven close paths is not. Do not wait for quadraui: this is src/core/, the shared engine both backends already consume, so the Platform-Neutrality Rule does not gate it.

Acceptance criteria

• With the non-adjacent repro above, closing C then B lands on A. Assert this at the black-box tier with a TuiDriver test (in-crate, #[cfg(test)], using the existing make_test_app fixtures) that reads the rendered tab bar — not only as an engine unit test.
• State in the PR that you ran the new test against unfixed develop and watched it fail. A test that passes both ways is worse than no test here (#448-F: GTK tab click (activate/close) dead when only ONE tab group exists #553).
• Right-click → Close on a background tab leaves the active tab unchanged.
• Dragging a tab to a new position does not change which tab a subsequent close activates.
• Closing tabs across two editor groups picks the correct successor per group.
• tab_mru contains no positional indices.
• Existing tab tests in src/core/engine/tests.rs pass with their assertions unchanged. If one needs its assertion edited, that is a behaviour change — stop and reconcile rather than adjusting the number.
• cargo build && cargo test EXIT=0.

Files

• src/core/engine/windows.rs
• src/core/engine/mod.rs
• src/core/engine/keys.rs
• src/core/engine/tests.rs
• src/tui_main/shell_app.rs

Out of scope

• The Ctrl-O / Ctrl-I jumplist — filed separately. That is a different structure (jump_list, cursor positions) in a different file, and conflating the two is how this becomes a two-session issue.
• Any change to tab ordering in the tab bar. This issue changes only which tab is activated, never where tabs sit.

[JDonaghy]

Sibling issue: #674 — the Ctrl-O/Ctrl-I jumplist half of the same user report. Both came from one bug report about navigation history, but they are different structures in different files (tab_mru in engine/windows.rs here; jump_list in engine/motions.rs there), so they are split to stay dispatchable.

Order matters: this issue lands first. #674's fix re-keys jumplist entries onto stable TabId/WindowIds and cites the index-keyed tab_mru here as the cautionary example — doing them in the other order means #674 builds on a structure this issue is about to change.

[JDonaghy]

Correction: make_test_app does not exist in vimcode

This issue's acceptance criteria say to write the black-box test using "the existing
make_test_app fixtures". There is no make_test_app in this repo — grep -rn "fn make_test_app" src/ returns nothing. That fixture belongs to coord-tui
(code-coordinator's tui/src/app.rs), and the phrasing was carried over from
code-coordinator's CLAUDE.md, which documents a different crate.

What to actually use

vimcode's TUI black-box tests live in-crate in src/tui_main/shell_app.rs under
#[cfg(test)], and are built with:

quadraui::tui::testing::driver_with_shell(TuiShellApp, ...)

Reuse the local fixtures already in that test module — app_with_sidebar_open()
(shell_app.rs:4068), app_with_ext_panel() (:4456) — and follow the existing
render_content_paints_*_via_shell_app tests (:4711 onwards) plus the
driver_click_on_* interaction tests (:4394, :4521) as the pattern.

The GTK equivalent is GtkDriver in src/gtk/testing.rs (harness from #646).

Everything else in this issue's acceptance criteria stands unchanged — including the
requirement to assert on rendered output rather than on engine state, and to state in the PR
that the new test was observed red against unfixed develop.

CLAUDE.md now names the correct entry point (aed1ca8), so a worker reading the repo rules
will get the right answer even if this comment is missed.

[JDonaghy]

Coordinator Assignment

Machine: dellserver
Repo: vimcode
Files: (unspecified)
Do not touch: (nothing else in flight)

Briefing

Issue #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)

Summary

Closing a tab activates its positional neighbour, not the tab you were last looking at. There is a tab_mru stack in the engine, it is maintained on every activation — and it is never consulted to choose the successor.

Reported by the user:

If I am looking at a tab, then open 2 tabs and then later close both, I want to see the tab that was at the top before I opened the new ones. That's not what happens.

Root cause

Engine::close_tab (src/core/engine/windows.rs:345) picks the next active tab by index arithmetic alone:

self.active_group_mut().tabs.remove(active_tab_idx);
...
// 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();

active_tab still holds the closed tab's index, so whatever tab shifted into that slot becomes active (the tab to the right), clamped to the last tab when the rightmost was closed. tab_mru_touch() is called after the decision and merely records whatever adjacency chose — the MRU stack is written but never read here.

⚠ The naive repro passes by accident, and a test written against it will pass with the bug fully present. With tabs [A, B, C] and A never revisited, closing C then B lands on A by pure adjacency — the right answer for the wrong reason. The defect only shows when the tab you were on is not adjacent to the new ones:

tabs: [X, A, Y, Z]     active = A (idx 1)
open B  -> [X, A, Y, Z, B]      active = B
open C  -> [X, A, Y, Z, B, C]   active = C
close C -> active = B           (idx 5 clamped to 4)
close B -> active = Z           (idx 4 clamped to 3)   <-- expected A

This is the #553 failure mode waiting to happen: a green test that never discriminated. Write the test with a non-adjacent prior tab, and verify it fails against current develop before you fix anything.

Second defect — closing a background tab steals focus

The user called this out explicitly: "you can close a tab without activating it." Today you cannot. close_tab_at(group_id, tab_idx) (windows.rs:430) is documented as "Used for right-click 'Close' on non-active tabs" and implemented as switch to the target group/tab, then close it. So closing a background tab:

• changes which tab is active as a side effect,
• pushes the closed tab onto the MRU and nav history on its way out,
• and therefore corrupts the very stack that should have decided the successor.

Closing a non-active tab must leave the active tab alone and simply remove the closed tab from the stack.

Third defect — the MRU stack is index-based and silently corrupted by reorder

tab_mru stores (GroupId, usize) — a positional index. tab_nav_history, right next to it, correctly stores (GroupId, TabId). The index form forces the fixup loop at windows.rs:392-396 on close, and it is not fixed up at all by the functions that move tabs around:

• reorder_tab_in_group (windows.rs:2174) — tabs.remove(from) + tabs.insert(to), no tab_mru touch. This is the tab drag-and-drop path (Consolidate editor-group tab drag-drop onto quadraui drop primitives (full TabGroupController adoption deferred — see quadraui#395) #515).
• move_tab_to_other_group (windows.rs:2004) — moves a tab between groups, no tab_mru touch.
• move_tab_to_target_group_at (:2058), move_tab_to_new_split (:2096) — same.

So today, dragging any tab silently re-points every MRU entry at a different tab. Fixing the close-successor without fixing this just makes the wrong answer arrive more confidently.

Recommended fix

1. Re-key tab_mru to (GroupId, TabId), matching tab_nav_history. This deletes the index-fixup loop at :392-396 and makes the reorder paths correct by construction rather than by remembering to patch them.
2. Add one well-named successor function — e.g. Engine::successor_tab_after_close(group, closed_tab_id) -> Option<(GroupId, TabId)> — that reads the MRU and falls back to adjacency only when the MRU has nothing live. Route every close path through it: close_tab, close_tab_at, close_other_tabs (:457), close_tabs_to_right (:480), close_tabs_to_left (:497), close_saved_tabs (:514), close_all_tabs (:799).
3. Make close_tab_at not activate its target. Remove the closed tab from tab_mru and tab_nav_history and leave active_tab untouched unless the tab being closed is the active one.

Keep the policy in that one function. quadraui#596 (WorkspaceController, open-N-view-one) and quadraui#597 (preview tier, lifted from vimcode) are absorbing exactly this kind of tab-lifecycle policy — vimcode#658 is the adoption issue. A single well-named engine function is liftable into WorkspaceController later; the same rule smeared across seven close paths is not. Do not wait for quadraui: this is src/core/, the shared engine both backends already consume, so the Platform-Neutrality Rule does not gate it.

Acceptance criteria

• With the non-adjacent repro above, closing C then B lands on A. Assert this at the black-box tier with a TuiDriver test (in-crate, #[cfg(test)], using the existing make_test_app fixtures) that reads the rendered tab bar — not only as an engine unit test.
• State in the PR that you ran the new test against unfixed develop and watched it fail. A test that passes both ways is worse than no test here (#448-F: GTK tab click (activate/close) dead when only ONE tab group exists #553).
• Right-click → Close on a background tab leaves the active tab unchanged.
• Dragging a tab to a new position does not change which tab a subsequent close activates.
• Closing tabs across two editor groups picks the correct successor per group.
• tab_mru contains no positional indices.
• Existing tab tests in src/core/engine/tests.rs pass with their assertions unchanged. If one needs its assertion edited, that is a behaviour change — stop and reconcile rather than adjusting the number.
• cargo build && cargo test EXIT=0.

Files

• src/core/engine/windows.rs
• src/core/engine/mod.rs
• src/core/engine/keys.rs
• src/core/engine/tests.rs
• src/tui_main/shell_app.rs

Out of scope

• The Ctrl-O / Ctrl-I jumplist — filed separately. That is a different structure (jump_list, cursor positions) in a different file, and conflating the two is how this becomes a two-session issue.
• Any change to tab ordering in the tab bar. This issue changes only which tab is activated, never where tabs sit.

[JDonaghy]

Coordinator: Assignment Complete

Machine: dellserver
Status: done
Exit code: 0
Duration: 34m 29s
Log: /home/john/.coord/logs/84385e42bc8e.log

[JDonaghy]

Coordinator: Assignment Complete

Machine: dellserver
Status: done
Exit code: 0
Duration: 10m 57s
Log: /home/john/.coord/logs/1202a8838def.log

[JDonaghy]

Coordinator: Assignment Complete

Machine: precision
Status: done
Exit code: 0
Duration: —

Summary

Verdict: request-changes

Review findings

Blocking findings

• close_active_tab_of_background_group (src/core/engine/windows.rs, new function added by this PR, invoked from close_tab_at when tab_idx == group.active_tab && group_id != self.active_group) has zero test coverage anywhere in the diff or pre-existing suite. This is new, non-trivial policy code — it re-implements a scoped version of close_tab's MRU-successor logic and a scoped copy of close_editor_group's "remove the whole group" fallback when the background group is down to its last tab — and it is reachable from real UI: context_menu_confirm's ContextMenuTarget::Tab { group_id, tab_idx } → "close" => self.close_tab_at(group_id, tab_idx) (windows.rs:1189) fires with group_id set to whatever group was right-clicked, which is routinely a different, unfocused split group. Every existing close_tab_at test (tests/context_menu.rs) and every new test in this PR (test_close_tab_at_background_tab_does_not_change_active_tab, test_close_tab_at_background_tab_shifts_active_index_when_before_it) passes group = engine.active_group, so they only ever exercise the "background tab inside the focused group" branch, never "active tab of an unfocused group" — the exact scenario the new function exists for. This is precisely defect v0.3.0: TUI-only binary (vcd) #2 from the issue ("closing a background tab steals focus") applied to the multi-group/split case, and the issue's own acceptance criteria call for exactly this ("Right-click → Close on a background tab leaves the active tab unchanged" and "Closing tabs across two editor groups picks the correct successor per group") without restricting it to same-group closes. Please add at least one engine-level test that opens a split, right-clicks (i.e. calls close_tab_at) on the active tab of the unfocused group, and asserts (a) self.active_group and the focused group's own active tab are untouched, and (b) the background group's own successor is chosen correctly — plus the "last tab in a background group closes the whole group" sub-branch (group.tabs.len() <= 1 inside close_active_tab_of_background_group), which is also currently unexercised.

Non-blocking concerns

• Per this repo's CLAUDE.md ("Testing (CRITICAL)" → multi-backend rule: "If the change touches a surface both backends render, ... cover both"), the tab bar is a shared GTK+TUI surface and this PR ships a black-box test only for TUI (shell_app.rs), not GTK (src/gtk/testing.rs's GtkDriver, which already has tab-bar-specific helpers like tab_center/tab_close_center). I checked recent history and single-backend driver tests are the norm here even for prior tab-bar fixes (#448-F: GTK tab click (activate/close) dead when only ONE tab group exists #553, Adopt promoted quadraui helpers: driver tab geometry + SidebarSystem reveal #659), so I'm not blocking on this, but a GTK regression test would close the letter-of-the-rule gap and catch any backend-specific wiring drift.
• Bulk-close paths (close_other_tabs, close_tabs_to_right, close_tabs_to_left, close_saved_tabs, close_all_tabs) were explicitly named in the issue's "Route every close path through it" list but are untouched by this diff. I traced through them: they all loop by faking active_group_mut().active_tab = i then calling the now-MRU-aware close_tab(), and each one pins the true final active tab explicitly after the loop, so the end state is correct by construction — they get the successor fix "for free" via delegation, and no bug is introduced. Still, the intermediate tab_mru_touch() calls fired once per closed background tab during these loops do write transient, never-really-visited tabs to the front of the global MRU stack before being overwritten; harmless today only because of the final override, but worth a comment (or a follow-up) noting the reliance is intentional, since it's easy for a future edit to break by reordering the final active_tab = … before the loop.
• The PR's "confirmed RED against unfixed code" claim required by CLAUDE.md is present in the branch's commit message ("All five engine tests and the driver test were confirmed to fail against the pre-fix code before the fix landed") — I could not check the actual GitHub PR description text (no gh access in this review), so please confirm that statement also appears in the PR body itself, not just the commit message.

Nits

• close_active_tab_of_background_group's "remove the whole background group" branch duplicates window/layout teardown logic from close_editor_group() but additionally prunes tab_mru/tab_nav_history for the closed group, which close_editor_group() itself still doesn't do — a nice improvement, but it leaves the two group-removal implementations subtly inconsistent. Worth a follow-up to unify them (e.g. have close_editor_group() call the same helper) rather than maintaining two copies of "tear down a whole group."
• Neither close_active_tab_of_background_group's normal-close path nor its whole-group-removal path calls anything to keep the background group's own tab-bar scroll offset sane (the focused-group path calls ensure_active_tab_visible()); purely cosmetic (tab scroll position) since it only matters once the user switches back to that group, and ensure_active_tab_visible()/equivalent presumably gets called on the actual group switch — but worth a quick sanity check.

[JDonaghy]

Review Complete — ⚠️ Changes Requested

Reviewer findings could not be posted directly to PR #682 (gh pr review was rejected — likely a self-review restriction). Findings are reproduced here.

Note: a completion comment was posted earlier but findings could not be extracted at that time. These are the retroactive findings.

Blocking findings

• close_active_tab_of_background_group (src/core/engine/windows.rs, new function added by this PR, invoked from close_tab_at when tab_idx == group.active_tab && group_id != self.active_group) has zero test coverage anywhere in the diff or pre-existing suite. This is new, non-trivial policy code — it re-implements a scoped version of close_tab's MRU-successor logic and a scoped copy of close_editor_group's "remove the whole group" fallback when the background group is down to its last tab — and it is reachable from real UI: context_menu_confirm's ContextMenuTarget::Tab { group_id, tab_idx } → "close" => self.close_tab_at(group_id, tab_idx) (windows.rs:1189) fires with group_id set to whatever group was right-clicked, which is routinely a different, unfocused split group. Every existing close_tab_at test (tests/context_menu.rs) and every new test in this PR (test_close_tab_at_background_tab_does_not_change_active_tab, test_close_tab_at_background_tab_shifts_active_index_when_before_it) passes group = engine.active_group, so they only ever exercise the "background tab inside the focused group" branch, never "active tab of an unfocused group" — the exact scenario the new function exists for. This is precisely defect v0.3.0: TUI-only binary (vcd) #2 from the issue ("closing a background tab steals focus") applied to the multi-group/split case, and the issue's own acceptance criteria call for exactly this ("Right-click → Close on a background tab leaves the active tab unchanged" and "Closing tabs across two editor groups picks the correct successor per group") without restricting it to same-group closes. Please add at least one engine-level test that opens a split, right-clicks (i.e. calls close_tab_at) on the active tab of the unfocused group, and asserts (a) self.active_group and the focused group's own active tab are untouched, and (b) the background group's own successor is chosen correctly — plus the "last tab in a background group closes the whole group" sub-branch (group.tabs.len() <= 1 inside close_active_tab_of_background_group), which is also currently unexercised.

Non-blocking concerns

• Per this repo's CLAUDE.md ("Testing (CRITICAL)" → multi-backend rule: "If the change touches a surface both backends render, ... cover both"), the tab bar is a shared GTK+TUI surface and this PR ships a black-box test only for TUI (shell_app.rs), not GTK (src/gtk/testing.rs's GtkDriver, which already has tab-bar-specific helpers like tab_center/tab_close_center). I checked recent history and single-backend driver tests are the norm here even for prior tab-bar fixes (#448-F: GTK tab click (activate/close) dead when only ONE tab group exists #553, Adopt promoted quadraui helpers: driver tab geometry + SidebarSystem reveal #659), so I'm not blocking on this, but a GTK regression test would close the letter-of-the-rule gap and catch any backend-specific wiring drift.
• Bulk-close paths (close_other_tabs, close_tabs_to_right, close_tabs_to_left, close_saved_tabs, close_all_tabs) were explicitly named in the issue's "Route every close path through it" list but are untouched by this diff. I traced through them: they all loop by faking active_group_mut().active_tab = i then calling the now-MRU-aware close_tab(), and each one pins the true final active tab explicitly after the loop, so the end state is correct by construction — they get the successor fix "for free" via delegation, and no bug is introduced. Still, the intermediate tab_mru_touch() calls fired once per closed background tab during these loops do write transient, never-really-visited tabs to the front of the global MRU stack before being overwritten; harmless today only because of the final override, but worth a comment (or a follow-up) noting the reliance is intentional, since it's easy for a future edit to break by reordering the final active_tab = … before the loop.
• The PR's "confirmed RED against unfixed code" claim required by CLAUDE.md is present in the branch's commit message ("All five engine tests and the driver test were confirmed to fail against the pre-fix code before the fix landed") — I could not check the actual GitHub PR description text (no gh access in this review), so please confirm that statement also appears in the PR body itself, not just the commit message.

Nits

• close_active_tab_of_background_group's "remove the whole background group" branch duplicates window/layout teardown logic from close_editor_group() but additionally prunes tab_mru/tab_nav_history for the closed group, which close_editor_group() itself still doesn't do — a nice improvement, but it leaves the two group-removal implementations subtly inconsistent. Worth a follow-up to unify them (e.g. have close_editor_group() call the same helper) rather than maintaining two copies of "tear down a whole group."
• Neither close_active_tab_of_background_group's normal-close path nor its whole-group-removal path calls anything to keep the background group's own tab-bar scroll offset sane (the focused-group path calls ensure_active_tab_visible()); purely cosmetic (tab scroll position) since it only matters once the user switches back to that group, and ensure_active_tab_visible()/equivalent presumably gets called on the actual group switch — but worth a quick sanity check.