Fix per-keystroke render-grid replay loop on iOS, instrument sync latency - #9146
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdds opt-in DEBUG latency tracing across macOS and iOS terminal paths, propagates render-grid sequence metadata through delivery queues, refines replay and input synchronization behavior, and introduces a Python CLI for analyzing correlated trace logs. ChangesLatency tracing and terminal delivery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalController
participant MobileTerminalByteTee
participant MobileTerminalRenderObserver
participant MobileHostService
participant MobileShellComposite
participant GhosttySurfaceView
TerminalController->>MobileTerminalByteTee: receive and apply terminal input
MobileTerminalByteTee->>MobileTerminalRenderObserver: publish terminal sequence
MobileTerminalRenderObserver->>MobileHostService: emit render-grid event with stateSeq
MobileHostService-->>MobileShellComposite: deliver terminal event
MobileShellComposite->>GhosttySurfaceView: apply output and mark sequence
GhosttySurfaceView-->>MobileShellComposite: stamp rendered presentation
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (21 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
![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
`@Packages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileLatencyTrace.swift`:
- Around line 50-58: Update the stamp(at:) overload and stampElapsed to check
isEnabled before writing trace output, matching the existing opt-in behavior of
the diagnostics API. Ensure write is called only when tracing is enabled while
preserving the current timestamp and field handling.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+LatencyProbe.swift:
- Around line 15-34: Remove the Task.sleep-based delays from the latency probe
auto-navigation and initial probe flows. Update the lifecycle coordination
around latencyProbeAutoNavigationTask and the corresponding initial probe logic
(lines 43–69) to trigger only after explicit navigation completion and
terminal-sink/output readiness, ensuring a claimed probe configuration is sent
exactly when those readiness conditions are satisfied.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalOutputDelivery.swift:
- Around line 33-41: Update recordTerminalRenderGridHistoryContinuity to remove
the surface’s entry whenever the frame is non-screen or lacks historyRows,
rather than returning while preserving stale history; continue storing
historyRows for valid screen frames. Also clear the corresponding continuity
entry in registerTerminalOutput when resetting the delivered-sequence baseline,
so subsequent deltas fail closed and request replay.
In `@Sources/TerminalController.swift`:
- Around line 14875-14877: Update the `HostLatencyTrace.stamp("host.in.applied",
...)` call in the input handling flow so it is emitted only after the actual
terminal write, not while queued input is merely accepted. If retaining the
current location, rename the stage to an acceptance label and update the
corresponding analyzer to use that label instead.
🪄 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 Plus
Run ID: 35d8a185-98b3-4620-817f-82ff1dcb125b
📒 Files selected for processing (28)
Packages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileLatencyTrace.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileLatencyProbe.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+LatencyProbe.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+StateSync.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalLane.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridEventFrameFixtures.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridFrameTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridInputCatchUpTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridReplayStalenessTests.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileTerminalOutputSinking.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LatencyTrace.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/App/HostLatencyTrace.swiftSources/Mobile/MobileHostConnectionEventQueue.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileStateSync.swiftSources/Mobile/MobileTerminalByteTee.swiftSources/Mobile/MobileTerminalRenderObserver.swiftSources/Mobile/MobileWorkspaceListObserver.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojscripts/mobile-latency-trace/README.mdscripts/mobile-latency-trace/analyze.py
Comment on lines
+15
to
+34
| latencyProbeAutoNavigationTask = Task { @MainActor [weak self] in | ||
| defer { self?.latencyProbeAutoNavigationTask = nil } | ||
| do { | ||
| try await Task.sleep(for: .seconds(1)) | ||
| guard let self, | ||
| self.connectionState == .connected, | ||
| self.terminalOutputStreamTokensBySurfaceID.isEmpty, | ||
| self.deeplinkWorkspaceNavigationRequest == nil, | ||
| let workspaceID = self.workspaces.first(where: { | ||
| !$0.terminals.isEmpty | ||
| })?.id, | ||
| MobileLatencyProbe.claimAutoNavigation() else { | ||
| return | ||
| } | ||
| self.navigateToWorkspaceForDeeplink(workspaceID, origin: .external) | ||
| } catch { | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Replace readiness sleeps with lifecycle completion.
The 1-second navigation delay and initial 3-second probe delay guess when navigation/output is ready. On a slow transition the claimed probe configuration can be consumed without sending; on a fast path this pads the measurement. Trigger from explicit navigation and terminal-sink readiness instead.
As per coding guidelines, “do not introduce … Task.sleep … for … readiness waits, or delayed coordination.”
Also applies to: 43-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+LatencyProbe.swift
around lines 15 - 34, Remove the Task.sleep-based delays from the latency probe
auto-navigation and initial probe flows. Update the lifecycle coordination
around latencyProbeAutoNavigationTask and the corresponding initial probe logic
(lines 43–69) to trigger only after explicit navigation completion and
terminal-sink/output readiness, ensuring a claimed probe configuration is sent
exactly when those readiness conditions are satisfied.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridReplayStalenessTests.swift (1)
14-75: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuarantee collector cleanup on failure paths.
The new test mounts a long-lived collector task but only calls
unmount()at the function’s final line. Any later throwing await or#requirecan bypass that cleanup and leave the stream task active for subsequent tests. Installdefer { collector.unmount() }immediately after mounting and remove the trailing manual cleanup.Suggested fix
collector.mount(store: store, surfaceID: "live-terminal") + defer { collector.unmount() } // test body - collector.unmount()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridReplayStalenessTests.swift` around lines 14 - 75, In screenAnchoredReplayBaselinesNextLiveDelta, install defer { collector.unmount() } immediately after collector.mount(store:surfaceID:) so cleanup runs on every exit path, including thrown awaits and `#require` failures. Remove the trailing explicit collector.unmount() call.Source: Coding guidelines
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
5429-5432: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd surface/connection identifiers to latency trace stamps.
The analyzer groups by stage, not by ownership context, so stamps without surface or connection scope can join records from different terminals or connections across active sessions.
- iOS input path: include
surfaceIDinin.send,in.settled,in.resp, andev.grid.MobileHostService.swift: include connection identity inhost.enqandhost.writealongside the sequence/depth data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 5429 - 5432, Update the latency trace stamps in MobileShellComposite.swift at lines 5429-5432, 5440, 7757-7759, and 8391-8395 to include surfaceID for in.send, in.settled, in.resp, and ev.grid. Update MobileHostService.swift at lines 540-546 and 2610-2617 so host.enq and host.write include the connection identity alongside sequence/depth data.Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift (1)
1203-1217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCalculate the actual cursor bottom edge.
ghostty_surface_ime_pointreturns x/y/width/height as the cursor rectangle, so rejectingy == 0hides valid first-row cursors and returning onlyypins to the rectangle origin instead of the bottom. Drop the row-origin check and return the measured bottom after checking the height is positive.Suggested fix
- guard y > 0 else { return nil } - return CGFloat(y) + guard height > 0 else { return nil } + return CGFloat(y + height)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift` around lines 1203 - 1217, Update cursorBottomInRenderPoints() to accept valid cursors with y == 0, reject only rectangles whose measured height is not positive, and return the cursor rectangle’s bottom edge by adding height to y rather than returning y alone.
🤖 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.
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 5429-5432: Update the latency trace stamps in
MobileShellComposite.swift at lines 5429-5432, 5440, 7757-7759, and 8391-8395 to
include surfaceID for in.send, in.settled, in.resp, and ev.grid. Update
MobileHostService.swift at lines 540-546 and 2610-2617 so host.enq and
host.write include the connection identity alongside sequence/depth data.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridReplayStalenessTests.swift`:
- Around line 14-75: In screenAnchoredReplayBaselinesNextLiveDelta, install
defer { collector.unmount() } immediately after
collector.mount(store:surfaceID:) so cleanup runs on every exit path, including
thrown awaits and `#require` failures. Remove the trailing explicit
collector.unmount() call.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 1203-1217: Update cursorBottomInRenderPoints() to accept valid
cursors with y == 0, reject only rectangles whose measured height is not
positive, and return the cursor rectangle’s bottom edge by adding height to y
rather than returning y alone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 82b9d967-a296-4571-b6c5-1cb364557879
📒 Files selected for processing (7)
Packages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileLatencyTrace.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridReplayStalenessTests.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/Mobile/MobileHostService.swiftSources/TerminalController.swift
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
5426-5445: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the surface token to input send/settlement stamps.
in.send/in.settledonly carryn, whilein.respcarriess. Pipelined requests can overlap across surfaces, so the analyzer’s global timestamp pairing can associate a response with the wrong batch. Emits=on both stamps and partitionpair_input_batchesby that token.Also applies to: 6822-6830
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 5426 - 5445, Update the DEBUG input tracing around sendRemoteTerminalInput and the corresponding settlement block to include the same surface token used by in.resp as s= on both in.send and in.settled stamps. Then update pair_input_batches to partition or match batches by that surface token before pairing timestamps, preserving correct pairing for overlapping requests across surfaces; apply the same change to the additional send/settlement block around the other referenced occurrence.scripts/mobile-latency-trace/analyze.py (3)
437-442: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilent pass-through when zero joins are possible for cross-clock validation.
If
joined_count == 0(e.g. surfaces never line up between the two logs),cross_clock_enabledstaysTruewith no warning even though the Mac/iOS clock-alignment check never actually validated anything — the resulting same-clock metrics would then be reported without any confirmed alignment.♻️ Warn explicitly when no joins could be validated
- if joined_count and violations / joined_count > 0.10: + if joined_count == 0: + cross_clock_enabled = False + warning = "WARNING: no host/iOS joins available to validate clock alignment; dropping cross-clock metrics." + elif violations / joined_count > 0.10: cross_clock_enabled = False warning = ( "WARNING: host.in.recv fell outside in.send→in.settled for " f"{violations}/{joined_count} joins; dropping cross-clock metrics." )🤖 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 `@scripts/mobile-latency-trace/analyze.py` around lines 437 - 442, Update the cross-clock validation conditional near cross_clock_enabled so joined_count == 0 disables cross-clock metrics and sets a warning explaining that no joins were available to validate clock alignment. Preserve the existing violation-rate warning and threshold behavior for cases with one or more joins.
296-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated grid→write→enqueued resolution vs the inline echo-hop block.
mac_frame_chain()and the inline block at lines 488-514 both implement the same four-first_after-call chain (host.grid → host.write → conn → host.enq bounded by write time), differing only in whether the initial grid lookup uses>= sequence(here, tolerating coalescing from an "applied" stamp) or== echo_sequence(there, tracing an already-resolved specific frame). The duplicated shape risks drifting further apart on the next edit. Consider parameterizingmac_frame_chain(e.g. anexact: boolor an explicit sequence-matcher) so both call sites share one implementation.🤖 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 `@scripts/mobile-latency-trace/analyze.py` around lines 296 - 333, Refactor mac_frame_chain to accept a configurable sequence-matching mode or predicate for its initial host.grid lookup, while retaining the existing coalescing >= sequence behavior for applied stamps. Replace the inline echo-hop host.grid → host.write → host.enq resolution with this shared helper using exact echo_sequence matching, preserving the connection and write-time bounds.
467-470: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile
id()-based dict key.
input_identitykeys a dict byid(pair)purely to recover the index ofbindinginsideprobe_bindings. This only stays correct becauseinput_pairsremains alive and unmodified for the rest ofanalyze(); it's an unusual pattern that would silently misbehave ifinput_pairswere ever filtered/rebuilt between construction and use (CPython can and does reuseid()values for garbage-collected objects). Storing the index directly avoids the indirection entirely.♻️ Store the index instead of reverse-looking it up via id()
- probe_bindings: list[tuple[Stamp, tuple[Stamp, Stamp, Stamp | None]]] = [] + probe_bindings: list[tuple[Stamp, tuple[Stamp, Stamp, Stamp | None], int]] = [] input_index = 0 for probe in ios.get("probe.send", []): while input_index < len(input_pairs) and input_pairs[input_index][0].time_us < probe.time_us: input_index += 1 if input_index >= len(input_pairs): break binding = input_pairs[input_index] + bound_index = input_index input_index += 1 - probe_bindings.append((probe, binding)) + probe_bindings.append((probe, binding, bound_index))And later:
- input_identity = {id(pair): index for index, pair in enumerate(input_pairs)} - for probe, binding in probe_bindings: - index = input_identity.get(id(binding)) + for probe, binding, index in probe_bindings: host_pair = host_by_input_index.get(index) if index is not None else None🤖 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 `@scripts/mobile-latency-trace/analyze.py` around lines 467 - 470, Replace the id()-based input_identity reverse lookup in analyze() with bindings that carry each input pair’s index directly through probe_bindings. Update the loop over probe_bindings to read that stored index and preserve the existing host_by_input_index lookup behavior.
🤖 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 `@scripts/mobile-latency-trace/analyze.py`:
- Around line 351-365: Bind the current loop values as lambda default arguments
in the ev.grid loop’s first_after predicate, using surface and sequence, and
apply the same change to every flagged lambda in the probe_bindings loop around
probe_bindings, including surface, echo_sequence, and connection. Preserve the
existing predicate logic while ensuring each closure retains that iteration’s
values.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 5426-5445: Update the DEBUG input tracing around
sendRemoteTerminalInput and the corresponding settlement block to include the
same surface token used by in.resp as s= on both in.send and in.settled stamps.
Then update pair_input_batches to partition or match batches by that surface
token before pairing timestamps, preserving correct pairing for overlapping
requests across surfaces; apply the same change to the additional
send/settlement block around the other referenced occurrence.
In `@scripts/mobile-latency-trace/analyze.py`:
- Around line 437-442: Update the cross-clock validation conditional near
cross_clock_enabled so joined_count == 0 disables cross-clock metrics and sets a
warning explaining that no joins were available to validate clock alignment.
Preserve the existing violation-rate warning and threshold behavior for cases
with one or more joins.
- Around line 296-333: Refactor mac_frame_chain to accept a configurable
sequence-matching mode or predicate for its initial host.grid lookup, while
retaining the existing coalescing >= sequence behavior for applied stamps.
Replace the inline echo-hop host.grid → host.write → host.enq resolution with
this shared helper using exact echo_sequence matching, preserving the connection
and write-time bounds.
- Around line 467-470: Replace the id()-based input_identity reverse lookup in
analyze() with bindings that carry each input pair’s index directly through
probe_bindings. Update the loop over probe_bindings to read that stored index
and preserve the existing host_by_input_index lookup behavior.
🪄 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 Plus
Run ID: 0bb175f9-6705-477d-aac4-59665069d207
📒 Files selected for processing (10)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileTerminalByteTee.swiftSources/Mobile/MobileTerminalRenderObserver.swiftSources/TerminalController.swiftscripts/mobile-latency-trace/README.mdscripts/mobile-latency-trace/analyze.py
Comment on lines
+351
to
+365
| surface = event.fields.get("s") | ||
| if sequence is None or surface is None: | ||
| continue | ||
| gate = first_after( | ||
| ios.get("gate", []), | ||
| event.time_us, | ||
| lambda stamp: stamp.fields.get("s") == surface | ||
| and stamp.integer("seq") == sequence, | ||
| ) | ||
| if gate is None: | ||
| continue | ||
| add_duration(metrics, "iOS: ev.grid → gate", event, gate) | ||
| if gate.fields.get("out") != "delivered": | ||
| continue | ||
| chain = frame_chain(surface, sequence, gate.time_us, ios) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Loop-variable closures flagged by Ruff (B023) — safe today, fragile for later edits.
The lambdas passed to first_after() inside the ev.grid loop (e.g. surface, sequence at lines 357-358) and the probe_bindings loop (surface, echo_sequence, connection at 491-519) close over loop-scoped variables. This works correctly today only because each lambda is consumed synchronously by first_after() before the loop variable is reassigned — but it's the classic Python late-binding trap: if a future refactor stores one of these lambdas (e.g. for batching or deferred evaluation), all deferred lookups will silently use the final iteration's values.
♻️ Bind current values as default args to silence B023 and future-proof the closures
- gate = first_after(
- ios.get("gate", []),
- event.time_us,
- lambda stamp: stamp.fields.get("s") == surface
- and stamp.integer("seq") == sequence,
- )
+ gate = first_after(
+ ios.get("gate", []),
+ event.time_us,
+ lambda stamp, surface=surface, sequence=sequence: stamp.fields.get("s") == surface
+ and stamp.integer("seq") == sequence,
+ )Apply the same pattern to the other flagged lambdas at 491-519.
Also applies to: 478-519
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 357-357: Function definition does not bind loop variable surface
(B023)
[warning] 358-358: Function definition does not bind loop variable sequence
(B023)
🤖 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 `@scripts/mobile-latency-trace/analyze.py` around lines 351 - 365, Bind the
current loop values as lambda default arguments in the ev.grid loop’s
first_after predicate, using surface and sequence, and apply the same change to
every flagged lambda in the probe_bindings loop around probe_bindings, including
surface, echo_sequence, and connection. Preserve the existing predicate logic
while ensuring each closure retains that iteration’s values.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
7819-7824: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCorrelate input responses with the batch ID. A successful terminal-lane send emits
in.settled ok=1but noin.resp; the analyzer then assigns a later RPC response to that earlier batch and shifts subsequent probe measurements.
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift#L7819-L7824: threadlatencyBatchNumberinto the response handler and emitn=<batch>onin.resp.scripts/mobile-latency-trace/analyze.py#L188-L196: match responses byn, rather than consuming the next response by timestamp; add a self-test with a successful no-response lane settlement before an RPC response.scripts/mobile-latency-trace/README.md#L65-L70: document the required keyed correlation schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 7819 - 7824, Correlate latency responses by batch ID across all affected sites: in Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift lines 7819-7824, thread latencyBatchNumber into the response handler and include n=<batch> in the in.resp trace; in scripts/mobile-latency-trace/analyze.py lines 188-196, match responses using n instead of consuming the next timestamp-ordered response and add a self-test covering a successful no-response settlement before an RPC response; in scripts/mobile-latency-trace/README.md lines 65-70, document the required keyed correlation schema.Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swift (1)
590-631: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCancel the input-ACK resubscribe retry from the non-barrier reset path too.
terminalOutputDidReset(surfaceID:streamToken:)schedules a per-surface delay task inhandleTerminalInputResponse, but the non-barrier branch only clears replay/delta state and requests a fresh replay. AddcancelTerminalInputAckResubscribeRetry(surfaceID: surfaceID)in that branch so a pending retry cannot run after the surface has been reset and a newrequestTerminalReplayhas been issued.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalOutputDelivery.swift around lines 590 - 631, Update the non-barrier reset branch in terminalOutputDidReset(surfaceID:streamToken:) to call cancelTerminalInputAckResubscribeRetry(surfaceID: surfaceID) before issuing the fresh requestTerminalReplay. Leave the existing barrier-reset handling unchanged and ensure any pending per-surface input-ACK retry is canceled before replay restarts.
🤖 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/MobileShellComposite.swift`:
- Around line 7905-7906: Remove the clock-based delayed retry flow around
terminalInputAckResubscribeRetryTask, including the stored task and its Task
closure that calls clock.sleep. Trigger subscription refresh from the
authoritative liveness/state transition already used by MobileShellComposite
instead of scheduling recovery after an arbitrary delay.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 7819-7824: Correlate latency responses by batch ID across all
affected sites: in
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift
lines 7819-7824, thread latencyBatchNumber into the response handler and include
n=<batch> in the in.resp trace; in scripts/mobile-latency-trace/analyze.py lines
188-196, match responses using n instead of consuming the next timestamp-ordered
response and add a self-test covering a successful no-response settlement before
an RPC response; in scripts/mobile-latency-trace/README.md lines 65-70, document
the required keyed correlation schema.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalOutputDelivery.swift:
- Around line 590-631: Update the non-barrier reset branch in
terminalOutputDidReset(surfaceID:streamToken:) to call
cancelTerminalInputAckResubscribeRetry(surfaceID: surfaceID) before issuing the
fresh requestTerminalReplay. Leave the existing barrier-reset handling unchanged
and ensure any pending per-surface input-ACK retry is canceled before replay
restarts.
🪄 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 Plus
Run ID: 4740c5b8-ede3-45dc-9492-acbe948900c9
📒 Files selected for processing (15)
Packages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileDebugLogSink.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileLatencyTrace.swiftPackages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileLatencyTraceWriter.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplayLifecycle.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplayRetry.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/InputAckRetryClock.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridInputCatchUpTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalOutputDeliveryQueueTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftscripts/mobile-latency-trace/README.mdscripts/mobile-latency-trace/analyze.py
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/MobileShellComposite`+TerminalOutputDelivery.swift:
- Around line 203-209: Update the DEBUG MobileLatencyTrace stamp in the advisory
branch of the terminal output delivery flow to emit out=advisory instead of
out=delivered, while preserving out=replay_req when requestReplay is true. Keep
out=delivered reserved for the path that actually calls
deliverTerminalRenderGrid.
🪄 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 Plus
Run ID: 2836ea41-ac97-4df5-adcd-189261c4920c
📒 Files selected for processing (12)
Packages/iOS/CmuxMobileDiagnostics/Sources/CmuxMobileDiagnostics/MobileLatencyTraceWriter.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridInputCatchUpTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/App/HostLatencyTrace.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileTerminalByteTee.swiftSources/Mobile/MobileTerminalRenderObserver.swiftSources/TerminalController.swiftscripts/mobile-latency-trace/analyze.py
Comment on lines
+203
to
+209
| #if DEBUG | ||
| MobileLatencyTrace.stamp( | ||
| "gate", | ||
| "s=\(renderGrid.surfaceID.prefix(8).lowercased()) " + | ||
| "seq=\(renderGrid.stateSeq) " + | ||
| "out=\(deliveryDecision.requestReplay ? "replay_req" : "delivered")" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not label advisory decisions as delivered.
This branch updates screen/policy state and may request replay, but it returns without calling deliverTerminalRenderGrid. Emitting out=delivered can make scripts/mobile-latency-trace/analyze.py treat the advisory stamp as an applied frame and pair it with a later same-sequence presentation. Use a distinct value such as out=advisory; reserve out=delivered for the actual delivery path.
Based on the supplied trace analyzer, gate.out=delivered is treated as a real delivery marker.
Suggested trace fix
- "out=\(deliveryDecision.requestReplay ? "replay_req" : "delivered")"
+ "out=\(deliveryDecision.requestReplay ? "replay_req" : "advisory")"📝 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.
Suggested change
| #if DEBUG | |
| MobileLatencyTrace.stamp( | |
| "gate", | |
| "s=\(renderGrid.surfaceID.prefix(8).lowercased()) " + | |
| "seq=\(renderGrid.stateSeq) " + | |
| "out=\(deliveryDecision.requestReplay ? "replay_req" : "delivered")" | |
| ) | |
| `#if` DEBUG | |
| MobileLatencyTrace.stamp( | |
| "gate", | |
| "s=\(renderGrid.surfaceID.prefix(8).lowercased()) " + | |
| "seq=\(renderGrid.stateSeq) " + | |
| "out=\(deliveryDecision.requestReplay ? "replay_req" : "advisory")" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalOutputDelivery.swift
around lines 203 - 209, Update the DEBUG MobileLatencyTrace stamp in the
advisory branch of the terminal output delivery flow to emit out=advisory
instead of out=delivered, while preserving out=replay_req when requestReplay is
true. Keep out=delivered reserved for the path that actually calls
deliverTerminalRenderGrid.
# Conflicts: # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift
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
1 participant
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.
Measurement-first pass at iOS↔Mac sync latency. The instrumentation found a structural bug: on screen-anchored render-grid connections (every modern host), every typed keystroke's echo rendered through a full
mobile.terminal.replayRPC plus the verified-replay freeze→fence→reveal pipeline instead of the live delta path.Mechanism of the bug: the live delivery tail records
terminalRenderGridHistoryContinuityBySurfaceIDfor screen-anchored frames, but the replay-response apply path never did. Cold attach always baselines via replay, so continuity startednil, every live delta failed the history chain check (base=0 delivered=nil), requested another replay, which again recorded no continuity — a permanent loop. The Mac side already rebases its emission chain onto served replays (MobileTerminalRenderObserver.adoptReplayBaseline) expecting exactly this contract. Introduced by #8860. Trace evidence: 40/40 probe keystrokes hitsync.render_grid_history_chain_break, zero live frames delivered.Fixes (commit-by-commit, regression test red→green):
fff620d0c3failing tests: replay-applied baseline must let the next linked live delta deliver without a new replay; a fresh input ack must not re-subscribe while events are flowing.359b8d7f81fix: sharedrecordTerminalRenderGridHistoryContinuityhelper called from both the live tail and the replay apply path; input-ack resubscribe (input_seq_wait) now skipped when the last terminal event is under 2 s old (terminalInputAckResubscribeSilenceThreshold), preserving lost-registration repair after silence. The per-ackmobile.events.subscribematters on real-RTT links where acks outrun echo frames.Instrumentation (all DEBUG-only, off unless
CMUX_LATENCY_TRACE=1orcmux.debug.latency-tracedefaults):LAT <stage> t=<uptime_us>stamps across probe.send → host.in.recv → host.in.applied → host.grid → host.write → ev.grid → gate → ap.yield → ap.done → rd.present, plus state-sync emit/apply.CMUX_LATENCY_PROBE=<n>:<ms>types n synthetic keystrokes through the production input path (auto-navigating to the first workspace so headless sims work).scripts/mobile-latency-trace/analyze.pyjoins both logs (sim and Mac share a clock domain) into the tables below; seescripts/mobile-latency-trace/README.md.Measured (iPhone 17 Pro Max sim ↔ tagged Mac over QUIC, same host, 40 keystrokes at 250 ms):
After-fix echo decomposition (p50): input wire+Mac apply 1.5 ms, echo capture→frame encode 2.6 ms, write+wire+decode+gate+apply 1.5 ms, display-link present 7.9 ms (120 Hz floor). On a physical iPhone the fix additionally removes one full wire RTT (the replay round trip) and ~31 ms of verified-replay freeze/reveal per keystroke.
Remaining known cost, intentionally not touched here: the stop-and-wait input batching RTT (probe.send→host.in.recv p95 29.7 ms when batches queue) is owned by #9036, which this PR avoids conflicting with (no
CmuxMobileRPCedits).Verification: CmuxMobileShell 813 tests (only the 3 known pre-existing failures), CmuxMobileShellModel 179 green, analyzer selftest green, live before/after traces above on tag
slat.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes an iOS render-grid continuity bug that forced a full replay on every keystroke; live deltas now link and apply normally. Adds opt-in iOS↔Mac latency tracing with a DEBUG typing probe and analyzer to measure end-to-end echo.
Bug Fixes
New Features
CMUX_LATENCY_TRACE=1orcmux.debug.latency-trace.CMUX_LATENCY_PROBE=<n>:<ms>with auto-navigation to the first workspace. Analyzer atscripts/mobile-latency-trace/analyze.pywith README; joins iOS/Mac logs, supports same-clock echo decomposition on Simulator, and state-sync emit→apply timing.Written for commit 24712a6. Summary will update on new commits.
Summary by CodeRabbit