fix: Hide worktree branches from branch switcher and show worktree tabs #602
Conversation
- Add checkedOutInWorktree field to BranchInfo to track which branches are already checked out in worktrees - Filter out worktree branches from the branch switch dropdown since git doesn't allow checking out a branch that's in another worktree - Show worktree tabs when worktrees exist, even if "Use Worktrees" setting is disabled, so users can navigate to their worktrees Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @kevinisyoung, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses user experience issues related to Git worktrees. It prevents common errors by intelligently filtering available branches in the UI and enhances discoverability by ensuring that existing worktrees are always visible, even when a specific feature setting is not explicitly enabled. The changes aim to make worktree management more intuitive and less prone to user frustration. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses two issues related to Git worktrees. Hiding branches already checked out in other worktrees from the branch switcher is a great UX improvement that prevents users from running into Git errors. Similarly, ensuring worktree tabs are visible when worktrees exist, regardless of the 'Use Worktrees' setting, provides a necessary navigation path for users who might have disabled the setting but still have active worktrees. The changes on both the server and UI sides are clear and well-implemented. I have one suggestion to improve error logging for better diagnostics.
| } catch { | ||
| // If we can't get worktree info, return empty map | ||
| } |
There was a problem hiding this comment.
While returning an empty map on failure is a reasonable fallback, swallowing the error completely can make debugging difficult if git worktree list fails for an unexpected reason. It's better to log the error using the existing logWorktreeError utility to aid in troubleshooting.
| } catch { | |
| // If we can't get worktree info, return empty map | |
| } | |
| } catch (error) { | |
| // If we can't get worktree info, log it and return an empty map so the main functionality isn't blocked. | |
| logWorktreeError(error, 'Failed to get worktree branches', cwd); | |
| } |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe changes introduce worktree-awareness to branch listing by adding a Changes
Sequence DiagramsequenceDiagram
participant Server as Server API
participant UI as UI Components
participant User as User
Server->>Server: Fetch local & remote branches
Server->>Server: Query worktree checkout info
Server->>Server: Map branches to worktree paths
Server->>UI: Return BranchInfo[] with checkedOutInWorktree
UI->>UI: Parse received branch data
UI->>UI: Filter out branches with checkedOutInWorktree set
UI->>UI: Check worktree panel visibility condition
UI->>User: Render filtered branch dropdown & worktree panel
User->>UI: Click branch or worktree
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsx (1)
88-109: Edge case: empty dropdown after worktree filtering.When all
filteredBranchesare checked out in worktrees, the filter at line 94 produces an empty list, but the empty state message at line 88 won't display (sincefilteredBranches.length > 0). Users would see an empty dropdown with no explanation.Consider computing the displayable branches once and using that for both the empty state check and rendering:
Suggested approach
+ const displayableBranches = filteredBranches.filter( + (branch) => !branch.checkedOutInWorktree + ); {isLoadingBranches ? ( <DropdownMenuItem disabled className="text-xs"> <Spinner size="xs" className="mr-2" /> Loading branches... </DropdownMenuItem> - ) : filteredBranches.length === 0 ? ( + ) : displayableBranches.length === 0 ? ( <DropdownMenuItem disabled className="text-xs"> {branchFilter ? 'No matching branches' : 'No branches found'} </DropdownMenuItem> ) : ( - filteredBranches - .filter((branch) => !branch.checkedOutInWorktree) // Hide branches already in worktrees - .map((branch) => ( + displayableBranches.map((branch) => ( // ... rest unchanged )) )}
🤖 Fix all issues with AI agents
In `@apps/server/src/routes/worktree/routes/list-branches.ts`:
- Line 92: The local const worktreePath returned from
worktreeBranches.get(cleanName) shadows the worktreePath from the request body;
rename the local variable (e.g., branchWorktreePath or branchPath) at the
declaration where worktreeBranches.get(cleanName) is used and update all
subsequent references in this function (including any null/undefined checks and
uses when constructing the branch object) so the outer request body worktreePath
remains distinct and unshadowed.
♻️ Duplicate comments (1)
apps/server/src/routes/worktree/routes/list-branches.ts (1)
46-48: Swallowing errors silently makes debugging difficult.This issue was already raised in a previous review: the empty catch block should log the error using
logWorktreeErrorto aid troubleshooting whengit worktree listfails unexpectedly.
🧹 Nitpick comments (2)
apps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsx (1)
19-19: Consider removing unusedbranchesprop from interface.The
branchesprop is declared in the interface but never used in the component (onlyfilteredBranchesis consumed). If this prop isn't needed for future functionality, removing it would clean up the API.apps/server/src/routes/worktree/routes/list-branches.ts (1)
9-13: Consider importingexecAsyncfromcommon.tsinstead of re-declaring.The
execAsyncutility is already exported from../common.js. Importing it would reduce duplication and maintain consistency across worktree routes.Suggested change
import type { Request, Response } from 'express'; -import { exec } from 'child_process'; -import { promisify } from 'util'; -import { getErrorMessage, logWorktreeError } from '../common.js'; - -const execAsync = promisify(exec); +import { execAsync, getErrorMessage, logWorktreeError } from '../common.js';
| .map((name) => { | ||
| // Remove any surrounding quotes (Windows git may preserve them) | ||
| const cleanName = name.trim().replace(/^['"]|['"]$/g, ''); | ||
| const worktreePath = worktreeBranches.get(cleanName); |
There was a problem hiding this comment.
Variable shadowing: worktreePath shadows outer scope.
The variable worktreePath declared here shadows the worktreePath from the request body (line 56). This is confusing and could lead to subtle bugs if the outer variable is accidentally used.
Suggested rename
- const worktreePath = worktreeBranches.get(cleanName);
+ const branchWorktreePath = worktreeBranches.get(cleanName);
return {
name: cleanName,
isCurrent: cleanName === currentBranch,
isRemote: false,
// Mark if this branch is checked out in a worktree (and it's not the current worktree)
- checkedOutInWorktree: worktreePath && cleanName !== currentBranch ? worktreePath : null,
+ checkedOutInWorktree: branchWorktreePath && cleanName !== currentBranch ? branchWorktreePath : null,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const worktreePath = worktreeBranches.get(cleanName); | |
| const branchWorktreePath = worktreeBranches.get(cleanName); | |
| return { | |
| name: cleanName, | |
| isCurrent: cleanName === currentBranch, | |
| isRemote: false, | |
| // Mark if this branch is checked out in a worktree (and it's not the current worktree) | |
| checkedOutInWorktree: branchWorktreePath && cleanName !== currentBranch ? branchWorktreePath : null, | |
| }; |
🤖 Prompt for AI Agents
In `@apps/server/src/routes/worktree/routes/list-branches.ts` at line 92, The
local const worktreePath returned from worktreeBranches.get(cleanName) shadows
the worktreePath from the request body; rename the local variable (e.g.,
branchWorktreePath or branchPath) at the declaration where
worktreeBranches.get(cleanName) is used and update all subsequent references in
this function (including any null/undefined checks and uses when constructing
the branch object) so the outer request body worktreePath remains distinct and
unshadowed.
2 participants
Summary
Problem
When users had worktrees but didn't have the "Use Worktrees" setting enabled:
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Style
✏️ Tip: You can customize this high-level summary in your review settings.