Decouple iOS primary terminal scroll#6081
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads ChangesActive-screen scroll forwarding gate
Configurable API base URL through build and runtime
Native auth LAN host validation
Debug science demo terminal view
Sequence DiagramsequenceDiagram
participant Shell as MobileShellComposite
participant Chunk as MobileTerminalOutputChunk
participant Coordinator as GhosttySurfaceRepresentable.Coordinator
participant SurfaceView as GhosttySurfaceView
participant Policy as MobileTerminalScrollForwardingPolicy
Shell->>Chunk: yield(data:, streamToken:, activeScreen: delivery.activeScreen)
Coordinator->>SurfaceView: applyTerminalOutputMetadata(activeScreen: chunk.activeScreen)
SurfaceView->>SurfaceView: store activeScreen
Coordinator->>SurfaceView: processOutputAndWait(chunk.data)
Note over SurfaceView: scroll gesture triggers flushPendingScrollIfNeeded
SurfaceView->>Policy: shouldForwardToHost(activeScreen:)
alt .alternate screen
Policy-->>SurfaceView: true → fire didScrollLines
else .primary screen
Policy-->>SurfaceView: false → skip didScrollLines
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~26 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c43d31cec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let cell = pendingScrollCell | ||
| pendingScrollLines = 0 | ||
| applyLocalScrollbackScroll(lines: lines, col: cell.col, row: cell.row) | ||
| guard scrollForwardingPolicy.shouldForwardToHost(activeScreen: activeScreen) else { |
There was a problem hiding this comment.
Here activeScreen is not authoritative for all chunks yet: raw-byte fallback events carry nil so the view remains at its default .primary, and render-grid cursor-only/no-row-change deltas are built in MobileTerminalRenderObserver.emitRenderGrid without snapshot.frame.activeScreen, so they encode .primary even while a TUI is still on the alternate screen. In both cases an alternate-screen app such as vim/less/htop stops receiving wheel events after that chunk because this returns before didScrollLines; keep forwarding for unknown state and preserve the screen on every delta before using it to suppress host scrolls.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR decouples iOS primary-screen terminal scrollback from the Mac host: scroll gestures now update a local Ghostty mirror immediately (pixel-precise, with display-link inertia) while alternate-screen scroll still forwards wheel events to the Mac for TUI mouse reporting. Active-screen and scrollback-row metadata are threaded through
Confidence Score: 4/5The scroll-decoupling and replay-hydration paths are well-structured and tested, but the PR touches several security-critical surfaces (ATS configuration, native auth redirect origin resolution) with open concerns raised in previous review threads that have not been addressed. The iOS terminal scroll change is carefully layered — the forwarding policy is a pure testable value type, the inertia loop uses real display-link timestamps, and the geometry-waiter pattern correctly avoids double-resume via dictionary remove-before-resume. The workspace-mapping retry queue is sound. The outstanding concerns about ATS scope and request-origin trust that previous reviewers flagged are still present in the diff and represent meaningful risk in the auth flow. ios/Config/Info.plist (ATS configuration), web/app/handler/native-auth-helpers.ts and web/app/handler/native-sign-in/route.ts (origin trust for native auth redirect), Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift (metadata/VT-byte ordering) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant U as User Gesture
participant PG as scrollPanGesture
participant DL as Display Link
participant GSV as GhosttySurfaceView
participant Policy as ScrollForwardingPolicy
participant LG as Local Ghostty Mirror
participant Mac as Mac Host
U->>PG: pan changed / ended
PG->>GSV: enqueueScrollMechanicsDelta(deltaY)
Note over GSV: pendingLocalScrollPixels += pixels
DL->>GSV: applyScrollInertia(now)
GSV->>GSV: enqueueScrollMechanicsDelta (inertia)
DL->>GSV: flushPendingScrollIfNeeded()
GSV->>Policy: shouldApplyLocally(activeScreen, decouple)
GSV->>Policy: shouldForwardToHost(activeScreen, decouple)
alt "primary screen + decouple=true"
Policy-->>GSV: "local=true, host=false"
GSV->>LG: ghostty_surface_scroll_to_offset(localScrollRowOffset)
else "alternate screen OR decouple=false"
Policy-->>GSV: "local=false, host=true"
GSV->>Mac: didScrollLines(lines, col, row)
end
note over GSV,LG: Metadata path (per chunk)
Mac-->>GSV: MobileTerminalOutputChunk(activeScreen, scrollbackRows, data)
GSV->>GSV: applyTerminalOutputMetadata(activeScreen, scrollbackRows)
GSV->>LG: prepareForReplayViewport(cols, rows)
GSV->>LG: processOutputAndWait(data)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant U as User Gesture
participant PG as scrollPanGesture
participant DL as Display Link
participant GSV as GhosttySurfaceView
participant Policy as ScrollForwardingPolicy
participant LG as Local Ghostty Mirror
participant Mac as Mac Host
U->>PG: pan changed / ended
PG->>GSV: enqueueScrollMechanicsDelta(deltaY)
Note over GSV: pendingLocalScrollPixels += pixels
DL->>GSV: applyScrollInertia(now)
GSV->>GSV: enqueueScrollMechanicsDelta (inertia)
DL->>GSV: flushPendingScrollIfNeeded()
GSV->>Policy: shouldApplyLocally(activeScreen, decouple)
GSV->>Policy: shouldForwardToHost(activeScreen, decouple)
alt "primary screen + decouple=true"
Policy-->>GSV: "local=true, host=false"
GSV->>LG: ghostty_surface_scroll_to_offset(localScrollRowOffset)
else "alternate screen OR decouple=false"
Policy-->>GSV: "local=false, host=true"
GSV->>Mac: didScrollLines(lines, col, row)
end
note over GSV,LG: Metadata path (per chunk)
Mac-->>GSV: MobileTerminalOutputChunk(activeScreen, scrollbackRows, data)
GSV->>GSV: applyTerminalOutputMetadata(activeScreen, scrollbackRows)
GSV->>LG: prepareForReplayViewport(cols, rows)
GSV->>LG: processOutputAndWait(data)
Reviews (19): Last reviewed commit: "Gate iOS replay on terminal geometry" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| surfaceView.applyTerminalOutputMetadata(activeScreen: chunk.activeScreen) | ||
| await surfaceView.processOutputAndWait(chunk.data) |
There was a problem hiding this comment.
processOutputAndWait suspension
applyTerminalOutputMetadata commits the new activeScreen (e.g., .primary) before the await processOutputAndWait suspension returns. Because the @MainActor is free to interleave work during that suspension, a display-link scroll flush can fire in the window where activeScreen already says .primary but local Ghostty is still rendering the alternate surface. In that state flushPendingScrollIfNeeded calls applyLocalScrollbackScroll on what is still an alternate-screen surface, while simultaneously suppressing host forwarding — the net effect is a scroll event that neither reaches the TUI nor lands usefully in the local scrollback. Swapping the call order so metadata is applied after processOutputAndWait closes this window.
| private var lastAppliedContentScale: CGFloat = 0 | ||
| private var surfaceHasReceivedOutput: Bool = false | ||
| private var shouldScrollInitialOutputToBottom = true | ||
| private var activeScreen: MobileTerminalRenderGridFrame.Screen = .primary |
There was a problem hiding this comment.
activeScreen survives stream tear-down
activeScreen is never reset when the output stream is detached and reattached (e.g., after a network drop or background transition). If the previous session ended while the terminal was in .alternate mode, the property stays .alternate until the first render-grid frame arrives in the new stream. During that window every primary-screen scroll gesture is forwarded to the host instead of staying local. Consider resetting activeScreen to .primary in the view's stream-teardown path, or alternatively applying the reset at the start of each new stream token before yielding the first chunk.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift (1)
516-627: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit
GhosttySurfaceView; this production file is beyond the repository’s size/responsibility limits.This type is already far past 800 lines and mixes rendering, input, layout/docking, transport hooks, and diagnostics in one class. The new scroll-forwarding state extends that accumulation; please extract focused components (for example scroll forwarding + metadata state) before further growth.
As per coding guidelines: “Flag Swift production files that exceed 400 lines … or exceed 800 lines … [and] 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/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift` around lines 516 - 627, The GhosttySurfaceView class exceeds the 800-line limit and violates coding guidelines by mixing rendering, input, layout, transport, and diagnostics responsibilities in one place. Extract the scroll-forwarding functionality and its related state (such as the scrollForwardingPolicy property and any associated metadata) into a separate dedicated component or helper type before adding further scroll-forwarding enhancements. This will reduce GhosttySurfaceView's size and single responsibility violations while keeping scroll-forwarding logic cohesive and testable in its own focused type.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/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 625-627: The code currently defaults to treating unknown screen
metadata (when nil) as primary screen and suppressing host forwarding, breaking
alternate-screen TUI wheel events on legacy hosts. Fix this by modifying the
scroll handling logic to forward scroll events when screen metadata is unknown
instead of suppressing them. At the activeScreen initialization site (lines
625-627), ensure the default handling accommodates nil metadata. At the scroll
processing logic (lines 1816-1819), modify the condition to forward scroll
events when metadata is nil rather than assuming primary screen behavior. At the
metadata handling site (lines 2124-2129), ensure nil metadata is preserved
through the pipeline rather than dropped so downstream logic can apply
forward-fallback behavior.
---
Outside diff comments:
In
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 516-627: The GhosttySurfaceView class exceeds the 800-line limit
and violates coding guidelines by mixing rendering, input, layout, transport,
and diagnostics responsibilities in one place. Extract the scroll-forwarding
functionality and its related state (such as the scrollForwardingPolicy property
and any associated metadata) into a separate dedicated component or helper type
before adding further scroll-forwarding enhancements. This will reduce
GhosttySurfaceView's size and single responsibility violations while keeping
scroll-forwarding logic cohesive and testable in its own focused type.
🪄 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: 4a84b25a-54e0-4598-a74d-1339eaab6b0a
📒 Files selected for processing (8)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalOutputDelivery.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/TerminalOutputDelivery.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/TerminalOutputDeliveryQueueTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileTerminalOutputSinking.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/MobileTerminalScrollForwardingPolicy.swiftPackages/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/MobileTerminalScrollForwardingPolicyTests.swift
| private var activeScreen: MobileTerminalRenderGridFrame.Screen = .primary | ||
| private let scrollForwardingPolicy = MobileTerminalScrollForwardingPolicy() | ||
| /// Serial background queue for `ghostty_surface_process_output`, which |
There was a problem hiding this comment.
Handle unknown screen metadata as “forward” to preserve raw-byte fallback behavior.
Line 625 initializes activeScreen to .primary, and Line 2127 drops nil metadata. For raw-byte fallback chunks (where metadata is nil), Line 1816 always treats scroll as primary and suppresses host forwarding, which breaks alternate-screen TUI wheel events on legacy-host compatibility paths.
💡 Suggested fix
- private var activeScreen: MobileTerminalRenderGridFrame.Screen = .primary
+ private var activeScreen: MobileTerminalRenderGridFrame.Screen?
@@
- guard scrollForwardingPolicy.shouldForwardToHost(activeScreen: activeScreen) else {
- return
- }
+ if let activeScreen,
+ !scrollForwardingPolicy.shouldForwardToHost(activeScreen: activeScreen) {
+ return
+ }
@@
public func applyTerminalOutputMetadata(
activeScreen: MobileTerminalRenderGridFrame.Screen?
) {
guard let activeScreen else { return }
self.activeScreen = activeScreen
}Also applies to: 1816-1819, 2124-2129
🤖 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/GhosttySurfaceView.swift`
around lines 625 - 627, The code currently defaults to treating unknown screen
metadata (when nil) as primary screen and suppressing host forwarding, breaking
alternate-screen TUI wheel events on legacy hosts. Fix this by modifying the
scroll handling logic to forward scroll events when screen metadata is unknown
instead of suppressing them. At the activeScreen initialization site (lines
625-627), ensure the default handling accommodates nil metadata. At the scroll
processing logic (lines 1816-1819), modify the condition to forward scroll
events when metadata is nil rather than assuming primary screen behavior. At the
metadata handling site (lines 2124-2129), ensure nil metadata is preserved
through the pipeline rather than dropped so downstream logic can apply
forward-fallback behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eee05fbfd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard scrollForwardingPolicy.shouldForwardToHost(activeScreen: activeScreen) else { | ||
| return | ||
| } |
There was a problem hiding this comment.
When activeScreen is .primary, this new guard returns before the delegate call, so GhosttySurfaceRepresentable never invokes store.scrollTerminal(...). That RPC path is also the only place that primes and refreshes the larger max_scrollback_rows render-grid prefetch, so after a user scrolls beyond the cold-attach local scrollback window the phone has no way to fetch older primary-screen history and scrollback stops short even though the Mac still has it.
Useful? React with 👍 / 👎.
| <key>NSAllowsArbitraryLoads</key> | ||
| <true/> |
There was a problem hiding this comment.
This shared iOS Info.plist is used by both Debug and Release/TestFlight configs, so setting NSAllowsArbitraryLoads here disables ATS for all app networking, not just the WKWebView case described by the surrounding comment. In a release build this means auth/API/pairing URLSession calls can use plain HTTP if a URL is ever misconfigured or overridden; the dev-only API override should be scoped to Debug or to a narrow exception instead of globally weakening ATS.
Useful? React with 👍 / 👎.
| <key>NSAllowsArbitraryLoads</key> | ||
| <true/> | ||
| <key>NSAllowsArbitraryLoadsInWebContent</key> | ||
| <true/> |
There was a problem hiding this comment.
NSAllowsArbitraryLoads globally disables ATS for every app connection
Adding NSAllowsArbitraryLoads: true removes ATS for all URLSession traffic — API, auth, and pairing — not just WKWebView. This directly contradicts the comment just above it ("the app's own API/auth/pairing traffic stays under ATS and still requires HTTPS"). The existing NSAllowsArbitraryLoadsInWebContent: true was already sufficient for WKWebView; the new key is redundant for that purpose but breaks ATS for everything else. If an HTTP CMUX_API_BASE_URL override needs to work in dev, the right fix is a domain-specific exception under NSExceptionDomains or gating this key to debug build configurations only — not shipping it unconditionally in production.
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 `@ios/Config/Info.plist`:
- Around line 68-71: Remove the NSAllowsArbitraryLoads key and its corresponding
true value from the Info.plist file (lines 68-69). This setting globally
disables App Transport Security for all app traffic, which contradicts the
documented scope that restricts HTTP to WKWebView only. Keep the
NSAllowsArbitraryLoadsInWebContent setting if needed for web content, but ensure
global ATS bypass is not enabled.
🪄 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: c4750bcc-a59a-4d25-a105-91833f3b8b21
📒 Files selected for processing (3)
ios/Config/Info.plistios/cmuxPackage/Sources/cmuxFeature/MobileAuthComposition.swiftios/scripts/reload.sh
| <key>NSAllowsArbitraryLoads</key> | ||
| <true/> | ||
| <key>NSAllowsArbitraryLoadsInWebContent</key> | ||
| <true/> |
There was a problem hiding this comment.
Remove global ATS bypass (NSAllowsArbitraryLoads)
Line 68 enables unrestricted HTTP for all app traffic, which defeats ATS globally and contradicts the scope documented in Lines 62-67 (“ONLY WKWebView”). Keep NSAllowsArbitraryLoadsInWebContent if needed, but remove NSAllowsArbitraryLoads (or replace with narrowly scoped exception domains).
Suggested fix
- <key>NSAllowsArbitraryLoads</key>
- <true/>
<key>NSAllowsArbitraryLoadsInWebContent</key>
<true/>📝 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.
| <key>NSAllowsArbitraryLoads</key> | |
| <true/> | |
| <key>NSAllowsArbitraryLoadsInWebContent</key> | |
| <true/> | |
| <key>NSAllowsArbitraryLoadsInWebContent</key> | |
| <true/> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ios/Config/Info.plist` around lines 68 - 71, Remove the
NSAllowsArbitraryLoads key and its corresponding true value from the Info.plist
file (lines 68-69). This setting globally disables App Transport Security for
all app traffic, which contradicts the documented scope that restricts HTTP to
WKWebView only. Keep the NSAllowsArbitraryLoadsInWebContent setting if needed
for web content, but ensure global ATS bypass is not enabled.
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 `@web/app/handler/after-sign-in/route.ts`:
- Around line 43-45: The isLoopbackHost function is missing 0.0.0.0 from its
list of recognized loopback hosts, causing inconsistency with the native app's
canonicalizedLoopbackURL implementation which treats 0.0.0.0 as a loopback host.
Add 0.0.0.0 to the host equality checks in the isLoopbackHost function by
including an additional OR condition that compares the host parameter to
"0.0.0.0".
In `@web/app/handler/native-sign-in/route.ts`:
- Around line 14-16: The firstHeaderValue function is duplicated across multiple
route files (native-sign-in/route.ts and after-sign-in/route.ts). Extract this
helper function to a shared utility module (for example, create a new file like
web/app/handler/utils.ts or similar), then update both route files to import
firstHeaderValue from the shared utility module instead of defining it locally.
Remove the duplicate function definitions from both route files.
🪄 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: 38de39aa-e300-4960-904e-cff9ce6691ba
📒 Files selected for processing (3)
web/app/handler/after-sign-in/route.tsweb/app/handler/native-sign-in/route.tsweb/tests/native-auth-routes.test.ts
| function isLoopbackHost(host: string): boolean { | ||
| return host === "localhost" || host === "127.0.0.1" || host === "::1" || host === "[::1]"; | ||
| } |
There was a problem hiding this comment.
Missing 0.0.0.0 in loopback host list.
The native app's canonicalizedLoopbackURL in Sources/Auth/AuthEnvironment.swift:288-301 treats 0.0.0.0 as a loopback host, but this function does not. This could cause inconsistent behavior between the native app and web host classification.
Proposed fix
function isLoopbackHost(host: string): boolean {
- return host === "localhost" || host === "127.0.0.1" || host === "::1" || host === "[::1]";
+ return host === "localhost" || host === "127.0.0.1" || host === "::1" || host === "[::1]" || host === "0.0.0.0";
}🤖 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 `@web/app/handler/after-sign-in/route.ts` around lines 43 - 45, The
isLoopbackHost function is missing 0.0.0.0 from its list of recognized loopback
hosts, causing inconsistency with the native app's canonicalizedLoopbackURL
implementation which treats 0.0.0.0 as a loopback host. Add 0.0.0.0 to the host
equality checks in the isLoopbackHost function by including an additional OR
condition that compares the host parameter to "0.0.0.0".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2438ab6e4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const protocol = requestProtocol(request); | ||
| const hostValues = [ | ||
| firstHeaderValue(request.headers.get("host")), | ||
| firstHeaderValue(request.headers.get("x-forwarded-host")), |
There was a problem hiding this comment.
When this route is reachable through a proxy that does not overwrite client-supplied X-Forwarded-Host, adding that header to the accepted origin set lets a request to the real site send X-Forwarded-Host: attacker.test with after_auth_return_to=https://attacker.test/handler/after-sign-in?...; sameOriginURL accepts it and the later redirect is built on that attacker origin. That creates an open redirect/phishing vector on the auth entrypoint, so forwarded hosts should only be trusted from a known proxy/dev context or the redirect should remain anchored to the actual request origin.
Useful? React with 👍 / 👎.
| function requestHostCandidates(request: NextRequest): Set<string> { | ||
| const hosts = new Set<string>(); | ||
| for (const value of [ | ||
| request.headers.get("host"), | ||
| request.headers.get("x-forwarded-host"), | ||
| request.nextUrl.host, | ||
| ]) { | ||
| const host = firstHeaderValue(value)?.split(":")[0]?.toLowerCase(); | ||
| if (host) hosts.add(host); | ||
| } | ||
| return hosts; |
There was a problem hiding this comment.
x-forwarded-host included in loopback-trust candidates
requestHostCandidates adds the x-forwarded-host header to the set of hosts evaluated for isLocalRequest. The prior implementation only checked the host header (falling back to nextUrl.hostname). Because isLocalRequest is also used in production to decide whether cmux-dev://auth-callback is a valid native return scheme, an attacker who can inject x-forwarded-host: localhost (e.g., hitting an origin server that doesn't sit behind Vercel's edge, or any non-Vercel deployment) causes isLocalRequest to return true in production, enabling cmux-dev:// as an accepted return URL for any session.
The same vector applies to requestOriginCandidates in native-sign-in/route.ts: a forged x-forwarded-host: evil.com would make sameOriginURL accept https://evil.com/handler/after-sign-in as a valid after_auth_return_to, redirecting the entire Stack auth flow to an attacker-controlled server. Consider restricting x-forwarded-host trust to non-production environments, or applying an allowlist of known proxy-set values.
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/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputDebugLog.swift`:
- Around line 10-17: The `log(_:)` method accepts an eager String parameter,
which forces callers to construct the message string (via `textSummary(...)` or
`dataSummary(...)`) before the function runs, adding unnecessary per-keystroke
overhead when debug logging is disabled. Change the parameter from `_ message:
String` to use a closure parameter (either `_ message: `@autoclosure` () ->
String` or `_ message: () -> String`) so the message construction is deferred
until after the `guard isEnabled else { return }` check. If using
`@autoclosure`, the callers will not need to change their call sites; if using a
regular closure, callers will need to wrap their message arguments in a closure.
In `@web/app/handler/native-auth-helpers.ts`:
- Around line 9-20: The requestHostCandidates function's port stripping logic
using split(":")[0] incorrectly handles IPv6 addresses in bracket notation,
turning [::1]:3000 or [::1] into just "[". Fix the port-stripping logic to
properly handle both IPv4 and IPv6 formats: if the host starts with "[", extract
up to the closing bracket to preserve the full IPv6 address; otherwise, use the
current split logic for standard hosts and ports. Additionally, add a regression
test case that verifies the function correctly handles IPv6 addresses in bracket
notation with and without ports.
🪄 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: fdff2094-bc95-4875-8569-e5b4abcf2e2b
📒 Files selected for processing (5)
Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalInputDebugLog.swiftweb/app/handler/after-sign-in/route.tsweb/app/handler/native-auth-helpers.tsweb/tests/native-auth-routes.test.ts
💤 Files with no reviewable changes (1)
- Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift
| static func log(_ message: String) { | ||
| #if DEBUG | ||
| if ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] != nil { | ||
| return | ||
| } | ||
| #endif | ||
| guard isEnabled else { return } | ||
| logger.debug("input: \(message, privacy: .public)") |
There was a problem hiding this comment.
Make debug logging lazy to avoid per-keystroke overhead when disabled.
On Line 10, log(_:) takes an eager String, so callers still build textSummary(...)/dataSummary(...) before guard isEnabled runs. On the input path this adds avoidable work even when debug logging is off.
Proposed fix
- static func log(_ message: String) {
- `#if` DEBUG
- if ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] != nil {
- return
- }
- `#endif`
- guard isEnabled else { return }
- logger.debug("input: \(message, privacy: .public)")
- }
+ static func log(_ message: `@autoclosure` () -> String) {
+ guard isEnabled else { return }
+ `#if` DEBUG
+ if ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] != nil {
+ return
+ }
+ `#endif`
+ logger.debug("input: \(message(), privacy: .public)")
+ }As per coding guidelines, typing-latency-sensitive paths must avoid adding unnecessary per-event work, and this helper is used on terminal input events.
🤖 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/TerminalInputDebugLog.swift`
around lines 10 - 17, The `log(_:)` method accepts an eager String parameter,
which forces callers to construct the message string (via `textSummary(...)` or
`dataSummary(...)`) before the function runs, adding unnecessary per-keystroke
overhead when debug logging is disabled. Change the parameter from `_ message:
String` to use a closure parameter (either `_ message: `@autoclosure` () ->
String` or `_ message: () -> String`) so the message construction is deferred
until after the `guard isEnabled else { return }` check. If using
`@autoclosure`, the callers will not need to change their call sites; if using a
regular closure, callers will need to wrap their message arguments in a closure.
Source: Coding guidelines
| function requestHostCandidates(request: NextRequest): Set<string> { | ||
| const hosts = new Set<string>(); | ||
| for (const value of [ | ||
| request.headers.get("host"), | ||
| request.headers.get("x-forwarded-host"), | ||
| request.nextUrl.host, | ||
| ]) { | ||
| const host = firstHeaderValue(value)?.split(":")[0]?.toLowerCase(); | ||
| if (host) hosts.add(host); | ||
| } | ||
| return hosts; | ||
| } |
There was a problem hiding this comment.
Preserve bracketed IPv6 hosts when normalizing request hosts.
Line 16 strips ports with split(":")[0], which turns [::1]:3000 and even bare [::1] into "[". Because web/app/handler/after-sign-in/route.ts now delegates native_app_return_to validation here, IPv6 localhost sign-in flows get misclassified as non-local and reject cmux-dev callbacks end-to-end. Please add a regression case for [::1]:<port> too.
Proposed fix
function requestHostCandidates(request: NextRequest): Set<string> {
const hosts = new Set<string>();
for (const value of [
request.headers.get("host"),
request.headers.get("x-forwarded-host"),
request.nextUrl.host,
]) {
- const host = firstHeaderValue(value)?.split(":")[0]?.toLowerCase();
+ const rawHost = firstHeaderValue(value)?.toLowerCase();
+ const host = rawHost?.startsWith("[")
+ ? rawHost.match(/^\[[^\]]+\]/)?.[0] ?? null
+ : rawHost?.split(":")[0] ?? null;
if (host) hosts.add(host);
}
return hosts;
}🤖 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 `@web/app/handler/native-auth-helpers.ts` around lines 9 - 20, The
requestHostCandidates function's port stripping logic using split(":")[0]
incorrectly handles IPv6 addresses in bracket notation, turning [::1]:3000 or
[::1] into just "[". Fix the port-stripping logic to properly handle both IPv4
and IPv6 formats: if the host starts with "[", extract up to the closing bracket
to preserve the full IPv6 address; otherwise, use the current split logic for
standard hosts and ports. Additionally, add a regression test case that verifies
the function correctly handles IPv6 addresses in bracket notation with and
without ports.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 171d624a18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for await chunk in store.terminalOutputStream(surfaceID: surfaceID) { | ||
| guard !Task.isCancelled else { return } | ||
| guard let surfaceView else { return } | ||
| surfaceView.applyTerminalOutputMetadata(activeScreen: chunk.activeScreen) |
There was a problem hiding this comment.
When a render-grid chunk switches screens, this updates activeScreen before the VT patch has been applied to the local Ghostty surface. While processOutputAndWait is still queued/running, a user scroll can hit the old local surface state but the new forwarding policy; for example an exit-from-alt-screen chunk marks the view primary, the still-alt local scroll drops the wheel bytes, and the guard suppresses the host scroll, so the gesture is lost. Apply the metadata only after the chunk has been processed, or make the local scroll/forward decision use the same committed screen state.
Useful? React with 👍 / 👎.
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 (1)
ios/Config/Info.plist (1)
64-69: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winComment incorrectly claims
NSAllowsArbitraryLoadsexempts only WKWebView.The comment states "This key exempts ONLY WKWebView web content from ATS", but
NSAllowsArbitraryLoads(line 70) defeats App Transport Security for all app network traffic (native networking, URLSession, etc.), not just WKWebView. The key that exempts only WKWebView isNSAllowsArbitraryLoadsInWebContent(line 72).Update the comment to accurately describe which setting affects which traffic, or remove the misleading "ONLY WKWebView" claim.
🤖 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/Config/Info.plist` around lines 64 - 69, The comment in the Info.plist file incorrectly claims that the setting exempts only WKWebView from ATS, but NSAllowsArbitraryLoads actually disables App Transport Security for all app network traffic (native networking, URLSession, etc.), while NSAllowsArbitraryLoadsInWebContent is the key that exempts only WKWebView. Update the comment to accurately distinguish between what NSAllowsArbitraryLoads does (disables ATS globally) versus what NSAllowsArbitraryLoadsInWebContent does (disables ATS only for WKWebView content), removing the misleading claim about the "ONLY WKWebView" exemption.
♻️ Duplicate comments (1)
ios/Config/Info.plist (1)
70-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNSAllowsArbitraryLoads bypasses ATS globally in production builds.
This duplicates the prior review concern.
NSAllowsArbitraryLoadsis hardcodedtrueinInfo.plist, so production App Store builds also ship with App Transport Security defeated for all network traffic. The PR objectives mention "broader ATS settings for local development," but the implementation applies to all build configurations.Either:
- Remove
NSAllowsArbitraryLoadsand rely solely onNSAllowsArbitraryLoadsInWebContentfor WKWebView exemptions, or- Make this setting conditional (e.g., via a build variable like
$(CMUX_DISABLE_ATS)that's only set byreload.shfor dev builds)🤖 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/Config/Info.plist` around lines 70 - 71, The NSAllowsArbitraryLoads key in Info.plist is hardcoded to true, which disables ATS globally for all build configurations including production builds, creating a security vulnerability. Either remove the NSAllowsArbitraryLoads key/value pair entirely and rely only on NSAllowsArbitraryLoadsInWebContent for WKWebView exemptions, or make this setting conditional by replacing the hardcoded true value with a build variable reference (such as $(CMUX_DISABLE_ATS)) that is only enabled for development builds via the build configuration script.
🤖 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 `@ios/Config/Info.plist`:
- Around line 64-69: The comment in the Info.plist file incorrectly claims that
the setting exempts only WKWebView from ATS, but NSAllowsArbitraryLoads actually
disables App Transport Security for all app network traffic (native networking,
URLSession, etc.), while NSAllowsArbitraryLoadsInWebContent is the key that
exempts only WKWebView. Update the comment to accurately distinguish between
what NSAllowsArbitraryLoads does (disables ATS globally) versus what
NSAllowsArbitraryLoadsInWebContent does (disables ATS only for WKWebView
content), removing the misleading claim about the "ONLY WKWebView" exemption.
---
Duplicate comments:
In `@ios/Config/Info.plist`:
- Around line 70-71: The NSAllowsArbitraryLoads key in Info.plist is hardcoded
to true, which disables ATS globally for all build configurations including
production builds, creating a security vulnerability. Either remove the
NSAllowsArbitraryLoads key/value pair entirely and rely only on
NSAllowsArbitraryLoadsInWebContent for WKWebView exemptions, or make this
setting conditional by replacing the hardcoded true value with a build variable
reference (such as $(CMUX_DISABLE_ATS)) that is only enabled for development
builds via the build configuration script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f423f48b-8b00-4785-b939-89b21e1ecba4
📒 Files selected for processing (5)
ios/Config/Info.plistios/Config/Shared.xcconfigios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swiftios/cmuxPackage/Sources/cmuxFeature/ScienceDemoTerminalView.swiftios/scripts/reload.sh
| } | ||
|
|
||
| const stackSignInURL = new URL("/handler/sign-in", request.nextUrl.origin); | ||
| const stackSignInURL = new URL("/handler/sign-in", afterSignInURL.origin); |
There was a problem hiding this comment.
Handoff cookie HTTPS mismatch
Medium Severity
Native sign-in now treats x-forwarded-proto and x-forwarded-host as trusted origins for redirects, but the auth handoff cookie still sets secure from request.nextUrl.protocol only. HTTPS dev or proxied setups can redirect on https while the cookie is not marked secure, breaking verified auto-open.
Reviewed by Cursor Bugbot for commit 8e4636f. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1697ccd9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ## Current fork changes | ||
|
|
||
| Current cmux pinned fork head: `5697db81`, which adds the Darwin-only | ||
| Current cmux pinned fork head: `b61a016d`, which adds precision pixel-scroll |
There was a problem hiding this comment.
Fresh evidence in this revision is that the parent now pins Ghostty to b61a016d, not the earlier 5c89072b, but scripts/ghosttykit-checksums.txt still has no checksum for that SHA; I verified GHOSTTY_SHA=b61a016d768e4ac0c4850ddd20f0db9d35c56146 ./scripts/download-prebuilt-ghosttykit.sh exits with the missing-checksum error. The CI workflow downloads the prebuilt GhosttyKit before it can fall back to building locally, so a cold cache for this new submodule SHA will block CI/release until the matching archive/checksum is published or the workflow is changed.
Useful? React with 👍 / 👎.
| const hosts = new Set<string>(); | ||
| for (const value of [ | ||
| request.headers.get("host"), | ||
| request.headers.get("x-forwarded-host"), |
There was a problem hiding this comment.
When a production deployment is behind a proxy that does not overwrite client-supplied X-Forwarded-Host, this line lets a request add X-Forwarded-Host: localhost; isLocalRequest() then returns true before the NODE_ENV === "production" check in isTrustedDevRequest(), so /handler/after-sign-in accepts cmux-dev:// or configured cmux-dev-*:// return URLs and appends Stack tokens to that dev callback scheme. Please only use forwarded hosts from a trusted proxy context, or keep the local/dev scheme gate anchored to the actual request host.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c15650375d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ## Current fork changes | ||
|
|
||
| Current cmux pinned fork head: `5697db81`, which adds the Darwin-only | ||
| Current cmux pinned fork head: `a0f40f77`, which adds precision pixel-scroll |
There was a problem hiding this comment.
Fresh evidence in this revision is that the pinned Ghostty fork head is now a0f40f77, but scripts/ghosttykit-checksums.txt still has no entry for a0f40f77b3def446c066cf1a0ed1ebb0cdbb2750; I verified GHOSTTY_SHA=a0f40f77b3def446c066cf1a0ed1ebb0cdbb2750 ./scripts/download-prebuilt-ghosttykit.sh exits with the missing-checksum error. The CI path in .github/workflows/ci.yml:249-265 downloads the prebuilt framework before installing zig when the cache is cold, so CI/release builds for this submodule SHA will fail until the matching archive/checksum is published or the workflow fallback is changed.
Useful? React with 👍 / 👎.
| label.numberOfLines = 0 | ||
| label.textColor = .white | ||
| label.backgroundColor = UIColor(red: 0x27 / 255.0, green: 0x28 / 255.0, blue: 0x22 / 255.0, alpha: 1) | ||
| label.text = "Ghostty runtime failed to initialize." |
There was a problem hiding this comment.
The repo's AGENTS.md says “All user-facing strings must be localized,” but this new demo surface shows hard-coded English when CMUXMobileDemoMode=science or when the runtime fails to initialize. That leaves the iOS demo/failure UI untranslated and bypasses the required localization audit; please move the visible label/terminal copy through localized resources such as Resources/Localizable.xcstrings before displaying or encoding it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f82bcbffc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ## Current fork changes | ||
|
|
||
| Current cmux pinned fork head: `5697db81`, which adds the Darwin-only | ||
| Current cmux pinned fork head: `f644b4c10`, which adds precision pixel-scroll |
There was a problem hiding this comment.
Fresh evidence in this revision is that the documented/current Ghostty fork head is now f644b4c10, but scripts/ghosttykit-checksums.txt still has no entry for f644b4c100e37009e0820e2dd4467d61e52f1959; I verified GHOSTTY_SHA=f644b4c100e37009e0820e2dd4467d61e52f1959 ./scripts/download-prebuilt-ghosttykit.sh exits with the missing-checksum error. Cold-cache CI and release paths call this downloader (.github/workflows/ci.yml:262-265, .github/workflows/release.yml:171-174), so this submodule bump will block those builds until the matching archive/checksum is published or the workflow fallback is changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6e9f9716b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ## Current fork changes | ||
|
|
||
| Current cmux pinned fork head: `5697db81`, which adds the Darwin-only | ||
| Current cmux pinned fork head: `55b399077`, which adds precision pixel-scroll |
There was a problem hiding this comment.
Fresh evidence in this revision is that the documented/current Ghostty fork head is now 55b3990776a09d324b1b233737c8b1d8026a5e1a, but scripts/ghosttykit-checksums.txt has no entry for that SHA; I verified GHOSTTY_SHA=55b3990776a09d324b1b233737c8b1d8026a5e1a ./scripts/download-prebuilt-ghosttykit.sh exits with the missing-checksum error. Cold-cache CI and release paths call this downloader (.github/workflows/ci.yml:262-265, .github/workflows/release.yml:171-174), so this submodule bump will block those builds until the matching archive/checksum is published or the workflow fallback is changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f73e2c3dd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ## Current fork changes | ||
|
|
||
| Current cmux pinned fork head: `5697db81`, which adds the Darwin-only | ||
| Current cmux pinned fork head: `5829274d4`, which adds precision pixel-scroll |
There was a problem hiding this comment.
Fresh evidence in this revision is that the parent now pins Ghostty to 5829274d415f11cfb83ba67f79cc26a86e7c674e, but scripts/ghosttykit-checksums.txt still has no entry for that SHA; I verified GHOSTTY_SHA=5829274d415f11cfb83ba67f79cc26a86e7c674e ./scripts/download-prebuilt-ghosttykit.sh exits with the missing-checksum error. Cold-cache CI/release paths call that downloader before they can use the new submodule contents, so this submodule bump will block those builds until the matching archive/checksum is published or the workflow fallback is changed.
Useful? React with 👍 / 👎.
| @MainActor | ||
| public func debugScrollToRowOffsetForTesting(_ rowOffset: Double) -> Bool { | ||
| guard let surface else { return false } | ||
| ghostty_surface_scroll_to_offset(surface, rowOffset) |
There was a problem hiding this comment.
When running the CmuxTerminal package tests in DEBUG, this new GhosttyKit call is compiled into the library, but Packages/CmuxTerminal/Tests/GhosttyRuntimeTestStubs still does not define ghostty_surface_scroll_to_offset; that stub target exists specifically to satisfy these Ghostty symbols when the package test runner is not linking the real archive. A standalone swift test --package-path Packages/CmuxTerminal with submodules initialized will therefore fail at link time with an undefined symbol until the C/header stub is added.
Useful? React with 👍 / 👎.
…l-demo # Conflicts: # Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c3b8822. Configure here.
| self?.cancelGeometryWaiter(id: waiterID) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Geometry wait ends too early
Medium Severity
prepareForReplayViewport waits on geometry sync but resumes every waiter when any sync finishes, without confirming replayViewportReady. The caller then applies full replay bytes immediately, so replay can run before cell metrics and render rect match the Mac grid.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c3b8822. Configure here.
| localScrollbackBoundsInitialized = true | ||
| if wasAtBottom || localScrollRowOffset > localScrollbackMaxRowOffset { | ||
| localScrollRowOffset = localScrollbackMaxRowOffset | ||
| } |
There was a problem hiding this comment.
Local offset not pushed to Ghostty
Medium Severity
applyTerminalOutputMetadata updates localScrollRowOffset when scrollback bounds change but only applyLocalScrollbackScroll calls ghostty_surface_scroll_to_offset. After replay or a snap-to-bottom, Swift state and Ghostty’s fractional viewport can diverge until the user scrolls.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c3b8822. Configure here.
1 participant
Summary
Testing
Notes
I also inspected https://github.com/parkers0405/ghostty-pixel-scroll. The relevant pattern is local visual scroll state updated independently from terminal host state, with host interaction reserved for cases that must affect the terminal application.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Large cross-cutting change to mobile terminal sync, scroll, and Ghostty fork behavior; incorrect screen routing or replay geometry could break TUIs or leave stale/blank terminals.
Overview
Primary-screen scroll on iPhone stays on the device instead of waiting on the Mac for normal scrollback. Render-grid output now carries active screen, scrollback row count, and full-replay viewport size through
MobileTerminalOutputChunk; the iOS Ghostty mirror applies that metadata, sizes the grid before full replay, and scrolls viaghostty_surface_scroll_to_offsetwith pan + display-link inertia.MobileTerminalScrollForwardingPolicyapplies scroll locally on primary when decoupling is on; alternate-screen gestures still forward wheel input to the host.Cold attach replay sends
scrollback_scope=fullso the Mac can hydrate retained history; replay is retried when workspace→terminal mapping was not ready yet. SharedMobileTerminalScrollbackBudgetconstants replace magic numbers on host replay/prefetch. Settings add Instant Terminal Scroll (decouplePrimaryScreenScroll, default on).Ghostty fork docs pin fractional row-offset / pixel-scroll rendering; macOS gets
debug.terminal.scroll_to_row_offsetfor verification. Smaller bundled changes: launch attach URL connect during session restore, DEBUG science demo terminal, native auth LAN origin fixes, and broader iOS ATS for dev.Reviewed by Cursor Bugbot for commit c3b8822. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Keeps iOS primary-screen terminal scrollback on-device with pixel-precise, inertia-based scrolling; alternate-screen scroll still forwards to the Mac. Hydrates full scrollback on cold attach, now waits for the final viewport geometry before replay, and fixes launch attach-URL pairing during session restore.
New Features
GhosttySurfaceViewapplies it andMobileTerminalScrollForwardingPolicykeeps primary scroll local and forwards only alternate-screen wheel input.ghosttyfor fractional row‑offset scrolling with guard rows; macOS live scroll submits fractional offsets; addsdebug.terminal.scroll_to_row_offset.scrollback_scope=full; host-coupled scroll prefetch remains bounded (600 rows).Bug Fixes
Written for commit c3b8822. Summary will update on new commits.
Summary by CodeRabbit