Make iOS terminal scrolling phone-owned (fix optimistic scroll jumps) - #7197
lawrencecchen wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR propagates full-replacement terminal output metadata into surface rendering, reworks scrollback prefetch routing for primary and alternate screens, and adds a full-replay stress scenario with UI validation and updated release notes. ChangesFull replacement propagation and viewport preservation
Scroll gesture routing and scrollback prefetch
Full-replay stress scenario and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GhosttySurfaceRepresentable
participant GhosttySurfaceView
participant GhosttyBinding
GhosttySurfaceRepresentable->>GhosttySurfaceView: chunk.isFullReplacement == true
GhosttySurfaceView->>GhosttySurfaceView: scrollbackOffsetFromBottom
GhosttySurfaceView->>GhosttySurfaceView: processOutputAndWait(data)
GhosttySurfaceView->>GhosttyBinding: scrollLocalViewportRows(-offsetFromBottom)
GhosttyBinding->>GhosttySurfaceView: drawForWakeup()
sequenceDiagram
participant MobileBottomScrollStressCoordinator
participant GhosttySurfaceView
participant DockProbe
MobileBottomScrollStressCoordinator->>GhosttySurfaceView: seed scrollback, scroll to bottom
MobileBottomScrollStressCoordinator->>GhosttySurfaceView: applyLocalScrollback
MobileBottomScrollStressCoordinator->>GhosttySurfaceView: processFullReplacementOutputAndWait(replay data)
GhosttySurfaceView->>DockProbe: recordScrollbarSnapshot updates
MobileBottomScrollStressCoordinator->>DockProbe: check offset preserved
DockProbe-->>MobileBottomScrollStressCoordinator: bottomStressPhase = done or regressed
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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: 532101b4fb
ℹ️ 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".
| enqueueTerminalScroll(TerminalScrollDelivery( | ||
| let delivery = TerminalScrollRouting.delivery( | ||
| surfaceID: surfaceID, | ||
| activeScreen: terminalActiveScreenBySurfaceID[surfaceID] ?? .primary, |
There was a problem hiding this comment.
When the host falls back to .rawBytes because it does not advertise terminal.render_grid.v1 (or the status probe fails), terminalActiveScreenBySurfaceID is not populated from render-grid frames, so this default treats every surface as primary. TerminalScrollRouting then suppresses the real delta (or sends delta_lines = 0), which means scrolling in alternate-screen programs such as vim/less/htop on an older Mac host no longer reaches the PTY; before this change every scroll gesture was forwarded. Please gate the phone-owned primary routing on render-grid support, or forward when the active screen is unknown.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR makes the phone the single owner of the primary-screen viewport, eliminating the double-scroll bug where a gesture was applied both to the local Ghostty mirror and the Mac's real
Confidence Score: 5/5Safe to merge. The routing logic is unit-tested, the offset-capture/restore invariant is XCUITest-verified red→green, and alternate-screen TUI forwarding is unchanged. The core scroll-routing and full-replay restore paths are exercised by new unit tests and a simulator-verified XCUITest. The only finding is a debug-only counter (
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User as User Gesture
participant MSC as MobileShellComposite
participant Routing as TerminalScrollDelivery.forScrollGesture
participant Mac as Mac RPC
participant GSV as GhosttySurfaceView (local mirror)
User->>MSC: scrollTerminal(lines, col, row)
MSC->>Routing: forScrollGesture(activeScreen, ...)
alt Primary screen
Routing-->>MSC: "delivery(lines=0, maxScrollbackRows=N)"
MSC->>Mac: "RPC delta_lines=0 + maxScrollbackRows"
Mac-->>MSC: "full render-grid snapshot (isFullReplacement=true)"
MSC->>GSV: processFullReplacementOutputAndWait(data)
GSV->>GSV: capture scrollbackOffsetFromBottom
GSV->>GSV: processOutputAndWait (ESC c reset + content)
GSV->>GSV: scrollLocalViewportRows(-offset)
Note over GSV: viewport position preserved
else Alternate screen (TUI)
Routing-->>MSC: "delivery(lines=N)"
MSC->>Mac: "RPC delta_lines=N"
Mac-->>MSC: render-grid update
MSC->>GSV: processOutputAndWait(data)
else Unknown screen (legacy/cold-attach)
Routing-->>MSC: "delivery(lines=N, maxScrollbackRows=M)"
MSC->>Mac: "RPC delta_lines=N + maxScrollbackRows"
end
Mac-->>GSV: GHOSTTY_ACTION_SCROLLBAR callback
GSV->>GSV: recordScrollbarSnapshot(total, offset, len)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User as User Gesture
participant MSC as MobileShellComposite
participant Routing as TerminalScrollDelivery.forScrollGesture
participant Mac as Mac RPC
participant GSV as GhosttySurfaceView (local mirror)
User->>MSC: scrollTerminal(lines, col, row)
MSC->>Routing: forScrollGesture(activeScreen, ...)
alt Primary screen
Routing-->>MSC: "delivery(lines=0, maxScrollbackRows=N)"
MSC->>Mac: "RPC delta_lines=0 + maxScrollbackRows"
Mac-->>MSC: "full render-grid snapshot (isFullReplacement=true)"
MSC->>GSV: processFullReplacementOutputAndWait(data)
GSV->>GSV: capture scrollbackOffsetFromBottom
GSV->>GSV: processOutputAndWait (ESC c reset + content)
GSV->>GSV: scrollLocalViewportRows(-offset)
Note over GSV: viewport position preserved
else Alternate screen (TUI)
Routing-->>MSC: "delivery(lines=N)"
MSC->>Mac: "RPC delta_lines=N"
Mac-->>MSC: render-grid update
MSC->>GSV: processOutputAndWait(data)
else Unknown screen (legacy/cold-attach)
Routing-->>MSC: "delivery(lines=N, maxScrollbackRows=M)"
MSC->>Mac: "RPC delta_lines=N + maxScrollbackRows"
end
Mac-->>GSV: GHOSTTY_ACTION_SCROLLBAR callback
GSV->>GSV: recordScrollbarSnapshot(total, offset, len)
Reviews (4): Last reviewed commit: "Keep legacy scroll forwarding while the ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| enum TerminalScrollRouting { | ||
| static func delivery( | ||
| surfaceID: String, | ||
| activeScreen: MobileTerminalRenderGridFrame.Screen, | ||
| lines: Double, | ||
| col: Int, | ||
| row: Int, | ||
| prefetchState: inout TerminalScrollbackPrefetchState | ||
| ) -> TerminalScrollDelivery? { | ||
| guard activeScreen == .primary else { | ||
| return TerminalScrollDelivery(surfaceID: surfaceID, lines: lines, col: col, row: row) | ||
| } | ||
| guard let maxScrollbackRows = prefetchState.rowsToPrefetch(forScrollLines: lines) else { | ||
| return nil | ||
| } | ||
| return TerminalScrollDelivery( | ||
| surfaceID: surfaceID, | ||
| lines: 0, | ||
| col: col, | ||
| row: row, | ||
| maxScrollbackRows: maxScrollbackRows | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
TerminalScrollRouting has no cases and exists solely to wrap static func delivery(...). This is the caseless-enum-as-namespace pattern flagged by the cmux no-ambient-global-state rule. A wrong value from delivery (e.g. forwarding a delta to the Mac from a primary-screen gesture) would re-introduce the double-scroll bug this PR is fixing, and there is no instance to scope or inject.
The natural fix is to promote delivery to a static func on TerminalScrollDelivery itself (e.g. TerminalScrollDelivery.forScrollGesture(surfaceID:activeScreen:lines:col:row:prefetchState:)), which already owns the return type and keeps all scroll routing logic in one place without an uninhabited namespace type.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 18 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="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift:56">
P2: Scrollback window can grow during mixed-direction scrolling, not just sustained upward history scrolling. The new deepening check keys off only the last positive delta while the threshold uses absolute movement from both directions, so a tiny upward tick after mostly downward movement can still page deeper.</violation>
</file>
<file name="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalScrollDelivery.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalScrollDelivery.swift:36">
P2: Alternate-screen wheel input can be dropped while the surface's active-screen state is still unknown, because missing `terminalActiveScreenBySurfaceID` is treated as `.primary`. Consider avoiding the primary default for unknown state, or ensuring the screen state is populated before scroll gestures are routed so TUIs still receive their first wheel events after attach/reset.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Sustained scrolling into history pages the window deeper so the | ||
| // local mirror can keep going past the initial window; scrolling back | ||
| // toward the bottom refreshes at the current depth instead. | ||
| if hasPrimedWindow, lines > 0 { |
There was a problem hiding this comment.
P2: Scrollback window can grow during mixed-direction scrolling, not just sustained upward history scrolling. The new deepening check keys off only the last positive delta while the threshold uses absolute movement from both directions, so a tiny upward tick after mostly downward movement can still page deeper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift, line 56:
<comment>Scrollback window can grow during mixed-direction scrolling, not just sustained upward history scrolling. The new deepening check keys off only the last positive delta while the threshold uses absolute movement from both directions, so a tiny upward tick after mostly downward movement can still page deeper.</comment>
<file context>
@@ -45,12 +50,50 @@ struct TerminalScrollbackPrefetchState: Equatable, Sendable {
+ // Sustained scrolling into history pages the window deeper so the
+ // local mirror can keep going past the initial window; scrolling back
+ // toward the bottom refreshes at the current depth instead.
+ if hasPrimedWindow, lines > 0 {
+ windowRows = min(windowRows + Self.defaultWindowRows, maxWindowRows)
+ }
</file context>
The iOS terminal mirror applies authoritative full render-grid replays (ESC c snapshot + repaint) through the shared output pipeline. When the user has scrolled the local mirror into scrollback, the reset leaves the rebuilt mirror pinned to the bottom, snapping the viewport mid-gesture. This adds the repro path only: full render-grid replacements are routed through a dedicated GhosttySurfaceView seam (currently a plain forward), the mirror's Ghostty scrollbar geometry and a monotonic update counter are recorded in all builds, a row-exact scroll_page_lines primitive is added, and a CMUX_FULL_REPLAY_SCROLL_STRESS harness scenario plus cmuxUITests/testScrollbackPositionSurvivesAuthoritativeFullReplay assert the scroll position survives an authoritative rebuild. The harness requires a post-replay scrollbar callback before judging, so the cached pre-replay geometry cannot produce a false pass. Red on this commit: simulator run ends with scrollAtBottom=1 and phase=regressed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fix for the red repro in the previous commit. The full-replacement seam captures the mirror's scrollback offset-from-bottom before feeding the ESC c snapshot and re-applies the same offset afterwards with the row-exact scroll_page_lines action (the wheel path is subject to the 3x discrete mouse-scroll-multiplier and would overshoot). At the bottom (offset 0, cold attach) this is a no-op, so live-pinned surfaces are unaffected. Invariant: an authoritative content rebuild never moves the phone-owned viewport. Green on this commit: the simulator repro ends with the offset preserved and phase=done. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
532101b to
39799cd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39799cd35a
ℹ️ 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".
| case .bytes: | ||
| false |
There was a problem hiding this comment.
When mobile.terminal.replay falls back to snapshot_data_b64 because a render-grid frame is unavailable or fails to decode, MobileShellComposite wraps those bytes with terminalSnapshotReplacementBytes, which prepends ESC c. This new flag still reports .bytes deliveries as not full replacements, so the consumer takes the plain processOutputAndWait path and the reset snaps the local mirror back to the bottom instead of restoring the phone-owned scrollback offset. Please mark the snapshot-byte replay path as full replacement too, or carry an explicit replacement kind through TerminalOutputDelivery.
Useful? React with 👍 / 👎.
…etch The phone's scroll RPC used to scroll the Mac's real viewport while the local mirror also scrolled optimistically. Render-grid exports follow the Mac's live vp_top, so every gesture was applied twice through two unreconciled owners: the mirror showed stale prefetched history while the hidden screen repainted with the Mac's scrolled viewport, and the two offsets drifted (jumps, duplicated rows, wrong position on release). TerminalScrollRouting now decides per gesture: primary screen sends no scroll delta at all (delta_lines=0 fetches history windows only), so the mirror is the single owner of the phone viewport and the Mac's viewport stays put; alternate screen still forwards wheel deltas to the real PTY for TUIs. Sustained scrolling into history pages the prefetch window deeper (600 rows per refresh, capped), and the Mac-side prefetch budget rises from 600 to 4800 rows to serve it. Old phones keep the previous Mac-side behavior; delta_lines=0 was already a no-op scroll. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
39799cd to
0506c05
Compare
| if offsetFromBottom > 0 { | ||
| scrollLocalViewportRows(-offsetFromBottom) | ||
| } | ||
| return true |
There was a problem hiding this comment.
Replay returns before restore finishes
Medium Severity
processFullReplacementOutputAndWait resumes after the ESC c apply and only schedules scrollLocalViewportRows asynchronously. The output consumer then acks and can start another full replacement while the mirror still reads as bottom-aligned, so the second pass captures a zero offset and skips restore—snapping scrollback after back-to-back prefetches or replays.
Reviewed by Cursor Bugbot for commit 0506c05. Configure here.
| struct TerminalScrollbackPrefetchState: Equatable, Sendable { | ||
| static let defaultWindowRows = 600 | ||
| static let defaultRefreshDistanceRows = 120.0 | ||
| static let defaultMaxWindowRows = 4800 |
There was a problem hiding this comment.
Scroll coalescing mixes screen modes
Medium Severity
Single-flight scroll coalescing sums lines on every pending delivery. Primary prefetch RPCs now use delta_lines = 0 with maxScrollbackRows, so a pending alternate-screen wheel delta merges into that packet and the Mac receives a non-zero delta_lines after the user is back on the primary screen—violating phone-owned primary scrolling.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0506c05. Configure here.
|
Applied Greptile's namespace finding (also caught by |
Pure code motion to satisfy the Swift file-length budget: GhosttySurfaceView.swift keeps only the two stored properties (extensions cannot store state); recordScrollbarSnapshot, scrollbackOffsetFromBottom, and processFullReplacementOutputAndWait move to GhosttySurfaceView+LocalScrollbackScroll.swift, their domain. Simulator repro re-run green after the move. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5912408. Configure here.
| let applied = await surfaceView.processOutputAndWait(chunk.data) | ||
| let applied = chunk.isFullReplacement | ||
| ? await surfaceView.processFullReplacementOutputAndWait(chunk.data) | ||
| : await surfaceView.processOutputAndWait(chunk.data) |
There was a problem hiding this comment.
Alt-screen full replay scroll restore
Medium Severity
Every output chunk with isFullReplacement goes through processFullReplacementOutputAndWait, which reapplies the mirror’s cached scrollback offset after an ESC c rebuild. Alternate-screen full snapshots use remoteGrid and Mac-owned viewport semantics; reusing the primary scroll offset there can call scroll_page_lines on the TUI buffer after entry or resync, misaligning the visible grid.
Reviewed by Cursor Bugbot for commit 5912408. Configure here.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift:56">
P2: Scrollback window can grow during mixed-direction scrolling, not just sustained upward history scrolling. The new deepening check keys off only the last positive delta while the threshold uses absolute movement from both directions, so a tiny upward tick after mostly downward movement can still page deeper.</violation>
</file>
<file name="Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swift">
<violation number="1" location="Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swift:57">
P2: Actor-isolation gap: `scrollbackOffsetFromBottom` reads `lastScrollbarSnapshot` (written by `@MainActor` `recordScrollbarSnapshot`) but isn't annotated `@MainActor`, and `processFullReplacementOutputAndWait` consumes it without isolation either. In Swift 5 this compiles silently, but if either is ever called off the main actor the read races with the writer. Current callers happen to be `@MainActor`; to close the gap, annotate both with `@MainActor`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| /// How many rows the local mirror's viewport currently sits above the | ||
| /// scrollback bottom. 0 when pinned to the live bottom. | ||
| var scrollbackOffsetFromBottom: Int { |
There was a problem hiding this comment.
P2: Actor-isolation gap: scrollbackOffsetFromBottom reads lastScrollbarSnapshot (written by @MainActor recordScrollbarSnapshot) but isn't annotated @MainActor, and processFullReplacementOutputAndWait consumes it without isolation either. In Swift 5 this compiles silently, but if either is ever called off the main actor the read races with the writer. Current callers happen to be @MainActor; to close the gap, annotate both with @MainActor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swift, line 57:
<comment>Actor-isolation gap: `scrollbackOffsetFromBottom` reads `lastScrollbarSnapshot` (written by `@MainActor` `recordScrollbarSnapshot`) but isn't annotated `@MainActor`, and `processFullReplacementOutputAndWait` consumes it without isolation either. In Swift 5 this compiles silently, but if either is ever called off the main actor the read races with the writer. Current callers happen to be `@MainActor`; to close the gap, annotate both with `@MainActor`.</comment>
<file context>
@@ -38,5 +38,50 @@ extension GhosttySurfaceView {
+
+ /// How many rows the local mirror's viewport currently sits above the
+ /// scrollback bottom. 0 when pinned to the live bottom.
+ var scrollbackOffsetFromBottom: Int {
+ guard let snapshot = lastScrollbarSnapshot else { return 0 }
+ return max(0, snapshot.total - snapshot.len - snapshot.offset)
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift (1)
47-62: 🎯 Functional Correctness | 🔵 TrivialCumulative full-depth re-fetch, not incremental — worth watching now the cap is 8x larger.
rowsToPrefetchreturns the entire accumulatedwindowRowsdepth on every priming/deepening step (600, 1200, 1800, … up tomaxWindowRows), and downstream (mobileTerminalScrollResponsePayload→mobileTerminalRenderGridFrame(..., scrollbackLines: scrollbackRows)) re-serializes the whole render grid from scratch each time rather than just the newly needed rows. This pattern already existed, but raisingmobileScrollPrefetchScrollbackLineBudgetfrom 600 to 4800 (seeSources/TerminalController+MobileScrollPrefetch.swift) means a single sustained scroll to full depth now re-transmits up to ~21,600 cumulative rows of JSON (600+1200+1800+…+4800) instead of ~2,100 previously — an ~8-10x increase in worst-case payload/CPU cost for JSON encode/decode over the socket during "sustained scrolling," which is exactly the use case this PR is optimizing for.Worth a quick bandwidth/CPU sanity check on a real device with a large scrollback buffer; if it's noticeable, consider fetching only the incremental row delta rather than the full window on each deepening step.
🤖 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 `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift` around lines 47 - 62, The scroll prefetch path in rowsToPrefetch currently re-requests the full accumulated window on each deepening step, which can balloon socket JSON work as mobileScrollPrefetchScrollbackLineBudget increases. Update TerminalScrollDelivery.rowsToPrefetch and the downstream mobileTerminalScrollResponsePayload/mobileTerminalRenderGridFrame flow so sustained scrolling fetches only the incremental row delta needed for the next depth, rather than re-serializing the entire scrollback window each time.
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 326-341: The test
`testScrollbackPositionSurvivesAuthoritativeFullReplay` is using a `waitForDock`
timeout that exactly matches the worst-case budget of
`runFullReplayOffsetScenario`, leaving no slack for polling or slower CI
execution. Increase the outer timeout in this test so it has real margin beyond
the coordinator flow’s internal waits, and keep the assertion on
`bottomStressPhase` unchanged.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 803-806: The “at bottom” scrollbar check is duplicated between
debugScrollbarAtBottomForTesting in GhosttySurfaceView and
bottomScrollDebugScrollbarAtBottom in
GhosttySurfaceView+BottomScrollStressDebug.swift. Consolidate the shared
lastScrollbarSnapshot-based condition into one computed property/helper and have
both call sites use it so the logic lives in a single place and cannot drift
apart.
In `@Sources/TerminalController`+MobileScrollPrefetch.swift:
- Around line 11-16: The increased mobile scroll prefetch cap is amplifying the
existing full-window re-fetch behavior, so the session payload can grow much
larger than intended. Review
`TerminalController+MobileScrollPrefetch.mobileScrollPrefetchScrollbackLineBudget`
together with `TerminalScrollbackPrefetchState.rowsToPrefetch` in
`TerminalScrollDelivery` and change the prefetch logic to request only the
incremental delta per scroll step, or reduce the new cap accordingly so deeper
history does not re-download the entire cumulative window each time.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift`:
- Around line 47-62: The scroll prefetch path in rowsToPrefetch currently
re-requests the full accumulated window on each deepening step, which can
balloon socket JSON work as mobileScrollPrefetchScrollbackLineBudget increases.
Update TerminalScrollDelivery.rowsToPrefetch and the downstream
mobileTerminalScrollResponsePayload/mobileTerminalRenderGridFrame flow so
sustained scrolling fetches only the incremental row delta needed for the next
depth, rather than re-serializing the entire scrollback window each time.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12bd33d4-7cf3-4115-8197-c6e74b3715b4
📒 Files selected for processing (19)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalScrollDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollDeliveryQueueTests.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileTerminalOutputSinking.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/Debug/GhosttySurfaceView+BottomScrollStressDebug.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttyRuntime.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/MobileBottomScrollStressCoordinator.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/MobileBottomScrollStressRepresentable.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/MobileBottomScrollStressView.swiftSources/TerminalController+MobileScrollPrefetch.swiftios/CHANGELOG.mdios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swiftios/cmuxUITests/cmuxUITests.swift
| @MainActor | ||
| func testScrollbackPositionSurvivesAuthoritativeFullReplay() throws { | ||
| let app = launchApp(mockData: false, environment: [ | ||
| "CMUX_FULL_REPLAY_SCROLL_STRESS": "1", | ||
| ]) | ||
| XCTAssertTrue(app.otherElements["MobileTerminalSurface"].waitForExistence(timeout: 8)) | ||
|
|
||
| let dock = waitForDock(in: app, timeout: 12, describe: "full-replay scroll stress completed") { | ||
| $0["bottomStressPhase"] == "done" || $0["bottomStressPhase"] == "regressed" | ||
| } | ||
| XCTAssertEqual( | ||
| dock["bottomStressPhase"], | ||
| "done", | ||
| "Authoritative full replay must not move the phone-owned scrollback viewport. dock=\(dock)" | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Outer timeout has zero margin over the coordinator's internal worst-case budget.
runFullReplayOffsetScenario's internal waits sum to exactly 12s in the worst case (mount 2s + bottom 2s + scrollback 2s + post-replay scrollbar-update wait 4s + preserved-check 2s), which equals this test's own waitForDock timeout. On a slower CI runner or simulator, this leaves no slack for the two unbounded processOutputAndWait/processFullReplacementOutputAndWait awaits or for waitForDock's own polling overhead, risking a spurious "timed out" failure on a build that actually would have passed.
🐛 Proposed fix: give the outer wait real margin over the internal budget
- let dock = waitForDock(in: app, timeout: 12, describe: "full-replay scroll stress completed") {
+ let dock = waitForDock(in: app, timeout: 18, describe: "full-replay scroll stress completed") {
$0["bottomStressPhase"] == "done" || $0["bottomStressPhase"] == "regressed"
}📝 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.
| @MainActor | |
| func testScrollbackPositionSurvivesAuthoritativeFullReplay() throws { | |
| let app = launchApp(mockData: false, environment: [ | |
| "CMUX_FULL_REPLAY_SCROLL_STRESS": "1", | |
| ]) | |
| XCTAssertTrue(app.otherElements["MobileTerminalSurface"].waitForExistence(timeout: 8)) | |
| let dock = waitForDock(in: app, timeout: 12, describe: "full-replay scroll stress completed") { | |
| $0["bottomStressPhase"] == "done" || $0["bottomStressPhase"] == "regressed" | |
| } | |
| XCTAssertEqual( | |
| dock["bottomStressPhase"], | |
| "done", | |
| "Authoritative full replay must not move the phone-owned scrollback viewport. dock=\(dock)" | |
| ) | |
| } | |
| `@MainActor` | |
| func testScrollbackPositionSurvivesAuthoritativeFullReplay() throws { | |
| let app = launchApp(mockData: false, environment: [ | |
| "CMUX_FULL_REPLAY_SCROLL_STRESS": "1", | |
| ]) | |
| XCTAssertTrue(app.otherElements["MobileTerminalSurface"].waitForExistence(timeout: 8)) | |
| let dock = waitForDock(in: app, timeout: 18, describe: "full-replay scroll stress completed") { | |
| $0["bottomStressPhase"] == "done" || $0["bottomStressPhase"] == "regressed" | |
| } | |
| XCTAssertEqual( | |
| dock["bottomStressPhase"], | |
| "done", | |
| "Authoritative full replay must not move the phone-owned scrollback viewport. dock=\(dock)" | |
| ) | |
| } |
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift` around lines 326 - 341, The test
`testScrollbackPositionSurvivesAuthoritativeFullReplay` is using a `waitForDock`
timeout that exactly matches the worst-case budget of
`runFullReplayOffsetScenario`, leaving no slack for polling or slower CI
execution. Increase the outer timeout in this test so it has real margin beyond
the coordinator flow’s internal waits, and keep the assertion on
`bottomStressPhase` unchanged.
| private var debugScrollbarAtBottomForTesting: Bool { | ||
| guard let snapshot = debugLastScrollbar else { return false } | ||
| guard let snapshot = lastScrollbarSnapshot else { return false } | ||
| return snapshot.total > snapshot.len && snapshot.offset >= max(0, snapshot.total - snapshot.len - 1) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate "at bottom" scrollbar logic.
debugScrollbarAtBottomForTesting here is identical to bottomScrollDebugScrollbarAtBottom in GhosttySurfaceView+BottomScrollStressDebug.swift (same total > len && offset >= max(0, total - len - 1) check against lastScrollbarSnapshot). Now that both are renamed to source from the same lastScrollbarSnapshot, this is a good moment to consolidate into one shared computed property to avoid the two copies drifting apart later.
♻️ Proposed consolidation
- private var debugScrollbarAtBottomForTesting: Bool {
- guard let snapshot = lastScrollbarSnapshot else { return false }
- return snapshot.total > snapshot.len && snapshot.offset >= max(0, snapshot.total - snapshot.len - 1)
- }
+ private var debugScrollbarAtBottomForTesting: Bool {
+ bottomScrollDebugScrollbarAtBottom
+ }📝 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.
| private var debugScrollbarAtBottomForTesting: Bool { | |
| guard let snapshot = debugLastScrollbar else { return false } | |
| guard let snapshot = lastScrollbarSnapshot else { return false } | |
| return snapshot.total > snapshot.len && snapshot.offset >= max(0, snapshot.total - snapshot.len - 1) | |
| } | |
| private var debugScrollbarAtBottomForTesting: Bool { | |
| bottomScrollDebugScrollbarAtBottom | |
| } |
🤖 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
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`
around lines 803 - 806, The “at bottom” scrollbar check is duplicated between
debugScrollbarAtBottomForTesting in GhosttySurfaceView and
bottomScrollDebugScrollbarAtBottom in
GhosttySurfaceView+BottomScrollStressDebug.swift. Consolidate the shared
lastScrollbarSnapshot-based condition into one computed property/helper and have
both call sites use it so the logic lives in a single place and cannot drift
apart.
Review finding from Cursor, Codex, and cubic on #7197: routing with `?? .primary` treated an unknown screen as primary, which dropped alt-screen wheel deltas before the first render grid arrives and permanently on legacy raw-bytes hosts that never send one. Unknown screen (nil) now routes the legacy way: forward the delta and request prefetch. Phone-owned routing engages once a render grid reports the mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the shared Cursor/Codex/cubic finding in 142dc3d: On cubic's prefetch-growth-during-mixed-scrolling point: growth requires the triggering refresh to be an upward tick and is capped at 4800 rows, so the worst case is a slightly larger capped window after mostly-downward movement. Leaving as is; direction-scoped accumulation can ride with the cut-2 scroll-delta export. |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
iOS optimistic scroll (#6067) had three unreconciled viewport owners: the phone's UIScrollView delta stream, the phone's local Ghostty mirror, and the Mac's real viewport (which the scroll RPC also moved). Render-grid exports follow the Mac's live
vp_top(ghostty/src/apprt/embedded.zig), so every phone gesture was applied twice: the mirror scrolled locally over stale prefetched history while the Mac's scrolled viewport repainted the mirror's hidden screen rows, and the two offsets drifted apart on clamping and coalescing. On top of that, scrollback prefetch responses arrive as fullESC csnapshots that reset the mirror and snapped the viewport to the bottom mid-drag.This makes the phone the single owner of the primary-screen viewport and the Mac the single owner of content:
TerminalScrollRouting(new, pure): primary-screen gestures send NO scroll delta to the Mac; the RPC is only used to fetch history windows (delta_lines=0, already a no-op scroll on the Mac, so old hosts are compatible). Alternate screen still forwards wheel deltas to the real PTY for TUIs.GhosttySurfaceView.processFullReplacementOutputAndWait, which captures the mirror's scrollback offset-from-bottom before theESC crebuild and re-applies it after via the row-exactscroll_page_linesbinding action (the wheel path is subject to the 3x discretemouse-scroll-multiplierand would overshoot). Invariant: an authoritative content rebuild never moves the phone-owned viewport. At the bottom (cold attach) it is a no-op.GHOSTTY_ACTION_SCROLLBAR) plus a monotonic update counter are now recorded in all builds (was DEBUG-only) to drive the offset restore.Red/green (simulator-verified locally)
1455fe519d:CMUX_FULL_REPLAY_SCROLL_STRESSharness +cmuxUITests/testScrollbackPositionSurvivesAuthoritativeFullReplay. The harness requires a post-replay scrollbar callback before judging, so the stale cached geometry cannot produce a false pass. Verified failing on iPhone 17 Pro sim: dock showsscrollAtBottom=1,bottomStressPhase=regressed(replay snapped the viewport to the bottom).75acd90510: offset capture/restore. Verified passing on the same sim.Testing
swift test --package-path Packages/iOS/CmuxMobileShellpassed (incl. newTerminalScrollRouting+ prefetch paging tests)CmuxMobileTerminal,CmuxMobileShellUI,ios/cmuxPackagebuild forarm64-apple-ios26.0-simulatorCMUX_ALLOW_LOCAL_TEST=1, isolated derived data)Localization
No user-facing strings changed (debug harness phases and test identifiers only); no localization updates needed.
Related
🤖 Generated with Claude Code
Note
Medium Risk
Changes core mobile terminal scroll routing, Mac RPC semantics, and viewport apply path for full snapshots—high interaction surface but well-tested and scoped to iOS shell/terminal layers.
Overview
Primary-screen scrolling no longer forwards wheel deltas to the Mac.
TerminalScrollDelivery.forScrollGestureroutes byactiveScreen: primary sendsdelta_lines = 0RPCs only when scrollback prefetch needs priming or deepening; alternate still forwards deltas for TUI mouse reporting; unknown screen keeps legacy forward+delta behavior until a render grid reports the mode. Prefetch windows page deeper on sustained upward scroll (capped at 4800 rows on Mac and phone).Full render-grid snapshots (
ESC c) are flagged withisFullReplacementonMobileTerminalOutputChunkand applied viaprocessFullReplacementOutputAndWait, which saves scrollback offset-from-bottom from Ghostty scrollbar callbacks and restores it with row-exactscroll_page_linesafter the rebuild so authoritative prefetch/replay does not snap the viewport to the bottom mid-gesture.Scrollbar geometry is recorded in all builds (not DEBUG-only) to drive that restore. New unit tests cover routing and prefetch paging; a DEBUG/UI test harness covers full-replay offset preservation.
Reviewed by Cursor Bugbot for commit 142dc3d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests