feat(ui): delete a local branch from a context menu - #43
Merged
Conversation
gitr could only read a repository. This is the first operation that writes to one, so it sets the pattern for the rest. No port in `domain`. The obvious move was a `RepositoryWriter` trait beside `RepositoryReader`, and it would have been the only one of its kind: clone and fetch are already inherent methods on `GitRunner`, called concretely from `Workspace`. Adding a port for a third operation would leave two conventions where there is currently one, so this follows the one that exists. Deleting the checked-out branch is allowed and switches to `main` first, or to `master` where there is no `main`. A repository with neither is not offered the entry at all rather than offered it and failing with nowhere to go, and the same holds for deleting the fallback branch itself while standing on it. The two steps live in one adapter call rather than being sequenced by the UI, because their failure semantics are the interesting part: a refused switch must leave both the checkout and the branch untouched, and that is what the test asserts. `git branch -d`, never `-D`. An unmerged branch is refused, and the refusal is recognised rather than reported as a bare exit status, so the notification says what is wrong instead of quoting a status code. Forcing is deliberately absent: an unmerged branch is only recoverable through the reflog, which makes it a different decision from this one and not a flag on it. No confirmation dialog. The house rule in `crates/ui/src/lib.rs` keeps modals out, and the consequence is carried by the menu item's own label — "Delete branch and switch to main" says what will happen before it happens.
A branch name appears in two places and only one of them could act on it. The deletion rule moves to `branch_actions`, shared by both surfaces rather than written twice. `Deletion::switch_to` returns a nested option because there are three outcomes, not two: not deletable at all, deletable where it stands, and deletable after switching. Collapsing that to a bool loses the middle case, which is the common one. Its four cases are unit-tested; the sidebar and the badge then carry no policy of their own. The badge needed a handle on the workspace, which the history panel did not have. It is set once in `Workspace::new` rather than passed through `install_default_layout`, which takes `&mut App` and so has no entity to weaken. The menu is offered on local branches only. A remote branch and a tag reach the same code path and fall out of it on the reference's own variant, so neither surface has to remember which section it is drawing.
The notification read "Could not delete fix/graph-lines: fix/graph-lines is not fully merged". The caller already names the branch it could not delete, so the error only has to give the reason. `NotMerged` drops its `branch` field rather than keeping it unread: the one place that reports it has the name in hand, and a field carried only to be discarded is a second source of truth for the same string.
Deleting a branch failed on every branch worth deleting, and the invariant was wrong rather than the code. `git branch -d` asks whether a branch's commits are ancestors of HEAD. A squash merge does not make them ancestors: it lands one new commit carrying their combined content, so the originals stay unreachable forever. This repository squash-merges every pull request, which made `-d` refuse all eleven of its branches — the feature was unusable in the workflow it shipped into. Forcing was not the answer. `-D` would delete an unmerged branch just as readily as a squash-merged one, and the point of refusing was never the flag, it was not losing work. So the branch's content is compared against the integration branch instead: replay its tree onto the merge base as a dangling commit, then ask `git cherry` whether an equivalent patch is already there. Content in, commits not — which is exactly what a squash merge leaves behind. Only then does `-D` run. Two ways this could delete something. A branch that is partly merged looks merged if only its ancestry is checked, so the comparison is over the whole tree, and a test keeps a commit back to prove a partial match is refused. And comparing the integration branch against itself always matches, which would have turned "delete main" into "force-delete main"; that is now an early return with its own test, found by reading rather than by running it. `NotMerged` now means something stronger than git's own phrasing, so it says so: commits that are in no other branch, rather than not fully merged.
`feat/single-instance` still refused to delete, and the reason is that it fed two pull requests. Its content reached main as two separate squash commits, so no single commit there carries the branch's combined diff — which is exactly what the previous check looked for, and all it looked for. `git merge-tree --write-tree` answers the real question instead: merge the branch into the integration branch and see whether the result differs from it. Nothing new to add means nothing to lose, however many squashes, rebases or cherry-picks carried it there. That alone is not enough either. `chantier/icon` and `docs/repository-migration` both landed and were then edited again on main, so replaying them three-way from their old merge base conflicts and `merge-tree` cannot answer. The earlier probe answers those, because their whole diff does appear in main's history as one squash commit. So both run, and either one is enough. They fail on disjoint shapes — one on a branch split across several commits, the other on a branch main has since rewritten — and neither can say yes without real evidence that the content is already there. Widening what is accepted, not weakening what is proved. Measured on this repository: every one of the eight merged branches is now deletable, and the only refusal is the branch being worked on, which genuinely has commits nowhere else.
Three rounds of merge detection to answer a question nobody asked. The request was to delete a local branch; the safety policy around it was mine, not the user's, and each attempt to make it correct made it more elaborate and still refused branches they wanted gone. `git branch -D`, once. `BranchError::NotMerged` is gone, and so are the two content probes, the integration-branch argument threaded through the UI, and the seven tests that pinned their behaviour. A deleted local branch is recoverable from the reflog, which is the same guarantee the command line gives. What stays is the part that was asked for: deleting the checked-out branch switches to main first, and a switch git refuses — uncommitted work that would be overwritten — leaves both the checkout and the branch untouched. That is git protecting the working tree, not a rule invented here.
`PopupMenu::label` for the heading, `IconName::Delete` on the entry. The icon cannot silently render blank, which is the usual failure here — gpui logs a missing asset and draws nothing. `IconName` is generated from the icons that `gpui-component-assets` ships, so the variant exists only because `delete.svg` does, and naming it is proof enough that it resolves. Both call sites now go through one `branch_menu` rather than assembling the menu themselves, so the title and the icon cannot drift between the sidebar and the history badge.
Three things, one cause: gpui-component sizes an icon for you and overrides whatever you set. `PopupMenu::render_icon` ends on `icon.xsmall()`, so passing a bigger `Icon` through `PopupMenuItem::icon` changed nothing. The same call is what indented the title: a menu holding any icon reserves a left column on *every* row, "Actions" included. Rendering the row through `PopupMenuItem::element` takes both back — the icon is sized here, and with no `icon` field set the column disappears and the title starts at the left edge. `Button` derives its icon's size from its own, so the title bar's three controls drew at `Size::Small`, which is `size_3p5()` — 14px. `Size::Medium` is `size_4()`, exactly the 16px the menu now uses, so dropping `.small()` matches them by construction rather than by a hand-tuned number. Measured: the title bar is 33px tall before and after, so the larger buttons cost no height.
`.small()` was its only user in this file. Caught by clippy after the previous commit was already pushed: the check ran, reported 101, and the commit went out anyway because the exit code was read but not acted on.
…drew Same override as the branch menu: `PopupMenu::render_icon` ends on `icon.xsmall()`, so these three drew at 12px whatever was passed. Rendering the row through `PopupMenuItem::element` takes the size back. Doing that exposed a second bug in the same call. `render_icon` picks the item's icon *or* the check, never both, so an item carrying an icon can never show one — the theme menu has drawn no check on the active preference since it was written, which is not what `theme_preference_control`'s own doc comment claims it does. The row now places the check itself, on the right, and the screenshot confirms it appears against System. The 16px lives in `density` now rather than beside the branch menu, since two unrelated menus agreeing on it by copy is how they stop agreeing later.
Both panels live in the dock area, so "outside both" is "outside the dock" — no bounds arithmetic and no per-panel bookkeeping. Expressed as the dock swallowing the click rather than the sidebar reporting it. The first attempt wrapped the sidebar in a div carrying the handler, which is a layout change smuggled in for an event handler's sake: that div becomes the flex child the sidebar used to be, and its sizing is now the wrapper's, not the sidebar's. Putting the handler on the row that already exists and stopping propagation inside the dock child — a div that is also already there — leaves every element and every style exactly as it was. The title bar is deliberately not part of "outside". Its own toggle would otherwise hide the panel and then reveal it in the same click, since the two handlers fire in sequence on one gesture. Not verified on the running app: the screen was in use, and driving it means posting synthetic clicks into whatever the user is doing.
…the tags Three things reported in a row, and the first two share a root. **The detail panel no longer opens at startup.** Nothing is selected then, so there is nothing to show; `Workspace::new` detaches it and a selection reattaches it. A single click now reveals it, where only a double click did: with the panel no longer always present, selecting a commit and seeing nothing would be the more surprising half of the change. **Arrow keys stopped moving through the history**, and revealing the panel is why. Attaching it moves focus into it — measured by diffing two screenshots after three Down presses, where the only pixels that changed were a caret blinking inside the detail panel. `reveal_detail` hands focus back to the history panel. That regression had a second cause, found by reading rather than running. Closing the panel on a click outside it was implemented by stopping the click at each commit row, which also stops it reaching the table's own focusable ancestor — so clicking a row would never focus the table, and the arrows would have stayed dead even with focus restored. The stop lives on the dock's wrapper again, outside the table rather than inside it, where it blocks nothing the table needs. The consequence is that the panel closes on a click in the sidebar or the status bar, not on a click in the history's own toolbar or empty space. Narrowing it to "a commit row only" needs the row to mark the click rather than swallow it, which is a different design and not a smaller one. **Tags are yellow.** `theme.yellow` resolves to `#ca8a04` under the light theme, an amber dark enough to read as brown beside the current branch's orange. Tags carry their own value now, as that orange already does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Right-click a local branch name — in the sidebar or on a history badge — and delete it.
gitr could only read a repository. This is the first operation that writes to one, so most
of what follows is about the precedent rather than about the feature.
No port in
domainThe obvious move was a
RepositoryWritertrait besideRepositoryReader. It would havebeen the only one of its kind: clone and fetch are already inherent methods on
GitRunner,called concretely from
Workspace, behind no port at all. Adding one for a third operationleaves two conventions where there is currently one, so this follows the one that exists —
GitRunner::delete_local_branchincrates/vcs/src/process/branch.rs, next toclone_bareand
fetch.CLAUDE.mddescribescrates/gitr/as "the only place adapters are wired to ports". Thatsentence does not describe the code: nothing is wired there today. Worth reconciling, but
not in a change that would then be the sole exception to it.
Delete means delete
git branch -D, once, with no merged-check of its own.This took three rounds to get right, and all three were wrong in the same direction. The
first used
git branch -dand refused all eleven branches in this repository —-dtests whether a branch's commits are ancestors of HEAD, and a squash merge never makes them
ancestors, so every squash-merged branch reads as unmerged. The second compared the branch's
whole diff against main; that missed branches that fed two pull requests. The third added a
merge-treeprobe alongside it; that one worked, and was an elaborate answer to a questionthat was never asked.
A local branch is a pointer. Deleting one is recoverable from the reflog, which is exactly
the guarantee the command line gives, and a client that second-guesses the request is worse
than one that performs it.
Deleting the branch you are standing on
Allowed, and it switches to
mainfirst —masterwhere there is nomain. Three cases,not two, which is why
Deletion::switch_toreturns a nested option: not deletable at all,deletable where it stands, deletable after switching. Collapsing that to a bool loses the
middle case, which is the common one.
Not offered when there is no fallback, and not offered for the fallback branch itself while
standing on it — switching to the branch being deleted is not a way out of it.
The two steps live in one adapter call rather than being sequenced by the UI, because their
failure semantics are the interesting part: a switch git refuses must leave both the
checkout and the branch untouched. A test asserts exactly that, by leaving an uncommitted
edit
git checkoutwill not overwrite. That refusal is git protecting the working tree, nota rule invented here.
No confirmation dialog
The house rule in
crates/ui/src/lib.rskeeps modals out of this crate. The consequence iscarried by the menu item's own label instead: "Delete branch and switch to main" says
what will happen before it happens.
Two surfaces, one rule
A branch name appears in the sidebar and on a history badge. Both offer the menu, and the
policy lives in
branch_actionsrather than being written twice. Local branches only — aremote branch and a tag reach the same code path and fall out of it on the reference's own
variant, so neither surface has to remember which section it is drawing.
SidebarTreeItemgained acontext_menubuilder mirroring itson_click. The badge neededa handle on the workspace, which the history panel did not have; it is set once in
Workspace::newrather than threaded throughinstall_default_layout, which takes&mut Appand so has no entity to weaken.Tests
Five integration tests against real repositories under
tempfile: a branch is deleted, abranch holding commits of its own is deleted too, deleting the checked-out branch switches
away first, a refused switch leaves both the checkout and the branch alone, and a missing
branch reports git's own reason.
Four unit tests on the switch-away policy and three on the fallback branch, all pure.
cargo test --workspacegreen;cargo clippy --workspace --all-targets -- -D warnings;cargo fmt --all --check.Verified by hand
Right-clicking a branch badge opens the menu and deletes the branch.
Two things got in the way of confirming that, both worth writing down.
A stale binary.
~/.cargo/bin/gitrwas running from a build made before this branchexisted, and gitr allows one instance: every
./target/debug/gitr .handed off to it overthe Unix socket and exited, so the window under test was never the build under test. The
symptom is indistinguishable from a broken feature.
$HOMEis what resolves the socket'sdirectory, so running the development build under a scratch
HOMEgives it its own socketand lets it coexist with an installed one — that is the way to test a branch without
quitting the app.
Synthetic right-clicks did not raise the menu, where a real one does.
ContextMenuExt::context_menuopens on aMouseDownEventgated byhitbox.is_hovered(window), and a postedmouseMoveddoes not appear to settle gpui'shover state before the button event lands. Driving this particular interaction is a human's
job.