Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark progress bars as cancellable where it appears we are respecting the token #3517

Merged
merged 6 commits into from
Apr 3, 2024

Conversation

robertbrignull
Copy link
Contributor

This PR should fix all instances of https://github.com/github/vscode-codeql/security/code-scanning?query=is%3Aopen+branch%3Amain+rule%3Avscode-codeql%2Fprogress-not-cancellable

These are cases where the progress bar is not marked as cancellable, but we are using the token as if it were cancellable. Theoretically everything should just work if we say the progress bar is cancellable, and this gives the user more power to cancel long-running operations.

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.

@robertbrignull robertbrignull requested review from a team as code owners March 26, 2024 17:07
Copy link
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM.

Is it worth testing each of these actions to make sure they are truly cancellable (that the token passed is actually used appropriately)? Otherwise we could be giving people a cancel button that is broken.

Copy link
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure most of these should be cancellable. It would probably be better to remove the token instead.

extensions/ql-vscode/src/extension.ts Outdated Show resolved Hide resolved
extensions/ql-vscode/src/databases/local-databases-ui.ts Outdated Show resolved Hide resolved
@robertbrignull
Copy link
Contributor Author

Thanks for looking deeper into what the cancellation token actually does in these commands. I was originally hoping that because we pass it to the query server that it'll be used accordingly. I've now also dug into the code for the query server and I agree those three cases aren't actually as cancellable as they appear. Since we expect restarting the query server and trimming the cache to be pretty fast I agree we don't need to make them cancellable. It also isn't any change in behaviour from before this PR and is just cleaning up the code so it should be safe.

@robertbrignull
Copy link
Contributor Author

I've updated the PR to only make the model editor one cancellable and for the query server ones leave them as not cancellable and clean up the code instead.

I decided to remove the token argument from the query runner onStart since all of the uses of it passed a callback that didn't actually use the token anyway.

I've also quickly tested each of these cases manually just to make sure they work still.

Copy link
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@robertbrignull robertbrignull merged commit 7da3740 into main Apr 3, 2024
28 checks passed
@robertbrignull robertbrignull deleted the robertbrignull/fix-token-alerts branch April 3, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants