Skip to content

Commit 2f48bc0

Browse files
Copilotalexr00
andauthored
Fix issues tree view stuck empty until git commit (#8638)
* Initial plan * 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> * Address review feedback: add comment to catch block, tighten test assertion 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> * Remove "just in case" event firing * Fix test --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com>
1 parent affe95e commit 2f48bc0

2 files changed

Lines changed: 108 additions & 10 deletions

File tree

src/issues/stateManager.ts

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -347,18 +347,21 @@ export class StateManager {
347347
singleRepoState.lastBranch = folderManager.repository.state.HEAD?.name;
348348
}
349349

350-
private setIssues(folderManager: FolderRepositoryManager, query: string): Promise<IssueItem[] | undefined> {
351-
return new Promise(async resolve => {
350+
private async setIssues(folderManager: FolderRepositoryManager, query: string): Promise<IssueItem[] | undefined> {
351+
try {
352352
const issues = await folderManager.getIssues(query, { fetchNextPage: false, fetchOnePagePerRepo: true });
353+
return issues?.items.map(item => {
354+
const issueItem: IssueItem = item as IssueItem;
355+
issueItem.uri = folderManager.repository.rootUri;
356+
return issueItem;
357+
});
358+
} catch {
359+
// Errors from fetching issues are expected (e.g. network failures).
360+
// Return undefined so the tree shows an empty state for this query.
361+
return undefined;
362+
} finally {
353363
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-
});
364+
}
362365
}
363366

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

src/test/issues/stateManager.test.ts

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,4 +169,99 @@ 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+
(sm as any)._queries = [{ label: 'Test', query: 'is:open assignee:@me repo:owner/repo', groupBy: [] }];
205+
206+
let changeEventCount = 0;
207+
sm.onDidChangeIssueData(() => changeEventCount++);
208+
209+
await (sm as any).setIssueData(mockFolderManager);
210+
211+
// setIssueData doesn't await setIssues - await the collection promises so the finally block fires
212+
const collection = sm.getIssueCollection(mockUri);
213+
const queryResult = await collection.get('Test');
214+
215+
// The event should have fired even though getIssues threw
216+
assert.ok(changeEventCount > 0, 'onDidChangeIssueData should fire even when getIssues fails');
217+
assert.strictEqual(queryResult?.issues, undefined, 'Issues should be undefined when getIssues fails');
218+
} finally {
219+
vscode.workspace.getConfiguration = originalGetConfiguration;
220+
}
221+
});
222+
223+
it('should not reject promises in issueCollection when getIssues throws', async function () {
224+
const mockUri = vscode.Uri.parse('file:///test');
225+
const mockFolderManager = {
226+
repository: { rootUri: mockUri, state: { HEAD: { commit: 'abc123' }, remotes: [] } },
227+
getIssues: async () => {
228+
throw new Error('API error');
229+
},
230+
getMaxIssue: async () => 0,
231+
};
232+
233+
const originalGetConfiguration = vscode.workspace.getConfiguration;
234+
vscode.workspace.getConfiguration = (section?: string) => {
235+
if (section === ISSUES_SETTINGS_NAMESPACE) {
236+
return {
237+
get: (key: string, defaultValue?: any) => {
238+
if (key === 'queries') {
239+
return [{ label: 'Test', query: 'is:open repo:owner/repo', groupBy: [] }];
240+
}
241+
return defaultValue;
242+
},
243+
} as any;
244+
}
245+
return originalGetConfiguration(section);
246+
};
247+
248+
try {
249+
const sm = new StateManager(undefined as any, {
250+
folderManagers: [mockFolderManager],
251+
credentialStore: { isAnyAuthenticated: () => true, getCurrentUser: async () => ({ login: 'testuser' }) },
252+
} as any, mockContext);
253+
254+
await (sm as any).setIssueData(mockFolderManager);
255+
256+
// Verify that the promises in issueCollection resolve (not reject)
257+
const collection = sm.getIssueCollection(mockUri);
258+
for (const [, promise] of collection) {
259+
const result = await promise;
260+
assert.ok(result !== undefined, 'Promise should resolve, not reject');
261+
assert.strictEqual(result.issues, undefined, 'Issues should be undefined on error');
262+
}
263+
} finally {
264+
vscode.workspace.getConfiguration = originalGetConfiguration;
265+
}
266+
});
172267
});

0 commit comments

Comments
 (0)