-
Notifications
You must be signed in to change notification settings - Fork 32
🤖 Add trunk branch selector for workspace creation #100
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
Conversation
db7f61b to
cfdbc06
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
ThomasK33
left a comment
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.
@chatgpt-codex-connector Addressed the notarization config issue - restored the flag that was accidentally removed during rebase.
|
To use Codex here, create an environment for this repo. |
|
✅ Addressed Codex feedback on notarization config Fixed the P1 issue - restored |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. |
| ? (activePrompt.fields[activePrompt.idx] ?? null) | ||
| : null; | ||
|
|
||
| useEffect(() => { |
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.
this code is kind of monstrous -- needs comment or simplification
src/components/NewWorkspaceModal.tsx
Outdated
| const loadBranches = async () => { | ||
| setIsLoadingBranches(true); | ||
| try { | ||
| const branchList = await window.api.workspace.listBranches(projectPath); |
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.
this is the wrong scoping for listBranches -- its an operation on the project. The IPC doesn't have a projects section. Working on reworking the IPC so the frontend doesn't directly access config and there is instead safe operations via a projects section.
| trunkBranch?: string; | ||
| } | ||
|
|
||
| export async function listLocalBranches(projectPath: string): Promise<string[]> { |
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.
should have a test
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.
alternatively and better would be to add a test for createWorktree
061f6ff to
6d6623e
Compare
6d6623e to
7afd242
Compare
ammario
left a comment
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.
good shit
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.
(essentially ignoring this file for the purpose of review)
src/components/NewWorkspaceModal.tsx
Outdated
| projectPath: string; | ||
| onClose: () => void; | ||
| onAdd: (branchName: string) => Promise<void>; | ||
| onAdd: (branchName: string, trunkBranch?: string) => Promise<void>; |
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.
why not make the trunkBranch required throughout? That should simplify the code paths.
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.
We can still infer a reasonable default from what's checked out in the project root — so i mean this from a code perspective not a UX perspective
src/components/NewWorkspaceModal.tsx
Outdated
| {isLoadingBranches ? ( | ||
| <option value="">Loading branches...</option> | ||
| ) : ( | ||
| <option value="">-- Select trunk branch (optional) --</option> |
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.
would rather we just default to the branch we found and will use vs. making this optional -- such behavior would have saved me a bunch of time as i've accidenally created workspaces off a detached head state vs. main
src/services/ipcMain.ts
Outdated
| ipcMain.handle( | ||
| IPC_CHANNELS.WORKSPACE_CREATE, | ||
| async (_event, projectPath: string, branchName: string) => { | ||
| async (_event, projectPath: string, branchName: string, trunkBranch?: string) => { |
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.
again re optional
src/App.tsx
Outdated
| try { | ||
| return await window.api.projects.listBranches(projectPath); | ||
| } catch (error) { | ||
| console.error("Failed to list branches:", error); |
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.
better for the error to propagate to the caller imo so that it can be displayed in a useful place vs the console
| for (let i = 1; i < branches.length; i += 1) { | ||
| expect(branches[i - 1].localeCompare(branches[i])).toBeLessThanOrEqual(0); | ||
| } | ||
| }); |
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.
appreciate the tests
src/git.ts
Outdated
| b.trim() === `remotes/origin/${branchName}` | ||
| ); | ||
| // If trunk branch is specified, always create a new branch from it | ||
| if (trunkBranch) { |
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.
again, this code would be simplified if it wasn't optional
7afd242 to
06fd0c5
Compare
06fd0c5 to
a919035
Compare
a919035 to
d44f319
Compare
Adds ability to select a trunk/base branch when creating a new workspace. When a trunk branch is selected, the new workspace branch is created from that trunk instead of from HEAD or an existing remote branch. UI Changes: - Command palette: Added trunk branch selection field with fuzzy search - Modal (CMD+N): Added dropdown to select trunk branch - Both UIs load local branches and show them as options - Trunk branch selection is optional (defaults to HEAD if not selected) Backend Changes: - Added listLocalBranches() to git.ts to enumerate local branches - Updated createWorktree() to accept optional trunkBranch parameter - New logic: if trunk is specified, always create new branch from trunk - Ignores remote branches when trunk is explicitly selected - Errors if local branch already exists (prevents overwrite) - Without trunk: preserves existing behavior (checkout remote or create from HEAD) IPC Layer: - Added WORKSPACE_LIST_BRANCHES channel to fetch branches for a project - Updated workspace.create to accept and pass through trunkBranch - Wired through preload.ts, ipcMain.ts, and type definitions Integration: - Updated App.tsx to pass trunk branch from modal to backend - Updated command palette prompts to collect trunk branch selection - Added getBranchesForProject callback to command registry context - Command palette supports async option loading with fuzzy filtering This enables creating feature branches from any local branch, useful for: - Creating branches from main instead of current HEAD - Starting new work from a specific release branch - Isolating work from unrelated changes on current branch _Generated with `cmux`_ Change-Id: Ic2a710104f3a5925e0e6bdff1d0e5ed772bcf108 Signed-off-by: Thomas Kosiewski <tk@coder.com> 🤖 Restore macOS notarization configuration The notarization flag was accidentally removed during rebase. This flag is required to submit the DMG for notarization so that distributed builds are not blocked by Gatekeeper on macOS 10.15+. Restores the 'notarize: true' flag in the mac build configuration. _Generated with `cmux`_ Change-Id: I3d4e4be981cdbb0cee0c822dd973b1a71e000108 Signed-off-by: Thomas Kosiewski <tk@coder.com>
d44f319 to
af99432
Compare
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.
there's minor ui stuff like the caret on the select is misaligned - but this an essential feature that i need desperately so lets get it in
Adds ability to select a trunk/base branch when creating a new workspace.
Changes
UI
Backend
listLocalBranches()to enumerate local branchescreateWorktree()to accept optionaltrunkBranchparameterIPC Layer
WORKSPACE_LIST_BRANCHESchannel to fetch branches for a projectworkspace.createto accept and pass throughtrunkBranchIntegration
Use Cases
This enables creating feature branches from any local branch:
maininstead of current HEADTesting
Generated with
cmux