Speed up iOS terminal scroll rendering#6035
Open
azooz2003-bit wants to merge 2 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughPer-surface terminal output is now queued with backpressure and optional replaceable render-grid coalescing. UI surfaces await processing and call terminalOutputDidProcess(surfaceID:) to advance queues; replay and live render-grid events are routed through the queue instead of falling back to raw-byte delivery. ChangesTerminal Output Queuing System
Sequence DiagramsequenceDiagram
participant StreamSource as terminalOutputStream
participant Coordinator as GhosttySurfaceRepresentable
participant View as GhosttySurfaceView
participant Store as MobileShellComposite
participant Queue as TerminalOutputDeliveryQueue
StreamSource->>Coordinator: emit data chunk
Coordinator->>View: await processOutputAndWait(data)
View->>View: main-actor UI updates (render grid / bytes)
View-->>Coordinator: return from await
Coordinator->>Store: terminalOutputDidProcess(surfaceID)
Store->>Queue: completeInFlight()
Queue-->>Store: next delivery bytes (if any)
Store-->>Coordinator: yield next bytes via continuation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (17 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 adds per-surface ACK-based backpressure to the iOS terminal output path. A new
Confidence Score: 4/5The backpressure queue logic and async handshake are structurally sound; the open items from prior reviews (stale-ACK after reconnect reset, double-ACK guard, ACK-when-skipped) remain unresolved and are the main reason to hold back from merging without a second look at the reset path. The queue invariants are well-tested and the The interaction between Important Files Changed
Sequence DiagramsequenceDiagram
participant Mac as Mac Host
participant MSC as MobileShellComposite
participant Q as DeliveryQueue
participant Stream as AsyncStream
participant GSR as GhosttySurfaceRepresentable
participant GSV as GhosttySurfaceView
participant LG as libghostty
Mac->>MSC: deliverTerminalRenderGrid(frame)
MSC->>Q: enqueue(delivery)
alt queue idle
Q-->>MSC: return delivery immediately
MSC->>Stream: continuation.yield(bytes)
GSR->>Stream: for await data
Stream-->>GSR: data
GSR->>GSV: await processOutputAndWait(data)
GSV->>LG: ghostty_surface_process_output (background queue)
LG-->>GSV: done
GSV-->>GSR: resume continuation
GSR->>MSC: terminalOutputDidProcess(surfaceID)
MSC->>Q: completeInFlight()
alt pending items exist
Q-->>MSC: next delivery
MSC->>Stream: continuation.yield(next.bytes)
else queue empty
Q-->>MSC: nil
end
else queue busy
Q->>Q: appendPending(delivery)
note over Q: replaceable frames coalesce with last pending replaceable
Q-->>MSC: nil
end
Reviews (2): Last reviewed commit: "Satisfy 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
+135
to
143
| outputTask = Task { @MainActor [weak surfaceView, weak store] in | ||
| guard let store else { return } | ||
| for await data in store.terminalOutputStream(surfaceID: surfaceID) { | ||
| guard !Task.isCancelled else { return } | ||
| surfaceView?.processOutput(data) | ||
| if let surfaceView { | ||
| await surfaceView.processOutputAndWait(data) | ||
| } | ||
| store.terminalOutputDidProcess(surfaceID: surfaceID) | ||
| } |
Contributor
There was a problem hiding this comment.
When surfaceView is nil, processOutputAndWait is skipped but store.terminalOutputDidProcess(surfaceID:) is still called unconditionally. The chunk has been dequeued from AsyncStream and discarded without ever reaching ghostty_surface_process_output, so the libghostty surface state does not reflect that data. The queue continues draining at full speed for as long as the view is nil, potentially skipping a run of frames. In practice the cold-attach replay on re-register catches up the surface, but the backpressure design assumes each ACK corresponds to an applied chunk — silently dropping chunks without an explicit comment or counter-measure makes this contract harder to audit and could mask future regressions where surfaceView goes nil unexpectedly mid-scroll.
Comment on lines
+61
to
+72
| mutating func completeInFlight() -> TerminalOutputDelivery? { | ||
| guard pendingHeadIndex < pending.count else { | ||
| inFlight = false | ||
| pending.removeAll(keepingCapacity: true) | ||
| pendingHeadIndex = 0 | ||
| return nil | ||
| } | ||
| let next = pending[pendingHeadIndex] | ||
| pendingHeadIndex += 1 | ||
| compactPendingStorageIfNeeded() | ||
| return next | ||
| } |
Contributor
There was a problem hiding this comment.
completeInFlight() has no guard against being called when inFlight is already false
If terminalOutputDidProcess is called spuriously — e.g. a second ACK from the same surface — completeInFlight() runs with inFlight == false and pending empty: the no-pending branch sets inFlight = false again (harmless today). But if inFlight is false and pending somehow has entries, the function would pop and return a pending item without ever setting inFlight = true, corrupting the queue invariant. A precondition(inFlight, "completeInFlight called with no item in flight") would make unintended double-ACKs loudly visible in debug builds.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift`:
- Around line 28-31: The Task's loop captures [weak self] but still calls
store.terminalOutputDidProcess(surfaceID:) even when self is nil; change the
loop in the Task { `@MainActor` [weak self] in for await data in
store.terminalOutputStream(surfaceID: surfaceID) { ... } } to guard the weak
self at the top of each iteration (e.g. guard let self = self else { break } )
so that you only append to self.lines and call
store.terminalOutputDidProcess(surfaceID:) when self is present, preventing
acknowledgement when the collector has been deallocated.
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 4308-4324: terminalOutputDidProcess(surfaceID:) currently advances
whichever queue is stored under terminalOutputQueuesBySurfaceID[surfaceID],
which allows stale ACKs to advance a replaced queue; fix by introducing a
per-stream generation/delivery token and validating it on ACK: add a generation
counter or UUID field to the terminal output queue (e.g. a generation on the
struct stored in terminalOutputQueuesBySurfaceID) and include that same token
with the continuation in terminalByteContinuationsBySurfaceID when a stream is
started; update resetTerminalOutputTracking() (and any remount/reset code) to
increment/rotate the generation; then change
terminalOutputDidProcess(surfaceID:) to load the queue, compare the
continuation's token to the queue.generation and only call
queue.completeInFlight() and continuation.yield(next.bytes) if the tokens match
(otherwise ignore the ACK). This ties ACKs to the exact stream generation and
prevents stale consumers from releasing the replacement queue.
🪄 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: 3d1c0c6f-d616-456d-9ec3-f6ac6dee5efc
📒 Files selected for processing (9)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileTerminalRenderGridFrame+TerminalOutputDelivery.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/TerminalOutputDelivery.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalOutputDeliveryQueueTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileTerminalOutputSinking.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
Comment on lines
28
to
+31
| task = Task { @MainActor [weak self] in | ||
| for await data in store.terminalOutputStream(surfaceID: surfaceID) { | ||
| self?.lines.append(String(data: data, encoding: .utf8) ?? "") | ||
| store.terminalOutputDidProcess(surfaceID: surfaceID) |
There was a problem hiding this comment.
Guard self availability before acknowledging output consumption.
The task captures [weak self], so if the collector is deallocated mid-stream, line 30's optional chaining silently skips appending the chunk to lines, but line 31 unconditionally calls store.terminalOutputDidProcess(surfaceID:). This tells the store the output was consumed even though the test helper didn't actually collect it, violating the backpressure contract.
🔒 Proposed fix to ensure atomic append-then-acknowledge
task = Task { `@MainActor` [weak self] in
for await data in store.terminalOutputStream(surfaceID: surfaceID) {
- self?.lines.append(String(data: data, encoding: .utf8) ?? "")
+ guard let self else { return }
+ self.lines.append(String(data: data, encoding: .utf8) ?? "")
store.terminalOutputDidProcess(surfaceID: 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 `@ios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift` around lines
28 - 31, The Task's loop captures [weak self] but still calls
store.terminalOutputDidProcess(surfaceID:) even when self is nil; change the
loop in the Task { `@MainActor` [weak self] in for await data in
store.terminalOutputStream(surfaceID: surfaceID) { ... } } to guard the weak
self at the top of each iteration (e.g. guard let self = self else { break } )
so that you only append to self.lines and call
store.terminalOutputDidProcess(surfaceID:) when self is present, preventing
acknowledgement when the collector has been deallocated.
Comment on lines
+4308
to
+4324
| /// Mark the current yielded terminal-output chunk as applied by the iOS surface. | ||
| /// | ||
| /// The mobile terminal render path keeps at most one chunk in flight per | ||
| /// mounted surface. Calling this method releases the next buffered chunk, if | ||
| /// any, and lets replaceable render-grid viewport frames collapse while | ||
| /// Ghostty is still processing older output. | ||
| /// - Parameter surfaceID: The terminal surface identifier. | ||
| public func terminalOutputDidProcess(surfaceID: String) { | ||
| guard var queue = terminalOutputQueuesBySurfaceID[surfaceID] else { return } | ||
| let next = queue.completeInFlight() | ||
| terminalOutputQueuesBySurfaceID[surfaceID] = queue | ||
| guard let next, | ||
| let continuation = terminalByteContinuationsBySurfaceID[surfaceID] else { | ||
| return | ||
| } | ||
| continuation.yield(next.bytes) | ||
| } |
There was a problem hiding this comment.
Key output ACKs to the stream generation, not just surfaceID.
terminalOutputDidProcess(surfaceID:) advances whichever queue currently lives under that surface. After resetTerminalOutputTracking() or a remount replaces terminalOutputQueuesBySurfaceID[surfaceID], the previous consumer can still be finishing its last chunk because the downstream loop ACKs only after awaiting surface application. That stale ACK will complete the replacement queue's new in-flight item and release buffered frames early, breaking the one-chunk backpressure contract during reconnect/remount.
This needs a per-stream or per-delivery token carried through the stream and ACK path so only the consumer that received a chunk can release its successor. Based on review stack context and the downstream ACK contract in Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift:128-145, queue advancement must stay tied to the same stream generation.
🤖 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 4308 - 4324, terminalOutputDidProcess(surfaceID:) currently
advances whichever queue is stored under
terminalOutputQueuesBySurfaceID[surfaceID], which allows stale ACKs to advance a
replaced queue; fix by introducing a per-stream generation/delivery token and
validating it on ACK: add a generation counter or UUID field to the terminal
output queue (e.g. a generation on the struct stored in
terminalOutputQueuesBySurfaceID) and include that same token with the
continuation in terminalByteContinuationsBySurfaceID when a stream is started;
update resetTerminalOutputTracking() (and any remount/reset code) to
increment/rotate the generation; then change
terminalOutputDidProcess(surfaceID:) to load the queue, compare the
continuation's token to the queue.generation and only call
queue.completeInFlight() and continuation.yield(next.bytes) if the tokens match
(otherwise ignore the ACK). This ties ACKs to the exact stream generation and
prevents stale consumers from releasing the replacement queue.
Comment on lines
+2165
to
+2183
| /// Process terminal output and return after the output has been applied. | ||
| /// | ||
| /// The call still performs libghostty output processing on the serial | ||
| /// background output queue. The returned async boundary lets callers apply | ||
| /// per-surface backpressure without blocking the main actor while Ghostty | ||
| /// consumes the chunk. | ||
| /// - Parameter data: VT or PTY bytes to feed into the surface. | ||
| public func processOutputAndWait(_ data: Data) async { | ||
| await withCheckedContinuation { continuation in | ||
| processOutput(data) { | ||
| continuation.resume() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func processOutput( | ||
| _ data: Data, | ||
| completion: (@MainActor @Sendable () -> Void)? | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Split the output-delivery lifecycle out of GhosttySurfaceView.
This file is already a multi-thousand-line surface that owns rendering, input, geometry, accessibility, composer docking, and platform-bridge behavior. Adding another public output/ack API here pushes further against the repo rule for oversized mixed-responsibility production Swift files; please extract the output-processing/backpressure coordination into a dedicated helper type in this package instead of growing this view further.
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 and Flag files that mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one place.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swift (1)
34-43:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftTie ACKs to a stream generation, not just
surfaceID.This still lets a stale consumer release a replacement queue.
resetTerminalOutputTracking()drops the queue map while leaving the continuation alive, andregisterTerminalOutput()overwrites the queue again on remount; after either transition,terminalOutputDidProcess(surfaceID:)will complete whichever queue currently sits under that surface. That breaks the one-chunk backpressure guarantee by either advancing a new queue from an old ACK or letting new deliveries yield immediately while the previous chunk is still being processed.
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swift#L34-L43: maketerminalOutputDidProcessvalidate a per-stream/per-queue token before callingcompleteInFlight().Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swift#L23-L31: carry that same token alongside the queued delivery/continuation path so a recreated queue cannot be mistaken for the previous stream.Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift#L2970-L2983: rotate or invalidate the token when terminal output tracking is reset.Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift#L4233-L4249: assign a fresh token on register/remount and clear it on unregister so late ACKs from the old mount are ignored.🤖 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`+TerminalOutputDelivery.swift around lines 34 - 43, The ACK handling must be tied to a per-stream token so stale ACKs cannot affect a replaced queue: update the terminal output queue and continuation storage to carry a streamToken alongside the queue/continuation (modify the structures touched in Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swift around lines 23-31), then in terminalOutputDidProcess(surfaceID:) (lines 34-43) verify the token matches before calling completeInFlight() and yielding the continuation; do not operate on the queue if the token differs. In Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift at 2970-2983, rotate or invalidate the token in resetTerminalOutputTracking() so old tokens can no longer be used, and in the register/unregister flow at 4233-4249 assign a fresh token on register/remount and clear it on unregister so late ACKs from the old mount are ignored; keep function names terminalOutputDidProcess, resetTerminalOutputTracking, and registerTerminalOutput as the anchor points for these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalHardwareKeyResolver.swift`:
- Around line 11-34: Add unit tests that verify
TerminalHardwareKeyResolver.data(input:modifierFlags:) maps UIKit inputs to the
same VT sequences produced by the TerminalKeyEncoder tables and ignores
unsupported modifier bits; create a new test case (e.g.,
TerminalHardwareKeyResolverTests) that: (1) checks navigation keys
(UIKeyCommand.inputUpArrow/inputDownArrow/inputLeftArrow/inputRightArrow/inputHome/inputEnd/inputPageUp/inputPageDown/inputDelete/inputEscape
and "\t" with and without .shift) produce the expected byte sequences, (2)
asserts controlInputs (letters "a".."z", "[", "]", "\\", " ", "2".."7", "3"?"4"?
— use the same string used in the implementation) with .control produce the
expected control bytes, (3) asserts shiftedControlInputs ("@","^","_","?") with
[.control, .shift] produce the expected bytes, and (4) verifies that adding an
unsupported modifier like .command does not change the resulting data; use
TerminalHardwareKeyResolver.data(input:modifierFlags:) to get actual output and
compare it against the authoritative expected sequences (reuse the mappings from
existing TerminalKeyEncoder tests or hardcode the known VT byte arrays) for each
case.
---
Duplicate comments:
In
`@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+TerminalOutputDelivery.swift:
- Around line 34-43: The ACK handling must be tied to a per-stream token so
stale ACKs cannot affect a replaced queue: update the terminal output queue and
continuation storage to carry a streamToken alongside the queue/continuation
(modify the structures touched in
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swift
around lines 23-31), then in terminalOutputDidProcess(surfaceID:) (lines 34-43)
verify the token matches before calling completeInFlight() and yielding the
continuation; do not operate on the queue if the token differs. In
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift at
2970-2983, rotate or invalidate the token in resetTerminalOutputTracking() so
old tokens can no longer be used, and in the register/unregister flow at
4233-4249 assign a fresh token on register/remount and clear it on unregister so
late ACKs from the old mount are ignored; keep function names
terminalOutputDidProcess, resetTerminalOutputTracking, and
registerTerminalOutput as the anchor points for these changes.
🪄 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: aba2306d-6240-40fb-bd2d-94964dd0975d
📒 Files selected for processing (5)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalHardwareKeyCommand.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalHardwareKeyResolver.swift
💤 Files with no reviewable changes (1)
- Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift
Comment on lines
+11
to
+34
| private static let keyCommands: [TerminalHardwareKeyCommand] = { | ||
| let navigation = [ | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputUpArrow, modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputDownArrow, modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputLeftArrow, modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputRightArrow, modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputLeftArrow, modifierFlags: [.alternate]), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputRightArrow, modifierFlags: [.alternate]), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputHome, modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputEnd, modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputPageUp, modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputPageDown, modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputDelete, modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputDelete, modifierFlags: [.alternate]), | ||
| TerminalHardwareKeyCommand(input: UIKeyCommand.inputEscape, modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: "\t", modifierFlags: []), | ||
| TerminalHardwareKeyCommand(input: "\t", modifierFlags: [.shift]), | ||
| ] | ||
| let controlInputs = Array("abcdefghijklmnopqrstuvwxyz[]\\ 234567/").map(String.init) | ||
| .map { TerminalHardwareKeyCommand(input: $0, modifierFlags: [.control]) } | ||
| let shiftedControlInputs = Array("@^_?").map(String.init) | ||
| .map { TerminalHardwareKeyCommand(input: $0, modifierFlags: [.control, .shift]) } | ||
| return navigation + controlInputs + shiftedControlInputs | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for tests covering TerminalHardwareKeyResolver encoding logic.
rg -n --type swift -C3 'TerminalHardwareKeyResolver' --glob '*Test*.swift'Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect resolver file to find the actual encoding/mapping behavior beyond keyCommands.
RESOLVER="Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalHardwareKeyResolver.swift"
echo "=== Resolver file (head/tail + key sections) ==="
ls -l "$RESOLVER"
sed -n '1,200p' "$RESOLVER" | nl -ba | sed -n '1,200p'
echo
sed -n '200,400p' "$RESOLVER" | nl -ba
# 2) Search for encoder/mapping APIs used by the resolver.
echo
echo "=== Search for related types/usages in code ==="
rg -n --type swift "TerminalHardwareKeyResolver|makeKeyCommands|keyCommands|TerminalHardwareKeyCommand" "$RESOLVER" Packages/CmuxMobileTerminal -S
# 3) Search for any tests that touch hardware key mapping/VT escape sequences indirectly.
echo
echo "=== Search for tests mentioning hardware keys / escape sequences ==="
rg -n --type swift "(HardwareKey|TerminalHardwareKey|UIKeyCommand\\.input|VT|\\x1b|escape sequence|CSI|\\[A|\\[B|\\[C|\\[D|\\[H|\\[F|\\[5~|\\[6~|\\[3~)" \
--glob '*Test*.swift' Packages -SRepository: manaflow-ai/cmux
Length of output: 308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
RESOLVER="Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalHardwareKeyResolver.swift"
echo "=== Resolver file (line-numbered excerpts) ==="
sed -n '1,120p' "$RESOLVER" | cat -n
echo
sed -n '120,240p' "$RESOLVER" | cat -n
echo
sed -n '240,360p' "$RESOLVER" | cat -n
echo
echo "=== Search for related types/usages in code ==="
rg -n --type swift "TerminalHardwareKeyResolver|makeKeyCommands|keyCommands|TerminalHardwareKeyCommand" "$RESOLVER" Packages/CmuxMobileTerminal -S
echo
echo "=== Search for any tests mentioning hardware keys / escape sequences ==="
rg -n --type swift "(HardwareKey|TerminalHardwareKey|UIKeyCommand\\.input|VT|\\x1b|escape sequence|CSI|\\[A|\\[B|\\[C|\\[D|\\[H|\\[F|\\[5~|\\[6~|\\[3~)" \
--glob '*Test*.swift' Packages -SRepository: manaflow-ai/cmux
Length of output: 21993
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate encoder + its tests
echo "=== Find TerminalKeyEncoder implementation ==="
rg -n --type swift "struct TerminalKeyEncoder|enum TerminalKeyEncoder|class TerminalKeyEncoder|TerminalKeyEncoder" Packages/CmuxMobileTerminalKit/Sources -S
echo
echo "=== Show TerminalKeyEncoderTests ==="
TEST="Packages/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalKeyEncoderTests.swift"
ls -l "$TEST"
sed -n '1,260p' "$TEST" | cat -n
echo
echo "=== Show TerminalArrowRepeatServiceTests (related escape sequences) ==="
TEST2="Packages/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalArrowRepeatServiceTests.swift"
ls -l "$TEST2"
sed -n '1,220p' "$TEST2" | cat -nRepository: manaflow-ai/cmux
Length of output: 8532
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for resolver-specific coverage in tests ==="
rg -n --type swift "TerminalHardwareKeyResolver\\.data\\(|TerminalHardwareKeyResolver\\.makeKeyCommands\\(|TerminalHardwareKeyResolver" \
--glob '*Test*.swift' Packages -S
echo
echo "=== Search for data(input:modifierFlags:) call sites (non-tests) ==="
rg -n --type swift "TerminalHardwareKeyResolver\\.data\\(input:|TerminalHardwareKeyResolver\\.data\\(text:|TerminalHardwareKeyResolver\\.data\\(" \
Packages/CmuxMobileTerminal -SRepository: manaflow-ai/cmux
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List Packages/CmuxMobileTerminal tests ==="
find Packages/CmuxMobileTerminal -maxdepth 3 -type d -name Tests -print
echo
echo "=== Search for UIKit-dependent tests patterns in CmuxMobileTerminal-related test targets ==="
rg -n --type swift "import UIKit|UIKeyModifierFlags|UIKeyCommand\\.input" Packages/*/Tests -S
echo
echo "=== Check for any TerminalHardwareKeyResolver tests scaffolding ==="
find Packages -maxdepth 4 -type f -name '*TerminalHardwareKey*' -printRepository: manaflow-ai/cmux
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
set +e
echo "=== ls Packages ==="
ls -la Packages | head -n 200
echo
echo "=== ls Packages/CmuxMobileTerminal ==="
ls -la Packages/CmuxMobileTerminal 2>&1
echo
echo "=== find test dirs under Packages/* ==="
find Packages -maxdepth 4 -type d -name Tests -print 2>&1 || true
echo
echo "=== Search for any UIKit usage inside any Swift test targets ==="
rg -n --type swift "import UIKit|UIKeyModifierFlags|UIKeyCommand\\.input" Packages/*/Tests -S 2>&1 || true
echo
echo "=== Find any test files mentioning TerminalHardwareKeyResolver ==="
rg -n --type swift "TerminalHardwareKeyResolver" Packages -S --glob '*Test*.swift' 2>&1 || true
echo
echo "=== Find any TerminalHardwareKey* files (impl + tests) ==="
find Packages -maxdepth 6 -type f -name '*TerminalHardwareKey*' -print 2>&1 || trueRepository: manaflow-ai/cmux
Length of output: 4853
Add unit tests for TerminalHardwareKeyResolver UIKeyCommand→VT encoding mapping
TerminalKeyEncoderTests already cover the VT byte tables for special keys/control characters, so the remaining gap is ensuring TerminalHardwareKeyResolver.data(input:modifierFlags:) maps UIKit inputs (arrows/home/end/page/delete/escape/tab) + modifiers (.alternate, .control, .control+.shift for @^_?) to the correct TerminalKeyEncoder outputs, and ignores unsupported modifier bits (e.g., .command).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalHardwareKeyResolver.swift`
around lines 11 - 34, Add unit tests that verify
TerminalHardwareKeyResolver.data(input:modifierFlags:) maps UIKit inputs to the
same VT sequences produced by the TerminalKeyEncoder tables and ignores
unsupported modifier bits; create a new test case (e.g.,
TerminalHardwareKeyResolverTests) that: (1) checks navigation keys
(UIKeyCommand.inputUpArrow/inputDownArrow/inputLeftArrow/inputRightArrow/inputHome/inputEnd/inputPageUp/inputPageDown/inputDelete/inputEscape
and "\t" with and without .shift) produce the expected byte sequences, (2)
asserts controlInputs (letters "a".."z", "[", "]", "\\", " ", "2".."7", "3"?"4"?
— use the same string used in the implementation) with .control produce the
expected control bytes, (3) asserts shiftedControlInputs ("@","^","_","?") with
[.control, .shift] produce the expected bytes, and (4) verifies that adding an
unsupported modifier like .command does not change the resulting data; use
TerminalHardwareKeyResolver.data(input:modifierFlags:) to get actual output and
compare it against the authoritative expected sequences (reuse the mappings from
existing TerminalKeyEncoder tests or hardcode the known VT byte arrays) for each
case.
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, 99 Swift Testing tests.ios/scripts/reload.sh --tag scrlpassed for iPhone 17 simulator, bundledev.cmux.ios.scrl.cmux-assets/task-fast-ios-terminal-scroll/ios-simulator/20260612-223448/cmux-scrl-final.png.Notes
xcrun devicectl list devices.swift test --package-path ios/cmuxPackageandswift test --package-path Packages/CmuxMobileTerminalare blocked by existing SwiftPM platform-manifest resolution errors: the UI/terminal libraries default to macOS 10.13 while dependencies require macOS 14.0.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Speeds up iOS terminal scrolling by adding per-surface ACK-based output backpressure and collapsing obsolete viewport frames during fast gestures. Also removes scroll debug logs from the hot path.
New Features
processOutputAndWaiton the iOS surface to await application; the UI now awaits and ACKs viaterminalOutputDidProcess.TerminalHardwareKeyResolverto satisfy file length limits.Migration
terminalOutputStreamdirectly, callterminalOutputDidProcess(surfaceID:)after applying each chunk.GhosttySurfaceRepresentablealready does this.Written for commit 5e8a16d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation