add pagination to listBranches and add method listAllBranches #50
Conversation
WalkthroughThe Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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 (2)
src/VCS/Adapter/Git/GitHub.php (2)
467-471: Consider updating the abstract method signature in the base class.Adding optional pagination parameters maintains backward compatibility, but creates an inconsistency where the base
Adapterclass signature doesn't reflect the pagination capabilities. Consider either:
- Preferred: Update the abstract method in
src/VCS/Adapter.phpline 196 to include optional pagination parameters:abstract public function listBranches(string $owner, string $repositoryName, int $page = 1, int $perPage = 30): array;
- Alternative: Keep the current design and document that
listBranchesmay optionally support pagination in specific adapters, whilelistAllBranchesprovides a consistent interface across all adapters.
498-502: Consider resilience improvements for production use.The current implementation doesn't handle API failures during pagination, which means partial results are lost if an error occurs. For repositories with many branches, this could be frustrating.
Optional improvements to consider:
- Error recovery: Catch exceptions and return partial results with a warning, or implement retry logic
- Rate limit awareness: GitHub API has rate limits; consider adding delay between requests or checking rate limit headers
- Performance: Replace
array_mergewith array append ($allBranches[] = ...in a foreach) to avoid creating new arrays on each iterationThese are nice-to-have improvements that can be deferred depending on expected usage patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/VCS/Adapter.php(1 hunks)src/VCS/Adapter/Git/GitHub.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/VCS/Adapter.php (1)
src/VCS/Adapter/Git/GitHub.php (1)
listAllBranches(492-505)
src/VCS/Adapter/Git/GitHub.php (1)
src/VCS/Adapter.php (2)
listBranches(196-196)listAllBranches(205-205)
🔇 Additional comments (2)
src/VCS/Adapter/Git/GitHub.php (1)
473-473: LGTM!The URL construction correctly adds pagination query parameters in the format expected by the GitHub API.
src/VCS/Adapter.php (1)
198-205: Implementation is complete and not a breaking change.The new abstract method is well-documented with clear pagination semantics. The GitHub adapter already provides a sound implementation that properly iterates through all pages using the existing
listBranchesmethod. Since theGitclass is abstract, it correctly defers implementation to concrete subclasses like GitHub—no breaking change occurs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/VCS/Adapter/GitHubTest.php (1)
395-411: LGTM! Test covers basic functionality and pagination.The test successfully verifies branch listing and pagination behavior. Note that this test overrides the
testListBranchesmethod from the base class (tests/VCS/Base.php), which is appropriate for testing GitHub-specific pagination features.Consider adding edge case coverage for robustness:
- Test behavior when requesting a page beyond available branches (e.g.,
page = 100)- Test with invalid parameters (e.g.,
page = 0,perPage = 0, negative values)- Test with a repository that has no branches or only one branch
- Verify order consistency across paginated requests
These additions would strengthen the test suite but are not critical for the current functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/VCS/Adapter/Git/GitHub.php(1 hunks)tests/VCS/Adapter/GitHubTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/VCS/Adapter/Git/GitHub.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/VCS/Adapter/GitHubTest.php (3)
tests/VCS/Base.php (1)
testListBranches(65-70)src/VCS/Adapter/Git/GitHub.php (1)
listBranches(471-483)src/VCS/Adapter.php (1)
listBranches(196-196)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
467-483: Test coverage for pagination parameters exists and is functional.The
testListBranchestest has been added to verify both default and custom pagination behavior. The test confirms that:
- Default pagination (page=1, perPage=30) returns the expected branches
- Custom pagination parameters (page=1, perPage=1) correctly limit results to one branch per page
However, the test could be enhanced to explicitly verify the query parameter construction in the API URL (e.g., asserting the
pageandper_pageparameters are correctly passed to/repos/$owner/$repositoryName/branches?page=1&per_page=1).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/VCS/Adapter/Git/GitHub.php(1 hunks)tests/VCS/Adapter/GitHubTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/VCS/Adapter/GitHubTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/VCS/Adapter/Git/GitHub.php (1)
src/VCS/Adapter.php (1)
listBranches(196-196)
🔇 Additional comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
471-483: Update PR description or implement thelistAllBranchesmethod.The PR description states that this pull request "introduces a new method listAllBranches," but this method does not exist anywhere in the codebase. Either the method was intentionally removed based on feedback and the PR description needs updating, or the implementation is incomplete and the method still needs to be added.
Clarify whether:
- The
listAllBranchesmethod was removed based on reviewer feedback (Meldiron's comment about consumer-side implementation), and the PR description should be updated accordingly.- The method should still be implemented and is missing from the current commit.
| * @param int $page Page number for pagination | ||
| * @param int $perPage Number of results per page | ||
| * @return array<string> List of branch names as array | ||
| */ | ||
| public function listBranches(string $owner, string $repositoryName): array | ||
| public function listBranches(string $owner, string $repositoryName, int $page = 1, int $perPage = 30): array |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify abstract method signature and find all adapter implementations
echo "=== Checking abstract method definition in Adapter.php ==="
rg -n -A 2 "abstract.*function listBranches" --type=php
echo -e "\n=== Finding all implementations of listBranches ==="
rg -n -B 2 -A 5 "public function listBranches" --type=php
echo -e "\n=== Finding all classes that extend Adapter or Git ==="
rg -n "class.*extends.*(Adapter|Git)" --type=phpRepository: utopia-php/vcs
Length of output: 1648
🏁 Script executed:
#!/bin/bash
# Check if there are other VCS adapters beyond GitHub
echo "=== Looking for all Adapter subclasses ==="
rg -n "class.*extends.*Adapter|class.*extends.*Git" --type=php
echo -e "\n=== Checking for all branches method calls in tests or elsewhere ==="
rg -n "listBranches" --type=php | head -20Repository: utopia-php/vcs
Length of output: 896
Update the abstract method signature to include pagination parameters for consistency.
The listBranches method signature in src/VCS/Adapter.php (line 196) does not include the optional pagination parameters added to the GitHub implementation:
abstract public function listBranches(string $owner, string $repositoryName): array;While the default values (int $page = 1, int $perPage = 30) in the GitHub implementation maintain backward compatibility for existing callers, the abstract method definition should be updated to match the implementation for architectural consistency and clarity about the interface contract.
🤖 Prompt for AI Agents
In src/VCS/Adapter.php around line 196, the abstract method listBranches lacks
the optional pagination parameters added in the GitHub implementation; update
the abstract signature to match by adding int $page = 1, int $perPage = 30 to
the method declaration and update its docblock return/type hints accordingly so
the interface contract matches implementations.
There was a problem hiding this comment.
Worried if some other providers would have different ways / not have pagination at all. We can add it in if the next provider has similar behaviours.
There was a problem hiding this comment.
@atharvadeosthale That's a very reasonable approach! Waiting to see how pagination works across multiple providers before standardizing the interface is a sound architectural decision. The default parameters you've added (int $page = 1, int $perPage = 30) already ensure backward compatibility for existing callers, so there's no breaking change risk.
Your strategy of deferring the abstract method update until there's empirical evidence from a second provider implementation follows good design principles and avoids premature abstraction. This keeps the interface flexible for now while the GitHub implementation validates the pagination pattern.
There was a problem hiding this comment.
Worried if some other providers would have different ways / not have pagination at all. We can add it in if the next provider has similar behaviours.
If there's no pagination on an X provider, we can simply not use the variable. Its best that the base adapter is the source of truth with needed params. cc @atharvadeosthale
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!
3 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.