fix(macos): auto-scroll when dragging selection beyond viewport#28
fix(macos): auto-scroll when dragging selection beyond viewport#28yyanezh wants to merge 3 commits into
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds drag-selection auto-scrolling to the macOS SurfaceView by tracking per-view state ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR adds auto-scroll during drag selection on macOS by starting a 50 ms repeating timer when the cursor exits the viewport. There is a P1 ordering bug:
Confidence Score: 4/5Not safe to merge as-is — P1 ordering bug means the selection-extension half of the feature is broken for any cursor held still outside the viewport. One clear P1 defect:
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant mouseDragged
participant startAutoScroll
participant stopAutoScroll
participant Timer
participant Core as Ghostty Core
User->>mouseDragged: drag cursor below viewport
mouseDragged->>mouseDragged: lastDragEvent = event (line 990)
mouseDragged->>startAutoScroll: startAutoScroll(scrollUp: false)
startAutoScroll->>stopAutoScroll: stopAutoScroll() (unconditional)
stopAutoScroll->>stopAutoScroll: lastDragEvent = nil ❌
startAutoScroll->>Timer: scheduledTimer(0.05s, repeating)
Note over Timer: 50 ms passes
Timer->>Core: sendMouseScroll(y: -1.0) ✅
Timer->>Timer: if let lastEvent = lastDragEvent → nil, skip
Note over Core: sendMousePos never called ❌
Note over Core: Selection does not extend
Note over mouseDragged,Timer: Fix: move lastDragEvent = event AFTER startAutoScroll
mouseDragged->>startAutoScroll: startAutoScroll(scrollUp: false)
startAutoScroll->>stopAutoScroll: stopAutoScroll()
stopAutoScroll->>stopAutoScroll: lastDragEvent = nil
startAutoScroll->>Timer: scheduledTimer(0.05s, repeating)
mouseDragged->>mouseDragged: lastDragEvent = event ✅
Note over Timer: 50 ms passes
Timer->>Core: sendMouseScroll(y: -1.0) ✅
Timer->>Core: sendMousePos(x, y) ✅
Note over Core: Selection extends correctly
Reviews (1): Last reviewed commit: "fix(macos): add auto-scroll when draggin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@macos/Sources/Ghostty/Surface` View/SurfaceView_AppKit.swift:
- Around line 220-223: The view's repeating auto-scroll timer (autoScrollTimer)
may continue firing after the view is deallocated because deinit does not call
stopAutoScroll(); update the SurfaceView_AppKit deinit to call stopAutoScroll()
to invalidate and nil out autoScrollTimer (the same cleanup performed by the
existing stopAutoScroll() method), ensuring the timer is removed from the run
loop and auto-scroll state (autoScrollUp/lastDragEvent) is cleared when the view
is torn down.
- Around line 1018-1039: The auto-scroll timer created in the autoScrollTimer
block uses Timer.scheduledTimer which attaches to RunLoop.Mode.default and won't
fire during drag tracking; change the creation to use an unscheduled Timer
(still using the same closure and keeping references to autoScrollTimer,
lastDragEvent, surfaceModel.sendMouseScroll, surfaceModel.sendMousePos and
convert(_:from:)) and then explicitly add it to RunLoop.main in
RunLoop.Mode.common so the timer fires during event-tracking drag mode; ensure
you preserve the existing weak self capture and the logic that re-sends mouse
position when lastDragEvent exists.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a6be798-1e06-47bd-a11a-4c8701eae8f7
📒 Files selected for processing (1)
macos/Sources/Ghostty/Surface View/SurfaceView_AppKit.swift
1 participant
Summary
mouseDraggedonly forwarded the position to Ghostty core with no scroll behavior — selecting was limited to the currently visible contentImplementation
mouseDraggedthat sends scroll events + updates selection position while the cursor is outside the view boundsmouseUpor when cursor returns to the viewportTest plan
Summary by cubic
Enables auto-scroll when dragging a text selection past the top or bottom of the terminal on macOS, letting you select beyond the visible area. Fixes the previous limit to on-screen content.
Bug Fixes
Refactors
Written for commit f0a063a. Summary will update on new commits.
Summary by CodeRabbit