Austinamoruso/mergeprs #43
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @austinamorusoyardstick, 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 delivers a significant new feature that streamlines the workflow for consolidating multiple pull requests. It provides users with an intuitive interface to select PRs, automates the complex git operations of fetching, cherry-picking, and committing changes into a new branch, and then facilitates the creation of a new, consolidated pull request. The changes span UI components, core application logic, git integration, and user documentation, aiming to enhance productivity for managing related PRs.
Highlights
- New Feature: Merge Multiple PRs: Introduced a comprehensive feature allowing users to select and merge multiple pull requests into a single consolidated PR. This includes UI for selection, backend logic for merging, and updated documentation.
- User Interface Enhancements: Added a new
prSelectorOverlayfor interactive PR selection, integrated a new 'M' keybinding, and updated the main menu to expose the 'Merge PRs' option. - Robust Backend Merge Logic: Implemented core merge functionality in
app/merge_prs.gothat creates a temporary worktree, cherry-picks commits from selected PRs, handles conflicts by skipping, creates a consolidated commit, pushes a new branch, and automatically creates a new PR via GitHub CLI. - Git Worktree and GitHub CLI Integration: Extended
session/gitwith new methods for listing open PRs (ListOpenPRs), cherry-picking branches (CherryPickBranch), committing merged changes (CommitMergedChanges), pushing branches (PushBranch), and creating pull requests (CreatePullRequest) using the GitHub CLI. - Dedicated Documentation: A new
README_MERGE_PRS.mdfile has been added, providing detailed instructions on how to use the new feature, including key bindings, error handling, and implementation specifics.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for merging multiple pull requests, covering UI, backend logic, and documentation. Key areas for improvement include correctness (avoiding fmt.Printf in TUI, correct git worktree creation), maintainability (simplifying logic, removing dead code), and robustness (improving error handling, robust gh CLI interaction).
app/merge_prs.go
Outdated
| repoPath := worktree.GetRepoPath() | ||
|
|
||
| // Create a new worktree for the merge | ||
| mergeWorktree, _, err := git.NewGitWorktree(repoPath, newBranchName) |
There was a problem hiding this comment.
The function git.NewGitWorktree is designed to take a sessionName and create a branch by adding a prefix (e.g., username/). Passing newBranchName, which is a fully formed branch name, as the sessionName will result in an incorrect final branch name.
Use a function that allows creating a worktree with a specific branch name directly, or modify NewGitWorktree to handle this case.
app/merge_prs.go
Outdated
| fmt.Printf("Successfully merged %d PRs into branch %s\n", len(successfulMerges), newBranchName) | ||
| if newPRNumber > 0 { | ||
| fmt.Printf("Created PR #%d\n", newPRNumber) | ||
| } | ||
| if len(failedMerges) > 0 { | ||
| fmt.Printf("Failed to merge %d PRs:\n", len(failedMerges)) | ||
| for _, failure := range failedMerges { | ||
| fmt.Printf(" - %s\n", failure) | ||
| } | ||
| } | ||
|
|
||
| // Clean up the merge worktree | ||
| if err := mergeWorktree.Cleanup(); err != nil { | ||
| // Log but don't fail | ||
| fmt.Printf("Warning: failed to cleanup merge worktree: %v\n", err) | ||
| } |
There was a problem hiding this comment.
Using fmt.Printf in a TUI application writes directly to standard output, which will corrupt the user interface. All user-facing messages and logs should be handled through the application's UI components (like an overlay or status bar) or the dedicated logger.
session/git/worktree_merge.go
Outdated
| _, err := g.runGitCommand(g.worktreePath, "cherry-pick", sha) | ||
| if err != nil { | ||
| // Check if it's a conflict | ||
| status, _ := g.runGitCommand(g.worktreePath, "status", "--porcelain") |
There was a problem hiding this comment.
The error returned from g.runGitCommand is being ignored here. If the git status command fails, you won't be able to reliably detect a merge conflict, which could lead to incorrect behavior or silent failures. The error should be checked and handled.
| status, _ := g.runGitCommand(g.worktreePath, "status", "--porcelain") | |
| status, err := g.runGitCommand(g.worktreePath, "status", "--porcelain") | |
| if err != nil { | |
| // Even if status fails, we should probably abort and return the original error. | |
| g.runGitCommand(g.worktreePath, "cherry-pick", "--abort") | |
| return fmt.Errorf("failed to cherry-pick commit %s: %w", sha, err) | |
| } |
app/merge_prs.go
Outdated
| progressText += fmt.Sprintf("%d. PR #%d: %s\n", i+1, pr.Number, pr.Title) | ||
| } | ||
| m.textOverlay = overlay.NewTextOverlay(progressText) | ||
| m.state = stateHelp |
There was a problem hiding this comment.
Reusing stateHelp to display a progress overlay is a misuse of the state and can be confusing for future maintenance. It would be clearer to introduce a dedicated state for this, such as stateProgress.
app/merge_prs.go
Outdated
| } | ||
|
|
||
| // Note: Not used but may be needed for future enhancements | ||
| _ = worktree.GetWorktreePath() |
There was a problem hiding this comment.
This line of code is unused and has no effect. Remove dead code to keep the codebase clean.
app/merge_prs.go
Outdated
| // Cherry-pick the PR's commits | ||
| if err := mergeWorktree.CherryPickBranch(pr.HeadRef); err != nil { | ||
| // Check if it's a merge conflict | ||
| if strings.Contains(err.Error(), "conflict") { |
There was a problem hiding this comment.
Relying on string matching (strings.Contains) to detect a merge conflict from an error message is fragile. The error message from the underlying git command could change in the future, which would break this logic.
CherryPickBranch should return a specific error type (e.g., var ErrMergeConflict = errors.New("merge conflict")) and then check for it here using errors.Is.
| for _, pr := range selectedPRs { | ||
| prNum := pr.Number | ||
| // Check if this PR was successfully merged | ||
| wasSuccessful := false | ||
| for _, num := range successfulMerges { | ||
| if num == prNum { | ||
| wasSuccessful = true | ||
| break | ||
| } | ||
| } | ||
| if wasSuccessful { | ||
| commitMessage += fmt.Sprintf("- PR #%d: %s\n", pr.Number, pr.Title) | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop to build the commit message has a nested loop, resulting in O(N*M) complexity, where N is the number of selected PRs and M is the number of successful merges. This can be optimized to O(N+M) by first converting successfulMerges into a map or set for O(1) lookups.
Here's a more efficient implementation:
successfulSet := make(map[int]struct{}, len(successfulMerges))
for _, num := range successfulMerges {
successfulSet[num] = struct{}{}
}
for _, pr := range selectedPRs {
if _, ok := successfulSet[pr.Number]; ok {
commitMessage += fmt.Sprintf("- PR #%d: %s\n", pr.Number, pr.Title)
}
}| if len(successfulMerges) == 0 { | ||
| return fmt.Errorf("failed to merge any PRs") | ||
| } |
There was a problem hiding this comment.
This check for len(successfulMerges) == 0 is redundant because the same check is performed on line 107, which causes an early return from the function. Remove this duplicate code.
| // Parse the PR URL to get the number | ||
| prURL := strings.TrimSpace(string(output)) | ||
| parts := strings.Split(prURL, "/") | ||
| if len(parts) > 0 { | ||
| prNumber := parts[len(parts)-1] | ||
| // Convert to int | ||
| var pr struct { | ||
| Number int `json:"number"` | ||
| } | ||
| // Use gh to get the PR number | ||
| cmd := exec.Command("gh", "pr", "view", prNumber, "--json", "number") | ||
| cmd.Dir = g.worktreePath | ||
| output, err := cmd.Output() | ||
| if err == nil { | ||
| if err := json.Unmarshal(output, &pr); err == nil { | ||
| return pr.Number, nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Parsing the PR number from the output URL is fragile and might break if GitHub changes its URL structure or if the gh CLI changes its output format.
A more robust approach would be to use the --json flag with gh pr create to get the PR number in a structured format. For example:
gh pr create --title "..." --body "..." --json number
This will return a JSON object like {"number": 123}, which you can easily unmarshal to get the PR number without relying on string parsing.
| selectedCount := len(o.selectedPRs) | ||
| for _, selected := range o.selectedPRs { | ||
| if !selected { | ||
| selectedCount-- | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop to count selected items is more complex than necessary because the selection logic on line 98 keeps deselected items in the map (e.g., selectedPRs[123] = false).
A cleaner implementation would be:
- Modify the selection logic on line 98 to
deleteitems from the map when they are deselected.// in Update() -> case msg.String() == " " if o.selectedPRs[pr.Number] { delete(o.selectedPRs, pr.Number) } else { o.selectedPRs[pr.Number] = true }
- Then, this counting logic can be simplified to just
selectedCount := len(o.selectedPRs).
This would make the code more efficient and easier to understand.
…change adds a new state constant `statePRSelector` to the application state enum. Looking at the recent commits, this is part of a feature for merging multiple PRs, and this specific change adds the state that will be used when selecting PRs to merge. Add statePRSelector state for PR selection UI
austinamorusoyardstick
force-pushed
the
austinamoruso/mergeprs
branch
from
7330c39 to
15a6714
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature for merging multiple pull requests. The implementation is generally well-structured, following good practices for TUI applications with bubbletea. The use of worktrees for isolation and gh CLI for GitHub operations is appropriate.
I've identified a few areas for improvement, mainly concerning code clarity, maintainability, and a potential bug in error handling. My comments focus on simplifying complex logic, removing redundancy, and ensuring robust error handling.
Overall, this is a solid contribution. Addressing the feedback will enhance the code's quality and long-term maintainability.
| output, err := cmd.Output() | ||
| if err == nil { | ||
| if err := json.Unmarshal(output, &pr); err == nil { | ||
| return pr.Number, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback: just return 0 if we can't parse the number | ||
| return 0, nil |
There was a problem hiding this comment.
The error handling when parsing the PR number is not robust. If gh pr view or json.Unmarshal fails, the function currently falls through and returns 0, nil. This masks a real error and could lead to silent failures or incorrect behavior in the calling code (e.g., assuming PR creation failed when it actually succeeded but number retrieval failed).
The function should return an error in these failure cases to ensure the caller is aware of the problem.
output, err := cmd.Output()
if err != nil {
return 0, fmt.Errorf("failed to view created PR to get number: %w", err)
}
if err := json.Unmarshal(output, &pr); err != nil {
return 0, fmt.Errorf("failed to parse PR number from view: %w", err)
}
return pr.Number, nil
}
// Fallback: return an error if we can't parse the number
return 0, fmt.Errorf("could not determine PR number from URL: %s", prURL)| m.prSelectorOverlay = overlay.NewPRSelectorOverlay(worktreePath, func(selectedPRs []*git.PullRequest) { | ||
| // Handle PR selection | ||
| m.state = stateDefault | ||
| m.prSelectorOverlay = nil | ||
| // Start the merge process will be handled via a message | ||
| if len(selectedPRs) > 0 { | ||
| // Send a message to trigger the merge | ||
| m.pendingMergePRs = selectedPRs | ||
| m.pendingMergeInstance = selectedInstance | ||
| } |
There was a problem hiding this comment.
The onComplete callback for NewPRSelectorOverlay contains redundant state management logic. It sets m.state to stateDefault and m.prSelectorOverlay to nil, which is also handled in the main Update loop when the overlay is closed (returns nil).
To avoid duplication and keep state management centralized, the callback should only be responsible for populating m.pendingMergePRs and m.pendingMergeInstance.
m.prSelectorOverlay = overlay.NewPRSelectorOverlay(worktreePath, func(selectedPRs []*git.PullRequest) {
// Handle PR selection
if len(selectedPRs) > 0 {
// Set pending PRs and instance for the main Update loop to handle.
m.pendingMergePRs = selectedPRs
m.pendingMergeInstance = selectedInstance
}
})| func (m *home) handlePRSelectorState(msg tea.KeyMsg) (tea.Model, tea.Cmd) { | ||
| if m.prSelectorOverlay == nil { | ||
| m.state = stateDefault | ||
| return m, nil | ||
| } | ||
|
|
||
| // Update the overlay with the key message | ||
| updatedOverlay, cmd := m.prSelectorOverlay.Update(msg) | ||
|
|
||
| // Check if overlay wants to close (returns nil) | ||
| if updatedOverlay == nil { | ||
| m.state = stateDefault | ||
| m.prSelectorOverlay = nil | ||
|
|
||
| // Check if we have pending PRs to merge | ||
| if len(m.pendingMergePRs) > 0 && m.pendingMergeInstance != nil { | ||
| // Start the merge process | ||
| mergeCmd := m.mergePRs(m.pendingMergeInstance, m.pendingMergePRs) | ||
| // Clear the pending fields | ||
| m.pendingMergePRs = nil | ||
| m.pendingMergeInstance = nil | ||
| return m, tea.Batch(cmd, mergeCmd) | ||
| } | ||
|
|
||
| return m, cmd | ||
| } | ||
|
|
||
| // Update the overlay pointer | ||
| if overlayPtr, ok := updatedOverlay.(*overlay.PRSelectorOverlay); ok { | ||
| m.prSelectorOverlay = overlayPtr | ||
| } | ||
|
|
||
| return m, cmd | ||
| } |
There was a problem hiding this comment.
The function handlePRSelectorState appears to be dead code. Its call is commented out in handleKeyPress, and its logic is almost entirely duplicated at the top of the main Update function (lines 283-312).
The implementation in Update is preferable as it handles all tea.Msg types for the overlay, not just tea.KeyMsg. This duplicated, unused function should be removed to improve code clarity and maintainability.
| var successfulMerges []int | ||
| var failedMerges []string |
There was a problem hiding this comment.
The logic for building the commit message later on (lines 109-123) is more complex than it needs to be because successfulMerges only stores PR numbers ([]int).
By changing successfulMerges to store *git.PullRequest objects, you can simplify the logic for generating the commit message and make the code more readable.
| var successfulMerges []int | |
| var failedMerges []string | |
| var successfulPRs []*git.PullRequest | |
| var failedMerges []string |
| continue | ||
| } | ||
|
|
||
| successfulMerges = append(successfulMerges, pr.Number) |
There was a problem hiding this comment.
To align with the change of successfulMerges to successfulPRs, you should append the pr object itself, not just its number.
| successfulMerges = append(successfulMerges, pr.Number) | |
| successfulPRs = append(successfulPRs, pr) |
| commitMessage := fmt.Sprintf("Merge %d PRs\n\n", len(successfulMerges)) | ||
| for _, pr := range selectedPRs { | ||
| prNum := pr.Number | ||
| // Check if this PR was successfully merged | ||
| wasSuccessful := false | ||
| for _, num := range successfulMerges { | ||
| if num == prNum { | ||
| wasSuccessful = true | ||
| break | ||
| } | ||
| } | ||
| if wasSuccessful { | ||
| commitMessage += fmt.Sprintf("- PR #%d: %s\n", pr.Number, pr.Title) | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop for constructing the commit message is complex due to the nested loop structure. If you change successfulMerges to be of type []*git.PullRequest as suggested earlier, this can be greatly simplified.
commitMessage := fmt.Sprintf("Merge %d PRs\n\n", len(successfulPRs))
for _, pr := range successfulPRs {
commitMessage += fmt.Sprintf("- PR #%d: %s\n", pr.Number, pr.Title)
}| prBody := fmt.Sprintf("This PR merges the following PRs:\n\n") | ||
| for _, pr := range selectedPRs { | ||
| prBody += fmt.Sprintf("- #%d: %s\n", pr.Number, pr.Title) | ||
| } |
There was a problem hiding this comment.
The PR body is constructed using all selectedPRs, which may include PRs that failed to merge. This is inconsistent with the commit message, which only lists successfully merged PRs, and the feature description in the README.
For clarity and consistency, the PR body should also list only the successfully merged PRs.
prBody := fmt.Sprintf("This PR merges the following PRs:\n\n")
for _, pr := range successfulPRs { // Use successfulPRs here
prBody += fmt.Sprintf("- #%d: %s\n", pr.Number, pr.Title)
}| if len(successfulMerges) == 0 { | ||
| return fmt.Errorf("failed to merge any PRs") | ||
| } |
There was a problem hiding this comment.
This check for len(successfulMerges) == 0 is redundant, as the same check is already performed on line 104. This duplicate check can be removed to simplify the code.
| var prs []struct { | ||
| Number int `json:"number"` | ||
| Title string `json:"title"` | ||
| State string `json:"state"` | ||
| HeadRefName string `json:"headRefName"` | ||
| BaseRefName string `json:"baseRefName"` | ||
| URL string `json:"url"` | ||
| HeadRefOid string `json:"headRefOid"` | ||
| } |
There was a problem hiding this comment.
Using an anonymous struct for JSON unmarshalling can make the code harder to read and maintain. It's better practice to define a named struct for clarity, especially if the structure might be reused or needs to be understood by other developers.
| var prs []struct { | |
| Number int `json:"number"` | |
| Title string `json:"title"` | |
| State string `json:"state"` | |
| HeadRefName string `json:"headRefName"` | |
| BaseRefName string `json:"baseRefName"` | |
| URL string `json:"url"` | |
| HeadRefOid string `json:"headRefOid"` | |
| } | |
| type ghPRInfo struct { | |
| Number int `json:"number"` | |
| Title string `json:"title"` | |
| State string `json:"state"` | |
| HeadRefName string `json:"headRefName"` | |
| BaseRefName string `json:"baseRefName"` | |
| URL string `json:"url"` | |
| HeadRefOid string `json:"headRefOid"` | |
| } | |
| var prs []ghPRInfo |
| selectedCount := len(o.selectedPRs) | ||
| for _, selected := range o.selectedPRs { | ||
| if !selected { | ||
| selectedCount-- | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to calculate selectedCount is a bit convoluted. While it appears to be functionally correct, its intent is not immediately clear. A more straightforward approach would be to iterate through the map and count the true values. This would improve code clarity and maintainability.
| selectedCount := len(o.selectedPRs) | |
| for _, selected := range o.selectedPRs { | |
| if !selected { | |
| selectedCount-- | |
| } | |
| } | |
| selectedCount := 0 | |
| for _, isSelected := range o.selectedPRs { | |
| if isSelected { | |
| selectedCount++ | |
| } | |
| } |
1 participant
This pull request introduces a new feature that allows users to merge multiple pull requests (PRs) into a single branch and create a consolidated PR. It includes updates to the user interface, backend logic, and documentation to support this functionality. Key changes include the addition of a PR selection overlay, the implementation of the merge logic, and updates to keybindings and menu options.
New Feature: Merge Multiple PRs
README_MERGE_PRS.mdexplaining how to use the new feature, including key bindings, error handling, and implementation details.prSelectorOverlayinapp/app.goto allow users to select PRs for merging. This includes updates to thehomestruct, state management, and key handling. [1] [2] [3] [4] [5] [6] [7] [8]M) for triggering the merge PRs feature inkeys/keys.go. [1] [2] [3]Backend Logic for PR Merging
mergePRsfunction and related commands inapp/merge_prs.go, which handle fetching, cherry-picking, and committing changes from selected PRs into a new branch.session/git/worktree_merge.gofor cherry-picking commits, committing merged changes, pushing branches, and creating PRs via the GitHub CLI.session/git/pr_comments.goto list all open PRs in the repository using the GitHub CLI.These changes collectively enable the new "Merge Multiple PRs" feature, providing users with a streamlined workflow for consolidating PRs into a single branch and creating a new PR.