Skip to content

fix(browser): reparent webviews into a single active host #241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yonaries merged 2 commits into from
Mar 16, 2026
Merged

fix(browser): reparent webviews into a single active host #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be14f4d
fix(browser): reparent webviews into a single active host
yonaries Mar 16, 2026
ea2a342
refactor: harden browser page host view transfers
yonaries Mar 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 141 additions & 26 deletions ora/Core/BrowserEngine/BrowserPageView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,121 @@
import AppKit
import SwiftUI

final class BrowserPageHostView: NSView {
private(set) weak var hostedContentView: NSView?

override init(frame frameRect: NSRect) {
super.init(frame: frameRect)
wantsLayer = true
autoresizesSubviews = true
}

@available(*, unavailable)
required init?(coder: NSCoder) {
fatalError("init(coder:) has not been implemented")
}

func host(contentView newContentView: NSView?) {
let previousContentView = hostedContentView
let isSameContentView = previousContentView === newContentView

if isSameContentView, newContentView?.superview === self {
return
}

let shouldRestoreFirstResponder = shouldTransferFirstResponder(from: previousContentView)

if isSameContentView {
hostedContentView = nil
} else {
detachHostedContentView()
}

guard let newContentView else {
refreshHostingLayout()
return
}

configure(contentView: newContentView)

if let previousHost = newContentView.superview as? BrowserPageHostView,
previousHost !== self
{
previousHost.releaseHostedContentView(newContentView)
}

if newContentView.superview !== self {
if newContentView.superview != 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)
}
}
Comment on lines +18 to +67
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


override func layout() {
super.layout()
hostedContentView?.frame = bounds
}

private func configure(contentView: NSView) {
contentView.wantsLayer = true
contentView.autoresizingMask = [.width, .height]
contentView.layer?.isOpaque = true
contentView.layer?.drawsAsynchronously = true
}

private func detachHostedContentView() {
guard let hostedContentView else {
return
}

if hostedContentView.superview === self {
hostedContentView.removeFromSuperview()
}

self.hostedContentView = nil
}

private func releaseHostedContentView(_ contentView: NSView) {
guard hostedContentView === contentView else {
return
}

detachHostedContentView()
refreshHostingLayout()
}

private func refreshHostingLayout() {
needsLayout = true
layoutSubtreeIfNeeded()
displayIfNeeded()
}

private func shouldTransferFirstResponder(from previousContentView: NSView?) -> Bool {
guard let previousContentView,
let firstResponder = window?.firstResponder as? NSView
else {
return false
}

return firstResponder === previousContentView || firstResponder.isDescendant(of: previousContentView)
}
}

struct BrowserPageView: NSViewRepresentable {
let page: BrowserPage
@EnvironmentObject var tabManager: TabManager
Expand All @@ -9,49 +124,44 @@ struct BrowserPageView: NSViewRepresentable {

func makeCoordinator() -> Coordinator {
Coordinator(
page: page,
tabManager: tabManager,
historyManager: historyManager,
privacyMode: privacyMode
)
}

func makeNSView(context: Context) -> NSView {
let wrapperView = NSView()
wrapperView.wantsLayer = true
wrapperView.autoresizesSubviews = true

func makeNSView(context: Context) -> BrowserPageHostView {
let hostView = BrowserPageHostView(frame: .zero)
let contentView = page.contentView
contentView.autoresizingMask = [.width, .height]
contentView.layer?.isOpaque = true
contentView.layer?.drawsAsynchronously = true
context.coordinator.setupMouseEventMonitoring(for: contentView)
wrapperView.addSubview(contentView)
contentView.frame = wrapperView.bounds

return wrapperView
context.coordinator.update(page: page, contentView: contentView)
hostView.host(contentView: contentView)
return hostView
}

func updateNSView(_ nsView: NSView, context: Context) {}
func updateNSView(_ nsView: BrowserPageHostView, context: Context) {
let contentView = page.contentView
context.coordinator.update(page: page, contentView: contentView)
nsView.host(contentView: contentView)
}

final class Coordinator: NSObject {
private let page: BrowserPage
weak var tabManager: TabManager?
weak var historyManager: HistoryManager?
weak var privacyMode: PrivacyMode?
private weak var page: BrowserPage?
private var mouseEventMonitor: Any?
private weak var contentView: NSView?

init(
page: BrowserPage,
tabManager: TabManager?,
historyManager: HistoryManager?,
privacyMode: PrivacyMode
) {
self.page = page
self.tabManager = tabManager
self.historyManager = historyManager
self.privacyMode = privacyMode
super.init()
startMouseEventMonitoring()
}

deinit {
Expand All @@ -60,11 +170,15 @@ struct BrowserPageView: NSViewRepresentable {
}
}

func setupMouseEventMonitoring(for contentView: NSView) {
func update(page: BrowserPage, contentView: NSView) {
self.page = page
self.contentView = contentView
}

private func startMouseEventMonitoring() {
mouseEventMonitor = NSEvent.addLocalMonitorForEvents(matching: [.otherMouseDown]) { [weak self] event in
Comment on lines +178 to 179
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

guard let self,
let page = self.page,
let contentView = self.contentView,
self.isEventInContentView(event, contentView: contentView)
else {
Expand All @@ -73,19 +187,19 @@ struct BrowserPageView: NSViewRepresentable {

switch event.buttonNumber {
case 2:
self.handleMiddleClick(at: event.locationInWindow, contentView: contentView)
self.handleMiddleClick(at: event.locationInWindow, contentView: contentView, page: page)
return nil
case 3:
if self.page.canGoBack {
if page.canGoBack {
DispatchQueue.main.async {
self.page.goBack()
page.goBack()
}
}
return nil
case 4:
if self.page.canGoForward {
if page.canGoForward {
DispatchQueue.main.async {
self.page.goForward()
page.goForward()
}
}
return nil
Expand All @@ -95,7 +209,7 @@ struct BrowserPageView: NSViewRepresentable {
}
}

private func handleMiddleClick(at location: NSPoint, contentView: NSView) {
private func handleMiddleClick(at location: NSPoint, contentView: NSView, page: BrowserPage) {
let locationInContentView = contentView.convert(location, from: nil)
guard locationInContentView.x.isFinite,
locationInContentView.y.isFinite,
Expand All @@ -115,8 +229,9 @@ struct BrowserPageView: NSViewRepresentable {
})();
"""

page.evaluateJavaScript(script) { [weak self] result, error in
page.evaluateJavaScript(script) { [weak self, weak page] result, error in
guard error == nil,
page != nil,
let urlString = result as? String,
let url = URL(string: urlString),
let tabManager = self?.tabManager,
Expand Down
18 changes: 6 additions & 12 deletions ora/Features/Browser/Views/BrowserSplitView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,14 @@ struct BrowserSplitView: View {
}

private func contentView() -> some View {
ZStack {
if tabManager.activeTab == nil {
Group {
if let activeTab = tabManager.activeTab {
BrowserContentContainer {
HomeView()
BrowserWebContentView(tab: activeTab)
}
}
let activeId = tabManager.activeTab?.id
ForEach(tabManager.tabsToRender) { tab in
if tab.isWebViewReady {
BrowserContentContainer {
BrowserWebContentView(tab: tab)
}
.opacity(tab.id == activeId ? 1 : 0)
.allowsHitTesting(tab.id == activeId)
} else {
BrowserContentContainer {
HomeView()
}
}
}
Expand Down
7 changes: 0 additions & 7 deletions ora/Features/Tabs/State/TabManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,6 @@ class TabManager: ObservableObject {
)
}

var tabsToRender: [Tab] {
guard let container = activeContainer else { return [] }
let specialTabs = container.tabs.filter { $0.type == .pinned || $0.type == .fav || $0.isPlayingMedia }
let combined = Set(recentTabs + specialTabs)
return Array(combined)
}

/// Note: Could be made injectable via init parameter if preferred
let tabSearchingService: TabSearchingProviding

Expand Down
6 changes: 3 additions & 3 deletions ora/Features/Tabs/Switcher/FloatingTabSwitcher.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,12 @@ struct FloatingTabSwitcher: View {
}
}
}
.padding(.horizontal, 28)
.padding(.vertical, 20)
.padding(.horizontal, 20)
.padding(.vertical, 18)
.background(BlurEffectView(material: .popover, blendingMode: .withinWindow))
.background(theme.background.opacity(0.3))
.cornerRadius(Constants.containerCornerRadius)
.shadow(color: .blue.opacity(0.07), radius: 16, x: 0, y: 12)
.shadow(color: theme.primary.opacity(0.07), radius: 16, x: 0, y: 12)
.background(keyboardHandler)
.overlay(containerBorder)
}
Expand Down
Loading
Loading