Skip to content

feat(window): persist main window size/position across launches - #97

Merged
byrongamatos merged 2 commits into
mainfrom
feat/persist-window-bounds
Jul 11, 2026
Merged

byrongamatos merged 2 commits into
mainfrom
feat/persist-window-bounds

Conversation

@gionnibgud

@gionnibgud gionnibgud commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Why

The main window always opens at a fixed 1400×900, forcing a manual resize every session. This persists the window geometry so the app reopens the way you left it.

What

  • Save on close: getNormalBounds() + isMaximized() written to the existing desktop prefs store (slopsmith-desktop.json, atomic write, fail-soft) — no new dependency.
  • Restore in createWindow(): saved bounds pass through a pure sanitizer (src/main/window-bounds.ts) validated against the current display layout before use.

Guardrails

Stale state Behavior
Garbage / partial / hand-edited config 1400×900 centered defaults
Size out of range clamped between 800×600 minimums and largest display workArea
Position off-screen (unplugged monitor, resolution change) position dropped → centered; size kept. Overlap test (≥100×50 px on some display), so negative multi-monitor coordinates stay valid
Closed while maximized normal bounds saved, re-maximized on restore
Closed while fullscreen restores windowed (deliberate — launching into fullscreen is jarring)
Reset app settings store file already in the delete-set → bounds reset too

Testing

  • 9 new unit tests for the sanitizer (tests/window-bounds.test.js, node --test, no electron import).
  • Verified end-to-end on macOS: clean-launch → resize → relaunch round-trip; off-screen seed re-centers; garbage seed falls back to defaults; maximized session restores maximized and preserves the windowed size underneath.
  • npm run typecheck clean; full suite passes (the only failure on a clean checkout is the pre-existing JUCE-submodule-dependent test).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Main window size, position, and maximized state are now remembered between launches.
    • Restores window placement using sanitized, display-aware bounds to better fit the current monitor setup.
    • Enforces minimum window dimensions on restore.
  • Bug Fixes
    • Prevents windows from reopening partially or entirely off-screen after monitor changes.
    • Discards unreliable saved positions when the window would barely be visible.
  • Tests
    • Added coverage for invalid, oversized, undersized, fractional, and multi-monitor window configurations.

The main window always opened at a fixed 1400x900, forcing a manual
resize every session. Save the window geometry (normal bounds +
maximized flag) to the existing desktop prefs store on close, and
restore it in createWindow.

Saved bounds are validated by a pure sanitizer against the current
display layout before use, so stale state degrades safely instead of
producing an off-screen or absurd window:
- garbage/partial config -> 1400x900 centered defaults
- size clamped between the 800x600 window minimums and the largest
  display's workArea
- position kept only when the window overlaps a display by at least
  100x50 px (unplugged monitor / resolution change -> re-center);
  negative multi-monitor coordinates remain valid
- maximized sessions save getNormalBounds() and re-maximize on
  restore; fullscreen deliberately restores windowed

No new dependency; reuses get/setDesktopConfig (atomic write,
fail-soft) in soundfont-manager.ts. The store file is already in the
reset-app-settings delete-set, so a config reset also resets bounds.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: gionnibgud <gionnibgud@gmail.com>
@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 24dbf677-ccf9-4833-8c53-1623f96acf44

📥 Commits

Reviewing files that changed from the base of the PR and between c9c345d and d2a763d.

📒 Files selected for processing (1)
  • src/main/main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/main.ts

📝 Walkthrough

Walkthrough

Window geometry is now persisted in desktop configuration, sanitized against current display work areas, restored during Electron window creation, constrained by minimum dimensions, and saved on close with maximized state.

Changes

Window bounds persistence

Layer / File(s) Summary
Window bounds validation and coverage
src/main/window-bounds.ts, tests/window-bounds.test.js
Adds geometry types, defaults, sanitization against display work areas, size clamping, visibility checks, coordinate handling, and automated coverage.
Window lifecycle integration
src/main/main.ts, src/main/soundfont-manager.ts
Restores sanitized bounds during window creation and persists normal bounds plus maximized state on close through desktop configuration.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DesktopConfig
  participant ElectronScreen
  participant sanitizeWindowBounds
  participant MainWindow
  DesktopConfig->>sanitizeWindowBounds: provide saved bounds
  ElectronScreen->>sanitizeWindowBounds: provide current display work areas
  sanitizeWindowBounds->>MainWindow: return validated window geometry
  MainWindow->>DesktopConfig: save normal bounds and maximized state on close
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: persisting the main window size and position across launches.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/persist-window-bounds

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/main/main.ts`:
- Around line 486-491: Wrap the setDesktopConfig call in the mainWindow close
handler with try/catch so synchronous configuration write or rename failures do
not escape the listener during shutdown. Preserve the existing destroyed-window
guard and normal window-bounds persistence, and handle the caught error using
the surrounding module’s established error-reporting approach.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8bd7026d-afdf-48b9-a144-b8af1bee6f71

📥 Commits

Reviewing files that changed from the base of the PR and between f66c27b and c9c345d.

📒 Files selected for processing (4)
  • src/main/main.ts
  • src/main/soundfont-manager.ts
  • src/main/window-bounds.ts
  • tests/window-bounds.test.js

Comment thread src/main/main.ts
The close listener called setDesktopConfig synchronously with no error
handling; a disk-full or permissions failure during the write would throw
unhandled inside the close handler, risking a shutdown crash. Wrap the
write in try/catch and log a warning instead. Flagged by CodeRabbit on PR #97.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: gionnibgud <gionnibgud@gmail.com>
@byrongamatos
byrongamatos merged commit 6349ed4 into main Jul 11, 2026
5 checks passed
@byrongamatos
byrongamatos deleted the feat/persist-window-bounds branch July 11, 2026 13:41
@shineuponthee

Copy link
Copy Markdown

Is there an option to start fullscreen, with no menu at the top? It's not jarring, it's annoying to have to manually fullscreen every time.

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.

3 participants