Skip to content

Commit 2c7d44e

Browse files
committed
Fix code review issues and CI build failure
This commit addresses all review comments from Copilot and fixes the GitHub Actions build failure: Build Fix: - Made cwd.zig macOS-only with comptime check to prevent Linux build failures - Added conditional import of cwd_mod in state.zig (struct{} for non-macOS) - Added early return in updateCwd for non-macOS platforms Review #1 - Ping-pong animation: - Replaced sawtooth animation with smooth ping-pong pattern - Animation now includes forward scroll, backward scroll, and idle at both ends - Eliminates jarring jump back to start Review #2 - Fade conditions: - Fixed inverted fade logic - fade_left now correctly triggers when scroll_offset > 0 - fade_right now correctly triggers when scroll_offset < scroll_range Review #3 - Documentation: - Added documentation clarifying cwd_basename is a subslice of cwd_path - Documents lifetime dependency between the two fields Review #4 & #5 - Silent failure logging: - Added log warnings for basename buffer overflow - Added log warnings for parent path buffer overflow - Helps debugging when paths exceed buffer limits Review #6 - Remove unused field: - Removed cwd_marquee_offset field (animation uses current_time instead) - Cleaned up unused code All fixes tested with: - zig build: Success - zig build test: All tests pass - zig fmt src/: Code formatted
1 parent 7c7de37 commit 2c7d44e

3 files changed

Lines changed: 49 additions & 11 deletions

File tree

src/cwd.zig

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,15 @@
11
const std = @import("std");
22
const posix = std.posix;
3+
const builtin = @import("builtin");
34

45
const log = std.log.scoped(.cwd);
56

7+
comptime {
8+
if (builtin.os.tag != .macos) {
9+
@compileError("cwd.zig: proc_pidinfo API is macOS-specific. This module should only be compiled on macOS.");
10+
}
11+
}
12+
613
pub const CwdError = error{
714
ProcessNotFound,
815
BufferTooSmall,

src/render/renderer.zig

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -786,7 +786,13 @@ fn renderCwdBar(
786786
if (std.mem.eql(u8, cwd_basename, "/")) {
787787
break :blk cwd_basename;
788788
}
789-
if (cwd_basename.len + 1 > basename_with_slash_buf.len) return;
789+
if (cwd_basename.len + 1 > basename_with_slash_buf.len) {
790+
log.warn("CWD basename too long for buffer (len={}, max_without_slash={}); skipping CWD bar rendering", .{
791+
cwd_basename.len,
792+
basename_with_slash_buf.len - 1,
793+
});
794+
return;
795+
}
790796
@memcpy(basename_with_slash_buf[0..cwd_basename.len], cwd_basename);
791797
basename_with_slash_buf[cwd_basename.len] = '/';
792798
break :blk basename_with_slash_buf[0 .. cwd_basename.len + 1];
@@ -825,7 +831,13 @@ fn renderCwdBar(
825831
if (parent_without_slash[parent_without_slash.len - 1] == '/') {
826832
break :blk parent_without_slash;
827833
} else {
828-
if (parent_without_slash.len + 1 > parent_path_buf.len) return;
834+
if (parent_without_slash.len + 1 > parent_path_buf.len) {
835+
log.warn(
836+
"render: parent path too long (required={} bytes, buffer size={}), skipping parent path rendering",
837+
.{ parent_without_slash.len + 1, parent_path_buf.len },
838+
);
839+
return;
840+
}
829841
@memcpy(parent_path_buf[0..parent_without_slash.len], parent_without_slash);
830842
parent_path_buf[parent_without_slash.len] = '/';
831843
break :blk parent_path_buf[0 .. parent_without_slash.len + 1];
@@ -868,17 +880,33 @@ fn renderCwdBar(
868880
const scroll_range_f: f32 = @floatFromInt(scroll_range);
869881
const idle_ms: f32 = 1000.0;
870882
const scroll_ms: f32 = scroll_range_f / MARQUEE_SPEED * 1000.0;
871-
const cycle_ms: f32 = idle_ms * 2.0 + scroll_ms;
883+
const cycle_ms: f32 = idle_ms * 2.0 + scroll_ms * 2.0;
872884
const cycle_ms_i64: i64 = @max(1, @as(i64, @intFromFloat(std.math.ceil(cycle_ms))));
873885
const elapsed_ms: f32 = @floatFromInt(@mod(current_time, cycle_ms_i64));
874886

875887
const scroll_offset: c_int = blk: {
888+
// Phase 1: initial idle at start (offset = 0)
876889
if (elapsed_ms < idle_ms) break :blk 0;
877-
if (elapsed_ms < idle_ms + scroll_ms) {
878-
const progress = (elapsed_ms - idle_ms) / scroll_ms;
890+
891+
// Phase 2: forward scroll from 0 to scroll_range
892+
const forward_start = idle_ms;
893+
const forward_end = idle_ms + scroll_ms;
894+
if (elapsed_ms < forward_end) {
895+
const progress = (elapsed_ms - forward_start) / scroll_ms;
879896
break :blk @intFromFloat(progress * scroll_range_f);
880897
}
881-
break :blk scroll_range;
898+
899+
// Phase 3: idle at end (offset = scroll_range)
900+
const end_idle_start = forward_end;
901+
const end_idle_end = end_idle_start + idle_ms;
902+
if (elapsed_ms < end_idle_end) {
903+
break :blk scroll_range;
904+
}
905+
906+
// Phase 4: backward scroll from scroll_range back to 0
907+
const backward_start = end_idle_end;
908+
const progress = (elapsed_ms - backward_start) / scroll_ms;
909+
break :blk @intFromFloat((1.0 - progress) * scroll_range_f);
882910
};
883911

884912
const parent_x = basename_x - parent_width + scroll_offset;
@@ -891,8 +919,8 @@ fn renderCwdBar(
891919

892920
_ = c.SDL_SetRenderClipRect(renderer, null);
893921

894-
const fade_left = scroll_offset < scroll_range;
895-
const fade_right = scroll_offset > 0;
922+
const fade_left = scroll_offset > 0;
923+
const fade_right = scroll_offset < scroll_range;
896924

897925
if (fade_left) {
898926
renderFadeGradient(renderer, bar_rect, true);

src/session/state.zig

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
const std = @import("std");
22
const posix = std.posix;
3+
const builtin = @import("builtin");
34
const ghostty_vt = @import("ghostty-vt");
45
const shell_mod = @import("../shell.zig");
56
const pty_mod = @import("../pty.zig");
67
const app_state = @import("../app/app_state.zig");
78
const c = @import("../c.zig");
8-
const cwd_mod = @import("../cwd.zig");
9+
const cwd_mod = if (builtin.os.tag == .macos) @import("../cwd.zig") else struct {};
910

1011
const log = std.log.scoped(.session_state);
1112

@@ -40,9 +41,10 @@ pub const SessionState = struct {
4041
notify_sock_z: [:0]const u8,
4142
allocator: std.mem.Allocator,
4243
cwd_path: ?[]const u8 = null,
44+
/// Subslice of cwd_path pointing to the basename. Always points within cwd_path's memory.
45+
/// When cwd_path is freed, this becomes invalid and must not be used.
4346
cwd_basename: ?[]const u8 = null,
4447
cwd_last_check: i64 = 0,
45-
cwd_marquee_offset: f32 = 0,
4648

4749
pub const InitError = shell_mod.Shell.SpawnError || MakeNonBlockingError || error{
4850
DivisionByZero,
@@ -222,6 +224,8 @@ pub const SessionState = struct {
222224
}
223225

224226
pub fn updateCwd(self: *SessionState, current_time: i64) void {
227+
if (builtin.os.tag != .macos) return;
228+
225229
if (!self.spawned or self.dead) return;
226230

227231
const shell = self.shell orelse return;
@@ -244,7 +248,6 @@ pub const SessionState = struct {
244248

245249
self.cwd_path = new_path;
246250
self.cwd_basename = cwd_mod.getBasename(new_path);
247-
self.cwd_marquee_offset = 0;
248251
self.dirty = true;
249252
}
250253
};

0 commit comments

Comments
 (0)