Repository navigation
Revert PR 8848 before Ghostty lag fix - #8893
lawrencecchen wants to merge 1 commit into
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe changes revise Ghostty surface bridge ownership and render handling, remove a terminal shortcut bypass and its tests, refresh the Ghostty fork pin and checksums, simplify resume-policy mapping, and update two test imports. ChangesGhostty surface lifecycle
Shortcut routing cleanup
Maintenance updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GhosttySurfaceView
participant outputQueue
participant ghostty_surface_free
participant GhosttySurfaceBridge
GhosttySurfaceView->>outputQueue: enqueue surface free with bridge
outputQueue->>GhosttySurfaceBridge: retain bridge
outputQueue->>ghostty_surface_free: free surface
ghostty_surface_free-->>outputQueue: free completes
outputQueue->>GhosttySurfaceBridge: release bridge
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- Around line 2498-2500: Update disposeSurface() and the enqueueSurfaceFree
teardown path so GhosttySurfaceView remains strongly retained until asynchronous
surface free and cleanup complete, rather than relying on currentBridge after it
is detached. Preserve the existing generation and queue coordination, and
release the view only from the free-completion path; alternatively, replace the
unretained uiview userdata created by makeSurface() with an ownership object
that remains pinned through teardown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 744a538b-e80a-4b45-ae55-1a53bb9c77bb
📒 Files selected for processing (15)
Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttyRuntime.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceBridge.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+RenderRecovery.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/iOS/CmuxMobileTerminal/Tests/CmuxMobileTerminalTests/GhosttyRuntimeActionTests.swiftSources/App/ShortcutBareStartRouting.swiftSources/AppDelegate.swiftSources/ControlSurfaceResumeTarget.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AppDelegateTerminalTypingShortcutFastPathTests.swiftcmuxTests/SidebarWorkspaceRowSuspensionTests.swiftcmuxTests/SidebarWorkspaceTableSuspensionTests.swiftdocs/ghostty-fork.mdghosttyscripts/ghosttykit-checksums.txt
💤 Files with no reviewable changes (8)
- cmuxTests/SidebarWorkspaceTableSuspensionTests.swift
- cmuxTests/AppDelegateTerminalTypingShortcutFastPathTests.swift
- Packages/iOS/CmuxMobileTerminal/Tests/CmuxMobileTerminalTests/GhosttyRuntimeActionTests.swift
- cmux.xcodeproj/project.pbxproj
- Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttyRuntime.swift
- cmuxTests/SidebarWorkspaceRowSuspensionTests.swift
- scripts/ghosttykit-checksums.txt
- Sources/App/ShortcutBareStartRouting.swift
| // backlog drains before the free. (Retain the bridge across the hop; it | ||
| // owns the userdata libghostty still references until the free.) | ||
| enqueueSurfaceFree(surface, bridge: currentBridge, generation: surfaceGeneration, on: currentQueue) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'disposeSurface|enqueueSurfaceFree|ghostty_platform_ios_s|uiview|detach\(\)' \
Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal ghosttyRepository: manaflow-ai/cmux
Length of output: 1866
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- GhosttySurfaceBridge.swift ---\n'
cat -n Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceBridge.swift
printf '\n--- GhosttySurfaceView.swift relevant sections ---\n'
sed -n '2460,2525p' Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift | cat -n
sed -n '1850,1892p' Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift | cat -n
sed -n '3448,3482p' Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift | cat -n
printf '\n--- RenderRecovery relevant section ---\n'
sed -n '180,225p' Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+RenderRecovery.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- GhosttySurfaceBridge.swift ---'
cat -n Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceBridge.swift
echo
echo '--- GhosttySurfaceView.swift relevant sections ---'
sed -n '2460,2525p' Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift | cat -n
sed -n '1850,1892p' Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift | cat -n
sed -n '3448,3482p' Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift | cat -n
echo
echo '--- RenderRecovery relevant section ---'
sed -n '180,225p' Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView+RenderRecovery.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 13650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Locate Ghostty source files ---'
git ls-files | rg '(^ghostty/|^P|\.h$|ghostty_surface\.c|ghostty\.h|ghostty_platform)' | head -200
echo
echo '--- Search IOS platform code for uiview/uiview callbacks ---'
rg -n 'uiview|ghostty_platform_ios|GHOSTTY_PLATFORM_IOS' ghostty Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal 2>/dev/null | head -300
echo
echo '--- Search surface free implementation/usages in Ghostty source ---'
rg -n 'ghostty_surface_free|surface_free|free.*surface|ghostty_surface_new|surface_new' ghostty Packages/iOS/CmuxMobileTerminal 2>/dev/null | head -400
echo
echo '--- Search userdata ownership comments in Ghostty source ---'
rg -n 'userdata|platform|iOS|UIKit|UIView|ios\\.uiview' ghostty/Packages ghostty || trueRepository: manaflow-ai/cmux
Length of output: 17635
🌐 Web query:
Ghostty ghostty_surface_free userdata iOS uiview platform ownership
💡 Result:
In the Ghostty architecture, these components relate to how the terminal emulator interfaces with the host platform's UI and memory management: ghostty_surface_free This is a C API function provided by libghostty [1][2]. It is used to deallocate a terminal surface and its associated resources, such as Metal rendering structures and terminal grid state [3][4]. When called, it initiates the teardown of the renderer and associated platform-specific layers (like IOSurface or Metal layers on Apple platforms) [5]. userdata The userdata pointer is an arbitrary void* provided in the ghostty_surface_config_s configuration struct during the initialization of a surface [1][3]. It allows the embedding application to associate its own context (e.g., a Controller, ViewModel, or View object) with the Ghostty surface [2]. This pointer can be retrieved at any time using the ghostty_surface_userdata C API function [1][2]. iOS UIView and Platform Ownership On iOS, Ghostty uses a platform-native approach where the embedding application provides a UIView into which Ghostty renders using Metal [3][4]. - Platform Data: The platform-specific configuration for iOS is defined in the ghostty_platform_ios_s struct, which contains a void* member named uiview to hold a reference to the host application's view [1][3]. - Ownership: Ghostty's core (libghostty) manages the terminal emulation, grid state, and Metal rendering pipeline [6][4]. However, the embedding application typically retains ownership of the native UI objects (like the UIView). Because Ghostty's renderer may perform asynchronous operations (e.g., presenting frames via Metal or Core Animation), issues such as use-after-free can occur if the host application frees its UI resources before Ghostty has finished its internal cleanup, necessitating careful synchronization during surface teardown [5]. The "lazy/pull" architecture often employed by Ghostty-based apps means the Zig core owns the terminal state, while the Swift/native wrapper queries it as needed [7].
Citations:
- 1: https://github.com/ghostty-org/ghostty/blob/main/include/ghostty.h
- 2: https://www.mintlify.com/ghostty-org/ghostty/api/rendering
- 3: https://www.mintlify.com/ghostty-org/ghostty/api/core-types
- 4: https://github.com/ghostty-org/ghostty
- 5: iOS: use-after-free on ghostty_surface_free — Metal completion/CA transaction presents freed IOSurface after renderer teardown ghostty-org/ghostty#13021
- 6: https://ghostty.org/docs/about
- 7: https://github.com/daiimus/geistty/blob/main/ARCHITECTURE.md
🌐 Web query:
github ghostty-src ghostty_platform_ios.c ghostty_surface_free uiview
💡 Result:
In the Ghostty terminal emulator codebase, ghostty_surface_free is a C API function used to deallocate a surface and its associated resources [1][2][3]. When operating on iOS, the surface uses platform-specific configuration defined by ghostty_platform_ios_s, which contains a void* pointer to a UIView [4][2][3]. Technical details regarding this implementation include: 1. Function Purpose: ghostty_surface_free serves as the primary cleanup mechanism for a terminal surface [1][2]. Internally, it triggers the application to close the surface, subsequently calling deinitialization routines [5][6]. 2. iOS Platform Data: The ghostty_platform_ios_s structure is defined in include/ghostty.h as a wrapper for a UIView pointer, allowing the terminal to render its output into that native iOS view, typically utilizing Metal for GPU acceleration [2][3]. 3. Implementation Context: The underlying logic is handled in Zig [5][6]. When ghostty_surface_free is called, it typically involves tearing down the renderer (such as Metal-based swap chains), stopping terminal threads, and releasing GPU-related resources like IOSurfaces and textures [7]. 4. Potential Issues: Developers should be aware of lifecycle management complexities, such as potential use-after-free scenarios if Metal completion handlers or Core Animation transactions attempt to reference resources (like an IOSurface) after they have been freed during the surface teardown process [7]. The C API and related structures are found within the include/ghostty.h header, and the platform-specific logic is managed in files such as src/apprt/embedded.zig [4][5][3].
Citations:
- 1: https://www.mintlify.com/ghostty-org/ghostty/api/rendering
- 2: https://www.mintlify.com/ghostty-org/ghostty/api/core-types
- 3: https://github.com/ghostty-org/ghostty/blob/f8a0a459/include/ghostty.h
- 4: https://github.com/ghostty-org/ghostty/blob/main/include/ghostty.h
- 5: https://github.com/ghostty-org/ghostty/blob/d31ac2be/src/apprt/embedded.zig
- 6: https://github.com/ghostty-org/ghostty/blob/9e080c5a/src/apprt/embedded.zig
- 7: iOS: use-after-free on ghostty_surface_free — Metal completion/CA transaction presents freed IOSurface after renderer teardown ghostty-org/ghostty#13021
🌐 Web query:
ghostty src embedded.zig uiview ghostty_platform_ios_s
💡 Result:
In the Ghostty terminal emulator codebase, ghostty_platform_ios_s is a C structure used within the embedded runtime API to provide platform-specific configuration for iOS [1][2]. It is defined in the public header file include/ghostty.h as follows: typedef struct { void* uiview; } ghostty_platform_ios_s; This structure contains a single member, uiview, which is a pointer to the UIView (cast to void*) where the terminal is intended to render [3][2]. The file src/apprt/embedded.zig acts as the Zig-side implementation of this embedded runtime [4][5]. Within this file, the Platform union (and its associated C compatibility union) handles the initialization and abstraction of these platform-specific views [6][7]. The init function in embedded.zig ensures that when the library is initialized, the uiview provided via the C ABI is properly cast and stored for use by Ghostty's rendering engine, which uses Metal to render to the specified view [6][3]. In practical usage, such as in the Swift-based surface view implementation, the host application passes a reference to its UIView—via an opaque pointer—to the libghostty configuration [8]. This allows Ghostty to function as an embedded component within a larger iOS application, rather than managing the entire application lifecycle itself [4][5].
Citations:
- 1: https://github.com/ghostty-org/ghostty/blob/main/include/ghostty.h
- 2: https://github.com/ghostty-org/ghostty/blob/master/include/ghostty.h
- 3: https://mintlify.wiki/ghostty-org/ghostty/api/core-types
- 4: https://github.com/ghostty-org/ghostty/blob/9e080c5a/src/apprt/embedded.zig
- 5: https://docsmith.aigne.io/discuss/docs/ghostty/architecture-apprt-ed730d
- 6: https://github.com/ghostty-org/ghostty/blob/65901966/src/apprt/embedded.zig
- 7: https://github.com/ghostty-org/ghostty/blob/d31ac2be/src/apprt/embedded.zig
- 8: https://github.com/ghostty-org/ghostty/blob/b23d567c/macos/Sources/Ghostty/Surface%20View/SurfaceView.swift
Keep the unretained platform view alive through teardown.
disposeSurface() detaches currentBridge before this asynchronous free, so retaining the bridge no longer keeps GhosttySurfaceView alive. Since makeSurface() stores self as an unretained ghostty_platform_ios_s.uiview, the pinned Ghostty teardown/cleanup path could still reference a released view. Retain the view on teardown until free completion, or replace the unretained platform userdata with a retained/pinned ownership object.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`
around lines 2498 - 2500, Update disposeSurface() and the enqueueSurfaceFree
teardown path so GhosttySurfaceView remains strongly retained until asynchronous
surface free and cleanup complete, rather than relying on currentBridge after it
is detached. Preserve the existing generation and queue coordination, and
release the view only from the free-completion path; alternatively, replace the
unretained uiview userdata created by makeSurface() with an ownership object
that remains pinned through teardown.
Greptile SummaryThis PR reverts the entirety of PR #8848, rolling back the Ghostty submodule from
Confidence Score: 5/5Clean revert to a known-good pre-8848 tree; the manual bridge retain/release pattern, submodule pin, and checksums are all internally consistent. Every changed file is either a mechanical reversal to shipped code, a test file deletion whose production counterpart is also removed, or documentation updated to match the new submodule pin. The Ghostty submodule, GhosttyKit checksums, and ghostty-fork.md are updated in lockstep. The #if DEBUG production static from ShortcutBareStartRouting.swift is correctly excised. The bridge lifecycle (passUnretained in makeSurface, explicit passRetained/release in enqueueSurfaceFree) is the same pattern that was stable before PR 8848, and the known view↔bridge cycle is re-documented with its tracking issue. No new rule violations are introduced. Files Needing Attention: No files require special attention. The acknowledged regression (serial frame-lease rotation from ghostty/pull/145) is intentional and tracked for reintroduction in PR #8889. Important Files Changed
Sequence DiagramsequenceDiagram
participant V as GhosttySurfaceView
participant B as GhosttySurfaceBridge
participant L as libghostty
participant Q as WorkQueue
V->>B: passUnretained(bridge) bridgePointer
V->>L: ghostty_surface_new(app, surfaceConfig)
L-->>V: createdSurface
L->>B: io_write_cb via unretained bridgePointer
B->>V: DispatchQueue.main.async handleOutboundBytes
V->>B: currentBridge.detach()
V->>Q: enqueueSurfaceFree(surface, bridge)
Q->>Q: passRetained(bridge) retainedBridge
Q->>L: ghostty_surface_free(surface)
Q->>B: retainedBridge.release()
Reviews (1): Last reviewed commit: "Revert "Merge pull request #8848 from ma..." | Re-trigger Greptile |
|
Closing because canonical autoreview correctly rejected a wholesale revert: it would also remove valid bounded renderer progress, frame-slot rotation, Kitty graphics limits, the iOS continuation handler, and required test imports. The targeted correction remains #8889: keep those fixes, advance Ghostty to current |
1 participant
Reverts Austin Wang’s #8848 exactly.
The merged branch pins superseded Ghostty
50ad1963d, includes the owned-userdata API removed by current Ghostty fork main through manaflow-ai/ghostty#146, and adds a separate terminal shortcut-routing fast path that is not required by the IOSurface frame-reuse root cause.This revert restores the exact tree from main immediately before PR 8848. The clean replacement is Lawrence Chen’s #8889, which retains Austin’s valid frame-slot rotation from manaflow-ai/ghostty#145 and will carry only the required iOS renderer continuation handler.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Reverts the typing‑lag investigation changes by removing the terminal typing shortcut fast path and the Ghostty owned‑userdata integration. Restores the pre-change
ghosttyintegration and updates the iOS surface bridge to the borrowed‑userdata model.Refactors
GHOSTTY_ACTION_RENDERhook and related tests; external render continuation handled by the renderer as before.ghostty_surface_newwith borrowed userdata; retain the bridge during surface free to prevent use‑after‑free.Dependencies
ghosttyto current fork main and updateGhosttyKitchecksum and fork docs.Written for commit 9068ec4. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests