iOS smooth scroll (stage 1): decouple non-alt scroll from the Mac#5628
iOS smooth scroll (stage 1): decouple non-alt scroll from the Mac#5628lawrencecchen wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTerminal output streams now deliver frame metadata and VT bytes together via ChangesLocal scrollback and metadata flow
🎯 4 (Complex) | ⏱️ ~75 minutes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (15 passed)
✨ 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 |
![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: 2ca9aa6693
ℹ️ 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".
| #endif | ||
| // Surface the active screen so the view gates local scroll; deliver before | ||
| // the bytes so the view can snap to live before the delta applies. | ||
| deliverTerminalFrameMeta(from: renderGrid) |
There was a problem hiding this comment.
When this new metadata path consumes every live render-grid frame, an empty delta can incorrectly flip the phone back to primary mode. MobileTerminalRenderObserver.emitRenderGrid builds the no-row-change frame without passing activeScreen, so it defaults to .primary at Sources/Mobile/MobileTerminalRenderObserver.swift:206-215; after an alt-screen TUI emits output that changes only the cursor/state sequence, this line delivers isAlternateScreen=false, causing subsequent scroll gestures to be handled locally instead of forwarded to the program until another row-changing frame arrives.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR implements Stage 1 iOS smooth scroll for the primary (non-alt) terminal screen: swipes now drive
Confidence Score: 5/5Safe to merge. The three previously-flagged races are structurally closed by the unified chunk stream; alt-screen TUI handling is byte-for-byte unchanged; engine logic is fully unit-tested. All previous inter-stream race conditions are resolved by collapsing metadata and bytes into a single ordered stream element. The MobileLocalScrollEngine state machine is clean and exhaustively unit-tested. The only open items are style-level: remoteClient could use private(set) instead of plain internal, and the processOutput(Data()) path for byteless full snapshots has an implicit assumption about libghostty's 0-byte input handling. Neither affects correctness in current usage. No files require special attention. GhosttySurfaceView.swift and MobileShellComposite.swift have minor access-control widening worth tightening before the pattern spreads. Important Files Changed
Reviews (4): Last reviewed commit: "iOS smooth scroll stage 1: deliver a fra..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Reached (or passed) the top of locally-held history while scrolling up: | ||
| // ask the host for ONE deeper-scrollback fetch (not per-frame). The fetch | ||
| // re-flows a deeper snapshot into the local surface, growing history. | ||
| if lines > 0, nextUpRows >= Double(heldScrollbackRows), nextUpRows > priorUpRows { | ||
| delegate?.ghosttySurfaceView(self, didReachLocalHistoryTopWithHeldScrollbackRows: heldScrollbackRows) | ||
| } |
There was a problem hiding this comment.
The condition nextUpRows >= Double(heldScrollbackRows) && nextUpRows > priorUpRows remains true for every subsequent upward-scroll flush once the position has passed the threshold — not just at the moment of first crossing. When heldScrollbackRows == 0 (initial state, before any snapshot arrives), nextUpRows >= 0.0 is vacuously true, so the delegate fires on the very first scroll frame and again on every frame until the RPC returns and setHeldScrollbackRows resets the position. The in-flight guard in requestTerminalReplay de-duplicates the actual RPC, but the delegate and the requestTerminalReplay guard-check still execute at up to 60 calls/sec during scrolling. The comment above this block says "ask the host for ONE deeper-scrollback fetch," and the trigger condition should enforce that guarantee rather than relying solely on the in-flight guard.
| public func terminalFrameMetaStream(surfaceID: String) -> AsyncStream<MobileTerminalFrameMeta> { | ||
| AsyncStream { continuation in | ||
| terminalFrameMetaContinuationsBySurfaceID[surfaceID] = continuation | ||
| continuation.onTermination = { [weak self] _ in | ||
| Task { @MainActor in | ||
| self?.terminalFrameMetaContinuationsBySurfaceID.removeValue(forKey: surfaceID) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
unregisterTerminalOutput
terminalOutputStream pairs registration in registerTerminalOutput with cleanup in unregisterTerminalOutput (called via onTermination). terminalFrameMetaStream only cleans up via its own onTermination → Task { @MainActor in removeValue }. If the output task is cancelled but the frame-meta task somehow outlives it, the meta continuation stays alive in terminalFrameMetaContinuationsBySurfaceID and future live frames continue to deliver metadata to a detached surface. detach() cancels both tasks today, but unregisterTerminalOutput not mirroring cleanup for the meta map leaves the two streams with asymmetric lifecycle guarantees. Consider also nullifying the meta continuation in unregisterTerminalOutput for belt-and-suspenders safety.
| #if DEBUG | ||
| mobileShellLog.info("CMUX_REPLAY live render_grid surface=\(renderGrid.surfaceID, privacy: .public) full=\(renderGrid.full, privacy: .public) spans=\(renderGrid.rowSpans.count, privacy: .public) cleared=\(renderGrid.clearedRows.count, privacy: .public) seq=\(renderGrid.stateSeq, privacy: .public) hasSink=true") | ||
| #endif | ||
| // Surface the active screen so the view gates local scroll; deliver before |
There was a problem hiding this comment.
@MainActor is not guaranteed
The comment says "deliver before the bytes so the view can snap to live before the delta applies." deliverTerminalFrameMeta and deliverTerminalBytes yield into two separate AsyncStream continuations consumed by two independent @MainActor tasks (frameMetaTask and outputTask). When both continuations get items in the same scheduler turn, Swift concurrency does not guarantee that frameMetaTask resumes and completes its loop body before outputTask resumes. The snap via ghostty_surface_binding_action("scroll_to_bottom") is idempotent so the worst case is an extra snap or a frame applying with a stale isAlternateScreen value, but the correctness argument rests on an unspecified implementation detail of the Swift runtime rather than a language-level guarantee.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eba96067c9
ℹ️ 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".
| // Primary screen: scroll the phone's own surface locally, no RPC. | ||
| scrollLocalSurface(lines: lines, atCell: cell) |
There was a problem hiding this comment.
When the connection is on the raw-bytes compatibility path, no MobileTerminalFrameMeta is ever delivered (handleTerminalBytesEvent only yields bytes and replay falls back to snapshot/raw when render_grid is absent), so isAlternateScreen stays at its new default false. In that context every one-finger scroll now takes this local-primary branch instead of the previous didScrollLines path, which means TUIs running on an older/no-terminal.render_grid.v1 host stop receiving mouse-wheel scroll events even though the code still advertises raw-bytes fallback support.
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: 2
🤖 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
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 1146-1147: Wrap the MobileDebugLog.anchormux(...) calls used on
the per-pan and per-flush hot paths inside conditional compilation guards so
they do not run in release builds; specifically, locate the anchormux calls that
log gesture.state.rawValue / gesture.translation(in: self) / isAlternateScreen
and the other anchormux probes mentioned and surround each call with `#if` DEBUG /
`#endif` so interpolation and logging cost is eliminated from
release/smooth-scroll paths.
🪄 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: 878e21bd-230e-4b9b-abf8-d36cdc6f8c83
📒 Files selected for processing (4)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/TerminalController.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 1162-1168: Don't clear the fetch-response latch in the gesture
.began handler; removing the line that sets localScrollFetchInFlight = false
prevents valid slow replays from being misclassified as cold-attaches. Instead,
leave localScrollFetchInFlight true in the .began case and only clear it when
the fetch response is actually consumed — e.g., from the code path that calls
setHeldScrollbackRows / handles the deeper-replay response and sets
localHistoryFullyLoaded — so that the latch survives until the snapshot consumer
processes the response (also apply same change around the 1257-1265 region).
🪄 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: e89df89e-798c-496b-8590-f66ee33ffc3a
📒 Files selected for processing (1)
Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift
| /// actor just before a live frame is dispatched to `outputQueue`; the caller | ||
| /// applies the `scroll_to_bottom` binding action off-main on that queue (in | ||
| /// `process_output` order) so a live delta's absolute-CUP paint lands in the | ||
| /// viewport rather than into scrolled-up history. The binding action is exact | ||
| /// (independent of the tracked offset), so a residual in `localScrollUpRowsExact` | ||
| /// can never leave the surface partway up. Returns `false` when no snap is | ||
| /// needed (at bottom, or alternate screen). Keeps the Mac as the single source | ||
| /// of truth; the phone only owns the scroll position. | ||
| private func consumeLocalScrollSnapRequest() -> Bool { | ||
| guard !isAlternateScreen, isLocalScrollActive else { return false } | ||
| MobileDebugLog.anchormux("scroll.snapLive from=\(String(format: "%.1f", localScrollUpRowsExact))") | ||
| localScrollUpRowsExact = 0 | ||
| return true | ||
| } | ||
|
|
There was a problem hiding this comment.
setHeldScrollbackRows clears the tracking offset without dispatching the scroll_to_bottom action
setHeldScrollbackRows zeroes localScrollUpRowsExact on the main actor, but the actual libghostty surface snap — dispatching scroll_to_bottom to outputQueue — only ever happens inside consumeLocalScrollSnapRequest() called from processOutput. Because frameMetaTask and outputTask are two independent @MainActor tasks consuming separate AsyncStreams, their resumption order is not guaranteed by the Swift runtime.
When a full snapshot arrives (deeper-fetch response or cold-attach) and frameMetaTask happens to be scheduled first:
setHeldScrollbackRowszeroeslocalScrollUpRowsExactto 0.processOutputruns next;consumeLocalScrollSnapRequestchecksisLocalScrollActive(localScrollUpRowsExact >= 0.5) and returnsfalse.- No
scroll_to_bottomis dispatched tooutputQueue; libghostty's actual scroll position is unchanged (still scrolled up). localScrollUpRowsExactnow says 0 while the libghostty surface is visually scrolled up — the two are out of sync.
From this point every live delta frame sees isLocalScrollActive = false and skips the snap. Live output paints at the VT cursor (the live viewport bottom in the buffer) but the window is positioned elsewhere in scrollback, so new content appears to vanish rather than paint into the visible area.
Stage 1 of the iOS terminal smooth-scroll work. In a NORMAL (primary, non-alt) shell, a swipe now scrolls the phone's own locally-held history with no per-frame `mobile.terminal.scroll` RPC to the Mac. Alt-screen behavior (TUIs: vim/less --mouse/htop/lazygit) is unchanged: it still forwards to the Mac so the program owns the scroll. How it works: - The phone tracks the active screen from each applied render-grid frame via a new per-surface metadata stream (the opaque byte stream cannot carry it). Primary -> scroll locally; alternate -> forward to the Mac. - Local scroll drives `ghostty_surface_mouse_scroll` on the phone's own libghostty surface (serialized on the same `outputQueue` as `process_output`), with no RPC. - When the local scroll reaches the top of locally-held history, the phone issues ONE deeper-scrollback replay fetch (not per-frame): the Mac's `mobile.terminal.replay` now takes an optional `scrollback_lines` param (clamped) and the primary full-snapshot reflow lands deeper history in the phone's surface so it can keep scrolling offline. - Snap-to-live: before any live frame applies, if the user is scrolled up the surface snaps to the live bottom via the exact `scroll_to_bottom` binding action, so a live delta's absolute-CUP paint lands in the viewport rather than into scrolled-up history. The Mac stays the single source of truth; the phone only owns a read-only scroll position. The local offset is accumulated as a Double (no per-frame rounding) so sub-row residuals do not drift it below the surface's real position; the snap is exact regardless, so correctness does not depend on offset precision. Behind the alt-mode gate, so TUIs are unaffected. Build-verified on the iOS simulator (arm64). Behavioral dogfood is gated on a paired device (the simulator cannot pair). Design + later stages (streamed scrolled-off lines; UIScrollView-hosted pixel-smooth + momentum) are in plans/feat-ios-smooth-scroll/DESIGN.md in cmuxterm-hq. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…unce) The deeper-scrollback fetch had no terminal condition: at the top of a shell whose whole scrollback is already held (e.g. a fresh attach with held=1), every scroll-to-top re-fired a replay RPC whose full-snapshot response re-anchored the view to the bottom, so a short-scrollback shell jittered on every swipe. Add a `localHistoryFullyLoaded` latch: when a deeper fetch returns no additional history (`scrollbackRows <= held`), stop firing fetches and let libghostty clamp at the oldest held line. Cleared on genuine growth, resize, or a fresh cold-attach snapshot. Also guard against a second fetch while one is in flight, and clear that in-flight latch on a fresh swipe (`.began`) so a dropped fetch self-heals without racing the metadata/byte delivery order. This realizes the "stop cleanly at the oldest known line" behavior the design doc already specified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oll gates Review fixes from PR #5628: - Mac: a no-row-change render-grid frame (cursor blink / seq bump) was built without activeScreen, defaulting to .primary, which silently flipped an alt-screen TUI back to local scrolling on the phone. The empty frame now carries the snapshot's real active screen. - Phone: until the first render-grid frame metadata arrives (older Mac host or the raw-byte compatibility path), every scroll keeps the legacy forward-to-Mac path instead of scrolling a history-less local mirror. - Phone: split the deeper-fetch latch into a dedupe/retry latch (cleared on a new pan and on snapshot arrival) and a classification latch (cleared only by a full snapshot), so a slow-but-valid deeper replay is still measured as a fetch result instead of being misread as a cold attach and re-opening the fully-loaded ceiling (fetch-bounce at the oldest line). - Phone: the tracked scroll offset is now cleared in exactly one place, the snap path that runs in process_output order when a frame's bytes apply. setHeldScrollbackRows and setActiveScreen no longer zero it out-of-band, which could race the separate meta/byte streams and suppress the snap, and the snap is no longer gated on the active screen so a stale offset left by an alt-flip mid-scroll still snaps before the alt program's rows paint. - Composite: byte-stream teardown also finishes the frame-meta continuation, and the comment claiming meta-before-bytes ordering now documents that cross-stream ordering is not guaranteed and nothing relies on it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tory fetches A deeper-scrollback fetch's full snapshot rebuilds the local surface at the live bottom, so every history page-in bounced the reader back to the bottom. When setHeldScrollbackRows classifies a snapshot as a fetch response and the reader is scrolled up, it now arms a restore; the snap path in processOutput applies the snapshot and then re-issues the preserved cumulative upward delta on the same serial queue, landing the reader back on the rows they were reading (the live bottom is unchanged by a deeper fetch, and libghostty clamps at the top of what it holds). Cold-attach snapshots still snap to live, and live-output snap-to-live behavior is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
force-pushed
the
feat-ios-smooth-scroll-stage1
branch
from
eba9606 to
b58d872
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift (1)
73-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSubscribe to frame metadata before opening the output stream.
Line 81 opens
terminalOutputStream, andregisterTerminalOutputimmediately triggers a replay. The metadata stream is not registered until Line 91, so the initial full-snapshot meta can be dropped if that replay wins the race. On an idle alt-screen attach, the view then starts with stale primary-screen state and the first scroll gestures go local instead of forwarding to the Mac.♻️ Suggested fix
func attach(surfaceView: GhosttySurfaceView) { self.surfaceView = surfaceView guard let store else { return } let surfaceID = surfaceID + let frameMetaStream = store.terminalFrameMetaStream(surfaceID: surfaceID) + let outputStream = store.terminalOutputStream(surfaceID: surfaceID) // Drive every output chunk into the libghostty surface. Ending this // task terminates the stream, which unregisters the surface and // clears its viewport pin on the Mac (see `terminalOutputStream`). outputTask = Task { `@MainActor` [weak surfaceView] in - for await data in store.terminalOutputStream(surfaceID: surfaceID) { + for await data in outputStream { guard !Task.isCancelled else { return } surfaceView?.processOutput(data) } } // Carry per-frame metadata (active screen + full-snapshot scrollback // depth) the opaque byte stream cannot: the view gates Stage 1 local // scroll on the active screen and tracks how much history it holds. A // separate stream so the byte channel stays pure content. frameMetaTask = Task { `@MainActor` [weak surfaceView] in - for await meta in store.terminalFrameMetaStream(surfaceID: surfaceID) { + for await meta in frameMetaStream { guard !Task.isCancelled else { return } surfaceView?.setActiveScreen(isAlternate: meta.isAlternateScreen) if meta.isFullSnapshot { surfaceView?.setHeldScrollbackRows(meta.scrollbackRows) }🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift` around lines 73 - 98, In GhosttySurfaceRepresentable.attach, the outputTask currently opens terminalOutputStream before frameMetaTask subscribes to terminalFrameMetaStream, which can lose the initial replayed full-snapshot meta; fix by creating/starting frameMetaTask (subscribing to terminalFrameMetaStream with the same surfaceID) before creating outputTask (terminalOutputStream) so metadata is registered first; ensure you still capture the same surfaceID/local variables and keep both tasks as `@MainActor` with weak surfaceView.
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 996-1051: This type has grown beyond a single responsibility;
extract the registry-refresh logic out of MobileShellComposite by creating a
dedicated helper/service (e.g. RegistryRouteRefresher) that owns the work
currently in refreshRoutesFromRegistry(for:stackUserID:): move the network call
to deviceRegistry.freshRoutes, the selection via
DeviceRegistryService.selectReconnectRoutes, the active-mac recheck
(pairedMacStore.activeMac), the shouldApplyRegistryRefresh guard, and the
pairedMacStore.upsert call into that new type, and inject dependencies
(deviceRegistry, pairedMacStore, identityProvider, mobileShellLog) via
initializer so MobileShellComposite simply calls
RegistryRouteRefresher.refresh(for:stackUserID:) (or similar) and awaits it; do
the same for the terminal metadata/replay pipeline by creating a
TerminalMetaReplayPipeline type that encapsulates the per-surface metadata
parsing, replay buffering, and streaming interfaces (move the code referenced
around lines 2628-2911), inject required dependencies, and replace the in-class
implementations with thin delegating calls so MobileShellComposite keeps only
orchestration logic and those subsystems are unit-testable in isolation.
- Around line 2670-2717: The meta-stream continuation is removed by plain
surfaceID which allows a stale termination to clobber a newer attach; change the
storage and teardown to be generation-aware by pairing the continuation with the
current attach token/generation and only removing/finishing if the stored
generation matches. Concretely: replace
terminalFrameMetaContinuationsBySurfaceID[String:
AsyncStream<MobileTerminalFrameMeta>.Continuation] with a map that stores
(generationToken, continuation) (or a small struct) when creating the stream in
terminalFrameMetaStream(surfaceID:), and in continuation.onTermination and in
unregisterTerminalOutput check that the generation token matches the current
attach/generation for that surface before calling removeValue(forKey:) or
finish() so old terminations cannot clear a newer continuation.
In
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 1239-1244: The single-boolean latch
localScrollFetchAwaitingSnapshot is racy and must be replaced with an explicit
replay-response discriminator: change localScrollFetchAwaitingSnapshot to hold a
unique request id or enum (e.g., localScrollFetchAwaitingSnapshotID: UUID? or an
incremental Int/enum) and set that id when issuing a deeper-scroll fetch; when
snapshots arrive (both replay responses and full snapshots), only clear or apply
fetch-result logic if the snapshot's metadata/request id matches
localScrollFetchAwaitingSnapshotID; ensure cold-attach/resize/full-snapshot
paths include the snapshot source id so they do not accidentally consume
unrelated latches, and clear the id only on a matching full snapshot or explicit
cancellation.
- Around line 1702-1719: The code snaps local history to live before verifying
the frame actually contains VT bytes, so calls like
consumeLocalScrollSnapRequest() and clearing localScrollUpRowsExact happen even
for empty frames; fix by deferring the snap/restore logic until after you
confirm the frame payload is non-empty (i.e., forwarded contains VT bytes) — for
example, only set snapLocalScrollToLive and apply
pendingLocalScrollRestoreUpRows/localScrollUpRowsExact when forwarded.isEmpty ==
false (or equivalent check) so processOutput(Data()) won't queue
scroll_to_bottom or clear localScrollUpRowsExact on empty frames; update the
same guard around the restore branch that references
pendingLocalScrollRestoreUpRows and MobileDebugLog.anchormux to match.
---
Outside diff comments:
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift`:
- Around line 73-98: In GhosttySurfaceRepresentable.attach, the outputTask
currently opens terminalOutputStream before frameMetaTask subscribes to
terminalFrameMetaStream, which can lose the initial replayed full-snapshot meta;
fix by creating/starting frameMetaTask (subscribing to terminalFrameMetaStream
with the same surfaceID) before creating outputTask (terminalOutputStream) so
metadata is registered first; ensure you still capture the same surfaceID/local
variables and keep both tasks as `@MainActor` with weak surfaceView.
🪄 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: 53d777d8-44e7-4486-90b4-67778c70c6b4
📒 Files selected for processing (5)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/Mobile/MobileTerminalRenderObserver.swiftSources/TerminalController.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift (1)
73-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSubscribe to frame metadata before opening the output stream.
Line 81 opens
terminalOutputStream, andregisterTerminalOutputimmediately triggers a replay. The metadata stream is not registered until Line 91, so the initial full-snapshot meta can be dropped if that replay wins the race. On an idle alt-screen attach, the view then starts with stale primary-screen state and the first scroll gestures go local instead of forwarding to the Mac.♻️ Suggested fix
func attach(surfaceView: GhosttySurfaceView) { self.surfaceView = surfaceView guard let store else { return } let surfaceID = surfaceID + let frameMetaStream = store.terminalFrameMetaStream(surfaceID: surfaceID) + let outputStream = store.terminalOutputStream(surfaceID: surfaceID) // Drive every output chunk into the libghostty surface. Ending this // task terminates the stream, which unregisters the surface and // clears its viewport pin on the Mac (see `terminalOutputStream`). outputTask = Task { `@MainActor` [weak surfaceView] in - for await data in store.terminalOutputStream(surfaceID: surfaceID) { + for await data in outputStream { guard !Task.isCancelled else { return } surfaceView?.processOutput(data) } } // Carry per-frame metadata (active screen + full-snapshot scrollback // depth) the opaque byte stream cannot: the view gates Stage 1 local // scroll on the active screen and tracks how much history it holds. A // separate stream so the byte channel stays pure content. frameMetaTask = Task { `@MainActor` [weak surfaceView] in - for await meta in store.terminalFrameMetaStream(surfaceID: surfaceID) { + for await meta in frameMetaStream { guard !Task.isCancelled else { return } surfaceView?.setActiveScreen(isAlternate: meta.isAlternateScreen) if meta.isFullSnapshot { surfaceView?.setHeldScrollbackRows(meta.scrollbackRows) }🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift` around lines 73 - 98, In GhosttySurfaceRepresentable.attach, the outputTask currently opens terminalOutputStream before frameMetaTask subscribes to terminalFrameMetaStream, which can lose the initial replayed full-snapshot meta; fix by creating/starting frameMetaTask (subscribing to terminalFrameMetaStream with the same surfaceID) before creating outputTask (terminalOutputStream) so metadata is registered first; ensure you still capture the same surfaceID/local variables and keep both tasks as `@MainActor` with weak surfaceView.
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 996-1051: This type has grown beyond a single responsibility;
extract the registry-refresh logic out of MobileShellComposite by creating a
dedicated helper/service (e.g. RegistryRouteRefresher) that owns the work
currently in refreshRoutesFromRegistry(for:stackUserID:): move the network call
to deviceRegistry.freshRoutes, the selection via
DeviceRegistryService.selectReconnectRoutes, the active-mac recheck
(pairedMacStore.activeMac), the shouldApplyRegistryRefresh guard, and the
pairedMacStore.upsert call into that new type, and inject dependencies
(deviceRegistry, pairedMacStore, identityProvider, mobileShellLog) via
initializer so MobileShellComposite simply calls
RegistryRouteRefresher.refresh(for:stackUserID:) (or similar) and awaits it; do
the same for the terminal metadata/replay pipeline by creating a
TerminalMetaReplayPipeline type that encapsulates the per-surface metadata
parsing, replay buffering, and streaming interfaces (move the code referenced
around lines 2628-2911), inject required dependencies, and replace the in-class
implementations with thin delegating calls so MobileShellComposite keeps only
orchestration logic and those subsystems are unit-testable in isolation.
- Around line 2670-2717: The meta-stream continuation is removed by plain
surfaceID which allows a stale termination to clobber a newer attach; change the
storage and teardown to be generation-aware by pairing the continuation with the
current attach token/generation and only removing/finishing if the stored
generation matches. Concretely: replace
terminalFrameMetaContinuationsBySurfaceID[String:
AsyncStream<MobileTerminalFrameMeta>.Continuation] with a map that stores
(generationToken, continuation) (or a small struct) when creating the stream in
terminalFrameMetaStream(surfaceID:), and in continuation.onTermination and in
unregisterTerminalOutput check that the generation token matches the current
attach/generation for that surface before calling removeValue(forKey:) or
finish() so old terminations cannot clear a newer continuation.
In
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 1239-1244: The single-boolean latch
localScrollFetchAwaitingSnapshot is racy and must be replaced with an explicit
replay-response discriminator: change localScrollFetchAwaitingSnapshot to hold a
unique request id or enum (e.g., localScrollFetchAwaitingSnapshotID: UUID? or an
incremental Int/enum) and set that id when issuing a deeper-scroll fetch; when
snapshots arrive (both replay responses and full snapshots), only clear or apply
fetch-result logic if the snapshot's metadata/request id matches
localScrollFetchAwaitingSnapshotID; ensure cold-attach/resize/full-snapshot
paths include the snapshot source id so they do not accidentally consume
unrelated latches, and clear the id only on a matching full snapshot or explicit
cancellation.
- Around line 1702-1719: The code snaps local history to live before verifying
the frame actually contains VT bytes, so calls like
consumeLocalScrollSnapRequest() and clearing localScrollUpRowsExact happen even
for empty frames; fix by deferring the snap/restore logic until after you
confirm the frame payload is non-empty (i.e., forwarded contains VT bytes) — for
example, only set snapLocalScrollToLive and apply
pendingLocalScrollRestoreUpRows/localScrollUpRowsExact when forwarded.isEmpty ==
false (or equivalent check) so processOutput(Data()) won't queue
scroll_to_bottom or clear localScrollUpRowsExact on empty frames; update the
same guard around the restore branch that references
pendingLocalScrollRestoreUpRows and MobileDebugLog.anchormux to match.
---
Outside diff comments:
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift`:
- Around line 73-98: In GhosttySurfaceRepresentable.attach, the outputTask
currently opens terminalOutputStream before frameMetaTask subscribes to
terminalFrameMetaStream, which can lose the initial replayed full-snapshot meta;
fix by creating/starting frameMetaTask (subscribing to terminalFrameMetaStream
with the same surfaceID) before creating outputTask (terminalOutputStream) so
metadata is registered first; ensure you still capture the same surfaceID/local
variables and keep both tasks as `@MainActor` with weak surfaceView.
🪄 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: 53d777d8-44e7-4486-90b4-67778c70c6b4
📒 Files selected for processing (5)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/Mobile/MobileTerminalRenderObserver.swiftSources/TerminalController.swift
🛑 Comments failed to post (4)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (2)
996-1051: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
MobileShellCompositeis absorbing too many responsibilities.This file is already well past the repo’s size threshold, and this PR adds two more subsystems to it: registry-route reconciliation and the per-surface terminal metadata/replay pipeline. Keeping pairing/reconnect, workspace state, terminal streaming, and registry refresh in the same 3k+ line type makes this path much harder to reason about and unit-test in isolation. Please peel the new registry-refresh and terminal-meta/replay code into dedicated helpers instead of extending this store further.
As per coding guidelines,
Packages/**/*.swift: "Flag Swift production files that exceed 400 lines without a clear single responsibility, or exceed 800 lines even with mostly coherent responsibility".Also applies to: 2628-2911
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 996 - 1051, This type has grown beyond a single responsibility; extract the registry-refresh logic out of MobileShellComposite by creating a dedicated helper/service (e.g. RegistryRouteRefresher) that owns the work currently in refreshRoutesFromRegistry(for:stackUserID:): move the network call to deviceRegistry.freshRoutes, the selection via DeviceRegistryService.selectReconnectRoutes, the active-mac recheck (pairedMacStore.activeMac), the shouldApplyRegistryRefresh guard, and the pairedMacStore.upsert call into that new type, and inject dependencies (deviceRegistry, pairedMacStore, identityProvider, mobileShellLog) via initializer so MobileShellComposite simply calls RegistryRouteRefresher.refresh(for:stackUserID:) (or similar) and awaits it; do the same for the terminal metadata/replay pipeline by creating a TerminalMetaReplayPipeline type that encapsulates the per-surface metadata parsing, replay buffering, and streaming interfaces (move the code referenced around lines 2628-2911), inject required dependencies, and replace the in-class implementations with thin delegating calls so MobileShellComposite keeps only orchestration logic and those subsystems are unit-testable in isolation.Source: Coding guidelines
2670-2717:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGuard meta-stream teardown by attach generation.
unregisterTerminalOutputandterminalFrameMetaStream(...).onTerminationboth tear down the metadata continuation by baresurfaceID. On a fast remount, an older stream can terminate after the new attach and remove or finish the current continuation for that same surface, leaving bytes flowing whileisAlternateScreen/scrollbackRowsstop updating. Tie continuation removal to an attach token/generation before clearing it.🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 2670 - 2717, The meta-stream continuation is removed by plain surfaceID which allows a stale termination to clobber a newer attach; change the storage and teardown to be generation-aware by pairing the continuation with the current attach token/generation and only removing/finishing if the stored generation matches. Concretely: replace terminalFrameMetaContinuationsBySurfaceID[String: AsyncStream<MobileTerminalFrameMeta>.Continuation] with a map that stores (generationToken, continuation) (or a small struct) when creating the stream in terminalFrameMetaStream(surfaceID:), and in continuation.onTermination and in unregisterTerminalOutput check that the generation token matches the current attach/generation for that surface before calling removeValue(forKey:) or finish() so old terminations cannot clear a newer continuation.Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift (2)
1239-1244:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCorrelate replay responses explicitly instead of using “next full snapshot wins.”
localScrollFetchAwaitingSnapshotis a single boolean, so any intervening full snapshot (resize/reconnect/cold reattach) consumes the latch before the deeper-replay response arrives. The real replay snapshot then falls through the cold-attach path, which drops the restore state and can bounce the reader back to live after the fetch they just triggered. This needs an explicit replay-response discriminator (request id, source bit, or equivalent metadata), not a bare “next snapshot” flag.Also applies to: 1298-1316
🤖 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/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift` around lines 1239 - 1244, The single-boolean latch localScrollFetchAwaitingSnapshot is racy and must be replaced with an explicit replay-response discriminator: change localScrollFetchAwaitingSnapshot to hold a unique request id or enum (e.g., localScrollFetchAwaitingSnapshotID: UUID? or an incremental Int/enum) and set that id when issuing a deeper-scroll fetch; when snapshots arrive (both replay responses and full snapshots), only clear or apply fetch-result logic if the snapshot's metadata/request id matches localScrollFetchAwaitingSnapshotID; ensure cold-attach/resize/full-snapshot paths include the snapshot source id so they do not accidentally consume unrelated latches, and clear the id only on a matching full snapshot or explicit cancellation.
1702-1719:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't snap out of local history on empty frame payloads.
The stack already emits empty render frames for unchanged-row emissions. Here the snap decision runs before checking whether
forwardedactually contains VT bytes, soprocessOutput(Data())still clearslocalScrollUpRowsExactand queuesscroll_to_bottom. That will kick the user back to live even though no frame bytes were applied.Possible fix
let forwarded = Self.forwardDaemonOutputBytes(data) + let hasFrameBytes = !forwarded.isEmpty // If the user is reading local (primary-screen) history, snap the surface // to the live bottom just before this frame so its absolute-CUP paint // lands in the viewport, not into scrolled-up history. Decided here on the // main actor (where the gesture mutated the offset); applied off-main in // `process_output` order below via the exact `scroll_to_bottom` action. - let snapLocalScrollToLive = consumeLocalScrollSnapRequest() + let snapLocalScrollToLive = hasFrameBytes && consumeLocalScrollSnapRequest() // If this frame is a deeper-scrollback fetch's snapshot (classified by // `setHeldScrollbackRows` off the metadata stream), restore the reader's // position after it applies instead of leaving them bounced to the // bottom. Re-arm the tracked offset here on the main actor so the // restore and the offset stay in step; the surface-side scroll runs // after `process_output` below, in queue order. var restoreLocalScrollUpRows: Double? - if snapLocalScrollToLive, let pending = pendingLocalScrollRestoreUpRows { + if hasFrameBytes, snapLocalScrollToLive, let pending = pendingLocalScrollRestoreUpRows { pendingLocalScrollRestoreUpRows = nil restoreLocalScrollUpRows = pending localScrollUpRowsExact = pending MobileDebugLog.anchormux("scroll.restore up=\(String(format: "%.1f", pending))") }Also applies to: 1729-1750
🤖 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/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift` around lines 1702 - 1719, The code snaps local history to live before verifying the frame actually contains VT bytes, so calls like consumeLocalScrollSnapRequest() and clearing localScrollUpRowsExact happen even for empty frames; fix by deferring the snap/restore logic until after you confirm the frame payload is non-empty (i.e., forwarded contains VT bytes) — for example, only set snapLocalScrollToLive and apply pendingLocalScrollRestoreUpRows/localScrollUpRowsExact when forwarded.isEmpty == false (or equivalent check) so processOutput(Data()) won't queue scroll_to_bottom or clear localScrollUpRowsExact on empty frames; update the same guard around the restore branch that references pendingLocalScrollRestoreUpRows and MobileDebugLog.anchormux to match.
… engine The stage-1 logic grew three god files past the Swift file-length budget (GhosttySurfaceView.swift +273, MobileShellComposite.swift +94, TerminalController.swift +19). Instead of refreshing budgets, split along the seams the logic already wanted: - MobileLocalScrollEngine (CmuxMobileTerminalKit): every Stage 1 gate and latch (routing, read-only scroll offset, deeper-fetch dedupe and classification latches, fully-loaded ceiling, snap-to-live + restore decisions) as a pure value type with 20 unit tests. The UIKit view keeps only gesture plumbing and libghostty C calls (GhosttySurfaceView+Scroll.swift), so stage 2's sub-line interpolation and momentum math lands in a testable type instead of view code. - MobileTerminalFrameMetaHub (CmuxMobileShell): owns the per-surface frame-metadata streams; MobileShellComposite+TerminalReplay.swift owns the cold-attach / deeper-scrollback replay path. - MobileReplayScrollbackBudget (Sources/Mobile): the Mac-side replay scrollback budgets and clamping, out of TerminalController. No behavior change; same decisions, same call sites, same wire protocol. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift (1)
1439-1445:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate hot-path
anchormuxprobes behind#if DEBUG.Line 1443 is on the per-frame output path; keeping this probe active in release adds avoidable work on a latency-sensitive route. Apply the same guard to the new probes in
Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+Scroll.swift(Lines 29, 75, 92).Suggested fix
- if snapLocalScrollToLive { - MobileDebugLog.anchormux( - "scroll.snapLive restore=\(restoreLocalScrollUpRows.map { String(format: "%.1f", $0) } ?? "none")" - ) - } + `#if` DEBUG + if snapLocalScrollToLive { + MobileDebugLog.anchormux( + "scroll.snapLive restore=\(restoreLocalScrollUpRows.map { String(format: "%.1f", $0) } ?? "none")" + ) + } + `#endif`As per coding guidelines, apply the cmux custom Swift lint rules in
.github/review-bot-rules/; debug instrumentation should not run on release hot paths.🤖 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/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift` around lines 1439 - 1445, The new MobileDebugLog.anchormux probes added in GhosttySurfaceView+Scroll.swift (referenced near the snap/snapping logic and the localScroll handling at the probes around lines 29, 75, and 92) must be guarded with the same compile-time check used elsewhere (`#if` DEBUG) to avoid executing debug instrumentation on the release per-frame hot path; wrap each MobileDebugLog.anchormux call in the file (and any related string construction or map formatting used just for the log) with `#if` DEBUG / `#endif` so the logging and its allocations are compiled out in release builds, matching the existing pattern used in GhosttySurfaceView.swift.Source: Coding guidelines
🤖 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
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView`+Scroll.swift:
- Around line 46-47: The gesture handler ends currently call
flushPendingScrollIfNeeded() without applying the gesture's final translation;
update the .ended and .cancelled branch in the pan handler (the method handling
the UIPanGestureRecognizer in GhosttySurfaceView+Scroll.swift) to first read the
recognizer's final translation (e.g., recognizer.translation(in: view)), convert
it into the same delta units used by addPendingScroll / appendPendingScroll,
call the existing function that queues scroll chunks (the same symbol used
elsewhere to accumulate scroll deltas), reset the recognizer translation to
.zero, and only then call flushPendingScrollIfNeeded() so the final residual
delta is not lost.
---
Duplicate comments:
In
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 1439-1445: The new MobileDebugLog.anchormux probes added in
GhosttySurfaceView+Scroll.swift (referenced near the snap/snapping logic and the
localScroll handling at the probes around lines 29, 75, and 92) must be guarded
with the same compile-time check used elsewhere (`#if` DEBUG) to avoid executing
debug instrumentation on the release per-frame hot path; wrap each
MobileDebugLog.anchormux call in the file (and any related string construction
or map formatting used just for the log) with `#if` DEBUG / `#endif` so the logging
and its allocations are compiled out in release builds, matching the existing
pattern used in GhosttySurfaceView.swift.
🪄 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: 8d32ca22-326f-46d1-8072-ed63dae0cf32
📒 Files selected for processing (10)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplay.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileTerminalFrameMetaHub.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+Scroll.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/MobileLocalScrollEngine.swiftPackages/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/MobileLocalScrollEngineTests.swiftSources/Mobile/MobileReplayScrollbackBudget.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
…e ordered chunk Review fixes from PR #5628 (Cursor bugbot, three findings, one root cause): the deeper-fetch scroll restore was armed on the frame-meta stream but consumed by the byte stream's next chunk, and the two streams raced. An interleaved live frame could consume a restore armed for a later snapshot (high), the restore went stale if the reader scrolled back to the bottom before the snapshot bytes ran (medium), and a snapshot's bytes could apply before their own metadata, dropping the restore entirely (medium). Instead of patching each window, remove the race class: a render-grid frame's metadata and VT bytes now travel as ONE element (MobileTerminalOutputChunk) on the single ordered output stream, so the view applies a frame's metadata immediately before that frame's bytes in the same synchronous main-actor iteration. The engine's arm-then-consume contract becomes structural: a deeper-fetch restore is consumed by the fetch snapshot's own apply, never by an interleaved live frame, and cannot go stale across a gesture. The separate MobileTerminalFrameMetaHub stream and its coordinator task are deleted; raw compatibility bytes flow as chunks with no metadata, and metadata-only frames (no row changes) still carry the active screen without reaching process_output. New CmuxMobileShellTests cover the delivery contract, and the CmuxMobileShell + CmuxMobileTerminalKit package suites are added to the CI swift-test gate. Also fold the pan gesture's residual translation into the pending scroll on .ended/.cancelled so the final chunk of a swipe is not dropped (CodeRabbit).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 102a8b8. Configure here.
| /// value), so it stays in step with the real surface position. | ||
| public mutating func consumeSnapRequest() -> SnapDecision { | ||
| guard isLocalScrollActive else { | ||
| return SnapDecision(snapToLive: false, restoreUpRows: nil) |
There was a problem hiding this comment.
Sub-half-row scroll skips snap
Medium Severity
When primary local scroll leaves upRowsExact below half a row, isLocalScrollActive is false so consumeSnapRequest() does not snap, but scrollLocalSurface still applies the same delta via ghostty_surface_mouse_scroll. Live frames then apply without scroll_to_bottom, so new output can paint into a slightly scrolled-up viewport.
Reviewed by Cursor Bugbot for commit 102a8b8. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplay.swift (1)
71-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't turn malformed replay payloads into silent success.
Line 71 swallows
MobileTerminalReplayResponse.decode(data)failures, and Lines 72-73 do the same for invalid base64 fields. That drops cold-attach or deeper-scrollback replays as a quiet no-op, so the surface stays stale and never reaches the existing error/re-auth path. Treat malformed replay payloads asinvalidResponseand fail into thecatchblock instead.Possible fix
- let payload = try? MobileTerminalReplayResponse.decode(data) - let bytes = payload?.dataBase64.flatMap { Data(base64Encoded: $0) } - let snapshotBytes = payload?.snapshotBase64.flatMap { Data(base64Encoded: $0) } + let payload = try MobileTerminalReplayResponse.decode(data) + let bytes = try payload.dataBase64.map { + guard let decoded = Data(base64Encoded: $0) else { + throw MobileShellConnectionError.invalidResponse + } + return decoded + } + let snapshotBytes = try payload.snapshotBase64.map { + guard let decoded = Data(base64Encoded: $0) else { + throw MobileShellConnectionError.invalidResponse + } + return decoded + }Also applies to: 114-117
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalReplay.swift around lines 71 - 74, Currently the code silently converts malformed replay payloads into nils (using try? and flatMap with Data(base64Encoded:)), which masks errors; change MobileTerminalReplayResponse.decode(data) to use try so decoding failures throw, and explicitly validate base64 fields (convert payload.dataBase64 and payload.snapshotBase64 with Data(base64Encoded:) and if they return nil throw MobileShellComposite.Error.invalidResponse) instead of mapping to nil; do the same for payload.renderGrid validation so any malformed payload triggers the catch path. Also apply the identical fix to the other occurrence that currently uses the same optional-decoding pattern (the block around lines 114-117).
🤖 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/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift`:
- Around line 28-29: The loop consuming store.terminalOutputStream(surfaceID:)
is appending empty strings for metadata-only chunks (chunk.bytes is empty),
which pollutes collector.lines; change the consumer in cmuxFeatureTests.swift
(the for await chunk in store.terminalOutputStream(...) body where
self?.lines.append(...) is called) to skip metadata-only frames by guarding that
String(data: chunk.bytes, encoding: .utf8) produces a non-empty string before
appending — e.g., continue when chunk.bytes.isEmpty or when the decoded string
is nil/empty — so collector.lines only receives real text lines for accurate
collector.lines.count-based waits/assertions.
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift`:
- Around line 91-95: The loop currently feeds every full snapshot into
MobileLocalScrollEngine.noteFullSnapshot(scrollbackRows:), which lets alt-screen
snapshots overwrite primary-screen latches; change the logic so that after
view.setActiveScreen(isAlternate: meta.isAlternateScreen) and
view.setHeldScrollbackRows(meta.scrollbackRows) you only call
MobileLocalScrollEngine.noteFullSnapshot(scrollbackRows:) when
meta.isAlternateScreen is false (i.e., for primary screen snapshots only).
Locate the invocation of noteFullSnapshot and wrap it with a guard checking
!meta.isAlternateScreen (keep view.setHeldScrollbackRows as-is so UI state still
receives the rows).
---
Outside diff comments:
In
`@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalReplay.swift:
- Around line 71-74: Currently the code silently converts malformed replay
payloads into nils (using try? and flatMap with Data(base64Encoded:)), which
masks errors; change MobileTerminalReplayResponse.decode(data) to use try so
decoding failures throw, and explicitly validate base64 fields (convert
payload.dataBase64 and payload.snapshotBase64 with Data(base64Encoded:) and if
they return nil throw MobileShellComposite.Error.invalidResponse) instead of
mapping to nil; do the same for payload.renderGrid validation so any malformed
payload triggers the catch path. Also apply the identical fix to the other
occurrence that currently uses the same optional-decoding pattern (the block
around lines 114-117).
🪄 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: 38dabc10-0a69-4d31-abc7-312aa6aa357d
📒 Files selected for processing (10)
.github/workflows/ci.ymlPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplay.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileTerminalOutputChunkDeliveryTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileTerminalOutputChunk.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileTerminalOutputSinking.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+Scroll.swiftPackages/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/MobileLocalScrollEngine.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
| for await chunk in store.terminalOutputStream(surfaceID: surfaceID) { | ||
| self?.lines.append(String(data: chunk.bytes, encoding: .utf8) ?? "") |
There was a problem hiding this comment.
Ignore metadata-only chunks when collecting output lines.
At Line 29, empty-byte chunks are appended as "". Since metadata-only frames are valid, this can add synthetic entries and make collector.lines.count-based waits/assertions pass on non-text events.
Suggested patch
- for await chunk in store.terminalOutputStream(surfaceID: surfaceID) {
- self?.lines.append(String(data: chunk.bytes, encoding: .utf8) ?? "")
- }
+ for await chunk in store.terminalOutputStream(surfaceID: surfaceID) {
+ guard !chunk.bytes.isEmpty else { continue }
+ self?.lines.append(String(decoding: chunk.bytes, as: UTF8.self))
+ }🤖 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/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift` around lines
28 - 29, The loop consuming store.terminalOutputStream(surfaceID:) is appending
empty strings for metadata-only chunks (chunk.bytes is empty), which pollutes
collector.lines; change the consumer in cmuxFeatureTests.swift (the for await
chunk in store.terminalOutputStream(...) body where self?.lines.append(...) is
called) to skip metadata-only frames by guarding that String(data: chunk.bytes,
encoding: .utf8) produces a non-empty string before appending — e.g., continue
when chunk.bytes.isEmpty or when the decoded string is nil/empty — so
collector.lines only receives real text lines for accurate
collector.lines.count-based waits/assertions.
| if let meta = chunk.meta { | ||
| view.setActiveScreen(isAlternate: meta.isAlternateScreen) | ||
| if meta.isFullSnapshot { | ||
| view.setHeldScrollbackRows(meta.scrollbackRows) | ||
| } |
There was a problem hiding this comment.
Only feed primary-screen full snapshots into noteFullSnapshot.
MobileLocalScrollEngine.noteFullSnapshot(scrollbackRows:) is documented as a primary snapshot input, but this loop invokes it for every full snapshot. If a replay/self-heal lands while meta.isAlternateScreen is true, the engine overwrites heldScrollbackRows plus its fetch/restore latches with alt-screen state, so when the session returns to primary it can immediately think local history is empty or fully loaded and mis-handle deeper-fetch restore/retry.
Suggested fix
if let meta = chunk.meta {
view.setActiveScreen(isAlternate: meta.isAlternateScreen)
- if meta.isFullSnapshot {
+ if meta.isFullSnapshot, !meta.isAlternateScreen {
view.setHeldScrollbackRows(meta.scrollbackRows)
}
}As per coding guidelines, replay/local-history paths must not silently drop or misapply history when cross-mode or stale state is possible.
📝 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.
| if let meta = chunk.meta { | |
| view.setActiveScreen(isAlternate: meta.isAlternateScreen) | |
| if meta.isFullSnapshot { | |
| view.setHeldScrollbackRows(meta.scrollbackRows) | |
| } | |
| if let meta = chunk.meta { | |
| view.setActiveScreen(isAlternate: meta.isAlternateScreen) | |
| if meta.isFullSnapshot, !meta.isAlternateScreen { | |
| view.setHeldScrollbackRows(meta.scrollbackRows) | |
| } | |
| } |
🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift`
around lines 91 - 95, The loop currently feeds every full snapshot into
MobileLocalScrollEngine.noteFullSnapshot(scrollbackRows:), which lets alt-screen
snapshots overwrite primary-screen latches; change the logic so that after
view.setActiveScreen(isAlternate: meta.isAlternateScreen) and
view.setHeldScrollbackRows(meta.scrollbackRows) you only call
MobileLocalScrollEngine.noteFullSnapshot(scrollbackRows:) when
meta.isAlternateScreen is false (i.e., for primary screen snapshots only).
Locate the invocation of noteFullSnapshot and wrap it with a guard checking
!meta.isAlternateScreen (keep view.setHeldScrollbackRows as-is so UI state still
receives the rows).
Source: Coding guidelines
…roll from the Mac, #5628) Composite drops the old inline requestTerminalReplay in favor of the branch's MobileShellComposite+TerminalReplay extension (deeper- scrollback fetch). Representable takes the branch's chunk/meta consume loop with dog's DEBUG stream-cancel diagnostic re-added. Surface view unions dog's composer/paste delegate methods with the branch's didReachLocalHistoryTop hook.
…#5596/#5625/#5628) over current main Beta queue (#5876/#5872/#5869/#5875/#5927/#5912/#5726/#5776/#5916) is now on main; conflicts resolved by taking main as authoritative for the merged workspace-list/notifications/read-state/close surface, while preserving the carry-set: terminal.paste capability (#5572), hidden-input strings (#5596), smooth-scroll/scroll-to-bottom (#5628), and the live notifications feed (notificationsStore + mobile.notifications.list/mark_read dispatch). Dropped the superseded mute design. Capability flags unified onto main's computed supportedHostCapabilities set (added computed supportsTerminalPaste + DEBUG supportsDogfoodChecklist). xcstrings merged (HEAD-precedence union, mute keys dropped). pbxproj took HEAD consistently; budget regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d observer + replay scrollback budget) Taking HEAD's pbxproj in the step-1 fold dropped the dog-only file references for Sources/Mobile/MobileNotificationListObserver.swift (#5726 notifications.updated feed) and MobileReplayScrollbackBudget.swift (#5628 smooth-scroll). Both are tracked on disk and referenced by AppDelegate but were unwired, breaking the macOS compile. Re-added the PBXBuildFile/PBXFileReference/group/sources entries; macOS Debug builds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Stage 1 of iOS terminal smooth scroll. Behind the alt-mode gate, TUIs unaffected.
What changed
In a NORMAL (primary, non-alt) shell, a swipe now scrolls the phone's own locally-held history with no per-frame
mobile.terminal.scrollRPC to the Mac. Alt-screen behavior (vim, less --mouse, htop, lazygit) is unchanged: it still forwards to the Mac so the program owns the scroll.MobileShellComposite.terminalFrameMetaStream, consumed inGhosttySurfaceRepresentable.Coordinator.)ghostty_surface_mouse_scrollon the phone's own libghostty surface, serialized on the sameoutputQueueasprocess_output, no RPC. (GhosttySurfaceView.scrollLocalSurface.)mobile.terminal.replaynow takes an optional, clampedscrollback_linesparam; the primary full-snapshot reflow lands deeper history into the phone's surface so it keeps scrolling offline. (TerminalController.v2MobileTerminalReplay,MobileShellComposite.requestDeeperScrollback.)scroll_to_bottombinding action, so a live delta's absolute-CUP paint lands in the viewport rather than into scrolled-up history. The Mac stays the single source of truth; the phone only owns a read-only scroll position into already-received history.The local offset is accumulated as a
Double(no per-frame rounding) so sub-row residuals don't drift it below the surface's real position; the snap is exact regardless, so correctness doesn't depend on offset precision.Why this is safe for TUIs
Everything is gated on
activeScreen. In the alternate screen the gesture path is byte-for-byte the old forward-to-Mac path (didScrollLinesdelegate,mobile.terminal.scroll). Only the primary screen takes the new local path. Flipping into alt mid-scroll immediately drops the local offset and reverts to forwarding.Known Stage 1 limitations (deferred to later stages)
Design doc
Full staged design (continuous local-scrollback accumulation: fetch-on-demand vs streamed scrolled-off lines with a recommendation; the one-source-of-truth / read-only-overlay answer to the prior "two sources of truth" objection; snap-to-live reconciliation; pixel-smooth via overscan+CALayer vs UIScrollView-hosted layer with a recommendation) lives in
plans/feat-ios-smooth-scroll/DESIGN.mdin the cmuxterm-hq control repo (not in this repo).Verification
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches mobile terminal sync, scroll routing, and Mac replay RPC semantics; alt-screen path is preserved but snap/restore and replay ordering are easy to get wrong under load.
Overview
Stage 1 iOS smooth scroll on the primary screen: pan gestures scroll the phone’s local libghostty history (no per-frame scroll RPC), while alternate screens still forward wheel deltas to the Mac unchanged.
Terminal output is now
MobileTerminalOutputChunk(frame metadata + VT bytes in one ordered stream element).GhosttySurfaceRepresentableapplies active-screen and scrollback metadata immediately beforeprocessOutput, fixing races between metadata and bytes. Live and replay render-grid paths usedeliverTerminalFrame; empty-byte frames still flow for active-screen flips.MobileLocalScrollEngine(inCmuxMobileTerminalKit) owns routing, offset tracking, snap-to-live, and deeper-fetch latches;GhosttySurfaceView+Scrollwires pan → localghostty_surface_mouse_scrollonoutputQueue.processOutputsnaps to live bottom before applying live output when the reader is scrolled up, and restores scroll position after a deeper-fetch snapshot.requestDeeperScrollback/ replay extension callsmobile.terminal.replaywith optionalscrollback_lines; the Mac clamps viaMobileReplayScrollbackBudget(max 2000). No-row-change Mac frames now propagateactiveScreen. CI runsswift testonCmuxMobileShellandCmuxMobileTerminalKit.Reviewed by Cursor Bugbot for commit 102a8b8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Stage 1 of iOS smooth scroll: on the primary (non‑alt) screen, swipes scroll locally on-device with no per-frame RPCs; TUIs on the alt screen are unchanged and still forward to the Mac. This update also delivers frame metadata and bytes as one ordered stream to remove races and keep the reader anchored across deeper-history fetches.
New Features
terminalOutputStreamnow yieldsMobileTerminalOutputChunk(frame metadata + VT bytes together). The view appliesactiveScreenand snapshotscrollbackRowsbefore the same frame’s bytes; empty-byte frames still carry the screen flag. This replaces the separate per-surface metadata stream.ghostty_surface_mouse_scrollon the phone’s libghostty surface (serialized onoutputQueue) viaGhosttySurfaceView.scrollLocalSurface. Until the first metadata arrives (older host/raw bytes), gestures keep forwarding to the Mac.store.requestDeeperScrollback→mobile.terminal.replay(scrollback_lines); clamped byMobileReplayScrollbackBudget(max 2000). No ceiling bounce when fully loaded.Refactors
MobileLocalScrollEngineinCmuxMobileTerminalKitwith unit tests;GhosttySurfaceView+Scroll.swiftholds the UIKit/libghostty glue. Pan-end now folds residual translation before the final flush.MobileTerminalFrameMetaHub; introducedMobileTerminalOutputChunkand updatedMobileTerminalOutputSinking/MobileShellCompositeto deliver metadata+bytes as one element. Added tests for the delivery contract and enabledCmuxMobileShellandCmuxMobileTerminalKitin CI Swift tests.Written for commit 102a8b8. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Tests