From f4df05ffcab9752cb5edfafc122c8aa9c15930d2 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sat, 22 Aug 2026 22:02:26 +0800 Subject: [PATCH] fix(sidebar): open the diff of the row whose counts were clicked (#706) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every sidebar row carries its tab's `+N −M`, and the counts are their own click target. They read the git path off `self.tabs[i]` — the row — and then called `toggle_diff_overlay`, which writes to `self.active`. The two are only the same tab when the row clicked is the one already in front. Clicking another row's counts therefore opened *that* row's repository as an overlay on the tab already in front: focus never moved, the front tab showed a diff from a directory it has nothing to do with, and the overlay stayed filed under the front tab afterwards, so every later read of "the active tab's overlay" kept returning it. The row's tab now comes forward first, and the overlay lands on it. Arriving from another tab opens rather than toggles. `open_diff_overlay` is itself the toggle — the close lives there, not in its `toggle_*` wrapper — so activating the target first was not enough on its own: a target that already had that diff open and in front answered a request to show it by closing it, leaving the screen on nothing that was asked for. `may_close` is what separates the two, and only a click on the tab already in front sets it. The bounds check is the same bug by another route: `activate` no-ops on an index it does not have, which would leave the open writing to whatever tab happened to be in front, and a row can outlive its tab between render and click. All three tests were run against the unfixed code, not only a green tree; two of them fail there. --- src/ui/diff_overlay.rs | 162 ++++++++++++++++++++++++++++++++++++++++- src/ui/tab_sidebar.rs | 10 ++- 2 files changed, 170 insertions(+), 2 deletions(-) diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index 2e869e31..eb70a1df 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -110,6 +110,47 @@ impl Tty7App { self.toggle_diff_overlay_at(host, cwd, None, window, cx) } + /// The sidebar's `+N −M`, which names a row that may not be the tab in + /// front. + /// + /// The overlay is stored per tab and `open_diff_overlay` writes to the + /// *active* one, so the row's tab has to come forward before it opens. + /// Without that, clicking another row's counts read that row's repository + /// and opened it as an overlay on the tab already in front: the front tab + /// kept focus and showed a diff belonging to a different repository, and + /// the overlay stayed filed under the front tab afterwards (#706). + /// + /// Arriving from another tab opens rather than toggles. The click asked to + /// see this diff, and a target that already had it open and in front would + /// answer a request to show it by closing it. + pub(crate) fn show_tab_diff_overlay( + &mut self, + index: usize, + host: crate::ui::host_ops::HostId, + cwd: PathBuf, + window: &mut Window, + cx: &mut Context, + ) { + // `activate` no-ops on an index it does not have, which would leave the + // open below writing to whichever tab happens to be in front — the bug + // itself. A row can outlive its tab between render and click. + if index >= self.tabs.len() { + return; + } + let switching = self.active != index; + self.activate(index, window, cx); + self.open_diff_overlay_inner( + host, + cwd, + DiffSource::Head, + None, + // Only a click on the tab already in front is a toggle. + !switching, + window, + cx, + ); + } + pub(crate) fn toggle_diff_overlay_at( &mut self, host: crate::ui::host_ops::HostId, @@ -131,6 +172,27 @@ impl Tty7App { focus: Option, window: &mut Window, cx: &mut Context, + ) { + self.open_diff_overlay_inner(host, cwd, source, focus, true, window, cx) + } + + /// `may_close` is what separates a toggle from a request to see something. + /// + /// Every caller that clicks the same control twice wants the second click + /// to put the overlay away, so they pass `true`. A click that *arrives from + /// another tab* has not asked for that: it named a diff it cannot currently + /// see, and answering by closing the one already open there would leave the + /// screen showing nothing it asked for. + #[allow(clippy::too_many_arguments)] + fn open_diff_overlay_inner( + &mut self, + host: crate::ui::host_ops::HostId, + cwd: PathBuf, + source: DiffSource, + focus: Option, + may_close: bool, + window: &mut Window, + cx: &mut Context, ) { let active = self.active; let was_front = self.tabs.get(active).is_some_and(|t| { @@ -148,7 +210,7 @@ impl Tty7App { .and_then(|t| t.diff_overlay.as_mut()) .filter(|o| o.cwd == cwd && o.host_id == host && o.source == source) { - Some(o) if o.focus == focus && was_front => { + Some(o) if o.focus == focus && was_front && may_close => { self.close_diff_overlay(window, cx); return; } @@ -2579,3 +2641,101 @@ mod render_idle_gpui_tests { let _ = std::fs::remove_dir_all(&root); } } + +#[cfg(all(test, unix))] +mod tab_target_gpui_tests { + use std::path::PathBuf; + + use gpui::TestAppContext; + + use crate::ui::app::test_window::harness_with_tabs; + use crate::ui::host_ops::HostId; + + /// #706: the sidebar's `+N −M` belongs to the row it is drawn on, not to + /// the tab in front. + /// + /// The counts used to call `toggle_diff_overlay`, which writes to the + /// active tab. Clicking tab B's counts from tab A therefore opened B's + /// repository as an overlay *on A*: A kept focus, showed a diff from a + /// directory it has nothing to do with, and kept that overlay filed under + /// its own name afterwards. + #[gpui::test] + fn another_rows_counts_open_that_rows_tab(cx: &mut TestAppContext) { + let (app, mut vcx, _streams) = harness_with_tabs(cx, 2); + let cwd = PathBuf::from("/w/repo-b"); + + app.update_in(&mut vcx, |app, window, cx| { + assert_eq!(app.active, 0, "the harness starts on the first tab"); + app.show_tab_diff_overlay(1, HostId::LOCAL, cwd.clone(), window, cx); + }); + + app.update(cx, |app, _| { + assert_eq!(app.active, 1, "the row's own tab comes forward"); + assert!( + app.tabs[0].diff_overlay.is_none(), + "the tab that was in front keeps no overlay it never asked for" + ); + let overlay = app.tabs[1] + .diff_overlay + .as_ref() + .expect("the overlay is filed under the tab whose counts were clicked"); + assert_eq!(overlay.cwd, cwd, "and it opens on that row's repository"); + }); + } + + /// Arriving from another tab opens; a second click, now that the tab is + /// already in front, toggles it away. Without the first half a target that + /// already had the overlay open would answer "show me this" by closing it. + #[gpui::test] + fn a_click_from_elsewhere_opens_and_the_next_one_closes(cx: &mut TestAppContext) { + let (app, mut vcx, _streams) = harness_with_tabs(cx, 2); + let cwd = PathBuf::from("/w/repo-b"); + + app.update_in(&mut vcx, |app, window, cx| { + app.show_tab_diff_overlay(1, HostId::LOCAL, cwd.clone(), window, cx); + }); + // Reached from tab 0 again: still an open, not a close. + app.update_in(&mut vcx, |app, window, cx| { + app.activate(0, window, cx); + app.show_tab_diff_overlay(1, HostId::LOCAL, cwd.clone(), window, cx); + }); + app.update(cx, |app, _| { + assert!( + app.tabs[1].diff_overlay.is_some(), + "a click that switches tabs asks to see the diff, not to hide it" + ); + }); + + // Already in front, so this one is the toggle. + app.update_in(&mut vcx, |app, window, cx| { + app.show_tab_diff_overlay(1, HostId::LOCAL, cwd.clone(), window, cx); + }); + app.update(cx, |app, _| { + assert!( + app.tabs[1].diff_overlay.is_none(), + "clicking the counts of the tab already in front closes it again" + ); + }); + } + + /// A row can outlive the tab it was drawn for. `activate` no-ops on an + /// index it does not have, so without the bounds check the open would fall + /// through to whichever tab happened to be in front — #706 again, by + /// another route. + #[gpui::test] + fn a_row_index_that_no_longer_exists_opens_nothing(cx: &mut TestAppContext) { + let (app, mut vcx, _streams) = harness_with_tabs(cx, 2); + + app.update_in(&mut vcx, |app, window, cx| { + app.show_tab_diff_overlay(7, HostId::LOCAL, PathBuf::from("/w/gone"), window, cx); + }); + + app.update(cx, |app, _| { + assert_eq!(app.active, 0, "nothing came forward"); + assert!( + app.tabs.iter().all(|t| t.diff_overlay.is_none()), + "and no tab picked up the missing row's overlay" + ); + }); + } +} diff --git a/src/ui/tab_sidebar.rs b/src/ui/tab_sidebar.rs index 16e08ed5..42381140 100644 --- a/src/ui/tab_sidebar.rs +++ b/src/ui/tab_sidebar.rs @@ -437,7 +437,15 @@ impl Tty7App { MouseButton::Left, cx.listener(move |this, _: &MouseDownEvent, window, cx| { cx.stop_propagation(); - this.toggle_diff_overlay(host, cwd.clone(), window, cx); + // `i`, not the active tab: these + // counts belong to this row (#706). + this.show_tab_diff_overlay( + i, + host, + cwd.clone(), + window, + cx, + ); }), ) });