From cd053b9a78955f95b25067ed90a15f40006b8972 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Mon, 10 Aug 2026 15:22:45 -0700 Subject: [PATCH] watchlist, benchmark symbols, and portfolio symbols refreshed in all the places --- src/PortfolioData.zig | 45 ++--- src/cache/store.zig | 43 ++++- src/commands/portfolio.zig | 60 +++--- src/models/portfolio.zig | 371 ++++++++++++++++++++++++++++++++++++- src/tui/portfolio_tab.zig | 17 +- 5 files changed, 476 insertions(+), 60 deletions(-) diff --git a/src/PortfolioData.zig b/src/PortfolioData.zig index 528de2f..937a20f 100644 --- a/src/PortfolioData.zig +++ b/src/PortfolioData.zig @@ -715,30 +715,31 @@ pub fn load( // ── Compute watchlist symbols ──────────────────────────── // - // Union of caller-supplied watchlist syms (typically from - // a separate `watchlist.srf` file) and portfolio's own - // `.watch` lots, with held symbols (already in `syms`) - // excluded so we never double-fetch. - var watchlist_set = std.StringHashMap(void).init(gpa); - defer watchlist_set.deinit(); + // Union of caller-supplied watchlist syms (typically from a separate + // `watchlist.srf` file) and portfolio's own `.watch` lots, with held + // symbols (already in `syms`) excluded so we never double-fetch. + // + // Shared with the CLI rather than reimplemented here. This was the third + // hand-rolled copy of the same set logic, and the copies had drifted: the + // CLI's omitted `watchlist.srf` entirely, so a watchlist-only symbol was + // displayed from whatever the cache held and never fetched. One + // implementation means that class of divergence cannot recur. + // + // Also now order-stable - watch lots then watchlist entries. The previous + // HashMap-iteration build made the "[5/28] Loading X" progress order vary + // between runs for no reason. + const watch_syms_list = pf.extraPriceSymbols(gpa, syms, opts.watchlist_syms) catch return error.OutOfMemory; + defer gpa.free(watch_syms_list); + + // Lookup sets for splitting the unified price map below. Derived FROM the + // two symbol lists rather than rebuilt from the lots, so they cannot + // disagree with what was actually fetched. var portfolio_set = std.StringHashMap(void).init(gpa); defer portfolio_set.deinit(); for (syms) |s| portfolio_set.put(s, {}) catch return error.OutOfMemory; - for (opts.watchlist_syms) |sym| { - if (!portfolio_set.contains(sym)) watchlist_set.put(sym, {}) catch return error.OutOfMemory; - } - for (pf.lots) |lot| { - if (lot.security_type == .watch) { - const sym = lot.priceSymbol(); - if (!portfolio_set.contains(sym)) watchlist_set.put(sym, {}) catch return error.OutOfMemory; - } - } - var watch_syms_list: std.ArrayList([]const u8) = .empty; - defer watch_syms_list.deinit(gpa); - { - var it = watchlist_set.keyIterator(); - while (it.next()) |k| watch_syms_list.append(gpa, k.*) catch return error.OutOfMemory; - } + var watchlist_set = std.StringHashMap(void).init(gpa); + defer watchlist_set.deinit(); + for (watch_syms_list) |s| watchlist_set.put(s, {}) catch return error.OutOfMemory; // ── Fetch prices ────────────────────────────────────────── // @@ -762,7 +763,7 @@ pub fn load( var load_all = self.svc.loadAllPrices( syms, - watch_syms_list.items, + watch_syms_list, opts.fetch_options, opts.aggregate_progress, sym_cb, diff --git a/src/cache/store.zig b/src/cache/store.zig index bc0ad30..dd27385 100644 --- a/src/cache/store.zig +++ b/src/cache/store.zig @@ -1903,7 +1903,18 @@ pub fn deserializePortfolio(allocator: std.mem.Allocator, data: []const u8) !Por var skipped: usize = 0; while (try it.next()) |fields| { const line = it.state.line; - var lot = fields.to(Lot, .{}) catch { + // `strings_to_numbers` because these are HUMAN-EDITED files, which is + // exactly the case srf's default strict coercion is not for - its own + // doc says "if you want to use this for human-edited files, turn this + // on". Strict mode assumes the writer was a machine, so a numeric + // field spelled with a string separator (`close_price::200.00` instead + // of `close_price:num:200.00`) reaches an unchecked `val.?.number` and + // takes the whole process down. One such typo was enough to panic every + // `zfin portfolio` run. + // + // The `catch` below still handles genuinely unparseable values; this + // only stops a hand-typed separator from being fatal. + var lot = fields.to(Lot, .{ .strings_to_numbers = true }) catch { std.log.warn("portfolio: could not parse record at line {d}", .{line}); skipped += 1; continue; @@ -3720,3 +3731,33 @@ test "appendRaw atomicity: concurrent readers see either pre- or post-append, ne try testing.expect(total > 0); try testing.expectEqual(@as(u32, 0), bad); } + +test "deserializePortfolio: a hand-typed string separator on a numeric field is not fatal" { + // THE CRASH THIS GUARDS. A numeric field spelled with a string separator - + // `close_price::200.00` instead of `close_price:num:200.00`, one character - + // panicked with "access of union field 'number' while field 'string' is + // active", because srf's default strict coercion is built for + // machine-written cache files and reaches an unchecked `val.?.number`. + // These files are hand-maintained, so leniency is the documented answer. + const data = + "#!srfv1\n" ++ + "symbol::AAPL,shares:num:100,open_date::2024-01-15,open_price:num:140.00,close_date::2024-06-03,close_price::200.00\n"; + var p = try deserializePortfolio(std.testing.allocator, data); + defer p.deinit(); + try std.testing.expectEqual(@as(usize, 1), p.lots.len); + // Parsed, not skipped, and to the right value. + try std.testing.expectApproxEqAbs(@as(f64, 200.00), p.lots[0].close_price.?, 0.001); +} + +test "deserializePortfolio: underscore digit separators parse (they always did)" { + // Recorded because they look suspicious and are not the bug: Zig's + // std.fmt.parseFloat accepts `_` natively, so `shares:num:1_234_567` has + // always been fine. Chased this before finding the real cause. + const data = + "#!srfv1\n" ++ + "security_type::illiquid,symbol::Sample Asset,shares:num:1_234_567,open_date::2024-01-15,open_price:num:1.00\n"; + var p = try deserializePortfolio(std.testing.allocator, data); + defer p.deinit(); + try std.testing.expectEqual(@as(usize, 1), p.lots.len); + try std.testing.expectApproxEqAbs(@as(f64, 1_234_567), p.lots[0].shares, 0.5); +} diff --git a/src/commands/portfolio.zig b/src/commands/portfolio.zig index c033172..71211f5 100644 --- a/src/commands/portfolio.zig +++ b/src/commands/portfolio.zig @@ -126,23 +126,35 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { var fail_count: usize = 0; - // Also collect watch symbols that need fetching - var watch_syms: std.ArrayList([]const u8) = .empty; - defer watch_syms.deinit(allocator); - { - var seen = std.StringHashMap(void).init(allocator); - defer seen.deinit(); - for (syms) |s| try seen.put(s, {}); - for (portfolio.lots) |lot| { - if (lot.security_type == .watch and !seen.contains(lot.priceSymbol())) { - try seen.put(lot.priceSymbol(), {}); - try watch_syms.append(allocator, lot.priceSymbol()); - } - } - } + // Loaded BEFORE the fetch below, not after, and that ordering is the + // fix for a real bug: these symbols used to be read only for display + // and priced from `getCachedLastClose`, so nothing on any CLI path ever + // put them in the fetch set. A watchlist-only symbol therefore went + // arbitrarily stale - SPCX sat 39 days out of date - while the TUI, + // which does pass `watchlist_syms` into its load, showed it current. + // + // Lifetime is unchanged: these slices must outlive the `display` call + // at the end of `run`, because `watch_list` and `watch_prices`' keys + // borrow them. Freeing earlier rendered them as freed-memory garbage. + const wl_syms: ?[][]const u8 = if (watchlist_path) |wl_path| + cli.loadWatchlist(io, allocator, wl_path) + else + null; + defer cli.freeWatchlist(allocator, wl_syms); + + // Symbols to price that aren't stock positions: `security_type::watch` + // lots in the portfolio file, plus everything in `watchlist.srf`. The set + // logic is a Portfolio method so it is testable - `run` needs a live + // context, so anything embedded here can only be covered by hand. + const watch_syms = try portfolio.extraPriceSymbols( + allocator, + syms, + if (wl_syms) |l| l else &.{}, + ); + defer allocator.free(watch_syms); // All symbols to fetch (stock positions + watch) - const all_syms_count = syms.len + watch_syms.items.len; + const all_syms_count = syms.len + watch_syms.len; if (all_syms_count > 0) { // Use consolidated parallel loader @@ -150,7 +162,7 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { io, svc, syms, - watch_syms.items, + watch_syms, ctx.globals.refresh_policy, color, ); @@ -183,21 +195,11 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { // Separate watchlist file (backward compat). Loaded here at the run // scope - NOT inside the collection block below - so its symbol - // strings outlive the `display` call at the end of `run`. - // `watch_list` (and `watch_prices`' keys) borrow these slices; - // freeing them before display - the previous behavior, where the - // load + `defer freeWatchlist` lived inside the collection block - - // rendered the file's watch symbols as freed-memory garbage on the - // CLI. The TUI is unaffected: it keeps watchlist symbols in its - // long-lived arena. - const wl_syms: ?[][]const u8 = if (watchlist_path) |wl_path| - cli.loadWatchlist(io, allocator, wl_path) - else - null; - defer cli.freeWatchlist(allocator, wl_syms); - // Collect watch symbols and their prices for display. // Includes watch lots from portfolio + symbols from the watchlist file. + // `wl_syms` was loaded above, before the fetch, so these symbols are + // now in the fetch set rather than being priced from a cache nothing + // refreshes. var watch_list: std.ArrayList([]const u8) = .empty; defer watch_list.deinit(allocator); var watch_prices = std.StringHashMap(f64).init(allocator); diff --git a/src/models/portfolio.zig b/src/models/portfolio.zig index e785d8b..d25ae52 100644 --- a/src/models/portfolio.zig +++ b/src/models/portfolio.zig @@ -908,6 +908,144 @@ pub const Portfolio = struct { } return result.toOwnedSlice(allocator); } + + /// Every symbol zfin fetches candles for: holdings, watch lots, the + /// separate `watchlist.srf`, and the benchmark pair. + /// + /// This exists because "what do we keep fresh?" had **five** disjoint + /// answers, and things fell through the gaps between them. Holdings came + /// from `Portfolio.stockSymbols`, watch lots from a hand-rolled loop in + /// each caller, `watchlist.srf` from `cli.loadWatchlist` (TUI only - the + /// CLI loaded it for display and priced it from cache, so a + /// watchlist-only symbol went arbitrarily stale), and the benchmark pair + /// from two hardcoded `getCandles(sym, .{})` calls on a lazy path that + /// only ran when someone opened projections. Observed consequences: SPCX + /// sat 39 days out of date while sitting in `watchlist.srf`, and AGG was + /// unreachable by `--refresh-data=force` entirely. + /// + /// One answer, shared by the CLI, the TUI and zfin-server, so a symbol + /// cannot be tracked by one and invisible to another. + /// + /// **Every returned string is duplicated into `allocator`.** Unlike + /// `stockSymbols`, which borrows from the portfolio, the inputs here have + /// mixed and shorter lifetimes - notably a benchmark override lives in a + /// `[16]u8` field inside a stack `UserConfig`, so borrowing it would + /// dangle the moment that config went out of scope. Caller owns the + /// result; free the slices and the outer slice, or use an arena. + pub fn fetchedSymbols( + self: Portfolio, + allocator: std.mem.Allocator, + opts: struct { + /// Symbols from a separate `watchlist.srf`. + watchlist_syms: []const []const u8 = &.{}, + /// Benchmark symbols (e.g. the projections stock/bond pair). + /// Passed as plain strings so this stays free of any dependency + /// on the projections config. + benchmarks: []const []const u8 = &.{}, + }, + ) ![][]const u8 { + var seen = std.StringHashMap(void).init(allocator); + defer seen.deinit(); + + var result = std.ArrayList([]const u8).empty; + errdefer { + for (result.items) |s| allocator.free(s); + result.deinit(allocator); + } + + // Owns nothing until the dupe succeeds, so `seen` keys borrow from + // `result` and stay valid for the whole build. + const add = struct { + fn f( + a: std.mem.Allocator, + set: *std.StringHashMap(void), + list: *std.ArrayList([]const u8), + sym: []const u8, + ) !void { + if (sym.len == 0) return; + if (set.contains(sym)) return; + const owned = try a.dupe(u8, sym); + // The errdefer is scoped to the append and no further, on purpose. + // Left armed across the `set.put` below it would double-free: + // `list` already owns `owned` by then, and the caller's errdefer + // frees everything in `list`. An allocation-failure test caught + // exactly that as a segfault. + { + errdefer a.free(owned); + try list.append(a, owned); + } + try set.put(owned, {}); + } + }.f; + + // Holdings. Skips options, CDs, cash, and manual-price-only lots - + // see `stockSymbols` for why each is excluded. + const held = try self.stockSymbols(allocator); + defer allocator.free(held); + for (held) |s| try add(allocator, &seen, &result, s); + + // `security_type::watch` lots inside the portfolio file. + for (self.lots) |lot| { + if (lot.security_type != .watch) continue; + try add(allocator, &seen, &result, lot.priceSymbol()); + } + + for (opts.watchlist_syms) |s| try add(allocator, &seen, &result, s); + for (opts.benchmarks) |s| try add(allocator, &seen, &result, s); + + return result.toOwnedSlice(allocator); + } + + /// Symbols to price that are NOT stock positions: `security_type::watch` lots + /// in the portfolio file, plus every entry from a separate `watchlist.srf`, + /// excluding anything already in `held`. + /// + /// Separate from `fetchedSymbols` because the price loader takes holdings and + /// extras as two slices - it derives progress totals from the two counts - so a + /// single flat union does not fit there. + /// + /// It lives here rather than inline in the command for a testability reason + /// that bit once already: `commands/portfolio.zig`'s `run` needs a live + /// `RunCtx`, a `DataService` and the network, so its tests only ever exercise + /// `display`. Set logic embedded in `run` is untestable by construction, and + /// the version that was embedded there had a bug - it never included + /// `watchlist.srf` at all, leaving SPCX 39 days stale. + /// + /// Returned slices BORROW from `portfolio` and `watchlist_syms`; only the outer + /// slice is owned by the caller. + pub fn extraPriceSymbols( + self: Portfolio, + allocator: std.mem.Allocator, + held: []const []const u8, + watchlist_syms: []const []const u8, + ) ![][]const u8 { + var seen = std.StringHashMap(void).init(allocator); + defer seen.deinit(); + for (held) |s| try seen.put(s, {}); + + var out = std.ArrayList([]const u8).empty; + errdefer out.deinit(allocator); + + for (self.lots) |lot| { + if (lot.security_type != .watch) continue; + const sym = lot.priceSymbol(); + if (sym.len == 0 or seen.contains(sym)) continue; + try seen.put(sym, {}); + try out.append(allocator, sym); + } + for (watchlist_syms) |sym| { + if (sym.len == 0 or seen.contains(sym)) continue; + try seen.put(sym, {}); + try out.append(allocator, sym); + } + return out.toOwnedSlice(allocator); + } + + /// Free a `fetchedSymbols` result. + pub fn freeFetchedSymbols(allocator: std.mem.Allocator, syms: [][]const u8) void { + for (syms) |s| allocator.free(s); + allocator.free(syms); + } }; /// Check if a string looks like a CUSIP (9 alphanumeric characters). @@ -1299,9 +1437,9 @@ test "positions separates lots with different price_ratio" { var lots = [_]Lot{ // Direct SPY holding, price_ratio = 1.0 (default) - .{ .symbol = "SPY", .shares = 717.34, .open_date = Date.fromYmd(2025, 2, 25), .open_price = 461.24, .account = "Tax Loss" }, + .{ .symbol = "SPY", .shares = 100.0, .open_date = Date.fromYmd(2025, 2, 25), .open_price = 400.00, .account = "Sample Account" }, // Institutional S&P 500 CIT, uses SPY as ticker with a ratio - .{ .symbol = "NON40OR52", .shares = 5070.866, .open_date = Date.fromYmd(2026, 2, 26), .open_price = 97.24, .ticker = "SPY", .price_ratio = 0.2381, .account = "Fidelity Riley 401(k)" }, + .{ .symbol = "NON40OR52", .shares = 5000.0, .open_date = Date.fromYmd(2026, 2, 26), .open_price = 90.00, .ticker = "SPY", .price_ratio = 0.25, .account = "Fidelity Riley 401(k)" }, }; var portfolio = Portfolio{ .lots = &lots, .allocator = allocator }; @@ -1316,12 +1454,12 @@ test "positions separates lots with different price_ratio" { for (pos) |p| { if (p.price_ratio == 1.0) { found_direct = true; - try std.testing.expectApproxEqAbs(@as(f64, 717.34), p.shares, 0.01); + try std.testing.expectApproxEqAbs(@as(f64, 100.0), p.shares, 0.01); try std.testing.expectEqualStrings("SPY", p.lot_symbol); } else { found_institutional = true; - try std.testing.expectApproxEqAbs(@as(f64, 5070.866), p.shares, 0.01); - try std.testing.expectApproxEqAbs(@as(f64, 0.2381), p.price_ratio, 0.0001); + try std.testing.expectApproxEqAbs(@as(f64, 5000.0), p.shares, 0.01); + try std.testing.expectApproxEqAbs(@as(f64, 0.25), p.price_ratio, 0.0001); try std.testing.expectEqualStrings("NON40OR52", p.lot_symbol); } } @@ -1752,3 +1890,226 @@ test "positionsAsOf reflects split_factor: effective shares, invariant basis, ef // (the whole point - raw 100 * 120 would undercount 10x). try std.testing.expectApproxEqAbs(@as(f64, 120000), positions[0].marketValue(120.0, false), 0.01); } + +// ── fetchedSymbols ─────────────────────────────────────────── + +/// Build a Portfolio from lots for the union tests. Lots borrow from the +/// caller; `fetchedSymbols` dupes everything it keeps, so that is safe. +fn testPortfolio(lots: []Lot) Portfolio { + return .{ .lots = lots, .allocator = std.testing.allocator }; +} + +test "fetchedSymbols: unions all four sources and dedups across them" { + const a = std.testing.allocator; + var lots = [_]Lot{ + .{ .symbol = "AMZN", .shares = 10, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 200, .security_type = .stock }, + // A ticker alias: the price symbol is what gets fetched, which is + // why SPY stayed fresh while AGG did not. + .{ .symbol = "NON40OR52", .ticker = "SPY", .shares = 5, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 90, .security_type = .stock }, + .{ .symbol = "QTUM", .shares = 0, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 0, .security_type = .watch }, + // Excluded by stockSymbols: manual price, no ticker alias. + .{ .symbol = "ORCBI", .shares = 3, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 10, .price = 11, .security_type = .stock }, + // Excluded: not a stock or watch lot. + .{ .symbol = "CASHX", .shares = 1, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 1, .security_type = .cash }, + }; + const wl = [_][]const u8{ "SPCX", "AMZN" }; // AMZN duplicates a holding + const bm = [_][]const u8{ "SPY", "AGG" }; // SPY duplicates the alias above + + const syms = try testPortfolio(&lots).fetchedSymbols(a, .{ + .watchlist_syms = &wl, + .benchmarks = &bm, + }); + defer Portfolio.freeFetchedSymbols(a, syms); + + // AMZN, SPY, QTUM, SPCX, AGG - five distinct, no duplicates. + try std.testing.expectEqual(@as(usize, 5), syms.len); + for ([_][]const u8{ "AMZN", "SPY", "QTUM", "SPCX", "AGG" }) |want| { + var found = false; + for (syms) |s| if (std.mem.eql(u8, s, want)) { + found = true; + }; + try std.testing.expect(found); + } + // Manual-price-only and cash lots stay out. + for (syms) |s| { + try std.testing.expect(!std.mem.eql(u8, s, "ORCBI")); + try std.testing.expect(!std.mem.eql(u8, s, "CASHX")); + } +} + +test "fetchedSymbols: a watchlist-only symbol is included" { + // THE SPCX REGRESSION. It sat in watchlist.srf 39 days out of date + // because no CLI path ever put it in the fetch set - the CLI loaded + // the file for display and priced it from cache. + const a = std.testing.allocator; + var lots = [_]Lot{ + .{ .symbol = "AMZN", .shares = 10, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 200, .security_type = .stock }, + }; + const wl = [_][]const u8{"SPCX"}; + const syms = try testPortfolio(&lots).fetchedSymbols(a, .{ .watchlist_syms = &wl }); + defer Portfolio.freeFetchedSymbols(a, syms); + + var found = false; + for (syms) |s| if (std.mem.eql(u8, s, "SPCX")) { + found = true; + }; + try std.testing.expect(found); +} + +test "fetchedSymbols: a benchmark symbol held nowhere is still included" { + // THE AGG REGRESSION. AGG is not held and not watched - it is the bond + // half of the benchmark comparison, fetched only from a lazy + // projections path with hardcoded default FetchOptions, so + // `--refresh-data=force` could never reach it. + const a = std.testing.allocator; + var lots = [_]Lot{ + .{ .symbol = "AMZN", .shares = 10, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 200, .security_type = .stock }, + }; + const bm = [_][]const u8{ "SPY", "AGG" }; + const syms = try testPortfolio(&lots).fetchedSymbols(a, .{ .benchmarks = &bm }); + defer Portfolio.freeFetchedSymbols(a, syms); + + try std.testing.expectEqual(@as(usize, 3), syms.len); + var found_agg = false; + for (syms) |s| if (std.mem.eql(u8, s, "AGG")) { + found_agg = true; + }; + try std.testing.expect(found_agg); +} + +test "fetchedSymbols: empty and blank inputs produce no entries" { + const a = std.testing.allocator; + var lots = [_]Lot{}; + const wl = [_][]const u8{""}; // blank line in watchlist.srf + const syms = try testPortfolio(&lots).fetchedSymbols(a, .{ .watchlist_syms = &wl }); + defer Portfolio.freeFetchedSymbols(a, syms); + try std.testing.expectEqual(@as(usize, 0), syms.len); +} + +test "fetchedSymbols: result outlives a stack-allocated benchmark override" { + // A projections override lives in a [16]u8 INSIDE the UserConfig + // struct, so borrowing it would dangle as soon as that config went out + // of scope. This is why the union dupes rather than borrows. + const a = std.testing.allocator; + var lots = [_]Lot{}; + var syms: [][]const u8 = undefined; + { + var buf: [16]u8 = undefined; + @memcpy(buf[0..4], "VBIL"); + const bm = [_][]const u8{buf[0..4]}; + syms = try testPortfolio(&lots).fetchedSymbols(a, .{ .benchmarks = &bm }); + @memset(&buf, 0xAA); // scribble over the source + } + defer Portfolio.freeFetchedSymbols(a, syms); + try std.testing.expectEqual(@as(usize, 1), syms.len); + try std.testing.expectEqualStrings("VBIL", syms[0]); +} + +test "watchSymbols: watchlist.srf entries are included, holdings excluded" { + // THE SPCX BUG, at the layer where it actually lived. The version embedded + // in `commands/portfolio.zig` never looked at watchlist.srf at all, so a + // watchlist-only symbol was displayed from whatever the cache happened to + // hold - 39 days old, in SPCX's case - and never fetched. + const a = std.testing.allocator; + var lots = [_]Lot{ + .{ .symbol = "AMZN", .shares = 10, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 200, .security_type = .stock }, + .{ .symbol = "QTUM", .shares = 0, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 0, .security_type = .watch }, + }; + const held = [_][]const u8{"AMZN"}; + const wl = [_][]const u8{ "SPCX", "QTUM", "AMZN", "" }; + + const out = try testPortfolio(&lots).extraPriceSymbols(a, &held, &wl); + defer a.free(out); + + // QTUM once (watch lot, deduped against the watchlist), SPCX from the + // file. AMZN is held so it belongs to the other slice, and the blank + // line is dropped. + try std.testing.expectEqual(@as(usize, 2), out.len); + try std.testing.expectEqualStrings("QTUM", out[0]); + try std.testing.expectEqualStrings("SPCX", out[1]); +} + +test "watchSymbols: a ticker alias on a watch lot is priced by its alias" { + const a = std.testing.allocator; + var lots = [_]Lot{ + .{ .symbol = "NON40OR52", .ticker = "SPY", .shares = 0, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 0, .security_type = .watch }, + }; + const out = try testPortfolio(&lots).extraPriceSymbols(a, &.{}, &.{}); + defer a.free(out); + try std.testing.expectEqual(@as(usize, 1), out.len); + try std.testing.expectEqualStrings("SPY", out[0]); +} + +test "watchSymbols: no watch lots and no watchlist yields an empty set" { + const a = std.testing.allocator; + var lots = [_]Lot{ + .{ .symbol = "AMZN", .shares = 10, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 200, .security_type = .stock }, + }; + const held = [_][]const u8{"AMZN"}; + const out = try testPortfolio(&lots).extraPriceSymbols(a, &held, &.{}); + defer a.free(out); + try std.testing.expectEqual(@as(usize, 0), out.len); +} + +/// OOM-path wrapper for `checkAllAllocationFailures`. +fn fetchedSymbolsOom(a: std.mem.Allocator, lots: []Lot, wl: []const []const u8, bm: []const []const u8) !void { + const syms = try (Portfolio{ .lots = lots, .allocator = a }).fetchedSymbols(a, .{ + .watchlist_syms = wl, + .benchmarks = bm, + }); + Portfolio.freeFetchedSymbols(a, syms); +} + +fn watchSymbolsOom(a: std.mem.Allocator, lots: []Lot, held: []const []const u8, wl: []const []const u8) !void { + const out = try (Portfolio{ .lots = lots, .allocator = a }).extraPriceSymbols(a, held, wl); + a.free(out); +} + +test "fetchedSymbols/watchSymbols: every allocation-failure path unwinds cleanly" { + // Covers the errdefer arms, which are otherwise unreachable: a partial + // build must free the strings it already duped, and the inner arm must + // free a dupe whose append then failed. + var lots = [_]Lot{ + .{ .symbol = "AMZN", .shares = 10, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 200, .security_type = .stock }, + .{ .symbol = "QTUM", .shares = 0, .open_date = Date.fromYmd(2026, 1, 2), .open_price = 0, .security_type = .watch }, + }; + const wl = [_][]const u8{"SPCX"}; + const bm = [_][]const u8{ "SPY", "AGG" }; + const held = [_][]const u8{"AMZN"}; + + try std.testing.checkAllAllocationFailures( + std.testing.allocator, + fetchedSymbolsOom, + .{ &lots, @as([]const []const u8, &wl), @as([]const []const u8, &bm) }, + ); + try std.testing.checkAllAllocationFailures( + std.testing.allocator, + watchSymbolsOom, + .{ &lots, @as([]const []const u8, &held), @as([]const []const u8, &wl) }, + ); +} + +test "extraPriceSymbols: order is stable - watch lots first, then the watchlist file" { + // `PortfolioData.load` used to build this through a StringHashMap, so + // iteration order - and therefore the "[5/28] Loading X" progress order - + // varied run to run for no reason. Callers may now rely on the order. + const a = std.testing.allocator; + var lots = [_]Lot{ + .{ .symbol = "AAPL", .shares = 10, .open_date = Date.fromYmd(2024, 1, 2), .open_price = 100, .security_type = .stock }, + .{ .symbol = "TSLA", .shares = 0, .open_date = Date.fromYmd(2024, 1, 2), .open_price = 0, .security_type = .watch }, + .{ .symbol = "NVDA", .shares = 0, .open_date = Date.fromYmd(2024, 1, 2), .open_price = 0, .security_type = .watch }, + }; + const held = [_][]const u8{"AAPL"}; + const wl = [_][]const u8{ "MSFT", "QTUM" }; + + // Run twice: a hash-order build would be free to differ between calls. + for (0..2) |_| { + const out = try testPortfolio(&lots).extraPriceSymbols(a, &held, &wl); + defer a.free(out); + try std.testing.expectEqual(@as(usize, 4), out.len); + try std.testing.expectEqualStrings("TSLA", out[0]); + try std.testing.expectEqualStrings("NVDA", out[1]); + try std.testing.expectEqualStrings("MSFT", out[2]); + try std.testing.expectEqualStrings("QTUM", out[3]); + } +} diff --git a/src/tui/portfolio_tab.zig b/src/tui/portfolio_tab.zig index d1f7e65..f97753f 100644 --- a/src/tui/portfolio_tab.zig +++ b/src/tui/portfolio_tab.zig @@ -420,10 +420,9 @@ pub const tab = struct { for (wl) |sym| watch_syms.append(a, sym) catch |err| std.log.debug("watch_syms append: {t}", .{err}); } - // Symbols to quote live = watchlist + held stock symbols. Held - // symbols are duped into the arena (see above). + // Symbols to quote live = held + everything on either watch source. + // Held symbols are duped into the arena (see above). var quote_syms: std.ArrayList([]const u8) = .empty; - for (watch_syms.items) |sym| quote_syms.append(a, sym) catch |err| std.log.debug("quote_syms append: {t}", .{err}); if (app.portfolio.file) |pf| { if (pf.stockSymbols(app.allocator)) |hs| { defer app.allocator.free(hs); // outer slice; strings duped into arena @@ -431,6 +430,18 @@ pub const tab = struct { const dup = a.dupe(u8, sym) catch continue; quote_syms.append(a, dup) catch |err| std.log.debug("quote_syms append: {t}", .{err}); } + // The same union the price load uses, so live quotes and + // candles cover the same set. Previously this took only + // `watchlist.srf` and skipped the portfolio's own `.watch` + // lots, so those rows silently fell back to the prior close + // while watchlist rows updated intraday - the same + // two-sources-of-watch-symbols split that left a + // watchlist-only symbol unfetched on the CLI, mirrored. + // Borrowed, not duped: these point into `pf.lots` and + // `app.watchlist`, both stable across this call. + if (pf.extraPriceSymbols(a, hs, watch_syms.items)) |extra| { + for (extra) |sym| quote_syms.append(a, sym) catch |err| std.log.debug("quote_syms append: {t}", .{err}); + } else |err| std.log.debug("extraPriceSymbols for live quotes: {t}", .{err}); } else |err| std.log.debug("stockSymbols for live quotes: {t}", .{err}); }