Skip to content

Fix splitter hitbox overlap and terminal scrollbar width resync#1950

Merged
austinywang merged 4 commits intomainfrom
issue-1632-splitter-hitbox-scrollbar
Mar 23, 2026
Merged

Fix splitter hitbox overlap and terminal scrollbar width resync#1950
austinywang merged 4 commits intomainfrom
issue-1632-splitter-hitbox-scrollbar

Conversation

@austinywang
Copy link
Copy Markdown
Contributor

@austinywang austinywang commented Mar 22, 2026

Fixes #1632

Summary

  • narrow the shared sidebar divider overlap so it stays generous on the sidebar side but only reaches 2 px into terminal/browser content
  • resync the terminal core surface width when the preferred scrollbar style changes so legacy gutters do not hide the right edge of the grid
  • add regressions for the terminal/browser hit-testing edge cases and the scrollbar-style width recalculation path

Testing

  • ./scripts/reload.sh --tag issue-1632-review
  • local tests not run per repo policy

Commit Structure

  • 2b38c76e failing regressions
  • fe8ae0ff fix

Summary by cubic

Fixes splitter hitbox overlap into terminal/browser content and resyncs terminal surface width when scrollbar style changes, keeping selection precise and the right grid edge visible.

  • Bug Fixes
    • Sidebar divider: keep a 6 px hit zone on the sidebar side and only 2 px into content; update hit-testing in window host views and the WKWebView pass-through band; set the overlay handle to the combined 8 px width; restore hosted inspector divider drag and reapply the stored width when available.
    • Terminal width: observe NSScroller.preferredScrollerStyleDidChangeNotification, retile only the scroll view, and immediately resync the core surface width for legacy vs. overlay scrollbars; add a debug-only surface size accessor and regressions for the reduced overlap and scroller-style recalculation.

Written for commit d77ae21. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Terminal windows now update sizing immediately when system scroll bar style changes.
  • Bug Fixes

    • Improved sidebar resizer hit detection using asymmetric zones for more accurate pointer interactions at dividers.
    • More consistent hosted-inspector divider behavior and layout reapplication.
  • Tests

    • Added and adjusted tests to validate resizer hit behavior and terminal surface sizing on scroller-style changes.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 22, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Mar 23, 2026 0:51am

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 22, 2026

📝 Walkthrough

Walkthrough

The diff makes sidebar-resizer hit regions asymmetric (separate sidebar/content-side widths) across host and panel views, updates ContentView resizer geometry to match, and adds a scroller-style change observer in GhosttySurfaceScrollView to force immediate terminal surface/width recalculation. Tests adjusted/added accordingly.

Changes

Cohort / File(s) Summary
Portal & host hit-testing
Sources/BrowserWindowPortal.swift, Sources/TerminalWindowPortal.swift
Replaced symmetric hitWidthPerSide with asymmetric sidebarSideHitWidth and contentSideHitWidth when computing divider pass-through regions; pass-through comparison unchanged otherwise.
Panel view pass-through
Sources/Panels/BrowserPanelView.swift
Narrowed/shifted sidebar-resizer guard band to use contentSideHitWidth; simplified hitTest return behavior to always return self for hosted-inspector divider interactions and reworked stored-width reapply gating.
Content view resizer geometry
Sources/ContentView.swift
Switched from a single symmetric hit inset to asymmetric sidebarResizerSidebarHitWidth/sidebarResizerContentHitWidth and totalHitWidth; updated dividerBandContains and overlay handle geometry to match.
Terminal / Ghostty scroll handling & debug plumbing
Sources/GhosttyTerminalView.swift
Added DEBUG-only accessors for pending surface size; register observer for NSScroller.preferredScrollerStyleDidChangeNotification; handler tiles the scroll view and calls synchronizeCoreSurface() to refresh terminal sizing immediately.
Tests
cmuxTests/BrowserPanelTests.swift, cmuxTests/TerminalAndGhosttyTests.swift
Loosened two hit-tolerance assertions; added helpers to build hosted terminal view and centralized hit assertions; added tests for sidebar pass-through edge behavior and for scroller-style notification causing pending surface width recalculation.

Sequence Diagram(s)

