From 93246163c677d226e184606f3782f54edccc7fc2 Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Wed, 17 Dec 2025 05:20:43 -0500 Subject: [PATCH] git: Fix deletion icon button in branch list deleting the wrong branch (#45087) Closes #45033 This bug happened because the deletion icon would use the selected entry index to choose what branch to delete. This works for all cases except when hovering on an entry, so the fix was passing in the entry index to the deletion button on_click handler. I also disabled the deletion button from working if a branch is HEAD, because it's an illegal operation to delete a branch a user is currently on. Finally, I made WeakEntity a non-optional field on `BranchList` because a workspace should always be present, and it's used to show toast notifications when a git operation fails. The popover view wouldn't have a workspace before, so users wouldn't get error messages when a git operation failed in that view. Release Notes: - git: Fix bug where branch list deletion button would delete the wrong branch --- crates/git_ui/src/branch_picker.rs | 139 ++++++++++++++++------------- crates/git_ui/src/commit_modal.rs | 11 ++- crates/git_ui/src/git_panel.rs | 16 +++- 3 files changed, 101 insertions(+), 65 deletions(-) diff --git a/crates/git_ui/src/branch_picker.rs b/crates/git_ui/src/branch_picker.rs index cbf3fea72a9937a0ab882f9ccca2c4274afdf0e2..7395f1588fececcf4f374ec0e66cdac6024656d7 100644 --- a/crates/git_ui/src/branch_picker.rs +++ b/crates/git_ui/src/branch_picker.rs @@ -72,25 +72,19 @@ pub fn open( let repository = workspace.project().read(cx).active_repository(cx); let style = BranchListStyle::Modal; workspace.toggle_modal(window, cx, |window, cx| { - BranchList::new( - Some(workspace_handle), - repository, - style, - rems(34.), - window, - cx, - ) + BranchList::new(workspace_handle, repository, style, rems(34.), window, cx) }) } pub fn popover( + workspace: WeakEntity, repository: Option>, window: &mut Window, cx: &mut App, ) -> Entity { cx.new(|cx| { let list = BranchList::new( - None, + workspace, repository, BranchListStyle::Popover, rems(20.), @@ -117,7 +111,7 @@ pub struct BranchList { impl BranchList { fn new( - workspace: Option>, + workspace: WeakEntity, repository: Option>, style: BranchListStyle, width: Rems, @@ -332,7 +326,7 @@ impl BranchFilter { } pub struct BranchListDelegate { - workspace: Option>, + workspace: WeakEntity, matches: Vec, all_branches: Option>, default_branch: Option, @@ -360,7 +354,7 @@ enum PickerState { impl BranchListDelegate { fn new( - workspace: Option>, + workspace: WeakEntity, repo: Option>, style: BranchListStyle, cx: &mut Context, @@ -464,7 +458,7 @@ impl BranchListDelegate { log::error!("Failed to delete branch: {}", e); } - if let Some(workspace) = workspace.and_then(|w| w.upgrade()) { + if let Some(workspace) = workspace.upgrade() { cx.update(|_window, cx| { if is_remote { show_error_toast( @@ -880,19 +874,21 @@ impl PickerDelegate for BranchListDelegate { Entry::NewUrl { .. } | Entry::NewBranch { .. } | Entry::NewRemoteName { .. } ); - let delete_branch_button = IconButton::new("delete", IconName::Trash) - .tooltip(move |_, cx| { - Tooltip::for_action_in( - "Delete Branch", - &branch_picker::DeleteBranch, - &focus_handle, - cx, - ) - }) - .on_click(cx.listener(|this, _, window, cx| { - let selected_idx = this.delegate.selected_index(); - this.delegate.delete_at(selected_idx, window, cx); - })); + let deleted_branch_icon = |entry_ix: usize, is_head_branch: bool| { + IconButton::new(("delete", entry_ix), IconName::Trash) + .tooltip(move |_, cx| { + Tooltip::for_action_in( + "Delete Branch", + &branch_picker::DeleteBranch, + &focus_handle, + cx, + ) + }) + .disabled(is_head_branch) + .on_click(cx.listener(move |this, _, window, cx| { + this.delegate.delete_at(entry_ix, window, cx); + })) + }; let create_from_default_button = self.default_branch.as_ref().map(|default_branch| { let tooltip_label: SharedString = format!("Create New From: {default_branch}").into(); @@ -1008,10 +1004,12 @@ impl PickerDelegate for BranchListDelegate { self.editor_position() == PickerEditorPosition::End && !is_new_items, |this| { this.map(|this| { + let is_head_branch = + entry.as_branch().is_some_and(|branch| branch.is_head); if self.selected_index() == ix { - this.end_slot(delete_branch_button) + this.end_slot(deleted_branch_icon(ix, is_head_branch)) } else { - this.end_hover_slot(delete_branch_button) + this.end_hover_slot(deleted_branch_icon(ix, is_head_branch)) } }) }, @@ -1236,7 +1234,7 @@ mod tests { use super::*; use git::repository::{CommitSummary, Remote}; - use gpui::{TestAppContext, VisualTestContext}; + use gpui::{AppContext, TestAppContext, VisualTestContext}; use project::{FakeFs, Project}; use rand::{Rng, rngs::StdRng}; use serde_json::json; @@ -1285,35 +1283,47 @@ mod tests { ] } - fn init_branch_list_test( + async fn init_branch_list_test( repository: Option>, branches: Vec, cx: &mut TestAppContext, ) -> (Entity, VisualTestContext) { - let window = cx.add_window(|window, cx| { - let mut delegate = - BranchListDelegate::new(None, repository, BranchListStyle::Modal, cx); - delegate.all_branches = Some(branches); - let picker = cx.new(|cx| Picker::uniform_list(delegate, window, cx)); - let picker_focus_handle = picker.focus_handle(cx); - picker.update(cx, |picker, _| { - picker.delegate.focus_handle = picker_focus_handle.clone(); - }); + let fs = FakeFs::new(cx.executor()); + let project = Project::test(fs, [], cx).await; - let _subscription = cx.subscribe(&picker, |_, _, _, cx| { - cx.emit(DismissEvent); - }); + let workspace = cx.add_window(|window, cx| Workspace::test_new(project, window, cx)); - BranchList { - picker, - picker_focus_handle, - width: rems(34.), - _subscription, - } - }); + let branch_list = workspace + .update(cx, |workspace, window, cx| { + cx.new(|cx| { + let mut delegate = BranchListDelegate::new( + workspace.weak_handle(), + repository, + BranchListStyle::Modal, + cx, + ); + delegate.all_branches = Some(branches); + let picker = cx.new(|cx| Picker::uniform_list(delegate, window, cx)); + let picker_focus_handle = picker.focus_handle(cx); + picker.update(cx, |picker, _| { + picker.delegate.focus_handle = picker_focus_handle.clone(); + }); + + let _subscription = cx.subscribe(&picker, |_, _, _, cx| { + cx.emit(DismissEvent); + }); - let branch_list = window.root(cx).unwrap(); - let cx = VisualTestContext::from_window(*window, cx); + BranchList { + picker, + picker_focus_handle, + width: rems(34.), + _subscription, + } + }) + }) + .unwrap(); + + let cx = VisualTestContext::from_window(*workspace, cx); (branch_list, cx) } @@ -1349,7 +1359,7 @@ mod tests { init_test(cx); let branches = create_test_branches(); - let (branch_list, mut ctx) = init_branch_list_test(None, branches, cx); + let (branch_list, mut ctx) = init_branch_list_test(None, branches, cx).await; let cx = &mut ctx; branch_list @@ -1425,7 +1435,7 @@ mod tests { .await; cx.run_until_parked(); - let (branch_list, mut ctx) = init_branch_list_test(repository.into(), branches, cx); + let (branch_list, mut ctx) = init_branch_list_test(repository.into(), branches, cx).await; let cx = &mut ctx; update_branch_list_matches_with_empty_query(&branch_list, cx).await; @@ -1490,7 +1500,7 @@ mod tests { .await; cx.run_until_parked(); - let (branch_list, mut ctx) = init_branch_list_test(repository.into(), branches, cx); + let (branch_list, mut ctx) = init_branch_list_test(repository.into(), branches, cx).await; let cx = &mut ctx; // Enable remote filter branch_list.update(cx, |branch_list, cx| { @@ -1548,7 +1558,7 @@ mod tests { create_test_branch("develop", false, None, Some(700)), ]; - let (branch_list, mut ctx) = init_branch_list_test(None, branches, cx); + let (branch_list, mut ctx) = init_branch_list_test(None, branches, cx).await; let cx = &mut ctx; update_branch_list_matches_with_empty_query(&branch_list, cx).await; @@ -1658,7 +1668,8 @@ mod tests { create_test_branch(FEATURE_BRANCH, false, None, Some(900)), ]; - let (branch_list, mut ctx) = init_branch_list_test(repository.into(), branches, test_cx); + let (branch_list, mut ctx) = + init_branch_list_test(repository.into(), branches, test_cx).await; let cx = &mut ctx; branch_list @@ -1717,7 +1728,7 @@ mod tests { let repository = init_fake_repository(cx).await; let branches = vec![create_test_branch("main", true, None, Some(1000))]; - let (branch_list, mut ctx) = init_branch_list_test(repository.into(), branches, cx); + let (branch_list, mut ctx) = init_branch_list_test(repository.into(), branches, cx).await; let cx = &mut ctx; branch_list @@ -1795,7 +1806,7 @@ mod tests { init_test(cx); let branches = vec![create_test_branch("main_branch", true, None, Some(1000))]; - let (branch_list, mut ctx) = init_branch_list_test(None, branches, cx); + let (branch_list, mut ctx) = init_branch_list_test(None, branches, cx).await; let cx = &mut ctx; branch_list @@ -1858,7 +1869,7 @@ mod tests { init_test(cx); let branches = vec![create_test_branch("main", true, None, Some(1000))]; - let (branch_list, mut ctx) = init_branch_list_test(None, branches, cx); + let (branch_list, mut ctx) = init_branch_list_test(None, branches, cx).await; let cx = &mut ctx; let subscription = cx.update(|_, cx| { @@ -1870,6 +1881,11 @@ mod tests { branch_list .update_in(cx, |branch_list, window, cx| { window.focus(&branch_list.picker_focus_handle); + assert!( + branch_list.picker_focus_handle.is_focused(window), + "Branch picker should be focused when selecting an entry" + ); + branch_list.picker.update(cx, |picker, cx| { picker .delegate @@ -1881,6 +1897,9 @@ mod tests { cx.run_until_parked(); branch_list.update_in(cx, |branch_list, window, cx| { + // Re-focus the picker since workspace initialization during run_until_parked + window.focus(&branch_list.picker_focus_handle); + branch_list.picker.update(cx, |picker, cx| { let last_match = picker.delegate.matches.last().unwrap(); assert!(last_match.is_new_url()); @@ -1914,7 +1933,7 @@ mod tests { .map(|i| create_test_branch(&format!("branch-{:02}", i), i == 0, None, Some(i * 100))) .collect(); - let (branch_list, mut ctx) = init_branch_list_test(None, branches, cx); + let (branch_list, mut ctx) = init_branch_list_test(None, branches, cx).await; let cx = &mut ctx; update_branch_list_matches_with_empty_query(&branch_list, cx).await; diff --git a/crates/git_ui/src/commit_modal.rs b/crates/git_ui/src/commit_modal.rs index 822b2c8385c2d573ceb2dc2872a685c47ff51379..291a96f47590b145b5c190150af54bd3d43c2fff 100644 --- a/crates/git_ui/src/commit_modal.rs +++ b/crates/git_ui/src/commit_modal.rs @@ -337,6 +337,7 @@ impl CommitModal { active_repo, is_amend_pending, is_signoff_enabled, + workspace, ) = self.git_panel.update(cx, |git_panel, cx| { let (can_commit, tooltip) = git_panel.configure_commit_button(cx); let title = git_panel.commit_button_title(); @@ -354,6 +355,7 @@ impl CommitModal { active_repo, is_amend_pending, is_signoff_enabled, + git_panel.workspace.clone(), ) }); @@ -375,7 +377,14 @@ impl CommitModal { .style(ButtonStyle::Transparent); let branch_picker = PopoverMenu::new("popover-button") - .menu(move |window, cx| Some(branch_picker::popover(active_repo.clone(), window, cx))) + .menu(move |window, cx| { + Some(branch_picker::popover( + workspace.clone(), + active_repo.clone(), + window, + cx, + )) + }) .with_handle(self.branch_list_handle.clone()) .trigger_with_tooltip( branch_picker_button, diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index 8b32a79311f1f52036d8e54d182139d45bf64d10..1426ed1e65412da5cb8be22e7592e5a42917b367 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -594,7 +594,7 @@ pub struct GitPanel { tracked_staged_count: usize, update_visible_entries_task: Task<()>, width: Option, - workspace: WeakEntity, + pub(crate) workspace: WeakEntity, context_menu: Option<(Entity, Point, Subscription)>, modal_open: bool, show_placeholders: bool, @@ -5617,10 +5617,14 @@ impl RenderOnce for PanelRepoFooter { .as_ref() .map(|panel| panel.read(cx).project.clone()); - let repo = self + let (workspace, repo) = self .git_panel .as_ref() - .and_then(|panel| panel.read(cx).active_repository.clone()); + .map(|panel| { + let panel = panel.read(cx); + (panel.workspace.clone(), panel.active_repository.clone()) + }) + .unzip(); let single_repo = project .as_ref() @@ -5708,7 +5712,11 @@ impl RenderOnce for PanelRepoFooter { }); let branch_selector = PopoverMenu::new("popover-button") - .menu(move |window, cx| Some(branch_picker::popover(repo.clone(), window, cx))) + .menu(move |window, cx| { + let workspace = workspace.clone()?; + let repo = repo.clone().flatten(); + Some(branch_picker::popover(workspace, repo, window, cx)) + }) .trigger_with_tooltip( branch_selector_button, Tooltip::for_action_title("Switch Branch", &zed_actions::git::Switch),