diff --git a/TODO.md b/TODO.md index 0b0e95b..82cbaa3 100644 --- a/TODO.md +++ b/TODO.md @@ -5,8 +5,9 @@ ordered roughly by priority within each section. Priority labels (`HIGH` / `MEDIUM` / `LOW`) mark items that deserve explicit ranking; unlabeled items are "someday, if the mood strikes." -**Next up:** configurable benchmark symbols (low-effort win) and the -manual-check accounts mechanism (medium effort, real user value). +**Next up:** configurable benchmark symbols (low-effort win, exercises +the post-refactor framework on a small feature) and the manual-check +accounts mechanism (medium effort, real user value). ## Projections: future enhancements @@ -175,136 +176,6 @@ renaming to `src/render.zig` to better describe what's left, or splitting the braille chart out (it's ~600 lines on its own). Not blocking — file it as cleanup if and when it bites. -## Refactor: TUI App struct knows too much about each tab — priority MEDIUM - -`src/tui.zig`'s `App` struct currently has dozens of tab-specific -fields scattered across its top level — `projections_loaded`, -`projections_disabled`, `projections_config`, `projections_ctx`, -`projections_horizon_idx`, `projections_image_id`, -`projections_image_width`, `projections_image_height`, -`projections_chart_dirty`, `projections_chart_visible`, -`projections_events_enabled`, `projections_value_min`, -`projections_value_max`, `projections_as_of`, -`projections_as_of_requested`, `projections_overlay_actuals`, -plus equivalents for portfolio, history, options, earnings, -analysis, perf, and quote tabs. - -This couples `App` to the implementation details of every tab. -Touching a single tab's state shape requires editing the central -struct, which makes refactors noisy and discourages tab modules -from owning their own state cleanly. - -**Proposal: each tab gets exactly ONE field on `App`** — a -struct (or pointer to a struct) defined in that tab's own file. -The tab module owns its state shape; `App` only carries the -top-level reference. - -Sketch: - -```zig -// src/tui/projections_tab.zig -pub const State = struct { - loaded: bool = false, - disabled: bool = false, - config: projections.UserConfig = .{}, - ctx: ?ProjectionContext = null, - horizon_idx: usize = 0, - chart: ChartState = .{}, - as_of: AsOfState = .{}, - overlay_actuals: bool = false, - // ... -}; - -// src/tui.zig -pub const App = struct { - // ... shared cross-tab state (today, allocator, vx_app, ...) ... - portfolio: portfolio_tab.State = .{}, - quote: quote_tab.State = .{}, - perf: perf_tab.State = .{}, - history: history_tab.State = .{}, - projections: projections_tab.State = .{}, - options: options_tab.State = .{}, - earnings: earnings_tab.State = .{}, - analysis: analysis_tab.State = .{}, -}; -``` - -After the migration, `app.projections_overlay_actuals` becomes -`app.projections.overlay_actuals`, etc. Each tab's State struct -documents its own invariants without polluting `App`. - -**Benefits:** - -- Tab modules become genuinely self-contained. Adding new state - to a tab only touches that tab's file. -- `App`'s field count drops from ~80 to ~10 (cross-cutting state - + 8 tab-state fields). -- Clearer ownership: when reading `App`, you can tell at a - glance which fields are shared vs tab-private. -- Easier to reason about lifetimes — each tab's `freeLoaded()` - operates on its own struct rather than reaching into App. -- Onboarding new contributors: "to add a tab feature, define - state in the tab file" instead of "edit two files in lockstep." - -**Migration approach:** - -This is a large mechanical refactor (~80+ field renames across -all tab files plus `tui.zig`). Best done as one focused PR: - -1. Define `State` in each tab file with all current fields. -2. Add the eight new fields to `App`; flip them on as defaults - (so old `app.projections_*` accesses temporarily still work - if we add accessor shims, but cleaner to just rip the - bandaid). -3. Sweep all `app._` references → `app..`. -4. Delete the old top-level fields from `App`. -5. Verify `zig build test` is unchanged. The refactor should be - strictly behavior-preserving. - -Risks: large diff (touches every tab file plus tui.zig), but -mechanical — no logic changes, no tests should move. The pre- -commit hooks catch any miss instantly. - -**While we're at it: action handler bodies should also move.** -The same shape problem shows up in `tui.zig`'s keybind-action -dispatch — `sort_reverse`, `toggle_chart`, `toggle_events`, -`account_filter`, and the per-tab branches inside them are -~100 lines of tab-specific logic living in the central event -loop. Concretely (line numbers as of writing): - -- `sort_reverse` (~line 1328) dual-dispatches by active tab: - portfolio flips sort direction + calls - `sortPortfolioAllocations` / `rebuildPortfolioRows`; - projections toggles overlay-actuals + reloads data + sets - status. None of that body is `App`-level concern; it's two - separate tabs' private state mutations. -- `toggle_chart` (~line 1382) flips - `projections_chart_visible` and resets `scroll_offset`. Pure - projections-tab business. -- `toggle_events` (~line 1390) flips - `projections_events_enabled` and triggers a reload. Same. -- `account_filter` (~line 1366) opens the account picker mode - with portfolio-tab-specific cursor positioning. - -The cleaner shape: each tab module exposes a -`handleAction(app, action) bool` (or similar) that returns -true when it consumed the action. `tui.zig`'s dispatch becomes -a thin "ask the active tab if it wants this action; otherwise -fall through to global handlers." The body of each `case` -shrinks to a one-liner that dispatches to -`tab_module.handleAction(self)`. - -This pairs naturally with the State-struct migration above — -the tab module's `handleAction` operates on its own State -struct rather than reaching into `App`. Some keybinds are -genuinely cross-cutting (quit, refresh, tab navigation, scroll) -and stay in the central handler. The split is "App owns -chrome; tab owns content." - -Driver: every overlay-actuals-shaped feature added to a tab -recently has involved adding 1–2 fields to `App`, and the -struct keeps growing. Eventually it becomes unreviewable. - ## Bug: braille charts use raw `close`, not `adj_close` — cliff at splits **Reproduction:** `zfin quote SOXX` (or the TUI quote tab). The @@ -371,27 +242,38 @@ on 2024-06-10) for a louder example. But it's confusing when scanning a chart and seeing a 50% drop that isn't real. -## Audit: manual-check accounts mechanism (NYL, Kelly's ESPP, etc.) — priority HIGH +## Audit: manual-check accounts mechanism — priority HIGH Some accounts/positions can't be reconciled from broker CSVs and need a -human-in-the-loop reminder at the audit step. Examples: +human-in-the-loop reminder at the audit step. Two recurring shapes: -- **NY Life** — no CSV export at all. Values only live in periodic - statements. -- **Kelly's ESPP** — accrued payroll-deduction cash doesn't appear in the - Fidelity positions CSV until the purchase date hits (typically every - 6 months). Between purchases the cash is a real contribution that - `zfin audit` can't see. -- Future: treat as an open category. +- **No-CSV-export accounts** (e.g. some insurance / annuity products) + where values only live in periodic statements. Git can't detect a + "change" because nothing changes locally; the user has to log in + to see the new value. +- **Payroll-deduction-then-purchase accounts** (e.g. ESPP) where + payroll-deducted cash doesn't appear in the broker positions CSV + until the purchase date hits (typically every 6 months). Between + purchases the cash is a real contribution that `zfin audit` can't + see. The existing `update_cadence::weekly|monthly|quarterly|none` field already sort-of covers this, but has two gaps: 1. It fires off the last *git-detected change*, not the last *human - review*. For NYL, the value sometimes hasn't changed in months — so - git never fires, cadence never trips. -2. ESPP needs weekly-ish attention while accumulating cash between - purchases, but the accrued balance is invisible to the CSV audit. + review*. For statement-only accounts, the value sometimes hasn't + changed in months — so git never fires, cadence never trips. +2. Payroll-deduction accounts need weekly-ish attention while + accumulating cash between purchases, but the accrued balance is + invisible to the CSV audit. + +Drift symptom seen in practice: several accounts on +`update_cadence::weekly` in `accounts.srf` weren't flagged as overdue +despite no changes in two weeks, because the cadence reads +git-detected change time rather than human-review time. The cadence +values themselves may also be wrong for these accounts — revisit +whether weekly is the right cadence vs. monthly/quarterly given how +rarely they actually change. ### Options @@ -402,21 +284,164 @@ B. **`last_refreshed::YYYY-MM-DD` field on `accounts.srf`** — explicit human-review timestamp, decoupled from git-detected changes. Audit compares `today - last_refreshed` against the cadence. User bumps the field when they check the statement. Probably the most - correct fit for NYL. + correct fit for statement-only accounts. C. **Sticky TODO list** — a `todos.srf` or `todo::` field on accounts that audit always surfaces until cleared. General-purpose; also covers "remember to rebalance on 5/15". -### ESPP-specific follow-through +### ESPP-style accrual follow-through -ESPP is also a contribution-attribution blind spot. If Kelly's paystub -deducts $X/week but the cash lot doesn't reach `portfolio.srf` until -the purchase date, the attribution math is under-counting contributions -and over-counting the purchase-week gain. Possible fixes are discussed -in the "Contributions diff" TODO below — option C there (per-account -`cash_is_contribution`) would make manually-entered ESPP -cash additions count correctly. +Payroll-deduction accounts are also a contribution-attribution blind +spot. If a paystub deducts $X/week but the cash lot doesn't reach +`portfolio.srf` until the purchase date, the attribution math is +under-counting contributions and over-counting the purchase-week +gain. Possible fixes are discussed in the "Contributions diff" TODO +below — option C there (per-account `cash_is_contribution`) would +make manually-entered ESPP-style cash additions count correctly. + +## Investigate: audit phantom discrepancy on freshly-added lots — priority MEDIUM + +**Observed during a weekly audit run:** `zfin audit` flagged a +discrepancy on a newly-added single-symbol lot that, on inspection, +was correctly entered in `portfolio.srf` and matched the brokerage. +The flag was a false positive. + +Possible causes (not yet diagnosed): + +- Audit logic mishandles lots whose `open_date::` is the audit's + reference date (today) — share count is "new" but no transaction + history yet supports it from the audit's perspective. +- The broker's positions CSV was internally inconsistent the morning + after a high-movement day. Other lots on the same export may + have been affected; only the new lot was noticed because it was new. +- Some interaction with rollup/DRIP classification on the new lot. + +Repro: next time a brand-new lot is added on a heavy-movement day, +check whether `zfin audit` flags it without cause. Compare the +audit's reported brokerage figure against the raw CSV value to +isolate audit-logic bug from CSV-input bug. + +Workaround for now: visual inspection. If `portfolio.srf` matches +the brokerage UI, ignore the audit flag for this cycle and move on. + +## Investigate: `compare --projections` block disagrees with standalone `projections` + +**Observed during a weekly review:** The embedded projections block +from `zfin compare --projections 1W --commit-before HEAD` shows +projected-return / SWR numbers that don't line up with what +`zfin projections` (standalone, no flags) and/or +`zfin projections --as-of 1W` produce for the same dates. + +The shape of the disagreement seen in practice was small but +consistent — a fraction-of-a-percent drift on projected return and +a few-thousand-dollar drift on safe-withdrawal dollar amounts — +across "then" and "now" sides of the same compare run. + +The "now" side should match `zfin projections`, and the "then" side +should match `zfin projections --as-of 1W`. Confirm which side(s) +disagree on next investigation pass. + +Candidates to investigate: + +- Different snapshot resolution between the two code paths. `compare` + with `1W` snaps to the nearest-earlier history file; standalone + `projections --as-of 1W` may resolve to the same file or a different + one. Check the muted "(requested X; nearest snapshot: Y)" notes from + both runs. +- Different valuation rules between paths. REPORT.md §2 already + documents that the commit-side pipeline values DRIP/rollup share + deltas using **current** cache prices, not snapshot prices — could + bleed into the embedded projections block if it's reading + commit-side state. +- `metadata.srf` / `projections.srf` loaded as-of-today vs. as-of-then. + REPORT.md §4 already calls this out as a known limitation; verify + whether both paths apply it consistently. +- The `--commit-before HEAD` flag affecting the projections block in + `compare` but not standalone `projections` (which doesn't take that + flag). + +Cross-check: run `zfin projections --vs 1W` and compare its compact +output against the `compare --projections` embedded block. If those +agree, the discrepancy is between `--vs` and standalone `projections`. +If they disagree, the bug is in `compare`'s embedding path specifically. + +## Investigate: missing portfolio "Week" return in `zfin projections` + +**Symptom:** The `zfin projections` CLI benchmark table has a "Week" +column populated for SPY/AGG/Benchmark rows but the **Your Portfolio** +row's Week cell is missing (or shows `--`). The 1Y/3Y/5Y/10Y portfolio +columns work fine. + +Lead: `src/views/projections.zig` line ~394 calls +`performance.weekReturn` for SPY and AGG candles, but a corresponding +call for the portfolio aggregate doesn't appear to be wired in. The +benchmark week return is composed weighted from SPY+AGG; the +portfolio side needs its own weekly aggregation across held lots. + +Probably a small fix — `weekReturn` already exists in +`src/analytics/performance.zig`, just needs to be applied to the +portfolio's value series (or to a synthetic weighted candle from +held positions) and threaded into the view model. Verify the TUI +projections tab has the same gap (`src/tui/projections_tab.zig`). + +## Investigate: small dollar-value discrepancy between consecutive `compare` runs + +**Symptom:** Last week's `zfin compare 1W` reported a "now" Total +Investable Assets that didn't match this week's `zfin compare 1W` +"then" value by a small but non-zero amount (low single-digit +thousands). The two numbers should be identical — both are reading +the same week-ago history snapshot file. + +Candidates to investigate: + +- Snapshot file mutated after last week's run (re-snapshot? manual + edit?). Check `git log -- history/-portfolio.srf` and + `git diff` against any prior version if tracked. +- Live cache prices changed between runs and the snapshot path is + somehow falling through to live prices for some symbols. The + snapshot SHOULD be self-contained (shares × snapshotted price), + but verify by re-running `zfin compare ` (same date + both ends) and confirming the same number both ways. +- Last week's reported "now" was computed against a working-copy + portfolio that was later edited (reconciliation tweak post-run), + while this week's "then" reads the committed snapshot. Cross-check + against any archived report output from last week. +- `price_ratio` or `adj_close` semantics differing between code paths + (the REPORT.md §2 caveat about commit-side using current prices + for DRIP/rollup deltas is a known inconsistency in attribution — + may or may not apply here). + +If the source is the working-copy-vs-snapshot mismatch (third bullet), +that's a workflow issue, not a bug — but worth confirming. + +## Investigate: detailed 401(k) contributions data source + +Found a more detailed contributions screen on at least one +employer-sponsored 401(k) provider portal — distinct from the +standard positions/holdings view we already pull from. Worth +investigating whether this unlocks better attribution than what +we get from the positions CSV alone, and whether other 401(k) +providers expose similar screens. + +Open questions to answer when picking this up: + +- Which screen specifically (path / URL within the portal)? Is there + an export option, or is it view-only / scrape territory? +- What fields does it expose (employee pre-tax, employer match, + after-tax / mega-backdoor, by-pay-period dates, per-fund + allocations)? +- Refresh cadence — per-paycheck, daily, on-demand? +- Can it be auto-discovered like the existing audit CSVs, or + is it manual-entry territory? + +If the export is structured and recurring, this could feed a +401(k)-specific contributions classifier that bypasses the lot-diff +heuristic for that account, similar to how `cash_is_contribution` +opts ESPP/HSA accounts into cash-based attribution. + +Related: ESPP-style accrual blind spot in the "Audit: manual-check +accounts mechanism" section above. ## In-kind transfer support (`type::in_kind`) — priority MEDIUM