sequenceDiagram
    participant OS as macOS
    participant NSScroller as NSScroller
    participant NC as NotificationCenter
    participant GhosttySV as GhosttySurfaceScrollView
    participant NSScroll as NSScrollView
    participant TerminalCore as Ghostty Terminal Core

    OS->>NSScroller: input device change (trackpad ↔ mouse)
    NSScroller->>NC: post preferredScrollerStyleDidChangeNotification
    NC->>GhosttySV: notification received
    activate GhosttySV
    GhosttySV->>GhosttySV: handlePreferredScrollerStyleChange() (ensure main thread)
    GhosttySV->>NSScroll: tile()
    activate NSScroll
    NSScroll->>NSScroll: recalc layout (account for scrollbar width)
    deactivate NSScroll
    GhosttySV->>TerminalCore: synchronizeCoreSurface()
    activate TerminalCore
    TerminalCore->>TerminalCore: recalc grid columns / pendingSurfaceSize
    deactivate TerminalCore
    deactivate GhosttySV
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Poem

🐰 I nibbled the divider into two fine parts,

one gentle for the sidebar, one kind to the charts.
When scrollbars change and the screen rearranges,
I wake the surface so no glyph ever vanishes.
Tiny hops, proud heart — hooray for small changes!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the two main changes: fixing splitter hitbox overlap and resyncing terminal scrollbar width, matching the core objectives of the PR.
Description check ✅ Passed The description includes summary of changes, rationale, testing approach, and commit structure. However, it lacks a demo video and the checklist is incomplete (no checkmarks provided).
Linked Issues check ✅ Passed The PR addresses both issues #1632 objectives: asymmetric sidebar divider hit-testing (narrowed to 2px on content side) and terminal surface width resync on scrollbar style changes via NSScroller notification handling.
Out of Scope Changes check ✅ Passed All changes are scoped to the two linked issues: sidebar divider hit-testing adjustments, terminal scrollbar resync logic, and related test coverage. No unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-1632-splitter-hitbox-scrollbar

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 6 files

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 22, 2026

Greptile Summary

This PR fixes two related UX regressions from issue #1632: (1) the shared sidebar divider's hit-box was eating too far into terminal/browser content (6 px on each side symmetrically), which broke column-0 text selection; it is now asymmetric — 6 px on the sidebar side, 2 px on the content side. (2) When the macOS scrollbar style preference changed from overlay to legacy (or back), the terminal grid width was not recalculated, leaving a gap where the scrollbar gutter had been; a new NSScroller.preferredScrollerStyleDidChangeNotification observer calls scrollView.tile() + synchronizeCoreSurface() to fix this. Both fixes are covered by new regression tests.

Key changes:

  • SidebarResizeInteraction — replaced symmetric hitWidthPerSide with asymmetric sidebarSideHitWidth / contentSideHitWidth pair; all four call-sites updated consistently
  • GhosttySurfaceScrollView — new scroller-style observer with proper main-thread dispatch; handlePreferredScrollerStyleChange intentionally calls only tile() + synchronizeCoreSurface() (not the broader reconcile path) to avoid content glitches
  • GhosttySurfaceScrollView.debugPendingSurfaceSize() — the wrapper is missing a #if DEBUG guard; its inner call to GhosttyNSView.debugPendingSurfaceSize() (which is #if DEBUG) will fail to compile in release builds — this must be fixed before merging
  • New tests use hardcoded 2 instead of SidebarResizeInteraction.contentSideHitWidth, which would silently drift if the constant changes

Confidence Score: 3/5

  • Not safe to merge until the missing #if DEBUG guard on GhosttySurfaceScrollView.debugPendingSurfaceSize() is fixed — it will cause a release build failure.
  • The two functional fixes (asymmetric hit-box and scroller-style width resync) are correct and well-tested. However, the GhosttySurfaceScrollView.debugPendingSurfaceSize() wrapper is compiled unconditionally while its only callee (GhosttyNSView.debugPendingSurfaceSize()) is inside #if DEBUG, producing a guaranteed compile error in release/distribution builds. This is a blocking issue that must be resolved before the PR can be merged.
  • Sources/GhosttyTerminalView.swift — the GhosttySurfaceScrollView.debugPendingSurfaceSize() method must be wrapped in #if DEBUG

Important Files Changed

Filename Overview
Sources/ContentView.swift Replaces symmetric hitWidthPerSide with asymmetric sidebarSideHitWidth (6 px) / contentSideHitWidth (2 px), and updates all three call-sites (dividerBandContains, leading-width calculation, overlay width) consistently.
Sources/GhosttyTerminalView.swift Adds NSScroller.preferredScrollerStyleDidChangeNotification observer that retiles the scroll view and re-syncs the core surface width. Also adds debugPendingSurfaceSize() accessor — the GhosttySurfaceScrollView wrapper is missing a #if DEBUG guard, which will cause a compile error in release builds.
Sources/TerminalWindowPortal.swift Applies the asymmetric sidebar hit-width constants to the terminal portal's shouldPassThroughToSidebarResizer check, consistent with ContentView changes.
Sources/BrowserWindowPortal.swift Mirrors the terminal portal change — uses contentSideHitWidth for the browser portal's sidebar pass-through region.
Sources/Panels/BrowserPanelView.swift Narrows the WKWebView leading-edge pass-through band from the old hitWidthPerSide (6 px) to contentSideHitWidth (2 px), aligning with the reduced terminal-side overlap.
cmuxTests/CmuxWebViewKeyEquivalentTests.swift Adds two new regression suites: terminal host view hit-test boundary test and scrollbar-style width recalculation test. Two existing assertions updated to hardcoded 2 instead of the removed hitWidthPerSide symbol; the new test also uses a local magic literal instead of SidebarResizeInteraction.contentSideHitWidth.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["System posts\nNSScroller.preferredScrollerStyleDidChangeNotification"] --> B{"Is main thread?"}
    B -- No --> C["DispatchQueue.main.async\n(handlePreferredScrollerStyleChange)"]
    C --> D["scrollView.tile()"]
    B -- Yes --> D
    D --> E["synchronizeCoreSurface()"]
    E --> F["scrollView.contentSize.width"]
    F --> G["surfaceView.pushTargetSurfaceSize(...)"]
    G --> H["Terminal grid redrawn\nwith correct column count"]

    subgraph "Sidebar Hit-Test Region (after fix)"
        I["◀ sidebarSideHitWidth = 6 px ▶|divider|◀ contentSideHitWidth = 2 px ▶"]
        J["Sidebar drag target (generous)"]
        K["Terminal/Browser content (text selection wins)"]
    end
Loading

Reviews (1): Last reviewed commit: "fix: narrow sidebar overlap and resync t..." | Re-trigger Greptile

Comment on lines 7408 to +7411
surfaceView.debugSimulateFileDrop(paths: paths)
}

func debugPendingSurfaceSize() -> CGSize? {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 #if DEBUG guard missing on public wrapper

GhosttySurfaceScrollView.debugPendingSurfaceSize() calls surfaceView.debugPendingSurfaceSize(), which is defined only inside a #if DEBUG block on GhosttyNSView. In a release build, the GhosttyNSView method is stripped, so this wrapper will fail to compile with "Value of type 'GhosttyNSView' has no member 'debugPendingSurfaceSize'".

Suggested change
surfaceView.debugSimulateFileDrop(paths: paths)
}
func debugPendingSurfaceSize() -> CGSize? {
#if DEBUG
func debugPendingSurfaceSize() -> CGSize? {
surfaceView.debugPendingSurfaceSize()
}
#endif

}

func testHostViewStopsSidebarPassThroughJustInsideTerminalContent() {
let terminalSideOverlapWidth: CGFloat = 2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Hardcoded magic constant should reference SidebarResizeInteraction

terminalSideOverlapWidth is a local CGFloat literal 2 that duplicates SidebarResizeInteraction.contentSideHitWidth. If the constant ever changes the test will silently diverge from production behaviour. The same pattern appears at lines 10549 and 10904 where the assertion threshold is also hardcoded as 2.

Suggested change
let terminalSideOverlapWidth: CGFloat = 2
let terminalSideOverlapWidth = SidebarResizeInteraction.contentSideHitWidth

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)

10903-10912: ⚠️ Potential issue | 🟡 Minor

Make the edge-distance assertion bidirectional.

This currently passes for any point that lands left of slot.frame.minX, so it can miss a bad coordinate conversion on the browser side. Use an absolute delta, or assert both lower and upper bounds.

