Skip to content

Preserve explicit wheel scrollback against passive follow #1965

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lawrencecchen merged 2 commits into from
Mar 23, 2026
Merged

Preserve explicit wheel scrollback against passive follow #1965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7634abe
Add regression test for wheel scrollback follow bug
lawrencecchen Mar 23, 2026
c1c028e
Preserve explicit wheel scrollback against passive follow
lawrencecchen Mar 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions Sources/GhosttyTerminalView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6052,6 +6052,7 @@ class GhosttyNSView: NSView, NSUserInterfaceValidations {
}

override func scrollWheel(with event: NSEvent) {
NotificationCenter.default.post(name: .ghosttyDidReceiveWheelScroll, object: self)
guard let surface = surface else { return }
Comment on lines +6055 to 6056
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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).

Comment on lines +6055 to 6056
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Wheel notification posted before surface guard

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:

Suggested change
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)

lastScrollEventTime = CACurrentMediaTime()
Self.focusLog("scrollWheel: surface=\(terminalSurface?.id.uuidString ?? "nil") firstResponder=\(String(describing: window?.firstResponder))")
Expand Down Expand Up @@ -6453,6 +6454,7 @@ enum GhosttyNotificationKey {
extension Notification.Name {
static let ghosttyDidUpdateScrollbar = Notification.Name("ghosttyDidUpdateScrollbar")
static let ghosttyDidUpdateCellSize = Notification.Name("ghosttyDidUpdateCellSize")
static let ghosttyDidReceiveWheelScroll = Notification.Name("ghosttyDidReceiveWheelScroll")
static let ghosttySearchFocus = Notification.Name("ghosttySearchFocus")
static let ghosttyConfigDidReload = Notification.Name("ghosttyConfigDidReload")
static let ghosttyDefaultBackgroundDidChange = Notification.Name("ghosttyDefaultBackgroundDidChange")
Expand Down Expand Up @@ -6586,6 +6588,8 @@ final class GhosttySurfaceScrollView: NSView {
/// When true, auto-scroll should be suspended to prevent the "doomscroll" bug
/// where the terminal fights the user's scroll position.
private var userScrolledAwayFromBottom = false
private var pendingExplicitWheelScroll = false
private var allowExplicitScrollbarSync = false
/// Threshold in points from bottom to consider "at bottom" (allows for minor float drift)
private static let scrollToBottomThreshold: CGFloat = 5.0
private var isActive = true
Expand Down Expand Up @@ -7011,6 +7015,14 @@ final class GhosttySurfaceScrollView: NSView {
self?.handleScrollbarUpdate(notification)
})

observers.append(NotificationCenter.default.addObserver(
forName: .ghosttyDidReceiveWheelScroll,
object: surfaceView,
queue: .main
) { [weak self] _ in
self?.pendingExplicitWheelScroll = true
})
Comment on lines +7022 to +7024
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reset pending wheel intent when no scroll delta is applied

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 👍 / 👎.


observers.append(NotificationCenter.default.addObserver(
forName: .ghosttySearchFocus,
object: nil,
Expand Down Expand Up @@ -9013,26 +9025,21 @@ final class GhosttySurfaceScrollView: NSView {
userScrolledAwayFromBottom = false
}

// Only auto-scroll if user hasn't manually scrolled away from bottom
// or if we're following terminal output (scrollbar shows we're at bottom)
let shouldAutoScroll = !userScrolledAwayFromBottom ||
(scrollbar.offset + scrollbar.len >= scrollbar.total)
// Passive bottom packets should not override an explicit scrollback review,
// but the first scrollbar packet caused by the user's own wheel input should
// still move the viewport to the requested scrollback position.
let shouldAutoScroll = !userScrolledAwayFromBottom || allowExplicitScrollbarSync

if shouldAutoScroll && !pointApproximatelyEqual(currentOrigin, targetOrigin) {
#if DEBUG
logDragGeometryChange(
event: "scrollOrigin",
old: currentOrigin,
new: targetOrigin
)
#endif
scrollView.contentView.scroll(to: targetOrigin)
didChangeGeometry = true
}
lastSentRow = Int(scrollbar.offset)
}
}

allowExplicitScrollbarSync = false

if didChangeGeometry {
scrollView.reflectScrolledClipView(scrollView.contentView)
}
Expand Down Expand Up @@ -9068,6 +9075,11 @@ final class GhosttySurfaceScrollView: NSView {
guard let scrollbar = notification.userInfo?[GhosttyNotificationKey.scrollbar] as? GhosttyScrollbar else {
return
}
if pendingExplicitWheelScroll {
userScrolledAwayFromBottom = scrollbar.offset + scrollbar.len < scrollbar.total
allowExplicitScrollbarSync = true
pendingExplicitWheelScroll = false
}
surfaceView.scrollbar = scrollbar
synchronizeScrollView()
}
Expand Down
97 changes: 97 additions & 0 deletions cmuxTests/TerminalAndGhosttyTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1590,6 +1590,30 @@ final class GhosttySurfaceOverlayTests: XCTestCase {
}
}

private final class ScrollbarPostingSurfaceView: GhosttyNSView {
var nextScrollbar: GhosttyScrollbar?

override func scrollWheel(with event: NSEvent) {
super.scrollWheel(with: event)
guard let nextScrollbar else { return }
NotificationCenter.default.post(
name: .ghosttyDidUpdateScrollbar,
object: self,
userInfo: [GhosttyNotificationKey.scrollbar: nextScrollbar]
)
}
}

private func makeScrollbar(total: UInt64, offset: UInt64, len: UInt64) -> GhosttyScrollbar {
GhosttyScrollbar(
c: ghostty_action_scrollbar_s(
total: total,
offset: offset,
len: len
)
)
}

private func findEditableTextField(in view: NSView) -> NSTextField? {
if let field = view as? NSTextField, field.isEditable {
return field
Expand Down Expand Up @@ -1675,6 +1699,79 @@ final class GhosttySurfaceOverlayTests: XCTestCase {
)
}

func testExplicitWheelScrollKeepsScrollbackPinnedAgainstLaterBottomPacket() {
let window = NSWindow(
contentRect: NSRect(x: 0, y: 0, width: 360, height: 240),
styleMask: [.titled, .closable],
backing: .buffered,
defer: false
)
defer { window.orderOut(nil) }

guard let contentView = window.contentView else {
XCTFail("Expected content view")
return
}

let surfaceView = ScrollbarPostingSurfaceView(frame: NSRect(x: 0, y: 0, width: 160, height: 120))
surfaceView.cellSize = CGSize(width: 10, height: 10)
let hostedView = GhosttySurfaceScrollView(surfaceView: surfaceView)
hostedView.frame = contentView.bounds
hostedView.autoresizingMask = [.width, .height]
contentView.addSubview(hostedView)

window.makeKeyAndOrderFront(nil)
window.displayIfNeeded()
contentView.layoutSubtreeIfNeeded()
hostedView.layoutSubtreeIfNeeded()
RunLoop.current.run(until: Date().addingTimeInterval(0.05))

guard let scrollView = hostedView.subviews.first(where: { $0 is NSScrollView }) as? NSScrollView else {
XCTFail("Expected hosted terminal scroll view")
return
}

NotificationCenter.default.post(
name: .ghosttyDidUpdateScrollbar,
object: surfaceView,
userInfo: [GhosttyNotificationKey.scrollbar: makeScrollbar(total: 100, offset: 90, len: 10)]
)
RunLoop.current.run(until: Date().addingTimeInterval(0.01))
XCTAssertEqual(scrollView.contentView.bounds.origin.y, 0, accuracy: 0.01)

surfaceView.nextScrollbar = makeScrollbar(total: 100, offset: 40, len: 10)

guard let cgEvent = CGEvent(
scrollWheelEvent2Source: nil,
units: .pixel,
wheelCount: 2,
wheel1: 0,
wheel2: -12,
wheel3: 0
), let scrollEvent = NSEvent(cgEvent: cgEvent) else {
XCTFail("Expected scroll wheel event")
return
}

scrollView.scrollWheel(with: scrollEvent)
RunLoop.current.run(until: Date().addingTimeInterval(0.01))
XCTAssertEqual(scrollView.contentView.bounds.origin.y, 500, accuracy: 0.01)

NotificationCenter.default.post(
name: .ghosttyDidUpdateScrollbar,
object: surfaceView,
userInfo: [GhosttyNotificationKey.scrollbar: makeScrollbar(total: 100, offset: 90, len: 10)]
)
RunLoop.current.run(until: Date().addingTimeInterval(0.01))

XCTAssertEqual(
scrollView.contentView.bounds.origin.y,
500,
accuracy: 0.01,
"A passive bottom packet should not yank the viewport after an explicit wheel scroll into scrollback"
)
}

func testInactiveOverlayVisibilityTracksRequestedState() {
let hostedView = GhosttySurfaceScrollView(
surfaceView: GhosttyNSView(frame: NSRect(x: 0, y: 0, width: 80, height: 50))
Expand Down
Loading