Allow users to close windows without confirming, delete last window #1598
Conversation
WalkthroughThe pull request introduces enhanced window management configuration options across multiple files in the project. The changes focus on adding two new settings: The modifications span several components of the application:
These changes provide users with more granular control over window management, specifically around closing windows with unsaved changes and preserving the last window's state. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/wconfig/defaultconfig/settings.json (1)
21-22: LGTM! Consider adding documentation.The default values for the new window settings are appropriate:
window:confirmonclose: true (safer default)window:savelastwindowonclose: true (better UX)Consider adding comments in the JSON to document these settings' purposes and their impact on user experience.
pkg/wconfig/settingsconfig.go (1)
109-110: Consider grouping with other window-related fields.While the implementation is correct, consider moving these fields to be grouped with other window-related fields (around line 90) for better code organization and maintainability.
WindowDisableHardwareAcceleration bool `json:"window:disablehardwareacceleration,omitempty"` WindowMaxTabCacheSize int `json:"window:maxtabcachesize,omitempty"` + WindowConfirmOnClose bool `json:"window:confirmonclose,omitempty"` + WindowSaveLastWindowOnClose bool `json:"window:savelastwindowonclose,omitempty"` WindowMagnifiedBlockOpacity *float64 `json:"window:magnifiedblockopacity,omitempty"` WindowMagnifiedBlockSize *float64 `json:"window:magnifiedblocksize,omitempty"` WindowMagnifiedBlockBlurPrimaryPx *int64 `json:"window:magnifiedblockblurprimarypx,omitempty"` WindowMagnifiedBlockBlurSecondaryPx *int64 `json:"window:magnifiedblockblursecondarypx,omitempty"` - WindowConfirmOnClose bool `json:"window:confirmonclose,omitempty"` - WindowSaveLastWindowOnClose bool `json:"window:savelastwindowonclose,omitempty"`emain/emain-window.ts (1)
Line range hint
232-252: Consider improving maintainability and user experience.
- Extract the confirmation logic into a separate method for better maintainability.
- Enhance the confirmation dialog message to be more descriptive.
- Structure debug logging consistently.
Here's a suggested refactor:
- const fullConfig = await FileService.GetFullConfig(); - if ( - fullConfig.settings["window:confirmonclose"] && - (numWindows > 1 || !fullConfig.settings["window:savelastwindowonclose"]) - ) { - console.log("numWindows > 1", numWindows); - const workspace = await WorkspaceService.GetWorkspace(this.workspaceId); - console.log("workspace", workspace); - if (isNonEmptyUnsavedWorkspace(workspace)) { - console.log("workspace has no name, icon, and multiple tabs", workspace); - const choice = dialog.showMessageBoxSync(this, { - type: "question", - buttons: ["Cancel", "Close Window"], - title: "Confirm", - message: "Window has unsaved tabs, closing window will delete existing tabs.\n\nContinue?", - }); - if (choice === 0) { - console.log("user cancelled close window", this.waveWindowId); - return; - } - } - console.log("deleteAllowed = true", this.waveWindowId); - this.deleteAllowed = true; - } + const fullConfig = await FileService.GetFullConfig(); + if (await this.shouldConfirmWindowClose(fullConfig, numWindows)) { + const workspace = await WorkspaceService.GetWorkspace(this.workspaceId); + if (await this.confirmCloseUnsavedWorkspace(workspace)) { + this.deleteAllowed = true; + } else { + return; + } + } + private async shouldConfirmWindowClose(fullConfig: FullConfigType, numWindows: number): Promise<boolean> { + return fullConfig.settings["window:confirmonclose"] && + (numWindows > 1 || !fullConfig.settings["window:savelastwindowonclose"]); + } + + private async confirmCloseUnsavedWorkspace(workspace: Workspace): Promise<boolean> { + if (!isNonEmptyUnsavedWorkspace(workspace)) { + return true; + } + + console.log("[Window] Confirming close of unsaved workspace", { + windowId: this.waveWindowId, + workspace + }); + + const choice = dialog.showMessageBoxSync(this, { + type: "question", + buttons: ["Cancel", "Close Window"], + title: "Confirm Close Window", + message: [ + "This window contains unsaved tabs that will be permanently deleted.", + "", + "• The workspace has not been saved", + "• Multiple tabs are open", + "", + "Would you like to close this window and delete all tabs?" + ].join("\n"), + defaultId: 0, + cancelId: 0 + }); + + const confirmed = choice === 1; + console.log("[Window] User response to close confirmation", { + windowId: this.waveWindowId, + confirmed + }); + + return confirmed; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emain/emain-window.ts(1 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/wconfig/defaultconfig/settings.json(2 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)
🔇 Additional comments (3)
pkg/wconfig/metaconsts.go (1)
82-83: LGTM! Constants are well-defined.
The new constants follow the established naming conventions and are properly grouped with other window-related constants.
frontend/types/gotypes.d.ts (1)
682-683: LGTM! Well-structured window management settings.
The new optional properties are well-organized within the window:* settings group and follow the established naming conventions.
emain/emain-window.ts (1)
232-236: LGTM! Well-implemented window close confirmation logic.
The implementation correctly handles the window close confirmation based on the new settings and workspace state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/docs/config.mdx (1)
83-83: Add a comma after "For reference".Minor grammatical improvement needed.
-For reference this is the current default configuration (v0.10.4): +For reference, this is the current default configuration (v0.10.4):🧰 Tools
🪛 LanguageTool
[uncategorized] ~83-~83: Possible missing comma found.
Context: ... | For reference this is the current default configurati...(AI_HYDRA_LEO_MISSING_COMMA)
emain/emain-window.ts (1)
234-240: Consider using structured logging for better debugging.The current implementation uses multiple
console.logstatements. Consider using structured logging for better debugging in production.- console.log("numWindows > 1 or user does not want last window saved", numWindows); - if (fullConfig.settings["window:confirmclose"]) { - console.log("confirmclose", this.waveWindowId); - const workspace = await WorkspaceService.GetWorkspace(this.workspaceId); - console.log("workspace", workspace); - if (isNonEmptyUnsavedWorkspace(workspace)) { - console.log("workspace has no name, icon, and multiple tabs", workspace); + log({ + event: "window_close_check", + numWindows, + windowId: this.waveWindowId, + confirmClose: fullConfig.settings["window:confirmclose"] + }); + if (fullConfig.settings["window:confirmclose"]) { + const workspace = await WorkspaceService.GetWorkspace(this.workspaceId); + if (isNonEmptyUnsavedWorkspace(workspace)) { + log({ + event: "unsaved_workspace_check", + windowId: this.waveWindowId, + workspaceId: workspace.oid, + numTabs: workspace.tabids?.length + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/docs/config.mdx(3 hunks)emain/emain-window.ts(1 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/wconfig/defaultconfig/settings.json(2 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/wconfig/defaultconfig/settings.json
- pkg/wconfig/metaconsts.go
- frontend/types/gotypes.d.ts
- pkg/wconfig/settingsconfig.go
🧰 Additional context used
🪛 LanguageTool
docs/docs/config.mdx
[uncategorized] ~83-~83: Possible missing comma found.
Context: ... | For reference this is the current default configurati...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (3)
docs/docs/config.mdx (2)
79-80: LGTM! Clear and well-documented configuration keys.
The new configuration keys are well-documented with clear descriptions of their functionality and default values.
106-107: LGTM! Default configuration values are appropriate.
The default values align with common UX patterns where users should be prompted before potentially destructive actions.
emain/emain-window.ts (1)
232-251: Verify edge case: Multiple windows with unsaved changes.
The window close logic is well-implemented with appropriate checks for configuration settings and unsaved changes. However, there's a potential edge case to consider.
Run the following script to check for similar window close confirmation implementations:
Consider enhancing the confirmation dialog message to show the number of unsaved tabs:
- "Window has unsaved tabs, closing window will delete existing tabs.\n\nContinue?",
+ `Window has ${workspace.tabids.length + workspace.pinnedtabids.length} unsaved tabs, closing window will delete existing tabs.\n\nContinue?`,✅ Verification successful
Window close confirmation is properly implemented with consistent behavior
The code correctly handles the edge case of multiple windows with unsaved changes:
- The
window:confirmclosesetting is properly defined and used across the codebase - The
isNonEmptyUnsavedWorkspacefunction has clear logic to identify unsaved workspaces (no name, no icon, and multiple tabs) - The confirmation dialog is shown only when necessary - when there are unsaved changes and the setting is enabled
- The implementation prevents accidental data loss by giving users a chance to cancel
The suggestion to show the number of unsaved tabs is not necessary since the current message clearly conveys the risk of data loss, which is the key information users need to make a decision.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent window close confirmation handling across the codebase.
# Search for window close confirmation dialogs
rg -A 5 'showMessageBoxSync.*Close Window'
# Search for workspace state checks
ast-grep --pattern 'isNonEmptyUnsavedWorkspace($workspace)'
Length of output: 106
Script:
#!/bin/bash
# Let's try a broader search for dialog-related code and workspace checks
# Search for dialog-related code
rg -A 5 "dialog\.showMessageBox"
# Search for workspace-related checks in a different way
rg -A 5 "isNonEmptyUnsavedWorkspace"
# Search for window close handling
rg -A 5 "window.*close"
Length of output: 121328
Script:
#!/bin/bash
# Let's try a more focused search for window close and workspace handling
# Search for window close confirmation implementation
rg -A 5 "window:confirmclose"
# Search for workspace state checks with context
rg -A 10 "isNonEmptyUnsavedWorkspace" --no-filename
# Search for workspace-related functions
ast-grep --pattern 'GetWorkspace($workspaceId)'
Length of output: 4247
Adds
window:savelastwindowandwindow:confirmclosesettings, which can be used to alter the default window close behavior. Both of these default totrueto maintain the existing default behavior.If
window:savelastwindowis unset, the same logic will be used as if the user had more than one window remaining (see below).If
window:confirmoncloseis unset, the user will no longer be prompted to confirm if they are closing a window whose workspace has unsaved changes (the workspace is not named and it has more than one tab).