Export authoritative terminal grids for embedded clients - #119
Export authoritative terminal grids for embedded clients#119azooz2003-bit wants to merge 24 commits into
Conversation
📝 WalkthroughWalkthroughThe public API now exposes synchronized PTY output sequencing, ticketed render presentation callbacks, and quiescent render-grid snapshots with status and sequence outputs. Renderer backends propagate presentation tickets, while PTY publication and render-grid serialization gain concurrency and opacity handling. ChangesPTY sequencing, render-grid export, and ticketed presentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThis PR exports an authoritative, visual-only terminal grid for embedded clients by introducing a
Confidence Score: 3/5Safe to merge for non-visual uses, but clients rendering the cursor on an unfocused surface will draw the wrong cursor shape until the style export is made focus-aware. The sequence ordering and mutex discipline are solid — src/apprt/embedded.zig — specifically the cursor field capture block where Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant IO as IO Read Thread
participant Termio as Termio.processOutput
participant Lock as renderer_state.mutex
participant Term as VT Parser / Terminal State
participant CB as pty_tee_cb (embedder)
participant Client as Embedded Client
IO->>Termio: processOutput(buf)
Termio->>Lock: lock()
Termio->>Term: processOutputLocked(buf)
Note over Term: Terminal state mutated,<br/>pty_output_seq advanced<br/>(chunk_seq → chunk_seq + len)
Termio->>Lock: unlock()
Termio->>CB: cb(userdata, buf, len, chunk_seq)
Note over CB: chunk_seq = start of this chunk
Client->>Lock: lockDemand() [ghostty_surface_output_sequence]
Lock-->>Client: pty_output_seq (≥ chunk_seq + len)
Client->>Lock: unlock()
Client->>Lock: lock() [ghostty_surface_render_grid_json]
Lock-->>Client: grid snapshot + state_seq
Client->>Lock: unlock()
Note over Client: state_seq ≥ chunk_seq + len<br/>means snapshot covers the chunk
%%{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 IO as IO Read Thread
participant Termio as Termio.processOutput
participant Lock as renderer_state.mutex
participant Term as VT Parser / Terminal State
participant CB as pty_tee_cb (embedder)
participant Client as Embedded Client
IO->>Termio: processOutput(buf)
Termio->>Lock: lock()
Termio->>Term: processOutputLocked(buf)
Note over Term: Terminal state mutated,<br/>pty_output_seq advanced<br/>(chunk_seq → chunk_seq + len)
Termio->>Lock: unlock()
Termio->>CB: cb(userdata, buf, len, chunk_seq)
Note over CB: chunk_seq = start of this chunk
Client->>Lock: lockDemand() [ghostty_surface_output_sequence]
Lock-->>Client: pty_output_seq (≥ chunk_seq + len)
Client->>Lock: unlock()
Client->>Lock: lock() [ghostty_surface_render_grid_json]
Lock-->>Client: grid snapshot + state_seq
Client->>Lock: unlock()
Note over Client: state_seq ≥ chunk_seq + len<br/>means snapshot covers the chunk
Reviews (1): Last reviewed commit: "Merge commit 'b4b6d69c82033e16137266a04b..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| cursor_visible = t.modes.get(.cursor_visible); | ||
| cursor_blinking = t.modes.get(.cursor_blinking); | ||
| cursor_style = s.cursor.cursor_style; |
There was a problem hiding this comment.
cursor_style ignores focus-based override to block_hollow
The PR correctly uses core_surface.focused to export cursor_opacity, mirroring the renderer's alpha logic in generic.zig. However, cursor_style = s.cursor.cursor_style still returns the terminal-mode style even when unfocused — but renderer/cursor.zig line 60 unconditionally overrides the style to .block_hollow for any unfocused surface (if (!opts.focused) return .block_hollow). A client consuming the JSON of an unfocused terminal with a bar or underline cursor will render the wrong shape, because the export says "style": "bar" while the renderer is actually showing a hollow block. The same core_surface.focused check used for opacity should also gate the style field.
| defer self.renderer_state.mutex.unlock(); | ||
| self.processOutputLocked(buf); | ||
| const chunk_seq = self.pty_output_seq; | ||
| self.pty_output_seq +%= @intCast(buf.len); |
There was a problem hiding this comment.
@intCast for a widening usize → u64 cast is unconventional; Zig's @intCast is typically reserved for potentially-truncating casts. @as(u64, buf.len) is the idiomatic way to perform a safe widening conversion.
| self.pty_output_seq +%= @intCast(buf.len); | |
| self.pty_output_seq +%= @as(u64, buf.len); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| try builder.close(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
return; at the end of the function body is redundant — the function already falls off the end of the scope after try builder.close(). Removing it keeps the code consistent with normal Zig style.
| try builder.close(); | |
| return; | |
| } | |
| try builder.close(); | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/apprt/embedded.zig (1)
2480-2538: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate cursor-color resolution logic.
cursor_color_overrideandcursor_text_colorboth independently computecell_foreground/cell_backgroundviacursor_cell_style.fg(...)/.bg(...)and both repeat the identical inverse-aware switch (cell-foreground/cell-background). Any future fix to cursor color resolution (e.g. an inverse-handling bug) has to be applied in two places or they'll silently diverge.♻️ Suggested extraction (illustrative, adapt types as needed)
+fn cursorCellColors( + style: terminal.Style, + cell: *const terminal.Cell, + foreground: terminal.color.RGB, + background: terminal.color.RGB, + palette: *const terminal.color.Palette, + bold_color: ?terminal.Style.BoldColor, +) struct { fg: terminal.color.RGB, bg: terminal.color.RGB } { + return .{ + .fg = style.fg(.{ .default = foreground, .palette = palette, .bold = bold_color }), + .bg = style.bg(cell, palette) orelse background, + }; +}Then both blocks call
cursorCellColors(s.cursor.style, s.cursor.page_cell, foreground, background, palette, bold_color)instead of re-derivingcell_foreground/cell_backgroundinline.🤖 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 `@src/apprt/embedded.zig` around lines 2480 - 2538, Extract the shared cell foreground/background and inverse-aware cursor color selection from the cursor_color_override and cursor_text_color blocks into a helper such as cursorCellColors. Update both blocks to reuse that helper while preserving their existing configured .color overrides and fallback values.
🤖 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 `@src/termio/Termio.zig`:
- Around line 68-78: Make updates to pty_tee_cb and pty_tee_userdata in
ghostty_surface_set_pty_tee_cb safe against concurrent reads by processOutput
after renderer_state.mutex is released. Either synchronize access to both fields
as one pair or enforce and document that the setter is init-only and cannot be
called after output begins; preserve consistent callback/userdata pairing.
---
Nitpick comments:
In `@src/apprt/embedded.zig`:
- Around line 2480-2538: Extract the shared cell foreground/background and
inverse-aware cursor color selection from the cursor_color_override and
cursor_text_color blocks into a helper such as cursorCellColors. Update both
blocks to reuse that helper while preserving their existing configured .color
overrides and fallback values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5de04da5-e9d7-4df1-a334-a3b4db27f43f
📒 Files selected for processing (3)
include/ghostty.hsrc/apprt/embedded.zigsrc/termio/Termio.zig
|
Reconciled this feature with current fork main at 6983c73. The exact GhosttyKit archive is published at https://github.com/manaflow-ai/ghostty/releases/tag/xcframework-6983c7363721e846104e8da76a4bd8a4b58ad6d7-crashsubdir-cmux-crash-v1 and its downloaded SHA-256 is 57baf462bdc03b2da0438b959e7164d3716d38a26c22b2c7a6010268ab77c75c. Local GhosttyKit build and exact cmux tagged rebuild passed. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apprt/embedded.zig (1)
2564-2625: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize the promised cursor width, opacity, and text color.
The snapshot adjusts wide-cell positioning and bakes opacity into
terminal_cursor_color, but the JSON cursor object still exports only row, column, visibility, style, and blinking. This omits the cursor width, explicit opacity, and cursor text color required by the PR objective, preventing faithful replay—especially on wide cells.🤖 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 `@src/apprt/embedded.zig` around lines 2564 - 2625, Update the cursor JSON serialization in the surrounding snapshot/export logic to include the promised cursor width, explicit opacity, and cursor text color alongside the existing row, column, visibility, style, and blinking fields. Reuse the computed cursor positioning, cursor_alpha, and resolved cursor color values from this flow so wide-cell placement and opacity are preserved for faithful replay.
🧹 Nitpick comments (1)
src/apprt/embedded.zig (1)
431-445: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd C-header parity coverage for
RenderGridStatus.The adjacent presentation enum is checked, but this new public enum is not. Add the equivalent ABI test to prevent silent name/value drift.
Proposed test
pub const RenderGridStatus = enum(c_int) { success = 0, retryable_not_quiescent = 1, failure = 2, }; + +test "ghostty.h render grid status" { + try renderer.lib.checkGhosttyHEnum( + RenderGridStatus, + "GHOSTTY_RENDER_GRID_", + ); +}🤖 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 `@src/apprt/embedded.zig` around lines 431 - 445, Add a C-header parity test for the public RenderGridStatus enum, alongside the existing “ghostty.h render presentation status” test. Use renderer.lib.checkGhosttyHEnum with RenderGridStatus and the appropriate “GHOSTTY_RENDER_GRID_” prefix, preserving the enum’s declared names and values.
🤖 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 `@include/ghostty.h`:
- Around line 504-509: Update the documentation for
ghostty_render_presentation_cb to describe the callback as reporting completion
of the render ticket’s terminal outcome, not necessarily host-layer
presentation. Clarify that only ghostty_render_presentation_status_e.PRESENTED
confirms presentation, while WRONG_SIZE_DISCARDED and BACKEND_FAILED represent
terminal outcomes that do not reach the presentation boundary.
- Around line 1279-1283: Define a separate, unique non-null ownership token for
conditional PTY tee clearing instead of using nullable callback userdata. Update
the declaration in include/ghostty.h#1279-1283 and the exported implementation
signature in src/apprt/embedded.zig#3051-3055 to accept and document that token,
then update Termio’s ownership comparison in src/termio/Termio.zig#82-94 to
compare the token directly while preserving support for nullable callback
userdata.
In `@src/renderer/generic.zig`:
- Around line 1547-1552: Update drawFrameWithPresentationTicket and its
renderNowWithTicket call path so any non-null presentation_ticket is forced
through the completion/synchronized path, even when sync is false or
needs_redraw is false. Ensure the ticket is completed when rebuilding is
skipped, while retaining the existing normal behavior for null tickets.
In `@src/renderer/Metal.zig`:
- Around line 111-114: Update Metal renderer teardown in prepareDeinit to
synchronously clear the presentation callback and userdata on the macOS path as
well as iOS, while keeping host detachment iOS-specific. Ensure clearing
completes on the main queue before teardown returns so retained layers or queued
blocks cannot invoke stale embedder userdata.
In `@src/termio/Termio.zig`:
- Around line 44-52: Update PtyOutputPublication.finish so it does not invoke
the PTY callback while self.mutex is held: under the lock, capture the callback,
userdata, and callback arguments while preserving the publication lifetime
fence, then unlock before invoking embedder code and complete the corresponding
in-flight handoff afterward. Ensure re-entry through processOutput,
setPtyTeeCallback, clearPtyTeeCallbackIfUserdata, or ptyOutputSequence cannot
deadlock.
---
Outside diff comments:
In `@src/apprt/embedded.zig`:
- Around line 2564-2625: Update the cursor JSON serialization in the surrounding
snapshot/export logic to include the promised cursor width, explicit opacity,
and cursor text color alongside the existing row, column, visibility, style, and
blinking fields. Reuse the computed cursor positioning, cursor_alpha, and
resolved cursor color values from this flow so wide-cell placement and opacity
are preserved for faithful replay.
---
Nitpick comments:
In `@src/apprt/embedded.zig`:
- Around line 431-445: Add a C-header parity test for the public
RenderGridStatus enum, alongside the existing “ghostty.h render presentation
status” test. Use renderer.lib.checkGhosttyHEnum with RenderGridStatus and the
appropriate “GHOSTTY_RENDER_GRID_” prefix, preserving the enum’s declared names
and values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c657c17-f301-4963-bc09-462f3822d192
📒 Files selected for processing (12)
include/ghostty.hsrc/apprt/embedded.zigsrc/renderer.zigsrc/renderer/Metal.zigsrc/renderer/OpenGL.zigsrc/renderer/Thread.zigsrc/renderer/generic.zigsrc/renderer/metal/Frame.zigsrc/renderer/metal/IOSurfaceLayer.zigsrc/renderer/presentation.zigsrc/terminal/stream.zigsrc/termio/Termio.zig
…e-grid # Conflicts: # include/ghostty.h # src/apprt/embedded.zig
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
1 participant
Summary
Verification
zig build -Demit-macos-app=false29392177968passedParent experiment for manaflow-ai/cmux#7160.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Exports an authoritative, visual-only terminal render grid with a visual state watermark and exact presentation tickets so embedded clients stay pixel- and frame-accurate.
New Features
state_seq; snapshot is gated and returnsretryable_not_quiescentuntil output/parsing completes or synchronized‑output ends.ghostty_render_presentation_cb; ticketed renders viaghostty_surface_render_now_with_ticket;ghostty_surface_invalidate_render_presentation_throughto fence stale frames; tickets are forced through presentation even with no new damage; final statuses:presented,wrong_size_discarded,backend_failed.ghostty_surface_render_grid_json(surface_id, id_len, scrollback_lines, out_state_seq*, out_status*).start_seqfor each chunk; publication is serialized and reentrant‑safe; accessorghostty_surface_pty_output_sequenceexposed for synchronized coordination.Migration
render_presentation_cbonghostty_surface_config_sand use ticketed render on iOS; handle statusespresented,wrong_size_discarded,backend_failed; call invalidate to drop stale tickets.(userdata, bytes, len, start_seq); optionally clear withghostty_surface_clear_pty_tee_cb_if_userdata.state_seqtoghostty_surface_render_grid_json; readout_state_seq/out_statusand retry onretryable_not_quiescent, or useghostty_surface_pty_output_sequencewhen needed.Written for commit 51d20b2. Summary will update on new commits.
Summary by CodeRabbit
New Features
API Updates
Tests