CodeQL model editor: user should be able to stop modeling before the main editor view is open

[norascheuch]

Opening the model editor takes a while. This PR gives the user the option to cancel the progress and not open the editor.

When cancelling the open process the user effectively cancels a query run. This results in an error message and a telemetry event.

From my investigation I deduct that this behaviour stems from the query server in https://github.com/github/vscode-codeql/blob/nora/make-model-editor-open-cancellable/extensions/ql-vscode/src/query-server/run-queries.ts#L69

I would be happy for feedback on how to resolve this issue.

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.

[shati-patel]

Thanks! This looks mostly fine to me, just had a poke at the error/telemetry issue you mentioned 🔍

(I wouldn't mind another review though—I'm still not that confident with the model editor code 🙈)

[shati-patel]

I think the error + telemetry event is coming from here (since we catch the UserCancellationException and treat it like a normal error). Maybe we can check for the type of error here?
Something like:

Suggested change

	} catch (err) {
	void showAndLogExceptionWithTelemetry(
	} catch (err) {
	if (err instanceof UserCancellationException) {
	this.panel?.dispose();
	return;
	}
	void showAndLogExceptionWithTelemetry(

[robertbrignull]

I agree that this is where the log message is coming from, so we have the power to silence it. Annoyingly, the error isn't actually a UserCancellationException :shakes_fist:. As far as I can tell the error is coming from a rpc library and not our code. We may need to add a custom check for this type of error, like if the message matches The request \(.*\) has been cancelled.

Adding in this.panel?.dispose() is also good because otherwise the panel lingers open in the "loading" state and you need to close it manually.

So maybe something like

if (
  getErrorMessage(err).match(/The request \(.*\) has been cancelled/i)
) {
  this.panel?.dispose();
  return;
}

[norascheuch]

This looks really good! Yes, the catch is catching this error, I was just not sure if there are other instances where we might get a The request \(.*\) has been cancelled that was not due to a user cancellation...

[shati-patel]

Annoyingly, the error isn't actually a UserCancellationException

Is it not? 😅 That if snippet worked when I tested locally (though maybe I didn't test all cases 🤔)

[norascheuch]

The if works, but it could be imaginable that something went wrong internally when running the query and an error message with the same text is produced, but we now swallow it.

[robertbrignull]

That if snippet worked when I tested locally

It didn't work for me 🤔 . I also tried adding a breakpoint there so I could poke at the error manually. I'm very happy to try again if you disagree.

it could be imaginable that something went wrong internally when running the query and an error message with the same text is produced, but we now swallow it.

Maybe but I'd expect the message wouldn't say "cancelled" then. How about we still log it but not show the popup? Keeping telemetry could be nice so we know how many people are cancelling this. We could do something like

vscode-codeql/extensions/ql-vscode/src/databases/github-databases/github-databases-module.ts

Lines 66 to 71 in d0e8112

	const error = redactableError(
	asError(e),
	)`Failed to prompt for GitHub repository download`;
	void this.app.logger.log(error.fullMessageWithStack);
	this.app.telemetry?.sendError(error);

[shati-patel]

The if works, but it could be imaginable that something went wrong internally when running the query and an error message with the same text is produced, but we now swallow it.

Sorry, I meant that my original suggestion of if (err instanceof UserCancellationException) worked fine for me! I was wondering what Robert meant by saying the error wasn't a UserCancellationException 🤔

[norascheuch]

ah! It depends on when you press the cancel button! There are two progress windows and for the first everything works, for the second it doesn't. So after the dependencies are installed and the query is actually run, we need the text match.

[koesie10]

Something which might be less hacky is the following, which checks the type of the other error and that it is a cancellation error:

import { ResponseError } from "vscode-jsonrpc";
import { LSPErrorCodes } from "vscode-languageclient";

// ...

      if (
        err instanceof ResponseError &&
        err.code === LSPErrorCodes.RequestCancelled
      ) {
        this.panel?.dispose();
        return;
      }

[robertbrignull]

Code LGTM (apart from the point that @shati-patel mentioned).

What you've done (i.e. checking the token in between each expensive operation) is exactly the right thing to do.

[robertbrignull]

I agree that this is where the log message is coming from, so we have the power to silence it. Annoyingly, the error isn't actually a UserCancellationException :shakes_fist:. As far as I can tell the error is coming from a rpc library and not our code. We may need to add a custom check for this type of error, like if the message matches The request \(.*\) has been cancelled.

Adding in this.panel?.dispose() is also good because otherwise the panel lingers open in the "loading" state and you need to close it manually.

So maybe something like

if (
  getErrorMessage(err).match(/The request \(.*\) has been cancelled/i)
) {
  this.panel?.dispose();
  return;
}

[norascheuch]

If we're confident no other non-user triggered The request \(.*\) has been cancelled error message can occur this should be good to go.

[robertbrignull]

🎉