Skip to content

Commit f1d810d

Browse files
Copilotalexr00
andauthored
Fix race condition in issues tree view where getIssues errors prevent tree refresh
The issues tree view could get permanently stuck empty because: 1. `setIssues()` used the `new Promise(async executor)` anti-pattern and only fired `_onDidChangeIssueData` on success. If `getIssues()` threw (e.g. due to a transient network error), the promise rejected silently, the event never fired, and the tree never learned to refresh. 2. `setIssueData()` didn't fire `_onDidChangeIssueData` after processing all queries, so if all queries failed, the tree stayed permanently empty until a git commit triggered a new `setIssueData` call. Fix: - Refactor `setIssues()` to use async/await properly, catch errors gracefully (resolving with undefined instead of rejecting), and fire the change event in a `finally` block so it always fires. - Fire `_onDidChangeIssueData` at the end of `setIssueData()` to guarantee the tree always gets a refresh signal after data loading completes. Agent-Logs-Url: https://github.com/microsoft/vscode-pull-request-github/sessions/42a1cce0-564c-42dd-afc9-909f2730d385 Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com>
1 parent 5431f94 commit f1d810d

File tree

2 files changed

+148
-10
lines changed

2 files changed

+148
-10
lines changed

src/issues/stateManager.ts

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -345,20 +345,22 @@ export class StateManager {
345345
singleRepoState.maxIssueNumber = await folderManager.getMaxIssue();
346346
singleRepoState.lastHead = folderManager.repository.state.HEAD?.commit;
347347
singleRepoState.lastBranch = folderManager.repository.state.HEAD?.name;
348+
this._onDidChangeIssueData.fire();
348349
}
349350

350-
private setIssues(folderManager: FolderRepositoryManager, query: string): Promise<IssueItem[] | undefined> {
351-
return new Promise(async resolve => {
351+
private async setIssues(folderManager: FolderRepositoryManager, query: string): Promise<IssueItem[] | undefined> {
352+
try {
352353
const issues = await folderManager.getIssues(query, { fetchNextPage: false, fetchOnePagePerRepo: true });
354+
return issues?.items.map(item => {
355+
const issueItem: IssueItem = item as IssueItem;
356+
issueItem.uri = folderManager.repository.rootUri;
357+
return issueItem;
358+
});
359+
} catch (e) {
360+
return undefined;
361+
} finally {
353362
this._onDidChangeIssueData.fire();
354-
resolve(
355-
issues?.items.map(item => {
356-
const issueItem: IssueItem = item as IssueItem;
357-
issueItem.uri = folderManager.repository.rootUri;
358-
return issueItem;
359-
}),
360-
);
361-
});
363+
}
362364
}
363365

364366
private async setCurrentIssueFromBranch(singleRepoState: SingleRepoState, branchName: string, silent: boolean = false) {

src/test/issues/stateManager.test.ts

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,4 +169,140 @@ describe('StateManager branch behavior with useBranchForIssues setting', functio
169169
vscode.workspace.getConfiguration = originalGetConfiguration;
170170
}
171171
});
172+
173+
it('should fire onDidChangeIssueData even when getIssues throws', async function () {
174+
const mockUri = vscode.Uri.parse('file:///test');
175+
const mockFolderManager = {
176+
repository: { rootUri: mockUri, state: { HEAD: { commit: 'abc123' }, remotes: [] } },
177+
getIssues: async () => {
178+
throw new Error('Network error');
179+
},
180+
getMaxIssue: async () => 0,
181+
};
182+
183+
const originalGetConfiguration = vscode.workspace.getConfiguration;
184+
vscode.workspace.getConfiguration = (section?: string) => {
185+
if (section === ISSUES_SETTINGS_NAMESPACE) {
186+
return {
187+
get: (key: string, defaultValue?: any) => {
188+
if (key === 'queries') {
189+
return [{ label: 'Test', query: 'is:open assignee:@me repo:owner/repo', groupBy: [] }];
190+
}
191+
return defaultValue;
192+
},
193+
} as any;
194+
}
195+
return originalGetConfiguration(section);
196+
};
197+
198+
try {
199+
const sm = new StateManager(undefined as any, {
200+
folderManagers: [mockFolderManager],
201+
credentialStore: { isAnyAuthenticated: () => true, getCurrentUser: async () => ({ login: 'testuser' }) },
202+
} as any, mockContext);
203+
204+
let changeEventCount = 0;
205+
sm.onDidChangeIssueData(() => changeEventCount++);
206+
207+
await (sm as any).setIssueData(mockFolderManager);
208+
209+
// The event should have fired even though getIssues threw
210+
assert.ok(changeEventCount > 0, 'onDidChangeIssueData should fire even when getIssues fails');
211+
212+
// The promise in the collection should resolve to undefined issues (not reject)
213+
const collection = sm.getIssueCollection(mockUri);
214+
const queryResult = await collection.get('Test');
215+
assert.strictEqual(queryResult?.issues, undefined, 'Issues should be undefined when getIssues fails');
216+
} finally {
217+
vscode.workspace.getConfiguration = originalGetConfiguration;
218+
}
219+
});
220+
221+
it('should fire onDidChangeIssueData after setIssueData completes', async function () {
222+
const mockUri = vscode.Uri.parse('file:///test');
223+
const mockFolderManager = {
224+
repository: { rootUri: mockUri, state: { HEAD: { commit: 'abc123' }, remotes: [] } },
225+
getIssues: async () => {
226+
return { items: [], hasMorePages: false, hasUnsearchedRepositories: false, totalCount: 0 };
227+
},
228+
getMaxIssue: async () => 0,
229+
};
230+
231+
const originalGetConfiguration = vscode.workspace.getConfiguration;
232+
vscode.workspace.getConfiguration = (section?: string) => {
233+
if (section === ISSUES_SETTINGS_NAMESPACE) {
234+
return {
235+
get: (key: string, defaultValue?: any) => {
236+
if (key === 'queries') {
237+
return [{ label: 'Test', query: 'is:open assignee:@me repo:owner/repo', groupBy: [] }];
238+
}
239+
return defaultValue;
240+
},
241+
} as any;
242+
}
243+
return originalGetConfiguration(section);
244+
};
245+
246+
try {
247+
const sm = new StateManager(undefined as any, {
248+
folderManagers: [mockFolderManager],
249+
credentialStore: { isAnyAuthenticated: () => true, getCurrentUser: async () => ({ login: 'testuser' }) },
250+
} as any, mockContext);
251+
252+
let changeEventCount = 0;
253+
sm.onDidChangeIssueData(() => changeEventCount++);
254+
255+
await (sm as any).setIssueData(mockFolderManager);
256+
257+
// The event should fire at least twice: once from setIssues (per-query) and once from setIssueData (final)
258+
assert.ok(changeEventCount >= 2, `onDidChangeIssueData should fire at least twice, but fired ${changeEventCount} times`);
259+
} finally {
260+
vscode.workspace.getConfiguration = originalGetConfiguration;
261+
}
262+
});
263+
264+
it('should not reject promises in issueCollection when getIssues throws', async function () {
265+
const mockUri = vscode.Uri.parse('file:///test');
266+
const mockFolderManager = {
267+
repository: { rootUri: mockUri, state: { HEAD: { commit: 'abc123' }, remotes: [] } },
268+
getIssues: async () => {
269+
throw new Error('API error');
270+
},
271+
getMaxIssue: async () => 0,
272+
};
273+
274+
const originalGetConfiguration = vscode.workspace.getConfiguration;
275+
vscode.workspace.getConfiguration = (section?: string) => {
276+
if (section === ISSUES_SETTINGS_NAMESPACE) {
277+
return {
278+
get: (key: string, defaultValue?: any) => {
279+
if (key === 'queries') {
280+
return [{ label: 'Test', query: 'is:open repo:owner/repo', groupBy: [] }];
281+
}
282+
return defaultValue;
283+
},
284+
} as any;
285+
}
286+
return originalGetConfiguration(section);
287+
};
288+
289+
try {
290+
const sm = new StateManager(undefined as any, {
291+
folderManagers: [mockFolderManager],
292+
credentialStore: { isAnyAuthenticated: () => true, getCurrentUser: async () => ({ login: 'testuser' }) },
293+
} as any, mockContext);
294+
295+
await (sm as any).setIssueData(mockFolderManager);
296+
297+
// Verify that the promises in issueCollection resolve (not reject)
298+
const collection = sm.getIssueCollection(mockUri);
299+
for (const [, promise] of collection) {
300+
const result = await promise;
301+
assert.ok(result !== undefined, 'Promise should resolve, not reject');
302+
assert.strictEqual(result.issues, undefined, 'Issues should be undefined on error');
303+
}
304+
} finally {
305+
vscode.workspace.getConfiguration = originalGetConfiguration;
306+
}
307+
});
172308
});

0 commit comments

Comments
 (0)