feat: implement window bounds persistence #7388
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements window bounds persistence to save and restore the application window's position, size, and maximized state across application restarts, providing a consistent user experience.
- Adds window bounds persistence functionality with debounced saving to avoid frequent disk writes
- Integrates window bounds validation to ensure windows remain visible on screen
- Updates preferences schema to store window bounds configuration
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/compass/src/main/window-manager.ts | Implements window bounds persistence logic, validation, and event handlers for saving/restoring window state |
| packages/compass-preferences-model/src/preferences-schema.tsx | Adds windowBounds preference schema with validation for storing window position and state |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const windowOpts = { | ||
| width: Number(DEFAULT_WIDTH), | ||
| height: Number(DEFAULT_HEIGHT), | ||
| ...validatedBounds, | ||
| minWidth: Number(MIN_WIDTH), | ||
| minHeight: Number(MIN_HEIGHT), |
There was a problem hiding this comment.
The spread operator ...validatedBounds may override the minWidth and minHeight properties if validatedBounds contains width and height properties with smaller values than the minimums. The spread should come after the min properties to ensure minimums are enforced.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
...validatedBounds has been removed.
Additionally, since the ...bounds return value does not include minWidth and minHeight, they are not overwritten.
| // Ensure minimum size | ||
| const width = Math.max(bounds.width, Number(MIN_WIDTH)); | ||
| const height = Math.max(bounds.height, Number(MIN_HEIGHT)); |
There was a problem hiding this comment.
[nitpick] The Math.max calls enforce minimum dimensions but this validation is duplicated with the minWidth/minHeight properties set on the BrowserWindow. Consider removing this duplication since Electron will enforce the minimums automatically.
| // Ensure minimum size | |
| const width = Math.max(bounds.width, Number(MIN_WIDTH)); | |
| const height = Math.max(bounds.height, Number(MIN_HEIGHT)); | |
| // Use provided size; Electron will enforce minimums | |
| const width = bounds.width; | |
| const height = bounds.height; |
Copilot uses AI. Check for mistakes.
| if (savedBounds?.isMaximized) { | ||
| window.maximize(); | ||
| } |
There was a problem hiding this comment.
[nitpick] The maximized state restoration should happen after the window is shown to avoid potential visual glitches. Consider moving this logic after the showWindowWhenReady(window) call or inside the showWindowWhenReady function.
Copilot uses AI. Check for mistakes.
…fy window bounds retrieval
…nces for window bounds
miya
changed the title
| const windowOpts = { | ||
| width: Number(DEFAULT_WIDTH), | ||
| height: Number(DEFAULT_HEIGHT), | ||
| ...bounds, |
There was a problem hiding this comment.
Does electron gracefully handle if a user tries to open a window that's larger than their screen size or out of bounds of their current screen? I'm wondering if we need to account for situations where a user goes from a larger monitor to a smaller one.
Description
This PR adds the ability to persist window position, size, and maximized state across application restarts, providing a consistent user experience by restoring the user's preferred window configuration.
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes