Setup Storybook for testing UI components

[koesie10]

This sets up Storybook for testing of React components. It adds stories for some of the MRVA components. It does not add stories for the main MRVA views since those are not independent of VSCode and need to be run from within VSCode.

Screen.Recording.2022-09-06.at.10.59.49.mov

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[charisk]

It does not add stories for the main MRVA views since those are not independent of VSCode and need to be run from within VSCode

Do you think it's possible to fake out being inside VS Code? From a very simplistic point of view, the webviews are "just" web pages rendered inside VS Code. I appreciate that reality is much more nuanced, but I was hoping we'd look into potentially setting up the whatever we need within Storybook to pretend we're inside the VS Code extension. I don't think we'll have many components that are completely independent of VS Code unfortunately.

[koesie10]

It does not add stories for the main MRVA views since those are not independent of VSCode and need to be run from within VSCode

Do you think it's possible to fake out being inside VS Code? From a very simplistic point of view, the webviews are "just" web pages rendered inside VS Code. I appreciate that reality is much more nuanced, but I was hoping we'd look into potentially setting up the whatever we need within Storybook to pretend we're inside the VS Code extension. I don't think we'll have many components that are completely independent of VS Code unfortunately.

We might be able to simulate some of VSCode's environment, or at least mock the function calls so that they are not undefined. However, I think this should not be necessary. For example, the RemoteQueries component is currently dependent on data received from VSCode:

vscode-codeql/extensions/ql-vscode/src/view/remote-queries/RemoteQueries.tsx

Lines 394 to 444 in da90651

	export function RemoteQueries(): JSX.Element {
	const [queryResult, setQueryResult] = useState<RemoteQueryResult>(emptyQueryResult);
	const [analysesResults, setAnalysesResults] = useState<AnalysisResults[]>([]);
	const [sort, setSort] = useState<Sort>('name');
	useEffect(() => {
	window.addEventListener('message', (evt: MessageEvent) => {
	if (evt.origin === window.origin) {
	const msg: ToRemoteQueriesMessage = evt.data;
	if (msg.t === 'setRemoteQueryResult') {
	setQueryResult(msg.queryResult);
	} else if (msg.t === 'setAnalysesResults') {
	setAnalysesResults(msg.analysesResults);
	}
	} else {
	// sanitize origin
	const origin = evt.origin.replace(/\n|\r/g, '');
	console.error(`Invalid event origin ${origin}`);
	}
	});
	});
	if (!queryResult) {
	return <div>Waiting for results to load.</div>;
	}
	try {
	return (
	<div className="vscode-codeql__remote-queries">
	<ThemeProvider colorMode="auto">
	<ViewTitle>{queryResult.queryTitle}</ViewTitle>
	<QueryInfo {...queryResult} />
	<Failures {...queryResult} />
	<Summary
	queryResult={queryResult}
	analysesResults={analysesResults}
	sort={sort}
	setSort={setSort} />
	<AnalysesResults
	queryResult={queryResult}
	analysesResults={analysesResults}
	totalResults={queryResult.totalResultCount}
	sort={sort} />
	</ThemeProvider>
	</div>
	);
	} catch (err) {
	console.error(err);
	return <div>There was an error displaying the view.</div>;
	}
	}

Rather than simulating sending a message to the window, I think we should change the components to have a very clear separation between UI and the VSCode communication. So, this component would take a queryResult and analysesResults prop. Then the only dependency on VSCode is when you click a button, for example:

vscode-codeql/extensions/ql-vscode/src/view/remote-queries/RemoteQueries.tsx

Lines 55 to 60 in da90651

	const downloadAllAnalysesResults = (query: RemoteQueryResult) => {
	vscode.postMessage({
	t: 'remoteQueryDownloadAllAnalysesResults',
	analysisSummaries: query.analysisSummaries
	});
	};

We can probably simulate this to be an action to Storybook instead. However, that still means we won't really be able to test this flow:

1. Click on "Download all" button
2. The spinner shows
3. When it is finished, the button disappears

I don't think this is necessarily a problem since we can make different stories with the state of the UI in these three states, but it is something to be aware of.

[charisk]

We might be able to simulate some of VSCode's environment, or at least mock the function calls so that they are not undefined. However, I think this should not be necessary.

I agree. We could design our components to be not directly use VS Code function calls. I was thinking more in terms of styling etc rather than interactivity e.g. so that we're able to use VS Code colour variables etc.

[koesie10]

We might be able to simulate some of VSCode's environment, or at least mock the function calls so that they are not undefined. However, I think this should not be necessary.

I agree. We could design our components to be not directly use VS Code function calls. I was thinking more in terms of styling etc rather than interactivity e.g. so that we're able to use VS Code colour variables etc.

VSCode colour variables do completely work (see https://github.com/github/vscode-codeql/pull/1497/files#diff-9b6fe650c4162362634b90f6d2fec716787e077e6db1febcd621986c5633155e), it's just the JavaScript window/vscode objects which do not match/do not exist which are normally inserted by VSCode.

[koesie10]

I've now added a mock of the VSCode API available in the webview, so we can now also render views which depend on the VSCode API:

[charisk]

Thank you for looking into this and getting all the bits working! It looks great and it will really improve our DX while building our new components and views!

[charisk]

Instead of having our own file that we have to manage/keep up-to date etc., is there a way to get everything from VSCode automatically?

[koesie10]

We might be able to generate it from the VSCode source code, but I don't think we can import it from VSCode itself.

The colors are defined here and the styles are created here. They are then injected into the WebView using this on the host side and this on the WebView side.

I think it will be a lot more work to generate the CSS variables from this than to copy the variables from VSCode once in a while.

[charisk]

Ah that's a shame. I suspected it would be a can of worms, but I thought I'd ask anyway 😂

In that case I think we should have some docs to tell people about this (maybe in the storybook part in the README?) and how to update them.

[koesie10]

Perhaps I can also add it to an Overview page in Storybook itself and refer to that from the CONTRIBUTING documentation?

[charisk]

SGTM!

[charisk]

I just had a brainwave! Feel free to ignore this if you want (totally up to you), but the actual toolkit has a storybook, so perhaps this is something they've solved already? See https://github.com/microsoft/vscode-webview-ui-toolkit/tree/main/.storybook

[koesie10]

I did see whether it was possible to replicate the way they are using the VSCode variables, but they are using a completely custom system. They have a system of "design tokens" (https://github.com/microsoft/vscode-webview-ui-toolkit/blob/6fc339ce26e7da15b9ba97c063f56d8a4eae85ad/src/design-tokens.ts) which specify a variable and a default. These are then used in the styles and will automatically use either the variable or the default. I don't think that is going to work for us since we'd need to modify the way we write the CSS files and set defaults for all variables we use. It could also result in having different defaults than the toolkit, so it might also get out of sync at some point.

[charisk]

I'm approving and leaving the last 2 unresolved comments up to you to look at or ignore!

Again, thanks for sorting this!

[koesie10]

The failing Dependency Review is due to dependencies of Storybook and is only used in dev environments. It would be quite hard to fix this since the dependencies are transitive dependencies of Storybook. Since they won't be used in production code, I think it should be fine to merge this.

[charisk]

The failing Dependency Review is due to dependencies of Storybook and is only used in dev environments. It would be quite hard to fix this since the dependencies are transitive dependencies of Storybook. Since they won't be used in production code, I think it should be fine to merge this.

Does this mean that all future PRs will have a failing check?

[koesie10]

The failing Dependency Review is due to dependencies of Storybook and is only used in dev environments. It would be quite hard to fix this since the dependencies are transitive dependencies of Storybook. Since they won't be used in production code, I think it should be fine to merge this.

Does this mean that all future PRs will have a failing check?

No, the check only checks new dependencies, so future PRs will not fail this check. We already have some open Dependabot alerts for other development dependencies and those do not fail any PRs either.