Skip to content

Commit 66a2314

Browse files
CopilotCopilot
authored
Fix stale worktree repo entry persisting in PR sidebar after worktree removal (#8560)
* Initial plan * Initial plan for fixing stale worktree repo entry Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com> * Fix splice bug in removeRepo and removeReviewManager, add tests The splice() calls were missing the deleteCount argument, causing all elements from the target index onwards to be removed instead of just the single target element. This caused: - Removing a worktree repo could also remove the main repo's manager - PRs would stop appearing under the main repo entry - Stale entries could persist in the sidebar Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com> * Add proactive stale worktree cleanup on refresh When the Refresh button is clicked in the PR sidebar, check if any tracked folder managers reference root URIs that no longer exist on disk (e.g., removed worktree directories). Remove those stale entries before refreshing the tree. Also: - PrsTreeModel now fires data change on repo removal so tree refreshes - ReviewsManager cleans up orphaned review managers when folder repos change Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com> * Fix removeMissingRepos tests: use real temp dirs instead of stubbing vscode.workspace.fs.stat The `stat` property on `vscode.workspace.fs` is non-configurable, so sinon cannot stub it. Replace with real filesystem operations: create temporary directories, then delete one to simulate a removed worktree. Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com> * Update git API * Replace removeMissingRepos with reactive worktree change detection via state.onDidChange Instead of checking filesystem on refresh, listen to each repository's state.onDidChange event and compare worktree paths. When a worktree is removed from the git state, automatically remove its folder manager. - Add checkWorktreeChanges() to RepositoriesManager that tracks worktree paths per repo and detects removals - Subscribe to state.onDidChange in registerFolderListeners - Remove removeMissingRepos() method and its call from pr.refreshList - Update MockRepository to support worktrees and fire state changes - Replace removeMissingRepos tests with worktree detection tests Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com> * Filter out stale WorkspaceFolderNodes in refreshAllQueryResults When a worktree is removed, the corresponding folder manager is removed from RepositoriesManager, but the WorkspaceFolderNode for it was still kept in _children. Now refreshAllQueryResults checks if each folder node's manager is still present, disposes stale nodes, and fires a root-level tree data change to rebuild the tree. Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * CCR suggestion
1 parent ecf3298 commit 66a2314

File tree

7 files changed

+317
-5
lines changed

7 files changed

+317
-5
lines changed

‎src/api/api.d.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,14 @@ export interface Remote {
5353
readonly isReadOnly: boolean;
5454
}
5555

56+
export interface Worktree {
57+
readonly name: string;
58+
readonly path: string;
59+
readonly ref: string;
60+
readonly main: boolean;
61+
readonly detached: boolean;
62+
}
63+
5664
export { Status } from './api1';
5765

5866
export interface Change {
@@ -71,6 +79,7 @@ export interface RepositoryState {
7179
readonly HEAD: Branch | undefined;
7280
readonly remotes: Remote[];
7381
readonly submodules: Submodule[];
82+
readonly worktrees?: Worktree[];
7483
readonly rebaseCommit: Commit | undefined;
7584

7685
readonly mergeChanges: Change[];

‎src/github/repositoriesManager.ts‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,39 @@ export class RepositoriesManager extends Disposable {
9191
folderManager.onDidChangeAnyPullRequests(e => this._onDidChangeAnyPullRequests.fire(e)),
9292
folderManager.onDidAddPullRequest(e => this._onDidAddPullRequest.fire(e)),
9393
folderManager.onDidChangeGithubRepositories(() => this._onDidAddAnyGitHubRepository.fire(folderManager)),
94+
folderManager.repository.state.onDidChange(() => this.checkWorktreeChanges(folderManager.repository)),
9495
];
9596
this._subs.set(folderManager, disposables);
9697
}
9798

99+
private _previousWorktrees: Map<string, Set<string>> = new Map();
100+
101+
private checkWorktreeChanges(repo: Repository): void {
102+
const worktrees = repo.state.worktrees;
103+
if (!worktrees) {
104+
return;
105+
}
106+
107+
const repoKey = repo.rootUri.toString();
108+
const currentPaths = new Set(worktrees.map(wt => vscode.Uri.file(wt.path).toString()));
109+
const previousPaths = this._previousWorktrees.get(repoKey);
110+
this._previousWorktrees.set(repoKey, currentPaths);
111+
112+
if (!previousPaths) {
113+
return;
114+
}
115+
116+
for (const previousPath of previousPaths) {
117+
if (!currentPaths.has(previousPath)) {
118+
const folderManager = this._folderManagers.find(m => m.repository.rootUri.toString() === previousPath);
119+
if (folderManager) {
120+
Logger.appendLine(`Removing folder manager for removed worktree ${previousPath}`, RepositoriesManager.ID);
121+
this.removeRepo(folderManager.repository);
122+
}
123+
}
124+
}
125+
}
126+
98127
insertFolderManager(folderManager: FolderRepositoryManager) {
99128
this.registerFolderListeners(folderManager);
100129

@@ -127,7 +156,7 @@ export class RepositoriesManager extends Disposable {
127156
const folderManager = this._folderManagers[existingFolderManagerIndex];
128157
disposeAll(this._subs.get(folderManager)!);
129158
this._subs.delete(folderManager);
130-
this._folderManagers.splice(existingFolderManagerIndex);
159+
this._folderManagers.splice(existingFolderManagerIndex, 1);
131160
folderManager.dispose();
132161
this.updateActiveReviewCount();
133162
this._onDidChangeFolderRepositories.fire({});

‎src/test/github/repositoriesManager.test.ts‎

Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import * as vscode from 'vscode';
7+
import { SinonSandbox, createSandbox } from 'sinon';
8+
import { default as assert } from 'assert';
9+
10+
import { RepositoriesManager } from '../../github/repositoriesManager';
11+
import { FolderRepositoryManager } from '../../github/folderRepositoryManager';
12+
import { GitApiImpl } from '../../api/api1';
13+
import { CredentialStore } from '../../github/credentials';
14+
15+
import { MockTelemetry } from '../mocks/mockTelemetry';
16+
import { MockExtensionContext } from '../mocks/mockExtensionContext';
17+
import { MockRepository } from '../mocks/mockRepository';
18+
import { MockCommandRegistry } from '../mocks/mockCommandRegistry';
19+
import { MockThemeWatcher } from '../mocks/mockThemeWatcher';
20+
import { CreatePullRequestHelper } from '../../view/createPullRequestHelper';
21+
22+
describe('RepositoriesManager', function () {
23+
let sinon: SinonSandbox;
24+
let context: MockExtensionContext;
25+
let telemetry: MockTelemetry;
26+
let credentialStore: CredentialStore;
27+
let reposManager: RepositoriesManager;
28+
let createPrHelper: CreatePullRequestHelper;
29+
let mockThemeWatcher: MockThemeWatcher;
30+
31+
beforeEach(function () {
32+
sinon = createSandbox();
33+
MockCommandRegistry.install(sinon);
34+
mockThemeWatcher = new MockThemeWatcher();
35+
context = new MockExtensionContext();
36+
telemetry = new MockTelemetry();
37+
credentialStore = new CredentialStore(telemetry, context);
38+
reposManager = new RepositoriesManager(credentialStore, telemetry);
39+
createPrHelper = new CreatePullRequestHelper();
40+
});
41+
42+
afterEach(function () {
43+
context.dispose();
44+
sinon.restore();
45+
});
46+
47+
describe('removeRepo', function () {
48+
it('removes only the specified repository when it is not at the last position', function () {
49+
const repo1 = new MockRepository();
50+
repo1.rootUri = vscode.Uri.file('/repo1');
51+
repo1.addRemote('origin', 'git@github.com:aaa/bbb');
52+
53+
const repo2 = new MockRepository();
54+
repo2.rootUri = vscode.Uri.file('/repo2');
55+
repo2.addRemote('origin', 'git@github.com:ccc/ddd');
56+
57+
const repo3 = new MockRepository();
58+
repo3.rootUri = vscode.Uri.file('/repo3');
59+
repo3.addRemote('origin', 'git@github.com:eee/fff');
60+
61+
reposManager.insertFolderManager(new FolderRepositoryManager(0, context, repo1, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
62+
reposManager.insertFolderManager(new FolderRepositoryManager(1, context, repo2, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
63+
reposManager.insertFolderManager(new FolderRepositoryManager(2, context, repo3, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
64+
65+
assert.strictEqual(reposManager.folderManagers.length, 3);
66+
67+
// Remove the repo at the first position
68+
reposManager.removeRepo(repo1);
69+
70+
// Only repo1 should be removed; repo2 and repo3 should remain
71+
assert.strictEqual(reposManager.folderManagers.length, 2);
72+
assert.strictEqual(reposManager.folderManagers[0].repository.rootUri.toString(), repo2.rootUri.toString());
73+
assert.strictEqual(reposManager.folderManagers[1].repository.rootUri.toString(), repo3.rootUri.toString());
74+
});
75+
76+
it('removes only the specified repository when it is at the last position', function () {
77+
const repo1 = new MockRepository();
78+
repo1.rootUri = vscode.Uri.file('/repo1');
79+
repo1.addRemote('origin', 'git@github.com:aaa/bbb');
80+
81+
const repo2 = new MockRepository();
82+
repo2.rootUri = vscode.Uri.file('/repo2');
83+
repo2.addRemote('origin', 'git@github.com:ccc/ddd');
84+
85+
reposManager.insertFolderManager(new FolderRepositoryManager(0, context, repo1, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
86+
reposManager.insertFolderManager(new FolderRepositoryManager(1, context, repo2, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
87+
88+
assert.strictEqual(reposManager.folderManagers.length, 2);
89+
90+
// Remove the repo at the last position
91+
reposManager.removeRepo(repo2);
92+
93+
assert.strictEqual(reposManager.folderManagers.length, 1);
94+
assert.strictEqual(reposManager.folderManagers[0].repository.rootUri.toString(), repo1.rootUri.toString());
95+
});
96+
97+
it('removes only the middle repository leaving others intact', function () {
98+
const repo1 = new MockRepository();
99+
repo1.rootUri = vscode.Uri.file('/repo1');
100+
repo1.addRemote('origin', 'git@github.com:aaa/bbb');
101+
102+
const repo2 = new MockRepository();
103+
repo2.rootUri = vscode.Uri.file('/repo2');
104+
repo2.addRemote('origin', 'git@github.com:ccc/ddd');
105+
106+
const repo3 = new MockRepository();
107+
repo3.rootUri = vscode.Uri.file('/repo3');
108+
repo3.addRemote('origin', 'git@github.com:eee/fff');
109+
110+
reposManager.insertFolderManager(new FolderRepositoryManager(0, context, repo1, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
111+
reposManager.insertFolderManager(new FolderRepositoryManager(1, context, repo2, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
112+
reposManager.insertFolderManager(new FolderRepositoryManager(2, context, repo3, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
113+
114+
assert.strictEqual(reposManager.folderManagers.length, 3);
115+
116+
// Remove the middle repo
117+
reposManager.removeRepo(repo2);
118+
119+
assert.strictEqual(reposManager.folderManagers.length, 2);
120+
assert.strictEqual(reposManager.folderManagers[0].repository.rootUri.toString(), repo1.rootUri.toString());
121+
assert.strictEqual(reposManager.folderManagers[1].repository.rootUri.toString(), repo3.rootUri.toString());
122+
});
123+
124+
it('does nothing when removing a repo that is not tracked', function () {
125+
const repo1 = new MockRepository();
126+
repo1.rootUri = vscode.Uri.file('/repo1');
127+
repo1.addRemote('origin', 'git@github.com:aaa/bbb');
128+
129+
const unknownRepo = new MockRepository();
130+
unknownRepo.rootUri = vscode.Uri.file('/unknown');
131+
132+
reposManager.insertFolderManager(new FolderRepositoryManager(0, context, repo1, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
133+
134+
assert.strictEqual(reposManager.folderManagers.length, 1);
135+
136+
reposManager.removeRepo(unknownRepo);
137+
138+
assert.strictEqual(reposManager.folderManagers.length, 1);
139+
assert.strictEqual(reposManager.folderManagers[0].repository.rootUri.toString(), repo1.rootUri.toString());
140+
});
141+
});
142+
143+
describe('worktree change detection', function () {
144+
it('removes folder manager when its worktree is removed from the main repo', function () {
145+
const mainRepo = new MockRepository();
146+
mainRepo.rootUri = vscode.Uri.file('/main-repo');
147+
mainRepo.addRemote('origin', 'git@github.com:aaa/bbb');
148+
149+
const worktreeRepo = new MockRepository();
150+
worktreeRepo.rootUri = vscode.Uri.file('/main-repo/worktrees/feature');
151+
worktreeRepo.addRemote('origin', 'git@github.com:aaa/bbb');
152+
153+
reposManager.insertFolderManager(new FolderRepositoryManager(0, context, mainRepo, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
154+
reposManager.insertFolderManager(new FolderRepositoryManager(1, context, worktreeRepo, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
155+
156+
assert.strictEqual(reposManager.folderManagers.length, 2);
157+
158+
// Set initial worktrees on the main repo (includes the worktree)
159+
mainRepo.setWorktrees([
160+
{ name: 'main-repo', path: '/main-repo', ref: 'main', main: true, detached: false },
161+
{ name: 'feature', path: '/main-repo/worktrees/feature', ref: 'feature', main: false, detached: false },
162+
]);
163+
164+
assert.strictEqual(reposManager.folderManagers.length, 2);
165+
166+
// Worktree is removed - main repo state changes with updated worktrees
167+
mainRepo.setWorktrees([
168+
{ name: 'main-repo', path: '/main-repo', ref: 'main', main: true, detached: false },
169+
]);
170+
171+
assert.strictEqual(reposManager.folderManagers.length, 1);
172+
assert.strictEqual(reposManager.folderManagers[0].repository.rootUri.toString(), mainRepo.rootUri.toString());
173+
});
174+
175+
it('does not remove folder managers when worktrees remain unchanged', function () {
176+
const mainRepo = new MockRepository();
177+
mainRepo.rootUri = vscode.Uri.file('/main-repo');
178+
mainRepo.addRemote('origin', 'git@github.com:aaa/bbb');
179+
180+
const worktreeRepo = new MockRepository();
181+
worktreeRepo.rootUri = vscode.Uri.file('/main-repo/worktrees/feature');
182+
worktreeRepo.addRemote('origin', 'git@github.com:aaa/bbb');
183+
184+
reposManager.insertFolderManager(new FolderRepositoryManager(0, context, mainRepo, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
185+
reposManager.insertFolderManager(new FolderRepositoryManager(1, context, worktreeRepo, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
186+
187+
// Set initial worktrees
188+
mainRepo.setWorktrees([
189+
{ name: 'main-repo', path: '/main-repo', ref: 'main', main: true, detached: false },
190+
{ name: 'feature', path: '/main-repo/worktrees/feature', ref: 'feature', main: false, detached: false },
191+
]);
192+
193+
// Fire state change again with same worktrees
194+
mainRepo.setWorktrees([
195+
{ name: 'main-repo', path: '/main-repo', ref: 'main', main: true, detached: false },
196+
{ name: 'feature', path: '/main-repo/worktrees/feature', ref: 'feature', main: false, detached: false },
197+
]);
198+
199+
assert.strictEqual(reposManager.folderManagers.length, 2);
200+
});
201+
202+
it('does nothing when worktrees property is not available', function () {
203+
const repo = new MockRepository();
204+
repo.rootUri = vscode.Uri.file('/repo');
205+
repo.addRemote('origin', 'git@github.com:aaa/bbb');
206+
207+
reposManager.insertFolderManager(new FolderRepositoryManager(0, context, repo, telemetry, new GitApiImpl(reposManager), credentialStore, createPrHelper, mockThemeWatcher));
208+
209+
assert.strictEqual(reposManager.folderManagers.length, 1);
210+
211+
// Fire state change without setting worktrees (stays undefined)
212+
(repo as any)._onDidChangeState.fire();
213+
214+
assert.strictEqual(reposManager.folderManagers.length, 1);
215+
});
216+
});
217+
});

‎src/test/mocks/mockRepository.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import { Uri } from 'vscode';
6+
import { EventEmitter, Uri } from 'vscode';
77
import { RefType } from '../../api/api1';
88

99
import type {
@@ -19,6 +19,7 @@ import type {
1919
BranchQuery,
2020
FetchOptions,
2121
RefQuery,
22+
Worktree,
2223
} from '../../api/api';
2324

2425
type Mutable<T> = {
@@ -67,18 +68,21 @@ export class MockRepository implements Repository {
6768
return Promise.reject(new Error(`Unexpected log(${options})`));
6869
}
6970

71+
private _onDidChangeState = new EventEmitter<void>();
72+
7073
private _state: Mutable<RepositoryState & { refs: Ref[] }> = {
7174
HEAD: {
7275
type: RefType.Head
7376
},
7477
refs: [],
7578
remotes: [],
7679
submodules: [],
80+
worktrees: undefined,
7781
rebaseCommit: undefined,
7882
mergeChanges: [],
7983
indexChanges: [],
8084
workingTreeChanges: [],
81-
onDidChange: () => ({ dispose() { } }),
85+
onDidChange: this._onDidChangeState.event,
8286
};
8387
private _config: Map<string, string> = new Map();
8488
private _branches: Branch[] = [];
@@ -337,4 +341,9 @@ export class MockRepository implements Repository {
337341
mergeAbort(): Promise<void> {
338342
return Promise.reject(new Error(`Unexpected mergeAbort`));
339343
}
344+
345+
setWorktrees(worktrees: Worktree[]) {
346+
this._state.worktrees = worktrees;
347+
this._onDidChangeState.fire();
348+
}
340349
}

‎src/view/prsTreeDataProvider.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -380,7 +380,21 @@ export class PullRequestsTreeDataProvider extends Disposable implements vscode.T
380380
}
381381

382382
if (this._children[0] instanceof WorkspaceFolderNode) {
383-
(this._children as WorkspaceFolderNode[]).forEach(folderNode => this.refreshQueryResultsForFolder(folderNode));
383+
const currentManagers = new Set(this._reposManager.folderManagers);
384+
const before = this._children.length;
385+
const validChildren = (this._children as WorkspaceFolderNode[]).filter(folderNode => {
386+
if (currentManagers.has(folderNode.folderManager)) {
387+
return true;
388+
}
389+
folderNode.dispose();
390+
return false;
391+
});
392+
if (validChildren.length !== before) {
393+
this._children = validChildren;
394+
this._onDidChangeTreeData.fire();
395+
return;
396+
}
397+
validChildren.forEach(folderNode => this.refreshQueryResultsForFolder(folderNode));
384398
return;
385399
}
386400
this.refreshQueryResultsForFolder();

‎src/view/prsTreeModel.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,23 @@ export class PrsTreeModel extends Disposable {
132132
if (changed.added) {
133133
repoEvents(changed.added);
134134
this._onDidChangeData.fire(changed.added);
135+
} else {
136+
const activeManagers = new Set(this._reposManager.folderManagers);
137+
138+
for (const [manager, disposables] of this._repoEvents) {
139+
if (!activeManagers.has(manager)) {
140+
disposeAll(disposables);
141+
this._repoEvents.delete(manager);
142+
}
143+
}
144+
145+
for (const [manager, disposables] of this._activePRDisposables) {
146+
if (!activeManagers.has(manager)) {
147+
disposeAll(disposables);
148+
this._activePRDisposables.delete(manager);
149+
}
150+
}
151+
this._onDidChangeData.fire();
135152
}
136153
}));
137154

0 commit comments

Comments
 (0)