Use installation/repositories API for searching repos
#53
Conversation
WalkthroughThe adapter replaces the GitHub search endpoint Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/VCS/Adapter/Git/GitHub.php`:
- Line 120: The array_filter call assigning $repositories uses case-sensitive
str_contains and the redundant null-coalescing ?? []; update the filter to use
case-insensitive matching via stripos (i.e., replace str_contains with a check
that stripos($repo['name'], $search) !== false) and remove the ?? [] so it
directly iterates $response['body']['repositories'] (which is already validated
earlier); keep the $repositories variable and array_filter call but swap the
matcher and drop the redundant fallback.
- Around line 109-125: The current implementation calls
$this->call(self::METHOD_GET, $url, ...) to fetch a single page and then filters
$response['body']['repositories'] locally, which breaks pagination and makes
'total' inaccurate; modify the logic in GitHub.php so that when $search is
non-empty you page through all results from $this->call(self::METHOD_GET, $url,
['Authorization' => "Bearer $this->accessToken"], ...) (looping with 'page'
param until no more items), accumulate all repositories, apply the local filter
(str_contains on $repo['name']), then return the filtered slice and accurate
total; keep single-page behavior when $search is empty to preserve performance.
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
107-107: Remove the unused$ownerparameter or update the endpoint to use owner-based filtering.The
$ownerparameter is declared in the method signature and documented in the abstract definition, but it's never used in the implementation since the method uses the/installation/repositoriesendpoint, which lists all repositories accessible to the GitHub App installation regardless of owner. Either remove the parameter to match the current endpoint behavior, or change the endpoint to use owner-specific filtering (e.g.,/users/{owner}/reposor/orgs/{owner}/repos).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/VCS/Adapter/Git/GitHub.php`:
- Around line 119-121: The code currently breaks out when
(!isset($response['body']['repositories'])) which silences API errors and can
return incomplete data; replace the break with throwing an exception (e.g.,
RuntimeException or a VCS-specific exception) from the GitHub adapter
(file/class GitHub.php) at the check for $response['body']['repositories'],
including contextual details such as the full $response payload (or status code
and error message) and the current request/page info so callers can surface and
handle the API error instead of silently continuing.
♻️ Duplicate comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
123-131: Stop condition uses filtered count; can truncate search results.If a page has <100 matches but more repositories exist, the loop ends early and misses later matches. Use the unfiltered page size to decide when to stop.
✅ Suggested fix
- // Filter repositories to only include those that match the search query. - $filteredRepositories = array_filter($response['body']['repositories'], fn ($repo) => empty($search) || stripos($repo['name'], $search) !== false); + $pageRepositories = $response['body']['repositories']; + // Filter repositories to only include those that match the search query. + $filteredRepositories = array_filter($pageRepositories, fn ($repo) => empty($search) || stripos($repo['name'], $search) !== false); // Merge with result so far. $repositories = array_merge($repositories, $filteredRepositories); - // If less than 100 repositories are returned, we have fetched all repositories. - if (\count($filteredRepositories) < 100) { + // If less than 100 repositories are returned, we have fetched all repositories. + if (\count($pageRepositories) < 100) { break; }
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
109-143: Avoid full scan when$searchis empty (perf regression).When no filter is needed, a single paged request is enough and avoids N+1 API calls.
♻️ Suggested refactor
public function searchRepositories(string $owner, int $page, int $per_page, string $search = ''): array { + if (empty($search)) { + $response = $this->call(self::METHOD_GET, '/installation/repositories', ['Authorization' => "Bearer $this->accessToken"], [ + 'page' => $page, + 'per_page' => $per_page, + ]); + if (!isset($response['body']['repositories'])) { + $status = $response['headers']['status-code'] ?? 'unknown'; + throw new Exception("Repositories list missing in response (status {$status})."); + } + return [ + 'items' => $response['body']['repositories'], + 'total' => $response['body']['total_count'] ?? \count($response['body']['repositories']), + ]; + } + $repositories = []; $currentPage = 1; while (true) { $url = '/installation/repositories'; $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [ 'page' => $currentPage, 'per_page' => 100, // Maximum allowed by GitHub API ]);
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/VCS/Adapter/Git/GitHub.php`:
- Around line 129-132: The loop break currently checks
count($filteredRepositories) (the post-filtered list) which causes premature
termination when many results are filtered out; change the condition to inspect
the raw page response count instead (the variable holding the unfiltered page
results—e.g. the array populated from the GitHub API response before filtering)
so use count(...) on that raw response variable rather than
$filteredRepositories inside the loop to determine when fewer than 100 items
were returned and to decide to break.
- Around line 107-108: The searchRepositories method currently takes an $owner
parameter that is never used; either implement local owner filtering, document
its deprecation, or remove it—choose one. To implement filtering, update
searchRepositories to, after fetching repositories from the
/installation/repositories response, filter the results by comparing
$repo['owner']['login'] to the $owner value (and handle empty $owner to mean no
filtering), preserving pagination semantics; alternatively add a docstring or
comment to searchRepositories noting that $owner is ignored by the API; or
remove the $owner parameter from the searchRepositories signature and update all
callers to avoid passing owner. Ensure the chosen approach updates the function
signature, callers, and any tests consistently.
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
109-143: Consider optimizing when no search term is provided.When
$searchis empty, the code fetches all pages only to slice the result at the end. For installations with hundreds of repositories, this is inefficient.When no search filtering is needed, you could use the API's native pagination directly:
Suggested optimization
public function searchRepositories(string $owner, int $page, int $per_page, string $search = ''): array { + $url = '/installation/repositories'; + + // When no search term, use API pagination directly + if (empty($search)) { + $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [ + 'page' => $page, + 'per_page' => $per_page, + ]); + + if (!isset($response['body']['repositories'])) { + throw new Exception("Repositories list missing in the response."); + } + + return [ + 'items' => $response['body']['repositories'], + 'total' => $response['body']['total_count'] ?? count($response['body']['repositories']), + ]; + } + + // When searching, fetch all and filter locally $repositories = []; - $currentPage = 1; while (true) { - $url = '/installation/repositories'; $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [ // ... rest of the loop
| // If less than 100 repositories are returned, we have fetched all repositories. | ||
| if (\count($filteredRepositories) < 100) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Loop termination condition checks filtered count instead of raw response count.
The break condition uses count($filteredRepositories) which is the count after filtering by search term. If a page returns 100 repositories but only a few match the search, the loop exits prematurely and skips remaining pages.
For example: Page 1 returns 100 repos, 5 match search → loop breaks → pages 2-N never fetched.
Proposed fix
// Filter repositories to only include those that match the search query.
$filteredRepositories = array_filter($response['body']['repositories'], fn ($repo) => empty($search) || stripos($repo['name'], $search) !== false);
// Merge with result so far.
$repositories = array_merge($repositories, $filteredRepositories);
// If less than 100 repositories are returned, we have fetched all repositories.
- if (\count($filteredRepositories) < 100) {
+ if (\count($response['body']['repositories']) < 100) {
break;
}📝 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.
| // If less than 100 repositories are returned, we have fetched all repositories. | |
| if (\count($filteredRepositories) < 100) { | |
| break; | |
| } | |
| // Filter repositories to only include those that match the search query. | |
| $filteredRepositories = array_filter($response['body']['repositories'], fn ($repo) => empty($search) || stripos($repo['name'], $search) !== false); | |
| // Merge with result so far. | |
| $repositories = array_merge($repositories, $filteredRepositories); | |
| // If less than 100 repositories are returned, we have fetched all repositories. | |
| if (\count($response['body']['repositories']) < 100) { | |
| break; | |
| } |
🤖 Prompt for AI Agents
In `@src/VCS/Adapter/Git/GitHub.php` around lines 129 - 132, The loop break
currently checks count($filteredRepositories) (the post-filtered list) which
causes premature termination when many results are filtered out; change the
condition to inspect the raw page response count instead (the variable holding
the unfiltered page results—e.g. the array populated from the GitHub API
response before filtering) so use count(...) on that raw response variable
rather than $filteredRepositories inside the loop to determine when fewer than
100 items were returned and to decide to break.
Related SER-642
Ensures only the repositories accessible to the installation is returned
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.