From 2a4a91207a3ebf27e675fc968c697a988dc42bbd Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Thu, 6 Apr 2023 13:58:15 +0200 Subject: [PATCH 1/5] Fix empty result view when switching between queries When updating to React 18, we removed the loading step from the updating of the state of the result view since React would batch the updates anyway. However, this caused a bug where the result view would be empty when switching between queries. This is because the result view would retain the old selected result set name. This would not happen previously because React would re-render the view at least once, which would cause the result view to be unmounted and re-created. This fixes it by resetting the selected result set if we can't find the result set in the new result sets. --- .../src/view/results/result-tables.tsx | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/extensions/ql-vscode/src/view/results/result-tables.tsx b/extensions/ql-vscode/src/view/results/result-tables.tsx index fae317f79..dca4a09c0 100644 --- a/extensions/ql-vscode/src/view/results/result-tables.tsx +++ b/extensions/ql-vscode/src/view/results/result-tables.tsx @@ -139,6 +139,31 @@ export class ResultTables extends React.Component< }; } + componentDidUpdate( + prevProps: Readonly, + prevState: Readonly, + snapshot?: any, + ) { + const resultSetExists = + this.props.parsedResultSets.resultSetNames.some( + (v) => this.state.selectedTable === v, + ) || + this.getResultSets().some( + (v) => this.state.selectedTable === v.schema.name, + ); + + // If the selected result set does not exist, select the default result set. + if (!resultSetExists) { + const selectedTable = + this.props.parsedResultSets.selectedTable || + getDefaultResultSet(this.getResultSets()); + this.setState({ + selectedTable, + selectedPage: `${this.props.parsedResultSets.pageNumber + 1}`, + }); + } + } + untoggleProblemsView() { this.setState({ problemsViewSelected: false, From 84928fa2fea55a1faf6ddb9b380c0cfd68bbe5e4 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Thu, 6 Apr 2023 14:09:09 +0200 Subject: [PATCH 2/5] Add test for empty result view bug --- .../view/results/__tests__/results.spec.tsx | 107 ++++++++++++++++-- 1 file changed, 95 insertions(+), 12 deletions(-) diff --git a/extensions/ql-vscode/src/view/results/__tests__/results.spec.tsx b/extensions/ql-vscode/src/view/results/__tests__/results.spec.tsx index f34e63906..c0a416347 100644 --- a/extensions/ql-vscode/src/view/results/__tests__/results.spec.tsx +++ b/extensions/ql-vscode/src/view/results/__tests__/results.spec.tsx @@ -1,5 +1,5 @@ import * as React from "react"; -import { render as reactRender, screen } from "@testing-library/react"; +import { act, render as reactRender, screen } from "@testing-library/react"; import { ResultsApp } from "../results"; import { Interpretation, @@ -20,18 +20,20 @@ const exampleSarif = fs.readJSONSync( describe(ResultsApp.name, () => { const render = () => reactRender(); const postMessage = async (msg: IntoResultsViewMsg) => { - // window.postMessage doesn't set the origin correctly, see - // https://github.com/jsdom/jsdom/issues/2745 - window.dispatchEvent( - new MessageEvent("message", { - source: window, - origin: window.location.origin, - data: msg, - }), - ); + await act(async () => { + // window.postMessage doesn't set the origin correctly, see + // https://github.com/jsdom/jsdom/issues/2745 + window.dispatchEvent( + new MessageEvent("message", { + source: window, + origin: window.location.origin, + data: msg, + }), + ); - // The event is dispatched asynchronously, so we need to wait for it to be handled. - await new Promise((resolve) => setTimeout(resolve, 0)); + // The event is dispatched asynchronously, so we need to wait for it to be handled. + await new Promise((resolve) => setTimeout(resolve, 0)); + }); }; it("renders results", async () => { @@ -95,6 +97,7 @@ describe(ResultsApp.name, () => { }, }, }; + await postMessage(message); expect( @@ -117,4 +120,84 @@ describe(ResultsApp.name, () => { screen.getByText("'x' is assigned a value but never used."), ).toBeInTheDocument(); }); + + it("renders results when switching between queries with different result set names", async () => { + render(); + + await postMessage({ + t: "setState", + interpretation: undefined, + origResultsPaths: { + resultsPath: "/a/b/c/results.bqrs", + interpretedResultsPath: "/a/b/c/interpretedResults.sarif", + }, + resultsPath: "/a/b/c/results.bqrs", + parsedResultSets: { + pageNumber: 0, + pageSize: 200, + numPages: 1, + numInterpretedPages: 0, + resultSet: { + schema: { + name: "#select", + rows: 1, + columns: [{ kind: "s" }], + pagination: { "step-size": 200, offsets: [13] }, + }, + rows: [["foobar1"]], + t: "RawResultSet", + }, + resultSetNames: ["#select"], + }, + sortedResultsMap: {}, + database: { + name: "test-db", + databaseUri: "test-db-uri", + }, + shouldKeepOldResultsWhileRendering: false, + metadata: {}, + queryName: "empty.ql", + queryPath: "/a/b/c/empty.ql", + }); + + expect(screen.getByText("foobar1")).toBeInTheDocument(); + + await postMessage({ + t: "setState", + interpretation: undefined, + origResultsPaths: { + resultsPath: "/a/b/c/results.bqrs", + interpretedResultsPath: "/a/b/c/interpretedResults.sarif", + }, + resultsPath: "/a/b/c/results.bqrs", + parsedResultSets: { + pageNumber: 0, + pageSize: 200, + numPages: 1, + numInterpretedPages: 0, + resultSet: { + schema: { + name: "#Quick_evaluation_of_expression", + rows: 1, + columns: [{ name: "#expr_result", kind: "s" }], + pagination: { "step-size": 200, offsets: [49] }, + }, + rows: [["foobar2"]], + t: "RawResultSet", + }, + resultSetNames: ["#Quick_evaluation_of_expression"], + }, + sortedResultsMap: {}, + database: { + name: "test-db", + databaseUri: "test-db-uri", + }, + shouldKeepOldResultsWhileRendering: false, + metadata: {}, + queryName: "Quick evaluation of empty.ql:1", + queryPath: "/a/b/c/empty.ql", + }); + + expect(screen.getByText("foobar2")).toBeInTheDocument(); + }); }); From 978af54e2a9493599bb20b956ccb54ef3fa730ed Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Thu, 6 Apr 2023 14:25:11 +0200 Subject: [PATCH 3/5] Fix potential concurrency bug in results view This was pointed out by CodeQL: when calling `setState` and using `this.props`, it may not be up-to-date because `setState` may run concurrently. Therefore, we should use the `setState` callback variant to ensure we get the latest props. This refactors the code a bit to ensure we're not using `this.props` anywhere, including in the `getResultSets` function which is called in the `setState` callback. --- .../src/view/results/result-tables.tsx | 119 ++++++++++-------- 1 file changed, 69 insertions(+), 50 deletions(-) diff --git a/extensions/ql-vscode/src/view/results/result-tables.tsx b/extensions/ql-vscode/src/view/results/result-tables.tsx index dca4a09c0..9ae2e5613 100644 --- a/extensions/ql-vscode/src/view/results/result-tables.tsx +++ b/extensions/ql-vscode/src/view/results/result-tables.tsx @@ -78,6 +78,52 @@ function renderResultCountString(resultSet: ResultSet): JSX.Element { ); } +function getInterpretedTableName(interpretation: Interpretation): string { + return interpretation?.data.t === "GraphInterpretationData" + ? GRAPH_TABLE_NAME + : ALERTS_TABLE_NAME; +} + +function getResultSetNames( + interpretation: Interpretation | undefined, + parsedResultSets: ParsedResultSets, +): string[] { + return interpretation + ? parsedResultSets.resultSetNames.concat([ + getInterpretedTableName(interpretation), + ]) + : parsedResultSets.resultSetNames; +} + +function getResultSets( + rawResultSets: readonly ResultSet[], + interpretation: Interpretation | undefined, +): ResultSet[] { + const resultSets: ResultSet[] = + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore 2783 + rawResultSets.map((rs) => ({ t: "RawResultSet", ...rs })); + + if (interpretation !== undefined) { + const tableName = getInterpretedTableName(interpretation); + resultSets.push({ + t: "InterpretedResultSet", + // FIXME: The values of version, columns, tupleCount are + // unused stubs because a InterpretedResultSet schema isn't used the + // same way as a RawResultSet. Probably should pull `name` field + // out. + schema: { + name: tableName, + rows: 1, + columns: [], + }, + name: tableName, + interpretation, + }); + } + return resultSets; +} + /** * Displays multiple `ResultTable` tables, where the table to be displayed is selected by a * dropdown. @@ -86,51 +132,13 @@ export class ResultTables extends React.Component< ResultTablesProps, ResultTablesState > { - private getResultSets(): ResultSet[] { - const resultSets: ResultSet[] = - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore 2783 - this.props.rawResultSets.map((rs) => ({ t: "RawResultSet", ...rs })); - - if (this.props.interpretation !== undefined) { - const tableName = this.getInterpretedTableName(); - resultSets.push({ - t: "InterpretedResultSet", - // FIXME: The values of version, columns, tupleCount are - // unused stubs because a InterpretedResultSet schema isn't used the - // same way as a RawResultSet. Probably should pull `name` field - // out. - schema: { - name: tableName, - rows: 1, - columns: [], - }, - name: tableName, - interpretation: this.props.interpretation, - }); - } - return resultSets; - } - - private getInterpretedTableName(): string { - return this.props.interpretation?.data.t === "GraphInterpretationData" - ? GRAPH_TABLE_NAME - : ALERTS_TABLE_NAME; - } - - private getResultSetNames(): string[] { - return this.props.interpretation - ? this.props.parsedResultSets.resultSetNames.concat([ - this.getInterpretedTableName(), - ]) - : this.props.parsedResultSets.resultSetNames; - } - constructor(props: ResultTablesProps) { super(props); const selectedTable = props.parsedResultSets.selectedTable || - getDefaultResultSet(this.getResultSets()); + getDefaultResultSet( + getResultSets(props.rawResultSets, props.interpretation), + ); const selectedPage = `${props.parsedResultSets.pageNumber + 1}`; this.state = { selectedTable, @@ -148,18 +156,23 @@ export class ResultTables extends React.Component< this.props.parsedResultSets.resultSetNames.some( (v) => this.state.selectedTable === v, ) || - this.getResultSets().some( + getResultSets(this.props.rawResultSets, this.props.interpretation).some( (v) => this.state.selectedTable === v.schema.name, ); // If the selected result set does not exist, select the default result set. if (!resultSetExists) { - const selectedTable = - this.props.parsedResultSets.selectedTable || - getDefaultResultSet(this.getResultSets()); - this.setState({ - selectedTable, - selectedPage: `${this.props.parsedResultSets.pageNumber + 1}`, + this.setState((state, props) => { + const selectedTable = + props.parsedResultSets.selectedTable || + getDefaultResultSet( + getResultSets(props.rawResultSets, props.interpretation), + ); + + return { + selectedTable, + selectedPage: `${props.parsedResultSets.pageNumber + 1}`, + }; }); } } @@ -328,8 +341,14 @@ export class ResultTables extends React.Component< render(): React.ReactNode { const { selectedTable } = this.state; - const resultSets = this.getResultSets(); - const resultSetNames = this.getResultSetNames(); + const resultSets = getResultSets( + this.props.rawResultSets, + this.props.interpretation, + ); + const resultSetNames = getResultSetNames( + this.props.interpretation, + this.props.parsedResultSets, + ); const resultSet = resultSets.find( (resultSet) => resultSet.schema.name === selectedTable, From 41e0dc2961607347005e51707e752ef490c0c684 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 11 Apr 2023 10:30:32 +0200 Subject: [PATCH 4/5] Remove unnecessary null check --- extensions/ql-vscode/src/view/results/result-tables.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/view/results/result-tables.tsx b/extensions/ql-vscode/src/view/results/result-tables.tsx index 9ae2e5613..ed3d5bae5 100644 --- a/extensions/ql-vscode/src/view/results/result-tables.tsx +++ b/extensions/ql-vscode/src/view/results/result-tables.tsx @@ -79,7 +79,7 @@ function renderResultCountString(resultSet: ResultSet): JSX.Element { } function getInterpretedTableName(interpretation: Interpretation): string { - return interpretation?.data.t === "GraphInterpretationData" + return interpretation.data.t === "GraphInterpretationData" ? GRAPH_TABLE_NAME : ALERTS_TABLE_NAME; } From 24c40af78f8a0c9e7651c84b59979488077f085b Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 11 Apr 2023 10:31:37 +0200 Subject: [PATCH 5/5] Add comment to @ts-ignore --- extensions/ql-vscode/src/view/results/result-tables.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/view/results/result-tables.tsx b/extensions/ql-vscode/src/view/results/result-tables.tsx index ed3d5bae5..24e60f1a0 100644 --- a/extensions/ql-vscode/src/view/results/result-tables.tsx +++ b/extensions/ql-vscode/src/view/results/result-tables.tsx @@ -101,7 +101,7 @@ function getResultSets( ): ResultSet[] { const resultSets: ResultSet[] = // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore 2783 + // @ts-ignore 2783 Avoid compilation error for overwriting the t property rawResultSets.map((rs) => ({ t: "RawResultSet", ...rs })); if (interpretation !== undefined) {