yenru0/zed
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

git: Fix deletion icon button in branch list deleting the wrong branch (#45087)

Browse Source 
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<Workspace> 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
This commit is contained in:
Anthony Eid authored and GitHub committed 2025-12-17 10:20:43 +00:00
1 parent af441f3b3c
commit c8cca9ce38
3 files changed
+101 -65
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

crates/git_ui/src/branch_picker.rs
+79 -60
View File
@@ -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<Workspace>,
repository: Option<Entity<Repository>>,
window: &mut Window,
cx: &mut App,
) -> Entity<BranchList> {
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<WeakEntity<Workspace>>,
workspace: WeakEntity<Workspace>,
repository: Option<Entity<Repository>>,
style: BranchListStyle,
width: Rems,
@@ -332,7 +326,7 @@ impl BranchFilter {
}
pub struct BranchListDelegate {
workspace: Option<WeakEntity<Workspace>>,
workspace: WeakEntity<Workspace>,
matches: Vec<Entry>,
all_branches: Option<Vec<Branch>>,
default_branch: Option<SharedString>,
@@ -360,7 +354,7 @@ enum PickerState {
impl BranchListDelegate {
fn new(
workspace: Option<WeakEntity<Workspace>>,
workspace: WeakEntity<Workspace>,
repo: Option<Entity<Repository>>,
style: BranchListStyle,
cx: &mut Context<BranchList>,
@@ -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<Entity<Repository>>,
branches: Vec<Branch>,
cx: &mut TestAppContext,
) -> (Entity<BranchList>, 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;
crates/git_ui/src/commit_modal.rs
+10 -1
View File
@@ -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,
crates/git_ui/src/git_panel.rs
+12 -4
View File
@@ -594,7 +594,7 @@ pub struct GitPanel {
tracked_staged_count: usize,
update_visible_entries_task: Task<()>,
width: Option<Pixels>,
workspace: WeakEntity<Workspace>,
pub(crate) workspace: WeakEntity<Workspace>,
context_menu: Option<(Entity<ContextMenu>, Point<Pixels>, 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),