From 509e8236e48852ae5348e9dd73ce14d3be9ff00d Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:29:09 +0200 Subject: [PATCH 1/2] Fix refresh-to-empty bug and reduce refreshes --- src/test/view/prsTree.test.ts | 259 +++++++++++++++++++++++++- src/view/prsTreeDataProvider.ts | 5 +- src/view/prsTreeModel.ts | 55 +++--- src/view/treeNodes/pullRequestNode.ts | 14 -- 4 files changed, 291 insertions(+), 42 deletions(-) diff --git a/src/test/view/prsTree.test.ts b/src/test/view/prsTree.test.ts index a1e834bbe4..9263623851 100644 --- a/src/test/view/prsTree.test.ts +++ b/src/test/view/prsTree.test.ts @@ -4,14 +4,14 @@ *--------------------------------------------------------------------------------------------*/ import * as vscode from 'vscode'; -import { SinonSandbox, SinonStub, createSandbox } from 'sinon'; +import { SinonSandbox, SinonSpy, SinonStub, createSandbox } from 'sinon'; import { default as assert } from 'assert'; import { Octokit } from '@octokit/rest'; import { ApolloClient, ApolloLink, InMemoryCache } from 'apollo-boost'; import { getEnterpriseAuthenticationMessage, PullRequestsTreeDataProvider } from '../../view/prsTreeDataProvider'; import { NotificationsManager } from '../../notifications/notificationsManager'; -import { FolderRepositoryManager, ReposManagerState } from '../../github/folderRepositoryManager'; +import { FolderRepositoryManager, ItemsResponseResult, ReposManagerState } from '../../github/folderRepositoryManager'; import { MockTelemetry } from '../mocks/mockTelemetry'; import { MockNotificationManager } from '../mocks/mockNotificationManager'; @@ -26,6 +26,7 @@ import { PullRequestOverviewPanel } from '../../github/pullRequestOverview'; import { convertRESTPullRequestToRawPullRequest, parseGraphQLPullRequest } from '../../github/utils'; import { PullRequestBuilder } from '../builders/rest/pullRequestBuilder'; import { PRNode } from '../../view/treeNodes/pullRequestNode'; +import { CategoryTreeNode } from '../../view/treeNodes/categoryNode'; import { GitHubRemote } from '../../common/remote'; import { Protocol } from '../../common/protocol'; import { CredentialStore, GitHub } from '../../github/credentials'; @@ -35,7 +36,7 @@ import { LoggingApolloClient, LoggingOctokit, RateLogger } from '../../github/lo import { AuthProvider, GitHubServerType } from '../../common/authentication'; import * as configuration from '../../authentication/configuration'; import { DataUri } from '../../common/uri'; -import { GithubItemStateEnum, IAccount, ITeam, PullRequestMergeability } from '../../github/interface'; +import { GithubItemStateEnum, IAccount, ITeam, PRType, PullRequestMergeability } from '../../github/interface'; import { asPromise } from '../../common/utils'; import { CreatePullRequestHelper } from '../../view/createPullRequestHelper'; import { MockThemeWatcher } from '../mocks/mockThemeWatcher'; @@ -109,7 +110,8 @@ describe('GitHub Pull Requests view', function () { function stackablePullRequest(repository: MockGitHubRepository, number: number, base: string, head: string): PullRequestModel { const remote = repository.remote; - const rest = new PullRequestBuilder().number(number) + const rest = new PullRequestBuilder().number(number).id(number) + .html_url(`https://github.com/${remote.owner}/${remote.repositoryName}/pull/${number}`) .base(ref => ref.ref(base)).head(ref => ref.ref(head)).build(); for (const ref of [rest.base, rest.head]) { ref.repo.owner.login = remote.owner; @@ -549,6 +551,255 @@ describe('GitHub Pull Requests view', function () { ); }); + describe('All Open', function () { + let folderManager: FolderRepositoryManager; + let pullRequest: PullRequestModel; + let nextPullRequest: PullRequestModel; + let getPullRequests: SinonStub; + + beforeEach(function () { + const repository = new MockRepository(); + folderManager = new FolderRepositoryManager(0, context, repository, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher); + reposManager.insertFolderManager(folderManager); + const url = 'https://github.com/aaa/bbb'; + const remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom); + const githubRepository = new MockGitHubRepository(remote, credentialStore, telemetry, sinon); + discoveredRepository = githubRepository; + pullRequest = stackablePullRequest(githubRepository, 1, 'main', 'feature'); + nextPullRequest = stackablePullRequest(githubRepository, 2, 'main', 'next-feature'); + sinon.stub(pullRequest, 'getStatusChecks').resolves([null, null]); + sinon.stub(nextPullRequest, 'getStatusChecks').resolves([null, null]); + getPullRequests = sinon.stub(folderManager, 'getPullRequests'); + }); + + it('serializes overlapping requests and reuses the cached result', async function () { + let resolveFetch!: (result: ItemsResponseResult) => void; + const pendingFetch = new Promise>(resolve => { resolveFetch = resolve; }); + let markStarted!: () => void; + const started = new Promise(resolve => { markStarted = resolve; }); + getPullRequests.onFirstCall().callsFake(() => { + markStarted(); + return pendingFetch; + }); + getPullRequests.onSecondCall().resolves({ + items: [], + hasMorePages: false, + hasUnsearchedRepositories: false, + }); + + const first = prsTreeModel.getAllPullRequests(folderManager, false); + await started; + const overlapping = prsTreeModel.getAllPullRequests(folderManager, false); + await new Promise(resolve => setImmediate(resolve)); + assert.strictEqual(getPullRequests.callCount, 1); + const result: ItemsResponseResult = { + items: [pullRequest], + hasMorePages: false, + hasUnsearchedRepositories: false, + }; + resolveFetch(result); + + const [firstResult, overlappingResult] = await Promise.all([first, overlapping]); + assert.strictEqual(firstResult, result); + assert.strictEqual(overlappingResult, result); + assert.strictEqual(await prsTreeModel.getAllPullRequests(folderManager, false), result); + assert.strictEqual(getPullRequests.callCount, 1); + }); + + it('releases the lock after a failed fetch so a queued request can succeed', async function () { + let rejectFetch!: (error: Error) => void; + const pendingFetch = new Promise>((_, reject) => { rejectFetch = reject; }); + let markStarted!: () => void; + const started = new Promise(resolve => { markStarted = resolve; }); + getPullRequests.onFirstCall().callsFake(() => { + markStarted(); + return pendingFetch; + }); + const result: ItemsResponseResult = { + items: [pullRequest], + hasMorePages: false, + hasUnsearchedRepositories: false, + }; + getPullRequests.onSecondCall().resolves(result); + + const error = new Error('Fetching pull requests failed'); + const failed = assert.rejects(prsTreeModel.getAllPullRequests(folderManager, false), error); + await started; + const retry = prsTreeModel.getAllPullRequests(folderManager, false); + await new Promise(resolve => setImmediate(resolve)); + assert.strictEqual(getPullRequests.callCount, 1); + rejectFetch(error); + + await failed; + assert.strictEqual(await retry, result); + assert.strictEqual(await prsTreeModel.getAllPullRequests(folderManager, false), result); + assert.strictEqual(getPullRequests.callCount, 2); + }); + + for (const loadMoreFirst of [true, false]) { + it(loadMoreFirst ? 'preserves results when refreshing during load more' : 'preserves results when loading more during a refresh', async function () { + getPullRequests.onFirstCall().resolves({ + items: [pullRequest], + hasMorePages: true, + hasUnsearchedRepositories: false, + }); + await prsTreeModel.getAllPullRequests(folderManager, false); + + let resolveFetch!: (result: ItemsResponseResult) => void; + const pendingFetch = new Promise>(resolve => { resolveFetch = resolve; }); + let markStarted!: () => void; + const started = new Promise(resolve => { markStarted = resolve; }); + getPullRequests.onSecondCall().callsFake(() => { + markStarted(); + return pendingFetch; + }); + getPullRequests.onThirdCall().resolves({ + items: loadMoreFirst ? [pullRequest, nextPullRequest] : [nextPullRequest], + hasMorePages: false, + hasUnsearchedRepositories: true, + totalCount: 2, + }); + + const first = prsTreeModel.getAllPullRequests(folderManager, loadMoreFirst, !loadMoreFirst); + await started; + const overlapping = prsTreeModel.getAllPullRequests(folderManager, !loadMoreFirst, loadMoreFirst); + await new Promise(resolve => setImmediate(resolve)); + assert.strictEqual(getPullRequests.callCount, 2); + resolveFetch({ + items: loadMoreFirst ? [nextPullRequest] : [pullRequest], + hasMorePages: !loadMoreFirst, + hasUnsearchedRepositories: loadMoreFirst, + totalCount: 2, + }); + + const [firstResult, overlappingResult] = await Promise.all([first, overlapping]); + assert.deepStrictEqual(firstResult, { + items: loadMoreFirst ? [pullRequest, nextPullRequest] : [pullRequest], + hasMorePages: !loadMoreFirst, + hasUnsearchedRepositories: loadMoreFirst, + totalCount: 2, + }); + assert.deepStrictEqual(overlappingResult, { + items: [pullRequest, nextPullRequest], + hasMorePages: false, + hasUnsearchedRepositories: true, + totalCount: 2, + }); + assert.strictEqual(await prsTreeModel.getAllPullRequests(folderManager, false), overlappingResult); + assert.deepStrictEqual(getPullRequests.getCalls().map(call => call.args), [ + [PRType.All, { fetchNextPage: false }], + [PRType.All, { fetchNextPage: loadMoreFirst }], + [PRType.All, { fetchNextPage: !loadMoreFirst }], + ]); + }); + } + + describe('Refresh notifications', function () { + let configurationChanged: vscode.EventEmitter; + let onDidChangeTreeData: SinonSpy; + let allCategory: CategoryTreeNode; + let localCategory: CategoryTreeNode; + + beforeEach(async function () { + configurationChanged = new vscode.EventEmitter(); + context.subscriptions.push(configurationChanged); + sinon.stub(vscode.workspace, 'onDidChangeConfiguration').callsFake(configurationChanged.event); + provider.dispose(); + provider = new PullRequestsTreeDataProvider(prsTreeModel, telemetry, context, reposManager); + sinon.stub(credentialStore, 'isAuthenticated').returns(true); + sinon.stub(reposManager, 'state').get(() => ReposManagerState.RepositoriesLoaded); + const githubRepository = discoveredRepository; + assert(githubRepository); + sinon.stub(folderManager, 'gitHubRepositories').get(() => [githubRepository]); + getPullRequests.resolves({ + items: [pullRequest, nextPullRequest], + hasMorePages: false, + hasUnsearchedRepositories: false, + }); + sinon.stub(folderManager, 'getLocalPullRequests').resolves([pullRequest, nextPullRequest]); + await prsTreeModel.getAllPullRequests(folderManager, false); + provider.initialize([], mockNotificationsManager as NotificationsManager); + const categories = await provider.getChildren(); + const all = categories.find(node => node instanceof CategoryTreeNode && node.type === PRType.All); + const local = categories.find(node => node instanceof CategoryTreeNode && node.type === PRType.LocalPullRequest); + assert(all instanceof CategoryTreeNode); + assert(local instanceof CategoryTreeNode); + allCategory = all; + localCategory = local; + context.subscriptions.push(...await allCategory.getChildren(), ...await localCategory.getChildren()); + await githubRepository.ensureCommentsController(); + onDidChangeTreeData = sinon.spy(); + context.subscriptions.push(provider.onDidChangeTreeData(onDidChangeTreeData)); + }); + + it('refreshes once when the manual Refresh command clears the cache', async function () { + const forceClearCache = sinon.spy(prsTreeModel, 'forceClearCache'); + const refreshCommand = (vscode.commands.registerCommand as SinonStub).getCalls() + .filter(call => call.args[0] === 'pr.refreshList').pop(); + assert(refreshCommand); + getPullRequests.resetHistory(); + + refreshCommand.args[1](); + + assert.strictEqual(forceClearCache.callCount, 1); + assert.strictEqual(onDidChangeTreeData.callCount, 1); + assert.deepStrictEqual(onDidChangeTreeData.firstCall.args, [provider.children]); + await prsTreeModel.getAllPullRequests(folderManager, false); + assert.strictEqual(getPullRequests.callCount, 1); + }); + + for (const setting of ['githubPullRequests.showPullRequestNumberInTree', 'githubPullRequests.pullRequestAvatarDisplay']) { + it(`refreshes once for ${setting}, regardless of the number of PR nodes`, function () { + const clearCache = sinon.spy(prsTreeModel, 'clearCache'); + + configurationChanged.fire({ affectsConfiguration: section => section === setting }); + + assert.strictEqual(onDidChangeTreeData.callCount, 1); + assert.deepStrictEqual(onDidChangeTreeData.firstCall.args, [undefined]); + assert.strictEqual(clearCache.callCount, 0); + }); + } + + it('does not refresh for unrelated settings', function () { + configurationChanged.fire({ affectsConfiguration: section => section === 'editor.fontSize' }); + + assert.strictEqual(onDidChangeTreeData.callCount, 0); + }); + + it('refreshes all affected PR copies in one event when switching checkouts', function () { + const copiesOf = (model: PullRequestModel) => [allCategory, localCategory] + .flatMap(category => category.children ?? []) + .filter((node): node is PRNode => node instanceof PRNode && node.pullRequestModel.equals(model)); + const oldCopies = copiesOf(pullRequest); + const newCopies = copiesOf(nextPullRequest); + assert.strictEqual(oldCopies.length, 2); + assert.strictEqual(newCopies.length, 2); + const assertRefreshed = (expected: PRNode[]) => { + assert.strictEqual(onDidChangeTreeData.callCount, 1); + const refreshed = onDidChangeTreeData.firstCall.args[0]; + assert(Array.isArray(refreshed)); + assert.strictEqual(refreshed.length, expected.length); + assert(expected.every(node => refreshed.includes(node))); + }; + + folderManager.activePullRequest = pullRequest; + assertRefreshed(oldCopies); + onDidChangeTreeData.resetHistory(); + + folderManager.activePullRequest = nextPullRequest; + assertRefreshed([...oldCopies, ...newCopies]); + assert.strictEqual(pullRequest.isActive, false); + assert.strictEqual(nextPullRequest.isActive, true); + onDidChangeTreeData.resetHistory(); + + folderManager.activePullRequest = nextPullRequest; + assert.strictEqual(onDidChangeTreeData.callCount, 0); + folderManager.activePullRequest = undefined; + assertRefreshed(newCopies); + }); + }); + }); + it('clears the tree immediately', async function () { const repository = new MockRepository(); await repository.addRemote('origin', 'git@github.com:aaa/bbb'); diff --git a/src/view/prsTreeDataProvider.ts b/src/view/prsTreeDataProvider.ts index 03ded633b6..b6b53da5b3 100644 --- a/src/view/prsTreeDataProvider.ts +++ b/src/view/prsTreeDataProvider.ts @@ -14,7 +14,7 @@ import { commands, contexts } from '../common/executeCommands'; import { Disposable } from '../common/lifecycle'; import Logger from '../common/logger'; import { Remote } from '../common/remote'; -import { EXPERIMENTAL_STACKS, FILE_LIST_LAYOUT, GITHUB_ENTERPRISE, PR_SETTINGS_NAMESPACE, QUERIES, REMOTES, URI, URIS } from '../common/settingKeys'; +import { EXPERIMENTAL_STACKS, FILE_LIST_LAYOUT, GITHUB_ENTERPRISE, PR_SETTINGS_NAMESPACE, PULL_REQUEST_AVATAR_DISPLAY, QUERIES, REMOTES, SHOW_PULL_REQUEST_NUMBER_IN_TREE, URI, URIS } from '../common/settingKeys'; import { areStacksEnabled, assertStacksEnabled } from '../common/settingsUtils'; import { ITelemetry } from '../common/telemetry'; import { createPRNodeIdentifier } from '../common/uri'; @@ -121,7 +121,6 @@ export class PullRequestsTreeDataProvider extends Disposable implements vscode.T })); this._register(vscode.commands.registerCommand('pr.refreshList', _ => { this.prsTreeModel.forceClearCache(); - this.refreshAllQueryResults(true); })); this._register(vscode.commands.registerCommand('pr.loadMore', (node: CategoryTreeNode) => { @@ -225,6 +224,8 @@ export class PullRequestsTreeDataProvider extends Disposable implements vscode.T this._register(vscode.workspace.onDidChangeConfiguration(e => { if (e.affectsConfiguration(`${PR_SETTINGS_NAMESPACE}.${FILE_LIST_LAYOUT}`) + || e.affectsConfiguration(`${PR_SETTINGS_NAMESPACE}.${SHOW_PULL_REQUEST_NUMBER_IN_TREE}`) + || e.affectsConfiguration(`${PR_SETTINGS_NAMESPACE}.${PULL_REQUEST_AVATAR_DISPLAY}`) || e.affectsConfiguration(`${GITHUB_ENTERPRISE}.${URIS}`) || e.affectsConfiguration(`${GITHUB_ENTERPRISE}.${URI}`)) { this.refreshAll(); diff --git a/src/view/prsTreeModel.ts b/src/view/prsTreeModel.ts index 29e4937183..98cdba7b98 100644 --- a/src/view/prsTreeModel.ts +++ b/src/view/prsTreeModel.ts @@ -58,6 +58,7 @@ export class PrsTreeModel extends Disposable { private readonly _repoEvents: Map = new Map(); private _getPullRequestsForQueryLock: Promise = Promise.resolve(); + private _getAllPullRequestsLock: Promise = Promise.resolve(); private _sentNoRepoTelemetry: boolean = false; public readonly copilotStateModel: CopilotStateModel; @@ -418,30 +419,40 @@ export class PrsTreeModel extends Disposable { } async getAllPullRequests(folderRepoManager: FolderRepositoryManager, fetchNextPage: boolean, update?: boolean): Promise> { - const cache = this.getFolderCache(folderRepoManager); - const allCache = cache.get(PRType.All); - if (!update && allCache && !allCache.clearRequested && !fetchNextPage) { - return allCache.items; - } + let release: () => void; + const lock = new Promise(resolve => { release = resolve; }); + const prev = this._getAllPullRequestsLock; + this._getAllPullRequestsLock = prev.then(() => lock); + await prev; - const prs = await folderRepoManager.getPullRequests( - PRType.All, - { fetchNextPage } - ); - if (fetchNextPage) { - prs.items = allCache?.items.items.concat(prs.items) ?? prs.items; - } - cache.set(PRType.All, { clearRequested: false, items: prs, maxKnownPR: undefined }); - prs.items.forEach(pr => this._allCachedPRs.add(pr)); + try { + const cache = this.getFolderCache(folderRepoManager); + const allCache = cache.get(PRType.All); + if (!update && allCache && !allCache.clearRequested && !fetchNextPage) { + return allCache.items; + } - /* __GDPR__ - "pr.expand.all" : {} - */ - this._telemetry.sendTelemetryEvent('pr.expand.all'); - // Don't await this._getChecks. It fires an event that will be listened to. - this._getChecks(prs.items); - this.hasLoaded = true; - return prs; + const prs = await folderRepoManager.getPullRequests( + PRType.All, + { fetchNextPage } + ); + if (fetchNextPage) { + prs.items = allCache?.items.items.concat(prs.items) ?? prs.items; + } + cache.set(PRType.All, { clearRequested: false, items: prs, maxKnownPR: undefined }); + prs.items.forEach(pr => this._allCachedPRs.add(pr)); + + /* __GDPR__ + "pr.expand.all" : {} + */ + this._telemetry.sendTelemetryEvent('pr.expand.all'); + // Don't await this._getChecks. It fires an event that will be listened to. + this._getChecks(prs.items); + this.hasLoaded = true; + return prs; + } finally { + release!(); + } } private forceClearQueriesContainingPullRequests(pullRequests: PullRequestChangeEvent[]): void { diff --git a/src/view/treeNodes/pullRequestNode.ts b/src/view/treeNodes/pullRequestNode.ts index 5c0a9f3e2f..c5c4a932c9 100644 --- a/src/view/treeNodes/pullRequestNode.ts +++ b/src/view/treeNodes/pullRequestNode.ts @@ -58,12 +58,6 @@ export class PRNode extends TreeNode implements vscode.CommentingRangeProvider2 ) { super(parent); this.registerSinceReviewChange(); - this.registerConfigurationChange(); - this._register(this._folderReposManager.onDidChangeActivePullRequest(e => { - if (e.new?.number === this.pullRequestModel.number || e.old?.number === this.pullRequestModel.number) { - this.refresh(this); - } - })); this._register(this._folderReposManager.themeWatcher.onDidChangeTheme(() => { this.refresh(this); })); @@ -141,14 +135,6 @@ export class PRNode extends TreeNode implements vscode.CommentingRangeProvider2 })); } - protected registerConfigurationChange() { - this._register(vscode.workspace.onDidChangeConfiguration(e => { - if (e.affectsConfiguration(`${PR_SETTINGS_NAMESPACE}.${SHOW_PULL_REQUEST_NUMBER_IN_TREE}`) || e.affectsConfiguration(`${PR_SETTINGS_NAMESPACE}.${PULL_REQUEST_AVATAR_DISPLAY}`)) { - this.refresh(); - } - })); - } - public async reopenNewPrDiffs(pullRequest: PullRequestModel) { let hasOpenDiff: boolean = false; vscode.window.tabGroups.all.map(tabGroup => { From 861f732da1cbed203d936fd49a9aca7628fcf40b Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:55:14 +0200 Subject: [PATCH 2/2] CCR --- src/test/view/prsTree.test.ts | 136 ++++++++++++++++++++++++++++++++++ src/view/prsTreeModel.ts | 21 ++++-- 2 files changed, 152 insertions(+), 5 deletions(-) diff --git a/src/test/view/prsTree.test.ts b/src/test/view/prsTree.test.ts index 9263623851..44cb43d9de 100644 --- a/src/test/view/prsTree.test.ts +++ b/src/test/view/prsTree.test.ts @@ -636,6 +636,101 @@ describe('GitHub Pull Requests view', function () { assert.strictEqual(getPullRequests.callCount, 2); }); + for (const cached of [false, true]) { + for (const force of [false, true]) { + it(`refetches after ${force ? 'force-clearing' : 'clearing'} ${cached ? 'an existing' : 'an initially empty'} cache during a fetch`, async function () { + const oldResult: ItemsResponseResult = { + items: [pullRequest], + hasMorePages: false, + hasUnsearchedRepositories: false, + }; + if (cached) { + getPullRequests.resolves(oldResult); + await prsTreeModel.getAllPullRequests(folderManager, false); + getPullRequests.resetHistory(); + } + + let resolveFetch!: (result: ItemsResponseResult) => void; + const pendingFetch = new Promise>(resolve => { resolveFetch = resolve; }); + let markStarted!: () => void; + const started = new Promise(resolve => { markStarted = resolve; }); + getPullRequests.onFirstCall().callsFake(() => { + markStarted(); + return pendingFetch; + }); + const freshResult: ItemsResponseResult = { + items: [nextPullRequest], + hasMorePages: false, + hasUnsearchedRepositories: false, + }; + getPullRequests.onSecondCall().resolves(freshResult); + + const first = prsTreeModel.getAllPullRequests(folderManager, false, true); + await started; + if (force) { + prsTreeModel.forceClearCache(true); + } else { + prsTreeModel.clearCache(true); + } + const refresh = prsTreeModel.getAllPullRequests(folderManager, false); + await new Promise(resolve => setImmediate(resolve)); + assert.strictEqual(getPullRequests.callCount, 1); + resolveFetch(oldResult); + + assert.strictEqual(await first, oldResult); + assert.strictEqual(await refresh, freshResult); + assert.strictEqual(await prsTreeModel.getAllPullRequests(folderManager, false), freshResult); + assert.strictEqual(getPullRequests.callCount, 2); + if (force) { + assert.strictEqual(prsTreeModel.hasPullRequest(pullRequest), false); + } + assert.strictEqual(prsTreeModel.hasPullRequest(nextPullRequest), true); + }); + } + } + + it('loads another folder independently while keeping requests in the first folder serialized', async function () { + const otherFolderManager = new FolderRepositoryManager(1, context, new MockRepository(), telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher); + context.subscriptions.push(otherFolderManager); + const otherResult: ItemsResponseResult = { + items: [nextPullRequest], + hasMorePages: false, + hasUnsearchedRepositories: false, + }; + const getOtherPullRequests = sinon.stub(otherFolderManager, 'getPullRequests').resolves(otherResult); + let resolveFetch!: (result: ItemsResponseResult) => void; + const pendingFetch = new Promise>(resolve => { resolveFetch = resolve; }); + let markStarted!: () => void; + const started = new Promise(resolve => { markStarted = resolve; }); + getPullRequests.callsFake(() => { + markStarted(); + return pendingFetch; + }); + const first = prsTreeModel.getAllPullRequests(folderManager, false); + await started; + const overlapping = prsTreeModel.getAllPullRequests(folderManager, false); + const other = prsTreeModel.getAllPullRequests(otherFolderManager, false); + const firstResult: ItemsResponseResult = { + items: [pullRequest], + hasMorePages: false, + hasUnsearchedRepositories: false, + }; + + try { + await new Promise(resolve => setImmediate(resolve)); + assert.strictEqual(getPullRequests.callCount, 1); + assert.strictEqual(getOtherPullRequests.callCount, 1); + assert.strictEqual(await other, otherResult); + assert.strictEqual(await prsTreeModel.getAllPullRequests(otherFolderManager, false), otherResult); + assert.strictEqual(getOtherPullRequests.callCount, 1); + } finally { + resolveFetch(firstResult); + await Promise.all([first, overlapping, other]); + } + assert.strictEqual(await overlapping, firstResult); + assert.strictEqual(getPullRequests.callCount, 1); + }); + for (const loadMoreFirst of [true, false]) { it(loadMoreFirst ? 'preserves results when refreshing during load more' : 'preserves results when loading more during a refresh', async function () { getPullRequests.onFirstCall().resolves({ @@ -748,6 +843,47 @@ describe('GitHub Pull Requests view', function () { assert.strictEqual(getPullRequests.callCount, 1); }); + it('loads fresh category children when manual Refresh supersedes an in-flight fetch', async function () { + getPullRequests.resetHistory(); + let resolveFetch!: (result: ItemsResponseResult) => void; + const pendingFetch = new Promise>(resolve => { resolveFetch = resolve; }); + let markStarted!: () => void; + const started = new Promise(resolve => { markStarted = resolve; }); + getPullRequests.onFirstCall().callsFake(() => { + markStarted(); + return pendingFetch; + }); + getPullRequests.onSecondCall().resolves({ + items: [nextPullRequest], + hasMorePages: false, + hasUnsearchedRepositories: false, + }); + const first = prsTreeModel.getAllPullRequests(folderManager, false, true); + await started; + const refreshCommand = (vscode.commands.registerCommand as SinonStub).getCalls() + .filter(call => call.args[0] === 'pr.refreshList').pop(); + assert(refreshCommand); + + refreshCommand.args[1](); + const refresh = allCategory.getChildren(); + resolveFetch({ + items: [pullRequest], + hasMorePages: false, + hasUnsearchedRepositories: false, + }); + await first; + const children = await refresh; + context.subscriptions.push(...children); + + assert.strictEqual(onDidChangeTreeData.callCount, 1); + assert.strictEqual(getPullRequests.callCount, 2); + assert.strictEqual(children.length, 1); + assert(children[0] instanceof PRNode); + assert.strictEqual(children[0].pullRequestModel, nextPullRequest); + assert.strictEqual(prsTreeModel.hasPullRequest(pullRequest), false); + assert.strictEqual(prsTreeModel.hasPullRequest(nextPullRequest), true); + }); + for (const setting of ['githubPullRequests.showPullRequestNumberInTree', 'githubPullRequests.pullRequestAvatarDisplay']) { it(`refreshes once for ${setting}, regardless of the number of PR nodes`, function () { const clearCache = sinon.spy(prsTreeModel, 'clearCache'); diff --git a/src/view/prsTreeModel.ts b/src/view/prsTreeModel.ts index 98cdba7b98..82a052a6e1 100644 --- a/src/view/prsTreeModel.ts +++ b/src/view/prsTreeModel.ts @@ -58,7 +58,8 @@ export class PrsTreeModel extends Disposable { private readonly _repoEvents: Map = new Map(); private _getPullRequestsForQueryLock: Promise = Promise.resolve(); - private _getAllPullRequestsLock: Promise = Promise.resolve(); + private readonly _getAllPullRequestsLocks = new WeakMap>(); + private _allPullRequestsCacheGeneration: number = 0; private _sentNoRepoTelemetry: boolean = false; public readonly copilotStateModel: CopilotStateModel; @@ -198,6 +199,7 @@ export class PrsTreeModel extends Disposable { } public forceClearCache(silent: boolean = false) { + this._allPullRequestsCacheGeneration++; this._cachedPRs.clear(); this._allCachedPRs.clear(); if (!silent) { @@ -214,6 +216,7 @@ export class PrsTreeModel extends Disposable { return; } + this._allPullRequestsCacheGeneration++; // Instead of clearing the entire cache, mark each cached query as requiring refresh. for (const queries of this._cachedPRs.values()) { for (const [, cachedPRs] of queries.entries()) { @@ -421,11 +424,13 @@ export class PrsTreeModel extends Disposable { async getAllPullRequests(folderRepoManager: FolderRepositoryManager, fetchNextPage: boolean, update?: boolean): Promise> { let release: () => void; const lock = new Promise(resolve => { release = resolve; }); - const prev = this._getAllPullRequestsLock; - this._getAllPullRequestsLock = prev.then(() => lock); + const prev = this._getAllPullRequestsLocks.get(folderRepoManager) ?? Promise.resolve(); + const queued = prev.then(() => lock); + this._getAllPullRequestsLocks.set(folderRepoManager, queued); await prev; try { + const cacheGeneration = this._allPullRequestsCacheGeneration; const cache = this.getFolderCache(folderRepoManager); const allCache = cache.get(PRType.All); if (!update && allCache && !allCache.clearRequested && !fetchNextPage) { @@ -439,8 +444,11 @@ export class PrsTreeModel extends Disposable { if (fetchNextPage) { prs.items = allCache?.items.items.concat(prs.items) ?? prs.items; } - cache.set(PRType.All, { clearRequested: false, items: prs, maxKnownPR: undefined }); - prs.items.forEach(pr => this._allCachedPRs.add(pr)); + // An invalidation during the fetch must not be undone by its response. + if (cacheGeneration === this._allPullRequestsCacheGeneration && cache.get(PRType.All) === allCache) { + cache.set(PRType.All, { clearRequested: false, items: prs, maxKnownPR: undefined }); + prs.items.forEach(pr => this._allCachedPRs.add(pr)); + } /* __GDPR__ "pr.expand.all" : {} @@ -452,6 +460,9 @@ export class PrsTreeModel extends Disposable { return prs; } finally { release!(); + if (this._getAllPullRequestsLocks.get(folderRepoManager) === queued) { + this._getAllPullRequestsLocks.delete(folderRepoManager); + } } }