fix(browser): reparent webviews into a single active host#241
Conversation
Greptile SummaryReplaces the previous multi-mounted ZStack+ForEach tab rendering strategy with a single-host reparenting approach. Instead of keeping multiple
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[BrowserSplitView.contentView] -->|activeTab exists| B[BrowserContentContainer]
A -->|no activeTab| H[HomeView]
B --> C[BrowserWebContentView]
C -->|tab.browserPage exists| D[BrowserPageView]
D -->|makeNSView / updateNSView| E[BrowserPageHostView]
E -->|host contentView| F{Same content view & superview?}
F -->|Yes| G[No-op early return]
F -->|No| I[Remove previous content view]
I --> J[Reparent new WKWebView into host]
J --> K[Set frame, layout, display]
K --> L{First responder was in old view?}
L -->|Yes| M[Transfer first responder to new view]
L -->|No| N[Done]
D -->|Coordinator.update| O[Retarget page & contentView refs]
O --> P[Global mouse monitor uses latest refs]
Last reviewed commit: 0819e75 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func host(contentView newContentView: NSView?) { | ||
| let previousContentView = hostedContentView | ||
| let isSameContentView = previousContentView === newContentView | ||
|
|
||
| if isSameContentView, newContentView?.superview === self { | ||
| return | ||
| } | ||
|
|
||
| let shouldRestoreFirstResponder = shouldTransferFirstResponder(from: previousContentView) | ||
|
|
||
| previousContentView?.removeFromSuperview() | ||
| hostedContentView = nil | ||
|
|
||
| guard let newContentView else { | ||
| needsLayout = true | ||
| layoutSubtreeIfNeeded() | ||
| displayIfNeeded() | ||
| return | ||
| } | ||
|
|
||
| configure(contentView: newContentView) | ||
|
|
||
| if newContentView.superview !== self { | ||
| if let previousHost = newContentView.superview as? BrowserPageHostView { | ||
| previousHost.hostedContentView = nil | ||
| } | ||
| newContentView.removeFromSuperview() | ||
| addSubview(newContentView) | ||
| } | ||
|
|
||
| newContentView.frame = bounds | ||
| hostedContentView = newContentView | ||
|
|
||
| needsLayout = true | ||
| layoutSubtreeIfNeeded() | ||
| newContentView.needsLayout = true | ||
| newContentView.layoutSubtreeIfNeeded() | ||
| newContentView.needsDisplay = true | ||
| displayIfNeeded() | ||
|
|
||
| if shouldRestoreFirstResponder { | ||
| window?.makeFirstResponder(newContentView) | ||
| } | ||
| } |
There was a problem hiding this comment.
Redundant removeFromSuperview when switching to a different content view
When the previous and new content views are different, previousContentView?.removeFromSuperview() on line 28 removes the old view. Then later at line 40-46, the code checks newContentView.superview !== self and removes newContentView from its current parent before adding it. However, if both previousContentView and newContentView happen to be children of self (e.g. if a stale hostedContentView was left behind from an earlier bug), removeFromSuperview() would be called twice on the same view in a single pass — once as previousContentView and once inside the superview !== self block if addSubview was somehow skipped. In practice the current code paths prevent this, but the dual-removal pattern is fragile.
More concretely: when previousContentView !== newContentView and newContentView.superview is another BrowserPageHostView, previousHost.hostedContentView is correctly nilled out. But the previous host's own host(contentView:) won't be called to clean up any layout/display state it may have queued — it just silently loses its reference. This is likely fine today since the old host will be deallocated, but could be a subtle source of stale-state bugs if hosts are ever reused.
| private func startMouseEventMonitoring() { | ||
| mouseEventMonitor = NSEvent.addLocalMonitorForEvents(matching: [.otherMouseDown]) { [weak self] event in |
There was a problem hiding this comment.
Global event monitor lives for coordinator's entire lifetime
NSEvent.addLocalMonitorForEvents is an app-wide listener. The monitor is registered once in init() and only removed in deinit. Because the coordinator's page and contentView are weak and retargeted on each SwiftUI update, the monitor effectively listens for all otherMouseDown events across the app for its entire lifetime.
If multiple BrowserPageView instances are ever alive simultaneously (e.g. during split-view or if SwiftUI keeps a stale coordinator), each coordinator will run isEventInContentView on every mouse event. This is unlikely to cause correctness issues (the hit-test guards are solid), but is worth noting for performance if the number of coordinators scales.
yonaries
force-pushed
the
fix/single-host-tab-reparenting-main
branch
from
798b36c to
ea2a342
Compare
1 participant
Summary
Verification