Add native-style screen edge snapping#161
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
@chid is attempting to deploy a commit to the Pablo Varela's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. WalkthroughThis PR extends SwiftShift's window management with a configurable snapping feature. It adds a user preference to enable/disable snapping, introduces a Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Swift` Shift/src/Manager/MouseTracker.swift:
- Around line 253-271: The loop over screenSnapRects in MouseTracker.swift is
snapping to edges of displays that are far from the cursor when only one axis
aligns; to fix, bail out early for screens that are not near the cursor by
checking proximity on both axes before computing adjacency and applying snaps:
inside the for screen in screenSnapRects loop (where cursorCG, snapDistance,
result and size are used), add a guard that ensures cursorCG.x is within
(screen.minX - snapDistance) .. (screen.maxX + snapDistance) AND cursorCG.y is
within (screen.minY - snapDistance) .. (screen.maxY + snapDistance); if not,
continue so only screens near the cursor are considered for
adjacentLeft/Right/Above/Below and result.x/result.y adjustments.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcccc577-c371-42a2-8507-0fa81b0977a6
📒 Files selected for processing (4)
Swift Shift/src/Manager/MouseTracker.swiftSwift Shift/src/Manager/Preferences.swiftSwift Shift/src/Manager/WindowManager.swiftSwift Shift/src/View/PreferencesView.swift
| for screen in screenSnapRects { | ||
| // An edge shared with an adjacent display (where the cursor currently | ||
| // sits) is an interior seam, not a real screen edge. Snapping there | ||
| // traps the window at the boundary instead of letting it cross. | ||
| let adjacentLeft = screenSnapRects.contains { $0 != screen && abs($0.maxX - screen.minX) <= tolerance && cursorCG.y >= $0.minY && cursorCG.y <= $0.maxY } | ||
| let adjacentRight = screenSnapRects.contains { $0 != screen && abs($0.minX - screen.maxX) <= tolerance && cursorCG.y >= $0.minY && cursorCG.y <= $0.maxY } | ||
| let adjacentAbove = screenSnapRects.contains { $0 != screen && abs($0.maxY - screen.minY) <= tolerance && cursorCG.x >= $0.minX && cursorCG.x <= $0.maxX } | ||
| let adjacentBelow = screenSnapRects.contains { $0 != screen && abs($0.minY - screen.maxY) <= tolerance && cursorCG.x >= $0.minX && cursorCG.x <= $0.maxX } | ||
|
|
||
| if !adjacentLeft, abs(cursorCG.x - screen.minX) <= snapDistance { | ||
| result.x = screen.minX | ||
| } else if !adjacentRight, abs(cursorCG.x - screen.maxX) <= snapDistance { | ||
| result.x = screen.maxX - size.width | ||
| } | ||
| if !adjacentAbove, abs(cursorCG.y - screen.minY) <= snapDistance { | ||
| result.y = screen.minY | ||
| } else if !adjacentBelow, abs(cursorCG.y - screen.maxY) <= snapDistance { | ||
| result.y = screen.maxY - size.height | ||
| } |
There was a problem hiding this comment.
Constrain edge snapping to screens near the cursor.
Line 253 iterates all screenSnapRects and can apply snaps for a different display if only one axis happens to align. On offset multi-monitor layouts this causes jumpy, incorrect snaps.
Proposed fix
for screen in screenSnapRects {
+ guard screen.insetBy(dx: -snapDistance, dy: -snapDistance).contains(cursorCG) else { continue }
// An edge shared with an adjacent display (where the cursor currently
// sits) is an interior seam, not a real screen edge. Snapping there
// traps the window at the boundary instead of letting it cross.
let adjacentLeft = screenSnapRects.contains { $0 != screen && abs($0.maxX - screen.minX) <= tolerance && cursorCG.y >= $0.minY && cursorCG.y <= $0.maxY }
let adjacentRight = screenSnapRects.contains { $0 != screen && abs($0.minX - screen.maxX) <= tolerance && cursorCG.y >= $0.minY && cursorCG.y <= $0.maxY }
let adjacentAbove = screenSnapRects.contains { $0 != screen && abs($0.maxY - screen.minY) <= tolerance && cursorCG.x >= $0.minX && cursorCG.x <= $0.maxX }
let adjacentBelow = screenSnapRects.contains { $0 != screen && abs($0.minY - screen.maxY) <= tolerance && cursorCG.x >= $0.minX && cursorCG.x <= $0.maxX }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for screen in screenSnapRects { | |
| // An edge shared with an adjacent display (where the cursor currently | |
| // sits) is an interior seam, not a real screen edge. Snapping there | |
| // traps the window at the boundary instead of letting it cross. | |
| let adjacentLeft = screenSnapRects.contains { $0 != screen && abs($0.maxX - screen.minX) <= tolerance && cursorCG.y >= $0.minY && cursorCG.y <= $0.maxY } | |
| let adjacentRight = screenSnapRects.contains { $0 != screen && abs($0.minX - screen.maxX) <= tolerance && cursorCG.y >= $0.minY && cursorCG.y <= $0.maxY } | |
| let adjacentAbove = screenSnapRects.contains { $0 != screen && abs($0.maxY - screen.minY) <= tolerance && cursorCG.x >= $0.minX && cursorCG.x <= $0.maxX } | |
| let adjacentBelow = screenSnapRects.contains { $0 != screen && abs($0.minY - screen.maxY) <= tolerance && cursorCG.x >= $0.minX && cursorCG.x <= $0.maxX } | |
| if !adjacentLeft, abs(cursorCG.x - screen.minX) <= snapDistance { | |
| result.x = screen.minX | |
| } else if !adjacentRight, abs(cursorCG.x - screen.maxX) <= snapDistance { | |
| result.x = screen.maxX - size.width | |
| } | |
| if !adjacentAbove, abs(cursorCG.y - screen.minY) <= snapDistance { | |
| result.y = screen.minY | |
| } else if !adjacentBelow, abs(cursorCG.y - screen.maxY) <= snapDistance { | |
| result.y = screen.maxY - size.height | |
| } | |
| for screen in screenSnapRects { | |
| guard screen.insetBy(dx: -snapDistance, dy: -snapDistance).contains(cursorCG) else { continue } | |
| // An edge shared with an adjacent display (where the cursor currently | |
| // sits) is an interior seam, not a real screen edge. Snapping there | |
| // traps the window at the boundary instead of letting it cross. | |
| let adjacentLeft = screenSnapRects.contains { $0 != screen && abs($0.maxX - screen.minX) <= tolerance && cursorCG.y >= $0.minY && cursorCG.y <= $0.maxY } | |
| let adjacentRight = screenSnapRects.contains { $0 != screen && abs($0.minX - screen.maxX) <= tolerance && cursorCG.y >= $0.minY && cursorCG.y <= $0.maxY } | |
| let adjacentAbove = screenSnapRects.contains { $0 != screen && abs($0.maxY - screen.minY) <= tolerance && cursorCG.x >= $0.minX && cursorCG.x <= $0.maxX } | |
| let adjacentBelow = screenSnapRects.contains { $0 != screen && abs($0.minY - screen.maxY) <= tolerance && cursorCG.x >= $0.minX && cursorCG.x <= $0.maxX } | |
| if !adjacentLeft, abs(cursorCG.x - screen.minX) <= snapDistance { | |
| result.x = screen.minX | |
| } else if !adjacentRight, abs(cursorCG.x - screen.maxX) <= snapDistance { | |
| result.x = screen.maxX - size.width | |
| } | |
| if !adjacentAbove, abs(cursorCG.y - screen.minY) <= snapDistance { | |
| result.y = screen.minY | |
| } else if !adjacentBelow, abs(cursorCG.y - screen.maxY) <= snapDistance { | |
| result.y = screen.maxY - size.height | |
| } | |
| } |
🤖 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 `@Swift` Shift/src/Manager/MouseTracker.swift around lines 253 - 271, The loop
over screenSnapRects in MouseTracker.swift is snapping to edges of displays that
are far from the cursor when only one axis aligns; to fix, bail out early for
screens that are not near the cursor by checking proximity on both axes before
computing adjacency and applying snaps: inside the for screen in screenSnapRects
loop (where cursorCG, snapDistance, result and size are used), add a guard that
ensures cursorCG.x is within (screen.minX - snapDistance) .. (screen.maxX +
snapDistance) AND cursorCG.y is within (screen.minY - snapDistance) ..
(screen.maxY + snapDistance); if not, continue so only screens near the cursor
are considered for adjacentLeft/Right/Above/Below and result.x/result.y
adjustments.
There was a problem hiding this comment.
Yeah this is an issue that I noticed that needs a bit of polish, but requires a lot of complexity to get right (as it would need to know the monitor layouts)
A simple solution to reduce the jerky behaviour would also be to have a dynamic guard (on position) or tolerance but fundamentally the snap behaviour and monitor move behaviour requires rework, alternatively you could have it so that snapping doesn't work on the boundaries between monitors.
In my view, I was able to 'work it out' after playing with it over a few minutes.
There was a problem hiding this comment.
@chid, understood — thanks for the context. The guard line I proposed is actually the "dynamic guard on position" you described: it's a single check that constrains snapping to only the display the cursor is near, without requiring any knowledge of the full monitor layout. It sidesteps the cross-axis false-positive without reworking the broader snap/move logic.
That said, the existing adjacency-seam detection already handles the most important case (preventing the window from getting trapped at display boundaries), so the current behaviour is reasonable for an initial release.
If you'd like, I can open a follow-up issue to track the remaining polish (e.g. the cursor-proximity guard + any further multi-monitor move/snap rework) so it doesn't get lost.
🧠 Learnings used
Learnt from: pablopunk
Repo: pablopunk/SwiftShift PR: 150
File: Swift Shift/src/Manager/WindowManager.swift:0-0
Timestamp: 2026-05-13T17:15:02.309Z
Learning: In SwiftShift's `getVisibleWindowRects(excluding:)` (Swift Shift/src/Manager/WindowManager.swift), the exclusion filter uses exact rect equality (`rect.equalTo(excludedRect)`) to remove only the dragged window itself from snap candidates. Containing or maximized background windows must NOT be excluded — their outer edges are valid snap targets. Actual snapping is gated by perpendicular-overlap and a 10px edge-proximity check in MouseTracker, not by this exclusion filter.
1 participant
Summary
Context
Addresses the feedback from #156 (comment).
Summary by CodeRabbit
New Features