From 93e81b841bf5aca4acf03097c952a0a455951eb6 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Thu, 27 Aug 2026 15:07:22 -0700 Subject: [PATCH] centralize and test/disambiguate row data in portfolio --- src/tui/portfolio_tab.zig | 123 +++++++-------- src/views/portfolio_sections.zig | 247 +++++++++++++++++++++++++++++++ 2 files changed, 302 insertions(+), 68 deletions(-) diff --git a/src/tui/portfolio_tab.zig b/src/tui/portfolio_tab.zig index 69dba6d..8ef9456 100644 --- a/src/tui/portfolio_tab.zig +++ b/src/tui/portfolio_tab.zig @@ -1943,10 +1943,10 @@ pub fn drawContent(state: *State, app: *App, arena: std.mem.Allocator, buf: []va std.fmt.bufPrint(&pnl_buf, "-{s}", .{gl_money}) catch "?"; var mv_buf: [24]u8 = undefined; const mv_str = std.fmt.bufPrint(&mv_buf, "{f}", .{Money.from(display_mv)}) catch "$?"; - var cost_buf2: [24]u8 = undefined; - const cost_str = std.fmt.bufPrint(&cost_buf2, "{f}", .{Money.from(display_avg_cost)}) catch "$?"; - var price_buf2: [24]u8 = undefined; - const price_str = std.fmt.bufPrint(&price_buf2, "{f}", .{Money.from(a.current_price)}) catch "$?"; + var cost_buf: [24]u8 = undefined; + const cost_str = std.fmt.bufPrint(&cost_buf, "{f}", .{Money.from(display_avg_cost)}) catch "$?"; + var price_buf: [24]u8 = undefined; + const price_str = std.fmt.bufPrint(&price_buf, "{f}", .{Money.from(a.current_price)}) catch "$?"; // Date + ST/LT: show for single-lot, blank for multi-lot var pos_date_buf: [10]u8 = undefined; @@ -1977,29 +1977,24 @@ pub fn drawContent(state: *State, app: *App, arena: std.mem.Allocator, buf: []va else a.weight; - // Pad each dynamic cell to this frame's column - // widths (weight + date stay fixed-spec). Symbol - // is left-justified; the numeric cells right. - var sym_buf: [64]u8 = undefined; - const sym_cell = blk: { - const sym_src = std.fmt.bufPrint(&sym_buf, "{s}", .{a.display_symbol}) catch a.display_symbol; - break :blk fmt.padRightToCols(&sym_buf, sym_src, cw.symbol_w); - }; var shr_raw_buf: [48]u8 = undefined; const shr_raw = std.fmt.bufPrint(&shr_raw_buf, "{d:.1}", .{display_shares}) catch "?"; - var shr_buf: [64]u8 = undefined; - const shr_cell = fmt.padLeftToCols(&shr_buf, shr_raw, cw.shares_w); - var cost_pad: [64]u8 = undefined; - const cost_cell = fmt.padLeftToCols(&cost_pad, cost_str, cw.price_w); - var prc_pad: [64]u8 = undefined; - const prc_cell = fmt.padLeftToCols(&prc_pad, price_str, cw.price_w); - var mv_pad: [64]u8 = undefined; - const mv_cell = fmt.padLeftToCols(&mv_pad, mv_str, cw.value_w); - var gl_pad: [64]u8 = undefined; - const gl_cell = fmt.padLeftToCols(&gl_pad, pnl_str, cw.gainloss_w); + + // Pad each dynamic cell to this frame's column + // widths (weight + date stay fixed-spec). Named in + // column order so this arm and the lot / watchlist + // arms can't disagree about which value goes where. + const cells = try views.padRowCells(arena, cw, .{ + .symbol = a.display_symbol, + .shares = shr_raw, + .cost = cost_str, + .price = price_str, + .value = mv_str, + .gainloss = pnl_str, + }); const text = try std.fmt.allocPrint(arena, "{s}{s}{s} {s} {s} {s} {s} {s} " ++ pl.weight_num ++ " " ++ pl.date_str ++ " {s}", .{ - arrow, star, sym_cell, shr_cell, cost_cell, prc_cell, mv_cell, gl_cell, display_weight * 100.0, date_col, acct_col, + arrow, star, cells.symbol, cells.shares, cells.cost, cells.price, cells.value, cells.gainloss, display_weight * 100.0, date_col, acct_col, }); // base: neutral text for main cols, green/red only for gain/loss col @@ -2024,8 +2019,8 @@ pub fn drawContent(state: *State, app: *App, arena: std.mem.Allocator, buf: []va // Compute lot gain/loss and market value if we have a price var lot_gl_str: []const u8 = ""; - var lot_mv_str: []const u8 = ""; - var lot_eff_price_str: []const u8 = ""; + var lot_value_str: []const u8 = ""; + var lot_price_str: []const u8 = ""; var lot_positive = true; if (app.portfolio.summary) |s| { // The LOT's effective price, not its position's - @@ -2040,41 +2035,38 @@ pub fn drawContent(state: *State, app: *App, arena: std.mem.Allocator, buf: []va if (gl >= 0) @as([]const u8, "+") else @as([]const u8, "-"), Money.from(if (gl >= 0) gl else -gl), }); - lot_mv_str = try std.fmt.allocPrint(arena, "{f}", .{Money.from(lot.effectiveShares() * use_price)}); + lot_value_str = try std.fmt.allocPrint(arena, "{f}", .{Money.from(lot.effectiveShares() * use_price)}); + // Price column: normally blank, since the position + // row above already shows it. A ratio'd lot prices + // off its own institutional NAV, so show that. if (views.hasOwnPrice(lot)) { - lot_eff_price_str = try std.fmt.allocPrint(arena, "{f}", .{Money.from(use_price)}); + lot_price_str = try std.fmt.allocPrint(arena, "{f}", .{Money.from(use_price)}); } } - var price_str2: [24]u8 = undefined; - const lot_price_str = std.fmt.bufPrint(&price_str2, "{f}", .{Money.from(lot.effectiveOpenPrice())}) catch "$?"; + // Avg Cost column: this lot's own per-share cost basis. + var lot_cost_buf: [24]u8 = undefined; + const lot_cost_str = std.fmt.bufPrint(&lot_cost_buf, "{f}", .{Money.from(lot.effectiveOpenPrice())}) catch "$?"; const status_str: []const u8 = if (lot.isOpen(app.today)) "open" else "closed"; const indicator = fmt.capitalGainsIndicator(app.today, lot.open_date); const lot_date_col = try std.fmt.allocPrint(arena, "{s} {s}", .{ date_str, indicator }); const acct_col: []const u8 = lot.account orelse ""; - var lot_sym_buf: [64]u8 = undefined; - const lot_sym_cell = blk: { - const sym_src = std.fmt.bufPrint(&lot_sym_buf, "{s}", .{status_str}) catch status_str; - break :blk fmt.padRightToCols(&lot_sym_buf, sym_src, cw.symbol_w); - }; - var lot_shr_raw: [48]u8 = undefined; - const lot_shr_raw_s = std.fmt.bufPrint(&lot_shr_raw, "{d:.1}", .{lot.effectiveShares()}) catch "?"; - var lot_shr_buf: [64]u8 = undefined; - const lot_shr_cell = fmt.padLeftToCols(&lot_shr_buf, lot_shr_raw_s, cw.shares_w); - var lot_cost_pad: [64]u8 = undefined; - const lot_cost_cell = fmt.padLeftToCols(&lot_cost_pad, lot_price_str, cw.price_w); - var lot_prc_pad: [64]u8 = undefined; - // Blank unless the lot is ratio'd, in which case it - // prices off its own institutional NAV rather than - // the base-ticker price on the position row above. - const lot_prc_cell = fmt.padLeftToCols(&lot_prc_pad, lot_eff_price_str, cw.price_w); - var lot_mv_pad: [64]u8 = undefined; - const lot_mv_cell = fmt.padLeftToCols(&lot_mv_pad, lot_mv_str, cw.value_w); - var lot_gl_pad: [64]u8 = undefined; - const lot_gl_cell = fmt.padLeftToCols(&lot_gl_pad, lot_gl_str, cw.gainloss_w); + var lot_shr_buf: [48]u8 = undefined; + const lot_shr_str = std.fmt.bufPrint(&lot_shr_buf, "{d:.1}", .{lot.effectiveShares()}) catch "?"; + + const cells = try views.padRowCells(arena, cw, .{ + // The Symbol column carries open/closed status on a + // lot row rather than a symbol. + .symbol = status_str, + .shares = lot_shr_str, + .cost = lot_cost_str, + .price = lot_price_str, + .value = lot_value_str, + .gainloss = lot_gl_str, + }); const text = try std.fmt.allocPrint(arena, " {s} {s} {s} {s} {s} {s} " ++ pl.weight_str ++ " " ++ pl.date_str ++ " {s}", .{ - lot_sym_cell, lot_shr_cell, lot_cost_cell, lot_prc_cell, lot_mv_cell, lot_gl_cell, "", lot_date_col, acct_col, + cells.symbol, cells.shares, cells.cost, cells.price, cells.value, cells.gainloss, "", lot_date_col, acct_col, }); const base_style = if (is_cursor) th.selectStyle() else th.mutedStyle(); const gl_col_style = if (is_cursor) th.selectStyle() else if (lot_positive) th.positiveStyle() else th.negativeStyle(); @@ -2088,29 +2080,24 @@ pub fn drawContent(state: *State, app: *App, arena: std.mem.Allocator, buf: []va } }, .watchlist => { - var price_str3: [16]u8 = undefined; + var watch_price_buf: [16]u8 = undefined; const ps: []const u8 = if (app.portfolio.watchlist_prices) |wp| - (if (wp.get(row.symbol)) |p| (std.fmt.bufPrint(&price_str3, "{f}", .{Money.from(p)}) catch "$?") else "--") + (if (wp.get(row.symbol)) |p| (std.fmt.bufPrint(&watch_price_buf, "{f}", .{Money.from(p)}) catch "$?") else "--") else "--"; const star2: []const u8 = if (is_active_sym) "* " else " "; - var w_sym_buf: [64]u8 = undefined; - const w_sym_cell = blk: { - const sym_src = std.fmt.bufPrint(&w_sym_buf, "{s}", .{row.symbol}) catch row.symbol; - break :blk fmt.padRightToCols(&w_sym_buf, sym_src, cw.symbol_w); - }; - var w_shr_buf: [64]u8 = undefined; - const w_shr_cell = fmt.padLeftToCols(&w_shr_buf, "--", cw.shares_w); - var w_avg_buf: [64]u8 = undefined; - const w_avg_cell = fmt.padLeftToCols(&w_avg_buf, "--", cw.price_w); - var w_prc_buf: [64]u8 = undefined; - const w_prc_cell = fmt.padLeftToCols(&w_prc_buf, ps, cw.price_w); - var w_mv_buf: [64]u8 = undefined; - const w_mv_cell = fmt.padLeftToCols(&w_mv_buf, "--", cw.value_w); - var w_gl_buf: [64]u8 = undefined; - const w_gl_cell = fmt.padLeftToCols(&w_gl_buf, "--", cw.gainloss_w); + // A watchlist row has no position, so only Symbol and Price + // carry data; the rest are placeholders. + const cells = try views.padRowCells(arena, cw, .{ + .symbol = row.symbol, + .shares = "--", + .cost = "--", + .price = ps, + .value = "--", + .gainloss = "--", + }); const text = try std.fmt.allocPrint(arena, " {s}{s} {s} {s} {s} {s} {s} " ++ pl.weight_str ++ " " ++ pl.date_str, .{ - star2, w_sym_cell, w_shr_cell, w_avg_cell, w_prc_cell, w_mv_cell, w_gl_cell, "watch", "", + star2, cells.symbol, cells.shares, cells.cost, cells.price, cells.value, cells.gainloss, "watch", "", }); const row_style = if (is_cursor) th.selectStyle() else th.contentStyle(); try lines.append(arena, .{ .text = text, .style = row_style }); diff --git a/src/views/portfolio_sections.zig b/src/views/portfolio_sections.zig index 3306358..92aa8d0 100644 --- a/src/views/portfolio_sections.zig +++ b/src/views/portfolio_sections.zig @@ -167,6 +167,86 @@ fn gainLossCols(amount: f64) usize { return 1 + moneyCols(if (amount < 0) -amount else amount); } +// ── Holdings-row cells ──────────────────────────────────────── + +/// The six variable-width cells of one holdings-table row, in COLUMN +/// ORDER - matching `PositionsLayout.header_labels`: +/// +/// Symbol | Shares | Avg Cost | Price | Market Value | Gain/Loss +/// +/// Used both for the raw (unpadded) values a renderer computes and for the +/// padded result of `padRowCells`. +/// +/// The point of naming these is that the TUI used to hand-roll the same six +/// cells three times - once each for position, lot and watchlist rows - with +/// per-arm local names that drifted apart. The lot arm ended up calling its +/// Avg Cost value `lot_price_str` and its Price value `lot_eff_price_str`, +/// so "price" named a cost and the two arms disagreed about the same two +/// columns. Nothing caught it, because a TUI row needs a live `App` to +/// render and there is no harness for that. A struct with named fields +/// can't be transposed silently the way two similar locals can. +pub const RowCells = struct { + symbol: []const u8, + shares: []const u8, + /// Avg Cost column: per-share cost basis. + cost: []const u8, + /// Price column: current per-share price. + price: []const u8, + value: []const u8, + gainloss: []const u8, +}; + +/// Pad each cell of `raw` to this frame's column widths. +/// +/// Symbol is left-justified; every numeric cell is right-justified, which +/// is the one place the columns differ in treatment. Padding is +/// DISPLAY-COLUMN aware (`padRightToCols` / `padLeftToCols`), so a +/// multibyte cell - the `—` no-data sentinel, or any glyph a caller +/// substitutes - occupies its true width instead of being under-padded by +/// two columns the way a byte-counting `{s:>N}` would. +/// +/// Cells are allocated in `arena` rather than caller stack buffers: twelve +/// scratch buffers in one function was what forced the `_buf2` / `_str3` +/// name suffixes, and the arena is already per-frame. +/// +/// A cell wider than its column is returned unchanged - over-wide is the +/// safe direction, since `computeWidths` sizes columns from the same data +/// and the row renderer truncates at terminal width anyway. +pub fn padRowCells( + arena: std.mem.Allocator, + w: PositionsWidths, + raw: RowCells, +) !RowCells { + return .{ + .symbol = try padCell(arena, raw.symbol, w.symbol_w, .left), + .shares = try padCell(arena, raw.shares, w.shares_w, .right), + .cost = try padCell(arena, raw.cost, w.price_w, .right), + .price = try padCell(arena, raw.price, w.price_w, .right), + .value = try padCell(arena, raw.value, w.value_w, .right), + .gainloss = try padCell(arena, raw.gainloss, w.gainloss_w, .right), + }; +} + +const Justify = enum { left, right }; + +/// Pad one cell to `cols` display columns, allocating in `arena`. +fn padCell(arena: std.mem.Allocator, content: []const u8, cols: usize, justify: Justify) ![]const u8 { + const have = fmt.displayCols(content); + if (have >= cols) return content; + // Worst case is all-single-column content, so `cols - have` bytes of + // padding is always enough; multibyte content needs less. + const buf = try arena.alloc(u8, content.len + (cols - have)); + switch (justify) { + // `padRightToCols` appends in place and requires its content to + // already sit at the start of the buffer; `padLeftToCols` copies. + .left => { + @memcpy(buf[0..content.len], content); + return fmt.padRightToCols(buf, buf[0..content.len], cols); + }, + .right => return fmt.padLeftToCols(buf, content, cols), + } +} + /// The effective price of a single LOT: the base-ticker price from /// `allocations` with this lot's `price_ratio` applied. The free-function /// counterpart to `Lot.effectivePrice`, which takes the raw price as an @@ -681,6 +761,173 @@ test "CDs.init: all matured yields empty active slice" { // ── effectivePriceFor / hasOwnPrice ───────────────────────────── +// ── padRowCells ─────────────────────────────────────────────── + +/// Widths with every column distinct, so a test can tell which column a +/// cell landed in purely from how wide it came back. +fn rcWidths() PositionsWidths { + return .{ + .symbol_w = 10, + .shares_w = 9, + .price_w = 8, + .value_w = 14, + .gainloss_w = 12, + }; +} + +test "padRowCells: every cell lands in its own column at its own width" { + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + const a = arena_state.allocator(); + const w = rcWidths(); + + const c = try padRowCells(a, w, .{ + .symbol = "AAPL", + .shares = "10.0", + .cost = "$150.00", + .price = "$175.00", + .value = "$1,750.00", + .gainloss = "+$250.00", + }); + + try testing.expectEqual(@as(usize, w.symbol_w), fmt.displayCols(c.symbol)); + try testing.expectEqual(@as(usize, w.shares_w), fmt.displayCols(c.shares)); + try testing.expectEqual(@as(usize, w.price_w), fmt.displayCols(c.cost)); + try testing.expectEqual(@as(usize, w.price_w), fmt.displayCols(c.price)); + try testing.expectEqual(@as(usize, w.value_w), fmt.displayCols(c.value)); + try testing.expectEqual(@as(usize, w.gainloss_w), fmt.displayCols(c.gainloss)); + + // Symbol is the one left-justified column; the numerics are right. + try testing.expect(std.mem.startsWith(u8, c.symbol, "AAPL")); + try testing.expect(std.mem.endsWith(u8, c.shares, "10.0")); + try testing.expect(std.mem.endsWith(u8, c.cost, "$150.00")); + try testing.expect(std.mem.endsWith(u8, c.price, "$175.00")); + try testing.expect(std.mem.endsWith(u8, c.value, "$1,750.00")); + try testing.expect(std.mem.endsWith(u8, c.gainloss, "+$250.00")); +} + +test "padRowCells: cost and price do not clobber each other" { + // These two share `price_w`. Three hand-rolled copies of this layout + // used to pad them into separate stack buffers; a shared or reused + // buffer would silently make the two columns equal, and no TUI test + // exists to notice. Pin it. + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + const a = arena_state.allocator(); + + const c = try padRowCells(a, rcWidths(), .{ + .symbol = "X", + .shares = "1", + .cost = "$1.00", + .price = "$9.00", + .value = "$9.00", + .gainloss = "+$8.00", + }); + try testing.expect(std.mem.endsWith(u8, c.cost, "$1.00")); + try testing.expect(std.mem.endsWith(u8, c.price, "$9.00")); + try testing.expect(!std.mem.eql(u8, c.cost, c.price)); + // Distinct backing memory, not two views of one buffer. + try testing.expect(c.cost.ptr != c.price.ptr); +} + +test "padRowCells: the COST value stays in the cost column" { + // The regression this whole type exists for. The TUI lot row named its + // Avg Cost value `lot_price_str` and its Price value + // `lot_eff_price_str`, so "price" named a cost and the position and lot + // arms disagreed about the same two columns. Renaming toward consistency + // risked transposing them, and a TUI row needs a live `App` to render, + // so nothing would have caught it. + // + // Named fields make the mapping assertable: feed unmistakable values and + // check each comes back from the field it was handed to. + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + const a = arena_state.allocator(); + + const c = try padRowCells(a, rcWidths(), .{ + .symbol = "SYM", + .shares = "SHR", + .cost = "COST", + .price = "PRICE", + .value = "VALUE", + .gainloss = "GL", + }); + try testing.expect(std.mem.indexOf(u8, c.cost, "COST") != null); + try testing.expect(std.mem.indexOf(u8, c.price, "PRICE") != null); + // ...and no leakage in either direction. + try testing.expect(std.mem.indexOf(u8, c.cost, "PRICE") == null); + try testing.expect(std.mem.indexOf(u8, c.price, "COST") == null); + try testing.expect(std.mem.indexOf(u8, c.value, "VALUE") != null); + try testing.expect(std.mem.indexOf(u8, c.gainloss, "GL") != null); + try testing.expect(std.mem.indexOf(u8, c.symbol, "SYM") != null); + try testing.expect(std.mem.indexOf(u8, c.shares, "SHR") != null); +} + +test "padRowCells: a multibyte cell is padded by display columns" { + // The no-data sentinel is one display column in three bytes. Padding it + // by BYTES under-fills the cell by two columns and skews every column to + // its right - the exact defect that hit the compare table's sentinel + // rows. `padRowCells` must not reintroduce it. + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + const a = arena_state.allocator(); + const w = rcWidths(); + + const c = try padRowCells(a, w, .{ + .symbol = "SYM", + .shares = "1", + .cost = fmt.no_data_sentinel, + .price = fmt.no_data_sentinel, + .value = "$1.00", + .gainloss = "+$0.00", + }); + try testing.expectEqual(@as(usize, w.price_w), fmt.displayCols(c.cost)); + try testing.expectEqual(@as(usize, w.price_w), fmt.displayCols(c.price)); + // Byte length exceeds the column width precisely because the glyph is + // multibyte - proof the padding counted columns, not bytes. + try testing.expect(c.cost.len > w.price_w); +} + +test "padRowCells: an empty cell fills its column" { + // A lot row leaves the Price cell blank unless the lot is ratio'd. It + // still has to occupy the column or the row shears. + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + const a = arena_state.allocator(); + const w = rcWidths(); + + const c = try padRowCells(a, w, .{ + .symbol = "open", + .shares = "100.0", + .cost = "$97.50", + .price = "", + .value = "$9,750.00", + .gainloss = "+$0.00", + }); + try testing.expectEqual(@as(usize, w.price_w), fmt.displayCols(c.price)); + try testing.expectEqualStrings(" ", c.price); +} + +test "padRowCells: an over-wide cell is returned unchanged" { + // Over-wide is the safe direction: `computeWidths` sizes columns from + // the same data, and the renderer truncates at terminal width. Silently + // clipping here would corrupt a figure instead of just crowding it. + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + const a = arena_state.allocator(); + + const huge = "$123,456,789.00"; + const c = try padRowCells(a, rcWidths(), .{ + .symbol = "SYM", + .shares = "1", + .cost = huge, + .price = "$1.00", + .value = "$1.00", + .gainloss = "+$0.00", + }); + try testing.expectEqualStrings(huge, c.cost); +} + test "effectivePriceFor: live price gets the lot's ratio applied" { // The bug this guards: `Allocation.current_price` is the RAW // base-ticker price. A proxied sleeve quoted off a $90.15 base at an