From c9b6e226d50dccdac8b4461175d64156a052fbc5 Mon Sep 17 00:00:00 2001 From: Ladislau Szomoru <3372902+lszomoru@users.noreply.github.com> Date: Mon, 27 Apr 2026 15:03:03 +0200 Subject: [PATCH 1/3] Revert "Agents - improve GitHub model handling (#312342)" This reverts commit 3981b5d4fd52539c8fd978ccd9a0613191d1855e. --- .../models/githubPullRequestCIModel.ts | 41 +++---------------- .../browser/models/githubPullRequestModel.ts | 41 ++++--------------- 2 files changed, 13 insertions(+), 69 deletions(-) diff --git a/src/vs/sessions/contrib/github/browser/models/githubPullRequestCIModel.ts b/src/vs/sessions/contrib/github/browser/models/githubPullRequestCIModel.ts index 1c14f56eb151b..3f0cec668525e 100644 --- a/src/vs/sessions/contrib/github/browser/models/githubPullRequestCIModel.ts +++ b/src/vs/sessions/contrib/github/browser/models/githubPullRequestCIModel.ts @@ -25,10 +25,7 @@ export class GitHubPullRequestCIModel extends Disposable { private readonly _overallStatus = observableValue(this, GitHubCIOverallStatus.Neutral); readonly overallStatus: IObservable = this._overallStatus; - private _pollingClients = 0; - private _refreshPromise: Promise | undefined; private readonly _pollScheduler: RunOnceScheduler; - private _disposed = false; constructor( @@ -46,19 +43,7 @@ export class GitHubPullRequestCIModel extends Disposable { /** * Refresh all CI check data. */ - refresh(): Promise { - if (!this._refreshPromise) { - this._refreshPromise = this._refresh().finally(() => { - if (this._refreshPromise) { - this._refreshPromise = undefined; - } - }); - } - - return this._refreshPromise; - } - - private async _refresh(): Promise { + async refresh(): Promise { try { const checks = await this._fetcher.getCheckRuns(this.owner, this.repo, this.headRef); this._checks.set(checks, undefined); @@ -93,43 +78,27 @@ export class GitHubPullRequestCIModel extends Disposable { * Start periodic polling. Each cycle refreshes CI check data. */ startPolling(intervalMs: number = DEFAULT_POLL_INTERVAL_MS): void { - this._pollScheduler.delay = intervalMs; - - this._pollingClients++; - if (this._pollingClients === 1) { - this._pollScheduler.cancel(); - this._pollScheduler.schedule(); - } + this._pollScheduler.cancel(); + this._pollScheduler.schedule(intervalMs); } /** * Stop periodic polling. */ stopPolling(): void { - if (this._pollingClients === 0) { - return; - } - - this._pollingClients--; - if (this._pollingClients === 0) { - this._pollScheduler.cancel(); - } + this._pollScheduler.cancel(); } private async _poll(): Promise { await this.refresh(); - // Re-schedule if not disposed (RunOnceScheduler is one-shot) - if (!this._disposed && this._pollingClients > 0) { + if (!this._disposed) { this._pollScheduler.schedule(); } } override dispose(): void { this._disposed = true; - this._pollingClients = 0; - this._refreshPromise = undefined; - super.dispose(); } } diff --git a/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts b/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts index 510e4811ab6b6..0af71dfbbe929 100644 --- a/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts +++ b/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts @@ -28,10 +28,7 @@ export class GitHubPullRequestModel extends Disposable { private readonly _reviewThreads = observableValue(this, []); readonly reviewThreads: IObservable = this._reviewThreads; - private _pollingClients = 0; - private _refreshPromise: Promise | undefined; private readonly _pollScheduler: RunOnceScheduler; - private _disposed = false; constructor( @@ -49,16 +46,8 @@ export class GitHubPullRequestModel extends Disposable { /** * Refresh all PR data: pull request info, mergeability, and review threads. */ - refresh(): Promise { - if (!this._refreshPromise) { - this._refreshPromise = this._refresh().finally(() => { - if (this._refreshPromise) { - this._refreshPromise = undefined; - } - }); - } - - return this._refreshPromise; + async refresh(): Promise { + await this._refresh(); } /** @@ -91,34 +80,23 @@ export class GitHubPullRequestModel extends Disposable { * Start periodic polling. Each cycle refreshes all PR data. */ startPolling(intervalMs: number = DEFAULT_POLL_INTERVAL_MS): void { - this._pollScheduler.delay = intervalMs; - - this._pollingClients++; - if (this._pollingClients === 1) { - this._pollScheduler.cancel(); - this._pollScheduler.schedule(); - } + this._pollScheduler.cancel(); + this._pollScheduler.schedule(intervalMs); } /** * Stop periodic polling. */ stopPolling(): void { - if (this._pollingClients === 0) { - return; - } - - this._pollingClients--; - if (this._pollingClients === 0) { - this._pollScheduler.cancel(); - } + this._pollScheduler.cancel(); } private async _poll(): Promise { await this.refresh(); - // Re-schedule if not disposed (RunOnceScheduler is one-shot) - if (!this._disposed && this._pollingClients > 0) { + // Re-schedule for next poll cycle + // as RunOnceScheduler is one-shot + if (!this._disposed) { this._pollScheduler.schedule(); } } @@ -139,9 +117,6 @@ export class GitHubPullRequestModel extends Disposable { override dispose(): void { this._disposed = true; - this._pollingClients = 0; - this._refreshPromise = undefined; - super.dispose(); } } From 2851a798a923a30d7652cc636faa7049f3426d4d Mon Sep 17 00:00:00 2001 From: Ladislau Szomoru <3372902+lszomoru@users.noreply.github.com> Date: Mon, 27 Apr 2026 15:03:43 +0200 Subject: [PATCH 2/3] Revert "Agents - switch to using GraphQL for the PullRequestModel (#309807)" This reverts commit 522c9dd40eed9c51b9f25543b4d66cca624b6c46. --- .../codeReview/browser/codeReviewService.ts | 2 +- .../browser/fetchers/githubPRFetcher.ts | 261 ++++++++---------- .../browser/models/githubPullRequestModel.ts | 60 ++-- .../test/browser/githubFetchers.test.ts | 209 +++++++------- .../github/test/browser/githubModels.test.ts | 33 ++- 5 files changed, 287 insertions(+), 278 deletions(-) diff --git a/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts b/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts index 4f5dde853c529..7f8cf951f56e7 100644 --- a/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts +++ b/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts @@ -704,7 +704,7 @@ export class CodeReviewService extends Disposable implements ICodeReviewService })); // Start polling and initial fetch - prModel.refresh().catch(err => { + 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); }); diff --git a/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts b/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts index 0bace27ca841e..a958e59e4c0c6 100644 --- a/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts +++ b/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts @@ -17,6 +17,30 @@ import { GitHubApiClient } from '../githubApiClient.js'; //#region GitHub API response types +interface IGitHubPRResponse { + readonly number: number; + readonly title: string; + readonly body: string | null; + readonly state: 'open' | 'closed'; + readonly draft: boolean; + readonly user: { readonly login: string; readonly avatar_url: string }; + readonly head: { readonly ref: string; readonly sha: string }; + readonly base: { readonly ref: string }; + readonly created_at: string; + readonly updated_at: string; + readonly merged_at: string | null; + readonly mergeable: boolean | null; + readonly mergeable_state: string; + readonly merged: boolean; +} + +interface IGitHubReviewResponse { + readonly id: number; + readonly user: { readonly login: string; readonly avatar_url: string }; + readonly state: string; + readonly submitted_at: string; +} + interface IGitHubReviewCommentResponse { readonly id: number; readonly body: string; @@ -29,44 +53,6 @@ interface IGitHubReviewCommentResponse { readonly in_reply_to_id?: number; } -type IGitHubGraphQLPullRequestState = 'OPEN' | 'CLOSED' | 'MERGED'; -type IGitHubGraphQLMergeable = 'MERGEABLE' | 'CONFLICTING' | 'UNKNOWN'; -type IGitHubGraphQLMergeStateStatus = 'BEHIND' | 'BLOCKED' | 'CLEAN' | 'DIRTY' | 'DRAFT' | 'HAS_HOOKS' | 'UNKNOWN' | 'UNSTABLE'; -type IGitHubGraphQLReviewState = 'APPROVED' | 'CHANGES_REQUESTED' | 'DISMISSED' | 'COMMENTED' | 'PENDING'; - -interface IGitHubGraphQLPullRequestResponse { - readonly repository: { - readonly pullRequest: IGitHubGraphQLPullRequestNode | null; - } | null; -} - -interface IGitHubGraphQLPullRequestNode { - readonly number: number; - readonly title: string; - readonly body: string; - readonly state: IGitHubGraphQLPullRequestState; - readonly isDraft: boolean; - readonly author: { readonly login: string; readonly avatarUrl: string } | null; - readonly headRefName: string; - readonly headRefOid: string; - readonly baseRefName: string; - readonly createdAt: string; - readonly updatedAt: string; - readonly mergedAt: string | null; - readonly mergeable: IGitHubGraphQLMergeable; - readonly mergeStateStatus: IGitHubGraphQLMergeStateStatus; - readonly reviews: { - readonly nodes: readonly { - readonly state: IGitHubGraphQLReviewState; - readonly submittedAt: string | null; - readonly author: { readonly login: string } | null; - }[]; - }; - readonly reviewThreads: { - readonly nodes: readonly IGitHubGraphQLReviewThreadNode[]; - }; -} - interface IGitHubIssueCommentResponse { readonly id: number; readonly body: string | null; @@ -75,6 +61,16 @@ interface IGitHubIssueCommentResponse { readonly updated_at: string; } +interface IGitHubGraphQLPullRequestReviewThreadsResponse { + readonly repository: { + readonly pullRequest: { + readonly reviewThreads: { + readonly nodes: readonly IGitHubGraphQLReviewThreadNode[]; + }; + } | null; + } | null; +} + interface IGitHubGraphQLReviewThreadNode { readonly id: string; readonly isResolved: boolean; @@ -107,36 +103,10 @@ interface IGitHubGraphQLResolveReviewThreadResponse { //#endregion -const GET_PULL_REQUEST_QUERY = [ - 'query GetPullRequestSnapshot($owner: String!, $repo: String!, $prNumber: Int!) {', +const GET_REVIEW_THREADS_QUERY = [ + 'query GetReviewThreads($owner: String!, $repo: String!, $prNumber: Int!) {', ' repository(owner: $owner, name: $repo) {', ' pullRequest(number: $prNumber) {', - ' number', - ' title', - ' body', - ' state', - ' isDraft', - ' author {', - ' login', - ' avatarUrl', - ' }', - ' headRefName', - ' headRefOid', - ' baseRefName', - ' createdAt', - ' updatedAt', - ' mergedAt', - ' mergeable', - ' mergeStateStatus', - ' reviews(first: 100) {', - ' nodes {', - ' state', - ' submittedAt', - ' author {', - ' login', - ' }', - ' }', - ' }', ' reviewThreads(first: 100) {', ' nodes {', ' id', @@ -188,29 +158,79 @@ export class GitHubPRFetcher { private readonly _apiClient: GitHubApiClient, ) { } - async getPullRequest(owner: string, repo: string, prNumber: number): Promise<{ - readonly pullRequest: IGitHubPullRequest; - readonly mergeability: IGitHubPullRequestMergeability; - readonly reviewThreads: readonly IGitHubPRReviewThread[]; - }> { - const data = await this._apiClient.graphql( - GET_PULL_REQUEST_QUERY, - 'githubApi.getPullRequest', - { owner, repo, prNumber }, + async getPullRequest(owner: string, repo: string, prNumber: number): Promise { + const data = await this._apiClient.request( + 'GET', + `/repos/${e(owner)}/${e(repo)}/pulls/${prNumber}`, + 'githubApi.getPullRequest' ); + return mapPullRequest(data); + } - const pr = data.repository?.pullRequest; - if (!pr) { - throw new Error(`Pull request not found: ${owner}/${repo}#${prNumber}`); + async getMergeability(owner: string, repo: string, prNumber: number): Promise { + const [pr, reviews] = await Promise.all([ + this._apiClient.request('GET', `/repos/${e(owner)}/${e(repo)}/pulls/${prNumber}`, 'githubApi.getMergeability.pr'), + this._apiClient.request('GET', `/repos/${e(owner)}/${e(repo)}/pulls/${prNumber}/reviews`, 'githubApi.getMergeability.reviews'), + ]); + + const blockers: IMergeBlocker[] = []; + + // Draft + if (pr.draft) { + blockers.push({ kind: MergeBlockerKind.Draft, description: 'Pull request is a draft' }); + } + + // Merge conflicts + if (pr.mergeable === false) { + blockers.push({ kind: MergeBlockerKind.Conflicts, description: 'Pull request has merge conflicts' }); + } + + // Changes requested — check most recent review per reviewer + const latestReviewByUser = new Map(); + for (const review of reviews) { + if (review.state === 'APPROVED' || review.state === 'CHANGES_REQUESTED' || review.state === 'DISMISSED') { + latestReviewByUser.set(review.user.login, review.state); + } + } + const hasChangesRequested = [...latestReviewByUser.values()].some(s => s === 'CHANGES_REQUESTED'); + if (hasChangesRequested) { + blockers.push({ kind: MergeBlockerKind.ChangesRequested, description: 'Changes have been requested' }); + } + + // Approval needed — check mergeable_state + if (pr.mergeable_state === 'blocked') { + const hasApproval = [...latestReviewByUser.values()].some(s => s === 'APPROVED'); + if (!hasApproval) { + blockers.push({ kind: MergeBlockerKind.ApprovalNeeded, description: 'Approval is required' }); + } + } + + // CI failures — mergeable_state 'unstable' indicates check failures + if (pr.mergeable_state === 'unstable') { + blockers.push({ kind: MergeBlockerKind.CIFailed, description: 'CI checks have failed' }); } return { - pullRequest: mapPullRequestFromGraphQL(pr), - mergeability: mapMergeabilityFromGraphQL(pr), - reviewThreads: pr.reviewThreads.nodes.map(mapReviewThread), + canMerge: blockers.length === 0 && pr.mergeable !== false && pr.state === 'open', + blockers, }; } + async getReviewThreads(owner: string, repo: string, prNumber: number): Promise { + const data = await this._apiClient.graphql( + GET_REVIEW_THREADS_QUERY, + 'githubApi.getReviewThreads', + { owner, repo, prNumber }, + ); + + const reviewThreads = data.repository?.pullRequest?.reviewThreads.nodes; + if (!reviewThreads) { + throw new Error(`Pull request not found: ${owner}/${repo}#${prNumber}`); + } + + return reviewThreads.map(mapReviewThread); + } + async postReviewComment( owner: string, repo: string, @@ -275,82 +295,31 @@ function mapUser(user: { readonly login: string; readonly avatar_url: string }): return { login: user.login, avatarUrl: user.avatar_url }; } -function mapPullRequestFromGraphQL(data: IGitHubGraphQLPullRequestNode): IGitHubPullRequest { +function mapPullRequest(data: IGitHubPRResponse): IGitHubPullRequest { let state: GitHubPullRequestState; - if (data.state === 'MERGED') { + if (data.merged) { state = GitHubPullRequestState.Merged; - } else if (data.state === 'CLOSED') { + } else if (data.state === 'closed') { state = GitHubPullRequestState.Closed; } else { state = GitHubPullRequestState.Open; } - const author = data.author ?? { login: 'ghost', avatarUrl: '' }; - return { number: data.number, title: data.title, - body: data.body, + body: data.body ?? '', state, - author, - headRef: data.headRefName, - headSha: data.headRefOid, - baseRef: data.baseRefName, - isDraft: data.isDraft, - createdAt: data.createdAt, - updatedAt: data.updatedAt, - mergedAt: data.mergedAt ?? undefined, - mergeable: data.mergeable === 'UNKNOWN' ? undefined : data.mergeable === 'MERGEABLE', - mergeableState: data.mergeStateStatus.toLowerCase(), - }; -} - -function mapMergeabilityFromGraphQL(pr: IGitHubGraphQLPullRequestNode): IGitHubPullRequestMergeability { - const blockers: IMergeBlocker[] = []; - - if (pr.isDraft) { - blockers.push({ kind: MergeBlockerKind.Draft, description: 'Pull request is a draft' }); - } - - if (pr.mergeable === 'CONFLICTING') { - blockers.push({ kind: MergeBlockerKind.Conflicts, description: 'Pull request has merge conflicts' }); - } - - const latestReviewByUser = new Map(); - for (const review of pr.reviews.nodes) { - if (!review.author) { - continue; - } - - if (review.state === 'APPROVED' || review.state === 'CHANGES_REQUESTED' || review.state === 'DISMISSED') { - const submittedAt = review.submittedAt ? Date.parse(review.submittedAt) : Number.NEGATIVE_INFINITY; - const login = review.author.login; - const previous = latestReviewByUser.get(login); - if (!previous || submittedAt >= previous.submittedAt) { - latestReviewByUser.set(login, { state: review.state, submittedAt }); - } - } - } - - const hasChangesRequested = [...latestReviewByUser.values()].some(review => review.state === 'CHANGES_REQUESTED'); - if (hasChangesRequested) { - blockers.push({ kind: MergeBlockerKind.ChangesRequested, description: 'Changes have been requested' }); - } - - if (pr.mergeStateStatus === 'BLOCKED') { - const hasApproval = [...latestReviewByUser.values()].some(review => review.state === 'APPROVED'); - if (!hasApproval) { - blockers.push({ kind: MergeBlockerKind.ApprovalNeeded, description: 'Approval is required' }); - } - } - - if (pr.mergeStateStatus === 'UNSTABLE') { - blockers.push({ kind: MergeBlockerKind.CIFailed, description: 'CI checks have failed' }); - } - - return { - canMerge: blockers.length === 0 && pr.mergeable !== 'CONFLICTING' && pr.state === 'OPEN', - blockers, + author: mapUser(data.user), + headRef: data.head.ref, + headSha: data.head.sha, + baseRef: data.base.ref, + isDraft: data.draft, + createdAt: data.created_at, + updatedAt: data.updated_at, + mergedAt: data.merged_at ?? undefined, + mergeable: data.mergeable ?? undefined, + mergeableState: data.mergeable_state, }; } diff --git a/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts b/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts index 0af71dfbbe929..8bcfbc0fbe9a2 100644 --- a/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts +++ b/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts @@ -5,7 +5,7 @@ import { RunOnceScheduler } from '../../../../../base/common/async.js'; import { Disposable } from '../../../../../base/common/lifecycle.js'; -import { IObservable, observableValue, transaction } from '../../../../../base/common/observable.js'; +import { IObservable, observableValue } from '../../../../../base/common/observable.js'; import { ILogService } from '../../../../../platform/log/common/log.js'; import { IGitHubPRComment, IGitHubPRReviewThread, IGitHubPullRequest, IGitHubPullRequestMergeability } from '../../common/types.js'; import { GitHubPRFetcher } from '../fetchers/githubPRFetcher.js'; @@ -47,7 +47,18 @@ export class GitHubPullRequestModel extends Disposable { * Refresh all PR data: pull request info, mergeability, and review threads. */ async refresh(): Promise { - await this._refresh(); + await Promise.all([ + this._refreshPullRequest(), + this._refreshMergeability(), + this._refreshThreads(), + ]); + } + + /** + * Refresh only the review threads. + */ + async refreshThreads(): Promise { + await this._refreshThreads(); } /** @@ -55,7 +66,7 @@ export class GitHubPullRequestModel extends Disposable { */ async postReviewComment(body: string, inReplyTo: number): Promise { const comment = await this._fetcher.postReviewComment(this.owner, this.repo, this.prNumber, body, inReplyTo); - await this._refresh(); + await this._refreshThreads(); return comment; } @@ -63,9 +74,7 @@ export class GitHubPullRequestModel extends Disposable { * Post a top-level issue comment on the PR. */ async postIssueComment(body: string): Promise { - const comment = await this._fetcher.postIssueComment(this.owner, this.repo, this.prNumber, body); - await this._refresh(); - return comment; + return this._fetcher.postIssueComment(this.owner, this.repo, this.prNumber, body); } /** @@ -73,7 +82,7 @@ export class GitHubPullRequestModel extends Disposable { */ async resolveThread(threadId: string): Promise { await this._fetcher.resolveThread(this.owner, this.repo, threadId); - await this._refresh(); + await this._refreshThreads(); } /** @@ -93,30 +102,41 @@ export class GitHubPullRequestModel extends Disposable { private async _poll(): Promise { await this.refresh(); - - // Re-schedule for next poll cycle - // as RunOnceScheduler is one-shot + // Re-schedule for next poll cycle (RunOnceScheduler is one-shot) if (!this._disposed) { this._pollScheduler.schedule(); } } - private async _refresh(): Promise { + override dispose(): void { + this._disposed = true; + super.dispose(); + } + + private async _refreshPullRequest(): Promise { try { const data = await this._fetcher.getPullRequest(this.owner, this.repo, this.prNumber); + this._pullRequest.set(data, undefined); + } catch (err) { + this._logService.error(`${LOG_PREFIX} Failed to refresh PR #${this.prNumber}:`, err); + } + } - transaction(tx => { - this._pullRequest.set(data.pullRequest, tx); - this._mergeability.set(data.mergeability, tx); - this._reviewThreads.set(data.reviewThreads, tx); - }); + private async _refreshMergeability(): Promise { + try { + const data = await this._fetcher.getMergeability(this.owner, this.repo, this.prNumber); + this._mergeability.set(data, undefined); } catch (err) { - this._logService.error(`${LOG_PREFIX} Failed to refresh PR snapshot for #${this.prNumber}:`, err); + this._logService.error(`${LOG_PREFIX} Failed to refresh mergeability for PR #${this.prNumber}:`, err); } } - override dispose(): void { - this._disposed = true; - super.dispose(); + private async _refreshThreads(): Promise { + try { + const data = await this._fetcher.getReviewThreads(this.owner, this.repo, this.prNumber); + this._reviewThreads.set(data, undefined); + } catch (err) { + this._logService.error(`${LOG_PREFIX} Failed to refresh threads for PR #${this.prNumber}:`, err); + } } } diff --git a/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts b/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts index 8d586f2059de1..0c338ceb251d4 100644 --- a/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts +++ b/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts @@ -122,56 +122,52 @@ suite('GitHubPRFetcher', () => { ensureNoDisposablesAreLeakedInTestSuite(); test('getPullRequest maps open PR', async () => { - mockApi.setNextResponse(makeGraphQLPRResponse({ state: 'OPEN', isDraft: false })); + mockApi.setNextResponse(makePRResponse({ state: 'open', merged: false, draft: false })); - const result = await fetcher.getPullRequest('owner', 'repo', 1); - assert.strictEqual(result.pullRequest.state, GitHubPullRequestState.Open); - assert.strictEqual(result.pullRequest.isDraft, false); - assert.strictEqual(result.pullRequest.number, 1); - assert.strictEqual(result.pullRequest.title, 'Test PR'); + const pr = await fetcher.getPullRequest('owner', 'repo', 1); + assert.strictEqual(pr.state, GitHubPullRequestState.Open); + assert.strictEqual(pr.isDraft, false); + assert.strictEqual(pr.number, 1); + assert.strictEqual(pr.title, 'Test PR'); }); test('getPullRequest maps merged PR', async () => { - mockApi.setNextResponse(makeGraphQLPRResponse({ state: 'MERGED', mergedAt: '2024-01-02T00:00:00Z' })); + mockApi.setNextResponse(makePRResponse({ state: 'closed', merged: true, draft: false })); - const result = await fetcher.getPullRequest('owner', 'repo', 1); - assert.strictEqual(result.pullRequest.state, GitHubPullRequestState.Merged); - assert.ok(result.pullRequest.mergedAt); + const pr = await fetcher.getPullRequest('owner', 'repo', 1); + assert.strictEqual(pr.state, GitHubPullRequestState.Merged); + assert.ok(pr.mergedAt); }); test('getPullRequest maps closed PR', async () => { - mockApi.setNextResponse(makeGraphQLPRResponse({ state: 'CLOSED' })); + mockApi.setNextResponse(makePRResponse({ state: 'closed', merged: false, draft: false })); - const result = await fetcher.getPullRequest('owner', 'repo', 1); - assert.strictEqual(result.pullRequest.state, GitHubPullRequestState.Closed); + const pr = await fetcher.getPullRequest('owner', 'repo', 1); + assert.strictEqual(pr.state, GitHubPullRequestState.Closed); }); - test('getPullRequest returns review threads', async () => { - mockApi.setNextResponse(makeGraphQLPRResponse({ - state: 'OPEN', - reviewThreads: [ - makeGraphQLReviewThread({ - id: 'thread-a', - path: 'src/a.ts', - line: 10, - isResolved: false, - comments: [ - makeGraphQLReviewComment({ databaseId: 100, path: 'src/a.ts', line: 10 }), - makeGraphQLReviewComment({ databaseId: 101, path: 'src/a.ts', line: 10, replyToDatabaseId: 100 }), - ], - }), - makeGraphQLReviewThread({ - id: 'thread-b', - path: 'src/b.ts', - line: 20, - isResolved: true, - comments: [makeGraphQLReviewComment({ databaseId: 200, path: 'src/b.ts', line: 20 })], - }), - ], - })); - - const result = await fetcher.getPullRequest('owner', 'repo', 1); - const threads = result.reviewThreads; + test('getReviewThreads returns GraphQL thread metadata', async () => { + mockApi.setNextResponse(makeGraphQLReviewThreadsResponse([ + makeGraphQLReviewThread({ + id: 'thread-a', + path: 'src/a.ts', + line: 10, + isResolved: false, + comments: [ + makeGraphQLReviewComment({ databaseId: 100, path: 'src/a.ts', line: 10 }), + makeGraphQLReviewComment({ databaseId: 101, path: 'src/a.ts', line: 10, replyToDatabaseId: 100 }), + ], + }), + makeGraphQLReviewThread({ + id: 'thread-b', + path: 'src/b.ts', + line: 20, + isResolved: true, + comments: [makeGraphQLReviewComment({ databaseId: 200, path: 'src/b.ts', line: 20 })], + }), + ])); + + const threads = await fetcher.getReviewThreads('owner', 'repo', 1); assert.strictEqual(threads.length, 2); const thread1 = threads.find(t => t.id === 'thread-a')!; @@ -202,41 +198,60 @@ suite('GitHubPRFetcher', () => { assert.deepStrictEqual(mockApi.graphqlCalls[0].variables, { threadId: 'thread-a' }); }); - test('getPullRequest detects draft blocker', async () => { - mockApi.setNextResponse(makeGraphQLPRResponse({ state: 'OPEN', isDraft: true, mergeable: 'MERGEABLE', mergeStateStatus: 'CLEAN' })); - - const result = await fetcher.getPullRequest('owner', 'repo', 1); - assert.strictEqual(result.mergeability.canMerge, false); - assert.ok(result.mergeability.blockers.some(b => b.kind === MergeBlockerKind.Draft)); + test('getMergeability detects draft blocker', async () => { + // getMergeability makes two requests (PR then reviews) + // Use a counter to return different responses + let callCount = 0; + const originalRequest = mockApi.request.bind(mockApi); + mockApi.request = async function (_method: string, _path: string, _body?: unknown): Promise { + if (callCount++ === 0) { + return makePRResponse({ state: 'open', merged: false, draft: true, mergeable: true, mergeable_state: 'clean' }) as T; + } + return [] as unknown as T; + }; + + const result = await fetcher.getMergeability('owner', 'repo', 1); + assert.strictEqual(result.canMerge, false); + assert.ok(result.blockers.some(b => b.kind === MergeBlockerKind.Draft)); + + // Restore + mockApi.request = originalRequest; }); - test('getPullRequest detects conflicts blocker', async () => { - mockApi.setNextResponse(makeGraphQLPRResponse({ - state: 'OPEN', - isDraft: false, - mergeable: 'CONFLICTING', - mergeStateStatus: 'DIRTY' - })); - - const result = await fetcher.getPullRequest('owner', 'repo', 1); - assert.strictEqual(result.mergeability.canMerge, false); - assert.ok(result.mergeability.blockers.some(b => b.kind === MergeBlockerKind.Conflicts)); + test('getMergeability detects conflicts blocker', async () => { + let callCount = 0; + const originalRequest = mockApi.request.bind(mockApi); + mockApi.request = async function (): Promise { + if (callCount++ === 0) { + return makePRResponse({ state: 'open', merged: false, draft: false, mergeable: false, mergeable_state: 'dirty' }) as T; + } + return [] as unknown as T; + }; + + const result = await fetcher.getMergeability('owner', 'repo', 1); + assert.strictEqual(result.canMerge, false); + assert.ok(result.blockers.some(b => b.kind === MergeBlockerKind.Conflicts)); + + mockApi.request = originalRequest; }); - test('getPullRequest detects changes requested blocker', async () => { - mockApi.setNextResponse(makeGraphQLPRResponse({ - state: 'OPEN', - isDraft: false, - mergeable: 'MERGEABLE', - mergeStateStatus: 'CLEAN', - reviews: [ - { state: 'CHANGES_REQUESTED', submittedAt: '2024-01-01T00:00:00Z', author: { login: 'reviewer' } }, - ], - })); - - const result = await fetcher.getPullRequest('owner', 'repo', 1); - assert.strictEqual(result.mergeability.canMerge, false); - assert.ok(result.mergeability.blockers.some(b => b.kind === MergeBlockerKind.ChangesRequested)); + test('getMergeability detects changes requested blocker', async () => { + let callCount = 0; + const originalRequest = mockApi.request.bind(mockApi); + mockApi.request = async function (): Promise { + if (callCount++ === 0) { + return makePRResponse({ state: 'open', merged: false, draft: false, mergeable: true, mergeable_state: 'clean' }) as T; + } + return [ + { id: 1, user: { login: 'reviewer', avatar_url: '' }, state: 'CHANGES_REQUESTED', submitted_at: '2024-01-01T00:00:00Z' }, + ] as unknown as T; + }; + + const result = await fetcher.getMergeability('owner', 'repo', 1); + assert.strictEqual(result.canMerge, false); + assert.ok(result.blockers.some(b => b.kind === MergeBlockerKind.ChangesRequested)); + + mockApi.request = originalRequest; }); }); @@ -348,37 +363,37 @@ suite('computeOverallCIStatus', () => { //#region Test Helpers -function makeGraphQLPRResponse(overrides: { - state?: 'OPEN' | 'CLOSED' | 'MERGED'; - isDraft?: boolean; - mergeable?: 'MERGEABLE' | 'CONFLICTING' | 'UNKNOWN'; - mergeStateStatus?: string; - mergedAt?: string | null; - reviews?: readonly { state: string; submittedAt: string | null; author: { login: string } | null }[]; - reviewThreads?: readonly ReturnType[]; -} = {}): unknown { +function makePRResponse(overrides: { + state: 'open' | 'closed'; + merged: boolean; + draft: boolean; + mergeable?: boolean | null; + mergeable_state?: string; +}): unknown { + return { + number: 1, + title: 'Test PR', + body: 'Test body', + state: overrides.state, + draft: overrides.draft, + user: { login: 'author', avatar_url: 'https://example.com/avatar' }, + head: { ref: 'feature-branch' }, + base: { ref: 'main' }, + created_at: '2024-01-01T00:00:00Z', + updated_at: '2024-01-02T00:00:00Z', + merged_at: overrides.merged ? '2024-01-02T00:00:00Z' : null, + mergeable: overrides.mergeable ?? true, + mergeable_state: overrides.mergeable_state ?? 'clean', + merged: overrides.merged, + }; +} + +function makeGraphQLReviewThreadsResponse(threads: readonly ReturnType[]): unknown { return { repository: { pullRequest: { - number: 1, - title: 'Test PR', - body: 'Test body', - state: overrides.state ?? 'OPEN', - isDraft: overrides.isDraft ?? false, - author: { login: 'author', avatarUrl: 'https://example.com/avatar' }, - headRefName: 'feature-branch', - headRefOid: 'abc123', - baseRefName: 'main', - createdAt: '2024-01-01T00:00:00Z', - updatedAt: '2024-01-02T00:00:00Z', - mergedAt: overrides.mergedAt ?? null, - mergeable: overrides.mergeable ?? 'MERGEABLE', - mergeStateStatus: overrides.mergeStateStatus ?? 'CLEAN', - reviews: { - nodes: overrides.reviews ?? [], - }, reviewThreads: { - nodes: overrides.reviewThreads ?? [], + nodes: threads, }, }, }, diff --git a/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts b/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts index 3220093f09446..e811b76c3cbc1 100644 --- a/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts +++ b/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts @@ -35,22 +35,22 @@ class MockPRFetcher { postReviewCommentCalls: { body: string; inReplyTo: number }[] = []; postIssueCommentCalls: { body: string }[] = []; - async getPullRequest(_owner: string, _repo: string, _prNumber: number): Promise<{ - readonly pullRequest: IGitHubPullRequest; - readonly mergeability: IGitHubPullRequestMergeability; - readonly reviewThreads: readonly IGitHubPRReviewThread[]; - }> { + async getPullRequest(_owner: string, _repo: string, _prNumber: number): Promise { if (!this.nextPR) { throw new Error('No mock PR'); } + return this.nextPR; + } + + async getMergeability(_owner: string, _repo: string, _prNumber: number): Promise { if (!this.nextMergeability) { throw new Error('No mock mergeability'); } - return { - pullRequest: this.nextPR, - mergeability: this.nextMergeability, - reviewThreads: this.nextThreads, - }; + return this.nextMergeability; + } + + async getReviewThreads(_owner: string, _repo: string, _prNumber: number): Promise { + return this.nextThreads; } async postReviewComment(_owner: string, _repo: string, _prNumber: number, body: string, inReplyTo: number): Promise { @@ -157,10 +157,17 @@ suite('GitHubPullRequestModel', () => { assert.strictEqual(model.reviewThreads.get().length, 1); }); + test('refreshThreads only updates threads', async () => { + const model = store.add(new GitHubPullRequestModel('owner', 'repo', 1, mockFetcher as unknown as GitHubPRFetcher, logService)); + mockFetcher.nextThreads = [makeThread('thread-100', 'src/a.ts'), makeThread('thread-200', 'src/b.ts')]; + + await model.refreshThreads(); + assert.strictEqual(model.pullRequest.get(), undefined); // not refreshed + assert.strictEqual(model.reviewThreads.get().length, 2); + }); + test('postReviewComment calls fetcher and refreshes threads', async () => { const model = store.add(new GitHubPullRequestModel('owner', 'repo', 1, mockFetcher as unknown as GitHubPRFetcher, logService)); - mockFetcher.nextPR = makePR(); - mockFetcher.nextMergeability = { canMerge: true, blockers: [] }; mockFetcher.nextThreads = []; const comment = await model.postReviewComment('LGTM', 100); @@ -171,8 +178,6 @@ suite('GitHubPullRequestModel', () => { test('postIssueComment calls fetcher', async () => { const model = store.add(new GitHubPullRequestModel('owner', 'repo', 1, mockFetcher as unknown as GitHubPRFetcher, logService)); - mockFetcher.nextPR = makePR(); - mockFetcher.nextMergeability = { canMerge: true, blockers: [] }; const comment = await model.postIssueComment('Great work!'); assert.strictEqual(comment.body, 'Great work!'); From be4486ae0f1341ec81981600646b2c2bc05ca650 Mon Sep 17 00:00:00 2001 From: Ladislau Szomoru <3372902+lszomoru@users.noreply.github.com> Date: Mon, 27 Apr 2026 15:52:52 +0200 Subject: [PATCH 3/3] Avoid duplicate network calls to get the pull request details --- .../browser/fetchers/githubPRFetcher.ts | 118 ++++++++++-------- .../browser/models/githubPullRequestModel.ts | 49 ++++---- .../sessions/contrib/github/common/types.ts | 9 +- .../test/browser/githubFetchers.test.ts | 104 ++++++++------- .../github/test/browser/githubModels.test.ts | 19 ++- 5 files changed, 168 insertions(+), 131 deletions(-) diff --git a/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts b/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts index a958e59e4c0c6..9197d31a274d4 100644 --- a/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts +++ b/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts @@ -6,12 +6,13 @@ import { GitHubPullRequestState, IGitHubPRComment, - IGitHubPRReviewThread, + IGitHubPullRequestReview, IGitHubPullRequest, IGitHubPullRequestMergeability, IGitHubUser, IMergeBlocker, MergeBlockerKind, + IGitHubPullRequestReviewThread, } from '../../common/types.js'; import { GitHubApiClient } from '../githubApiClient.js'; @@ -167,56 +168,16 @@ export class GitHubPRFetcher { return mapPullRequest(data); } - async getMergeability(owner: string, repo: string, prNumber: number): Promise { - const [pr, reviews] = await Promise.all([ - this._apiClient.request('GET', `/repos/${e(owner)}/${e(repo)}/pulls/${prNumber}`, 'githubApi.getMergeability.pr'), - this._apiClient.request('GET', `/repos/${e(owner)}/${e(repo)}/pulls/${prNumber}/reviews`, 'githubApi.getMergeability.reviews'), - ]); - - const blockers: IMergeBlocker[] = []; - - // Draft - if (pr.draft) { - blockers.push({ kind: MergeBlockerKind.Draft, description: 'Pull request is a draft' }); - } - - // Merge conflicts - if (pr.mergeable === false) { - blockers.push({ kind: MergeBlockerKind.Conflicts, description: 'Pull request has merge conflicts' }); - } - - // Changes requested — check most recent review per reviewer - const latestReviewByUser = new Map(); - for (const review of reviews) { - if (review.state === 'APPROVED' || review.state === 'CHANGES_REQUESTED' || review.state === 'DISMISSED') { - latestReviewByUser.set(review.user.login, review.state); - } - } - const hasChangesRequested = [...latestReviewByUser.values()].some(s => s === 'CHANGES_REQUESTED'); - if (hasChangesRequested) { - blockers.push({ kind: MergeBlockerKind.ChangesRequested, description: 'Changes have been requested' }); - } - - // Approval needed — check mergeable_state - if (pr.mergeable_state === 'blocked') { - const hasApproval = [...latestReviewByUser.values()].some(s => s === 'APPROVED'); - if (!hasApproval) { - blockers.push({ kind: MergeBlockerKind.ApprovalNeeded, description: 'Approval is required' }); - } - } - - // CI failures — mergeable_state 'unstable' indicates check failures - if (pr.mergeable_state === 'unstable') { - blockers.push({ kind: MergeBlockerKind.CIFailed, description: 'CI checks have failed' }); - } - - return { - canMerge: blockers.length === 0 && pr.mergeable !== false && pr.state === 'open', - blockers, - }; + async getReviews(owner: string, repo: string, prNumber: number): Promise { + const data = await this._apiClient.request( + 'GET', + `/repos/${e(owner)}/${e(repo)}/pulls/${prNumber}/reviews`, + 'githubApi.getReviews', + ); + return data.map(mapReview); } - async getReviewThreads(owner: string, repo: string, prNumber: number): Promise { + async getReviewThreads(owner: string, repo: string, prNumber: number): Promise { const data = await this._apiClient.graphql( GET_REVIEW_THREADS_QUERY, 'githubApi.getReviewThreads', @@ -285,6 +246,54 @@ export class GitHubPRFetcher { } } +/** + * Compute pull request mergeability from a PR and its reviews. Pure function + * so callers that already have the PR payload don't need to refetch it. + */ +export function computeMergeability(pr: IGitHubPullRequest, reviews: readonly IGitHubPullRequestReview[]): IGitHubPullRequestMergeability { + const blockers: IMergeBlocker[] = []; + + // Draft + if (pr.isDraft) { + blockers.push({ kind: MergeBlockerKind.Draft, description: 'Pull request is a draft' }); + } + + // Merge conflicts + if (pr.mergeable === false) { + blockers.push({ kind: MergeBlockerKind.Conflicts, description: 'Pull request has merge conflicts' }); + } + + // Changes requested — check most recent review per reviewer + const latestReviewByUser = new Map(); + for (const review of reviews) { + if (review.state === 'APPROVED' || review.state === 'CHANGES_REQUESTED' || review.state === 'DISMISSED') { + latestReviewByUser.set(review.author.login, review.state); + } + } + const hasChangesRequested = [...latestReviewByUser.values()].some(s => s === 'CHANGES_REQUESTED'); + if (hasChangesRequested) { + blockers.push({ kind: MergeBlockerKind.ChangesRequested, description: 'Changes have been requested' }); + } + + // Approval needed — check mergeable_state + if (pr.mergeableState === 'blocked') { + const hasApproval = [...latestReviewByUser.values()].some(s => s === 'APPROVED'); + if (!hasApproval) { + blockers.push({ kind: MergeBlockerKind.ApprovalNeeded, description: 'Approval is required' }); + } + } + + // CI failures — mergeable_state 'unstable' indicates check failures + if (pr.mergeableState === 'unstable') { + blockers.push({ kind: MergeBlockerKind.CIFailed, description: 'CI checks have failed' }); + } + + return { + canMerge: blockers.length === 0 && pr.mergeable !== false && pr.state === GitHubPullRequestState.Open, + blockers, + }; +} + //#region Helpers function e(value: string): string { @@ -323,6 +332,15 @@ function mapPullRequest(data: IGitHubPRResponse): IGitHubPullRequest { }; } +function mapReview(data: IGitHubReviewResponse): IGitHubPullRequestReview { + return { + id: data.id, + author: mapUser(data.user), + state: data.state, + submittedAt: data.submitted_at, + }; +} + function mapReviewComment(data: IGitHubReviewCommentResponse): IGitHubPRComment { return { id: data.id, @@ -337,7 +355,7 @@ function mapReviewComment(data: IGitHubReviewCommentResponse): IGitHubPRComment }; } -function mapReviewThread(thread: IGitHubGraphQLReviewThreadNode): IGitHubPRReviewThread { +function mapReviewThread(thread: IGitHubGraphQLReviewThreadNode): IGitHubPullRequestReviewThread { return { id: thread.id, isResolved: thread.isResolved, diff --git a/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts b/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts index 8bcfbc0fbe9a2..415b0f60a6328 100644 --- a/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts +++ b/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts @@ -5,10 +5,10 @@ import { RunOnceScheduler } from '../../../../../base/common/async.js'; import { Disposable } from '../../../../../base/common/lifecycle.js'; -import { IObservable, observableValue } from '../../../../../base/common/observable.js'; +import { IObservable, observableValue, transaction } from '../../../../../base/common/observable.js'; import { ILogService } from '../../../../../platform/log/common/log.js'; -import { IGitHubPRComment, IGitHubPRReviewThread, IGitHubPullRequest, IGitHubPullRequestMergeability } from '../../common/types.js'; -import { GitHubPRFetcher } from '../fetchers/githubPRFetcher.js'; +import { IGitHubPRComment, IGitHubPullRequest, IGitHubPullRequestMergeability, IGitHubPullRequestReviewThread } from '../../common/types.js'; +import { computeMergeability, GitHubPRFetcher } from '../fetchers/githubPRFetcher.js'; const LOG_PREFIX = '[GitHubPullRequestModel]'; const DEFAULT_POLL_INTERVAL_MS = 60_000; @@ -25,8 +25,8 @@ export class GitHubPullRequestModel extends Disposable { private readonly _mergeability = observableValue(this, undefined); readonly mergeability: IObservable = this._mergeability; - private readonly _reviewThreads = observableValue(this, []); - readonly reviewThreads: IObservable = this._reviewThreads; + private readonly _reviewThreads = observableValue(this, []); + readonly reviewThreads: IObservable = this._reviewThreads; private readonly _pollScheduler: RunOnceScheduler; private _disposed = false; @@ -45,11 +45,12 @@ export class GitHubPullRequestModel extends Disposable { /** * Refresh all PR data: pull request info, mergeability, and review threads. + * The PR payload is fetched once and used to compute both `pullRequest` and + * `mergeability`, avoiding duplicate `GET /pulls/:number` calls per cycle. */ async refresh(): Promise { await Promise.all([ - this._refreshPullRequest(), - this._refreshMergeability(), + this._refreshPullRequestAndMergeability(), this._refreshThreads(), ]); } @@ -108,26 +109,21 @@ export class GitHubPullRequestModel extends Disposable { } } - override dispose(): void { - this._disposed = true; - super.dispose(); - } - - private async _refreshPullRequest(): Promise { + private async _refreshPullRequestAndMergeability(): Promise { try { - const data = await this._fetcher.getPullRequest(this.owner, this.repo, this.prNumber); - this._pullRequest.set(data, undefined); - } catch (err) { - this._logService.error(`${LOG_PREFIX} Failed to refresh PR #${this.prNumber}:`, err); - } - } + const [pr, reviews] = await Promise.all([ + this._fetcher.getPullRequest(this.owner, this.repo, this.prNumber), + this._fetcher.getReviews(this.owner, this.repo, this.prNumber), + ]); - private async _refreshMergeability(): Promise { - try { - const data = await this._fetcher.getMergeability(this.owner, this.repo, this.prNumber); - this._mergeability.set(data, undefined); + const mergeability = computeMergeability(pr, reviews); + + transaction(tx => { + this._pullRequest.set(pr, tx); + this._mergeability.set(mergeability, tx); + }); } catch (err) { - this._logService.error(`${LOG_PREFIX} Failed to refresh mergeability for PR #${this.prNumber}:`, err); + this._logService.error(`${LOG_PREFIX} Failed to refresh PR #${this.prNumber}:`, err); } } @@ -139,4 +135,9 @@ export class GitHubPullRequestModel extends Disposable { this._logService.error(`${LOG_PREFIX} Failed to refresh threads for PR #${this.prNumber}:`, err); } } + + override dispose(): void { + this._disposed = true; + super.dispose(); + } } diff --git a/src/vs/sessions/contrib/github/common/types.ts b/src/vs/sessions/contrib/github/common/types.ts index 723b061de737c..4ed6d7e2164eb 100644 --- a/src/vs/sessions/contrib/github/common/types.ts +++ b/src/vs/sessions/contrib/github/common/types.ts @@ -90,6 +90,13 @@ export interface IGitHubPullRequestMergeability { readonly blockers: readonly IMergeBlocker[]; } +export interface IGitHubPullRequestReview { + readonly id: number; + readonly author: IGitHubUser; + readonly state: string; + readonly submittedAt: string; +} + /** * Compute the PR status icon from a state value. * Accepts both the `GitHubPullRequestState` enum values and the @@ -128,7 +135,7 @@ export interface IGitHubPRComment { readonly inReplyToId: number | undefined; } -export interface IGitHubPRReviewThread { +export interface IGitHubPullRequestReviewThread { readonly id: string; readonly isResolved: boolean; readonly path: string; diff --git a/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts b/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts index 0c338ceb251d4..e3eddbc105157 100644 --- a/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts +++ b/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts @@ -6,11 +6,11 @@ import assert from 'assert'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { DisposableStore } from '../../../../../base/common/lifecycle.js'; -import { GitHubPRFetcher } from '../../browser/fetchers/githubPRFetcher.js'; +import { GitHubPRFetcher, computeMergeability } from '../../browser/fetchers/githubPRFetcher.js'; import { GitHubPRCIFetcher, computeOverallCIStatus } from '../../browser/fetchers/githubPRCIFetcher.js'; import { GitHubRepositoryFetcher } from '../../browser/fetchers/githubRepositoryFetcher.js'; import { GitHubApiClient, GitHubApiError } from '../../browser/githubApiClient.js'; -import { GitHubCheckConclusion, GitHubCheckStatus, GitHubCIOverallStatus, GitHubPullRequestState, MergeBlockerKind } from '../../common/types.js'; +import { GitHubCheckConclusion, GitHubCheckStatus, GitHubCIOverallStatus, GitHubPullRequestState, IGitHubPullRequestReview, IGitHubPullRequest, MergeBlockerKind } from '../../common/types.js'; class MockApiClient { @@ -198,60 +198,50 @@ suite('GitHubPRFetcher', () => { assert.deepStrictEqual(mockApi.graphqlCalls[0].variables, { threadId: 'thread-a' }); }); - test('getMergeability detects draft blocker', async () => { - // getMergeability makes two requests (PR then reviews) - // Use a counter to return different responses - let callCount = 0; - const originalRequest = mockApi.request.bind(mockApi); - mockApi.request = async function (_method: string, _path: string, _body?: unknown): Promise { - if (callCount++ === 0) { - return makePRResponse({ state: 'open', merged: false, draft: true, mergeable: true, mergeable_state: 'clean' }) as T; - } - return [] as unknown as T; - }; - - const result = await fetcher.getMergeability('owner', 'repo', 1); + test('getReviews maps API response', async () => { + mockApi.setNextResponse([ + { id: 1, user: { login: 'reviewer', avatar_url: '' }, state: 'APPROVED', submitted_at: '2024-01-01T00:00:00Z' }, + { id: 2, user: { login: 'other', avatar_url: '' }, state: 'CHANGES_REQUESTED', submitted_at: '2024-01-02T00:00:00Z' }, + ]); + + const reviews = await fetcher.getReviews('owner', 'repo', 1); + assert.deepStrictEqual(reviews, [ + { id: 1, author: { login: 'reviewer', avatarUrl: '' }, state: 'APPROVED', submittedAt: '2024-01-01T00:00:00Z' }, + { id: 2, author: { login: 'other', avatarUrl: '' }, state: 'CHANGES_REQUESTED', submittedAt: '2024-01-02T00:00:00Z' }, + ]); + assert.strictEqual(mockApi.requestCalls.length, 1); + assert.strictEqual(mockApi.requestCalls[0].path, '/repos/owner/repo/pulls/1/reviews'); + }); + + test('computeMergeability detects draft blocker', () => { + const pr = makePR({ state: GitHubPullRequestState.Open, isDraft: true, mergeable: true, mergeableState: 'clean' }); + const result = computeMergeability(pr, []); assert.strictEqual(result.canMerge, false); assert.ok(result.blockers.some(b => b.kind === MergeBlockerKind.Draft)); - - // Restore - mockApi.request = originalRequest; }); - test('getMergeability detects conflicts blocker', async () => { - let callCount = 0; - const originalRequest = mockApi.request.bind(mockApi); - mockApi.request = async function (): Promise { - if (callCount++ === 0) { - return makePRResponse({ state: 'open', merged: false, draft: false, mergeable: false, mergeable_state: 'dirty' }) as T; - } - return [] as unknown as T; - }; - - const result = await fetcher.getMergeability('owner', 'repo', 1); + test('computeMergeability detects conflicts blocker', () => { + const pr = makePR({ state: GitHubPullRequestState.Open, isDraft: false, mergeable: false, mergeableState: 'dirty' }); + const result = computeMergeability(pr, []); assert.strictEqual(result.canMerge, false); assert.ok(result.blockers.some(b => b.kind === MergeBlockerKind.Conflicts)); - - mockApi.request = originalRequest; }); - test('getMergeability detects changes requested blocker', async () => { - let callCount = 0; - const originalRequest = mockApi.request.bind(mockApi); - mockApi.request = async function (): Promise { - if (callCount++ === 0) { - return makePRResponse({ state: 'open', merged: false, draft: false, mergeable: true, mergeable_state: 'clean' }) as T; - } - return [ - { id: 1, user: { login: 'reviewer', avatar_url: '' }, state: 'CHANGES_REQUESTED', submitted_at: '2024-01-01T00:00:00Z' }, - ] as unknown as T; - }; - - const result = await fetcher.getMergeability('owner', 'repo', 1); + test('computeMergeability detects changes requested blocker', () => { + const pr = makePR({ state: GitHubPullRequestState.Open, isDraft: false, mergeable: true, mergeableState: 'clean' }); + const reviews: IGitHubPullRequestReview[] = [ + { id: 1, author: { login: 'reviewer', avatarUrl: '' }, state: 'CHANGES_REQUESTED', submittedAt: '2024-01-01T00:00:00Z' }, + ]; + const result = computeMergeability(pr, reviews); assert.strictEqual(result.canMerge, false); assert.ok(result.blockers.some(b => b.kind === MergeBlockerKind.ChangesRequested)); + }); - mockApi.request = originalRequest; + test('computeMergeability returns canMerge for clean open PR', () => { + const pr = makePR({ state: GitHubPullRequestState.Open, isDraft: false, mergeable: true, mergeableState: 'clean' }); + const result = computeMergeability(pr, []); + assert.strictEqual(result.canMerge, true); + assert.strictEqual(result.blockers.length, 0); }); }); @@ -363,6 +353,30 @@ suite('computeOverallCIStatus', () => { //#region Test Helpers +function makePR(overrides: { + state: GitHubPullRequestState; + isDraft: boolean; + mergeable: boolean | undefined; + mergeableState: string; +}): IGitHubPullRequest { + return { + number: 1, + title: 'Test PR', + body: 'Test body', + state: overrides.state, + author: { login: 'author', avatarUrl: '' }, + headRef: 'feature', + headSha: 'abc123', + baseRef: 'main', + isDraft: overrides.isDraft, + createdAt: '2024-01-01T00:00:00Z', + updatedAt: '2024-01-02T00:00:00Z', + mergedAt: undefined, + mergeable: overrides.mergeable, + mergeableState: overrides.mergeableState, + }; +} + function makePRResponse(overrides: { state: 'open' | 'closed'; merged: boolean; diff --git a/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts b/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts index e811b76c3cbc1..7cd1239391c34 100644 --- a/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts +++ b/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts @@ -13,7 +13,7 @@ import { GitHubRepositoryModel } from '../../browser/models/githubRepositoryMode import { GitHubPRFetcher } from '../../browser/fetchers/githubPRFetcher.js'; import { GitHubPRCIFetcher } from '../../browser/fetchers/githubPRCIFetcher.js'; import { GitHubRepositoryFetcher } from '../../browser/fetchers/githubRepositoryFetcher.js'; -import { GitHubCIOverallStatus, GitHubCheckConclusion, GitHubCheckStatus, GitHubPullRequestState, IGitHubCICheck, IGitHubPRComment, IGitHubPRReviewThread, IGitHubPullRequest, IGitHubPullRequestMergeability, IGitHubRepository } from '../../common/types.js'; +import { GitHubCIOverallStatus, GitHubCheckConclusion, GitHubCheckStatus, GitHubPullRequestState, IGitHubCICheck, IGitHubPRComment, IGitHubPullRequestReview, IGitHubPullRequest, IGitHubRepository, IGitHubPullRequestReviewThread } from '../../common/types.js'; //#region Mock Fetchers @@ -30,8 +30,8 @@ class MockRepositoryFetcher { class MockPRFetcher { nextPR: IGitHubPullRequest | undefined; - nextMergeability: IGitHubPullRequestMergeability | undefined; - nextThreads: IGitHubPRReviewThread[] = []; + nextReviews: IGitHubPullRequestReview[] = []; + nextThreads: IGitHubPullRequestReviewThread[] = []; postReviewCommentCalls: { body: string; inReplyTo: number }[] = []; postIssueCommentCalls: { body: string }[] = []; @@ -42,14 +42,11 @@ class MockPRFetcher { return this.nextPR; } - async getMergeability(_owner: string, _repo: string, _prNumber: number): Promise { - if (!this.nextMergeability) { - throw new Error('No mock mergeability'); - } - return this.nextMergeability; + async getReviews(_owner: string, _repo: string, _prNumber: number): Promise { + return this.nextReviews; } - async getReviewThreads(_owner: string, _repo: string, _prNumber: number): Promise { + async getReviewThreads(_owner: string, _repo: string, _prNumber: number): Promise { return this.nextThreads; } @@ -148,7 +145,7 @@ suite('GitHubPullRequestModel', () => { test('refresh populates all observables', async () => { const model = store.add(new GitHubPullRequestModel('owner', 'repo', 1, mockFetcher as unknown as GitHubPRFetcher, logService)); mockFetcher.nextPR = makePR(); - mockFetcher.nextMergeability = { canMerge: true, blockers: [] }; + mockFetcher.nextReviews = []; mockFetcher.nextThreads = [makeThread('thread-100', 'src/a.ts')]; await model.refresh(); @@ -280,7 +277,7 @@ function makePR(): IGitHubPullRequest { }; } -function makeThread(id: string, path: string): IGitHubPRReviewThread { +function makeThread(id: string, path: string): IGitHubPullRequestReviewThread { return { id, isResolved: false,