💡 Possible fix
-        XCTAssertLessThanOrEqual(dividerPointInHost.x - slot.frame.minX, 2)
+        XCTAssertLessThanOrEqual(abs(dividerPointInHost.x - slot.frame.minX), 2)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 10903 - 10912,
The assertion comparing dividerPointInHost.x to slot.frame.minX is one-sided and
can miss points left of the slot; change the check on dividerPointInHost.x -
slot.frame.minX (currently using XCTAssertLessThanOrEqual) to a bidirectional
check so the absolute difference is within tolerance (e.g. use the absolute
delta of dividerPointInHost.x and slot.frame.minX) or assert both lower and
upper bounds; update the assertion around dividerPointInHost, slot.frame.minX
and the current tolerance (2) to ensure fabs(dividerPointInHost.x -
slot.frame.minX) <= 2 (or equivalent XCTAssertGreaterThanOrEqual/
XCTAssertLessThanOrEqual pair).
🤖 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 12747-12778: Set a known .overlay baseline before measuring to
avoid flaky results: before calling hostedView.debugPendingSurfaceSize() and
reading scrollView.contentSize.width, force scrollView.scrollerStyle = .overlay,
post NSScroller.preferredScrollerStyleDidChangeNotification (or otherwise
trigger the scroller-style observer) and run the runloop to let the UI settle;
then capture initialSurfaceSize and initialContentWidth. This ensures the
initial measurements come from the overlay (max-width) state so subsequent
switch-to-legacy checks on scrollView.scrollerStyle,
hostedView.debugPendingSurfaceSize(), and legacyContentWidth behave
deterministically.

---

Outside diff comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 10903-10912: The assertion comparing dividerPointInHost.x to
slot.frame.minX is one-sided and can miss points left of the slot; change the
check on dividerPointInHost.x - slot.frame.minX (currently using
XCTAssertLessThanOrEqual) to a bidirectional check so the absolute difference is
within tolerance (e.g. use the absolute delta of dividerPointInHost.x and
slot.frame.minX) or assert both lower and upper bounds; update the assertion
around dividerPointInHost, slot.frame.minX and the current tolerance (2) to
ensure fabs(dividerPointInHost.x - slot.frame.minX) <= 2 (or equivalent
XCTAssertGreaterThanOrEqual/ XCTAssertLessThanOrEqual pair).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 40ba91f0-151e-4456-ba3a-21c5f2e80ca7

📥 Commits

Reviewing files that changed from the base of the PR and between 661a4e8 and fe8ae0f.

📒 Files selected for processing (6)
  • Sources/BrowserWindowPortal.swift
  • Sources/ContentView.swift
  • Sources/GhosttyTerminalView.swift
  • Sources/Panels/BrowserPanelView.swift
  • Sources/TerminalWindowPortal.swift
  • cmuxTests/CmuxWebViewKeyEquivalentTests.swift

Comment on lines +12747 to +12778
guard let initialSurfaceSize = hostedView.debugPendingSurfaceSize() else {
XCTFail("Expected an initial terminal surface size")
return
}

let initialContentWidth = scrollView.contentSize.width
XCTAssertEqual(initialSurfaceSize.width, initialContentWidth, accuracy: 0.5)

scrollView.scrollerStyle = .legacy
scrollView.layoutSubtreeIfNeeded()
let legacyContentWidth = scrollView.contentSize.width
XCTAssertLessThan(
legacyContentWidth,
initialContentWidth,
"Legacy scrollbars should reserve width in the scroll view content area"
)
XCTAssertEqual(
hostedView.debugPendingSurfaceSize()?.width,
initialSurfaceSize.width,
accuracy: 0.5,
"Changing the scroll view style alone should leave the terminal grid stale until the scroller-style observer runs"
)

NotificationCenter.default.post(name: NSScroller.preferredScrollerStyleDidChangeNotification, object: nil)
RunLoop.current.run(until: Date().addingTimeInterval(0.05))

