Skip to content

feat: add tmux control mode embedder API#16

Open
jamesainslie wants to merge 1 commit intomanaflow-ai:mainfrom
jamesainslie:tmux-embedder-api
Open

feat: add tmux control mode embedder API#16
jamesainslie wants to merge 1 commit intomanaflow-ai:mainfrom
jamesainslie:tmux-embedder-api

Conversation

@jamesainslie
Copy link

@jamesainslie jamesainslie commented Mar 20, 2026

Summary

  • Add C API and internal plumbing for tmux control mode events to reach the embedder (cmux)
  • The Ghostty Viewer state machine already parses the tmux protocol; this change surfaces those events across the Zig→C→Swift boundary
  • Add GHOSTTY_ACTION_TMUX_CONTROL action with event types for enter/exit/windows/pane_output/layout changes

Changes

  • viewer.zig: Add .pane_output Action variant, emit for tracked panes alongside internal Terminal feeding
  • action.zig: Add TmuxControl struct with Event enum (10 event types) and C extern struct for ABI
  • surface.zig: Add tmux_control surface message variant with WriteReq for safe cross-thread data transfer
  • stream_handler.zig: Wire .exit, .pane_output, .windows through surface messages; add JSON serialization for windows payload via std.json.Stringify.valueAlloc
  • layout.zig: Add jsonStringify for recursive Layout tree serialization (flattened JSON objects)
  • Surface.zig: Handle tmux_control message, convert to performAction dispatch
  • ghostty.h: Add ghostty_tmux_event_e, ghostty_action_tmux_control_s, GHOSTTY_ACTION_TMUX_CONTROL

Test plan

  • All existing tmux viewer tests pass (175/175)
  • ghostty.h Action.Key enum sync test passes
  • ghostty.h TmuxControl.Event enum sync test passes
  • CValue union size assertion passes (24 bytes on 64-bit)
  • Full zig build -Dapp-runtime=none succeeds
  • Updated existing test to verify .pane_output action emission for tracked panes
  • Verified untracked pane output does NOT emit .pane_output action

Summary by cubic

Adds a new embedder-facing API to deliver tmux control mode events via a C action, enabling native window/pane management and streamed pane output. Wires Viewer events through Zig→C and serializes windows/layout state to JSON.

  • New Features

    • New GHOSTTY_ACTION_TMUX_CONTROL with ghostty_tmux_event_e and payload (event, id, data, data_len) in ghostty.h.
    • Emits pane_output for tracked panes; Viewer still feeds its internal Terminal for canonical state.
    • Forwards exit, pane_output, and windows_changed from Viewer through a tmux_control surface message to performAction.
    • Windows state serialized to JSON (includes session_id, tmux_version, and windows[] with layout), using custom Layout.jsonStringify.
  • Migration

    • Handle GHOSTTY_ACTION_TMUX_CONTROL in the embedder and switch on 10 events (enter/exit, windows_changed, pane_output, layout_change, window add/close/renamed, session changed/renamed).
    • Interpret id as pane_id for pane_output, window_id for window/layout events; ignore for enter/exit/session events.
    • data contents:
      • pane_output: raw bytes to feed the pane’s terminal surface.
      • windows_changed: JSON payload like {"session_id":N,"tmux_version":"X.Y","windows":[{"id":N,"width":N,"height":N,"layout":{...}}]} where layout objects are flattened (pane: {"width":N,"height":N,"x":N,"y":N,"pane":N}; splits: {"...","horizontal":[...]} | {"...","vertical":[...]}).

Written for commit d0ac04e. Summary will update on new commits.

Summary by CodeRabbit

  • New Features
    • Added tmux control event action type to public API.
    • Implemented handling for tmux control events including enter, exit, windows changed, pane output, layout changes, and window/session modifications.
    • Added layout serialization to JSON format.

Add C API and internal plumbing for tmux control mode events to reach
the embedder (cmux). The Ghostty Viewer state machine already parses
the tmux protocol; this change surfaces those events across the
Zig→C→Swift boundary.

