-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: prevent UI freeze when showing save dialogs (#6870) #6871
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import * as vscode from "vscode" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new utility module should have unit tests to verify the non-blocking behavior and error handling. Could we add tests to ensure:
|
||
|
|
||
| /** | ||
| * Wraps vscode.window.showSaveDialog in a non-blocking way to prevent UI freezing | ||
| * This addresses the issue where the save dialog can cause the entire VSCode UI to freeze | ||
| * on certain systems (particularly macOS with specific configurations). | ||
| * | ||
| * @param options - The save dialog options | ||
| * @returns Promise that resolves to the selected URI or undefined if cancelled | ||
| */ | ||
| export async function showSaveDialogSafe(options: vscode.SaveDialogOptions): Promise<vscode.Uri | undefined> { | ||
| // Use setImmediate to defer the dialog call to the next iteration of the event loop | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be helpful to add a comment explaining why setImmediate specifically prevents the UI freeze? This would help future maintainers understand the mechanism behind this fix. For example: 'setImmediate schedules the callback to execute after the current event loop completes, allowing the UI thread to process pending events and remain responsive.' |
||
| // This prevents the UI thread from being blocked | ||
| return new Promise<vscode.Uri | undefined>((resolve) => { | ||
| setImmediate(async () => { | ||
| try { | ||
| const result = await vscode.window.showSaveDialog(options) | ||
| resolve(result) | ||
| } catch (error) { | ||
| console.error("Error showing save dialog:", error) | ||
| resolve(undefined) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is resolving to undefined on error intentional? The error is logged, but callers can't distinguish between user cancellation and an actual error. Would it be better to re-throw the error after logging, or is this defensive behavior by design? |
||
| } | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Wraps vscode.window.showOpenDialog in a non-blocking way to prevent UI freezing | ||
| * | ||
| * @param options - The open dialog options | ||
| * @returns Promise that resolves to the selected URIs or undefined if cancelled | ||
| */ | ||
| export async function showOpenDialogSafe(options: vscode.OpenDialogOptions): Promise<vscode.Uri[] | undefined> { | ||
| // Use setImmediate to defer the dialog call to the next iteration of the event loop | ||
| return new Promise<vscode.Uri[] | undefined>((resolve) => { | ||
| setImmediate(async () => { | ||
| try { | ||
| const result = await vscode.window.showOpenDialog(options) | ||
| resolve(result) | ||
| } catch (error) { | ||
| console.error("Error showing open dialog:", error) | ||
| resolve(undefined) | ||
| } | ||
| }) | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test file src/core/config/tests/importExport.spec.ts still mocks vscode.window.showOpenDialog and vscode.window.showSaveDialog directly. Should the tests be updated to mock the new safe wrapper functions instead?