Repository navigation
Expose manual embedded IO for iOS - #53
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 48 minutes and 30 seconds.Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/renderer/Thread.zig">
<violation number="1" location="src/renderer/Thread.zig:202">
P1: `renderNow` executes renderer and mailbox processing on an arbitrary caller thread while the dedicated renderer thread is still active, introducing thread-affinity violations and race conditions.</violation>
</file>
<file name="src/termio/Termio.zig">
<violation number="1" location="src/termio/Termio.zig:448">
P2: Manual backend drops `.selection_scroll` messages, so selection autoscroll ticks are never generated.</violation>
<violation number="2" location="src/termio/Termio.zig:452">
P1: Manual backend ignores `.start_synchronized_output`, removing the watchdog reset for synchronized-output mode.</violation>
</file>
<file name="src/Surface.zig">
<violation number="1" location="src/Surface.zig:2557">
P2: `applyPendingResizeIfNeeded` returns too early and can skip pixel-dimension updates when grid size is unchanged.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| /// This bypasses the xev event loop, which is necessary on iOS where | ||
| /// xev async notifications do not reach the renderer thread. | ||
| pub fn renderNow(self: *Thread) void { | ||
| self.drainMailbox() catch |err| |
There was a problem hiding this comment.
P1: renderNow executes renderer and mailbox processing on an arbitrary caller thread while the dedicated renderer thread is still active, introducing thread-affinity violations and race conditions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/renderer/Thread.zig, line 202:
<comment>`renderNow` executes renderer and mailbox processing on an arbitrary caller thread while the dedicated renderer thread is still active, introducing thread-affinity violations and race conditions.</comment>
<file context>
@@ -195,6 +195,24 @@ pub fn deinit(self: *Thread) void {
+/// This bypasses the xev event loop, which is necessary on iOS where
+/// xev async notifications do not reach the renderer thread.
+pub fn renderNow(self: *Thread) void {
+ self.drainMailbox() catch |err|
+ log.err("renderNow: error draining mailbox err={}", .{err});
+
</file context>
| .jump_to_prompt => |v| self.jumpToPrompt(v) catch |err| { | ||
| log.warn("manual inline jump_to_prompt failed err={}", .{err}); | ||
| }, | ||
| .start_synchronized_output => {}, |
There was a problem hiding this comment.
P1: Manual backend ignores .start_synchronized_output, removing the watchdog reset for synchronized-output mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/termio/Termio.zig, line 452:
<comment>Manual backend ignores `.start_synchronized_output`, removing the watchdog reset for synchronized-output mode.</comment>
<file context>
@@ -394,13 +398,96 @@ pub fn queueMessage(
+ .jump_to_prompt => |v| self.jumpToPrompt(v) catch |err| {
+ log.warn("manual inline jump_to_prompt failed err={}", .{err});
+ },
+ .start_synchronized_output => {},
+ .linefeed_mode => |v| self.manual_linefeed_mode.store(v, .monotonic),
+ .focused => |v| self.focusGained(&td, v) catch |err| {
</file context>
| log.warn("manual inline clear_screen failed err={}", .{err}); | ||
| }, | ||
| .scroll_viewport => |v| self.scrollViewport(v), | ||
| .selection_scroll => {}, |
There was a problem hiding this comment.
P2: Manual backend drops .selection_scroll messages, so selection autoscroll ticks are never generated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/termio/Termio.zig, line 448:
<comment>Manual backend drops `.selection_scroll` messages, so selection autoscroll ticks are never generated.</comment>
<file context>
@@ -394,13 +398,96 @@ pub fn queueMessage(
+ log.warn("manual inline clear_screen failed err={}", .{err});
+ },
+ .scroll_viewport => |v| self.scrollViewport(v),
+ .selection_scroll => {},
+ .jump_to_prompt => |v| self.jumpToPrompt(v) catch |err| {
+ log.warn("manual inline jump_to_prompt failed err={}", .{err});
</file context>
| defer self.renderer_state.mutex.unlock(); | ||
| const t = self.renderer_state.terminal; | ||
|
|
||
| if (t.cols == grid_size.columns and t.rows == grid_size.rows) return; |
There was a problem hiding this comment.
P2: applyPendingResizeIfNeeded returns too early and can skip pixel-dimension updates when grid size is unchanged.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Surface.zig, line 2557:
<comment>`applyPendingResizeIfNeeded` returns too early and can skip pixel-dimension updates when grid size is unchanged.</comment>
<file context>
@@ -2517,6 +2543,32 @@ fn resize(self: *Surface, size: rendererpkg.ScreenSize) !void {
+ defer self.renderer_state.mutex.unlock();
+ const t = self.renderer_state.terminal;
+
+ if (t.cols == grid_size.columns and t.rows == grid_size.rows) return;
+
+ t.resize(
</file context>
| if (t.cols == grid_size.columns and t.rows == grid_size.rows) return; | |
| if (t.cols == grid_size.columns and t.rows == grid_size.rows and t.width_px == grid_size.columns * self.size.cell.width and t.height_px == grid_size.rows * self.size.cell.height) return; |
Greptile SummaryThis PR exposes a manual embedded IO mode for iOS through the C API, allowing a host app to own the PTY/session while Ghostty handles only rendering and input encoding. It wires a new
Confidence Score: 3/5The P1 td.loop = undefined issue requires a comment or assertion before merge; the overall design is sound but has one latent crash risk. One P1 (undefined loop pointer used as a stack local without any guard or assertion) pulls the score below the P1 ceiling of 4. The P2 mode-guard omission on process_output and the silent message drops add marginal risk. Core logic is well-structured. src/termio/Termio.zig — queueMessageManual undefined loop field; src/apprt/embedded.zig — ghostty_surface_process_output missing manual-mode check Important Files Changed
Sequence DiagramsequenceDiagram
participant iOS as iOS Client
participant CAPI as C API (embedded.zig)
participant Surface as Surface.zig
participant Termio as Termio.zig
participant Manual as Manual Backend
participant Renderer as Renderer Thread
Note over iOS,Renderer: Surface creation with manual IO
iOS->>CAPI: ghostty_surface_new(io_mode=MANUAL, io_write_cb)
CAPI->>Surface: init(use_manual_io=true)
Surface->>Manual: Manual.init(write_cb, write_userdata)
Note over iOS,Renderer: Input path (keystrokes to PTY)
iOS->>CAPI: ghostty_surface_text_input(text)
CAPI->>Surface: textInputCallback(text)
Surface->>Surface: completeTextInput encode newline to CR
Surface->>Termio: queueMessage(write_req)
Termio->>Termio: queueMessageManual inline no xev
Termio->>Manual: queueWrite(data, linefeed_mode)
Manual->>iOS: io_write_cb(userdata, ptr, len)
Note over iOS,Renderer: Output path (PTY bytes to terminal)
iOS->>CAPI: ghostty_surface_process_output(bytes)
CAPI->>Termio: processOutput(buf)
Termio->>Termio: acquire renderer_state.mutex
Termio->>Termio: terminal_stream.nextSlice(buf)
Note over iOS,Renderer: Synchronous render cycle
iOS->>CAPI: ghostty_surface_render_now()
CAPI->>Surface: applyPendingResizeIfNeeded()
Surface->>Termio: terminal.resize if size mismatch
CAPI->>Renderer: renderNow()
Renderer->>Renderer: drainMailbox()
Renderer->>Renderer: updateFrame()
Renderer->>Renderer: drawFrame(true)
Reviews (1): Last reviewed commit: "Expose manual embedded IO for iOS" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| var td: ThreadData = .{ | ||
| .alloc = self.alloc, | ||
| .loop = undefined, | ||
| .renderer_state = self.renderer_state, | ||
| .surface_mailbox = self.surface_mailbox, | ||
| .backend = .{ .manual = .{} }, | ||
| .mailbox = &self.mailbox, | ||
| }; |
There was a problem hiding this comment.
td.loop is set to undefined — latent segfault risk
queueMessageManual constructs a stack ThreadData with .loop = undefined. Currently no code path in this function reaches td.loop because Manual.queueWrite ignores td, but any future change that adds td.loop usage inside resize, focusGained, clearScreen, or sizeReport will silently dereference a garbage pointer. A comment documenting this invariant — or an explicit comptime assertion that the manual backend's downstream calls never touch loop — would prevent this footgun.
| surface.preeditCallback(if (len == 0) null else ptr[0..len]); | ||
| } | ||
|
|
||
| /// Process output bytes as if they were read from the PTY. | ||
| export fn ghostty_surface_process_output( | ||
| surface: *Surface, | ||
| ptr: [*]const u8, | ||
| len: usize, | ||
| ) void { | ||
| if (len == 0) return; | ||
| surface.core_surface.io.processOutput(ptr[0..len]); | ||
| } | ||
|
|
||
| /// Returns true if the surface currently has mouse capturing |
There was a problem hiding this comment.
ghostty_surface_process_output lacks a manual-mode guard
processOutput is called unconditionally regardless of io_mode. On an exec-mode surface the PTY read thread is already calling processOutput concurrently; injecting additional bytes from the C API would interleave with PTY data and corrupt the terminal parse state. A guard avoids accidental misuse:
export fn ghostty_surface_process_output(
surface: *Surface,
ptr: [*]const u8,
len: usize,
) void {
if (len == 0) return;
if (surface.io_mode != .manual) return;
surface.core_surface.io.processOutput(ptr[0..len]);
}| .selection_scroll => {}, | ||
| .jump_to_prompt => |v| self.jumpToPrompt(v) catch |err| { | ||
| log.warn("manual inline jump_to_prompt failed err={}", .{err}); | ||
| }, | ||
| .start_synchronized_output => {}, |
There was a problem hiding this comment.
.selection_scroll => {} and .start_synchronized_output => {} are no-ops with no comment. For selection_scroll, auto-scrolling during text selection won't work in manual mode. For start_synchronized_output, the synchronized-output timer that prevents partial-update flicker won't start. If these are intentionally unsupported on iOS (xev timer not available), a short comment clarifying that would help future maintainers distinguish "deliberate omission" from "forgotten case".
1eafdc6 to
22fa801
Compare
1 participant
Summary
Verification
zig build testCMUX_GHOSTTYKIT_NO_PREBUILT=1 ./scripts/ensure-ghosttykit.shfrom cmux parent worktreecmux iOS requirement
This keeps the old iOS app API surface available without taking stale xcframework/build-system changes from the old branch.