Make handleCompareWith cleaner

Author: robertbrignull

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

@@ -15,7 +15,6 @@ import {
 import { QueryHistoryConfig } from "../config";
 import {
   showAndLogErrorMessage,
-  showAndLogExceptionWithTelemetry,
   showAndLogInformationMessage,
   showAndLogWarningMessage,
   showBinaryChoiceDialog,
@@ -25,7 +24,7 @@ import { extLogger } from "../common";
 import { URLSearchParams } from "url";
 import { DisposableObject } from "../pure/disposable-object";
 import { ONE_HOUR_IN_MS, TWO_HOURS_IN_MS } from "../pure/time";
-import { asError, assertNever, getErrorMessage } from "../pure/helpers-pure";
+import { assertNever, getErrorMessage } from "../pure/helpers-pure";
 import { CompletedLocalQueryInfo, LocalQueryInfo } from "../query-results";
 import {
   getActionsWorkflowRunUrl,
@@ -54,7 +53,6 @@ import { VariantAnalysisManager } from "../variant-analysis/variant-analysis-man
 import { VariantAnalysisHistoryItem } from "./variant-analysis-history-item";
 import { getTotalResultCount } from "../variant-analysis/shared/variant-analysis";
 import { HistoryTreeDataProvider } from "./history-tree-data-provider";
-import { redactableError } from "../pure/errors";
 import { QueryHistoryDirs } from "./query-history-dirs";
 import { QueryHistoryCommands } from "../common/commands";
 import { App } from "../common/app";
@@ -587,43 +585,46 @@ export class QueryHistoryManager extends DisposableObject {
     }
   }
 
+  isSuccessfulCompletedLocalQueryInfo(
+    item: QueryHistoryInfo,
+  ): item is CompletedLocalQueryInfo {
+    return item.t === "local" && item.completedQuery?.successful === true;
+  }
+
   async handleCompareWith(
     singleItem: QueryHistoryInfo,
     multiSelect: QueryHistoryInfo[] | undefined,
   ) {
-    const { finalSingleItem, finalMultiSelect } = this.determineSelection(
-      singleItem,
-      multiSelect,
-    );
-
-    try {
-      // local queries only
-      if (finalSingleItem?.t !== "local") {
-        throw new Error("Please select a local query.");
-      }
+    multiSelect ||= [singleItem];
 
-      if (!finalSingleItem.completedQuery?.successful) {
-        throw new Error(
-          "Please select a query that has completed successfully.",
-        );
-      }
+    if (
+      !this.isSuccessfulCompletedLocalQueryInfo(singleItem) ||
+      !multiSelect.every(this.isSuccessfulCompletedLocalQueryInfo)
+    ) {
+      throw new Error(
+        "Please only select local queries that have completed successfully.",
+      );
+    }
 
-      const from = this.compareWithItem || singleItem;
-      const to = await this.findOtherQueryToCompare(from, finalMultiSelect);
+    const fromItem =
+      this.compareWithItem &&
+      this.isSuccessfulCompletedLocalQueryInfo(this.compareWithItem) &&
+      multiSelect.includes(this.compareWithItem)
+        ? this.compareWithItem
+        : singleItem;
 
-      if (from.completed && to?.completed) {
-        await this.doCompareCallback(
-          from as CompletedLocalQueryInfo,
-          to as CompletedLocalQueryInfo,
-        );
-      }
+    let toItem: CompletedLocalQueryInfo | undefined = undefined;
+    try {
+      toItem = await this.findOtherQueryToCompare(fromItem, multiSelect);
     } catch (e) {
-      void showAndLogExceptionWithTelemetry(
-        redactableError(
-          asError(e),
-        )`Failed to compare queries: ${getErrorMessage(e)}`,
+      void showAndLogErrorMessage(
+        `Failed to compare queries: ${getErrorMessage(e)}`,
       );
     }
+
+    if (toItem !== undefined) {
+      await this.doCompareCallback(fromItem, toItem);
+    }
   }
 
   async handleItemClicked(
@@ -1177,57 +1178,40 @@ export class QueryHistoryManager extends DisposableObject {
   }
 
   private async findOtherQueryToCompare(
-    singleItem: QueryHistoryInfo,
-    multiSelect: QueryHistoryInfo[],
+    fromItem: CompletedLocalQueryInfo,
+    allItemsSelected: CompletedLocalQueryInfo[],
   ): Promise<CompletedLocalQueryInfo | undefined> {
-    // Variant analyses cannot be compared
-    if (
-      singleItem.t !== "local" ||
-      multiSelect.some((s) => s.t !== "local") ||
-      !singleItem.completedQuery
-    ) {
-      return undefined;
-    }
-    const dbName = singleItem.initialInfo.databaseInfo.name;
+    const dbName = fromItem.initialInfo.databaseInfo.name;
 
     // if exactly 2 queries are selected, use those
-    if (multiSelect?.length === 2) {
-      // return the query that is not the first selected one
-      const otherQuery = (
-        singleItem === multiSelect[0] ? multiSelect[1] : multiSelect[0]
-      ) as LocalQueryInfo;
-      if (!otherQuery.completedQuery) {
-        throw new Error("Please select a completed query.");
-      }
-      if (!otherQuery.completedQuery.successful) {
-        throw new Error("Please select a successful query.");
-      }
-      if (otherQuery.initialInfo.databaseInfo.name !== dbName) {
+    if (allItemsSelected.length === 2) {
+      const otherItem =
+        fromItem === allItemsSelected[0]
+          ? allItemsSelected[1]
+          : allItemsSelected[0];
+      if (otherItem.initialInfo.databaseInfo.name !== dbName) {
         throw new Error("Query databases must be the same.");
       }
-      return otherQuery as CompletedLocalQueryInfo;
+      return otherItem;
     }
 
-    if (multiSelect?.length > 2) {
+    if (allItemsSelected.length > 2) {
       throw new Error("Please select no more than 2 queries.");
     }
 
     // otherwise, let the user choose
     const comparableQueryLabels = this.treeDataProvider.allHistory
+      .filter(this.isSuccessfulCompletedLocalQueryInfo)
       .filter(
-        (otherQuery) =>
-          otherQuery !== singleItem &&
-          otherQuery.t === "local" &&
-          otherQuery.completedQuery &&
-          otherQuery.completedQuery.successful &&
-          otherQuery.initialInfo.databaseInfo.name === dbName,
+        (otherItem) =>
+          otherItem !== fromItem &&
+          otherItem.initialInfo.databaseInfo.name === dbName,
       )
       .map((item) => ({
         label: this.labelProvider.getLabel(item),
-        description: (item as CompletedLocalQueryInfo).initialInfo.databaseInfo
-          .name,
-        detail: (item as CompletedLocalQueryInfo).completedQuery.statusString,
-        query: item as CompletedLocalQueryInfo,
+        description: item.initialInfo.databaseInfo.name,
+        detail: item.completedQuery.statusString,
+        query: item,
       }));
     if (comparableQueryLabels.length < 1) {
       throw new Error("No other queries available to compare with.");

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

@@ -977,24 +977,6 @@ describe("QueryHistoryManager", () => {
         expect(showQuickPickSpy).not.toBeCalled();
       });
 
-      it("should throw an error when a query is not successful", async () => {
-        const thisQuery = localQueryHistory[3];
-        queryHistoryManager = await createMockQueryHistory(allHistory);
-        allHistory[0] = createMockLocalQueryInfo({
-          dbName: "a",
-          queryWithResults: createMockQueryWithResults({
-            didRunSuccessfully: false,
-          }),
-        });
-
-        await expect(
-          (queryHistoryManager as any).findOtherQueryToCompare(thisQuery, [
-            thisQuery,
-            allHistory[0],
-          ]),
-        ).rejects.toThrow("Please select a successful query.");
-      });
-
       it("should throw an error when a databases are not the same", async () => {
         queryHistoryManager = await createMockQueryHistory(allHistory);
 
@@ -1043,6 +1025,26 @@ describe("QueryHistoryManager", () => {
         ]);
         expect(doCompareCallback).not.toBeCalled();
       });
+
+      it("should throw an error when a query is not successful", async () => {
+        const thisQuery = localQueryHistory[3];
+        queryHistoryManager = await createMockQueryHistory(allHistory);
+        allHistory[0] = createMockLocalQueryInfo({
+          dbName: "a",
+          queryWithResults: createMockQueryWithResults({
+            didRunSuccessfully: false,
+          }),
+        });
+
+        await expect(
+          queryHistoryManager.handleCompareWith(thisQuery, [
+            thisQuery,
+            allHistory[0],
+          ]),
+        ).rejects.toThrow(
+          "Please only select local queries that have completed successfully.",
+        );
+      });
     });
 
     describe("updateCompareWith", () => {