Fix iOS rendered terminal scrolling and replay reconstruction - #7905
Fix iOS rendered terminal scrolling and replay reconstruction#7905azooz2003-bit wants to merge 104 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 scrolling now supports directional runs, bidirectional history prefetching, interaction epochs, render revisions, optimistic local scrolling, authoritative reconciliation, mount-safe output delivery, and replay recovery across shared models, host RPCs, iOS rendering, tests, and Ghostty integration. ChangesTerminal scroll synchronization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 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}
Greptile SummaryThis PR reworks iOS terminal scrolling and replay synchronization. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (46): Last reviewed commit: "build: pin bounded render grid Ghostty" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| interactionEpoch = try container.decodeIfPresent(UInt64.self, forKey: .interactionEpoch) | ||
| clientScrollRevision = try container.decodeIfPresent(UInt64.self, forKey: .clientScrollRevision) | ||
| renderRevision = try container.decodeIfPresent(UInt64.self, forKey: .renderRevision) | ||
| renderGrid = try? container.decodeIfPresent(MobileTerminalRenderGridFrame.self, forKey: .renderGrid) |
There was a problem hiding this comment.
When render_grid is present but fails to decode, try? silently converts it to nil while retaining accepted and render_revision. The session then completes reconciliation and advances the revision floor without applying the authoritative frame, so a later valid event at that revision is rejected as stale and the optimistic viewport can remain visible.
| renderGrid = try? container.decodeIfPresent(MobileTerminalRenderGridFrame.self, forKey: .renderGrid) | |
| renderGrid = try container.decodeIfPresent(MobileTerminalRenderGridFrame.self, forKey: .renderGrid) |
There was a problem hiding this comment.
Verified on current HEAD: decoding now uses throwing decodeIfPresent, and scrollResponseRejectsMalformedNestedRenderGrid covers the malformed nested frame. The response cannot advance reconciliation after a malformed grid.
— Claude Code
| prefetchWindow: prefetchWindow(for: lines) | ||
| ) | ||
| isAwaitingAuthoritativeReconciliation = true | ||
| guard enqueueLocal(request) else { return } |
There was a problem hiding this comment.
If a long reversing gesture fills the directional-run bound, this return happens after the revision is incremented and authoritative reconciliation is marked pending, but before the remote request is queued. No response can reconcile that revision, so live render-grid events remain deferred until another lifecycle event happens to reset or settle the session.
There was a problem hiding this comment.
Verified non-actionable on current HEAD. enqueueLocal calls recoverFromLaneFailure when append rejects the bounded journal; invalidate clears isAwaitingAuthoritativeReconciliation and requests replay before submit returns. journalOverflowRecoversCleanly asserts the cleared barrier and replay epoch.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@ghostty`:
- Line 1: Push ghostty submodule commit 267824293722b63a13513a923031ac1090b7de88
to origin/main, then retain this gitlink pointer so fresh checkouts and CI can
resolve the referenced commit.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 7092-7121: Extract the stale-epoch handling branch from the
response closure in requestTerminalReplay into a focused helper, preserving its
logging, in-flight cleanup, barrier-aware retry, barrier clearing, and
early-return behavior. Pass the existing surface, request, epoch, barrier, and
dropped-output values through the helper so the branch remains independently
testable without changing adjacent sequence-staleness or delivery logic.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalOutputDeliveryQueueTests.swift`:
- Around line 61-64: Replace the fixed `Task.yield()` loop following
`oldConsumer.cancel()` and `await oldConsumer.value` with the existing
`pollUntil(...)` helper, polling the actual predicate that confirms the old
consumer has been unregistered or cleanup has completed before continuing
assertions.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollSessionTests.swift`:
- Around line 217-221: Replace the fixed-count Task.yield loop in settleTasks
with a deadline-bounded predicate polling helper, following the established
pollUntil pattern. Update every call site to await settleUntil with the concrete
state predicate needed before each assertion, such as pending remote work, and
require the result before accessing state. Remove blind settleTasks-based
synchronization while preserving the existing test assertions.
🪄 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: 4caa1010-0b21-4ce7-aa70-dd09b7cd0823
📒 Files selected for processing (34)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileTerminalRenderGrid.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileTerminalRenderGridReplay.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileTerminalScrollRun.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/MobileTerminalRenderGridTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileTerminalScrollResponse.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileTerminalDTODecodeTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalScrollDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalViewport.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollSession.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalOutputDeliveryQueueTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalReplayPrefetchTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollDeliveryQueueTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollReplayEpochTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollSessionTests.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileTerminalOutputSinking.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+RenderRecovery.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceViewDelegate.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceWorkSnapshot.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+Mobile.swiftSources/Mobile/MobileTerminalRenderObserver.swiftSources/TerminalController+MobileScrollPrefetch.swiftSources/TerminalController.swiftdocs/ghostty-fork.mdghosttyios/cmuxPackage/Tests/cmuxFeatureTests/TerminalInputScrollToBottomTests.swiftios/cmuxUITests/cmuxUITests.swift
💤 Files with no reviewable changes (2)
- Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollDeliveryQueueTests.swift
- Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift
| @@ -1 +1 @@ | |||
| Subproject commit e215e78bf04df3f7cecbef665eec051a203baf6a | |||
| Subproject commit 267824293722b63a13513a923031ac1090b7de88 | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cd ghostty || { echo "ghostty submodule dir not found"; exit 1; }
git fetch origin --quiet 2>/dev/null || true
echo "Pinned commit from parent:"
git -C .. ls-tree HEAD ghostty
TARGET=267824293722b63a13513a923031ac1090b7de88
echo "Commit exists locally?"; git cat-file -t "$TARGET" 2>/dev/null || echo "MISSING"
echo "Ancestor of origin/main?"; git merge-base --is-ancestor "$TARGET" origin/main 2>/dev/null && echo "yes" || echo "NO/unknown"
git remote -vRepository: manaflow-ai/cmux
Length of output: 425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd ghostty
echo "Remote main tip:"
git ls-remote origin refs/heads/main
echo
echo "Fetch remote main into local refs (read-only):"
git fetch origin main:refs/remotes/origin/main --quiet
TARGET=267824293722b63a13513a923031ac1090b7de88
echo
echo "Does origin/main contain the target commit?"
if git merge-base --is-ancestor "$TARGET" origin/main; then
echo yes
else
echo no
fi
echo
echo "Commit metadata:"
git show -s --format='%H %D %s' "$TARGET"Repository: manaflow-ai/cmux
Length of output: 476
Push the ghostty submodule commit to origin/main before landing this pointer. 267824293722b63a13513a923031ac1090b7de88 is on origin/feat/mobile-render-grid-bidirectional, not origin/main, so this gitlink can be orphaned for fresh checkouts and CI.
🤖 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 `@ghostty` at line 1, Push ghostty submodule commit
267824293722b63a13513a923031ac1090b7de88 to origin/main, then retain this
gitlink pointer so fresh checkouts and CI can resolve the referenced commit.
There was a problem hiding this comment.
The commit is reachable from origin/feat/mobile-render-grid-bidirectional and dependency PR manaflow-ai/ghostty#107 is open, clean, and separately awaiting merge authorization. This review loop is explicitly not authorized to merge that PR or push Ghostty main, so the dependency remains tracked here.
— Claude Code
There was a problem hiding this comment.
@azooz2003-bit Thanks for the context — that makes sense. Since 267824293722b63a13513a923031ac1090b7de88 is reachable from origin/feat/mobile-render-grid-bidirectional and tracked via ghostty#107, and merging that upstream branch is outside this review's scope/authority, I'll leave this as-is for now.
Just flagging for visibility: this repo's ghostty gitlink will remain unresolvable for anyone doing a fresh checkout (including CI) until ghostty#107 is merged to main. Worth treating that merge as a hard prerequisite before landing this PR, so the pointer doesn't go stale/orphaned in the meantime.
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/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalOutputDelivery.swift`:
- Around line 231-234: Replace the eager filtering and reallocation in
discardPendingOptimisticScrollDeliveries with lazy generation invalidation:
advance the optimistic-scroll generation/state there without scanning pending.
Update popPending to skip and discard stale supersedable deliveries during
traversal, while preserving barriers and non-supersedable entries; ensure
repeated scroll intents traverse the queue amortized linearly.
🪄 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: e5187888-787f-4af0-b8cc-81bec68e221e
📒 Files selected for processing (5)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputReconciliation.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputStream.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollSession.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalOutputDeliveryScrollTests.swift
This comment has been minimized.
This comment has been minimized.
|
Found 1 test failure on Blacksmith runners: Failure
|
…arch # Conflicts: # Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift # Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swift # Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+Mobile.swift # docs/ghostty-fork.md # ghostty # scripts/ghosttykit-checksums.txt
|
Too many files changed for review. ( Bypass the limit by tagging |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
1 participant
Summary
Ghostty dependency: manaflow-ai/ghostty#107
Verification
Packages/iOS/CmuxMobileShell: 418 tests in 35 suites passedPackages/Shared/CMUXMobileCore: 148 tests in 10 suites passedPackages/iOS/CmuxMobileRPC: 76 tests in 9 suites passediscrlpassed48F127C0-4E28-4118-AA0A-9255F812BE29d238b7c933and fix commit2e0a315c4fDogfood
Use the paired
iscrlbuild with the 15k fast log, 12k reversal markers, and 10k diff history workspaces. Fast reverse scroll, reconnect mid-history, background/foreground, rotate, and return to the newest row.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Large refactor of mobile terminal output, scroll reconciliation, and Mac RPC wire format; regressions would show as wrong viewport, cursor, or history after fast scroll, reconnect, or typing.
Overview
Reworks iOS remote terminal scrolling and sync so gestures, clicks, and typing share one per-surface interaction owner instead of independent fire-and-forget RPCs.
Render-grid protocol gains
render_revision, scroll-forward history (scrollforward_rows/ spans), and richer cursor metadata (location,active_row). Replay now stitches older → viewport → newer history and restores cursor position with legacy fallbacks when metadata is missing.MobileTerminalScrollRunand typedMobileTerminalScrollResponsesupport ordered run batches and epoch/revision reconciliation with the Mac.Output delivery moves from raw byte chunks to
MobileTerminalSurfaceMutation(output, local scroll, scroll-to-bottom, barriers) with delivery IDs, optimistic scroll coalescing, scroll-reconciliation supersession, deferred live grids during in-flight scroll, andrender_revisionstaleness floors alongside existing byte-seq checks. Cold replay and scroll RPCs pass bidirectional prefetch windows (e.g. 600/120 rows).Text/paste/image input and viewport clears are routed through the scroll session with
interaction_epoch; RPC client disconnect is deferred while input requests are in flight.Reviewed by Cursor Bugbot for commit cbe892f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes iOS terminal scroll ordering, replay, and render continuity with bounded, direction‑aware per‑surface transactions and strict geometry/output ordering. Isolates render‑event overflow to the affected topic and keeps other streams and sinks alive; pins
ghosttyto the bounded, cursor‑aware render‑grid build.New Features
terminal.scroll.ordered_runs.v1.MobileEventEnvelopepre‑extractssurfaceIDfor pre‑decode ordering.Bug Fixes
Written for commit 736eb39. Summary will update on new commits.
Summary by CodeRabbit