Agents - switch to using GraphQL for the PullRequestModel#309807
Agents - switch to using GraphQL for the PullRequestModel#309807
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Agents window GitHub PR integration to fetch a single “PR snapshot” via GraphQL (PR details + mergeability + review threads) and adjusts the reactive model, service usage, and tests accordingly.
Changes:
- Replace the PR fetcher’s REST/GraphQL mix with a single GraphQL snapshot query and mapping.
- Simplify
GitHubPullRequestModelrefresh behavior to one fetch + atomic observable updates (and removerefreshThreads). - Update
CodeReviewServiceand browser tests to the new refresh/fetcher shape.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts | Switch to GraphQL snapshot query and derive PR/mergeability/threads from one response. |
| src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts | Refresh now fetches a full snapshot and updates observables in a transaction; refreshThreads removed. |
| src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts | Use prModel.refresh() for initial load (instead of threads-only refresh). |
| src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts | Update tests to validate the new GraphQL snapshot behavior and return shape. |
| src/vs/sessions/contrib/github/test/browser/githubModels.test.ts | Update model tests to match the new fetcher contract and refresh behavior. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts:147
- The GraphQL snapshot query hard-limits
reviews,reviewThreads, and per-threadcommentstofirst: 100without any pagination. This can lead to missing threads/comments in the UI, and (more importantly) incorrect mergeability blocker detection when there are >100 reviews or threads. Consider addingpageInfo { hasNextPage endCursor }and paging withafter, or at least requesting the most recent items explicitly (e.g.last: 100) and treating truncated data asUnknownmergeability.
' reviews(first: 100) {',
' nodes {',
' state',
' submittedAt',
' author {',
' login',
' }',
' }',
' }',
' reviewThreads(first: 100) {',
' nodes {',
' id',
' isResolved',
' path',
' line',
' comments(first: 100) {',
' nodes {',
- Files reviewed: 5/5 changed files
- Comments generated: 2
| // Start polling and initial fetch | ||
| prModel.refreshThreads().catch(err => { | ||
| prModel.refresh().catch(err => { | ||
| this._logService.error('[CodeReviewService] Failed to fetch PR review threads:', err); | ||
| data.state.set({ kind: PRReviewStateKind.Error, reason: String(err) }, undefined); | ||
| }); |
There was a problem hiding this comment.
prModel.refresh().catch(...) is unlikely to ever run because GitHubPullRequestModel.refresh() swallows fetch errors internally (it logs but doesn’t rethrow). As a result, the PR review UI won’t transition to Error on refresh failures, and the log message also no longer matches what’s being fetched (it’s a full PR snapshot, not just threads). Consider either propagating refresh errors from the model (rethrow / return a result) or handling failures via a dedicated observable/error signal; also update the log message accordingly.
See below for a potential fix:
// Start polling and trigger the initial PR snapshot refresh.
void prModel.refresh();
No description provided.