Skip to content

Fix hidden browser slot inspector focus crash #1211

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
lawrencecchen merged 2 commits into from
Mar 12, 2026
Merged

Fix hidden browser slot inspector focus crash #1211

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
3b2a1c3
Add browser slot responder crash regression test
lawrencecchen Mar 12, 2026
fce5545
Yield hidden browser slot responders
lawrencecchen Mar 12, 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
30 changes: 30 additions & 0 deletions Sources/BrowserWindowPortal.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1530,6 +1530,12 @@ final class BrowserPaneDropTargetView: NSView {

final class WindowBrowserSlotView: NSView {
override var isOpaque: Bool { false }
override var isHidden: Bool {
didSet {
guard isHidden, !oldValue, let window else { return }
yieldOwnedFirstResponderIfNeeded(in: window, reason: "slotHidden")
}
}
private let paneDropTargetView = BrowserPaneDropTargetView(frame: .zero)
private let dropZoneOverlayView = BrowserDropZoneOverlayView(frame: .zero)
private var searchOverlayHostingView: NSHostingView<BrowserSearchOverlay>?
Expand Down Expand Up @@ -1571,6 +1577,13 @@ final class WindowBrowserSlotView: NSView {
nil
}

override func viewWillMove(toWindow newWindow: NSWindow?) {
if newWindow == nil, let currentWindow = window {
yieldOwnedFirstResponderIfNeeded(in: currentWindow, reason: "slotWillLeaveWindow")
}
super.viewWillMove(toWindow: newWindow)
}

override func layout() {
super.layout()
paneDropTargetView.frame = bounds
Expand Down Expand Up @@ -1739,6 +1752,23 @@ final class WindowBrowserSlotView: NSView {
return window.makeFirstResponder(nil)
}

@discardableResult
private func yieldOwnedFirstResponderIfNeeded(in window: NSWindow, reason: String) -> Bool {
guard let firstResponder = window.firstResponder,
let owningView = firstResponder.browserPortalOwningView,
owningView === self || owningView.isDescendant(of: self) else {
return false
}
#if DEBUG
dlog(
"browser.slot.firstResponder.yield reason=\(reason) " +
"slot=\(browserPortalDebugToken(self)) " +
"responder=\(String(describing: type(of: firstResponder)))"
)
#endif
return window.makeFirstResponder(nil)
}

func pinHostedWebView(_ webView: WKWebView) {
guard webView.superview === self else { return }

Expand Down
44 changes: 43 additions & 1 deletion cmuxTests/CmuxWebViewKeyEquivalentTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2434,7 +2434,9 @@ final class BrowserSessionHistoryRestoreTests: XCTestCase {

@MainActor
final class BrowserDeveloperToolsVisibilityPersistenceTests: XCTestCase {
private final class WKInspectorProbeView: NSView {}
private final class WKInspectorProbeView: NSView {
override var acceptsFirstResponder: Bool { true }
}

private final class FakeInspector: NSObject {
private(set) var attachCount = 0
Expand Down Expand Up @@ -11515,6 +11517,46 @@ final class BrowserWindowPortalLifecycleTests: XCTestCase {
)
}

func testHidingBrowserSlotYieldsOwnedInspectorFirstResponder() {
let window = NSWindow(
contentRect: NSRect(x: 0, y: 0, width: 520, height: 320),
styleMask: [.titled, .closable],
backing: .buffered,
defer: false
)
defer { window.orderOut(nil) }
realizeWindowLayout(window)

guard let contentView = window.contentView else {
XCTFail("Expected content view")
return
}

let slot = WindowBrowserSlotView(frame: NSRect(x: 40, y: 24, width: 260, height: 180))
contentView.addSubview(slot)

let inspectorContainer = NSView(frame: slot.bounds)
inspectorContainer.autoresizingMask = [.width, .height]
let inspectorView = WKInspectorProbeView(frame: inspectorContainer.bounds)
inspectorView.autoresizingMask = [.width, .height]
inspectorContainer.addSubview(inspectorView)
slot.addSubview(inspectorContainer)
contentView.layoutSubtreeIfNeeded()

XCTAssertTrue(
window.makeFirstResponder(inspectorView),
"Precondition failed: inspector probe should become first responder"
Comment on lines +11546 to +11548

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Use first-responder-capable probe in slot-hide test

This precondition assumes window.makeFirstResponder(inspectorView) can succeed, but the WKInspectorProbeView used in BrowserWindowPortalLifecycleTests still inherits NSView without overriding acceptsFirstResponder, which defaults to false in AppKit. In that case this assertion fails and the regression does not actually cover the slot-hiding focus-yield path the fix relies on.

Useful? React with 👍 / 👎.

)
XCTAssertTrue(window.firstResponder === inspectorView)

slot.isHidden = true

XCTAssertNil(
window.firstResponder,
"Hiding a browser slot should yield any owned inspector responder before it goes off-screen"
)
}

@greptile-apps greptile-apps Bot Mar 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing test coverage for viewWillMove(toWindow:) path

The PR adds two guard sites — the isHidden didSet and viewWillMove(toWindow: nil) — but the new regression test only exercises the isHidden path. The window-removal path could silently regress without any test catching it. A companion test that removes the slot from its window while an inspector responder is active would give full coverage for this fix.

Comment on lines +11520 to +11558

@coderabbitai coderabbitai Bot Mar 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

This regression never reaches the hide path.

The acceptsFirstResponder override added on Lines 2437-2438 is on a different nested WKInspectorProbeView type. This test instantiates BrowserWindowPortalLifecycleTests.WKInspectorProbeView on Line 11540, and that local type still inherits NSView.acceptsFirstResponder == false, so the precondition on Line 11547 will fail before slot.isHidden = true is exercised.

Suggested fix 
private final class WKInspectorProbeView: NSView {
    override var acceptsFirstResponder: Bool { true }
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 11520 - 11558,
The test testHidingBrowserSlotYieldsOwnedInspectorFirstResponder never reaches
the hide path because the WKInspectorProbeView used in the test (the nested
BrowserWindowPortalLifecycleTests.WKInspectorProbeView) doesn't override
acceptsFirstResponder, so window.makeFirstResponder(inspectorView) fails; fix by
updating that local WKInspectorProbeView definition to override var
acceptsFirstResponder: Bool { true } so the inspectorView can become first
responder before slot.isHidden is toggled.


func testHiddenPortalSyncDoesNotStealLocallyHostedDevToolsWebViewDuringResize() {
let window = NSWindow(
contentRect: NSRect(x: 0, y: 0, width: 520, height: 320),
Expand Down
Loading