diff --git a/src/extension.ts b/src/extension.ts index 42f4534a0d..6c99976cee 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -27,7 +27,9 @@ import { CopilotRemoteAgentManager } from './github/copilotRemoteAgent'; import { CredentialStore } from './github/credentials'; import { FolderRepositoryManager } from './github/folderRepositoryManager'; import { FolderRepositoryManagerResolver } from './github/folderRepositoryManagerResolver'; +import { IssueOverviewPanel } from './github/issueOverview'; import { OverviewRestorer } from './github/overviewRestorer'; +import { PullRequestOverviewPanel } from './github/pullRequestOverview'; import { RepositoriesManager } from './github/repositoriesManager'; import { registerBuiltinGitProvider, registerLiveShareGitProvider } from './gitProviders/api'; import { GitHubContactServiceProvider } from './gitProviders/GitHubContactServiceProvider'; @@ -276,10 +278,21 @@ async function init( if (e.provider.id !== AuthProvider.github && e.provider.id !== AuthProvider.githubEnterprise) { return; } + if (e.accountChanged) { + IssueOverviewPanel.clearAll(); + PullRequestOverviewPanel.clearAll(); + activePrViewCoordinator.clearForAuthChange(); + createPrHelper.clearForAuthChange(); + const reviewsCleanup = reviewsManager.clearForAuthChange(); + issueStateManager.clearForAuthChange(); + notificationsManager.clear(); + reposManager.clearForAuthChange(); + await reviewsCleanup; + } await reposManager.refreshRepositories(); await Promise.all(reviewsManager.reviewManagers.map(reviewManager => reviewManager.updateState(true))); - tree.refreshAll(true); - await issueStateManager.refreshForAuthChange(); + reviewsManager.refreshPullRequestsTree(!e.accountChanged); + await issueStateManager.refreshAfterAuthChange(); notificationsManager.refresh(); })); diff --git a/src/github/credentials.ts b/src/github/credentials.ts index 9abe709905..d5974e07e6 100644 --- a/src/github/credentials.ts +++ b/src/github/credentials.ts @@ -49,12 +49,28 @@ interface ExistingSession { scopes: string[]; } +export function hasAccountChanged(currentAccountId: string | undefined, newSession: vscode.AuthenticationSession | undefined): boolean { + return currentAccountId !== newSession?.account.id; +} + export async function findExistingSession( authProviderId: AuthProvider, getSession: AuthenticationSessionGetter = (providerId, scopes, options) => vscode.authentication.getSession(providerId, scopes, options), ): Promise { - // Establish the preferred account with the normal scopes before looking for broader sessions. - // Otherwise, a single broader session from another account can override the workspace preference. + // Establish the preferred account across all scopes before looking for its best session. + // A scope-specific lookup can otherwise fall back to the only account with those scopes. + const preferredSession = await getSession(authProviderId, [], { silent: true }); + if (preferredSession) { + const scopesInPreferenceOrder = [SCOPES_WITH_ADDITIONAL, SCOPES_OLD, SCOPES_OLDEST]; + for (const scopes of scopesInPreferenceOrder) { + const session = await getSession(authProviderId, scopes, { silent: true, account: preferredSession.account }); + if (session) { + return { session, scopes }; + } + } + return { session: preferredSession, scopes: [...preferredSession.scopes] }; + } + const scopePreferences = [ { scopes: SCOPES_OLD, broaderScopes: [SCOPES_WITH_ADDITIONAL] }, { scopes: SCOPES_OLDEST, broaderScopes: [SCOPES_WITH_ADDITIONAL, SCOPES_OLD] }, @@ -88,12 +104,18 @@ interface AuthResult { canceled: boolean; } +export interface CredentialStoreSessionsChangeEvent extends vscode.AuthenticationSessionsChangeEvent { + accountChanged: boolean; +} + export class CredentialStore extends Disposable { private static readonly ID = 'Authentication'; private _githubAPI: GitHub | undefined; private _sessionId: string | undefined; + private _accountId: string | undefined; private _githubEnterpriseAPI: GitHub | undefined; private _enterpriseSessionId: string | undefined; + private _enterpriseAccountId: string | undefined; private _isInitialized: boolean = false; private _onDidInitialize: vscode.EventEmitter = new vscode.EventEmitter(); public readonly onDidInitialize: vscode.Event = this._onDidInitialize.event; @@ -108,7 +130,7 @@ export class CredentialStore extends Disposable { // is invalidated again soon after re-auth will still trigger another prompt. private static readonly AUTH_ERROR_COOLDOWN_MS = 60_000; - private _onDidChangeSessions: vscode.EventEmitter = new vscode.EventEmitter(); + private _onDidChangeSessions: vscode.EventEmitter = new vscode.EventEmitter(); public readonly onDidChangeSessions = this._onDidChangeSessions.event; private _onDidGetSession: vscode.EventEmitter = new vscode.EventEmitter(); @@ -130,6 +152,7 @@ export class CredentialStore extends Disposable { return; } let sessionChanged = false; + let accountChanged = false; if (currentProvider) { const newSession = await this.getSession(currentProvider, { silent: true }, currentProvider === AuthProvider.github ? this._scopes : this._scopesEnterprise, false); const currentSessionId = currentProvider === AuthProvider.github ? this._sessionId : this._enterpriseSessionId; @@ -138,11 +161,15 @@ export class CredentialStore extends Disposable { } sessionChanged = true; if (currentProvider === AuthProvider.github) { + accountChanged = hasAccountChanged(this._accountId, newSession.session); this._githubAPI = undefined; this._sessionId = undefined; + this._accountId = undefined; } else { + accountChanged = hasAccountChanged(this._enterpriseAccountId, newSession.session); this._githubEnterpriseAPI = undefined; this._enterpriseSessionId = undefined; + this._enterpriseAccountId = undefined; } } const promises: Promise[] = []; @@ -158,10 +185,10 @@ export class CredentialStore extends Disposable { if (this.isAnyAuthenticated()) { this._onDidGetSession.fire(); if (sessionChanged && !this._isSamling) { - this._onDidChangeSessions.fire(e); + this._onDidChangeSessions.fire({ ...e, accountChanged }); } } else if (!this._isSamling) { - this._onDidChangeSessions.fire(e); + this._onDidChangeSessions.fire({ ...e, accountChanged }); } } @@ -196,6 +223,7 @@ export class CredentialStore extends Disposable { const github = await this.createHub(token, authProviderId); this._githubAPI = github; this._sessionId = 'environment-token'; + this._accountId = undefined; if (!this._isInitialized) { this._isInitialized = true; this._onDidInitialize.fire(); @@ -261,8 +289,10 @@ export class CredentialStore extends Disposable { if (session) { if (!isEnterprise(authProviderId)) { this._sessionId = session.id; + this._accountId = session.account.id; } else { this._enterpriseSessionId = session.id; + this._enterpriseAccountId = session.account.id; } let github: GitHub | undefined; try { @@ -413,6 +443,10 @@ export class CredentialStore extends Disposable { return this._githubEnterpriseAPI; } + public getAccountId(authProviderId: AuthProvider): string | undefined { + return isEnterprise(authProviderId) ? this._enterpriseAccountId : this._accountId; + } + public areScopesOld(authProviderId: AuthProvider): boolean { if (!isEnterprise(authProviderId)) { return !this.allScopesIncluded(this._scopes, SCOPES_OLD); diff --git a/src/github/folderRepositoryManager.ts b/src/github/folderRepositoryManager.ts index b398e27722..ae5063cc0e 100644 --- a/src/github/folderRepositoryManager.ts +++ b/src/github/folderRepositoryManager.ts @@ -181,6 +181,8 @@ enum PagedDataType { const CACHED_TEMPLATE_BODY = 'templateBody'; +type UserCacheKind = 'assignableUsers' | 'teamReviewers' | 'mentionableUsers' | 'orgProjects'; + export class FolderRepositoryManager extends Disposable { static ID = 'FolderRepositoryManager'; @@ -195,6 +197,7 @@ export class FolderRepositoryManager extends Disposable { private _teamReviewers?: { [key: string]: ITeam[] }; private _fetchAssignableUsersPromise?: Promise<{ [key: string]: IAccount[] }>; private _fetchTeamReviewersPromise?: Promise<{ [key: string]: ITeam[] }>; + private _accountCacheToken = {}; private _gitBlameCache: { [key: string]: string } = {}; private _githubManager: GitHubManager; private _repositoryPageInformation: Map = new Map(); @@ -480,6 +483,21 @@ export class FolderRepositoryManager extends Disposable { return this._state; } + clearForAuthChange(): void { + this._sessionIgnoredRemoteNames.clear(); + this._inaccessibleRepos.clear(); + this._repositoryPageInformation.clear(); + this._gitBlameCache = {}; + this._mentionableUsers = undefined; + this._fetchMentionableUsersPromise = undefined; + this._assignableUsers = undefined; + this._fetchAssignableUsersPromise = undefined; + this._teamReviewers = undefined; + this._fetchTeamReviewersPromise = undefined; + this._accountCacheToken = {}; + this._updatingRepositories = undefined; + } + private set state(state: ReposManagerState) { if (state !== this._state) { this._state = state; @@ -677,23 +695,15 @@ export class FolderRepositoryManager extends Disposable { return undefined; } - private async getCachedFromGlobalState(userKind: 'assignableUsers' | 'teamReviewers' | 'mentionableUsers' | 'orgProjects'): Promise<{ [key: string]: T[] } | undefined> { + private async getCachedFromGlobalState(userKind: UserCacheKind): Promise<{ [key: string]: T[] } | undefined> { Logger.appendLine(`Trying to use globalState for ${userKind}.`, this.id); - const usersCacheLocation = vscode.Uri.joinPath(this.context.globalStorageUri, userKind); - let usersCacheExists; - try { - usersCacheExists = await vscode.workspace.fs.stat(usersCacheLocation); - } catch (e) { - // file doesn't exit - } - if (!usersCacheExists) { - Logger.appendLine(`GlobalState does not exist for ${userKind}.`, this.id); - return undefined; - } - const cache: { [key: string]: T[] } = {}; const hasAllRepos = (await Promise.all(this._githubRepositories.map(async (repo) => { + const usersCacheLocation = this.getAccountCacheLocation(userKind, repo); + if (!usersCacheLocation) { + return false; + } const key = `${repo.remote.owner}/${repo.remote.repositoryName}.json`; const repoSpecificFile = vscode.Uri.joinPath(usersCacheLocation, key); let repoSpecificCache; @@ -721,18 +731,32 @@ export class FolderRepositoryManager extends Disposable { return undefined; } - private async saveInGlobalState(userKind: 'assignableUsers' | 'teamReviewers' | 'mentionableUsers' | 'orgProjects', cache: { [key: string]: T[] }): Promise { - const cacheLocation = vscode.Uri.joinPath(this.context.globalStorageUri, userKind); - await Promise.all(this._githubRepositories.map(async (repo) => { + private getAccountCacheLocation(userKind: UserCacheKind, repo: GitHubRepository): vscode.Uri | undefined { + const accountId = this._credentialStore.getAccountId(repo.remote.authProviderId); + if (!accountId) { + return undefined; + } + return vscode.Uri.joinPath(this.context.globalStorageUri, userKind, encodeURIComponent(repo.remote.authProviderId), encodeURIComponent(accountId)); + } + + private async saveInGlobalState(userKind: UserCacheKind, cache: { [key: string]: T[] }): Promise { + const repositories = [...this._githubRepositories]; + await Promise.all(repositories.map(async (repo) => { + const cacheLocation = this.getAccountCacheLocation(userKind, repo); + if (!cacheLocation) { + return; + } const key = `${repo.remote.owner}/${repo.remote.repositoryName}.json`; const repoSpecificFile = vscode.Uri.joinPath(cacheLocation, key); + await vscode.workspace.fs.createDirectory(vscode.Uri.joinPath(cacheLocation, repo.remote.owner)); await vscode.workspace.fs.writeFile(repoSpecificFile, new TextEncoder().encode(JSON.stringify(cache[repo.remote.remoteName]))); })); } private createFetchMentionableUsersPromise(): Promise<{ [key: string]: IAccount[] }> { + const accountCacheToken = this._accountCacheToken; const cache: { [key: string]: IAccount[] } = {}; - return new Promise<{ [key: string]: IAccount[] }>(resolve => { + return new Promise<{ [key: string]: IAccount[] }>((resolve, reject) => { const promises = this._githubRepositories.map(async githubRepository => { const data = await githubRepository.getMentionableUsers(); cache[githubRepository.remote.remoteName] = data; @@ -740,15 +764,20 @@ export class FolderRepositoryManager extends Disposable { }); Promise.all(promises).then(() => { + if (accountCacheToken !== this._accountCacheToken) { + resolve({}); + return; + } this._mentionableUsers = cache; this._fetchMentionableUsersPromise = undefined; this.saveInGlobalState('mentionableUsers', cache) - .then(() => resolve(cache)); - }); + .then(() => resolve(cache), reject); + }, reject); }); } async getMentionableUsers(clearCache?: boolean): Promise<{ [key: string]: IAccount[] }> { + const accountCacheToken = this._accountCacheToken; if (clearCache) { delete this._mentionableUsers; delete this._fetchMentionableUsersPromise; @@ -760,6 +789,9 @@ export class FolderRepositoryManager extends Disposable { } const globalStateMentionableUsers = clearCache ? undefined : await this.getCachedFromGlobalState('mentionableUsers'); + if (accountCacheToken !== this._accountCacheToken) { + return {}; + } if (!this._fetchMentionableUsersPromise) { this._fetchMentionableUsersPromise = this.createFetchMentionableUsersPromise(); @@ -770,6 +802,7 @@ export class FolderRepositoryManager extends Disposable { } async getAssignableUsers(clearCache?: boolean): Promise<{ [key: string]: IAccount[] }> { + const accountCacheToken = this._accountCacheToken; if (clearCache) { delete this._assignableUsers; delete this._fetchAssignableUsersPromise; @@ -781,11 +814,14 @@ export class FolderRepositoryManager extends Disposable { } const globalStateAssignableUsers = clearCache ? undefined : await this.getCachedFromGlobalState('assignableUsers'); + if (accountCacheToken !== this._accountCacheToken) { + return {}; + } if (!this._fetchAssignableUsersPromise) { const cache: { [key: string]: IAccount[] } = {}; const allAssignableUsers: IAccount[] = []; - this._fetchAssignableUsersPromise = new Promise(resolve => { + this._fetchAssignableUsersPromise = new Promise((resolve, reject) => { const promises = this._githubRepositories.map(async githubRepository => { const data = await githubRepository.getAssignableUsers(); cache[githubRepository.remote.remoteName] = data.sort(loginComparator); @@ -794,12 +830,21 @@ export class FolderRepositoryManager extends Disposable { }); Promise.all(promises).then(() => { + if (accountCacheToken !== this._accountCacheToken) { + resolve({}); + return; + } this._assignableUsers = cache; this._fetchAssignableUsersPromise = undefined; - this.saveInGlobalState('assignableUsers', cache); - resolve(cache); - this._onDidChangeAssignableUsers.fire(allAssignableUsers); - }); + this.saveInGlobalState('assignableUsers', cache).then(() => { + if (accountCacheToken !== this._accountCacheToken) { + resolve({}); + return; + } + resolve(cache); + this._onDidChangeAssignableUsers.fire(allAssignableUsers); + }, reject); + }, reject); }); return globalStateAssignableUsers ?? this._fetchAssignableUsersPromise; } @@ -808,6 +853,7 @@ export class FolderRepositoryManager extends Disposable { } async getTeamReviewers(refreshKind: TeamReviewerRefreshKind): Promise<{ [key: string]: ITeam[] }> { + const accountCacheToken = this._accountCacheToken; if (refreshKind === TeamReviewerRefreshKind.Force) { delete this._teamReviewers; } @@ -818,14 +864,16 @@ export class FolderRepositoryManager extends Disposable { } const globalStateTeamReviewers = (refreshKind === TeamReviewerRefreshKind.Force) ? undefined : await this.getCachedFromGlobalState('teamReviewers'); + if (accountCacheToken !== this._accountCacheToken) { + return {}; + } if (globalStateTeamReviewers) { this._teamReviewers = globalStateTeamReviewers; return globalStateTeamReviewers || {}; } - if (!this._fetchTeamReviewersPromise) { const cache: { [key: string]: ITeam[] } = {}; - return (this._fetchTeamReviewersPromise = new Promise(async (resolve) => { + return (this._fetchTeamReviewersPromise = new Promise(async (resolve, reject) => { // Keep track of the org teams we have already gotten so we don't make duplicate calls const orgTeams: Map = new Map(); // Go through one github repo at a time so that we don't make overlapping auth calls @@ -842,10 +890,14 @@ export class FolderRepositoryManager extends Disposable { cache[githubRepository.remote.remoteName] = allTeamsForOrg.filter(team => team.repositoryNames.includes(githubRepository.remote.repositoryName)).sort(teamComparator); } + if (accountCacheToken !== this._accountCacheToken) { + resolve({}); + return; + } this._teamReviewers = cache; this._fetchTeamReviewersPromise = undefined; - this.saveInGlobalState('teamReviewers', cache); - resolve(cache); + this.saveInGlobalState('teamReviewers', cache) + .then(() => resolve(accountCacheToken === this._accountCacheToken ? cache : {}), reject); })); } @@ -853,8 +905,9 @@ export class FolderRepositoryManager extends Disposable { } private createFetchOrgProjectsPromise(): Promise<{ [key: string]: IProject[] }> { + const accountCacheToken = this._accountCacheToken; const cache: { [key: string]: IProject[] } = {}; - return new Promise<{ [key: string]: IProject[] }>(async resolve => { + return new Promise<{ [key: string]: IProject[] }>(async (resolve, reject) => { // Keep track of the org teams we have already gotten so we don't make duplicate calls const orgProjects: Map = new Map(); // Go through one github repo at a time so that we don't make overlapping auth calls @@ -870,8 +923,12 @@ export class FolderRepositoryManager extends Disposable { cache[githubRepository.remote.remoteName] = orgProjects.get(githubRepository.remote.owner) ?? []; } - await this.saveInGlobalState('orgProjects', cache); - resolve(cache); + if (accountCacheToken !== this._accountCacheToken) { + resolve({}); + return; + } + this.saveInGlobalState('orgProjects', cache) + .then(() => resolve(accountCacheToken === this._accountCacheToken ? cache : {}), reject); }); } @@ -880,7 +937,11 @@ export class FolderRepositoryManager extends Disposable { return this.createFetchOrgProjectsPromise(); } + const accountCacheToken = this._accountCacheToken; const globalStateProjects = await this.getCachedFromGlobalState('orgProjects'); + if (accountCacheToken !== this._accountCacheToken) { + return {}; + } return globalStateProjects ?? this.createFetchOrgProjectsPromise(); } diff --git a/src/github/issueOverview.ts b/src/github/issueOverview.ts index 04d010d116..cc7b02de03 100644 --- a/src/github/issueOverview.ts +++ b/src/github/issueOverview.ts @@ -118,6 +118,12 @@ export class IssueOverviewPanel extends W return this._panels.get(panelKey(owner, repo, number)); } + public static clearAll(): void { + const panels = Array.from(this._panels.values()); + this._panels.clear(); + panels.forEach(panel => panel.dispose()); + } + /** * Build a short panel title: `# `. * The item title is truncated to approximately `maxLength` characters on a diff --git a/src/github/repositoriesManager.ts b/src/github/repositoriesManager.ts index 78958f5a88..a147f42dc5 100644 --- a/src/github/repositoriesManager.ts +++ b/src/github/repositoriesManager.ts @@ -252,6 +252,12 @@ export class RepositoriesManager extends Disposable { return this._credentialStore; } + clearForAuthChange(): void { + for (const folderManager of this._folderManagers) { + folderManager.clearForAuthChange(); + } + } + async refreshRepositories(): Promise { await Promise.all(this._folderManagers.map(folderManager => folderManager.updateRepositories(false, true))); this.updateState(); diff --git a/src/issues/stateManager.ts b/src/issues/stateManager.ts index e865208f38..4c4316162f 100644 --- a/src/issues/stateManager.ts +++ b/src/issues/stateManager.ts @@ -209,14 +209,18 @@ export class StateManager { } } - async refreshForAuthChange() { + clearForAuthChange(): void { this.resolvedIssues.clear(); for (const state of this._singleRepoStates.values()) { if (state) { - state.issueCollection.clear(); + state.issueCollection = new Map(); state.userMap = undefined; } } + this._onDidChangeIssueData.fire(); + } + + async refreshAfterAuthChange() { if (this.manager.credentialStore.isAnyAuthenticated()) { await this.refresh(); } else { @@ -335,7 +339,8 @@ export class StateManager { if (!singleRepoState) { return; } - singleRepoState.issueCollection.clear(); + const issueCollection = singleRepoState.issueCollection; + issueCollection.clear(); const enterpriseRemotes = (await parseRepositoryRemotesAsync(folderManager.repository)).filter( remote => remote.isEnterprise ); @@ -354,7 +359,7 @@ export class StateManager { ).then(issues => ({ groupBy: query.groupBy ?? [], issues })); if (items) { - singleRepoState.issueCollection.set(query.label, items); + issueCollection.set(query.label, items); } } singleRepoState.maxIssueNumber = await folderManager.getMaxIssue(folderManager.repository); diff --git a/src/notifications/notificationsManager.ts b/src/notifications/notificationsManager.ts index 7c8bea0289..d288c7fd93 100644 --- a/src/notifications/notificationsManager.ts +++ b/src/notifications/notificationsManager.ts @@ -227,6 +227,7 @@ export class NotificationsManager extends Disposable implements vscode.TreeDataP } public async getNotifications(): Promise { + const notificationsCache = this._notifications; let pollInterval = this._pollingDuration; let lastModified = this._pollingLastModified; if (this._fetchNotifications) { @@ -250,9 +251,12 @@ export class NotificationsManager extends Disposable implements vscode.TreeDataP notification, model, kind: 'notification' }); })); + if (notificationsCache !== this._notifications) { + return undefined; + } for (const [key, value] of notificationTreeItems.entries()) { - this._notifications.set(key, value); + notificationsCache.set(key, value); } this._hasNextPage = notificationsData.hasNextPage; @@ -261,14 +265,17 @@ export class NotificationsManager extends Disposable implements vscode.TreeDataP // Calculate notification priority if (this._sortingMethod === NotificationsSortMethod.Priority) { - const notificationsWithoutPriority = Array.from(this._notifications.values()) + const notificationsWithoutPriority = Array.from(notificationsCache.values()) .filter(notification => notification.priority === undefined); const notificationPriorities = await this._notificationProvider .getNotificationsPriority(notificationsWithoutPriority); + if (notificationsCache !== this._notifications) { + return undefined; + } for (const { key, priority, priorityReasoning } of notificationPriorities) { - const notification = this._notifications.get(key); + const notification = notificationsCache.get(key); if (!notification) { continue; } @@ -276,11 +283,11 @@ export class NotificationsManager extends Disposable implements vscode.TreeDataP notification.priority = priority; notification.priorityReason = priorityReasoning; - this._notifications.set(key, notification); + notificationsCache.set(key, notification); } } - const notifications = Array.from(this._notifications.values()); + const notifications = Array.from(notificationsCache.values()); this._updateContext(); this._onDidChangeNotifications.fire(notifications); @@ -306,16 +313,25 @@ export class NotificationsManager extends Disposable implements vscode.TreeDataP return Array.from(this._notifications.values()); } - public refresh(): void { - if (this._notifications.size !== 0) { - const updates = Array.from(this._notifications.values()); - this._onDidChangeNotifications.fire(updates); - } - + private clearNotifications(): NotificationTreeItem[] { + const updates = Array.from(this._notifications.values()); this._pageCount = 1; this._dateTime = new Date(); - this._notifications.clear(); + this._notifications = new Map(); + this._updateContext(); + return updates; + } + public clear(): void { + const updates = this.clearNotifications(); + this._fetchNotifications = false; + this._onDidChangeNotifications.fire(updates); + this._onDidChangeTreeData.fire(); + } + + public refresh(): void { + const updates = this.clearNotifications(); + this._onDidChangeNotifications.fire(updates); this._refresh(true); } diff --git a/src/test/github/credentials.test.ts b/src/test/github/credentials.test.ts index e6163eac31..36dbdb31b0 100644 --- a/src/test/github/credentials.test.ts +++ b/src/test/github/credentials.test.ts @@ -8,7 +8,7 @@ import { Octokit } from '@octokit/rest'; import { createSandbox, SinonSandbox } from 'sinon'; import * as vscode from 'vscode'; import { AuthProvider } from '../../common/authentication'; -import { CredentialStore, findExistingSession, GitHub } from '../../github/credentials'; +import { CredentialStore, findExistingSession, GitHub, hasAccountChanged } from '../../github/credentials'; import { LoggingApolloClient, LoggingOctokit, RateLogger } from '../../github/loggingOctokit'; import { MockExtensionContext } from '../mocks/mockExtensionContext'; import { MockTelemetry } from '../mocks/mockTelemetry'; @@ -52,13 +52,16 @@ describe('CredentialStore', function () { const result = await findExistingSession(AuthProvider.github, async (_providerId, scopes, options) => { requests.push({ scopes, accountId: options.account?.id }); - if (options.account?.id === 'second') { - return undefined; + if (!scopes.length) { + return secondAccountDefault; + } + if (options.account?.id === 'second' && scopesEqual(scopes, defaultScopes)) { + return secondAccountDefault; } if (scopesEqual(scopes, defaultScopes)) { return secondAccountDefault; } - if (scopesEqual(scopes, additionalScopes)) { + if (!options.account && scopesEqual(scopes, additionalScopes)) { return firstAccountAdditional; } return undefined; @@ -67,7 +70,35 @@ describe('CredentialStore', function () { strictEqual(result?.session, secondAccountDefault); deepStrictEqual(result?.scopes, defaultScopes); deepStrictEqual(requests, [ - { scopes: defaultScopes, accountId: undefined }, + { scopes: [], accountId: undefined }, + { scopes: additionalScopes, accountId: 'second' }, + { scopes: defaultScopes, accountId: 'second' }, + ]); + }); + + it('uses the preferred account when accounts have different scope sets', async function () { + const firstAccountDefault = createSession('first-default', 'first', defaultScopes); + const secondAccountAdditional = createSession('second-additional', 'second', additionalScopes); + const requests: { scopes: readonly string[], accountId?: string }[] = []; + + const result = await findExistingSession(AuthProvider.github, async (_providerId, scopes, options) => { + requests.push({ scopes, accountId: options.account?.id }); + if (!scopes.length) { + return secondAccountAdditional; + } + if (!options.account && scopesEqual(scopes, defaultScopes)) { + return firstAccountDefault; + } + if (options.account?.id === 'second' && scopesEqual(scopes, additionalScopes)) { + return secondAccountAdditional; + } + return undefined; + }); + + strictEqual(result?.session, secondAccountAdditional); + deepStrictEqual(result?.scopes, additionalScopes); + deepStrictEqual(requests, [ + { scopes: [], accountId: undefined }, { scopes: additionalScopes, accountId: 'second' }, ]); }); @@ -77,6 +108,9 @@ describe('CredentialStore', function () { const preferredAdditional = createSession('preferred-additional', 'preferred', additionalScopes); const result = await findExistingSession(AuthProvider.github, async (_providerId, scopes, options) => { + if (!scopes.length) { + return preferredDefault; + } if (options.account?.id === 'preferred' && scopesEqual(scopes, additionalScopes)) { return preferredAdditional; } @@ -104,7 +138,7 @@ describe('CredentialStore', function () { const additionalSession = createSession('additional', 'additional', additionalScopes); const additionalResult = await findExistingSession(AuthProvider.github, async (_providerId, scopes, options) => { - if (!options.account && scopesEqual(scopes, additionalScopes)) { + if (!scopes.length || (options.account?.id === 'additional' && scopesEqual(scopes, additionalScopes))) { return additionalSession; } return undefined; @@ -115,6 +149,21 @@ describe('CredentialStore', function () { }); }); + describe('hasAccountChanged', function () { + it('does not treat a new session for the same account as an account change', function () { + const session = createSession('new-session', 'account', additionalScopes); + + strictEqual(hasAccountChanged('account', session), false); + }); + + it('detects account changes and sign out', function () { + const session = createSession('new-session', 'new-account', defaultScopes); + + strictEqual(hasAccountChanged('old-account', session), true); + strictEqual(hasAccountChanged('old-account', undefined), true); + }); + }); + it('retries the current user request after a failure', async function () { const telemetry = new MockTelemetry(); const credentialStore = new CredentialStore(telemetry, new MockExtensionContext()); diff --git a/src/test/github/externalUriOpener.test.ts b/src/test/github/externalUriOpener.test.ts index 811f585d59..595e87e8f5 100644 --- a/src/test/github/externalUriOpener.test.ts +++ b/src/test/github/externalUriOpener.test.ts @@ -7,7 +7,6 @@ import { default as assert } from 'assert'; import { createSandbox, SinonSandbox } from 'sinon'; import * as vscode from 'vscode'; import { RemoteOnlyRepository } from '../../api/remoteOnlyRepository'; -import { OPEN_PULL_LINKS, PR_SETTINGS_NAMESPACE } from '../../common/settingKeys'; import { CredentialStore } from '../../github/credentials'; import { registerGitHubIssueOrPullRequestExternalUriOpener } from '../../github/externalUriOpener'; import { FolderRepositoryManager } from '../../github/folderRepositoryManager'; @@ -33,8 +32,6 @@ describe('GitHubIssueOrPullRequestExternalUriOpener', () => { const credentialStore = new CredentialStore(telemetry, context); const repositoriesManager = new RepositoriesManager(credentialStore, telemetry); const folderRepositoryManagerResolver = new FolderRepositoryManagerResolver(context, repositoriesManager, telemetry); - const configuration = vscode.workspace.getConfiguration(PR_SETTINGS_NAMESPACE); - const previousSettingValue = configuration.inspect(OPEN_PULL_LINKS)?.globalValue; let opener: vscode.ExternalUriOpener | undefined; let registration: vscode.Disposable | undefined; let cancellation: vscode.CancellationTokenSource | undefined; @@ -49,7 +46,6 @@ describe('GitHubIssueOrPullRequestExternalUriOpener', () => { sandbox.stub(vscode.window, 'showErrorMessage').resolves(undefined); try { - await configuration.update(OPEN_PULL_LINKS, true, vscode.ConfigurationTarget.Global); registration = registerGitHubIssueOrPullRequestExternalUriOpener( context, folderRepositoryManagerResolver, @@ -57,6 +53,7 @@ describe('GitHubIssueOrPullRequestExternalUriOpener', () => { ); const uri = vscode.Uri.parse('https://github.com/microsoft/vscode/issues/1'); assert.ok(opener); + sandbox.stub(opener as any, 'isOpenPullLinksEnabled').returns(true); cancellation = new vscode.CancellationTokenSource(); await opener.openExternalUri(uri, { sourceUri: uri }, cancellation.token); @@ -65,7 +62,6 @@ describe('GitHubIssueOrPullRequestExternalUriOpener', () => { } finally { cancellation?.dispose(); registration?.dispose(); - await configuration.update(OPEN_PULL_LINKS, previousSettingValue, vscode.ConfigurationTarget.Global); folderRepositoryManagerResolver.dispose(); repositoriesManager.dispose(); credentialStore.dispose(); diff --git a/src/test/github/folderRepositoryManager.test.ts b/src/test/github/folderRepositoryManager.test.ts index 5ff6a3870c..b8b07b7f4d 100644 --- a/src/test/github/folderRepositoryManager.test.ts +++ b/src/test/github/folderRepositoryManager.test.ts @@ -85,6 +85,76 @@ describe('PullRequestManager', function () { /Repository owner\/missing is not accessible\./, ); }); + + it('clears account-specific repository state on auth change', function () { + const internal = manager as unknown as { + _sessionIgnoredRemoteNames: Set; + _inaccessibleRepos: Set; + _repositoryPageInformation: Map; + _gitBlameCache: Record; + _mentionableUsers?: Record; + _assignableUsers?: Record; + _teamReviewers?: Record; + _accountCacheToken: object; + }; + internal._sessionIgnoredRemoteNames.add('origin'); + internal._inaccessibleRepos.add('owner/repo'); + internal._repositoryPageInformation.set('query', {}); + internal._gitBlameCache.file = 'user'; + internal._mentionableUsers = { origin: [] }; + internal._assignableUsers = { origin: [] }; + internal._teamReviewers = { origin: [] }; + const oldAccountCacheToken = internal._accountCacheToken; + + manager.clearForAuthChange(); + + assert.strictEqual(internal._sessionIgnoredRemoteNames.size, 0); + assert.strictEqual(internal._inaccessibleRepos.size, 0); + assert.strictEqual(internal._repositoryPageInformation.size, 0); + assert.deepStrictEqual(internal._gitBlameCache, {}); + assert.strictEqual(internal._mentionableUsers, undefined); + assert.strictEqual(internal._assignableUsers, undefined); + assert.strictEqual(internal._teamReviewers, undefined); + assert.notStrictEqual(internal._accountCacheToken, oldAccountCacheToken); + }); + + it('does not publish user data into a replacement account cache', async function () { + let resolveUsers: (users: []) => void; + const users = new Promise<[]>(resolve => resolveUsers = resolve); + const internal = manager as unknown as { + _githubRepositories: { remote: { remoteName: string }, getAssignableUsers(): Promise<[]> }[]; + _assignableUsers?: Record; + }; + internal._githubRepositories = [{ + remote: { remoteName: 'origin' }, + getAssignableUsers: () => users, + }]; + + const pendingUsers = manager.getAssignableUsers(true); + manager.clearForAuthChange(); + resolveUsers!([]); + + assert.deepStrictEqual(await pendingUsers, {}); + assert.strictEqual(internal._assignableUsers, undefined); + }); + + it('scopes persisted user data to the authentication provider and account', function () { + const url = 'https://github.com/owner/repo'; + const remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom); + const githubRepository = new GitHubRepository(1, remote, repository.rootUri, manager.credentialStore, telemetry); + const getAccountId = sinon.stub(manager.credentialStore, 'getAccountId'); + const internal = manager as unknown as { + getAccountCacheLocation(userKind: string, repo: GitHubRepository): Uri | undefined; + }; + getAccountId.returns('first-account'); + const firstAccountLocation = internal.getAccountCacheLocation('assignableUsers', githubRepository); + getAccountId.returns('second-account'); + const secondAccountLocation = internal.getAccountCacheLocation('assignableUsers', githubRepository); + + assert.notStrictEqual(firstAccountLocation?.toString(), secondAccountLocation?.toString()); + assert.ok(firstAccountLocation?.toString().includes('github')); + assert.ok(firstAccountLocation?.toString().includes('first-account')); + }); }); describe('getPullRequestDefaults', function () { diff --git a/src/test/github/pullRequestOverview.test.ts b/src/test/github/pullRequestOverview.test.ts index d9e2bb8322..e1a15bf2f5 100644 --- a/src/test/github/pullRequestOverview.test.ts +++ b/src/test/github/pullRequestOverview.test.ts @@ -113,6 +113,16 @@ describe('PullRequestOverview', function () { restoredWebviewPanel.dispose(); }); + it('clears all open pull request panels', function () { + const dispose = sinon.spy(); + (PullRequestOverviewPanel as any)._panels.set(panelKey('aaa', 'bbb', 1000), { dispose }); + + PullRequestOverviewPanel.clearAll(); + + assert.strictEqual(dispose.calledOnce, true); + assert.strictEqual(PullRequestOverviewPanel.findPanel('aaa', 'bbb', 1000), undefined); + }); + it('builds the active PR URL before the PR has loaded', async function () { repo.addGraphQLPullRequest(builder => { builder.pullRequest(response => { diff --git a/src/test/issues/stateManager.test.ts b/src/test/issues/stateManager.test.ts index 9102149dc9..39be75dd07 100644 --- a/src/test/issues/stateManager.test.ts +++ b/src/test/issues/stateManager.test.ts @@ -8,6 +8,7 @@ import * as vscode from 'vscode'; import { StateManager } from '../../issues/stateManager'; import { CurrentIssue } from '../../issues/currentIssue'; import { USE_BRANCH_FOR_ISSUES, ISSUES_SETTINGS_NAMESPACE } from '../../common/settingKeys'; +import { MockRepository } from '../mocks/mockRepository'; // Mock classes for testing class MockFolderRepositoryManager { @@ -86,6 +87,43 @@ describe('StateManager branch behavior with useBranchForIssues setting', functio } }); + it('clears account-specific issue caches immediately', function () { + let issueDataChanged = 0; + stateManager.onDidChangeIssueData(() => issueDataChanged++); + stateManager.resolvedIssues.set('owner/repo', {} as never); + const state = { + issueCollection: new Map([['query', Promise.resolve({ issues: [], groupBy: [] })]]), + userMap: Promise.resolve(new Map()), + }; + (stateManager as any)._singleRepoStates.set('/test', state); + + stateManager.clearForAuthChange(); + + assert.strictEqual(stateManager.resolvedIssues.size, 0); + assert.strictEqual(state.issueCollection.size, 0); + assert.strictEqual(state.userMap, undefined); + assert.strictEqual(issueDataChanged, 1); + }); + + it('does not publish issue queries into a replacement collection', async function () { + const repository = new MockRepository(); + const internal = stateManager as any; + internal._queries = [{ label: 'My Issues', query: 'is:open' }]; + internal.getCurrentUser = async () => 'old-account'; + const folderManager = { + repository, + getMaxIssue: async () => 0, + getIssues: async () => ({ items: [] }), + }; + + const pendingIssueData = internal.setIssueData(folderManager); + stateManager.clearForAuthChange(); + await pendingIssueData; + + const state = internal._singleRepoStates.get(repository.rootUri.path); + assert.strictEqual(state.issueCollection.size, 0); + }); + it('should checkout default branch when useBranchForIssues is not off', async function () { // Mock workspace configuration to return 'on' const originalGetConfiguration = vscode.workspace.getConfiguration; diff --git a/src/test/notifications/notificationsManager.test.ts b/src/test/notifications/notificationsManager.test.ts new file mode 100644 index 0000000000..1724787bcd --- /dev/null +++ b/src/test/notifications/notificationsManager.test.ts @@ -0,0 +1,91 @@ +/*--------------------------------------------------------------------------------------------- + * 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, SinonSandbox } from 'sinon'; +import { CredentialStore } from '../../github/credentials'; +import { RepositoriesManager } from '../../github/repositoriesManager'; +import { NotificationTreeItem } from '../../notifications/notificationItem'; +import { NotificationsManager } from '../../notifications/notificationsManager'; +import { NotificationsProvider } from '../../notifications/notificationsProvider'; +import { MockCommandRegistry } from '../mocks/mockCommandRegistry'; +import { MockExtensionContext } from '../mocks/mockExtensionContext'; + +describe('NotificationsManager', function () { + let sinon: SinonSandbox; + let manager: NotificationsManager; + + beforeEach(function () { + sinon = createSandbox(); + MockCommandRegistry.install(sinon); + manager = new NotificationsManager( + {} as NotificationsProvider, + {} as CredentialStore, + {} as RepositoriesManager, + new MockExtensionContext(), + ); + }); + + afterEach(function () { + manager.dispose(); + sinon.restore(); + }); + + it('clears cached notifications without fetching', function () { + const notification = {} as NotificationTreeItem; + const internal = manager as unknown as { + _notifications: Map; + _fetchNotifications: boolean; + }; + internal._notifications.set('notification', notification); + internal._fetchNotifications = true; + const onDidChangeNotifications = sinon.spy(); + const onDidChangeTreeData = sinon.spy(); + manager.onDidChangeNotifications(onDidChangeNotifications); + manager.onDidChangeTreeData(onDidChangeTreeData); + + manager.clear(); + + assert.strictEqual(internal._notifications.size, 0); + assert.strictEqual(internal._fetchNotifications, false); + assert.deepStrictEqual(onDidChangeNotifications.firstCall.args[0], [notification]); + assert.strictEqual(onDidChangeTreeData.calledOnce, true); + }); + + it('does not publish notifications into a replacement cache', async function () { + let resolveNotifications: (notifications: { + notifications: []; + hasNextPage: boolean; + pollInterval: number; + lastModified: string; + }) => void; + const notifications = new Promise<{ + notifications: []; + hasNextPage: boolean; + pollInterval: number; + lastModified: string; + }>(resolve => resolveNotifications = resolve); + const getNotifications = sinon.stub().returns(notifications); + const internal = manager as unknown as { + _notificationProvider: { getNotifications: typeof getNotifications }; + _fetchNotifications: boolean; + }; + internal._notificationProvider = { getNotifications }; + internal._fetchNotifications = true; + + const pendingNotifications = manager.getNotifications(); + manager.clear(); + resolveNotifications!({ + notifications: [], + hasNextPage: false, + pollInterval: 60, + lastModified: '', + }); + + assert.strictEqual(await pendingNotifications, undefined); + assert.deepStrictEqual(manager.getAllNotifications(), []); + assert.strictEqual(internal._fetchNotifications, false); + }); +}); diff --git a/src/test/view/prsTree.test.ts b/src/test/view/prsTree.test.ts index 881d2a8385..c5f0e44ded 100644 --- a/src/test/view/prsTree.test.ts +++ b/src/test/view/prsTree.test.ts @@ -136,6 +136,25 @@ describe('GitHub Pull Requests view', function () { ); }); + it('clears the tree immediately', async function () { + const repository = new MockRepository(); + repository.addRemote('origin', 'git@github.com:aaa/bbb'); + const folderManager = new FolderRepositoryManager(0, context, repository, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher); + sinon.stub(folderManager, 'getPullRequestDefaults').resolves({ owner: 'aaa', repo: 'bbb', base: 'main' }); + reposManager.insertFolderManager(folderManager); + sinon.stub(credentialStore, 'isAuthenticated').returns(true); + await folderManager.updateRepositories(); + provider.initialize([], mockNotificationsManager as NotificationsManager); + await provider.getChildren(); + const onDidChangeTreeData = sinon.spy(); + provider.onDidChangeTreeData(onDidChangeTreeData); + + provider.clear(); + + assert.deepStrictEqual(await provider.cachedChildren(), []); + assert(onDidChangeTreeData.calledOnce); + }); + it('refreshes tree when GitHub repositories are discovered in existing folder manager', async function () { const repository = new MockRepository(); repository.addRemote('origin', 'git@github.com:aaa/bbb'); diff --git a/src/test/view/reviewManager.test.ts b/src/test/view/reviewManager.test.ts index f2c017feeb..41dde8d0d5 100644 --- a/src/test/view/reviewManager.test.ts +++ b/src/test/view/reviewManager.test.ts @@ -124,6 +124,29 @@ describe('ReviewManager polling', function () { assert.strictEqual(latestScheduledDelay(), POLL_MIN_INTERVAL_MS * POLL_BACKOFF_MULTIPLIER); }); + it('clears account-specific review state on auth change', async function () { + const internal = reviewManager as unknown as { + _lastCommitSha?: string; + _cachedMaxPRNumbers?: Map; + _cachedBranchName?: string; + _staleMetadataCheckedBranches: Set; + clear(quitReviewMode: boolean): Promise; + }; + internal._lastCommitSha = 'commit'; + internal._cachedMaxPRNumbers = new Map([['owner/repo', 1]]); + internal._cachedBranchName = 'branch'; + internal._staleMetadataCheckedBranches.add('branch'); + const clear = sinon.stub(internal, 'clear').resolves(); + + await reviewManager.clearForAuthChange(); + + assert.strictEqual(internal._lastCommitSha, undefined); + assert.strictEqual(internal._cachedMaxPRNumbers, undefined); + assert.strictEqual(internal._cachedBranchName, undefined); + assert.strictEqual(internal._staleMetadataCheckedBranches.size, 0); + assert.strictEqual(clear.calledOnceWithExactly(true), true); + }); + it('resets polling interval to minimum when a change is detected', async function () { // doPoll detects a change by comparing ReviewManager's internal _prNumber / // _lastCommitSha before and after updateState, so the stub must mutate one of diff --git a/src/view/createPullRequestHelper.ts b/src/view/createPullRequestHelper.ts index 63339033e8..1be3425290 100644 --- a/src/view/createPullRequestHelper.ts +++ b/src/view/createPullRequestHelper.ts @@ -284,6 +284,10 @@ export class CreatePullRequestHelper extends Disposable { } + clearForAuthChange(): void { + this.reset(); + } + override dispose() { this.reset(); super.dispose(); diff --git a/src/view/prsTreeDataProvider.ts b/src/view/prsTreeDataProvider.ts index bfbed43df9..ddbb84c1fb 100644 --- a/src/view/prsTreeDataProvider.ts +++ b/src/view/prsTreeDataProvider.ts @@ -365,6 +365,13 @@ export class PullRequestsTreeDataProvider extends Disposable implements vscode.T this._onDidChangeTreeData.fire(); } + clear() { + this.prsTreeModel.forceClearCache(true); + this._children.forEach(child => child.dispose()); + this._children = []; + this._onDidChangeTreeData.fire(); + } + private tryReset(reset: boolean) { if (reset) { this.prsTreeModel.clearCache(true); @@ -565,9 +572,7 @@ export class PullRequestsTreeDataProvider extends Disposable implements vscode.T const gitHubFolderManagers = this._reposManager.folderManagers.filter(manager => manager.gitHubRepositories.length > 0); if (!element) { - if (this._children && this._children.length) { - this._children.forEach(dispose => dispose.dispose()); - } + this._children.forEach(child => child.dispose()); let result: WorkspaceFolderNode[] | CategoryTreeNode[]; if (gitHubFolderManagers.length === 1) { diff --git a/src/view/prsTreeModel.ts b/src/view/prsTreeModel.ts index 7a2ecc0824..29e4937183 100644 --- a/src/view/prsTreeModel.ts +++ b/src/view/prsTreeModel.ts @@ -196,10 +196,12 @@ export class PrsTreeModel extends Disposable { return this._queriedPullRequests.get(identifier); } - public forceClearCache() { + public forceClearCache(silent: boolean = false) { this._cachedPRs.clear(); this._allCachedPRs.clear(); - this._onDidChangeData.fire(); + if (!silent) { + this._onDidChangeData.fire(); + } } public hasPullRequest(pr: PullRequestModel): boolean { diff --git a/src/view/reviewManager.ts b/src/view/reviewManager.ts index 3abaa96d7b..7fcd656f0a 100644 --- a/src/view/reviewManager.ts +++ b/src/view/reviewManager.ts @@ -1525,6 +1525,14 @@ export class ReviewManager extends Disposable { } } + async clearForAuthChange(): Promise { + this._lastCommitSha = undefined; + this._cachedMaxPRNumbers = undefined; + this._cachedBranchName = undefined; + this._staleMetadataCheckedBranches.clear(); + await this.clear(true); + } + private async clear(quitReviewMode: boolean) { if (quitReviewMode) { const activePullRequest = this._folderRepoManager.activePullRequest; diff --git a/src/view/reviewsManager.ts b/src/view/reviewsManager.ts index c01319d92d..7b46888fe6 100644 --- a/src/view/reviewsManager.ts +++ b/src/view/reviewsManager.ts @@ -51,6 +51,15 @@ export class ReviewsManager extends Disposable { return this._reviewManagers; } + async clearForAuthChange(): Promise { + this._prsTreeDataProvider.clear(); + await Promise.all(this._reviewManagers.map(reviewManager => reviewManager.clearForAuthChange())); + } + + refreshPullRequestsTree(reset: boolean = false): void { + this._prsTreeDataProvider.refreshAll(reset); + } + private registerListeners(): void { this._register(vscode.workspace.onDidChangeConfiguration(async e => { if (e.affectsConfiguration('githubPullRequests.showInSCM')) { @@ -232,4 +241,3 @@ async function handleUncommittedChanges(repository: Repository): Promise