Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSingle-clicks in sidebar empty areas now support threshold-based window dragging through an AppKit event handler. The sidebar delegates eligible presses to the handler. Tests cover dragging, click preservation, cancellation, movability restoration, and inactive-window handling. ChangesSidebar empty-area dragging
Weak test reference fix
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SidebarWorkspaceTableView
participant SidebarEmptyAreaWindowDragController
participant NSWindow
SidebarWorkspaceTableView->>SidebarEmptyAreaWindowDragController: handle single-click
SidebarEmptyAreaWindowDragController->>NSWindow: track pointer events
SidebarEmptyAreaWindowDragController->>NSWindow: performDrag(with:) after threshold
SidebarEmptyAreaWindowDragController->>NSWindow: restore movability
🚥 Pre-merge checks | ✅ 23 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (23 passed)
✨ Finishing Touches🧪 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 |
|
@codex review |
@kenifxyz I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 2 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 703341fe60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/VerticalTabsSidebar+EmptyAreasAndFooter.swift (1)
103-107: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftRemove the test-only event-pump seam from production code.
nextEventis only used by the supplied tests to replace AppKit’s real event pump. Keep the production method tied to its real event source and move deterministic event-sequence testing behind a test-target helper or isolated state-machine test. As per path instructions, production Swift source must not add test-only or debug-only seams underSources/**.🤖 Prompt for 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. In `@Sources/VerticalTabsSidebar`+EmptyAreasAndFooter.swift around lines 103 - 107, Remove the injectable nextEvent parameter from perform and bind the event-pumping logic directly to AppKit’s production event source. Update callers and tests accordingly, moving deterministic event-sequence control into a test-target helper or isolated state-machine test without adding test-only seams under Sources.Source: Path instructions
🤖 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.
Outside diff comments:
In `@Sources/VerticalTabsSidebar`+EmptyAreasAndFooter.swift:
- Around line 103-107: Remove the injectable nextEvent parameter from perform
and bind the event-pumping logic directly to AppKit’s production event source.
Update callers and tests accordingly, moving deterministic event-sequence
control into a test-target helper or isolated state-machine test without adding
test-only seams under Sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bc9588f2-d7b3-4bc0-bd24-1f94b043567f
📒 Files selected for processing (1)
Sources/VerticalTabsSidebar+EmptyAreasAndFooter.swift
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@austinywang any chance i could get a review on this please? would really improve the quality of life of cmux! |
251673a to
1e95362
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmuxTests/TitlebarInteractiveControlTests.swift`:
- Around line 324-340: The pressWithoutMovementIsNotADrag test should also
verify mouse-up replay, not just that dragging is suppressed. After
SidebarEmptyAreaWindowDrag.perform returns, dequeue the queued event and assert
it is the original mouse-up, and add equivalent coverage for movement below the
drag threshold to ensure both non-drag paths repost the terminating event.
In `@Sources/VerticalTabsSidebar`+EmptyAreasAndFooter.swift:
- Around line 561-613: Convert SidebarEmptyAreaWindowDrag from a static
namespace into a constructable `@MainActor` handler with immutable nextEvent and
drag-threshold dependencies. Move pumpTrackingEvent and perform to instance
methods, remove the static-only entry points, and update each AppKit boundary to
create or receive the handler and call its instance method while preserving
injectable event-pump behavior for tests.
🪄 Autofix
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 Plus
Run ID: f4365033-1961-4f10-8b6c-fdf9d2cdf47a
📒 Files selected for processing (4)
Sources/Sidebar/AppKitList/SidebarWorkspaceTableClipView.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableViewImpl.swiftSources/VerticalTabsSidebar+EmptyAreasAndFooter.swiftcmuxTests/TitlebarInteractiveControlTests.swift
|
Maintainer follow-up is complete on the upstream branch, now at Review fixes:
The latest focused workflow run 31148661946 completed successfully on the updated branch. On the same upstream tip, the canonical structured review reports no findings, the cmux architecture policy is clean, Xcode project normalization and all 652 test-file wiring checks pass, and the merge simulation against current The latest review feedback is fully addressed:
This PR still points to |
|
Final upstream tip @kenifxyz, please fast-forward |
There was a problem hiding this comment.
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 `@cmuxTests/TitlebarInteractiveControlTests.swift`:
- Around line 403-409: Update the detached-view test around
SidebarEmptyAreaWindowDragController to count invocations of the injected
nextEvent closure, then assert that count remains zero alongside the existing
passThrough and performDrag assertions.
🪄 Autofix
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 Plus
Run ID: 68afc8f3-8159-4fd9-b3b5-e39db866c7c1
📒 Files selected for processing (9)
Sources/Sidebar/AppKitList/SidebarWorkspaceTableClipView.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableViewImpl.swiftSources/Sidebar/SidebarEmptyAreaWindowDragController.swiftSources/Sidebar/SidebarEmptyAreaWindowDragOutcome.swiftSources/Sidebar/SidebarEmptyAreaWindowDragTrackingEvent.swiftSources/VerticalTabsSidebar+EmptyAreasAndFooter.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AppDelegateEqualizeSplitsShortcutTests.swiftcmuxTests/TitlebarInteractiveControlTests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1004eb8. Configure here.
|
Final review follow-up is now @kenifxyz, please fast-forward the fork branch from |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Sidebar/SidebarEmptyAreaWindowDragController.swift (1)
21-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject invalid drag thresholds.
dragThresholdaccepts negative and non-finite values. A negative value starts dragging on the first tracked movement. A NaN value prevents any drag becausedistance >= dragThresholdis always false. Validate the threshold before storing it so the controller cannot represent an invalid drag policy.As per coding guidelines, Swift changes must not leave invalid state representable.
🤖 Prompt for 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. In `@Sources/Sidebar/SidebarEmptyAreaWindowDragController.swift` around lines 21 - 24, Validate dragThreshold in the initializer of SidebarEmptyAreaWindowDragController before storing it, rejecting negative and non-finite values so only valid finite non-negative thresholds can be represented. Preserve the existing default and nextEvent behavior while ensuring invalid input cannot initialize the controller.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@Sources/Sidebar/SidebarEmptyAreaWindowDragController.swift`:
- Around line 21-24: Validate dragThreshold in the initializer of
SidebarEmptyAreaWindowDragController before storing it, rejecting negative and
non-finite values so only valid finite non-negative thresholds can be
represented. Preserve the existing default and nextEvent behavior while ensuring
invalid input cannot initialize the controller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a0001825-b7e2-4be1-ac21-cec2aff2f3d1
📒 Files selected for processing (2)
Sources/Sidebar/SidebarEmptyAreaWindowDragController.swiftcmuxTests/TitlebarInteractiveControlTests.swift
|
Threshold-validation follow-up is complete on the upstream branch at
This fixes CodeRabbit’s outside-diff threshold finding at the controller ownership boundary and adds a real red/green regression history. @kenifxyz, please fast-forward |
|
Final upstream evidence for
@kenifxyz, please fast-forward the fork PR branch to |
|
@kenifxyz One-hour synchronization follow-up: #9212 still points to From a clone where git fetch https://github.com/manaflow-ai/cmux.git feature/sidebar-empty-area-window-drag
git push origin FETCH_HEAD:feature/sidebar-empty-area-window-dragThe final tip has a green exact-head focused run, a clean canonical autoreview, a clean cmux policy gate, and a clean merge simulation against current main. I am monitoring and will close out required PR checks as soon as the head updates. |
|
Synced to The branch is now one commit ahead of The bug: two tests failed on macOS 26 and no runner could see it
#expect(replayed.locationInWindow == original.locationInWindow)
So the assertion tracked display geometry and AppKit's event-reconstitution behaviour, which differs across macOS majors, rather than replay itself. The headless macOS 15 runner does not reproduce it, which is why every exact-head check stayed green while the suite was red on real macOS 26 hardware. To be clear, this was a test-fixture bug and not a product bug. The tests covering actual drag behaviour all passed, and the controller is fine.
Objection to
|
done! |
|
@austinywang branch is synced at |
14c454e to
9570dd4
Compare
9570dd4 to
f9ee034
Compare
Greptile SummaryAdds native window dragging from otherwise empty sidebar space while preserving existing click behavior.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code failure identified. The new behavior is limited to single presses in native sidebar regions classified as empty, preserves non-drag clicks by replaying the terminating mouse-up, bypasses row and double-click handling, and restores temporary window state after dragging. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Single mouse-down in sidebar] --> B{Native hit target}
B -->|Workspace row| C[Existing table handling]
B -->|Empty table or clip area| D[Track drag and mouse-up events]
D --> E{Travel at least 4 points?}
E -->|Yes| F[Temporarily enable window movement]
F --> G[Perform window drag]
E -->|No, mouse-up| H[Repost mouse-up]
H --> I[Continue normal mouse-down handling]
D -->|Cancelled| J[Consume cancelled sequence]
Reviews (1): Last reviewed commit: "Merge current main and finalize sidebar ..." | Re-trigger Greptile |
|
Maintainer closeout for synchronized PR head f9ee034:\n\n- Fast-forwarded the reviewed branch onto current main and updated the fork PR head with an exact --force-with-lease (observed old head 9570dd4).\n- Kept the drag threshold as the controller's private 4pt policy; removed the unreachable production precondition and process-exit tests.\n- Removed the macOS-window-geometry-sensitive locationInWindow replay assertion; event identity and replay behavior remain covered.\n- The unrelated AppDelegateEqualizeSplitsShortcutTests edit is no longer in the PR diff.\n\nVerification: current-base canonical autoreview clean (Codex 0.95), cmux policy clean, PBX test-wiring lint passed for 702 files, project normalization and diff checks passed, and all 7 GitHub PR checks are green.\n\nThe tagged reload was attempted twice but the host ran out of disk space during GhosttyKit/SwiftPM diagnostics (NoSpaceLeft); no app was launched or dogfood approval claimed. PR remains open and mergeable pending tagged dogfood and explicit merge approval. |
|
Final gate after base 3084031: current-base autoreview and cmux policy are clean, all 7 PR checks are green, and GitHub reports the PR mergeable. The only remaining gate is runtime dogfood; the tagged build cannot complete on this host while disk-full SwiftPM/Metal diagnostics persist. No merge was performed. |
|
Follow-up build attempt: using the preprovisioned cached GhosttyKit and disabled automatic package resolution reached cmux Swift compilation, but Xcode failed with NoSpaceLeft while writing index/object files. No source/compiler diagnostic was reported. The tagged DerivedData was removed afterward; no code or PR changes were made. |
f9ee034 to
e43cf11
Compare
|
@austinywang is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
The empty space below the last workspace group did not move the window when dragged, though the titlebar strip above the sidebar did. macOS users expect background-drag anywhere in a sidebar, and issue manaflow-ai#3119 records the non-minimal sidebar as already behaving that way. Track the press synchronously from mouseDown with nextEvent(matching:), the same shape as SidebarDividerTrackingView, rather than through an NSGestureRecognizer. NSWindow.performDrag(with:) runs its own modal tracking loop and consumes the terminating mouse-up, so a recognizer that calls it never receives the event that would drive it back to .possible: it parks in a terminal state, reset() never runs, and it stops recognizing for the rest of the window's life. A press that never travels 4pt is handed back intact - the mouse-up is reposted before falling through, because NSTableView's own mouseDown tracking loop blocks waiting for it. Double-clicks bypass the drag path entirely so doubleClickEmptyArea() still fires, and the table view gates on row(at:) < 0 so rows keep their existing click handling. Fixes manaflow-ai#9203
The vertically expanding SidebarEmptyArea is mounted as a full-height background behind the rows in workspaceScrollContent, where a SwiftUI shield sized to the rows stops end-of-list interactions falling through. That shield cannot block an AppKit view, so a representable there would out-hit-test it and turn row presses into window drags. The AppKit sidebar covers this region through the table and clip views, so only the legacy SwiftUI sidebar loses empty-area window drag.
e43cf11 to
04d2929
Compare
|
Top-level review disposition, re-checked against PR HEAD
The five inline threads are all resolved with replies. No review is |
Final review audit (re-checked against PR HEAD)HEAD is Thread IDs re-checked:
The seam disagreement is intentional: the controller's event source is a constructor-injected production dependency with AppKit as its default, which is the testable ownership boundary required by cmux architecture. The threshold disagreement is intentional: production callers cannot provide a threshold; the shipped policy is a private fixed 4-point value. The Current gates: GitHub reports |
3 participants
Summary
mouseDownwithnextEvent(matching:)instead of anNSGestureRecognizer.Fixes #9203
Root cause
The empty region below the workspace list sits inside the sidebar's
NSScrollView.NSScrollViewreportsmouseDownCanMoveWindow == false, so the scroll-view background swallowed the press and AppKit's background-drag never engaged.mouseDownCanMoveWindowis wired up in the titlebar accessory views,WindowDragHandleView, andMinimalModeSidebarControls, but never reached the left sidebar's list body — which is why #3119 could describe the non-minimal sidebar as draggable while this specific region was not.The first attempt here used an
NSGestureRecognizerand was wrong in a way worth recording:NSWindow.performDrag(with:)runs its own modal tracking loop and consumes the terminating mouse-up. A recognizer that calls it therefore never receives the event that would drive it back to.possible— it parks in a terminal state,reset()never runs, and it silently stops recognizing for the rest of the window's life. Manual testing showed exactly that: the drag worked, then stopped working permanently.Design
SidebarEmptyAreaWindowDrag.perform(with:in:)runs a synchronous tracking loop frommouseDown, the same shape asSidebarDividerTrackingViewin the same directory:performDrag, consume the event, returntrue.postEvent(_:atStart: true)and returnfalse, so the caller falls through to normal handling.NSTableView's ownmouseDowntracking loop blocks waiting for that mouse-up; swallowing it would hang the click.Three call sites share the one path. The table view gates on
row(at:) < 0so rows keep their existing click handling; the clip view needs no gate because presses only reach it when they miss the table's frame entirely. Double-clicks bypass the drag path sodoubleClickEmptyArea()still fires. Window-drag suppression is respected via the existingisWindowDragSuppressed/withTemporaryWindowMovableEnabledhelpers, matchingTitlebarAccessoryContainerView.Testing
SidebarEmptyAreaWindowDragTests, 4/4 passing locally viascripts/test-unit.sh -only-testing:cmuxTests/SidebarEmptyAreaWindowDragTests: drag past threshold moves the window and restoresisMovable; press without movement stays a click; sub-threshold jitter stays a click; a detached view is a no-op.CMUX_SKIP_ZIG_BUILD=1because Xcode 26.6 no longer bundles the Metal toolchain, so the Ghostty CLI helper phase cannot compileshaders.metalhere. That phase is untouched by this change.Heads-up:
cmuxTestsdoes not compile on currentmainTwo pre-existing breaks, both in files this PR does not touch, block the whole test target. I stubbed them locally to get a run and reverted before committing.
cmuxTests/WorkspaceRemoteConnectionTests.swift:24— Fix stale SSH workspace connection status #9085 addedbeginReadinessDelivery()andfinishReadinessDelivery(succeeded:)toControlRemotePTYLifecycleCommitLeasewithout updatingManualRemotePTYLifecycleCommitLease.cmuxTests/AppDelegateEqualizeSplitsShortcutTests.swift:7894— call is missing the now-requiredmanagedAgentResumeBindingargument.CI on this PR will likely fail on both until they are fixed on
main. Happy to fold the fixes in here if maintainers prefer, but I have kept them out to hold scope.Demo Video
Not attached. The behavior is a pointer interaction covered by the four behavior tests plus manual verification on a tagged DEBUG build. Happy to record a clip if a reviewer would like one.
Checklist
CHANGELOG.mdis assembled per release by maintainers, and recent merged PRs do not touch it)Summary by CodeRabbit
Note
Medium Risk
Touches low-level AppKit mouse tracking and
performDragon the main sidebar table/clip views; incorrect event handling could break row clicks or hang tracking, but behavior is gated to empty space and covered by tests.Overview
Adds window dragging from empty space below the last workspace row in the native AppKit sidebar, aligned with titlebar and other background-drag regions (fixes #9203).
A new
SidebarEmptyAreaWindowDragControllerruns a synchronous tracking loop frommouseDown(same pattern asSidebarDividerTrackingView), not anNSGestureRecognizer, becauseperformDrag(with:)breaks recognizer state permanently. Movement past a 4pt threshold callsperformDragvia existingwithTemporaryWindowMovableEnabled/ suppression helpers; otherwise the terminating mouse-up is reposted so clicks, double-clicks, andNSTableViewtracking still work.SidebarWorkspaceTableViewImplandSidebarWorkspaceTableClipViewhook single-click empty-area presses; double-clicks stay on the existing empty-area path. The clip view alsoacceptsFirstMousefor inactive windows. SwiftUI’s full-height empty hit target is documented as SwiftUI-only so native views own drags without stealing row presses.Tests cover drag, click, jitter, cancellation, detached views, and first-mouse; plus a small test compile fix in
AppDelegateEqualizeSplitsShortcutTests.Reviewed by Cursor Bugbot for commit 14c454e. Bugbot is set up for automated code reviews on this repo. Configure here.