diff --git a/extensions/ql-vscode/src/query-history/query-history-manager.ts b/extensions/ql-vscode/src/query-history/query-history-manager.ts index a57216b947f..5bf0caf84a3 100644 --- a/extensions/ql-vscode/src/query-history/query-history-manager.ts +++ b/extensions/ql-vscode/src/query-history/query-history-manager.ts @@ -396,38 +396,34 @@ export class QueryHistoryManager extends DisposableObject { } async handleOpenQuery( - singleItem: QueryHistoryInfo | undefined, + singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ): Promise { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - if (!this.assertSingleQuery(finalMultiSelect) || !finalSingleItem) { + if (!this.assertSingleQuery(multiSelect)) { return; } - if (finalSingleItem.t === "variant-analysis") { + if (singleItem.t === "variant-analysis") { await this.variantAnalysisManager.openQueryFile( - finalSingleItem.variantAnalysis.id, + singleItem.variantAnalysis.id, ); return; } let queryPath: string; - switch (finalSingleItem.t) { + switch (singleItem.t) { case "local": - queryPath = finalSingleItem.initialInfo.queryPath; + queryPath = singleItem.initialInfo.queryPath; break; default: - assertNever(finalSingleItem); + assertNever(singleItem); } const textDocument = await workspace.openTextDocument(Uri.file(queryPath)); const editor = await window.showTextDocument(textDocument, ViewColumn.One); - if (finalSingleItem.t === "local") { - const queryText = finalSingleItem.initialInfo.queryText; - if (queryText !== undefined && finalSingleItem.initialInfo.isQuickQuery) { + if (singleItem.t === "local") { + const queryText = singleItem.initialInfo.queryText; + if (queryText !== undefined && singleItem.initialInfo.isQuickQuery) { await editor.edit((edit) => edit.replace( textDocument.validateRange( @@ -459,16 +455,12 @@ export class QueryHistoryManager extends DisposableObject { } async handleRemoveHistoryItem( - singleItem: QueryHistoryInfo | undefined, + singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - const toDelete = finalMultiSelect || [finalSingleItem]; + multiSelect ||= [singleItem]; await Promise.all( - toDelete.map(async (item) => { + multiSelect.map(async (item) => { if (item.t === "local") { // Removing in progress local queries is not supported. They must be cancelled first. if (item.status !== QueryStatus.InProgress) { @@ -562,18 +554,13 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ): Promise { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - - if (!this.assertSingleQuery(finalMultiSelect) || !finalSingleItem) { + if (!this.assertSingleQuery(multiSelect)) { return; } const response = await window.showInputBox({ placeHolder: `(use default: ${this.queryHistoryConfigListener.format})`, - value: finalSingleItem.userSpecifiedLabel ?? "", + value: singleItem.userSpecifiedLabel ?? "", title: "Set query label", prompt: "Set the query history item label. See the description of the codeQL.queryHistory.format setting for more information.", @@ -581,8 +568,7 @@ export class QueryHistoryManager extends DisposableObject { // undefined response means the user cancelled the dialog; don't change anything if (response !== undefined) { // Interpret empty string response as 'go back to using default' - finalSingleItem.userSpecifiedLabel = - response === "" ? undefined : response; + singleItem.userSpecifiedLabel = response === "" ? undefined : response; await this.refreshTreeView(); } } @@ -591,25 +577,22 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); + multiSelect ||= [singleItem]; try { // local queries only - if (finalSingleItem?.t !== "local") { + if (singleItem?.t !== "local") { throw new Error("Please select a local query."); } - if (!finalSingleItem.completedQuery?.successful) { + if (!singleItem.completedQuery?.successful) { throw new Error( "Please select a query that has completed successfully.", ); } const from = this.compareWithItem || singleItem; - const to = await this.findOtherQueryToCompare(from, finalMultiSelect); + const to = await this.findOtherQueryToCompare(from, multiSelect); if (from.completed && to?.completed) { await this.doCompareCallback( @@ -627,36 +610,32 @@ export class QueryHistoryManager extends DisposableObject { } async handleItemClicked( - singleItem: QueryHistoryInfo | undefined, + singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - if (!this.assertSingleQuery(finalMultiSelect) || !finalSingleItem) { + if (!this.assertSingleQuery(multiSelect)) { return; } - this.treeDataProvider.setCurrentItem(finalSingleItem); + this.treeDataProvider.setCurrentItem(singleItem); const now = new Date(); const prevItemClick = this.lastItemClick; - this.lastItemClick = { time: now, item: finalSingleItem }; + this.lastItemClick = { time: now, item: singleItem }; if ( prevItemClick !== undefined && now.valueOf() - prevItemClick.time.valueOf() < DOUBLE_CLICK_TIME && - finalSingleItem === prevItemClick.item + singleItem === prevItemClick.item ) { // show original query file on double click - await this.handleOpenQuery(finalSingleItem, [finalSingleItem]); + await this.handleOpenQuery(singleItem, [singleItem]); } else if ( - finalSingleItem.t === "variant-analysis" || - finalSingleItem.status === QueryStatus.Completed + singleItem.t === "variant-analysis" || + singleItem.status === QueryStatus.Completed ) { // show results on single click (if results view is available) - await this.openQueryResults(finalSingleItem); + await this.openQueryResults(singleItem); } } @@ -705,32 +684,27 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - - if (!this.assertSingleQuery(finalMultiSelect) || !finalSingleItem) { + if (!this.assertSingleQuery(multiSelect)) { return; } let externalFilePath: string | undefined; - if (finalSingleItem.t === "local") { - if (finalSingleItem.completedQuery) { + if (singleItem.t === "local") { + if (singleItem.completedQuery) { externalFilePath = join( - finalSingleItem.completedQuery.query.querySaveDir, + singleItem.completedQuery.query.querySaveDir, "timestamp", ); } - } else if (finalSingleItem.t === "variant-analysis") { + } else if (singleItem.t === "variant-analysis") { externalFilePath = join( this.variantAnalysisManager.getVariantAnalysisStorageLocation( - finalSingleItem.variantAnalysis.id, + singleItem.variantAnalysis.id, ), "timestamp", ); } else { - assertNever(finalSingleItem); + assertNever(singleItem); } if (externalFilePath) { @@ -779,25 +753,13 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - // Only applicable to an individual local query - if ( - !this.assertSingleQuery(finalMultiSelect) || - !finalSingleItem || - finalSingleItem.t !== "local" - ) { + if (!this.assertSingleQuery(multiSelect) || singleItem.t !== "local") { return; } - if (finalSingleItem.evalLogLocation) { - await tryOpenExternalFile( - this.app.commands, - finalSingleItem.evalLogLocation, - ); + if (singleItem.evalLogLocation) { + await tryOpenExternalFile(this.app.commands, singleItem.evalLogLocation); } else { this.warnNoEvalLogs(); } @@ -807,32 +769,23 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - // Only applicable to an individual local query - if ( - !this.assertSingleQuery(finalMultiSelect) || - !finalSingleItem || - finalSingleItem.t !== "local" - ) { + if (!this.assertSingleQuery(multiSelect) || singleItem.t !== "local") { return; } - if (finalSingleItem.evalLogSummaryLocation) { + if (singleItem.evalLogSummaryLocation) { await tryOpenExternalFile( this.app.commands, - finalSingleItem.evalLogSummaryLocation, + singleItem.evalLogSummaryLocation, ); return; } // Summary log file doesn't exist. if ( - finalSingleItem.evalLogLocation && - (await pathExists(finalSingleItem.evalLogLocation)) + singleItem.evalLogLocation && + (await pathExists(singleItem.evalLogLocation)) ) { // If raw log does exist, then the summary log is still being generated. this.warnInProgressEvalLogSummary(); @@ -845,21 +798,13 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); // Only applicable to an individual local query - if ( - !this.assertSingleQuery(finalMultiSelect) || - !finalSingleItem || - finalSingleItem.t !== "local" - ) { + if (!this.assertSingleQuery(multiSelect) || singleItem.t !== "local") { return; } // If the JSON summary file location wasn't saved, display error - if (finalSingleItem.jsonEvalLogSummaryLocation === undefined) { + if (singleItem.jsonEvalLogSummaryLocation === undefined) { this.warnInProgressEvalLogViewer(); return; } @@ -867,16 +812,16 @@ export class QueryHistoryManager extends DisposableObject { // TODO(angelapwen): Stream the file in. try { const evalLogData: EvalLogData[] = await parseViewerData( - finalSingleItem.jsonEvalLogSummaryLocation, + singleItem.jsonEvalLogSummaryLocation, ); const evalLogTreeBuilder = new EvalLogTreeBuilder( - finalSingleItem.getQueryName(), + singleItem.getQueryName(), evalLogData, ); this.evalLogViewer.updateRoots(await evalLogTreeBuilder.getRoots()); } catch (e) { throw new Error( - `Could not read evaluator log summary JSON file to generate viewer data at ${finalSingleItem.jsonEvalLogSummaryLocation}.`, + `Could not read evaluator log summary JSON file to generate viewer data at ${singleItem.jsonEvalLogSummaryLocation}.`, ); } } @@ -885,14 +830,9 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - - const selected = finalMultiSelect || [finalSingleItem]; + multiSelect ||= [singleItem]; - const results = selected.map(async (item) => { + const results = multiSelect.map(async (item) => { if (item.status === QueryStatus.InProgress) { if (item.t === "local") { item.cancel(); @@ -913,18 +853,13 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] = [], ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - - if (!this.assertSingleQuery(finalMultiSelect) || !finalSingleItem) { + if (!this.assertSingleQuery(multiSelect)) { return; } - if (finalSingleItem.t === "variant-analysis") { + if (singleItem.t === "variant-analysis") { await this.variantAnalysisManager.openQueryText( - finalSingleItem.variantAnalysis.id, + singleItem.variantAnalysis.id, ); return; } @@ -932,14 +867,13 @@ export class QueryHistoryManager extends DisposableObject { const params = new URLSearchParams({ isQuickEval: String( !!( - finalSingleItem.t === "local" && - finalSingleItem.initialInfo.quickEvalPosition + singleItem.t === "local" && singleItem.initialInfo.quickEvalPosition ), ), - queryText: encodeURIComponent(getQueryText(finalSingleItem)), + queryText: encodeURIComponent(getQueryText(singleItem)), }); - const queryId = getQueryId(finalSingleItem); + const queryId = getQueryId(singleItem); const uri = Uri.parse(`codeql:${queryId}.ql?${params.toString()}`, true); const doc = await workspace.openTextDocument(uri); @@ -950,22 +884,16 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - // Local queries only if ( - !this.assertSingleQuery(finalMultiSelect) || - !finalSingleItem || - finalSingleItem.t !== "local" || - !finalSingleItem.completedQuery + !this.assertSingleQuery(multiSelect) || + singleItem.t !== "local" || + !singleItem.completedQuery ) { return; } - const query = finalSingleItem.completedQuery.query; + const query = singleItem.completedQuery.query; const hasInterpretedResults = query.canHaveInterpretedResults(); if (hasInterpretedResults) { await tryOpenExternalFile( @@ -973,7 +901,7 @@ export class QueryHistoryManager extends DisposableObject { query.resultsPaths.interpretedResultsPath, ); } else { - const label = this.labelProvider.getLabel(finalSingleItem); + const label = this.labelProvider.getLabel(singleItem); void showAndLogInformationMessage( `Query ${label} has no interpreted results.`, ); @@ -984,21 +912,15 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - // Local queries only if ( - !this.assertSingleQuery(finalMultiSelect) || - !finalSingleItem || - finalSingleItem.t !== "local" || - !finalSingleItem.completedQuery + !this.assertSingleQuery(multiSelect) || + singleItem.t !== "local" || + !singleItem.completedQuery ) { return; } - const query = finalSingleItem.completedQuery.query; + const query = singleItem.completedQuery.query; if (await query.hasCsv()) { void tryOpenExternalFile(this.app.commands, query.csvPath); return; @@ -1012,24 +934,18 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - // Local queries only if ( - !this.assertSingleQuery(finalMultiSelect) || - !finalSingleItem || - finalSingleItem.t !== "local" || - !finalSingleItem.completedQuery + !this.assertSingleQuery(multiSelect) || + singleItem.t !== "local" || + !singleItem.completedQuery ) { return; } await tryOpenExternalFile( this.app.commands, - await finalSingleItem.completedQuery.query.ensureCsvAlerts( + await singleItem.completedQuery.query.ensureCsvAlerts( this.qs.cliServer, this.dbm, ), @@ -1040,26 +956,18 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - // Local queries only if ( - !this.assertSingleQuery(finalMultiSelect) || - !finalSingleItem || - finalSingleItem.t !== "local" || - !finalSingleItem.completedQuery + !this.assertSingleQuery(multiSelect) || + singleItem.t !== "local" || + !singleItem.completedQuery ) { return; } await tryOpenExternalFile( this.app.commands, - await finalSingleItem.completedQuery.query.ensureDilPath( - this.qs.cliServer, - ), + await singleItem.completedQuery.query.ensureDilPath(this.qs.cliServer), ); } @@ -1067,20 +975,14 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - - if (!this.assertSingleQuery(finalMultiSelect) || !finalSingleItem) { - return; - } - - if (finalSingleItem.t === "local") { + if ( + !this.assertSingleQuery(multiSelect) || + singleItem.t !== "variant-analysis" + ) { return; } - const actionsWorkflowRunUrl = getActionsWorkflowRunUrl(finalSingleItem); + const actionsWorkflowRunUrl = getActionsWorkflowRunUrl(singleItem); await this.app.commands.execute( "vscode.open", @@ -1092,23 +994,17 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ) { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - // Variant analyses only if ( - !this.assertSingleQuery(finalMultiSelect) || - !finalSingleItem || - finalSingleItem.t !== "variant-analysis" + !this.assertSingleQuery(multiSelect) || + singleItem.t !== "variant-analysis" ) { return; } await this.app.commands.execute( "codeQL.copyVariantAnalysisRepoList", - finalSingleItem.variantAnalysis.id, + singleItem.variantAnalysis.id, ); } @@ -1116,22 +1012,16 @@ export class QueryHistoryManager extends DisposableObject { singleItem: QueryHistoryInfo, multiSelect: QueryHistoryInfo[] | undefined, ): Promise { - const { finalSingleItem, finalMultiSelect } = this.determineSelection( - singleItem, - multiSelect, - ); - // Variant analysis only if ( - !this.assertSingleQuery(finalMultiSelect) || - !finalSingleItem || - finalSingleItem.t !== "variant-analysis" + !this.assertSingleQuery(multiSelect) || + singleItem.t !== "variant-analysis" ) { return; } await this.variantAnalysisManager.exportResults( - finalSingleItem.variantAnalysis.id, + singleItem.variantAnalysis.id, ); } @@ -1277,52 +1167,6 @@ export class QueryHistoryManager extends DisposableObject { } } - /** - * If no items are selected, attempt to grab the selection from the treeview. - * However, often the treeview itself does not have any selection. In this case, - * grab the selection from the `treeDataProvider` current item. - * - * We need to use this method because when clicking on commands from the view title - * bar, the selections are not passed in. - * - * @param singleItem the single item selected, or undefined if no item is selected - * @param multiSelect a multi-select or undefined if no items are selected - */ - private determineSelection( - singleItem: QueryHistoryInfo | undefined, - multiSelect: QueryHistoryInfo[] | undefined, - ): { - finalSingleItem: QueryHistoryInfo | undefined; - finalMultiSelect: QueryHistoryInfo[]; - } { - if (!singleItem && !multiSelect?.[0]) { - const selection = this.treeView.selection; - const current = this.treeDataProvider.getCurrent(); - if (selection?.length) { - return { - finalSingleItem: selection[0], - finalMultiSelect: [...selection], - }; - } else if (current) { - return { - finalSingleItem: current, - finalMultiSelect: [current], - }; - } - } - - // ensure we only return undefined if we have neither a single or multi-selecion - if (singleItem && !multiSelect?.[0]) { - multiSelect = [singleItem]; - } else if (!singleItem && multiSelect?.[0]) { - singleItem = multiSelect[0]; - } - return { - finalSingleItem: singleItem, - finalMultiSelect: multiSelect || [], - }; - } - async refreshTreeView(): Promise { this.treeDataProvider.refresh(); await this.writeQueryHistory(); diff --git a/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-manager.test.ts b/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-manager.test.ts index 2ca728c6b32..c74636f922a 100644 --- a/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-manager.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-manager.test.ts @@ -247,20 +247,6 @@ describe("QueryHistoryManager", () => { ).toBeUndefined(); }); }); - - describe("no selection", () => { - it("should do nothing", async () => { - queryHistoryManager = await createMockQueryHistory(allHistory); - - await queryHistoryManager.handleItemClicked(undefined!, []); - - expect(localQueriesResultsViewStub.showResults).not.toHaveBeenCalled(); - expect(variantAnalysisManagerStub.showView).not.toHaveBeenCalled(); - expect( - queryHistoryManager.treeDataProvider.getCurrent(), - ).toBeUndefined(); - }); - }); }); describe("handleRemoveHistoryItem", () => { @@ -831,93 +817,6 @@ describe("QueryHistoryManager", () => { }); }); - describe("determineSelection", () => { - const singleItem = "a"; - const multipleItems = ["b", "c", "d"]; - - it("should get the selection from parameters", async () => { - queryHistoryManager = await createMockQueryHistory(allHistory); - const selection = (queryHistoryManager as any).determineSelection( - singleItem, - multipleItems, - ); - expect(selection).toEqual({ - finalSingleItem: singleItem, - finalMultiSelect: multipleItems, - }); - }); - - it("should get the selection when single selection is empty", async () => { - queryHistoryManager = await createMockQueryHistory(allHistory); - const selection = (queryHistoryManager as any).determineSelection( - undefined, - multipleItems, - ); - expect(selection).toEqual({ - finalSingleItem: multipleItems[0], - finalMultiSelect: multipleItems, - }); - }); - - it("should get the selection when multi-selection is empty", async () => { - queryHistoryManager = await createMockQueryHistory(allHistory); - const selection = (queryHistoryManager as any).determineSelection( - singleItem, - undefined, - ); - expect(selection).toEqual({ - finalSingleItem: singleItem, - finalMultiSelect: [singleItem], - }); - }); - - it("should get the selection from the treeView when both selections are empty", async () => { - queryHistoryManager = await createMockQueryHistory(allHistory); - const p = new Promise((done) => { - queryHistoryManager!.treeView.onDidChangeSelection((s) => { - if (s.selection[0] !== allHistory[1]) { - return; - } - const selection = (queryHistoryManager as any).determineSelection( - undefined, - undefined, - ); - expect(selection).toEqual({ - finalSingleItem: allHistory[1], - finalMultiSelect: [allHistory[1]], - }); - done(); - }); - }); - - // I can't explain why, but the first time the onDidChangeSelection event fires, the selection is - // not correct (it is inexplicably allHistory[2]). So we fire the event a second time to get the - // correct selection. - await queryHistoryManager.treeView.reveal(allHistory[0], { - select: true, - }); - await queryHistoryManager.treeView.reveal(allHistory[1], { - select: true, - }); - await p; - }); - - it.skip("should get the selection from the treeDataProvider when both selections and the treeView are empty", async () => { - queryHistoryManager = await createMockQueryHistory(allHistory); - await queryHistoryManager.treeView.reveal(allHistory[1], { - select: true, - }); - const selection = (queryHistoryManager as any).determineSelection( - undefined, - undefined, - ); - expect(selection).toEqual({ - finalSingleItem: allHistory[1], - finalMultiSelect: [allHistory[1]], - }); - }); - }); - describe("Local Queries", () => { describe("findOtherQueryToCompare", () => { it("should find the second query to compare when one is selected", async () => {