Retry review comments with smaller pages - #8890
Retry review comments with smaller pages#8890Tamir Duberstein (tamird) wants to merge 2 commits into
Conversation
|
human here: confirmed without these changes i am unable to fetch review comments; with these changes, review comments reappear |
There was a problem hiding this comment.
Pull request overview
This PR improves resilience when refreshing PR review threads by making the GraphQL review-thread pagination adaptive: it keeps the normal 20-thread page size, but on HTTP 502 retries the same cursor with smaller pages (5 then 1) and continues fetching subsequent pages at the reduced size. It also preserves original variables when falling back to the legacy query unless explicit replacement variables are provided.
Changes:
- Add adaptive paging + retry-on-502 logic for fetching review threads, preserving cursors across retries.
- Update shared GraphQL queries to accept a
firstpage-size variable for both current and legacy review-thread queries. - Add regression tests covering cursor retention, retry exhaustion behavior, and legacy-fallback variable preservation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/github/pullRequestModel.ts | Implements 502-only retry with progressive page-size reduction while preserving cursors. |
| src/github/githubRepository.ts | Preserves original variables when executing a legacy fallback unless replacements are explicitly provided. |
| src/github/queriesShared.gql | Adds $first to PullRequestComments and LegacyPullRequestComments and uses it in reviewThreads(first: ...). |
| src/test/github/pullRequestModel.test.ts | Adds tests for retry behavior, page-size reduction, and cursor retention across retries. |
| src/test/github/githubRepository.test.ts | Adds tests ensuring legacy fallback uses original variables unless replacements are supplied. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4df848f to
1b8afc3
Compare
| assert.strictEqual(error.lastCall.args[0], 'Failed to get pull request review comments: Error: Review comments response did not include a repository.'); | ||
| }); | ||
|
|
||
| it('should update the cache when then cache is initialized', async function () { |
There was a problem hiding this comment.
This mock uses subset matching: QueryProvider compares only keys in the expected variable map, so additional first and after values do not prevent a match. This exact cache-initialization test passes, and the new pagination test explicitly checks both values. No change is needed. Prepared with Codex.
|
Dirk Bäumer (@dbaeumer) Alex Ross (@alexr00) could you kindly have a look at this? |
1b8afc3 to
24f9d71
Compare
fd5f8d8 allowed legacy queries to replace their variables, but left the fallback variables optional. PullRequestComments supplied only a query, so retrying discarded owner, name, number, and the pagination cursor and failed with invalid-variable errors. Require variables whenever a legacy query is supplied. Pass the review-comment variables explicitly on every page, while preserving separate argument maps for queries with different inputs. Cover replacement and legacy pagination at the repository boundary. Handle missing repository data before reading review threads, so a missing response produces the intended diagnostic.
GitHub can return an HTML 502 response for a review-thread query that succeeds when fewer threads are requested. The fixed page size makes the entire refresh fail, including pages already fetched. Retain the normal 20-thread page from e0e7627. On HTTP 502, retry the same cursor with five threads and then one, retaining the smaller size for later pages. Stop reducing at one and leave other errors alone. Both current and legacy queries accept the page size, preserving the legacy pagination added by 5523166. Cover cursor retention, retry exhaustion, non-502 failures, and reduced page sizes through the legacy query.
24f9d71 to
f70dc3e
Compare
Depends on #8889, which requires explicit variables for legacy queries. This branch includes that prerequisite; the pagination change is f70dc3e.
GitHub can return an HTML 502 response for a review-thread query that succeeds when fewer threads are requested. The fixed page size makes the entire refresh fail, including pages already fetched.
Retain the normal 20-thread page from e0e7627. On HTTP 502, retry the same cursor with five threads and then one, retaining the smaller size for later pages. Stop reducing at one and leave other errors alone. Both current and legacy queries receive the page size and cursor, preserving the legacy pagination added by 5523166. Regression coverage protects cursor retention, retry exhaustion, non-502 failures, and reduced page sizes through the legacy query.
Prepared with Codex.