Repository navigation
devtools resize layout - #1189
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughIntroduces hosted-inspector width tracking, drag-state guards, and size-approximate throttling across portal/host/slot/panel/view layers; adds DevTools presentation state (attached/detached) with grace-period handling, preferred-width persistence APIs, debounced divider reapply, and expanded debug instrumentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Slot as WindowBrowserSlotView
participant Host as WindowBrowserHostView
participant Portal as WindowBrowserPortal
User->>Slot: mouseDown (divider)
Slot->>Slot: isHostedInspectorDividerDragActive = true
User->>Slot: mouseDragged
Slot->>Host: validate drag state
alt invalid
Host->>Slot: clear isHostedInspectorDividerDragActive
Slot->>Slot: return early
else valid
Slot->>Portal: request inspector width apply
Portal->>Portal: sizeApproximatelyEqual(newSize, lastHostedInspectorLayoutBoundsSize)
alt changed
Portal->>Host: reapplyHostedInspectorDividerIfNeeded -> true
Host->>Portal: update frames, notify
else unchanged
Portal->>Portal: skip re-layout
end
end
User->>Slot: mouseUp
Slot->>Slot: isHostedInspectorDividerDragActive = false
sequenceDiagram
participant UI as DevTools UI
participant Panel as BrowserPanel
participant View as BrowserPanelView
participant Observer as Window Close Observer
UI->>Panel: revealDeveloperTools(inspector)
Panel->>Panel: syncDeveloperToolsPresentationPreferenceFromUI()
alt presentation -> detached
Panel->>Observer: installDetachedDeveloperToolsWindowCloseObserver()
Observer->>Panel: window closed -> schedule dismissal
else presentation -> attached
Panel->>View: prepareDeveloperToolsForRevealIfNeeded()
View->>View: recordPreferredAttachedDeveloperToolsWidth(...)
end
UI->>Panel: toggleDeveloperTools()
Panel->>Panel: setPreferredDeveloperToolsPresentation(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
Sources/Panels/BrowserPanel.swift (1)
4132-4144: Consider extracting shared inspector view helpers.The
visibleDescendants(in:)andisInspectorView(_:)methods are duplicated acrossBrowserPanel.swift,BrowserWindowPortal.swift, andBrowserPanelView.swift. While encapsulation justifies some duplication, consolidating these into a shared internal utility (e.g.,InspectorViewHelpers) would reduce maintenance overhead if the detection logic needs to evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 4132 - 4144, The duplicated logic in visibleDescendants(in:) and isInspectorView(_:) should be consolidated into a single internal helper type (e.g., InspectorViewHelpers) with static methods visibleDescendants(in:) and isInspectorView(_:), move the existing implementations there, make it internal, and update all call sites in BrowserPanel, BrowserWindowPortal, and BrowserPanelView to call InspectorViewHelpers.visibleDescendants(in:) and InspectorViewHelpers.isInspectorView(_:) so future changes live in one place.Sources/Panels/BrowserPanelView.swift (2)
4007-4013: Simplified mouse event handlers delegate tosuper.The custom hosted inspector divider drag handling has been removed. Both
mouseDownandmouseDraggednow simply callsuper, delegating event handling to WebKit's native inspector implementation. This aligns with the commit message about routing inspector hits to native WebKit.Note that
hostedInspectorDividerDragstate and related drag computation code (e.g.,HostedInspectorDividerDragState) appear to be unused after this change. Consider removing the dead code if native WebKit drag handling is the intended long-term approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 4007 - 4013, mouseDown(with:) and mouseDragged(with:) were simplified to call super, but leftover state and helper code for the custom inspector divider drag (e.g., the hostedInspectorDividerDrag property, HostedInspectorDividerDragState type, and any functions that compute drag/resize for the hosted inspector) are now unused and should be removed; locate and delete the HostedInspectorDividerDragState declaration, the hostedInspectorDividerDrag property/variable, and any methods referenced only by the old drag handling (search for symbols like hostedInspectorDividerDrag, HostedInspectorDividerDragState, computeHostedInspectorDrag, or similar) and remove their usages and tests, or mark them as deprecated if you need to preserve behavior temporarily.
4187-4196: Debounced divider reapply scheduling.The
scheduleHostedInspectorDividerReapplymethod properly cancels pending work before scheduling new work, providing effective debouncing. The method name says "Reapply" but callscaptureHostedInspectorPreferredWidthFromCurrentLayout- this is intentional as it captures the current WebKit-managed layout state rather than applying a stored preference.Consider renaming to
scheduleHostedInspectorWidthCapturefor clarity, though this is a minor nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 4187 - 4196, Rename the misleading method scheduleHostedInspectorDividerReapply to scheduleHostedInspectorWidthCapture to match its actual behavior (it captures WebKit-managed layout via captureHostedInspectorPreferredWidthFromCurrentLayout rather than reapplying a stored divider), and update all references/usages (calls, delegates, unit tests, and any documentation/comments) to the new name while preserving the current implementation and semantics (including hostedInspectorReapplyWorkItem handling and DispatchWorkItem logic); also update any related comment text to reflect “width capture” wording.Sources/BrowserWindowPortal.swift (3)
170-171: Consider removing the unusedminimumInspectorWidthparameter.The parameter is explicitly marked as unused (
_: CGFloat) and always passed as0at the call site (line 966). If minimum width enforcement is no longer needed, consider removing this parameter to clean up the API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` around lines 170 - 171, The initializer/method currently takes an unused parameter named minimumInspectorWidth (_: CGFloat); remove this parameter from the signature (and any internal placeholder) and delete its argument at all call sites (e.g., the call that passes 0), then update any related documentation/comments and tests that referenced minimumInspectorWidth to reflect the simplified API (look for the parameter name minimumInspectorWidth and the function/initializer that includes inspectorFrame: NSRect to locate the code).
987-987: Minor: String-based detection for live drag state.The
isLiveDragflag is determined by comparingreason == "drag". This is somewhat fragile as it depends on the exact string passed from callers. Consider using a dedicated boolean parameter for more explicit control.♻️ Alternative approach
private func applyHostedInspectorDividerWidth( _ preferredWidth: CGFloat, to hit: HostedInspectorDividerHit, - reason: String + reason: String, + isLiveDrag: Bool = false ) -> (pageFrame: NSRect, inspectorFrame: NSRect) { ... - let isLiveDrag = reason == "drag" ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` at line 987, Replace the fragile string check for live-drag with an explicit boolean parameter: stop deriving isLiveDrag via reason == "drag" and instead add a Bool parameter (e.g., isLiveDrag or isDragging) to the BrowserWindowPortal method that currently accepts reason and defines isLiveDrag, update the method body to use that boolean, and update all callers to pass the new flag (or provide an overload/optional default to preserve compatibility). Ensure you remove the string comparison and any callers that relied on "drag" as a sentinel and update tests/usages accordingly.
1081-1084: Consider extracting duplicatesizeApproximatelyEqualhelpers to a shared utility.The
sizeApproximatelyEqualhelper is duplicated identically in bothWindowBrowserHostView(lines 1081-1084) andWindowBrowserSlotView(lines 1609-1612). Similarly,rectApproximatelyEqualis duplicated across multiple classes in this file and inBrowserPanel.swift.Consider extracting these to a shared extension or utility function to reduce duplication.
♻️ Suggested extraction
// Could be added as a private extension at file scope or in a shared utilities file private extension NSSize { func approximatelyEquals(_ other: NSSize, epsilon: CGFloat = 0.5) -> Bool { abs(width - other.width) <= epsilon && abs(height - other.height) <= epsilon } } private extension NSRect { func approximatelyEquals(_ other: NSRect, epsilon: CGFloat = 0.5) -> Bool { abs(origin.x - other.origin.x) <= epsilon && abs(origin.y - other.origin.y) <= epsilon && abs(size.width - other.size.width) <= epsilon && abs(size.height - other.size.height) <= epsilon } }Also applies to: 1609-1612
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` around lines 1081 - 1084, The duplicate helpers sizeApproximatelyEqual and rectApproximatelyEqual (used in WindowBrowserHostView, WindowBrowserSlotView and BrowserPanel) should be extracted into a single shared utility — e.g. create private extensions on NSSize and NSRect (or a small Utilities file) that add approximatelyEquals(_:, epsilon:) methods and replace all occurrences of sizeApproximatelyEqual and rectApproximatelyEqual to call these extensions; update callers in WindowBrowserHostView and WindowBrowserSlotView (and BrowserPanel) to use the new methods to remove duplication and keep the same epsilon semantics.
🤖 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/BrowserWindowPortal.swift`:
- Around line 170-171: The initializer/method currently takes an unused
parameter named minimumInspectorWidth (_: CGFloat); remove this parameter from
the signature (and any internal placeholder) and delete its argument at all call
sites (e.g., the call that passes 0), then update any related
documentation/comments and tests that referenced minimumInspectorWidth to
reflect the simplified API (look for the parameter name minimumInspectorWidth
and the function/initializer that includes inspectorFrame: NSRect to locate the
code).
- Line 987: Replace the fragile string check for live-drag with an explicit
boolean parameter: stop deriving isLiveDrag via reason == "drag" and instead add
a Bool parameter (e.g., isLiveDrag or isDragging) to the BrowserWindowPortal
method that currently accepts reason and defines isLiveDrag, update the method
body to use that boolean, and update all callers to pass the new flag (or
provide an overload/optional default to preserve compatibility). Ensure you
remove the string comparison and any callers that relied on "drag" as a sentinel
and update tests/usages accordingly.
- Around line 1081-1084: The duplicate helpers sizeApproximatelyEqual and
rectApproximatelyEqual (used in WindowBrowserHostView, WindowBrowserSlotView and
BrowserPanel) should be extracted into a single shared utility — e.g. create
private extensions on NSSize and NSRect (or a small Utilities file) that add
approximatelyEquals(_:, epsilon:) methods and replace all occurrences of
sizeApproximatelyEqual and rectApproximatelyEqual to call these extensions;
update callers in WindowBrowserHostView and WindowBrowserSlotView (and
BrowserPanel) to use the new methods to remove duplication and keep the same
epsilon semantics.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 4132-4144: The duplicated logic in visibleDescendants(in:) and
isInspectorView(_:) should be consolidated into a single internal helper type
(e.g., InspectorViewHelpers) with static methods visibleDescendants(in:) and
isInspectorView(_:), move the existing implementations there, make it internal,
and update all call sites in BrowserPanel, BrowserWindowPortal, and
BrowserPanelView to call InspectorViewHelpers.visibleDescendants(in:) and
InspectorViewHelpers.isInspectorView(_:) so future changes live in one place.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 4007-4013: mouseDown(with:) and mouseDragged(with:) were
simplified to call super, but leftover state and helper code for the custom
inspector divider drag (e.g., the hostedInspectorDividerDrag property,
HostedInspectorDividerDragState type, and any functions that compute drag/resize
for the hosted inspector) are now unused and should be removed; locate and
delete the HostedInspectorDividerDragState declaration, the
hostedInspectorDividerDrag property/variable, and any methods referenced only by
the old drag handling (search for symbols like hostedInspectorDividerDrag,
HostedInspectorDividerDragState, computeHostedInspectorDrag, or similar) and
remove their usages and tests, or mark them as deprecated if you need to
preserve behavior temporarily.
- Around line 4187-4196: Rename the misleading method
scheduleHostedInspectorDividerReapply to scheduleHostedInspectorWidthCapture to
match its actual behavior (it captures WebKit-managed layout via
captureHostedInspectorPreferredWidthFromCurrentLayout rather than reapplying a
stored divider), and update all references/usages (calls, delegates, unit tests,
and any documentation/comments) to the new name while preserving the current
implementation and semantics (including hostedInspectorReapplyWorkItem handling
and DispatchWorkItem logic); also update any related comment text to reflect
“width capture” wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7eba51bb-f500-4180-a798-4959e8a190a7
📒 Files selected for processing (3)
Sources/BrowserWindowPortal.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swift
Contributor
Greptile SummaryThis PR refactors the devtools (WebKit inspector) resize layout across three files, addressing proportional resizing on window resize, drag-handling consolidation, and attached vs. detached presentation tracking. Key behavioral changes:
Issues found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User drags inspector divider] --> B[WindowBrowserHostView\nmouseDown / mouseDragged]
B --> C[Set isHostedInspectorDividerDragActive = true\non WindowBrowserSlotView]
C --> D[clampedDividerX\nrespects minimumInspectorWidth]
D --> E[recordPreferredHostedInspectorWidth\nstores absolute + fraction]
E --> F[applyHostedInspectorDividerWidth\nresizedFrames from containerBounds]
F --> G[Set needsDisplay on\npage + inspector + container + slot]
G --> H[mouseUp → isHostedInspectorDividerDragActive = false]
H --> I{Portal sync\ntriggered?}
I -- drag still active --> J[refreshHostedWebViewPresentation\nSKIPPED]
I -- drag complete --> K[reapplyHostedInspectorDividerIfNeeded\nuses resolvedPreferredHostedInspectorWidth\nfraction × containerBounds.width]
L[Window resize / layout] --> M{bounds size changed?}
M -- No change ε=0.5 --> N[SKIP reapply]
M -- Changed --> O[captureHostedInspectorPreferredWidthFromCurrentLayout\nreads current inspector frame,\nupdates stored fraction]
P[DevTools toggle] --> Q{Current presentation?}
Q -- unknown --> R[prepareDeveloperToolsForRevealIfNeeded\nattach inspector]
Q -- attached --> S[scheduleDetachedWindowDismissal\nif shouldDismiss]
Q -- detached --> T[skip dismissal,\nset grace deadline]
R --> U[syncDeveloperToolsPresentationPreferenceFromUI\nupdate .attached / .detached]
S --> U
T --> U
Last reviewed commit: 8b66628 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| hit.inspectorView.needsLayout = true | ||
| hit.containerView.needsLayout = true | ||
| hit.slotView.needsLayout = true | ||
| let isLiveDrag = reason == "drag" |
Contributor
There was a problem hiding this comment.
isLiveDrag unused in non-debug builds
isLiveDrag is declared outside the #if DEBUG block but is only referenced inside it. In non-debug (release) builds the compiler will emit an "immutable value 'isLiveDrag' was never used" warning. The same pattern exists in BrowserPanelView.swift at line 4280.
Move the declaration inside the conditional compilation block to eliminate the warning:
Suggested change
| let isLiveDrag = reason == "drag" | |
| #if DEBUG | |
| let isLiveDrag = reason == "drag" | |
| dlog( |
Then adjust the closing #endif accordingly to remove the separate dlog block.
Comment on lines
+171
to
172
| minimumInspectorWidth _: CGFloat | ||
| ) -> (pageFrame: NSRect, inspectorFrame: NSRect) { |
Contributor
There was a problem hiding this comment.
minimumInspectorWidth parameter is silently discarded
The parameter minimumInspectorWidth _: CGFloat is accepted by the function signature but the wildcard _ makes it immediately inaccessible — all logic has been rewritten to ignore it entirely. Both current call-sites pass 0, so there is no functional impact, but this leaves the API misleading: callers see a minimumInspectorWidth argument they can tune, while the implementation ignores it.
Consider one of:
- Removing the parameter from the signature entirely (and updating the two call-sites that pass
0) - Or re-introducing the minimum-width clamping logic so the parameter has the expected effect
Suggested change
| minimumInspectorWidth _: CGFloat | |
| ) -> (pageFrame: NSRect, inspectorFrame: NSRect) { | |
| minimumInspectorWidth _: CGFloat = 0 |
(Providing a default at least makes it clearer the value is unused, though removing it outright is cleaner.)
Comment on lines
+3243
to
3245
| func preferredAttachedDeveloperToolsWidthState() -> (width: CGFloat?, widthFraction: CGFloat?) { | ||
| (preferredAttachedDeveloperToolsWidth, preferredAttachedDeveloperToolsWidthFraction) | ||
| } |
Contributor
There was a problem hiding this comment.
preferredAttachedDeveloperToolsWidthState() is never called
This function is introduced in this PR but has no call-site anywhere in the codebase — a search of the entire Sources/ tree returns only this definition. The companion method setPreferredHostedInspectorWidth(width:widthFraction:) in HostContainerView (BrowserPanelView.swift:3663) is similarly orphaned.
If this is scaffolding for a follow-up PR, adding a // TODO: comment would make the intent explicit. If it was meant to be wired up in this PR (e.g., to restore the inspector width when re-entering inline hosting mode), the restoration path appears to be missing.
| hit.inspectorView.needsLayout = true | ||
| hit.containerView.needsLayout = true | ||
| needsLayout = true | ||
| let isLiveDrag = reason == "drag" |
Contributor
There was a problem hiding this comment.
isLiveDrag unused in non-debug builds
Same issue as BrowserWindowPortal.swift line 987: isLiveDrag is declared in the main function body but is only referenced inside the #if DEBUG block below. In release builds the compiler will warn about the unused binding.
Suggested change
| let isLiveDrag = reason == "drag" | |
| #if DEBUG | |
| let isLiveDrag = reason == "drag" |
(Shift the declaration into the debug block, removing the standalone #if DEBUG line that opens it.)
![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="Sources/Panels/BrowserPanelView.swift">
<violation number="1" location="Sources/Panels/BrowserPanelView.swift:4039">
P2: Guard the async dock-configuration callback against stale frontend responses; otherwise an in-flight `evaluateJavaScript` result can reapply outdated dock state after the host switches modes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| hostedInspectorFrontendWebView.evaluateJavaScript( | ||
| "typeof WI === 'undefined' ? null : WI.dockConfiguration" | ||
| ) { [weak self] result, _ in | ||
| self?.applyHostedInspectorDockConfiguration(result as? String, reason: reason) |
There was a problem hiding this comment.
P2: Guard the async dock-configuration callback against stale frontend responses; otherwise an in-flight evaluateJavaScript result can reapply outdated dock state after the host switches modes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/BrowserPanelView.swift, line 4039:
<comment>Guard the async dock-configuration callback against stale frontend responses; otherwise an in-flight `evaluateJavaScript` result can reapply outdated dock state after the host switches modes.</comment>
<file context>
@@ -3877,12 +3927,159 @@ struct WebViewRepresentable: NSViewRepresentable {
+ hostedInspectorFrontendWebView.evaluateJavaScript(
+ "typeof WI === 'undefined' ? null : WI.dockConfiguration"
+ ) { [weak self] result, _ in
+ self?.applyHostedInspectorDockConfiguration(result as? String, reason: reason)
+ }
+ }
</file context>
Suggested change
| self?.applyHostedInspectorDockConfiguration(result as? String, reason: reason) | |
| guard let self, self.hostedInspectorFrontendWebView === hostedInspectorFrontendWebView else { return } | |
| self.applyHostedInspectorDockConfiguration(result as? String, reason: reason) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Sources/Panels/BrowserPanel.swift (2)
2900-2929: Consider usingTask {@mainactorin }instead ofMainActor.assumeIsolated.The observer callback uses
MainActor.assumeIsolatedat line 2909. While the notification is delivered on.mainqueue (ensuring main thread execution),MainActor.assumeIsolatedis a Swift concurrency primitive that asserts MainActor isolation at runtime. In release builds, if the assumption is violated, this can lead to undefined behavior.Since you're already deferring subsequent work via
DispatchQueue.main.async, consider wrapping theisDetachedInspectorWindowcheck similarly for consistency:♻️ Suggested approach
detachedDeveloperToolsWindowCloseObserver = NotificationCenter.default.addObserver( forName: NSWindow.willCloseNotification, object: nil, queue: .main ) { [weak self] notification in guard let self, let window = notification.object as? NSWindow else { return } - let isDetachedInspectorWindow = MainActor.assumeIsolated { - Self.isDetachedInspectorWindow(window) - } - guard isDetachedInspectorWindow else { return } DispatchQueue.main.async { [weak self] in guard let self else { return } + guard Self.isDetachedInspectorWindow(window) else { return } guard self.preferredDeveloperToolsPresentation == .detached else { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 2900 - 2929, The use of MainActor.assumeIsolated inside installDetachedDeveloperToolsWindowCloseObserver should be replaced with a safe MainActor-bound check; locate the notification closure that calls Self.isDetachedInspectorWindow(window) and either perform that call inside a Task { `@MainActor` in ... } or move the call into the existing DispatchQueue.main.async block (before checking preferredDeveloperToolsPresentation/visible) so it runs on the main actor without assuming isolation; update the closure to use Task { `@MainActor` in let isDetached = Self.isDetachedInspectorWindow(window) ... } or equivalent so you remove MainActor.assumeIsolated and ensure main-actor safety for isDetachedInspectorWindow.
4165-4184: Visibility change fromprivate extensiontoextensionexposes private WebKit API helpers.The
WKWebViewextension is now non-private, exposingcmuxInspectorObject()andcmuxInspectorFrontendWebView()to other files. Per the AI summary, this aligns with cross-file dependencies for hosted inspector layout.Consider documenting that these methods rely on private WebKit selectors (
_inspector,inspectorWebView) which could change or break in future macOS/WebKit releases:📝 Suggested documentation
extension WKWebView { + /// Access the private WKInspector object. + /// - Warning: Relies on private WebKit selector `_inspector` which may change in future releases. func cmuxInspectorObject() -> NSObject? {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 4165 - 4184, The extension exposing cmuxInspectorObject() and cmuxInspectorFrontendWebView() uses private WebKit selectors and should not be public; change the extension back to a file-private or private extension (so cmuxInspectorObject and cmuxInspectorFrontendWebView are not visible across files) and add a short doc comment above these methods noting they rely on private selectors "_inspector" and "inspectorWebView" which may break in future WebKit/macOS releases; ensure you keep the selector lookup logic (NSSelectorFromString, perform(_:).takeUnretainedValue()) in the same functions so only visibility and the explanatory comment are changed.
🤖 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 3627-3628: The restore/capture/hit-test logic must use the new 1
px minimum constant so the inspector can be recovered; update any comparisons
and clamps that reference zero or ignore width==1 to use
hostedInspectorDividerHitExpansion and minimumHostedInspectorWidth (instead of
minimumInspectorWidth: 0) and ensure equality is handled (e.g. treat width >=
minimumHostedInspectorWidth as visible, capture widths of exactly
minimumHostedInspectorWidth, and clamp restored widths with max(storedWidth,
minimumHostedInspectorWidth)). Adjust the restore, capture and hit-test code
paths that currently use 0 or exclusive checks so they consistently use
minimumHostedInspectorWidth and the hit expansion constant.
- Around line 3702-3704: The evaluateJavaScript completion handlers can run
after hostedInspectorFrontendWebView has been cleared/replaced, causing stale
dock state to be applied; to fix, capture the current
hostedInspectorFrontendWebView into a local constant (e.g., let currentWebView =
hostedInspectorFrontendWebView) immediately before calling evaluateJavaScript in
the methods that schedule JS evaluation, and inside the completion handler check
that hostedInspectorFrontendWebView === currentWebView (or that currentWebView
is non-nil and still valid) before calling
applyHostedInspectorDockConfiguration(...); apply this same guard wherever
evaluateJavaScript completion can call applyHostedInspectorDockConfiguration
(refer to prepareForWindowPortalHosting, setHostedInspectorFrontendWebView, and
the calls that invoke applyHostedInspectorDockConfiguration) so late callbacks
are ignored if the frontend/host changed.
---
Nitpick comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 2900-2929: The use of MainActor.assumeIsolated inside
installDetachedDeveloperToolsWindowCloseObserver should be replaced with a safe
MainActor-bound check; locate the notification closure that calls
Self.isDetachedInspectorWindow(window) and either perform that call inside a
Task { `@MainActor` in ... } or move the call into the existing
DispatchQueue.main.async block (before checking
preferredDeveloperToolsPresentation/visible) so it runs on the main actor
without assuming isolation; update the closure to use Task { `@MainActor` in let
isDetached = Self.isDetachedInspectorWindow(window) ... } or equivalent so you
remove MainActor.assumeIsolated and ensure main-actor safety for
isDetachedInspectorWindow.
- Around line 4165-4184: The extension exposing cmuxInspectorObject() and
cmuxInspectorFrontendWebView() uses private WebKit selectors and should not be
public; change the extension back to a file-private or private extension (so
cmuxInspectorObject and cmuxInspectorFrontendWebView are not visible across
files) and add a short doc comment above these methods noting they rely on
private selectors "_inspector" and "inspectorWebView" which may break in future
WebKit/macOS releases; ensure you keep the selector lookup logic
(NSSelectorFromString, perform(_:).takeUnretainedValue()) in the same functions
so only visibility and the explanatory comment are changed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e86be82-4421-4647-a4ba-aa86b977eb05
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swift
Comment on lines
+3627
to
+3628
| private static let hostedInspectorDividerHitExpansion: CGFloat = 10 | ||
| private static let minimumHostedInspectorWidth: CGFloat = 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align the restore/capture/hit-test thresholds with the new 1 px minimum.
The drag path clamps to 1 px, but the restore path still reapplies with minimumInspectorWidth: 0, the capture path ignores widths at exactly 1 px, and hit-testing stops treating the inspector as visible once it reaches 1 px. That makes a minimized inspector hard or impossible to recover after a resize/reattach, and it can also snap back to an older stored width.
Suggested fix
- guard inspectorWidth > 1 else { return }
+ guard inspectorWidth >= Self.minimumHostedInspectorWidth else { return }
let nextFrames = hit.dockSide.resizedFrames(
preferredWidth: preferredWidth,
in: containerBounds,
pageFrame: hit.pageView.frame,
inspectorFrame: hit.inspectorView.frame,
- minimumInspectorWidth: 0
+ minimumInspectorWidth: Self.minimumHostedInspectorWidth
)
fileprivate static func isVisibleHostedInspectorCandidate(_ view: NSView) -> Bool {
!view.isHidden &&
view.alphaValue > 0 &&
- view.frame.width > 1 &&
+ view.frame.width >= Self.minimumHostedInspectorWidth &&
view.frame.height > 1
}Also applies to: 4595-4597, 4644-4650, 4710-4714
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanelView.swift` around lines 3627 - 3628, The
restore/capture/hit-test logic must use the new 1 px minimum constant so the
inspector can be recovered; update any comparisons and clamps that reference
zero or ignore width==1 to use hostedInspectorDividerHitExpansion and
minimumHostedInspectorWidth (instead of minimumInspectorWidth: 0) and ensure
equality is handled (e.g. treat width >= minimumHostedInspectorWidth as visible,
capture widths of exactly minimumHostedInspectorWidth, and clamp restored widths
with max(storedWidth, minimumHostedInspectorWidth)). Adjust the restore, capture
and hit-test code paths that currently use 0 or exclusive checks so they
consistently use minimumHostedInspectorWidth and the hit expansion constant.
Comment on lines
+3702
to
+3704
| func setHostedInspectorFrontendWebView(_ webView: WKWebView?) { | ||
| hostedInspectorFrontendWebView = webView | ||
| } |
There was a problem hiding this comment.
Ignore stale dock-sync callbacks after the frontend/host changes.
prepareForWindowPortalHosting() cancels the queued work item, but it does not cancel an evaluateJavaScript that has already started. A late completion from the previous inspector frontend can still call applyHostedInspectorDockConfiguration(...) after hostedInspectorFrontendWebView was cleared or replaced, which can reparent/layout the local host using stale dock state.
Suggested fix
+ private var hostedInspectorDockConfigurationGeneration: UInt64 = 0
+
func setHostedInspectorFrontendWebView(_ webView: WKWebView?) {
hostedInspectorFrontendWebView = webView
+ hostedInspectorDockConfigurationGeneration &+= 1
}
private func syncHostedInspectorDockConfiguration(reason: String) {
hostedInspectorDockConfigurationSyncWorkItem = nil
guard let hostedInspectorFrontendWebView else { return }
+ let generation = hostedInspectorDockConfigurationGeneration
hostedInspectorFrontendWebView.evaluateJavaScript(
"typeof WI === 'undefined' ? null : WI.dockConfiguration"
- ) { [weak self] result, _ in
- self?.applyHostedInspectorDockConfiguration(result as? String, reason: reason)
+ ) { [weak self, weak hostedInspectorFrontendWebView] result, _ in
+ guard let self,
+ generation == self.hostedInspectorDockConfigurationGeneration,
+ hostedInspectorFrontendWebView === self.hostedInspectorFrontendWebView else { return }
+ self.applyHostedInspectorDockConfiguration(result as? String, reason: reason)
}
}Also applies to: 3857-3862, 4033-4040, 4043-4074
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanelView.swift` around lines 3702 - 3704, The
evaluateJavaScript completion handlers can run after
hostedInspectorFrontendWebView has been cleared/replaced, causing stale dock
state to be applied; to fix, capture the current hostedInspectorFrontendWebView
into a local constant (e.g., let currentWebView =
hostedInspectorFrontendWebView) immediately before calling evaluateJavaScript in
the methods that schedule JS evaluation, and inside the completion handler check
that hostedInspectorFrontendWebView === currentWebView (or that currentWebView
is non-nil and still valid) before calling
applyHostedInspectorDockConfiguration(...); apply this same guard wherever
evaluateJavaScript completion can call applyHostedInspectorDockConfiguration
(refer to prepareForWindowPortalHosting, setHostedInspectorFrontendWebView, and
the calls that invoke applyHostedInspectorDockConfiguration) so late callbacks
are ignored if the frontend/host changed.
This was referenced Mar 12, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…ols-resize-layout devtools resize layout
This was referenced Jun 13, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Stabilizes side-docked DevTools layout and resizing across inline and portal hosting. Persists attached inspector width proportionally, syncs dock side, routes divider interactions to WebKit when possible, and reduces layout churn. Addresses Linear #1183.
New Features
WI.dockConfigurationand re-parents when bottom-docked.Bug Fixes
WKcompanion views exist.WKInspectorviews when moving related subviews.Written for commit 06c5cac. Summary will update on new commits.
Summary by CodeRabbit