fix(balances): materialize entries dated before the opening anchor#2434
fix(balances): materialize entries dated before the opening anchor#2434vlnd0 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds a ChangesPre-anchor Balance Materialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07d39a50fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| def calc_start_date | ||
| incremental? ? @window_start_date : account.opening_anchor_date | ||
| incremental? ? @window_start_date : calculation_start_date |
There was a problem hiding this comment.
Preserve pre-anchor rows during incremental purges
When a manual account has a pre-anchor reconciliation, a full recalculation now creates balances before account.opening_anchor_date, but a later incremental sync still enters this branch for any edit after the anchor. I checked Balance::Materializer#purge_stale_balances: in incremental forward mode it preserves only from account.opening_anchor_date and deletes date < oldest_valid_date, so the next normal sync with window_start_date after the anchor removes the newly materialized pre-anchor rows again. The incremental preservation lower bound needs to use the same start date that can precede the opening anchor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — confirmed and fixed in the latest push.
Balance::Materializer#purge_stale_balances was still using account.opening_anchor_date as the incremental-purge lower bound in both branches (the sorted_balances.empty? tail-purge and the oldest_valid_date computation), so a later incremental sync after the anchor would delete the pre-anchor rows a full sync had just created.
Fix: exposed Balance::BaseCalculator#calculation_start_date (the same min(opening_anchor_date, oldest_entry_date) bound the calculator iterates from) as a public method and used it in both purge branches instead of opening_anchor_date. Now the preservation bound matches the calculation bound, so pre-anchor balances survive incremental syncs.
Added a regression test: Balance::MaterializerTest#test_incremental_sync_preserves_balances_dated_before_the_opening_anchor — a pre-anchor balance is preserved through an incremental sync whose window starts after the anchor.
93480c7 to
703e7aa
Compare
jjmata
left a comment
There was a problem hiding this comment.
The root-cause analysis and fix are solid — bounding on min(opening_anchor_date, oldest_entry_date) is exactly the right model, and the three regression tests cover the important paths (forward seed, reverse loop, incremental purge).
One thing worth considering: calculation_start_date fires an extra entries.minimum(:date) query every time it's called, and within a single sync it's called more than once. Looking at materializer.rb, purge_stale_balances references it on two code paths (the early-return incremental branch and the oldest_valid_date assignment), and the forward/reverse calculators call it via calc_start_date/downto. That's 3–4 extra queries per sync.
A simple memoization would eliminate the redundant hits:
def calculation_start_date
@calculation_start_date ||= [ account.opening_anchor_date, account.entries.minimum(:date) ].compact.min
endSince BaseCalculator instances are per-sync and not reused, there's no staleness concern. Worth doing before this lands, especially for accounts with large entry histories where MIN(date) is a non-trivial scan.
Everything else looks great — the return [0, 0] seed guard in opening_starting_balances is clean, the downto(calculation_start_date) bound in the reverse calculator is minimal, and the public visibility with a doc comment is the right call for the materializer to share the same lower bound.
Generated by Claude Code
703e7aa to
89584b8
Compare
|
Thanks for the review! Applied the memoization in the latest push: def calculation_start_date
@calculation_start_date ||= [ account.opening_anchor_date, account.entries.minimum(:date) ].compact.min
endAgreed on the reasoning — calculator instances are per-sync so there's no staleness risk, and this collapses the 3–4 |
|
@jjmata if all ok, i suggest to merge |
Problem
Manual Valuation/reconciliation entries dated before an account's
opening_anchorare stored but never materialized intoBalancerows, so the net-worth chart is empty for that period and a normal sync never backfills it.Root cause: both balance calculators use
account.opening_anchor_dateas the hard lower bound of iteration:Balance::ForwardCalculator#calc_start_date→account.opening_anchor_dateBalance::ReverseCalculator→current_anchor_date.downto(account.opening_anchor_date)and
Account::OpeningBalanceManager#opening_datereturns the opening-anchor valuation's date whenever one exists. The system intends the opening anchor to sit at/before the oldest entry (set_opening_balanceeven validates this), but the reconciliation path (create_reconciliation) does not move the anchor back when a valuation is created with an earlier date. The invariant breaks, and every entry before the anchor is silently clipped out of the series.How to reproduce
opening_anchorat, say, today).account.balanceshas no row for that date → net-worth chart flat/empty before the anchor.Fix
Bound the calculation window on
min(opening_anchor_date, oldest_entry_date)instead of the anchor date alone (new sharedBalance::BaseCalculator#calculation_start_date).*_adjustmentsfield is affected — later totals are correct.downtobound;use_opening_anchor_for_date?still keys off the anchor's real date, so the anchor's own treatment is unchanged and pre-anchor reconciliation waypoints reset normally.No data migration and no anchor mutation — purely a calculation-window fix, so it heals existing accounts on the next sync.
Tests
forward_calculator_test.rb: "materializes entries dated before the opening anchor" — reconciliation before the anchor is materialized; window starts at the earliest entry; balances correct across the anchor.reverse_calculator_test.rb: "materializes reconciliation waypoints dated before the opening anchor".Summary by CodeRabbit