Fix browser inspector responder crash on hide - #1178
lawrencecchen wants to merge 16 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds logic to yield owned first responder when portal slots hide or leave a window; implements local-inline WKWebView leasing, attach/reattach/refresh flows and a view reattach token to force SwiftUI reattachment after host transitions. Changes
Sequence Diagram(s)sequenceDiagram
participant Slot as WindowBrowserSlotView
participant Window as NSWindow
participant RC as ResponderChain
participant WV as WKWebView
Slot->>Slot: isHidden = true / viewWillMove(toWindow: nil)
Note right of Slot: detect leaving window or hidden state
Slot->>Window: yieldOwnedFirstResponderIfNeeded(reason: "slot hidden")
Window->>RC: query firstResponder
RC->>WV: if responder in slot hierarchy -> request resignFirstResponder
WV-->>RC: resign result
RC-->>Window: responder released
Window-->>Slot: confirmed
sequenceDiagram
participant Panel as BrowserPanelView
participant Coord as WebViewRepresentable.Coordinator
participant Slot as localInlineSlotView
participant WV as WKWebView
participant Host as HostContainerView
Panel->>Coord: set localInlineSlotView / update focus metadata
Coord->>Slot: attachLocalInlineHostedWebView(...)
Coord->>WV: cmuxReattachLocalHostRenderingState(reason)
WV->>Host: request layout/render refresh
Host->>WV: geometry/window changed -> retryDeferredAttachIfNeeded
WV-->>Coord: rendering & focus updated
Coord-->>Panel: sync state, maybe requestViewReattach()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 11491-11503: The test fails because
BrowserWindowPortalLifecycleTests.WKInspectorProbeView does not allow becoming
first responder; update the WKInspectorProbeView class to override
acceptsFirstResponder and return true so inspectorView can become first
responder in the test, ensuring window.makeFirstResponder(inspectorView) and the
subsequent window.firstResponder assertion can succeed; locate the
WKInspectorProbeView definition and add the acceptsFirstResponder override (and
optionally ensure any becomeFirstResponder-related behavior is handled
consistently).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac144355-5338-4c48-9558-5db0f6380eb7
📒 Files selected for processing (2)
Sources/BrowserWindowPortal.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
![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: cabdcbb4e2
ℹ️ 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".
Greptile SummaryThis PR fixes a crash where WebKit reactivates a stale inspector-owned first responder after a browser slot is hidden or removed from its window. The production fix in
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant WindowBrowserSlotView
participant NSWindow
participant WKInspectorResponder
Note over App,WKInspectorResponder: Bug scenario (before fix)
App->>WindowBrowserSlotView: isHidden = true
Note over WKInspectorResponder: Still first responder — WebKit crashes on reactivation
Note over App,WKInspectorResponder: Fixed flow (isHidden path)
App->>WindowBrowserSlotView: isHidden = true
WindowBrowserSlotView->>WindowBrowserSlotView: isHidden.didSet (isHidden=true, !oldValue)
WindowBrowserSlotView->>WindowBrowserSlotView: yieldOwnedFirstResponderIfNeeded(reason: "slotHidden")
WindowBrowserSlotView->>NSWindow: firstResponder (check ownership)
NSWindow-->>WindowBrowserSlotView: WKInspectorResponder (isDescendant of slot)
WindowBrowserSlotView->>NSWindow: makeFirstResponder(nil)
NSWindow-->>WKInspectorResponder: resignFirstResponder()
Note over App,WKInspectorResponder: Fixed flow (remove-from-window path)
App->>WindowBrowserSlotView: viewWillMove(toWindow: nil)
WindowBrowserSlotView->>WindowBrowserSlotView: yieldOwnedFirstResponderIfNeeded(reason: "slotWillLeaveWindow")
WindowBrowserSlotView->>NSWindow: makeFirstResponder(nil)
NSWindow-->>WKInspectorResponder: resignFirstResponder()
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/CmuxWebViewKeyEquivalentTests.swift">
<violation number="1" location="cmuxTests/CmuxWebViewKeyEquivalentTests.swift:9533">
P2: Avoid fixed RunLoop sleeps in this async test; it can be flaky when main-queue work is delayed. Wait on a predicate/expectation for the selector count instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
11612-11650:⚠️ Potential issue | 🟠 MajorMake this probe focusable and assert yield, not
nil.
BrowserWindowPortalLifecycleTests.WKInspectorProbeViewon Line 11276 still inheritsNSView’s defaultacceptsFirstResponder == false, so the precondition on Line 11638 fails before this regression path runs. Also, the behavior under test is that the hidden inspector stops owning first responder; requiringwindow.firstResponder == nilon Line 11646 is stricter than necessary and can vary with AppKit fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 11612 - 11650, Make the WKInspectorProbeView focusable by overriding acceptsFirstResponder to return true in the WKInspectorProbeView class so the precondition in testHidingBrowserSlotYieldsOwnedInspectorFirstResponder can succeed; then relax the postcondition in testHidingBrowserSlotYieldsOwnedInspectorFirstResponder by asserting the inspectorView no longer owns first responder (e.g. XCTAssertFalse(window.firstResponder === inspectorView) or XCTAssertNotEqual(window.firstResponder as? NSView, inspectorView)) instead of requiring window.firstResponder == nil.
🧹 Nitpick comments (2)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
9495-9536: Loosen these refresh assertions so harmless scheduling changes don't flap CI.Both tests hard-code the exact
firedSelectorssequence/count and a 60 ms run-loop delay. That couples them to the current immediate/async/delayed implementation rather than the stable contract, so a benign refactor to the refresh scheduling will fail CI even if local-inline hosting still reattaches correctly.Also applies to: 9538-9608
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 9495 - 9536, The test testLocalInlineHostedRefreshReattachesRenderingStateAcrossAsyncPasses hard-codes exact firedSelectors sequence/count and a fixed 60ms RunLoop sleep; change it to assert only the stable contract: after calling WebViewRepresentable.refreshLocalInlineHostedWebViewPresentation on ReattachProbeWebView, verify that the key selectors (e.g., "viewDidUnhide", "_enterInWindow", "_endDeferringViewInWindowChangesSync") appear in webView.firedSelectors (use contains/contains(where:) or subsequence checks) and that the total firedSelectors count increases (or is >= an expected minimum) rather than equals 9; replace the fixed RunLoop.current.run(until:) wait with a short polling/expectation loop that waits until the condition is met (with a reasonable timeout) so scheduling changes don't flake the test.Sources/Panels/BrowserPanelView.swift (1)
4548-4556: Consider documenting the delayed refresh timing.The
0.03second delay is a magic number. A brief comment explaining why this specific timing was chosen (e.g., "allow layout pass to complete" or "match WebKit rendering cycle") would help maintainability.📝 Suggested documentation
- DispatchQueue.main.asyncAfter(deadline: .now() + 0.03) { [weak webView, weak container] in + // Delayed pass allows WebKit's internal layout cycle to complete after reparenting. + DispatchQueue.main.asyncAfter(deadline: .now() + 0.03) { [weak webView, weak container] in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 4548 - 4556, Add a short inline comment above the DispatchQueue.main.asyncAfter call that explains why the 0.03 second delay is used (for example: to allow a layout pass or WebKit render cycle to complete before invoking Self.runLocalInlineHostedWebViewRefreshPass). Reference the surrounding symbols (DispatchQueue.main.asyncAfter, runLocalInlineHostedWebViewRefreshPass, webView, container) and state the intent (e.g., “delay to allow layout/rendering to settle”) so future maintainers understand the magic number and can adjust or justify it later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 22-28: Add an inline comment above the firedSelectors array
explaining each private WebKit selector's intent and the rendering state it
restores: describe that "viewDidUnhide" is used to trigger visibility/layout
sync after the web view becomes visible, "_enterInWindow" is used to
re-establish window-related view state when the view re-enters the window
hierarchy, and "_endDeferringViewInWindowChangesSync" is used to end any
deferred window-change batching so pending layout/paint updates are applied;
mention that these are optional private APIs checked via
cmuxLocalHostCallVoidIfAvailable and may disappear in future WebKit releases so
the comment helps future maintainers decide on fallback behavior if any selector
stops existing.
---
Duplicate comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 11612-11650: Make the WKInspectorProbeView focusable by overriding
acceptsFirstResponder to return true in the WKInspectorProbeView class so the
precondition in testHidingBrowserSlotYieldsOwnedInspectorFirstResponder can
succeed; then relax the postcondition in
testHidingBrowserSlotYieldsOwnedInspectorFirstResponder by asserting the
inspectorView no longer owns first responder (e.g.
XCTAssertFalse(window.firstResponder === inspectorView) or
XCTAssertNotEqual(window.firstResponder as? NSView, inspectorView)) instead of
requiring window.firstResponder == nil.
---
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 9495-9536: The test
testLocalInlineHostedRefreshReattachesRenderingStateAcrossAsyncPasses hard-codes
exact firedSelectors sequence/count and a fixed 60ms RunLoop sleep; change it to
assert only the stable contract: after calling
WebViewRepresentable.refreshLocalInlineHostedWebViewPresentation on
ReattachProbeWebView, verify that the key selectors (e.g., "viewDidUnhide",
"_enterInWindow", "_endDeferringViewInWindowChangesSync") appear in
webView.firedSelectors (use contains/contains(where:) or subsequence checks) and
that the total firedSelectors count increases (or is >= an expected minimum)
rather than equals 9; replace the fixed RunLoop.current.run(until:) wait with a
short polling/expectation loop that waits until the condition is met (with a
reasonable timeout) so scheduling changes don't flake the test.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 4548-4556: Add a short inline comment above the
DispatchQueue.main.asyncAfter call that explains why the 0.03 second delay is
used (for example: to allow a layout pass or WebKit render cycle to complete
before invoking Self.runLocalInlineHostedWebViewRefreshPass). Reference the
surrounding symbols (DispatchQueue.main.asyncAfter,
runLocalInlineHostedWebViewRefreshPass, webView, container) and state the intent
(e.g., “delay to allow layout/rendering to settle”) so future maintainers
understand the magic number and can adjust or justify it later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76eb30a0-5d86-4ade-a66b-4cde419c0c12
📒 Files selected for processing (2)
Sources/Panels/BrowserPanelView.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e86e08819d
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Panels/BrowserPanelView.swift (1)
4591-4599: Consider documenting the delayed refresh timing.The 0.03s delay is empirically chosen to stabilize rendering after reparent/focus churn. A brief comment explaining why this specific value was selected (e.g., "allows WebKit's internal layout pass to settle") would help future maintainers understand if adjustments are needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 4591 - 4599, Add a brief inline comment above the DispatchQueue.main.asyncAfter call explaining the 0.03s delay: note that this small delay (0.03 seconds) was chosen empirically to allow WebKit's internal layout/reparenting/focus churn to settle before calling Self.runLocalInlineHostedWebViewRefreshPass(webView,in:container,reason:phase:), so future maintainers understand the rationale and can adjust if stability changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 4591-4599: Add a brief inline comment above the
DispatchQueue.main.asyncAfter call explaining the 0.03s delay: note that this
small delay (0.03 seconds) was chosen empirically to allow WebKit's internal
layout/reparenting/focus churn to settle before calling
Self.runLocalInlineHostedWebViewRefreshPass(webView,in:container,reason:phase:),
so future maintainers understand the rationale and can adjust if stability
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f107f30-84c6-474b-bbb4-93b054db418f
📒 Files selected for processing (2)
Sources/Panels/BrowserPanelView.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09ca6298e3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4cd15509c
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Sources/Panels/BrowserPanel.swift (1)
1738-1743: Consider consolidating withPortalHostLease.
LocalInlineHostLeasehas identical fields toPortalHostLease(lines 1732-1737). If the separation is intentional for type safety between portal and local-inline contexts, this is fine. Otherwise, a singleHostLeasetype with a discriminator or typealias could reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 1738 - 1743, LocalInlineHostLease duplicates PortalHostLease fields; consolidate by creating a single HostLease struct (with fields hostId: ObjectIdentifier, paneId: UUID, inWindow: Bool, area: CGFloat) and replace usages of LocalInlineHostLease and PortalHostLease with HostLease, or if you need type distinction keep HostLease and add a small enum discriminator (e.g., kind: .portal | .localInline) or define typealiases PortalHostLease = HostLease and LocalInlineHostLease = HostLease to remove duplicated definitions; update all references to LocalInlineHostLease and PortalHostLease accordingly (search for those type names in the file and callers such as any factory/consumer functions).cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
9815-9840:Mirrorlabels here are still SDK internals.This helper now depends on child labels like
allowDeletevs_allowDeleteinside an Apple type. An Xcode/SDK update can rename those fields without changing drag behavior and break this test anyway. Prefer asserting through stable public behavior, or centralize this behind a single compatibility shim that skips when the labels are absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 9815 - 9840, The test relies on fragile Mirror child labels inside InternalTabDragConfigurationProvider (via dragOperationValue(_:labels:)) which are SDK internals and can change; replace these brittle label checks with a stable approach: either (a) exercise public API/behavior of configuration.operationsWithinApp and operationsOutsideApp (e.g., attempt to perform or query allowed NSDragOperation behavior via the public API) in testDisablesExternalOperationsForInternalTabDrags, or (b) centralize the reflection into a single compatibility shim function (keep dragOperationValue but make it return nil if none of the provided labels are present and have the test skip/assert appropriately), referencing dragOperationValue, testDisablesExternalOperationsForInternalTabDrags, InternalTabDragConfigurationProvider, operationsWithinApp and operationsOutsideApp so callers locate and update the usage.
🤖 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 9653-9694: The test
testLocalInlineHostedRefreshReattachesRenderingStateAcrossAsyncPasses is brittle
because it pins RunLoop.current.run(until: Date().addingTimeInterval(0.06)) and
asserts webView.firedSelectors.count == 9; instead, change the verification to
wait for the eventual postcondition (e.g., that the expected selector(s)
indicating the async reattach occurred are present) using an XCTestExpectation
or a short polling loop with a reasonable timeout, and remove the hardcoded
0.06s sleep and exact "9" count assertion; locate the test function and the call
to WebViewRepresentable.refreshLocalInlineHostedWebViewPresentation and replace
the fixed-runloop/count checks with an assertion that the async phase completed
(for example, that webView.firedSelectors contains the key selectors like
"_enterInWindow" or "_endDeferringViewInWindowChangesSync") within a timeout.
- Around line 9330-9344: ReattachProbeWebView currently overrides viewDidUnhide,
_enterInWindow, and _endDeferringViewInWindowChangesSync which masks the real
WebKit behavior; change the probe so it does NOT implement the underscored
selectors (remove or rename the `@objc` methods and the override) and instead
detect those calls by either (a) using a plain WKWebView instance for the
underscored-selector path, or (b) implementing test-only forwarding that calls
through to super (or uses method(for:) to invoke the real implementation) while
still recording the selector—ensure BrowserPanelView's selector checks
(responds(to:) and method(for:)) see the same behavior as a real WKWebView.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 4705-4728: The retryDeferredAttachIfNeeded closure can early-exit
when webView.superview === slotView even if the slot is zero-sized, preventing
recovery; fix by deferring the attach until the slot is properly sized: move or
add the slot size check (slotView.bounds.width/height > 1) before the guard that
returns on webView.superview === slotView so we only treat the view as "already
attached" when the slot has non-zero size; apply the same reorder/size-check
change to the other similar closure referenced around lines 4841-4848 (the other
retryDeferredAttachIfNeeded instance) and keep using the existing
claimLocalInlineHost, host.window, layoutSubtreeIfNeeded calls as before.
---
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 9815-9840: The test relies on fragile Mirror child labels inside
InternalTabDragConfigurationProvider (via dragOperationValue(_:labels:)) which
are SDK internals and can change; replace these brittle label checks with a
stable approach: either (a) exercise public API/behavior of
configuration.operationsWithinApp and operationsOutsideApp (e.g., attempt to
perform or query allowed NSDragOperation behavior via the public API) in
testDisablesExternalOperationsForInternalTabDrags, or (b) centralize the
reflection into a single compatibility shim function (keep dragOperationValue
but make it return nil if none of the provided labels are present and have the
test skip/assert appropriately), referencing dragOperationValue,
testDisablesExternalOperationsForInternalTabDrags,
InternalTabDragConfigurationProvider, operationsWithinApp and
operationsOutsideApp so callers locate and update the usage.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 1738-1743: LocalInlineHostLease duplicates PortalHostLease fields;
consolidate by creating a single HostLease struct (with fields hostId:
ObjectIdentifier, paneId: UUID, inWindow: Bool, area: CGFloat) and replace
usages of LocalInlineHostLease and PortalHostLease with HostLease, or if you
need type distinction keep HostLease and add a small enum discriminator (e.g.,
kind: .portal | .localInline) or define typealiases PortalHostLease = HostLease
and LocalInlineHostLease = HostLease to remove duplicated definitions; update
all references to LocalInlineHostLease and PortalHostLease accordingly (search
for those type names in the file and callers such as any factory/consumer
functions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58df16ae-e0f6-4e21-879b-d875c4bdb049
📒 Files selected for processing (4)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Workspace.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd399f1afb
ℹ️ 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".
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/BrowserPanelView.swift">
<violation number="1" location="Sources/Panels/BrowserPanelView.swift:4770">
P1: `onGeometryChanged` claims the local-inline host without checking current pane ownership, so stale host callbacks can steal the lease from the active pane.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (3)
9442-9456:⚠️ Potential issue | 🟠 MajorThis probe is exercising its own selector stubs, not WebKit.
BrowserPanelHostContainerViewTests.ReattachProbeWebViewdefines the same selectors the refresh helper probes and calls, andviewDidUnhide()never reachessuper. These tests can pass by hitting the probe's methods instead of the realWKWebViewpath. Use a plainWKWebView, or only record public hooks while forwarding tosuper.#!/bin/bash set -euo pipefail echo "Probe subclass under test:" sed -n '9442,9456p' cmuxTests/CmuxWebViewKeyEquivalentTests.swift echo echo "Selector-driven refresh code in BrowserPanel.swift:" rg -n -C3 'runLocalInlineHostedWebViewRefreshPass|responds\(to:|method\(for:|viewDidUnhide|_enterInWindow|_endDeferringViewInWindowChangesSync' Sources/Panels/BrowserPanel.swift🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 9442 - 9456, The probe subclass ReattachProbeWebView is shadowing WebKit selectors (viewDidUnhide, _enterInWindow, _endDeferringViewInWindowChangesSync) so tests hit the probe's stubs instead of real WKWebView behaviour; fix by replacing ReattachProbeWebView with a plain WKWebView in the test or modify ReattachProbeWebView to forward to super for those selectors (call super.viewDidUnhide() in viewDidUnhide and invoke the superclass implementations for _enterInWindow and _endDeferringViewInWindowChangesSync) while still appending to firedSelectors so only public hooks are recorded and the real WebKit paths are exercised.
9798-9805:⚠️ Potential issue | 🟡 MinorThese async assertions are pinned to the current scheduler.
Sleeping for
0.06and expecting exactly9callbacks bakes in today's immediate/async/delayed refresh shape. A slower host or an extra/coalesced pass will fail these tests without a behavior regression; wait for the eventual postcondition instead of the exact pass count.Also applies to: 9857-9877
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 9798 - 9805, The test currently pins async behavior by calling RunLoop.current.run(until: Date().addingTimeInterval(0.06)) and asserting an exact firedSelectors.count of 9; change this to an eventual-wait pattern: create an XCTestExpectation and use XCTNSPredicateExpectation or XCTWaiter to wait until webView.firedSelectors.count is at least (or contains) the expected events (e.g., >= 9 or contains the specific selector names), replacing the fixed sleep and exact-count assertion; locate the assertions around webView.firedSelectors and the RunLoop call in CmuxWebViewKeyEquivalentTests (the block that asserts firedSelectors and calls RunLoop.current.run) and update them to wait on the predicate with a reasonable timeout instead of relying on Date().addingTimeInterval(0.06).
11918-11929:⚠️ Potential issue | 🟠 MajorThis regression still never makes the inspector probe first responder.
BrowserWindowPortalLifecycleTests.WKInspectorProbeViewis still a plainNSView, sowindow.makeFirstResponder(inspectorView)returns false and the hide path never runs. The override added earlier in the file needs to be applied to this probe class too.#!/bin/bash set -euo pipefail echo "BrowserWindowPortalLifecycleTests probe definition:" sed -n '11546,11558p' cmuxTests/CmuxWebViewKeyEquivalentTests.swift echo echo "Regression test block:" sed -n '11892,11929p' cmuxTests/CmuxWebViewKeyEquivalentTests.swiftPossible fix
-private final class WKInspectorProbeView: NSView {} +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 11918 - 11929, The test fails because BrowserWindowPortalLifecycleTests.WKInspectorProbeView is still a plain NSView so window.makeFirstResponder(inspectorView) returns false; update WKInspectorProbeView to mirror the earlier probe override by subclassing NSView (or editing the existing class) and adding the first-responder overrides used elsewhere: override var acceptsFirstResponder: Bool { true } and override func becomeFirstResponder() -> Bool { true } (and optionally override resignFirstResponder to return true) so the inspector probe can become first responder and the hide-path assertion runs.
🧹 Nitpick comments (2)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
5545-5562: Assert the reattach result, not the token bump.This only proves
viewReattachTokenincrements. If the representable stops consuming that token, the test still passes while the moved browser never reattaches. Please drive a mountedWebViewRepresentableand assert theWKWebViewactually lands in the destination pane aftermoveTab.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 5545 - 5562, The test currently only verifies that browserSplitPanel.viewReattachToken increments when calling workspace.bonsplitController.moveTab(browserTabId, toPane: originalPaneId) but does not verify the representable actually reattaches its WKWebView; update the test to drive a mounted WebViewRepresentable (the component that consumes viewReattachToken) after moveTab and assert that the underlying WKWebView instance is now hosted in the destination pane: use the WebViewRepresentable mounting helper used elsewhere in tests to locate the active WKWebView for browserTabId and assert workspace.paneId(forPanelId: browserSplitPanel.id) resolves to originalPaneId and that the found WKWebView’s window/panel matches that pane, ensuring the reattach result is asserted instead of only the token bump.Sources/Panels/BrowserPanelView.swift (1)
4591-4621: Consider extracting the delayed refresh interval to a named constant.The 0.03-second (30ms) delay in the delayed refresh pass is a magic number. While the triple-pass refresh strategy (immediate, async, delayed) is a solid resilience pattern for WebKit rendering recovery, extracting this to a named constant would improve maintainability and make the intent clearer.
💡 Suggested refactor
+ private static let localInlineRefreshDelaySeconds: TimeInterval = 0.03 + static func refreshLocalInlineHostedWebViewPresentation( _ webView: WKWebView, in container: WindowBrowserSlotView, reason: String ) { guard !container.isHidden else { return } Self.runLocalInlineHostedWebViewRefreshPass( webView, in: container, reason: reason, phase: "immediate" ) DispatchQueue.main.async { [weak webView, weak container] in guard let webView, let container else { return } Self.runLocalInlineHostedWebViewRefreshPass( webView, in: container, reason: reason, phase: "async" ) } - DispatchQueue.main.asyncAfter(deadline: .now() + 0.03) { [weak webView, weak container] in + DispatchQueue.main.asyncAfter(deadline: .now() + localInlineRefreshDelaySeconds) { [weak webView, weak container] in guard let webView, let container else { return } Self.runLocalInlineHostedWebViewRefreshPass( webView, in: container, reason: reason, phase: "delayed" ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 4591 - 4621, The 0.03s magic delay in refreshLocalInlineHostedWebViewPresentation should be extracted to a named static constant (e.g., static let delayedRefreshInterval: TimeInterval = 0.03) so the intent is clear and maintainable; update the DispatchQueue.main.asyncAfter call to use this constant and keep the rest of the triple-pass calls to runLocalInlineHostedWebViewRefreshPass unchanged so callers (refreshLocalInlineHostedWebViewPresentation and runLocalInlineHostedWebViewRefreshPass) continue to work the same but with a readable, single source of truth for the delayed interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 9442-9456: The probe subclass ReattachProbeWebView is shadowing
WebKit selectors (viewDidUnhide, _enterInWindow,
_endDeferringViewInWindowChangesSync) so tests hit the probe's stubs instead of
real WKWebView behaviour; fix by replacing ReattachProbeWebView with a plain
WKWebView in the test or modify ReattachProbeWebView to forward to super for
those selectors (call super.viewDidUnhide() in viewDidUnhide and invoke the
superclass implementations for _enterInWindow and
_endDeferringViewInWindowChangesSync) while still appending to firedSelectors so
only public hooks are recorded and the real WebKit paths are exercised.
- Around line 9798-9805: The test currently pins async behavior by calling
RunLoop.current.run(until: Date().addingTimeInterval(0.06)) and asserting an
exact firedSelectors.count of 9; change this to an eventual-wait pattern: create
an XCTestExpectation and use XCTNSPredicateExpectation or XCTWaiter to wait
until webView.firedSelectors.count is at least (or contains) the expected events
(e.g., >= 9 or contains the specific selector names), replacing the fixed sleep
and exact-count assertion; locate the assertions around webView.firedSelectors
and the RunLoop call in CmuxWebViewKeyEquivalentTests (the block that asserts
firedSelectors and calls RunLoop.current.run) and update them to wait on the
predicate with a reasonable timeout instead of relying on
Date().addingTimeInterval(0.06).
- Around line 11918-11929: The test fails because
BrowserWindowPortalLifecycleTests.WKInspectorProbeView is still a plain NSView
so window.makeFirstResponder(inspectorView) returns false; update
WKInspectorProbeView to mirror the earlier probe override by subclassing NSView
(or editing the existing class) and adding the first-responder overrides used
elsewhere: override var acceptsFirstResponder: Bool { true } and override func
becomeFirstResponder() -> Bool { true } (and optionally override
resignFirstResponder to return true) so the inspector probe can become first
responder and the hide-path assertion runs.
---
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 5545-5562: The test currently only verifies that
browserSplitPanel.viewReattachToken increments when calling
workspace.bonsplitController.moveTab(browserTabId, toPane: originalPaneId) but
does not verify the representable actually reattaches its WKWebView; update the
test to drive a mounted WebViewRepresentable (the component that consumes
viewReattachToken) after moveTab and assert that the underlying WKWebView
instance is now hosted in the destination pane: use the WebViewRepresentable
mounting helper used elsewhere in tests to locate the active WKWebView for
browserTabId and assert workspace.paneId(forPanelId: browserSplitPanel.id)
resolves to originalPaneId and that the found WKWebView’s window/panel matches
that pane, ensuring the reattach result is asserted instead of only the token
bump.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 4591-4621: The 0.03s magic delay in
refreshLocalInlineHostedWebViewPresentation should be extracted to a named
static constant (e.g., static let delayedRefreshInterval: TimeInterval = 0.03)
so the intent is clear and maintainable; update the
DispatchQueue.main.asyncAfter call to use this constant and keep the rest of the
triple-pass calls to runLocalInlineHostedWebViewRefreshPass unchanged so callers
(refreshLocalInlineHostedWebViewPresentation and
runLocalInlineHostedWebViewRefreshPass) continue to work the same but with a
readable, single source of truth for the delayed interval.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c547d13-72b7-4e65-9ee3-3f3a8ec62f6d
📒 Files selected for processing (3)
Sources/BrowserWindowPortal.swiftSources/Panels/BrowserPanelView.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
|
@codex review |
…-activation-crash # Conflicts: # cmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13e216ca38
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c480a179b
ℹ️ 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".
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
./scripts/setup.sh./scripts/reload.sh --tag task-webkit-inspector-activation-crashSummary by cubic
Fixes a crash when hiding a browser slot while the WebKit Inspector is first responder. Hardens local inline DevTools hosting with lease-based ownership, explicit pane handoff, focus- and token-gated reattach, and layout-preserving reparent to avoid jank.
WindowBrowserSlotViewyields its owned first responder on hide and when leaving its window.prepareLocalInlineHostReplacementForNextDistinctClaim; prevents off-window hosts from stealing DevTools and retries attach when a host joins a window; honors token-gated refreshes withviewReattachToken, reattaches after tab moves and when the window becomes key, and refreshes after focus transitions without reparent; repairs visibility/in-window state with immediate/async/delayed passes; moves non-WebKit DevTools companion views with the web view during reparent; preserves hosted subview frames across reparent via a normalized layout snapshot.Written for commit 70bebeb. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests