feat(macos): persist + restore main window geometry across launches#32449
Conversation
`createWindow()` now spreads `restoreBounds("main", ...)` into the
BrowserWindow constructor and registers `trackWindowState("main", win)`
so the next launch comes up where the user left it.
Geometry lives in its own `electron-store` instance (`window-state.json`
under userData) rather than the renderer-facing settings store — which
keeps the strict-schema'd settings file clean of main-process-only data
and avoids exposing window position through the contextBridge to a
renderer that has no business reading or writing it. Updated
`settings.ts`'s docstring to point at the new module.
The persistence shape (~80 lines in `window-state.ts`):
- `restoreBounds(key, defaults)` — falls through to defaults on first
run. When state exists, clamps it into the closest still-connected
display's `workArea` via `screen.getDisplayMatching`, so unplugging
the external monitor since last launch doesn't strand the window
off-screen.
- `track(key, win)` — saves on `close` synchronously (normal exit) and
on `resize`/`move` with a 500ms debounce (crash recovery — covers
the case where `close` never fires). Reads `getNormalBounds()` so
un-fullscreening on the next launch doesn't reveal a 1×1 sliver,
and tracks `isFullScreen()` separately as a flag passed back to the
BrowserWindow constructor.
Designed for reuse: future thread pop-outs, the About window, and the
onboarding window can all `track("thread.<id>", win)` /
`track("about", win)` / etc. against the same store without
clobbering each other.
Default size (1280×800) kept unchanged — moving to Swift's 1200×900
target lives in LUM-1965 alongside the tray icon work.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Devin review caught a real bug. `getNormalBounds()` per Electron's docs returns the position and size of the window "in normal state" — regardless of minimized / maximized / fullscreen. The extra `isMinimized()` guard wasn't protecting anything (the bounds are already correct), and it was actively dropping state in two scenarios: 1. **Quit while minimized.** Common macOS flow: minimize to dock, later Cmd+Q. The `close` handler called `persist()` which then skipped saving because the window was still minimized — losing whatever position the user moved to since the last debounced save. 2. **Resize/move within 500ms of minimize.** Debounced timer fires while minimized, persist skips, latest geometry never saved. Switched the guard to just `isDestroyed()` (the only condition where the bounds API would actually throw) and updated the docstring to call out the `getNormalBounds()`-handles-minimized-correctly point so the next reader doesn't re-introduce the check.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE — reviewed at 3782a385
Solid, self-contained persistence layer. The architecture decision (separate electron-store instance, main-process only, key-namespaced for future windows) is well-reasoned and matches the codebase's discipline in settings.ts. The off-screen clamping math is correct, Devin's isMinimized() finding was correctly fixed at HEAD, and the show: false + ready-to-show pattern ensures the window doesn't flash at the wrong geometry on restore.
Full analysis
Singleton store — lazy init ✅
let instance: Store<StoreSchema> | null = null with const store() = ... is the right pattern for electron-store instances — they're expensive to instantiate and screen isn't available until after app ready. Lazy init is safe here since restoreBounds / track are only called from createWindow(), which runs post-ready.
restoreBounds() — clamping math ✅
const width = Math.min(saved.width, wa.width);
const height = Math.min(saved.height, wa.height);
const x = Math.max(wa.x, Math.min(saved.x, wa.x + wa.width - width));
const y = Math.max(wa.y, Math.min(saved.y, wa.y + wa.height - height));This is correct. The window's right edge is guaranteed to stay within wa.x + wa.width (x ≤ wa.x + wa.width - width → x + width ≤ wa.x + wa.width). Same for the bottom edge. The window can't bleed off any edge of the work area. screen.getDisplayMatching uses the display that has the greatest overlap with the saved rect — right behavior when the original display is gone (the window lands on whatever physical screen is closest, not at negative coordinates).
Omitting x/y from the first-run defaults is intentional and correct — Electron centers the window when no position is specified.
track() — Devin's isMinimized() finding, fixed at HEAD ✅
Original 5dde09fb had if (win.isMinimized()) return inside persist(). Devin correctly flagged that getNormalBounds() already returns the pre-minimize geometry regardless of minimized state (per Electron docs), so the guard was actively harmful — it dropped state on "Cmd+Q while minimized" and the "resize-then-minimize within debounce window" case. Fixed in 3782a385 to guard only on isDestroyed(). ✅
The close event handler clearing the debounce timer before calling persist() directly is correct — avoids the race where the debounce fires after win.isDestroyed() returns true.
getNormalBounds() vs getBounds() ✅
getNormalBounds() is the right call. getBounds() on a fullscreen/maximized window returns the full display rectangle — restoring from that would give a 1×1 or full-screen-sized window on normal launch. getNormalBounds() returns the restored-size geometry regardless of current state. Docstring explains this correctly.
fullscreen in BrowserWindow constructor ✅
restoreBounds returns fullscreen: saved.isFullScreen in the spread. BrowserWindow accepts fullscreen as a constructor option. On first run (defaults path), the returned object is { width, height } (no fullscreen) — Electron defaults to false, which is correct. On restore, fullscreen: false is equivalent to the default. No unexpected behavior.
electron-store schema discipline — non-blocking note 📝
settings.ts uses schema + rootSchema: { additionalProperties: false } per the codebase's established pattern (PR #32198). The window-state store is created without a schema:
instance = new Store<StoreSchema>({
name: "window-state",
defaults: { windows: {} },
});For a dynamic-keyed record, strict schema is harder but doable via JSON Schema's additionalProperties:
schema: {
windows: {
type: "object" as const,
additionalProperties: {
type: "object" as const,
properties: {
x: { type: "number" as const },
y: { type: "number" as const },
width: { type: "number" as const },
height: { type: "number" as const },
isFullScreen: { type: "boolean" as const },
},
required: ["x", "y", "width", "height", "isFullScreen"],
},
},
}Not blocking — the TypeScript types already guard the write path, and the main-process-only constraint means the data is never user-supplied. But worth a follow-up to stay consistent with the codebase's electron-store discipline.
Fullscreen crash recovery — minor gap, non-blocking 📝
enter-full-screen isn't explicitly tracked. For normal quit (close fires synchronously), isFullScreen() is read correctly — no gap. For the crash-recovery path (debounced resize/move saves), macOS fullscreen transitions may not fire resize before enter-full-screen completes, meaning a crash during the animation window could miss the fullscreen flag. Next launch would open in normal window mode, which is acceptable degradation but worth noting.
If you want to close this gap: add win.on("enter-full-screen", persist) and win.on("leave-full-screen", persist) with immediate (non-debounced) saves. Fullscreen transitions are rare and not drag-intensive, so no need to debounce them.
Merge gate
- ✅ Vex APPROVED (this review)
- ⏳ Devin reviewed stale
5dde09fb. Codex no review yet. Triggering both below. - ✅
bun run typecheckclean,bun run buildclean (+1.5 kB — expected for new module) - Manual test plan outlined in PR body (5 flows) — the off-screen recovery case (#4) is the one to hit before merge
|
@devin review this PR |
|
@codex review |
There was a problem hiding this comment.
✦ APPROVE — reviewed at 3782a385
Solid, self-contained persistence layer. The architecture decision (separate electron-store instance, main-process only, key-namespaced for future windows) is well-reasoned and matches the codebase's discipline in settings.ts. The off-screen clamping math is correct, Devin's isMinimized() finding was correctly fixed at HEAD, and the show: false + ready-to-show pattern ensures the window doesn't flash at the wrong geometry on restore.
Full analysis
Singleton store — lazy init ✅
let instance: Store<StoreSchema> | null = null with const store() = ... is the right pattern for electron-store instances — they're expensive to instantiate and screen isn't available until after app ready. Lazy init is safe here since restoreBounds / track are only called from createWindow(), which runs post-ready.
restoreBounds() — clamping math ✅
const width = Math.min(saved.width, wa.width);
const height = Math.min(saved.height, wa.height);
const x = Math.max(wa.x, Math.min(saved.x, wa.x + wa.width - width));
const y = Math.max(wa.y, Math.min(saved.y, wa.y + wa.height - height));This is correct. The window's right edge is guaranteed to stay within wa.x + wa.width (x ≤ wa.x + wa.width - width → x + width ≤ wa.x + wa.width). Same for the bottom edge. The window can't bleed off any edge of the work area. screen.getDisplayMatching uses the display that has the greatest overlap with the saved rect — right behavior when the original display is gone (the window lands on whatever physical screen is closest, not at negative coordinates).
Omitting x/y from the first-run defaults is intentional and correct — Electron centers the window when no position is specified.
track() — Devin's isMinimized() finding, fixed at HEAD ✅
Original 5dde09fb had if (win.isMinimized()) return inside persist(). Devin correctly flagged that getNormalBounds() already returns the pre-minimize geometry regardless of minimized state (per Electron docs), so the guard was actively harmful — it dropped state on "Cmd+Q while minimized" and the "resize-then-minimize within debounce window" case. Fixed in 3782a385 to guard only on isDestroyed(). ✅
The close event handler clearing the debounce timer before calling persist() directly is correct — avoids the race where the debounce fires after win.isDestroyed() returns true.
getNormalBounds() vs getBounds() ✅
getNormalBounds() is the right call. getBounds() on a fullscreen/maximized window returns the full display rectangle — restoring from that would give a 1×1 or full-screen-sized window on normal launch. getNormalBounds() returns the restored-size geometry regardless of current state. Docstring explains this correctly.
fullscreen in BrowserWindow constructor ✅
restoreBounds returns fullscreen: saved.isFullScreen in the spread. BrowserWindow accepts fullscreen as a constructor option. On first run (defaults path), the returned object is { width, height } (no fullscreen) — Electron defaults to false, which is correct. On restore, fullscreen: false is equivalent to the default. No unexpected behavior.
electron-store schema discipline — non-blocking note 📝
settings.ts uses schema + rootSchema: { additionalProperties: false } per the codebase's established pattern (PR #32198). The window-state store is created without a schema:
instance = new Store<StoreSchema>({
name: "window-state",
defaults: { windows: {} },
});For a dynamic-keyed record, strict schema is harder but doable via JSON Schema's additionalProperties:
schema: {
windows: {
type: "object" as const,
additionalProperties: {
type: "object" as const,
properties: {
x: { type: "number" as const },
y: { type: "number" as const },
width: { type: "number" as const },
height: { type: "number" as const },
isFullScreen: { type: "boolean" as const },
},
required: ["x", "y", "width", "height", "isFullScreen"],
},
},
}Not blocking — the TypeScript types already guard the write path, and the main-process-only constraint means the data is never user-supplied. But worth a follow-up to stay consistent with the codebase's electron-store discipline.
Fullscreen crash recovery — minor gap, non-blocking 📝
enter-full-screen isn't explicitly tracked. For normal quit (close fires synchronously), isFullScreen() is read correctly — no gap. For the crash-recovery path (debounced resize/move saves), macOS fullscreen transitions may not fire resize before enter-full-screen completes, meaning a crash during the animation window could miss the fullscreen flag. Next launch would open in normal window mode, which is acceptable degradation but worth noting.
If you want to close this gap: add win.on("enter-full-screen", persist) and win.on("leave-full-screen", persist) with immediate (non-debounced) saves. Fullscreen transitions are rare and not drag-intensive, so no need to debounce them.
Merge gate
- ✅ Vex APPROVED (this review)
- ⏳ Devin reviewed stale
5dde09fb. Codex no review yet. Triggering both below. - ✅
bun run typecheckclean,bun run buildclean (+1.5 kB — expected for new module) - Manual test plan outlined in PR body (5 flows) — the off-screen recovery case (#4) is the one to hit before merge
There was a problem hiding this comment.
✦ APPROVE — reviewed at 3782a385
Value: First-run UX stays clean (Electron centers the window), returning users get their geometry back, and both the disconnected-monitor and resolution-shrink failure modes are handled without a third-party library.
Full analysis
restoreBounds — clamping math
const width = Math.min(saved.width, wa.width);
const height = Math.min(saved.height, wa.height);
const x = Math.max(wa.x, Math.min(saved.x, wa.x + wa.width - width));
const y = Math.max(wa.y, Math.min(saved.y, wa.y + wa.height - height));Correct. Clamp dimensions first, then constrain the origin so the clamped window fits inside the work area. The ordering matters (width before x) and it's right here.
getNormalBounds() choice
Using getNormalBounds() rather than getBounds() is the load-bearing decision: it returns the pre-fullscreen/pre-maximize rect regardless of current display mode, so the stored bounds are always the "restored" size. Electron's docs confirm this. The fix Boss applied (drop isMinimized() guard) is correct for the same reason — getNormalBounds() is equally stable when the window is minimized, so the guard was strictly harmful: it dropped a save in exactly the crash-recovery case (resize → minimize → Cmd+Q within the 500ms debounce window). isDestroyed() is the right and only guard.
close handler ordering
win.on("close", () => {
if (saveTimer) {
clearTimeout(saveTimer);
saveTimer = null;
}
persist();
});Cancels the pending debounce before the synchronous persist(). Without this, the debounce timer could fire after isDestroyed() becomes true (depending on event loop timing after close fires). The guard in persist() would catch it, but the explicit cancel is cleaner. ✅
Separate electron-store instance
Own file, own schema, main-process-only — the right call. Keeping geometry out of settings.ts's strict additionalProperties: false schema avoids an awkward schema extension, and the key-namespaced design (track("main", win), future track("thread.<id>", win)) already anticipates the multi-window work in LUM-1870/1971.
Devin's "3 additional findings"
Only the isMinimized() finding appeared inline; the other three are gated to the Devin portal. Worth a quick glance there if you haven't already — the inline one was the real finding and it's addressed.
Anti-patterns
SwiftUI anti-patterns don't apply (Electron/TS). No React, no Zustand, no fire-and-forget async. Main-process TypeScript with synchronous electron-store writes — clean.
Vellum Constitution — Defensive engineering: two failure modes that bit others (unplugged monitor, resolution shrink) are covered before they can hit users.
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
2 participants
Summary
Wires window-geometry persistence in the Electron app so the main window comes back where you left it. Restores width/height/x/y on launch and tracks fullscreen state. Handles the disconnected-display case so the window can't end up off-screen.
Closes LUM-1936.
What's in
apps/macos/src/main/window-state.ts(new, ~140 lines)Two functions:
restoreBounds(key, defaults)— returns a bounds object to spread into theBrowserWindowconstructor. Falls through todefaultson first run (nox/y, so Electron centers the window — the right first-run UX). When state exists, matches the saved rectangle to the closest still-connected display viascreen.getDisplayMatchingand clamps it into that display'sworkArea. Covers two failure modes that bitelectron-window-stateusers in similar codebases:track(key, win)— wires the window to persist onclose(synchronous, normal-exit path) and onresize/movewith a 500ms debounce (crash recovery — covers the case whereclosenever fires). ReadsgetNormalBounds()so un-fullscreening on the next launch reveals the proper restored size, not a 1×1 sliver of the last fullscreen state.isFullScreen()is tracked separately as a flag and passed back through the constructor on restore, so the window comes up in the same display mode it was left in.Storage uses its own
electron-storeinstance (window-state.jsonunderuserData) rather than the renderer-facingsettings.tsstore. Three reasons:additionalProperties: falseand a strict per-key schema — geometry has no business sitting alongsidehotkeys/theme/featureFlags.track("thread.<id>", win)/track("about", win)against the same store without colliding with each other or the strict settings schema.apps/macos/src/main/index.tscreateWindow()now spreadsrestoreBounds("main", { width: 1280, height: 800 })into the constructor and callstrackWindowState("main", mainWindow)right after creation.apps/macos/src/main/settings.tsDocstring updated — the "if window-state restore is wired in a future ticket, it lives in its own keyspace or via a dedicated library" note now points at
./window-state.tsinstead of describing it hypothetically.How to test locally
bun run devTest plan
bun run typecheckclean.bun run buildclean. Main bundle 408.19 kB (+1.5 kB from window-state module).Out of scope (deliberate)
minWidth: 800, minHeight: 600— that's LUM-1965 alongside the tray icon work. Kept the existing 1280×800 default here so this PR is just the persistence layer.keyparameter; their respective tickets will calltrack("...", win)when each window lands.https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
Generated by Claude Code