From c8cca9ce38fa44d7bdeb75337f5cd8333ed4086e 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 cbf3fea72a..7395f1588f 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 branch_list = window.root(cx).unwrap(); - let cx = VisualTestContext::from_window(*window, cx); + let _subscription = cx.subscribe(&picker, |_, _, _, cx| { + cx.emit(DismissEvent); + }); + + 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 822b2c8385..291a96f475 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 8b32a79311..1426ed1e65 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),