Preserve explicit wheel scrollback against passive follow#1965
Preserve explicit wheel scrollback against passive follow#1965lawrencecchen merged 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request implements a notification-based coordination mechanism to prevent unwanted viewport auto-scrolling when users interact with scrollback. An explicit mouse wheel scroll now sets a pending state that temporarily overrides the standard scrollbar synchronization logic, ensuring that a subsequent scrollbar update won't yank the viewport away from the user's scrollback position. Changes
Sequence DiagramsequenceDiagram
participant User
participant GhosttyNSView
participant NotificationCenter
participant GhosttySurfaceScrollView
participant Scrollbar
User->>GhosttyNSView: Mouse wheel scroll
GhosttyNSView->>NotificationCenter: Post .ghosttyDidReceiveWheelScroll
NotificationCenter->>GhosttySurfaceScrollView: Notify wheel scroll received
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: Set pendingExplicitWheelScroll=true
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: Update viewport position
Scrollbar->>NotificationCenter: Post scrollbar update
NotificationCenter->>GhosttySurfaceScrollView: Notify scrollbar changed
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: handleScrollbarUpdate()
alt pendingExplicitWheelScroll is true
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: Recompute userScrolledAwayFromBottom
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: Set allowExplicitScrollbarSync=true
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: Clear pendingExplicitWheelScroll
end
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: synchronizeScrollView()
alt allowExplicitScrollbarSync is true
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: Apply scrollbar sync override
end
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: Set allowExplicitScrollbarSync=false
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: c1c028e628
ℹ️ 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".
| ) { [weak self] _ in | ||
| self?.pendingExplicitWheelScroll = true | ||
| }) |
There was a problem hiding this comment.
pendingExplicitWheelScroll is set for every wheel event, but it is only cleared later inside handleScrollbarUpdate. If a wheel event does not produce a scrollbar action (for example at scrollback bounds or momentum-only phases), this flag can remain set and cause the next passive scrollbar packet to be treated as explicit, which enables allowExplicitScrollbarSync and can jump the viewport (including back to bottom). That reintroduces the same “yank out of scrollback” behavior this change is meant to prevent.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
cmuxTests/TerminalAndGhosttyTests.swift (1)
1594-1603: ConsumenextScrollbarafter posting to avoid stale replays.
nextScrollbarpersists across wheel events, which can accidentally replay old scrollbar packets and make follow-up tests order-dependent.♻️ Proposed tweak
override func scrollWheel(with event: NSEvent) { super.scrollWheel(with: event) guard let nextScrollbar else { return } + self.nextScrollbar = nil NotificationCenter.default.post( name: .ghosttyDidUpdateScrollbar, object: self, userInfo: [GhosttyNotificationKey.scrollbar: nextScrollbar] ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TerminalAndGhosttyTests.swift` around lines 1594 - 1603, nextScrollbar is retained between scrollWheel events and can replay stale scrollbar updates; in the scrollWheel(with:) override (the method referencing nextScrollbar, GhosttyScrollbar, Notification.Name.ghosttyDidUpdateScrollbar and GhosttyNotificationKey.scrollbar) post the notification as you already do and then immediately clear/consume nextScrollbar (set it to nil) so it doesn't persist for subsequent events, ensuring each wheel event only sends its fresh scrollbar packet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 6055-6056: The pendingExplicitWheelScroll boolean is sticky and
armed before verifying surface, causing unrelated packets to be treated as
explicit wheel input; change this to a per-wheel one-shot token/counter (e.g.,
an incrementing wheelToken or small counter) that is only incremented/armed
after the guard let surface succeeds (move the arm to after the surface check
where NotificationCenter.default.post(name: .ghosttyDidReceiveWheelScroll,
object: self) is called), give the token a bounded lifetime (timeout or
decrement after processing), and update the code paths that currently clear
pendingExplicitWheelScroll on scrollbar packet receipt to instead validate and
clear only the matching token/counter; apply the same token-based fix to the
other occurrences referenced (around lines with pendingExplicitWheelScroll at
6591-6592, 7018-7024, 9078-9082).
---
Nitpick comments:
In `@cmuxTests/TerminalAndGhosttyTests.swift`:
- Around line 1594-1603: nextScrollbar is retained between scrollWheel events
and can replay stale scrollbar updates; in the scrollWheel(with:) override (the
method referencing nextScrollbar, GhosttyScrollbar,
Notification.Name.ghosttyDidUpdateScrollbar and
GhosttyNotificationKey.scrollbar) post the notification as you already do and
then immediately clear/consume nextScrollbar (set it to nil) so it doesn't
persist for subsequent events, ensuring each wheel event only sends its fresh
scrollbar packet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2442d2e7-7c82-4610-9023-f3377707cc68
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swift
| NotificationCenter.default.post(name: .ghosttyDidReceiveWheelScroll, object: self) | ||
| guard let surface = surface else { return } |
There was a problem hiding this comment.
Make wheel intent a one-shot token, not a sticky Bool.
pendingExplicitWheelScroll is armed on every wheel event here, but it is only cleared when a scrollbar packet arrives. If that wheel cannot produce a packet — for example Line 6056 returns early because surface is nil, or the user is already at the scrollback boundary — the next unrelated bottom packet is still treated as explicit input and follow gets re-enabled again. A single Bool also collapses multiple wheel events into one pending sync. Please switch this to a per-wheel token/counter with a bounded lifetime, and only arm it after the surface guard succeeds.
Also applies to: 6591-6592, 7018-7024, 9078-9082
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 6055 - 6056, The
pendingExplicitWheelScroll boolean is sticky and armed before verifying surface,
causing unrelated packets to be treated as explicit wheel input; change this to
a per-wheel one-shot token/counter (e.g., an incrementing wheelToken or small
counter) that is only incremented/armed after the guard let surface succeeds
(move the arm to after the surface check where
NotificationCenter.default.post(name: .ghosttyDidReceiveWheelScroll, object:
self) is called), give the token a bounded lifetime (timeout or decrement after
processing), and update the code paths that currently clear
pendingExplicitWheelScroll on scrollbar packet receipt to instead validate and
clear only the matching token/counter; apply the same token-based fix to the
other occurrences referenced (around lines with pendingExplicitWheelScroll at
6591-6592, 7018-7024, 9078-9082).
Greptile SummaryThis PR fixes the "doomscroll reversion" bug where performing a mouse-wheel scroll into scrollback would be silently overridden by the next passive terminal output packet that updated the scrollbar at the bottom. The fix introduces a two-flag handshake ( Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[scrollWheel event fires] --> B[Post ghosttyDidReceiveWheelScroll]
B --> C[pendingExplicitWheelScroll = true]
D[ghosttyDidUpdateScrollbar arrives] --> E{pendingExplicitWheelScroll?}
E -- yes --> F[userScrolledAwayFromBottom =\noffset+len < total]
F --> G[allowExplicitScrollbarSync = true]
G --> H[pendingExplicitWheelScroll = false]
H --> I[synchronizeScrollView]
E -- no --> I
I --> J{isLiveScrolling?}
J -- yes --> K[skip scroll-to-position]
J -- no --> L{shouldAutoScroll =\nnot userScrolledAwayFromBottom\nOR allowExplicitScrollbarSync}
L -- true --> M[scroll contentView to targetOrigin]
L -- false --> N[viewport unchanged\npassive bottom packet blocked]
M --> O[allowExplicitScrollbarSync = false]
K --> O
N --> O
Reviews (1): Last reviewed commit: "Preserve explicit wheel scrollback again..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| NotificationCenter.default.post(name: .ghosttyDidReceiveWheelScroll, object: self) | ||
| guard let surface = surface else { return } |
There was a problem hiding this comment.
The notification fires even when surface is nil, which leaves pendingExplicitWheelScroll = true in the paired GhosttySurfaceScrollView with no subsequent ghosttyDidUpdateScrollbar to drain it. If a surface is later attached (e.g. during a split reattach), its first scrollbar packet will be incorrectly treated as an explicit wheel scroll, potentially snapping the viewport to an unintended position.
Moving the post to just after the guard ensures the flag is only set when there is an actual surface to drive the corresponding scrollbar update:
| NotificationCenter.default.post(name: .ghosttyDidReceiveWheelScroll, object: self) | |
| guard let surface = surface else { return } | |
| override func scrollWheel(with event: NSEvent) { | |
| guard let surface = surface else { return } | |
| NotificationCenter.default.post(name: .ghosttyDidReceiveWheelScroll, object: self) |
…ix-mainline Preserve explicit wheel scrollback against passive follow
1 participant
Summary
Verification
Notes
GhosttySurfaceOverlayTestsstill has unrelated local failures intestSearchOverlayFocusesSearchFieldAfterDeferredAttachandtestSearchOverlayMountDoesNotRetainTerminalSurfacewhen run as a whole class in this environmentSummary by cubic
Fixes scrollback being pulled back to bottom after a wheel scroll. We now treat the next scrollbar update after a wheel event as explicit user intent and keep the viewport pinned.
.ghosttyDidReceiveWheelScrollfromscrollWheeland track pending explicit wheel intent.userScrolledAwayFromBottom; block passive bottom packets from re-enabling follow.testExplicitWheelScrollKeepsScrollbackPinnedAgainstLaterBottomPacketwith a helperScrollbarPostingSurfaceView.Written for commit c1c028e. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests