Repository navigation
Fix browser pane flicker during multi-split resize - #2574
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR enhances interactive split divider drag detection in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 `@Sources/BrowserWindowPortal.swift`:
- Around line 2170-2198: The current detection only checks the first
arrangedSubviews pair and misses later dividers; update the logic in the
hit-test block that uses splitView, arrangedSubviews and dividerThickness to
iterate adjacent subview pairs (0/1, 1/2, …) compute each dividerRect
(respecting splitView.isVertical vs horizontal and using first.maxX/first.maxY
and second frames) and test location against each dividerRect.insetBy(dx: -4,
dy: -4); if any contains(location) set
window.browserPortalHasInteractiveSplitDividerDrag = true (and return/exit
early) so drags on any divider latch correctly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a007cce-406c-4105-aad5-cc18244d41de
📒 Files selected for processing (2)
Sources/BrowserWindowPortal.swiftSources/GhosttyTerminalView.swift
| guard splitView.arrangedSubviews.count >= 2 else { return } | ||
|
|
||
| let location = splitView.convert(event.locationInWindow, from: nil) | ||
| let first = splitView.arrangedSubviews[0].frame | ||
| let second = splitView.arrangedSubviews[1].frame | ||
| let thickness = splitView.dividerThickness | ||
| let dividerRect: NSRect | ||
|
|
||
| if splitView.isVertical { | ||
| guard first.width > 1, second.width > 1 else { return } | ||
| dividerRect = NSRect( | ||
| x: max(0, first.maxX), | ||
| y: 0, | ||
| width: thickness, | ||
| height: splitView.bounds.height | ||
| ) | ||
| } else { | ||
| guard first.height > 1, second.height > 1 else { return } | ||
| dividerRect = NSRect( | ||
| x: 0, | ||
| y: max(0, first.maxY), | ||
| width: splitView.bounds.width, | ||
| height: thickness | ||
| ) | ||
| } | ||
|
|
||
| if dividerRect.insetBy(dx: -4, dy: -4).contains(location) { | ||
| window.browserPortalHasInteractiveSplitDividerDrag = true | ||
| } |
There was a problem hiding this comment.
Divider latch detection only checks the first divider pair.
Line 2173–2174 only inspects arrangedSubviews[0]/[1], so drags on later dividers in a 3+ pane NSSplitView won’t set the latch flag. That leaves a path for redundant external geometry sync during active drag.
Suggested fix
- guard splitView.arrangedSubviews.count >= 2 else { return }
-
- let location = splitView.convert(event.locationInWindow, from: nil)
- let first = splitView.arrangedSubviews[0].frame
- let second = splitView.arrangedSubviews[1].frame
- let thickness = splitView.dividerThickness
- let dividerRect: NSRect
-
- if splitView.isVertical {
- guard first.width > 1, second.width > 1 else { return }
- dividerRect = NSRect(
- x: max(0, first.maxX),
- y: 0,
- width: thickness,
- height: splitView.bounds.height
- )
- } else {
- guard first.height > 1, second.height > 1 else { return }
- dividerRect = NSRect(
- x: 0,
- y: max(0, first.maxY),
- width: splitView.bounds.width,
- height: thickness
- )
- }
-
- if dividerRect.insetBy(dx: -4, dy: -4).contains(location) {
- window.browserPortalHasInteractiveSplitDividerDrag = true
- }
+ let location = splitView.convert(event.locationInWindow, from: nil)
+ let thickness = splitView.dividerThickness
+ let dividerCount = max(0, splitView.arrangedSubviews.count - 1)
+ guard dividerCount > 0 else { return }
+
+ for dividerIndex in 0..<dividerCount {
+ let first = splitView.arrangedSubviews[dividerIndex].frame
+ let second = splitView.arrangedSubviews[dividerIndex + 1].frame
+ let dividerRect: NSRect
+
+ if splitView.isVertical {
+ guard first.width > 1 || second.width > 1 else { continue }
+ dividerRect = NSRect(
+ x: max(0, first.maxX),
+ y: 0,
+ width: thickness,
+ height: splitView.bounds.height
+ )
+ } else {
+ guard first.height > 1 || second.height > 1 else { continue }
+ dividerRect = NSRect(
+ x: 0,
+ y: max(0, first.maxY),
+ width: splitView.bounds.width,
+ height: thickness
+ )
+ }
+
+ if dividerRect.insetBy(dx: -4, dy: -4).contains(location) {
+ window.browserPortalHasInteractiveSplitDividerDrag = true
+ return
+ }
+ }📝 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.
| guard splitView.arrangedSubviews.count >= 2 else { return } | |
| let location = splitView.convert(event.locationInWindow, from: nil) | |
| let first = splitView.arrangedSubviews[0].frame | |
| let second = splitView.arrangedSubviews[1].frame | |
| let thickness = splitView.dividerThickness | |
| let dividerRect: NSRect | |
| if splitView.isVertical { | |
| guard first.width > 1, second.width > 1 else { return } | |
| dividerRect = NSRect( | |
| x: max(0, first.maxX), | |
| y: 0, | |
| width: thickness, | |
| height: splitView.bounds.height | |
| ) | |
| } else { | |
| guard first.height > 1, second.height > 1 else { return } | |
| dividerRect = NSRect( | |
| x: 0, | |
| y: max(0, first.maxY), | |
| width: splitView.bounds.width, | |
| height: thickness | |
| ) | |
| } | |
| if dividerRect.insetBy(dx: -4, dy: -4).contains(location) { | |
| window.browserPortalHasInteractiveSplitDividerDrag = true | |
| } | |
| let location = splitView.convert(event.locationInWindow, from: nil) | |
| let thickness = splitView.dividerThickness | |
| let dividerCount = max(0, splitView.arrangedSubviews.count - 1) | |
| guard dividerCount > 0 else { return } | |
| for dividerIndex in 0..<dividerCount { | |
| let first = splitView.arrangedSubviews[dividerIndex].frame | |
| let second = splitView.arrangedSubviews[dividerIndex + 1].frame | |
| let dividerRect: NSRect | |
| if splitView.isVertical { | |
| guard first.width > 1 || second.width > 1 else { continue } | |
| dividerRect = NSRect( | |
| x: max(0, first.maxX), | |
| y: 0, | |
| width: thickness, | |
| height: splitView.bounds.height | |
| ) | |
| } else { | |
| guard first.height > 1 || second.height > 1 else { continue } | |
| dividerRect = NSRect( | |
| x: 0, | |
| y: max(0, first.maxY), | |
| width: splitView.bounds.width, | |
| height: thickness | |
| ) | |
| } | |
| if dividerRect.insetBy(dx: -4, dy: -4).contains(location) { | |
| window.browserPortalHasInteractiveSplitDividerDrag = true | |
| return | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/BrowserWindowPortal.swift` around lines 2170 - 2198, The current
detection only checks the first arrangedSubviews pair and misses later dividers;
update the logic in the hit-test block that uses splitView, arrangedSubviews and
dividerThickness to iterate adjacent subview pairs (0/1, 1/2, …) compute each
dividerRect (respecting splitView.isVertical vs horizontal and using
first.maxX/first.maxY and second frames) and test location against each
dividerRect.insetBy(dx: -4, dy: -4); if any contains(location) set
window.browserPortalHasInteractiveSplitDividerDrag = true (and return/exit
early) so drags on any divider latch correctly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR eliminates browser pane flicker during multi-split divider drags by latching interactive drag state on Confidence Score: 5/5Safe to merge — no P0/P1 issues; latch logic is correct and well-guarded against stale state. All findings are P2 or lower. The latch uses a well-established associated-object pattern, auto-clears on mouse-up via the getter, and falls back to raw event-state detection when the divider hit test can't confirm the drag. The final portal sync after drag end is preserved via the post-mouse-up didResizeSubviewsNotification path. No data loss, no security surface, no broken contracts. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppKit
participant BrowserWindowPortal
participant NSWindow
participant GeometrySync
User->>AppKit: Drag split divider (leftMouseDragged)
AppKit->>BrowserWindowPortal: willResizeSubviewsNotification
BrowserWindowPortal->>BrowserWindowPortal: noteInteractiveSplitDividerDragIfNeeded()
Note over BrowserWindowPortal: Checks: mouse button down,<br/>recent event, divider hit test
BrowserWindowPortal->>NSWindow: browserPortalHasInteractiveSplitDividerDrag = true
AppKit->>BrowserWindowPortal: didResizeSubviewsNotification
BrowserWindowPortal->>BrowserWindowPortal: shouldTreatSplitResizeAsExternalGeometry()
BrowserWindowPortal->>NSWindow: browserPortalHasInteractiveSplitDividerDrag (get)
NSWindow-->>BrowserWindowPortal: true (latch active + mouse still down)
BrowserWindowPortal->>GeometrySync: suppressed (no flicker)
User->>AppKit: Release mouse button
AppKit->>BrowserWindowPortal: didResizeSubviewsNotification (final)
BrowserWindowPortal->>NSWindow: browserPortalHasInteractiveSplitDividerDrag (get)
NSWindow-->>BrowserWindowPortal: false (auto-cleared, mouse released)
BrowserWindowPortal->>GeometrySync: scheduleExternalGeometrySynchronize()
GeometrySync->>GeometrySync: synchronizeAllEntriesFromExternalGeometryChange()
Reviews (1): Last reviewed commit: "Fix ghostty callback return values" | Re-trigger Greptile |
…icker-multi-split # Conflicts:
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 `@Sources/GhosttyTerminalView.swift`:
- Around line 1233-1252: The callbacks for insertText and onFailure currently
call MainActor.assumeIsolated directly but may be invoked off the main thread;
update those closures to mirror the defensive pattern used in
TerminalImageTransferPlanner.execute/executeImageTransferPlan by checking
Thread.isMainThread and calling the UI updates
(callbackContext.terminalSurface?.hostedView.endImageTransferIndicator(for:
operation) and NSSound.beep() in onFailure) immediately if on main thread,
otherwise dispatch them with DispatchQueue.main.async; keep calls to
completeClipboardRequest(with:) as-is but ensure the UI-ending calls run on the
main thread and preserve any weak captures (e.g., callbackContext) used in the
original closures.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e659150-ce71-445a-bdd8-3b49c3bc638f
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
| insertText: { text in | ||
| MainActor.assumeIsolated { | ||
| callbackContext.terminalSurface?.hostedView.endImageTransferIndicator( | ||
| for: operation | ||
| ) | ||
| } | ||
| completeClipboardRequest(with: text) | ||
| }, | ||
| onFailure: { _ in | ||
| MainActor.assumeIsolated { | ||
| callbackContext.terminalSurface?.hostedView.endImageTransferIndicator( | ||
| for: operation | ||
| ) | ||
| } | ||
| NSSound.beep() | ||
| #if DEBUG | ||
| dlog("terminal.remotePasteUpload.failed surface=\(callbackContext.surfaceId.uuidString.prefix(5))") | ||
| #endif | ||
| completeClipboardRequest(with: "") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
planner_file=$(rg -n --type swift 'struct TerminalImageTransferPlanner|enum TerminalImageTransferPlanner|class TerminalImageTransferPlanner' | head -n1 | cut -d: -f1)
if [[ -z "${planner_file:-}" ]]; then
echo "TerminalImageTransferPlanner definition not found"
exit 1
fi
printf '=== %s ===\n' "$planner_file"
rg -n -C6 'static func execute\(|insertText:|onFailure:|DispatchQueue\.main|MainActor\.assumeIsolated' "$planner_file"Repository: manaflow-ai/cmux
Length of output: 3245
🏁 Script executed:
#!/bin/bash
# Get complete finishUpload function
rg -n -A30 'private static func finishUpload' Sources/TerminalImageTransfer.swiftRepository: manaflow-ai/cmux
Length of output: 1283
🏁 Script executed:
#!/bin/bash
# Find executeImageTransferPlan in GhosttyTerminalView.swift to see the "defensive hop"
rg -n -B5 -A15 'executeImageTransferPlan' Sources/GhosttyTerminalView.swiftRepository: manaflow-ai/cmux
Length of output: 1663
🏁 Script executed:
#!/bin/bash
# Check how uploadWorkspaceRemote/uploadDetectedSSH handlers are passed and whether they hop to main
rg -n -B3 -A10 'uploadWorkspaceRemote:|uploadDetectedSSH:' Sources/GhosttyTerminalView.swift | head -100Repository: manaflow-ai/cmux
Length of output: 3881
🏁 Script executed:
#!/bin/bash
# Get the complete insertText and onFailure callbacks in executeImageTransferPlan
sed -n '7265,7300p' Sources/GhosttyTerminalView.swiftRepository: manaflow-ai/cmux
Length of output: 1512
Mirror the defensive main-thread dispatch pattern used in executeImageTransferPlan.
The insertText and onFailure callbacks at lines 1233–1252 use MainActor.assumeIsolated directly without checking thread context, but TerminalImageTransferPlanner.execute invokes these callbacks from async completion handlers in finishUpload (lines 326, 328) with no main-thread guarantee. The executeImageTransferPlan path (lines 7265–7290) correctly handles this with explicit checks and dispatch:
insertText: { [weak self] text in
if Thread.isMainThread {
send()
} else {
DispatchQueue.main.async(execute: send)
}
},
onFailure: { [weak self] _ in
DispatchQueue.main.async {
NSSound.beep()
// ...
}
}Update lines 1233–1252 to match this defensive threading pattern for consistency and safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 1233 - 1252, The callbacks
for insertText and onFailure currently call MainActor.assumeIsolated directly
but may be invoked off the main thread; update those closures to mirror the
defensive pattern used in
TerminalImageTransferPlanner.execute/executeImageTransferPlan by checking
Thread.isMainThread and calling the UI updates
(callbackContext.terminalSurface?.hostedView.endImageTransferIndicator(for:
operation) and NSSound.beep() in onFailure) immediately if on main thread,
otherwise dispatch them with DispatchQueue.main.async; keep calls to
completeClipboardRequest(with:) as-is but ensure the UI-ending calls run on the
main thread and preserve any weak captures (e.g., callbackContext) used in the
original closures.
This branch was successfully deployed
1 participant
Fixes #2571
Summary
Verification
Notes
Summary by CodeRabbit
Bug Fixes
Chores