Fix browser panes reloading when switching workspaces - #1136
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR modifies BrowserWindowPortal to preserve hidden portal entries when their anchor is missing or invalid, returning nil instead of detaching them. It adds corresponding test coverage verifying that hidden portals survive anchor removal and are properly rebound when the anchor is restored. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes browser panes reloading when switching workspaces by preserving hidden portal entries in Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant SwiftUI
participant Portal as WindowBrowserPortal
participant Entry as PortalEntry (hidden)
participant WKWebView
Note over SwiftUI,WKWebView: Workspace deactivation
SwiftUI->>Portal: updateEntryVisibility(visibleInUI: false)
SwiftUI->>Portal: synchronizeWebViewForAnchor(oldAnchor)
Portal->>Entry: mark slot hidden
Entry->>WKWebView: slot.isHidden = true
Note over SwiftUI,WKWebView: SwiftUI unmounts anchor
SwiftUI->>SwiftUI: oldAnchor.removeFromSuperview()
SwiftUI->>Portal: synchronizeWebViewForAnchor(oldAnchor)
Portal->>Portal: pruneDeadEntries()
Note over Portal: anchorInvalidForCurrentHost = true<br/>BEFORE: hidden → return webViewId (prune ❌)<br/>AFTER: return nil (preserve ✅)
Portal-->>Entry: entry kept alive, WKWebView stays attached
Note over SwiftUI,WKWebView: Workspace reactivation
SwiftUI->>Portal: bind(webView, to: newAnchor, visibleInUI: true)
Portal->>Entry: anchorView = newAnchor, visibleInUI = true
Portal->>Entry: slot.isHidden = false
Entry->>WKWebView: displayIfNeeded() — no reload
Last reviewed commit: 57eeaed |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if anchorInvalidForCurrentHost { | ||
| return entry.visibleInUI ? nil : webViewId | ||
| // Hidden browser portals can legitimately be off-tree between workspace | ||
| // deactivation and the next rebind. Preserve them until an explicit detach | ||
| // (panel close, window teardown, or web view replacement) says otherwise. | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Multi-window anchor migration may leak entries permanently
The anchorInvalidForCurrentHost condition covers three sub-cases: anchor.window !== currentWindow, anchor.superview == nil, and the reference-view check. The first sub-case — anchor migrated to a different NSWindow — is not the same as the "workspace switch on the same window" scenario the comment describes.
In a multi-window configuration, if a hidden portal's anchorView ends up on a different window and the corresponding workspace is later closed without going back to that window, the entry will never be cleaned up unless a direct detachWebView call is made. The previous code at least pruned hidden entries in that state.
Consider narrowing the "always-preserve" logic to the specific conditions that workspace switching triggers:
if anchorInvalidForCurrentHost {
// Hidden portals whose anchor is just off-tree (superview == nil or outside the reference
// view) are legitimately mid-transition during workspace deactivation; preserve them.
// If the anchor has moved to a different NSWindow, the portal is truly orphaned and
// should still be pruned while hidden.
let anchorOnDifferentWindow = anchor.window !== currentWindow
if anchorOnDifferentWindow && !entry.visibleInUI {
return webViewId
}
return nil
}Same consideration applies to the guard let anchor = entry.anchorView else path above — a fully-deallocated anchor (weak ref went nil) is an equally strong signal that the entry is orphaned.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/BrowserWindowPortal.swift (1)
2804-2820:⚠️ Potential issue | 🟠 MajorAvoid making orphaned hidden portals immortal.
Both branches now unconditionally
return nil, sopruneDeadEntries()will never reclaim an entry once its anchor has been deallocated or moved off-tree. Because the hiddenWindowBrowserSlotViewstays attached tohostView, that can retain theWKWebViewand its WebKit process indefinitely unless some external path happens to calldetach. The workspace-switch fix needs a bounded grace/rebind policy here, not permanent retention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` around lines 2804 - 2820, The current logic in pruneDeadEntries()/the block handling WindowBrowserSlotView always returns nil when anchorInvalidForCurrentHost, preventing pruning and leaking WKWebView; change it to enforce a bounded grace/rebind window instead of permanent retention: record a timestamp (e.g. lastHiddenAt) on WindowBrowserSlotView when it becomes off-tree/anchorInvalidForCurrentHost, and only return nil (preserve) if that timestamp is within a short grace period; otherwise return webViewId so pruneDeadEntries() can reclaim it (also ensure detach() still forces immediate reclamation). Use the existing symbols anchor, container, hostView, webViewId, pruneDeadEntries(), WindowBrowserSlotView and detach to locate and implement this timed-grace policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/BrowserWindowPortal.swift`:
- Around line 2804-2820: The current logic in pruneDeadEntries()/the block
handling WindowBrowserSlotView always returns nil when
anchorInvalidForCurrentHost, preventing pruning and leaking WKWebView; change it
to enforce a bounded grace/rebind window instead of permanent retention: record
a timestamp (e.g. lastHiddenAt) on WindowBrowserSlotView when it becomes
off-tree/anchorInvalidForCurrentHost, and only return nil (preserve) if that
timestamp is within a short grace period; otherwise return webViewId so
pruneDeadEntries() can reclaim it (also ensure detach() still forces immediate
reclamation). Use the existing symbols anchor, container, hostView, webViewId,
pruneDeadEntries(), WindowBrowserSlotView and detach to locate and implement
this timed-grace policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54807481-7f56-4058-be8e-5d317618d27a
📒 Files selected for processing (2)
Sources/BrowserWindowPortal.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
* Add workspace browser portal regression test * Preserve hidden browser portals across workspace switches
This branch was successfully deployed
1 participant
Summary
WKWebViewis reused on workspace returnTesting
./scripts/reload.sh --tag issue-1132-browser-refreshFixes #1132
Summary by cubic
Prevents browser panes from reloading when switching workspaces by preserving hidden portal entries and reusing their
WKWebViewon return. Keeps workspace switches fast and seamless.WKWebView.Written for commit 57eeaed. Summary will update on new commits.
Summary by CodeRabbit