Repository navigation
Prevent attached DevTools from re-entering unsafe side-dock layouts - #1230
Merged
Merged
Conversation
…-1183-devtools-resize-layout # Conflicts: # cmuxTests/CmuxWebViewKeyEquivalentTests.swift
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors inspector divider sizing to use container-bound anchoring and explicit frame normalization, propagates a minimumInspectorWidth through resizing flows, adds WebKit companion-subview detection to preserve managed frames, implements staged Developer Tools visibility transitions with debouncing, and adds adaptive side/bottom docking logic and tests. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR makes the attached DevTools panel safe to use in narrow side-pane layouts by (1) detecting when a side-docked inspector would leave too little page width and automatically requesting Key changes:
Minor issues:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[layout / setFrameSize / setFrameOrigin] --> B[normalizeHostedInspectorLayoutIfNeeded]
B --> C{enforceAdaptiveBottomDockIfNeeded}
C -->|dividerCandidate && tooNarrow| D[recordHostedInspectorSideDockWidth]
D --> E{within cooldown?}
E -->|yes| F[return true — skip further layout]
E -->|no| G{hostedInspectorFrontendWebView?}
G -->|nil| H[return false]
G -->|set| I[evaluateJS: WI._dockBottom]
I --> J[scheduleHostedInspectorDockConfigurationSync]
I --> K[updateHostedInspectorDockControlAvailabilityIfNeeded]
K --> L[evaluateJS: patch WI._dockLeft/_dockRight/_togglePreviousDockConfiguration]
C -->|not narrow / no candidate| M[promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded]
M --> N{isHostedInspectorSideDockActive?}
N -->|yes| O[layoutHostedInspectorSideDockIfNeeded]
N -->|no| P[captureHostedInspectorPreferredWidthFromCurrentLayout]
Q[toggleDeveloperTools / showDeveloperTools / hideDeveloperTools] --> R[enqueueDeveloperToolsVisibilityTransition]
R --> S{isDeveloperToolsTransitionInFlight?}
S -->|yes| T[queue pending target, return true]
S -->|no| U[performDeveloperToolsVisibilityTransition]
U --> V{targetVisible?}
V -->|true + not yet visible| W[revealDeveloperTools]
V -->|true + already visible| X[clear grace deadline]
V -->|false + currently visible| Y[concealDeveloperTools]
V -->|false + already hidden| Z[cancelRestoreRetry + clearForceRefresh]
W --> AA[scheduleDeveloperToolsTransitionSettle 150ms]
Y --> AA
AA --> BB[finishDeveloperToolsTransition]
BB --> CC{pendingTarget != currentVisible?}
CC -->|yes| U
CC -->|no| DD[clear transitionTargetVisible]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+5036
to
+5047
| hit.pageView.needsDisplay = true | ||
| hit.pageView.setNeedsDisplay(hit.pageView.bounds) | ||
| hit.inspectorView.needsDisplay = true | ||
| hit.inspectorView.setNeedsDisplay(hit.inspectorView.bounds) | ||
| hit.containerView.needsDisplay = true | ||
| hit.containerView.setNeedsDisplay(hit.containerView.bounds) | ||
| if let localInlineSlotView { | ||
| localInlineSlotView.needsDisplay = true | ||
| localInlineSlotView.setNeedsDisplay(localInlineSlotView.bounds) | ||
| } | ||
| needsDisplay = true | ||
| setNeedsDisplay(bounds) |
Contributor
There was a problem hiding this comment.
Redundant needsDisplay + setNeedsDisplay(bounds) pairs
Each view gets needsDisplay = true immediately followed by setNeedsDisplay(view.bounds). Setting needsDisplay = true already marks the view's full visible rect as dirty, which is exactly what setNeedsDisplay(bounds) does. The paired calls are no-ops after the first line in each pair.
Suggested change
| hit.pageView.needsDisplay = true | |
| hit.pageView.setNeedsDisplay(hit.pageView.bounds) | |
| hit.inspectorView.needsDisplay = true | |
| hit.inspectorView.setNeedsDisplay(hit.inspectorView.bounds) | |
| hit.containerView.needsDisplay = true | |
| hit.containerView.setNeedsDisplay(hit.containerView.bounds) | |
| if let localInlineSlotView { | |
| localInlineSlotView.needsDisplay = true | |
| localInlineSlotView.setNeedsDisplay(localInlineSlotView.bounds) | |
| } | |
| needsDisplay = true | |
| setNeedsDisplay(bounds) | |
| hit.pageView.setNeedsDisplay(hit.pageView.bounds) | |
| hit.inspectorView.setNeedsDisplay(hit.inspectorView.bounds) | |
| hit.containerView.setNeedsDisplay(hit.containerView.bounds) | |
| if let localInlineSlotView { | |
| localInlineSlotView.setNeedsDisplay(localInlineSlotView.bounds) | |
| } | |
| setNeedsDisplay(bounds) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
9421-9431: Record semantic dock actions here instead of raw JavaScript strings.The new narrow-pane coverage now depends on exact frontend snippets, so harmless refactors in the injected script will break the test without changing behavior. Prefer capturing typed events like “request bottom dock” / “disable side dock” in this test double.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 9421 - 9431, The test double TrackingInspectorFrontendWebView currently records raw JavaScript in evaluatedJavaScript which is brittle; change it to record semantic dock actions instead by introducing a typed enum (e.g., DockAction with cases like requestBottomDock, disableSideDock, enableSideDock, etc.) and replacing evaluatedJavaScript: [String] with recordedActions: [DockAction]; update the override of evaluateJavaScript(_:completionHandler:) to parse/map incoming javaScriptString patterns (or substrings) to the correct DockAction and append that action (and call the completion handler) so tests assert on semantic actions rather than exact JS snippets.
🤖 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 2609-2611: Replace the fixed 0.5s sleep in
waitForDeveloperToolsTransitions() with a state-based poll: repeatedly check the
relevant inspector state/counts (the same conditions your tests expect after the
transition) until they match or a short timeout elapses, sleeping briefly
between polls; update waitForDeveloperToolsTransitions to accept/derive the
expected state or counts, poll those using RunLoop.run(mode:before:) or
DispatchQueue with small intervals, and throw or XCTFail on timeout so fast runs
don't wait and slow CI won't race the transition.
- Around line 9876-9919: The test is flaky because it asserts
inspectorView.evaluatedJavaScript immediately after setFrameSize/layout calls
while the bottom-dock fallback is throttle/cooldown driven; change the
testBrowserPanelHostRequestsBottomDockWhenSideDockLeavesTooLittlePageWidth test
to wait/poll (reuse the existing wait helper or an XCTest expectation with a
short timeout and polling) until inspectorView.evaluatedJavaScript contains the
expected strings ("WI._dockBottom()" and "const allowSideDock = false;") before
making the two XCTAssertTrue checks, keeping the calls to host.setFrameSize,
contentView.layoutSubtreeIfNeeded and host.layoutSubtreeIfNeeded as-is and
referencing inspectorView.evaluatedJavaScript and
promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded to locate the relevant
test logic.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 3842-3849: The function shouldAllowHostedInspectorManualSideDock()
currently only uses recordedHostedInspectorSideDockWidth (falling back to
Self.minimumHostedInspectorWidth) which ignores the previously
restored/persisted preferred width; update the baselineWidth calculation to
prefer the restored/persisted inspector width (e.g.
restoredHostedInspectorSideDockWidth or persistedHostedInspectorSideDockWidth)
when non-nil, then fall back to recordedHostedInspectorSideDockWidth and finally
Self.minimumHostedInspectorWidth so the restored width is used when deciding
whether to keep side-dock controls enabled.
- Around line 4452-4454: The current layout() sequence promotes the side dock
via promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded() before checking
enforceAdaptiveBottomDockIfNeeded(reason:), which can momentarily recreate a
too-narrow left/right layout; change the call order in layout() to run
enforceAdaptiveBottomDockIfNeeded(reason: "host.layout") (and its subsequent
updateHostedInspectorDockControlAvailabilityIfNeeded(reason:)) before calling
promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded(), mirroring the safe
ordering used by normalizeHostedInspectorLayoutIfNeeded(...), so the bottom-dock
guard runs first and prevents transient re-promotion into an unsafe side-dock
layout.
---
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 9421-9431: The test double TrackingInspectorFrontendWebView
currently records raw JavaScript in evaluatedJavaScript which is brittle; change
it to record semantic dock actions instead by introducing a typed enum (e.g.,
DockAction with cases like requestBottomDock, disableSideDock, enableSideDock,
etc.) and replacing evaluatedJavaScript: [String] with recordedActions:
[DockAction]; update the override of evaluateJavaScript(_:completionHandler:) to
parse/map incoming javaScriptString patterns (or substrings) to the correct
DockAction and append that action (and call the completion handler) so tests
assert on semantic actions rather than exact JS snippets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24fb210c-3e77-40f7-ad22-e64130d39e89
📒 Files selected for processing (4)
Sources/BrowserWindowPortal.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
Comment on lines
+2609
to
+2611
| private func waitForDeveloperToolsTransitions() { | ||
| RunLoop.current.run(until: Date().addingTimeInterval(0.5)) | ||
| } |
There was a problem hiding this comment.
Make this wait helper state-based instead of sleeping for 500 ms.
A fixed RunLoop sleep is both slow and flaky here: fast runs still pay the full half-second, and slow CI runs can still race the transition. Poll the expected inspector state/counts and fail on timeout instead.
⏱️ Suggested helper shape
- private func waitForDeveloperToolsTransitions() {
- RunLoop.current.run(until: Date().addingTimeInterval(0.5))
- }
+ private func waitForDeveloperToolsTransitions(
+ timeout: TimeInterval = 1.0,
+ until condition: `@escaping` () -> Bool
+ ) {
+ let deadline = Date().addingTimeInterval(timeout)
+ while !condition() && Date() < deadline {
+ RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01))
+ }
+ XCTAssertTrue(condition(), "Timed out waiting for DevTools transition")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func waitForDeveloperToolsTransitions() { | |
| RunLoop.current.run(until: Date().addingTimeInterval(0.5)) | |
| } | |
| private func waitForDeveloperToolsTransitions( | |
| timeout: TimeInterval = 1.0, | |
| until condition: `@escaping` () -> Bool | |
| ) { | |
| let deadline = Date().addingTimeInterval(timeout) | |
| while !condition() && Date() < deadline { | |
| RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01)) | |
| } | |
| XCTAssertTrue(condition(), "Timed out waiting for DevTools transition") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 2609 - 2611,
Replace the fixed 0.5s sleep in waitForDeveloperToolsTransitions() with a
state-based poll: repeatedly check the relevant inspector state/counts (the same
conditions your tests expect after the transition) until they match or a short
timeout elapses, sleeping briefly between polls; update
waitForDeveloperToolsTransitions to accept/derive the expected state or counts,
poll those using RunLoop.run(mode:before:) or DispatchQueue with small
intervals, and throw or XCTFail on timeout so fast runs don't wait and slow CI
won't race the transition.
Comment on lines
+9876
to
+9919
| func testBrowserPanelHostRequestsBottomDockWhenSideDockLeavesTooLittlePageWidth() { | ||
| let window = NSWindow( | ||
| contentRect: NSRect(x: 0, y: 0, width: 420, height: 260), | ||
| styleMask: [.titled, .closable], | ||
| backing: .buffered, | ||
| defer: false | ||
| ) | ||
| defer { window.orderOut(nil) } | ||
| guard let contentView = window.contentView else { | ||
| XCTFail("Expected content view") | ||
| return | ||
| } | ||
|
|
||
| let host = WebViewRepresentable.HostContainerView(frame: NSRect(x: 180, y: 0, width: 280, height: contentView.bounds.height)) | ||
| host.autoresizingMask = [.minXMargin, .height] | ||
| contentView.addSubview(host) | ||
|
|
||
| let slotView = host.ensureLocalInlineSlotView() | ||
| let pageView = WKWebView(frame: NSRect(x: 0, y: 0, width: 120, height: host.bounds.height)) | ||
| let inspectorView = TrackingInspectorFrontendWebView( | ||
| frame: NSRect(x: 120, y: 0, width: slotView.bounds.width - 120, height: host.bounds.height) | ||
| ) | ||
| slotView.addSubview(pageView) | ||
| slotView.addSubview(inspectorView) | ||
| host.pinHostedWebView(pageView, in: slotView) | ||
| host.setHostedInspectorFrontendWebView(inspectorView) | ||
| contentView.layoutSubtreeIfNeeded() | ||
| host.layoutSubtreeIfNeeded() | ||
|
|
||
| XCTAssertTrue(host.promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded()) | ||
|
|
||
| host.setFrameSize(NSSize(width: 210, height: host.frame.height)) | ||
| contentView.layoutSubtreeIfNeeded() | ||
| host.layoutSubtreeIfNeeded() | ||
|
|
||
| XCTAssertTrue( | ||
| inspectorView.evaluatedJavaScript.contains(where: { $0.contains("WI._dockBottom()") }), | ||
| "Narrow pane widths should request bottom-docked DevTools instead of leaving the side-docked inspector in an unstable layout" | ||
| ) | ||
| XCTAssertTrue( | ||
| inspectorView.evaluatedJavaScript.contains(where: { $0.contains("const allowSideDock = false;") }), | ||
| "Once a narrow pane proves it cannot safely side-dock DevTools, the inspector frontend should hide and disable left/right dock controls" | ||
| ) | ||
| } |
There was a problem hiding this comment.
Yield before asserting the bottom-dock fallback fired.
These assertions run immediately after setFrameSize and layout. Since this PR’s fallback path is throttle/cooldown driven, the resize can still have the JS request queued when evaluatedJavaScript is checked, which makes the test timing-sensitive. Reuse the wait helper or poll until the expected calls are recorded before asserting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 9876 - 9919, The
test is flaky because it asserts inspectorView.evaluatedJavaScript immediately
after setFrameSize/layout calls while the bottom-dock fallback is
throttle/cooldown driven; change the
testBrowserPanelHostRequestsBottomDockWhenSideDockLeavesTooLittlePageWidth test
to wait/poll (reuse the existing wait helper or an XCTest expectation with a
short timeout and polling) until inspectorView.evaluatedJavaScript contains the
expected strings ("WI._dockBottom()" and "const allowSideDock = false;") before
making the two XCTAssertTrue checks, keeping the calls to host.setFrameSize,
contentView.layoutSubtreeIfNeeded and host.layoutSubtreeIfNeeded as-is and
referencing inspectorView.evaluatedJavaScript and
promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded to locate the relevant
test logic.
Comment on lines
+3842
to
+3849
| private func shouldAllowHostedInspectorManualSideDock() -> Bool { | ||
| let containerWidth = max(0, bounds.width) | ||
| guard containerWidth > 1 else { return true } | ||
| let baselineWidth = max( | ||
| Self.minimumHostedInspectorWidth, | ||
| recordedHostedInspectorSideDockWidth ?? Self.minimumHostedInspectorWidth | ||
| ) | ||
| return containerWidth - baselineWidth >= Self.minimumHostedInspectorPageWidthForSideDock |
There was a problem hiding this comment.
Use the restored inspector width when deciding whether side-dock controls stay enabled.
shouldAllowHostedInspectorManualSideDock() only looks at recordedHostedInspectorSideDockWidth, so a fresh attach falls back to the 120px minimum even though the persisted preferred width was already restored earlier. In a narrow pane, that can leave left/right dock controls enabled until the user clicks them, at which point the remembered wider side dock immediately becomes unsafe again.
Suggested fix
private func shouldAllowHostedInspectorManualSideDock() -> Bool {
let containerWidth = max(0, bounds.width)
guard containerWidth > 1 else { return true }
- let baselineWidth = max(
- Self.minimumHostedInspectorWidth,
- recordedHostedInspectorSideDockWidth ?? Self.minimumHostedInspectorWidth
- )
+ let preferredWidth = resolvedPreferredHostedInspectorWidth(in: bounds)
+ let baselineWidth = max(
+ Self.minimumHostedInspectorWidth,
+ recordedHostedInspectorSideDockWidth ?? 0,
+ preferredWidth ?? 0
+ )
return containerWidth - baselineWidth >= Self.minimumHostedInspectorPageWidthForSideDock
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanelView.swift` around lines 3842 - 3849, The function
shouldAllowHostedInspectorManualSideDock() currently only uses
recordedHostedInspectorSideDockWidth (falling back to
Self.minimumHostedInspectorWidth) which ignores the previously
restored/persisted preferred width; update the baselineWidth calculation to
prefer the restored/persisted inspector width (e.g.
restoredHostedInspectorSideDockWidth or persistedHostedInspectorSideDockWidth)
when non-nil, then fall back to recordedHostedInspectorSideDockWidth and finally
Self.minimumHostedInspectorWidth so the restored width is used when deciding
whether to keep side-dock controls enabled.
Comment on lines
+4452
to
+4454
| _ = promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded() | ||
| if enforceAdaptiveBottomDockIfNeeded(reason: "host.layout") { | ||
| updateHostedInspectorDockControlAvailabilityIfNeeded(reason: "host.layout") |
There was a problem hiding this comment.
Run the bottom-dock guard before re-promoting a side-dock layout.
Line 4452 promotes the current split back into managed side-dock state before Line 4453 checks whether that split is already too narrow. That means a resize can briefly re-enter the unsafe left/right layout the PR is trying to suppress. normalizeHostedInspectorLayoutIfNeeded(...) already does this in the safer order; layout() should match it.
Suggested fix
override func layout() {
super.layout()
- _ = promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded()
if enforceAdaptiveBottomDockIfNeeded(reason: "host.layout") {
updateHostedInspectorDockControlAvailabilityIfNeeded(reason: "host.layout")
notifyGeometryChangedIfNeeded()
`#if` DEBUG
debugLogHostedInspectorLayoutIfNeeded(reason: "layout")
`#endif`
return
}
+ _ = promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded()
if let previousSize = lastHostedInspectorLayoutBoundsSize,
Self.sizeApproximatelyEqual(previousSize, bounds.size, epsilon: 0.5) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanelView.swift` around lines 4452 - 4454, The current
layout() sequence promotes the side dock via
promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded() before checking
enforceAdaptiveBottomDockIfNeeded(reason:), which can momentarily recreate a
too-narrow left/right layout; change the call order in layout() to run
enforceAdaptiveBottomDockIfNeeded(reason: "host.layout") (and its subsequent
updateHostedInspectorDockControlAvailabilityIfNeeded(reason:)) before calling
promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded(), mirroring the safe
ordering used by normalizeHostedInspectorLayoutIfNeeded(...), so the bottom-dock
guard runs first and prevents transient re-promotion into an unsafe side-dock
layout.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files
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/BrowserPanel.swift">
<violation number="1" location="Sources/Panels/BrowserPanel.swift:3292">
P1: `showDeveloperToolsConsole()` fails to switch to the console tab when opening from a closed state.</violation>
</file>
<file name="Sources/BrowserWindowPortal.swift">
<violation number="1" location="Sources/BrowserWindowPortal.swift:592">
P2: The `applyHostedInspectorDividerWidth` call uses the global `Self.minimumHostedInspectorWidth` instead of the local dynamically calculated `minimumInspectorWidth`. This causes the inspector to aggressively snap to the global minimum during a drag, defeating the purpose of dynamically computing the minimum based on `initialInspectorFrame.width`.</violation>
</file>
<file name="cmuxTests/CmuxWebViewKeyEquivalentTests.swift">
<violation number="1" location="cmuxTests/CmuxWebViewKeyEquivalentTests.swift:2610">
P3: Replace this fixed delay with a condition-based wait loop so transition assertions are deterministic and the test doesn’t always pay a 500ms sleep penalty.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @discardableResult | ||
| func showDeveloperToolsConsole() -> Bool { | ||
| guard showDeveloperTools() else { return false } | ||
| guard !isDeveloperToolsTransitionInFlight else { return true } |
There was a problem hiding this comment.
P1: showDeveloperToolsConsole() fails to switch to the console tab when opening from a closed state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/BrowserPanel.swift, line 3292:
<comment>`showDeveloperToolsConsole()` fails to switch to the console tab when opening from a closed state.</comment>
<file context>
@@ -3184,30 +3278,18 @@ extension BrowserPanel {
@discardableResult
func showDeveloperToolsConsole() -> Bool {
guard showDeveloperTools() else { return false }
+ guard !isDeveloperToolsTransitionInFlight else { return true }
guard let inspector = webView.cmuxInspectorObject() else { return true }
// WebKit private inspector API differs by OS; try known console selectors.
</file context>
| inspectorView: dragState.inspectorView, | ||
| dockSide: dragState.dockSide | ||
| ), | ||
| minimumInspectorWidth: Self.minimumHostedInspectorWidth, |
There was a problem hiding this comment.
P2: The applyHostedInspectorDividerWidth call uses the global Self.minimumHostedInspectorWidth instead of the local dynamically calculated minimumInspectorWidth. This causes the inspector to aggressively snap to the global minimum during a drag, defeating the purpose of dynamically computing the minimum based on initialInspectorFrame.width.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/BrowserWindowPortal.swift, line 592:
<comment>The `applyHostedInspectorDividerWidth` call uses the global `Self.minimumHostedInspectorWidth` instead of the local dynamically calculated `minimumInspectorWidth`. This causes the inspector to aggressively snap to the global minimum during a drag, defeating the purpose of dynamically computing the minimum based on `initialInspectorFrame.width`.</comment>
<file context>
@@ -572,6 +589,7 @@ final class WindowBrowserHostView: NSView {
inspectorView: dragState.inspectorView,
dockSide: dragState.dockSide
),
+ minimumInspectorWidth: Self.minimumHostedInspectorWidth,
reason: "drag"
)
</file context>
Suggested change
| minimumInspectorWidth: Self.minimumHostedInspectorWidth, | |
| minimumInspectorWidth: minimumInspectorWidth, |
| } | ||
|
|
||
| private func waitForDeveloperToolsTransitions() { | ||
| RunLoop.current.run(until: Date().addingTimeInterval(0.5)) |
There was a problem hiding this comment.
P3: Replace this fixed delay with a condition-based wait loop so transition assertions are deterministic and the test doesn’t always pay a 500ms sleep penalty.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/CmuxWebViewKeyEquivalentTests.swift, line 2610:
<comment>Replace this fixed delay with a condition-based wait loop so transition assertions are deterministic and the test doesn’t always pay a 500ms sleep penalty.</comment>
<file context>
@@ -2606,6 +2606,10 @@ final class BrowserDeveloperToolsVisibilityPersistenceTests: XCTestCase {
}
+ private func waitForDeveloperToolsTransitions() {
+ RunLoop.current.run(until: Date().addingTimeInterval(0.5))
+ }
+
</file context>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (2)
2609-2611:⚠️ Potential issue | 🟡 MinorMake the DevTools wait helper state-based.
This fixed 500 ms sleep is still both slow and flaky: fast runs always pay the delay, and slow CI can still race the transition. Poll the expected inspector state/counts and fail on timeout instead.
⏱️ Suggested shape
- private func waitForDeveloperToolsTransitions() { - RunLoop.current.run(until: Date().addingTimeInterval(0.5)) - } + private func waitForDeveloperToolsTransitions( + timeout: TimeInterval = 1.0, + until condition: `@escaping` () -> Bool + ) { + let deadline = Date().addingTimeInterval(timeout) + while !condition() && Date() < deadline { + RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01)) + } + XCTAssertTrue(condition(), "Timed out waiting for DevTools transition") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 2609 - 2611, Replace the fixed 0.5s RunLoop sleep in waitForDeveloperToolsTransitions() with a state-based polling loop: repeatedly pump the RunLoop (e.g. short intervals) and check the actual DevTools/inspector state or expected counts (the condition your tests assert) until the condition is true or a timeout elapses; on timeout fail the helper so tests fail fast. Locate waitForDeveloperToolsTransitions() and implement a predicate-based wait that returns when the inspector state/counts match expected values or throws/asserts on timeout instead of unconditionally waiting 0.5s.
9876-9919:⚠️ Potential issue | 🟡 MinorWait for the bottom-dock fallback JS before asserting.
These assertions still run immediately after
setFrameSizeand layout. If the fallback request is queued,evaluatedJavaScriptcan still be empty here and make the test timing-sensitive. Poll until the expected calls are recorded before asserting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 9876 - 9919, The test currently asserts on inspectorView.evaluatedJavaScript immediately after host.setFrameSize, which is racing with async JS calls; replace the direct XCTAssertTrue checks in testBrowserPanelHostRequestsBottomDockWhenSideDockLeavesTooLittlePageWidth() with a short polling/wait mechanism (e.g. XCTNSPredicateExpectation or a manual loop with timeout ~1–2s and small sleep interval) that repeatedly checks inspectorView.evaluatedJavaScript for entries containing "WI._dockBottom()" and "const allowSideDock = false;" after calling host.setFrameSize(...) and layoutSubtreeIfNeeded(), then assert only once the expected JS calls are observed or fail on timeout; reference inspectorView.evaluatedJavaScript, host.setFrameSize, contentView.layoutSubtreeIfNeeded(), and host.layoutSubtreeIfNeeded() when making the change.
🤖 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 2609-2611: Replace the fixed 0.5s RunLoop sleep in
waitForDeveloperToolsTransitions() with a state-based polling loop: repeatedly
pump the RunLoop (e.g. short intervals) and check the actual DevTools/inspector
state or expected counts (the condition your tests assert) until the condition
is true or a timeout elapses; on timeout fail the helper so tests fail fast.
Locate waitForDeveloperToolsTransitions() and implement a predicate-based wait
that returns when the inspector state/counts match expected values or
throws/asserts on timeout instead of unconditionally waiting 0.5s.
- Around line 9876-9919: The test currently asserts on
inspectorView.evaluatedJavaScript immediately after host.setFrameSize, which is
racing with async JS calls; replace the direct XCTAssertTrue checks in
testBrowserPanelHostRequestsBottomDockWhenSideDockLeavesTooLittlePageWidth()
with a short polling/wait mechanism (e.g. XCTNSPredicateExpectation or a manual
loop with timeout ~1–2s and small sleep interval) that repeatedly checks
inspectorView.evaluatedJavaScript for entries containing "WI._dockBottom()" and
"const allowSideDock = false;" after calling host.setFrameSize(...) and
layoutSubtreeIfNeeded(), then assert only once the expected JS calls are
observed or fail on timeout; reference inspectorView.evaluatedJavaScript,
host.setFrameSize, contentView.layoutSubtreeIfNeeded(), and
host.layoutSubtreeIfNeeded() when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4459d997-d6ee-4225-858c-a3d9f59bb17b
📒 Files selected for processing (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift
This was referenced Mar 14, 2026
This was referenced Mar 22, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…ols-side-dock-guard Prevent attached DevTools from re-entering unsafe side-dock layouts
This was referenced Apr 22, 2026
This was referenced May 11, 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
./scripts/reload.sh --tag issue-1183-devtools-resize-layoutSummary by cubic
Prevents attached DevTools from entering unsafe side-dock layouts on narrow panes by auto-switching to bottom dock and disabling side-dock controls when needed. Coalesces visibility toggles to avoid flicker and redundant Inspector calls, aligning with Linear issue 1183.
New Features
Bug Fixes
WKWebViewtest override signature.Written for commit f429907. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests