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

Do not * activate the GHPRI extension #4051

Closed
wants to merge 1 commit into from

Conversation

joyceerhl
Copy link
Contributor

@joyceerhl joyceerhl commented Oct 13, 2022

Part of #4046

It seems like the original intent of adding the * activation event was to improve the perceived speed of loading the PR diff when the current workspace is a PR, but this has the side effect of activating GHPRI even if the user doesn't have a PR checked out. Additionally, when testing this change with sideloaded GHPRI running out of sources in insiders.vscode.dev, it did not seem like the PR diff took much longer than usual to load.

This change seems safe to me and worthwhile to test out in insiders because if we can avoid unnecessary extension activation, we can reduce network and web worker extension host contention and hopefully improve overall performance of vscode.dev. Please let me know if I've missed some context!

@joyceerhl joyceerhl enabled auto-merge (squash) October 13, 2022 18:32
@joyceerhl joyceerhl requested a review from alexr00 October 13, 2022 18:33
@@ -30,7 +30,6 @@
"vscode.github-authentication"
],
"activationEvents": [
"*",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Realized that this PR as it stands isn't enough, because GHPRI sets a lot of context keys on activation which hide commands, and all of those context keys probably need to be revisited to ensure that functionality isn't improperly hidden. @alexr00 maybe we can chat about this further when you are in next week?

@joyceerhl joyceerhl closed this Oct 21, 2022
auto-merge was automatically disabled October 21, 2022 16:04

Pull request was closed

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.

1 participant