Repository navigation
Fix hidden browser slot inspector focus crash - #1211
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds responder-management hooks to BrowserWindowPortal that yield owned first responder when browser slots become hidden or are detached from their window. Introduces a private helper to check ownership and resign responders, integrated into slot visibility and window-transition lifecycle events. Includes a corresponding test to verify the fix. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 a crash (issue #1210) where WebKit reactivates a stale inspector UI responder after app activation by ensuring
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["isHidden = true (didSet)"] -->|"isHidden && !oldValue && window != nil"| C
B["viewWillMove(toWindow: nil)"] -->|"newWindow == nil && window != nil"| C
C["yieldOwnedFirstResponderIfNeeded(in: window)"]
C --> D{"window.firstResponder\nexists?"}
D -->|No| Z["return false (no-op)"]
D -->|Yes| E{"browserPortalOwningView\nnon-nil?"}
E -->|No| Z
E -->|Yes| F{"owningView === slot\nor isDescendant(of: slot)?"}
F -->|No| Z
F -->|Yes| G["window.makeFirstResponder(nil)"]
G --> H["First responder cleared\nWebKit stale UI prevented"]
Last reviewed commit: fce5545 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| window.firstResponder, | ||
| "Hiding a browser slot should yield any owned inspector responder before it goes off-screen" | ||
| ) | ||
| } |
There was a problem hiding this comment.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9b026f5-d9fa-4498-9fef-bef1ced5b412
📒 Files selected for processing (2)
Sources/BrowserWindowPortal.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
| 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" | ||
| ) | ||
| 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" | ||
| ) | ||
| } |
There was a problem hiding this comment.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fce5545675
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| XCTAssertTrue( | ||
| window.makeFirstResponder(inspectorView), | ||
| "Precondition failed: inspector probe should become first responder" |
There was a problem hiding this comment.
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 👍 / 👎.
…n-browser-slot-inspector-focus-crash Fix hidden browser slot inspector focus crash
This branch was successfully deployed
1 participant
Summary
WindowBrowserSlotViewhides or leaves its window so WebKit does not reactivate stale inspector UI on app activationTesting
./scripts/setup.sh./scripts/reload.sh --tag issue-1210-inspector-focus(build succeeded and launchedcmux DEV issue-1210-inspector-focus.app)Issues
Summary by cubic
Fix a focus crash when a hidden browser slot still owns first responder by yielding it when the slot hides or leaves its window, preventing stale WebKit inspector reactivation on app focus (closes #1210).
Written for commit fce5545. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests