-
Notifications
You must be signed in to change notification settings - Fork 897
feat(desktop): worktree bar refactor + open workspace modal base branch #438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ce00113
worktree bar refactor + open workspace modal base branch
AviPeltz 8de9caa
lint fix
AviPeltz 9ad072c
merged shit together and made button nicer
AviPeltz 7794f27
lint fix
AviPeltz 671863b
fixed main branch tab and a bunch of other ish about remote branches
AviPeltz 9c80f28
fixed some branch shit and removed unused hasOrigin
AviPeltz 4be2d2d
fix md file
AviPeltz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| # Branch Workspace Improvements | ||
|
|
||
| Potential improvements identified during code review. Create Linear tickets as needed. | ||
|
|
||
| ## Medium Priority | ||
|
|
||
| ### 1. Cache `hasOrigin` at project level | ||
| **Location**: `workspaces.ts:460`, `projects.ts` | ||
|
|
||
| Currently `hasOriginRemote()` is called on every `getActive` poll until base branch detection completes. Cache this at the project level in the database to avoid repeated git calls. | ||
|
|
||
| ### 2. Race condition in worktree creation | ||
| **Location**: `workspaces.ts:78-86` | ||
|
|
||
| The branch existence check and the actual worktree creation are not atomic. If a branch is deleted between the check and use, it fails with a generic git error. Consider: | ||
| - Wrapping in a retry with exponential backoff | ||
| - Catching the specific git error and providing a clear message | ||
|
|
||
| ### 3. `getDefaultBranch` for local repos uses current branch | ||
| **Location**: `git.ts:287-306` | ||
|
|
||
| For repos without a remote, `getDefaultBranch` returns the current branch. If the user is on a feature branch, that becomes the "default" for new worktrees, which may not be intended. Consider always preferring `main`/`master` if they exist locally. | ||
|
|
||
| ### 4. Disable Create button when no branches available | ||
| **Location**: `NewWorkspaceModal.tsx` | ||
|
|
||
| If `getBranches` returns an empty array (e.g., new repo with no commits), the Create button is still enabled. Should be disabled with a helpful message. | ||
|
|
||
| ## Low Priority | ||
|
|
||
| ### 5. Rename `fetch` parameter to `gitFetch` | ||
| **Location**: `workspaces.ts` - `getBranches` procedure | ||
|
|
||
| The `fetch` parameter controls whether to run `git fetch`, but could be confused with React Query's fetch behavior. Rename to `gitFetch` or `refreshRemote` for clarity. | ||
|
|
||
| ### 6. Extract magic numbers to constants | ||
| **Location**: `WorkspaceHeader.tsx` | ||
|
|
||
| Values like `max-w-[480px]`, `h-[22px]`, `max-w-[180px]` should be design tokens or named constants. | ||
|
|
||
| ### 7. Lazy compute branch arrays | ||
| **Location**: `projects.ts:185-196` | ||
|
|
||
| `localBranches` and `remoteBranches` arrays are always built but only used in the fallback path. Could be computed lazily for minor perf improvement. | ||
|
|
||
| ### 8. Add loading skeleton to base branch picker | ||
| **Location**: `NewWorkspaceModal.tsx` | ||
|
|
||
| Currently shows a disabled button while branches load. A skeleton would feel more polished. | ||
|
|
||
| ### 9. Optimistic UI for branch switching | ||
| **Location**: `BranchSwitcher.tsx` | ||
|
|
||
| The `switchBranchWorkspace` mutation could use optimistic updates to feel snappier. | ||
|
|
||
| ## Testing | ||
|
|
||
| ### 10. Add unit tests for git utilities | ||
| **Location**: `git.ts` | ||
|
|
||
| Functions like `getDefaultBranch`, `branchExistsOnRemote`, `detectBaseBranch`, and `checkBranchCheckoutSafety` have complex logic that would benefit from unit tests with mocked `simple-git`. | ||
|
|
||
| Priority candidates: | ||
| - `getDefaultBranch` - many code paths | ||
| - `detectBaseBranch` - merge-base logic | ||
| - `checkBranchCheckoutSafety` - safety checks before checkout | ||
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix style issues in Medium Priority section.
Lines 27 and 44 contain incomplete sentences missing subjects. Additionally, clarify whether any of these medium-priority items are blocking the current PR or are intended as post-release improvements.
🔎 Proposed style fixes
🧰 Tools
🪛 LanguageTool
[style] ~27-~27: To form a complete sentence, be sure to include a subject.
Context: ...s), the Create button is still enabled. Should be disabled with a helpful message. ##...
(MISSING_IT_THERE)
🤖 Prompt for AI Agents