XCTAssertEqual(scrollView.scrollerStyle, .legacy)
XCTAssertEqual(
hostedView.debugPendingSurfaceSize()?.width,
legacyContentWidth,
accuracy: 0.5,
"Preferred scroller style changes should recalculate the terminal grid width immediately"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, locate the test file
find . -name "CmuxWebViewKeyEquivalentTests.swift" -type f

Repository: manaflow-ai/cmux

Length of output: 107


🏁 Script executed:

#!/bin/bash
# Read the specific lines from the test file
sed -n '12747,12778p' ./cmuxTests/CmuxWebViewKeyEquivalentTests.swift

Repository: manaflow-ai/cmux

Length of output: 1489


🏁 Script executed:

#!/bin/bash
# Read broader context around these lines to understand test setup
sed -n '12700,12800p' ./cmuxTests/CmuxWebViewKeyEquivalentTests.swift

Repository: manaflow-ai/cmux

Length of output: 4053


Seed the test from a known .overlay baseline to avoid false failures.

The test captures initialContentWidth without explicitly setting a scroller style first. If the test host process boots with .legacy scroller style already active, initialContentWidth will already reflect the narrowed legacy width. When the test then switches to .legacy and measures legacyContentWidth, the two will be equal, causing XCTAssertLessThan to fail even when the notification-resync logic is correct.

Force .overlay style and trigger the notification sync before capturing initial sizes to establish a known maximum-width baseline.

Suggested fix
+        scrollView.scrollerStyle = .overlay
+        scrollView.layoutSubtreeIfNeeded()
+        NotificationCenter.default.post(name: NSScroller.preferredScrollerStyleDidChangeNotification, object: nil)
+        RunLoop.current.run(until: Date().addingTimeInterval(0.05))
+
         guard let initialSurfaceSize = hostedView.debugPendingSurfaceSize() else {
             XCTFail("Expected an initial terminal surface size")
             return
         }
         let initialContentWidth = scrollView.contentSize.width
+        XCTAssertEqual(scrollView.scrollerStyle, .overlay)
         XCTAssertEqual(initialSurfaceSize.width, initialContentWidth, accuracy: 0.5)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 12747 - 12778,
Set a known .overlay baseline before measuring to avoid flaky results: before
calling hostedView.debugPendingSurfaceSize() and reading
scrollView.contentSize.width, force scrollView.scrollerStyle = .overlay, post
NSScroller.preferredScrollerStyleDidChangeNotification (or otherwise trigger the
scroller-style observer) and run the runloop to let the UI settle; then capture
initialSurfaceSize and initialContentWidth. This ensures the initial
measurements come from the overlay (max-width) state so subsequent
switch-to-legacy checks on scrollView.scrollerStyle,
hostedView.debugPendingSurfaceSize(), and legacyContentWidth behave
deterministically.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
cmuxTests/BrowserPanelTests.swift (1)

550-550: Prefer referencing SidebarResizeInteraction.contentSideHitWidth over hardcoding 2.

Both assertions use a hardcoded 2 that corresponds to the production constant SidebarResizeInteraction.contentSideHitWidth. Referencing the constant directly keeps the tests in sync if the hit-zone width ever changes.

♻️ Suggested fix

Line 550:

 XCTAssertTrue(
-    abs(dividerPointInHost.x - slot.frame.minX) <= 2,
+    abs(dividerPointInHost.x - slot.frame.minX) <= SidebarResizeInteraction.contentSideHitWidth,
     "Expected collapsed hosted divider to overlap the browser slot leading-edge resizer zone"
 )

Line 908:

-XCTAssertLessThanOrEqual(dividerPointInHost.x - slot.frame.minX, 2)
+XCTAssertLessThanOrEqual(dividerPointInHost.x - slot.frame.minX, SidebarResizeInteraction.contentSideHitWidth)

Also applies to: 908-908

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/BrowserPanelTests.swift` at line 550, Replace the hardcoded numeric
hit-width 2 in the test assertions with the production constant
SidebarResizeInteraction.contentSideHitWidth so the assertions use
SidebarResizeInteraction.contentSideHitWidth instead of 2 (e.g. update the
expression comparing abs(dividerPointInHost.x - slot.frame.minX) <= 2 and the
similar assertion at the other location to use
SidebarResizeInteraction.contentSideHitWidth); this keeps the tests in sync if
the hit-zone width changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@cmuxTests/BrowserPanelTests.swift`:
- Line 550: Replace the hardcoded numeric hit-width 2 in the test assertions
with the production constant SidebarResizeInteraction.contentSideHitWidth so the
assertions use SidebarResizeInteraction.contentSideHitWidth instead of 2 (e.g.
update the expression comparing abs(dividerPointInHost.x - slot.frame.minX) <= 2
and the similar assertion at the other location to use
SidebarResizeInteraction.contentSideHitWidth); this keeps the tests in sync if
the hit-zone width changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 10a6c814-e01d-4955-8819-44c56e366e08

📥 Commits

Reviewing files that changed from the base of the PR and between fe8ae0f and 1073aad.

📒 Files selected for processing (7)
  • Sources/BrowserWindowPortal.swift
  • Sources/ContentView.swift
  • Sources/GhosttyTerminalView.swift
  • Sources/Panels/BrowserPanelView.swift
  • Sources/TerminalWindowPortal.swift
  • cmuxTests/BrowserPanelTests.swift
  • cmuxTests/TerminalAndGhosttyTests.swift
✅ Files skipped from review due to trivial changes (2)
  • Sources/BrowserWindowPortal.swift
  • Sources/Panels/BrowserPanelView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
  • Sources/TerminalWindowPortal.swift

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/Panels/BrowserPanelView.swift`:
- Around line 5138-5152: The divider hit path in hitTest(_:) currently always
returns self causing right/other mouse events to be handled by
HostContainerView; change the end of that branch to inspect the current NSEvent
(via NSApp.currentEvent) and only accept the hosted-inspector divider hit
(return self) when the event type is a left-button event (e.g. .leftMouseDown,
.leftMouseDragged, .leftMouseUp); for any other event types, return nativeHit so
the event continues to WebKit/Inspector. Ensure this check is applied just
before the debugLogHitTest/return self block and use the existing variables
hostedInspectorHit and nativeHit to decide the fallback.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4228d4fc-6392-4f3e-be25-9cdb5c7774ad

📥 Commits

Reviewing files that changed from the base of the PR and between 8396885 and d77ae21.

📒 Files selected for processing (2)
  • Sources/Panels/BrowserPanelView.swift
  • cmuxTests/TerminalAndGhosttyTests.swift
✅ Files skipped from review due to trivial changes (1)
  • cmuxTests/TerminalAndGhosttyTests.swift

Comment on lines +5138 to 5152
if nativeHit !== hostedInspectorHit.inspectorView &&
!hostedInspectorHit.inspectorView.isDescendant(of: nativeHit) {
return nativeHit
}
}
#if DEBUG
debugLogHitTest(
stage: isSideDockHit ? "hitTest.hostedInspectorManual" : "hitTest.hostedInspectorFallback",
stage: "hitTest.hostedInspectorManual",
point: point,
passThrough: false,
hitView: hostedInspectorHit.inspectorView
hitView: self
)
#endif
return isSideDockHit ? self : hostedInspectorHit.inspectorView
return self
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In AppKit, when NSView.hitTest(_:)returns a container view instead of the underlying descendant, arerightMouseDown(with:)andotherMouseDown(with:) dispatched to that container?

💡 Result:

Yes, when NSView.hitTest(_:) returns a container view instead of a descendant view, the rightMouseDown(with:) and otherMouseDown(with:) events are dispatched to that container view.

Citations:


🏁 Script executed:

# First, verify the file exists and examine the code around lines 5138-5152
wc -l Sources/Panels/BrowserPanelView.swift

Repository: manaflow-ai/cmux

Length of output: 102


🏁 Script executed:

# Read the specific section mentioned in the review
sed -n '5130,5160p' Sources/Panels/BrowserPanelView.swift

Repository: manaflow-ai/cmux

Length of output: 1177


🏁 Script executed:

# Find HostContainerView class to check mouse event handlers
rg -A 20 "class HostContainerView" Sources/Panels/

Repository: manaflow-ai/cmux

Length of output: 1697


🏁 Script executed:

# Get the full HostContainerView class implementation with all methods
rg -A 200 "^\s*final class HostContainerView: NSView" Sources/Panels/BrowserPanelView.swift | head -300

Repository: manaflow-ai/cmux

Length of output: 10329


🏁 Script executed:

# More specific search for mouse event handlers in HostContainerView
rg "(mouseDown|rightMouseDown|otherMouseDown|mouseMoved)" Sources/Panels/BrowserPanelView.swift | grep -A 2 -B 2 "HostContainerView\|func.*[mM]ouse"

Repository: manaflow-ai/cmux

Length of output: 511


🏁 Script executed:

# Search for all methods in HostContainerView to understand its structure
ast-grep --pattern 'final class HostContainerView {
  $$$
}' | head -100

Repository: manaflow-ai/cmux

Length of output: 42


🏁 Script executed:

# Search for mouse event handlers specifically in HostContainerView
rg -n "override func (mouseDown|rightMouseDown|otherMouseDown|mouseMoved)" Sources/Panels/BrowserPanelView.swift

Repository: manaflow-ai/cmux

Length of output: 236


🏁 Script executed:

# Get the full HostContainerView class boundaries to understand its scope
rg -n "^\s*final class HostContainerView|^    }" Sources/Panels/BrowserPanelView.swift | head -20

Repository: manaflow-ai/cmux

Length of output: 255


🏁 Script executed:

# Find the exact line where HostContainerView is defined and where it ends
rg -n "final class HostContainerView" Sources/Panels/BrowserPanelView.swift

Repository: manaflow-ai/cmux

Length of output: 108


🏁 Script executed:

# Now find the closing brace of HostContainerView class by searching for its extent
sed -n '4705,4750p' Sources/Panels/BrowserPanelView.swift

Repository: manaflow-ai/cmux

Length of output: 2276


🏁 Script executed:

# Get a better view - search for where HostContainerView class definition starts
grep -n "final class HostContainerView" Sources/Panels/BrowserPanelView.swift

Repository: manaflow-ai/cmux

Length of output: 108


🏁 Script executed:

# Now that we know HostContainerView likely starts around line 4705+, let's find mouse handlers around that area
sed -n '5150,5180p' Sources/Panels/BrowserPanelView.swift

Repository: manaflow-ai/cmux

Length of output: 1354


🏁 Script executed:

# Search for rightMouseDown and otherMouseDown in entire HostContainerView class
# We know it starts at line 4287, so let's search after that line
rg -n "override func (rightMouseDown|otherMouseDown)" Sources/Panels/BrowserPanelView.swift

Repository: manaflow-ai/cmux

Length of output: 42


🏁 Script executed:

# Verify the mouseMoved handler around line 5115 is in HostContainerView
sed -n '5110,5125p' Sources/Panels/BrowserPanelView.swift

Repository: manaflow-ai/cmux

Length of output: 699


🏁 Script executed:

# Get a complete picture - show all override methods in HostContainerView starting from line 4287
sed -n '4287,6392p' Sources/Panels/BrowserPanelView.swift | grep -n "override func" | head -20

Repository: manaflow-ai/cmux

Length of output: 922


🏁 Script executed:

# Final confirmation - view the exact hitTest logic one more time to ensure understanding
sed -n '5115,5160p' Sources/Panels/BrowserPanelView.swift

Repository: manaflow-ai/cmux

Length of output: 1923


Gate divider hits to left-button events only.

hitTest(_:) unconditionally returns self for hosted-inspector divider hits, but HostContainerView only implements mouseDown, mouseDragged, and mouseUp handlers—not rightMouseDown or otherMouseDown. Secondary-button clicks in the divider band will dispatch to the host container instead of reaching WebKit/Inspector.

Add an event-type check before accepting the divider hit:

Suggested adjustment
-            if let hostedInspectorHit {
-                if let nativeHit = nativeHostedInspectorHit(at: point, hostedInspectorHit: hostedInspectorHit) {
+            if let hostedInspectorHit {
+                let nativeHit = nativeHostedInspectorHit(at: point, hostedInspectorHit: hostedInspectorHit)
+                if let nativeHit {
 `#if` DEBUG
                     debugLogHitTest(stage: "hitTest.hostedInspectorNative", point: point, passThrough: false, hitView: nativeHit)
 `#endif`
                     if nativeHit !== hostedInspectorHit.inspectorView &&
                         !hostedInspectorHit.inspectorView.isDescendant(of: nativeHit) {
                         return nativeHit
                     }
                 }
+                let shouldUseManualDividerHit: Bool = {
+                    switch NSApp.currentEvent?.type {
+                    case .leftMouseDown, .leftMouseDragged, .leftMouseUp:
+                        return true
+                    default:
+                        return false
+                    }
+                }()
+                guard shouldUseManualDividerHit else {
+                    return nativeHit ?? super.hitTest(point)
+                }
 `#if` DEBUG
                 debugLogHitTest(
                     stage: "hitTest.hostedInspectorManual",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/Panels/BrowserPanelView.swift` around lines 5138 - 5152, The divider
hit path in hitTest(_:) currently always returns self causing right/other mouse
events to be handled by HostContainerView; change the end of that branch to
inspect the current NSEvent (via NSApp.currentEvent) and only accept the
hosted-inspector divider hit (return self) when the event type is a left-button
event (e.g. .leftMouseDown, .leftMouseDragged, .leftMouseUp); for any other
event types, return nativeHit so the event continues to WebKit/Inspector. Ensure
this check is applied just before the debugLogHitTest/return self block and use
the existing variables hostedInspectorHit and nativeHit to decide the fallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar splitter hitbox extends too far into terminal; text cut off behind scrollbar on display config change

1 participant