Fix Browser DevTools host reconciliation#4980
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors Developer Tools lifecycle into explicit phases, centralizes host-update reconciliation, tightens detached DevTools window detection and close routing, updates host-binding logic in both local-inline and window-portal flows, and adds regression tests for close and host-update scenarios. ChangesDevTools Lifecycle & Host Update Reconciliation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1bdb357. Configure here.
Greptile SummaryThis PR fixes two related DevTools bugs: detached inspector-window close actions are now routed through the owning panel's inspector frontend (not a
Confidence Score: 4/5Safe to merge; the lifecycle state machine is internally consistent and the detached-close routing fix is well-scoped. The reconcile logic correctly gates manual-close recording behind allowsHostHiddenManualClose (phase .visible only) and !isDeveloperToolsTransitionInFlight, so in-flight transitions cannot be silently killed. The one concern is that markDeveloperToolsLifecycleVisible() has no guard on preferredDeveloperToolsVisible, leaving a narrow window where phase.preservesVisibleIntent = true while preferred = false could spuriously enable the local-inline host path — the state is representable even if no current call site produces it. Sources/Panels/BrowserPanel.swift — effectiveDeveloperToolsVisibilityIntent() and shouldUseLocalInlineDeveloperToolsHosting() dual-intent; cmuxTests/BrowserConfigTests.swift — 50 ms RunLoop budgets in testLocalInlineHostUpdateRespectsManualDeveloperToolsClose. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SwiftUI updateNSView] --> B{Local inline or portal?}
B -- local inline --> C[capture wasDeveloperToolsVisibleBeforeHostUpdate]
B -- portal --> D[capture wasDeveloperToolsVisibleBeforeHostUpdate]
C --> E[reconcileDeveloperToolsAfterHostUpdate]
D --> F{portalHostAccepted & window != nil?}
F -- yes --> E
F -- no --> G[skip reconcile]
E --> H{webView attached to window?}
H -- no --> I[syncDeveloperToolsPreferenceFromInspector preserveIntent=true]
H -- yes --> J[noteDeveloperToolsHostAttached]
J --> K[compute hasPendingRestore & shouldRestore]
K --> L{wasVisible & stable host & phase==.visible & no transition & !hasPendingRestore?}
L -- yes --> M[recordManualClose preferred=false phase=.hidden]
L -- no --> N{shouldRestore?}
N -- yes --> O[restoreDeveloperToolsAfterAttachIfNeeded]
N -- no --> P[syncDeveloperToolsPreferenceFromInspector]
O --> Q{inspector visible?}
Q -- yes --> R[markLifecycleVisible]
Q -- no --> S[markLifecyclePendingVisible scheduleDeveloperToolsRestoreRetry]
style M fill:#f99,stroke:#c00
style R fill:#9f9,stroke:#090
style I fill:#ff9,stroke:#990
Reviews (2): Last reviewed commit: "fix: scope detached devtools windows to ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if wasVisibleBeforeHostUpdate, | ||
| !isDeveloperToolsVisible(), | ||
| !didAttachHost, | ||
| !didChangeHostVisibility, | ||
| !hasPendingRestore { |
There was a problem hiding this comment.
hasPendingRestore covers forceDeveloperToolsRefreshOnNextAttach and developerToolsRestoreRetryWorkItem, but not isDeveloperToolsTransitionInFlight (developerToolsTransitionSettleWorkItem != nil). If a stable SwiftUI host re-render lands in the narrow window between enqueueDeveloperToolsVisibilityTransition(to: true) being called and the inspector confirming isVisible == true, the combination of wasVisibleBeforeHostUpdate = true and !isDeveloperToolsVisible() will trigger recordDeveloperToolsManualCloseDuringStableHostUpdate(), cancelling the settle work item and setting preferredDeveloperToolsVisible = false — effectively killing an in-flight reveal that the user initiated.
| if wasVisibleBeforeHostUpdate, | |
| !isDeveloperToolsVisible(), | |
| !didAttachHost, | |
| !didChangeHostVisibility, | |
| !hasPendingRestore { | |
| if wasVisibleBeforeHostUpdate, | |
| !isDeveloperToolsVisible(), | |
| !didAttachHost, | |
| !didChangeHostVisibility, | |
| !hasPendingRestore, | |
| !isDeveloperToolsTransitionInFlight { |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6160-6173: The per-panel detached DevTools enumeration is too
broad because isDetachedDeveloperToolsWindow(_:) falls back to
Self.isDetachedInspectorWindow(_:), causing detachedDeveloperToolsWindows() to
pick up other panels' inspectors; change the logic so the panel only considers
windows that satisfy detachedDeveloperToolsWindowBelongsToPanel(_:). Concretely,
remove or stop using the Self.isDetachedInspectorWindow(window) fallback in
isDetachedDeveloperToolsWindow(_:) (or, alternatively, change
detachedDeveloperToolsWindows() to filter solely with
detachedDeveloperToolsWindowBelongsToPanel(_:)) so only this panel's detached
developer tools are returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9db83708-3283-44de-ba0e-fb5887bac2ea
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftcmuxTests/BrowserConfigTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6454-6472: The current logic finalizes a queued hide immediately
when isDeveloperToolsVisible() is false even if an earlier show() is still
in-flight; update the guard branches (the block using pendingTargetVisible,
isDeveloperToolsVisible(), preferredDeveloperToolsVisible and the calls to
markDeveloperToolsLifecycleHidden/Visible/PendingVisible and
performDeveloperToolsVisibilityTransition) to first check for any in-flight
show/open operation (e.g. an "inFlightShow"/"isOpeningDeveloperTools"/lifecycle
state indicating pending visible) and if such an in-flight show exists, do not
mark lifecycle hidden nor finalize the queued hide—either preserve or re-queue
the pending hide (keep lifecycle pending) so the earlier show completion can
take effect; otherwise proceed with the existing calls to
markDeveloperToolsLifecycleHidden/Visible or
performDeveloperToolsVisibilityTransition as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9daa2f3-2671-4d2e-a409-ead279125323
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftcmuxTests/BrowserConfigTests.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/Panels/BrowserPanel.swift (1)
6679-6710:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQueued hide can still lose to an earlier async open.
Line 6708 avoids classifying one host-update path as a manual close, but the earlier race is still reachable: if
hideis queued during an in-flightshow()andfinishDeveloperToolsTransition()runs while the inspector is still hidden, the code finalizes.hiddenwithout ever applying the queued conceal. A late completion of the earliershow()can still reopen DevTools after the user just closed them. Keep the hide pending until the open settles, or explicitly conceal once the inspector eventually surfaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0118b1f-9494-4be9-80b0-9bc01f5efa3f
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftcmuxTests/BrowserConfigTests.swift
1 participant
Fixes #2933
Summary:
Tests:
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches WebKit inspector visibility, window close interception, and portal/local-inline hosting paths where incorrect state could reopen DevTools or leave stray windows; scope is browser-panel–local with added regression tests.
Overview
DevTools close routing and host reconciliation are reworked so panel ownership and lifecycle state drive behavior instead of window titles or unconditional restore.
Detached closes no longer depend on a
Web Inspectortitle prefix:AppDelegateforwards close actions to browser panels, which match detached windows via the owning inspector frontend (detachedDeveloperToolsWindowBelongsToPanel). Stray detached windows are dismissed only when they belong to that panel, avoiding closing another panel’s detached inspector.A
DeveloperToolsLifecyclePhase(hidden/opening/visible/closing/restoring) centralizes visible intent, pending opens during host churn, and when a stable host hide counts as a manual close.reconcileDeveloperToolsAfterHostUpdatereplaces always calling restore after local-inline and portal host updates: restore when attaching or visibility changes; record manual close when DevTools were visible, the host is stable, and WebKit reports hidden; otherwise sync preference while preserving intent when detached or mid-transition.BrowserPanelViewpasses host-attach and visibility deltas into that reconcile path instead ofrestoreDeveloperToolsAfterAttachIfNeededalone.Reviewed by Cursor Bugbot for commit f23fc90. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes DevTools close routing and host reconciliation in Browser panels. Centralizes lifecycle so pending opens survive host churn, while stable host hides are treated as manual closes and other panels’ detached windows aren’t touched.
AppDelegateforwards actions and panels determine ownership (no window title heuristics).Written for commit f23fc90. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests