Skip to content

Agents - revert the adoption of GraphQL for polling#312758

Merged
lszomoru merged 3 commits intomainfrom
lszomoru/github-rest-api-restore
Apr 27, 2026
Merged

Agents - revert the adoption of GraphQL for polling#312758
lszomoru merged 3 commits intomainfrom
lszomoru/github-rest-api-restore

Conversation

@lszomoru
Copy link
Copy Markdown
Member

@lszomoru lszomoru commented Apr 27, 2026

This reverts commit 522c9dd.
This reverts commit 3981b5d.

Reduce duplicate calls to get the details of a pull request.

Copilot AI review requested due to automatic review settings April 27, 2026 13:06
@lszomoru lszomoru enabled auto-merge (squash) April 27, 2026 13:06
@lszomoru lszomoru self-assigned this Apr 27, 2026
@lszomoru lszomoru added this to the 1.119.0 milestone Apr 27, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reverts a prior change that adopted GraphQL for PR polling in the Agents (sessions) GitHub integration, moving PR snapshot/mergeability fetching back toward REST while keeping GraphQL where still needed (review threads).

Changes:

  • Split PR fetching into separate calls for PR details, mergeability, and review threads (instead of a single combined GraphQL snapshot).
  • Update reactive models to refresh threads independently and simplify polling logic.
  • Adjust tests to match the new fetcher/model API shapes.
Show a summary per file
File Description
src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts Reworks PR fetching to REST for PR + mergeability, and GraphQL for review threads.
src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts Updates refresh/polling and adds refreshThreads() to refresh threads independently.
src/vs/sessions/contrib/github/browser/models/githubPullRequestCIModel.ts Simplifies refresh/polling behavior for CI checks.
src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts Switches initial PR review loading to use refreshThreads().
src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts Updates fetcher tests/helpers to align with REST PR responses + GraphQL threads.
src/vs/sessions/contrib/github/test/browser/githubModels.test.ts Updates model tests for the new fetcher/model API split and refreshThreads().

Copilot's findings

Comments suppressed due to low confidence (2)

src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts:711

  • After switching the initial fetch to refreshThreads(), this still starts polling via prModel.startPolling(), whose poll cycle refreshes all PR data (PR details + mergeability + threads). If CodeReviewService only needs review threads, consider using a threads-only polling path to avoid extra REST calls and reduce risk of GitHub API rate limiting.
		prModel.refreshThreads().catch(err => {
			this._logService.error('[CodeReviewService] Failed to fetch PR review threads:', err);
			data.state.set({ kind: PRReviewStateKind.Error, reason: String(err) }, undefined);
		});
		prModel.startPolling();

src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts:194

  • The code/comment says it checks the "most recent review per reviewer", but the implementation just overwrites map entries in iteration order and ignores submitted_at. If the API returns reviews newest-first (or any non-guaranteed ordering), this can misclassify changes-requested/approval blockers. Track the latest review per user by comparing timestamps (similar to the previous GraphQL-based logic) so blocker detection is order-independent.
		// Changes requested — check most recent review per reviewer
		const latestReviewByUser = new Map<string, string>();
		for (const review of reviews) {
			if (review.state === 'APPROVED' || review.state === 'CHANGES_REQUESTED' || review.state === 'DISMISSED') {
				latestReviewByUser.set(review.user.login, review.state);
			}
		}
  • Files reviewed: 6/6 changed files
  • Comments generated: 5

Comment thread src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts Outdated
Comment thread src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts
@lszomoru lszomoru merged commit cb297c5 into main Apr 27, 2026
26 checks passed
@lszomoru lszomoru deleted the lszomoru/github-rest-api-restore branch April 27, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants