Smooth iOS terminal scrollback with local prefetch#6067
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds mobile terminal scroll prefetch end-to-end: ChangesMobile Terminal Scroll Prefetch
Sequence DiagramsequenceDiagram
participant GhosttySurfaceView
participant MobileShellComposite
participant TerminalScrollbackPrefetchState
participant RemoteServer
participant TerminalController
GhosttySurfaceView->>GhosttySurfaceView: flushPendingScrollIfNeeded()
GhosttySurfaceView->>GhosttySurfaceView: applyLocalScrollbackScroll(lines, col, row)
GhosttySurfaceView->>MobileShellComposite: scrollTerminal(surfaceID, lines, col, row)
MobileShellComposite->>TerminalScrollbackPrefetchState: rowsToPrefetch(forScrollLines: lines)
TerminalScrollbackPrefetchState-->>MobileShellComposite: maxScrollbackRows (Int?)
MobileShellComposite->>MobileShellComposite: enqueue TerminalScrollDelivery(maxScrollbackRows)
MobileShellComposite->>RemoteServer: mobile.terminal.scroll RPC { max_scrollback_rows }
RemoteServer->>TerminalController: v2MobileTerminalScroll(params)
TerminalController->>TerminalController: mobileTerminalScrollResponsePayload(params)
TerminalController-->>RemoteServer: { workspace_id, surface_id, render_grid, seq, ... }
RemoteServer-->>MobileShellComposite: MobileTerminalReplayResponse (renderGrid)
MobileShellComposite->>MobileShellComposite: deliverAuthoritativeTerminalRenderGrid(source: "scroll_prefetch")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (8 errors, 1 warning)
✅ Passed checks (12 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 achieves smooth iOS terminal scrollback by: (1) applying scroll deltas immediately to the local Ghostty mirror before the Mac RPC completes, and (2) piggybacking a bounded render-grid snapshot (up to 600 rows) on the first scroll and every 120 rows thereafter so the local mirror stays ahead of user deceleration. Both the prefetch RPC response and the live render-grid event now route through a shared
Confidence Score: 4/5The dual-delivery path (scroll_prefetch RPC response + live render-grid event) introduced in this PR has a known open correctness gap in the stale-frame guard that can cause the same frame to be delivered twice; that gap is under active discussion in prior review threads but is not yet fixed in this diff. The optimistic local-scroll and prefetch machinery is well-structured and the new tests cover the key state-machine behaviours. The main risk is the shared deliverAuthoritativeTerminalRenderGrid guard: because both delivery paths can arrive with the same stateSeq (the Mac emits a live event and returns the RPC response for the same scroll in rapid succession), the guard's current comparison allows both to pass through. This is an existing open thread, not newly introduced, but the new prefetch path is what makes the race reachable in practice. MobileShellComposite.swift (deliverAuthoritativeTerminalRenderGrid stale-frame guard) and MobileShellComposite+TerminalScrollDelivery.swift (performTerminalScroll dual-path delivery) Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User Gesture
participant GSV as GhosttySurfaceView (iOS)
participant Mirror as Local Ghostty Mirror
participant Shell as MobileShellComposite
participant Mac as TerminalController (Mac)
User->>GSV: scroll gesture (pan/decelerate)
GSV->>Mirror: applyLocalScrollbackScroll(lines, col, row)
Note over Mirror: Immediately moves viewport into prefetched scrollback
GSV->>Shell: scrollTerminal(surfaceID, lines, col, row)
Shell->>Shell: rowsToPrefetch() → maxScrollbackRows? advance hasPrimedWindow / accum
Shell->>Mac: RPC mobile.terminal.scroll (+ max_scrollback_rows if prefetch window due)
Mac->>Mac: mobileScroll(deltaLines)
alt prefetch requested and primary screen
Mac-->>Shell: "response {render_grid, seq}"
Shell->>Shell: "deliverAuthoritativeTerminalRenderGrid source=scroll_prefetch (stale guard)"
Shell->>Mirror: deliverTerminalRenderGrid (corrects local state)
else no prefetch / alt screen
Mac-->>Shell: "response {workspace_id, surface_id}"
end
Mac-)Shell: live render_grid event (async)
Shell->>Shell: "deliverAuthoritativeTerminalRenderGrid source=event (stale guard)"
Shell->>Mirror: deliverTerminalRenderGrid (authoritative)
Reviews (4): Last reviewed commit: "Refresh Swift file length budget" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4112
to
+4113
| if let deliveredSeq = deliveredTerminalByteEndSeqBySurfaceID[renderGrid.surfaceID], | ||
| deliveredSeq > renderGrid.stateSeq { |
Contributor
There was a problem hiding this comment.
>, so a render-grid frame with stateSeq equal to an already-delivered frame bypasses the guard and is delivered a second time. With this PR introducing a second delivery path (scroll prefetch response + live event), both paths can produce a frame at the same stateSeq when the Mac processes the scroll and emits the event before responding to the RPC. The guard should be >= to treat an equal seq as stale.
Suggested change
| if let deliveredSeq = deliveredTerminalByteEndSeqBySurfaceID[renderGrid.surfaceID], | |
| deliveredSeq > renderGrid.stateSeq { | |
| if let deliveredSeq = deliveredTerminalByteEndSeqBySurfaceID[renderGrid.surfaceID], | |
| deliveredSeq >= renderGrid.stateSeq { |
Comment on lines
4344
to
+4347
| #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 | ||
| deliverTerminalRenderGrid(renderGrid, surfaceID: renderGrid.surfaceID) | ||
| deliverAuthoritativeTerminalRenderGrid(renderGrid, source: "event") |
Contributor
There was a problem hiding this comment.
#if DEBUG log now fires before the stale-frame check inside deliverAuthoritativeTerminalRenderGrid, so frames that are subsequently rejected as stale will still emit CMUX_REPLAY live render_grid … hasSink=true. Moving the log inside deliverAuthoritativeTerminalRenderGrid (after the staleness guard) would keep the signal accurate for debugging.
Suggested change
| #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 | |
| deliverTerminalRenderGrid(renderGrid, surfaceID: renderGrid.surfaceID) | |
| deliverAuthoritativeTerminalRenderGrid(renderGrid, source: "event") | |
| deliverAuthoritativeTerminalRenderGrid(renderGrid, source: "event") |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift`:
- Around line 42-50: In the rowsToPrefetch method, when
accumulatedRowsSincePrefetch reaches or exceeds refreshDistanceRows and triggers
a prefetch, resetting accumulatedRowsSincePrefetch to 0 on Line 49 discards the
overflow distance. Instead of setting accumulatedRowsSincePrefetch to 0, set it
to the overflow amount by subtracting refreshDistanceRows from the current
accumulatedRowsSincePrefetch value. This preserves the extra traveled distance
beyond the threshold, ensuring consistent refresh timing across large scroll
deltas.
In `@Sources/TerminalController`+MobileScrollPrefetch.swift:
- Around line 59-63: The code at line 59 uses unsafe NSNumber.intValue coercion
on the max_scrollback_rows parameter, which silently accepts and coerces
non-integer JSON values and booleans. Replace the entire expression
`(params["max_scrollback_rows"] as? NSNumber)?.intValue ?? 0` with
`v2StrictInt(params, "max_scrollback_rows") ?? 0` to apply strict numeric
validation that rejects non-integers and boolean values explicitly, consistent
with the codebase pattern used for port numbers and other resource budgets in
TerminalController v2 handlers.
🪄 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: 058216f4-51bd-4fd8-bf25-374036fcd1cd
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalScrollDelivery.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollDeliveryQueueTests.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/TerminalController+MobileScrollPrefetch.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/TerminalController.swift
Comment on lines
+42
to
+50
| mutating func rowsToPrefetch(forScrollLines lines: Double) -> Int? { | ||
| guard lines != 0, windowRows > 0 else { return nil } | ||
| accumulatedRowsSincePrefetch += abs(lines) | ||
| guard !hasPrimedWindow || accumulatedRowsSincePrefetch >= refreshDistanceRows else { | ||
| return nil | ||
| } | ||
| hasPrimedWindow = true | ||
| accumulatedRowsSincePrefetch = 0 | ||
| return windowRows |
There was a problem hiding this comment.
Preserve overflow distance after each prefetch trigger.
On Line 49, resetting accumulatedRowsSincePrefetch to 0 drops extra traveled distance above refreshDistanceRows. With coalesced/large deltas, this under-fires refreshes and drifts below the intended “about every N rows” cadence.
💡 Proposed fix
mutating func rowsToPrefetch(forScrollLines lines: Double) -> Int? {
guard lines != 0, windowRows > 0 else { return nil }
accumulatedRowsSincePrefetch += abs(lines)
guard !hasPrimedWindow || accumulatedRowsSincePrefetch >= refreshDistanceRows else {
return nil
}
+ let wasPrimed = hasPrimedWindow
hasPrimedWindow = true
- accumulatedRowsSincePrefetch = 0
+ accumulatedRowsSincePrefetch = wasPrimed
+ ? accumulatedRowsSincePrefetch.truncatingRemainder(dividingBy: refreshDistanceRows)
+ : 0
return windowRows
}🤖 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/TerminalScrollDelivery.swift`
around lines 42 - 50, In the rowsToPrefetch method, when
accumulatedRowsSincePrefetch reaches or exceeds refreshDistanceRows and triggers
a prefetch, resetting accumulatedRowsSincePrefetch to 0 on Line 49 discards the
overflow distance. Instead of setting accumulatedRowsSincePrefetch to 0, set it
to the overflow amount by subtracting refreshDistanceRows from the current
accumulatedRowsSincePrefetch value. This preserves the extra traveled distance
beyond the threshold, ensuring consistent refresh timing across large scroll
deltas.
Comment on lines
+59
to
+63
| let requestedRows = (params["max_scrollback_rows"] as? NSNumber)?.intValue ?? 0 | ||
| return min( | ||
| max(0, requestedRows), | ||
| Self.mobileScrollPrefetchScrollbackLineBudget | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify existing TerminalController parsing convention and this callsite.
set -euo pipefail
echo "== v2 numeric helper definitions/usages in Sources/TerminalController* =="
rg -n --type=swift -C2 '\bv2(Int|StrictInt)\s*\(' Sources/TerminalController.swift Sources/TerminalController+*.swift
echo
echo "== current max_scrollback_rows parsing in mobile prefetch extension =="
rg -n --type=swift -C3 'max_scrollback_rows|NSNumber|intValue|mobileScrollPrefetchRows' Sources/TerminalController+MobileScrollPrefetch.swiftRepository: manaflow-ai/cmux
Length of output: 9676
🏁 Script executed:
rg -n --type=swift -B2 -A5 'func v2(Int|StrictInt)\(' Sources/TerminalController.swift | head -50Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
rg -n --type=swift 'func v2Int|func v2StrictInt' Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
rg -n --type=swift 'v2Int|v2StrictInt' Sources/TerminalController.swift | grep 'func\|private' | head -20Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
fd --type f --extension swift . Sources | xargs rg -l 'func v2Int|func v2StrictInt' | head -5Repository: manaflow-ai/cmux
Length of output: 114
🏁 Script executed:
rg -n --type=swift -A8 'func v2Int|func v2StrictInt' Sources/TerminalControllerV2ParamParsingSupport.swiftRepository: manaflow-ai/cmux
Length of output: 1088
🏁 Script executed:
cat -n Sources/TerminalControllerV2ParamParsingSupport.swift | sed -n '159,180p'Repository: manaflow-ai/cmux
Length of output: 923
🏁 Script executed:
cat -n Sources/TerminalController+MobileScrollPrefetch.swift | sed -n '58,65p'Repository: manaflow-ai/cmux
Length of output: 409
Use v2StrictInt for strict numeric validation of max_scrollback_rows instead of unsafe NSNumber.intValue coercion.
Line 59 uses NSNumber.intValue, which silently accepts and coerces non-integer JSON values and booleans. Replace with v2StrictInt(params, "max_scrollback_rows") ?? 0 to match the codebase's strict validation pattern (used for port numbers and other resource budgets). This rejects non-integers and boolean values explicitly rather than coercing them, ensuring request semantics are preserved consistently with other TerminalController v2 handlers.
🤖 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 `@Sources/TerminalController`+MobileScrollPrefetch.swift around lines 59 - 63,
The code at line 59 uses unsafe NSNumber.intValue coercion on the
max_scrollback_rows parameter, which silently accepts and coerces non-integer
JSON values and booleans. Replace the entire expression
`(params["max_scrollback_rows"] as? NSNumber)?.intValue ?? 0` with
`v2StrictInt(params, "max_scrollback_rows") ?? 0` to apply strict numeric
validation that rejects non-integers and boolean values explicitly, consistent
with the codebase pattern used for port numbers and other resource budgets in
TerminalController v2 handlers.
Source: Learnings
azooz2003-bit
force-pushed
the
task-ios-scrollback-window-cache
branch
from
6793c36 to
0fa6418
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift (1)
42-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve overflow distance after each refresh trigger.
Line 49 still zeroes
accumulatedRowsSincePrefetch, which drops overflow from large/coalesced deltas and stretches the next refresh interval beyondrefreshDistanceRows.💡 Proposed fix
mutating func rowsToPrefetch(forScrollLines lines: Double) -> Int? { guard lines != 0, windowRows > 0 else { return nil } accumulatedRowsSincePrefetch += abs(lines) guard !hasPrimedWindow || accumulatedRowsSincePrefetch >= refreshDistanceRows else { return nil } + let wasPrimed = hasPrimedWindow hasPrimedWindow = true - accumulatedRowsSincePrefetch = 0 + accumulatedRowsSincePrefetch = wasPrimed + ? accumulatedRowsSincePrefetch.truncatingRemainder(dividingBy: refreshDistanceRows) + : 0 return windowRows }🤖 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/TerminalScrollDelivery.swift` around lines 42 - 50, The rowsToPrefetch(forScrollLines lines: Double) method currently resets accumulatedRowsSincePrefetch to zero after triggering a refresh, which discards any accumulated overflow that exceeded the refreshDistanceRows threshold. Instead of setting accumulatedRowsSincePrefetch to 0, preserve the overflow by setting it to the amount that exceeded the threshold (the difference between the accumulated rows and refreshDistanceRows). This ensures that overflow from large or coalesced scroll deltas carries forward and does not artificially stretch the interval before the next refresh trigger.
🤖 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/TerminalScrollDelivery.swift`:
- Around line 34-40: The initializer in TerminalScrollDelivery is clamping
refreshDistanceRows with max(1, refreshDistanceRows) on line 39, but this does
not guard against non-finite values like NaN or infinity. When NaN is passed,
max(1, NaN) still returns NaN, which causes the >= refreshDistanceRows
comparison check to always fail, leaving the refresh mechanism silently stuck.
Add a check to ensure refreshDistanceRows is finite (not NaN and not infinite)
before assigning it to self.refreshDistanceRows, either by rejecting invalid
input with a guard condition or by providing a safe default fallback value when
non-finite input is detected.
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollDeliveryQueueTests.swift`:
- Around line 70-78: Add a regression test case to the
terminalScrollbackPrefetchStatePrimesThenRefreshesByDistance test function that
verifies the prefetch cadence behavior when a single large scroll delta crosses
the refreshDistanceRows threshold by more than one unit. After the existing
`#expect` statements, add a case that demonstrates overflow-carry behavior by
scrolling a large distance (e.g., greater than twice the refreshDistanceRows
value of 10) and verify that the remainder is properly preserved between
prefetch triggers, ensuring the cadence logic correctly handles multi-threshold
crossings in a single operation.
---
Duplicate comments:
In
`@Packages/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swift`:
- Around line 42-50: The rowsToPrefetch(forScrollLines lines: Double) method
currently resets accumulatedRowsSincePrefetch to zero after triggering a
refresh, which discards any accumulated overflow that exceeded the
refreshDistanceRows threshold. Instead of setting accumulatedRowsSincePrefetch
to 0, preserve the overflow by setting it to the amount that exceeded the
threshold (the difference between the accumulated rows and refreshDistanceRows).
This ensures that overflow from large or coalesced scroll deltas carries forward
and does not artificially stretch the interval before the next refresh trigger.
🪄 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: eb0b4967-9884-47c5-ae71-f297a7f2579c
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalScrollDelivery.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollDeliveryQueueTests.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/TerminalController+MobileScrollPrefetch.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/TerminalController.swift
Comment on lines
+34
to
+40
| init( | ||
| windowRows: Int = Self.defaultWindowRows, | ||
| refreshDistanceRows: Double = Self.defaultRefreshDistanceRows | ||
| ) { | ||
| self.windowRows = max(0, windowRows) | ||
| self.refreshDistanceRows = max(1, refreshDistanceRows) | ||
| } |
There was a problem hiding this comment.
Sanitize non-finite refreshDistanceRows in initializer.
Line 39 clamps lower bound, but NaN can still propagate and make the >= refreshDistanceRows check never pass after priming. Guarding finite input avoids a silent stuck-refresh state.
💡 Proposed fix
init(
windowRows: Int = Self.defaultWindowRows,
refreshDistanceRows: Double = Self.defaultRefreshDistanceRows
) {
self.windowRows = max(0, windowRows)
- self.refreshDistanceRows = max(1, refreshDistanceRows)
+ let sanitizedRefresh = refreshDistanceRows.isFinite ? refreshDistanceRows : Self.defaultRefreshDistanceRows
+ self.refreshDistanceRows = max(1, sanitizedRefresh)
}🤖 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/TerminalScrollDelivery.swift`
around lines 34 - 40, The initializer in TerminalScrollDelivery is clamping
refreshDistanceRows with max(1, refreshDistanceRows) on line 39, but this does
not guard against non-finite values like NaN or infinity. When NaN is passed,
max(1, NaN) still returns NaN, which causes the >= refreshDistanceRows
comparison check to always fail, leaving the refresh mechanism silently stuck.
Add a check to ensure refreshDistanceRows is finite (not NaN and not infinite)
before assigning it to self.refreshDistanceRows, either by rejecting invalid
input with a guard condition or by providing a safe default fallback value when
non-finite input is detected.
Comment on lines
+70
to
+78
| @Test func terminalScrollbackPrefetchStatePrimesThenRefreshesByDistance() { | ||
| var state = TerminalScrollbackPrefetchState(windowRows: 600, refreshDistanceRows: 10) | ||
|
|
||
| #expect(state.rowsToPrefetch(forScrollLines: 0) == nil) | ||
| #expect(state.rowsToPrefetch(forScrollLines: 1) == 600) | ||
| #expect(state.rowsToPrefetch(forScrollLines: 4) == nil) | ||
| #expect(state.rowsToPrefetch(forScrollLines: -5.5) == nil) | ||
| #expect(state.rowsToPrefetch(forScrollLines: 0.5) == 600) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add an overflow-carry regression case for prefetch cadence.
This test covers prime + exact-threshold crossing, but it doesn’t lock behavior for a large single delta (e.g., crossing refreshDistanceRows by more than one threshold). Add one case that verifies remainder is preserved between triggers.
🤖 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/Tests/CmuxMobileShellTests/TerminalScrollDeliveryQueueTests.swift`
around lines 70 - 78, Add a regression test case to the
terminalScrollbackPrefetchStatePrimesThenRefreshesByDistance test function that
verifies the prefetch cadence behavior when a single large scroll delta crosses
the refreshDistanceRows threshold by more than one unit. After the existing
`#expect` statements, add a case that demonstrates overflow-carry behavior by
scrolling a large distance (e.g., greater than twice the refreshDistanceRows
value of 10) and verify that the remainder is properly preserved between
prefetch triggers, ensuring the cadence logic correctly handles multi-threshold
crossings in a single operation.
0fa6418 to
472ce81
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/TerminalController+MobileScrollPrefetch.swift (1)
58-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
v2StrictIntfor strict numeric validation ofmax_scrollback_rowsinstead of unsafeNSNumber.intValuecoercion.Line 59 uses
NSNumber.intValue, which silently accepts and coerces non-integer JSON values and booleans. Replace withv2StrictInt(params, "max_scrollback_rows") ?? 0to match the codebase's strict validation pattern and reject non-integers explicitly.🔧 Proposed fix
private func mobileScrollPrefetchRows(params: [String: Any]) -> Int { - let requestedRows = (params["max_scrollback_rows"] as? NSNumber)?.intValue ?? 0 + let requestedRows = v2StrictInt(params, "max_scrollback_rows") ?? 0 return min( max(0, requestedRows), Self.mobileScrollPrefetchScrollbackLineBudget ) }Based on learnings: "In Swift request/JSON handlers (e.g., v2 JSON-socket handlers) in Sources, prefer using the v2 helpers for numeric parsing—use
v2Int(params, "<key>")for normal integer inputs andv2StrictInt(...)when strictness is required—rather than casting withas? Int."🤖 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 `@Sources/TerminalController`+MobileScrollPrefetch.swift around lines 58 - 64, The mobileScrollPrefetchRows function uses unsafe NSNumber.intValue coercion on line 59 to extract the max_scrollback_rows parameter, which silently accepts and converts non-integer JSON values and booleans. Replace the current extraction logic `(params["max_scrollback_rows"] as? NSNumber)?.intValue ?? 0` with `v2StrictInt(params, "max_scrollback_rows") ?? 0` to enforce strict numeric validation that explicitly rejects non-integer inputs, aligning with the codebase's standard validation pattern for JSON request handlers.Source: Learnings
🤖 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 1485-1679: Extract the presence sync subsystem from
MobileShellComposite into a dedicated collaborator class. Move the presenceMap
property, presenceTask property, and all presence-related methods
(evaluatePresenceSubscription, startPresenceSubscription, applyPresenceUpdate,
syncPushedRoutes with both variants, and applyPushedRoutes) into a new class
that owns the subscribe stream, backoff loop, and route-syncing logic. Keep
MobileShellComposite as an orchestration layer that instantiates and delegates
to this new collaborator, removing the presence implementation details and
keeping only the method calls that trigger the delegated operations. This
reduces MobileShellComposite's size and responsibility scope by separating the
presence pipeline from session state, paired-Mac persistence, transport, and
view-facing concerns.
- Around line 4319-4337: The freshness check in the
deliverAuthoritativeTerminalRenderGrid function only compares against the
delivered sequence from deliveredTerminalByteEndSeqBySurfaceID but should also
consider the pending sequence from pendingTerminalByteEndSeqBySurfaceID to
prevent stale frames from being applied after an input response has been
acknowledged. Update the condition that currently checks if deliveredSeq is
greater than renderGrid.stateSeq to use the maximum of both deliveredSeq and the
corresponding pendingTerminalByteEndSeqBySurfaceID value as the freshness floor,
rejecting the frame if stateSeq is older than this combined threshold.
---
Duplicate comments:
In `@Sources/TerminalController`+MobileScrollPrefetch.swift:
- Around line 58-64: The mobileScrollPrefetchRows function uses unsafe
NSNumber.intValue coercion on line 59 to extract the max_scrollback_rows
parameter, which silently accepts and converts non-integer JSON values and
booleans. Replace the current extraction logic `(params["max_scrollback_rows"]
as? NSNumber)?.intValue ?? 0` with `v2StrictInt(params, "max_scrollback_rows")
?? 0` to enforce strict numeric validation that explicitly rejects non-integer
inputs, aligning with the codebase's standard validation pattern for JSON
request handlers.
🪄 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: 7b1dbb9f-1818-4fd2-9aeb-84bcaafd301b
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalScrollDelivery.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollDeliveryQueueTests.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/TerminalController+MobileScrollPrefetch.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/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: 2
♻️ Duplicate comments (1)
Sources/TerminalController+MobileScrollPrefetch.swift (1)
58-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
v2StrictIntfor strict numeric validation ofmax_scrollback_rowsinstead of unsafeNSNumber.intValuecoercion.Line 59 uses
NSNumber.intValue, which silently accepts and coerces non-integer JSON values and booleans. Replace withv2StrictInt(params, "max_scrollback_rows") ?? 0to match the codebase's strict validation pattern and reject non-integers explicitly.🔧 Proposed fix
private func mobileScrollPrefetchRows(params: [String: Any]) -> Int { - let requestedRows = (params["max_scrollback_rows"] as? NSNumber)?.intValue ?? 0 + let requestedRows = v2StrictInt(params, "max_scrollback_rows") ?? 0 return min( max(0, requestedRows), Self.mobileScrollPrefetchScrollbackLineBudget ) }Based on learnings: "In Swift request/JSON handlers (e.g., v2 JSON-socket handlers) in Sources, prefer using the v2 helpers for numeric parsing—use
v2Int(params, "<key>")for normal integer inputs andv2StrictInt(...)when strictness is required—rather than casting withas? Int."🤖 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 `@Sources/TerminalController`+MobileScrollPrefetch.swift around lines 58 - 64, The mobileScrollPrefetchRows function uses unsafe NSNumber.intValue coercion on line 59 to extract the max_scrollback_rows parameter, which silently accepts and converts non-integer JSON values and booleans. Replace the current extraction logic `(params["max_scrollback_rows"] as? NSNumber)?.intValue ?? 0` with `v2StrictInt(params, "max_scrollback_rows") ?? 0` to enforce strict numeric validation that explicitly rejects non-integer inputs, aligning with the codebase's standard validation pattern for JSON request handlers.Source: Learnings
🤖 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 1485-1679: Extract the presence sync subsystem from
MobileShellComposite into a dedicated collaborator class. Move the presenceMap
property, presenceTask property, and all presence-related methods
(evaluatePresenceSubscription, startPresenceSubscription, applyPresenceUpdate,
syncPushedRoutes with both variants, and applyPushedRoutes) into a new class
that owns the subscribe stream, backoff loop, and route-syncing logic. Keep
MobileShellComposite as an orchestration layer that instantiates and delegates
to this new collaborator, removing the presence implementation details and
keeping only the method calls that trigger the delegated operations. This
reduces MobileShellComposite's size and responsibility scope by separating the
presence pipeline from session state, paired-Mac persistence, transport, and
view-facing concerns.
- Around line 4319-4337: The freshness check in the
deliverAuthoritativeTerminalRenderGrid function only compares against the
delivered sequence from deliveredTerminalByteEndSeqBySurfaceID but should also
consider the pending sequence from pendingTerminalByteEndSeqBySurfaceID to
prevent stale frames from being applied after an input response has been
acknowledged. Update the condition that currently checks if deliveredSeq is
greater than renderGrid.stateSeq to use the maximum of both deliveredSeq and the
corresponding pendingTerminalByteEndSeqBySurfaceID value as the freshness floor,
rejecting the frame if stateSeq is older than this combined threshold.
---
Duplicate comments:
In `@Sources/TerminalController`+MobileScrollPrefetch.swift:
- Around line 58-64: The mobileScrollPrefetchRows function uses unsafe
NSNumber.intValue coercion on line 59 to extract the max_scrollback_rows
parameter, which silently accepts and converts non-integer JSON values and
booleans. Replace the current extraction logic `(params["max_scrollback_rows"]
as? NSNumber)?.intValue ?? 0` with `v2StrictInt(params, "max_scrollback_rows")
?? 0` to enforce strict numeric validation that explicitly rejects non-integer
inputs, aligning with the codebase's standard validation pattern for JSON
request handlers.
🪄 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: 7b1dbb9f-1818-4fd2-9aeb-84bcaafd301b
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalScrollDelivery.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollDeliveryQueueTests.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/TerminalController+MobileScrollPrefetch.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/TerminalController.swift
🛑 Comments failed to post (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (2)
1485-1679: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Extract the presence sync subsystem instead of extending the god-store.
Lines 1485-1679 add a full subscribe/backoff/store-sync/reconnect subsystem into
MobileShellComposite, which is already far past the repo’s size/responsibility ceiling and already mixes session state, paired-Mac persistence, transport, terminal delivery, and view-facing draft state. Please move this presence pipeline behind a dedicated collaborator and keepMobileShellCompositeas orchestration only.As per coding guidelines,
Flag Swift production files that exceed 400 lines without a clear single responsibility, or exceed 800 lines even with mostly coherent responsibilityandFlag files that mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one place.🤖 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 1485 - 1679, Extract the presence sync subsystem from MobileShellComposite into a dedicated collaborator class. Move the presenceMap property, presenceTask property, and all presence-related methods (evaluatePresenceSubscription, startPresenceSubscription, applyPresenceUpdate, syncPushedRoutes with both variants, and applyPushedRoutes) into a new class that owns the subscribe stream, backoff loop, and route-syncing logic. Keep MobileShellComposite as an orchestration layer that instantiates and delegates to this new collaborator, removing the presence implementation details and keeping only the method calls that trigger the delegated operations. This reduces MobileShellComposite's size and responsibility scope by separating the presence pipeline from session state, paired-Mac persistence, transport, and view-facing concerns.Source: Coding guidelines
4319-4337:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop authoritative frames older than the pending input-ack floor.
Line 4328 only rejects frames older than
deliveredTerminalByteEndSeqBySurfaceID. IfhandleTerminalInputResponsehas already recorded a higherpendingTerminalByteEndSeqBySurfaceID, this shared helper can still apply a delayed live/scroll-prefetch frame whosestateSeqis behind an acknowledged host state and briefly roll the surface backward. Usemax(deliveredSeq, pendingSeq)as the freshness floor here before marking delivery.Suggested fix
func deliverAuthoritativeTerminalRenderGrid( _ renderGrid: MobileTerminalRenderGridFrame, expectedSurfaceID: String? = nil, source: String ) { guard expectedSurfaceID == nil || renderGrid.surfaceID == expectedSurfaceID, hasTerminalOutputSink(surfaceID: renderGrid.surfaceID) else { return } - if let deliveredSeq = deliveredTerminalByteEndSeqBySurfaceID[renderGrid.surfaceID], - deliveredSeq > renderGrid.stateSeq { + let deliveredSeq = deliveredTerminalByteEndSeqBySurfaceID[renderGrid.surfaceID] ?? 0 + let pendingSeq = pendingTerminalByteEndSeqBySurfaceID[renderGrid.surfaceID] ?? 0 + let freshnessFloor = max(deliveredSeq, pendingSeq) + if freshnessFloor > renderGrid.stateSeq { MobileDebugLog.anchormux( - "sync.render_grid_stale source=\(source) surface=\(renderGrid.surfaceID) delivered=\(deliveredSeq) frame=\(renderGrid.stateSeq)" + "sync.render_grid_stale source=\(source) surface=\(renderGrid.surfaceID) floor=\(freshnessFloor) frame=\(renderGrid.stateSeq)" ) return } markTerminalBytesDelivered(surfaceID: renderGrid.surfaceID, endSeq: renderGrid.stateSeq) deliverTerminalRenderGrid(renderGrid, surfaceID: renderGrid.surfaceID)🤖 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 4319 - 4337, The freshness check in the deliverAuthoritativeTerminalRenderGrid function only compares against the delivered sequence from deliveredTerminalByteEndSeqBySurfaceID but should also consider the pending sequence from pendingTerminalByteEndSeqBySurfaceID to prevent stale frames from being applied after an input response has been acknowledged. Update the condition that currently checks if deliveredSeq is greater than renderGrid.stateSeq to use the maximum of both deliveredSeq and the corresponding pendingTerminalByteEndSeqBySurfaceID value as the freshness floor, rejecting the frame if stateSeq is older than this combined threshold.
azooz2003-bit
force-pushed
the
task-ios-scrollback-window-cache
branch
from
472ce81 to
304a7fe
Compare
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)
cmux.xcodeproj/project.pbxproj (1)
250-281:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing
packageProductDependenciesentries for new package products.Line 1715 and Line 1726 add
CmuxTerminalandCmuxWorkspaceWindowto thecmuxFrameworks phase, and Lines 4672-4681 define theirXCSwiftPackageProductDependencyobjects. But thePBXNativeTarget "cmux"packageProductDependencieslist (Line 2660 onward) does not include these two product IDs, leaving target/package wiring incomplete.Suggested pbxproj fix
packageProductDependencies = ( A5001231 /* Sparkle */, A5001251 /* Sentry */, A5001271 /* PostHog */, @@ CC0DE30000000000000000A2 /* CmuxRemoteSession */, C750300000000000000000A2 /* CmuxTerminalEngine */, C750400000000000000000A2 /* CmuxTerminalServices */, + C750500000000000000000A2 /* CmuxTerminal */, + C750400000000000000000D2 /* CmuxWorkspaceWindow */, A8BD195031FC4B82B4354297 /* StackAuth */, EFB18E3B3099DFE2ECA3C263 /* CMUXMobileCore */, );As per coding guidelines, for
cmux.xcodeproj/**we should flag project wiring changes that can bypass expected integration behavior; this is a target wiring completeness gap.Also applies to: 1715-1726, 2888-2890, 4396-4403, 4672-4681
🤖 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 `@cmux.xcodeproj/project.pbxproj` around lines 250 - 281, The `PBXNativeTarget "cmux"` configuration is missing `packageProductDependencies` entries for the `CmuxTerminal` and `CmuxWorkspaceWindow` packages that were added to the Frameworks build phase and defined as `XCSwiftPackageProductDependency` objects. Locate the `packageProductDependencies` array in the `cmux` target definition and add the product reference IDs for both `CmuxTerminal` and `CmuxWorkspaceWindow` to complete the target/package wiring. Ensure that every framework product added to the build files also has a corresponding entry in the target's `packageProductDependencies` list to maintain project configuration consistency.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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Line 543: The scroll-prefetch bookkeeping additions (including the
terminalScrollbackPrefetchStatesBySurfaceID property and related functionality)
are causing MobileShellComposite.swift to exceed the file-size budget. Extract
all scroll-prefetch state management and authoritative render-grid delivery
helpers into a new, smaller scroll or output-focused component, then remove the
corresponding code from MobileShellComposite.swift. This will restore the file
to within budget and isolate the scroll-prefetch logic for better testability
and separation of concerns.
In
`@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalScrollDelivery.swift:
- Around line 86-100: The MobileTerminalReplayResponse.decode(data) call is
executing on the main actor within the scroll hot path, blocking frame rendering
and scroll gesture responsiveness. Move this CPU-bound JSON decoding operation
to a background task by wrapping the decode call and subsequent validation logic
in a detached Task or DispatchQueue.global() context, then dispatch the
deliverAuthoritativeTerminalRenderGrid call back to the main actor if needed.
This keeps the main actor free to handle frame rendering during scrolling.
---
Outside diff comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Around line 250-281: The `PBXNativeTarget "cmux"` configuration is missing
`packageProductDependencies` entries for the `CmuxTerminal` and
`CmuxWorkspaceWindow` packages that were added to the Frameworks build phase and
defined as `XCSwiftPackageProductDependency` objects. Locate the
`packageProductDependencies` array in the `cmux` target definition and add the
product reference IDs for both `CmuxTerminal` and `CmuxWorkspaceWindow` to
complete the target/package wiring. Ensure that every framework product added to
the build files also has a corresponding entry in the target's
`packageProductDependencies` list to maintain project configuration consistency.
🪄 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: 8b221658-a68b-4f72-8a1c-13fd28f96978
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalScrollDelivery.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/TerminalScrollDelivery.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalScrollDeliveryQueueTests.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+LocalScrollbackScroll.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftSources/TerminalController+MobileScrollPrefetch.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/TerminalController.swift
| var terminalOutputQueuesBySurfaceID: [String: TerminalOutputDeliveryQueue] | ||
| var terminalScrollQueueTokensBySurfaceID: [String: UUID] | ||
| var terminalScrollQueuesBySurfaceID: [String: TerminalScrollDeliveryQueue] | ||
| var terminalScrollbackPrefetchStatesBySurfaceID: [String: TerminalScrollbackPrefetchState] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Keep new scroll-prefetch bookkeeping out of this over-budget store.
These additions further grow MobileShellComposite.swift, which is already well past the repo’s Swift file-size budget. Please move the scroll-prefetch state and authoritative render-grid delivery helper into a smaller scroll/output-focused component so this path stays isolated and testable.
As per coding guidelines, 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: 670-670, 3083-3083, 4319-4337, 4371-4371
🤖 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`
at line 543, The scroll-prefetch bookkeeping additions (including the
terminalScrollbackPrefetchStatesBySurfaceID property and related functionality)
are causing MobileShellComposite.swift to exceed the file-size budget. Extract
all scroll-prefetch state management and authoritative render-grid delivery
helpers into a new, smaller scroll or output-focused component, then remove the
corresponding code from MobileShellComposite.swift. This will restore the file
to within budget and isolate the scroll-prefetch logic for better testability
and separation of concerns.
Source: Coding guidelines
Comment on lines
+86
to
+100
| let data = try await client.sendRequest(request) | ||
| guard let maxScrollbackRows = delivery.maxScrollbackRows, | ||
| maxScrollbackRows > 0, | ||
| remoteClient === client else { | ||
| return | ||
| } | ||
| guard let payload = try? MobileTerminalReplayResponse.decode(data), | ||
| let renderGrid = payload.renderGrid, | ||
| renderGrid.surfaceID == delivery.surfaceID else { | ||
| return | ||
| } | ||
| deliverAuthoritativeTerminalRenderGrid( | ||
| renderGrid, | ||
| expectedSurfaceID: delivery.surfaceID, | ||
| source: "scroll_prefetch" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 '`@MainActor`|func performTerminalScroll|MobileTerminalReplayResponse\.decode' \
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift \
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalScrollDelivery.swift
echo
rg -n -C3 'max_scrollback_rows|render_grid|prefetch|scrollback' \
Sources/TerminalController+MobileScrollPrefetch.swiftRepository: manaflow-ai/cmux
Length of output: 22542
Move scroll-prefetch JSON decode off the main actor.
MobileTerminalReplayResponse.decode(data) executes on the main actor inside performTerminalScroll(_:) within the scroll hot path. JSON decoding is CPU-bound work that can block frame rendering and scroll gesture responsiveness; detach it to a background task to avoid hitching.
♻️ Suggested change
let data = try await client.sendRequest(request)
guard let maxScrollbackRows = delivery.maxScrollbackRows,
maxScrollbackRows > 0,
remoteClient === client else {
return
}
- guard let payload = try? MobileTerminalReplayResponse.decode(data),
+ let payload = await Task.detached(priority: .userInitiated) {
+ try? MobileTerminalReplayResponse.decode(data)
+ }.value
+ guard remoteClient === client,
+ let payload,
let renderGrid = payload.renderGrid,
renderGrid.surfaceID == delivery.surfaceID else {
return
}🤖 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`+TerminalScrollDelivery.swift
around lines 86 - 100, The MobileTerminalReplayResponse.decode(data) call is
executing on the main actor within the scroll hot path, blocking frame rendering
and scroll gesture responsiveness. Move this CPU-bound JSON decoding operation
to a background task by wrapping the decode call and subsequent validation logic
in a detached Task or DispatchQueue.global() context, then dispatch the
deliverAuthoritativeTerminalRenderGrid call back to the main actor if needed.
This keeps the main actor free to handle frame rendering during scrolling.
azooz2003-bit
added a commit
that referenced
this pull request
Jun 14, 2026
#6067 (iOS terminal scrollback) merged after the first re-sync. Only the regenerable file-length budget conflicted; TerminalController.swift and pbxproj auto-merged (new TerminalController+MobileScrollPrefetch.swift is main's, wired by main). Budget regenerated; ghostty pinned to origin/main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Summary
Testing
swift test --package-path Packages/CmuxMobileShellpassed, 130 testsgit diff --check origin/main...HEADpassedpython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsvpassed./scripts/reload-cloud.sh --tag swbkfell back local and built macOS tagswbkios/scripts/reload.sh --tag swbk --simulator "iPhone 17"passed and launched simulator bundledev.cmux.ios.swbkios/scripts/reload-cloud.sh --tag swbk --device-id 4A52829D-6427-599F-A166-4058881D2DF4 --wait 60blocked on missing ASC API key; CoreDevice listed the physical iPhone as unavailableRelated
Summary by CodeRabbit
Release Notes
New Features
Performance / Reliability
Tests