feat: Add Gitea pull request and comment endpoints#68
feat: Add Gitea pull request and comment endpoints#68jaysomani wants to merge 3 commits intoutopia-php:mainfrom
Conversation
- Implement getPullRequest to fetch PR details - Implement getPullRequestFromBranch to find PR by branch name - Implement createPullRequest to create new PRs - Implement createComment, getComment, updateComment for PR comments - Add comprehensive tests with full workflow coverage - Add edge case tests for invalid inputs - Follow PR utopia-php#64 null safety pattern Tests: 7 new tests, all passing
WalkthroughThe Gitea VCS adapter was extended: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3277ea7614
ℹ️ 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
🧹 Nitpick comments (1)
tests/VCS/Adapter/GiteaTest.php (1)
106-116: These PR tests never add a commit to the head branch.Lines 106-108 and the same pattern in the other PR tests create a feature branch, but the subsequent
createFile()calls still write to the default branch.src/VCS/Adapter/Git/Gitea.php'screateFile()payload has no branch/ref field, so these cases are not actually exercising a branch with head-only changes and can get flaky if Gitea rejects no-op PRs.I’d either extend
createFile()to accept a branch/ref for tests, or create the feature-branch commit through the contents API directly before opening the PR.Also applies to: 152-163, 448-463, 665-676
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/VCS/Adapter/GiteaTest.php` around lines 106 - 116, The tests create a branch with createBranch() but then call createFile() which currently writes to the default branch (Gitea::createFile lacks a branch/ref), so the PR head has no commits; update the tests by either (A) extending createFile() to accept an optional branch/ref parameter and using that when creating test files on 'comment-test' (and adjust the Gitea adapter implementation to include the ref in the payload), or (B) create the branch commit directly via the Gitea contents API before calling createPullRequest(); apply the chosen approach consistently for the other failing tests that use createBranch() + createFile() (the blocks around lines referenced).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/VCS/Adapter/Git/Gitea.php`:
- Around line 449-464: The current call builds $url with
"?state=open&sort=recentupdate&limit=1" and then filters $responseBody for the
branch, which can miss PRs; remove the "limit=1" (or replace with a server-side
head filter if Gitea supports it) so the API returns all matching open PRs, then
keep the existing loop over $responseBody ($response, $responseBody, $prHead,
$prHeadRef) to find the PR whose head ref equals $branch and return it; ensure
you still handle an empty result by returning [].
---
Nitpick comments:
In `@tests/VCS/Adapter/GiteaTest.php`:
- Around line 106-116: The tests create a branch with createBranch() but then
call createFile() which currently writes to the default branch
(Gitea::createFile lacks a branch/ref), so the PR head has no commits; update
the tests by either (A) extending createFile() to accept an optional branch/ref
parameter and using that when creating test files on 'comment-test' (and adjust
the Gitea adapter implementation to include the ref in the payload), or (B)
create the branch commit directly via the Gitea contents API before calling
createPullRequest(); apply the chosen approach consistently for the other
failing tests that use createBranch() + createFile() (the blocks around lines
referenced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff55d472-6fd1-4ebf-86db-9d78ff264369
📒 Files selected for processing (2)
src/VCS/Adapter/Git/Gitea.phptests/VCS/Adapter/GiteaTest.php
- Remove limit=1 from getPullRequestFromBranch to fetch all PRs - Add branch parameter to createFile for branch-specific commits - Update tests to commit files to feature branches
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/VCS/Adapter/GiteaTest.php (2)
101-145: Always delete the test repo infinally.This test creates remote state and only cleans it up on the happy path. Any failed assertion or API error before Line 144 leaks repositories into the shared Gitea instance and can make later runs flaky.
♻️ Suggested cleanup pattern
public function testCommentWorkflow(): void { $repositoryName = 'test-comment-workflow-' . \uniqid(); - $this->vcsAdapter->createRepository(self::$owner, $repositoryName, false); - - $this->vcsAdapter->createFile(self::$owner, $repositoryName, 'README.md', '# Test'); - $this->vcsAdapter->createBranch(self::$owner, $repositoryName, 'comment-test', 'main'); - $this->vcsAdapter->createFile(self::$owner, $repositoryName, 'test.txt', 'test', 'Add test file', 'comment-test'); - - $pr = $this->vcsAdapter->createPullRequest( - self::$owner, - $repositoryName, - 'Comment Test PR', - 'comment-test', - 'main' - ); + $this->vcsAdapter->createRepository(self::$owner, $repositoryName, false); + + try { + $this->vcsAdapter->createFile(self::$owner, $repositoryName, 'README.md', '# Test'); + $this->vcsAdapter->createBranch(self::$owner, $repositoryName, 'comment-test', 'main'); + $this->vcsAdapter->createFile(self::$owner, $repositoryName, 'test.txt', 'test', 'Add test file', 'comment-test'); + + $pr = $this->vcsAdapter->createPullRequest( + self::$owner, + $repositoryName, + 'Comment Test PR', + 'comment-test', + 'main' + ); - $prNumber = $pr['number'] ?? 0; - $this->assertGreaterThan(0, $prNumber); + $prNumber = $pr['number'] ?? 0; + $this->assertGreaterThan(0, $prNumber); - $originalComment = 'This is a test comment'; - $commentId = $this->vcsAdapter->createComment(self::$owner, $repositoryName, $prNumber, $originalComment); + $originalComment = 'This is a test comment'; + $commentId = $this->vcsAdapter->createComment(self::$owner, $repositoryName, $prNumber, $originalComment); - $this->assertNotEmpty($commentId); - $this->assertIsString($commentId); + $this->assertNotEmpty($commentId); + $this->assertIsString($commentId); - $retrievedComment = $this->vcsAdapter->getComment(self::$owner, $repositoryName, $commentId); - $this->assertSame($originalComment, $retrievedComment); + $retrievedComment = $this->vcsAdapter->getComment(self::$owner, $repositoryName, $commentId); + $this->assertSame($originalComment, $retrievedComment); + } finally { + $this->vcsAdapter->deleteRepository(self::$owner, $repositoryName); + } - - $this->vcsAdapter->deleteRepository(self::$owner, $repositoryName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/VCS/Adapter/GiteaTest.php` around lines 101 - 145, The testCommentWorkflow method currently only deletes the created repository on the happy path; wrap the main test logic in a try/finally so the cleanup always runs: declare repositoryName as you already do, then enclose creation/assertions (calls to createRepository, createFile, createBranch, createPullRequest, createComment, getComment, updateComment, etc.) in a try block and call $this->vcsAdapter->deleteRepository(self::$owner, $repositoryName) inside a finally block to ensure the repo is removed even on failures.
193-205: Make the invalid-case tests assert one concrete contract.These tests currently pass on any string/array, including raw Gitea error payloads, so they do not lock in the adapter’s not-found behavior. Assert a specific outcome (
''/[]orexpectException()), not just the type.Also applies to: 481-493
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/VCS/Adapter/GiteaTest.php` around lines 193 - 205, The test testGetCommentInvalidId currently only asserts the return is a string allowing raw API errors; update it (and the similar tests around the other block) to assert a single concrete contract for the "not found" case: either assertSame('', $result) if adapter should return an empty string, or change the test to expectException(...) if the adapter should throw; locate and modify the call to getComment in testGetCommentInvalidId (and the analogous tests) and replace the loose assertIsString/assertIsArray with a specific assertion matching the adapter's intended behavior, keeping createRepository/createFile/deleteRepository setup/teardown intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/VCS/Adapter/Git/Gitea.php`:
- Around line 191-203: The Gitea adapter added an optional $branch parameter to
Gitea::createFile(string $owner, string $repositoryName, string $filepath,
string $content, string $message = 'Add file', string $branch = '') but the
abstract Git::createFile() contract still defines only five parameters; make the
branch behavior part of the shared contract by updating the abstract method
signature in Git::createFile to accept the optional sixth parameter (string
$branch = '') and then update all concrete implementations and any call sites to
accept/forward this optional $branch argument (or alternatively add a new
branch-aware method on Git if you prefer to keep the old signature, ensuring
Git::createFile remains unchanged and adding e.g. createFileOnBranch with the
same parameters used in Gitea::createFile).
- Around line 456-471: Replace the paginated list fetch with a direct PR lookup
using Gitea's dedicated endpoint: construct the URL
"/repos/{$owner}/{$repositoryName}/pulls/{$targetBranch}/{$branch}" (using the
current target/base branch variable and the source $branch), call
$this->call(self::METHOD_GET, $url, ['Authorization' => "token
$this->accessToken"]), then parse the returned response body and return the PR
if present (or [] on not-found/error). Update the logic that currently iterates
$responseBody/$prHeadRef to instead use this single-call flow so pagination is
avoided.
- Around line 382-391: The createPullRequest() and getPullRequest() methods
currently return $response['body'] without checking HTTP status, so non-2xx
responses (e.g. 422/404/401) propagate error payloads; update both methods to
mirror the existing pattern used in createBranch()/createFile(): read $status =
$response['status'], if $status is not in the expected 2xx range then
throw/return an error (include the response body/message) or call the same error
handling routine used elsewhere, otherwise return $response['body']; reference
the createPullRequest(), getPullRequest(), $response['status'] and
$response['body'] symbols and reuse the same guard logic already implemented for
createBranch()/createFile().
---
Nitpick comments:
In `@tests/VCS/Adapter/GiteaTest.php`:
- Around line 101-145: The testCommentWorkflow method currently only deletes the
created repository on the happy path; wrap the main test logic in a try/finally
so the cleanup always runs: declare repositoryName as you already do, then
enclose creation/assertions (calls to createRepository, createFile,
createBranch, createPullRequest, createComment, getComment, updateComment, etc.)
in a try block and call $this->vcsAdapter->deleteRepository(self::$owner,
$repositoryName) inside a finally block to ensure the repo is removed even on
failures.
- Around line 193-205: The test testGetCommentInvalidId currently only asserts
the return is a string allowing raw API errors; update it (and the similar tests
around the other block) to assert a single concrete contract for the "not found"
case: either assertSame('', $result) if adapter should return an empty string,
or change the test to expectException(...) if the adapter should throw; locate
and modify the call to getComment in testGetCommentInvalidId (and the analogous
tests) and replace the loose assertIsString/assertIsArray with a specific
assertion matching the adapter's intended behavior, keeping
createRepository/createFile/deleteRepository setup/teardown intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8ed472e-7adc-479f-84ea-6a2939c54353
📒 Files selected for processing (2)
src/VCS/Adapter/Git/Gitea.phptests/VCS/Adapter/GiteaTest.php
| public function createFile(string $owner, string $repositoryName, string $filepath, string $content, string $message = 'Add file', string $branch = ''): array | ||
| { | ||
| $url = "/repos/{$owner}/{$repositoryName}/contents/{$filepath}"; | ||
|
|
||
| $payload = [ | ||
| 'content' => base64_encode($content), | ||
| 'message' => $message | ||
| ]; | ||
|
|
||
| // Add branch if specified | ||
| if (!empty($branch)) { | ||
| $payload['branch'] = $branch; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Promote branch-aware file creation to the shared Git contract.
Line 191 adds a sixth parameter only on Gitea::createFile(), but the abstract Git::createFile() contract still exposes the 5-argument version. That makes branch-targeted writes adapter-specific and invisible to callers using the common abstraction. If this feature is meant to be portable, move $branch into the base contract; otherwise, add a separate branch-aware method instead.
♻️ Suggested contract alignment
-abstract public function createFile(string $owner, string $repositoryName, string $filepath, string $content, string $message = 'Add file'): array;
+abstract public function createFile(
+ string $owner,
+ string $repositoryName,
+ string $filepath,
+ string $content,
+ string $message = 'Add file',
+ string $branch = ''
+): array;
📝 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.
| public function createFile(string $owner, string $repositoryName, string $filepath, string $content, string $message = 'Add file', string $branch = ''): array | |
| { | |
| $url = "/repos/{$owner}/{$repositoryName}/contents/{$filepath}"; | |
| $payload = [ | |
| 'content' => base64_encode($content), | |
| 'message' => $message | |
| ]; | |
| // Add branch if specified | |
| if (!empty($branch)) { | |
| $payload['branch'] = $branch; | |
| } | |
| abstract public function createFile( | |
| string $owner, | |
| string $repositoryName, | |
| string $filepath, | |
| string $content, | |
| string $message = 'Add file', | |
| string $branch = '' | |
| ): array; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/VCS/Adapter/Git/Gitea.php` around lines 191 - 203, The Gitea adapter
added an optional $branch parameter to Gitea::createFile(string $owner, string
$repositoryName, string $filepath, string $content, string $message = 'Add
file', string $branch = '') but the abstract Git::createFile() contract still
defines only five parameters; make the branch behavior part of the shared
contract by updating the abstract method signature in Git::createFile to accept
the optional sixth parameter (string $branch = '') and then update all concrete
implementations and any call sites to accept/forward this optional $branch
argument (or alternatively add a new branch-aware method on Git if you prefer to
keep the old signature, ensuring Git::createFile remains unchanged and adding
e.g. createFileOnBranch with the same parameters used in Gitea::createFile).
There was a problem hiding this comment.
@Meldiron - I've addressed the other review feedback, but need guidance on this one.
CodeRabbit suggests adding the $branch parameter to the abstract Git::createFile() method in src/VCS/Adapter/Git.php. However, this is a breaking change that would affect all VCS adapters (GitHub, GitLab, Bitbucket, etc.).
Options:
-
Add optional
$branch = ''to abstractGit::createFile()- Pro: Consistent interface across all adapters
- Con: Breaking change, other adapters need to update their signatures
-
Create new
createFileOnBranch()method in Gitea only- Pro: Non-breaking, Gitea-specific
- Con: Inconsistent API across adapters
-
Keep current implementation with
$branchonly in Gitea'screateFile()- Pro: Works fine, minimal changes
- Con: Violates Liskov Substitution Principle
My preference: Option 2 (new method), but I'll follow whatever approach fits the project's architecture best.
What would you like me to do?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
- Add HTTP status checks to createPullRequest and getPullRequest - Add try/finally blocks to tests for proper cleanup on failures - Make error test assertions concrete (empty string or exception) - Improve testGetPullRequestWithInvalidNumber to use try/finally Note: Issue utopia-php#1 (adding branch parameter to abstract Git::createFile) requires architectural decision - will address after maintainer feedback
1 participant
Tests: 7 new tests, all passing
Summary by CodeRabbit
New Features
Tests