diff --git a/src/vs/sessions/contrib/changes/browser/checksActions.ts b/src/vs/sessions/contrib/changes/browser/checksActions.ts index 8543a6f34ed574..b1972914adc240 100644 --- a/src/vs/sessions/contrib/changes/browser/checksActions.ts +++ b/src/vs/sessions/contrib/changes/browser/checksActions.ts @@ -5,7 +5,6 @@ import { Codicon } from '../../../../base/common/codicons.js'; import { Disposable } from '../../../../base/common/lifecycle.js'; -import { derived } from '../../../../base/common/observable.js'; import { localize, localize2 } from '../../../../nls.js'; import { Action2, MenuId, registerAction2 } from '../../../../platform/actions/common/actions.js'; import { ContextKeyExpr, IContextKeyService, RawContextKey } from '../../../../platform/contextkey/common/contextkey.js'; @@ -103,30 +102,12 @@ class ActiveSessionFailedCIChecksContextContribution extends Disposable implemen constructor( @IContextKeyService contextKeyService: IContextKeyService, - @ISessionsManagementService sessionManagementService: ISessionsManagementService, @IGitHubService gitHubService: IGitHubService, ) { super(); - const ciModelObs = derived(this, reader => { - const session = sessionManagementService.activeSession.read(reader); - if (!session) { - return undefined; - } - const gitHubInfo = session.gitHubInfo.read(reader); - if (!gitHubInfo?.pullRequest) { - return undefined; - } - const prModel = gitHubService.getPullRequest(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequest.number); - const pr = prModel.pullRequest.read(reader); - if (!pr) { - return undefined; - } - return gitHubService.getPullRequestCI(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequest.number, pr.headSha); - }); - this._register(bindContextKey(hasActiveSessionFailedCIChecks, contextKeyService, reader => { - const ciModel = ciModelObs.read(reader); + const ciModel = gitHubService.activeSessionPullRequestCIObs.read(reader); if (!ciModel) { return false; } @@ -167,18 +148,11 @@ class FixCIChecksAction extends Action2 { return; } - const gitHubInfo = activeSession.gitHubInfo.get(); - if (!gitHubInfo?.pullRequest) { - return; - } - - const prModel = gitHubService.getPullRequest(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequest.number); - const pr = prModel.pullRequest.get(); - if (!pr) { + const ciModel = gitHubService.activeSessionPullRequestCIObs.get(); + if (!ciModel) { return; } - const ciModel = gitHubService.getPullRequestCI(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequest.number, pr.headSha); const checks = ciModel.checks.get(); const failedChecks = getFailedChecks(checks); if (failedChecks.length === 0) { diff --git a/src/vs/sessions/contrib/changes/browser/checksViewModel.ts b/src/vs/sessions/contrib/changes/browser/checksViewModel.ts index bfee6b00863812..debb22969d5654 100644 --- a/src/vs/sessions/contrib/changes/browser/checksViewModel.ts +++ b/src/vs/sessions/contrib/changes/browser/checksViewModel.ts @@ -9,7 +9,7 @@ import { URI } from '../../../../base/common/uri.js'; import { IGitHubService } from '../../github/browser/githubService.js'; import { GitHubPullRequestCIModel } from '../../github/browser/models/githubPullRequestCIModel.js'; import { ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; -import { structuralEquals } from '../../../../base/common/equals.js'; +import { isEqual } from '../../../../base/common/resources.js'; export class ChecksViewModel extends Disposable { readonly activeSessionResourceObs: IObservable; @@ -21,52 +21,13 @@ export class ChecksViewModel extends Disposable { ) { super(); - this.activeSessionResourceObs = derived(this, reader => { + this.activeSessionResourceObs = derivedOpts({ equalsFn: isEqual }, reader => { const session = sessionManagementService.activeSession.read(reader); return session?.resource; }); - const pullRequestInfoObs = derivedOpts<{ owner: string; repo: string; prNumber: number; headSha: string } | undefined>({ - equalsFn: structuralEquals - }, reader => { - const session = sessionManagementService.activeSession.read(reader); - if (!session) { - return undefined; - } - - const gitHubInfo = session.gitHubInfo.read(reader); - if (!gitHubInfo?.pullRequest) { - return undefined; - } - - const prModel = gitHubService.getPullRequest(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequest.number); - const pr = prModel.pullRequest.read(reader); - if (!pr) { - return undefined; - } - - return { - owner: gitHubInfo.owner, - repo: gitHubInfo.repo, - prNumber: gitHubInfo.pullRequest.number, - headSha: pr.headSha - }; - }); - this.checksObs = derived(this, reader => { - const pullRequestInfo = pullRequestInfoObs.read(reader); - if (!pullRequestInfo) { - return undefined; - } - - // Use the PR's headSha (commit SHA) rather than the branch - // name so CI checks can still be fetched after branch deletion - // (e.g. after the PR is merged). - const ciModel = gitHubService.getPullRequestCI(pullRequestInfo.owner, pullRequestInfo.repo, pullRequestInfo.prNumber, pullRequestInfo.headSha); - ciModel.refresh(); - reader.store.add(ciModel.startPolling()); - - return ciModel; + return gitHubService.activeSessionPullRequestCIObs.read(reader); }); } } diff --git a/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts b/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts index 7c044a6750cadf..5368024aff738d 100644 --- a/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts +++ b/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts @@ -3,8 +3,8 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { Disposable, DisposableStore, toDisposable } from '../../../../base/common/lifecycle.js'; -import { autorun, derivedOpts, IObservable, observableValue, transaction } from '../../../../base/common/observable.js'; +import { Disposable } from '../../../../base/common/lifecycle.js'; +import { autorun, derivedOpts, IObservable, ISettableObservable, observableValue, transaction } from '../../../../base/common/observable.js'; import { isEqual } from '../../../../base/common/resources.js'; import { URI, UriComponents } from '../../../../base/common/uri.js'; import { IRange, Range } from '../../../../editor/common/core/range.js'; @@ -19,7 +19,6 @@ import { isIChatSessionFileChange2 } from '../../../../workbench/contrib/chat/co import { IGitHubService } from '../../github/browser/githubService.js'; import { ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; import { ISessionFileChange } from '../../../services/sessions/common/session.js'; -import { structuralEquals } from '../../../../base/common/equals.js'; // --- Types ------------------------------------------------------------------- @@ -234,17 +233,11 @@ interface IStoredCodeReviewComment { // --- Implementation ---------------------------------------------------------- interface ISessionReviewData { - readonly state: ReturnType>; + readonly state: ISettableObservable; } -type IPullRequestReviewThreadsModel = ReturnType; - interface IPRSessionReviewData { - readonly state: ReturnType>; - readonly disposables: DisposableStore; - readonly pollingDisposable: DisposableStore; - reviewThreadsModel?: IPullRequestReviewThreadsModel; - initialized: boolean; + readonly state: ISettableObservable; } function isRawCodeReviewRangeWithPositions(range: IRawCodeReviewRange): range is IRawCodeReviewRangeWithPositions { @@ -332,43 +325,60 @@ export class CodeReviewService extends Disposable implements ICodeReviewService return this._sessionsManagementService.activeSession.read(reader)?.resource; }); - const gitHubInfoObs = derivedOpts<{ owner: string; repo: string; pullRequestNumber: number } | undefined>({ equalsFn: structuralEquals }, reader => { - const gitHubInfo = this._sessionsManagementService.activeSession.read(reader)?.gitHubInfo.read(reader); - if (!gitHubInfo?.pullRequest) { - return undefined; - } - - return { - owner: gitHubInfo.owner, - repo: gitHubInfo.repo, - pullRequestNumber: gitHubInfo.pullRequest.number, - }; - }); - + // Subscribe to the active session's PR review threads model and project + // review threads into per-session PR review state. The model lifetime is + // owned by `IGitHubService.activeSessionPullRequestReviewThreadsObs`; we + // only consume it here. this._register(autorun(reader => { const activeSessionResource = activeSessionResourceObs.read(reader); if (!activeSessionResource) { return; } - const gitHubInfo = gitHubInfoObs.read(reader); - const data = this._ensurePRReviewInitialized(activeSessionResource, gitHubInfo); - - if (!data.reviewThreadsModel) { + const reviewThreadsModel = this._gitHubService.activeSessionPullRequestReviewThreadsObs.read(reader); + if (!reviewThreadsModel) { return; } - // Initial fetch of review threads - data.reviewThreadsModel.refresh().catch(err => { - this._logService.error('[CodeReviewService] Failed to fetch PR review threads:', err); - data.state.set({ kind: PRReviewStateKind.Error, reason: String(err) }, undefined); - }); + const data = this._getOrCreatePRReviewData(activeSessionResource); + if (data.state.read(undefined).kind === PRReviewStateKind.None) { + data.state.set({ kind: PRReviewStateKind.Loading }, undefined); + } - // Start polling of review threads - data.pollingDisposable.add(data.reviewThreadsModel.startPolling()); + const session = this._sessionsManagementService.getSession(activeSessionResource); + const workspace = session?.workspace.read(undefined); + + // Map review threads -> local state. + reader.store.add(autorun(innerReader => { + const threads = reviewThreadsModel.reviewThreads.read(innerReader); + const converted = this._convertedPRCommentsBySession.get(activeSessionResource.toString()); + const comments: IPRReviewComment[] = []; + + for (const thread of threads) { + if (thread.isResolved) { + continue; + } + const threadId = String(thread.id); + if (converted?.has(threadId)) { + continue; + } + const baseUri = workspace?.repositories[0]?.workingDirectory ?? workspace?.repositories[0]?.uri; + if (!baseUri) { + continue; + } + const fileUri = URI.joinPath(baseUri, thread.path); + const line = thread.line ?? 1; + const firstComment = thread.comments[0]; + comments.push({ + id: String(thread.id), + uri: fileUri, + range: new Range(line, 1, line, 1), + body: firstComment?.body ?? '', + author: firstComment?.author.login ?? '', + }); + } - reader.store.add(toDisposable(() => { - data.pollingDisposable.clear(); + data.state.set({ kind: PRReviewStateKind.Loaded, comments }, undefined); })); })); } @@ -631,11 +641,13 @@ export class CodeReviewService extends Disposable implements ICodeReviewService const session = this._sessionsManagementService.getSession(sessionResource); const gitHubInfo = session?.gitHubInfo.get(); if (gitHubInfo?.pullRequest) { - const reviewThreadsModel = this._gitHubService.getPullRequestReviewThreads(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequest.number); + const modelRef = this._gitHubService.createPullRequestReviewThreadsModelReference(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequest.number); try { - await reviewThreadsModel.resolveThread(threadId); + await modelRef.object.resolveThread(threadId); } catch (err) { this._logService.warn('[CodeReviewService] Failed to resolve PR thread on GitHub:', err); + } finally { + modelRef.dispose(); } } @@ -676,85 +688,19 @@ export class CodeReviewService extends Disposable implements ICodeReviewService if (!data) { data = { state: observableValue(`prReview.state.${key}`, { kind: PRReviewStateKind.None }), - disposables: new DisposableStore(), - pollingDisposable: new DisposableStore(), - initialized: false, }; - data.disposables.add(data.pollingDisposable); this._prReviewBySession.set(key, data); } return data; } - private _ensurePRReviewInitialized(sessionResource: URI, gitHubInfo: { owner: string; repo: string; pullRequestNumber: number } | undefined): IPRSessionReviewData { - const data = this._getOrCreatePRReviewData(sessionResource); - if (data.initialized) { - return data; - } - - const session = this._sessionsManagementService.getSession(sessionResource); - if (!session || !gitHubInfo) { - return data; - } - - data.initialized = true; - data.state.set({ kind: PRReviewStateKind.Loading }, undefined); - - const reviewThreadsModel = this._gitHubService.getPullRequestReviewThreads(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequestNumber); - const workspace = session.workspace.get(); - data.reviewThreadsModel = reviewThreadsModel; - - // Watch the PR review threads model and map to local state - data.disposables.add(autorun(reader => { - const threads = reviewThreadsModel.reviewThreads.read(reader); - const converted = this._convertedPRCommentsBySession.get(sessionResource.toString()); - const comments: IPRReviewComment[] = []; - - for (const thread of threads) { - if (thread.isResolved) { - continue; - } - const threadId = String(thread.id); - if (converted?.has(threadId)) { - continue; - } - const baseUri = workspace?.repositories[0]?.workingDirectory ?? workspace?.repositories[0]?.uri; - if (!baseUri) { - continue; - } - const fileUri = URI.joinPath(baseUri, thread.path); - const line = thread.line ?? 1; - const firstComment = thread.comments[0]; - comments.push({ - id: String(thread.id), - uri: fileUri, - range: new Range(line, 1, line, 1), - body: firstComment?.body ?? '', - author: firstComment?.author.login ?? '', - }); - } - - data.state.set({ kind: PRReviewStateKind.Loaded, comments }, undefined); - })); - - return data; - } - private _disposePRReview(sessionResource: URI): void { const key = sessionResource.toString(); this._convertedPRCommentsBySession.delete(key); - const data = this._prReviewBySession.get(key); - if (data) { - data.disposables.dispose(); - - this._prReviewBySession.delete(key); - } + this._prReviewBySession.delete(key); } override dispose(): void { - for (const data of this._prReviewBySession.values()) { - data.disposables.dispose(); - } this._prReviewBySession.clear(); super.dispose(); diff --git a/src/vs/sessions/contrib/codeReview/test/browser/codeReviewService.test.ts b/src/vs/sessions/contrib/codeReview/test/browser/codeReviewService.test.ts index 27c14bfd805070..d712d6e0159553 100644 --- a/src/vs/sessions/contrib/codeReview/test/browser/codeReviewService.test.ts +++ b/src/vs/sessions/contrib/codeReview/test/browser/codeReviewService.test.ts @@ -7,10 +7,10 @@ import assert from 'assert'; import { Codicon } from '../../../../../base/common/codicons.js'; import { URI } from '../../../../../base/common/uri.js'; import { Range } from '../../../../../editor/common/core/range.js'; -import { IObservable, observableValue } from '../../../../../base/common/observable.js'; +import { IObservable, derived, observableValue } from '../../../../../base/common/observable.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; -import { DisposableStore, IDisposable, toDisposable } from '../../../../../base/common/lifecycle.js'; +import { DisposableStore, ImmortalReference, IReference } from '../../../../../base/common/lifecycle.js'; import { ICommandService } from '../../../../../platform/commands/common/commands.js'; import { Emitter, Event } from '../../../../../base/common/event.js'; import { mock } from '../../../../../base/test/common/mock.js'; @@ -19,7 +19,6 @@ import { InMemoryStorageService, IStorageService, StorageScope, StorageTarget } import { IChatSessionFileChange, IChatSessionFileChange2 } from '../../../../../workbench/contrib/chat/common/chatSessionsService.js'; import { IGitHubService } from '../../../github/browser/githubService.js'; import { GitHubPRFetcher } from '../../../github/browser/fetchers/githubPRFetcher.js'; -import { GitHubPullRequestModel } from '../../../github/browser/models/githubPullRequestModel.js'; import { GitHubPullRequestReviewThreadsModel } from '../../../github/browser/models/githubPullRequestReviewThreadsModel.js'; import { IGitHubPRComment, IGitHubPullRequestReviewThread } from '../../../github/common/types.js'; import { IGitHubInfo, ISession, ISessionWorkspace } from '../../../../services/sessions/common/session.js'; @@ -205,45 +204,30 @@ suite('CodeReviewService', () => { } } - class MockGitHubPullRequestReviewThreadsModel extends GitHubPullRequestReviewThreadsModel { - startPollingCalls = 0; - stopPollingCalls = 0; - - override startPolling(intervalMs?: number): IDisposable { - this.startPollingCalls++; - const polling = super.startPolling(intervalMs); - return toDisposable(() => { - this.stopPollingCalls++; - polling.dispose(); - }); - } - } - class MockGitHubService extends mock() { readonly legacyFetcher = new MockReviewThreadsFetcher(); readonly reviewThreadsFetcher = new MockReviewThreadsFetcher(); - private readonly _pullRequestModel: GitHubPullRequestModel; - private readonly _reviewThreadsModels = new Map(); + private readonly _reviewThreadsModels = new Map(); private readonly _reviewThreadsFetchers = new Map(); getPullRequestCalls = 0; getPullRequestReviewThreadsCalls = 0; - constructor(disposables: DisposableStore, logService: ILogService) { + override readonly activeSessionPullRequestReviewThreadsObs: IObservable; + + constructor(sessionsManagementService: MockSessionsManagementService) { super(); - this._pullRequestModel = disposables.add(new GitHubPullRequestModel('owner', 'repo', 1, this.legacyFetcher as unknown as GitHubPRFetcher, logService)); this._reviewThreadsFetchers.set(this._key('owner', 'repo', 1), this.reviewThreadsFetcher); - } - - override getPullRequest(): GitHubPullRequestModel { - this.getPullRequestCalls++; - return this._pullRequestModel; - } - override getPullRequestReviewThreads(owner: string, repo: string, prNumber: number): GitHubPullRequestReviewThreadsModel { - this.getPullRequestReviewThreadsCalls++; - return this.getReviewThreadsModel(owner, repo, prNumber); + this.activeSessionPullRequestReviewThreadsObs = derived(reader => { + const session = sessionsManagementService.activeSession.read(reader); + const gitHubInfo = session?.gitHubInfo.read(reader); + if (!gitHubInfo?.pullRequest) { + return undefined; + } + return this.getReviewThreadsModel(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequest.number); + }); } getReviewThreadsFetcher(owner: string, repo: string, prNumber: number): MockReviewThreadsFetcher { @@ -256,16 +240,21 @@ suite('CodeReviewService', () => { return fetcher; } - getReviewThreadsModel(owner: string, repo: string, prNumber: number): MockGitHubPullRequestReviewThreadsModel { + getReviewThreadsModel(owner: string, repo: string, prNumber: number): GitHubPullRequestReviewThreadsModel { const key = this._key(owner, repo, prNumber); let model = this._reviewThreadsModels.get(key); if (!model) { - model = store.add(new MockGitHubPullRequestReviewThreadsModel(owner, repo, prNumber, this.getReviewThreadsFetcher(owner, repo, prNumber) as unknown as GitHubPRFetcher, new NullLogService())); + model = store.add(new GitHubPullRequestReviewThreadsModel(owner, repo, prNumber, this.getReviewThreadsFetcher(owner, repo, prNumber) as unknown as GitHubPRFetcher, new NullLogService())); this._reviewThreadsModels.set(key, model); } return model; } + override createPullRequestReviewThreadsModelReference(owner: string, repo: string, prNumber: number): IReference { + this.getPullRequestReviewThreadsCalls++; + return new ImmortalReference(this.getReviewThreadsModel(owner, repo, prNumber)); + } + private _key(owner: string, repo: string, prNumber: number): string { return `${owner}/${repo}#${prNumber}`; } @@ -278,12 +267,13 @@ suite('CodeReviewService', () => { instantiationService.stub(ICommandService, commandService); const logService = new NullLogService(); instantiationService.stub(ILogService, logService); - gitHubService = new MockGitHubService(store, logService); - instantiationService.stub(IGitHubService, gitHubService); sessionsManagement = new MockSessionsManagementService(store); instantiationService.stub(ISessionsManagementService, sessionsManagement); + gitHubService = new MockGitHubService(sessionsManagement); + instantiationService.stub(IGitHubService, gitHubService); + storageService = store.add(new InMemoryStorageService()); instantiationService.stub(IStorageService, storageService); @@ -326,6 +316,10 @@ suite('CodeReviewService', () => { sessionsManagement.setActiveSession(session); await tick(); + + // Polling is owned by GitHubPullRequestPollingContribution; refresh + // manually here to seed the review threads model with data. + await gitHubService.getReviewThreadsModel('owner', 'repo', 1).refresh(); await tick(); const state = service.getPRReviewState(session).get(); @@ -340,72 +334,13 @@ suite('CodeReviewService', () => { }, { comments: [{ id: 'thread-100', uri: 'file:///workspace/src/a.ts', body: 'Comment on src/a.ts', author: 'reviewer' }], getPullRequestCalls: 0, - getPullRequestReviewThreadsCalls: 1, + getPullRequestReviewThreadsCalls: 0, legacyThreadRefreshes: 0, reviewThreadRefreshes: 1, }); } }); - test('only active session PR review model is polled', async () => { - const session2 = URI.parse('test://session/2'); - sessionsManagement.addSession(session); - sessionsManagement.setGitHubInfo(session, makeGitHubInfo(1)); - sessionsManagement.addSession(session2); - sessionsManagement.setGitHubInfo(session2, makeGitHubInfo(2)); - gitHubService.getReviewThreadsFetcher('owner', 'repo', 1).nextThreads = [makePRThread('thread-100', 'src/a.ts')]; - gitHubService.getReviewThreadsFetcher('owner', 'repo', 2).nextThreads = [makePRThread('thread-200', 'src/b.ts')]; - - sessionsManagement.setActiveSession(session); - await tick(); - await tick(); - - const session1Model = gitHubService.getReviewThreadsModel('owner', 'repo', 1); - const session2Model = gitHubService.getReviewThreadsModel('owner', 'repo', 2); - assert.deepStrictEqual({ - session1StartPollingCalls: session1Model.startPollingCalls, - session1StopPollingCalls: session1Model.stopPollingCalls, - session2StartPollingCalls: session2Model.startPollingCalls, - session2StopPollingCalls: session2Model.stopPollingCalls, - }, { - session1StartPollingCalls: 1, - session1StopPollingCalls: 0, - session2StartPollingCalls: 0, - session2StopPollingCalls: 0, - }); - - sessionsManagement.setActiveSession(session2); - await tick(); - await tick(); - - assert.deepStrictEqual({ - session1StartPollingCalls: session1Model.startPollingCalls, - session1StopPollingCalls: session1Model.stopPollingCalls, - session2StartPollingCalls: session2Model.startPollingCalls, - session2StopPollingCalls: session2Model.stopPollingCalls, - }, { - session1StartPollingCalls: 1, - session1StopPollingCalls: 1, - session2StartPollingCalls: 1, - session2StopPollingCalls: 0, - }); - - sessionsManagement.setActiveSession(undefined); - await tick(); - - assert.deepStrictEqual({ - session1StartPollingCalls: session1Model.startPollingCalls, - session1StopPollingCalls: session1Model.stopPollingCalls, - session2StartPollingCalls: session2Model.startPollingCalls, - session2StopPollingCalls: session2Model.stopPollingCalls, - }, { - session1StartPollingCalls: 1, - session1StopPollingCalls: 1, - session2StartPollingCalls: 1, - session2StopPollingCalls: 1, - }); - }); - test('resolvePRReviewThread uses dedicated review threads model', async () => { sessionsManagement.addSession(session); sessionsManagement.setGitHubInfo(session, makeGitHubInfo()); diff --git a/src/vs/sessions/contrib/copilotChatSessions/browser/copilotChatSessionsProvider.ts b/src/vs/sessions/contrib/copilotChatSessions/browser/copilotChatSessionsProvider.ts index eaf38381f17ebc..5d058db667ce3f 100644 --- a/src/vs/sessions/contrib/copilotChatSessions/browser/copilotChatSessionsProvider.ts +++ b/src/vs/sessions/contrib/copilotChatSessions/browser/copilotChatSessionsProvider.ts @@ -918,8 +918,8 @@ class AgentSessionAdapter implements ICopilotChatSession { if (!base?.pullRequest || !this._gitHubService) { return base; } - const prModel = this._gitHubService.getPullRequest(base.owner, base.repo, base.pullRequest.number); - const livePR = prModel.pullRequest.read(reader); + const prModelRef = reader.store.add(this._gitHubService.createPullRequestModelReference(base.owner, base.repo, base.pullRequest.number)); + const livePR = prModelRef.object.pullRequest.read(reader); if (!livePR) { return base; } diff --git a/src/vs/sessions/contrib/github/browser/github.contribution.ts b/src/vs/sessions/contrib/github/browser/github.contribution.ts index 8fac796e1ff46c..8951162f31c68d 100644 --- a/src/vs/sessions/contrib/github/browser/github.contribution.ts +++ b/src/vs/sessions/contrib/github/browser/github.contribution.ts @@ -3,8 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { structuralEquals } from '../../../../base/common/equals.js'; -import { Disposable, DisposableMap } from '../../../../base/common/lifecycle.js'; +import { Disposable, DisposableMap, DisposableStore } from '../../../../base/common/lifecycle.js'; import { autorun, derivedOpts } from '../../../../base/common/observable.js'; import { isEqual } from '../../../../base/common/resources.js'; import { URI } from '../../../../base/common/uri.js'; @@ -28,34 +27,55 @@ export class GitHubPullRequestPollingContribution extends Disposable implements super(); const activeSessionResourceObs = derivedOpts({ equalsFn: isEqual }, reader => { - return this._sessionsManagementService.activeSession.read(reader)?.resource; - }); - - const gitHubInfoObs = derivedOpts<{ owner: string; repo: string; pullRequestNumber: number } | undefined>({ equalsFn: structuralEquals }, reader => { - const gitHubInfo = this._sessionsManagementService.activeSession.read(reader)?.gitHubInfo.read(reader); - if (!gitHubInfo?.pullRequest) { + const activeSession = this._sessionsManagementService.activeSession.read(reader); + if (!activeSession || !activeSession.resource || activeSession.isArchived.read(reader)) { return undefined; } - return { - owner: gitHubInfo.owner, - repo: gitHubInfo.repo, - pullRequestNumber: gitHubInfo.pullRequest.number, - }; + return activeSession.resource; }); + // Pull request model this._register(autorun(reader => { const activeSessionResource = activeSessionResourceObs.read(reader); - const activeSession = this._sessionsManagementService.activeSession.read(reader); - if (!activeSessionResource || !activeSession || activeSession.isArchived.read(reader)) { + if (!activeSessionResource) { return; } - const gitHubInfo = gitHubInfoObs.read(reader); - if (!gitHubInfo) { + + const model = this._gitHubService.activeSessionPullRequestObs.read(reader); + model?.refresh(); + })); + + // Pull request CI model + this._register(autorun(reader => { + const activeSessionResource = activeSessionResourceObs.read(reader); + if (!activeSessionResource) { + return; + } + + const model = this._gitHubService.activeSessionPullRequestCIObs.read(reader); + if (!model) { return; } - const prModel = this._gitHubService.getPullRequest(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequestNumber); - prModel.refresh(); + + model.refresh(); + reader.store.add(model.startPolling()); + })); + + // Pull request review threads model + this._register(autorun(reader => { + const activeSessionResource = activeSessionResourceObs.read(reader); + if (!activeSessionResource) { + return; + } + + const model = this._gitHubService.activeSessionPullRequestReviewThreadsObs.read(reader); + if (!model) { + return; + } + + model.refresh(); + reader.store.add(model.startPolling()); })); this._sessionsManagementService.onDidChangeSessions(this._onDidChangeSessions, this, this._store); @@ -101,8 +121,13 @@ export class GitHubPullRequestPollingContribution extends Disposable implements return; } - const model = this._gitHubService.getPullRequest(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequest.number); - this._pullRequests.set(key, model.startPolling()); + const disposables = new DisposableStore(); + const modelRef = this._gitHubService.createPullRequestModelReference(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequest.number); + + disposables.add(modelRef); + disposables.add(modelRef.object.startPolling()); + + this._pullRequests.set(key, disposables); } private _disposePolling(session: ISession): void { diff --git a/src/vs/sessions/contrib/github/browser/githubService.ts b/src/vs/sessions/contrib/github/browser/githubService.ts index 61025047583ae5..06157b33238f7f 100644 --- a/src/vs/sessions/contrib/github/browser/githubService.ts +++ b/src/vs/sessions/contrib/github/browser/githubService.ts @@ -3,52 +3,46 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { Disposable, DisposableMap } from '../../../../base/common/lifecycle.js'; +import { Disposable, IReference } from '../../../../base/common/lifecycle.js'; import { createDecorator, IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js'; -import { ILogService } from '../../../../platform/log/common/log.js'; import { IGitHubChangedFile } from '../common/types.js'; import { GitHubApiClient } from './githubApiClient.js'; -import { GitHubRepositoryFetcher } from './fetchers/githubRepositoryFetcher.js'; -import { GitHubPRFetcher } from './fetchers/githubPRFetcher.js'; -import { GitHubPRCIFetcher } from './fetchers/githubPRCIFetcher.js'; -import { GitHubRepositoryModel } from './models/githubRepositoryModel.js'; -import { GitHubPullRequestModel } from './models/githubPullRequestModel.js'; -import { GitHubPullRequestReviewThreadsModel } from './models/githubPullRequestReviewThreadsModel.js'; -import { GitHubPullRequestCIModel } from './models/githubPullRequestCIModel.js'; +import { GitHubRepositoryModel, GitHubRepositoryModelReferenceCollection } from './models/githubRepositoryModel.js'; +import { GitHubPullRequestModel, GitHubPullRequestModelReferenceCollection } from './models/githubPullRequestModel.js'; +import { GitHubPullRequestReviewThreadsModel, GitHubPullRequestReviewThreadsModelReferenceCollection } from './models/githubPullRequestReviewThreadsModel.js'; +import { GitHubPullRequestCIModel, GitHubPullRequestCIModelReferenceCollection } from './models/githubPullRequestCIModel.js'; import { GitHubChangesFetcher } from './fetchers/githubChangesFetcher.js'; import { getPullRequestKey } from '../common/utils.js'; +import { derived, derivedOpts, IObservable } from '../../../../base/common/observable.js'; +import { structuralEquals } from '../../../../base/common/equals.js'; +import { ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; export interface IGitHubService { readonly _serviceBrand: undefined; - /** - * Get or create a reactive model for a GitHub repository. - * The model is cached by owner/repo key and disposed when the service is disposed. - */ - getRepository(owner: string, repo: string): GitHubRepositoryModel; + activeSessionPullRequestObs: IObservable; + activeSessionPullRequestCIObs: IObservable; + activeSessionPullRequestReviewThreadsObs: IObservable; /** - * Get or create a reactive model for a GitHub pull request. - * The model is cached by owner/repo/prNumber key and disposed when the service is disposed. + * Get a reference to a reactive model for a GitHub repository. */ - getPullRequest(owner: string, repo: string, prNumber: number): GitHubPullRequestModel; + createRepositoryModelReference(owner: string, repo: string): IReference; /** - * Dispose and remove cached models associated with a GitHub pull request, if they exist. + * Get a reference to a reactive model for a GitHub pull request. */ - disposePullRequest(owner: string, repo: string, prNumber: number): void; + createPullRequestModelReference(owner: string, repo: string, prNumber: number): IReference; /** - * Get or create a reactive model for review threads on a GitHub pull request. - * The model is cached by owner/repo/prNumber key and disposed when the service is disposed. + * Get a reference to a reactive model for review threads on a GitHub pull request. */ - getPullRequestReviewThreads(owner: string, repo: string, prNumber: number): GitHubPullRequestReviewThreadsModel; + createPullRequestReviewThreadsModelReference(owner: string, repo: string, prNumber: number): IReference; /** - * Get or create a reactive model for CI checks on a pull request head SHA. - * The model is cached by owner/repo/prNumber/headSha key and disposed when the service is disposed. + * Get a reference to a reactive model for CI checks on a pull request head SHA. */ - getPullRequestCI(owner: string, repo: string, prNumber: number, headSha: string): GitHubPullRequestCIModel; + createPullRequestCIModelReference(owner: string, repo: string, prNumber: number, headSha: string): IReference; /** * List files changed between two refs using the GitHub compare API. @@ -58,105 +52,121 @@ export interface IGitHubService { export const IGitHubService = createDecorator('sessionsGitHubService'); -const LOG_PREFIX = '[GitHubService]'; - export class GitHubService extends Disposable implements IGitHubService { declare readonly _serviceBrand: undefined; - private readonly _apiClient: GitHubApiClient; - private readonly _repoFetcher: GitHubRepositoryFetcher; - private readonly _changesFetcher: GitHubChangesFetcher; - private readonly _prFetcher: GitHubPRFetcher; - private readonly _ciFetcher: GitHubPRCIFetcher; + readonly activeSessionPullRequestObs: IObservable; + readonly activeSessionPullRequestCIObs: IObservable; + readonly activeSessionPullRequestReviewThreadsObs: IObservable; - private readonly _repositories = this._register(new DisposableMap()); - private readonly _pullRequests = this._register(new DisposableMap()); - private readonly _pullRequestReviewThreads = this._register(new DisposableMap()); - private readonly _ciModels = this._register(new DisposableMap>()); + private readonly _changesFetcher: GitHubChangesFetcher; + private readonly _repositoryReferences: GitHubRepositoryModelReferenceCollection; + private readonly _pullRequestReferences: GitHubPullRequestModelReferenceCollection; + private readonly _pullRequestReviewThreadsReferences: GitHubPullRequestReviewThreadsModelReferenceCollection; + private readonly _pullRequestCIReferences: GitHubPullRequestCIModelReferenceCollection; constructor( @IInstantiationService instantiationService: IInstantiationService, - @ILogService private readonly _logService: ILogService, + @ISessionsManagementService sessionManagementService: ISessionsManagementService, ) { super(); - this._apiClient = this._register(instantiationService.createInstance(GitHubApiClient)); - - this._repoFetcher = new GitHubRepositoryFetcher(this._apiClient); - this._changesFetcher = new GitHubChangesFetcher(this._apiClient); - this._prFetcher = new GitHubPRFetcher(this._apiClient); - this._ciFetcher = new GitHubPRCIFetcher(this._apiClient); + const apiClient = this._register(instantiationService.createInstance(GitHubApiClient)); + + this._changesFetcher = new GitHubChangesFetcher(apiClient); + + this._repositoryReferences = instantiationService.createInstance(GitHubRepositoryModelReferenceCollection, apiClient); + this._pullRequestReferences = instantiationService.createInstance(GitHubPullRequestModelReferenceCollection, apiClient); + this._pullRequestReviewThreadsReferences = instantiationService.createInstance(GitHubPullRequestReviewThreadsModelReferenceCollection, apiClient); + this._pullRequestCIReferences = instantiationService.createInstance(GitHubPullRequestCIModelReferenceCollection, apiClient); + + const gitHubInfoObs = derivedOpts<{ owner: string; repo: string; pullRequestNumber: number } | undefined>({ equalsFn: structuralEquals }, + reader => { + const gitHubInfo = sessionManagementService.activeSession.read(reader)?.gitHubInfo.read(reader); + + if (!gitHubInfo?.pullRequest) { + return undefined; + } + + return { + owner: gitHubInfo.owner, + repo: gitHubInfo.repo, + pullRequestNumber: gitHubInfo.pullRequest.number + }; + }); + + this.activeSessionPullRequestObs = derived(reader => { + const gitHubInfo = gitHubInfoObs.read(reader); + if (!gitHubInfo) { + return undefined; + } + + const prModelRef = this.createPullRequestModelReference(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequestNumber); + reader.store.add(prModelRef); + + return prModelRef.object; + }); + + const pullRequestInfoObs = derivedOpts<{ owner: string; repo: string; prNumber: number; headSha: string } | undefined>({ equalsFn: structuralEquals }, + reader => { + const pullRequest = this.activeSessionPullRequestObs.read(reader); + const pullRequestDetails = pullRequest?.pullRequest.read(reader); + + if (!pullRequest || !pullRequestDetails) { + return undefined; + } + + return { + owner: pullRequest.owner, + repo: pullRequest.repo, + prNumber: pullRequest.prNumber, + headSha: pullRequestDetails.headSha + }; + }); + + this.activeSessionPullRequestCIObs = derived(reader => { + const pullRequestInfo = pullRequestInfoObs.read(reader); + if (!pullRequestInfo) { + return undefined; + } + + const prModelRef = this.createPullRequestCIModelReference(pullRequestInfo.owner, pullRequestInfo.repo, pullRequestInfo.prNumber, pullRequestInfo.headSha); + reader.store.add(prModelRef); + + return prModelRef.object; + }); + + this.activeSessionPullRequestReviewThreadsObs = derived(reader => { + const gitHubInfo = gitHubInfoObs.read(reader); + if (!gitHubInfo) { + return undefined; + } + + const reviewThreadsModelRef = this.createPullRequestReviewThreadsModelReference(gitHubInfo.owner, gitHubInfo.repo, gitHubInfo.pullRequestNumber); + reader.store.add(reviewThreadsModelRef); + + return reviewThreadsModelRef.object; + }); } - getRepository(owner: string, repo: string): GitHubRepositoryModel { - const key = `${owner}/${repo}`; - let model = this._repositories.get(key); - if (!model) { - this._logService.trace(`${LOG_PREFIX} Creating repository model for ${key}`); - model = new GitHubRepositoryModel(owner, repo, this._repoFetcher, this._logService); - this._repositories.set(key, model); - } - return model; + createRepositoryModelReference(owner: string, repo: string): IReference { + return this._repositoryReferences.acquire(`${owner}/${repo}`, owner, repo); } - getPullRequest(owner: string, repo: string, prNumber: number): GitHubPullRequestModel { - const key = getPullRequestKey(owner, repo, prNumber); - let model = this._pullRequests.get(key); - if (!model) { - this._logService.trace(`${LOG_PREFIX} Creating PR model for ${key}`); - model = new GitHubPullRequestModel(owner, repo, prNumber, this._prFetcher, this._logService); - this._pullRequests.set(key, model); - } - return model; + createPullRequestModelReference(owner: string, repo: string, prNumber: number): IReference { + return this._pullRequestReferences.acquire(getPullRequestKey(owner, repo, prNumber), owner, repo, prNumber); } - getPullRequestReviewThreads(owner: string, repo: string, prNumber: number): GitHubPullRequestReviewThreadsModel { - const key = getPullRequestKey(owner, repo, prNumber); - let model = this._pullRequestReviewThreads.get(key); - if (!model) { - this._logService.trace(`${LOG_PREFIX} Creating PR review threads model for ${key}`); - model = new GitHubPullRequestReviewThreadsModel(owner, repo, prNumber, this._prFetcher, this._logService); - this._pullRequestReviewThreads.set(key, model); - } - return model; + createPullRequestReviewThreadsModelReference(owner: string, repo: string, prNumber: number): IReference { + return this._pullRequestReviewThreadsReferences.acquire(getPullRequestKey(owner, repo, prNumber), owner, repo, prNumber); } - getPullRequestCI(owner: string, repo: string, prNumber: number, headSha: string): GitHubPullRequestCIModel { - const key = getPullRequestKey(owner, repo, prNumber); - let models = this._ciModels.get(key); - if (!models) { - models = new DisposableMap(); - this._ciModels.set(key, models); - } - - let model = models.get(headSha); - if (!model) { - models.clearAndDisposeAll(); - this._logService.trace(`${LOG_PREFIX} Creating CI model for ${key}/${headSha}`); - model = new GitHubPullRequestCIModel(owner, repo, headSha, this._ciFetcher, this._logService); - models.set(headSha, model); - } - return model; + createPullRequestCIModelReference(owner: string, repo: string, prNumber: number, headSha: string): IReference { + return this._pullRequestCIReferences.acquire(`${getPullRequestKey(owner, repo, prNumber)}/${headSha}`, owner, repo, prNumber, headSha); } getChangedFiles(owner: string, repo: string, base: string, head: string): Promise { return this._changesFetcher.getChangedFiles(owner, repo, base, head); } - - disposePullRequest(owner: string, repo: string, prNumber: number): void { - const key = getPullRequestKey(owner, repo, prNumber); - - this._pullRequests.deleteAndDispose(key); - this._pullRequestReviewThreads.deleteAndDispose(key); - this._ciModels.deleteAndDispose(key); - } - - override dispose(): void { - this._pullRequests.dispose(); - this._pullRequestReviewThreads.dispose(); - this._ciModels.dispose(); - - super.dispose(); - } } diff --git a/src/vs/sessions/contrib/github/browser/models/githubPullRequestCIModel.ts b/src/vs/sessions/contrib/github/browser/models/githubPullRequestCIModel.ts index 57cb15f4f0ae40..99b8c8ddec9e9b 100644 --- a/src/vs/sessions/contrib/github/browser/models/githubPullRequestCIModel.ts +++ b/src/vs/sessions/contrib/github/browser/models/githubPullRequestCIModel.ts @@ -4,15 +4,38 @@ *--------------------------------------------------------------------------------------------*/ import { RunOnceScheduler } from '../../../../../base/common/async.js'; -import { Disposable, IDisposable, toDisposable } from '../../../../../base/common/lifecycle.js'; +import { Disposable, IDisposable, ReferenceCollection, toDisposable } from '../../../../../base/common/lifecycle.js'; import { IObservable, observableValue } from '../../../../../base/common/observable.js'; import { ILogService } from '../../../../../platform/log/common/log.js'; import { GitHubCIOverallStatus, IGitHubCICheck } from '../../common/types.js'; +import { GitHubApiClient } from '../githubApiClient.js'; import { computeOverallCIStatus, GitHubPRCIFetcher } from '../fetchers/githubPRCIFetcher.js'; const LOG_PREFIX = '[GitHubPullRequestCIModel]'; const DEFAULT_POLL_INTERVAL_MS = 60_000; +export class GitHubPullRequestCIModelReferenceCollection extends ReferenceCollection { + private readonly _fetcher: GitHubPRCIFetcher; + + constructor( + apiClient: GitHubApiClient, + @ILogService private readonly _logService: ILogService + ) { + super(); + this._fetcher = new GitHubPRCIFetcher(apiClient); + } + + protected override createReferencedObject(key: string, owner: string, repo: string, prNumber: number, headSha: string): GitHubPullRequestCIModel { + this._logService.trace(`[GitHubPullRequestCIModelReferenceCollection][createReferencedObject] Creating CI model for ${key}`); + return new GitHubPullRequestCIModel(owner, repo, prNumber, headSha, this._fetcher, this._logService); + } + + protected override destroyReferencedObject(key: string, object: GitHubPullRequestCIModel): void { + this._logService.trace(`[GitHubPullRequestCIModelReferenceCollection][destroyReferencedObject] Disposing CI model for ${key}`); + object.dispose(); + } +} + /** * Reactive model for CI check status on a pull request head ref. * Wraps fetcher data in observables and supports periodic polling. @@ -34,6 +57,7 @@ export class GitHubPullRequestCIModel extends Disposable { constructor( readonly owner: string, readonly repo: string, + readonly prNumber: number, readonly headSha: string, private readonly _fetcher: GitHubPRCIFetcher, private readonly _logService: ILogService, @@ -74,7 +98,7 @@ export class GitHubPullRequestCIModel extends Disposable { this._overallStatus.set(computeOverallCIStatus(response.data), undefined); } } catch (err) { - this._logService.error(`${LOG_PREFIX} Failed to refresh CI checks for ${this.owner}/${this.repo}@${this.headSha}:`, err); + this._logService.error(`${LOG_PREFIX} Failed to refresh CI checks for ${this.owner}/${this.repo}#${this.prNumber}@${this.headSha}:`, err); } } diff --git a/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts b/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts index 31d1132b09887d..3c39d0d76d4412 100644 --- a/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts +++ b/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts @@ -4,15 +4,38 @@ *--------------------------------------------------------------------------------------------*/ import { RunOnceScheduler } from '../../../../../base/common/async.js'; -import { Disposable, IDisposable, toDisposable } from '../../../../../base/common/lifecycle.js'; +import { Disposable, DisposableSet, IDisposable, ReferenceCollection, toDisposable } from '../../../../../base/common/lifecycle.js'; import { IObservable, observableValue, transaction } from '../../../../../base/common/observable.js'; import { ILogService } from '../../../../../platform/log/common/log.js'; import { IGitHubPRComment, IGitHubPullRequest, IGitHubPullRequestMergeability, IGitHubPullRequestReview } from '../../common/types.js'; import { computeMergeability, GitHubPRFetcher } from '../fetchers/githubPRFetcher.js'; +import { GitHubApiClient } from '../githubApiClient.js'; const LOG_PREFIX = '[GitHubPullRequestModel]'; const DEFAULT_POLL_INTERVAL_MS = 60_000; +export class GitHubPullRequestModelReferenceCollection extends ReferenceCollection { + private readonly _fetcher: GitHubPRFetcher; + + constructor( + apiClient: GitHubApiClient, + @ILogService private readonly _logService: ILogService + ) { + super(); + this._fetcher = new GitHubPRFetcher(apiClient); + } + + protected override createReferencedObject(key: string, owner: string, repo: string, prNumber: number): GitHubPullRequestModel { + this._logService.trace(`[GitHubPullRequestModelReferenceCollection][createReferencedObject] Creating PR model for ${key}`); + return new GitHubPullRequestModel(owner, repo, prNumber, this._fetcher, this._logService); + } + + protected override destroyReferencedObject(key: string, object: GitHubPullRequestModel): void { + this._logService.trace(`[GitHubPullRequestModelReferenceCollection][destroyReferencedObject] Disposing PR model for ${key}`); + object.dispose(); + } +} + /** * Reactive model for a GitHub pull request. Wraps fetcher data in * observables, supports on-demand refresh, and can poll periodically. @@ -32,8 +55,8 @@ export class GitHubPullRequestModel extends Disposable { private _refreshPromise: Promise | undefined = undefined; - private _pollingClientCount = 0; private readonly _pollScheduler: RunOnceScheduler; + private readonly _pollingDisposables = this._register(new DisposableSet()); constructor( readonly owner: string, @@ -72,25 +95,26 @@ export class GitHubPullRequestModel extends Disposable { * Start periodic polling. Each cycle refreshes all PR data. */ startPolling(intervalMs: number = DEFAULT_POLL_INTERVAL_MS): IDisposable { - if (this._pollingClientCount++ === 0) { - this._pollScheduler.schedule(intervalMs); - } - - return toDisposable(() => { - if (this._store.isDisposed) { - return; - } + const disposable = toDisposable(() => { + this._pollingDisposables.deleteAndDispose(disposable); - if (--this._pollingClientCount === 0) { + if (this._pollingDisposables.size === 0) { this._pollScheduler.cancel(); } }); + this._pollingDisposables.add(disposable); + + if (this._pollingDisposables.size === 1) { + this._pollScheduler.schedule(intervalMs); + } + + return disposable; } private async _poll(): Promise { await this.refresh(); // Re-schedule for next poll cycle (RunOnceScheduler is one-shot) - if (!this._store.isDisposed && this._pollingClientCount > 0) { + if (!this._store.isDisposed && this._pollingDisposables.size > 0) { this._pollScheduler.schedule(); } } diff --git a/src/vs/sessions/contrib/github/browser/models/githubPullRequestReviewThreadsModel.ts b/src/vs/sessions/contrib/github/browser/models/githubPullRequestReviewThreadsModel.ts index b9c0d3222977cc..8d46b366339b58 100644 --- a/src/vs/sessions/contrib/github/browser/models/githubPullRequestReviewThreadsModel.ts +++ b/src/vs/sessions/contrib/github/browser/models/githubPullRequestReviewThreadsModel.ts @@ -4,15 +4,38 @@ *--------------------------------------------------------------------------------------------*/ import { RunOnceScheduler } from '../../../../../base/common/async.js'; -import { Disposable, IDisposable, toDisposable } from '../../../../../base/common/lifecycle.js'; +import { Disposable, IDisposable, ReferenceCollection, toDisposable } from '../../../../../base/common/lifecycle.js'; import { IObservable, observableValue } from '../../../../../base/common/observable.js'; import { ILogService } from '../../../../../platform/log/common/log.js'; import { IGitHubPRComment, IGitHubPullRequestReviewThread } from '../../common/types.js'; +import { GitHubApiClient } from '../githubApiClient.js'; import { GitHubPRFetcher } from '../fetchers/githubPRFetcher.js'; const LOG_PREFIX = '[GitHubPullRequestReviewThreadsModel]'; const DEFAULT_POLL_INTERVAL_MS = 60_000; +export class GitHubPullRequestReviewThreadsModelReferenceCollection extends ReferenceCollection { + private readonly _fetcher: GitHubPRFetcher; + + constructor( + apiClient: GitHubApiClient, + @ILogService private readonly _logService: ILogService + ) { + super(); + this._fetcher = new GitHubPRFetcher(apiClient); + } + + protected override createReferencedObject(key: string, owner: string, repo: string, prNumber: number): GitHubPullRequestReviewThreadsModel { + this._logService.trace(`[GitHubPullRequestReviewThreadsModelReferenceCollection][createReferencedObject] Creating PR review threads model for ${key}`); + return new GitHubPullRequestReviewThreadsModel(owner, repo, prNumber, this._fetcher, this._logService); + } + + protected override destroyReferencedObject(key: string, object: GitHubPullRequestReviewThreadsModel): void { + this._logService.trace(`[GitHubPullRequestReviewThreadsModelReferenceCollection][destroyReferencedObject] Disposing PR review threads model for ${key}`); + object.dispose(); + } +} + /** * Reactive model for GitHub pull request review threads. Review threads are * fetched through GraphQL, so they have a separate refresh and polling cadence diff --git a/src/vs/sessions/contrib/github/browser/models/githubRepositoryModel.ts b/src/vs/sessions/contrib/github/browser/models/githubRepositoryModel.ts index ccaa9405ab6e11..3eff420228320b 100644 --- a/src/vs/sessions/contrib/github/browser/models/githubRepositoryModel.ts +++ b/src/vs/sessions/contrib/github/browser/models/githubRepositoryModel.ts @@ -3,14 +3,37 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { Disposable } from '../../../../../base/common/lifecycle.js'; +import { Disposable, ReferenceCollection } from '../../../../../base/common/lifecycle.js'; import { IObservable, observableValue } from '../../../../../base/common/observable.js'; import { ILogService } from '../../../../../platform/log/common/log.js'; import { IGitHubRepository } from '../../common/types.js'; +import { GitHubApiClient } from '../githubApiClient.js'; import { GitHubRepositoryFetcher } from '../fetchers/githubRepositoryFetcher.js'; const LOG_PREFIX = '[GitHubRepositoryModel]'; +export class GitHubRepositoryModelReferenceCollection extends ReferenceCollection { + private readonly _fetcher: GitHubRepositoryFetcher; + + constructor( + apiClient: GitHubApiClient, + @ILogService private readonly _logService: ILogService + ) { + super(); + this._fetcher = new GitHubRepositoryFetcher(apiClient); + } + + protected override createReferencedObject(key: string, owner: string, repo: string): GitHubRepositoryModel { + this._logService.trace(`[GitHubRepositoryModelReferenceCollection][createReferencedObject] Creating repository model for ${key}`); + return new GitHubRepositoryModel(owner, repo, this._fetcher, this._logService); + } + + protected override destroyReferencedObject(key: string, object: GitHubRepositoryModel): void { + this._logService.trace(`[GitHubRepositoryModelReferenceCollection][destroyReferencedObject] Disposing repository model for ${key}`); + object.dispose(); + } +} + /** * Reactive model for a GitHub repository. Wraps fetcher data * in observables and supports on-demand refresh. diff --git a/src/vs/sessions/contrib/github/test/browser/githubContribution.test.ts b/src/vs/sessions/contrib/github/test/browser/githubContribution.test.ts index f66abb253cd2f4..0973ae476a8762 100644 --- a/src/vs/sessions/contrib/github/test/browser/githubContribution.test.ts +++ b/src/vs/sessions/contrib/github/test/browser/githubContribution.test.ts @@ -7,13 +7,13 @@ import assert from 'assert'; import { Codicon } from '../../../../../base/common/codicons.js'; import { Emitter, Event } from '../../../../../base/common/event.js'; import { IMarkdownString } from '../../../../../base/common/htmlContent.js'; -import { DisposableStore, IDisposable, toDisposable } from '../../../../../base/common/lifecycle.js'; +import { DisposableStore, IDisposable, ImmortalReference, IReference, toDisposable } from '../../../../../base/common/lifecycle.js'; import { IObservable, observableValue } from '../../../../../base/common/observable.js'; +import { GitHubPullRequestModel } from '../../browser/models/githubPullRequestModel.js'; import { URI } from '../../../../../base/common/uri.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { mock } from '../../../../../base/test/common/mock.js'; import { GitHubPullRequestPollingContribution } from '../../browser/github.contribution.js'; -import { GitHubPullRequestModel } from '../../browser/models/githubPullRequestModel.js'; import { IGitHubService } from '../../browser/githubService.js'; import { IChat, IGitHubInfo, ISession, ISessionCapabilities, ISessionFileChange, ISessionWorkspace, SessionStatus } from '../../../../services/sessions/common/session.js'; import { IActiveSession, ISessionsChangeEvent, ISessionsManagementService } from '../../../../services/sessions/common/sessionsManagement.js'; @@ -204,12 +204,18 @@ class TestGitHubService extends mock() { private readonly _models = new Map(); - override getPullRequest(owner: string, repo: string, prNumber: number): GitHubPullRequestModel { - return this._getModel(owner, repo, prNumber) as unknown as GitHubPullRequestModel; - } + override readonly activeSessionPullRequestObs = observableValue('test.activePR', undefined); + override readonly activeSessionPullRequestCIObs = observableValue('test.activePRCI', undefined); + override readonly activeSessionPullRequestReviewThreadsObs = observableValue('test.activePRReviewThreads', undefined); - override disposePullRequest(owner: string, repo: string, prNumber: number): void { - this._getModel(owner, repo, prNumber).dispose(); + override createPullRequestModelReference(owner: string, repo: string, prNumber: number): IReference { + const key = `${owner}/${repo}/${prNumber}`; + let model = this._models.get(key); + if (!model) { + model = new TestPullRequestModel(); + this._models.set(key, model); + } + return new ImmortalReference(model as unknown as GitHubPullRequestModel); } snapshot(): Record { @@ -220,16 +226,6 @@ class TestGitHubService extends mock() { }] as const); return Object.fromEntries(entries); } - - private _getModel(owner: string, repo: string, prNumber: number): TestPullRequestModel { - const key = `${owner}/${repo}/${prNumber}`; - let model = this._models.get(key); - if (!model) { - model = new TestPullRequestModel(); - this._models.set(key, model); - } - return model; - } } class TestPullRequestModel implements IDisposable { diff --git a/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts b/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts index 8b5a906e00bde1..8a7a7bec5c12ba 100644 --- a/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts +++ b/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts @@ -8,10 +8,10 @@ import { DeferredPromise, timeout } from '../../../../../base/common/async.js'; import { DisposableStore } from '../../../../../base/common/lifecycle.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { runWithFakedTimers } from '../../../../../base/test/common/timeTravelScheduler.js'; -import { NullLogService } from '../../../../../platform/log/common/log.js'; +import { NullLogService, ILogService } from '../../../../../platform/log/common/log.js'; import { GitHubPullRequestModel } from '../../browser/models/githubPullRequestModel.js'; import { GitHubPullRequestReviewThreadsModel } from '../../browser/models/githubPullRequestReviewThreadsModel.js'; -import { GitHubPullRequestCIModel, parseWorkflowRunId } from '../../browser/models/githubPullRequestCIModel.js'; +import { GitHubPullRequestCIModel, GitHubPullRequestCIModelReferenceCollection, parseWorkflowRunId } from '../../browser/models/githubPullRequestCIModel.js'; import { GitHubRepositoryModel } from '../../browser/models/githubRepositoryModel.js'; import { GitHubPRFetcher } from '../../browser/fetchers/githubPRFetcher.js'; import { GitHubPRCIFetcher } from '../../browser/fetchers/githubPRCIFetcher.js'; @@ -499,10 +499,18 @@ suite('GitHubPullRequestCIModel', () => { const store = new DisposableStore(); let mockFetcher: MockCIFetcher; + let collection: TestCIReferenceCollection; const logService = new NullLogService(); + function acquireModel(owner: string = 'owner', repo: string = 'repo', prNumber: number = 1, headSha: string = 'abc'): GitHubPullRequestCIModel { + const ref = collection.acquire(`${owner}/${repo}/${prNumber}/${headSha}`, owner, repo, prNumber, headSha); + store.add(ref); + return ref.object; + } + setup(() => { mockFetcher = new MockCIFetcher(); + collection = new TestCIReferenceCollection(mockFetcher as unknown as GitHubPRCIFetcher, logService); }); teardown(() => store.clear()); @@ -510,13 +518,19 @@ suite('GitHubPullRequestCIModel', () => { ensureNoDisposablesAreLeakedInTestSuite(); test('initial state is empty', () => { - const model = store.add(new GitHubPullRequestCIModel('owner', 'repo', 'abc', mockFetcher as unknown as GitHubPRCIFetcher, logService)); + const model = acquireModel(); assert.deepStrictEqual(model.checks.get(), []); assert.strictEqual(model.overallStatus.get(), GitHubCIOverallStatus.Neutral); }); + test('acquiring with the same key returns the same model', () => { + const first = acquireModel(); + const second = acquireModel(); + assert.strictEqual(first, second); + }); + test('refresh populates checks and computes overall status', async () => { - const model = store.add(new GitHubPullRequestCIModel('owner', 'repo', 'abc', mockFetcher as unknown as GitHubPRCIFetcher, logService)); + const model = acquireModel(); mockFetcher.nextChecks = [ { id: 1, name: 'build', status: GitHubCheckStatus.Completed, conclusion: GitHubCheckConclusion.Success, startedAt: undefined, completedAt: undefined, detailsUrl: undefined }, { id: 2, name: 'test', status: GitHubCheckStatus.Completed, conclusion: GitHubCheckConclusion.Failure, startedAt: undefined, completedAt: undefined, detailsUrl: undefined }, @@ -528,7 +542,7 @@ suite('GitHubPullRequestCIModel', () => { }); test('refresh shares an in-progress request', async () => { - const model = store.add(new GitHubPullRequestCIModel('owner', 'repo', 'abc', mockFetcher as unknown as GitHubPRCIFetcher, logService)); + const model = acquireModel(); mockFetcher.nextChecks = [ { id: 1, name: 'build', status: GitHubCheckStatus.Completed, conclusion: GitHubCheckConclusion.Success, startedAt: undefined, completedAt: undefined, detailsUrl: undefined }, ]; @@ -560,13 +574,13 @@ suite('GitHubPullRequestCIModel', () => { }); test('getCheckRunAnnotations delegates to fetcher', async () => { - const model = store.add(new GitHubPullRequestCIModel('owner', 'repo', 'abc', mockFetcher as unknown as GitHubPRCIFetcher, logService)); + const model = acquireModel(); const result = await model.getCheckRunAnnotations(1); assert.strictEqual(result, 'mock annotations'); }); test('rerunFailedCheck refreshes after an in-progress refresh completes', async () => { - const model = store.add(new GitHubPullRequestCIModel('owner', 'repo', 'abc', mockFetcher as unknown as GitHubPRCIFetcher, logService)); + const model = acquireModel(); mockFetcher.nextChecks = [ { id: 1, name: 'build', status: GitHubCheckStatus.Completed, conclusion: GitHubCheckConclusion.Failure, startedAt: undefined, completedAt: undefined, detailsUrl: 'https://github.com/owner/repo/actions/runs/12345/job/67890' }, ]; @@ -592,7 +606,7 @@ suite('GitHubPullRequestCIModel', () => { }); test('polling stops when the last client stops polling', () => runWithFakedTimers({ useFakeTimers: true }, async () => { - const model = store.add(new GitHubPullRequestCIModel('owner', 'repo', 'abc', mockFetcher as unknown as GitHubPRCIFetcher, logService)); + const model = acquireModel(); mockFetcher.nextChecks = [ { id: 1, name: 'build', status: GitHubCheckStatus.Completed, conclusion: GitHubCheckConclusion.Success, startedAt: undefined, completedAt: undefined, detailsUrl: undefined }, ]; @@ -647,6 +661,22 @@ suite('parseWorkflowRunId', () => { //#region Test Helpers +class TestCIReferenceCollection extends GitHubPullRequestCIModelReferenceCollection { + constructor( + private readonly _testFetcher: GitHubPRCIFetcher, + logService: ILogService, + ) { + // The base constructor instantiates a fetcher from the apiClient; pass a + // dummy because we override createReferencedObject below to inject the + // test fetcher instead. + super(undefined as never, logService); + } + + protected override createReferencedObject(_key: string, owner: string, repo: string, prNumber: number, headSha: string): GitHubPullRequestCIModel { + return new GitHubPullRequestCIModel(owner, repo, prNumber, headSha, this._testFetcher, new NullLogService()); + } +} + function makeRepository(): IGitHubRepository { return { owner: 'owner', diff --git a/src/vs/sessions/contrib/github/test/browser/githubService.test.ts b/src/vs/sessions/contrib/github/test/browser/githubService.test.ts index 48118319f38d2d..35148dba6ebf57 100644 --- a/src/vs/sessions/contrib/github/test/browser/githubService.test.ts +++ b/src/vs/sessions/contrib/github/test/browser/githubService.test.ts @@ -28,99 +28,129 @@ suite('GitHubService', () => { ensureNoDisposablesAreLeakedInTestSuite(); - test('getRepository returns cached model for same key', () => { - const model1 = service.getRepository('owner', 'repo'); - const model2 = service.getRepository('owner', 'repo'); - assert.strictEqual(model1, model2); + test('createRepositoryModelReference returns shared model for same key', () => { + const ref1 = store.add(service.createRepositoryModelReference('owner', 'repo')); + const ref2 = store.add(service.createRepositoryModelReference('owner', 'repo')); + assert.strictEqual(ref1.object, ref2.object); }); - test('getRepository returns different models for different repos', () => { - const model1 = service.getRepository('owner', 'repo1'); - const model2 = service.getRepository('owner', 'repo2'); - assert.notStrictEqual(model1, model2); + test('createRepositoryModelReference returns different models for different repos', () => { + const ref1 = store.add(service.createRepositoryModelReference('owner', 'repo1')); + const ref2 = store.add(service.createRepositoryModelReference('owner', 'repo2')); + assert.notStrictEqual(ref1.object, ref2.object); }); - test('getPullRequest returns cached model for same key', () => { - const model1 = service.getPullRequest('owner', 'repo', 1); - const model2 = service.getPullRequest('owner', 'repo', 1); - assert.strictEqual(model1, model2); + test('createRepositoryModelReference disposes model when last reference is released', () => { + const ref1 = service.createRepositoryModelReference('owner', 'repo'); + const ref2 = service.createRepositoryModelReference('owner', 'repo'); + const model = ref1.object; + assert.strictEqual(ref2.object, model); + + // Release one reference; model must still be alive and shared. + ref1.dispose(); + const ref3 = store.add(service.createRepositoryModelReference('owner', 'repo')); + assert.strictEqual(ref3.object, model); + + // Release remaining references; the next acquire should create a new model. + ref2.dispose(); + ref3.dispose(); + const ref4 = store.add(service.createRepositoryModelReference('owner', 'repo')); + assert.notStrictEqual(ref4.object, model); }); - test('getPullRequest returns different models for different PRs', () => { - const model1 = service.getPullRequest('owner', 'repo', 1); - const model2 = service.getPullRequest('owner', 'repo', 2); - assert.notStrictEqual(model1, model2); + test('createPullRequestModelReference returns shared model for same key', () => { + const ref1 = store.add(service.createPullRequestModelReference('owner', 'repo', 1)); + const ref2 = store.add(service.createPullRequestModelReference('owner', 'repo', 1)); + assert.strictEqual(ref1.object, ref2.object); }); - test('disposePullRequest removes cached pull request model', () => { - const model1 = service.getPullRequest('owner', 'repo', 1); + test('createPullRequestModelReference returns different models for different PRs', () => { + const ref1 = store.add(service.createPullRequestModelReference('owner', 'repo', 1)); + const ref2 = store.add(service.createPullRequestModelReference('owner', 'repo', 2)); + assert.notStrictEqual(ref1.object, ref2.object); + }); - service.disposePullRequest('owner', 'repo', 1); + test('createPullRequestModelReference disposes model when last reference is released', () => { + const ref1 = service.createPullRequestModelReference('owner', 'repo', 1); + const ref2 = service.createPullRequestModelReference('owner', 'repo', 1); + const model = ref1.object; + assert.strictEqual(ref2.object, model); - const model2 = service.getPullRequest('owner', 'repo', 1); - assert.notStrictEqual(model1, model2); - }); + ref1.dispose(); + const ref3 = store.add(service.createPullRequestModelReference('owner', 'repo', 1)); + assert.strictEqual(ref3.object, model); - test('getPullRequestReviewThreads returns cached model for same key', () => { - const model1 = service.getPullRequestReviewThreads('owner', 'repo', 1); - const model2 = service.getPullRequestReviewThreads('owner', 'repo', 1); - assert.strictEqual(model1, model2); + ref2.dispose(); + ref3.dispose(); + const ref4 = store.add(service.createPullRequestModelReference('owner', 'repo', 1)); + assert.notStrictEqual(ref4.object, model); }); - test('getPullRequestReviewThreads returns different models for different PRs', () => { - const model1 = service.getPullRequestReviewThreads('owner', 'repo', 1); - const model2 = service.getPullRequestReviewThreads('owner', 'repo', 2); - assert.notStrictEqual(model1, model2); + test('createPullRequestReviewThreadsModelReference returns shared model for same key', () => { + const ref1 = store.add(service.createPullRequestReviewThreadsModelReference('owner', 'repo', 1)); + const ref2 = store.add(service.createPullRequestReviewThreadsModelReference('owner', 'repo', 1)); + assert.strictEqual(ref1.object, ref2.object); }); - test('getPullRequestCI returns cached model for same key', () => { - const model1 = service.getPullRequestCI('owner', 'repo', 1, 'abc123'); - const model2 = service.getPullRequestCI('owner', 'repo', 1, 'abc123'); - assert.strictEqual(model1, model2); + test('createPullRequestReviewThreadsModelReference returns different models for different PRs', () => { + const ref1 = store.add(service.createPullRequestReviewThreadsModelReference('owner', 'repo', 1)); + const ref2 = store.add(service.createPullRequestReviewThreadsModelReference('owner', 'repo', 2)); + assert.notStrictEqual(ref1.object, ref2.object); }); - test('getPullRequestCI uses prNumber before the head ref', () => { - const model = service.getPullRequestCI('owner', 'repo', 1, 'abc123'); + test('createPullRequestReviewThreadsModelReference disposes model when last reference is released', () => { + const ref1 = service.createPullRequestReviewThreadsModelReference('owner', 'repo', 1); + const ref2 = service.createPullRequestReviewThreadsModelReference('owner', 'repo', 1); + const model = ref1.object; + assert.strictEqual(ref2.object, model); - assert.strictEqual(model.headSha, 'abc123'); - }); + ref1.dispose(); + const ref3 = store.add(service.createPullRequestReviewThreadsModelReference('owner', 'repo', 1)); + assert.strictEqual(ref3.object, model); - test('getPullRequestCI returns different models for different refs', () => { - const model1 = service.getPullRequestCI('owner', 'repo', 1, 'abc'); - const model2 = service.getPullRequestCI('owner', 'repo', 1, 'def'); - assert.notStrictEqual(model1, model2); + ref2.dispose(); + ref3.dispose(); + const ref4 = store.add(service.createPullRequestReviewThreadsModelReference('owner', 'repo', 1)); + assert.notStrictEqual(ref4.object, model); }); - test('getPullRequestCI returns different models for different pull requests', () => { - const model1 = service.getPullRequestCI('owner', 'repo', 1, 'abc'); - const model2 = service.getPullRequestCI('owner', 'repo', 2, 'abc'); - assert.notStrictEqual(model1, model2); + test('createPullRequestCIModelReference returns shared model for same key', () => { + const ref1 = store.add(service.createPullRequestCIModelReference('owner', 'repo', 1, 'abc')); + const ref2 = store.add(service.createPullRequestCIModelReference('owner', 'repo', 1, 'abc')); + assert.strictEqual(ref1.object, ref2.object); }); - test('getPullRequestCI only retains the current head ref model', () => { - const model1 = service.getPullRequestCI('owner', 'repo', 1, 'abc'); - service.getPullRequestCI('owner', 'repo', 1, 'def'); - - const model2 = service.getPullRequestCI('owner', 'repo', 1, 'abc'); - - assert.notStrictEqual(model1, model2); + test('createPullRequestCIModelReference returns different models for different head refs', () => { + const ref1 = store.add(service.createPullRequestCIModelReference('owner', 'repo', 1, 'abc')); + const ref2 = store.add(service.createPullRequestCIModelReference('owner', 'repo', 1, 'def')); + assert.notStrictEqual(ref1.object, ref2.object); }); - test('getPullRequestCI retains current head ref models per pull request', () => { - const pr1Model = service.getPullRequestCI('owner', 'repo', 1, 'abc'); - service.getPullRequestCI('owner', 'repo', 2, 'def'); + test('createPullRequestCIModelReference returns different models for different pull requests', () => { + const ref1 = store.add(service.createPullRequestCIModelReference('owner', 'repo', 1, 'abc')); + const ref2 = store.add(service.createPullRequestCIModelReference('owner', 'repo', 2, 'abc')); + assert.notStrictEqual(ref1.object, ref2.object); + }); - assert.strictEqual(service.getPullRequestCI('owner', 'repo', 1, 'abc'), pr1Model); + test('createPullRequestCIModelReference exposes the requested head ref on the model', () => { + const ref = store.add(service.createPullRequestCIModelReference('owner', 'repo', 1, 'abc123')); + assert.strictEqual(ref.object.headSha, 'abc123'); }); - test('disposing service does not throw', () => { - service.getRepository('owner', 'repo'); - service.getPullRequest('owner', 'repo', 1); - service.getPullRequestReviewThreads('owner', 'repo', 1); - service.getPullRequestCI('owner', 'repo', 1, 'abc'); + test('createPullRequestCIModelReference disposes model when last reference is released', () => { + const ref1 = service.createPullRequestCIModelReference('owner', 'repo', 1, 'abc'); + const ref2 = service.createPullRequestCIModelReference('owner', 'repo', 1, 'abc'); + const model = ref1.object; + assert.strictEqual(ref2.object, model); + + ref1.dispose(); + const ref3 = store.add(service.createPullRequestCIModelReference('owner', 'repo', 1, 'abc')); + assert.strictEqual(ref3.object, model); - // Disposing the service should not throw and should clean up models - assert.doesNotThrow(() => service.dispose()); + ref2.dispose(); + ref3.dispose(); + const ref4 = store.add(service.createPullRequestCIModelReference('owner', 'repo', 1, 'abc')); + assert.notStrictEqual(ref4.object, model); }); });