Repository navigation
Fix right sidebar titlebar double-click - #3750
Merged
Merged
Conversation
The right sidebar mode bar is a custom titlebar region, so empty space in it should dispatch the same zoom/minimize action as other titlebar strips. This regression test mounts the real SwiftUI sidebar in an AppKit window and sends a double-click through NSApp so CI exercises the hit-testing path instead of a source-text assertion. Constraint: Local tests are intentionally not run in this workflow; CI owns test execution. Confidence: medium Scope-risk: narrow Directive: Keep this test on the runtime event path; do not replace it with structural source assertions. Tested: Not run locally per instruction. Not-tested: CI has not run yet.
The right sidebar mode bar is part of the custom titlebar chrome but only registered the minimal-mode hit region, so empty-space double-clicks never reached the standard macOS titlebar action. A small SwiftUI modifier now owns the WindowDragHandleView plus TitlebarDoubleClickMonitorView pairing, and the right sidebar mode bar opts into that same interaction path. Constraint: Do not run xcodebuild directly; final verification must use the tagged reload script. Rejected: Add a one-off ZStack directly in RightSidebarPanelView | repeats the existing two-layer pattern and makes future titlebar chrome easier to miss. Confidence: medium Scope-risk: narrow Directive: Use titlebarDoubleClickRegion() for future custom titlebar strips with empty draggable space. Tested: git diff --check Not-tested: Local unit/UI tests were not run per instruction; CI must execute the regression test.
The base branch advanced with tab-bar click-zone and CI updates after the local fix commits were created. Merging origin/main now keeps the PR branch testable against the same infrastructure and click-routing code that CI will use. Constraint: Preserve the red/green regression and fix commits below this merge. Confidence: high Scope-risk: moderate Directive: Do not squash away the two underlying issue-3746 commits before verifying the regression-test story in the PR. Tested: git status --short --branch Not-tested: Local tests/build not run after merge; CI and final reload remain pending.
Dogfooding the first helper version showed the ZStack wrapper could let the WindowDragHandleView representable influence the surrounding SwiftUI layout. Making the drag handle a background keeps the interaction layer bounded to the content's established frame while retaining the same empty-space hit path. Constraint: Right sidebar chrome height must remain owned by rightSidebarChromeBar(). Rejected: Add a fixed frame inside the helper | would bake one caller's sizing into a generic titlebar modifier. Confidence: medium Scope-risk: narrow Directive: Keep titlebarDoubleClickRegion() layout-neutral; callers own size before applying it. Tested: git diff --check Not-tested: Local tests not run per instruction; tagged reload/manual dogfood is next.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a stable drag-handle identifier and deferment helpers, updates the titlebar double-click monitor to defer when a registered drag handle would capture the click, embeds the drag-handle + monitor into the right-sidebar mode bar, and adds tests that synthesize double-clicks asserting zoom/miniaturize behavior. ChangesTitlebar Double-Click Region for Right Sidebar
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ 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 |
Contributor
Greptile SummaryThis PR fixes the right sidebar mode bar so that empty titlebar space correctly participates in macOS drag and double-click (zoom/minimize) interactions, without breaking the Files/Find/Vault button clicks that share the same bar.
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to the right sidebar mode bar and its hit-testing path, the regression test exercises the real view hierarchy, and the existing button click paths are unaffected. All changed logic is confined to a narrow path (clicks within the mode bar bounds that also pass a registered-control check). The new helpers add no new mutable state, no actor isolation concerns, and no blocking primitives. The synchronous NSApp.sendEvent regression test gives good coverage of the core double-click dispatch path. Sources/WindowDragHandleView.swift — the new view-tree walk starts from Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ZStack as ZStack (modeBar)
participant ModeBtn as ModeBarButton
participant DragHandle as WindowDragHandleView
participant Monitor as TitlebarDoubleClickMonitorView (local monitor)
participant HitCheck as isMinimalModeTitlebarControlHit
participant WalkFn as titlebarDoubleClickMonitorHasCapturingDragHandle
participant Window as NSWindow
User->>ZStack: leftMouseDown on empty space
ZStack->>DragHandle: hit-test (foreground passes through)
DragHandle-->>Window: performDrag / handleDoubleClick
User->>ZStack: leftMouseDown on ModeBarButton
ZStack->>ModeBtn: hit-test (button captured)
ModeBtn-->>Monitor: event forwarded to local monitor
Monitor->>HitCheck: isMinimalModeTitlebarControlHit?
HitCheck-->>Monitor: true
Monitor->>WalkFn: titlebarDoubleClickMonitorHasCapturingDragHandle(contentView)
WalkFn-->>Monitor: true (drag handle found at point)
Monitor-->>Monitor: defer to registered control → pass event through
ModeBtn-->>User: mode switch action fires
Reviews (6): Last reviewed commit: "Keep titlebar strip fix current with mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| window.displayIfNeeded() | ||
| hostingView.layoutSubtreeIfNeeded() | ||
|
|
||
| let emptyModeBarPoint = NSPoint(x: 690, y: 242) |
Contributor
There was a problem hiding this comment.
NSPoint(x: 690, y: 242) is the only thing keeping the test pointed at "empty space." The window is 720×260 with titlebarHeight: 36, so this places the click 30 px from the right edge and 18 px from the top — both of which can shift silently if a mode button is added, padding changes, or the titlebarHeight parameter changes. When that happens, the point either lands on a button (causing isMinimalModeTitlebarControlHit to return true and bail the monitor) or falls outside the view bounds, and zoomCallCount stays 0. The test would then fail without pointing at a layout regression — or, worse, the point could land in a new empty region that still passes, masking a real geometry change. Deriving the target point from the actual laid-out view geometry would make the assertion track the real empty-space region robustly.
| } | ||
|
|
||
| NSApp.sendEvent(event) | ||
| RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) |
Contributor
There was a problem hiding this comment.
RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) assumes the zoom action completes within 50 ms. NSApp.sendEvent dispatches synchronously through local monitors, so RecordingTitlebarActionWindow.zoom should be called before sendEvent returns — the spin loop appears unnecessary. But if any internal path defers the action asynchronously (e.g., a future refactor wraps handleTitlebarDoubleClick in a DispatchQueue.main.async), the 50 ms window may silently become too short on a loaded CI runner, yielding a false "zoomCallCount == 0" without a clear synchronization error. Removing the spin or waiting on an explicit signal would make the test deterministic rather than timing-dependent.
Contributor
Author
There was a problem hiding this comment.
Already addressed in the current branch head: the regression test no longer uses a RunLoop/timing window. It sends the synthetic event through NSApp.sendEvent and asserts the recording window call counts synchronously.
— Claude Code
The regression should prove the right sidebar exposes a real empty titlebar hit region without depending on a hardcoded pixel or a timing window. Tagging the drag-handle view lets the test locate the laid-out helper and ask the same capture predicate used by the production hit path for a valid empty point. Constraint: Local tests are intentionally not run for this branch; CI owns XCTest execution. Rejected: Keep the absolute test point | fragile if the right sidebar mode bar layout changes. Rejected: Keep a short RunLoop spin | NSApp.sendEvent dispatches this path synchronously and the wait can introduce timing noise. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest execution per workflow policy
![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 and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1e7bed2. Configure here.
Cursor flagged that the right-sidebar minimal-mode host registration made the monitor layer defer across the whole mode bar. The monitor now treats a registered point as empty chrome when the colocated drag handle would capture that same point, so foreground controls still win while the monitor remains a real double-click safety net. Constraint: The right sidebar keeps its existing minimal-mode hit-region registration for drag/control routing. Rejected: Remove the monitor from the right sidebar helper | this would satisfy current behavior but drop the two-layer titlebar pattern requested for the fix. Rejected: Ignore the registered-control guard entirely | button double-clicks could be consumed by the titlebar monitor. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest execution per workflow policy
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The right-sidebar strip installed its drag handle as a SwiftUI background, which could participate in probing but did not reliably become the AppKit mouseDown receiver in the live hierarchy. Using a layout-neutral overlay matches the intended event order: foreground buttons still win through windowDragHandleShouldCaptureHit, while true empty chrome is claimed by the drag handle. Constraint: The helper must not affect mode-bar sizing after the earlier ZStack regression. Rejected: Return to a ZStack wrapper | it previously expanded the chrome layout. Rejected: Keep the drag handle as a background | live hit testing still leaves empty strip clicks without drag/double-click behavior. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest execution per workflow policy
Dogfooding showed the layout-neutral helper left the empty right-sidebar strip as a SwiftUI hosting hit instead of a real AppKit mouse-down target. The strip now uses the same proven shape as the main custom titlebar: a WindowDragHandleView sibling behind the tab buttons, with the double-click monitor layered on the fixed chrome container. Constraint: The right-sidebar chrome height remains owned by rightSidebarChromeBar(). Rejected: Handle drag from the local monitor | window.performDrag returns immediately when invoked from the monitor and subsequent drag events still fall into file-drop routing. Rejected: Keep the generic titlebarDoubleClickRegion helper | its background/overlay variants were layout-neutral but did not reliably receive live mouse-down delivery here. Confidence: high Scope-risk: narrow Tested: ./scripts/reload.sh --tag issue-3746-right-sidebar-titlebar-doubleclick --launch Tested: Manual drag from empty space to the right of Vault moved the window and logged titlebar.dragHandle.mouseDown. Tested: Manual double-click in the same empty space zoomed the window and logged titlebar.monitor.doubleClick result=performed(...zoom). Tested: Manual clicks on Find and Vault switched right-sidebar modes. Not-tested: Local XCTest execution per workflow policy
origin/main moved while the right-sidebar titlebar strip fix was being re-verified, so the PR branch was merged forward before CI iteration. There were no conflicts; this keeps the review branch testing against the current app code. Constraint: iterate-pr sync requires the PR branch to include the latest base branch before acting on CI. Confidence: high Scope-risk: moderate Tested: git merge origin/main completed without conflicts Not-tested: Local tests not run per workflow policy
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3746.
Bug
The right sidebar mode bar is custom titlebar chrome, but empty space in the Files / Find / Vault row only registered the minimal hit region and did not opt into the standard titlebar double-click/drag interaction path. Double-clicking empty space there did nothing while other titlebar regions zoomed/minimized normally.
Change
titlebarDoubleClickRegion()SwiftUI modifier that layersWindowDragHandleViewandTitlebarDoubleClickMonitorViewbehind an existing chrome view.RightSidebarPanelView.modeBarwhile keepingMinimalModeTitlebarControlHitRegionView.RightSidebarPanelView, sends a double-click throughNSApp.sendEvent, and asserts the window titlebar action is invoked.Manual Results
Before: baseline build
issue-3746-right-sidebar-titlebar-doubleclick-baselineleft window bounds unchanged (526,181,460,360) after double-clicking empty space between Files and Find.After: tagged build
issue-3746-right-sidebar-titlebar-doubleclickbuilds and launches; the right-sidebar mode bar remains correctly sized at the top, empty space routes toWindowDragHandleViewin the debug log, and Files / Find / Vault button clicks still switch modes. Automation-generated double-clicks reportclickCount=1, so the actual clickCount=2 titlebar action is covered by the XCUnit regression and CI.Files Changed
Sources/WindowDragHandleView.swiftSources/RightSidebarPanelView.swiftcmuxTests/WindowAndDragTests.swiftTest Plan
WindowDragHandleHitTests.testRightSidebarModeBarEmptySpaceDoubleClickPerformsTitlebarAction.Note
Medium Risk
Adjusts custom titlebar hit-testing/monitoring to change which clicks are intercepted, which could subtly affect drag/double-click behavior in other titlebar-adjacent regions. Includes a new integration-style XCTest to reduce regression risk.
Overview
Fixes the right sidebar mode bar’s empty titlebar space so it participates in standard macOS titlebar interactions (drag + double-click zoom/minimize) by layering
WindowDragHandleViewbehind the mode buttons and addingTitlebarDoubleClickMonitorViewto the bar background.Updates the double-click monitor to defer to minimal-mode registered control hit regions only when no co-located drag handle would capture the click, and tags drag-handle views with a stable
identifierto support this lookup.Adds an XCTest regression that hosts the real
RightSidebarPanelView, synthesizes a double-click into an actually-capturable empty point, and asserts the window’s standard titlebar action is invoked.Reviewed by Cursor Bugbot for commit b8f65cd. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Restores standard macOS titlebar double‑click and drag in the right sidebar mode bar. Empty space between Files/Find/Vault now zooms or minimizes per system setting and stays draggable. Fixes #3746.
WindowDragHandleViewsibling behind the mode bar buttons and aTitlebarDoubleClickMonitorViewbackground; keepMinimalModeTitlebarControlHitRegionView.NSApp.sendEvent, and verifies the window performs the standard titlebar action.Written for commit b8f65cd. Summary will update on new commits.
Summary by CodeRabbit