From 8986537d93b6275dba58b70149f48468803fb9e1 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Sun, 28 Jun 2026 07:20:19 -0700 Subject: [PATCH] fix visuals on review tab --- src/tui.zig | 128 ++++++++++++++++++++++++--- src/tui/review_tab.zig | 192 +++++++++++++++++++++++++++++++++++------ 2 files changed, 282 insertions(+), 38 deletions(-) diff --git a/src/tui.zig b/src/tui.zig index d44f19a..64ab42a 100644 --- a/src/tui.zig +++ b/src/tui.zig @@ -747,8 +747,7 @@ pub const App = struct { if (mouse.col >= col and mouse.col < col + lbl_len) { if (self.isDisabled(t)) return; self.active_tab = t; - self.scroll_offset = 0; - self.loadTabData(); + self.switchedToTab(); ctx.queueRefresh() catch |err| std.log.debug("queueRefresh failed: {t}", .{err}); return ctx.consumeAndRedraw(); } @@ -1048,7 +1047,7 @@ pub const App = struct { if (self.input_len > 0) { self.setActiveSymbol(self.input_buf[0..self.input_len]); self.active_tab = .quote; - self.loadTabData(); + self.switchedToTab(); ctx.queueRefresh() catch |err| std.log.debug("queueRefresh failed: {t}", .{err}); } self.mode = .normal; @@ -1061,6 +1060,10 @@ pub const App = struct { fn handleNormalKey(self: *App, ctx: *vaxis.vxfw.EventContext, key: vaxis.Key) void { // Ctrl+L: full screen redraw (standard TUI convention, not configurable) if (key.codepoint == 'l' and key.mods.ctrl) { + // Also drop any status override so the bar returns to + // the active tab's default hint - "redraw" should make + // the screen pristine, status line included. + self.resetStatus(); ctx.queueRefresh() catch |err| std.log.debug("queueRefresh failed: {t}", .{err}); return ctx.consumeAndRedraw(); } @@ -1100,15 +1103,13 @@ pub const App = struct { }, .prev_tab => { self.prevTab(); - self.scroll_offset = 0; - self.loadTabData(); + self.switchedToTab(); ctx.queueRefresh() catch |err| std.log.debug("queueRefresh failed: {t}", .{err}); return ctx.consumeAndRedraw(); }, .next_tab => { self.nextTab(); - self.scroll_offset = 0; - self.loadTabData(); + self.switchedToTab(); ctx.queueRefresh() catch |err| std.log.debug("queueRefresh failed: {t}", .{err}); return ctx.consumeAndRedraw(); }, @@ -1118,8 +1119,7 @@ pub const App = struct { const target = tabs[idx]; if (self.isDisabled(target)) return; self.active_tab = target; - self.scroll_offset = 0; - self.loadTabData(); + self.switchedToTab(); ctx.queueRefresh() catch |err| std.log.debug("queueRefresh failed: {t}", .{err}); return ctx.consumeAndRedraw(); } @@ -1374,6 +1374,27 @@ pub const App = struct { self.status_len = len; } + /// Drop any active status override so the next draw falls back + /// to the active tab's default hint (globals + `status_hints`). + /// Called on navigation events - tab switches and the Ctrl+L + /// redraw - so a stale message set on one screen can't linger + /// past the point where it stops being relevant. + pub fn resetStatus(self: *App) void { + self.status_len = 0; + } + + /// Common tail for every "the active tab changed" path: reset + /// the viewport scroll, drop any stale status override (so the + /// new tab shows its own default hint unless its `activate` + /// sets something), and load the new tab's data. Centralized so + /// no switch site can forget a step. Callers set `active_tab` + /// (directly or via `nextTab`/`prevTab`) before calling this. + fn switchedToTab(self: *App) void { + self.scroll_offset = 0; + self.resetStatus(); + self.loadTabData(); + } + /// Cell pixel size for the active terminal, used by tabs that /// render bitmap charts via the Kitty graphics protocol. Falls /// back to (8, 16) when vaxis hasn't reported pixel dimensions @@ -1728,14 +1749,37 @@ pub const App = struct { // ASCII: single byte, single column buf[row * width + col] = .{ .char = .{ .grapheme = ascii_g[byte] }, .style = s }; bi += 1; + col += 1; } else { // Multi-byte UTF-8: determine sequence length const seq_len: usize = if (byte >= 0xF0) 4 else if (byte >= 0xE0) 3 else if (byte >= 0xC0) 2 else 1; - const end = @min(bi + seq_len, line.text.len); - buf[row * width + col] = .{ .char = .{ .grapheme = line.text[bi..end] }, .style = s }; + var end = @min(bi + seq_len, line.text.len); + // Fold a trailing U+FE0F (bytes EF B8 8F) variation + // selector into THIS cell and report the cell as 2 + // display columns wide. zfin's wide glyphs (severity + // / status-grid emoji) all carry FE0F to force emoji + // presentation; vaxis defaults a cell to width 1 and + // only measures the grapheme when width == 0, so + // without this it tracks each emoji as a single + // column. Its partial-redraw diff then advances the + // terminal cursor one column short per emoji, which + // is what smears stale glyphs across the screen as + // the findings list scrolls. Telling it the true + // width keeps the cursor accounting aligned. The + // skipped second column stays the row-fill blank; + // vaxis skips it because it advances its buffer index + // by the cell width. Only fold when both columns fit. + var cell_width: u8 = 1; + if (col + 1 < width and end + 3 <= line.text.len and + line.text[end] == 0xEF and line.text[end + 1] == 0xB8 and line.text[end + 2] == 0x8F) + { + end += 3; + cell_width = 2; + } + buf[row * width + col] = .{ .char = .{ .grapheme = line.text[bi..end], .width = cell_width }, .style = s }; bi = end; + col += cell_width; } - col += 1; } } } @@ -2843,6 +2887,66 @@ test "formatStatusHint: single fragment has no separator" { try testing.expectEqualStrings("ctrl+s save", out); } +test "resetStatus: clears an active status override" { + var app: App = undefined; + app.setStatus("stuck message"); + try testing.expect(app.status_len > 0); + app.resetStatus(); + try testing.expectEqual(@as(usize, 0), app.status_len); +} + +// ── drawStyledContent: wide-glyph (FE0F) width handling ──────── + +test "drawStyledContent: folds a trailing FE0F into one width-2 cell" { + const w: u16 = 8; + var buf: [w]vaxis.Cell = undefined; + const lines = [_]StyledLine{ + .{ .text = "a\u{26A0}\u{FE0F}b", .style = .{} }, // a + warning-emoji + b + }; + try App.drawStyledContent(undefined, undefined, &buf, w, 1, &lines); + + // col 0: plain ASCII, default width 1. + try testing.expectEqualStrings("a", buf[0].char.grapheme); + try testing.expectEqual(@as(u8, 1), buf[0].char.width); + + // col 1: both codepoints (base + FE0F) in one cell, 2 cols wide. + try testing.expectEqualStrings("\u{26A0}\u{FE0F}", buf[1].char.grapheme); + try testing.expectEqual(@as(u8, 2), buf[1].char.width); + + // col 2: the emoji's second column - left as the row-fill blank; + // vaxis skips it because the prior cell reports width 2. + try testing.expectEqualStrings(" ", buf[2].char.grapheme); + + // col 3: the next glyph lands after the 2-wide emoji, not at col 2. + try testing.expectEqualStrings("b", buf[3].char.grapheme); +} + +test "drawStyledContent: a multibyte glyph without FE0F stays one column" { + const w: u16 = 4; + var buf: [w]vaxis.Cell = undefined; + const lines = [_]StyledLine{ + .{ .text = "\u{2014}x", .style = .{} }, // em-dash (1 col) + x + }; + try App.drawStyledContent(undefined, undefined, &buf, w, 1, &lines); + try testing.expectEqualStrings("\u{2014}", buf[0].char.grapheme); + try testing.expectEqual(@as(u8, 1), buf[0].char.width); + try testing.expectEqualStrings("x", buf[1].char.grapheme); +} + +test "drawStyledContent: an FE0F emoji with no room for two columns is not folded" { + // width 2: 'a' takes col 0, leaving only col 1 - not enough room + // for a 2-wide cell, so the emoji is left at width 1 (truncation + // territory at the right edge, same as before the fold). + const w: u16 = 2; + var buf: [w]vaxis.Cell = undefined; + const lines = [_]StyledLine{ + .{ .text = "a\u{26A0}\u{FE0F}", .style = .{} }, + }; + try App.drawStyledContent(undefined, undefined, &buf, w, 1, &lines); + try testing.expectEqualStrings("a", buf[0].char.grapheme); + try testing.expectEqual(@as(u8, 1), buf[1].char.width); +} + // ── symbol toggle helpers ───────────────────────────────────── test "shouldStashSymbol: stashes a differing non-empty symbol" { diff --git a/src/tui/review_tab.zig b/src/tui/review_tab.zig index c17179b..c1f79db 100644 --- a/src/tui/review_tab.zig +++ b/src/tui/review_tab.zig @@ -269,6 +269,22 @@ pub const tab = struct { pub const deactivate = framework.noopDeactivate(State); + /// Status-bar override. While collecting an ack note, render the + /// note-entry key hints full-width; otherwise the App-level + /// default status applies. Driving the prompt off `input_mode` + /// (instead of a one-shot `setStatus`) means it appears exactly + /// when the modal is open and the App restores the tab's default + /// hint the instant we leave it - no frozen prompt after Esc or + /// commit. It also marks the tab modal, so a stray tab-bar click + /// can't switch tabs out from under a half-typed note. + pub fn statusOverride(state: *State, app: *App) ?framework.StatusOverride { + _ = app; + return switch (state.input_mode) { + .normal => null, + .ack_note => .{ .hint = "Type reasoning. Enter = next line. Ctrl+Enter = save. Esc = cancel." }, + }; + } + /// Framework poll-tick hook: true while the observation panel /// has async checks in flight. Drives the App's poll timer so /// the `tick` hook below gets called without user input. The @@ -416,6 +432,11 @@ pub const tab = struct { /// Wheel events fall through to App's scroll handling via /// `onWheelMove` returning false (see below). pub fn handleMouse(state: *State, app: *App, mouse: vaxis.Mouse) bool { + // While collecting an ack note the tab is modal (statusOverride + // is non-null, so the App routes every mouse event here). + // Swallow them all: a click that moved the cursor mid-note + // would make commitAckNote write to a different finding. + if (state.input_mode == .ack_note) return true; if (mouse.button != .left) return false; if (mouse.type != .press) return false; const view = state.view orelse return false; @@ -542,6 +563,9 @@ pub const tab = struct { return true; }; state.note_len = 0; + // The bar just grew by a line; keep its bottom on + // screen so the next line the user types is visible. + ensureAckInputVisible(state, app); return true; }, .committed => { @@ -667,15 +691,37 @@ fn wrapCursor(current: usize, delta: isize, total: usize) usize { /// (acceptable - the cursor IS at row zero anyway). fn ensureCursorVisible(state: *const State, scroll_offset: *usize, visible_height: usize) void { const visual = cursorVisualRow(state) orelse return; - if (visual < scroll_offset.*) { - scroll_offset.* = visual; - return; - } - if (visible_height > 0 and visual >= scroll_offset.* + visible_height) { - scroll_offset.* = visual - visible_height + 1; + ensureRowVisible(visual, scroll_offset, visible_height); +} + +/// Scroll `scroll_offset` the minimum amount so content row +/// `target` lands inside the `[scroll_offset, scroll_offset + +/// visible_height)` viewport. Pure over the offset pointer; +/// shared by the cursor-follow path and the ack-input-follow path. +fn ensureRowVisible(target: usize, scroll_offset: *usize, visible_height: usize) void { + if (target < scroll_offset.*) { + scroll_offset.* = target; + } else if (visible_height > 0 and target >= scroll_offset.* + visible_height) { + scroll_offset.* = target - visible_height + 1; } } +/// While the inline note-input bar is open, keep its bottom line +/// (the hint row, below the in-progress input line) on screen. +/// Without this, acknowledging a finding that sits on the last +/// visible row opens the whole input bar below the viewport and +/// the user types blind. Called when entering ack mode and after +/// each committed fragment (which grows the bar by one line). +fn ensureAckInputVisible(state: *State, app: *App) void { + if (state.input_mode != .ack_note) return; + const cv = cursorVisualRow(state) orelse return; + const fv = state.findings_view orelse return; + const local = cursorLocalIndex(state); + if (local >= fv.rows.len) return; + const bottom = cv + expansionLineCount(fv.rows[local]) + inputBarLineCount(state); + ensureRowVisible(bottom, &app.scroll_offset, app.visible_height); +} + /// Compute the cursor's content-area row index (0 = first /// rendered line). Returns null when neither view is loaded or /// when the section indices haven't been populated yet (first @@ -765,7 +811,13 @@ fn ackCurrentFinding(state: *State, app: *App) void { // input mode from `.normal`, and `commitAckNote`/`cancelAckNote` // both clear it on exit). Defensive: clear anyway. clearNoteFragments(state, app.allocator); - app.setStatus("Type reasoning. Enter = next line. Ctrl+Enter = save. Esc = cancel."); + // No setStatus: the `statusOverride` hook renders the input + // prompt while `input_mode == .ack_note` and the App restores + // the tab's default hint automatically when we exit the mode + // (on Esc or commit) - no stale prompt left frozen on the bar. + // Scroll so the freshly-opened input bar is actually on screen + // even when the finding sat on the last visible row. + ensureAckInputVisible(state, app); } /// Un-acknowledge the cursor-selected finding. Flips the journal @@ -876,7 +928,9 @@ fn cancelAckNote(state: *State, app: *App) void { state.input_mode = .normal; state.note_len = 0; clearNoteFragments(state, app.allocator); - // No setStatus: the disappearing input bar is its own feedback. + // No setStatus needed: flipping `input_mode` back to `.normal` + // makes `statusOverride` return null, so the App falls straight + // back to the tab's default hint - the bar visibly resets. } /// Commit the in-progress ack note: dupe any final unflushed @@ -943,10 +997,11 @@ fn commitAckNote(state: *State, app: *App) void { }; rebuildFindingsView(state, app); - // No setStatus on success: the visible removal of the row - // from the findings list (or the "[acked]" prefix when - // show_acked is on) is feedback enough. Leaving the prior - // help/hint visible is the user's preference. + // No setStatus on success: the `defer` above flips `input_mode` + // back to `.normal`, so `statusOverride` returns null and the + // bar falls back to the tab's default hint. The row leaving the + // findings list (or gaining an "[acked]" prefix when show_acked + // is on) is the confirmation. } // ── Data loading ────────────────────────────────────────────── @@ -1416,19 +1471,16 @@ pub fn buildStyledLines(state: *State, app: *App, arena: std.mem.Allocator) ![]c return lines.toOwnedSlice(arena); } -/// Glyph used in the findings table's severity column. Each is two -/// display columns wide (emoji-presentation) so renderers don't have -/// to width-correct. /// Per-finding-row glyph indicating severity. Each glyph string /// includes a trailing U+FE0F variation selector to force emoji -/// presentation. The variation selector also serves a critical -/// rendering role here: `drawStyledContent` allocates one buffer -/// cell per UTF-8 sequence (advancing col by 1), so a single- -/// codepoint emoji like ❌ takes 1 buffer cell while terminals -/// render it as 2 visual cols, desyncing the col counter from -/// terminal display. Appending FE0F gives it a second codepoint -/// (which gets its own cell) so buffer-col advancement matches -/// terminal-col advancement at exactly 2 per emoji. +/// (two-display-column) presentation. The selector is also the +/// signal `drawStyledContent` keys off: when it sees a sequence +/// followed by FE0F it folds both codepoints into one buffer cell +/// and marks it `width = 2`, so vaxis advances the terminal cursor +/// by the true width. Without that, vaxis would track the emoji as +/// one column and its scroll-time partial redraws would smear stale +/// glyphs. (The renderer owns the width fix now; the FE0F here just +/// guarantees the glyph it's correcting is genuinely 2 wide.) fn severityGlyph(sev: observations.Severity) []const u8 { return switch (sev) { .warn => "⚠️", // U+26A0 + FE0F @@ -1447,9 +1499,8 @@ fn severityGlyph(sev: observations.Severity) []const u8 { /// in the renderer means the renderer is ready when the async /// path lands. /// Per-check status-grid glyph. See `severityGlyph` for the -/// FE0F-trailing convention - it forces emoji presentation and -/// gives the renderer a second cell to track so buffer-col -/// advancement matches terminal-col advancement. +/// FE0F-trailing convention - it forces emoji presentation and lets +/// `drawStyledContent` mark the cell two columns wide. fn checkStatusGlyph(result: observations.CheckResult) []const u8 { return switch (result) { .pass => "✅\u{FE0F}", @@ -2916,6 +2967,95 @@ test "onCursorMove: empty table returns false" { state.findings_view = null; } +// ── ensureRowVisible / ack-input scroll-into-view ────────────── + +test "ensureRowVisible: scrolls down so a below-viewport row is last visible" { + var off: usize = 0; + ensureRowVisible(23, &off, 10); + try testing.expectEqual(@as(usize, 14), off); // 23 - 10 + 1 +} + +test "ensureRowVisible: scrolls up so an above-viewport row is first visible" { + var off: usize = 30; + ensureRowVisible(5, &off, 10); + try testing.expectEqual(@as(usize, 5), off); +} + +test "ensureRowVisible: no change when the row is already on screen" { + var off: usize = 10; + ensureRowVisible(12, &off, 10); + try testing.expectEqual(@as(usize, 10), off); +} + +test "ensureAckInputVisible: pulls a bottom-row ack input bar onto screen" { + // One finding, no holdings -> unified cursor 0 is that finding. + // Pretend it renders at content row 20 (findings_first_row) and + // the viewport is only 10 rows tall starting at offset 0, so the + // freshly-opened input bar (detail + in-progress + hint = 3 rows + // below the finding) sits entirely below the fold. + var findings = [_]observations_view.FindingRow{ + .{ .severity = .warn, .kind = "k", .target = "t", .text = "x", .is_acked = false }, + }; + const empty_view: review_view.ReviewView = .{ + .rows = &.{}, + .totals = std.mem.zeroes(review_view.ReviewTotals), + .as_of = zfin.Date.fromYmd(2026, 6, 8), + .total_liquid = 0, + .portfolio_path = "x", + }; + var state: State = .{ + .view = empty_view, + .cursor = 0, + .expanded_finding = 0, + .input_mode = .ack_note, + .findings_first_row = 20, + .findings_view = .{ + .rows = &findings, + .total_active = 1, + .total_acked = 0, + .total_resolved = 0, + }, + }; + var app: App = undefined; + app.scroll_offset = 0; + app.visible_height = 10; + + ensureAckInputVisible(&state, &app); + // finding at row 20; bottom of the bar at 20 + 1 + 2 = 23; the + // offset advances so row 23 is the last visible: 23 - 10 + 1 = 14. + try testing.expectEqual(@as(usize, 14), app.scroll_offset); + + state.view = null; + state.findings_view = null; +} + +test "ensureAckInputVisible: no-op when not collecting a note" { + var state: State = .{ .input_mode = .normal, .findings_first_row = 20 }; + var app: App = undefined; + app.scroll_offset = 7; + app.visible_height = 10; + ensureAckInputVisible(&state, &app); + try testing.expectEqual(@as(usize, 7), app.scroll_offset); +} + +// ── statusOverride (ack-note prompt) ─────────────────────────── + +test "statusOverride: null in normal mode, key hints while collecting a note" { + var state: State = .{}; + var app: App = undefined; + try testing.expect(tab.statusOverride(&state, &app) == null); + + state.input_mode = .ack_note; + const ov = tab.statusOverride(&state, &app) orelse return error.TestUnexpectedResult; + switch (ov) { + .hint => |hint| { + try testing.expect(std.mem.indexOf(u8, hint, "Ctrl+Enter") != null); + try testing.expect(std.mem.indexOf(u8, hint, "Esc") != null); + }, + else => return error.TestUnexpectedResult, + } +} + test "onCursorMove: wheel-sized delta still moves by one row" { // Wheel events arrive as ±3 per detent. The cursor should // step by ±1 regardless - wheel == j/k, not "skip three rows".