Skip to content

Fix sidebar hover drag-handle event dispatch - #4818

Closed
austinywang wants to merge 5 commits into
mainfrom
issue-750-cmux-shutdowns-when-move-mouse-pointer
Closed

austinywang wants to merge 5 commits into
mainfrom
issue-750-cmux-shutdowns-when-move-mouse-pointer

Conversation

@austinywang

@austinywang austinywang commented May 26, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #750

Summary

  • Add regression coverage for stale titlebar drag-handle event dispatch before the source fix.
  • Follow-up commit will make drag-handle view hit-testing depend on active NSWindow event dispatch, not stale NSApp.currentEvent.

Test plan

  • Not run locally per repository policy.
  • CI should fail on the first test-only commit, then pass after the fix commit.

Reproduction

  • Existing issue report reproduces on cmux 0.61.0 / macOS 14.5 by hovering the sidebar top button group.
  • PR Fix exclusive-access crash in WindowDragHandleView hitTest #736 already documents reproduction on v0.61.0 and fixed-after verification for the direct hover path; this PR closes the remaining stale-currentEvent class around that same drag-handle route.

Note

Low Risk
Localized AppKit event/hit-test plumbing for the titlebar drag handle with tests; no auth, data, or broad behavior changes beyond fixing stale-event drags.

Overview
Fixes unintended titlebar/window drags when sidebar hover or layout hit-testing sees a stale leftMouseDown on NSApp.currentEvent (#750).

Window event dispatch is now bracketed in sendEvent with beginWindowDragHandleEventDispatch / endWindowDragHandleEventDispatch, maintaining a main-thread stack of active window + event type. WindowDragHandleView only participates in hitTest when that stack says a matching leftMouseDown is being delivered—not on passive events like mouseMoved.

Regression tests cover stale vs active dispatch and that hover stays transparent during dispatch.

Reviewed by Cursor Bugbot for commit 6ec7079. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Fixes unintended titlebar drags by scoping the drag-handle hit-test to active window event dispatch; fixes #750. Wraps NSWindow.sendEvent with begin/end tokens backed by a main-thread-asserted dispatch stack so only an in-flight leftMouseDown can capture; stale NSApp.currentEvent reads and hover/passive events are ignored, with tests for stale, active, and hover cases.

Written for commit 68f1924. Summary will update on new commits. Review in cubic


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.

Summary by CodeRabbit

  • Bug Fixes
    • Improved window drag handle hit-testing so stale or unrelated mouse events no longer trigger unintended drags.
  • Tests
    • Added coverage verifying drag-handle hit-testing rejects stale and passive events unless part of an active drag dispatch.

Review Change Stack

@vercel

vercel Bot commented May 26, 2026 •

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment May 26, 2026 8:33pm
cmux-staging Building Building Preview, Comment May 26, 2026 8:33pm

@coderabbitai

coderabbitai Bot commented May 26, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR adds a dispatch-token stack for window-drag event dispatch, instruments NSWindow.cmux_sendEvent(_) to push/pop tokens during dispatch, updates WindowDragHandleView.hitTest to consult the dispatch state, and adds tests plus an NSEvent factory to validate gating behavior.

Changes

Window-drag dispatch + tests

Layer / File(s) Summary
Dispatch state, token type, and APIs
Sources/WindowDragHandleView.swift
Introduces WindowDragHandleEventDispatchToken and a private global dispatch stack that records window and event type per active dispatch; adds beginWindowDragHandleEventDispatch, endWindowDragHandleEventDispatch, and windowDragHandleViewHitTestingAllowsCurrentEvent(_:) predicate used to decide allowed hit-testing events.
NSWindow event dispatch instrumentation
Sources/AppDelegate.swift
Wraps NSWindow.cmux_sendEvent(_:) dispatch with beginWindowDragHandleEventDispatch(window:event:) and defers endWindowDragHandleEventDispatch(_) so a token exists for the duration of dispatch.
Tests and NSEvent factory helper
cmuxTests/WindowAndDragTests.swift
Adds WindowDragHandleHitTests.makeMouseEvent(...) helper and two tests: testDragHandleViewHitTestingRequiresActiveWindowEventDispatch (stale leftMouseDown rejected until dispatch token active) and testDragHandleViewHitTestingStillRejectsPassiveEventsDuringDispatch (passive events remain rejected even during active dispatch).

Sequence Diagram(s)

sequenceDiagram
  participant NSWindow_cmux_sendEvent
  participant DispatchAPI as beginWindowDragHandleEventDispatch/endWindowDragHandleEventDispatch
  participant WindowDragHandleView_hitTest
  NSWindow_cmux_sendEvent->>DispatchAPI: beginWindowDragHandleEventDispatch(window,event)
  NSWindow_cmux_sendEvent->>WindowDragHandleView_hitTest: dispatch event / hitTest calls
  WindowDragHandleView_hitTest->>DispatchAPI: windowDragHandleViewHitTestingAllowsCurrentEvent(currentEvent)?
  DispatchAPI-->>WindowDragHandleView_hitTest: allowed / rejected
  NSWindow_cmux_sendEvent->>DispatchAPI: endWindowDragHandleEventDispatch(token) (deferred)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • manaflow-ai/cmux#4290: Shares window-drag and event-dispatch flow changes related to pane/tab/drag handling.

Poem

🐰 I patch the hop where drags were lost,
A token stack to count the cost,
Stale clicks turned away, motion stays aloof,
Tests make sure the gate is proof,
Hop hop—events now behave!


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Cmux Swift Actor Isolation ❌ Error New public functions access nonisolated(unsafe) mutable shared state without @MainActor annotation or dispatchPrecondition, violating Swift 6 actor isolation rules. Add @MainActor to the three new functions or add dispatchPrecondition(condition: .onQueue(.main)) to enforce main-thread access at runtime, as suggested in review comments.
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.
Description check ❓ Inconclusive PR description comprehensively covers changes, testing approach, and context but does not follow the template structure with all required sections clearly organized. Reorganize description to match template: add explicit 'Summary' section summarizing what/why, 'Testing' section detailing verification, 'Demo Video' section if applicable, and ensure checklist items are addressed.
✅ Passed checks (14 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing drag-handle event dispatch for the sidebar hover issue.
Linked Issues check ✅ Passed The PR directly addresses issue #750 by implementing dispatch token tracking and updating hit-testing logic to prevent stale currentEvent from triggering drag-handle activation.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to addressing the stale event dispatch issue: test coverage, dispatch token mechanism, and hit-testing updates tied directly to #750.
Cmux Swift Blocking Runtime ✅ Passed Production code introduces no blocking/timing synchronization. Uses only deterministic main-thread-only state tracking with nonisolated(unsafe) variables, no semaphores, locks, sleeps, or sync calls.
Cmux No Hacky Sleeps ✅ Passed All changes are in Swift files. The rule explicitly covers only TypeScript, JavaScript, shell, or build/runtime scripts, with Swift code deferred to swift-blocking-runtime.md.
Cmux Swift Concurrency ✅ Passed Code adds synchronous AppKit event dispatch lifecycle management using nonisolated(unsafe) state, not legacy async patterns like dispatch queues or fire-and-forget Tasks.
Cmux Swift @Concurrent ✅ Passed All new functions are synchronous and main-thread-only; no async functions lack @concurrent, no invalid @concurrent usage, and no heavy async work without explicit hops.
Cmux Swift File And Package Boundaries ✅ Passed PR adds +27 net lines to WindowDragHandleView.swift (below >250 threshold). Event dispatch code is focused AppKit window glue, appropriately placed for app-target, with good test coverage.
Cmux Swift Logging ✅ Passed No logging violations detected. The PR adds no print, debugPrint, dump, NSLog, or Logger statements in production code, and test code logging is permitted.
Cmux User-Facing Error Privacy ✅ Passed No user-facing error messages or sensitive information exposed. All new code is internal implementation with no user-visible output.
Cmux Full Internationalization ✅ Passed All changes are internal implementation details with no user-facing text. Test file additions are exempt. No localization changes required.
Cmux Swiftui State Layout ✅ Passed PR introduces no new SwiftUI state patterns. Changes are AppKit event dispatch infrastructure only. Existing @Published state is legacy code touched incidentally (allowed per rules).
Cmux Architecture Rethink ✅ Passed Synchronous dispatch-lifecycle witness stack distinguishes active from stale events. No timing repairs, locks, observers, split ownership, or symptom patterns. Clear invariant and root-cause fix.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR adds only internal event dispatch tracking utilities without creating or materially changing any standalone user-visible windows, only test-only fixture windows.
✨ 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-750-cmux-shutdowns-when-move-mouse-pointer

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.

@greptile-apps

greptile-apps Bot commented May 26, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes unintended titlebar drags (#750) by replacing the naive NSApp.currentEvent?.type == .leftMouseDown check in WindowDragHandleView.hitTest with a dispatch-stack gate that only allows hits during active NSWindow.sendEvent delivery of a leftMouseDown. AppDelegate's sendEvent override now brackets each event with beginWindowDragHandleEventDispatch / endWindowDragHandleEventDispatch (via defer), and two regression tests cover the stale-event and passive-event rejection cases.

  • WindowDragHandleView.swift: Introduces WindowDragHandleEventDispatchState, a process-global static stack of in-flight dispatch frames, and replaces the raw event-type guard in hitTest with windowDragHandleViewHitTestingAllowsCurrentEvent, which requires both a leftMouseDown type and an active matching frame.
  • AppDelegate.swift: Adds a three-line bracket around the existing sendEvent body to push/pop the dispatch frame with a defer-cleaned token.
  • cmuxTests/WindowAndDragTests.swift: Adds a semantically correct makeMouseEvent helper and two regression tests verifying that stale and passive events are rejected before and during dispatch respectively.

Confidence Score: 5/5

Safe to merge — the change is tightly scoped to AppKit hit-test plumbing with no auth, data, or broad behavioral impact, and the defer-based cleanup ensures the dispatch stack stays consistent even on early returns.

The production logic change is small and well-guarded: defer ensures token cleanup regardless of early returns in sendEvent, the dispatchPrecondition calls enforce main-thread invariants at runtime, and the two new regression tests directly cover the stale-event and passive-event paths. No data flow, auth, or user-facing behavior is touched beyond the drag-handle hit path.

Sources/WindowDragHandleView.swift — the new dispatch-state enum uses nonisolated(unsafe) static storage rather than @MainActor; worth revisiting when the follow-up structural fix lands.

Important Files Changed

Filename Overview
Sources/WindowDragHandleView.swift Adds a main-thread dispatch stack (begin/end tokens) to distinguish real leftMouseDown delivery from stale NSApp.currentEvent reads; hitTest now requires an active dispatch frame rather than just checking the event type. Uses nonisolated(unsafe) static vars guarded by dispatchPrecondition instead of @MainActor.
Sources/AppDelegate.swift Wraps the NSWindow.sendEvent override with beginWindowDragHandleEventDispatch / endWindowDragHandleEventDispatch (via defer) to bracket the dispatch lifetime for the new hit-test guard. Change is minimal and the defer ensures cleanup even on early returns.
cmuxTests/WindowAndDragTests.swift Adds makeMouseEvent helper (correctly setting click count and pressure to 0 for non-button events) and two new regression tests: one verifying that a stale leftMouseDown is rejected without an active dispatch token, and one confirming that passive mouseMoved events are rejected even when inside a dispatch bracket.

Sequence Diagram

sequenceDiagram
    participant AppKit
    participant NSWindow_sendEvent as NSWindow.sendEvent
    participant DispatchState as WindowDragHandleEventDispatchState
    participant hitTest as WindowDragHandleView.hitTest

    AppKit->>NSWindow_sendEvent: leftMouseDown event
    NSWindow_sendEvent->>DispatchState: begin(window, event) → token
    NSWindow_sendEvent->>AppKit: super.sendEvent(event)
    AppKit->>hitTest: hitTest(point)
    hitTest->>DispatchState: containsActiveDispatch(event)?
    DispatchState-->>hitTest: true (frame present)
    hitTest-->>AppKit: self (drag handle captures hit)
    NSWindow_sendEvent->>DispatchState: defer: end(token)

    Note over AppKit,hitTest: Stale / passive event (no active frame)
    AppKit->>hitTest: hitTest(point) [mouseMoved or stale leftMouseDown]
    hitTest->>DispatchState: containsActiveDispatch(event)?
    DispatchState-->>hitTest: false (no matching frame)
    hitTest-->>AppKit: nil (transparent, no drag)
Loading

Reviews (4): Last reviewed commit: "fix: return drag dispatch token after pr..." | Re-trigger Greptile

Comment thread cmuxTests/WindowAndDragTests.swift
coderabbitai[bot]
coderabbitai Bot previously requested changes May 26, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Sources/WindowDragHandleView.swift`:
- Around line 199-215: The three public functions
beginWindowDragHandleEventDispatch(window:event:),
endWindowDragHandleEventDispatch(_:), and
windowDragHandleViewHitTestingAllowsCurrentEvent(_:) call into the
main-thread-only WindowDragHandleEventDispatchState but lack runtime
enforcement; add a main-thread precondition (e.g.
dispatchPrecondition(condition: .onQueue(.main)) or equivalent) at the start of
each of these functions to assert they run on the main thread (matching the
pattern used in minimalModeTrafficLightFrameInContentCoordinates) so misuse is
caught early.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ae08c139-a145-4bad-baf2-14e493ca38bd

📥 Commits

Reviewing files that changed from the base of the PR and between d9e0da1 and cb441e8.

📒 Files selected for processing (2)
  • Sources/AppDelegate.swift
  • Sources/WindowDragHandleView.swift

Comment thread Sources/WindowDragHandleView.swift
@austinywang
austinywang dismissed coderabbitai[bot]’s stale review May 26, 2026 21:14

CodeRabbit confirmed the requested main-thread precondition change is addressed, the inline thread is resolved, and the latest CodeRabbit status check passes.

@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026

This branch was successfully deployed

1 active deployment
Preview – cmux — 68f1924d Deployed May 26, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cmux shutdowns when move mouse pointer at sidebar top button group

3 participants