Add overpayment detection for SimpleFIN liabilities (default ON) with heuristic-based classification and robust fallbacks #412
Conversation
WalkthroughAdds liability-aware balance normalization across SimpleFIN importer and processor, introduces a new OverpaymentAnalyzer that classifies liability balances using recent transactions with caching and fallbacks, extracts numeric helpers into a concern, and adds tests plus empty fixtures for the analyzer and processor scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Import as Importer/Processor
participant Analyzer as OverpaymentAnalyzer
participant DB as Entries DB
participant Payload as Provider Payload
participant Cache as Rails.cache
Import->>Analyzer: call(observed_balance)
alt analyzer enabled
Analyzer->>DB: query recent Entries (amounts, dates)
alt entries present
DB-->>Analyzer: entries
else no entries
Analyzer->>Payload: read raw_transactions_payload
Payload-->>Analyzer: provider transactions
end
Analyzer->>Analyzer: compute metrics & classification
Analyzer->>Cache: read/write sticky classification
Analyzer-->>Import: Result(classification, reason, metrics)
else disabled or error
Analyzer-->>Import: Result(:unknown)
end
Import->>Import: apply normalization (liability/investment/fallback)
Import->>DB: adapter.update_balance / persist normalized values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/simplefin_account/processor.rb (1)
40-62: Fix nilbalanceusage and align process_account! with SimpleFIN liability heuristic
process_account!currently usesbalancewithout ever initializing it, which directly explains both pipeline failures:
- Line 49:
balance = -balanceraisesNoMethodError: undefined method '-@' for nil.- Line 60:
account.update!(balance: balance, ...)hitsValidation failed: Balance can't be blank.On top of that, this method still uses the old “always invert for liabilities” rule, while the importer now has a dual-field heuristic using both
balanceandavailable-balance. That means overpayment edge cases remain incorrect on the full processing path.You can fix all three issues (nil bug, validation error, and heuristic mismatch) by computing
balancelocally with the same dual-field logic already used inSimplefinItem::Importer#import_account_minimal_and_balance, leveraging the new helpers:- # Update account balance and cash balance from latest SimpleFin data - account = simplefin_account.current_account - - # Normalize balances for liabilities (SimpleFIN typically uses opposite sign) - # App convention: - # - Liabilities: positive => you owe; negative => provider owes you (overpayment/credit) - # Since providers often send the opposite sign, ALWAYS invert for liabilities so - # that both debt and overpayment cases are represented correctly. - if [ "CreditCard", "Loan" ].include?(account.accountable_type) - balance = -balance - end + # Update account balance and cash balance from latest SimpleFin data + account = simplefin_account.current_account + + # Normalize balances for liabilities using both current_balance and available_balance. + # App convention: + # - Liabilities: positive => you owe; negative => provider owes you (overpayment/credit) + # Many SimpleFIN institutions send opposite or inconsistent signs across these fields, + # so use a dual-field heuristic to map both debt and overpayment correctly. + bal = to_decimal(simplefin_account.current_balance) + avail = to_decimal(simplefin_account.available_balance) + observed = bal.nonzero? ? bal : avail + + balance = observed + if [ "CreditCard", "Loan" ].include?(account.accountable_type) + both_present = bal.nonzero? && avail.nonzero? + if both_present && same_sign?(bal, avail) + if bal.positive? && avail.positive? + # Both positive → upstream overpayment/credit → represent as negative + balance = -observed.abs + elsif bal.negative? && avail.negative? + # Both negative → upstream debt already negative → represent as positive + balance = observed.abs + end + else + # Mixed signs or only one side present → simple invert to respect liability convention + balance = -observed + end + end @@ - # Calculate cash balance correctly for investment accounts - cash_balance = if account.accountable_type == "Investment" + # Calculate cash balance correctly for investment accounts + cash_balance = if account.accountable_type == "Investment" calculator = SimplefinAccount::Investments::BalanceCalculator.new(simplefin_account) calculator.cash_balance else balance endThis ensures
balanceis always a concreteBigDecimal(never nil), matches the importer’s normalization behavior for SimpleFIN liabilities, and keeps investmentcash_balancebehavior unchanged.
🧹 Nitpick comments (3)
app/models/simplefin_account/processor.rb (1)
101-115: Wire up or centralize numeric helpers to avoid dead code and driftThe new helpers:
def to_decimal(value) ... end def same_sign?(a, b) ... endare currently unused in this class, while identical definitions exist (and are used) in
SimplefinItem::Importer. That creates two problems:
- They don’t help fix the current
process_account!bug unless actually used.- Duplication across classes risks subtle drift in normalization behavior over time.
Once you wire these helpers into
process_account!(as suggested above), consider extracting them into a small shared SimpleFIN-focused module (e.g.,Simplefin::NumericHelpers) and including it in bothSimplefinAccount::ProcessorandSimplefinItem::Importer, so the normalization logic stays in sync.app/models/simplefin_item/importer.rb (2)
109-151: Heuristic looks solid; consider adding mismatch logging and simplifyingcashThe liability normalization here is well-structured:
- Converts both balance fields to
BigDecimalwithto_decimal.- Uses linked
acct.accountable_typeORSimplefin::AccountTypeMapper.inferto decide liability status.- Applies a dual-field sign heuristic so:
- Mixed-sign balances (e.g., negative
balance, positiveavailable-balance) invert to positive debt.- Both-positive cases invert to negative (overpayment/credit).
- Both-negative cases become positive (debt).
Two small follow-ups you might want to consider:
Surface mapper vs linked type disagreements
Right now,
is_liabilityis simplyis_linked_liability || is_mapper_liability. When the mapper infersCreditCard/Loanbutacct.accountable_typeis something else (or vice versa), that’s useful observability signal for debugging mislinked accounts.You could optionally log or register a hint when they disagree, e.g.:
is_mapper_liability = inferred && [ "CreditCard", "Loan" ].include?(inferred.accountable_type)is_liability = is_linked_liability || is_mapper_liability
mapper_type = inferred&.accountable_typeis_mapper_liability = [ "CreditCard", "Loan" ].include?(mapper_type)is_liability = is_linked_liability || is_mapper_liabilityif is_liability && is_linked_liability != is_mapper_liabilityRails.logger.info("SimpleFin liability type mismatch for acct=#{acct.id}: " \"linked=#{acct.accountable_type.inspect} inferred=#{mapper_type.inspect}")end(Or feed this into `register_error`/stats if you prefer UI surfacing.)
Simplify
cashassignmentThe
cashbranch currently returnsnormalizedin both cases:cash = if acct.accountable_type == "Investment" # Leave investment cash to investment calculators in full run normalized else normalized endSince both branches are identical, this can safely collapse to:
cash = normalizedand, if the comment is still relevant for future behavior, move it above that line.
768-781: Deduplicate numeric helpers shared with SimplefinAccount::ProcessorThe helpers here:
def to_decimal(value) ... end def same_sign?(a, b) ... endare identical to the ones defined in
SimplefinAccount::Processor. Keeping two separate copies increases the chance of subtle divergence in SimpleFIN normalization behavior later.Since both importer and processor now rely on the same balance/sign semantics, consider extracting these into a small shared module under the SimpleFIN namespace (e.g.,
Simplefin::NumericHelpersorSimplefin::BalanceUtils) and including it in both classes, rather than redefining them.Based on learnings, cross-integration balance handling (e.g., Plaid and EnableBanking processors) is kept consistent via shared patterns, so centralizing these SimpleFIN helpers would follow the same spirit without touching other providers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/simplefin_account/processor.rb(1 hunks)app/models/simplefin_item/importer.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rb: Application supports two modes: 'managed' and 'self_hosted' via Rails.application.config.app_mode
Use Current.user and Current.family instead of current_user / current_family for authentication context
Optimize database queries with proper indexes to prevent N+1 queries using includes/joins
**/*.rb: Use Rails pluralization in i18n:t("transactions.count", count: @transactions.count).
Runbin/rubocop -f github -afor Ruby linting with auto-correct before pull requests.
**/*.rb: Ruby code should use 2-space indent,snake_casefor methods and variables, andCamelCasefor classes and modules, following Rails conventions for folders and file names
Prefer environment variables over hard-coded values for configuration
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/models/**/*.rb: Keep business logic in models using skinny controllers and fat models pattern
Store all monetary values in base currency (user's primary currency) and use Money objects for conversion
Use ActiveRecord validations for form convenience and complex business logic validations
Use Rails concerns and POROs for organizing business logic instead of services folder
app/models/**/*.rb: All monetary values stored in base currency (user's primary currency). UseMoneyobjects for currency conversion and formatting.
Place business logic inapp/models/folder. Avoid creatingapp/services/folder. Use Rails concerns and POROs for organization.
Models should answer questions about themselves using instance methods: useaccount.balance_seriesnotAccountSeries.new(account).call.
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
**/*.{rb,erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{rb,erb,html.erb}: UseCurrent.userfor the current user context in controllers and views. Do NOT usecurrent_user.
UseCurrent.familyfor the current family context. Do NOT usecurrent_family.
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
**/*.{erb,html.erb,rb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html.erb,rb}: All user-facing strings must use localization (i18n) via thet()helper. Update locale files for each new or changed element.
Use server-side formatting for currencies, numbers, and dates. Do not format in JavaScript.
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
{app/models/**/*.rb,db/migrate/**/*.rb}
📄 CodeRabbit inference engine (CLAUDE.md)
Simple validations (null checks, unique indexes) should be in the database. ActiveRecord validations for form convenience (prefer client-side when possible). Complex validations and business logic in ActiveRecord.
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb}: UseCurrent.userfor accessing the current user. Do NOT usecurrent_user
UseCurrent.familyfor accessing the current family. Do NOT usecurrent_family
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
**/*.{rb,js,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
**/*.{rb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use ActiveRecord validations for complex validations and business logic. Simple validations may be mirrored in ActiveRecord for form error handling convenience, but prioritize client-side form validation when possible
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
app/**/*.{rb,js,ts,jsx,tsx,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Code should be organized in
app/directory (Rails MVC, services, jobs, mailers, components), with JS inapp/javascript/, and styles/assets inapp/assets/(Tailwind, images, fonts)
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Run
bin/brakemansecurity scan before major PRs to check for static analysis of common Rails issues
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
🧠 Learnings (9)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account sync must perform transfer auto-matching, calculate daily balance records from start_date to Date.current, calculate holdings, and optionally enrich transaction data
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
📚 Learning: 2025-11-04T16:21:39.347Z
Learnt from: sokie
Repo: we-promise/sure PR: 276
File: app/controllers/reports_controller.rb:80-84
Timestamp: 2025-11-04T16:21:39.347Z
Learning: The we-promise/sure codebase uses a custom Money class (lib/money.rb) that treats numeric inputs as dollar amounts, not fractional units (cents). When Money.new receives a numeric value, it converts it directly to BigDecimal via BigDecimal(obj.to_s) without multiplication. This differs from the standard ruby-money gem which expects cents/fractional units.
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
📚 Learning: 2025-11-24T16:55:43.069Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.069Z
Learning: Applies to app/models/**/*.rb : All monetary values stored in base currency (user's primary currency). Use `Money` objects for currency conversion and formatting.
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/models/**/*.rb : Store all monetary values in base currency (user's primary currency) and use Money objects for conversion
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
📚 Learning: 2025-11-24T16:55:43.069Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.069Z
Learning: Applies to app/models/**/*.rb : Models should answer questions about themselves using instance methods: use `account.balance_series` not `AccountSeries.new(account).call`.
Applied to files:
app/models/simplefin_account/processor.rb
🧬 Code graph analysis (2)
app/models/simplefin_item/importer.rb (3)
app/models/simplefin_account/processor.rb (2)
to_decimal(102-111)same_sign?(113-115)app/models/simplefin/account_type_mapper.rb (1)
infer(37-88)app/models/account/provider_import_adapter.rb (1)
update_balance(110-115)
app/models/simplefin_account/processor.rb (3)
app/models/simplefin_item/importer.rb (2)
to_decimal(768-777)same_sign?(779-781)app/models/provider/configurable.rb (1)
value(209-223)lib/money/arithmetic.rb (2)
positive?(53-55)negative?(49-51)
🪛 GitHub Actions: Pull Request
app/models/simplefin_account/processor.rb
[error] 49-49: NoMethodError: undefined method '-@' for nil in SimplefinAccount::Processor#process_account!
[error] 60-60: ActiveRecord::RecordInvalid: Validation failed: Balance can't be blank in SimplefinAccount::Processor#process_account!
|
How do you feel about this one @luckyPipewrench? Do we still need it? I never heard back from SimpleFIN support about my BofA overpayment issue. |
|
Working on this one today. Should have it done soon, I've finally kept a overpaid balance long enough for it show on the simplefin sync. |
- Updated `SimplefinAccount::Processor` to normalize liability balances based on observed values, ensuring correct handling of debts and overpayments. - Enhanced `SimplefinItem::Importer` to apply similar normalization rules during imports, improving consistency. - Added multiple test cases in `SimplefinAccountProcessorTest` to validate edge cases for liabilities and mixed-sign scenarios. - Introduced helper methods (`to_decimal`, `same_sign?`) to simplify numeric operations in normalization logic.
…fication - Introduced `SimplefinAccount::Liabilities::OverpaymentAnalyzer` to classify liability balances as credit, debt, or unknown using transaction history. - Updated `SimplefinAccount::Processor` and `SimplefinItem::Importer` to integrate heuristic-based balance normalization with fallback logic for ambiguous cases. - Added comprehensive unit tests in `OverpaymentAnalyzerTest` to validate classification logic and edge cases. - Enhanced logging and observability around classification results and fallback scenarios.
luckyPipewrench
force-pushed
the
simplefin-liabilities-normalization
branch
from
4a5e032 to
a1a6d4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/models/simplefin_item/importer.rb (2)
108-189: Redundantcashbranch (no-op) — simplify or implement intended investment behavior.
cashreturnsnormalizedin both branches, so theInvestmentconditional currently adds no value.- cash = if acct.accountable_type == "Investment" - # Leave investment cash to investment calculators in full run - normalized - else - normalized - end + cash = normalized
860-874: De-duplicate numeric helpers across importer/processor/analyzer.
to_decimal/same_sign?are now repeated in multiple classes; consider extracting to a small shared module (e.g.,Simplefin::DecimalHelpers) to keep behavior consistent.test/models/simplefin_account/liabilities/overpayment_analyzer_test.rb (1)
27-34: Prefer deleting the exact cache key vsdelete_matched.
More portable across cache stores and less work.- Rails.cache.delete_matched("simplefin:sfa:#{@sfa.id}:liability_sign_hint") rescue nil + Rails.cache.delete("simplefin:sfa:#{@sfa.id}:liability_sign_hint") rescue nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb(1 hunks)app/models/simplefin_account/processor.rb(2 hunks)app/models/simplefin_item/importer.rb(2 hunks)test/models/simplefin_account/liabilities/overpayment_analyzer_test.rb(1 hunks)test/models/simplefin_account_processor_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rb: Application supports two modes: 'managed' and 'self_hosted' via Rails.application.config.app_mode
Use Current.user and Current.family instead of current_user / current_family for authentication context
Optimize database queries with proper indexes to prevent N+1 queries using includes/joins
**/*.rb: Use Rails pluralization in i18n:t("transactions.count", count: @transactions.count).
Runbin/rubocop -f github -afor Ruby linting with auto-correct before pull requests.
**/*.rb: Ruby code should use 2-space indent,snake_casefor methods and variables, andCamelCasefor classes and modules, following Rails conventions for folders and file names
Prefer environment variables over hard-coded values for configuration
Files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbtest/models/simplefin_account_processor_test.rb
test/**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
test/**/*.rb: Use Minitest with fixtures (never RSpec or FactoryBot) for testing
Use mocha gem for mocks and VCR for external API tests
Use fixtures for base cases in tests (2-3 per model) and create edge cases on-the-fly within test context
Test critical code paths only: test Commands for correct params, Queries for output
test/**/*.rb: Use Rails' built-in Minitest framework with fixtures for test data. Avoid RSpec and FactoryBot.
Use VCR for external API testing.
Usemochagem for stubs and mocks. PreferOpenStructfor mock instances. Only mock what's necessary.Write tests using Minitest and fixtures as the testing framework
test/**/*.rb: Always use Minitest and fixtures for testing, NEVER rspec or factories
For tests that require a large number of fixture records, use Rails helpers to help create the records needed for the test, then inline the creation instead of defining them in fixtures
Distinguish between commands and query methods. Test output of query methods; test that commands were called with the correct params.
Never test the implementation details of one class in another class's test suite
Use themochagem for stubs and mocks
Always preferOpenStructwhen creating mock instances, or in complex cases, a mock class
Only mock what's necessary. If you're not testing return values, don't mock a return value.
Files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbtest/models/simplefin_account_processor_test.rb
**/*.{rb,erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{rb,erb,html.erb}: UseCurrent.userfor the current user context in controllers and views. Do NOT usecurrent_user.
UseCurrent.familyfor the current family context. Do NOT usecurrent_family.
Files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbtest/models/simplefin_account_processor_test.rb
**/*.{erb,html.erb,rb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html.erb,rb}: All user-facing strings must use localization (i18n) via thet()helper. Update locale files for each new or changed element.
Use server-side formatting for currencies, numbers, and dates. Do not format in JavaScript.
Files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbtest/models/simplefin_account_processor_test.rb
**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb}: UseCurrent.userfor accessing the current user. Do NOT usecurrent_user
UseCurrent.familyfor accessing the current family. Do NOT usecurrent_family
Files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbtest/models/simplefin_account_processor_test.rb
**/*.{rb,js,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbtest/models/simplefin_account_processor_test.rb
**/*.{rb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use ActiveRecord validations for complex validations and business logic. Simple validations may be mirrored in ActiveRecord for form error handling convenience, but prioritize client-side form validation when possible
Files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbtest/models/simplefin_account_processor_test.rb
test/**/*_test.rb
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*_test.rb: Test files should use*_test.rbnaming convention and mirrorapp/structure (e.g.,test/models/*_test.rb)
Use Minitest framework for tests with names*_test.rb, run viabin/rails test, and usetest/fixturesand VCR cassettes for HTTP testing
Files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbtest/models/simplefin_account_processor_test.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/models/**/*.rb: Keep business logic in models using skinny controllers and fat models pattern
Store all monetary values in base currency (user's primary currency) and use Money objects for conversion
Use ActiveRecord validations for form convenience and complex business logic validations
Use Rails concerns and POROs for organizing business logic instead of services folder
app/models/**/*.rb: All monetary values stored in base currency (user's primary currency). UseMoneyobjects for currency conversion and formatting.
Place business logic inapp/models/folder. Avoid creatingapp/services/folder. Use Rails concerns and POROs for organization.
Models should answer questions about themselves using instance methods: useaccount.balance_seriesnotAccountSeries.new(account).call.
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
{app/models/**/*.rb,db/migrate/**/*.rb}
📄 CodeRabbit inference engine (CLAUDE.md)
Simple validations (null checks, unique indexes) should be in the database. ActiveRecord validations for form convenience (prefer client-side when possible). Complex validations and business logic in ActiveRecord.
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
app/**/*.{rb,js,ts,jsx,tsx,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Code should be organized in
app/directory (Rails MVC, services, jobs, mailers, components), with JS inapp/javascript/, and styles/assets inapp/assets/(Tailwind, images, fonts)
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Run
bin/brakemansecurity scan before major PRs to check for static analysis of common Rails issues
Files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
🧠 Learnings (11)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to test/**/*.rb : Use fixtures for base cases in tests (2-3 per model) and create edge cases on-the-fly within test context
Applied to files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rb
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account model is a Rails delegated type with subtypes: Depository, Investment, Crypto, Property, Vehicle, OtherAsset (assets) and CreditCard, Loan, OtherLiability (liabilities)
Applied to files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbapp/models/simplefin_account/processor.rbtest/models/simplefin_account_processor_test.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbtest/models/simplefin_account_processor_test.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbtest/models/simplefin_account_processor_test.rb
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account sync must perform transfer auto-matching, calculate daily balance records from start_date to Date.current, calculate holdings, and optionally enrich transaction data
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbtest/models/simplefin_account_processor_test.rb
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rbtest/models/simplefin_account_processor_test.rb
📚 Learning: 2025-11-24T16:55:43.069Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.069Z
Learning: Applies to app/models/**/*.rb : All monetary values stored in base currency (user's primary currency). Use `Money` objects for currency conversion and formatting.
Applied to files:
app/models/simplefin_item/importer.rb
📚 Learning: 2025-11-04T16:21:39.347Z
Learnt from: sokie
Repo: we-promise/sure PR: 276
File: app/controllers/reports_controller.rb:80-84
Timestamp: 2025-11-04T16:21:39.347Z
Learning: The we-promise/sure codebase uses a custom Money class (lib/money.rb) that treats numeric inputs as dollar amounts, not fractional units (cents). When Money.new receives a numeric value, it converts it directly to BigDecimal via BigDecimal(obj.to_s) without multiplication. This differs from the standard ruby-money gem which expects cents/fractional units.
Applied to files:
app/models/simplefin_item/importer.rbapp/models/simplefin_account/processor.rb
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/models/**/*.rb : Store all monetary values in base currency (user's primary currency) and use Money objects for conversion
Applied to files:
app/models/simplefin_item/importer.rb
📚 Learning: 2025-11-04T15:16:23.341Z
Learnt from: sokie
Repo: we-promise/sure PR: 276
File: app/controllers/reports_controller.rb:159-169
Timestamp: 2025-11-04T15:16:23.341Z
Learning: In the Budget model, the methods `actual_spending` and `allocated_spending` return BigDecimal values (not Money objects), so division operations like `budget.actual_spending / budget.allocated_spending` work correctly without needing conversion to fractional units.
Applied to files:
app/models/simplefin_account/processor.rb
🧬 Code graph analysis (3)
test/models/simplefin_account/liabilities/overpayment_analyzer_test.rb (2)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (2)
new(13-232)call(31-58)app/models/account/current_balance_manager.rb (1)
current_balance(16-23)
app/models/simplefin_item/importer.rb (3)
app/models/simplefin_account/processor.rb (1)
same_sign?(187-189)app/models/simplefin/account_type_mapper.rb (2)
infer(37-88)new(7-89)app/models/simplefin_account/liabilities/overpayment_analyzer.rb (2)
new(13-232)call(31-58)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (4)
lib/money/arithmetic.rb (3)
abs(41-43)positive?(53-55)negative?(49-51)app/models/provider/configurable.rb (1)
present?(226-228)app/models/account.rb (1)
start_date(222-225)lib/simplefin/date_utils.rb (1)
parse_provider_date(9-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (4)
app/models/simplefin_item/importer.rb (1)
114-181: Verify whetherOtherLiabilityshould also be normalized.
Right now onlyCreditCard/Loanare treated as liabilities in balances-only mode; if SimpleFIN can link toOtherLiability, it will keep legacy signs.test/models/simplefin_account_processor_test.rb (1)
85-180: Nice coverage for liability sign edge cases; verify “mislinked depository becomes negative” doesn’t break downstream assumptions.
The last test asserts a depository account ends up with a negative balance after mapper inference; that may affect any code that assumes “asset balances are non-negative”.test/models/simplefin_account/liabilities/overpayment_analyzer_test.rb (1)
36-78: Verifyentryable: Transaction.newis valid forEntryin this codebase.
IfEntryrequires a persistedTransaction, these tests will error atcreate!.app/models/simplefin_account/processor.rb (1)
70-123: Normalization + analyzer fallback flow looks solid; keep an eye on operational blast radius.
Given this can flip signs and affect net worth, I’d strongly suggest validating with a small sample of known-problem accounts (esp. loans) before broad rollout.
| def enabled? | ||
| env = ENV["SIMPLEFIN_CC_OVERPAYMENT_HEURISTIC"].to_s | ||
| env_specified = env.present? | ||
| env_enabled = env_specified ? (%w[1 true yes on].include?(env.downcase)) : false | ||
| # Allow dynamic Setting override; Setting[] returns value or nil | ||
| setting_val = Setting["simplefin_cc_overpayment_detection"] | ||
| setting_enabled = case setting_val | ||
| when true then true | ||
| when false then false | ||
| when String then %w[1 true yes on].include?(setting_val.downcase) | ||
| else | ||
| nil | ||
| end | ||
| # Default behavior: ENABLED unless explicitly disabled via Setting or ENV | ||
| if setting_enabled.nil? | ||
| env_specified ? env_enabled : true | ||
| else | ||
| setting_enabled | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file context and method to understand intent
cat -n app/models/simplefin_account/liabilities/overpayment_analyzer.rb | head -100Repository: we-promise/sure
Length of output: 4105
🏁 Script executed:
# Search for any documentation or comments about the feature rollout
rg -i "overpayment" --type rb -B2 -A2 | head -50Repository: we-promise/sure
Length of output: 85
🏁 Script executed:
# Look for other feature flag patterns in the codebase
rg "ENV\[" --type rb | grep -i "feature\|flag\|heuristic" | head -20Repository: we-promise/sure
Length of output: 85
🏁 Script executed:
# Check for related settings or configuration
rg "simplefin_cc_overpayment" --type rb -B2 -A2Repository: we-promise/sure
Length of output: 85
🏁 Script executed:
# Look for any PR or issue documentation
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec rg -l "overpayment" {} \;Repository: we-promise/sure
Length of output: 85
🏁 Script executed:
# Search for overpayment references without file type restrictions
grep -r "overpayment" . --include="*.md" --include="*.rb" --include="*.txt" 2>/dev/null | head -30Repository: we-promise/sure
Length of output: 3552
🏁 Script executed:
# Check the documentation file that was found
cat docs/onboarding/guide.md | grep -i "overpayment\|simplefin" -B2 -A2 | head -40Repository: we-promise/sure
Length of output: 572
🏁 Script executed:
# Search for Setting keys related to overpayment to understand operational control
grep -r "simplefin_cc_overpayment" . 2>/dev/nullRepository: we-promise/sure
Length of output: 2022
🏁 Script executed:
# Look for any git history or comments about feature rollout
grep -r "feature.*flag\|gradual.*rollout\|beta\|experimental" . --include="*.rb" --include="*.md" 2>/dev/null | grep -i "overpayment\|heuristic" | head -20Repository: we-promise/sure
Length of output: 163
🏁 Script executed:
# Check if there are any specs or tests for the enabled? method
grep -r "enabled\?" . --include="*spec.rb" --include="*test.rb" 2>/dev/null | grep -i "overpayment" -B2 -A2Repository: we-promise/sure
Length of output: 41
🏁 Script executed:
cat test/models/simplefin_account/liabilities/overpayment_analyzer_test.rbRepository: we-promise/sure
Length of output: 3919
🏁 Script executed:
# Check the importer.rb context around the "feature-flagged" comment
grep -B5 -A10 "feature-flagged" ./app/models/simplefin_item/importer.rbRepository: we-promise/sure
Length of output: 676
Default behavior should be DISABLED for a feature-flagged heuristic.
The test setup explicitly enables this with Setting["simplefin_cc_overpayment_detection"] = "true" and disables it in teardown to prevent test bleeding. This pattern confirms the feature should default to off, not on. The importer.rb comment also labels it "feature-flagged," which typically implies opt-in behavior. Change the default at line 77 from true to false:
- if setting_enabled.nil?
- env_specified ? env_enabled : true
+ if setting_enabled.nil?
+ env_specified ? env_enabled : false🤖 Prompt for AI Agents
In app/models/simplefin_account/liabilities/overpayment_analyzer.rb around lines
62–81, the method enabled? currently defaults to true when no ENV or Setting is
specified; change the default to disabled by replacing the final branch so that
when setting_enabled.nil? it returns env_specified ? env_enabled : false (i.e.
default false), ensuring feature-flagged heuristic is opt-in and matches tests
that explicitly enable/disable it.
There was a problem hiding this comment.
Keeping default ON is intentional. If SimpleFin fixes how they send the balances, we can later change the default globally, it’s a one‑line change.
There was a problem hiding this comment.
@luckyPipewrench, understood! That makes sense — keeping it enabled by default allows you to quickly disable the normalization globally if SimpleFin changes their sign semantics upstream. The single-toggle approach is pragmatic for this scenario.
🧠 Learnings used
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account sync must perform transfer auto-matching, calculate daily balance records from start_date to Date.current, calculate holdings, and optionally enrich transaction data
Learnt from: Himmelschmidt
Repo: we-promise/sure PR: 104
File: app/models/simplefin_item/importer.rb:34-45
Timestamp: 2025-09-03T01:15:59.638Z
Learning: In SimpleFin integration, the default initial sync period is intentionally set to 7 days (not 3 years) to provide quick setup, with users able to optionally extend to 3 years via sync_start_date selection. This design balances setup speed with comprehensive data access options.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
- Updated `sticky_key` method in `OverpaymentAnalyzer` to handle missing `@sfa.id` with a default value. - Enhanced `SimplefinAccount::Processor` to use `with_indifferent_access` for `raw_payload` and `org_data`, improving robustness in liability type inference.
|
Wow, this was more complicated than you'd think it should be! But it works! @jjmata |
luckyPipewrench
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (2)
62-81: Consider simplifying feature flag logic for readability.The logic is functionally correct and handles ENV/Setting precedence appropriately, with default-enabled behavior as intentionally designed. However, the nested conditionals make it harder to follow.
Consider this more readable structure:
def enabled? - env = ENV["SIMPLEFIN_CC_OVERPAYMENT_HEURISTIC"].to_s - env_specified = env.present? - env_enabled = env_specified ? (%w[1 true yes on].include?(env.downcase)) : false - # Allow dynamic Setting override; Setting[] returns value or nil - setting_val = Setting["simplefin_cc_overpayment_detection"] - setting_enabled = case setting_val - when true then true - when false then false - when String then %w[1 true yes on].include?(setting_val.downcase) - else - nil - end - # Default behavior: ENABLED unless explicitly disabled via Setting or ENV - if setting_enabled.nil? - env_specified ? env_enabled : true - else - setting_enabled - end + # Setting override takes precedence, then ENV, then default enabled + setting_val = Setting["simplefin_cc_overpayment_detection"] + return parse_bool(setting_val) unless setting_val.nil? + + env_val = ENV["SIMPLEFIN_CC_OVERPAYMENT_HEURISTIC"] + return parse_bool(env_val) if env_val.present? + + true # Default enabled +end + +def parse_bool(value) + case value + when true, false then value + when String then %w[1 true yes on].include?(value.downcase) + else false + end end
119-143: Add logging for transaction gathering errors.The fallback logic from Entry records to raw payload is well-designed. However, Line 141 swallows all exceptions silently without logging, which could hide important errors during transaction gathering (e.g., data format issues, parsing failures).
Add minimal logging before returning the empty array:
rescue => e - [] + Rails.logger.debug("SimpleFIN transaction gathering failed for sfa=#{@sfa.id}: #{e.class} - #{e.message}") + [] endThis maintains the graceful degradation while providing visibility into gathering failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb(1 hunks)app/models/simplefin_account/processor.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rb: Application supports two modes: 'managed' and 'self_hosted' via Rails.application.config.app_mode
Use Current.user and Current.family instead of current_user / current_family for authentication context
Optimize database queries with proper indexes to prevent N+1 queries using includes/joins
**/*.rb: Use Rails pluralization in i18n:t("transactions.count", count: @transactions.count).
Runbin/rubocop -f github -afor Ruby linting with auto-correct before pull requests.
**/*.rb: Ruby code should use 2-space indent,snake_casefor methods and variables, andCamelCasefor classes and modules, following Rails conventions for folders and file names
Prefer environment variables over hard-coded values for configuration
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/models/**/*.rb: Keep business logic in models using skinny controllers and fat models pattern
Store all monetary values in base currency (user's primary currency) and use Money objects for conversion
Use ActiveRecord validations for form convenience and complex business logic validations
Use Rails concerns and POROs for organizing business logic instead of services folder
app/models/**/*.rb: All monetary values stored in base currency (user's primary currency). UseMoneyobjects for currency conversion and formatting.
Place business logic inapp/models/folder. Avoid creatingapp/services/folder. Use Rails concerns and POROs for organization.
Models should answer questions about themselves using instance methods: useaccount.balance_seriesnotAccountSeries.new(account).call.
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
**/*.{rb,erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{rb,erb,html.erb}: UseCurrent.userfor the current user context in controllers and views. Do NOT usecurrent_user.
UseCurrent.familyfor the current family context. Do NOT usecurrent_family.
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
**/*.{erb,html.erb,rb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html.erb,rb}: All user-facing strings must use localization (i18n) via thet()helper. Update locale files for each new or changed element.
Use server-side formatting for currencies, numbers, and dates. Do not format in JavaScript.
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
{app/models/**/*.rb,db/migrate/**/*.rb}
📄 CodeRabbit inference engine (CLAUDE.md)
Simple validations (null checks, unique indexes) should be in the database. ActiveRecord validations for form convenience (prefer client-side when possible). Complex validations and business logic in ActiveRecord.
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb}: UseCurrent.userfor accessing the current user. Do NOT usecurrent_user
UseCurrent.familyfor accessing the current family. Do NOT usecurrent_family
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
**/*.{rb,js,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
**/*.{rb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use ActiveRecord validations for complex validations and business logic. Simple validations may be mirrored in ActiveRecord for form error handling convenience, but prioritize client-side form validation when possible
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
app/**/*.{rb,js,ts,jsx,tsx,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Code should be organized in
app/directory (Rails MVC, services, jobs, mailers, components), with JS inapp/javascript/, and styles/assets inapp/assets/(Tailwind, images, fonts)
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Run
bin/brakemansecurity scan before major PRs to check for static analysis of common Rails issues
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
🧠 Learnings (12)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account sync must perform transfer auto-matching, calculate daily balance records from start_date to Date.current, calculate holdings, and optionally enrich transaction data
Applied to files:
app/models/simplefin_account/processor.rb
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-11-04T15:16:23.341Z
Learnt from: sokie
Repo: we-promise/sure PR: 276
File: app/controllers/reports_controller.rb:159-169
Timestamp: 2025-11-04T15:16:23.341Z
Learning: In the Budget model, the methods `actual_spending` and `allocated_spending` return BigDecimal values (not Money objects), so division operations like `budget.actual_spending / budget.allocated_spending` work correctly without needing conversion to fractional units.
Applied to files:
app/models/simplefin_account/processor.rb
📚 Learning: 2025-11-04T16:21:39.347Z
Learnt from: sokie
Repo: we-promise/sure PR: 276
File: app/controllers/reports_controller.rb:80-84
Timestamp: 2025-11-04T16:21:39.347Z
Learning: The we-promise/sure codebase uses a custom Money class (lib/money.rb) that treats numeric inputs as dollar amounts, not fractional units (cents). When Money.new receives a numeric value, it converts it directly to BigDecimal via BigDecimal(obj.to_s) without multiplication. This differs from the standard ruby-money gem which expects cents/fractional units.
Applied to files:
app/models/simplefin_account/processor.rb
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account model is a Rails delegated type with subtypes: Depository, Investment, Crypto, Property, Vehicle, OtherAsset (assets) and CreditCard, Loan, OtherLiability (liabilities)
Applied to files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-03T01:15:59.638Z
Learnt from: Himmelschmidt
Repo: we-promise/sure PR: 104
File: app/models/simplefin_item/importer.rb:34-45
Timestamp: 2025-09-03T01:15:59.638Z
Learning: In SimpleFin integration, the default initial sync period is intentionally set to 7 days (not 3 years) to provide quick setup, with users able to optionally extend to 3 years via sync_start_date selection. This design balances setup speed with comprehensive data access options.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T12:23:31.324Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T11:40:26.913Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_entry/processor.rb:10-12
Timestamp: 2025-09-13T11:40:26.913Z
Learning: The plaid_id field is used consistently across multiple banking integrations (Plaid, SimpleFin, and Enable Banking) as a common external identifier field. A comprehensive refactor to use a proper external_id field should be done across all integrations in a separate PR to maintain consistency and avoid scope creep.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T11:27:55.271Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
🧬 Code graph analysis (1)
app/models/simplefin_account/processor.rb (1)
app/models/simplefin_item/importer.rb (2)
to_decimal(861-870)same_sign?(872-874)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (7)
app/models/simplefin_account/processor.rb (3)
42-48: LGTM! Balance extraction with safe fallback.The extraction logic correctly prioritizes posted balance over available balance when non-zero, with safe BigDecimal conversion via the helper method.
50-70: LGTM! Liability determination with proper hash access.The indifferent access handling correctly addresses the past review concern about string-only hash keys. The warning log for mismatched liability types is valuable for identifying accounts that may need relinking.
72-126: LGTM! Robust overpayment analysis with comprehensive fallbacks.The integration provides excellent resilience with multiple error handling layers (NameError for missing analyzer, general exceptions, and Sentry failures). The INFO logging with metrics provides good observability for both positive classifications and unknown cases.
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (4)
1-29: LGTM! Clean structure with Result struct and sensible defaults.The Result struct with keyword arguments and DEFAULTS hash provide a clean, configurable foundation. Constructor signature is well-designed with keyword arguments.
Minor note:
to_d(line 27) vsto_decimalnaming inconsistency with processor.rb, though both serve the same purpose.
31-58: LGTM! Well-structured main logic with appropriate guards.The guard clauses follow Ruby best practices, and the sticky cache mechanism effectively prevents classification flapping. The flow from transaction gathering → metrics computation → classification → cache writing is logical and maintainable.
83-117: LGTM! Consistent configuration getters with proper validation.The configuration methods follow a consistent pattern with Setting overrides, fallbacks to DEFAULTS, and appropriate validation. The validations ensure safe values (e.g.,
> 0for most,>= 0for statement_guard_days).
145-195: LGTM! Sound heuristic with statement guard and dynamic epsilon.The metrics computation and classification logic are well-designed:
- Statement guard prevents false positives from recent large payments
- Dynamic epsilon (max of base or 0.5% of observed) scales appropriately
- Credit/debt checks use sensible thresholds
- Minimum payment count requirement reduces false positives from sparse data
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
Outdated
Show resolved
Hide resolved
… and apply across models - Moved `to_decimal` and `same_sign?` methods into a new `SimplefinNumericHelpers` concern for reuse. - Updated `OverpaymentAnalyzer`, `Processor`, and `Importer` to include the concern and remove redundant method definitions. - Added empty fixtures for `simplefin_accounts` and `simplefin_items` to ensure test isolation. - Refactored `OverpaymentAnalyzerTest` to reduce fixture dependencies and ensure cleanup of created records.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (1)
208-226:sticky_keystill risks collisions for non-persisted records.
Current fallback"new"will collide if analyzer is ever invoked on multiple non-persisted instances withinsticky_days. If that can’t happen, add an assertion/comment; otherwise includeobject_idin the fallback.def sticky_key - id = @sfa.id || "new" + id = @sfa.id || "tmp:#{@sfa.object_id}" "simplefin:sfa:#{id}:liability_sign_hint" end
🧹 Nitpick comments (6)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (4)
13-60: Good separation + safe fallbacks, but sticky payload should be schema-stable across cache stores.
read_stickyassumessticky[:expires_at]round-trips as aTimeand is comparable to@now. With some cache stores/serializers,expires_atmay deserialize as aString, which can raise and force callers into rescue/fallback (Processor rescues, but analyzer should be robust itself).- sticky = read_sticky - if sticky && sticky[:expires_at] > @now + sticky = read_sticky + expires_at = + case sticky&.dig(:expires_at) + when Time then sticky[:expires_at] + when String then Time.parse(sticky[:expires_at]) rescue nil + else nil + end + if sticky && expires_at && expires_at > @now return Result.new(classification: sticky[:value].to_sym, reason: "sticky_hint", metrics: {}) end(Also consider writing
expires_atas an ISO8601 string to begin with.)
84-119: Config readers: minor ergonomics—centralize common “Setting-or-default” parsing.
These methods repeat the same pattern; not urgent, but a small helper would reduce drift (especially for integer bounds).
120-145: Preferpluckto avoid instantiating AR objects; don’t silently swallow gather errors without any trace.
select + mapbuilds AR instances;pluckavoids that. Also,rescue => _e; []makes “unknown → fallback” indistinguishable from “bug parsing provider payload”. Consider at least adebuglog (rate-limited if needed).- entries = account.entries.where("date >= ?", start_date).select(:amount, :date) - txns = entries.map { |e| { amount: to_decimal(e.amount), date: e.date } } + rows = account.entries.where("date >= ?", start_date).pluck(:amount, :date) + txns = rows.map { |amount, date| { amount: to_decimal(amount), date: date } } return txns if txns.size >= min_txns @@ - rescue => _e + rescue => e + Rails.logger.debug("SimpleFIN overpayment gather_transactions failed for sfa=#{@sfa.id}: #{e.class} - #{e.message}") rescue nil [] end
198-206:convert_provider_amountduplicatesto_decimal; simplify to reduce divergence risk.def convert_provider_amount(val) - amt = case val - when String then BigDecimal(val) rescue BigDecimal("0") - when Numeric then BigDecimal(val.to_s) - else BigDecimal("0") - end # Negate to convert banking convention (expenses negative) -> Maybe convention - -amt + -to_decimal(val) endapp/models/simplefin_account/processor.rb (2)
33-127: Overpayment flow is robust; avoid rescue for control-flow and ensureobservedselection matches desired precedence.
Two small nits:
rescue NameErrorfor “Analyzer not loaded” is a bit broad; consider checkingdefined?(SimplefinAccount::Liabilities::OverpaymentAnalyzer)instead.observed = bal.nonzero? ? bal : availmakes “0 balance” fall through toavail. If SimpleFIN can legitimately report0posted with meaningfulavail, that’s fine; otherwise it may create unexpected flips.- rescue NameError - # Analyzer not loaded; keep legacy behavior - balance = normalize_liability_balance(observed, bal, avail) + rescue NameError + balance = normalize_liability_balance(observed, bal, avail) rescue => e @@ end(Optional alternative to avoid the NameError path entirely:)
if defined?(SimplefinAccount::Liabilities::OverpaymentAnalyzer) # call analyzer else balance = normalize_liability_balance(observed, bal, avail) end
181-191:normalize_liability_balance:both_presentisn’t a boolean (harmless), but clarity helps.
bal.nonzero? && avail.nonzero?returns a BigDecimal or nil; works, but reads oddly.- both_present = bal.nonzero? && avail.nonzero? + both_present = bal.nonzero? != nil && avail.nonzero? != nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/models/concerns/simplefin_numeric_helpers.rb(1 hunks)app/models/simplefin_account/liabilities/overpayment_analyzer.rb(1 hunks)app/models/simplefin_account/processor.rb(3 hunks)app/models/simplefin_item/importer.rb(3 hunks)test/fixtures/simplefin_accounts.yml(1 hunks)test/fixtures/simplefin_items.yml(1 hunks)test/models/simplefin_account/liabilities/overpayment_analyzer_test.rb(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/fixtures/simplefin_items.yml
- test/fixtures/simplefin_accounts.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/models/simplefin_account/liabilities/overpayment_analyzer_test.rb
- app/models/simplefin_item/importer.rb
🧰 Additional context used
📓 Path-based instructions (10)
**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rb: Application supports two modes: 'managed' and 'self_hosted' via Rails.application.config.app_mode
Use Current.user and Current.family instead of current_user / current_family for authentication context
Optimize database queries with proper indexes to prevent N+1 queries using includes/joins
**/*.rb: Use Rails pluralization in i18n:t("transactions.count", count: @transactions.count).
Runbin/rubocop -f github -afor Ruby linting with auto-correct before pull requests.
**/*.rb: Ruby code should use 2-space indent,snake_casefor methods and variables, andCamelCasefor classes and modules, following Rails conventions for folders and file names
Prefer environment variables over hard-coded values for configuration
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/models/**/*.rb: Keep business logic in models using skinny controllers and fat models pattern
Store all monetary values in base currency (user's primary currency) and use Money objects for conversion
Use ActiveRecord validations for form convenience and complex business logic validations
Use Rails concerns and POROs for organizing business logic instead of services folder
app/models/**/*.rb: All monetary values stored in base currency (user's primary currency). UseMoneyobjects for currency conversion and formatting.
Place business logic inapp/models/folder. Avoid creatingapp/services/folder. Use Rails concerns and POROs for organization.
Models should answer questions about themselves using instance methods: useaccount.balance_seriesnotAccountSeries.new(account).call.
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
**/*.{rb,erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{rb,erb,html.erb}: UseCurrent.userfor the current user context in controllers and views. Do NOT usecurrent_user.
UseCurrent.familyfor the current family context. Do NOT usecurrent_family.
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
**/*.{erb,html.erb,rb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html.erb,rb}: All user-facing strings must use localization (i18n) via thet()helper. Update locale files for each new or changed element.
Use server-side formatting for currencies, numbers, and dates. Do not format in JavaScript.
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
{app/models/**/*.rb,db/migrate/**/*.rb}
📄 CodeRabbit inference engine (CLAUDE.md)
Simple validations (null checks, unique indexes) should be in the database. ActiveRecord validations for form convenience (prefer client-side when possible). Complex validations and business logic in ActiveRecord.
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb}: UseCurrent.userfor accessing the current user. Do NOT usecurrent_user
UseCurrent.familyfor accessing the current family. Do NOT usecurrent_family
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
**/*.{rb,js,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
**/*.{rb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use ActiveRecord validations for complex validations and business logic. Simple validations may be mirrored in ActiveRecord for form error handling convenience, but prioritize client-side form validation when possible
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
app/**/*.{rb,js,ts,jsx,tsx,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Code should be organized in
app/directory (Rails MVC, services, jobs, mailers, components), with JS inapp/javascript/, and styles/assets inapp/assets/(Tailwind, images, fonts)
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Run
bin/brakemansecurity scan before major PRs to check for static analysis of common Rails issues
Files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
🧠 Learnings (19)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-11-24T16:56:13.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.406Z
Learning: Use Rails concerns for code organization around model 'traits' and shared functionality, not just for moving code to another location. Prefer models answering questions about themselves with methods like account.balance_series over service-like patterns
Applied to files:
app/models/simplefin_account/processor.rbapp/models/concerns/simplefin_numeric_helpers.rb
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/models/**/*.rb : Use Rails concerns and POROs for organizing business logic instead of services folder
Applied to files:
app/models/simplefin_account/processor.rb
📚 Learning: 2025-09-13T12:23:31.324Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Applied to files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-11-24T16:55:43.069Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.069Z
Learning: Applies to app/models/**/*.rb : Place business logic in `app/models/` folder. Avoid creating `app/services/` folder. Use Rails concerns and POROs for organization.
Applied to files:
app/models/simplefin_account/processor.rb
📚 Learning: 2025-11-24T16:56:13.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.406Z
Learning: Organize large pieces of business logic into Rails concerns and POROs rather than separate service folders. Place business logic directly in app/models/ folder
Applied to files:
app/models/simplefin_account/processor.rb
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/models/**/*.rb : Keep business logic in models using skinny controllers and fat models pattern
Applied to files:
app/models/simplefin_account/processor.rb
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account sync must perform transfer auto-matching, calculate daily balance records from start_date to Date.current, calculate holdings, and optionally enrich transaction data
Applied to files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
app/models/simplefin_account/processor.rbapp/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account model is a Rails delegated type with subtypes: Depository, Investment, Crypto, Property, Vehicle, OtherAsset (assets) and CreditCard, Loan, OtherLiability (liabilities)
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-03T01:15:59.638Z
Learnt from: Himmelschmidt
Repo: we-promise/sure PR: 104
File: app/models/simplefin_item/importer.rb:34-45
Timestamp: 2025-09-03T01:15:59.638Z
Learning: In SimpleFin integration, the default initial sync period is intentionally set to 7 days (not 3 years) to provide quick setup, with users able to optionally extend to 3 years via sync_start_date selection. This design balances setup speed with comprehensive data access options.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T11:40:26.913Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_entry/processor.rb:10-12
Timestamp: 2025-09-13T11:40:26.913Z
Learning: The plaid_id field is used consistently across multiple banking integrations (Plaid, SimpleFin, and Enable Banking) as a common external identifier field. A comprehensive refactor to use a proper external_id field should be done across all integrations in a separate PR to maintain consistency and avoid scope creep.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T11:27:55.271Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-11-04T16:21:39.347Z
Learnt from: sokie
Repo: we-promise/sure PR: 276
File: app/controllers/reports_controller.rb:80-84
Timestamp: 2025-11-04T16:21:39.347Z
Learning: The we-promise/sure codebase uses a custom Money class (lib/money.rb) that treats numeric inputs as dollar amounts, not fractional units (cents). When Money.new receives a numeric value, it converts it directly to BigDecimal via BigDecimal(obj.to_s) without multiplication. This differs from the standard ruby-money gem which expects cents/fractional units.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rbapp/models/concerns/simplefin_numeric_helpers.rb
📚 Learning: 2025-11-24T16:55:43.069Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.069Z
Learning: Applies to app/models/**/*.rb : All monetary values stored in base currency (user's primary currency). Use `Money` objects for currency conversion and formatting.
Applied to files:
app/models/concerns/simplefin_numeric_helpers.rb
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/models/**/*.rb : Store all monetary values in base currency (user's primary currency) and use Money objects for conversion
Applied to files:
app/models/concerns/simplefin_numeric_helpers.rb
📚 Learning: 2025-11-24T16:56:13.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.406Z
Learning: Applies to **/*.{rb,js,erb} : Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Applied to files:
app/models/concerns/simplefin_numeric_helpers.rb
🧬 Code graph analysis (2)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (2)
app/models/concerns/simplefin_numeric_helpers.rb (1)
to_decimal(8-17)lib/simplefin/date_utils.rb (1)
parse_provider_date(9-27)
app/models/concerns/simplefin_numeric_helpers.rb (2)
app/models/simplefin_item/importer.rb (2)
to_decimal(862-871)same_sign?(873-875)lib/money/arithmetic.rb (2)
positive?(53-55)negative?(49-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (4)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (2)
63-82: Enablement toggle logic matches “default ON” intent; consider making precedence explicit in comment/tests.
Right now,Setting["simplefin_cc_overpayment_detection"]overridesENV["SIMPLEFIN_CC_OVERPAYMENT_HEURISTIC"], and default is ON when neither is set. That’s fine given PR objectives, but it’s easy to misread later.
146-197: Classification logic is coherent; one edge-case to sanity-check: debt classification gating onmin_payments.
For accounts with mostly charges inside the window (few/no payments yet),:debtwill stay:unknownand fall back to sign-only. If that’s intentional (avoid flipping on sparse evidence), OK; otherwise consider allowing:debtwhen charges dominance is strong even withpayments_count < min_payments.app/models/concerns/simplefin_numeric_helpers.rb (1)
1-22: Nice extraction;same_sign?is intentionally “false if either is zero” (good for your use).
to_decimal’s “invalid string → 0” behavior is consistent with “robust fallback” goals; just be aware it can mask upstream data issues (which is probably OK here). As per coding guidelines, using a shared concern for DRY model logic is the right direction.app/models/simplefin_account/processor.rb (1)
1-29:include SimplefinNumericHelpersis the right reuse point.
No concerns with this integration point.
…ency - Simplified `enabled?` method in `OverpaymentAnalyzer` for clearer precedence order (Setting > ENV > default). - Added `parse_bool` helper to streamline boolean parsing. - Enhanced error handling with detailed logging for transaction gathering failures. - Improved `sticky_key` method to use a temporary object ID fallback when `@sfa.id` is missing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (2)
82-116: Consider extracting repetitive setting getter pattern.All six configuration methods follow the same pattern: fetch Setting with presence check, coerce to type, validate, fallback to default. While explicit and clear, this could be DRYed with a helper:
def setting_with_default(key, default, type: :integer, &validator) val = Setting["simplefin_cc_overpayment_#{key}"] v = case type when :integer then (val.presence || default).to_i when :decimal then to_decimal(val.presence || default) end validator ? (validator.call(v) ? v : default) : (v > 0 ? v : default) endHowever, the current explicit approach is also maintainable, so this is optional.
176-195: Classification logic is sound; consider extracting magic number.The epsilon-based bounds and statement guard provide robust classification. The 0.5% adaptive epsilon at line 182 is a magic number that could be extracted:
EPSILON_PERCENTAGE = BigDecimal("0.005") # 0.5% of observed balance def classify(m) # ... eps = [ epsilon_base, (@observed.abs * EPSILON_PERCENTAGE) ].max # ... endHowever, this is minor and the current code is clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rb: Application supports two modes: 'managed' and 'self_hosted' via Rails.application.config.app_mode
Use Current.user and Current.family instead of current_user / current_family for authentication context
Optimize database queries with proper indexes to prevent N+1 queries using includes/joins
**/*.rb: Use Rails pluralization in i18n:t("transactions.count", count: @transactions.count).
Runbin/rubocop -f github -afor Ruby linting with auto-correct before pull requests.
**/*.rb: Ruby code should use 2-space indent,snake_casefor methods and variables, andCamelCasefor classes and modules, following Rails conventions for folders and file names
Prefer environment variables over hard-coded values for configuration
Files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/models/**/*.rb: Keep business logic in models using skinny controllers and fat models pattern
Store all monetary values in base currency (user's primary currency) and use Money objects for conversion
Use ActiveRecord validations for form convenience and complex business logic validations
Use Rails concerns and POROs for organizing business logic instead of services folder
app/models/**/*.rb: All monetary values stored in base currency (user's primary currency). UseMoneyobjects for currency conversion and formatting.
Place business logic inapp/models/folder. Avoid creatingapp/services/folder. Use Rails concerns and POROs for organization.
Models should answer questions about themselves using instance methods: useaccount.balance_seriesnotAccountSeries.new(account).call.
Files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
**/*.{rb,erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{rb,erb,html.erb}: UseCurrent.userfor the current user context in controllers and views. Do NOT usecurrent_user.
UseCurrent.familyfor the current family context. Do NOT usecurrent_family.
Files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
**/*.{erb,html.erb,rb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html.erb,rb}: All user-facing strings must use localization (i18n) via thet()helper. Update locale files for each new or changed element.
Use server-side formatting for currencies, numbers, and dates. Do not format in JavaScript.
Files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
{app/models/**/*.rb,db/migrate/**/*.rb}
📄 CodeRabbit inference engine (CLAUDE.md)
Simple validations (null checks, unique indexes) should be in the database. ActiveRecord validations for form convenience (prefer client-side when possible). Complex validations and business logic in ActiveRecord.
Files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb}: UseCurrent.userfor accessing the current user. Do NOT usecurrent_user
UseCurrent.familyfor accessing the current family. Do NOT usecurrent_family
Files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
**/*.{rb,js,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
**/*.{rb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use ActiveRecord validations for complex validations and business logic. Simple validations may be mirrored in ActiveRecord for form error handling convenience, but prioritize client-side form validation when possible
Files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
app/**/*.{rb,js,ts,jsx,tsx,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Code should be organized in
app/directory (Rails MVC, services, jobs, mailers, components), with JS inapp/javascript/, and styles/assets inapp/assets/(Tailwind, images, fonts)
Files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Run
bin/brakemansecurity scan before major PRs to check for static analysis of common Rails issues
Files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
🧠 Learnings (11)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account sync must perform transfer auto-matching, calculate daily balance records from start_date to Date.current, calculate holdings, and optionally enrich transaction data
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account model is a Rails delegated type with subtypes: Depository, Investment, Crypto, Property, Vehicle, OtherAsset (assets) and CreditCard, Loan, OtherLiability (liabilities)
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-03T01:15:59.638Z
Learnt from: Himmelschmidt
Repo: we-promise/sure PR: 104
File: app/models/simplefin_item/importer.rb:34-45
Timestamp: 2025-09-03T01:15:59.638Z
Learning: In SimpleFin integration, the default initial sync period is intentionally set to 7 days (not 3 years) to provide quick setup, with users able to optionally extend to 3 years via sync_start_date selection. This design balances setup speed with comprehensive data access options.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T12:23:31.324Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T11:40:26.913Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_entry/processor.rb:10-12
Timestamp: 2025-09-13T11:40:26.913Z
Learning: The plaid_id field is used consistently across multiple banking integrations (Plaid, SimpleFin, and Enable Banking) as a common external identifier field. A comprehensive refactor to use a proper external_id field should be done across all integrations in a separate PR to maintain consistency and avoid scope creep.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-09-13T11:27:55.271Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-11-04T16:21:39.347Z
Learnt from: sokie
Repo: we-promise/sure PR: 276
File: app/controllers/reports_controller.rb:80-84
Timestamp: 2025-11-04T16:21:39.347Z
Learning: The we-promise/sure codebase uses a custom Money class (lib/money.rb) that treats numeric inputs as dollar amounts, not fractional units (cents). When Money.new receives a numeric value, it converts it directly to BigDecimal via BigDecimal(obj.to_s) without multiplication. This differs from the standard ruby-money gem which expects cents/fractional units.
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
📚 Learning: 2025-11-24T16:56:30.669Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account sync must perform transfer auto-matching, calculate daily balance records from start_date to Date.current, calculate holdings, and optionally enrich transaction data
Applied to files:
app/models/simplefin_account/liabilities/overpayment_analyzer.rb
🧬 Code graph analysis (1)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (3)
app/models/concerns/simplefin_numeric_helpers.rb (1)
to_decimal(8-17)lib/money/arithmetic.rb (4)
abs(41-43)class(22-24)positive?(53-55)negative?(49-51)lib/simplefin/date_utils.rb (1)
parse_provider_date(9-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (7)
app/models/simplefin_account/liabilities/overpayment_analyzer.rb (7)
1-24: LGTM! Clear documentation and well-structured constants.The class documentation clearly explains the classification approach, fallback strategy, and conventions. The DEFAULTS hash provides sensible thresholds for the heuristic.
26-30: LGTM! Clean initialization with testable time injection.The
now:parameter enables deterministic testing of time-dependent behavior (sticky cache expiry, statement guards).
63-80: LGTM! Flexible configuration with clear precedence.The three-tier fallback (Setting → ENV → default) provides good operational control, and
parse_boolhandles various formats robustly.
118-143: LGTM! Robust transaction gathering with appropriate fallbacks.The two-tier approach (entries → raw payload) provides resilience, and the exception handler ensures the analyzer degrades gracefully rather than failing hard.
145-174: LGTM! Accurate metrics computation with BigDecimal precision.The separation of charges (positive) and payments (negative) correctly handles Maybe's transaction convention, and the recent_payment tracking enables the statement boundary guard.
197-224: LGTM! Clean helper methods with proper convention conversion.The negation in
convert_provider_amountcorrectly converts SimpleFIN's banking convention to Maybe's expense convention, and the sticky cache key properly handles unpersisted records.
37-37: Verify exclusion of OtherLiability type.The check only includes
"CreditCard"and"Loan", but the Account model includesOtherLiabilityas a liability type according to project learnings. If OtherLiability accounts can experience overpayments, they should be included.#!/bin/bash # Description: Check if OtherLiability is used in the codebase and whether it should support overpayment detection # Find OtherLiability references rg -n "OtherLiability" --type rb -C 3 # Check Account model for liability types ast-grep --pattern 'class OtherLiability'
2 participants
SimplefinAccount::Liabilities::OverpaymentAnalyzerthat classifies a SimpleFIN liability balance as::credit→ overpayment/credit state → stored as negative:debt→ owe money → stored as positive:unknown→ ambiguous → fall back to existing sign-normalizationSimplefinAccount::Processor#process_account!(regular syncs)SimplefinItem::Importer#import_account_minimal_and_balance(balances-only discovery)Setting["simplefin_cc_overpayment_detection"] = "false"SIMPLEFIN_CC_OVERPAYMENT_HEURISTIC=falsecredit/debt) and alsounknownbefore fallback; includes compact metrics for diagnosis.Why
SimpleFIN reports all liabilities as negative, regardless of whether it’s normal debt or an overpayment/credit. That breaks sign semantics in the app (we show debt as positive, credit as negative). Transaction history provides the signal needed to disambiguate reliably.
Heuristic details (production-safe)
Entryrecords within a window (default 120 days)charges_total = sum(amount for amount > 0)payments_total = sum(-amount for amount < 0)|observed|(with epsilon slack), classify:credit:debt:unknown:unknownor any analyzer error, reuses the existing sign-only normalization rulesEdge cases and how they’re handled
:unknown→ fallback logic applies (unchanged behavior):unknownto avoid churnbalancevsavailable-balance) → if analyzer returns:unknown, legacy mixed-sign logic still resolves consistentlyObservability
credit/debtclassifications with{charges_total, payments_total, tx_count}unknown: INFO log with{reason, tx_count, charges_total, payments_total, observed}before falling backunknownlog in balances-only importer path for parityBackward compatibility and safety
Known limitations (acceptable trade-offs)
Rollback/Toggle
Setting["simplefin_cc_overpayment_detection"] = "false"SIMPLEFIN_CC_OVERPAYMENT_HEURISTIC=false(restart processes)Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.