Honor SDKROOT for local Apple deps#18
Conversation
|
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:
📝 WalkthroughWalkthroughAdd a local Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Zig caller
participant MsgSendFactory as MsgSend(comptime)
participant ABI as ABI selector
participant Cfn as objc_msgSend* (C)
participant ObjCRuntime as ObjC Runtime
Caller->>MsgSendFactory: msgSend(target, Return, sel, args)
activate MsgSendFactory
MsgSendFactory->>MsgSendFactory: normalize selector, determine Return ABI
MsgSendFactory->>ABI: choose objc_msgSend variant (arch & ABI)
activate ABI
ABI-->>Cfn: select & cast C function
deactivate ABI
MsgSendFactory->>Cfn: invoke with .auto calling convention
activate Cfn
Cfn->>ObjCRuntime: dispatch message to receiver
activate ObjCRuntime
ObjCRuntime-->>Cfn: execute method, return value
deactivate ObjCRuntime
Cfn-->>MsgSendFactory: raw result
MsgSendFactory->>MsgSendFactory: adapt raw result to Zig return type
MsgSendFactory-->>Caller: return typed result
deactivate MsgSendFactory
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 makes the local Apple SDK helper ( Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[build.zig.zon\nzig_objc = local path] --> B[pkg/zig-objc/build.zig\naddAppleSDK]
A --> C[pkg/apple-sdk/build.zig\naddPaths]
B --> D{SDKROOT set?}
C --> D
D -- Yes --> E[Use SDKROOT directly\ndupe into cache]
D -- No --> F[xcrun / getSdk / findNative]
E --> G[Add framework, include,\nlibrary paths to module]
F --> G
H[src/termio/Termio.zig\nqueueMessage] --> I{backend?}
I -- .manual --> J[queueMessageManual\ninline sync dispatch]
I -- .exec --> K[mailbox.send + notify]
J --> L[handlers: resize, write,\nconfig, focus, etc.]
J --> M[renderer_wakeup.notify]
Reviews (1): Last reviewed commit: "build: honor SDKROOT for local apple dep..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn queueMessageManual(self: *Termio, msg: termio.Message) void { | ||
| var td: ThreadData = .{ | ||
| .alloc = self.alloc, | ||
| .loop = undefined, | ||
| .renderer_state = self.renderer_state, | ||
| .surface_mailbox = self.surface_mailbox, | ||
| .backend = .{ .manual = .{} }, | ||
| .mailbox = &self.mailbox, | ||
| }; | ||
|
|
||
| switch (msg) { | ||
| .color_scheme_report => |v| self.colorSchemeReport(&td, v.force) catch |err| { |
There was a problem hiding this comment.
loop = undefined risks UB in message handlers
ThreadData is constructed with loop = undefined. Any of the downstream handlers (colorSchemeReport, changeConfig, resize, sizeReport, clearScreen, focusGained) that dereference td.loop to schedule callbacks or interact with the event loop will invoke undefined behaviour at runtime.
If this is intentional (all .manual-path handlers are guaranteed never to touch td.loop), it would be worth adding a brief comment to that effect — otherwise this is a silent footgun for any future handler that does touch the loop.
| if (sdkroot_override) |sdkroot| { | ||
| var libc: std.zig.LibCInstallation = .{}; | ||
| libc.include_dir = try std.fs.path.join(b.allocator, &.{ sdkroot, "usr", "include" }); | ||
| libc.sys_include_dir = try std.fs.path.join(b.allocator, &.{ sdkroot, "usr", "include" }); | ||
| break :libc libc; |
There was a problem hiding this comment.
LibCInstallation written to libc.txt
When SDKROOT is set, only include_dir and sys_include_dir are populated; fields like crt_dir, lib_dir, gcc_dir, and headers_dir remain null. LibCInstallation.render() will emit empty values for those fields in the generated libc.txt.
In practice this is fine for ARM macOS targets (no CRT/libc stubs needed), but it's worth documenting the assumption with a comment, e.g.:
// SDKROOT override: only header paths are required for Apple Silicon targets.
// CRT/lib/gcc dirs are not needed and left null.If this ever needs to work on other Apple targets (iOS device, tvOS, etc.) the missing crt_dir/lib_dir may cause linker errors.
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!
| msg: termio.Message, | ||
| mutex: MutexState, | ||
| ) void { | ||
| switch (self.backend) { | ||
| .manual => { | ||
| self.queueMessageManual(msg); | ||
| return; | ||
| }, | ||
| .exec => {}, | ||
| } |
There was a problem hiding this comment.
The queueMessage early-return, queueMessageManual, and queueWriteManual additions are iOS-specific manual-backend changes that have no connection to the SDKROOT/Apple SDK or zig-objc vendoring described in the PR. Bundling them makes the diff harder to reason about and review.
Consider splitting these out into a separate PR so each change can be reviewed and reverted independently if needed.
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!
![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: ed09153257
ℹ️ 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 linefeed = self.manual_linefeed_mode.load(.monotonic); | ||
| switch (self.backend) { | ||
| .manual => |*manual| try manual.queueWrite(self.alloc, td, data, linefeed), |
There was a problem hiding this comment.
Manual I/O writes now go through queueWriteManual, which reads manual_linefeed_mode, but that atomic is only updated when a .linefeed_mode message goes through queueMessageManual. The parser emits .linefeed_mode via stream_handler.messageWriter into the mailbox, where Thread.drainMailbox updates Thread.flags.linefeed_mode instead, so mode changes (e.g. DEC mode 20) never reach the inline write path. In manual backend sessions, writes after linefeed-mode toggles will use the wrong CR/CRLF behavior.
Useful? React with 👍 / 👎.
| log.warn("manual inline clear_screen failed err={}", .{err}); | ||
| }, | ||
| .scroll_viewport => |v| self.scrollViewport(v), | ||
| .selection_scroll => {}, |
There was a problem hiding this comment.
When backend is manual, queueMessage now short-circuits into queueMessageManual, and .selection_scroll is treated as a no-op. Previously this message was handled in Thread.drainMailbox to start/stop the scroll timer that emits selection_scroll_tick; dropping it removes auto-scroll while dragging selections outside the viewport in manual/embedded mode.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
pkg/zig-objc/src/property.zig (2)
13-19: Document memory ownership forcopyAttributeValue.The
property_copyAttributeValueC function allocates memory that the caller must free. The return type?[:0]u8doesn't convey this ownership. Consider adding a doc comment noting that the caller must free the returned slice (e.g., viaobjc.free()), or rename to indicate ownership transfer.📝 Suggested documentation
- /// Returns the value of a property attribute given the attribute name. + /// Returns the value of a property attribute given the attribute name. + /// The caller is responsible for freeing the returned slice using `objc.free()`. pub fn copyAttributeValue(self: Property, attr: [:0]const u8) ?[:0]u8 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zig-objc/src/property.zig` around lines 13 - 19, The function Property.copyAttributeValue returns a pointer from the C call c.property_copyAttributeValue which allocates memory the caller must free; update the API documentation (doc comment for copyAttributeValue) to clearly state that the returned slice transfers ownership and must be freed (e.g., via objc.free()) or consider renaming the method to make ownership transfer explicit (e.g., intoOwnedCopyAttributeValue) and document the required free call; mention both Property.copyAttributeValue and the underlying c.property_copyAttributeValue in the comment so callers know where the allocation originates.
34-39: Use proper error handling in test instead of.?.Using
.?on optionals in tests will panic onnull, producing less informative failures. Useorelse return error.Xfor clearer diagnostics, consistent with the pattern used inprotocol.zigtests.🧪 Suggested fix
test { const testing = std.testing; - const NSObject = objc.getClass("NSObject").?; + const NSObject = objc.getClass("NSObject") orelse return error.ClassNotFound; - const prop = NSObject.getProperty("className").?; + const prop = NSObject.getProperty("className") orelse return error.PropertyNotFound; try testing.expectEqualStrings("className", prop.getName()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zig-objc/src/property.zig` around lines 34 - 39, The test currently uses the panic unwrap operator (?.) on objc.getClass and NSObject.getProperty which will abort on null; change those to explicit optional handling using `orelse return error.<Name>` (matching the pattern in the protocol.zig tests) so the test returns a clear error instead of panicking; update the test to allow returning errors if necessary and replace both `objc.getClass("NSObject").?` and `NSObject.getProperty("className").?` with `orelse return error.YourErrorName` (or the same error variant used in protocol.zig) to provide informative test failures when lookup fails.pkg/zig-objc/src/block.zig (1)
303-329: Test doesn't verify object was retained after block copy.The test copies and releases a block containing an
objc.c.id, but doesn't verify the object's retain count changed appropriately. Given the parameter order bug indescCopyHelper, this test would pass even with broken copy semantics. Consider adding retain count assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zig-objc/src/block.zig` around lines 303 - 329, The test currently copies/releases TestBlock but doesn't assert the objc.c.id was retained; update the test (around the TestBlock.init / TestBlock.copy / TestBlock.release sequence) to record the object's retain count via objc message (e.g., call NSObject.msgSend with Sel.registerName("retainCount") or equivalent) before the copy, assert it increments after calling TestBlock.copy(&block), and then assert it decrements after TestBlock.release(copied); this ensures descCopyHelper actually retains/releases the objc.c.id when copying/releasing the block.pkg/zig-objc/src/iterator.zig (1)
71-75: Type mismatch: loop variableiisusizebut cast target isc_int.The loop
for (0..@as(c_int, 10))iterates with ausizeindex, but@as(c_int, 10)suggests the intent is to passc_intvalues. At line 72,i(ausize) is passed tonumberWithInt:which expects a Cint. This works due to implicit coercion but is inconsistent with line 71's explicit cast.♻️ Suggested fix for clarity
- for (0..@as(c_int, 10)) |i| { - const i_number = NSNumber.msgSend(objc.Object, "numberWithInt:", .{i}); + for (0..10) |i| { + const i_number = NSNumber.msgSend(objc.Object, "numberWithInt:", .{`@as`(c_int, `@intCast`(i))});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zig-objc/src/iterator.zig` around lines 71 - 75, The loop mixes usize and c_int: keep the loop index consistent by casting the loop variable to c_int when calling NSNumber.msgSend or iterate using a c_int index; specifically, in the block that creates i_number and calls numberWithInt: (symbols: i, i_number, NSNumber.msgSend, "numberWithInt:", array.msgSend), replace the implicit conversion with an explicit `@intCast`(c_int, i) when passing i to numberWithInt: or rewrite the loop as a c_int counter (e.g., var i: c_int = 0; while (i < 10) { ... i += 1 }) so the value passed to numberWithInt: is a c_int.pkg/zig-objc/src/object.zig (1)
157-159: Discardedinitreturn value may cause confusion.In Objective-C,
initcan return a different object than the receiver (e.g., in class clusters). While this works for NSObject, discarding the result is a code smell and may cause issues if this pattern is copied elsewhere.♻️ Suggested fix
- const obj = NSObject.msgSend(objc.Object, objc.Sel.registerName("alloc"), .{}); - _ = obj.msgSend(objc.Object, objc.Sel.registerName("init"), .{}); + const obj = NSObject.msgSend(objc.Object, objc.Sel.registerName("alloc"), .{}) + .msgSend(objc.Object, objc.Sel.registerName("init"), .{}); try testing.expectEqual(`@as`(c_ulong, 1), retainCount(obj));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zig-objc/src/object.zig` around lines 157 - 159, The test discards the result of calling init (objc.Sel.registerName("init")) on the allocated object, which can be wrong for class-cluster style initializers; change the code to capture the result of init (e.g., let initialized = obj.msgSend(...init...)) and use that initialized value for subsequent calls such as retainCount(initialized) (and propagate/return initialized instead of the original obj variable); update any references to obj after init to use the initialized variable and ensure you handle a possible null/alternative object returned by init.pkg/zig-objc/src/class.zig (1)
59-67: Consider providing type encoding toreplaceMethodfor consistency withaddMethod.
replaceMethodpassesnullfor the type encoding (line 66), whileaddMethodcorrectly usescomptimeEncodeto generate proper type information. Whilenullworks when replacing an existing method (the runtime can infer the types), providing the encoding would be more consistent and could help catch signature mismatches at runtime.♻️ Suggested fix
pub fn replaceMethod(self: Class, name: [:0]const u8, imp: anytype) void { - const fn_info = `@typeInfo`(`@TypeOf`(imp)).@"fn"; + const Fn = `@TypeOf`(imp); + const fn_info = `@typeInfo`(Fn).@"fn"; assert(std.meta.eql(fn_info.calling_convention, std.builtin.CallingConvention.c)); assert(fn_info.is_var_args == false); assert(fn_info.params.len >= 2); assert(fn_info.params[0].type == c.id); assert(fn_info.params[1].type == c.SEL); - _ = c.class_replaceMethod(self.value, objc.sel(name).value, `@ptrCast`(&imp), null); + const encoding = comptime objc.comptimeEncode(Fn); + _ = c.class_replaceMethod(self.value, objc.sel(name).value, `@ptrCast`(&imp), &encoding); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zig-objc/src/class.zig` around lines 59 - 67, replaceMethod currently passes null for the ObjC type encoding to c.class_replaceMethod, which is inconsistent with addMethod that uses comptimeEncode; update replaceMethod to compute and pass the encoding using comptimeEncode (the same logic used in addMethod) for the given imp function signature before calling c.class_replaceMethod so that the runtime receives correct type info and signature mismatches can be detected; make sure to reference the function signature via `@typeInfo`(`@TypeOf`(imp)).@"fn" (as already done) and pass the resulting encoding string instead of null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/zig-objc/build.zig`:
- Around line 96-98: Paths passed to b.pathJoin in the
m.addSystemFrameworkPath/m.addSystemIncludePath/m.addLibraryPath calls include
leading slashes which can be treated as absolute by std.Build.pathJoin; remove
the leading slashes and split the components so pathJoin receives path segments
(e.g., replace "/System/Library/Frameworks" with
"System","Library","Frameworks", and "/usr/include" with "usr","include",
"/usr/lib" with "usr","lib") when calling b.pathJoin(&.{ path, ... }) to ensure
correct joined paths; update the three call sites referencing b.pathJoin
accordingly.
- Around line 21-32: The test target named by the tests variable calls
addAppleSDK unconditionally (try addAppleSDK(b, tests.root_module)), which
differs from the objc module that only calls addAppleSDK when the build option
"add-paths" is enabled; change the test logic to guard the addAppleSDK call with
the same add-paths check used by the objc module so tests respect the add-paths
option (i.e., only call addAppleSDK(b, tests.root_module) when the add-paths
flag is true).
In `@pkg/zig-objc/README.md`:
- Around line 46-47: The example description for macosVersionAtLeast uses plural
"versions"; change the sentence to singular phrasing so it reads "running macOS
version is at least the given arguments." Update the README example that
mentions macosVersionAtLeast to use "running macOS version is…" (referencing the
macosVersionAtLeast symbol) to correct the grammar.
- Line 79: The README usage example shows an outdated build signature using
"*std.build.Builder"; update the example to the current Zig API by changing the
build function parameter type to "*std.Build" (i.e., the "build" function
signature used in the snippet should accept *std.Build), and scan any other
examples in the README that reference std.build.Builder to ensure they use
std.Build consistently so users won't hit build errors when copying the snippet.
In `@pkg/zig-objc/src/block.zig`:
- Around line 123-135: The descCopyHelper function has its parameters swapped
(declared as (src, dst)) but the Descriptor and ObjC runtime expect (dst, src);
fix by changing the parameter order to (dst: *anyopaque, src: *anyopaque) in
descCopyHelper and update the local casts/names (real_dst and real_src) to match
that order so the _Block_object_assign reads from real_src and writes to
real_dst (i.e., ensure `@ptrCast/`@field usages reference the correct source and
destination pointers consistent with the Descriptor signature).
In `@pkg/zig-objc/src/c.zig`:
- Line 11: The code currently treats Objective-C BOOL as true only when `result
== 1` (the mapping line `i8 => result == 1`), which incorrectly classifies other
non-zero true values as false; update that expression to treat any non-zero
`c.BOOL` as true by replacing `result == 1` with `result != 0` (or an equivalent
non-zero check) wherever `c.BOOL`/the `i8` mapping is evaluated so that `i8 =>
result != 0` correctly reflects Objective-C BOOL semantics.
In `@pkg/zig-objc/src/class.zig`:
- Line 168: Two tests both create a dynamic Objective-C class named "my_object"
(see allocateClassPair and the Ivars test), which can collide when tests run
concurrently or if teardown fails; change the class name to be unique per test
(e.g., include the test name or a random/suffix) when calling allocateClassPair
and any subsequent replaceMethod/ivar usage so each test creates and tears down
a distinct class (update references to the class name in allocateClassPair,
replaceMethod, and Ivars-related calls).
- Line 156: Rename the misspelled test identifier "copyProperyList" to
"copyPropertyList" in the test declaration (test "copyProperyList") so the test
name is correct; search for any references to the old name (e.g., test runners
or comments) and update them as well to maintain consistency.
- Line 165: The test name string contains a typo: change the test declaration
labeled "allocatecClassPair and replaceMethod" to "allocateClassPair and
replaceMethod" in the test block (the test with the name string
"allocatecClassPair and replaceMethod"); update any other occurrences or
assertions that reference that test name to match the corrected spelling so
identifiers remain consistent (look for the test declaration in class.zig and
adjust the test name accordingly).
In `@pkg/zig-objc/src/iterator.zig`:
- Around line 41-50: The code dereferences self.state.mutationsPtr and
self.state.itemsPtr unconditionally (in the iteration setup inside
iterator.zig), which can panic when mutationsPtr/itemsPtr are null for immutable
collections; update the logic in the iterator initialization (the block that
reads self.initial_mutations_value and sets self.slice) to first check whether
self.state.mutationsPtr is non-null (e.g., if (self.state.mutationsPtr) |mutPtr|
{ ... } else { ... }) and use the safely captured mutPtr to read the mutation
value instead of `.?.*`, and similarly check self.state.itemsPtr before slicing
(or set self.slice to an empty/zero-length slice when itemsPtr is null) so no
null pointer is directly dereferenced in the branches that reference
mutationsPtr or itemsPtr.
In `@pkg/zig-objc/src/object.zig`:
- Around line 110-118: The ivar lookup result from c.object_getInstanceVariable
must be null-checked to avoid passing null into
c.object_getIvar/c.object_setIvar; update getInstanceVariable(name) to check the
returned ivar and return an optional (e.g., ?Object) by returning null when ivar
== null and otherwise returning fromId(c.object_getIvar(...)), and update
setInstanceVariable(name, val) to check ivar == null and skip the
c.object_setIvar call (or return an error/flag) instead of calling it with a
null ivar; adjust call sites (e.g., the "Ivars" test) to handle the new
optional/flag return.
In `@src/termio/Termio.zig`:
- Around line 409-415: The inline dispatch for .manual currently bypasses the
mailbox and breaks the mutex contract; change the .manual case in the backend
switch so it only short-circuits to queueMessageManual for write-only paths
(e.g., linefeed_mode / write_*) and otherwise routes the message through the
mailbox path or ensures queueMessageManual honors the incoming MutexState.
Concretely, in Termio.zig update the switch over self.backend/.manual to inspect
the message kind or the MutexState passed to queueMessage (the callers that use
queueMessage(..., .locked)) and: 1) if the msg is a pure write path (write_*,
linefeed_mode) call queueMessageManual; 2) otherwise enqueue via the mailbox
path so handlers like changeConfig, resize, sizeReport, clearScreen,
scrollViewport, jumpToPrompt, focusGained, and colorSchemeReport continue to
acquire renderer_state.mutex as expected.
---
Nitpick comments:
In `@pkg/zig-objc/src/block.zig`:
- Around line 303-329: The test currently copies/releases TestBlock but doesn't
assert the objc.c.id was retained; update the test (around the TestBlock.init /
TestBlock.copy / TestBlock.release sequence) to record the object's retain count
via objc message (e.g., call NSObject.msgSend with
Sel.registerName("retainCount") or equivalent) before the copy, assert it
increments after calling TestBlock.copy(&block), and then assert it decrements
after TestBlock.release(copied); this ensures descCopyHelper actually
retains/releases the objc.c.id when copying/releasing the block.
In `@pkg/zig-objc/src/class.zig`:
- Around line 59-67: replaceMethod currently passes null for the ObjC type
encoding to c.class_replaceMethod, which is inconsistent with addMethod that
uses comptimeEncode; update replaceMethod to compute and pass the encoding using
comptimeEncode (the same logic used in addMethod) for the given imp function
signature before calling c.class_replaceMethod so that the runtime receives
correct type info and signature mismatches can be detected; make sure to
reference the function signature via `@typeInfo`(`@TypeOf`(imp)).@"fn" (as already
done) and pass the resulting encoding string instead of null.
In `@pkg/zig-objc/src/iterator.zig`:
- Around line 71-75: The loop mixes usize and c_int: keep the loop index
consistent by casting the loop variable to c_int when calling NSNumber.msgSend
or iterate using a c_int index; specifically, in the block that creates i_number
and calls numberWithInt: (symbols: i, i_number, NSNumber.msgSend,
"numberWithInt:", array.msgSend), replace the implicit conversion with an
explicit `@intCast`(c_int, i) when passing i to numberWithInt: or rewrite the loop
as a c_int counter (e.g., var i: c_int = 0; while (i < 10) { ... i += 1 }) so
the value passed to numberWithInt: is a c_int.
In `@pkg/zig-objc/src/object.zig`:
- Around line 157-159: The test discards the result of calling init
(objc.Sel.registerName("init")) on the allocated object, which can be wrong for
class-cluster style initializers; change the code to capture the result of init
(e.g., let initialized = obj.msgSend(...init...)) and use that initialized value
for subsequent calls such as retainCount(initialized) (and propagate/return
initialized instead of the original obj variable); update any references to obj
after init to use the initialized variable and ensure you handle a possible
null/alternative object returned by init.
In `@pkg/zig-objc/src/property.zig`:
- Around line 13-19: The function Property.copyAttributeValue returns a pointer
from the C call c.property_copyAttributeValue which allocates memory the caller
must free; update the API documentation (doc comment for copyAttributeValue) to
clearly state that the returned slice transfers ownership and must be freed
(e.g., via objc.free()) or consider renaming the method to make ownership
transfer explicit (e.g., intoOwnedCopyAttributeValue) and document the required
free call; mention both Property.copyAttributeValue and the underlying
c.property_copyAttributeValue in the comment so callers know where the
allocation originates.
- Around line 34-39: The test currently uses the panic unwrap operator (?.) on
objc.getClass and NSObject.getProperty which will abort on null; change those to
explicit optional handling using `orelse return error.<Name>` (matching the
pattern in the protocol.zig tests) so the test returns a clear error instead of
panicking; update the test to allow returning errors if necessary and replace
both `objc.getClass("NSObject").?` and `NSObject.getProperty("className").?`
with `orelse return error.YourErrorName` (or the same error variant used in
protocol.zig) to provide informative test failures when lookup fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 582a3793-5e27-4f81-a1f6-e08201bee00c
📒 Files selected for processing (19)
build.zig.zonpkg/apple-sdk/build.zigpkg/zig-objc/LICENSEpkg/zig-objc/README.mdpkg/zig-objc/build.zigpkg/zig-objc/build.zig.zonpkg/zig-objc/src/autorelease.zigpkg/zig-objc/src/block.zigpkg/zig-objc/src/c.zigpkg/zig-objc/src/class.zigpkg/zig-objc/src/encoding.zigpkg/zig-objc/src/iterator.zigpkg/zig-objc/src/main.zigpkg/zig-objc/src/msg_send.zigpkg/zig-objc/src/object.zigpkg/zig-objc/src/property.zigpkg/zig-objc/src/protocol.zigpkg/zig-objc/src/sel.zigsrc/termio/Termio.zig
| const tests = b.addTest(.{ | ||
| .name = "objc-test", | ||
| .root_module = b.createModule(.{ | ||
| .root_source_file = b.path("src/main.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }), | ||
| }); | ||
| tests.linkSystemLibrary("objc"); | ||
| tests.linkFramework("Foundation"); | ||
| tests.linkFramework("AppKit"); // Required by 'tagged pointer' test. | ||
| try addAppleSDK(b, tests.root_module); |
There was a problem hiding this comment.
Test module doesn't respect add_paths option.
The objc module at line 17 conditionally calls addAppleSDK based on the add-paths option, but the test module at line 32 always calls addAppleSDK unconditionally. This inconsistency could cause test builds to fail when add-paths=false is set.
🔧 Suggested fix
tests.linkSystemLibrary("objc");
tests.linkFramework("Foundation");
tests.linkFramework("AppKit"); // Required by 'tagged pointer' test.
- try addAppleSDK(b, tests.root_module);
+ if (add_paths) try addAppleSDK(b, tests.root_module);
b.installArtifact(tests);📝 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.
| const tests = b.addTest(.{ | |
| .name = "objc-test", | |
| .root_module = b.createModule(.{ | |
| .root_source_file = b.path("src/main.zig"), | |
| .target = target, | |
| .optimize = optimize, | |
| }), | |
| }); | |
| tests.linkSystemLibrary("objc"); | |
| tests.linkFramework("Foundation"); | |
| tests.linkFramework("AppKit"); // Required by 'tagged pointer' test. | |
| try addAppleSDK(b, tests.root_module); | |
| const tests = b.addTest(.{ | |
| .name = "objc-test", | |
| .root_module = b.createModule(.{ | |
| .root_source_file = b.path("src/main.zig"), | |
| .target = target, | |
| .optimize = optimize, | |
| }), | |
| }); | |
| tests.linkSystemLibrary("objc"); | |
| tests.linkFramework("Foundation"); | |
| tests.linkFramework("AppKit"); // Required by 'tagged pointer' test. | |
| if (add_paths) try addAppleSDK(b, tests.root_module); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/zig-objc/build.zig` around lines 21 - 32, The test target named by the
tests variable calls addAppleSDK unconditionally (try addAppleSDK(b,
tests.root_module)), which differs from the objc module that only calls
addAppleSDK when the build option "add-paths" is enabled; change the test logic
to guard the addAppleSDK call with the same add-paths check used by the objc
module so tests respect the add-paths option (i.e., only call addAppleSDK(b,
tests.root_module) when the add-paths flag is true).
| m.addSystemFrameworkPath(.{ .cwd_relative = b.pathJoin(&.{ path, "/System/Library/Frameworks" }) }); | ||
| m.addSystemIncludePath(.{ .cwd_relative = b.pathJoin(&.{ path, "/usr/include" }) }); | ||
| m.addLibraryPath(.{ .cwd_relative = b.pathJoin(&.{ path, "/usr/lib" }) }); |
There was a problem hiding this comment.
Remove leading / from path components in pathJoin.
The paths passed to b.pathJoin have leading slashes (e.g., "/System/Library/Frameworks"). Unlike std.fs.path.join, std.Build.pathJoin may not handle this correctly, potentially producing double slashes or treating them as absolute paths. The similar implementation in pkg/apple-sdk/build.zig uses std.fs.path.join with separate components ("System", "Library", "Frameworks").
🔧 Suggested fix
- m.addSystemFrameworkPath(.{ .cwd_relative = b.pathJoin(&.{ path, "/System/Library/Frameworks" }) });
- m.addSystemIncludePath(.{ .cwd_relative = b.pathJoin(&.{ path, "/usr/include" }) });
- m.addLibraryPath(.{ .cwd_relative = b.pathJoin(&.{ path, "/usr/lib" }) });
+ m.addSystemFrameworkPath(.{ .cwd_relative = b.pathJoin(&.{ path, "System", "Library", "Frameworks" }) });
+ m.addSystemIncludePath(.{ .cwd_relative = b.pathJoin(&.{ path, "usr", "include" }) });
+ m.addLibraryPath(.{ .cwd_relative = b.pathJoin(&.{ path, "usr", "lib" }) });📝 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.
| m.addSystemFrameworkPath(.{ .cwd_relative = b.pathJoin(&.{ path, "/System/Library/Frameworks" }) }); | |
| m.addSystemIncludePath(.{ .cwd_relative = b.pathJoin(&.{ path, "/usr/include" }) }); | |
| m.addLibraryPath(.{ .cwd_relative = b.pathJoin(&.{ path, "/usr/lib" }) }); | |
| m.addSystemFrameworkPath(.{ .cwd_relative = b.pathJoin(&.{ path, "System", "Library", "Frameworks" }) }); | |
| m.addSystemIncludePath(.{ .cwd_relative = b.pathJoin(&.{ path, "usr", "include" }) }); | |
| m.addLibraryPath(.{ .cwd_relative = b.pathJoin(&.{ path, "usr", "lib" }) }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/zig-objc/build.zig` around lines 96 - 98, Paths passed to b.pathJoin in
the m.addSystemFrameworkPath/m.addSystemIncludePath/m.addLibraryPath calls
include leading slashes which can be treated as absolute by std.Build.pathJoin;
remove the leading slashes and split the components so pathJoin receives path
segments (e.g., replace "/System/Library/Frameworks" with
"System","Library","Frameworks", and "/usr/include" with "usr","include",
"/usr/lib" with "usr","lib") when calling b.pathJoin(&.{ path, ... }) to ensure
correct joined paths; update the three call sites referencing b.pathJoin
accordingly.
| `macosVersionAtLeast` that returns true if the running macOS versions | ||
| is at least the given arguments. |
There was a problem hiding this comment.
Fix grammar in the example description.
Line 46-Line 47 should use singular phrasing: “running macOS version is…”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/zig-objc/README.md` around lines 46 - 47, The example description for
macosVersionAtLeast uses plural "versions"; change the sentence to singular
phrasing so it reads "running macOS version is at least the given arguments."
Update the README example that mentions macosVersionAtLeast to use "running
macOS version is…" (referencing the macosVersionAtLeast symbol) to correct the
grammar.
| Add this repository to your `build.zig.zon` file. Then: | ||
|
|
||
| ```zig | ||
| pub fn build(b: *std.build.Builder) !void { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify build API usage patterns in this repository.
rg -nP --type=zig 'pub fn build\s*\(\s*b:\s*\*std\.Build\s*\)'
rg -nP --type=zig 'std\.build\.Builder'Repository: manaflow-ai/ghostty
Length of output: 2035
🏁 Script executed:
cat -n pkg/zig-objc/README.md | sed -n '70,90p'Repository: manaflow-ai/ghostty
Length of output: 659
Update the build function signature in the usage example to use the current Zig API.
Line 79 documents *std.build.Builder, which is outdated. All build scripts in this repository (and current Zig versions) use *std.Build. Users copying this snippet will encounter build errors.
Suggested fix
-pub fn build(b: *std.build.Builder) !void {
+pub fn build(b: *std.Build) !void {📝 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.
| pub fn build(b: *std.build.Builder) !void { | |
| pub fn build(b: *std.Build) !void { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/zig-objc/README.md` at line 79, The README usage example shows an
outdated build signature using "*std.build.Builder"; update the example to the
current Zig API by changing the build function parameter type to "*std.Build"
(i.e., the "build" function signature used in the snippet should accept
*std.Build), and scan any other examples in the README that reference
std.build.Builder to ensure they use std.Build consistently so users won't hit
build errors when copying the snippet.
| fn descCopyHelper(src: *anyopaque, dst: *anyopaque) callconv(.c) void { | ||
| const real_src: *Context = @ptrCast(@alignCast(src)); | ||
| const real_dst: *Context = @ptrCast(@alignCast(dst)); | ||
| inline for (captures_info.fields) |field| { | ||
| if (field.type == objc.c.id) { | ||
| _Block_object_assign( | ||
| @ptrCast(&@field(real_dst, field.name)), | ||
| @field(real_src, field.name), | ||
| .object, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Parameter order swapped in descCopyHelper.
The copy helper signature in Descriptor (line 260) is (dst, src), matching the ObjC runtime's expected signature. However, the implementation at line 123 declares parameters as (src, dst), which swaps them. This causes the helper to read from the destination and write to the source, corrupting memory during block copies.
🐛 Proposed fix
- fn descCopyHelper(src: *anyopaque, dst: *anyopaque) callconv(.c) void {
- const real_src: *Context = `@ptrCast`(`@alignCast`(src));
- const real_dst: *Context = `@ptrCast`(`@alignCast`(dst));
+ fn descCopyHelper(dst: *anyopaque, src: *anyopaque) callconv(.c) void {
+ const real_dst: *Context = `@ptrCast`(`@alignCast`(dst));
+ const real_src: *Context = `@ptrCast`(`@alignCast`(src));
inline for (captures_info.fields) |field| {
if (field.type == objc.c.id) {
_Block_object_assign(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/zig-objc/src/block.zig` around lines 123 - 135, The descCopyHelper
function has its parameters swapped (declared as (src, dst)) but the Descriptor
and ObjC runtime expect (dst, src); fix by changing the parameter order to (dst:
*anyopaque, src: *anyopaque) in descCopyHelper and update the local casts/names
(real_dst and real_src) to match that order so the _Block_object_assign reads
from real_src and writes to real_dst (i.e., ensure `@ptrCast/`@field usages
reference the correct source and destination pointers consistent with the
Descriptor signature).
| try testing.expect(list.len > 0); | ||
| } | ||
|
|
||
| test "allocatecClassPair and replaceMethod" { |
There was a problem hiding this comment.
Typo in test name: "allocatecClassPair" should be "allocateClassPair".
✏️ Proposed fix
-test "allocatecClassPair and replaceMethod" {
+test "allocateClassPair and replaceMethod" {📝 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.
| test "allocatecClassPair and replaceMethod" { | |
| test "allocateClassPair and replaceMethod" { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/zig-objc/src/class.zig` at line 165, The test name string contains a
typo: change the test declaration labeled "allocatecClassPair and replaceMethod"
to "allocateClassPair and replaceMethod" in the test block (the test with the
name string "allocatecClassPair and replaceMethod"); update any other
occurrences or assertions that reference that test name to match the corrected
spelling so identifiers remain consistent (look for the test declaration in
class.zig and adjust the test name accordingly).
| test "allocatecClassPair and replaceMethod" { | ||
| const testing = std.testing; | ||
| const NSObject = getClass("NSObject").?; | ||
| var my_object = allocateClassPair(NSObject, "my_object").?; |
There was a problem hiding this comment.
Potential test collision: both tests use the same class name "my_object".
Both the "allocateClassPair and replaceMethod" test (line 168) and the "Ivars" test (line 188) create a dynamic class named "my_object". If tests run concurrently or if disposal fails, this could cause intermittent test failures. Consider using unique class names.
✏️ Proposed fix
test "Ivars" {
const testing = std.testing;
const NSObject = getClass("NSObject").?;
- var my_object = allocateClassPair(NSObject, "my_object").?;
+ var my_object = allocateClassPair(NSObject, "my_object_ivars").?;Also applies to: 188-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/zig-objc/src/class.zig` at line 168, Two tests both create a dynamic
Objective-C class named "my_object" (see allocateClassPair and the Ivars test),
which can collide when tests run concurrently or if teardown fails; change the
class name to be unique per test (e.g., include the test name or a
random/suffix) when calling allocateClassPair and any subsequent
replaceMethod/ivar usage so each test creates and tears down a distinct class
(update references to the class name in allocateClassPair, replaceMethod, and
Ivars-related calls).
| if (self.initial_mutations_value) |value| { | ||
| // Call the mutation handler if the mutations value has | ||
| // changed since the start of iteration. | ||
| if (value != self.state.mutationsPtr.?.*) { | ||
| objc.c.objc_enumerationMutation(self.object.value); | ||
| } | ||
| } else { | ||
| self.initial_mutations_value = self.state.mutationsPtr.?.*; | ||
| } | ||
| self.slice = self.state.itemsPtr.?[0..count]; |
There was a problem hiding this comment.
Potential null dereference of mutationsPtr.
The mutationsPtr field can be null for immutable collections that don't track mutations. Lines 44 and 48 dereference it unconditionally with .?.* and .?, which will panic if mutationsPtr is null.
🛡️ Suggested fix
if (self.initial_mutations_value) |value| {
// Call the mutation handler if the mutations value has
// changed since the start of iteration.
- if (value != self.state.mutationsPtr.?.*) {
+ if (self.state.mutationsPtr) |ptr| {
+ if (value != ptr.*) {
+ objc.c.objc_enumerationMutation(self.object.value);
+ }
+ }
- objc.c.objc_enumerationMutation(self.object.value);
- }
} else {
- self.initial_mutations_value = self.state.mutationsPtr.?.*;
+ if (self.state.mutationsPtr) |ptr| {
+ self.initial_mutations_value = ptr.*;
+ }
}📝 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.
| if (self.initial_mutations_value) |value| { | |
| // Call the mutation handler if the mutations value has | |
| // changed since the start of iteration. | |
| if (value != self.state.mutationsPtr.?.*) { | |
| objc.c.objc_enumerationMutation(self.object.value); | |
| } | |
| } else { | |
| self.initial_mutations_value = self.state.mutationsPtr.?.*; | |
| } | |
| self.slice = self.state.itemsPtr.?[0..count]; | |
| if (self.initial_mutations_value) |value| { | |
| // Call the mutation handler if the mutations value has | |
| // changed since the start of iteration. | |
| if (self.state.mutationsPtr) |ptr| { | |
| if (value != ptr.*) { | |
| objc.c.objc_enumerationMutation(self.object.value); | |
| } | |
| } | |
| } else { | |
| if (self.state.mutationsPtr) |ptr| { | |
| self.initial_mutations_value = ptr.*; | |
| } | |
| } | |
| self.slice = self.state.itemsPtr.?[0..count]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/zig-objc/src/iterator.zig` around lines 41 - 50, The code dereferences
self.state.mutationsPtr and self.state.itemsPtr unconditionally (in the
iteration setup inside iterator.zig), which can panic when mutationsPtr/itemsPtr
are null for immutable collections; update the logic in the iterator
initialization (the block that reads self.initial_mutations_value and sets
self.slice) to first check whether self.state.mutationsPtr is non-null (e.g., if
(self.state.mutationsPtr) |mutPtr| { ... } else { ... }) and use the safely
captured mutPtr to read the mutation value instead of `.?.*`, and similarly
check self.state.itemsPtr before slicing (or set self.slice to an
empty/zero-length slice when itemsPtr is null) so no null pointer is directly
dereferenced in the branches that reference mutationsPtr or itemsPtr.
| pub fn getInstanceVariable(self: Object, name: [:0]const u8) Object { | ||
| const ivar = c.object_getInstanceVariable(self.value, name, null); | ||
| return fromId(c.object_getIvar(self.value, ivar)); | ||
| } | ||
|
|
||
| pub fn setInstanceVariable(self: Object, name: [:0]const u8, val: Object) void { | ||
| const ivar = c.object_getInstanceVariable(self.value, name, null); | ||
| c.object_setIvar(self.value, ivar, val.value); | ||
| } |
There was a problem hiding this comment.
Missing null check for ivar lookup could cause undefined behavior.
If name doesn't correspond to a valid instance variable, object_getInstanceVariable returns null. Passing this null ivar to object_getIvar/object_setIvar results in undefined behavior and likely crashes.
🐛 Proposed fix with null handling
pub fn getInstanceVariable(self: Object, name: [:0]const u8) ?Object {
const ivar = c.object_getInstanceVariable(self.value, name, null);
+ if (ivar == null) return null;
- return fromId(c.object_getIvar(self.value, ivar));
+ return fromId(c.object_getIvar(self.value, ivar.?));
}
- pub fn setInstanceVariable(self: Object, name: [:0]const u8, val: Object) void {
+ pub fn setInstanceVariable(self: Object, name: [:0]const u8, val: Object) bool {
const ivar = c.object_getInstanceVariable(self.value, name, null);
- c.object_setIvar(self.value, ivar, val.value);
+ if (ivar == null) return false;
+ c.object_setIvar(self.value, ivar.?, val.value);
+ return true;
}Note: This change would also require updating the call site in class.zig test "Ivars" (line 199-200) to handle the new return type/optional.
📝 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.
| pub fn getInstanceVariable(self: Object, name: [:0]const u8) Object { | |
| const ivar = c.object_getInstanceVariable(self.value, name, null); | |
| return fromId(c.object_getIvar(self.value, ivar)); | |
| } | |
| pub fn setInstanceVariable(self: Object, name: [:0]const u8, val: Object) void { | |
| const ivar = c.object_getInstanceVariable(self.value, name, null); | |
| c.object_setIvar(self.value, ivar, val.value); | |
| } | |
| pub fn getInstanceVariable(self: Object, name: [:0]const u8) ?Object { | |
| const ivar = c.object_getInstanceVariable(self.value, name, null); | |
| if (ivar == null) return null; | |
| return fromId(c.object_getIvar(self.value, ivar.?)); | |
| } | |
| pub fn setInstanceVariable(self: Object, name: [:0]const u8, val: Object) bool { | |
| const ivar = c.object_getInstanceVariable(self.value, name, null); | |
| if (ivar == null) return false; | |
| c.object_setIvar(self.value, ivar.?, val.value); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/zig-objc/src/object.zig` around lines 110 - 118, The ivar lookup result
from c.object_getInstanceVariable must be null-checked to avoid passing null
into c.object_getIvar/c.object_setIvar; update getInstanceVariable(name) to
check the returned ivar and return an optional (e.g., ?Object) by returning null
when ivar == null and otherwise returning fromId(c.object_getIvar(...)), and
update setInstanceVariable(name, val) to check ivar == null and skip the
c.object_setIvar call (or return an error/flag) instead of calling it with a
null ivar; adjust call sites (e.g., the "Ivars" test) to handle the new
optional/flag return.
| switch (self.backend) { | ||
| .manual => { | ||
| self.queueMessageManual(msg); | ||
| return; | ||
| }, | ||
| .exec => {}, | ||
| } |
There was a problem hiding this comment.
Don't bypass the mailbox for lock-sensitive manual messages.
Line 409 now routes every .manual message through inline dispatch and drops the mutex contract. Handlers like changeConfig, resize, sizeReport, clearScreen, scrollViewport, jumpToPrompt, focusGained, and colorSchemeReport all take renderer_state.mutex themselves, so any caller that uses queueMessage(..., .locked) will self-deadlock here. It also removes mailbox-thread serialization for paths like resize, which writes self.size before taking the lock.
If the iOS workaround is only for writes, keep the inline fast path to .linefeed_mode / .write_* and leave the rest on the mailbox path, or make the manual handlers explicitly honor MutexState.
Possible narrow fix
pub fn queueMessage(
self: *Termio,
msg: termio.Message,
mutex: MutexState,
) void {
switch (self.backend) {
- .manual => {
- self.queueMessageManual(msg);
- return;
- },
+ .manual => switch (mutex) {
+ .locked => {},
+ .unlocked => switch (msg) {
+ .linefeed_mode,
+ .write_small,
+ .write_stable,
+ .write_alloc,
+ => {
+ self.queueMessageManual(msg);
+ return;
+ },
+ else => {},
+ },
+ },
.exec => {},
}
self.mailbox.send(msg, switch (mutex) {
.locked => self.renderer_state.mutex,Also applies to: 424-485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/termio/Termio.zig` around lines 409 - 415, The inline dispatch for
.manual currently bypasses the mailbox and breaks the mutex contract; change the
.manual case in the backend switch so it only short-circuits to
queueMessageManual for write-only paths (e.g., linefeed_mode / write_*) and
otherwise routes the message through the mailbox path or ensures
queueMessageManual honors the incoming MutexState. Concretely, in Termio.zig
update the switch over self.backend/.manual to inspect the message kind or the
MutexState passed to queueMessage (the callers that use queueMessage(...,
.locked)) and: 1) if the msg is a pure write path (write_*, linefeed_mode) call
queueMessageManual; 2) otherwise enqueue via the mailbox path so handlers like
changeConfig, resize, sizeReport, clearScreen, scrollViewport, jumpToPrompt,
focusGained, and colorSchemeReport continue to acquire renderer_state.mutex as
expected.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 19 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="pkg/zig-objc/build.zig">
<violation number="1" location="pkg/zig-objc/build.zig:32">
P2: Honor the add-paths option for the tests module too; otherwise `zig build test -Dadd-paths=false` still tries to locate the Apple SDK and fails without Xcode.</violation>
</file>
<file name="pkg/zig-objc/src/msg_send.zig">
<violation number="1" location="pkg/zig-objc/src/msg_send.zig:68">
P2: `objc_super` uses the `super_class` field name from the Objective-C runtime headers. `.class` doesn’t exist, so this initializer won’t compile. Use `.super_class` instead.</violation>
</file>
<file name="pkg/zig-objc/src/iterator.zig">
<violation number="1" location="pkg/zig-objc/src/iterator.zig:16">
P3: Fix the documentation link typo so the reference resolves correctly.</violation>
<violation number="2" location="pkg/zig-objc/src/iterator.zig:41">
P1: Avoid force-unwrapping `mutationsPtr` and `itemsPtr`; guard null pointers before dereferencing so iteration does not panic on empty or terminal enumeration states.</violation>
</file>
<file name="pkg/zig-objc/src/block.zig">
<violation number="1" location="pkg/zig-objc/src/block.zig:123">
P1: descCopyHelper’s parameter order is reversed relative to the descriptor signature, so the runtime will pass `(dst, src)` but the helper treats them as `(src, dst)`, leading to incorrect retain/assign behavior for captured objc.id fields.</violation>
</file>
<file name="pkg/zig-objc/src/object.zig">
<violation number="1" location="pkg/zig-objc/src/object.zig:112">
P1: Guard the ivar lookup result before passing it to `object_getIvar`; calling runtime ivar accessors with a null ivar pointer can crash.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| fn descCopyHelper(src: *anyopaque, dst: *anyopaque) callconv(.c) void { | ||
| const real_src: *Context = @ptrCast(@alignCast(src)); | ||
| const real_dst: *Context = @ptrCast(@alignCast(dst)); |
There was a problem hiding this comment.
P1: descCopyHelper’s parameter order is reversed relative to the descriptor signature, so the runtime will pass (dst, src) but the helper treats them as (src, dst), leading to incorrect retain/assign behavior for captured objc.id fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/zig-objc/src/block.zig, line 123:
<comment>descCopyHelper’s parameter order is reversed relative to the descriptor signature, so the runtime will pass `(dst, src)` but the helper treats them as `(src, dst)`, leading to incorrect retain/assign behavior for captured objc.id fields.</comment>
<file context>
@@ -0,0 +1,329 @@
+ _Block_release(@ptrCast(@alignCast(ctx)));
+ }
+
+ fn descCopyHelper(src: *anyopaque, dst: *anyopaque) callconv(.c) void {
+ const real_src: *Context = @ptrCast(@alignCast(src));
+ const real_dst: *Context = @ptrCast(@alignCast(dst));
</file context>
| fn descCopyHelper(src: *anyopaque, dst: *anyopaque) callconv(.c) void { | |
| const real_src: *Context = @ptrCast(@alignCast(src)); | |
| const real_dst: *Context = @ptrCast(@alignCast(dst)); | |
| fn descCopyHelper(dst: *anyopaque, src: *anyopaque) callconv(.c) void { | |
| const real_dst: *Context = @ptrCast(@alignCast(dst)); | |
| const real_src: *Context = @ptrCast(@alignCast(src)); |
| &self.buffer, | ||
| self.buffer.len, | ||
| }); | ||
| if (self.initial_mutations_value) |value| { |
There was a problem hiding this comment.
P1: Avoid force-unwrapping mutationsPtr and itemsPtr; guard null pointers before dereferencing so iteration does not panic on empty or terminal enumeration states.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/zig-objc/src/iterator.zig, line 41:
<comment>Avoid force-unwrapping `mutationsPtr` and `itemsPtr`; guard null pointers before dereferencing so iteration does not panic on empty or terminal enumeration states.</comment>
<file context>
@@ -0,0 +1,109 @@
+ &self.buffer,
+ self.buffer.len,
+ });
+ if (self.initial_mutations_value) |value| {
+ // Call the mutation handler if the mutations value has
+ // changed since the start of iteration.
</file context>
|
|
||
| pub fn getInstanceVariable(self: Object, name: [:0]const u8) Object { | ||
| const ivar = c.object_getInstanceVariable(self.value, name, null); | ||
| return fromId(c.object_getIvar(self.value, ivar)); |
There was a problem hiding this comment.
P1: Guard the ivar lookup result before passing it to object_getIvar; calling runtime ivar accessors with a null ivar pointer can crash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/zig-objc/src/object.zig, line 112:
<comment>Guard the ivar lookup result before passing it to `object_getIvar`; calling runtime ivar accessors with a null ivar pointer can crash.</comment>
<file context>
@@ -0,0 +1,220 @@
+
+ pub fn getInstanceVariable(self: Object, name: [:0]const u8) Object {
+ const ivar = c.object_getInstanceVariable(self.value, name, null);
+ return fromId(c.object_getIvar(self.value, ivar));
+ }
+
</file context>
| tests.linkSystemLibrary("objc"); | ||
| tests.linkFramework("Foundation"); | ||
| tests.linkFramework("AppKit"); // Required by 'tagged pointer' test. | ||
| try addAppleSDK(b, tests.root_module); |
There was a problem hiding this comment.
P2: Honor the add-paths option for the tests module too; otherwise zig build test -Dadd-paths=false still tries to locate the Apple SDK and fails without Xcode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/zig-objc/build.zig, line 32:
<comment>Honor the add-paths option for the tests module too; otherwise `zig build test -Dadd-paths=false` still tries to locate the Apple SDK and fails without Xcode.</comment>
<file context>
@@ -0,0 +1,99 @@
+ tests.linkSystemLibrary("objc");
+ tests.linkFramework("Foundation");
+ tests.linkFramework("AppKit"); // Required by 'tagged pointer' test.
+ try addAppleSDK(b, tests.root_module);
+ b.installArtifact(tests);
+
</file context>
| const msg_send_ptr: *const Fn = @ptrCast(msg_send_fn); | ||
| var super: c.objc_super = .{ | ||
| .receiver = target.value, | ||
| .class = superclass.value, |
There was a problem hiding this comment.
P2: objc_super uses the super_class field name from the Objective-C runtime headers. .class doesn’t exist, so this initializer won’t compile. Use .super_class instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/zig-objc/src/msg_send.zig, line 68:
<comment>`objc_super` uses the `super_class` field name from the Objective-C runtime headers. `.class` doesn’t exist, so this initializer won’t compile. Use `.super_class` instead.</comment>
<file context>
@@ -0,0 +1,244 @@
+ const msg_send_ptr: *const Fn = @ptrCast(msg_send_fn);
+ var super: c.objc_super = .{
+ .receiver = target.value,
+ .class = superclass.value,
+ };
+ const result = @call(.auto, msg_send_ptr, .{ &super, sel.value } ++ args);
</file context>
| .class = superclass.value, | |
| .super_class = superclass.value, |
| /// objects in an Objective-C collection. This can be used with any object | ||
| /// that conforms to the `NSFastEnumeration` protocol. | ||
| /// | ||
| /// [1]: Nhttps://developer.apple.com/documentation/foundation/nsfastenumeration |
There was a problem hiding this comment.
P3: Fix the documentation link typo so the reference resolves correctly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/zig-objc/src/iterator.zig, line 16:
<comment>Fix the documentation link typo so the reference resolves correctly.</comment>
<file context>
@@ -0,0 +1,109 @@
+/// objects in an Objective-C collection. This can be used with any object
+/// that conforms to the `NSFastEnumeration` protocol.
+///
+/// [1]: Nhttps://developer.apple.com/documentation/foundation/nsfastenumeration
+pub const Iterator = struct {
+ object: objc.Object,
</file context>
- Fix xcframework build for iOS targets (SDKROOT handling, libtool, lipo) - Add IOSurface-based rendering for iOS manual IO mode - Add surface config for manual text input handling - Honor SDKROOT for local apple dependency resolution in build system
There was a problem hiding this comment.
3 issues found across 17 files (changes from recent commits).
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/generic.zig">
<violation number="1" location="src/renderer/generic.zig:1501">
P2: Avoid warning-level logging in the normal `!needs_redraw` render path; this can spam logs at frame rate on iOS.</violation>
</file>
<file name="src/renderer/Metal.zig">
<violation number="1" location="src/renderer/Metal.zig:266">
P2: Avoid `warn`-level logging in the iOS present paths. These logs run on hot render/present code and will spam release logs and add avoidable runtime overhead.</violation>
</file>
<file name="src/renderer/metal/IOSurfaceLayer.zig">
<violation number="1" location="src/renderer/metal/IOSurfaceLayer.zig:148">
P2: Avoid warning-level logging on normal frame presentation; this can spam warn logs in the hot render path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 745a70a079
ℹ️ 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".
| switch (self.backend) { | ||
| .manual => { | ||
| self.queueMessageManual(msg); | ||
| return; | ||
| }, |
There was a problem hiding this comment.
Routing .manual through queueMessageManual bypasses mailbox serialization entirely, so writes from surface input now call manual.queueWrite inline on the caller thread while stream-generated responses still flow through stream_handler.messageWriter and Thread.drainMailbox. In manual/embed mode this can invoke io_write_cb from two threads and reorder bytes (for example protocol replies racing with user keystrokes), which breaks deterministic host I/O behavior.
Useful? React with 👍 / 👎.
| if (comptime builtin.os.tag == .ios) { | ||
| log.warn( | ||
| "ios drawFrame no_redraw surface={}x{} size_changed={} cells_rebuilt={} sync={}", |
There was a problem hiding this comment.
This warning is in a frame-hot branch (!needs_redraw) that is hit continuously on idle renders, and similar warn instrumentation was added in other present callbacks in this commit. Since warn-level logs are enabled in normal builds, iOS will continuously format and emit unified-log entries during rendering, which adds avoidable overhead and can hurt frame pacing and battery life.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/zig-objc/build.zig (1)
32-32:⚠️ Potential issue | 🟡 MinorGuard test SDK path injection behind
add-paths.Line 32 still applies
addAppleSDKunconditionally, so-Dadd-paths=falseis ignored for tests while honored for theobjcmodule (Line 17).Suggested fix
- try addAppleSDK(b, tests.root_module); + if (add_paths) try addAppleSDK(b, tests.root_module);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zig-objc/build.zig` at line 32, The call to addAppleSDK(b, tests.root_module) is being executed unconditionally, ignoring the -Dadd-paths flag; wrap or gate that call with the same add-paths option check used earlier for the objc module so tests only get addAppleSDK applied when add-paths is enabled (i.e., mirror the conditional used around the objc module declaration and call to addAppleSDK to check the build/options.add_paths flag before calling addAppleSDK for tests.root_module).
🧹 Nitpick comments (6)
src/renderer/generic.zig (1)
1500-1575: Consider usinglog.debuginstead oflog.warnfor these iOS diagnostic logs.These three logging blocks fire during normal resize/redraw operations and would generate significant noise in production. The
log.warnlevel is typically reserved for potentially problematic conditions rather than routine flow tracing.If these are temporary debugging aids for the iOS manual IO work described in the PR, consider either:
- Using
log.debuglevel for ongoing diagnostics- Removing them once the iOS debugging is complete
♻️ Suggested change
if (comptime builtin.os.tag == .ios) { - log.warn( + log.debug( "ios drawFrame early presentLastTarget size_changed surface={}x{} grid={}x{} cells={}x{} has_presented={}",Apply the same change to all three
log.warncalls in this function (lines 1501, 1538, 1565).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/generic.zig` around lines 1500 - 1575, Replace the three iOS diagnostic log calls from log.warn to log.debug inside the drawFrame flow: the "ios drawFrame early presentLastTarget" block before try self.api.presentLastTarget(), the "ios drawFrame wait_cells" block inside the size_changed check that calls presentLastTarget(), and the "ios drawFrame no_redraw" block in the !needs_redraw branch; update those three log.warn invocations to log.debug so routine resize/redraw diagnostics use debug level instead of warn.src/renderer/metal/IOSurfaceLayer.zig (1)
140-164: Consider usinglog.debuginstead oflog.warnfor these diagnostic logs.These new logs appear to be for iOS debugging purposes (per the PR objectives around iOS manual IO). Using
log.warnfor routine presentation events will create noise in production logs since they fire on every successful surface present.If these are temporary debug aids, consider reverting to
log.debug(as the discard case originally was) or removing them before merge. If they're intended for long-term diagnostics,log.debugis more appropriate for non-error operational events.♻️ Suggested change to use debug level
- log.warn( + log.debug( "ios setSurfaceCallback discard surface={}x{} layer={}x{}", .{ surface.getWidth(), surface.getHeight(), width, height }, ); return; } -log.warn( +log.debug( "ios setSurfaceCallback present surface={}x{} layer={}x{}", .{ surface.getWidth(), surface.getHeight(), width, height }, );And similarly for
setSurfaceUncheckedCallback:-log.warn( +log.debug( "ios setSurfaceUncheckedCallback present surface={}x{}", .{ surface.getWidth(), surface.getHeight() }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/metal/IOSurfaceLayer.zig` around lines 140 - 164, Replace the noisy warning-level logs with debug-level logging: in the setSurfaceCallback function change the two log.warn calls (the "discard surface=..." and "present surface=... layer=...") to log.debug so routine surface present/discard events don't clutter production logs, and likewise change the log.warn in setSurfaceUncheckedCallback ("ios setSurfaceUncheckedCallback present surface=...") to log.debug; keep the rest of the handling (layer.setProperty("contents", surface), defer surface.release(), etc.) unchanged.src/renderer/Metal.zig (1)
264-304: Consider usinglog.debuginstead oflog.warnfor these iOS diagnostic logs.These logs fire on every present operation, which would generate significant noise in production logs. The
log.warnlevel is typically reserved for concerning conditions, not routine operational tracing.Since these appear to be temporary iOS debugging aids (per PR objectives), consider using
log.debugor removing them before merge.♻️ Suggested change
if (comptime builtin.os.tag == .ios) { - log.warn( + log.debug( "ios present sync={} surface={}x{} layer_bounds={}", .{ sync, target.surface.getWidth(), target.surface.getHeight(), self.layer.layer.getProperty(graphics.Rect, "bounds"), }, ); }And similarly for
presentLastTarget.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/Metal.zig` around lines 264 - 304, Change the iOS diagnostic log level from warn to debug in both places: the present flow where the code logs "ios present sync=... surface=... layer_bounds=..." (the block that references target.surface and self.layer.layer.getProperty) and in the pub inline fn presentLastTarget method that logs "ios presentLastTarget surface=... layer_bounds=..." (the block that references surface.getWidth/Height and self.layer.layer.getProperty). Replace log.warn with log.debug in both locations, keeping the existing message string and argument tuple intact so only the log level changes.src/termio/Manual.zig (1)
100-125: Optional: extract repeated callback/setup into a tiny test helper.Both tests duplicate the same callback +
ArrayListplumbing. A helper would reduce repetition and keep future test additions focused on behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termio/Manual.zig` around lines 100 - 125, Extract the duplicated test setup (the std.ArrayList u8 allocation/deinit and the write callback `cb`) into a small helper used by both tests: create a helper (e.g., makeTestOutput) that allocates and returns the `out` ArrayList plus a C-callback closure compatible with `Manual.init` (`write_cb` and `write_userdata`) and a cleanup function or allocator so callers can defer deinit; then replace the inline `var out: std.ArrayList(u8) = .empty` and the `cb` struct in both tests with calls to this helper and use the returned `write_cb` and `write_userdata` when calling `Manual.init` and `manual.queueWrite` to remove duplication while preserving behavior.src/build/LibtoolStep.zig (1)
35-38: Consider forwardingTMPDIRfor system consistency.The shell script uses
${TMPDIR:-/tmp}(line 45), but sinceenv_maponly containsPATH,TMPDIRis unset in the subprocess and will always fall back to/tmp. On macOS,TMPDIRis typically set to a user-specific temp directory (e.g.,/var/folders/...).If you want to honor the system's temp directory convention:
🔧 Proposed fix
if (env.get("PATH")) |path| env_map.put("PATH", path) catch `@panic`("OOM"); +if (env.get("TMPDIR")) |tmpdir| env_map.put("TMPDIR", tmpdir) catch `@panic`("OOM"); run_step.env_map = env_map;This applies equally to
XCFrameworkStep.zig,LipoStep.zig, and other steps that may use temp directories internally.Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/LibtoolStep.zig` around lines 35 - 38, The env_map only forwards PATH so TMPDIR is missing causing the subprocess to fall back to /tmp; update the code that builds env_map (e.g., in LibtoolStep.zig — where env_map and run_step.env_map are set) to also check env.get("TMPDIR") and, if present, env_map.put("TMPDIR", tmpdir) (with the same OOM handling pattern) so the child process inherits the system/user temp dir; apply the same change to XCFrameworkStep.zig, LipoStep.zig and any other step that constructs env_map for subprocesses.include/ghostty.h (1)
1102-1102: Document this entrypoint’s semantics next to the declaration.
ghostty_surface_text_inputnow differs materially fromghostty_surface_text(committed typingvspaste). A short declaration comment here would make it much harder for embedders to accidentally bypass bracketed-paste/protection behavior.Suggested doc tweak
void ghostty_surface_text(ghostty_surface_t, const char*, uintptr_t); +// Committed typing semantics (IME/on-screen keyboard text). Use +// `ghostty_surface_text` for paste semantics instead. void ghostty_surface_text_input(ghostty_surface_t, const char*, uintptr_t);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/ghostty.h` at line 1102, Add a short declaration comment for ghostty_surface_text_input explaining its semantics (i.e., that it represents committed typing input and not a paste) and how it differs from ghostty_surface_text (paste), including any expectations around bracketed-paste/protection behavior so embedders know not to bypass those protections; place this comment immediately above the ghostty_surface_text_input declaration to clarify usage for callers.
🤖 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/Surface.zig`:
- Around line 6363-6388: completeTextInput currently always forwards committed
text to the PTY, bypassing DEC KAM / disable_keyboard; add the same guard used
for normal typing to return early when the Surface.disable_keyboard (or
equivalent keyboard-disable flag) is set—i.e., after the empty-data check, check
self.disable_keyboard and simply return (without encoding or queueing the IO) so
IME/on-screen keyboard input is suppressed consistently with the typing path in
the Surface code.
---
Duplicate comments:
In `@pkg/zig-objc/build.zig`:
- Line 32: The call to addAppleSDK(b, tests.root_module) is being executed
unconditionally, ignoring the -Dadd-paths flag; wrap or gate that call with the
same add-paths option check used earlier for the objc module so tests only get
addAppleSDK applied when add-paths is enabled (i.e., mirror the conditional used
around the objc module declaration and call to addAppleSDK to check the
build/options.add_paths flag before calling addAppleSDK for tests.root_module).
---
Nitpick comments:
In `@include/ghostty.h`:
- Line 1102: Add a short declaration comment for ghostty_surface_text_input
explaining its semantics (i.e., that it represents committed typing input and
not a paste) and how it differs from ghostty_surface_text (paste), including any
expectations around bracketed-paste/protection behavior so embedders know not to
bypass those protections; place this comment immediately above the
ghostty_surface_text_input declaration to clarify usage for callers.
In `@src/build/LibtoolStep.zig`:
- Around line 35-38: The env_map only forwards PATH so TMPDIR is missing causing
the subprocess to fall back to /tmp; update the code that builds env_map (e.g.,
in LibtoolStep.zig — where env_map and run_step.env_map are set) to also check
env.get("TMPDIR") and, if present, env_map.put("TMPDIR", tmpdir) (with the same
OOM handling pattern) so the child process inherits the system/user temp dir;
apply the same change to XCFrameworkStep.zig, LipoStep.zig and any other step
that constructs env_map for subprocesses.
In `@src/renderer/generic.zig`:
- Around line 1500-1575: Replace the three iOS diagnostic log calls from
log.warn to log.debug inside the drawFrame flow: the "ios drawFrame early
presentLastTarget" block before try self.api.presentLastTarget(), the "ios
drawFrame wait_cells" block inside the size_changed check that calls
presentLastTarget(), and the "ios drawFrame no_redraw" block in the
!needs_redraw branch; update those three log.warn invocations to log.debug so
routine resize/redraw diagnostics use debug level instead of warn.
In `@src/renderer/Metal.zig`:
- Around line 264-304: Change the iOS diagnostic log level from warn to debug in
both places: the present flow where the code logs "ios present sync=...
surface=... layer_bounds=..." (the block that references target.surface and
self.layer.layer.getProperty) and in the pub inline fn presentLastTarget method
that logs "ios presentLastTarget surface=... layer_bounds=..." (the block that
references surface.getWidth/Height and self.layer.layer.getProperty). Replace
log.warn with log.debug in both locations, keeping the existing message string
and argument tuple intact so only the log level changes.
In `@src/renderer/metal/IOSurfaceLayer.zig`:
- Around line 140-164: Replace the noisy warning-level logs with debug-level
logging: in the setSurfaceCallback function change the two log.warn calls (the
"discard surface=..." and "present surface=... layer=...") to log.debug so
routine surface present/discard events don't clutter production logs, and
likewise change the log.warn in setSurfaceUncheckedCallback ("ios
setSurfaceUncheckedCallback present surface=...") to log.debug; keep the rest of
the handling (layer.setProperty("contents", surface), defer surface.release(),
etc.) unchanged.
In `@src/termio/Manual.zig`:
- Around line 100-125: Extract the duplicated test setup (the std.ArrayList u8
allocation/deinit and the write callback `cb`) into a small helper used by both
tests: create a helper (e.g., makeTestOutput) that allocates and returns the
`out` ArrayList plus a C-callback closure compatible with `Manual.init`
(`write_cb` and `write_userdata`) and a cleanup function or allocator so callers
can defer deinit; then replace the inline `var out: std.ArrayList(u8) = .empty`
and the `cb` struct in both tests with calls to this helper and use the returned
`write_cb` and `write_userdata` when calling `Manual.init` and
`manual.queueWrite` to remove duplication while preserving behavior.
🪄 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: a9cdbeae-5b34-4261-a1e8-8c4a63c3a25c
📒 Files selected for processing (17)
include/ghostty.hmacos/Sources/Ghostty/Ghostty.Surface.swiftpkg/zig-objc/build.zigsrc/Surface.zigsrc/apprt/embedded.zigsrc/build/GhosttyLib.zigsrc/build/GhosttyXCFramework.zigsrc/build/GhosttyXcodebuild.zigsrc/build/LibtoolStep.zigsrc/build/LipoStep.zigsrc/build/MetallibStep.zigsrc/build/XCFrameworkStep.zigsrc/input.zigsrc/renderer/Metal.zigsrc/renderer/generic.zigsrc/renderer/metal/IOSurfaceLayer.zigsrc/termio/Manual.zig
✅ Files skipped from review due to trivial changes (2)
- src/input.zig
- src/build/MetallibStep.zig
| fn completeTextInput( | ||
| self: *Surface, | ||
| data: []const u8, | ||
| ) !void { | ||
| if (data.len == 0) return; | ||
|
|
||
| var data_duped: ?[]u8 = null; | ||
| const encoded = input.text.encode(data) catch |err| switch (err) { | ||
| error.MutableRequired => encoded: { | ||
| const buf: []u8 = try self.alloc.dupe(u8, data); | ||
| errdefer self.alloc.free(buf); | ||
| data_duped = buf; | ||
| break :encoded input.text.encode(buf); | ||
| }, | ||
| }; | ||
| defer if (data_duped) |v| self.alloc.free(v); | ||
|
|
||
| if (self.child_exited) { | ||
| self.close(); | ||
| return; | ||
| } | ||
|
|
||
| self.queueIo(try termio.Message.writeReq( | ||
| self.alloc, | ||
| encoded, | ||
| ), .unlocked); |
There was a problem hiding this comment.
Honor keyboard-disable mode here too.
Lines 2762-2766 already suppress normal typing when DEC KAM / disable_keyboard is active, but this path writes committed text straight to the PTY. That lets IME/on-screen keyboard input—and every ghostty_surface_text_input caller—bypass keyboard-disable mode.
Suggested fix
fn completeTextInput(
self: *Surface,
data: []const u8,
) !void {
if (data.len == 0) return;
+
+ if (self.config.vt_kam_allowed) {
+ self.renderer_state.mutex.lock();
+ defer self.renderer_state.mutex.unlock();
+ if (self.io.terminal.modes.get(.disable_keyboard)) return;
+ }
var data_duped: ?[]u8 = null;
const encoded = input.text.encode(data) catch |err| switch (err) {📝 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.
| fn completeTextInput( | |
| self: *Surface, | |
| data: []const u8, | |
| ) !void { | |
| if (data.len == 0) return; | |
| var data_duped: ?[]u8 = null; | |
| const encoded = input.text.encode(data) catch |err| switch (err) { | |
| error.MutableRequired => encoded: { | |
| const buf: []u8 = try self.alloc.dupe(u8, data); | |
| errdefer self.alloc.free(buf); | |
| data_duped = buf; | |
| break :encoded input.text.encode(buf); | |
| }, | |
| }; | |
| defer if (data_duped) |v| self.alloc.free(v); | |
| if (self.child_exited) { | |
| self.close(); | |
| return; | |
| } | |
| self.queueIo(try termio.Message.writeReq( | |
| self.alloc, | |
| encoded, | |
| ), .unlocked); | |
| fn completeTextInput( | |
| self: *Surface, | |
| data: []const u8, | |
| ) !void { | |
| if (data.len == 0) return; | |
| if (self.config.vt_kam_allowed) { | |
| self.renderer_state.mutex.lock(); | |
| defer self.renderer_state.mutex.unlock(); | |
| if (self.io.terminal.modes.get(.disable_keyboard)) return; | |
| } | |
| var data_duped: ?[]u8 = null; | |
| const encoded = input.text.encode(data) catch |err| switch (err) { | |
| error.MutableRequired => encoded: { | |
| const buf: []u8 = try self.alloc.dupe(u8, data); | |
| errdefer self.alloc.free(buf); | |
| data_duped = buf; | |
| break :encoded input.text.encode(buf); | |
| }, | |
| }; | |
| defer if (data_duped) |v| self.alloc.free(v); | |
| if (self.child_exited) { | |
| self.close(); | |
| return; | |
| } | |
| self.queueIo(try termio.Message.writeReq( | |
| self.alloc, | |
| encoded, | |
| ), .unlocked); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Surface.zig` around lines 6363 - 6388, completeTextInput currently always
forwards committed text to the PTY, bypassing DEC KAM / disable_keyboard; add
the same guard used for normal typing to return early when the
Surface.disable_keyboard (or equivalent keyboard-disable flag) is set—i.e.,
after the empty-data check, check self.disable_keyboard and simply return
(without encoding or queueing the IO) so IME/on-screen keyboard input is
suppressed consistently with the typing path in the Surface code.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/input/text.zig (1)
18-21: Consider CRLF edge case.The
\n→\rreplacement would convert\r\n(CRLF) line endings to\r\r. If iOS text input can produce CRLF sequences (e.g., from pasted content), this may cause double carriage returns. If this is intentional, a brief doc comment noting the behavior would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/input/text.zig` around lines 18 - 21, The current comptime mutable branch uses std.mem.replaceScalar(u8, data, '\n', '\r') which turns CRLF ("\r\n") into "\r\r"; change the logic to first normalize CRLF sequences before swapping LF to CR (e.g., scan/replace "\r\n" -> "\n" or remove preceding '\r' when encountering '\n'), then run the single-byte replacement on remaining '\n' bytes; update the code paths that reference data and the comptime mutable branch (where std.mem.replaceScalar is called) to implement this two-step normalization, or add a short doc comment on the comptime mutable behavior if you intend to keep the current "\r\r" result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/input/text.zig`:
- Around line 18-21: The current comptime mutable branch uses
std.mem.replaceScalar(u8, data, '\n', '\r') which turns CRLF ("\r\n") into
"\r\r"; change the logic to first normalize CRLF sequences before swapping LF to
CR (e.g., scan/replace "\r\n" -> "\n" or remove preceding '\r' when encountering
'\n'), then run the single-byte replacement on remaining '\n' bytes; update the
code paths that reference data and the comptime mutable branch (where
std.mem.replaceScalar is called) to implement this two-step normalization, or
add a short doc comment on the comptime mutable behavior if you intend to keep
the current "\r\r" result.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff171a15f2
ℹ️ 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".
| if (sdkroot_override) |sdkroot| { | ||
| var libc: std.zig.LibCInstallation = .{}; | ||
| libc.include_dir = try std.fs.path.join(b.allocator, &.{ sdkroot, "usr", "include" }); | ||
| libc.sys_include_dir = try std.fs.path.join(b.allocator, &.{ sdkroot, "usr", "include" }); | ||
| break :libc libc; |
There was a problem hiding this comment.
Using SDKROOT unconditionally here makes one environment value override SDK discovery for every Apple target in the same zig build process. In this commit, GhosttyXCFramework now builds macOS, iOS, and iOS-simulator variants together (src/build/GhosttyXCFramework.zig), so exporting SDKROOT to a macOS SDK causes iOS/simulator compiles to pick macOS include/framework paths and fail. The override should only be used when it matches the current target SDK (or be made target-specific), otherwise fall back to findNative/xcrun per target.
Useful? React with 👍 / 👎.
On the embedded apprt (iOS), the renderer thread's main loop is never started, so the mailbox is never drained. Config changes pushed via ghostty_surface_update_config sit in the mailbox indefinitely. This means background color, palette, and font changes from config files are never applied to the renderer. Fix by draining the mailbox at the start of each draw call in embedded mode. This processes pending config changes, visibility updates, and other messages before rendering the frame.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/apprt/embedded.zig">
<violation number="1" location="src/apprt/embedded.zig:796">
P1: Avoid draining the renderer mailbox from the app thread. This creates a second consumer of renderer messages and runs renderer-thread state mutations on the wrong thread.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Instead of draining the full mailbox (which requires thread state that isn't initialized in embedded mode), apply the renderer DerivedConfig directly when ghostty_surface_update_config is called. This ensures background color, palette, and font changes take effect immediately without depending on the render thread's main loop.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/embedded.zig`:
- Around line 1623-1643: The created renderer_config (from
renderer.Renderer.DerivedConfig.init) allocates resources that must always be
freed; add a defer renderer_config.deinit() immediately after the init so deinit
runs on both success and failure paths, then remove the explicit deinit() in the
changeConfig error handler; keep calling
surface.core_surface.renderer.changeConfig(&renderer_config) and
surface.core_surface.updateConfig(config) as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| // In embedded mode, the renderer thread's mailbox is never drained | ||
| // (threadMain is not started). Apply config directly to the renderer | ||
| // instead of going through the mailbox. | ||
| const alloc = surface.core_surface.alloc; | ||
| var renderer_config = renderer.Renderer.DerivedConfig.init( | ||
| alloc, | ||
| config, | ||
| ) catch |err| { | ||
| log.err("error creating renderer config err={}", .{err}); | ||
| return; | ||
| }; | ||
| surface.core_surface.renderer.changeConfig(&renderer_config) catch |err| { | ||
| log.err("error applying renderer config err={}", .{err}); | ||
| renderer_config.deinit(); | ||
| return; | ||
| }; | ||
|
|
||
| // Also update via the normal path for non-renderer state | ||
| surface.core_surface.updateConfig(config) catch |err| { | ||
| log.err("error updating config err={}", .{err}); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how changeConfig handles the DerivedConfig pointer - does it store it or copy?
ast-grep --pattern $'pub fn changeConfig(self: *Self, config: *DerivedConfig) !void {
$$$
}'
# Also check if DerivedConfig has any fields that need cleanup
rg -n "pub const DerivedConfig" --type zig -A 20 | head -60Repository: manaflow-ai/ghostty
Length of output: 3414
🏁 Script executed:
# Find the changeConfig function in the renderer
rg -n "fn changeConfig" --type zig -A 15 src/renderer/Repository: manaflow-ai/ghostty
Length of output: 2260
🏁 Script executed:
# Get the full DerivedConfig definition from renderer/generic.zig around line 542
sed -n '542,580p' src/renderer/generic.zigRepository: manaflow-ai/ghostty
Length of output: 1872
🏁 Script executed:
# Check if DerivedConfig has a deinit method
rg -n "pub fn deinit" --type zig src/renderer/generic.zig -B 2 -A 5Repository: manaflow-ai/ghostty
Length of output: 1596
Add defer to ensure renderer_config.deinit() is always called.
The renderer_config is created with allocated fields (arena, links) that require cleanup. The changeConfig function only reads from the DerivedConfig pointer—it does not take ownership. Currently, deinit() is called on the error path but not on success, causing a memory leak every time the config is applied successfully.
Add defer renderer_config.deinit(); immediately after the init() call to ensure cleanup in both paths:
Suggested fix
var renderer_config = renderer.Renderer.DerivedConfig.init(
alloc,
config,
) catch |err| {
log.err("error creating renderer config err={}", .{err});
return;
};
defer renderer_config.deinit();
surface.core_surface.renderer.changeConfig(&renderer_config) catch |err| {
log.err("error applying renderer config err={}", .{err});
return;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/apprt/embedded.zig` around lines 1623 - 1643, The created renderer_config
(from renderer.Renderer.DerivedConfig.init) allocates resources that must always
be freed; add a defer renderer_config.deinit() immediately after the init so
deinit runs on both success and failure paths, then remove the explicit deinit()
in the changeConfig error handler; keep calling
surface.core_surface.renderer.changeConfig(&renderer_config) and
surface.core_surface.updateConfig(config) as before.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e177565c8
ℹ️ 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".
| surface.core_surface.renderer.changeConfig(&renderer_config) catch |err| { | ||
| log.err("error applying renderer config err={}", .{err}); | ||
| renderer_config.deinit(); | ||
| return; |
There was a problem hiding this comment.
This catch block can free memory that the renderer already adopted. renderer.changeConfig assigns self.config = config.* before executing fallible work (try self.prepBackgroundImage() in src/renderer/generic.zig), so an error can be returned after ownership has moved. Calling renderer_config.deinit() here can therefore trigger use-after-free/double-free on later renders when config application fails mid-flight (for example, with a new background image path that errors during decode/load).
Useful? React with 👍 / 👎.
| if (sdkroot_override) |sdkroot| { | ||
| gop.value_ptr.* = try b.allocator.dupe(u8, sdkroot); |
There was a problem hiding this comment.
The SDK override is applied unconditionally for every target in this dependency, so a single SDKROOT value is reused even when the build includes multiple Apple SDKs. In the same Zig invocation we build macOS, iOS, and iOS-simulator artifacts (see src/build/GhosttyXCFramework.zig), so exporting a macOS SDKROOT causes the iOS variants of zig_objc to pick macOS headers/framework paths and fail. The override should only be used when it matches m.resolved_target, otherwise fall back to per-target SDK discovery.
Useful? React with 👍 / 👎.
The renderer's changeConfig and drainMailbox crash in embedded mode because GPU resources aren't initialized for the render thread. Revert to simple updateConfig which pushes to mailbox. The initial surface creation should already have the right config from the app.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/generic.zig">
<violation number="1" location="src/renderer/generic.zig:1408">
P2: Avoid per-frame warn logging in the render loop. This will spam logs and add overhead for normal non‑black backgrounds.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/generic.zig">
<violation number="1" location="src/renderer/generic.zig:1408">
P2: Unconditional warn-level logging in the render update path will spam logs every frame and add avoidable overhead. Use debug-level logging or reintroduce a conditional guard so it only logs when needed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…lack On iOS embedded, the terminal state colors may not be initialized from config before the first drawFrame. Fall back to the renderer's config background color when terminal_state.colors.background is all zeros.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/generic.zig">
<violation number="1" location="src/renderer/generic.zig:1412">
P2: The new fallback treats pure black (`0,0,0`) as "unset", so valid terminal black backgrounds are overridden by config background.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 504b27a2ee
ℹ️ 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 bg = if (ts_bg.r != 0 or ts_bg.g != 0 or ts_bg.b != 0) | ||
| ts_bg | ||
| else |
There was a problem hiding this comment.
The new fallback treats RGB(0,0,0) as "unset" and replaces it with self.config.background, but 0,0,0 is also a valid terminal background color. Any app/theme that legitimately sets black (for example via OSC/SGR/default-color changes) will now render with the config background instead of black whenever that config is non-black, so the frame color becomes incorrect.
Useful? React with 👍 / 👎.
| .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.
Manual message handling now drops .start_synchronized_output entirely, which removes the reset timer previously started in Thread.drainMailbox. In manual/embedded mode, if the host enables synchronized output (mode 2026) and never sends the terminating reset sequence, rendering can remain paused indefinitely because the renderer skips frames while that mode is set; the watchdog was the safety net for this exact case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/generic.zig">
<violation number="1" location="src/renderer/generic.zig:1407">
P2: Hardcoding the background color to red overrides terminal/config background settings and will render all frames red. Restore the dynamic background color computation so user settings and terminal state are respected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5eb5833149
ℹ️ 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".
| @intFromFloat(@round(self.config.background_opacity * 255.0)), | ||
| }; | ||
| // EXPERIMENT: hardcode red background to test if the shader works at all | ||
| self.uniforms.bg_color = .{ 255, 0, 0, 255 }; |
There was a problem hiding this comment.
Replacing the background update with a constant .{ 255, 0, 0, 255 } forces a red background for normal rendering and ignores both terminal state and configured theme colors, so OSC/background updates and user color schemes no longer display correctly. This is a visible functional regression across regular draws, not just a debug-only path.
Useful? React with 👍 / 👎.
| text.withCString { ptr in | ||
| // len includes the null terminator so we do len - 1 | ||
| ghostty_surface_text(surface, ptr, UInt(len - 1)) | ||
| ghostty_surface_text_input(surface, ptr, UInt(len - 1)) |
There was a problem hiding this comment.
Switching sendText to ghostty_surface_text_input removes paste encoding/protection for every caller of this method; unlike ghostty_surface_text, the new path skips bracketed-paste framing and control-byte filtering. In this codebase, drag-and-drop text is routed through insertText into sendText (SurfaceView_AppKit.performDragOperation), so dropped content now reaches the terminal as raw typed input and can execute multiline/untrusted payloads immediately.
Useful? React with 👍 / 👎.
On iOS embedded, the size_changed and stale-grid guards in drawFrame call presentLastTarget() and return before the bg_color shader runs. This means the background color from config is never rendered, showing black forever. Skip these guards on iOS so the full render pipeline runs and the bg_color uniform actually gets painted.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88b9fc6ff2
ℹ️ 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".
| .tl_px_x = -1, | ||
| .tl_px_y = -1, | ||
| .offset_start = 0, | ||
| .offset_len = 0, |
There was a problem hiding this comment.
ghostty_surface_read_text_html is documented as "same as" ghostty_surface_read_text, but this path hardcodes tl_px_* and offset_* to sentinel values instead of returning the real selection viewport metadata. Any embedder that relies on those fields for placement/range mapping will get incorrect coordinates and offsets when using the HTML API for the same selection. Populate these fields using the same viewport calculation used by readTextLocked so both APIs stay behaviorally compatible.
Useful? React with 👍 / 👎.
…ream-sync Merge upstream-main into main
1. IOSurface 64-byte row alignment for Metal on iOS 2. CAIOSurfaceLayer base class on iOS for compositing 3. 1px size tolerance for Retina floating-point rounding 4. Dynamic contentsScale correction 5. loopEnter setNeedsDisplay to trigger first frame 6. setSurface (main-thread-safe) instead of setSurfaceSync on iOS 7. Proper deinit: clear display callback, remove sublayer 8. loopExit: clear display callback when renderer thread exits 9. Disable CFRelease background thread on iOS 10. Add ghostty_surface_draw_now() C API for CADisplayLink pacing 11. Restore drawFrame early-return guards (needed for correct rendering)
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
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/generic.zig">
<violation number="1">
P2: This unconditional `presentLastTarget` + `return` reintroduces the iOS early-return path during resize/grid mismatch, which can prevent the first real frame/background from rendering.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -1534,6 +1534,18 @@ pub fn Renderer(comptime GraphicsAPI: type) type { | |||
| sync; | |||
There was a problem hiding this comment.
P2: This unconditional presentLastTarget + return reintroduces the iOS early-return path during resize/grid mismatch, which can prevent the first real frame/background from rendering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/renderer/generic.zig, line 1500:
<comment>This unconditional `presentLastTarget` + `return` reintroduces the iOS early-return path during resize/grid mismatch, which can prevent the first real frame/background from rendering.</comment>
<file context>
@@ -1497,13 +1497,8 @@ pub fn Renderer(comptime GraphicsAPI: type) type {
- try self.api.presentLastTarget();
- return;
- }
+ try self.api.presentLastTarget();
+ return;
}
</file context>
| sync; | |
| if (comptime builtin.os.tag != .ios) { | |
| try self.api.presentLastTarget(); | |
| return; | |
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc0ee3142f
ℹ️ 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".
| switch (self.backend) { | ||
| .manual => { | ||
| self.queueMessageManual(msg); | ||
| return; |
There was a problem hiding this comment.
The manual fast-path bypasses mailbox.send(..., .locked) and executes queueMessageManual inline, so writes that are queued while renderer_state.mutex is already held (for example the .locked queueIo calls in Surface) now invoke io_write_cb under that lock. If an embedder's write callback synchronously feeds bytes back via ghostty_surface_process_output, Termio.processOutput tries to lock the same mutex and can deadlock on manual/embedded sessions. Keeping the mailbox path (or explicitly dropping the lock before invoking manual writes) avoids this re-entrant lock inversion.
Useful? React with 👍 / 👎.
1 participant
This makes the local Apple SDK helper honor
SDKROOTand vendorszig_objcso local Zig builds can use the same override path.Needed for local cmux daemon builds on Xcode 26.4, where the Xcode macOS SDK stubs broke Zig 0.15.2 linking for
arm64-macos.Tested with a local raw Zig daemon build and a tagged local cmux reload.
Summary by cubic
Honors
SDKROOTfor Apple SDK discovery, vendorszig_objc, and ships a reliable iOS manual I/O path with IOSurface rendering and committed text input. Also hardens iOS Metal rendering and XCFramework builds to unblock local cmux on Xcode 26.4 (arm64‑macOS, Zig 0.15.2).New Features
.manual; newghostty_surface_draw_nowfor CADisplayLink pacing.ghostty_surface_text_inputandsrc/input/text.zigto encode committed text (LF→CR).dumpHTMLLockedandghostty_surface_read_text_htmlfor HTML output.opaquecompositing, contentsScale fix with 1px tolerance, first-frame trigger viasetNeedsDisplay, safe teardown (clear display callback, remove sublayer), restored draw guards, and CoreText CFRelease thread disabled on iOS.Dependencies
zig_objcunderpkg/zig-objc;build.zig.zonpointszig_objcto the local path.pkg/apple-sdk/build.zig: honorsSDKROOT(includes/libs), falling back toxcrun.libtoolfor large object lists, and a link‑order fix.Written for commit bc0ee31. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Chores