Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded an installationId parameter to the VCS adapter API and updated implementations and tests to pass installation context. GitHub adapter: if an installation has access to all repos, it uses /search/repositories and returns items and total from GitHub; if access is limited, it uses /installation/repositories — delegating pagination when search is empty, or fetching pages of 100 repos, locally filtering by name when search is non-empty, accumulating results, slicing for the requested page, and returning total as the accumulated count. Error checks added for missing repository lists. Gitea adapter signature updated; test call sites adjusted. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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
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 109-136: The loop always pages through all repositories even when
$search is empty; change the method in GitHub (the block using $repositories,
$currentPage and the call(self::METHOD_GET, '/installation/repositories', ...))
to short-circuit: if empty($search) make a single API call with the requested
'page' and 'per_page' parameters and return or use that result directly (no
while loop), otherwise keep the existing paging+filtering logic (array_filter on
$response['body']['repositories']) to aggregate pages until fewer than 100 items
are returned; ensure you still throw the same exception when 'repositories' is
missing.
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 112-126: The code returns 'total' using
$repositories['body']['total_count'] without guarding its existence; update the
GitHub adapter (class GitHub, where you call $this->call(...)) to validate that
$repositories['body']['total_count'] is set and is an integer—if it's missing or
invalid, throw a clear Exception (e.g., "total_count missing from GitHub
response") rather than returning a misleading fallback; ensure you still
validate $repositories['body']['repositories'] as before and reference the
$repositories['body']['repositories'] and $repositories['body']['total_count']
keys when constructing the return value.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
108-190: Previous concern still applies:$owneris unused in the installation-specific path.In the
hasAccessToAllRepositoriesbranch (line 120),$owneris used in the search query. However, in the installation-scoped path (lines 136–189),$owneris never referenced. If the installation spans repos across multiple owners, the results won't be owner-filtered, creating a behavioral inconsistency between the two code paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/VCS/Adapter/Git/GitHub.php` around lines 108 - 190, searchRepositories currently ignores $owner in the installation-scoped path, causing inconsistent results; update the installation-specific flows in searchRepositories to filter repositories by owner: when delegating pagination (empty $search) filter $repositories['body']['repositories'] by repo['owner']['login'] === $owner before returning and adjust 'total' accordingly, and when fetching all pages for a search include owner in the array_filter predicate (e.g. require both stripos($repo['name'], $search) !== false and repo['owner']['login'] === $owner) so repositories, $filteredRepositories and final $repositoriesInRequestedPage reflect the owner filter. Ensure you reference $owner, $repositories, $filteredRepositories and $repositoriesInRequestedPage in the fixes.
🧹 Nitpick comments (2)
src/VCS/Adapter/Git/GitHub.php (2)
110-113: Extra API call on every search request adds latency.
/app/installations/{id}is called on everysearchRepositoriesinvocation just to checkrepository_selection. This value is unlikely to change frequently. Consider caching it (e.g., alongside the access token ininitializeVariables) to avoid an extra round-trip on every search.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/VCS/Adapter/Git/GitHub.php` around lines 110 - 113, The searchRepositories path currently makes an extra API call to /app/installations/{id} to compute $hasAccessToAllRepositories; move that retrieval into initializeVariables (alongside the access token/JWT) and cache the repository_selection there so searchRepositories can read the cached value instead of calling call(...) each time. Concretely, in initializeVariables fetch and store repository_selection (e.g. as $this->repositorySelection or similar) when you obtain/refresh the installation token/JWT, update uses of $hasAccessToAllRepositories in searchRepositories to reference the cached property, and ensure you refresh/clear the cached repository_selection whenever tokens are refreshed or installationId changes.
157-182: Unboundedwhile (true)loop has no safety cap.If an installation has access to thousands of repositories, this loop will make dozens of API calls, potentially hitting rate limits or causing timeouts (the
call()method has a 15-second timeout per request). Consider adding a maximum iteration guard to prevent runaway behavior.Suggested fix
$currentPage = 1; - while (true) { + $maxPages = 50; // Safety cap: 50 pages × 100 repos = 5000 repos max + while ($currentPage <= $maxPages) { $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [ 'page' => $currentPage, 'per_page' => 100, // Maximum allowed by GitHub API ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/VCS/Adapter/Git/GitHub.php` around lines 157 - 182, The unbounded while(true) in the repository paging loop can run indefinitely; add a maximum-iteration guard (e.g. $maxPages or MAX_PAGES constant) and change the loop to stop or throw when $currentPage > $maxPages to prevent runaway API calls. Specifically, update the code around $currentPage, the while loop that calls call(self::METHOD_GET, ...), and the pagination logic that merges into $repositories so that after a configurable maximum page count you break and optionally log/throw a descriptive error about hitting the page cap (include $search and $currentPage in the message). Ensure the new guard preserves existing behavior when fewer than 100 repos are returned and still increments $currentPage as before.
🤖 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/GitHub.php`:
- Around line 119-124: The search query can have a leading space when $search is
empty, so update the call that builds the q parameter (the call to $this->call
with METHOD_GET using $url) to trim or conditionally construct the q string: if
$search is non-empty prepend "$search " else omit it, then append "user:{$owner}
fork:true" (or simply rtrim the concatenated string) before passing into the 'q'
entry so no leading space appears when $search is ''. Ensure this change is
applied where $search is used to build the q => "{$search} user:{$owner}
fork:true".
In `@tests/VCS/Base.php`:
- Around line 54-55: The code in tests/VCS/Base.php sets $installationId using
System::getEnv('INSTALLATION_ID') but other tests use
TESTS_GITHUB_INSTALLATION_ID; update the env lookup so $installationId =
System::getEnv('TESTS_GITHUB_INSTALLATION_ID') ?? '' (or otherwise use the same
TESTS_GITHUB_INSTALLATION_ID variable) before calling
$this->vcsAdapter->searchRepositories to ensure the correct GitHub installation
ID is used consistently with GitHubTest.php and docker-compose.yml.
---
Duplicate comments:
In `@src/VCS/Adapter/Git/GitHub.php`:
- Around line 108-190: searchRepositories currently ignores $owner in the
installation-scoped path, causing inconsistent results; update the
installation-specific flows in searchRepositories to filter repositories by
owner: when delegating pagination (empty $search) filter
$repositories['body']['repositories'] by repo['owner']['login'] === $owner
before returning and adjust 'total' accordingly, and when fetching all pages for
a search include owner in the array_filter predicate (e.g. require both
stripos($repo['name'], $search) !== false and repo['owner']['login'] === $owner)
so repositories, $filteredRepositories and final $repositoriesInRequestedPage
reflect the owner filter. Ensure you reference $owner, $repositories,
$filteredRepositories and $repositoriesInRequestedPage in the fixes.
---
Nitpick comments:
In `@src/VCS/Adapter/Git/GitHub.php`:
- Around line 110-113: The searchRepositories path currently makes an extra API
call to /app/installations/{id} to compute $hasAccessToAllRepositories; move
that retrieval into initializeVariables (alongside the access token/JWT) and
cache the repository_selection there so searchRepositories can read the cached
value instead of calling call(...) each time. Concretely, in initializeVariables
fetch and store repository_selection (e.g. as $this->repositorySelection or
similar) when you obtain/refresh the installation token/JWT, update uses of
$hasAccessToAllRepositories in searchRepositories to reference the cached
property, and ensure you refresh/clear the cached repository_selection whenever
tokens are refreshed or installationId changes.
- Around line 157-182: The unbounded while(true) in the repository paging loop
can run indefinitely; add a maximum-iteration guard (e.g. $maxPages or MAX_PAGES
constant) and change the loop to stop or throw when $currentPage > $maxPages to
prevent runaway API calls. Specifically, update the code around $currentPage,
the while loop that calls call(self::METHOD_GET, ...), and the pagination logic
that merges into $repositories so that after a configurable maximum page count
you break and optionally log/throw a descriptive error about hitting the page
cap (include $search and $currentPage in the message). Ensure the new guard
preserves existing behavior when fewer than 100 repos are returned and still
increments $currentPage as before.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/VCS/Adapter.phpsrc/VCS/Adapter/Git/GitHub.phpsrc/VCS/Adapter/Git/Gitea.phptests/VCS/Base.php
Meldiron
reviewed
Feb 25, 2026
Meldiron
approved these changes
Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related SER-642
Ensures only the repositories accessible to the installation is returned
Summary by CodeRabbit