Repository navigation
Fix #2791: preserve window position across sleep/wake with multiple monitors - #3882
Merged
Merged
Conversation
The sleep/wake regression needs a behavior-level guard that real cmux main windows enroll in AppKit frame autosave and do not share a live autosave name. Current main-window construction leaves frameAutosaveName empty, so this test is expected to fail before the implementation commit. Constraint: Regression-test commit must contain no production fix so CI can prove the issue first goes red. Confidence: high Scope-risk: narrow Tested: Not run locally per repository testing policy. Not-tested: Local xcodebuild and app launch intentionally skipped for the failing-test-only commit.
Main windows now register AppKit frame autosave names as soon as they are created. The first standalone main window uses a stable primary name when available, while additional live windows use UUID-scoped names so AppKit accepts every autosave registration. The old cmux lastWindowGeometry fallback was a competing frame owner: it could remap or persist frames independently of AppKit's screen topology logic. Session snapshots still store window layout with workspace content, but the standalone fallback no longer reads or writes frame data for new-window placement. Constraint: Do not add sleep/wake timers or screen-change repair hooks; use the platform frame autosave mechanism Apple exposes for this window state. Rejected: Add didWake/screensDidChange repositioning | would introduce another lifecycle repair path and preserve split ownership. Confidence: medium Scope-risk: moderate Tested: git diff --check Not-tested: Local tests and local build intentionally skipped per repo policy and user instruction; CI will run after PR push.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAppDelegate switches from writing custom persisted window-geometry payloads to using AppKit frame-autosave names: windows register per-window autosave names, autosaved frames are applied conditionally during creation, session snapshot persistence no longer includes geometry data, and teardown clears ephemeral autosave names. Tests added for autosave and legacy-key cleanup. ChangesAppKit Frame Autosave Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Greptile SummarySwitches main-window frame persistence from a cmux-owned
Confidence Score: 5/5Safe to merge — the close-path ordering is correct, UUID autosave keys are cleaned up, and the survivor promotion handoff guards against AppKit frame reloads. The critical sequencing decisions — releasing the primary name after context unregistration, pre-seeding the primary slot before setFrameAutosaveName, and correcting any AppKit-reloaded frame after promotion — are all handled correctly. Previous review findings about silent promotion failure and UUID key accumulation have been properly addressed. The only noteworthy gap is that removeLegacyPersistedWindowGeometry no longer runs at startup, so the v2 UserDefaults key lingers until the first session write after upgrade; since nothing reads the v2 key anymore this has no behavioral consequence. No files require special attention. The AppDelegate close-path logic is the most complex part of the change, but the ordering has been validated by the promotion tests and the prior review iterations. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[windowWillClose] --> B{window holds primary name?}
B -- yes --> C[shouldReleasePrimary = true]
B -- no --> D[removeEphemeralAutosaveName: clear UUID name + removeFrame]
C --> E[removeEphemeralAutosaveName: no-op guarded by != primary]
D --> F[unregisterMainWindowContext]
E --> F
F -- nil --> G[return early - name not cleared, no promotion]
F -- context --> H{shouldReleasePrimary?}
H -- yes --> I[setFrameAutosaveName empty - release primary slot]
H -- no --> J[promotePrimary excluding window]
I --> J
J --> K{live primary owner excluding window?}
K -- yes --> L[return - primary still live]
K -- no --> M{survivor in registered contexts?}
M -- no --> N[return - last window closing]
M -- yes --> O[saveFrame to primary - seed slot before registering]
O --> P{setFrameAutosaveName primary succeeds?}
P -- no --> Q[return - UUID key not removed]
P -- yes --> R{frame changed by AppKit?}
R -- yes --> S[setFrame back to survivorFrame]
R -- no --> T[saveFrame - confirm survivor frame]
S --> T
T --> U[removeFrame for survivor UUID key]
Reviews (6): Last reviewed commit: "Prove promoted autosave frame payload" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Secondary main windows need AppKit frame autosave during their lifetime, but their UUID-scoped autosave keys are not stable restore points. Closing those windows now removes the matching AppKit frame key while preserving the primary slot. This also makes autosave registration explicit instead of hiding the side effect inside a boolean chain, and adds a regression that saves a secondary frame then verifies close removes it. Constraint: Primary frame autosave must survive close because it is the stable main-window restore slot. Rejected: Keep UUID autosave keys indefinitely | leaks per-window keys that are never read again. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local tests/build intentionally skipped per repo policy and user instruction; CI will verify.
The production close path now removes UUID-scoped frame autosave names, and this test also removes the secondary name in teardown so the registration contract cannot dirty UserDefaults even if a future close path changes. Constraint: Preserve the primary autosave slot across tests while cleaning per-window UUID slots. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local tests/build intentionally skipped per repo policy and user instruction; CI will verify.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Cursor Bugbot flagged two cleanup gaps after the AppKit autosave migration: ephemeral windows could keep their autosave registration active during close, and the retired v2 cmux geometry key was not part of legacy cleanup. The fix keeps AppKit as the single frame owner by clearing UUID-scoped autosave names before removing their saved frame and deleting the retired v2 key alongside the v1 key. Constraint: Local test execution is intentionally skipped per task instructions; CI owns verification for this PR. Rejected: Keep only NSWindow.removeFrame for UUID windows | AppKit can still write through an active frameAutosaveName before teardown completes. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest or app build, per explicit instruction to use CI and defer reload until CI is green.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmuxTests/AppDelegateBareSpaceShortcutRoutingTests.swift`:
- Around line 381-383: The helper appKitFrameAutosaveDefaultsKey(_ name:
NSWindow.FrameAutosaveName) reconstructs AppKit's internal UserDefaults key
format ("NSWindow Frame {name}") which is an undocumented implementation detail;
add a brief documentation comment above this function stating that the string
format intentionally mirrors AppKit's internal convention (and may be brittle
across OS versions) so future maintainers understand why the hard-coded format
is used.
In `@Sources/AppDelegate.swift`:
- Around line 6776-6793: The primary autosave slot can remain pointing at a
non-live window, so update
mainWindowFrameAutosaveName(windowId:wantsPrimarySlot:) to first check whether
any live NSApp.windows currently own Self.primaryMainWindowFrameAutosaveName
and, if none and wantsPrimarySlot is true, return
Self.primaryMainWindowFrameAutosaveName so the new window can be promoted;
additionally enhance removeEphemeralMainWindowFrameAutosaveNameIfNeeded(_:) to,
after clearing an ephemeral autosave name, check for a missing live primary
owner and if none assign the primary slot to the first suitable live main window
(via setFrameAutosaveName(Self.primaryMainWindowFrameAutosaveName)) so the
primary slot is always held by a live window.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9be3322-0fa7-4b25-9488-89bd66eb3cce
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/AppDelegateBareSpaceShortcutRoutingTests.swift
Review feedback exposed a remaining lifecycle gap in the AppKit autosave handoff: the stable primary autosave name could outlive its window while another cmux window stayed open. The close path now promotes a surviving main window into the primary slot, saves its frame immediately, and retires its old UUID-scoped frame. The retired cmux geometry write plumbing is also removed so the legacy key is only ever cleaned up, not rewritten. Constraint: AppKit frame autosave is the single source of truth for window frame persistence after this migration. Rejected: Preserve the old persistedGeometryData parameter | it made a removed key look writable and kept a path back to competing frame persistence. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest or app build, per explicit instruction to use CI and defer reload until CI is green.
austinywang
dismissed
coderabbitai[bot]’s stale review
May 12, 2026 01:44
Addressed by f5bf158; CodeRabbit approved the latest head.
Greptile found that primary-slot promotion could run while the closing primary NSWindow still held the autosave name during windowWillClose. The close path now clears that registration before promotion, and promotion only removes the survivor's old UUID frame after AppKit accepts the primary name. Constraint: The primary autosave UserDefaults frame must remain preserved while live ownership moves to a surviving cmux window. Rejected: Keep ignoring setFrameAutosaveName's return value | it could remove the survivor's UUID frame after a failed promotion. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest or app build, per explicit instruction to use CI and defer reload until CI is green.
The primary autosave name should be released only after the closing window is confirmed as a registered main window. This keeps the promotion fix precise: unregister the closing context, clear the primary autosave registration from that closing NSWindow, then promote a survivor into the stable primary slot. Constraint: WindowWillClose still contains the closing NSWindow in NSApp.windows, so promotion must explicitly release the old autosave owner first. Rejected: Clear the primary autosave name before unregistering | it mutates a window even if context unregister unexpectedly fails. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest or app build, per explicit instruction to use CI and defer reload until CI is green.
AppKit reloads an autosaved frame when a window adopts that name, so the primary-slot promotion now seeds the stable name with the survivor frame before registration and restores the survivor frame if AppKit applies anything during the transition. Constraint: setFrameAutosaveName can reload the associated frame as part of registration Rejected: Assign primary autosave name first and save afterward | that can capture the closed primary window frame after moving the survivor Confidence: high Scope-risk: narrow Directive: Keep primary autosave promotion frame-neutral; do not register the survivor under a stale primary name before seeding the saved frame Tested: git diff --check Not-tested: Local XCTest/builds intentionally not run per project instruction; CI will run on PR
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3f7f03c. Configure here.
There was a problem hiding this comment.
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 `@cmuxTests/AppDelegateBareSpaceShortcutRoutingTests.swift`:
- Around line 405-412: The test currently only checks that
primaryFrameAutosaveKey exists in UserDefaults and that secondWindow preserved
its onscreen frame, but it must verify the actual payload was promoted; read the
stored value for primaryFrameAutosaveKey from defaults, decode/convert it into a
CGRect/NSRect (matching how frames are archived in the app), and assert that the
decoded frame equals survivorFrameBeforePromotion (use same accuracy as existing
assertions). Use the same UserDefaults instance (`defaults`) and the same
reference values (`survivorFrameBeforePromotion`, `primaryFrameAutosaveKey`,
`secondWindow`) so the test fails if the stored primary autosave was not
updated.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f436e5bc-e54a-4e08-9db0-4cb920d56eb5
📒 Files selected for processing (3)
Sources/App/SessionSnapshotDebugBenchmark.swiftSources/AppDelegate.swiftcmuxTests/AppDelegateBareSpaceShortcutRoutingTests.swift
The promotion test now loads the stable primary autosave entry into a probe window so it proves the stored payload matches the survivor frame, not just that the key exists. The autosave-name helper also owns the live-primary check directly instead of accepting a redundant caller-computed flag. Constraint: Review gate required proving AppKit's stored primary autosave frame, not only the live survivor frame Rejected: Keep the key-presence assertion alone | it would still pass with a stale primary autosave payload Confidence: high Scope-risk: narrow Directive: Primary autosave promotion tests must verify both live-frame preservation and the restorable autosave payload Tested: git diff --check Not-tested: Local XCTest/builds intentionally not run per project instruction; CI will run on PR
austinywang
dismissed
coderabbitai[bot]’s stale review
May 12, 2026 03:06
Superseded by fbc7d24; the actionable inline thread was resolved with a probe-window autosave payload assertion, and the latest CodeRabbit check is passing.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2791
Summary
Verification
Note
Medium Risk
Changes main-window placement and persistence by switching from cmux-owned geometry to AppKit frame autosave, which could affect window restore/positioning across multi-window scenarios. Includes new autosave-name promotion/cleanup logic that needs validation across edge cases (multiple windows, close ordering, sleep/wake).
Overview
Main window geometry persistence is switched to AppKit frame autosave: new windows now register a stable primary autosave name (
cmux.mainWindow.primary) and UUID-scoped names for additional windows, and AppKit-saved frames are applied when not restoring from a session snapshot or source window.The PR removes the
lastWindowGeometryfallback path (including snapshot persistence/debug plumbing) and expands legacy cleanup to delete both v1 and v2 geometry keys, making AppKit the single frame source.On window close, ephemeral autosave entries are removed and, if the primary window closes, a surviving window is promoted into the stable autosave slot without changing its on-screen frame. Tests are expanded to cover autosave name registration, legacy-key cleanup/ignoring, ephemeral cleanup, and primary-slot promotion.
Reviewed by Cursor Bugbot for commit fbc7d24. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #2791: Preserves main‑window position across sleep/wake and monitor changes by using AppKit frame autosave with a stable primary slot and frame‑neutral promotion on close. Removes cmux‑owned geometry so AppKit is the single source of truth.
cmux.mainWindow.primaryfor the first live window; UUID‑scoped for others); AppKit‑saved frames apply when not restoring from a snapshot or source window.cmux.mainWindow.primary, seeds the primary slot with the survivor’s frame, restores that frame if AppKit reloads, then retires the survivor’s old UUID key only after promotion succeeds.lastWindowGeometryread/write paths (including snapshot plumbing) and expanded legacy cleanup to delete both v1 and v2 keys.Written for commit fbc7d24. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests