From 8a4f08e368af6a97a87e15933347042ac6afcf43 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 10 Sep 2026 11:28:04 +0800 Subject: [PATCH 1/2] fix(ui): remember the focused pane as focus arrives, not at switch time (#843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tab's `last_focused` was written by one sample taken on the way out of `activate`, and that sample asks which leaf holds focus at that instant. By then focus is routinely somewhere else: the switcher's own search input, a palette that just closed, the tab strip, the file tree `activate` focuses itself, or a pane restored and never clicked. With no leaf focused the write was skipped, the field kept a stale pane or the `None` it was born with, and `focus_target` fell through `leaf_matching_or_first` to the tab's first leaf. Two panes, click the right one, switch away and back, land in the left one. `watch_pane_focus` already subscribed to every pane's focus-in — it existed to repaint the chrome that marks the focused pane — so the record now rides on that callback instead of a new one. Focus-in is the only moment that knows the answer without having to guess when to look, and the subscription's cost and lifetime were already paid for. Which tab gets the write is asked of the layout rather than assumed to be the active one, so a pane dragged into another tab is remembered by the tab holding it now; a leaf no tab holds is recorded nowhere. Connecting slots are watched too, since a pane can be focused while it is still coming up. `remember_active_pane` stays. It never writes a wrong answer, only sometimes none, and its callers want the field settled at a named moment — before a pane is detached, before a tab is torn down — while the layout can still answer. Deliberately left alone: a restored tab still opens on its first leaf until someone touches a pane. That is a different hole — persistence, not a focus race — and closing it means giving a leaf an identity that survives a restart. `SessionTab` has none, and an ordinal into the tree is not one either, because `session_to_pane` drops leaves that cannot be respawned and shifts every index after them. The report's third suggestion, preferring a most-recently-active leaf recorded elsewhere, was checked and dropped: `last_used` exists on `Tab` for the switcher's MRU column and there is no per-leaf equivalent anywhere. No timestamp was invented to create one, and with focus-in authoritative there is nothing left for it to break a tie about. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- src/ui/app.rs | 242 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 236 insertions(+), 6 deletions(-) diff --git a/src/ui/app.rs b/src/ui/app.rs index d2f043e9..a0c40a49 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -3455,6 +3455,16 @@ impl Tty7App { } } + /// Sample which pane holds focus right now and record it against the + /// active tab. + /// + /// A sample only ever writes the truth, but it can only write it when + /// there is one to read: with focus one handle off the panes it finds no + /// leaf and leaves the field alone. That is why it is no longer the only + /// writer (see [`Tty7App::remember_focused_leaf`]) — it stays because the + /// callers below want the answer settled at a named moment, before a pane + /// is detached or a tab is torn down and the layout stops being able to + /// answer at all. pub(crate) fn remember_active_pane(&mut self, window: &Window, cx: &App) { let active = self.active; if let Some(tab) = self.tabs.get_mut(active) { @@ -3464,6 +3474,24 @@ impl Tty7App { } } + /// Record `leaf` as the pane its tab comes back to, as focus arrives in it. + /// + /// Sampling at switch time asks which leaf holds focus *at that instant*, + /// and by then focus is routinely somewhere else: the switcher's own + /// search input, a palette that just closed, the tab strip, a pane + /// restored and never clicked. The sample then wrote nothing and the tab + /// kept a stale pane — or the `None` it was born with — and came back to + /// its first leaf instead of the one the reader was working in (#843). + /// + /// Focus-in is the one moment that knows the answer without having to + /// guess when to look, so it is the primary writer now. The tab is found + /// by the leaf rather than assumed to be the active one: a pane dragged + /// into another tab is focused after the move, and it is the tab holding + /// it now that has to remember it. + pub(crate) fn remember_focused_leaf(&mut self, leaf: gpui::EntityId) { + remember_leaf_in(&mut self.tabs, leaf); + } + fn focus_leaf(&self, leaf: &PaneSlot, window: &mut Window, cx: &mut App) { let handle = leaf.focus_handle(cx); window.focus(&handle, cx); @@ -8525,6 +8553,8 @@ pub(crate) fn new_terminal( }, ) .detach(); + let handle = pending.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, pending.entity_id(), window, cx); start_pane_spawn(pending.clone(), window, cx); Ok(PaneSlot::Connecting(pending)) } @@ -8590,7 +8620,8 @@ fn build_terminal_view( ) .detach(); watch_open_file_requests(&view, window, cx); - watch_pane_focus(&view, window, cx); + let handle = view.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, view.entity_id(), window, cx); view } @@ -8618,13 +8649,42 @@ fn watch_open_file_requests( .detach(); } -fn watch_pane_focus(view: &Entity, window: &mut Window, cx: &mut Context) { - let handle = view.read(cx).focus_handle.clone(); +/// Record `leaf` as the pane the tab holding it comes back to. +/// +/// Which tab that is gets asked of the layout rather than assumed to be the +/// active one: a pane dragged into another tab takes focus with it, and it is +/// the tab holding it now whose memory the arrival should change. A leaf no +/// tab holds — one that has just closed, or arrived after its slot went away — +/// is recorded nowhere. +fn remember_leaf_in(tabs: &mut [Tab], leaf: gpui::EntityId) { + let held = tabs + .iter_mut() + .find(|tab| tab.pane.leaves().iter().any(|l| l.entity_id() == leaf)); + if let Some(tab) = held { + tab.last_focused = Some(leaf); + } +} + +/// Repaint the chrome that marks the focused pane, and record the leaf as the +/// one its tab returns to (#843). +/// +/// Every leaf is watched, connecting slots included: a pane can be focused +/// while it is still coming up, and if the tab is left in that moment the +/// answer has to already be written down. +fn watch_pane_focus( + handle: &gpui::FocusHandle, + leaf: gpui::EntityId, + window: &mut Window, + cx: &mut Context, +) { let app = cx.weak_entity(); window - .on_focus_in(&handle, cx, move |_window, cx| { + .on_focus_in(handle, cx, move |_window, cx| { if let Some(app) = app.upgrade() { - app.update(cx, |_, cx| cx.notify()); + app.update(cx, |app, cx| { + app.remember_focused_leaf(leaf); + cx.notify(); + }); } }) .detach(); @@ -8656,7 +8716,8 @@ pub(crate) fn new_terminal_native( ) .detach(); watch_open_file_requests(&view, window, cx); - watch_pane_focus(&view, window, cx); + let handle = view.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, view.entity_id(), window, cx); Ok(view) } @@ -11060,3 +11121,172 @@ mod close_window_action_tests { ); } } + +/// #843: which pane a tab comes back to. +#[cfg(test)] +mod tab_focus_memory_tests { + use super::{Pane, PaneSlot, Tab, remember_leaf_in}; + use crate::ui::pending_pane::{PendingPane, PendingSpawn}; + use gpui::{ + AppContext as _, Axis, Context, Entity, IntoElement, Render, Styled as _, TestAppContext, + Window, div, + }; + + /// A window has to exist for focus to live in, but nothing this file asks + /// is about what a pane paints. + struct Blank; + impl Render for Blank { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div().size_full() + } + } + + /// A leaf that owns a real focus handle without owning a shell. Focus + /// tracking asks the slot, not the terminal behind it, so a connecting + /// pane answers every question here exactly as a running one would — and + /// connecting panes are watched for focus now too, so this is not a + /// stand-in for the case under test but one of its cases. + fn leaf(cx: &mut Context) -> Entity { + cx.new(|cx| { + PendingPane::new( + "test", + PendingSpawn { + workspace: None, + working_directory: None, + restore_pane: None, + shell: None, + agent: None, + agent_session_id: None, + agent_launch_argv: None, + owner: None, + font_size: 14., + }, + cx, + ) + }) + } + + fn two_pane_tab(a: &Entity, b: &Entity) -> Tab { + Tab::new(Pane::split_node( + Axis::Horizontal, + 0.5, + Pane::Leaf(PaneSlot::Connecting(a.clone())), + Pane::Leaf(PaneSlot::Connecting(b.clone())), + )) + } + + fn one_pane_tab(a: &Entity) -> Tab { + Tab::new(Pane::Leaf(PaneSlot::Connecting(a.clone()))) + } + + /// The mechanism behind the report: `remember_active_pane`'s sample asks + /// which leaf holds focus *at that instant*, and a switch begun with focus + /// one handle away — the switcher's own search input, a palette closing, + /// the tab strip — finds no leaf and has nothing to write. This is why the + /// sample cannot be the only writer, and it is pinned here so that a change + /// making `focused_leaf` tolerant would have to argue with a test rather + /// than silently make this fix look unnecessary. + #[gpui::test] + fn a_switch_begun_off_the_panes_samples_nothing(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b, elsewhere) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), cx.focus_handle())) + .unwrap(); + let tab = two_pane_tab(&a, &b); + + window + .update(cx, |_, window, cx| { + let on_b = b.read(cx).focus_handle.clone(); + window.focus(&on_b, cx); + assert_eq!( + tab.pane.focused_leaf(window, cx).map(|l| l.entity_id()), + Some(b.entity_id()), + "with focus in the pane the sample would have found it" + ); + + window.focus(&elsewhere, cx); + assert!( + tab.pane.focused_leaf(window, cx).is_none(), + "one handle off the pane and the switch-away sample has \ + nothing to write" + ); + }) + .unwrap(); + } + + /// So focus-in writes instead, and the tab comes back to the pane focus + /// was last in even though it had wandered off the panes before the switch + /// ever started. + #[gpui::test] + fn a_tab_comes_back_to_the_pane_focus_was_last_in(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b) = window.update(cx, |_, _, cx| (leaf(cx), leaf(cx))).unwrap(); + let mut tabs = vec![two_pane_tab(&a, &b)]; + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(a.entity_id()), + "a tab nobody has worked in yet still opens on its first leaf" + ); + + // The reader clicks into the right-hand pane: focus arrives, and that + // is the moment the tab is told. + remember_leaf_in(&mut tabs, b.entity_id()); + + // Focus then leaves the panes — the switcher opens, a palette closes — + // and the tab is switched away from. `remember_active_pane` finds no + // focused leaf and writes nothing, which is now harmless. + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(b.entity_id()), + "#843: the tab has to come back to the pane the reader was in" + ); + } + + /// A pane dragged into another tab is focused where it lands, so the + /// arrival has to change that tab's memory and not the one it left — the + /// reason the tab is found by the leaf rather than taken to be the active + /// one. + #[gpui::test] + fn a_moved_pane_is_remembered_by_the_tab_that_holds_it_now(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b, c) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), leaf(cx))) + .unwrap(); + // Tab 0 is the active one and holds `a`; `b` and `c` live in tab 1. + let mut tabs = vec![one_pane_tab(&a), two_pane_tab(&b, &c)]; + + remember_leaf_in(&mut tabs, c.entity_id()); + + assert_eq!( + tabs[1].focus_target().map(|l| l.entity_id()), + Some(c.entity_id()), + "the tab holding the focused pane is the one that remembers it" + ); + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(a.entity_id()), + "and no other tab's memory is touched" + ); + } + + /// A pane that arrives after its slot has gone — a spawn landing on a + /// closed tab, a leaf killed mid-flight — belongs to no tab, and must not + /// leave a memory behind for the first tab that happens to be looked at. + #[gpui::test] + fn a_leaf_no_tab_holds_is_recorded_nowhere(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b, gone) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), leaf(cx))) + .unwrap(); + let mut tabs = vec![two_pane_tab(&a, &b)]; + + remember_leaf_in(&mut tabs, b.entity_id()); + remember_leaf_in(&mut tabs, gone.entity_id()); + + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(b.entity_id()), + "a stranger's arrival leaves the tab's own answer alone" + ); + } +} From eb419128ca02a0084c2db1599fddc171edc2104b Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:05:02 +0800 Subject: [PATCH 2/2] fix(ui): carry a tab's focus memory onto the pane its pending slot became MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Watching connecting slots is what lets a pane focused while it is still coming up be recorded, but the record names the *pending* slot, and that id dies the moment `land_pane` swaps the slot for the pane that came up in it. The memory then names an entity no tab holds, `focus_target` falls through `leaf_matching_or_first`, and the tab comes back to its first leaf — the bug this branch is about, one landing later. Nothing else writes the answer down in that window. `land_pane` re-focuses the pane it built only when the pending slot still held focus, and with focus off the panes the switch-away sample has nothing to read either. So: split a pane, let focus wander off the panes while the new one is still connecting, switch away and back, and you land in the pane you did not ask for. `replace_leaf_in` does the swap and carries the memory with it, and `respawn_native_ssh_in_place` — the other place a live pane is substituted for a dead one — uses it instead of repeating the walk. Also pins the wiring this branch changed. The tests here drove `remember_leaf_in` directly and never the subscription that calls it, so deleting the one line that records a focus arrival left all four green. `test_window::harness` makes the round trip reachable — real panes, real gpui focus, a real `activate` there and back — and both new tests fail without the code they are about. Claude-Session: https://claude.ai/code/session_01JRqYZ9E153WpSHGS2AW3BM --- src/ui/app.rs | 152 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 140 insertions(+), 12 deletions(-) diff --git a/src/ui/app.rs b/src/ui/app.rs index a0c40a49..9830f652 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -3543,9 +3543,7 @@ impl Tty7App { view.read(cx).run_command_line(&cmd); } let slot = PaneSlot::Ready(view.clone()); - self.tabs - .iter_mut() - .any(|tab| tab.pane.replace_leaf(slot_id, slot.clone())); + replace_leaf_in(&mut self.tabs, slot_id, slot.clone()); if was_focused { self.focus_leaf(&slot, window, cx); } @@ -3749,14 +3747,11 @@ impl Tty7App { return; } }; - for tab in &mut self.tabs { - if tab - .pane - .replace_leaf(dead.entity_id(), PaneSlot::Ready(fresh.clone())) - { - break; - } - } + replace_leaf_in( + &mut self.tabs, + dead.entity_id(), + PaneSlot::Ready(fresh.clone()), + ); self.maximized = None; self.focus_leaf(&PaneSlot::Ready(fresh), window, cx); self.save_session(cx); @@ -8665,6 +8660,30 @@ fn remember_leaf_in(tabs: &mut [Tab], leaf: gpui::EntityId) { } } +/// Put `new` where the slot `old` named stood, carrying that tab's focus +/// memory across with it. +/// +/// The memory has to move because the id it holds does not survive the swap. +/// Focus arriving in a pane that is still coming up is recorded against the +/// *pending* slot — that is why connecting slots are watched at all — and that +/// slot's id dies the moment the pane lands. Left behind, the memory names an +/// entity no tab holds, `focus_target` falls through `leaf_matching_or_first`, +/// and the tab comes back to its first leaf: #843 again, one landing later. +/// +/// Nothing else writes the answer down in that case. `land_pane` re-focuses +/// the pane it built only when the pending slot still held focus, and with +/// focus off the panes the switch-away sample has nothing to read either. +fn replace_leaf_in(tabs: &mut [Tab], old: gpui::EntityId, new: PaneSlot) { + for tab in tabs.iter_mut() { + if tab.pane.replace_leaf(old, new.clone()) { + if tab.last_focused == Some(old) { + tab.last_focused = Some(new.entity_id()); + } + break; + } + } +} + /// Repaint the chrome that marks the focused pane, and record the leaf as the /// one its tab returns to (#843). /// @@ -11125,7 +11144,7 @@ mod close_window_action_tests { /// #843: which pane a tab comes back to. #[cfg(test)] mod tab_focus_memory_tests { - use super::{Pane, PaneSlot, Tab, remember_leaf_in}; + use super::{Pane, PaneSlot, Tab, remember_leaf_in, replace_leaf_in}; use crate::ui::pending_pane::{PendingPane, PendingSpawn}; use gpui::{ AppContext as _, Axis, Context, Entity, IntoElement, Render, Styled as _, TestAppContext, @@ -11289,4 +11308,113 @@ mod tab_focus_memory_tests { "a stranger's arrival leaves the tab's own answer alone" ); } + + /// A pane focused while it was still coming up is remembered under its + /// *pending* slot, and that id dies the moment the pane lands in its + /// place. The memory has to come along with the swap, or the landing is + /// itself what puts the tab back on its first leaf. + /// + /// What lands here is another slot rather than a running pane: the swap has + /// to move an id from one slot to another, and which kind of slot arrived + /// is no part of the question. + #[gpui::test] + fn a_landing_pane_inherits_what_its_pending_slot_was_told(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, connecting, landed, other) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), leaf(cx), leaf(cx))) + .unwrap(); + let mut tabs = vec![two_pane_tab(&a, &connecting)]; + + // Focus arrives while the pane is still connecting, then the pane it + // was waiting for lands in that slot. + remember_leaf_in(&mut tabs, connecting.entity_id()); + replace_leaf_in( + &mut tabs, + connecting.entity_id(), + PaneSlot::Connecting(landed.clone()), + ); + + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(landed.entity_id()), + "#843: the memory follows the pane, not the slot it arrived in" + ); + + // A landing somewhere else in the tab is not an answer to this + // question and does not touch it. + replace_leaf_in(&mut tabs, a.entity_id(), PaneSlot::Connecting(other)); + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(landed.entity_id()), + "another pane landing leaves the tab's answer alone" + ); + } + + /// The wiring, end to end. `watch_pane_focus` is the subscription that + /// writes the record, and a real round trip through `activate` has to come + /// back to the pane focus last arrived in — with focus off the panes well + /// before the switch was made, which is the moment the switch-away sample + /// cannot see (#843). + #[gpui::test] + fn a_tab_switch_returns_to_the_pane_focus_arrived_in(cx: &mut TestAppContext) { + use super::{test_window::harness, watch_pane_focus}; + use crate::terminal::view::quiet_test_pane; + + let (app, mut vcx) = harness(cx); + let (left, right, elsewhere, _held) = app.update_in(&mut vcx, |app, window, cx| { + let (left, left_stream) = quiet_test_pane(1, window, cx); + let (right, right_stream) = quiet_test_pane(2, window, cx); + let (only, only_stream) = quiet_test_pane(3, window, cx); + app.tabs.push(Tab::new(Pane::split_node( + Axis::Horizontal, + 0.5, + Pane::leaf(PaneSlot::Ready(left.clone())), + Pane::leaf(PaneSlot::Ready(right.clone())), + ))); + app.tabs.push(Tab::new(Pane::leaf(PaneSlot::Ready(only)))); + app.active = 0; + // The subscription every spawn path registers for the pane it + // built. + for view in [&left, &right] { + let handle = view.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, view.entity_id(), window, cx); + } + cx.notify(); + ( + left, + right, + cx.focus_handle(), + (left_stream, right_stream, only_stream), + ) + }); + vcx.background_executor.run_until_parked(); + + // The reader clicks into the right-hand pane. + app.update_in(&mut vcx, |_, window, cx| { + let handle = right.read(cx).focus_handle.clone(); + handle.focus(window, cx); + }); + vcx.background_executor.run_until_parked(); + + // Focus then leaves the panes altogether — a palette closing, the tab + // strip, the switcher's own search input — before the tab is left. + app.update_in(&mut vcx, |_, window, cx| elsewhere.focus(window, cx)); + vcx.background_executor.run_until_parked(); + + app.update_in(&mut vcx, |app, window, cx| app.activate(1, window, cx)); + vcx.background_executor.run_until_parked(); + app.update_in(&mut vcx, |app, window, cx| app.activate(0, window, cx)); + vcx.background_executor.run_until_parked(); + + app.update_in(&mut vcx, |_, window, cx| { + assert!( + right.read(cx).focus_handle.is_focused(window), + "#843: the tab has to come back to the pane the reader was in" + ); + assert!( + !left.read(cx).focus_handle.is_focused(window), + "and not to the first leaf" + ); + }); + } }