Fix terminal scrollback resets during output and resize#2506
Fix terminal scrollback resets during output and resize#2506austinywang wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR refactors scroll-position auto-scroll logic in GhosttyTerminalView to prevent unwanted crashes to bottom during agent output and terminal resize. It introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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. ✨ 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 |
Greptile SummaryThis PR fixes two related scrollback-reset bugs (#2504): when streaming output grew the terminal history and when a resize changed the viewport height, the Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[scrollbar notification arrives] --> B{pendingExplicitWheelScroll?}
B -- yes --> C[userScrolledAwayFromBottom = !scrollbarIsAtBottom\nallowExplicitScrollbarSync = true\npendingExplicitWheelScroll = false]
C --> D[surfaceView.scrollbar = scrollbar]
B -- no --> D
D --> E[synchronizeScrollView]
E --> F{isLiveScrolling?}
F -- yes --> G[skip scroll sync]
F -- no --> H{cellHeight > 0 && scrollbar?}
H -- no --> G
H -- yes --> I[compute targetOrigin from scrollbar model]
I --> J{shouldAutoScroll?}
J --> K{allowExplicitScrollbarSync?}
K -- yes --> L[auto-scroll to targetOrigin]
K -- no --> M{!userScrolledAwayFromBottom?}
M -- yes --> L
M -- no --> N{!scrollbarIsAtBottom?}
N -- yes → non-bottom packet → preserve rows --> L
N -- no → passive bottom packet → stay pinned --> O[skip scroll]
L --> P[reflectScrolledClipView]
O --> P
G --> P
Reviews (1): Last reviewed commit: "fix: preserve manual scrollback during o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func scrollbarIsAtBottom(_ scrollbar: GhosttyScrollbar) -> Bool { | ||
| scrollbar.offset >= scrollbar.total || scrollbar.len >= scrollbar.total - scrollbar.offset | ||
| } |
There was a problem hiding this comment.
scrollbarIsAtBottom
The guard scrollbar.offset >= scrollbar.total is always a strict subset of the second condition. When offset >= total, then total - offset <= 0, and since len is non-negative, len >= total - offset is necessarily true. The first arm can never fire without the second also being true, so it adds no coverage.
If the intent is to protect against a hypothetical unsigned-underflow scenario (e.g. if these fields were C unsigned integers), a comment explaining that would help. Otherwise consider simplifying to the single condition:
| private func scrollbarIsAtBottom(_ scrollbar: GhosttyScrollbar) -> Bool { | |
| scrollbar.offset >= scrollbar.total || scrollbar.len >= scrollbar.total - scrollbar.offset | |
| } | |
| private func scrollbarIsAtBottom(_ scrollbar: GhosttyScrollbar) -> Bool { | |
| scrollbar.len >= scrollbar.total - scrollbar.offset | |
| } |
![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 the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 9813-9819: The current shouldAutoScroll gating (using
allowExplicitScrollbarSync, userScrolledAwayFromBottom, and
scrollbarIsAtBottom(scrollbar)) suppresses packets that merely touch the bottom
even when a resize preserves the user's manual anchor; instead, change the logic
in the shouldAutoScroll calculation to compute whether the incoming packet would
change the manual anchor (e.g. compare the anchoredRow/targetOrigin before and
after applying the packet's offset/len/total) and only treat the packet as a
snap-to-bottom when it would actually move the anchor; update the code paths
around shouldAutoScroll, scrollbarIsAtBottom(scrollbar), and any resize/packet
handling to use this anchored-row comparison so near-bottom resize packets are
not dropped.
🪄 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: f692f21e-1dd0-40f5-acd3-d64269cfe4f2
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swift
| // While reviewing scrollback, still honor non-bottom scrollbar updates so | ||
| // streaming output and resize churn preserve the same visible rows. Only | ||
| // passive packets that would snap us back to bottom stay suppressed. | ||
| let shouldAutoScroll = | ||
| allowExplicitScrollbarSync || | ||
| !userScrolledAwayFromBottom || | ||
| !scrollbarIsAtBottom(scrollbar) |
There was a problem hiding this comment.
Don't suppress near-bottom resize packets.
This treats every atBottom packet as a snap-to-bottom, but resize can increase len enough that a packet now satisfies offset + len == total while still preserving the user's anchored row. Example: total=100, offset=70, len=20 should move from targetOrigin = 10 * cellHeight to 0 when len grows to 30; with the current gate, that update is dropped and the viewport shifts upward instead of staying pinned to the same scrollback. Please gate on whether the packet changes the manual anchor, not just whether it technically includes the bottom.
Possible direction
- let shouldAutoScroll =
- allowExplicitScrollbarSync ||
- !userScrolledAwayFromBottom ||
- !scrollbarIsAtBottom(scrollbar)
+ let preservesCurrentAnchor =
+ userScrolledAwayFromBottom &&
+ lastSentRow.map(UInt64.init) == scrollbar.offset
+ let shouldAutoScroll =
+ allowExplicitScrollbarSync ||
+ !userScrolledAwayFromBottom ||
+ preservesCurrentAnchor ||
+ !scrollbarIsAtBottom(scrollbar)Also applies to: 9867-9867, 9890-9892
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 9813 - 9819, The current
shouldAutoScroll gating (using allowExplicitScrollbarSync,
userScrolledAwayFromBottom, and scrollbarIsAtBottom(scrollbar)) suppresses
packets that merely touch the bottom even when a resize preserves the user's
manual anchor; instead, change the logic in the shouldAutoScroll calculation to
compute whether the incoming packet would change the manual anchor (e.g. compare
the anchoredRow/targetOrigin before and after applying the packet's
offset/len/total) and only treat the packet as a snap-to-bottom when it would
actually move the anchor; update the code paths around shouldAutoScroll,
scrollbarIsAtBottom(scrollbar), and any resize/packet handling to use this
anchored-row comparison so near-bottom resize packets are not dropped.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/TerminalAndGhosttyTests.swift">
<violation number="1" location="cmuxTests/TerminalAndGhosttyTests.swift:2234">
P2: New regression tests use fixed 10ms RunLoop sleeps for async scrollbar/scroll updates, which can cause flaky assertions on slower CI; use condition-based waiting instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| object: surfaceView, | ||
| userInfo: [GhosttyNotificationKey.scrollbar: makeScrollbar(total: 100, offset: 90, len: 10)] | ||
| ) | ||
| RunLoop.current.run(until: Date().addingTimeInterval(0.01)) |
There was a problem hiding this comment.
P2: New regression tests use fixed 10ms RunLoop sleeps for async scrollbar/scroll updates, which can cause flaky assertions on slower CI; use condition-based waiting instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/TerminalAndGhosttyTests.swift, line 2234:
<comment>New regression tests use fixed 10ms RunLoop sleeps for async scrollbar/scroll updates, which can cause flaky assertions on slower CI; use condition-based waiting instead.</comment>
<file context>
@@ -2194,6 +2194,148 @@ final class GhosttySurfaceOverlayTests: XCTestCase {
+ object: surfaceView,
+ userInfo: [GhosttyNotificationKey.scrollbar: makeScrollbar(total: 100, offset: 90, len: 10)]
+ )
+ RunLoop.current.run(until: Date().addingTimeInterval(0.01))
+
+ surfaceView.nextScrollbar = makeScrollbar(total: 100, offset: 40, len: 10)
</file context>
1 participant
Summary
Fixes #2504
Summary by cubic
Preserves manual scrollback during streaming output and window resize so the same rows stay visible and the viewport doesn’t snap to bottom. Only auto-scrolls to bottom on explicit wheel/scrollbar sync or when already at bottom; fixes #2504.
Written for commit 7065b66. Summary will update on new commits.
Summary by CodeRabbit