diff --git a/src/test/view/reviewManager.test.ts b/src/test/view/reviewManager.test.ts new file mode 100644 index 0000000000..9d47032089 --- /dev/null +++ b/src/test/view/reviewManager.test.ts @@ -0,0 +1,214 @@ +/*--------------------------------------------------------------------------------------------- + * 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 { SinonFakeTimers, SinonSandbox, createSandbox } from 'sinon'; +import * as vscode from 'vscode'; +import { GitApiImpl } from '../../api/api1'; +import { ITelemetry } from '../../common/telemetry'; +import { CredentialStore } from '../../github/credentials'; +import { FolderRepositoryManager } from '../../github/folderRepositoryManager'; +import { PullRequestModel } from '../../github/pullRequestModel'; +import { RepositoriesManager } from '../../github/repositoriesManager'; +import { CreatePullRequestHelper } from '../../view/createPullRequestHelper'; +import { PullRequestChangesTreeDataProvider } from '../../view/prChangesTreeDataProvider'; +import { PullRequestsTreeDataProvider } from '../../view/prsTreeDataProvider'; +import { ReviewManager, ShowPullRequest } from '../../view/reviewManager'; +import { WebviewViewCoordinator } from '../../view/webviewViewCoordinator'; +import { MockPrsTreeModel } from '../mocks/mockPRsTreeModel'; +import { MockCommandRegistry } from '../mocks/mockCommandRegistry'; +import { MockExtensionContext } from '../mocks/mockExtensionContext'; +import { MockRepository } from '../mocks/mockRepository'; +import { MockTelemetry } from '../mocks/mockTelemetry'; +import { MockThemeWatcher } from '../mocks/mockThemeWatcher'; +import { PrsTreeModel } from '../../view/prsTreeModel'; + +describe('ReviewManager polling', function () { + const POLL_MIN_INTERVAL_MS = 1000 * 60 * 5; + const POLL_MAX_INTERVAL_MS = 1000 * 60 * 30; + const POLL_BACKOFF_MULTIPLIER = 2; + + let sinon: SinonSandbox; + let clock: SinonFakeTimers; + let repository: MockRepository; + let telemetry: ITelemetry; + let context: MockExtensionContext; + let reposManager: RepositoriesManager; + let gitApi: GitApiImpl; + let manager: FolderRepositoryManager; + let reviewManager: ReviewManager; + let onDidChangeWindowStateCallback: ((state: vscode.WindowState) => unknown) | undefined; + let isWindowFocused: boolean; + let setTimeoutSpy: sinon.SinonSpy; + + beforeEach(function () { + sinon = createSandbox(); + clock = sinon.useFakeTimers(); + MockCommandRegistry.install(sinon); + setTimeoutSpy = sinon.spy(global, 'setTimeout'); + + isWindowFocused = true; + sinon.stub(vscode.window, 'state').get(() => ({ focused: isWindowFocused } as vscode.WindowState)); + sinon.stub(vscode.window, 'onDidChangeWindowState').callsFake(listener => { + onDidChangeWindowStateCallback = listener; + return new vscode.Disposable(() => { }); + }); + + telemetry = new MockTelemetry(); + context = new MockExtensionContext(); + repository = new MockRepository(); + repository.addRemote('origin', 'git@github.com:aaa/bbb'); + + const credentialStore = new CredentialStore(telemetry, context); + reposManager = new RepositoriesManager(credentialStore, telemetry); + gitApi = new GitApiImpl(reposManager); + + const mockPrsTreeModel = new MockPrsTreeModel() as unknown as PrsTreeModel; + const prsTreeProvider = new PullRequestsTreeDataProvider(mockPrsTreeModel, telemetry, context, reposManager); + const activePrViewCoordinator = new WebviewViewCoordinator(context); + const createPrHelper = new CreatePullRequestHelper(); + const themeWatcher = new MockThemeWatcher(); + + manager = new FolderRepositoryManager(0, context, repository, telemetry, gitApi, credentialStore, createPrHelper, themeWatcher); + reposManager.insertFolderManager(manager); + + const changesTreeProvider = new PullRequestChangesTreeDataProvider(gitApi, reposManager); + reviewManager = new ReviewManager( + 0, + context, + repository, + manager, + telemetry, + changesTreeProvider, + prsTreeProvider, + new ShowPullRequest(), + activePrViewCoordinator, + createPrHelper, + gitApi, + ); + }); + + afterEach(function () { + reviewManager.dispose(); + sinon.restore(); + clock.restore(); + }); + + async function flushMicrotasks() { + await Promise.resolve(); + await Promise.resolve(); + } + + function latestScheduledDelay(): number { + const delays = setTimeoutSpy.getCalls() + .map(call => call.args[1]) + .filter((delay): delay is number => typeof delay === 'number'); + assert.ok(delays.length > 0, 'expected at least one scheduled timer'); + return delays[delays.length - 1]; + } + + it('backs off polling interval when no change is detected', async function () { + sinon.stub(reviewManager, 'updateState').resolves(); + + assert.strictEqual(latestScheduledDelay(), POLL_MIN_INTERVAL_MS); + clock.tick(POLL_MIN_INTERVAL_MS); + await flushMicrotasks(); + + assert.strictEqual(latestScheduledDelay(), POLL_MIN_INTERVAL_MS * POLL_BACKOFF_MULTIPLIER); + }); + + 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 + // those to emulate an observable change. + const internal = reviewManager as unknown as { _prNumber?: number }; + let pollCount = 0; + sinon.stub(reviewManager, 'updateState').callsFake(async () => { + pollCount++; + if (pollCount === 2) { + internal._prNumber = (internal._prNumber ?? 0) + 1; + } + }); + + assert.strictEqual(latestScheduledDelay(), POLL_MIN_INTERVAL_MS); + clock.tick(POLL_MIN_INTERVAL_MS); + await flushMicrotasks(); + assert.strictEqual(latestScheduledDelay(), POLL_MIN_INTERVAL_MS * POLL_BACKOFF_MULTIPLIER); + + clock.tick(POLL_MIN_INTERVAL_MS * POLL_BACKOFF_MULTIPLIER); + await flushMicrotasks(); + assert.strictEqual(latestScheduledDelay(), POLL_MIN_INTERVAL_MS); + }); + + it('applies normal backoff on focus-triggered refresh when no change is detected', async function () { + const MAX_FOCUS_JITTER_MS = 60_000; + sinon.stub(reviewManager, 'updateState').resolves(); + + clock.tick(POLL_MIN_INTERVAL_MS); + await flushMicrotasks(); + assert.strictEqual(latestScheduledDelay(), Math.min(POLL_MAX_INTERVAL_MS, POLL_MIN_INTERVAL_MS * POLL_BACKOFF_MULTIPLIER)); + + isWindowFocused = false; + const callsBeforeSkip = setTimeoutSpy.callCount; + clock.tick(POLL_MIN_INTERVAL_MS * POLL_BACKOFF_MULTIPLIER); + await flushMicrotasks(); + assert.strictEqual(setTimeoutSpy.callCount, callsBeforeSkip, 'no timer should be scheduled while unfocused after skip'); + + isWindowFocused = true; + onDidChangeWindowStateCallback!({ focused: true } as vscode.WindowState); + await flushMicrotasks(); + + // Focus schedules a jittered poll; advance past the max jitter to run it. + clock.tick(MAX_FOCUS_JITTER_MS); + await flushMicrotasks(); + + assert.strictEqual(latestScheduledDelay(), POLL_MIN_INTERVAL_MS * POLL_BACKOFF_MULTIPLIER * POLL_BACKOFF_MULTIPLIER); + }); + + it('window focus triggers a one-off poll when there is no scheduled poll', async function () { + assert.ok(onDidChangeWindowStateCallback, 'window state listener should be registered'); + + sinon.stub(reviewManager, 'updateState').resolves(); + + isWindowFocused = false; + const callsBeforeSkip = setTimeoutSpy.callCount; + clock.tick(POLL_MIN_INTERVAL_MS); + await flushMicrotasks(); + assert.strictEqual(setTimeoutSpy.callCount, callsBeforeSkip, 'timer should not be rescheduled while unfocused'); + + onDidChangeWindowStateCallback!({ focused: true } as vscode.WindowState); + await flushMicrotasks(); + + assert.ok(setTimeoutSpy.callCount > callsBeforeSkip, 'focus should cause one-off refresh and schedule next poll'); + }); + + it('polling refreshes state when active PR exists and new PR activity is detected', async function () { + // Stub the getter rather than assigning through the setter, which has side effects. + sinon.stub(manager, 'activePullRequest').get(() => ({ number: 123 } as unknown as PullRequestModel)); + const updateStateStub = sinon.stub(reviewManager, 'updateState').resolves(); + sinon.stub(reviewManager as unknown as { hasNewPullRequests: () => Promise }, 'hasNewPullRequests').resolves(true); + + clock.tick(POLL_MIN_INTERVAL_MS); + await flushMicrotasks(); + + assert.strictEqual(updateStateStub.called, true, 'poll should refresh state when active PR may be stale'); + }); + + it('caps backoff at the maximum interval', async function () { + sinon.stub(reviewManager, 'updateState').resolves(); + + clock.tick(POLL_MIN_INTERVAL_MS); + await flushMicrotasks(); + assert.strictEqual(latestScheduledDelay(), POLL_MIN_INTERVAL_MS * 2); + + clock.tick(POLL_MIN_INTERVAL_MS * 2); + await flushMicrotasks(); + assert.strictEqual(latestScheduledDelay(), POLL_MIN_INTERVAL_MS * 4); + + clock.tick(POLL_MIN_INTERVAL_MS * 4); + await flushMicrotasks(); + assert.strictEqual(latestScheduledDelay(), POLL_MIN_INTERVAL_MS * 6); + }); +}); diff --git a/src/view/reviewManager.ts b/src/view/reviewManager.ts index c3ff30c921..43cecd3d34 100644 --- a/src/view/reviewManager.ts +++ b/src/view/reviewManager.ts @@ -50,6 +50,9 @@ import { WebviewViewCoordinator } from './webviewViewCoordinator'; export class ReviewManager extends Disposable { public static ID = 'Review'; + private static readonly MIN_POLL_INTERVAL_MS = 1000 * 60 * 5; + private static readonly MAX_POLL_INTERVAL_MS = 1000 * 60 * 30; + private static readonly POLL_BACKOFF_MULTIPLIER = 2; private readonly _localToDispose: vscode.Disposable[] = []; private readonly _reviewModel: ReviewModel = new ReviewModel(); @@ -86,6 +89,7 @@ export class ReviewManager extends Disposable { */ private readonly _staleMetadataCheckedBranches = new Set(); private _pollHandle: NodeJS.Timeout | undefined; + private _pollIntervalMs = ReviewManager.MIN_POLL_INTERVAL_MS; /** * Flag set when the "Checkout" action is used and cleared on the next git * state update, once review mode has been entered. Used to disambiguate @@ -147,9 +151,11 @@ export class ReviewManager extends Disposable { this.pollForStateChange(); this._register(vscode.window.onDidChangeWindowState(state => { if (state.focused && !this._pollHandle) { - // Polling was skipped because the window was not focused. - // Schedule a poll with a randomized delay (jitter) so that in - // multi-repo setups all ReviewManagers don't poll at the same time. + // Polling was skipped because the window was not focused. Schedule a + // poll with a randomized delay (jitter) so that in multi-repo setups + // all ReviewManagers don't poll at the same time. Reuse _pollHandle so + // subsequent focus events don't trigger overlapping polls; doPoll + // applies the normal backoff rules. const jitter = 15_000 + Math.floor(Math.random() * 45_000); // 15-60s this._pollHandle = setTimeout(() => this.doPoll(), jitter); } @@ -241,7 +247,7 @@ export class ReviewManager extends Disposable { if (this._pollHandle) { clearTimeout(this._pollHandle); } - this._pollHandle = setTimeout(() => this.doPoll(), 1000 * 60 * 5); + this._pollHandle = setTimeout(() => this.doPoll(), this._pollIntervalMs); } private async doPoll() { @@ -256,9 +262,33 @@ export class ReviewManager extends Disposable { return; } Logger.appendLine('Polling for state change...', this.id); + let hasDetectedChange = false; try { - if (!this._validateStatusInProgress && !this._folderRepoManager.activePullRequest) { - await this.updateState(); + if (!this._validateStatusInProgress) { + const shouldRefreshForActivePr = !!this._folderRepoManager.activePullRequest && await this.hasNewPullRequests(); + if (shouldRefreshForActivePr) { + Logger.appendLine('Polling detected potential new PR activity while a PR is active. Refreshing state.', this.id); + } + + if (!this._folderRepoManager.activePullRequest || shouldRefreshForActivePr) { + const previousPrNumber = this._prNumber; + const previousLastCommitSha = this._lastCommitSha; + await this.updateState(); + hasDetectedChange = + previousPrNumber !== this._prNumber || + previousLastCommitSha !== this._lastCommitSha; + } + } + + if (hasDetectedChange) { + this._pollIntervalMs = ReviewManager.MIN_POLL_INTERVAL_MS; + Logger.appendLine(`Polling detected change. Next poll in ${Math.round(this._pollIntervalMs / 1000)}s.`, this.id); + } else { + this._pollIntervalMs = Math.min( + ReviewManager.MAX_POLL_INTERVAL_MS, + this._pollIntervalMs * ReviewManager.POLL_BACKOFF_MULTIPLIER, + ); + Logger.appendLine(`Polling found no change. Backing off to ${Math.round(this._pollIntervalMs / 1000)}s.`, this.id); } } catch (e) { Logger.warn(`Polling for state change failed: ${formatError(e)}`, this.id); @@ -671,6 +701,9 @@ export class ReviewManager extends Disposable { const hasPushedChanges = branch.commit !== oldLastCommitSha && branch.ahead === 0 && branch.behind === 0; if (!this.justSwitchedToReviewMode && (previousPrNumber === pr.number) && !hasPushedChanges && (this._isShowingLastReviewChanges === pr.showChangesSinceReview)) { + // No meaningful state change; keep the previous commit SHA so polling + // doesn't treat this as a change on the next cycle. + this._lastCommitSha = oldLastCommitSha; return; } this._isShowingLastReviewChanges = pr.showChangesSinceReview;