Changes:
- viewer.zig: Add .pane_output Action variant, emit for tracked panes
- action.zig: Add TmuxControl struct with Event enum and C extern type
- surface.zig: Add tmux_control surface message with WriteReq data
- stream_handler.zig: Wire .exit, .pane_output, .windows through surface
  messages; add JSON serialization for windows payload
- layout.zig: Add jsonStringify for recursive Layout tree serialization
- Surface.zig: Handle tmux_control message, convert to performAction
- ghostty.h: Add ghostty_tmux_event_e, ghostty_action_tmux_control_s,
  GHOSTTY_ACTION_TMUX_CONTROL
@coderabbitai
Copy link

coderabbitai bot commented Mar 20, 2026

📝 Walkthrough

Walkthrough

This pull request introduces comprehensive tmux control event support by adding new public API types for action payloads, implementing message handling and forwarding through the Surface layer, defining corresponding action and message types, adding JSON serialization for layout data, and updating the terminal viewer and stream handler to dispatch various tmux events (exit, windows_changed, pane_output) as surface messages.

Changes

Cohort / File(s) Summary
Public API Types
include/ghostty.h
Added new ghostty_tmux_event_e enum and ghostty_action_tmux_control_s struct; extended ghostty_action_tag_e enum with GHOSTTY_ACTION_TMUX_CONTROL tag and ghostty_action_u union with tmux_control member.
Action & Message Types
src/apprt/action.zig, src/apprt/surface.zig
Added TmuxControl struct with nested Event enum and C ABI binding in action module; added corresponding Message.TmuxControlMsg and Message.tmux_control union variant in surface module.
Message Handling
src/Surface.zig
Added handling for .tmux_control case that defers data deinitialization, forwards action to rt_app.performAction, and propagates errors.
Serialization
src/terminal/tmux/layout.zig
Added jsonStringify method for custom JSON serialization of layout tree structure with recursive child serialization.
Event Processing & Dispatch
src/terminal/tmux/viewer.zig, src/termio/stream_handler.zig
Extended viewer with PaneOutput type and .pane_output action variant; updated .output notification to forward pane output conditionally. Updated stream_handler to handle .exit, .windows, and .pane_output actions, including JSON serialization of window state via new serializeTmuxWindows helper.

Sequence Diagram(s)

sequenceDiagram
    participant TmuxViewer as Tmux Viewer
    participant StreamHandler as Stream Handler
    participant LayoutSer as Layout Serializer
    participant Surface
    participant RTApp as RT App

    Note over TmuxViewer,RTApp: Tmux Event Flow

    TmuxViewer->>StreamHandler: Viewer.Action (.exit / .windows / .pane_output)
    
    alt .windows action
        StreamHandler->>LayoutSer: layout.jsonStringify()
        LayoutSer->>LayoutSer: Recursively serialize tree
        LayoutSer-->>StreamHandler: JSON bytes
        StreamHandler->>StreamHandler: serializeTmuxWindows()
    end
    
    StreamHandler->>Surface: surface.WriteReq(tmux_control.<br/>event, id, data)
    Surface->>Surface: handleMessage(.tmux_control)
    Surface->>RTApp: performAction({<br/>event, id, data})
    RTApp-->>Surface: Action result
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Hark! New tmux events hop our way,
With actions, messages in array,
Layouts serialize with grace so deep,
As panes and windows our vigil keep—
Control flows swift from view to stream! 🌳✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: add tmux control mode embedder API' accurately summarizes the main change: exposing tmux control-mode events to embedders via a new C API.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can validate your CodeRabbit configuration file in your editor.

If your editor has YAML language server, you can enable auto-completion and validation by adding # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json at the top of your CodeRabbit configuration file.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 7 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/termio/stream_handler.zig">

<violation number="1" location="src/termio/stream_handler.zig:448">
P2: Caller-owned JSON buffer from `serializeTmuxWindows` is never freed after `WriteReq.init`, causing repeated leaks on tmux windows updates.</violation>
</file>

