Skip to content

Commit f9ab09d

Browse files
authored
Fix race condition in handleRemoveHistoryItem causing wrong current item on Windows
The `handleRemoveHistoryItem` method called `getCurrent()` after several async operations. On Windows, VS Code fires `onDidChangeSelection` asynchronously during awaits (after `refresh()` from `remove()`), which could change the current item to an unexpected value before `getCurrent()` was called. Fix by: 1. Capturing the current item *before* any async operations in `handleRemoveHistoryItem`, and restoring it afterwards if the item is still in history (i.e. it was not one of the removed items). Also add an explicit `setCurrentItem(current)` call before `reveal` to sync the internal state in case a deferred `onDidChangeSelection` changed it during the awaits. 2. Mocking `showInformationMessageWithAction` in the test to return immediately instead of calling the real VS Code API, which could trigger additional async event processing. Fixes the failing "Test (windows-latest)" CI job: QueryHistoryManager › handleRemoveHistoryItem › when the item is a variant analysis › when in progress › when the item being removed is not selected › should not change the selection
1 parent 1c0f453 commit f9ab09d

2 files changed

Lines changed: 25 additions & 5 deletions

File tree

extensions/ql-vscode/src/query-history/query-history-manager.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -565,6 +565,13 @@ export class QueryHistoryManager extends DisposableObject {
565565
}
566566

567567
async handleRemoveHistoryItem(items: QueryHistoryInfo[]) {
568+
// Capture the current item before any async operations. VS Code may fire
569+
// onDidChangeSelection asynchronously during awaits below (e.g. after the
570+
// tree is refreshed by remove()), changing treeDataProvider.getCurrent() to
571+
// an unexpected item. By saving it up front we can restore the correct
572+
// selection afterwards.
573+
const previousCurrent = this.treeDataProvider.getCurrent();
574+
568575
await Promise.all(
569576
items.map(async (item) => {
570577
if (item.t === "local") {
@@ -602,8 +609,22 @@ export class QueryHistoryManager extends DisposableObject {
602609
);
603610

604611
await this.writeQueryHistory();
605-
const current = this.treeDataProvider.getCurrent();
612+
613+
// If the previously-current item is still in history (i.e. it was not one
614+
// of the items being removed), keep it as the current item. Otherwise fall
615+
// back to whatever getCurrent() returns, which was set by remove() when the
616+
// current item itself was deleted.
617+
const current =
618+
previousCurrent !== undefined &&
619+
this.treeDataProvider.allHistory.includes(previousCurrent)
620+
? previousCurrent
621+
: this.treeDataProvider.getCurrent();
622+
606623
if (current !== undefined) {
624+
// Explicitly sync the internal current-item state before revealing, in
625+
// case a deferred onDidChangeSelection event changed it during the awaits
626+
// above.
627+
this.treeDataProvider.setCurrentItem(current);
607628
await this.treeView.reveal(current, { select: true });
608629
await this.openQueryResults(current);
609630
}

extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-manager.test.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -331,10 +331,9 @@ describe("QueryHistoryManager", () => {
331331
.spyOn(dialog, "showBinaryChoiceDialog")
332332
.mockResolvedValue(true);
333333

334-
showInformationMessageWithActionSpy = jest.spyOn(
335-
dialog,
336-
"showInformationMessageWithAction",
337-
);
334+
showInformationMessageWithActionSpy = jest
335+
.spyOn(dialog, "showInformationMessageWithAction")
336+
.mockResolvedValue(false);
338337
});
339338

340339
describe("when in progress", () => {

0 commit comments

Comments
 (0)