Repository navigation
Restore window positions when external displays reconnect - #2491
joshuaswanson wants to merge 8 commits into
Conversation
Sites with strict CSP (Airtable, various React SPAs) block scripts injected into WKContentWorld.page, causing js_error from browser.eval and snapshot --interactive. Switch all evaluateJavaScript calls and WKUserScript registrations targeting browser panel webviews to use .defaultClient, which is CSP-exempt but retains full DOM access.
… add contentWorld parameter to BrowserPanel.evaluateJavaScript
|
@joshuaswanson is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR addresses two distinct problems: (1) the primary feature — restoring managed windows to their previous positions on external displays when those displays reconnect, and (2) a housekeeping migration of all The display-restoration mechanism is clean in its overall design: Key changes:
Issues found:
Confidence Score: 4/5Safe to merge after addressing the frame-cache cleanup on window close; the WKContentWorld migration is mechanical and correct throughout. One P1 defect exists: stale ObjectIdentifier entries in displayWindowFrameCache can cause wrong window positioning via address reuse after a window closes. This is a real correctness bug on the changed code path. The remaining findings are P2 (design trade-off and comment wording). The WKContentWorld migration across BrowserPanel, CmuxWebView, and TerminalController looks mechanically correct. Sources/AppDelegate.swift — specifically the window close path in unregisterMainWindow, which needs to purge the corresponding displayWindowFrameCache entries. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant macOS
participant AppDelegate
participant Cache as displayWindowFrameCache
participant NC as NotificationCenter
Note over User,NC: Normal usage — window on external display
User->>macOS: Move/resize window on Display A
macOS->>NC: NSWindow.didMoveNotification
NC->>AppDelegate: handleWindowDidMove(_:)
AppDelegate->>Cache: Store frame keyed by [displayA][windowID]
Note over User,NC: External display disconnects
macOS->>macOS: Move window to primary display
macOS->>NC: NSWindow.didMoveNotification
NC->>AppDelegate: handleWindowDidMove(_:)
AppDelegate->>Cache: Store frame keyed by [primary][windowID]
Note over Cache: displayA entry preserved
macOS->>NC: NSApplication.didChangeScreenParametersNotification
NC->>AppDelegate: handleScreenParametersDidChange(_:)
AppDelegate->>AppDelegate: reconnected = currentIDs minus knownIDs (empty)
AppDelegate->>AppDelegate: knownConnectedDisplayIDs updated
Note over User,NC: External display reconnects
macOS->>NC: NSApplication.didChangeScreenParametersNotification
NC->>AppDelegate: handleScreenParametersDidChange(_:)
AppDelegate->>AppDelegate: reconnected = {displayA}
AppDelegate->>Cache: Look up displayWindowFrameCache[displayA]
Cache-->>AppDelegate: {windowID: savedFrame}
AppDelegate->>AppDelegate: clampFrame(savedFrame, within: screen.visibleFrame)
AppDelegate->>macOS: window.setFrame(clamped, animate: true)
macOS->>User: Window animates back to Display A
Reviews (1): Last reviewed commit: "Restore window positions when external d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func cacheWindowFrameForDisplay(_ window: NSWindow) { | ||
| guard mainWindowContexts[ObjectIdentifier(window)] != nil else { return } | ||
| guard let displayID = window.screen?.cmuxDisplayID else { return } | ||
| displayWindowFrameCache[displayID, default: [:]][ObjectIdentifier(window)] = window.frame |
There was a problem hiding this comment.
displayWindowFrameCache entries are added whenever a window moves or resizes, but they are never removed when a window closes. unregisterMainWindow (called from willCloseNotification) clears mainWindowContexts and many other per-window maps, but leaves displayWindowFrameCache untouched.
Because ObjectIdentifier is derived from the object's memory address, once a window is deallocated its address can be reused by a new NSWindow. If a new window is registered in mainWindowContexts at the same ObjectIdentifier, handleScreenParametersDidChange will find the stale entry and silently move the brand-new window to the position that belonged to the long-closed window the next time that display reconnects.
The fix is to purge all cache entries for the closing window's ObjectIdentifier inside unregisterMainWindow, at the same point where the other per-window state (commandPaletteVisibilityByWindowId, etc.) is cleaned up.
| @objc private func handleScreenParametersDidChange(_ notification: Notification) { | ||
| let currentDisplayIDs = Set(NSScreen.screens.compactMap { $0.cmuxDisplayID }) | ||
| let reconnected = currentDisplayIDs.subtracting(knownConnectedDisplayIDs) | ||
| knownConnectedDisplayIDs = currentDisplayIDs | ||
|
|
||
| guard !reconnected.isEmpty else { return } | ||
|
|
||
| for displayID in reconnected { | ||
| guard let cachedFrames = displayWindowFrameCache[displayID], | ||
| let screen = NSScreen.screens.first(where: { $0.cmuxDisplayID == displayID }) else { | ||
| continue | ||
| } | ||
| for (windowID, frame) in cachedFrames { | ||
| guard let (_, ctx) = mainWindowContexts.first(where: { $0.key == windowID }), | ||
| let window = ctx.window else { | ||
| continue | ||
| } | ||
| let clamped = Self.clampFrame( | ||
| frame, | ||
| within: screen.visibleFrame, | ||
| minWidth: CGFloat(SessionPersistencePolicy.minimumWindowWidth), | ||
| minHeight: CGFloat(SessionPersistencePolicy.minimumWindowHeight) | ||
| ) | ||
| window.setFrame(clamped, display: true, animate: true) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
displayWindowFrameCache stores frames per display without any notion of why the window left the display. Consider this sequence:
- External display A is connected; window lives on A.
- User manually drags the window to the primary display (cache now has entries for both displays).
- External display A disconnects and later reconnects.
handleScreenParametersDidChange sees display A as "reconnected," finds the stale cache entry for the window under display A's ID, and moves the window back to A — overriding the user's explicit choice. Filtering out windows that are already on a still-connected display before restoring would prevent this.
| } | ||
| } | ||
| } else { | ||
| // macOS < 11.0: contentWorld parameter is ignored; JS runs in .page |
There was a problem hiding this comment.
evaluateJavaScript fallback
The comment says "contentWorld parameter is ignored" which implies the parameter exists but is silently discarded. What actually happens is that the evaluateJavaScript(_:in:in:completionHandler:) overload that accepts a WKContentWorld does not exist on macOS < 11.0, so the legacy evaluateJavaScript(_:completionHandler:) is used, which always executes in the .page world with no way to opt out.
| // macOS < 11.0: contentWorld parameter is ignored; JS runs in .page | |
| // macOS < 11.0: WKContentWorld API unavailable; JS always runs in .page world | |
| webView.evaluateJavaScript(script) { value, error in |
| completion(false) | ||
| return | ||
| } | ||
| webView.evaluateJavaScript(Self.addressBarFocusRestoreScript) { [weak self] result, error in | ||
| webView.evaluateJavaScript(Self.addressBarFocusRestoreScript, in: nil, in: .defaultClient) { [weak self] callResult in | ||
| let result: Any? | ||
| let error: Error? | ||
| switch callResult { | ||
| case .success(let value): result = value; error = nil | ||
| case .failure(let err): result = nil; error = err | ||
| } | ||
|
|
||
| guard let self else { | ||
| completion(false) | ||
| return |
There was a problem hiding this comment.
Result into legacy variables
The callback decomposes the Result back into separate result and error variables to feed the existing downstream code, unlike every other converted site in this PR which switches on the Result directly. The current approach works correctly but is inconsistent with the pattern used elsewhere in the same changeset.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:7110">
P1: Switching browser eval/addscript execution from page world to defaultClient can break scripts that rely on page-defined globals/functions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| timeout: timeout, | ||
| preferAsync: true, | ||
| contentWorld: .page | ||
| contentWorld: .defaultClient |
There was a problem hiding this comment.
P1: Switching browser eval/addscript execution from page world to defaultClient can break scripts that rely on page-defined globals/functions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TerminalController.swift, line 7110:
<comment>Switching browser eval/addscript execution from page world to defaultClient can break scripts that rely on page-defined globals/functions.</comment>
<file context>
@@ -7106,33 +7107,15 @@ class TerminalController {
timeout: timeout,
preferAsync: true,
- contentWorld: .page
+ contentWorld: .defaultClient
)
} else {
</file context>
📝 WalkthroughWalkthroughPer-display window-frame caching and reconnection-aware restoration were added to AppDelegate; numerous WKWebView evaluateJavaScript call sites and injected WKUserScript instances were migrated to run in the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant DispMgr as AppDelegate
participant Screen as NSScreen(s)
participant Window as MainWindow
participant Cache as FrameCache
App->>DispMgr: applicationDidFinishLaunching
DispMgr->>Screen: read NSScreen.screens -> knownConnectedDisplayIDs
DispMgr->>DispMgr: register screen & window observers
Window->>DispMgr: moved / resized / screen changed
DispMgr->>Cache: cacheWindowFrameForDisplay(windowUUID, displayID, frame)
Note over Screen,DispMgr: display disconnect / relayout detected
Screen->>DispMgr: didChangeScreenParametersNotification
DispMgr->>DispMgr: compute disconnected vs reconnected IDs
DispMgr->>DispMgr: set isHandlingDisplayDisconnect (suppress pruning briefly)
alt reconnected displays exist
loop each cached window for displayID
DispMgr->>Cache: get cached frame for displayID
Cache-->>DispMgr: return cached frame
DispMgr->>DispMgr: clamp frame to display.visibleFrame (apply min size)
DispMgr->>Window: setFrame(clampedFrame, display:true, animate:true)
end
end
Window->>DispMgr: unregisterMainWindow (on close)
DispMgr->>Cache: purge cached frames for windowUUID across displays
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:3571">
P2: Clearing this window’s cached frame on all other displays during every move/resize can erase the disconnected monitor’s last-known frame when macOS auto-relocates windows after a display unplug. That leaves handleScreenParametersDidChange with no cached frame to restore on reconnect, reintroducing the “window doesn’t return to external display” bug.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/TerminalController.swift (2)
7049-7118:⚠️ Potential issue | 🟠 MajorIsolate user-authored JS in the page world, not the client world.
Lines 7110–7118 move
v2RunBrowserJavaScriptto.defaultClient, an isolated sandbox. This breaks public APIs (browser.eval,browser.wait(function=...),browser.addscript,browser.addinitscript) that accept user-written JS—users cannot access page-defined globals, functions, or state. For example,browser.wait(function='typeof window.MyAppGlobal !== undefined')will fail becauseMyAppGlobaldoesn't exist in the isolated world.The telemetry hooks (console, error, dialog capture) are intentionally in
.defaultClientto avoid CAPTCHA interference, but public JS-execution paths require page-world access.Add a
contentWorldparameter tov2RunBrowserJavaScriptdefaulting to.page, and explicitly pass.defaultClientonly for internal cmux-owned operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 7049 - 7118, The v2RunBrowserJavaScript function is forcing execution into the isolated .defaultClient world which prevents user-authored scripts from seeing page globals; change v2RunBrowserJavaScript to accept a new contentWorld parameter (defaulting to .page) and forward that to v2RunJavaScript (instead of always using .defaultClient), and only call it with .defaultClient from internal cmux telemetry/own operations; update both the macOS-11 and fallback evaluateFallback v2RunJavaScript calls to use the new contentWorld argument so public APIs (browser.eval, browser.wait, browser.addscript, browser.addinitscript) run in the page world while internal hooks can explicitly request .defaultClient.
9354-9402:⚠️ Potential issue | 🟠 MajorHooks in
.defaultClientcannot observe page-world console/error/dialog activity.Lines 9360/9370 install telemetry hooks in
.defaultClient, and lines 9402/10053/10091 readwindow.__cmuxDialogQueue,window.__cmuxConsoleLog, andwindow.__cmuxErrorLogfrom that same isolated world. Because client worlds do not share JS state with the page, page-originatedconsole.*,error,alert,confirm, andprompttraffic will not populate these structures.browser.console.list,browser.errors.list, and dialog automation will only capture cmux-side JavaScript unless hooks/readers use.pageworld or implement explicit bridging between worlds.The CAPTCHA concern (detecting overridden
console.*methods in cross-origin iframes) applies to iframe injection, not the.pagevs.defaultClientchoice on the main frame. Consider whether capturing page-world activity is a required feature, and if so, implement.pageworld injection for the main frame.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 9354 - 9402, The telemetry and dialog hook injections in v2BrowserEnsureTelemetryHooks and v2BrowserEnsureDialogHooks currently use contentWorld: .defaultClient which cannot observe page-world console/error/dialog activity that v2BrowserDialogRespond (which reads window.__cmuxDialogQueue), and the console/error readers expect; change the injection calls to use contentWorld: .page (or detect/main-frame and use .page for the main frame) when injecting BrowserPanel.telemetryHookBootstrapScriptSource and BrowserPanel.dialogTelemetryHookBootstrapScriptSource via v2RunJavaScript so the page and client worlds share those window.__cmux* globals, ensuring browser.console.list, browser.errors.list and dialog automation capture page-originated events; keep .defaultClient only if you intentionally want cmux-side isolation.
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
8147-8149: Consider using a consistent wrapper for WebKit evaluateJavaScript calls.Lines 8147 and 8355 directly call
webView.evaluateJavaScript(script, in: nil, in: .defaultClient)with completion handlers, whileBrowserPanelalready provides anevaluateJavaScript()wrapper with async/throws semantics. The raw calls use a different calling convention (callback-based vs. async), but maintaining a single centralized path for these calls reduces the risk of future drift or inconsistencies (e.g., ifcontentWorldhandling changes). If the callback pattern is needed here, consider adding a callback-based wrapper toBrowserPanelrather than calling the raw API directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 8147 - 8149, The two direct calls to panel.webView.evaluateJavaScript(script, in: nil, in: .defaultClient) should be routed through BrowserPanel’s centralized evaluateJavaScript wrapper to keep contentWorld handling consistent; either replace these raw callback-based usages with the existing async/throws BrowserPanel.evaluateJavaScript call (await and handle errors) or add a callback-based overload to BrowserPanel (e.g., evaluateJavaScript(_:in:completion:)) and use that from AppDelegate, updating the call sites that currently capture callResult and cast payload so they use the wrapper’s result/error handling instead.
🤖 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 2435-2460: Seed the display cache immediately when a main window
becomes managed by populating knownConnectedDisplayIDs from the window's current
screen/frame instead of waiting for move/resize/screen-change notifications:
after the observers are registered (the block that sets knownConnectedDisplayIDs
= Set(NSScreen.screens.compactMap { $0.cmuxDisplayID }) and adds
NSApplication.didChangeScreenParametersNotification /
NSWindow.didChangeScreenNotification / NSWindow.didMoveNotification /
NSWindow.didResizeNotification), also examine the managed window's current
screen via window.screen and its frame (window.frame) and add that screen's
cmuxDisplayID into knownConnectedDisplayIDs (or call the same seeding logic used
by handleScreenParametersDidChange) so a window already on an external display
is backfilled; make the same change at the other occurrence noted (the block
around lines 3577-3590) and ensure any restoration logic in
handleScreenParametersDidChange uses this preseeded set.
- Around line 3567-3575: cacheWindowFrameForDisplay currently removes cached
frames for other displays when a window's screen changes, which clears the old
display entry during disconnect-driven moves; stop deleting otherDisplayID
entries here and only update the mapping for the current display. Specifically,
in cacheWindowFrameForDisplay remove the loop that calls
displayWindowFrameCache[otherDisplayID]?.removeValue(forKey: windowID) so
existing entries for other displays are preserved, and add an explicit pruning
path to be invoked only on user-initiated moves (e.g., from the window-move
handler or a new method like pruneCachedFramesForWindow(_:) referenced by
handleScreenParametersDidChange) that will remove stale display entries when the
user explicitly moves a window.
---
Outside diff comments:
In `@Sources/TerminalController.swift`:
- Around line 7049-7118: The v2RunBrowserJavaScript function is forcing
execution into the isolated .defaultClient world which prevents user-authored
scripts from seeing page globals; change v2RunBrowserJavaScript to accept a new
contentWorld parameter (defaulting to .page) and forward that to v2RunJavaScript
(instead of always using .defaultClient), and only call it with .defaultClient
from internal cmux telemetry/own operations; update both the macOS-11 and
fallback evaluateFallback v2RunJavaScript calls to use the new contentWorld
argument so public APIs (browser.eval, browser.wait, browser.addscript,
browser.addinitscript) run in the page world while internal hooks can explicitly
request .defaultClient.
- Around line 9354-9402: The telemetry and dialog hook injections in
v2BrowserEnsureTelemetryHooks and v2BrowserEnsureDialogHooks currently use
contentWorld: .defaultClient which cannot observe page-world
console/error/dialog activity that v2BrowserDialogRespond (which reads
window.__cmuxDialogQueue), and the console/error readers expect; change the
injection calls to use contentWorld: .page (or detect/main-frame and use .page
for the main frame) when injecting
BrowserPanel.telemetryHookBootstrapScriptSource and
BrowserPanel.dialogTelemetryHookBootstrapScriptSource via v2RunJavaScript so the
page and client worlds share those window.__cmux* globals, ensuring
browser.console.list, browser.errors.list and dialog automation capture
page-originated events; keep .defaultClient only if you intentionally want
cmux-side isolation.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 8147-8149: The two direct calls to
panel.webView.evaluateJavaScript(script, in: nil, in: .defaultClient) should be
routed through BrowserPanel’s centralized evaluateJavaScript wrapper to keep
contentWorld handling consistent; either replace these raw callback-based usages
with the existing async/throws BrowserPanel.evaluateJavaScript call (await and
handle errors) or add a callback-based overload to BrowserPanel (e.g.,
evaluateJavaScript(_:in:completion:)) and use that from AppDelegate, updating
the call sites that currently capture callResult and cast payload so they use
the wrapper’s result/error handling instead.
🪄 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
Run ID: 79c112c9-3d69-4eb1-bf36-a96b317c96d2
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/Panels/BrowserPanel.swiftSources/Panels/CmuxWebView.swiftSources/TerminalController.swift
| // Track external display connect/disconnect for window position restoration. | ||
| knownConnectedDisplayIDs = Set(NSScreen.screens.compactMap { $0.cmuxDisplayID }) | ||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(handleScreenParametersDidChange(_:)), | ||
| name: NSApplication.didChangeScreenParametersNotification, | ||
| object: nil | ||
| ) | ||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(handleWindowDidChangeScreen(_:)), | ||
| name: NSWindow.didChangeScreenNotification, | ||
| object: nil | ||
| ) | ||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(handleWindowDidMove(_:)), | ||
| name: NSWindow.didMoveNotification, | ||
| object: nil | ||
| ) | ||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(handleWindowDidResize(_:)), | ||
| name: NSWindow.didResizeNotification, | ||
| object: nil | ||
| ) |
There was a problem hiding this comment.
Seed the display cache when a main window becomes managed.
Right now the cache is only populated by future move/resize/screen-change notifications. A window that's already sitting on an external display when it registers can unplug before any of those fire, leaving handleScreenParametersDidChange with nothing to restore. Please backfill from the current window.frame once registration completes.
Also applies to: 3577-3590
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 2435 - 2460, Seed the display cache
immediately when a main window becomes managed by populating
knownConnectedDisplayIDs from the window's current screen/frame instead of
waiting for move/resize/screen-change notifications: after the observers are
registered (the block that sets knownConnectedDisplayIDs =
Set(NSScreen.screens.compactMap { $0.cmuxDisplayID }) and adds
NSApplication.didChangeScreenParametersNotification /
NSWindow.didChangeScreenNotification / NSWindow.didMoveNotification /
NSWindow.didResizeNotification), also examine the managed window's current
screen via window.screen and its frame (window.frame) and add that screen's
cmuxDisplayID into knownConnectedDisplayIDs (or call the same seeding logic used
by handleScreenParametersDidChange) so a window already on an external display
is backfilled; make the same change at the other occurrence noted (the block
around lines 3577-3590) and ensure any restoration logic in
handleScreenParametersDidChange uses this preseeded set.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
3567-3594:⚠️ Potential issue | 🟠 MajorManual moves/resizes still leave stale disconnected-display restore targets behind.
After display A disconnects, every later
didMove/didResizeon display B still preserves A because A is no longer inknownConnectedDisplayIDs. If the user intentionally repositions the window while A is unplugged, reconnecting A will still snap the window back to the stale cached frame. You still need an explicit prune path for user-initiated moves/resizes, and only the disconnect-driven relayout path should preserve the old display entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3567 - 3594, cacheWindowFrameForDisplay currently preserves cached frames for disconnected displays, causing stale restore positions when the user manually moves/resizes on another display; change cacheWindowFrameForDisplay to accept a flag (e.g., preserveDisconnected: Bool = true) that controls whether to skip pruning entries for displays not in knownConnectedDisplayIDs, update the loop to prune otherDisplayID entries unconditionally when preserveDisconnected is false, and call cacheWindowFrameForDisplay(window, preserveDisconnected: false) from the user-driven handlers handleWindowDidMove, handleWindowDidResize and handleWindowDidChangeScreen while keeping the disconnect/relayout callers using the default true to preserve entries.
🤖 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 2305-2309: displayWindowFrameCache currently keys per-window
frames by ObjectIdentifier(window), which breaks when the logical window is
re-instantiated; change the cache to key by the stable logical identifier
(MainWindowContext.windowId) instead of ObjectIdentifier(window), update all
uses that read/write the cache (store/restore/cleanup paths) to accept and pass
windowId, and ensure close-time purge logic removes entries for any old object
IDs by removing entries keyed by that windowId; refer to
displayWindowFrameCache, knownConnectedDisplayIDs, MainWindowContext.windowId
and any places that currently build ObjectIdentifier(window) to locate and
update the code paths.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 3567-3594: cacheWindowFrameForDisplay currently preserves cached
frames for disconnected displays, causing stale restore positions when the user
manually moves/resizes on another display; change cacheWindowFrameForDisplay to
accept a flag (e.g., preserveDisconnected: Bool = true) that controls whether to
skip pruning entries for displays not in knownConnectedDisplayIDs, update the
loop to prune otherDisplayID entries unconditionally when preserveDisconnected
is false, and call cacheWindowFrameForDisplay(window, preserveDisconnected:
false) from the user-driven handlers handleWindowDidMove, handleWindowDidResize
and handleWindowDidChangeScreen while keeping the disconnect/relayout callers
using the default true to preserve entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
8152-8155: Prefer the newBrowserPanel.evaluateJavaScriptwrapper here as well.These DEBUG helpers call WebKit directly, so they bypass the
BrowserPanelexecution path this PR introduced forpreferAsync,contentWorld, and older-macOS fallback behavior. That makes the UI-test JS path diverge from the production browser path. Please confirm the bypass is intentional; otherwise route these through the wrapper too.Also applies to: 8360-8361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 8152 - 8155, The DEBUG helper is calling WebKit directly via panel.webView.evaluateJavaScript which bypasses the new BrowserPanel.evaluateJavaScript wrapper (and thus skips preferAsync, contentWorld and macOS fallback behavior); change this to call BrowserPanel.evaluateJavaScript (the wrapper method on BrowserPanel) instead, passing the same script and completion handling and preserving the existing weak self capture and payload parsing; apply the same replacement for the other occurrence referenced (around the second location) so both debug paths use BrowserPanel.evaluateJavaScript rather than webView.evaluateJavaScript.
🤖 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 3567-3578: The cacheWindowFrameForDisplay logic must avoid
restoring windows that were intentionally moved on a different (connected)
display after a disconnect: when caching a window frame into
displayWindowFrameCache for the current display (in cacheWindowFrameForDisplay),
clear any stale entries for that same ctx.windowId that live under disconnected
displayIDs by removing displayWindowFrameCache[oldDisplayID]?[windowID] if
oldDisplayID is not equal to displayID (instead of only pruning connected
displays), and/or record a per-window "lastMovedAt" timestamp or a
pendingRestore flag on mainWindowContexts[ObjectIdentifier(window)] that you set
here; then update the reconnect/restore path to consult that timestamp/flag
(e.g., skip restoration if lastMovedAt is newer than the disconnect time or
pendingRestore is false). Use the existing symbols displayWindowFrameCache,
mainWindowContexts, ctx.windowId, knownConnectedDisplayIDs and the reconnect
restore code to implement this recency check.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 8152-8155: The DEBUG helper is calling WebKit directly via
panel.webView.evaluateJavaScript which bypasses the new
BrowserPanel.evaluateJavaScript wrapper (and thus skips preferAsync,
contentWorld and macOS fallback behavior); change this to call
BrowserPanel.evaluateJavaScript (the wrapper method on BrowserPanel) instead,
passing the same script and completion handling and preserving the existing weak
self capture and payload parsing; apply the same replacement for the other
occurrence referenced (around the second location) so both debug paths use
BrowserPanel.evaluateJavaScript rather than webView.evaluateJavaScript.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| private func cacheWindowFrameForDisplay(_ window: NSWindow) { | ||
| guard let ctx = mainWindowContexts[ObjectIdentifier(window)] else { return } | ||
| guard let displayID = window.screen?.cmuxDisplayID else { return } | ||
| let windowID = ctx.windowId | ||
| for otherDisplayID in displayWindowFrameCache.keys where otherDisplayID != displayID { | ||
| // Only prune entries for displays that are still connected. | ||
| // Disconnected display entries are preserved for restoration. | ||
| if knownConnectedDisplayIDs.contains(otherDisplayID) { | ||
| displayWindowFrameCache[otherDisplayID]?.removeValue(forKey: windowID) | ||
| } | ||
| } | ||
| displayWindowFrameCache[displayID, default: [:]][windowID] = window.frame |
There was a problem hiding this comment.
Don't restore windows that were intentionally rehomed while a display was away.
The disconnected-display entry survives later moves/resizes on a connected display, and the reconnect path restores every window still cached under that old display. If a user unplugs display A, keeps working on display B, and manually repositions the window on B, reconnecting A will still snap it back to A. This needs a per-window recency / pending-restore check so post-disconnect user moves clear the stale entry.
Also applies to: 3603-3620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 3567 - 3578, The
cacheWindowFrameForDisplay logic must avoid restoring windows that were
intentionally moved on a different (connected) display after a disconnect: when
caching a window frame into displayWindowFrameCache for the current display (in
cacheWindowFrameForDisplay), clear any stale entries for that same ctx.windowId
that live under disconnected displayIDs by removing
displayWindowFrameCache[oldDisplayID]?[windowID] if oldDisplayID is not equal to
displayID (instead of only pruning connected displays), and/or record a
per-window "lastMovedAt" timestamp or a pendingRestore flag on
mainWindowContexts[ObjectIdentifier(window)] that you set here; then update the
reconnect/restore path to consult that timestamp/flag (e.g., skip restoration if
lastMovedAt is newer than the disconnect time or pendingRestore is false). Use
the existing symbols displayWindowFrameCache, mainWindowContexts, ctx.windowId,
knownConnectedDisplayIDs and the reconnect restore code to implement this
recency check.
…during system relayout
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 3599-3635: The reconnect logic restores windows from
displayWindowFrameCache but session saving still reads live window.frame and
displaySnapshot(for:), which can record fallback-display data and overwrite the
cached placement; update buildSessionSnapshot(...) and
persistWindowGeometry(from:) to prefer using displayWindowFrameCache and the
cached displayID/frame when present (lookup by window UUID and/or displayID)
instead of the current NSScreen-derived values, and fall back to
window.frame/displaySnapshot(for:) only if no cached entry exists; ensure you
reference displayWindowFrameCache, window UUID keys used in mainWindowContexts,
and the same clamping/SessionPersistencePolicy minimums so persisted snapshots
match the restored geometry.
- Around line 3570-3597: cacheWindowFrameForDisplay currently always prunes
other-display entries, which causes disconnected-display entries to be removed
by non-user-driven geometry changes; change cacheWindowFrameForDisplay(_:) to
accept a Bool (e.g., shouldPruneOtherDisplays) or add an internal flag to
control pruning and only run the for-otherDisplayID removal when
shouldPruneOtherDisplays is true; then call cacheWindowFrameForDisplay(window,
shouldPruneOtherDisplays: true) from user-driven handlers handleWindowDidMove
and handleWindowDidResize, but call it with shouldPruneOtherDisplays: false (or
leave default false) from handleWindowDidChangeScreen so cross-display pruning
only happens on real user moves/resizes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| private func cacheWindowFrameForDisplay(_ window: NSWindow) { | ||
| guard let ctx = mainWindowContexts[ObjectIdentifier(window)] else { return } | ||
| guard let displayID = window.screen?.cmuxDisplayID else { return } | ||
| let windowID = ctx.windowId | ||
| for otherDisplayID in displayWindowFrameCache.keys where otherDisplayID != displayID { | ||
| if isHandlingDisplayDisconnect && !knownConnectedDisplayIDs.contains(otherDisplayID) { | ||
| // Preserve disconnected display entries during system relayout. | ||
| continue | ||
| } | ||
| displayWindowFrameCache[otherDisplayID]?.removeValue(forKey: windowID) | ||
| } | ||
| displayWindowFrameCache[displayID, default: [:]][windowID] = window.frame | ||
| } | ||
|
|
||
| @objc private func handleWindowDidMove(_ notification: Notification) { | ||
| guard let window = notification.object as? NSWindow else { return } | ||
| cacheWindowFrameForDisplay(window) | ||
| } | ||
|
|
||
| @objc private func handleWindowDidResize(_ notification: Notification) { | ||
| guard let window = notification.object as? NSWindow else { return } | ||
| cacheWindowFrameForDisplay(window) | ||
| } | ||
|
|
||
| @objc private func handleWindowDidChangeScreen(_ notification: Notification) { | ||
| guard let window = notification.object as? NSWindow else { return } | ||
| cacheWindowFrameForDisplay(window) | ||
| } |
There was a problem hiding this comment.
Only prune other display entries on confirmed user moves/resizes.
didMove, didResize, and didChangeScreen all funnel through the same pruning path here. After the one-turn isHandlingDisplayDisconnect window closes, any later non-user geometry change on the fallback display clears the disconnected-display entry too, so reconnect restore stops working. Keep the frame update here, but gate cross-display pruning on real user-initiated moves/resizes only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 3570 - 3597,
cacheWindowFrameForDisplay currently always prunes other-display entries, which
causes disconnected-display entries to be removed by non-user-driven geometry
changes; change cacheWindowFrameForDisplay(_:) to accept a Bool (e.g.,
shouldPruneOtherDisplays) or add an internal flag to control pruning and only
run the for-otherDisplayID removal when shouldPruneOtherDisplays is true; then
call cacheWindowFrameForDisplay(window, shouldPruneOtherDisplays: true) from
user-driven handlers handleWindowDidMove and handleWindowDidResize, but call it
with shouldPruneOtherDisplays: false (or leave default false) from
handleWindowDidChangeScreen so cross-display pruning only happens on real user
moves/resizes.
| @objc private func handleScreenParametersDidChange(_ notification: Notification) { | ||
| let currentDisplayIDs = Set(NSScreen.screens.compactMap { $0.cmuxDisplayID }) | ||
| let reconnected = currentDisplayIDs.subtracting(knownConnectedDisplayIDs) | ||
| let disconnected = knownConnectedDisplayIDs.subtracting(currentDisplayIDs) | ||
| knownConnectedDisplayIDs = currentDisplayIDs | ||
|
|
||
| // Briefly suppress pruning of disconnected display entries so that | ||
| // the system-driven window relocation preserves the restore frame. | ||
| if !disconnected.isEmpty { | ||
| isHandlingDisplayDisconnect = true | ||
| DispatchQueue.main.async { [weak self] in | ||
| self?.isHandlingDisplayDisconnect = false | ||
| } | ||
| } | ||
|
|
||
| guard !reconnected.isEmpty else { return } | ||
|
|
||
| for displayID in reconnected { | ||
| guard let cachedFrames = displayWindowFrameCache[displayID], | ||
| let screen = NSScreen.screens.first(where: { $0.cmuxDisplayID == displayID }) else { | ||
| continue | ||
| } | ||
| for (windowUUID, frame) in cachedFrames { | ||
| guard let ctx = mainWindowContexts.values.first(where: { $0.windowId == windowUUID }), | ||
| let window = ctx.window else { | ||
| continue | ||
| } | ||
| let clamped = Self.clampFrame( | ||
| frame, | ||
| within: screen.visibleFrame, | ||
| minWidth: CGFloat(SessionPersistencePolicy.minimumWindowWidth), | ||
| minHeight: CGFloat(SessionPersistencePolicy.minimumWindowHeight) | ||
| ) | ||
| window.setFrame(clamped, display: true, animate: true) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Wire the disconnected-display cache into session persistence too.
This reconnect handler restores the live window, but buildSessionSnapshot(...) and persistWindowGeometry(from:) still save the current window.frame / displaySnapshot(for:) from the fallback display. If the app terminates before another save after reconnection, the saved session is still overwritten and the next launch loses the original placement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 3599 - 3635, The reconnect logic
restores windows from displayWindowFrameCache but session saving still reads
live window.frame and displaySnapshot(for:), which can record fallback-display
data and overwrite the cached placement; update buildSessionSnapshot(...) and
persistWindowGeometry(from:) to prefer using displayWindowFrameCache and the
cached displayID/frame when present (lookup by window UUID and/or displayID)
instead of the current NSScreen-derived values, and fall back to
window.frame/displaySnapshot(for:) only if no cached entry exists; ensure you
reference displayWindowFrameCache, window UUID keys used in mainWindowContexts,
and the same clamping/SessionPersistencePolicy minimums so persisted snapshots
match the restored geometry.
|
Display reconnect frame restoration is covered by the current window geometry handling on main: 38c93ff. |
2 participants
Windows don't restore to external displays when reconnected. macOS moves windows to the primary display when an external display disconnects, and the 8-second autosave overwrites the saved position before the display comes back.
This caches each managed window's frame per display ID whenever it moves or resizes. When
NSApplication.didChangeScreenParametersNotificationfires and a previously-disconnected display reappears, windows that were on it get moved back to their cached positions (clamped to the screen's visible frame, with animation).Summary by cubic
Restore window positions to their original external displays when they reconnect, and run browser JS in
WKContentWorld.defaultClientso it works on strict CSP sites.NSApplication.didChangeScreenParametersNotification; clamp to the screen’s visible frame with animation; purge on window close.cmuxDisplayIDto detect reconnections and keep entries for disconnected displays so autosave doesn’t overwrite saved frames.WKContentWorld.defaultClientfor allevaluateJavaScriptandWKUserScriptinjections; addpreferAsyncto enforce thecontentWorld, expose acontentWorldparam onBrowserPanel.evaluateJavaScript, remove the isolated-world retry, update call sites toResult-based completions, and retain a macOS < 11 fallback.Written for commit 4c83c9f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Improvements