Skip to content

feat(balance): Preserve historical balances as waypoints for linked accounts#1663

Open
CrossDrain wants to merge 13 commits intowe-promise:mainfrom
CrossDrain:CrossDrain/issue1492
Open

feat(balance): Preserve historical balances as waypoints for linked accounts#1663
CrossDrain wants to merge 13 commits intowe-promise:mainfrom
CrossDrain:CrossDrain/issue1492

Conversation

@CrossDrain
Copy link
Copy Markdown
Contributor

@CrossDrain CrossDrain commented May 3, 2026

Overview

Resolves the issue where linked accounts (e.g., Plaid, SnapTrade) lose their historical balance data over time. This PR fixes #1492, fixes #1484.

Previously, CurrentBalanceManager destructively overwrote the single current_anchor on every sync. Because the reverse calculator had only today's balance to work backward from, any gaps or inaccuracies in the transaction history led to drifted, incorrect historical balances and missing data on the Net Worth chart.

This PR implements an "anchor rotation" system to gracefully accumulate daily API-reported balances and uses them as rigid waypoints to correct historical balance drift.

Changes Made

  • Anchor Rotation (CurrentBalanceManager): Before overwriting a current_anchor, we now check if it belongs to a previous day. If so, it is preserved by converting its kind to reconciliation (named "Manual balance update"), and a fresh current_anchor is laid down for today.
    • Note: Same-day syncs (e.g. hourly jobs) still update the anchor in-place to prevent an explosion of reconciliation entries.
  • Waypoint Support (ReverseCalculator): Updated the reverse calculation loop to actively search for reconciliation valuations. When it hits a waypoint during its reverse walk, it trusts the API-reported valuation and hard-resets the balance, neutralizing any drift caused by missing historical transactions.
  • Monitoring: Added a Rails.logger.info hook inside the conversion block to easily track anchor rotations in production.
  • Testing: Added comprehensive unit tests proving the rotation logic (cross-day vs. same-day) and integration tests confirming that the waypoints successfully halt drift in the reverse calculator.

Architectural Note

The original implementation plan (suggested in #1492) assumed that simply converting the anchors to reconciliations was sufficient to preserve history. However, Balance::Materializer purges stale records and recalculates them on sync. Without the ReverseCalculator update included in this PR, the system would have ignored the preserved data entirely and overwritten the correct history with drifted transaction flows.

Summary by CodeRabbit

  • Bug Fixes

    • Linked-account balance updates performed on a different day now atomically preserve the prior day's balance as a historical reconciliation point and create or update today's anchor consistently.
    • Reverse balance calculation now treats reconciliation waypoints as reset points so derived balances align with reconciliation dates.
  • Tests

    • Updated and added tests validating reconciliation preservation, its impact on reverse calculations, and unchanged same-day behavior.

…napTrade, Plaid)

When updating a linked account's balance, the previous day's current_anchor
is now preserved as a reconciliation valuation before being replaced. This
creates a chain of API-reported balance waypoints over time. The
ReverseCalculator has been updated to treat these reconciliation valuations
as reset points during reverse syncs, ensuring historical balances accurately
reflect the known API-reported values even with incomplete transaction history.
@brin-security-scanner brin-security-scanner Bot added the contributor:flagged Contributor flagged for review by trust analysis. label May 3, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 3, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Linked-account balance updates now convert a previous-day current_anchor into a reconciliation valuation inside a DB transaction before creating/updating today's current_anchor. The reverse balance calculator now treats reconciliation valuations as waypoints that reset computed cash/non-cash balances from the reconciliation date onward.

Changes

Reconciliation Waypoint Preservation & Consumption

Layer / File(s) Summary
Core Anchor Preservation
app/models/account/current_balance_manager.rb
set_current_balance_for_linked_account now runs inside an ActiveRecord::Base.transaction. New helper preserve_anchor_as_reconciliation_if_stale converts a stale (not Date.current) current_anchor valuation to kind: "reconciliation", updates its entry name via Valuation.build_reconciliation_name(...), and clears the memoized @current_anchor_valuation.
Current Anchor Update/Create
app/models/account/current_balance_manager.rb
After potential preservation, the method re-checks current_anchor_valuation and either calls update_current_anchor or create_current_anchor, returning a Result that reflects changes_made. create_current_anchor no longer reloads all valuations; it clears @current_anchor_valuation so subsequent reads see the new anchor.
Reverse Calculator Waypoint Handling
app/models/balance/reverse_calculator.rb
calculate now calls sync_cache.get_valuation(date) and, when a reconciliation valuation is present, recomputes end_cash_balance/end_non_cash_balance from valuation.amount (respecting account cash/non-cash split), sets start_* to those values, and forces market_value_change = 0; otherwise falls back to opening-anchor or derived-start logic.
Comment Clarification
app/models/balance/reverse_calculator.rb
Comment updated to clarify that only opening_anchor_date uses opening-anchor handling and reconciliation waypoints are handled separately.
Tests — Current Balance Manager
test/models/account/current_balance_manager_test.rb
Replaces previous linked-account anchor tests with two scenarios: (1) previous-day update converts stale anchor to reconciliation (preserving amount and setting reconciliation entry name) and creates today's current_anchor; (2) same-day calls either no-op for identical value or update existing current_anchor amount without changing kind/date/counts.
Tests — Reverse Calculator
test/models/balance/reverse_calculator_test.rb
Replaces a test that expected reconciliation valuations to be ignored with tests asserting reconciliation valuations act as waypoints that reset reverse-calculated balances on and after the reconciliation date; updates regression expectations accordingly.
Manifests
Gemfile
Manifest listed in diffs.

Sequence Diagram

sequenceDiagram
    actor User
    participant LinkedAcct as Linked Account
    participant CurrBalMgr as CurrentBalanceManager
    participant Valuations as Valuations/Entries
    participant RevCalc as ReverseCalculator
    participant SyncCache as SyncCache

    User->>LinkedAcct: set_current_balance(new_amount, today)
    LinkedAcct->>CurrBalMgr: set_current_balance_for_linked_account()
    activate CurrBalMgr

    CurrBalMgr->>Valuations: load current_anchor (if any)
    alt stale previous-day current_anchor exists
        CurrBalMgr->>Valuations: update valuation.kind -> "reconciliation"
        CurrBalMgr->>Valuations: update entry.name via Valuation.build_reconciliation_name(...)
        CurrBalMgr->>CurrBalMgr: clear memoized `@current_anchor_valuation`
    end

    CurrBalMgr->>Valuations: create or update today's current_anchor (clear memoized anchor after create)
    deactivate CurrBalMgr

    User->>RevCalc: calculate(date_range)
    activate RevCalc
    loop For each date (oldest -> newest)
        RevCalc->>SyncCache: get_valuation(date)
        SyncCache-->>RevCalc: valuation (maybe)
        alt reconciliation valuation present
            RevCalc->>RevCalc: set end balances from valuation.amount
            RevCalc->>RevCalc: set start balances = end balances
            RevCalc->>RevCalc: set market_value_change = 0
        else
            RevCalc->>RevCalc: use opening anchor or derive from prior balances
        end
    end
    deactivate RevCalc
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Suggested labels

contributor:verified

Suggested reviewers

  • jjmata
  • sokie

Poem

🐰 I hop through ledgers, tidy and spry,
Yesterday's anchor learns to stand by.
Today gets fresh, the old one stays bright,
Reconciled waypoints guide balance through night. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: preserving historical balances as waypoints for linked accounts through an anchor-rotation and waypoint system in the balance calculation logic.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@brin-security-scanner brin-security-scanner Bot added the pr:verified PR passed security analysis. label May 3, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d31c2f7a99

ℹ️ 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".

Comment thread app/models/balance/reverse_calculator.rb Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
app/models/account/current_balance_manager.rb (1)

99-111: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make stale-anchor rotation and new-anchor creation atomic.

Line 102 can convert the old current_anchor to reconciliation, but Lines 109-110 create the replacement anchor outside that same transaction. If creation fails, the account can be left without any current_anchor.

Proposed fix
 def set_current_balance_for_linked_account(balance)
-  preserve_anchor_as_reconciliation_if_stale if current_anchor_valuation
-
-  # Re-check: the memoized value was cleared if the anchor was converted
-  if current_anchor_valuation
-    changes_made = update_current_anchor(balance)
-    Result.new(success?: true, changes_made?: changes_made, error: nil)
-  else
-    create_current_anchor(balance)
-    Result.new(success?: true, changes_made?: true, error: nil)
-  end
+  ActiveRecord::Base.transaction do
+    preserve_anchor_as_reconciliation_if_stale if current_anchor_valuation
+
+    # Re-check: the memoized value was cleared if the anchor was converted
+    if current_anchor_valuation
+      changes_made = update_current_anchor(balance)
+      Result.new(success?: true, changes_made?: changes_made, error: nil)
+    else
+      create_current_anchor(balance)
+      Result.new(success?: true, changes_made?: true, error: nil)
+    end
+  end
 end

Also applies to: 123-136

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/account/current_balance_manager.rb` around lines 99 - 111, The
stale-anchor rotation and new-anchor creation must be made atomic: wrap the
preserve_anchor_as_reconciliation_if_stale call and the subsequent
create_current_anchor/update_current_anchor calls inside a single database
transaction so that conversion and replacement happen together or are rolled
back on error; modify set_current_balance_for_linked_account to start a
transaction around the branch that may call
preserve_anchor_as_reconciliation_if_stale and then call create_current_anchor
or update_current_anchor within that transaction, returning Result only after
commit, and apply the same transactional fix to the analogous block referenced
later (the other method handling anchor rotation/creation).
app/models/balance/reverse_calculator.rb (1)

15-42: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Limit waypoint reset to reconciliation valuations only.

Line 27 currently resets balances for any valuation returned by sync_cache.get_valuation(date). Since that lookup can return non-reconciliation valuations too, this can bypass normal flow reversal on those dates.

Proposed fix
-        valuation = sync_cache.get_valuation(date)
+        valuation = sync_cache.get_valuation(date)
+        reconciliation_waypoint = valuation if valuation&.entryable&.reconciliation?
@@
-        elsif valuation
+        elsif reconciliation_waypoint
@@
-            total_balance: valuation.amount,
+            total_balance: reconciliation_waypoint.amount,
             date: date
           )
-          end_non_cash_balance = valuation.amount - end_cash_balance
+          end_non_cash_balance = reconciliation_waypoint.amount - end_cash_balance
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/balance/reverse_calculator.rb` around lines 15 - 42, The current
branch treats any valuation returned by sync_cache.get_valuation(date) as a
reconciliation waypoint and resets balances; change it to only reset when the
valuation is actually a reconciliation. Update the elsif block to check a
reconciliation marker on the valuation (e.g., if valuation.reconciliation? or
valuation.reconciliation == true) before performing the reset logic in
reverse_calculator.rb (the block using derive_cash_balance_on_date_from_total,
end_cash_balance/end_non_cash_balance,
start_cash_balance/start_non_cash_balance, and market_value_change). If the
valuation is present but not a reconciliation, fall through to the existing else
(normal flow reversal) instead of resetting.
test/models/balance/reverse_calculator_test.rb (1)

31-76: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add a regression case for current-day flows alongside a reconciliation waypoint.

This scenario validates the waypoint reset well, but it does not protect against accidentally skipping current-day transaction/trade flow reversal when a current_anchor valuation is present on Date.current (Line 34 setup path). A focused case with a same-day transaction would close that gap.

Suggested test shape
+  test "reconciliation waypoint does not suppress current-day flow reversal" do
+    account = create_account_with_ledger(
+      account: { type: Depository, balance: 20000, cash_balance: 20000, currency: "USD" },
+      entries: [
+        { type: "current_anchor", date: Date.current, balance: 20000 },
+        { type: "transaction", date: Date.current, amount: 500 },
+        { type: "reconciliation", date: 2.days.ago, balance: 17000 },
+        { type: "opening_anchor", date: 4.days.ago, balance: 15000 }
+      ]
+    )
+
+    calculated = Balance::ReverseCalculator.new(account).calculate
+    # Assert Date.current start/end reflect reversal of today's transaction.
+  end
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/models/balance/reverse_calculator_test.rb` around lines 31 - 76, Add a
regression test to the existing "reconciliation valuations act as waypoints..."
test suite that ensures same-day (Date.current) transaction/trade flows are
processed/reversed even when a current_anchor valuation exists; update or create
a new test that uses create_account_with_ledger to include a current_anchor at
Date.current, a reconciliation waypoint earlier (e.g., 2.days.ago), and also
inject a transaction/trade entry dated Date.current (or a flow affecting that
day) so that Balance::ReverseCalculator#calculate must account for that flow;
assert expected results with assert_calculated_ledger_balances ensuring the
Date.current ledger shows the flow effect (not ignored) while earlier dates
still reset to the reconciliation waypoint.
🧹 Nitpick comments (1)
test/models/account/current_balance_manager_test.rb (1)

234-234: ⚡ Quick win

Avoid hard-coded reconciliation label in assertion.

Use the name builder instead of a literal string so this test stays stable if label text changes.

Proposed fix
-      assert_equal "Manual balance update", preserved_valuation.entry.name
+      assert_equal Valuation.build_reconciliation_name(`@linked_account.accountable_type`), preserved_valuation.entry.name
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/models/account/current_balance_manager_test.rb` at line 234, The
assertion uses a hard-coded label string; replace the literal "Manual balance
update" with the canonical name builder used by the reconciliation code so the
test follows production naming logic—specifically, change the expectation on
preserved_valuation.entry.name to call the reconciliation name builder (the same
builder used when creating the entry in production) instead of the literal
string so the test remains stable when label text changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app/models/account/current_balance_manager.rb`:
- Line 130: The info-level log in Rails.logger.info currently includes sensitive
monetary data (entry.amount); remove the amount from the info payload and log
only non-sensitive identifiers (e.g., account.id and entry.date) in the
current_balance_manager.rb code path where the conversion to reconciliation is
logged; if the amount is needed for troubleshooting, either log it at debug
level or log a redacted/boolean indicator (e.g., "has_amount=true") instead of
entry.amount, and update the log call that references entry.amount accordingly.

---

Outside diff comments:
In `@app/models/account/current_balance_manager.rb`:
- Around line 99-111: The stale-anchor rotation and new-anchor creation must be
made atomic: wrap the preserve_anchor_as_reconciliation_if_stale call and the
subsequent create_current_anchor/update_current_anchor calls inside a single
database transaction so that conversion and replacement happen together or are
rolled back on error; modify set_current_balance_for_linked_account to start a
transaction around the branch that may call
preserve_anchor_as_reconciliation_if_stale and then call create_current_anchor
or update_current_anchor within that transaction, returning Result only after
commit, and apply the same transactional fix to the analogous block referenced
later (the other method handling anchor rotation/creation).

In `@app/models/balance/reverse_calculator.rb`:
- Around line 15-42: The current branch treats any valuation returned by
sync_cache.get_valuation(date) as a reconciliation waypoint and resets balances;
change it to only reset when the valuation is actually a reconciliation. Update
the elsif block to check a reconciliation marker on the valuation (e.g., if
valuation.reconciliation? or valuation.reconciliation == true) before performing
the reset logic in reverse_calculator.rb (the block using
derive_cash_balance_on_date_from_total, end_cash_balance/end_non_cash_balance,
start_cash_balance/start_non_cash_balance, and market_value_change). If the
valuation is present but not a reconciliation, fall through to the existing else
(normal flow reversal) instead of resetting.

In `@test/models/balance/reverse_calculator_test.rb`:
- Around line 31-76: Add a regression test to the existing "reconciliation
valuations act as waypoints..." test suite that ensures same-day (Date.current)
transaction/trade flows are processed/reversed even when a current_anchor
valuation exists; update or create a new test that uses
create_account_with_ledger to include a current_anchor at Date.current, a
reconciliation waypoint earlier (e.g., 2.days.ago), and also inject a
transaction/trade entry dated Date.current (or a flow affecting that day) so
that Balance::ReverseCalculator#calculate must account for that flow; assert
expected results with assert_calculated_ledger_balances ensuring the
Date.current ledger shows the flow effect (not ignored) while earlier dates
still reset to the reconciliation waypoint.

---

Nitpick comments:
In `@test/models/account/current_balance_manager_test.rb`:
- Line 234: The assertion uses a hard-coded label string; replace the literal
"Manual balance update" with the canonical name builder used by the
reconciliation code so the test follows production naming logic—specifically,
change the expectation on preserved_valuation.entry.name to call the
reconciliation name builder (the same builder used when creating the entry in
production) instead of the literal string so the test remains stable when label
text changes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 775ce475-0f7a-4068-95fa-d142af60911d

📥 Commits

Reviewing files that changed from the base of the PR and between faf31b9 and d31c2f7.

📒 Files selected for processing (4)
  • app/models/account/current_balance_manager.rb
  • app/models/balance/reverse_calculator.rb
  • test/models/account/current_balance_manager_test.rb
  • test/models/balance/reverse_calculator_test.rb

Comment thread app/models/account/current_balance_manager.rb Outdated
The ReverseCalculator was incorrectly treating the current_anchor valuation
(on Date.current) as a reconciliation waypoint, causing it to reset the
balance and ignore same-day transactions. This fix adds a check to ensure
only true reconciliation entries (entryable.reconciliation?) trigger the
reset behavior.

Additionally, set_current_balance_for_linked_account is now wrapped in a
database transaction to ensure atomicity when preserving stale anchors and
creating/updating the current anchor. Logging has been improved to use
debug level for amount details.

A regression test was added to verify that same-day flows are correctly
processed when a current_anchor exists on the current date.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/models/account/current_balance_manager_test.rb (1)

230-234: ⚡ Quick win

Add a date assertion to fully validate historical preservation of the converted anchor.

The test verifies kind, amount, and name on the preserved reconciliation, but omits the entry.date. The entire motivation of this feature is that the anchor is preserved at its original historical date — if production code ever inadvertently updated the date during conversion, no assertion here would catch it. Inside the travel_to block, Date.yesterday refers to the original creation date, making this a one-liner.

✅ Proposed addition
  preserved_valuation = Valuation.find(original_id)
  assert_equal "reconciliation", preserved_valuation.kind
  assert_equal 1000, preserved_valuation.entry.amount
  assert_equal Valuation.build_reconciliation_name(`@linked_account.accountable_type`), preserved_valuation.entry.name
+ assert_equal Date.yesterday, preserved_valuation.entry.date
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/models/account/current_balance_manager_test.rb` around lines 230 - 234,
The test currently checks preserved_valuation.kind/amount/name but misses
verifying the preserved entry date; inside the travel_to block add an assertion
that preserved_valuation.entry.date equals Date.yesterday to ensure the
converted anchor kept its original historical date (locate preserved_valuation
from Valuation.find(original_id) and assert on preserved_valuation.entry.date).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/models/account/current_balance_manager_test.rb`:
- Around line 230-234: The test currently checks
preserved_valuation.kind/amount/name but misses verifying the preserved entry
date; inside the travel_to block add an assertion that
preserved_valuation.entry.date equals Date.yesterday to ensure the converted
anchor kept its original historical date (locate preserved_valuation from
Valuation.find(original_id) and assert on preserved_valuation.entry.date).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bc0c8428-7ea5-4a40-8b96-d312825b038b

📥 Commits

Reviewing files that changed from the base of the PR and between d31c2f7 and 0741ed8.

📒 Files selected for processing (4)
  • app/models/account/current_balance_manager.rb
  • app/models/balance/reverse_calculator.rb
  • test/models/account/current_balance_manager_test.rb
  • test/models/balance/reverse_calculator_test.rb
✅ Files skipped from review due to trivial changes (1)
  • app/models/balance/reverse_calculator.rb
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/models/balance/reverse_calculator_test.rb
  • app/models/account/current_balance_manager.rb

Add validation that valuation entries created during balance
preservation are dated as of yesterday. This prevents future-dated
entries and maintains temporal accuracy in financial snapshots.
Copy link
Copy Markdown
Collaborator

jjmata commented May 4, 2026

Solid approach and well-documented. A few things worth discussing before merge:

Behavior reversal for existing user-created reconciliation valuations
The previous code explicitly ignored reconciliation valuations in the reverse sync (with a comment calling it "an artificial constraint"). This PR makes them hard-reset waypoints. That's the right call for anchors preserved by this new mechanism — but are there existing reconciliation valuations that users created manually (e.g., via "Manual balance update" in the UI) for linked accounts? Those would now also reset the reverse calculation, which is a silent behavior change for existing data. Worth verifying that the scope of affected valuations is understood.

Redundant nested transaction in preserve_anchor_as_reconciliation_if_stale
The method opens ActiveRecord::Base.transaction but is already called from inside the outer transaction in set_current_balance_for_linked_account. Rails handles this via savepoints so it's not wrong, but the inner transaction block is unnecessary noise and could be removed.

Debug log leaks balance amount

Rails.logger.debug("[AnchorRotation] ... amount=#{entry.amount}")

debug is off in production by default, but if ever enabled this emits raw financial values into logs. The info log above safely uses has_amount: true/false instead — consider doing the same for the debug line, or dropping it since the info log already covers the rotation event.

Misplaced comment indentation
The # Return value unused; side effects (reload + memoization clear) are what matter comment sits between preserve_anchor_as_reconciliation_if_stale and create_current_anchor at an odd indentation level — it reads as applying to create_current_anchor but seems to describe preserve_anchor_as_reconciliation_if_stale. Minor but confusing for a future reader.

The waypoint concept and the two-level tests (rotation + reverse calculator) are well-designed and the comprehensive test coverage is appreciated.


Generated by Claude Code

@CrossDrain
Copy link
Copy Markdown
Contributor Author

I've pushed updates to address Claude's feedback: (1dbcc0b)

  • Debug log leaking balance: I've dropped the debug log completely. Including the raw amount was initially intentional to make local troubleshooting easier, but I completely understand the privacy concern and agree that the info log is sufficient.
  • Misplaced comment: That was just an oopsie on my part! I've removed the comment entirely, as both methods already clearly document their side effects inline just before they return, so the extra comment wasn't even needed.
  • Redundant transaction: I've removed the inner ActiveRecord::Base.transaction block as requested.

Regarding the existing user-created reconciliations on linked accounts—the UI actually hides the "Update balance" button for linked accounts, so users can't actively create manual reconciliations for them. That said, what do you think about accounts that were originally created as manual accounts (and had manual reconciliations added) but were later connected to an institution? In that scenario, treating those historical manual valuations as hard-reset waypoints seems like the right call since they represent confirmed, user-verified balances from before the account was linked. Does that perspective make sense to you, or do you think we should handle those specific edge cases differently?

@jjmata jjmata removed the contributor:flagged Contributor flagged for review by trust analysis. label May 4, 2026
@brin-security-scanner brin-security-scanner Bot added the contributor:flagged Contributor flagged for review by trust analysis. label May 4, 2026
@jjmata jjmata removed the contributor:flagged Contributor flagged for review by trust analysis. label May 4, 2026
@jjmata jjmata requested a review from sokie May 4, 2026 22:59
@jjmata
Copy link
Copy Markdown
Collaborator

jjmata commented May 4, 2026

Can you review, @sokie? 🙏

Copy link
Copy Markdown
Collaborator

jjmata commented May 5, 2026

Following up on CrossDrain's open question about accounts that migrated from manual to linked: the reasoning is solid. A reconciliation valuation on a previously-manual account represents a user-confirmed balance snapshot, and treating it as a hard-reset waypoint is exactly the right semantic. The only inherent limitation is that if the user originally entered an incorrect value, the waypoint will anchor the error — but that's not addressable at this layer, and the UI already allows editing or deleting reconciliation entries if that happens.

One additional test scenario worth adding to increase confidence: multiple accumulated waypoints (e.g., 3–5 consecutive daily syncs creating a chain of reconciliation waypoints). The current tests cover the single-waypoint case in the reverse calculator. A multi-waypoint fixture would confirm that the calculator correctly threads through each waypoint in sequence — resetting independently at each one — without unexpected interactions near the opening_anchor boundary or within the derive_cash_balance_on_date_from_total path. The logic looks correct for this case given how the elsif branch works in isolation, but a chain of waypoints is the steady-state production scenario after the feature has been running for weeks.

Minor: preserve_anchor_as_reconciliation_if_stale calls account.valuations.reload and then immediately sets @current_anchor_valuation = nil. The reload forces a full fetch of all valuations on the account (potentially many records for long-lived accounts). Since current_anchor_valuation queries via a scoped account.valuations.current_anchor.includes(:entry).first call, whether the full-collection reload is actually needed depends on whether the association proxy's cache would otherwise return stale in-memory records for the chained scope. Worth verifying — if @current_anchor_valuation = nil alone is sufficient (because the chained scope bypasses the loaded collection cache), the reload is unnecessary overhead per sync.


Generated by Claude Code

…ceManager and add regression test for consecutive reconciliation waypoints
@brin-security-scanner brin-security-scanner Bot added the contributor:flagged Contributor flagged for review by trust analysis. label May 5, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
app/models/account/current_balance_manager.rb (2)

134-134: 💤 Low value

has_amount flag adds no signal — consider dropping it.

entry.amount is a Numeric/BigDecimal, and Rails' Numeric#present? is true for any value including 0. Since a persisted valuation entry effectively always has a non-nil amount, has_amount will be true in virtually every log line and conveys no diagnostic value. For traceability, account.id + date are already sufficient; if you want a stable join key for ops, prefer entry_id/valuation_id.

Proposed simplification
-      Rails.logger.info("[AnchorRotation] Converted current_anchor to reconciliation for account #{account.id}, date=#{entry.date}, has_amount=#{entry.amount.present?}")
+      Rails.logger.info("[AnchorRotation] Converted current_anchor to reconciliation for account #{account.id}, entry_id=#{entry.id}, date=#{entry.date}")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/account/current_balance_manager.rb` at line 134, The log line in
current_balance_manager.rb that calls Rails.logger.info with "[AnchorRotation]
Converted current_anchor to reconciliation for account #{account.id},
date=#{entry.date}, has_amount=#{entry.amount.present?}" includes a redundant
has_amount flag because entry.amount.present? is always true for persisted
numeric/BigDecimal values; remove the has_amount fragment or replace it with a
stable identifier like entry.id (or valuation_id) for traceability. Locate the
Rails.logger.info call (in the CurrentBalanceManager / anchor rotation
conversion code) and update the message to omit has_amount or append
"entry_id=#{entry.id}" (or "valuation_id=...") instead, keeping account.id and
entry.date intact.

155-176: 💤 Low value

Inner ActiveRecord::Base.transaction is now redundant.

When update_current_anchor is called from set_current_balance_for_linked_account (Line 109), it already runs inside the outer transaction opened on Line 102, so the inner block creates an unnecessary savepoint. This mirrors the inner-transaction concern that was already addressed in preserve_anchor_as_reconciliation_if_stale. Since this method only mutates a single entry, the outer transaction is sufficient.

Proposed simplification
     def update_current_anchor(balance)
       changes_made = false

-      ActiveRecord::Base.transaction do
-        # Update associated entry attributes
-        entry = current_anchor_valuation.entry
+      # Update associated entry attributes
+      entry = current_anchor_valuation.entry

-        if entry.amount != balance
-          entry.amount = balance
-          changes_made = true
-        end
+      if entry.amount != balance
+        entry.amount = balance
+        changes_made = true
+      end

-        if entry.date != Date.current
-          entry.date = Date.current
-          changes_made = true
-        end
+      if entry.date != Date.current
+        entry.date = Date.current
+        changes_made = true
+      end

-        entry.save! if entry.changed?
-      end
+      entry.save! if entry.changed?

       changes_made
     end

Note: if update_current_anchor is ever called outside the linked-account flow, the outer transaction still applies because set_current_balance is the only public entry point and it always wraps via the linked path; otherwise the single entry.save! is itself atomic.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/account/current_balance_manager.rb` around lines 155 - 176, Remove
the redundant inner ActiveRecord::Base.transaction block inside
update_current_anchor: delete the transaction do/end wrapper and leave the
existing logic that updates entry (via current_anchor_valuation.entry), sets
entry.amount and entry.date as needed, and calls entry.save! if entry.changed?;
this relies on the outer transaction started by
set_current_balance_for_linked_account and mirrors the earlier change made in
preserve_anchor_as_reconciliation_if_stale to avoid unnecessary savepoints.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@app/models/account/current_balance_manager.rb`:
- Line 134: The log line in current_balance_manager.rb that calls
Rails.logger.info with "[AnchorRotation] Converted current_anchor to
reconciliation for account #{account.id}, date=#{entry.date},
has_amount=#{entry.amount.present?}" includes a redundant has_amount flag
because entry.amount.present? is always true for persisted numeric/BigDecimal
values; remove the has_amount fragment or replace it with a stable identifier
like entry.id (or valuation_id) for traceability. Locate the Rails.logger.info
call (in the CurrentBalanceManager / anchor rotation conversion code) and update
the message to omit has_amount or append "entry_id=#{entry.id}" (or
"valuation_id=...") instead, keeping account.id and entry.date intact.
- Around line 155-176: Remove the redundant inner ActiveRecord::Base.transaction
block inside update_current_anchor: delete the transaction do/end wrapper and
leave the existing logic that updates entry (via
current_anchor_valuation.entry), sets entry.amount and entry.date as needed, and
calls entry.save! if entry.changed?; this relies on the outer transaction
started by set_current_balance_for_linked_account and mirrors the earlier change
made in preserve_anchor_as_reconciliation_if_stale to avoid unnecessary
savepoints.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ee06f244-070f-4bd1-a6ce-eb3ec4860777

📥 Commits

Reviewing files that changed from the base of the PR and between 1dbcc0b and 57c9f9f.

📒 Files selected for processing (2)
  • app/models/account/current_balance_manager.rb
  • test/models/balance/reverse_calculator_test.rb

@CrossDrain
Copy link
Copy Markdown
Contributor Author

All the points have been addressed in the last commit (57c9f9f):

  1. Removed both valuations.reload calls (in preserve_anchor_as_reconciliation_if_stale and create_current_anchor) in CurrentBalanceManager (app/models/account/current_balance_manager.rb), eliminating a full-table fetch of all valuations per sync to avoid unnecessary database queries.

  2. Added a regression test for consecutive reconciliation waypoints in test/models/balance/reverse_calculator_test.rb to ensure the reverse calculator handles sequential waypoints correctly. It's a chain of 4 consecutive daily reconciliation waypoints with non-monotonic balance values (up/down/up/down), plus transactions in the gap before the opening_anchor. This covers the steady-state production scenario after several days of syncs and confirms that each waypoint resets independently via the elsif branch without carry-forward leaking between them or unexpected interactions at the opening_anchor boundary.

@jjmata jjmata removed the contributor:flagged Contributor flagged for review by trust analysis. label May 6, 2026
@brin-security-scanner brin-security-scanner Bot added the contributor:verified Contributor passed trust analysis. label May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis.

Development

Successfully merging this pull request may close these issues.

Persist daily balance snapshots for linked accounts (SnapTrade, Plaid) Bug: investment balance history is zero

2 participants