Repository navigation
Fix #1802, #1917: prevent duplicate/stray windows on display reconnect - #1914
elvistranhere wants to merge 4 commits into
Conversation
|
@elvistranhere is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughMonitor screen-parameter changes and debounce session-snapshot saves; disable macOS automatic window restoration globally and per-window; cancel pending snapshot work on termination; mark various import/debug panels and main windows as non-restorable. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application (AppDelegate)
participant NC as NotificationCenter
participant Deb as Debounce WorkItem
participant SS as SessionSaver (main actor)
App->>NC: register for didChangeScreenParametersNotification
NC->>App: didChangeScreenParametersNotification
App->>Deb: scheduleScreenChangeSnapshot() (create/cancel work item)
Deb-->>Deb: debounce 0.5s
Deb->>App: invoke (if not terminating)
App->>SS: saveSessionSnapshot(includeScrollback: false) on main actor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
…econnect Disable macOS native window restoration so that unplugging/replugging an external monitor no longer spawns a second identical window. The app already manages its own session persistence via SessionPersistenceStore, so system-level restoration is unnecessary and actively harmful. Three changes: - Set NSQuitAlwaysKeepsWindows to false in applicationDidFinishLaunching - Mark every registered window as isRestorable = false - Observe didChangeScreenParametersNotification to save a consistent session snapshot after display topology changes
6fa5585 to
ee54d85
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Greptile SummaryThis PR addresses duplicate/stray window creation on external display reconnect (#1802, #1917) by disabling macOS system-level window restoration app-wide and supplementing the app's own Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant macOS
participant AppDelegate
participant DispatchQueue
participant SessionPersistenceStore
macOS->>AppDelegate: didChangeScreenParametersNotification
AppDelegate->>AppDelegate: scheduleScreenChangeSnapshot()
AppDelegate->>AppDelegate: cancel previous DispatchWorkItem
AppDelegate->>DispatchQueue: asyncAfter(0.5s, workItem)
Note over macOS,AppDelegate: Notification may fire 5-10x during single event
macOS->>AppDelegate: didChangeScreenParametersNotification (again)
AppDelegate->>AppDelegate: scheduleScreenChangeSnapshot()
AppDelegate->>AppDelegate: cancel previous DispatchWorkItem ✓
AppDelegate->>DispatchQueue: asyncAfter(0.5s, new workItem)
DispatchQueue->>AppDelegate: workItem fires (debounced)
AppDelegate->>SessionPersistenceStore: saveSessionSnapshot(includeScrollback: false)
Note over AppDelegate,SessionPersistenceStore: On termination: workItem cancelled,<br/>full snapshot saved synchronously
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Prevents macOS from creating duplicate cmux windows when an external display is disconnected/reconnected by disabling native window restoration and ensuring the app persists a consistent session snapshot after display-topology changes.
Changes:
- Disable macOS “reopen/restoration” behavior via
NSQuitAlwaysKeepsWindows. - Mark main windows as non-restorable (
window.isRestorable = false). - Observe
NSApplication.didChangeScreenParametersNotificationto persist a fresh session snapshot after display changes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rver - Use UserDefaults.register(defaults:) instead of .set to avoid overwriting user-level preferences - Debounce didChangeScreenParametersNotification with 0.5s coalescing since it can fire many times during a single display reconfiguration
elvistranhere
left a comment
elvistranhere
left a comment
There was a problem hiding this comment.
Addressed all feedback — switched to register(defaults:) and added 0.5s debounce on the screen change observer. Regarding the regression test for isRestorable: per the project's test policy, tests must verify observable runtime behavior, not property assertions — so skipping a property-check test here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 3315-3323: The screen-change observer calls
saveSessionSnapshot(includeScrollback: false) asynchronously and can race with
termination writes; modify the observer/termination flow so session persistence
writes are serialized and the final quit snapshot cannot be clobbered by a
queued screen-change save: route termination writes through
sessionPersistenceQueue.sync (or add a write-generation/cancellation token
checked by saveSessionSnapshot) and ensure saveSessionSnapshot consults that
generation/token (or the queue) before performing its write; reference
saveSessionSnapshot(includeScrollback:), sessionPersistenceQueue,
isTerminatingApp and the NotificationCenter observer so the display-change
handler either performs its save on sessionPersistenceQueue synchronously or is
canceled/ignored when a termination write is in-flight.
Ensures the debounced screen-change snapshot cannot race with the final quit snapshot by explicitly cancelling the work item in applicationShouldTerminate.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
2647-2649:⚠️ Potential issue | 🟠 MajorQueued screen-change saves can still overwrite the final quit snapshot.
Line 3336 still goes through the async
sessionPersistenceQueuepath. If that debounced save fires just before Line 2648, canceling the work item no longer helps, and the older/no-scrollback write can run after the synchronous quit save. Drain or serializesessionPersistenceQueuebefore every terminating save, or gate async writes with a generation/token so the final snapshot always wins.Also applies to: 3331-3340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 2647 - 2649, Queued async saves can still race and overwrite the synchronous final quit snapshot; ensure the terminating save always wins by either draining/serializing sessionPersistenceQueue before calling saveSessionSnapshot or by adding a generation/token guard to all async writes so only the latest generation is persisted. Update the shutdown path where isTerminatingApp is set and screenChangeSnapshotWorkItem is cancelled to: flush or await completion of sessionPersistenceQueue (or acquire its serial lock) before calling _ = saveSessionSnapshot(includeScrollback: true, removeWhenEmpty: false), or implement and check a persistenceGeneration token in saveSessionSnapshot and the sessionPersistenceQueue task submissions (increment at termination) so queued tasks skip older generations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 2647-2649: Queued async saves can still race and overwrite the
synchronous final quit snapshot; ensure the terminating save always wins by
either draining/serializing sessionPersistenceQueue before calling
saveSessionSnapshot or by adding a generation/token guard to all async writes so
only the latest generation is persisted. Update the shutdown path where
isTerminatingApp is set and screenChangeSnapshotWorkItem is cancelled to: flush
or await completion of sessionPersistenceQueue (or acquire its serial lock)
before calling _ = saveSessionSnapshot(includeScrollback: true, removeWhenEmpty:
false), or implement and check a persistenceGeneration token in
saveSessionSnapshot and the sessionPersistenceQueue task submissions (increment
at termination) so queued tasks skip older generations.
|
Confirmed this issue occurs in the nightly build. Duplicate windows appear when an external display is disconnected and reconnected. |
Related issues filed for remaining window-spawning bugsThis PR fixes the main window duplication on display reconnect (#1802), but investigation found additional related issues:
|
Popup panels, import wizard/progress dialogs, and debug windows were never marked non-restorable, allowing macOS to recreate them as blank/duplicate windows on display reconnect or wake from sleep.
elvistranhere
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
|
@greptileai review |
|
@copilot review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@elvistranhere Just wanted to say thanks for this PR, had the same issue and was about to dive into the code when i found it, can confirm it fixed the issue 🙏 |
thanks! @lawrencecchen love to get this resolved |
|
@elvistranhere I've been struggling with this bug, thanks for fixing it. @lawrencecchen looking to get it resolved! |
|
Thanks for this! The duplicate-window-on-reconnect bug is fixed (SwiftUI window restore is off) landed on main in #3164. You opened this first, so you got there first. Closing since main covers it now. |
Summary
NSQuitAlwaysKeepsWindows,isRestorable) so unplugging/replugging an external monitor no longer spawns a duplicate windowdidChangeScreenParametersNotificationobserver to save a consistent session snapshot after display topology changesisRestorable = falseon all non-main windows (browser popups, import dialogs, debug panels) to prevent macOS from restoring any window type on display reconnectSessionPersistenceStore, so system-level restoration is unnecessary and was causing the duplicateTest plan
Fixes #1802
Also addresses #1917