Retry review comments with smaller pages - #8890
Retry review comments with smaller pages#8890Tamir Duberstein (tamird) wants to merge 2 commits into
Conversation
fd5f8d8 allowed legacy queries to replace their variables, but also cleared them when a caller supplied only a fallback query. PullRequestComments then retried without owner, name, or number, masking the original failure with invalid-variable errors. Keep the original variables unless the fallback supplies replacements. Cover both inherited and explicitly replaced variables at the repository query boundary.
|
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.
| if (!data.repository) { | ||
| throw new Error('Review comments response did not include a repository.'); | ||
| } | ||
| const page = data.repository.pullRequest.reviewThreads; | ||
| reviewThreads.push(...page.nodes); |
There was a problem hiding this comment.
Fixed in 1b8afc3. The response is now explicitly nullable, and the guard uses data?.repository. A regression test verifies that null data produces the intended diagnostic without retrying. The full extension-host suite passes. Prepared with Codex.
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. Handle null data from an unavailable GraphQL client as a terminal failure. Cover cursor retention, retry exhaustion, non-502 failures, and missing query data.
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.
Depends on #8889, which preserves the variables needed by the legacy query. This branch currently includes that prerequisite; the additional change is 4df848f.
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. Regression coverage protects cursor retention, retry exhaustion, and non-502 failures.
Prepared with Codex.