diff --git a/src/github/copilotPrWatcher.ts b/src/github/copilotPrWatcher.ts index 429fa0c561..d36eaeaee7 100644 --- a/src/github/copilotPrWatcher.ts +++ b/src/github/copilotPrWatcher.ts @@ -12,7 +12,9 @@ import { debounce } from '../common/async'; import { COPILOT_ACCOUNTS } from '../common/comment'; import { COPILOT_LOGINS, copilotEventToStatus, CopilotPRStatus } from '../common/copilot'; import { Disposable } from '../common/lifecycle'; +import Logger from '../common/logger'; import { DEV_MODE, PR_SETTINGS_NAMESPACE, QUERIES } from '../common/settingKeys'; +import { formatError } from '../common/utils'; import { PrsTreeModel } from '../view/prsTreeModel'; export function isCopilotQuery(query: string): boolean { @@ -190,6 +192,7 @@ export class CopilotStateModel extends Disposable { } export class CopilotPRWatcher extends Disposable { + private static readonly ID = 'CopilotPRWatcher'; private readonly _model: CopilotStateModel; constructor(private readonly _reposManager: RepositoriesManager, private readonly _prsTreeModel: PrsTreeModel) { @@ -206,9 +209,10 @@ export class CopilotPRWatcher extends Disposable { } private _initialize() { - this._prsTreeModel.refreshCopilotStateChanges(true); this._pollForChanges(); - const updateFullState = debounce(() => this._prsTreeModel.refreshCopilotStateChanges(true), 50); + const updateFullState = debounce(() => this._prsTreeModel.refreshCopilotStateChanges(true).catch(error => { + Logger.error(`Refreshing Copilot pull request state failed: ${formatError(error)}`, CopilotPRWatcher.ID); + }), 50); this._register(this._reposManager.onDidChangeAnyPullRequests(e => { if (e.some(pr => COPILOT_ACCOUNTS[pr.model.author.login])) { if (!this._model.isInitialized) { @@ -256,7 +260,7 @@ export class CopilotPRWatcher extends Disposable { private async _pollForChanges(): Promise { // Skip polling if dev mode is enabled const devMode = vscode.workspace.getConfiguration(PR_SETTINGS_NAMESPACE).get(DEV_MODE, false); - if (devMode) { + if (devMode || this.isDisposed) { return; } @@ -265,9 +269,15 @@ export class CopilotPRWatcher extends Disposable { this._pollTimeout = undefined; } this._lastPollTime = Date.now(); - const shouldContinue = await this._prsTreeModel.refreshCopilotStateChanges(true); + try { + if (!await this._prsTreeModel.refreshCopilotStateChanges(true)) { + return; + } + } catch (error) { + Logger.error(`Refreshing Copilot pull request state failed: ${formatError(error)}`, CopilotPRWatcher.ID); + } - if (shouldContinue) { + if (!this.isDisposed && !this._pollTimeout) { this._pollTimeout = setTimeout(() => { this._pollForChanges(); }, this._pollInterval); diff --git a/src/github/folderRepositoryManager.ts b/src/github/folderRepositoryManager.ts index 43d9e715b9..75eb16dda3 100644 --- a/src/github/folderRepositoryManager.ts +++ b/src/github/folderRepositoryManager.ts @@ -127,6 +127,8 @@ export interface ItemsResponseResult { hasMorePages: boolean; hasUnsearchedRepositories: boolean; totalCount?: number; + // Pages reached across repositories, including pages with no matching items. + paginationProgress?: number; } export class NoGitHubReposError extends Error { @@ -1259,7 +1261,7 @@ export class FolderRepositoryManager extends Disposable { this.telemetry.sendTelemetryEvent('branch.delete'); } - // Keep track of how many pages we've fetched for each query, so when we reload we pull the same ones. + // Track reached pages, including failed requests, so reloading retries all of them. private totalFetchedPages = new Map(); /** @@ -1268,9 +1270,9 @@ export class FolderRepositoryManager extends Disposable { * 2) Fetch Next: fetch the next page from this remote, or if it has no more pages, the first page from the next remote that does have pages * 3) Restore: fetch all the pages you previously have fetched * - * When `options.fetchNextPage === false`, we are in case 2. + * When `options.fetchNextPage === true`, we are in case 2. * Otherwise: - * If `this.totalFetchQueries[queryId] === 0`, we are in case 1. + * If `this.totalFetchedPages.get(queryId)` is zero or unset, we are in case 1. * Otherwise, we're in case 3. */ private async fetchPagedData( @@ -1286,7 +1288,8 @@ export class FolderRepositoryManager extends Disposable { items: [], hasMorePages: false, hasUnsearchedRepositories: false, - totalCount: 0 + totalCount: 0, + paginationProgress: 0 }; } @@ -1329,6 +1332,12 @@ export class FolderRepositoryManager extends Disposable { // If we are in case 1 or 3, don't filter out repos that are out of pages, as we will be querying from the start. return info && (options.fetchNextPage === false || info.hasMorePages !== false); }); + const hasMorePages = () => githubRepositories.some(repo => + this._repositoryPageInformation.get(repo.remote.url.toString() + queryId)?.hasMorePages === true + ); + const paginationProgress = () => githubRepositoriesWithGitRemotes.reduce((total, repo) => + total + (this._repositoryPageInformation.get(repo.remote.url.toString() + queryId)?.pullRequestPage ?? 0), 0 + ); for (let i = 0; i < githubRepositories.length; i++) { const githubRepository = githubRepositories[i]; @@ -1362,16 +1371,17 @@ export class FolderRepositoryManager extends Disposable { }; if (options.fetchNextPage) { - // Case 2. Fetch a single new page, and increment the global number of pages fetched for this query. + // Case 2. Advance both counters before fetching so failures don't shorten the restore limit. pageInformation.pullRequestPage++; - addPage(await fetchPage(pageInformation.pullRequestPage)); setTotalFetchedPages(getTotalFetchedPages() + 1); + addPage(await fetchPage(pageInformation.pullRequestPage)); } else { // Case 1&3. Fetch all the pages we have fetched in the past, or in case 1, just a single page. if (pageInformation.pullRequestPage === 0) { // Case 1. Pretend we have previously fetched the first page, then hand off to the case 3 machinery to "fetch all pages we have fetched in the past" pageInformation.pullRequestPage = 1; + setTotalFetchedPages(getTotalFetchedPages() + 1); } const pages = await Promise.all( @@ -1393,25 +1403,22 @@ export class FolderRepositoryManager extends Disposable { const shouldBreakEarly = hasReceivedData && (isFetchingNextPage || hasReachedPreviousFetchLimit) && !hasUserConfiguredRemotes; if (shouldBreakEarly) { - if (getTotalFetchedPages() === 0) { - // We're in case 1, manually set number of pages we looked through until we found first results. - setTotalFetchedPages(pagesFetched); - } - return { items: itemData.items, - hasMorePages: pageInformation.hasMorePages, + hasMorePages: hasMorePages(), hasUnsearchedRepositories: i < githubRepositories.length - 1, totalCount: itemData.totalCount, + paginationProgress: paginationProgress() }; } } return { items: itemData.items, - hasMorePages: itemData.hasMorePages, + hasMorePages: hasMorePages(), hasUnsearchedRepositories: false, - totalCount: itemData.totalCount + totalCount: itemData.totalCount, + paginationProgress: paginationProgress() }; } diff --git a/src/test/view/prsTreeModel.test.ts b/src/test/view/prsTreeModel.test.ts new file mode 100644 index 0000000000..ac2df21270 --- /dev/null +++ b/src/test/view/prsTreeModel.test.ts @@ -0,0 +1,499 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { default as assert } from 'assert'; +import { createSandbox, SinonFakeTimers, SinonSandbox, SinonSpy, SinonStub } from 'sinon'; +import * as vscode from 'vscode'; +import { GitApiImpl } from '../../api/api1'; +import { GitHubServerType } from '../../common/authentication'; +import { CopilotPRStatus } from '../../common/copilot'; +import Logger from '../../common/logger'; +import { Protocol } from '../../common/protocol'; +import { GitHubRemote } from '../../common/remote'; +import { DEV_MODE, PR_SETTINGS_NAMESPACE, QUERIES } from '../../common/settingKeys'; +import { CopilotPRWatcher } from '../../github/copilotPrWatcher'; +import { CredentialStore } from '../../github/credentials'; +import { FolderRepositoryManager, ItemsResponseResult } from '../../github/folderRepositoryManager'; +import { GitHubRepository, PullRequestChangeEvent } from '../../github/githubRepository'; +import { PullRequestModel } from '../../github/pullRequestModel'; +import { RepositoriesManager } from '../../github/repositoriesManager'; +import { convertRESTPullRequestToRawPullRequest } from '../../github/utils'; +import { CreatePullRequestHelper } from '../../view/createPullRequestHelper'; +import { PrsTreeModel } from '../../view/prsTreeModel'; +import { PullRequestBuilder } from '../builders/rest/pullRequestBuilder'; +import { MockCommandRegistry } from '../mocks/mockCommandRegistry'; +import { MockExtensionContext } from '../mocks/mockExtensionContext'; +import { MockRepository } from '../mocks/mockRepository'; +import { MockTelemetry } from '../mocks/mockTelemetry'; +import { MockThemeWatcher } from '../mocks/mockThemeWatcher'; + +describe('Copilot pull request polling', function () { + const query = 'repo:${owner}/${repository} is:open author:copilot-swe-agent'; + let sinon: SinonSandbox; + let context: MockExtensionContext; + let telemetry: MockTelemetry; + let credentialStore: CredentialStore; + let reposManager: RepositoriesManager; + let repository: MockRepository; + let folderManager: FolderRepositoryManager; + let model: PrsTreeModel; + let origin: GitHubRepository; + let pages: Map; + let settings: Map; + let inspect: SinonStub; + let maxKnownPR: SinonStub; + let fetchPages: SinonStub; + let timeline: SinonStub; + let watcher: CopilotPRWatcher | undefined; + let createdPullRequests: PullRequestModel[]; + + async function addRemote(name: string, repo: string): Promise { + const url = `https://github.com/octo/${repo}.git`; + await repository.addRemote(name, url); + const remote = new GitHubRemote(name, url, new Protocol(url), GitHubServerType.GitHubDotCom); + const githubRepository = new GitHubRepository(1, remote, repository.rootUri, credentialStore, telemetry, true); + sinon.stub(githubRepository, 'getDefaultBranch').resolves('main'); + folderManager.gitHubRepositories.push(githubRepository); + pages.set(githubRepository, [[]]); + return githubRepository; + } + + function pullRequest(number: number, githubRepository: GitHubRepository = origin): PullRequestModel { + const raw = new PullRequestBuilder().number(number).user(user => user.login('copilot-swe-agent')).build(); + const pr = new PullRequestModel(credentialStore, telemetry, githubRepository, githubRepository.remote, + convertRESTPullRequestToRawPullRequest(raw, githubRepository)); + createdPullRequests.push(pr); + return pr; + } + + function fetchedPages(): number[] { + return fetchPages.getCalls().map(call => call.args[2]); + } + + function boundedQueries(limit: number): SinonStub { + const getPullRequests = model.getPullRequestsForQuery.bind(model); + const queries = sinon.stub(model, 'getPullRequestsForQuery').callsFake((...args) => { + assert.ok(queries.callCount <= limit, 'Pagination exceeded the expected number of queries'); + return getPullRequests(...args); + }); + return queries; + } + + beforeEach(async function () { + sinon = createSandbox(); + watcher = undefined; + createdPullRequests = []; + pages = new Map(); + settings = new Map([ + [QUERIES, [{ label: 'Copilot', query }]], + [DEV_MODE, false], + ]); + inspect = sinon.stub().returns(undefined); + const configuration: vscode.WorkspaceConfiguration = { + get: sinon.stub().callsFake((key: string, fallback?: unknown) => settings.has(key) ? settings.get(key) : fallback), + has: sinon.stub().callsFake((key: string) => settings.has(key)), + inspect, + update: sinon.stub().rejects(new Error('Tests must not write settings')), + }; + sinon.stub(vscode.workspace, 'getConfiguration').returns(configuration); + MockCommandRegistry.install(sinon); + context = new MockExtensionContext(); + telemetry = new MockTelemetry(); + credentialStore = new CredentialStore(telemetry, context); + reposManager = new RepositoriesManager(credentialStore, telemetry); + repository = new MockRepository(); + folderManager = new FolderRepositoryManager(0, context, repository, telemetry, new GitApiImpl(reposManager), + credentialStore, new CreatePullRequestHelper(), new MockThemeWatcher()); + sinon.stub(folderManager, 'getPullRequestDefaults').resolves({ owner: 'octo', repo: 'repo', base: 'main' }); + sinon.stub(folderManager, 'getActiveGitHubRemotes').callsFake(() => folderManager.gitHubRepositories.map(repo => repo.remote)); + origin = await addRemote('origin', 'repo'); + maxKnownPR = sinon.stub(origin, 'getMaxPullRequest').resolves(100); + reposManager.insertFolderManager(folderManager); + model = new PrsTreeModel(telemetry, reposManager, context); + sinon.stub(PullRequestModel.prototype, 'getStatusChecks').resolves([null, null]); + timeline = sinon.stub(PullRequestModel.prototype, 'getCopilotTimelineEvents').resolves([]); + fetchPages = sinon.stub(folderManager, 'getPullRequestsForCategory').callsFake(async (repo, _query, page = 1) => { + const repoPages = pages.get(repo)!; + assert.ok(page <= repoPages.length, `Unexpected page ${page}`); + return { + items: repoPages[page - 1], + hasMorePages: page < repoPages.length, + totalCount: repoPages.reduce((total, items) => total + items.length, 0), + }; + }); + }); + + afterEach(function () { + watcher?.dispose(); + model.dispose(); + for (const pr of createdPullRequests) { + pr.dispose(); + } + for (const repo of folderManager.gitHubRepositories) { + repo.dispose(); + } + for (const manager of [...reposManager.folderManagers]) { + reposManager.removeRepo(manager.repository); + } + reposManager.dispose(); + credentialStore.dispose(); + context.dispose(); + sinon.restore(); + }); + + for (const initialized of [false, true]) { + it(`advances three pages and processes each pull request once when initialized=${initialized}`, async function () { + const prs = [pullRequest(1), pullRequest(2), pullRequest(3)]; + pages.set(origin, prs.map(pr => [pr])); + if (initialized) { + model.copilotStateModel.setInitialized(); + } + const queries = boundedQueries(3); + + assert.strictEqual(await model.refreshCopilotStateChanges(true), true); + + assert.deepStrictEqual(fetchedPages(), [1, 2, 3]); + assert.deepStrictEqual(queries.getCalls().map(call => call.args[1]), [false, true, true]); + assert.deepStrictEqual(timeline.getCalls().map(call => call.thisValue), prs); + for (const call of timeline.getCalls()) { + assert.deepStrictEqual(call.args, [false, !initialized]); + } + assert.strictEqual(model.copilotStateModel.isInitialized, true); + assert.strictEqual(model.copilotStateModel.all.length, 3); + }); + } + + it('fetches a new page after a previously complete query grows', async function () { + const first = pullRequest(1); + const second = pullRequest(2); + boundedQueries(3); + pages.set(origin, [[first]]); + await model.refreshCopilotStateChanges(true); + timeline.resetHistory(); + pages.set(origin, [[first], [second]]); + maxKnownPR.resolves(101); + + assert.strictEqual(await model.refreshCopilotStateChanges(true), true); + + assert.deepStrictEqual(fetchedPages(), [1, 1, 2]); + assert.deepStrictEqual(timeline.getCalls().map(call => call.thisValue), [first, second]); + assert.strictEqual(model.copilotStateModel.all.length, 2); + }); + + it('continues an incomplete cached query rather than fetching its first page again', async function () { + const first = pullRequest(1); + const second = pullRequest(2); + boundedQueries(3); + pages.set(origin, [[first], [second]]); + await model.getPullRequestsForQuery(folderManager, false, query); + model.copilotStateModel.setInitialized(); + + await model.refreshCopilotStateChanges(true); + + assert.deepStrictEqual(fetchedPages(), [1, 2]); + assert.deepStrictEqual(timeline.getCalls().map(call => call.thisValue), [first, second]); + }); + + it('reuses a complete unchanged cache while refreshing timeline state', async function () { + const pr = pullRequest(1); + pages.set(origin, [[pr]]); + await model.refreshCopilotStateChanges(true); + timeline.resetHistory(); + + await model.refreshCopilotStateChanges(true); + + assert.deepStrictEqual(fetchedPages(), [1]); + sinon.assert.calledOnce(timeline); + }); + + it('preserves cumulative query results for sidebar callers', async function () { + const first = pullRequest(1); + const second = pullRequest(2); + pages.set(origin, [[first], [second]]); + + const firstPage = await model.getPullRequestsForQuery(folderManager, false, query); + const secondPage = await model.getPullRequestsForQuery(folderManager, true, query); + const cached = await model.getPullRequestsForQuery(folderManager, false, query); + + assert.deepStrictEqual(firstPage.items, [first]); + assert.deepStrictEqual(secondPage.items, [first, second]); + assert.strictEqual(cached, secondPage); + assert.strictEqual(firstPage.paginationProgress, 1); + assert.strictEqual(secondPage.paginationProgress, 2); + }); + + it('advances through empty filtered pages without mistaking them for stalled pagination', async function () { + const pr = pullRequest(1); + pages.set(origin, [[], [], [pr]]); + model.copilotStateModel.setInitialized(); + boundedQueries(3); + + await model.refreshCopilotStateChanges(true); + + assert.deepStrictEqual(fetchedPages(), [1, 2, 3]); + sinon.assert.calledOnce(timeline); + assert.strictEqual(model.copilotStateModel.all[0].item, pr); + }); + + for (const userConfiguredRemotes of [false, true]) { + it(`drains earlier remotes when the last remote has no more pages with configuredRemotes=${userConfiguredRemotes}`, async function () { + const upstream = await addRemote('upstream', 'other'); + const prs = [pullRequest(1), pullRequest(2), pullRequest(3, upstream)]; + pages.set(origin, [[prs[0]], [prs[1]]]); + pages.set(upstream, [[prs[2]]]); + if (userConfiguredRemotes) { + inspect.returns({ globalValue: ['origin', 'upstream'] }); + } + + await model.refreshCopilotStateChanges(true); + + assert.deepStrictEqual(fetchPages.getCalls().map(call => [call.args[0].remote.remoteName, call.args[2]]), + [['origin', 1], ['upstream', 1], ['origin', 2]]); + assert.strictEqual(timeline.callCount, 3); + assert.strictEqual(model.copilotStateModel.all.length, 3); + }); + } + + it('continues to an unsearched remote when a cached first remote is terminal', async function () { + const upstream = await addRemote('upstream', 'other'); + const first = pullRequest(1); + const second = pullRequest(2, upstream); + pages.set(origin, [[first]]); + pages.set(upstream, [[second]]); + const cached = await model.getPullRequestsForQuery(folderManager, false, query); + assert.strictEqual(cached.hasMorePages, false); + assert.strictEqual(cached.hasUnsearchedRepositories, true); + model.copilotStateModel.setInitialized(); + + await model.refreshCopilotStateChanges(true); + + assert.deepStrictEqual(fetchedPages(), [1, 1]); + assert.deepStrictEqual(timeline.getCalls().map(call => call.thisValue), [first, second]); + }); + + it('processes duplicate pull requests from multiple worktrees only once', async function () { + const pr = pullRequest(1); + pages.set(origin, [[pr, pr]]); + const worktree = new MockRepository(); + worktree.rootUri = vscode.Uri.file('C:\\users\\test\\worktree'); + const worktreeManager = new FolderRepositoryManager(1, context, worktree, telemetry, new GitApiImpl(reposManager), + credentialStore, new CreatePullRequestHelper(), new MockThemeWatcher()); + sinon.stub(worktreeManager, 'getPullRequestDefaults').resolves({ owner: 'octo', repo: 'repo', base: 'main' }); + sinon.stub(worktreeManager, 'getPullRequests').resolves({ + items: [pr], hasMorePages: false, hasUnsearchedRepositories: false, paginationProgress: 1, + }); + reposManager.insertFolderManager(worktreeManager); + + await model.refreshCopilotStateChanges(true); + + sinon.assert.calledOnce(timeline); + assert.strictEqual(model.copilotStateModel.all.length, 1); + }); + + for (const progress of [undefined, 1]) { + it(`rejects non-advancing results without changing existing state when progress=${progress}`, async function () { + const pr = pullRequest(1); + model.copilotStateModel.set([{ item: pr, status: CopilotPRStatus.Started }]); + model.copilotStateModel.setInitialized(); + const response: ItemsResponseResult = { + items: [pr], hasMorePages: true, hasUnsearchedRepositories: false, paginationProgress: progress, + }; + const queries = sinon.stub(model, 'getPullRequestsForQuery').callsFake(async () => { + assert.ok(queries.callCount <= 2, 'Pagination must stop after detecting stalled progress'); + return progress === undefined ? response : { ...response }; + }); + + await assert.rejects(model.refreshCopilotStateChanges(true), /pagination did not advance/); + + sinon.assert.calledTwice(queries); + sinon.assert.notCalled(timeline); + assert.strictEqual(model.copilotStateModel.get('octo', 'repo', 1), CopilotPRStatus.Started); + queries.restore(); + pages.set(origin, [[pr]]); + assert.strictEqual(await model.refreshCopilotStateChanges(true), true); + }); + } + + it('restores reached pages after a failed request instead of skipping the failed page on retry', async function () { + const prs = [pullRequest(1), pullRequest(2), pullRequest(3)]; + pages.set(origin, prs.map(pr => [pr])); + boundedQueries(4); + model.copilotStateModel.set([{ item: prs[2], status: CopilotPRStatus.Started }]); + model.copilotStateModel.setInitialized(); + const error = new Error('Temporary request failure'); + fetchPages.onSecondCall().rejects(error); + + await assert.rejects(model.refreshCopilotStateChanges(true), error); + sinon.assert.notCalled(timeline); + assert.strictEqual(model.copilotStateModel.get('octo', 'repo', 3), CopilotPRStatus.Started); + + assert.strictEqual(await model.refreshCopilotStateChanges(true), true); + assert.deepStrictEqual(fetchedPages(), [1, 2, 1, 2, 3]); + assert.deepStrictEqual(timeline.getCalls().map(call => call.thisValue), prs); + }); + + for (const fetchOnePagePerRepo of [false, true]) { + it(`preserves other remotes on sidebar recovery after a failed page with fetchOnePagePerRepo=${fetchOnePagePerRepo}`, async function () { + const upstream = await addRemote('upstream', 'other'); + const first = pullRequest(1); + const second = pullRequest(2); + const third = pullRequest(3, upstream); + pages.set(origin, [[first]]); + pages.set(upstream, [[third]]); + + let response = await model.getPullRequestsForQuery(folderManager, false, query, fetchOnePagePerRepo); + if (response.hasUnsearchedRepositories) { + response = await model.getPullRequestsForQuery(folderManager, true, query); + } + assert.deepStrictEqual(response.items, [first, third]); + + pages.set(origin, [[first], [second]]); + maxKnownPR.resolves(101); + model.clearCopilotCaches(); + response = await model.getPullRequestsForQuery(folderManager, false, query, fetchOnePagePerRepo); + assert.deepStrictEqual(response.items, [first, third]); + assert.strictEqual(response.hasMorePages, true); + + const error = new Error('Temporary request failure'); + fetchPages.onCall(fetchPages.callCount).rejects(error); + await assert.rejects(model.getPullRequestsForQuery(folderManager, true, query), error); + fetchPages.resetHistory(); + + const restored = await model.getPullRequestsForQuery(folderManager, false, query); + assert.deepStrictEqual(fetchPages.getCalls().map(call => [call.args[0].remote.remoteName, call.args[2]]), + [['origin', 1], ['origin', 2], ['upstream', 1]]); + assert.deepStrictEqual(restored.items, [first, second, third]); + assert.strictEqual(restored.hasMorePages, false); + assert.strictEqual(restored.hasUnsearchedRepositories, false); + assert.strictEqual(restored.paginationProgress, 3); + assert.strictEqual(await model.getPullRequestsForQuery(folderManager, false, query), restored); + }); + } + + describe('watcher recovery', function () { + let clock: SinonFakeTimers; + let focusedWindow: boolean; + let setTimeoutSpy: SinonSpy; + + async function advanceClock(milliseconds: number = 0): Promise { + clock.tick(milliseconds); + await new Promise(resolve => setImmediate(resolve)); + } + + function scheduledPolls(): number { + return setTimeoutSpy.getCalls().filter(call => call.args[1] >= 2 * 60 * 1000).length; + } + + beforeEach(function () { + clock = sinon.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout', 'Date'] }); + setTimeoutSpy = sinon.spy(global, 'setTimeout'); + focusedWindow = true; + sinon.stub(vscode.window, 'state').get(() => ({ active: focusedWindow, focused: focusedWindow })); + }); + + for (const focused of [false, true]) { + it(`logs failures and retries at the normal interval when focused=${focused}`, async function () { + focusedWindow = focused; + const error = new Error('Temporary polling failure'); + const refresh = sinon.stub(model, 'refreshCopilotStateChanges').resolves(true); + refresh.onFirstCall().rejects(error); + const log = sinon.stub(Logger, 'error'); + watcher = new CopilotPRWatcher(reposManager, model); + + await advanceClock(); + sinon.assert.calledOnce(refresh); + sinon.assert.calledWithExactly(log, 'Refreshing Copilot pull request state failed: Temporary polling failure', 'CopilotPRWatcher'); + assert.strictEqual(scheduledPolls(), 1); + + await advanceClock((focused ? 2 : 5) * 60 * 1000); + sinon.assert.calledTwice(refresh); + assert.strictEqual(scheduledPolls(), 2); + }); + } + + it('does not schedule more polling when refresh returns false', async function () { + const refresh = sinon.stub(model, 'refreshCopilotStateChanges').resolves(false); + watcher = new CopilotPRWatcher(reposManager, model); + + await advanceClock(); + + sinon.assert.calledOnce(refresh); + assert.strictEqual(scheduledPolls(), 0); + }); + + it('does not start polling in development mode', async function () { + settings.set(DEV_MODE, true); + const refresh = sinon.stub(model, 'refreshCopilotStateChanges'); + watcher = new CopilotPRWatcher(reposManager, model); + + await advanceClock(); + + sinon.assert.notCalled(refresh); + assert.strictEqual(scheduledPolls(), 0); + }); + + it('does not restart polling after disposal while a refresh is pending', async function () { + let resolve: (value: boolean) => void; + const pending = new Promise(r => resolve = r); + sinon.stub(model, 'refreshCopilotStateChanges').returns(pending); + watcher = new CopilotPRWatcher(reposManager, model); + watcher.dispose(); + resolve!(true); + + await advanceClock(); + + assert.strictEqual(scheduledPolls(), 0); + }); + + it('cancels a scheduled polling timer on disposal', async function () { + const refresh = sinon.stub(model, 'refreshCopilotStateChanges').resolves(true); + watcher = new CopilotPRWatcher(reposManager, model); + await advanceClock(); + watcher.dispose(); + + await advanceClock(5 * 60 * 1000); + + sinon.assert.calledOnce(refresh); + assert.strictEqual(scheduledPolls(), 1); + }); + + it('schedules only one timer when concurrent polls share a pending refresh', async function () { + const configurationChanges = new vscode.EventEmitter(); + context.subscriptions.push(configurationChanges); + sinon.stub(vscode.workspace, 'onDidChangeConfiguration').callsFake(configurationChanges.event); + let resolve: (value: boolean) => void; + const pending = new Promise(r => resolve = r); + const refresh = sinon.stub(model, 'refreshCopilotStateChanges').returns(pending); + watcher = new CopilotPRWatcher(reposManager, model); + configurationChanges.fire({ affectsConfiguration: key => key === `${PR_SETTINGS_NAMESPACE}.${QUERIES}` }); + configurationChanges.fire({ affectsConfiguration: key => key === `${PR_SETTINGS_NAMESPACE}.${QUERIES}` }); + resolve!(true); + + await advanceClock(); + + sinon.assert.calledThrice(refresh); + assert.strictEqual(scheduledPolls(), 1); + }); + + it('logs failures from debounced refreshes without losing the polling timer', async function () { + const changes = new vscode.EventEmitter(); + context.subscriptions.push(changes); + sinon.stub(reposManager, 'onDidChangeAnyPullRequests').callsFake(changes.event); + const refresh = sinon.stub(model, 'refreshCopilotStateChanges').resolves(true); + refresh.onSecondCall().rejects(new Error('Debounced refresh failed')); + const log = sinon.stub(Logger, 'error'); + model.copilotStateModel.setInitialized(); + watcher = new CopilotPRWatcher(reposManager, model); + await advanceClock(); + changes.fire([{ model: pullRequest(1), event: {} }]); + + await advanceClock(50); + + sinon.assert.calledTwice(refresh); + sinon.assert.calledWithExactly(log, 'Refreshing Copilot pull request state failed: Debounced refresh failed', 'CopilotPRWatcher'); + assert.strictEqual(scheduledPolls(), 1); + }); + }); +}); diff --git a/src/view/prsTreeModel.ts b/src/view/prsTreeModel.ts index 82a052a6e1..28818b0c70 100644 --- a/src/view/prsTreeModel.ts +++ b/src/view/prsTreeModel.ts @@ -370,6 +370,7 @@ export class PrsTreeModel extends Disposable { return repo.getMaxPullRequest(); } + // Returns the cumulative query results, including all previously fetched pages. async getPullRequestsForQuery(folderRepoManager: FolderRepositoryManager, fetchNextPage: boolean, query: string, fetchOnePagePerRepo: boolean = false): Promise> { let release: () => void; const lock = new Promise(resolve => { release = resolve; }); @@ -416,6 +417,10 @@ export class PrsTreeModel extends Disposable { this._getChecks(prs.items); this.hasLoaded = true; return prs; + } catch (error) { + // Restore all reached pages on retry, including any page whose request failed. + this.getFolderCache(folderRepoManager).delete(query); + throw error; } finally { release!(); } @@ -565,30 +570,41 @@ export class PrsTreeModel extends Disposable { } const changes: CodingAgentPRAndStatus[] = []; + const pullRequests = new Map(); for (const folderManager of this._reposManager.folderManagers) { initialized++; - const items: PullRequestModel[] = []; - let hasMore = true; + let response: ItemsResponseResult; + let previousResponse: ItemsResponseResult | undefined; + let fetchNextPage = false; do { - const prs = await this.getPullRequestsForQuery(folderManager, !this.copilotStateModel.isInitialized, copilotQuery, true); - items.push(...prs.items); - hasMore = prs.hasMorePages; - } while (hasMore); - - for (const pr of items) { - unseenKeys.delete(this.copilotStateModel.makeKey(pr.remote.owner, pr.remote.repositoryName, pr.number)); - const copilotEvents = await pr.getCopilotTimelineEvents(false, !this.copilotStateModel.isInitialized); - let latestEvent = copilotEventToStatus(copilotEvents[copilotEvents.length - 1]); - if (latestEvent === CopilotPRStatus.None) { - if (!COPILOT_ACCOUNTS[pr.author.login]) { - continue; - } - latestEvent = CopilotPRStatus.Started; + response = await this.getPullRequestsForQuery(folderManager, fetchNextPage, copilotQuery, true); + if (previousResponse && (response.hasMorePages || response.hasUnsearchedRepositories) + && (response === previousResponse + || (response.paginationProgress !== undefined && previousResponse.paginationProgress !== undefined + && response.paginationProgress <= previousResponse.paginationProgress))) { + throw new Error('Copilot pull request pagination did not advance.'); } - const lastStatus = this.copilotStateModel.get(pr.remote.owner, pr.remote.repositoryName, pr.number) ?? CopilotPRStatus.None; - if (latestEvent !== lastStatus) { - changes.push({ item: pr, status: latestEvent }); + previousResponse = response; + fetchNextPage = true; + } while (response.hasMorePages || response.hasUnsearchedRepositories); + + for (const pr of response.items) { + pullRequests.set(this.copilotStateModel.makeKey(pr.remote.owner, pr.remote.repositoryName, pr.number), pr); + } + } + for (const [key, pr] of pullRequests) { + unseenKeys.delete(key); + const copilotEvents = await pr.getCopilotTimelineEvents(false, !this.copilotStateModel.isInitialized); + let latestEvent = copilotEventToStatus(copilotEvents[copilotEvents.length - 1]); + if (latestEvent === CopilotPRStatus.None) { + if (!COPILOT_ACCOUNTS[pr.author.login]) { + continue; } + latestEvent = CopilotPRStatus.Started; + } + const lastStatus = this.copilotStateModel.get(pr.remote.owner, pr.remote.repositoryName, pr.number) ?? CopilotPRStatus.None; + if (latestEvent !== lastStatus) { + changes.push({ item: pr, status: latestEvent }); } } for (const key of unseenKeys) {