From 82853af05fa5ac6c953246f59d5d19a01374a0b2 Mon Sep 17 00:00:00 2001 From: Sola Date: Sun, 4 Oct 2026 22:01:55 +0000 Subject: [PATCH 1/3] dock: Hand the focus to the tab that replaces a closed one Closing the focused tab, by its close button, the Close menu item, or `DockArea::remove_panel`, slides a neighbour into its place but leaves the window's focus on the panel that has just gone. Nothing is focused afterwards, so the next keystroke, including another close shortcut, lands nowhere until the user clicks into the dock. When the removed panel held the focus, the panel its group now displays takes it over. Closing a tab that did not have the focus leaves the focus where it is. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01DHBYT8JhqczL6GhYZoKgww --- crates/base/src/dock/dock_area.rs | 17 +++++++ crates/kit/tests/dock.rs | 85 +++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+) diff --git a/crates/base/src/dock/dock_area.rs b/crates/base/src/dock/dock_area.rs index 207facdade..3ebec18066 100644 --- a/crates/base/src/dock/dock_area.rs +++ b/crates/base/src/dock/dock_area.rs @@ -623,11 +623,28 @@ impl DockArea { let Some(region) = self.placement_of_panel(panel) else { return; }; + // A panel takes the keyboard with it when it goes. If it had it, the + // panel its group displays in its place takes it over, or the next + // keystroke would land nowhere. + let had_focus = self + .panel(panel) + .is_some_and(|view| view.focus_handle(cx).contains_focused(window, cx)); + let node = self + .layout(region) + .and_then(|tree| tree.find_panel_node(panel)); let Some(tree) = self.tree_mut(region) else { return; }; let result = tree.remove_panel(panel); self.commit(result, window, cx); + + if had_focus + && let Some(next) = node + .and_then(|node| self.groups.get(&node)) + .and_then(|group| group.entity.read(cx).active_panel(cx)) + { + next.focus_handle(cx).focus(window, cx); + } } } diff --git a/crates/kit/tests/dock.rs b/crates/kit/tests/dock.rs index 3fa79b7abe..81cd464668 100644 --- a/crates/kit/tests/dock.rs +++ b/crates/kit/tests/dock.rs @@ -172,3 +172,88 @@ async fn dock_moves_a_tab_between_groups_and_zooms_the_result(cx: &mut TestAppCo }) .await; } + +struct TwoTabs { + area: Entity, + alpha: Entity, + beta: Entity, + elsewhere: FocusHandle, +} +impl Render for TwoTabs { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div() + .size_full() + .child(div().track_focus(&self.elsewhere).h(px(20.))) + .child(self.area.clone()) + } +} + +fn two_tabs(cx: &mut TestAppContext) -> (gpui_kit::AnyWindowHandle, Entity) { + cx.update(gpui_kit::init); + let (handle, view) = common::open_window(cx, Some(size(px(900.), px(600.))), |window, cx| { + let (area, _) = DockSkin::dock_area("editor", None, window, cx); + let alpha = cx.new(|cx| Document { + name: "alpha", + focus: cx.focus_handle(), + }); + let beta = cx.new(|cx| Document { + name: "beta", + focus: cx.focus_handle(), + }); + let layout = DockLayout::tabs() + .panel_view(panel_handle(alpha.clone()), cx) + .panel_view(panel_handle(beta.clone()), cx); + area.update(cx, |area, cx| area.set_center(layout, window, cx)); + cx.new(|cx| TwoTabs { + area, + alpha, + beta, + elsewhere: cx.focus_handle(), + }) + }); + (handle.into(), view) +} + +#[gpui_kit::test] +fn dock_hands_the_focus_to_the_tab_that_replaces_a_closed_one(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + cx.update_window(handle, |_, window, cx| { + let view = view.read(cx); + let (area, alpha, beta) = (view.area.clone(), view.alpha.clone(), view.beta.clone()); + window.render_frame(cx); + let alpha_focus = alpha.read(cx).focus.clone(); + alpha_focus.focus(window, cx); + window.render_frame(cx); + assert!(alpha.read(cx).focus.is_focused(window)); + + area.update(cx, |area, cx| area.remove_panel(alpha, window, cx)); + window.render_frame(cx); + assert!(window.find("beta").visible()); + assert!( + beta.read(cx).focus.is_focused(window), + "closing the focused tab must leave the keyboard on the one shown in its place" + ); + }) + .unwrap(); +} + +#[gpui_kit::test] +fn dock_leaves_the_focus_alone_when_a_tab_without_it_closes(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + cx.update_window(handle, |_, window, cx| { + let view = view.read(cx); + let (area, alpha, elsewhere) = ( + view.area.clone(), + view.alpha.clone(), + view.elsewhere.clone(), + ); + window.render_frame(cx); + elsewhere.focus(window, cx); + window.render_frame(cx); + + area.update(cx, |area, cx| area.remove_panel(alpha, window, cx)); + window.render_frame(cx); + assert!(elsewhere.is_focused(window)); + }) + .unwrap(); +} From 4c3d037cb122a6df59b28c858cfe8b333c0f9b01 Mon Sep 17 00:00:00 2001 From: Jason Lee Date: Thu, 8 Oct 2026 14:47:03 +0800 Subject: [PATCH 2/3] dock: Preserve callback focus and handle closed tab groups --- crates/base/src/dock/dock_area.rs | 83 ++++++++++-- crates/kit/tests/dock.rs | 215 ++++++++++++++++++++++++++---- 2 files changed, 258 insertions(+), 40 deletions(-) diff --git a/crates/base/src/dock/dock_area.rs b/crates/base/src/dock/dock_area.rs index 3ebec18066..f0718dcaa6 100644 --- a/crates/base/src/dock/dock_area.rs +++ b/crates/base/src/dock/dock_area.rs @@ -623,29 +623,90 @@ impl DockArea { let Some(region) = self.placement_of_panel(panel) else { return; }; - // A panel takes the keyboard with it when it goes. If it had it, the - // panel its group displays in its place takes it over, or the next - // keystroke would land nowhere. + let focused = window.focused(cx); let had_focus = self .panel(panel) .is_some_and(|view| view.focus_handle(cx).contains_focused(window, cx)); - let node = self - .layout(region) - .and_then(|tree| tree.find_panel_node(panel)); + // Prefer the same group, then the groups following it in layout order, + // then preceding groups from nearest to farthest. Preserve this order + // before normalization removes an emptied group or collapses its split. + let mut candidates = Vec::new(); + if had_focus && let Some(tree) = self.layout(region) { + tree.root().walk(&mut |node| { + if matches!(node.kind(), PaneRef::Tabs { .. }) { + candidates.push(node.id()); + } + }); + if let Some(ix) = tree + .find_panel_node(panel) + .and_then(|node| candidates.iter().position(|candidate| *candidate == node)) + { + candidates = candidates[ix..] + .iter() + .chain(candidates[..ix].iter().rev()) + .copied() + .collect(); + } + } let Some(tree) = self.tree_mut(region) else { return; }; let result = tree.remove_panel(panel); self.commit(result, window, cx); - if had_focus - && let Some(next) = node - .and_then(|node| self.groups.get(&node)) - .and_then(|group| group.entity.read(cx).active_panel(cx)) - { + // on_removed may deliberately focus another control (or clear focus). + // Automatic handoff is only the fallback when the callback left it alone. + if !had_focus || window.focused(cx) != focused { + return; + } + let next = candidates + .into_iter() + .find_map(|node| self.focus_target_in_group(node, cx)) + .or_else(|| { + [ + DockPlacement::Center, + DockPlacement::Left, + DockPlacement::Right, + DockPlacement::Bottom, + ] + .into_iter() + .filter(|placement| *placement != region) + .filter(|placement| { + *placement == DockPlacement::Center + || self + .docks + .get(placement) + .is_some_and(|pane| pane.dock.is_open()) + }) + .find_map(|placement| { + self.layout(placement) + .and_then(|tree| self.focus_target_in_node(tree.root(), cx)) + }) + }); + if let Some(next) = next { next.focus_handle(cx).focus(window, cx); } } + + fn focus_target_in_group(&self, node: NodeId, cx: &App) -> Option> { + if self.zoomed.is_some_and(|zoom| zoom != Zoomed::Group(node)) { + return None; + } + let group = self.groups.get(&node)?.entity.read(cx); + if group.is_collapsed() { + return None; + } + group.active_panel(cx) + } + + fn focus_target_in_node(&self, node: &PaneNode, cx: &App) -> Option> { + match node.kind() { + PaneRef::Tabs { .. } => self.focus_target_in_group(node.id(), cx), + PaneRef::Split { children, .. } => children + .iter() + .find_map(|child| self.focus_target_in_node(child, cx)), + } + } } /// Zooming. diff --git a/crates/kit/tests/dock.rs b/crates/kit/tests/dock.rs index 81cd464668..85a34b9325 100644 --- a/crates/kit/tests/dock.rs +++ b/crates/kit/tests/dock.rs @@ -3,6 +3,7 @@ use gpui_kit::component::dock::{ BasePanel, DockArea, DockLayout, DockSkin, Panel, PanelControl, PanelEvent, PanelStyle, panel_handle, }; +use gpui_kit::component::input::{Input, InputState}; use gpui_kit::test::{TestAppContextExt, TestSupportExt, TestWindowExt}; use gpui_kit::{ App, AppContext, Context, Entity, EventEmitter, FocusHandle, Focusable, TestAppContext, Window, @@ -12,11 +13,33 @@ use std::time::Duration; struct Document { name: &'static str, focus: FocusHandle, + input: Entity, + removed_focus: Option, + visible: bool, +} +impl Document { + fn new(name: &'static str, window: &mut Window, cx: &mut Context) -> Self { + Self { + name, + focus: cx.focus_handle(), + input: cx.new(|cx| InputState::new(window, cx)), + removed_focus: None, + visible: true, + } + } } impl BasePanel for Document { fn panel_name(&self) -> &'static str { self.name } + fn visible(&self, _: &App) -> bool { + self.visible + } + fn on_removed(&mut self, window: &mut Window, cx: &mut Context) { + if let Some(focus) = &self.removed_focus { + focus.focus(window, cx); + } + } } impl Panel for Document { fn title(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { @@ -40,6 +63,7 @@ impl Render for Document { .track_focus(&self.focus) .size_full() .child(self.name) + .child(Input::new(&self.input).id("editor")) } } struct Editor { @@ -56,14 +80,8 @@ async fn dock_switches_and_reorders_real_tabs(cx: &mut TestAppContext) { let (handle, _) = common::open_window(cx, Some(size(px(900.), px(600.))), |window, cx| { let (area, skin) = DockSkin::dock_area("editor", None, window, cx); skin.set_panel_style(PanelStyle::TabBar, cx); - let a = cx.new(|cx| Document { - name: "alpha", - focus: cx.focus_handle(), - }); - let b = cx.new(|cx| Document { - name: "beta", - focus: cx.focus_handle(), - }); + let a = cx.new(|cx| Document::new("alpha", window, cx)); + let b = cx.new(|cx| Document::new("beta", window, cx)); let layout = DockLayout::tabs() .panel_view(panel_handle(a), cx) .panel_view(panel_handle(b), cx); @@ -101,18 +119,9 @@ async fn dock_moves_a_tab_between_groups_and_zooms_the_result(cx: &mut TestAppCo cx.update(gpui_kit::init); let (handle, _) = common::open_window(cx, Some(size(px(900.), px(600.))), |window, cx| { let (area, _) = DockSkin::dock_area("editor", None, window, cx); - let a = cx.new(|cx| Document { - name: "alpha", - focus: cx.focus_handle(), - }); - let b = cx.new(|cx| Document { - name: "beta", - focus: cx.focus_handle(), - }); - let c = cx.new(|cx| Document { - name: "gamma", - focus: cx.focus_handle(), - }); + let a = cx.new(|cx| Document::new("alpha", window, cx)); + let b = cx.new(|cx| Document::new("beta", window, cx)); + let c = cx.new(|cx| Document::new("gamma", window, cx)); let layout = DockLayout::h_split() .child( DockLayout::tabs() @@ -191,15 +200,11 @@ impl Render for TwoTabs { fn two_tabs(cx: &mut TestAppContext) -> (gpui_kit::AnyWindowHandle, Entity) { cx.update(gpui_kit::init); let (handle, view) = common::open_window(cx, Some(size(px(900.), px(600.))), |window, cx| { - let (area, _) = DockSkin::dock_area("editor", None, window, cx); - let alpha = cx.new(|cx| Document { - name: "alpha", - focus: cx.focus_handle(), - }); - let beta = cx.new(|cx| Document { - name: "beta", - focus: cx.focus_handle(), - }); + let (area, skin) = DockSkin::dock_area("editor", None, window, cx); + skin.set_panel_style(PanelStyle::TabBar, cx); + skin.set_close_button_visible(true, cx); + let alpha = cx.new(|cx| Document::new("alpha", window, cx)); + let beta = cx.new(|cx| Document::new("beta", window, cx)); let layout = DockLayout::tabs() .panel_view(panel_handle(alpha.clone()), cx) .panel_view(panel_handle(beta.clone()), cx); @@ -257,3 +262,155 @@ fn dock_leaves_the_focus_alone_when_a_tab_without_it_closes(cx: &mut TestAppCont }) .unwrap(); } + +fn close_visible_tab(window: &mut Window, name: &str, ix: usize, cx: &mut App) { + let panel = window.find(name.to_owned()); + let group_ix = panel + .path() + .iter() + .position(|id| *id == "tab-panel".into()) + .unwrap(); + let group = panel.path()[group_ix - 1].clone(); + window.within(group).click(("close-tab", ix), cx); +} + +#[gpui_kit::test] +async fn dock_close_button_transfers_focus_from_a_child_input(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + let beta = cx + .update_window(handle, |_, window, cx| { + let beta = view.read(cx).beta.clone(); + window.render_frame(cx); + window.within("alpha").click("editor", cx); + assert_eq!(window.within("alpha").find("editor").focused(), Some(true)); + close_visible_tab(window, "alpha", 0, cx); + beta + }) + .unwrap(); + cx.wait_for(handle, Duration::from_secs(1), |window, cx| { + window.try_find("alpha").is_none() + && window.try_find("beta").is_some_and(|panel| panel.visible()) + && beta.read(cx).focus.is_focused(window) + }) + .await; +} + +#[gpui_kit::test] +async fn dock_close_button_preserves_focus_chosen_by_on_removed(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + let elsewhere = cx + .update_window(handle, |_, window, cx| { + let view = view.read(cx); + let (alpha, elsewhere) = (view.alpha.clone(), view.elsewhere.clone()); + alpha.update(cx, |panel, _| panel.removed_focus = Some(elsewhere.clone())); + window.render_frame(cx); + window.within("alpha").click("editor", cx); + close_visible_tab(window, "alpha", 0, cx); + elsewhere + }) + .unwrap(); + cx.wait_for(handle, Duration::from_secs(1), |window, _| { + window.try_find("alpha").is_none() && elsewhere.is_focused(window) + }) + .await; +} + +#[gpui_kit::test] +async fn dock_close_button_focuses_the_remaining_split_when_a_group_disappears( + cx: &mut TestAppContext, +) { + for close_alpha in [true, false] { + let (handle, view) = two_tabs(cx); + let next = cx + .update_window(handle, |_, window, cx| { + let view = view.read(cx); + let (area, alpha, beta) = + (view.area.clone(), view.alpha.clone(), view.beta.clone()); + let layout = DockLayout::h_split() + .child( + DockLayout::tabs().panel_view(panel_handle(alpha.clone()), cx), + None, + ) + .child( + DockLayout::tabs().panel_view(panel_handle(beta.clone()), cx), + None, + ); + area.update(cx, |area, cx| area.set_center(layout, window, cx)); + window.render_frame(cx); + let (name, next) = if close_alpha { + ("alpha", beta) + } else { + ("beta", alpha) + }; + window.within(name).click("editor", cx); + close_visible_tab(window, name, 0, cx); + next + }) + .unwrap(); + cx.wait_for(handle, Duration::from_secs(1), |window, cx| { + let closed = if close_alpha { "alpha" } else { "beta" }; + window.try_find(closed).is_none() + && next.read(cx).focus.is_focused(window) + && window.find(next.read(cx).name).visible() + }) + .await; + } +} + +#[gpui_kit::test] +async fn dock_close_button_preserves_focus_when_closing_an_inactive_tab(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + let input = cx + .update_window(handle, |_, window, cx| { + let input = view.read(cx).alpha.read(cx).input.clone(); + window.render_frame(cx); + window.within("alpha").click("editor", cx); + close_visible_tab(window, "alpha", 1, cx); + input + }) + .unwrap(); + cx.wait_for(handle, Duration::from_secs(1), |window, cx| { + let beta = view.read(cx).beta.clone(); + let area = view.read(cx).area.clone(); + area.read(cx).panel(beta.entity_id().into()).is_none() + && input.focus_handle(cx).is_focused(window) + }) + .await; +} + +#[gpui_kit::test] +async fn dock_close_button_skips_a_hidden_neighbor_when_handing_off_focus(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + let beta = cx + .update_window(handle, |_, window, cx| { + let view = view.read(cx); + let (area, alpha, beta) = (view.area.clone(), view.alpha.clone(), view.beta.clone()); + let hidden = cx.new(|cx| Document { + visible: false, + ..Document::new("hidden", window, cx) + }); + let layout = DockLayout::h_split() + .child(DockLayout::tabs().panel_view(panel_handle(alpha), cx), None) + .child( + DockLayout::tabs().panel_view(panel_handle(hidden), cx), + None, + ) + .child( + DockLayout::tabs().panel_view(panel_handle(beta.clone()), cx), + None, + ); + area.update(cx, |area, cx| area.set_center(layout, window, cx)); + window.render_frame(cx); + assert!(window.try_find("hidden").is_none()); + window.within("alpha").click("editor", cx); + close_visible_tab(window, "alpha", 0, cx); + beta + }) + .unwrap(); + cx.wait_for(handle, Duration::from_secs(1), |window, cx| { + window.try_find("alpha").is_none() + && window.find("beta").visible() + && beta.read(cx).focus.is_focused(window) + }) + .await; +} From a23aee9746d49e20ecbfa5fe47650df817a978b2 Mon Sep 17 00:00:00 2001 From: Jason Lee Date: Thu, 8 Oct 2026 14:50:22 +0800 Subject: [PATCH 3/3] docs: Define the order of pull request reviews --- AGENTS.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 69a3f01bc6..74e61b1b94 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -243,6 +243,17 @@ Text input system based on Rope data structure: else reads as a competing context. A callback receiving both takes the GPUI one as `cx` and names the other after what it holds (`trigger`, `state`). +## Pull Request Reviews + +Every PR review must present its conclusions in this order: + +1. Explain the PR's purpose: the problem it addresses and the scope of its changes. +2. Assess whether the requirement and proposed approach are reasonable. +3. Report concrete findings, followed by validation performed and its limitations. + +Do not start a review with findings alone; include the purpose and reasonableness +assessment even when no issues are found. + ## Code Style - Follow naming and organization patterns from existing code