feat(desktop): persist window size and position across app restarts#550
feat(desktop): persist window size and position across app restarts#550Kitenite merged 2 commits intosuperset-sh:mainfrom
Conversation
📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This is gonna help a ton with testing as well. Thanks @andreasasprou ! |
![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 (1)
apps/desktop/src/main/lib/window-state/bounds-validation.test.ts (1)
8-9: Consider importing constants from implementation.The magic numbers
MIN_VISIBLE_OVERLAPandMIN_WINDOW_SIZEare duplicated from the implementation. If these values change in the source, tests could pass with incorrect assumptions.🔎 Proposed refactor to import constants
If the implementation exports these constants (or can be refactored to do so), import them instead:
+import { + getInitialWindowBounds, + isVisibleOnAnyDisplay, + MIN_VISIBLE_OVERLAP, + MIN_WINDOW_SIZE, +} from "./bounds-validation"; - -const MIN_VISIBLE_OVERLAP = 50; -const MIN_WINDOW_SIZE = 400;Otherwise, add a comment documenting the source of these values:
+// Must match constants in bounds-validation.ts const MIN_VISIBLE_OVERLAP = 50; const MIN_WINDOW_SIZE = 400;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/main/lib/window-state/bounds-validation.test.tsapps/desktop/src/main/lib/window-state/index.tsapps/desktop/src/main/lib/window-state/window-state.test.tsapps/desktop/src/main/lib/window-state/window-state.tsapps/desktop/test-setup.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/lib/window-state/index.ts
- apps/desktop/src/main/lib/window-state/window-state.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/window-state/window-state.test.tsapps/desktop/test-setup.tsapps/desktop/src/main/lib/window-state/bounds-validation.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/main/lib/window-state/window-state.test.tsapps/desktop/test-setup.tsapps/desktop/src/main/lib/window-state/bounds-validation.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate tests with implementation files using .test.ts or .test.tsx suffix
Files:
apps/desktop/src/main/lib/window-state/window-state.test.tsapps/desktop/src/main/lib/window-state/bounds-validation.test.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/main/lib/window-state/window-state.test.tsapps/desktop/test-setup.tsapps/desktop/src/main/lib/window-state/bounds-validation.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/main/lib/window-state/window-state.test.tsapps/desktop/test-setup.tsapps/desktop/src/main/lib/window-state/bounds-validation.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (4)
apps/desktop/test-setup.ts (1)
102-109: LGTM! Mock setup supports window-state testing.The extended screen mock provides the necessary display bounds for window visibility and positioning tests. The single-display configuration is appropriate as a default—individual tests can override these mocks for multi-monitor scenarios.
apps/desktop/src/main/lib/window-state/window-state.test.ts (1)
1-288: LGTM! Comprehensive type guard test coverage.The test suite thoroughly exercises
isValidWindowStateacross valid inputs, boundary conditions, type errors, and edge cases. The inclusion of forward-compatibility testing (line 66-77) and extreme values (MAX_SAFE_INTEGER at line 79-89) demonstrates defensive thinking.apps/desktop/src/main/lib/window-state/bounds-validation.test.ts (2)
11-180: LGTM! Thorough visibility logic testing.The test suite for
isVisibleOnAnyDisplaycovers single/multi-display configurations, edge boundaries (line 53-62), insufficient overlap (line 88-97), negative coordinates, display gaps, and edge cases including no displays (line 164-169). The boundary testing at exactlyMIN_VISIBLE_OVERLAPdemonstrates careful validation of strict inequality.
182-377: LGTM! Comprehensive initial bounds calculation testing.The test suite for
getInitialWindowBoundscovers:
- Centering behavior when no saved state or display disconnected
- Exact position restoration when visible
- Dimension clamping to work area (lines 274-318)
- Edge case where work area < MIN_WINDOW_SIZE (lines 338-353)
- isMaximized state preservation across scenarios
- Multi-monitor positioning
The coverage is thorough and demonstrates proper handling of real-world scenarios like resolution changes and display disconnection.
Why I did it in the first place 😄 @Kitenite |
2 participants
Summary
This PR adds window size and position persistence so the app remembers its bounds across restarts.
Details
Architecture
~/.superset/window-state.json, separate from UI state (tabs/theme) inapp-state.jsonWhy separate from app-state?
Different ownership, lifecycle, and access patterns → separate persistence mechanisms. This also avoids concurrent write issues with LowDB.
How it works
window-state.json→ validate bounds visible on connected displays → restore position/sizeDesign Principles
Number.isFinite()validationapp-state/MainWindow()calls 3 helper functions, all logic in dedicated moduleManual Test Checklist
Happy path
https://www.loom.com/share/534ed26be7ba43edac81557415a34d6a
Core Functionality
Multi-Monitor
Edge Cases
~/.superset/window-state.json→ reopen → falls back to defaultsFiles Changed
New Files
apps/desktop/src/main/lib/window-state/window-state.ts— Load/save with atomic writes, validationapps/desktop/src/main/lib/window-state/bounds-validation.ts— Multi-monitor validation, bounds calculationapps/desktop/src/main/lib/window-state/index.ts— Barrel exportModified Files
apps/desktop/src/main/lib/app-environment.ts— AddedWINDOW_STATE_PATHconstantapps/desktop/src/main/windows/main.ts— Integrated window state persistenceSummary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
✏️ Tip: You can customize this high-level summary in your review settings.