Fix: VCS branch pagination#12311
Conversation
Greptile SummaryThis PR replaces the previously unbounded GitHub branch-listing loop with a bounded
Confidence Score: 4/5The change is safe to merge with the caveat that repositories with more than 1,000 branches will receive an unhelpful exception; the core pagination logic is correct. The bounded loop and sentinel check are arithmetically correct. The remaining concern is the error message telling users to narrow results via search when search is applied after listBranches returns, leaving users of large repositories with no viable recovery path. src/Appwrite/Platform/Modules/VCS/Http/Installations/Repositories/Branches/XList.php — the exception message in listBranches warrants a closer look Important Files Changed
Reviews (5): Last reviewed commit: "Merge branch '1.9.x' into vcs-branch-pag..." | Re-trigger Greptile |
✨ Benchmark resultsComparing 1.9.x (before) to vcs-branch-pagination (after). Before
After
Delta
Top API waits
|
| $branches = []; | ||
| $maxPages = \intdiv(self::GITHUB_BRANCHES_MAX_RESULTS, self::GITHUB_BRANCHES_PER_PAGE); | ||
|
|
||
| for ($page = 1; $page <= $maxPages; $page++) { |
There was a problem hiding this comment.
We should accept pagination details as params to endpoint. Notice we already accept queries, lets utilize those - offset and limit queries.
I think we already have something similar done in other VCS endpoints, lets check there.
If not, any other list endpoint should showcase how it should be done - queries param, supporting offset and limit.
We should also proxy search term itself into github call, if that is possible by github APIs
|
|
||
| $remainingBranches = $github->listBranches($owner, $repositoryName, self::GITHUB_BRANCHES_PER_PAGE, $maxPages + 1); | ||
| if (!empty($remainingBranches)) { | ||
| throw new Exception(Exception::GENERAL_QUERY_INVALID, 'Repository has too many branches to list. Narrow the results using the search parameter.'); |
There was a problem hiding this comment.
Misleading error message —
search cannot prevent this exception
The error message instructs users to "narrow the results using the search parameter," but the search parameter is applied client-side in the action method (line 104) after listBranches returns. Because the exception is thrown inside listBranches before any search filtering, passing search still triggers the same exception for any repository with more than 1,000 branches. Users who follow this advice will receive the exact same error and have no way to recover using the suggested approach.
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testFunctionsStats |
1 | 10.18s | Logs |
UsageTest::testPrepareSitesStats |
1 | 7ms | Logs |
UsageTest::testEmbeddingsTextUsageDoesNotBreakProjectUsage |
1 | 5ms | Logs |
WebhooksCustomServerTest::testDeleteDeployment |
1 | 28ms | Logs |
WebhooksCustomServerTest::testDeleteFunction |
1 | 13ms | Logs |
WebhooksCustomServerTest::testCreateCollection |
1 | 10ms | Logs |
WebhooksCustomServerTest::testCreateAttributes |
1 | 10ms | Logs |
WebhooksCustomServerTest::testCreateDocument |
1 | 10ms | Logs |
WebhooksCustomServerTest::testUpdateDocument |
1 | 10ms | Logs |
WebhooksCustomServerTest::testDeleteDocument |
1 | 10ms | Logs |
WebhooksCustomServerTest::testCreateTable |
1 | 10ms | Logs |
WebhooksCustomServerTest::testCreateColumns |
1 | 11ms | Logs |
WebhooksCustomServerTest::testCreateRow |
1 | 11ms | Logs |
WebhooksCustomServerTest::testUpdateRow |
1 | 11ms | Logs |
WebhooksCustomServerTest::testDeleteRow |
1 | 11ms | Logs |
WebhooksCustomServerTest::testCreateStorageBucket |
1 | 7ms | Logs |
WebhooksCustomServerTest::testUpdateStorageBucket |
1 | 11ms | Logs |
WebhooksCustomServerTest::testCreateBucketFile |
1 | 11ms | Logs |
WebhooksCustomServerTest::testUpdateBucketFile |
1 | 5ms | Logs |
WebhooksCustomServerTest::testDeleteBucketFile |
1 | 5ms | Logs |
WebhooksCustomServerTest::testDeleteStorageBucket |
1 | 14ms | Logs |
WebhooksCustomServerTest::testCreateTeam |
1 | 5ms | Logs |
WebhooksCustomServerTest::testUpdateTeam |
1 | 10ms | Logs |
WebhooksCustomServerTest::testUpdateTeamPrefs |
1 | 12ms | Logs |
WebhooksCustomServerTest::testDeleteTeam |
1 | 5ms | Logs |
WebhooksCustomServerTest::testCreateTeamMembership |
1 | 13ms | Logs |
WebhooksCustomServerTest::testDeleteTeamMembership |
1 | 12ms | Logs |
WebhooksCustomServerTest::testWebhookAutoDisable |
1 | 29ms | Logs |
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist