Fix branch selector confusion during merge/rebase conflicts#2017
Conversation
When a repo is mid-rebase (or merge/cherry-pick/revert), the new task input's branch selector showed three confusing things: 1. The first item in the dropdown was `(no` — simple-git's `branchLocal()` parser misreads git's `(no branch, rebasing main)` pseudo-branch line. Switched `getAllBranches` to `git for-each-ref refs/heads/`, which only returns real local branches. 2. The label said "No branch" with no hint that an operation was in progress. Added a `getGitBusyState` query that exposes rebase/merge/cherry-pick/revert state, and the selector now shows "Rebasing", "Merging", etc. when there is no current branch. 3. Selecting a branch threw a checkout error because git refuses checkout while a working-tree operation is in progress. The selector is now disabled in local mode while busy, with a tooltip explaining how to recover. The busy-state probe (rebase-merge/, MERGE_HEAD, etc.) was already implemented inside `CaptureCheckpointSaga`; extracted it to `inspectGitBusyState` in `queries.ts` so the saga and the new public `getGitBusyState(baseDir)` share one implementation. Generated-By: PostHog Code Task-Id: e47ff9ba-d59b-4212-bc43-c49a5cd8db16
bd0f44d to
899a337
Compare
Without this, aborting/finishing a rebase or merge from outside the app leaves the branch selector showing a stale "Rebasing" label until the next 30s poll. Generated-By: PostHog Code Task-Id: c5674c04-c95c-4bfe-bfcf-248a921b4aae
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/code/src/renderer/features/git-interaction/components/BranchSelector.tsx:243-246
**Tooltip may never appear on a disabled button**
The tooltip's `open` prop depends on `hovered`, which is only set by `onMouseEnter`/`onMouseLeave` on the inner `Button`. The HTML spec prohibits disabled form elements from receiving pointer events, so once `localBusy` is true the button is disabled and mouse events stop firing — the `disabledReason` tooltip ("Rebasing in progress — finish or abort it to switch branches.") will never show. Moving the `onMouseEnter`/`onMouseLeave` handlers to the `ComboboxTrigger` wrapper (or a surrounding `div`) would let the tooltip fire regardless of the button's disabled state.
### Issue 2 of 3
packages/git/src/queries.ts:393-397
**Duplicate type definitions (OnceAndOnlyOnce)**
`GitBusyOperation` and `GitBusyState` are now defined identically in both `packages/git/src/queries.ts` (here) and `apps/code/src/shared/types.ts`. The `schemas.ts` re-exports them from `shared/types`, while `checkpoint.ts` re-exports from `queries.ts`. The two definitions must be kept in sync by hand. One canonical definition should be used — if the monorepo boundary prevents `packages/git` from importing `@shared/types`, then `shared/types` should import and re-export from `packages/git/src/queries` rather than duplicating.
### Issue 3 of 3
packages/git/src/queries.test.ts:372-397
**Rebase-conflict setup is duplicated (OnceAndOnlyOnce)**
The steps to produce a mid-rebase conflict repo (create `feature`, make a conflicting commit on `main`, check out `feature`, trigger `git rebase`) are written out in full both inside `setupRebaseConflict` (used by the `getAllBranches` suite) and again inline in the `getGitBusyState` "detects an in-progress rebase" test. Extracting `setupRebaseConflict` to a module-level helper — or having the busy-state test call it — would remove the duplication and align with the team's preference for shared test utilities.
Reviews (1): Last reviewed commit: "fix(git): invalidate busy-state on git e..." | Re-trigger Greptile |
| <Tooltip | ||
| content={displayedBranch ?? "Switch branch"} | ||
| content={disabledReason ?? displayedBranch ?? "Switch branch"} | ||
| side="bottom" | ||
| open={hovered && !open && !effectiveLoading} |
There was a problem hiding this comment.
Tooltip may never appear on a disabled button
The tooltip's open prop depends on hovered, which is only set by onMouseEnter/onMouseLeave on the inner Button. The HTML spec prohibits disabled form elements from receiving pointer events, so once localBusy is true the button is disabled and mouse events stop firing — the disabledReason tooltip ("Rebasing in progress — finish or abort it to switch branches.") will never show. Moving the onMouseEnter/onMouseLeave handlers to the ComboboxTrigger wrapper (or a surrounding div) would let the tooltip fire regardless of the button's disabled state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/git-interaction/components/BranchSelector.tsx
Line: 243-246
Comment:
**Tooltip may never appear on a disabled button**
The tooltip's `open` prop depends on `hovered`, which is only set by `onMouseEnter`/`onMouseLeave` on the inner `Button`. The HTML spec prohibits disabled form elements from receiving pointer events, so once `localBusy` is true the button is disabled and mouse events stop firing — the `disabledReason` tooltip ("Rebasing in progress — finish or abort it to switch branches.") will never show. Moving the `onMouseEnter`/`onMouseLeave` handlers to the `ComboboxTrigger` wrapper (or a surrounding `div`) would let the tooltip fire regardless of the button's disabled state.
How can I resolve this? If you propose a fix, please make it concise.| export type GitBusyOperation = "rebase" | "merge" | "cherry-pick" | "revert"; | ||
|
|
||
| export type GitBusyState = | ||
| | { busy: false } | ||
| | { busy: true; operation: GitBusyOperation }; |
There was a problem hiding this comment.
Duplicate type definitions (OnceAndOnlyOnce)
GitBusyOperation and GitBusyState are now defined identically in both packages/git/src/queries.ts (here) and apps/code/src/shared/types.ts. The schemas.ts re-exports them from shared/types, while checkpoint.ts re-exports from queries.ts. The two definitions must be kept in sync by hand. One canonical definition should be used — if the monorepo boundary prevents packages/git from importing @shared/types, then shared/types should import and re-export from packages/git/src/queries rather than duplicating.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/git/src/queries.ts
Line: 393-397
Comment:
**Duplicate type definitions (OnceAndOnlyOnce)**
`GitBusyOperation` and `GitBusyState` are now defined identically in both `packages/git/src/queries.ts` (here) and `apps/code/src/shared/types.ts`. The `schemas.ts` re-exports them from `shared/types`, while `checkpoint.ts` re-exports from `queries.ts`. The two definitions must be kept in sync by hand. One canonical definition should be used — if the monorepo boundary prevents `packages/git` from importing `@shared/types`, then `shared/types` should import and re-export from `packages/git/src/queries` rather than duplicating.
How can I resolve this? If you propose a fix, please make it concise.| it("detects an in-progress rebase", async () => { | ||
| repoDir = await setupRepo("main"); | ||
| const git = createGitClient(repoDir); | ||
|
|
||
| await git.checkoutLocalBranch("feature"); | ||
| await writeFile(path.join(repoDir, "file.txt"), "feature change\n"); | ||
| await git.add(["file.txt"]); | ||
| await git.commit("on feature"); | ||
|
|
||
| await git.checkout("main"); | ||
| await writeFile(path.join(repoDir, "file.txt"), "main change\n"); | ||
| await git.add(["file.txt"]); | ||
| await git.commit("on main"); | ||
|
|
||
| await git.checkout("feature"); | ||
| try { | ||
| await git.rebase(["main"]); | ||
| } catch { | ||
| // expected: conflict | ||
| } | ||
|
|
||
| expect(await getGitBusyState(repoDir)).toEqual({ | ||
| busy: true, | ||
| operation: "rebase", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Rebase-conflict setup is duplicated (OnceAndOnlyOnce)
The steps to produce a mid-rebase conflict repo (create feature, make a conflicting commit on main, check out feature, trigger git rebase) are written out in full both inside setupRebaseConflict (used by the getAllBranches suite) and again inline in the getGitBusyState "detects an in-progress rebase" test. Extracting setupRebaseConflict to a module-level helper — or having the busy-state test call it — would remove the duplication and align with the team's preference for shared test utilities.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/git/src/queries.test.ts
Line: 372-397
Comment:
**Rebase-conflict setup is duplicated (OnceAndOnlyOnce)**
The steps to produce a mid-rebase conflict repo (create `feature`, make a conflicting commit on `main`, check out `feature`, trigger `git rebase`) are written out in full both inside `setupRebaseConflict` (used by the `getAllBranches` suite) and again inline in the `getGitBusyState` "detects an in-progress rebase" test. Extracting `setupRebaseConflict` to a module-level helper — or having the busy-state test call it — would remove the duplication and align with the team's preference for shared test utilities.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
When the local repo is mid-rebase (or merge/cherry-pick/revert), the new task input's branch selector behaved badly in three ways. This PR cleans all three up.
(nosimple-git'sbranchLocal()parser greedily mis-parses git's* (no branch, rebasing main)pseudo-branch line and returns(noas a "branch name"getAllBranchestogit for-each-ref refs/heads/so only real branches are returnedgetCurrentBranchreturnsnullduring rebase (HEAD is detached on the rebase pseudo-ref)getGitBusyStatetRPC query reportsrebase/merge/cherry-pick/revert; the selector now shows "Rebasing", "Merging", etc. when there's no current branchFailed to checkout …checkoutwhile an operation is in progressThe busy-state probe (
rebase-merge/,MERGE_HEAD,CHERRY_PICK_HEAD,REVERT_HEAD) was already implemented insideCaptureCheckpointSaga. It's now extracted toinspectGitBusyStateinqueries.tsso the saga and the new publicgetGitBusyState(baseDir)share one implementation — no behavior change for the saga; existing 18 checkpoint tests still pass.Worktree and cloud modes are unaffected (
busyStateonly applies when!isSelectionOnly).Test plan
getAllBranchesfiltering the(nopseudo-branch during a rebase conflictgetGitBusyStatereporting{ busy: true, operation: "rebase" }mid-rebasecheckpoint sagastests still pass after extractinginspectGitBusyStateCreated with PostHog Code