Keep iOS terminal render recovery live - #7301
Closed
azooz2003-bit wants to merge 28 commits into
Closed
azooz2003-bit wants to merge 28 commits into
azooz2003-bit wants to merge 28 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:
📝 WalkthroughWalkthroughGhosttySurfaceView now keeps render recovery active while it captures and replays snapshot fallback state. MobileShellComposite and the root view now track foreground activity to gate render-grid liveness, with tests covering recovery, foreground-resume, chat/terminal presentation behavior, and dictation tap capture. ChangesRender pipeline recovery and snapshot fallback
Foreground-active render-grid liveness
Chat presentation and input handoff
Dictation tap request capture
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (24 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 |
Contributor
Greptile SummaryThis PR fixes a frozen-terminal regression on iOS where a single stalled Ghostty
Confidence Score: 5/5The recovery pipeline change is well-isolated with comprehensive regression tests covering the stuck-free, backlog-cap, and snapshot-fallback paths; the ping/pong path correctly cancels waiters on backgrounding before any restart decision. All core correctness invariants are preserved: surfaceHasReceivedOutput still gates snapshot hiding; finishTerminalEventPong uses removeValue to prevent double-resumption; cancelTerminalEventPongWaiters drains correctly on backgrounding; off-main snapshot reads are ordered ahead of surface frees on the serial outputQueue. Visibility widenings follow the canonical pattern consumed via @testable import with no ForTesting wrapper in production callers. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant RootView as CMUXMobileRootView
participant Shell as MobileShellComposite
participant Mac as MobileHostService
participant SurfaceView as GhosttySurfaceView
RootView->>Shell: setSceneForegroundActive(true, sceneID)
RootView->>Shell: resumeForegroundRefresh()
Shell->>Mac: mobile.events.subscribe (reassert)
Mac-->>Shell: already_subscribed true
Shell->>Mac: mobile.events.ping
Mac->>Shell: event mobile.events.pong
Mac-->>Shell: delivered true
Note over Shell: stream healthy, no restart
Note over SurfaceView: Render recovery
SurfaceView->>SurfaceView: captureSnapshotFallbackForRecovery()
SurfaceView->>SurfaceView: "initializeSurface() surfaceHasReceivedOutput=false"
SurfaceView->>SurfaceView: flushSnapshotFallbackPresentation()
Mac->>SurfaceView: process_output replay
SurfaceView->>SurfaceView: "surfaceHasReceivedOutput=true hide snapshot"
%%{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 RootView as CMUXMobileRootView
participant Shell as MobileShellComposite
participant Mac as MobileHostService
participant SurfaceView as GhosttySurfaceView
RootView->>Shell: setSceneForegroundActive(true, sceneID)
RootView->>Shell: resumeForegroundRefresh()
Shell->>Mac: mobile.events.subscribe (reassert)
Mac-->>Shell: already_subscribed true
Shell->>Mac: mobile.events.ping
Mac->>Shell: event mobile.events.pong
Mac-->>Shell: delivered true
Note over Shell: stream healthy, no restart
Note over SurfaceView: Render recovery
SurfaceView->>SurfaceView: captureSnapshotFallbackForRecovery()
SurfaceView->>SurfaceView: "initializeSurface() surfaceHasReceivedOutput=false"
SurfaceView->>SurfaceView: flushSnapshotFallbackPresentation()
Mac->>SurfaceView: process_output replay
SurfaceView->>SurfaceView: "surfaceHasReceivedOutput=true hide snapshot"
Reviews (19): Last reviewed commit: "Stabilize iOS reconnect recovery" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+782
to
+790
| @discardableResult | ||
| func simulateRenderRecoveryWithStuckPriorFreeForTesting() -> Bool { | ||
| pendingSurfaceFreeCount = max(pendingSurfaceFreeCount, Self.surfaceFreeBacklogWarningThreshold) | ||
| return recoverRenderPipeline( | ||
| reason: "test_stuck_prior_free", | ||
| stalledMs: Int(Self.renderPipelineStallDeadline * 1000), | ||
| replay: .delegateWhenNoCaller | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
simulateRenderRecoveryWithStuckPriorFreeForTesting() is a new #if DEBUG-gated method named …ForTesting that manipulates the private field pendingSurfaceFreeCount and calls the private method recoverRenderPipeline with no production caller — this is the pattern cmux-no-test-debug-seam-in-production-source explicitly rejects. The #if DEBUG guard does not make the accessor acceptable in shipping Sources/.
The canonical fix already demonstrated by isRenderDispatchSuppressed in this same file: widen pendingSurfaceFreeCount and recoverRenderPipeline from private to internal, remove this wrapper from production source, and have the test set the count and call recoverRenderPipeline directly via its existing @testable import CmuxMobileTerminal.
Rule Used: Flag Swift files under a production Sources path (... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 782-791: Remove the test-only seam from GhosttySurfaceView by
eliminating simulateRenderRecoveryWithStuckPriorFreeForTesting() from Sources
and exposing the underlying recovery state/path through existing production
symbols instead. Keep recoverRenderPipeline and pendingSurfaceFreeCount testable
via `@testable` import from the test target, and update the tests to exercise the
same behavior without a public method named for testing.
🪄 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: 96032ab0-6fea-4b18-b974-3d840118bce7
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftios/cmuxPackage/Tests/cmuxFeatureTests/TerminalViewportSpacingTests.swift
Collaborator
Author
|
Verification update after initial PR body:
Remaining disclosure: |
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/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 2396-2400: The recovery snapshot logic in GhosttySurfaceView
should not use a time-based throttle for correctness-critical text capture.
Update the recovery path around recoverySnapshotText and
workQueue.lastRecoverySnapshotTime so it always uses the authoritative viewport
snapshot from the serialized surface transition/output flow, or fails closed
when no current snapshot is available. Remove the fixed 0.5s staleness window
and keep the “last visible terminal text” state sourced directly from the
current surface snapshot path.
- Around line 3829-3837: The early return in
updateSnapshotFallback(text:html:clearWhenEmpty:) treats an empty text snapshot
as empty overall, which skips a valid HTML fallback. Update the logic so text
and html are evaluated independently: only clear/hide snapshotFallbackView when
both inputs are empty (or when clearWhenEmpty explicitly applies), and allow
lastSnapshotFallbackHTML/html to render even if snapshot is empty. Keep the fix
localized to updateSnapshotFallback(text:html:clearWhenEmpty:) and its use of
snapshotFallbackView and lastSnapshotFallbackHTML.
🪄 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: ad9fe435-0c05-4467-9e43-b8ab72369770
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceWorkQueue.swiftios/cmuxPackage/Tests/cmuxFeatureTests/TerminalViewportSpacingTests.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/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 183-211: The chat overlay logic in
WorkspaceDetailView.detailSurfaceContent only wraps detailContent() when
activeBrowser is nil, so chatContent never appears in browser mode. Update the
activeBrowser branch to use the same terminalContentWithChatOverlay-style ZStack
behavior around browserContent(browser), or otherwise prevent isChatMode from
being shown while a browser is active. Keep the fix localized to
detailSurfaceContent and terminalContentWithChatOverlay so the overlay mounts
consistently regardless of whether browserContent or detailContent is rendered.
🪄 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: 4c4f345c-1d90-409f-82a0-8782366b312d
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+AgentChat.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.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/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swift`:
- Around line 108-121: The visibility lookup in focusTerminalInput duplicates
the same filter/sort/compactMap/first-where predicate used by
copyableTerminalText, so the visibility contract is defined in two places.
Extract a shared helper such as resolveVisibleSurfaceView(hostSurfaceID:) in
GhosttySurfaceRegistry and have both focusTerminalInput and copyableTerminalText
use it. Keep the existing hidden/window/alpha checks and ordering behavior
identical in the shared helper.
🪄 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: 54939a54-9da2-4568-9e9a-fd6a8de2a07c
📒 Files selected for processing (5)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+AgentChat.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceRegistry.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftios/cmuxPackage/Tests/cmuxFeatureTests/TerminalViewportSpacingTests.swift
Comment on lines
+108
to
+121
| public static func focusTerminalInput(surfaceID: String) -> Bool { | ||
| registeredSurfaceViews = registeredSurfaceViews.filter { $0.value.value != nil } | ||
| guard let matchingView = registeredSurfaceViews | ||
| .sorted(by: { $0.key < $1.key }) | ||
| .compactMap(\.value.value) | ||
| .first(where: { candidate in | ||
| candidate.hostSurfaceID == surfaceID && candidate.surface != nil | ||
| && candidate.window != nil && !candidate.isHidden | ||
| && candidate.alpha > 0.01 | ||
| }) | ||
| else { | ||
| return false | ||
| } | ||
| matchingView.focusInput() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Duplicate lookup logic vs. copyableTerminalText.
The filter/sort/compactMap/first-where visibility predicate here is nearly identical to the one in copyableTerminalText above. Consider extracting a shared resolveVisibleSurfaceView(hostSurfaceID:) helper to keep the visibility contract (hidden/window/alpha thresholds) defined once.
♻️ Proposed refactor
+ private static func resolveVisibleSurfaceView(hostSurfaceID surfaceID: String) -> GhosttySurfaceView? {
+ registeredSurfaceViews = registeredSurfaceViews.filter { $0.value.value != nil }
+ return registeredSurfaceViews
+ .sorted { $0.key < $1.key }
+ .compactMap(\.value.value)
+ .first { candidate in
+ candidate.hostSurfaceID == surfaceID && candidate.surface != nil
+ && candidate.window != nil && !candidate.isHidden
+ && candidate.alpha > 0.01
+ }
+ }
+
`@MainActor`
public static func focusTerminalInput(surfaceID: String) -> Bool {
- registeredSurfaceViews = registeredSurfaceViews.filter { $0.value.value != nil }
- guard let matchingView = registeredSurfaceViews
- .sorted(by: { $0.key < $1.key })
- .compactMap(\.value.value)
- .first(where: { candidate in
- candidate.hostSurfaceID == surfaceID && candidate.surface != nil
- && candidate.window != nil && !candidate.isHidden
- && candidate.alpha > 0.01
- })
- else {
+ guard let matchingView = resolveVisibleSurfaceView(hostSurfaceID: surfaceID) else {
return false
}
matchingView.focusInput()
return true
}🤖 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/GhosttySurfaceRegistry.swift`
around lines 108 - 121, The visibility lookup in focusTerminalInput duplicates
the same filter/sort/compactMap/first-where predicate used by
copyableTerminalText, so the visibility contract is defined in two places.
Extract a shared helper such as resolveVisibleSurfaceView(hostSurfaceID:) in
GhosttySurfaceRegistry and have both focusTerminalInput and copyableTerminalText
use it. Keep the existing hidden/window/alpha checks and ordering behavior
identical in the shared helper.
…der-freeze # Conflicts: # .github/swift-file-length-budget.tsv # Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift
Collaborator
Author
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort 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 2877a79. Configure here.
| refreshTerminalEventSubscription( | ||
| reason: reason, | ||
| restartOnFailure: restartOnSubscriptionFailure | ||
| ) |
There was a problem hiding this comment.
Foreground resume replays surfaces redundantly on verified streams
Low Severity
resyncTerminalOutput with restartOnSubscriptionFailure: true always replays all mounted surfaces immediately (line 6843-6844), and then refreshTerminalEventSubscription replays them again (line 6424-6425) when alreadySubscribed == nil (legacy hosts omitting the field). This double replay fires on every foreground resume for affected host versions, sending two full terminal buffer RPCs per mounted surface even when the ping/pong proves the stream is healthy.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2877a79. Configure here.
azooz2003-bit
enabled auto-merge (squash)
July 9, 2026 18:10
auto-merge was automatically disabled
September 23, 2026 08:35
Pull request was closed
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
xcodebuild test -workspace ios/cmux.xcworkspace -scheme cmux-ios -destination 'platform=iOS Simulator,id=2B86B925-1A19-4C96-B04B-34DFD37DB168' -only-testing:cmuxFeatureTests/TerminalViewportSpacingTests -derivedDataPath /tmp/cmux-tfzreg-suite -resultBundlePath /tmp/cmux-tfzreg-suite.xcresultfailed withrender.recover.pausedand reset count0.xcodebuild test -workspace ios/cmux.xcworkspace -scheme cmux-ios -destination 'platform=iOS Simulator,id=2B86B925-1A19-4C96-B04B-34DFD37DB168' -only-testing:cmuxFeatureTests/TerminalViewportSpacingTests -derivedDataPath /tmp/cmux-tfzfix-suite -resultBundlePath /tmp/cmux-tfzfix-suite.xcresultpassed.git diff --check origin/main...HEADpassed../scripts/reload.sh --tag tfrzpassed after cloud reload was blocked by missing hq helperscripts/lib/maclease-heartbeat.sh../ios/scripts/reload.sh --tag tfrz --simulator cmux-tfzreg-codex --no-setuppassed on simulatorcmux-tfzreg-codex, UDID2B86B925-1A19-4C96-B04B-34DFD37DB168./tmp/tfrz-evidence, including simulator screenshots and/tmp/tfrz-evidence/ios-relaunch-frames/contact-sheet.png.Verification gaps
Still loadingorReconnectingafter auto sign-in and pairing attempts. Route warming for/handler/sign-inand/handler/after-sign-inreturned 500 because this machine has dogfood email/password credentials but not the Stack server/public keys needed for the local Next.js auth routes.xctraceTime Profiler attach and all-processes runs hung past their time limits and produced non-exportable partial traces, so profiler evidence is not available from this environment.Summary by cubic
Keeps iOS terminal recovery live and preserves the last visible frame across reconnects until replay. Foreground resume reasserts the push subscription, proves delivery with
mobile.events.ping/mobile.events.pong, refreshes reconnect routes from host status, and restarts only when proof or reassertion fails (or the host lacksterminal.event_ping.v1); runs only when the first scene becomes active.Bug Fixes
setSceneForegroundActive(_:, sceneID:); resume only on first active scene; cancels pong waiters and probe timeouts on background; subscribes to and consumesmobile.events.pong; sendsmobile.events.pingonly when supported; restarts on a non‑answering reassert or if the pong isn’t consumed; legacy hosts (noalready_subscribedor noterminal.event_ping.v1) use the restart path. Adds tests for pong‑drop restarts, legacy acks (replay without restart), healthy‑stream foreground resumes, and all‑scenes‑inactive gating.ComposerDictationControllerfor CI stability.New Features
mobile.events.pong, implementsmobile.events.pingon the host (MobileHostService), and verifies delivery on foreground without tearing down a healthy stream.routesinmobile.host.statusand updates persisted attach routes to handle dev ephemeral ports. Adds decoding tests.Written for commit 2877a79. Summary will update on new commits.
Summary by CodeRabbit
scenePhase, preventing unnecessary watchdog/silence failures while backgrounded.setAppForegroundActive(_:)and afocusTerminalInput(surfaceID:)API for restoring keyboard focus.Note
Medium Risk
Changes span iOS terminal rendering, mobile RPC event plumbing, and foreground sync behavior—high user impact but scoped to mobile attach paths with substantial new regression tests.
Overview
Improves iOS remote terminal reliability across render recovery, Mac push events, and foreground resume—without treating every app switch like a full reconnect.
Render recovery no longer pauses the active
GhosttySurfaceViewwhen one old surface free is still draining. Recovery stays live with backlog warnings at one pending free and hard stop at two; while paused, output/geometry is rejected instead of queued onto a wedged surface. Recovery captures and shows the last visible terminal text (per-surface cache + off-main reads) until Mac replay lands, including on remount viaapplyCachedSnapshotFallback.Foreground / push stream: Multi-scene
setSceneForegroundActivegates liveness so background silence is not misread as a dead stream. Resume reasserts subscriptions (restartEventStream: false) and, when the host advertisesterminal.event_ping.v1, proves delivery withmobile.events.ping/mobile.events.pongbefore trusting the stream; failed proof or stuck reassert triggers listener restart + replay. Older Macs without ping use a legacy restart path when subscribe ack cannot be proven.Mac host: Adds
mobile.events.ping,terminal.event_ping.v1, androutesonmobile.host.statusso phones can refresh persisted reconnect routes after handshake. Chat/browser/terminal chrome closes the browser when entering chat or opening browser and suppresses terminal autofocus on chrome-driven remounts.Reviewed by Cursor Bugbot for commit 2877a79. Bugbot is set up for automated code reviews on this repo. Configure here.