Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Add an API to merge a given review by it’s ID.
estib-vega
force-pushed
the
merge-review
branch
from
3b29951 to
70c91dc
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds a merge API for reviews (pull requests/merge requests) to the GitButler forge integration. The implementation follows the established patterns for forge operations and adds support for both GitHub and GitLab.
Changes:
- Added
merge_reviewfunction to the forge API that merges a review by its ID - Implemented GitHub PR merge via the GitHub API (PUT endpoint with merge method options)
- Implemented GitLab MR merge via the GitLab API (PUT endpoint with squash option)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but-api/src/legacy/forge.rs | Adds the merge_review API function that accepts a review ID and delegates to the forge-specific implementation |
| crates/but-forge/src/lib.rs | Exports the new merge_review function from the review module |
| crates/but-forge/src/review.rs | Implements the core merge_review logic that routes to GitHub or GitLab based on forge type |
| crates/but-github/src/client.rs | Adds merge_pull_request method and supporting types (MergeMethod enum, MergePullRequestParams) |
| crates/but-github/src/lib.rs | Exports the new merge-related types for public API use |
| crates/but-github/src/pr.rs | Adds merge wrapper function that creates the client and invokes the merge operation |
| crates/but-gitlab/src/client.rs | Adds merge_merge_request method and MergeMergeRequestParams struct |
| crates/but-gitlab/src/lib.rs | Exports the new MergeMergeRequestParams type |
| crates/but-gitlab/src/mr.rs | Adds merge wrapper function with a typo in the error message |
| crates/but-server/src/lib.rs | Registers the merge_review command handler in the server command dispatcher |
| crates/gitbutler-tauri/src/main.rs | Registers the Tauri command for merge_review |
| GitLabClient::from_storage(storage, preferred_account)? | ||
| .merge_merge_request(¶ms) | ||
| .await | ||
| .context("Faile to merge MR") |
There was a problem hiding this comment.
Spelling error in error message: "Faile" should be "Failed".
| .context("Faile to merge MR") | |
| .context("Failed to merge MR") |
| } | ||
| } | ||
|
|
||
| /// Merge a review to it's target branch |
There was a problem hiding this comment.
Grammar error in comment: "it's" should be "its" (possessive form, not contraction).
| /// Merge a review to it's target branch | |
| /// Merge a review to its target branch |
| let response = self.client.put(&url).json(&body).send().await?; | ||
|
|
||
| if !response.status().is_success() { | ||
| bail!("Failed to merge merge request: {}", response.status()); |
There was a problem hiding this comment.
Consider including the response body in the error message for better debugging. Similar to the pattern used in create_merge_request at line 166 of this file, the error could be:
let status = response.status();
let error_text = response.text().await.unwrap_or_default();
bail!("Failed to merge merge request: {status} - {error_text}")This provides more context about why the merge failed (e.g., conflicts, permissions, CI checks).
| bail!("Failed to merge merge request: {}", response.status()); | |
| let status = response.status(); | |
| let error_text = response.text().await.unwrap_or_default(); | |
| bail!("Failed to merge merge request: {status} - {error_text}"); |
| let response = self.client.put(&url).json(&body).send().await?; | ||
|
|
||
| if !response.status().is_success() { | ||
| bail!("Failed to merge pull request: {}", response.status()); |
There was a problem hiding this comment.
Consider including the response body in the error message for better debugging. Similar to the pattern used in GitLab's create_merge_request (crates/but-gitlab/src/client.rs:166), the error could be:
let status = response.status();
let error_text = response.text().await.unwrap_or_default();
bail!("Failed to merge pull request: {status} - {error_text}")This provides more context about why the merge failed (e.g., conflicts, permissions, required checks).
| bail!("Failed to merge pull request: {}", response.status()); | |
| let status = response.status(); | |
| let error_text = response.text().await.unwrap_or_default(); | |
| bail!("Failed to merge pull request: {status} - {error_text}"); |
| let preferred_account = preferred_forge_user.as_ref().and_then(|user| user.github()); | ||
| let pr_number = review_number | ||
| .try_into() | ||
| .context("PR: Failed to cast usize to i64, somehow")?; |
There was a problem hiding this comment.
The error message should be more user-friendly and consistent with the pattern used in other functions. Change "PR: Failed to cast usize to i64, somehow" to "PR number is too large" to match the error message in but_github::pr::get (line 54 of crates/but-github/src/pr.rs).
| .context("PR: Failed to cast usize to i64, somehow")?; | |
| .context("PR number is too large")?; |
| let project_id = GitLabProjectId::new(owner, repo); | ||
| let mr_iid = review_number | ||
| .try_into() | ||
| .context("MR: Failed to cast usize to i64, somehow")?; |
There was a problem hiding this comment.
The error message should be more user-friendly and consistent with the pattern used in other functions. Change "MR: Failed to cast usize to i64, somehow" to "MR number is too large" to match the error message in but_gitlab::mr::get (line 52 of crates/but-gitlab/src/mr.rs).
| .context("MR: Failed to cast usize to i64, somehow")?; | |
| .context("MR number is too large")?; |
1 participant
Add an API to merge a given review by it’s ID.