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

Avoid awaiting VS Code popup notifications #557

Merged
merged 1 commit into from
Jul 24, 2024
Merged

Conversation

dhruvmanila
Copy link
Member

Summary

This PR fixes a bug to avoid awaiting the VS Code popup notifications.

How to reproduce this?

For posterity, this is how to reproduce this bug:

  1. Keep the extension setting to only contain:
{
	"ruff.nativeServer": true
}
  1. Add the following additional setting which is incompatible with the native server:
{
	"ruff.nativeServer": true,
	"ruff.format.args": ["--line-length=10"]
}
  1. Saving the new settings will trigger a restart and a notification will popup which warns you about this incompatible setting (Do NOT dismiss the notification window)
  2. Remove the ruff.format.args and save the settings which should then trigger a restart again

But, the restart triggered by (3) never completed because the notification was awaited and the user never dismissed it. This is where the extension hangs.

Solution

I've updated all vscode.window.show(Error|Warning)Message calls to not be awaited and instead use the callback mechanism if there's a need to respond to user's selection.

Test Plan

Follow the same steps as above and make sure that the restart happens.

Screen.Recording.2024-07-24.at.17.14.56.mov

Comment on lines +250 to +254
vscode.window.showWarningMessage(message, "Show Logs").then((selection) => {
if (selection) {
outputChannel.show();
}
});
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe add an error handler that logs a message if showing the message box fails for whatever reason

Suggested change
vscode.window.showWarningMessage(message, "Show Logs").then((selection) => {
if (selection) {
outputChannel.show();
}
});
vscode.window.showWarningMessage(message, "Show Logs").then((selection) => {
if (selection) {
outputChannel.show();
}
}).catch(ex => logError("Failed to show warning message: {ex}"));

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do it but there are multiple instances of these calls 🤷‍♂️

@dhruvmanila dhruvmanila merged commit 4060b01 into main Jul 24, 2024
6 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/no-await branch July 24, 2024 13:00
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.

2 participants