Since this is your first cubic review, here's how it works:

  • cubic automatically reviews your code and comments on bugs and improvements
  • Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
  • Add one-off context when rerunning by tagging @cubic-dev-ai with guidance or docs links (including llms.txt)
  • Ask questions if you need clarification on any suggestion

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

.windows => {
// TODO
.windows => |windows| {
const json = serializeTmuxWindows(
Copy link

@cubic-dev-ai cubic-dev-ai bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Caller-owned JSON buffer from serializeTmuxWindows is never freed after WriteReq.init, causing repeated leaks on tmux windows updates.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/termio/stream_handler.zig, line 448:

<comment>Caller-owned JSON buffer from `serializeTmuxWindows` is never freed after `WriteReq.init`, causing repeated leaks on tmux windows updates.</comment>

<file context>
@@ -443,8 +444,37 @@ pub const StreamHandler = struct {
-                        .windows => {
-                            // TODO
+                        .windows => |windows| {
+                            const json = serializeTmuxWindows(
+                                self.alloc,
+                                viewer,
</file context>
Fix with Cubic

@greptile-apps
Copy link

greptile-apps bot commented Mar 20, 2026

Greptile Summary

This PR surfaces tmux control mode events from the Ghostty Viewer state machine across the Zig→C→Swift boundary, enabling embedders to manage native windows/panes that mirror the tmux server state. It adds a GHOSTTY_ACTION_TMUX_CONTROL action with a 10-value event enum, wires three viewer actions (exit, windows, pane_output) through a new tmux_control surface message, and adds JSON serialization for the windows payload.

Key points:

  • The architecture is sound: WriteReq is used correctly for safe cross-thread data transfer, the C ABI struct layout is correct (24 bytes on 64-bit), and the defer v.data.deinit() / performAction ordering in Surface.zig keeps data valid for the C callback.
  • Memory leak in the .windows handler of stream_handler.zig: the JSON string produced by serializeTmuxWindows is allocated on the heap, then WriteReq.init copies its contents (into a small buffer or a duplicate allocation), and the original allocation is never freed — on every windows_changed event.
  • Six of the ten declared C API event types (layout_change, window_add, window_close, window_renamed, session_changed, session_renamed) are not yet emitted by any code path; embedder consumers registering for these events will never receive them.
  • @intCast(out.pane_id) from usize to u32 is unsafe in ReleaseFast builds; a bounded pane ID assertion or a wider field type would be safer.

Confidence Score: 3/5

  • Safe to merge after fixing the memory leak in the .windows handler; the rest of the plumbing is architecturally sound.
  • Score reduced primarily due to the confirmed memory leak in stream_handler.zig (the JSON allocation from serializeTmuxWindows is never freed), which will grow unbounded with every windows_changed event. The six unimplemented-but-declared C API event types and the unsafe @intCast are secondary concerns that do not block functionality today but warrant attention.
  • src/termio/stream_handler.zig — the .windows event handler leaks the JSON allocation on every invocation.

Important Files Changed

Filename Overview
src/termio/stream_handler.zig Wires .exit, .windows, and .pane_output viewer actions to surface messages; contains a memory leak where the heap-allocated JSON string from serializeTmuxWindows is never freed after WriteReq.init copies it.
src/apprt/action.zig Adds TmuxControl struct with Event enum, C extern struct for ABI, and cval() conversion; 6 of 10 declared event types are never emitted by any current code path.
src/apprt/surface.zig Adds tmux_control message variant with TmuxControlMsg struct using WriteReq for safe cross-thread data transfer; implementation looks correct.
src/Surface.zig Handles tmux_control surface message by calling performAction and deferring deinit on the WriteReq; lifetime of data passed to the C callback is correctly valid during the call.
src/terminal/tmux/viewer.zig Adds pane_output action emission for tracked panes; @intCast(out.pane_id) from usize to u32 is unsafe in ReleaseFast builds and repeated in stream_handler.zig.
src/terminal/tmux/layout.zig Adds jsonStringify for recursive Layout tree serialization; implementation correctly uses jw writer API and handles all three content variants.
include/ghostty.h Adds ghostty_tmux_event_e, ghostty_action_tmux_control_s, and GHOSTTY_ACTION_TMUX_CONTROL; struct layout (4+4+8+8 = 24 bytes on 64-bit) matches the Zig C extern struct.

Sequence Diagram

sequenceDiagram
    participant tmux as tmux server
    participant SH as StreamHandler (I/O thread)
    participant Viewer as tmux Viewer
    participant Mailbox as App Mailbox
    participant Surf as Surface (app thread)
    participant Embedder as Embedder (C/Swift)

    tmux->>SH: DCS control mode data
    SH->>Viewer: next(.{ .tmux = notification })
    Viewer-->>SH: []Action

    alt exit action
        SH->>Mailbox: tmux_control { event=exit }
    else windows action
        SH->>SH: serializeTmuxWindows() produces json heap alloc
        SH->>SH: WriteReq.init copies json data
        SH->>Mailbox: tmux_control { event=windows_changed, data=WriteReq }
    else pane_output action
        SH->>SH: WriteReq.init copies pane data
        SH->>Mailbox: tmux_control { event=pane_output, id=pane_id, data=WriteReq }
    end

    Mailbox->>Surf: handleMessage(.tmux_control)
    Surf->>Surf: defer v.data.deinit()
    Surf->>Embedder: performAction GHOSTTY_ACTION_TMUX_CONTROL
    Note over Surf,Embedder: data pointer valid during callback and freed after return
Loading

Last reviewed commit: "feat: add tmux contr..."

Comment on lines +447 to +465
.windows => |windows| {
const json = serializeTmuxWindows(
self.alloc,
viewer,
windows,
) catch |err| {
log.warn("failed to serialize tmux windows: {}", .{err});
break :tmux;
};
self.surfaceMessageWriter(.{
.tmux_control = .{
.event = .windows_changed,
.data = try apprt.surface.Message.WriteReq.init(
self.alloc,
json,
),
},
});
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Memory leak: json allocation never freed

serializeTmuxWindows returns a heap-allocated slice owned by self.alloc (via std.json.Stringify.valueAlloc). WriteReq.init always copies the data — either into its small inline buffer or via alloc.dupe — it never takes ownership of the original slice. As a result, the original json allocation is leaked every time a windows_changed event fires.

Additionally, if WriteReq.init returns error.OutOfMemory, the try propagates the error upward while json is still allocated, and there is no errdefer to clean it up.

The fix is to always free json after WriteReq.init returns:

.windows => |windows| {
    const json = serializeTmuxWindows(
        self.alloc,
        viewer,
        windows,
    ) catch |err| {
        log.warn("failed to serialize tmux windows: {}", .{err});
        break :tmux;
    };
    defer self.alloc.free(json);
    self.surfaceMessageWriter(.{
        .tmux_control = .{
            .event = .windows_changed,
            .data = try apprt.surface.Message.WriteReq.init(
                self.alloc,
                json,
            ),
        },
    });
},

Comment on lines +1030 to +1036
test "ghostty.h TmuxControl.Event" {
try lib.checkGhosttyHEnum(Event, "GHOSTTY_TMUX_");
}
};

// Sync with: ghostty_action_tmux_control_s
pub const C = extern struct {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Six C API event types are defined but never emitted

GHOSTTY_TMUX_LAYOUT_CHANGE, GHOSTTY_TMUX_WINDOW_ADD, GHOSTTY_TMUX_WINDOW_CLOSE, GHOSTTY_TMUX_WINDOW_RENAMED, GHOSTTY_TMUX_SESSION_CHANGED, and GHOSTTY_TMUX_SESSION_RENAMED appear in ghostty_tmux_event_e and in the Event enum here, but no code path in stream_handler.zig currently emits them (only exit, windows_changed, and pane_output are wired). An embedder author who writes a handler for e.g. GHOSTTY_TMUX_LAYOUT_CHANGE will never receive it.

If these are intentionally reserved for a future PR, it would help to add a comment noting their unimplemented status (e.g., // Not yet emitted – reserved for future use) so consumers aren't silently confused.

Comment on lines +489 to +495
if (self.panes.contains(out.pane_id)) {
var arena = self.action_arena.promote(self.alloc);
defer self.action_arena = arena.state;
actions.append(
arena.allocator(),
.{ .pane_output = .{ .pane_id = out.pane_id, .data = out.data } },
) catch return self.defunct();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 @intCast(out.pane_id) silently truncates large pane IDs in ReleaseFast builds

out.pane_id is usize (64-bit on 64-bit targets) while TmuxControlMsg.id is u32. @intCast panics on overflow in debug/safe builds, but in ReleaseFast / ReleaseSmall mode the behaviour is undefined. While tmux pane IDs rarely exceed 32-bit range in practice, the cast is also applied downstream in stream_handler.zig at line 471 (@intCast(out.pane_id)), making the same assumption in two places.

Consider asserting or saturating the cast, or widening id to usize/u64 to match the viewer's type.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
src/terminal/tmux/layout.zig (1)

31-66: Add a schema-locking test for Layout.jsonStringify.

Line [33] introduces a public serializer used across layers; please add a golden-style test that asserts exact keys and nesting (pane vs horizontal/vertical) to prevent accidental contract drift.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/terminal/tmux/layout.zig` around lines 31 - 66, Add a golden-style,
schema-locking test for Layout.jsonStringify that verifies the exact JSON keys
and nesting to prevent contract drift: create tests that call
Layout.jsonStringify (using the same jw writer) for at least three cases—a .pane
layout, a .horizontal layout with child layouts, and a .vertical layout with
child layouts—and assert the serialized output exactly matches a stored golden
JSON string (or parse and assert exact object shape and key sets/order:
"width","height","x","y" plus exactly one of "pane" or "horizontal"/"vertical"
with correct nested arrays). Ensure the test fails on any extra/missing keys or
structural changes and references Layout.jsonStringify and jw in the test so it
exercises the same code path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/apprt/action.zig`:
- Around line 1011-1040: The public ABI fixes TmuxControl IDs to u32 which
forces downcasts on 64-bit hosts; change the id field types to u64: update the
top-level id default (currently "id: u32 = 0") to "id: u64 = 0" and the extern C
struct (pub const C) id from u32 to u64, and also update the corresponding C
definition (ghostty_action_tmux_control_s / ghostty.h) to use uint64_t so the
Zig Event/C ABI matches 64-bit internal pane/window ids without implicit
truncation.
- Around line 343-346: The enum variant .tmux_control was added to Action.Key
but performAction's comptime exhaustive switch in function performAction does
not handle it, which will break compilation; update the exhaustive switch to
handle the new variant by adding a case for .tmux_control that either implements
the intended behavior (using the TmuxControl payload) or forwards to the
existing unimplemented handler, or include .tmux_control in the catch-all
unimplemented branch at the end of the switch so the switch remains exhaustive;
look for the performAction function and the Action.Key switch and add the
.tmux_control arm (or route it to the unimplemented logic) accordingly.

In `@src/terminal/tmux/viewer.zig`:
- Around line 486-495: The code forwards pane_output for any pane present in
self.panes, but syncLayouts() can insert panes before their
capture-pane/bootstrap completes; change the condition that appends .pane_output
so it only forwards output for panes whose bootstrap has completed (e.g., check
a bootstrap-complete flag or set, such as a Pane.bootstrap_complete boolean or a
separate self.bootstrapped set) instead of just
self.panes.contains(out.pane_id); update the condition around actions.append in
the block with self.action_arena/promote so it requires the bootstrap check (and
ensure any new bookkeeping is updated when capture-pane/bootstrap finishes).

In `@src/termio/stream_handler.zig`:
- Around line 447-464: The serialized tmux windows JSON returned by
serializeTmuxWindows is caller-owned and currently leaked after calling
apprt.surface.Message.WriteReq.init; update the .windows branch so that after
successfully creating the WriteReq (apprt.surface.Message.WriteReq.init) you
free the temporary JSON buffer (the value returned by serializeTmuxWindows)
using the allocator (or use a defer that frees json after init succeeds),
ensuring you free json regardless of early returns or errors and before calling
self.surfaceMessageWriter with the .tmux_control .windows_changed message.
- Around line 431-435: The embedder never receives real tmux control-mode
enter/exit because those events are consumed inside dcsCommand instead of being
forwarded; update dcsCommand (or its callers) so that when it detects
control-mode .enter or .exit it invokes self.surfaceMessageWriter with
{.tmux_control = .{ .event = .enter }} and {.tmux_control = .{ .event = .exit }}
(or returns a sentinel that viewer.next() propagates) rather than swallowing the
event, ensuring the same tmux_control enter/exit notifications produced in the
viewer.next()/viewer.next() defunct path are emitted for clean lifecycle
transitions.

---

Nitpick comments:
In `@src/terminal/tmux/layout.zig`:
- Around line 31-66: Add a golden-style, schema-locking test for
Layout.jsonStringify that verifies the exact JSON keys and nesting to prevent
contract drift: create tests that call Layout.jsonStringify (using the same jw
writer) for at least three cases—a .pane layout, a .horizontal layout with child
layouts, and a .vertical layout with child layouts—and assert the serialized
output exactly matches a stored golden JSON string (or parse and assert exact
object shape and key sets/order: "width","height","x","y" plus exactly one of
"pane" or "horizontal"/"vertical" with correct nested arrays). Ensure the test
fails on any extra/missing keys or structural changes and references
Layout.jsonStringify and jw in the test so it exercises the same code path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fe0d64d6-6e3e-4810-aa78-006ba3ccdd8f

📥 Commits

Reviewing files that changed from the base of the PR and between b9989c1 and d0ac04e.

📒 Files selected for processing (7)
  • include/ghostty.h
  • src/Surface.zig
  • src/apprt/action.zig
  • src/apprt/surface.zig
  • src/terminal/tmux/layout.zig
  • src/terminal/tmux/viewer.zig
  • src/termio/stream_handler.zig

Comment on lines +343 to +346
/// A tmux control mode event from the Viewer. The embedder uses
/// this to create/destroy native windows and panes that mirror
/// the tmux server state.
tmux_control: TmuxControl,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Action.Key excerpt ---'
sed -n '343,418p' src/apprt/action.zig

printf '\n%s\n' '--- GTK performAction excerpt ---'
sed -n '661,768p' src/apprt/gtk/class/application.zig

printf '\n%s\n' '--- tmux_control references under src/apprt ---'
rg -n '\.tmux_control\b|tmux_control' src/apprt

Repository: manaflow-ai/ghostty

Length of output: 6315


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Get the full performAction function
printf '%s\n' '--- Full performAction function ---'
rg -A 200 'pub fn performAction' src/apprt/gtk/class/application.zig | head -250

# Check if .tmux_control is handled anywhere in the switch
printf '\n%s\n' '--- Check for tmux_control handling in application.zig ---'
grep -n 'tmux_control' src/apprt/gtk/class/application.zig || echo "No tmux_control found"

# Check the exact line count of the file
printf '\n%s\n' '--- Line count of application.zig ---'
wc -l src/apprt/gtk/class/application.zig

Repository: manaflow-ai/ghostty

Length of output: 7328


Add .tmux_control to the exhaustive switch in src/apprt/gtk/class/application.zig:661-768 before merging.

The performAction function uses a comptime exhaustive switch. Adding the .tmux_control enum variant to Action.Key without handling it in this switch will cause a compile error. Either add an explicit handler or include it in the unimplemented catch-all case at the end of the switch.

The switch currently lists unimplemented cases like .secure_input, .close_all_windows, etc., but .tmux_control is missing entirely.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/apprt/action.zig` around lines 343 - 346, The enum variant .tmux_control
was added to Action.Key but performAction's comptime exhaustive switch in
function performAction does not handle it, which will break compilation; update
the exhaustive switch to handle the new variant by adding a case for
.tmux_control that either implements the intended behavior (using the
TmuxControl payload) or forwards to the existing unimplemented handler, or
include .tmux_control in the catch-all unimplemented branch at the end of the
switch so the switch remains exhaustive; look for the performAction function and
the Action.Key switch and add the .tmux_control arm (or route it to the
unimplemented logic) accordingly.

Comment on lines +1011 to +1040
event: Event,
/// Contextual ID: pane_id for pane_output, window_id for window_*
/// and layout_change events. Unused for enter/exit/session events.
id: u32 = 0,
data: []const u8 = &.{},

pub const Event = enum(c_int) {
enter = 0,
exit = 1,
windows_changed = 2,
pane_output = 3,
layout_change = 4,
window_add = 5,
window_close = 6,
window_renamed = 7,
session_changed = 8,
session_renamed = 9,

// Sync with: ghostty_tmux_event_e
test "ghostty.h TmuxControl.Event" {
try lib.checkGhosttyHEnum(Event, "GHOSTTY_TMUX_");
}
};

// Sync with: ghostty_action_tmux_control_s
pub const C = extern struct {
event: Event,
id: u32,
data: [*]const u8,
data_len: usize,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Widen TmuxControl.id before freezing this ABI.

Internal tmux pane/window ids are still usize, but the new public payload fixes them to u32. That bakes a downcast into every emitter on 64-bit builds. Since this C ABI is brand new, prefer u64/uint64_t here (and in ghostty_action_tmux_control_s) before consumers depend on the narrower shape.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/apprt/action.zig` around lines 1011 - 1040, The public ABI fixes
TmuxControl IDs to u32 which forces downcasts on 64-bit hosts; change the id
field types to u64: update the top-level id default (currently "id: u32 = 0") to
"id: u64 = 0" and the extern C struct (pub const C) id from u32 to u64, and also
update the corresponding C definition (ghostty_action_tmux_control_s /
ghostty.h) to use uint64_t so the Zig Event/C ABI matches 64-bit internal
pane/window ids without implicit truncation.

Comment on lines +486 to +495
// Also emit a pane_output action for the embedder, but only
// for tracked panes. Untracked pane output is silently dropped
// by receivedOutput above and should not be forwarded.
if (self.panes.contains(out.pane_id)) {
var arena = self.action_arena.promote(self.alloc);
defer self.action_arena = arena.state;
actions.append(
arena.allocator(),
.{ .pane_output = .{ .pane_id = out.pane_id, .data = out.data } },
) catch return self.defunct();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't forward pane_output until pane bootstrap completes.

self.panes.contains(out.pane_id) only means the pane exists in the map. syncLayouts() inserts new panes before the capture-pane/bootstrap commands finish, so live %output can be forwarded ahead of the initial snapshot. That will make embedders render duplicated or out-of-order content on busy panes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/terminal/tmux/viewer.zig` around lines 486 - 495, The code forwards
pane_output for any pane present in self.panes, but syncLayouts() can insert
panes before their capture-pane/bootstrap completes; change the condition that
appends .pane_output so it only forwards output for panes whose bootstrap has
completed (e.g., check a bootstrap-complete flag or set, such as a
Pane.bootstrap_complete boolean or a separate self.bootstrapped set) instead of
just self.panes.contains(out.pane_id); update the condition around
actions.append in the block with self.action_arena/promote so it requires the
bootstrap check (and ensure any new bookkeeping is updated when
capture-pane/bootstrap finishes).

Comment on lines +431 to +435
self.surfaceMessageWriter(.{
.tmux_control = .{
.event = .exit,
},
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Normal tmux enter/exit still never reaches the embedder.

Lines 431-435 only run when viewer.next() returns .exit from the defunct/error path. The real control-mode .enter/.exit notifications are still consumed earlier in dcsCommand, so embedders miss the clean lifecycle transitions this API now advertises.

Possible fix
                     .enter => {
                         // Setup our viewer state
                         assert(self.tmux_viewer == null);
                         const viewer = try self.alloc.create(terminal.tmux.Viewer);
                         errdefer self.alloc.destroy(viewer);
                         viewer.* = try .init(self.alloc);
                         errdefer viewer.deinit();
                         self.tmux_viewer = viewer;
+                        self.surfaceMessageWriter(.{
+                            .tmux_control = .{ .event = .enter },
+                        });
                         break :tmux;
                     },

                     .exit => {
+                        self.surfaceMessageWriter(.{
+                            .tmux_control = .{ .event = .exit },
+                        });
                         // Free our viewer state if we have one
                         if (self.tmux_viewer) |viewer| {
                             viewer.deinit();
                             self.alloc.destroy(viewer);
                             self.tmux_viewer = null;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/termio/stream_handler.zig` around lines 431 - 435, The embedder never
receives real tmux control-mode enter/exit because those events are consumed
inside dcsCommand instead of being forwarded; update dcsCommand (or its callers)
so that when it detects control-mode .enter or .exit it invokes
self.surfaceMessageWriter with {.tmux_control = .{ .event = .enter }} and
{.tmux_control = .{ .event = .exit }} (or returns a sentinel that viewer.next()
propagates) rather than swallowing the event, ensuring the same tmux_control
enter/exit notifications produced in the viewer.next()/viewer.next() defunct
path are emitted for clean lifecycle transitions.

Comment on lines +447 to +464
.windows => |windows| {
const json = serializeTmuxWindows(
self.alloc,
viewer,
windows,
) catch |err| {
log.warn("failed to serialize tmux windows: {}", .{err});
break :tmux;
};
self.surfaceMessageWriter(.{
.tmux_control = .{
.event = .windows_changed,
.data = try apprt.surface.Message.WriteReq.init(
self.alloc,
json,
),
},
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Free the temporary windows JSON after WriteReq.init().

The helper at Lines 1583-1585 returns caller-owned memory, and WriteReq.init() copies those bytes into message-owned storage. As written, every .windows_changed event leaks the serialized JSON buffer.

Possible fix
                         .windows => |windows| {
                             const json = serializeTmuxWindows(
                                 self.alloc,
                                 viewer,
                                 windows,
                             ) catch |err| {
                                 log.warn("failed to serialize tmux windows: {}", .{err});
                                 break :tmux;
                             };
+                            defer self.alloc.free(json);
                             self.surfaceMessageWriter(.{
                                 .tmux_control = .{
                                     .event = .windows_changed,
                                     .data = try apprt.surface.Message.WriteReq.init(
                                         self.alloc,
                                         json,
📝 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.

Suggested change
.windows => |windows| {
const json = serializeTmuxWindows(
self.alloc,
viewer,
windows,
) catch |err| {
log.warn("failed to serialize tmux windows: {}", .{err});
break :tmux;
};
self.surfaceMessageWriter(.{
.tmux_control = .{
.event = .windows_changed,
.data = try apprt.surface.Message.WriteReq.init(
self.alloc,
json,
),
},
});
.windows => |windows| {
const json = serializeTmuxWindows(
self.alloc,
viewer,
windows,
) catch |err| {
log.warn("failed to serialize tmux windows: {}", .{err});
break :tmux;
};
defer self.alloc.free(json);
self.surfaceMessageWriter(.{
.tmux_control = .{
.event = .windows_changed,
.data = try apprt.surface.Message.WriteReq.init(
self.alloc,
json,
),
},
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/termio/stream_handler.zig` around lines 447 - 464, The serialized tmux
windows JSON returned by serializeTmuxWindows is caller-owned and currently
leaked after calling apprt.surface.Message.WriteReq.init; update the .windows
branch so that after successfully creating the WriteReq
(apprt.surface.Message.WriteReq.init) you free the temporary JSON buffer (the
value returned by serializeTmuxWindows) using the allocator (or use a defer that
frees json after init succeeds), ensuring you free json regardless of early
returns or errors and before calling self.surfaceMessageWriter with the
.tmux_control .windows_changed message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant