feat: add incremental balance sync windows after provider imports#1880
feat: add incremental balance sync windows after provider imports#1880claytonlin1110 wants to merge 4 commits into
Conversation
superagent-security
Bot
added
contributor:verified
|
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 (5)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR centralizes per-account balance sync scheduling into a new concern, adds Account::BalanceSyncWindow to derive incremental windows (7‑day lookback + touched-entry signals), enables bounded incremental reverse recalculation with fallback detection, and adapts Materializer purge logic. Several provider item models adopt the new concern and small private hooks. ChangesIncremental balance sync support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfc51a48ee
ℹ️ 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".
| if prior | ||
| Rails.logger.info("Incremental reverse sync from #{@window_start_date}, preserving balances before #{@window_start_date}") | ||
| @fell_back = false | ||
| return [ @window_start_date, account.opening_anchor_date ].max |
There was a problem hiding this comment.
When window_start_date is present, this branch enables incremental reverse calculation solely based on the existence of prior_balance, but the reverse algorithm never seeds from that prior balance or validates the boundary. In reverse mode, days before window_start_date depend on the recomputed balance at window_start_date, so any changed flow/valuation on or after the window can require updates to window_start_date - 1 and earlier. Because the materializer now preserves pre-window rows in incremental mode, linked accounts can keep stale historical balances after provider imports.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (1)
app/models/coinstats_item.rb (1)
75-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale documentation from removed method.
These YARD comments (
@param parent_sync,@param window_start_date,@param window_end_date,@return [Array<Hash>]) describe the removedschedule_account_syncsmethod, not theupsert_coinstats_snapshot!method below them.🧹 Proposed fix to remove stale comments
- # Queues balance sync jobs for all visible accounts. - # `@param` parent_sync [Sync, nil] Parent sync for tracking - # `@param` window_start_date [Date, nil] Start of sync window - # `@param` window_end_date [Date, nil] End of sync window - # `@return` [Array<Hash>] Results with success status per account # Persists raw API response for debugging and reprocessing. # `@param` accounts_snapshot [Hash] Raw API response data def upsert_coinstats_snapshot!(accounts_snapshot)🤖 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/coinstats_item.rb` around lines 75 - 79, Remove the stale YARD param/return comments that refer to the deleted schedule_account_syncs method and update the documentation above upsert_coinstats_snapshot! so it only documents that method; specifically, delete the lines mentioning `@param` parent_sync, `@param` window_start_date, `@param` window_end_date and `@return` [Array<Hash>] and ensure any remaining comment text matches upsert_coinstats_snapshot!'s purpose and parameters (if any).
🤖 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.
Inline comments:
In `@app/models/account/balance_sync_window.rb`:
- Line 16: The current early return of parent_window_start_date.to_date bypasses
applying the account-level opening-anchor floor and allows parent windows
earlier than allowed; change the logic so that when parent_window_start_date is
present you convert it to a date, then compare it with opening_anchor_floor and
return the later date (e.g., compute start = parent_window_start_date.to_date
and return [start, opening_anchor_floor].max) so the opening-anchor floor is
always enforced; modify the method containing parent_window_start_date and
opening_anchor_floor references accordingly.
---
Outside diff comments:
In `@app/models/coinstats_item.rb`:
- Around line 75-79: Remove the stale YARD param/return comments that refer to
the deleted schedule_account_syncs method and update the documentation above
upsert_coinstats_snapshot! so it only documents that method; specifically,
delete the lines mentioning `@param` parent_sync, `@param` window_start_date, `@param`
window_end_date and `@return` [Array<Hash>] and ensure any remaining comment text
matches upsert_coinstats_snapshot!'s purpose and parameters (if any).
🪄 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: 536c28ef-5d62-438b-83d8-2740806ce7b4
📒 Files selected for processing (21)
app/models/account/balance_sync_window.rbapp/models/account/schedules_balance_syncs.rbapp/models/balance/materializer.rbapp/models/balance/reverse_calculator.rbapp/models/binance_item.rbapp/models/brex_item.rbapp/models/coinbase_item.rbapp/models/coinstats_item.rbapp/models/enable_banking_item.rbapp/models/ibkr_item.rbapp/models/indexa_capital_item.rbapp/models/kraken_item.rbapp/models/lunchflow_item.rbapp/models/mercury_item.rbapp/models/plaid_item.rbapp/models/simplefin_item.rbapp/models/snaptrade_item.rbapp/models/sophtron_item.rbtest/models/account/balance_sync_window_test.rbtest/models/account/schedules_balance_syncs_test.rbtest/models/balance/reverse_calculator_test.rb
| # @param last_synced_at [Time, nil] Provider item freshness timestamp | ||
| # @return [Date, nil] nil means full balance recalculation | ||
| def for_account(account, parent_sync: nil, parent_window_start_date: nil, import_window_start_date: nil, last_synced_at: nil) | ||
| return parent_window_start_date.to_date if parent_window_start_date.present? |
There was a problem hiding this comment.
Apply opening-anchor floor even for explicit parent windows.
Returning early here bypasses the floor in Lines 26-27, so an explicit parent window can go earlier than the account’s valid floor. That breaks the incremental-window contract and can trigger unnecessary backfill recalculation.
Proposed fix
- return parent_window_start_date.to_date if parent_window_start_date.present?
+ candidates = []
+ candidates << parent_window_start_date.to_date if parent_window_start_date.present?
- candidates = []
candidates << import_window_start_date.to_date if import_window_start_date.present?
candidates << entries_touched_since(parent_sync, account) if parent_sync
candidates << (last_synced_at.to_date - LOOKBACK) if last_synced_at.present?📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return parent_window_start_date.to_date if parent_window_start_date.present? | |
| candidates = [] | |
| candidates << parent_window_start_date.to_date if parent_window_start_date.present? | |
| candidates << import_window_start_date.to_date if import_window_start_date.present? | |
| candidates << entries_touched_since(parent_sync, account) if parent_sync | |
| candidates << (last_synced_at.to_date - LOOKBACK) if last_synced_at.present? |
🤖 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/balance_sync_window.rb` at line 16, The current early
return of parent_window_start_date.to_date bypasses applying the account-level
opening-anchor floor and allows parent windows earlier than allowed; change the
logic so that when parent_window_start_date is present you convert it to a date,
then compare it with opening_anchor_floor and return the later date (e.g.,
compute start = parent_window_start_date.to_date and return [start,
opening_anchor_floor].max) so the opening-anchor floor is always enforced;
modify the method containing parent_window_start_date and opening_anchor_floor
references accordingly.
|
@jjmata what do you think about this improve? |
|
Large refactor with a good motivation. A few concerns worth addressing before merge: 1. Behavior change for 2. Orphaned comment in # @param window_start_date [Date, nil] Start of sync window
# @param window_end_date [Date, nil] End of sync window
# @return [Array<Hash>] Results with success status per account
# Persists raw API response...That comment now documents nothing — it should be removed. 3. 4. Generated by Claude Code |
|
@jjmata updated addressed issues. |
2 participants
Summary
Account::BalanceSyncWindowto resolve an effectivewindow_start_dateper account after provider imports (explicit parent window, import window, entries touched during the sync, orlast_synced_at - 7 days, floored atopening_anchor_date).Account::SchedulesBalanceSyncsand wire all provider items through it so balance syncs receive the derived window instead of always passingnil.Balance::ReverseCalculatorandBalance::Materializerso linked accounts only recalculate balances from the window onward when a prior persisted balance exists.Why
Daily provider syncs were recomputing full balance history on every run even when imports only changed a few days of transactions. This reduces CPU/DB work on self-hosted and managed instances without UI changes.
Test plan
bin/rails test test/models/account/balance_sync_window_test.rbbin/rails test test/models/account/schedules_balance_syncs_test.rbbin/rails test test/models/balance/reverse_calculator_test.rbbin/rails test test/models/balance/materializer_test.rbwindow_start_datewhenlast_synced_atis set or entries were updated during the importSummary by CodeRabbit
New Features
Improvements
Tests