Fix SimpleFin investment holdings and comprehensive integration improvements #104
Conversation
- Remove "Skip - don't add" option from account setup
- Simplify account setup flow to require all accounts be assigned types
- Update controller logic to handle all accounts without skipping
- Redirect to accounts page instead of SimpleFin items page
- Add I18N message support with t(".success")
- Remove skipped account filtering from syncer - All unlinked accounts now block sync until setup is complete - Remove skipped account UI elements from setup view - Streamline sync flow without complex skipped/non-skipped logic
- Update SimplefinAccount::Processor to recalculate balances during sync - Properly calculate cash_balance for investment accounts using BalanceCalculator - Handle negative balances for credit cards and loans correctly - Ensure account balance and cash balance are updated from latest SimpleFin data
- Add comprehensive English translations for SimpleFin UI - Create edit view for SimpleFin re-authentication - Support status messages, errors, and user feedback - Match translation structure with Plaid integration
- Add investment processors for transactions, holdings, and balance calculation - Add liability processors for credit cards and loans - Add transaction processor for standard account transactions - Add account importer for SimpleFin account data - Organize processors by account type for maintainable architecture
- Add Stimulus controller to show loading state during form submission - Disable button and show loading text to prevent double submissions - Improve user experience during SimpleFin account setup
- Add edit and update actions to SimpleFin items routes - Enable re-authentication flow for expired SimpleFin connections - Match route structure with Plaid items for consistency
- Add institution_id, institution_name, institution_domain fields - Add institution_url, institution_color for UI customization - Add raw_institution_payload for complete institution data storage - Enable better institution organization and display
- Add institution summary and connected institutions methods - Store and retrieve institution metadata from SimpleFin API - Add institution-aware import functionality - Support multiple institutions per SimpleFin connection
- Fix cash balance calculation in Account.create_from_simplefin_account - Add nil check for plaid_provider in remove_plaid_item method - Ensure proper balance handling for investment accounts during creation
- Add parent sync assignment for existing syncs when parent_sync is provided - Ensure sync hierarchy is maintained when expanding sync windows - Fix sync relationship consistency in nested sync operations
- Improve SimpleFin connection display and status information - Add better visual styling and user feedback - Match UI consistency with Plaid item views
- Add institution metadata fields to simplefin_items table - Support institution tracking and organization features
- Update controller tests for edit/update actions and removed skip functionality - Update model tests for institution metadata and enhanced features - Ensure test coverage for SimpleFin improvements
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds edit/update endpoints and UI for reconnecting SimpleFin items; refactors import/sync into modular processors for transactions, investments, and liabilities; introduces holdings and investment ingestion, DB migrations and schema changes, a Stimulus loading-button controller, network hardening, cash_balance logic, locales, and tests. Changes
Sequence Diagram(s)%%{init: {"themeVariables": {"actorBorder":"#2b6cb0","actorBackground":"#ebf8ff","sequenceNoteBackground":"#fefff6"}}}%%
sequenceDiagram
autonumber
actor User
participant UI as Edit Modal
participant C as SimplefinItemsController
participant Prov as Provider::Simplefin
participant NewSI as New SimplefinItem
participant OldSI as Old SimplefinItem
participant DB as Database
User->>UI: Submit setup_token
UI->>C: PATCH /simplefin_items/:id
C->>C: validate token presence
alt token blank
C-->>UI: render :edit (422) with i18n error
else valid token
C->>Prov: exchange setup_token → access_url & accounts
Prov-->>C: accounts payload
C->>NewSI: create updated item with token and name
C->>NewSI: import latest accounts
C->>DB: transaction: rebind accounts by account_id, update old simplefin_account pointers, mark OldSI for deletion
C->>NewSI: clear requires_update
C-->>User: redirect to accounts_path with i18n success
end
sequenceDiagram
autonumber
participant Syncer as SimplefinItem::Syncer
participant Prov as Provider::Simplefin
participant SI as SimplefinItem
participant Proc as SimplefinAccount::Processor
participant Sync as Sync(model)
Syncer->>Sync: status_text "Importing accounts..."
Syncer->>Prov: fetch accounts payload
Syncer->>Sync: status_text "Checking account configuration..."
Syncer->>SI: compute linked/unlinked counts
alt unlinked > 0
Syncer->>SI: set pending_account_setup = true
Syncer->>Sync: status_text "<N> accounts need setup..."
else
Syncer->>SI: clear pending_account_setup
Syncer->>Sync: status_text "Processing transactions and holdings..."
loop each linked simplefin_account
Syncer->>Proc: process(simplefin_account)
end
Syncer->>Sync: status_text "Calculating balances..."
Syncer->>SI: schedule_account_syncs
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Himmelschmidt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request delivers a comprehensive overhaul of the SimpleFin integration, primarily focusing on resolving long-standing issues with investment account holdings and significantly enhancing the overall reliability and user experience. It introduces a more robust and modular architecture for processing various account types, streamlines the re-authentication process for expired connections, and improves error handling across the board. The changes aim to provide a more stable, informative, and user-friendly financial data synchronization experience.
Highlights
- Investment Holdings & Balance Fixes: The pull request resolves critical issues with SimpleFin investment accounts, ensuring that holdings data is properly processed and displayed, and introduces accurate cash balance calculations for these accounts.
- Enhanced Re-authentication Process: A new re-authentication flow has been implemented, providing a dedicated user interface and backend logic for users to easily update expired SimpleFin connections with new setup tokens.
- Modular Data Processing Architecture: The SimpleFin integration now utilizes specialized data processors for different account types (investments, liabilities, general transactions), leading to more robust data handling, better error isolation, and improved sync reliability.
- Improved User Experience and Sync Reliability: Significant improvements have been made to the overall user experience and sync reliability, including the addition of loading states, comprehensive I18N translations, enhanced API error handling with increased timeouts, and the removal of the 'skip account' option to ensure all accounts are properly categorized.
- Richer Institution Metadata: The system now captures and displays more detailed institution metadata from SimpleFin, providing users with clearer information about their connected financial institutions.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive pull request that significantly improves the SimpleFin integration. The architectural changes, such as introducing specialized data processors and a re-authentication flow, are well-executed and enhance maintainability and user experience. The bug fixes for investment holdings and sync statuses are critical improvements. My review includes a few suggestions to further improve data integrity and code quality, such as adding a transaction to the update flow and removing some potentially unused code.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🔭 Outside diff range comments (5)
app/models/provider/simplefin.rb (2)
32-40: Normalize dates to UTC when building Unix timestampsDate#to_time without UTC may shift by local timezone and drift the intended window. Use UTC to produce consistent epoch seconds.
Apply this diff:
- if start_date - start_timestamp = start_date.to_time.to_i + if start_date + start_timestamp = start_date.to_time.utc.to_i query_params["start-date"] = start_timestamp.to_s end - if end_date - end_timestamp = end_date.to_time.to_i + if end_date + end_timestamp = end_date.to_time.utc.to_i query_params["end-date"] = end_timestamp.to_s end
59-72: Sanitize error logs to avoid leaking response bodiesLogging raw response bodies can leak PII and embedded credentials. Log codes and a small excerpt at most; ideally nothing from body.
Apply this diff:
- when 400 - Rails.logger.error "SimpleFin API: Bad request - #{response.body}" + when 400 + Rails.logger.error "SimpleFin API: Bad request (code 400)" raise SimplefinError.new("Bad request to SimpleFin API: #{response.body}", :bad_request) when 403 raise SimplefinError.new("Access URL is no longer valid", :access_forbidden) when 402 raise SimplefinError.new("Payment required to access this account", :payment_required) else - Rails.logger.error "SimpleFin API: Unexpected response - Code: #{response.code}, Body: #{response.body}" + Rails.logger.error "SimpleFin API: Unexpected response - Code: #{response.code}" raise SimplefinError.new("Failed to fetch accounts: #{response.code} #{response.message} - #{response.body}", :fetch_failed)If you want to keep some context for troubleshooting, consider logging a scrubbed, truncated body snippet (e.g., first 200 chars) after removing credentials, but default to not logging body content.
app/models/plaid_item.rb (1)
105-114: Harden JSON parsing in rescue; don’t assume response_body shapeJSON.parse may raise if response_body is nil or not JSON. Safely parse and branch on ITEM_NOT_FOUND without risking another exception in the rescue path.
Apply this diff:
- rescue Plaid::ApiError => e - json_response = JSON.parse(e.response_body) + rescue Plaid::ApiError => e + json_response = begin + JSON.parse(e.response_body.to_s) + rescue JSON::ParserError + {} + endapp/models/account.rb (1)
60-63: Bug: create_and_sync overwrites provided cash_balanceattributes.merge(cash_balance: attributes[:balance]) unconditionally overwrites any precomputed cash_balance (e.g., for Investment accounts), negating the BalanceCalculator’s result.
Apply:
- account = new(attributes.merge(cash_balance: attributes[:balance])) + # Only default cash_balance to balance when not explicitly provided + attributes = attributes.reverse_merge(cash_balance: attributes[:balance]) + account = new(attributes)app/controllers/simplefin_items_controller.rb (1)
190-194: render_error renders :new unconditionally; allow choosing :edit for update flows.Make the view configurable and stop overriding @simplefin_item on update errors.
- def render_error(message, setup_token = nil) - @simplefin_item = Current.family.simplefin_items.build(setup_token: setup_token) - @error_message = message - render :new, status: :unprocessable_entity - end + def render_error(message, setup_token = nil, context: :new) + @error_message = message + if context == :edit + # Preserve the existing record so form_with routes to update + @simplefin_item.setup_token = setup_token if @simplefin_item + render :edit, status: :unprocessable_entity + else + @simplefin_item = Current.family.simplefin_items.build(setup_token: setup_token) + render :new, status: :unprocessable_entity + end + endNote: With this change, ensure update action calls pass context: :edit (see prior comments).
🧹 Nitpick comments (21)
app/models/provider/simplefin.rb (1)
1-94: Align with Provider base conventions (with_provider_response, inheritance)Per guidelines, concrete providers should inherit from Provider and wrap operations with with_provider_response for consistent error handling. This class currently includes HTTParty directly and handles errors ad hoc.
I recommend:
- Inherit from Provider (if available in the codebase) and centralize error translation in with_provider_response.
- Add Accept/Content-Type headers for JSON endpoints and use HTTParty’s format: :json to ensure correct parsing and error signaling.
I can propose a refactor diff once we confirm the Provider base API.
app/models/simplefin_account/investments/balance_calculator.rb (1)
1-47: Consider reusing parsing helpers to avoid duplicationHoldingsProcessor already implements parse_decimal with logging. Extract a shared helper (e.g., SimplefinAccount::Investments::Parsing) and use it here to keep behavior consistent between processors and calculators.
I can draft a small concern/module and refactor both classes to use it if you’d like.
app/views/simplefin_items/setup_accounts.html.erb (1)
86-87: Add ARIA announcement for better accessibility of loading stateConsider adding an ARIA live region on the submit button so screen readers announce the status change when the label switches to a spinner.
Apply:
<%= render DS::Button.new( text: "Create Accounts", variant: "primary", icon: "plus", type: "submit", - class: "flex-1", - data: { loading_button_target: "button" } + class: "flex-1", + aria: { live: "polite" }, + data: { loading_button_target: "button" } ) %>config/locales/views/simplefin_items/en.yml (1)
28-28: Add trailing newlineYAML linter flags missing newline at EOF. Add one to satisfy tooling.
- syncing: Syncing... + syncing: Syncing... +app/models/simplefin_account/liabilities/loan_processor.rb (1)
22-24: Remove or improve the informal comment.The comment acknowledges uncertainty about SimpleFin's loan metadata capabilities. Consider either removing it or making it more actionable for future development.
- # I don't know if SimpleFin typically provide detailed loan metadata - # like interest rates, terms, etc. but we can update what's available + # TODO: Implement loan-specific attribute processing when SimpleFin + # provides metadata like interest rates, terms, payment schedulesapp/views/simplefin_items/edit.html.erb (1)
47-58: Localize button and link text.Aligns with new translations and keeps copy consistent.
<%= render DS::Button.new( - text: "Update Connection", + text: t(".actions.update"), variant: "primary", icon: "refresh-cw", type: "submit", class: "flex-1" ) %> <%= render DS::Link.new( - text: "Cancel", + text: t(".actions.cancel"), variant: "secondary", href: accounts_path ) %>app/models/simplefin_account/liabilities/credit_processor.rb (2)
25-29: Avoid saving when nothing changed; guard save! with a change check.Small optimization and avoids unnecessary writes.
- account.accountable.available_credit = credit_limit if credit_limit > 0 - account.accountable.save! + if credit_limit > 0 && account.accountable.available_credit != credit_limit + account.accountable.available_credit = credit_limit + account.accountable.save! + end
32-46: DRY parse_decimal across processors.parse_decimal duplicates logic found in other processors (e.g., Investments::HoldingsProcessor). Extract to a shared utility (e.g., SimplefinAccount::Parsing.decimal(value)).
If keeping local, LGTM; otherwise consider introducing:
- app/models/simplefin_account/parsing.rb with decimal(value)
- Replace local parse_decimal calls across processors.
app/models/simplefin_account/importer.rb (1)
34-49: Avoid recomputing balance for non-investments; compute cash from known balance.Minor perf/readability improvement.
- def update_balance_information - # Update balance based on account type - balance = calculate_balance - cash_balance = calculate_cash_balance + def update_balance_information + # Update balance based on account type + balance = calculate_balance + cash_balance = if account.accountable_type == "Investment" + balance_calculator.cash_balance + else + balance + endapp/controllers/simplefin_items_controller.rb (1)
18-21: Normalize input token (strip) and localize the error string.Trim whitespace to avoid false negatives; error should be translated.
- setup_token = simplefin_params[:setup_token] + setup_token = simplefin_params[:setup_token].to_s.strip - - return render_error("Please enter a SimpleFin setup token.") if setup_token.blank? + return render_error(t(".errors.missing_token"), setup_token, context: :edit) if setup_token.blank?app/models/simplefin_item.rb (2)
89-96: Avoid unnecessary includes(:account)The method doesn’t use the associated Account; drop the includes to reduce query work.
- simplefin_accounts.includes(:account) + simplefin_accounts .where.not(org_data: nil) .map { |acc| acc.org_data } .uniq { |org| org["domain"] || org["name"] }
68-76: Method name with bang implies persistenceupsert_institution_data! doesn’t save; the bang name suggests a dangerous/persistent operation. Either persist in this method or drop the bang.
Would you like me to prepare a refactor to persist here and adjust callers?
app/models/simplefin_account/processor.rb (1)
12-15: Duplicate nil-account guards; consolidate and add logging onceYou return early here, and process_account! also guards/logs. Because of the early return, the later log is unreachable. Consolidate the check here and log once.
- unless simplefin_account.account.present? - return - end + unless simplefin_account.account.present? + Rails.logger.error("SimpleFin account #{simplefin_account.id} has no associated Account - this should not happen after manual setup") + return + endAnd remove the duplicate guard/logging inside process_account!.
app/models/simplefin_account/transactions/processor.rb (1)
30-31: Remove unused currency parameter from parse_amountThe currency argument isn’t used; simplify signature and call site.
- amount = parse_amount(data[:amount], account.currency) + amount = parse_amount(data[:amount]) @@ - def parse_amount(amount_value, currency) + def parse_amount(amount_value)Also applies to: 65-72
app/models/simplefin_account/investments/holdings_processor.rb (1)
78-92: Handle float UNIX timestamps in parse_holding_dateSome providers emit timestamps as floats. Support Float alongside Integer.
- case created_timestamp - when Integer + case created_timestamp + when Integer, Float Time.at(created_timestamp).to_dateapp/models/simplefin_account/investments/transactions_processor.rb (3)
8-17: Consider adding error handling for the entire processing loop.While individual transaction processing has error handling, the outer loop could benefit from a rescue block to ensure partial failures don't halt processing of subsequent transactions.
def process return unless simplefin_account.account&.accountable_type == "Investment" return unless simplefin_account.raw_transactions_payload.present? transactions_data = simplefin_account.raw_transactions_payload + begin transactions_data.each do |transaction_data| process_investment_transaction(transaction_data) end + rescue => e + Rails.logger.error("Failed to process SimpleFin investment transactions batch: #{e.message}") + raise + end end
29-32: Consider implementing trade detection logic.The comment indicates future enhancement for detecting trade-specific patterns. This would be valuable for proper investment tracking.
Would you like me to help implement logic to detect and create Trade entries for investment-specific transaction patterns (e.g., buy/sell operations)? I can open a new issue to track this enhancement.
76-93: Consider making date parsing more lenient for investment transactions.The date parsing raises an exception which could halt processing of all subsequent transactions. Consider returning a default date or the current date with a warning log instead.
def parse_date(date_value) case date_value when String Date.parse(date_value) when Integer, Float Time.at(date_value).to_date when Time, DateTime date_value.to_date when Date date_value else Rails.logger.error("SimpleFin investment transaction has invalid date value: #{date_value.inspect}") - raise ArgumentError, "Invalid date format: #{date_value.inspect}" + Rails.logger.warn("Using current date as fallback for invalid date: #{date_value.inspect}") + Date.current end rescue ArgumentError, TypeError => e Rails.logger.error("Failed to parse SimpleFin investment transaction date '#{date_value}': #{e.message}") - raise ArgumentError, "Unable to parse transaction date: #{date_value.inspect}" + Rails.logger.warn("Using current date as fallback for unparseable date: #{date_value.inspect}") + Date.current endapp/models/simplefin_item/importer.rb (3)
13-26: Good error handling for authentication issues.The rescue block properly handles authentication errors by marking the item as requiring update. However, the re-raise pattern on lines 22-24 could be simplified.
rescue Provider::Simplefin::SimplefinError => e # Handle authentication errors by marking item as requiring update if e.error_type == :access_forbidden simplefin_item.update!(status: :requires_update) - raise e - else - raise e end + raise
28-32: Consider logging errors before returning.When errors are present in the response, the method returns early after handling errors. Consider adding a log statement for better observability.
# Handle errors if present in response if accounts_data[:errors] && accounts_data[:errors].any? + Rails.logger.warn("SimpleFin import encountered errors: #{accounts_data[:errors].inspect}") handle_errors(accounts_data[:errors]) return end
71-81: Consider extracting account type normalization.The account_type assignment on line 73 uses multiple fallbacks. Consider extracting this to a helper method for clarity.
+ def normalize_account_type(account_data) + account_data["type"] || account_data[:type] || "unknown" + end + # Update all attributes including transactions simplefin_account.assign_attributes( name: account_data[:name], - account_type: account_data["type"] || account_data[:type] || "unknown", + account_type: normalize_account_type(account_data), currency: account_data[:currency] || "USD",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
app/controllers/simplefin_items_controller.rb(6 hunks)app/javascript/controllers/loading_button_controller.js(1 hunks)app/models/account.rb(1 hunks)app/models/concerns/syncable.rb(1 hunks)app/models/plaid_item.rb(1 hunks)app/models/provider/simplefin.rb(2 hunks)app/models/simplefin_account/importer.rb(1 hunks)app/models/simplefin_account/investments/balance_calculator.rb(1 hunks)app/models/simplefin_account/investments/holdings_processor.rb(1 hunks)app/models/simplefin_account/investments/transactions_processor.rb(1 hunks)app/models/simplefin_account/liabilities/credit_processor.rb(1 hunks)app/models/simplefin_account/liabilities/loan_processor.rb(1 hunks)app/models/simplefin_account/processor.rb(1 hunks)app/models/simplefin_account/transactions/processor.rb(1 hunks)app/models/simplefin_item.rb(2 hunks)app/models/simplefin_item/importer.rb(4 hunks)app/models/simplefin_item/syncer.rb(1 hunks)app/views/simplefin_items/_simplefin_item.html.erb(3 hunks)app/views/simplefin_items/edit.html.erb(1 hunks)app/views/simplefin_items/setup_accounts.html.erb(2 hunks)config/locales/views/simplefin_items/en.yml(1 hunks)config/routes.rb(1 hunks)db/migrate/20250813014953_remove_institution_fields_from_simplefin_items.rb(1 hunks)db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb(1 hunks)db/schema.rb(2 hunks)test/controllers/simplefin_items_controller_test.rb(1 hunks)test/models/simplefin_item_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (27)
db/migrate/**/*.rb
📄 CodeRabbit Inference Engine (.cursor/rules/project-conventions.mdc)
Enforce simple validations (NOT NULL, unique indexes) at the database level
Files:
db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rbdb/migrate/20250813014953_remove_institution_fields_from_simplefin_items.rb
{app,lib}/**/*.{rb,erb}
📄 CodeRabbit Inference Engine (CLAUDE.md)
{app,lib}/**/*.{rb,erb}: Use Current.user for the authenticated user; do not use current_user
Use Current.family for the current family; do not use current_family
Files:
app/models/plaid_item.rbapp/models/simplefin_account/liabilities/credit_processor.rbapp/views/simplefin_items/edit.html.erbapp/models/concerns/syncable.rbapp/models/simplefin_account/liabilities/loan_processor.rbapp/models/simplefin_account/investments/balance_calculator.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/account.rbapp/models/simplefin_item/syncer.rbapp/models/simplefin_account/transactions/processor.rbapp/models/provider/simplefin.rbapp/models/simplefin_account/importer.rbapp/views/simplefin_items/setup_accounts.html.erbapp/controllers/simplefin_items_controller.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/investments/transactions_processor.rbapp/views/simplefin_items/_simplefin_item.html.erbapp/models/simplefin_item/importer.rbapp/models/simplefin_item.rb
{app/**/*.rb,app/**/*.erb}
📄 CodeRabbit Inference Engine (.cursor/rules/general-rules.mdc)
{app/**/*.rb,app/**/*.erb}: UseCurrent.userfor the current user; do not usecurrent_userin Rails code and templates
UseCurrent.familyfor the current family; do not usecurrent_familyin Rails code and templates
Files:
app/models/plaid_item.rbapp/models/simplefin_account/liabilities/credit_processor.rbapp/views/simplefin_items/edit.html.erbapp/models/concerns/syncable.rbapp/models/simplefin_account/liabilities/loan_processor.rbapp/models/simplefin_account/investments/balance_calculator.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/account.rbapp/models/simplefin_item/syncer.rbapp/models/simplefin_account/transactions/processor.rbapp/models/provider/simplefin.rbapp/models/simplefin_account/importer.rbapp/views/simplefin_items/setup_accounts.html.erbapp/controllers/simplefin_items_controller.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/investments/transactions_processor.rbapp/views/simplefin_items/_simplefin_item.html.erbapp/models/simplefin_item/importer.rbapp/models/simplefin_item.rb
app/models/**/{*,**/*.rb}
📄 CodeRabbit Inference Engine (.cursor/rules/project-conventions.mdc)
Use Rails concerns and POROs organized around model traits in app/models and app/models/concerns
Files:
app/models/plaid_item.rbapp/models/simplefin_account/liabilities/credit_processor.rbapp/models/concerns/syncable.rbapp/models/simplefin_account/liabilities/loan_processor.rbapp/models/simplefin_account/investments/balance_calculator.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/account.rbapp/models/simplefin_item/syncer.rbapp/models/simplefin_account/transactions/processor.rbapp/models/provider/simplefin.rbapp/models/simplefin_account/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/investments/transactions_processor.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_item.rb
app/models/**/*.rb
📄 CodeRabbit Inference Engine (.cursor/rules/project-conventions.mdc)
app/models/**/*.rb: Models should answer questions about themselves (e.g., Account#balance_series) rather than external service objects
Use ActiveRecord validations for complex validations and business logic (may mirror DB constraints for UX)Domain models should not call Provider::Registry directly; instead, add a Provided concern within the model’s namespace to choose providers and expose convenience methods (e.g., app/models//provided.rb).
Files:
app/models/plaid_item.rbapp/models/simplefin_account/liabilities/credit_processor.rbapp/models/concerns/syncable.rbapp/models/simplefin_account/liabilities/loan_processor.rbapp/models/simplefin_account/investments/balance_calculator.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/account.rbapp/models/simplefin_item/syncer.rbapp/models/simplefin_account/transactions/processor.rbapp/models/provider/simplefin.rbapp/models/simplefin_account/importer.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/investments/transactions_processor.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_item.rb
{app/helpers/**/*.rb,app/models/**/*.rb,app/views/**/*.html.erb}
📄 CodeRabbit Inference Engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates server-side (helpers/models) and pass formatted values to Stimulus for display only
Files:
app/models/plaid_item.rbapp/models/simplefin_account/liabilities/credit_processor.rbapp/views/simplefin_items/edit.html.erbapp/models/concerns/syncable.rbapp/models/simplefin_account/liabilities/loan_processor.rbapp/models/simplefin_account/investments/balance_calculator.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/account.rbapp/models/simplefin_item/syncer.rbapp/models/simplefin_account/transactions/processor.rbapp/models/provider/simplefin.rbapp/models/simplefin_account/importer.rbapp/views/simplefin_items/setup_accounts.html.erbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/investments/transactions_processor.rbapp/views/simplefin_items/_simplefin_item.html.erbapp/models/simplefin_item/importer.rbapp/models/simplefin_item.rb
{app/models/**/*.rb,app/controllers/**/*.rb}
📄 CodeRabbit Inference Engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/models/plaid_item.rbapp/models/simplefin_account/liabilities/credit_processor.rbapp/models/concerns/syncable.rbapp/models/simplefin_account/liabilities/loan_processor.rbapp/models/simplefin_account/investments/balance_calculator.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/account.rbapp/models/simplefin_item/syncer.rbapp/models/simplefin_account/transactions/processor.rbapp/models/provider/simplefin.rbapp/models/simplefin_account/importer.rbapp/controllers/simplefin_items_controller.rbapp/models/simplefin_account/processor.rbapp/models/simplefin_account/investments/transactions_processor.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_item.rb
app/javascript/controllers/**/*.{js,ts}
📄 CodeRabbit Inference Engine (CLAUDE.md)
app/javascript/controllers/**/*.{js,ts}: Keep Stimulus controllers lightweight and simple (fewer than 7 targets), single-responsibility, with a clear public API and private internals
Global Stimulus controllers must live in app/javascript/controllers/
Files:
app/javascript/controllers/loading_button_controller.js
app/javascript/**/*.{js,ts}
📄 CodeRabbit Inference Engine (CLAUDE.md)
app/javascript/**/*.{js,ts}: Prefer query params for UI state instead of localStorage/sessionStorage
Perform currency/number/date formatting on the server; avoid client-side formatting
Files:
app/javascript/controllers/loading_button_controller.js
{app/javascript/**/*.js,app/controllers/**/*.rb}
📄 CodeRabbit Inference Engine (.cursor/rules/project-conventions.mdc)
Prefer query params in the URL for UI state over localStorage/sessionStorage; use DB only if persistence is necessary
Files:
app/javascript/controllers/loading_button_controller.jsapp/controllers/simplefin_items_controller.rb
app/javascript/**/*.js
📄 CodeRabbit Inference Engine (.cursor/rules/project-conventions.mdc)
Avoid implementing currency/number/date formatting in client-side Stimulus/JS; keep controllers for display behavior
Files:
app/javascript/controllers/loading_button_controller.js
{app/javascript/controllers/**/*.@(js|ts),app/components/**/*.@(js|ts),app/views/**/*.html.erb,app/components/**/*.html.erb}
📄 CodeRabbit Inference Engine (.cursor/rules/stimulus_conventions.mdc)
Use declarative Stimulus actions in ERB (data-action) instead of imperative event listeners in controllers (e.g., avoid addEventListener in connect/initializers).
Files:
app/javascript/controllers/loading_button_controller.jsapp/views/simplefin_items/edit.html.erbapp/views/simplefin_items/setup_accounts.html.erbapp/views/simplefin_items/_simplefin_item.html.erb
{app/javascript/controllers/**/*.@(js|ts),app/components/**/*.@(js|ts)}
📄 CodeRabbit Inference Engine (.cursor/rules/stimulus_conventions.mdc)
{app/javascript/controllers/**/*.@(js|ts),app/components/**/*.@(js|ts)}: Keep Stimulus controllers lightweight: prefer fewer than 7 targets; use private methods and expose a clear public API.
Keep Stimulus controllers focused: no domain logic in controllers; aim for single responsibility; leverage Stimulus callbacks, actions, targets, values, and classes.
Files:
app/javascript/controllers/loading_button_controller.js
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit Inference Engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Author styles assuming TailwindCSS v4.x is in use across the app
Always start by referencing app/assets/tailwind/maybe-design-system.css to understand the base primitives, functional tokens, and component tokens
Prefer functional tokens from maybe-design-system.css over raw colors/utilities (e.g., usetext-primaryovertext-white,bg-containeroverbg-white,border border-primaryoverborder border-gray-200)
Files:
app/javascript/controllers/loading_button_controller.jsapp/views/simplefin_items/edit.html.erbapp/views/simplefin_items/setup_accounts.html.erbapp/views/simplefin_items/_simplefin_item.html.erb
app/javascript/controllers/**/*_controller.js
📄 CodeRabbit Inference Engine (.cursor/rules/view_conventions.mdc)
Global Stimulus controllers in app/javascript/controllers/ can be used across any view
Files:
app/javascript/controllers/loading_button_controller.js
{app/javascript/controllers/**/*_controller.js,app/components/**/*_controller.js}
📄 CodeRabbit Inference Engine (.cursor/rules/view_conventions.mdc)
Use Stimulus targets to reference DOM elements instead of manual document.getElementById calls
Files:
app/javascript/controllers/loading_button_controller.js
test/**/*_test.rb
📄 CodeRabbit Inference Engine (CLAUDE.md)
test/**/*_test.rb: Use VCR for external API testing
Use the mocha gem for stubs and mocks in tests
Files:
test/controllers/simplefin_items_controller_test.rbtest/models/simplefin_item_test.rb
test/**/*.rb
📄 CodeRabbit Inference Engine (.cursor/rules/testing.mdc)
test/**/*.rb: Write tests with Minitest; do not use RSpec in this codebase
Use fixtures for test data; do not use factories
For tests needing many records, use Rails helpers to create them and inline the creation in the test (e.g., helpers in test/support)
Test boundaries correctly: for query methods, assert returned output; for command methods, assert the command was invoked with correct params
Do not test one class’s implementation details in another class’s test suite
Use the mocha gem for stubs and mocks
Prefer OpenStruct for simple mock instances, or define a small mock class for complex cases
Only mock what’s necessary; don’t mock return values you don’t assert on
Files:
test/controllers/simplefin_items_controller_test.rbtest/models/simplefin_item_test.rb
{app/views,app/components}/**/*.html.erb
📄 CodeRabbit Inference Engine (CLAUDE.md)
{app/views,app/components}/**/*.html.erb: In views, always use the icon helper; never call lucide_icon directly
Prefer semantic HTML and server-rendered Hotwire patterns (Turbo) over client-side JS widgets
Use Tailwind functional tokens (e.g., text-primary, bg-container, border-primary) instead of raw color utilities like text-white or bg-white
Keep domain/business logic out of view templates; views should primarily render data
Stimulus controllers must use declarative actions (data-action and data--target) rather than inline JavaScript
Pass data to Stimulus via data--value attributes; do not embed inline JavaScript
Files:
app/views/simplefin_items/edit.html.erbapp/views/simplefin_items/setup_accounts.html.erbapp/views/simplefin_items/_simplefin_item.html.erb
app/views/**/*.html.erb
📄 CodeRabbit Inference Engine (.cursor/rules/project-conventions.mdc)
app/views/**/*.html.erb: Prefer native semantic HTML elements over custom JS components (e.g., for modals,/for disclosures)
Use Turbo Frames to segment pages instead of JS-driven client-side solutions where possible
Prefer client-side form validation when possible for UX
app/views/**/*.html.erb: Use ViewComponents when the element has complex logic/styling, is reused across contexts, needs variants/sizes or slots, requires interactivity/Stimulus, or needs accessibility/ARIA support
Use partials when the element is primarily static HTML with minimal logic, used in few contexts, simple template content, has no variants/configuration, and is mainly for content organization
Prefer components over partials: if a suitable component exists in app/components, use it; otherwise consider a partial or create a component based on the criteria
Integrate Stimulus declaratively in views: HTML declares actions/targets; controllers respond (avoid imperative JS in templates)
Files:
app/views/simplefin_items/edit.html.erbapp/views/simplefin_items/setup_accounts.html.erbapp/views/simplefin_items/_simplefin_item.html.erb
{app/views/**/*.{html,html+turbo-stream}.erb,app/helpers/**/*.rb}
📄 CodeRabbit Inference Engine (.cursor/rules/project-conventions.mdc)
Always use the icon helper (icon) from application_helper.rb; never call lucide_icon directly
Files:
app/views/simplefin_items/edit.html.erbapp/views/simplefin_items/setup_accounts.html.erbapp/views/simplefin_items/_simplefin_item.html.erb
{app/components/**/*.@(js|ts),app/views/**/*.html.erb,app/components/**/*.html.erb}
📄 CodeRabbit Inference Engine (.cursor/rules/stimulus_conventions.mdc)
Component Stimulus controllers under app/components must only be used within their component views, not in app/views.
Files:
app/views/simplefin_items/edit.html.erbapp/views/simplefin_items/setup_accounts.html.erbapp/views/simplefin_items/_simplefin_item.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit Inference Engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/simplefin_items/edit.html.erbapp/views/simplefin_items/setup_accounts.html.erbapp/views/simplefin_items/_simplefin_item.html.erb
{app/views/**/*.html.erb,app/components/**/*.html.erb}
📄 CodeRabbit Inference Engine (.cursor/rules/view_conventions.mdc)
{app/views/**/*.html.erb,app/components/**/*.html.erb}: Keep domain logic out of views; compute classes and logic in component/view helpers, not inside ERB templates
Pass data from Rails to Stimulus via data-*-value attributes; do not embed inline JavaScript in templates
Files:
app/views/simplefin_items/edit.html.erbapp/views/simplefin_items/setup_accounts.html.erbapp/views/simplefin_items/_simplefin_item.html.erb
app/models/provider/{!(concepts)/**/*.rb,!(registry).rb}
📄 CodeRabbit Inference Engine (.cursor/rules/project-design.mdc)
Concrete provider implementations must live under the Provider:: namespace, inherit from Provider, wrap operations with with_provider_response, and raise on invalid data.
Files:
app/models/provider/simplefin.rb
app/views/**/_*.html.erb
📄 CodeRabbit Inference Engine (.cursor/rules/view_conventions.mdc)
Name partials with an underscore prefix (e.g., _trend_change.html.erb, _form_errors.html.erb)
Files:
app/views/simplefin_items/_simplefin_item.html.erb
app/views/!(shared)/**/_*.html.erb
📄 CodeRabbit Inference Engine (.cursor/rules/view_conventions.mdc)
Place context-specific partials in the relevant controller's view directory (not in shared/)
Files:
app/views/simplefin_items/_simplefin_item.html.erb
🧠 Learnings (7)
📚 Learning: 2025-08-12T01:00:40.409Z
Learnt from: CR
PR: we-promise/sure#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-12T01:00:40.409Z
Learning: Applies to app/javascript/controllers/**/*.{js,ts} : Keep Stimulus controllers lightweight and simple (fewer than 7 targets), single-responsibility, with a clear public API and private internals
Applied to files:
app/javascript/controllers/loading_button_controller.js
📚 Learning: 2025-08-12T01:03:58.034Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-08-12T01:03:58.034Z
Learning: Applies to {app/javascript/controllers/**/*.@(js|ts),app/components/**/*.@(js|ts)} : Keep Stimulus controllers lightweight: prefer fewer than 7 targets; use private methods and expose a clear public API.
Applied to files:
app/javascript/controllers/loading_button_controller.js
📚 Learning: 2025-08-12T01:03:58.034Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-08-12T01:03:58.034Z
Learning: Applies to {app/javascript/controllers/**/*.@(js|ts),app/components/**/*.@(js|ts)} : Keep Stimulus controllers focused: no domain logic in controllers; aim for single responsibility; leverage Stimulus callbacks, actions, targets, values, and classes.
Applied to files:
app/javascript/controllers/loading_button_controller.js
📚 Learning: 2025-08-12T01:05:24.245Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-08-12T01:05:24.245Z
Learning: Applies to app/javascript/controllers/**/*_controller.js : Global Stimulus controllers in app/javascript/controllers/ can be used across any view
Applied to files:
app/javascript/controllers/loading_button_controller.js
📚 Learning: 2025-08-12T01:05:24.245Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-08-12T01:05:24.245Z
Learning: Applies to app/components/**/*_controller.js : Component-scoped Stimulus controllers in app/components/ should only be used within their component templates
Applied to files:
app/javascript/controllers/loading_button_controller.js
📚 Learning: 2025-08-12T01:05:24.245Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-08-12T01:05:24.245Z
Learning: Applies to app/views/**/*.html.erb : Integrate Stimulus declaratively in views: HTML declares actions/targets; controllers respond (avoid imperative JS in templates)
Applied to files:
app/javascript/controllers/loading_button_controller.js
📚 Learning: 2025-08-12T01:01:56.633Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/general-rules.mdc:0-0
Timestamp: 2025-08-12T01:01:56.633Z
Learning: Ignore i18n methods and files; hardcode strings in English for now to optimize development speed
Applied to files:
config/locales/views/simplefin_items/en.yml
🧬 Code Graph Analysis (16)
app/models/simplefin_account/liabilities/credit_processor.rb (3)
app/models/simplefin_account/importer.rb (3)
initialize(2-79)initialize(3-5)account(20-22)app/models/simplefin_account/liabilities/loan_processor.rb (4)
initialize(2-32)initialize(3-5)process(7-12)account(17-19)app/models/concerns/accountable.rb (1)
extend(1-36)
app/models/concerns/syncable.rb (1)
app/models/family.rb (2)
sync(107-115)needs_sync?(117-119)
app/models/simplefin_account/liabilities/loan_processor.rb (8)
app/models/simplefin_account/importer.rb (3)
initialize(2-79)initialize(3-5)account(20-22)app/models/simplefin_account/investments/balance_calculator.rb (2)
initialize(3-47)initialize(4-6)app/models/simplefin_account/liabilities/credit_processor.rb (4)
initialize(2-47)initialize(3-5)process(7-12)account(17-19)app/models/account/current_balance_manager.rb (1)
current_balance(16-23)lib/money/arithmetic.rb (1)
abs(41-43)db/migrate/20240202191425_create_account_loans.rb (2)
change(2-6)change(1-7)db/migrate/20240202192333_create_account_other_liabilities.rb (2)
change(2-6)change(1-7)app/models/concerns/accountable.rb (1)
extend(1-36)
app/models/simplefin_account/investments/balance_calculator.rb (3)
app/models/simplefin_account/importer.rb (2)
initialize(2-79)initialize(3-5)app/models/simplefin_account/investments/holdings_processor.rb (3)
initialize(1-109)initialize(2-4)holdings_data(64-68)app/models/account/current_balance_manager.rb (1)
current_balance(16-23)
app/models/simplefin_account/investments/holdings_processor.rb (3)
app/models/simplefin_account/liabilities/credit_processor.rb (3)
process(7-12)account(17-19)parse_decimal(32-46)app/models/simplefin_account/investments/transactions_processor.rb (2)
process(8-17)account(22-24)app/models/trade/create_form.rb (1)
security(22-29)
db/migrate/20250813014953_remove_institution_fields_from_simplefin_items.rb (3)
db/migrate/20240203030754_remove_type_from_accounts.rb (2)
change(1-5)change(2-4)db/migrate/20240209174912_redo_money_storage.rb (2)
change(1-11)change(2-10)db/migrate/20240206031739_replace_money_field.rb (2)
change(1-7)change(2-6)
app/models/account.rb (4)
app/controllers/accounts_controller.rb (1)
family(67-69)app/models/assistant/function/get_accounts.rb (2)
name(1-40)name(3-5)db/migrate/20240209174912_redo_money_storage.rb (2)
change(1-11)change(2-10)db/migrate/20240206031739_replace_money_field.rb (2)
change(2-6)change(1-7)
app/models/simplefin_item/syncer.rb (3)
app/controllers/simplefin_items_controller.rb (1)
sync(103-112)app/models/simplefin_item.rb (2)
import_latest_simplefin_data(32-34)process_accounts(36-40)app/models/family.rb (1)
sync(107-115)
app/models/simplefin_account/transactions/processor.rb (3)
app/models/simplefin_account/processor.rb (1)
process(11-20)app/models/simplefin_account/liabilities/credit_processor.rb (2)
process(7-12)account(17-19)app/models/simplefin_account/investments/transactions_processor.rb (4)
process(8-17)account(22-24)parse_amount(59-74)parse_date(76-93)
app/models/simplefin_account/importer.rb (8)
app/models/simplefin_account/transactions/processor.rb (2)
initialize(4-6)attr_reader(1-103)app/models/simplefin_account/investments/balance_calculator.rb (4)
initialize(3-47)initialize(4-6)balance(8-11)cash_balance(13-22)app/models/simplefin_account/investments/holdings_processor.rb (3)
initialize(1-109)initialize(2-4)account(60-62)app/models/simplefin_account/liabilities/credit_processor.rb (3)
initialize(2-47)initialize(3-5)account(17-19)app/models/simplefin_account/liabilities/loan_processor.rb (3)
initialize(2-32)initialize(3-5)account(17-19)app/models/concerns/enrichable.rb (1)
enrich_attributes(34-51)app/models/account/current_balance_manager.rb (1)
current_balance(16-23)lib/money/arithmetic.rb (1)
abs(41-43)
app/views/simplefin_items/setup_accounts.html.erb (3)
db/migrate/20240209174912_redo_money_storage.rb (2)
change(1-11)change(2-10)db/migrate/20240202015428_create_accounts.rb (1)
change(2-15)db/migrate/20240203030754_remove_type_from_accounts.rb (1)
change(1-5)
app/controllers/simplefin_items_controller.rb (4)
app/models/family/simplefin_connectable.rb (1)
create_simplefin_item!(12-24)app/models/account.rb (1)
destroy_later(149-152)app/models/simplefin_item.rb (1)
destroy_later(27-30)app/models/concerns/syncable.rb (1)
sync_later(14-40)
app/models/simplefin_account/processor.rb (5)
app/models/simplefin_account/transactions/processor.rb (1)
process(8-21)app/models/plaid_account/processor.rb (5)
process(14-19)process_account!(31-75)process_transactions(77-81)process_investments(83-88)process_liabilities(90-101)app/models/account/current_balance_manager.rb (1)
current_balance(16-23)app/controllers/concerns/accountable_resource.rb (1)
accountable_type(73-75)lib/money/arithmetic.rb (1)
abs(41-43)
app/models/simplefin_account/investments/transactions_processor.rb (9)
app/models/simplefin_account/transactions/processor.rb (4)
initialize(4-6)process(8-21)parse_amount(65-82)parse_date(84-102)app/models/simplefin_account/importer.rb (3)
initialize(2-79)initialize(3-5)account(20-22)app/models/simplefin_account/investments/balance_calculator.rb (2)
initialize(3-47)initialize(4-6)app/models/simplefin_account/investments/holdings_processor.rb (4)
initialize(1-109)initialize(2-4)process(6-55)account(60-62)app/controllers/concerns/accountable_resource.rb (1)
accountable_type(73-75)app/models/plaid_item/accounts_snapshot.rb (1)
transactions_data(74-81)app/models/plaid_entry/processor.rb (1)
amount(64-66)db/migrate/20240202192238_create_account_investments.rb (2)
change(1-7)change(2-6)db/migrate/20240202191746_add_accountable_to_account.rb (1)
change(2-6)
app/models/simplefin_item/importer.rb (3)
app/models/simplefin_item/provided.rb (1)
simplefin_provider(4-6)app/models/provider/simplefin.rb (1)
get_accounts(27-73)app/models/account.rb (1)
start_date(175-178)
app/models/simplefin_item.rb (2)
app/models/plaid_item/accounts_snapshot.rb (1)
accounts(10-12)app/models/account.rb (1)
institution_domain(134-147)
🪛 YAMLlint (1.37.1)
config/locales/views/simplefin_items/en.yml
[error] 28-28: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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 (33)
app/models/plaid_item.rb (1)
102-103: Good guard to avoid nil provider errorsEarly-return prevents NoMethodError when plaid_provider is absent while still allowing destroy to proceed. This is safe with Rails’ before_destroy in modern versions (requires throw(:abort) to halt).
db/schema.rb (1)
13-13: Schema version bump looks correctNo action needed.
app/views/simplefin_items/setup_accounts.html.erb (2)
15-19: Good declarative Stimulus + Turbo targetingThe form correctly uses declarative Stimulus actions and targets _top via data-turbo-frame. This aligns with our Stimulus/Turbo guidelines.
12-20: Verify interaction between local: true and Turbo form submissionIn some Rails/Turbo setups, form_with local: true may disable Turbo enhancements. Since you’re relying on data-turbo-frame="_top" and a submit Stimulus action, ensure Turbo is still handling the submission. If Turbo is not intercepting, remove local: true.
Run this quick check and adjust accordingly:
- Observe in DevTools Network whether the form is submitted as a fetch/XHR (Turbo) vs. full page load.
- If it’s a full page load, remove local: true.
Apply this diff if needed:
- local: true, + # local: true, # Let Turbo handle submissionconfig/routes.rb (1)
254-260: Routes extension for SimpleFin items looks correctAdding edit/update and the complete_account_setup member route matches the new setup/edit flows described in the PR. No routing concerns here.
app/models/account.rb (1)
100-101: accounts.cash_balance column exists and is correctly typedVerified that
accounts.cash_balanceis defined indb/schema.rb(precision: 19, scale: 4, default: "0.0") and was added by20241204235400_add_balance_components.rb. No schema changes needed.db/migrate/20250813014953_remove_institution_fields_from_simplefin_items.rb (1)
1-8: LGTM! Clean migration following Rails best practices.The migration properly removes the obsolete institution fields as described in the PR objectives. The structure follows the established pattern in the codebase and the field types are correctly specified for proper rollback capability.
test/models/simplefin_item_test.rb (4)
65-81: LGTM! Comprehensive test coverage for institution data upsetting.The test properly verifies all the expected field mappings from the org_data hash to the SimplefinItem attributes. The test data structure matches what would be expected from SimpleFin API responses.
83-94: LGTM! Good fallback logic testing for display names.The test covers all three scenarios: no institution data (fallback to item name), with institution name, and domain fallback when name is nil. This ensures robust display behavior.
122-147: LGTM! Thorough testing of institution summary logic.The test covers all edge cases: no institutions, single institution, and multiple institutions. The implementation properly handles the different summary formats based on count.
96-120: org_data field verified on SimplefinAccount
Theorg_datacolumn is present indb/schema.rb(added via20250808143007_add_extra_simplefin_account_fields.rb) and is mapped in theSimplefinAccountmodel (org_data: snapshot[:org]). The test’s references toorg_dataare valid—no changes required.app/models/concerns/syncable.rb (1)
23-26: LGTM! Safe parent assignment with proper guards.The implementation correctly:
- Checks for both
parent_syncpresence and absence of existing parent- Uses
update!to ensure validation failures cause rollback- Maintains the existing transaction/lock context
This change aligns with the multi-phase sync orchestration mentioned in the PR objectives.
app/models/simplefin_item/syncer.rb (5)
9-11: LGTM! Safe status updates with proper guard.The status update is properly guarded with
respond_to?(:status_text)to avoid errors if the sync object doesn't support status text.
14-15: LGTM! Clear phase separation and status reporting.The phase-based approach with descriptive status messages improves user experience by providing visibility into sync progress.
17-24: LGTM! Improved early return handling with status feedback.The logic properly:
- Sets pending_account_setup flag for unlinked accounts
- Provides user feedback via status text
- Clears the flag when all accounts are linked
26-28: LGTM! Consistent phase pattern continues.Phase 3 follows the established pattern with status updates and delegates to the new modular processing system.
30-36: LGTM! Final phase with proper sync orchestration.The final phase correctly passes the parent sync and window parameters to enable the parent-child sync relationships as implemented in the Syncable concern.
app/models/simplefin_account/liabilities/loan_processor.rb (1)
1-12: LGTM! Clean processor structure following established patterns.The class follows the same initialization and processing pattern as other account processors in the codebase (like CreditProcessor). The early return guard ensures it only processes Loan accounts.
app/views/simplefin_items/edit.html.erb (1)
1-9: Localize visible strings in the header.Match the PR’s I18N push; move copy into locales and reference via t().
Example:
-<% content_for :title, "Update SimpleFin Connection" %> +<% content_for :title, t(".title") %> - <% dialog.with_header(title: "Update SimpleFin Connection") do %> + <% dialog.with_header(title: t(".title")) do %> <div class="flex items-center gap-2"> <%= icon "building-2", class: "text-primary" %> - <span class="text-primary">Get a new setup token to reconnect your SimpleFin account</span> + <span class="text-primary"><%= t(".subtitle") %></span> </div>Follow up by adding the keys under views/simplefin_items/edit.*.yml.
⛔ Skipped due to learnings
Learnt from: CR PR: we-promise/sure#0 File: .cursor/rules/general-rules.mdc:0-0 Timestamp: 2025-08-12T01:01:56.633Z Learning: Ignore i18n methods and files; hardcode strings in English for now to optimize development speedapp/models/simplefin_account/liabilities/credit_processor.rb (1)
21-29: Ignore fallback foravailable_balancepayload key
The SimpleFin importer and tests consistently use the"available-balance"key onraw_payload, sodig("available-balance")is correct. There’s no evidence of a snake_case"available_balance"key in the payload.• app/models/simplefin_item/importer.rb: uses
account_data[:"available-balance"]
• test/models/simplefin_account_test.rb: raw_payload fixture has"available-balance" => 1800.0No changes required.
Likely an incorrect or invalid review comment.
test/controllers/simplefin_items_controller_test.rb (1)
76-82: Error rendering on update correctly uses the:newtemplate
Therender_errorhelper inSimplefinItemsController#updateinvokesrender :new, status: :unprocessable_entity, and your test rightly asserts the “new” form copy. No change is needed unless you opt to switch to rendering the:edittemplate on update failures. If you do, update this test accordingly. For example:- patch simplefin_item_url(@simplefin_item), params: { - simplefin_item: { setup_token: "" } - } - - assert_response :unprocessable_entity - assert_includes response.body, "Please enter a SimpleFin setup token" + patch simplefin_item_url(@simplefin_item), params: { + simplefin_item: { setup_token: "" } + } + + assert_response :unprocessable_entity + assert_includes response.body, "Update SimpleFin Connection" # edit page copy + assert_includes response.body, "Please enter a SimpleFin setup token"app/models/simplefin_account/importer.rb (1)
51-65: Balance sign normalization looks good.Ensures liabilities have positive balances. Mirror this logic anywhere else balances are assigned to avoid regressions.
app/controllers/simplefin_items_controller.rb (1)
103-111: Sync flow looks good; HTML+JSON responses supported and idempotent.Not restarting an in-flight sync and redirecting back to accounts_path is appropriate.
app/models/simplefin_item.rb (2)
37-39: Only processing accounts with associated Account looks goodJoining on :account avoids nil-account edge cases downstream.
97-106: Use I18N for institution_summary stringsHard-coded English strings in a model will bypass localization. Prefer I18n.t with proper keys, and include count for pluralization.
- case institutions.count - when 0 - "No institutions connected" - when 1 - institutions.first["name"] || institutions.first["domain"] || "1 institution" - else - "#{institutions.count} institutions" - end + count = institutions.count + case count + when 0 + I18n.t("simplefin_items.institution_summary.none") + when 1 + institutions.first["name"].presence || + institutions.first["domain"].presence || + I18n.t("simplefin_items.institution_summary.one") + else + I18n.t("simplefin_items.institution_summary.many", count: count) + endIf you prefer keeping copy decisions in the view layer, return a data structure here and render the message via translations in the view.
⛔ Skipped due to learnings
Learnt from: CR PR: we-promise/sure#0 File: .cursor/rules/general-rules.mdc:0-0 Timestamp: 2025-08-12T01:01:56.633Z Learning: Ignore i18n methods and files; hardcode strings in English for now to optimize development speedapp/models/simplefin_account/processor.rb (1)
32-40: Liability balance normalization is correctCoercing CreditCard/Loan balances to positive matches the app’s expectations for liabilities.
app/models/simplefin_account/investments/holdings_processor.rb (1)
41-50: LGTM: transactional save with cleanupSaving then pruning future-dated holdings for the same security keeps data consistent after updates.
app/models/simplefin_account/investments/transactions_processor.rb (3)
1-7: LGTM! Clean initialization pattern.The class structure follows Rails conventions with proper namespace organization for investment-specific processing.
33-54: Good implementation with proper deduplication.The transaction processing correctly:
- Uses external_id for deduplication via plaid_id
- Creates appropriate Entry and Transaction records
- Handles currency from the account
59-74: Confirm and align SimpleFin amount sign conventionsThe
parse_amountinapp/models/simplefin_account/investments/transactions_processor.rb(lines 59–74) currently negates every value based on a “banking convention” comment, whereas the generalparse_amount(amount_value, currency)inapp/models/simplefin_account/transactions/processor.rbdoes not invert the sign.• app/models/simplefin_account/investments/transactions_processor.rb (lines 59–74)
• app/models/simplefin_account/transactions/processor.rb (theparse_amount(amount_value, currency)method)Please verify against the SimpleFin API documentation whether credit/debit amounts should be negated, then ensure both implementations—and accompanying tests—consistently reflect the correct convention.
app/models/simplefin_item/importer.rb (3)
57-89: Good defensive programming with duplicate prevention.The account import logic has proper safeguards:
- Early return for blank account_id (line 61)
- Final validation before save (lines 84-86)
- Using assign_attributes for atomic updates
The implementation effectively prevents duplicate account creation.
92-113: Comprehensive error detection logic.The error handling properly:
- Extracts messages from various error formats
- Detects authentication issues through multiple patterns
- Updates item status when authentication is needed
Good defensive programming with support for both string and hash error formats.
45-50: Review rationale for 20-year initial sync windowPlease confirm that using
20.years.agoas the fallback fordetermine_sync_start_datealigns with both data availability (most banks cap at ~2 years of history) and your users’ expectations—this will help avoid unnecessary lookups, performance impacts, and excessive data storage.• File: app/models/simplefin_item/importer.rb
Lines: 45–50
db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb
Outdated
Show resolved
Hide resolved
db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Himmelschmidt <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb (1)
3-8: Avoid remove-then-add: this will irreversibly drop existing institution dataThis migration re-adds columns that a prior migration removed. Running them sequentially deletes existing values. Prefer a safe rename/backfill strategy or amend the earlier migration.
Options:
- Amend 20250813014953_remove_institution_fields_from_simplefin_items.rb to rename old columns (e.g., old_institution_id), add new ones, backfill, then drop old ones.
- Or replace that removal migration with a single migration that adds new columns and backfills in-place.
I can draft the amended migration. Would you like me to open a follow-up PR with a reversible, data-preserving migration plan?
🧹 Nitpick comments (1)
db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb (1)
7-7: Constrain color to expected hex length (optional)If storing a hex like #RRGGBB, add a length limit (7). Consider a CHECK constraint if you want stricter validation.
Apply:
- add_column :simplefin_items, :institution_color, :string + add_column :simplefin_items, :institution_color, :string, limit: 7Optional stricter validation:
execute "ALTER TABLE simplefin_items ADD CONSTRAINT institution_color_hex CHECK (institution_color ~ '^#?[0-9A-Fa-f]{6}$')"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
db/migrate/**/*.rb
📄 CodeRabbit Inference Engine (.cursor/rules/project-conventions.mdc)
Enforce simple validations (NOT NULL, unique indexes) at the database level
Files:
db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb
⏰ 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 (1)
db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb (1)
8-8: Good choice using jsonbjsonb enables efficient indexing/operators and is generally preferred in Postgres.
db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb
Outdated
Show resolved
Hide resolved
db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb
Outdated
Show resolved
Hide resolved
- Add loadingText Stimulus value for configurable loading text - Remove unused originalText variable - Update view to pass loading text via data attribute
This class was added in the PR but is never called anywhere in the codebase. The actual SimpleFin account processing is handled by SimplefinAccount::Processor and its specialized sub-processors.
- Call import_latest_simplefin_data before account transfer to ensure new SimpleFin accounts exist with account_id populated - Prevents silent failure where accounts become orphaned when refreshing expired SimpleFin tokens
- Update render_error method to accept context parameter for template selection - Fix update action to render :edit template instead of :new on errors - Replace hardcoded error messages with localized strings using t() calls - Add comprehensive error message keys to SimpleFin locale file
- Add aria-disabled and aria-busy attributes for screen readers - Use semantic span elements instead of divs for button content - Add aria-hidden to decorative spinner element
- Add raw_holdings_payload column to simplefin_accounts table - Separates holdings data from general account data for cleaner processing - Follows same pattern as raw_transactions_payload for consistency - Enables proper SimpleFin holdings processing pipeline
- Add external_id and cost_basis columns to holdings table - Update holdings processor to use external_id for precise matching - Capture all available SimpleFin holdings data (shares, market_value, cost_basis, etc.) - Use SimpleFin holding ID as external_id with "simplefin_" prefix - Calculate price from market_value/shares when available - Store raw holdings payload in simplefin_accounts for complete data retention This enables better holdings tracking than composite key approach and ensures we capture all SimpleFin data even if not immediately used in our models.
- Add MerchantDetector that uses payee field directly for merchant creation - Enhance SimpleFin entry name generation combining payee + description - Remove transaction processor category matching logic - Create dedicated SimpleFin entry processor Uses SimpleFin's clean payee data without unnecessary filtering.
- Add source field to ProviderMerchant model for provider-specific merchant tracking - Fix DataEnrichment to handle string transaction IDs correctly with to_i conversion Enables per-provider merchant deduplication and fixes transaction lookups.
- Convert string account_ids to integers for proper account lookup - Ensure account selection works correctly with form submissions Fixes account filtering when setting up SimpleFin sync.
FYI, I've now changed this to chunked syncing system with a 3 year max historical data sync. SimpleFin returns an error of "365 days" but the real limit seems to be 60. They also have a 24 requests per day limit, so I think limiting to 3 years of historical data is fair |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
app/views/simplefin_items/_simplefin_item.html.erb (1)
21-24: Localized “deletion in progress” — looks goodThis addresses prior feedback to localize the label.
app/models/simplefin_item.rb (1)
57-64: Indifferent access fix for snapshot — good defensive hardeningNormalizing keys prevents silent misses when provider parsing changes.
test/controllers/simplefin_items_controller_test.rb (1)
51-79: Great—test now returns a different item to exercise rebind/delete.This resolves the prior masking of destroy/rebind behavior and asserts scheduled_for_deletion on the original. Nice.
🧹 Nitpick comments (38)
app/models/provider_merchant.rb (1)
2-2: Mirror uniqueness at the DB layer (partial unique index for STI)The model validates
nameuniqueness scoped to:source, but without a DB constraint duplicates can slip in under concurrency. For STI onmerchants, consider a partial unique index:# db/migrate/XXXXXX_add_partial_unique_index_to_provider_merchants.rb class AddPartialUniqueIndexToProviderMerchants < ActiveRecord::Migration[7.2] def change add_index :merchants, [:name, :source], unique: true, where: "type = 'ProviderMerchant'", name: "index_provider_merchants_on_name_source_unique" end enddb/migrate/20250901004029_add_sync_start_date_to_simplefin_items.rb (1)
3-3: Consider indexingsync_start_date(and confirm nullability is intended)If queries filter by this date (e.g., for backfills or chunking), add an index:
def change - add_column :simplefin_items, :sync_start_date, :date + add_column :simplefin_items, :sync_start_date, :date + add_index :simplefin_items, :sync_start_date endAlso confirm
NULLis acceptable; if not, addnull: falseplus a backfill.db/migrate/20250901005519_add_sync_stats_to_syncs.rb (1)
3-3: Preferjsonbovertextforsync_stats
jsonbgives native casting, validation, and indexing options. Since this is a new migration in this PR, consider switching now:- add_column :syncs, :sync_stats, :text + add_column :syncs, :sync_stats, :jsonb, default: {}, null: falseIf this migration already ran in any env, use a follow-up migration to convert with
USING sync_stats::jsonb.app/models/sync.rb (1)
24-25: Avoidserializeif the column isjsonb; ensure sane defaults if staying on text
- If you switch the column to
jsonb(recommended), drop the serializer and rely on AR’s native JSON casting:- serialize :sync_stats, coder: JSON + # jsonb column: native casting; no serializer needed
- If you keep
text+ JSON, set a default to avoidnilsurprises:# inside Sync after_initialize { self.sync_stats ||= {} }db/migrate/20250901182423_add_raw_holdings_payload_to_simplefin_accounts.rb (1)
3-3: Default to an empty array and disallow NULL for raw_holdings_payloadPrevents nil checks across the codebase and aligns with other payload defaults.
- add_column :simplefin_accounts, :raw_holdings_payload, :jsonb + add_column :simplefin_accounts, :raw_holdings_payload, :jsonb, default: [], null: falsedb/schema.rb (3)
703-703: Consider default and NOT NULL on raw_holdings_payloadSetting default: [] and null: false simplifies processors and avoids nil branches.
722-724: Optional: add light constraints for institution metadata
- institution_domain is fine as string; if you’ll query/filter by it, consider an index.
- institution_color could use a CHECK for hex format if you want guardrails.
758-758: Should sync_stats be structured (jsonb) instead of text?If you’ll aggregate or query stats, prefer jsonb with default {}.
app/models/simplefin_account/investments/holdings_processor.rb (6)
32-33: Prefer account currency as fallback over hard-coded USDUse the account’s currency when SimpleFin omits currency.
- h.currency = simplefin_holding["currency"] || "USD" + h.currency = simplefin_holding["currency"].presence || account&.currency || "USD" @@ - currency: simplefin_holding["currency"] || "USD", + currency: simplefin_holding["currency"].presence || account&.currency || "USD",Also applies to: 50-51
81-87: Pass exchange MIC to Security::Resolver when encoded in symbolSimpleFin symbols may arrive as "TICKER|EXCHANGE_MIC". Parse and pass MIC to improve resolution accuracy.
- def resolve_security(symbol, description) - # Use Security::Resolver to find or create the security - Security::Resolver.new(symbol).resolve + def resolve_security(symbol, description) + ticker_symbol, exchange_mic = symbol.to_s.split("|", 2) + Security::Resolver.new( + ticker_symbol, + exchange_operating_mic: exchange_mic + ).resolve
26-34: Minor: avoid duplicate assignment of invariant fieldssecurity/date/currency are set in the initializer block and then reassigned. Keep one source of truth.
- holding.assign_attributes( - security: security, - date: holding_date, - currency: simplefin_holding["currency"] || "USD", + holding.assign_attributes( + currency: simplefin_holding["currency"].presence || account&.currency || "USD", qty: qty, price: price, amount: market_value, cost_basis: cost_basis )Also applies to: 47-55
89-99: Expand date parsing to handle Float epoch secondsCovers providers sending epoch as float.
- case created_timestamp - when Integer + case created_timestamp + when Integer, Float Time.at(created_timestamp).to_date
76-79: Optional: simplify once raw_holdings_payload is non-nullIf you set default []/null: false on the column, you can drop the
|| [].
1-68: Align BalanceCalculator to use raw_holdings_payloadBalanceCalculator currently sums from raw_payload["holdings"]; with this PR you introduced raw_holdings_payload. Update it to read the dedicated column to keep cash calculation accurate.
I can prep a small patch to switch BalanceCalculator to raw_holdings_payload and add a fallback for legacy data. Want me to draft it?
config/locales/views/simplefin_items/en.yml (5)
47-47: Add missing trailing newline.Fix YAMLlint warning.
Apply:
- update: Update connection + update: Update connection +
13-13: Punctuate destroy success for consistency.Most messages end with punctuation; this one should too.
- success: SimpleFin connection will be removed + success: SimpleFin connection will be removed.
24-26: Tighten help text phrasing.Clearer description of expected token.
- help_text: "The token should be a long string starting with letters and numbers" + help_text: "The token is a long alphanumeric string from SimpleFin Bridge."
30-31: Normalize button/title casing.Use consistent Title Case for actions.
- add_new: Add new connection + add_new: Add New Connection - delete: Delete + delete: Delete - setup_action: Set Up New Accounts + setup_action: Set Up New AccountsAlso applies to: 34-35, 42-42
43-47: Add i18n keys for sync phase messages used in Syncer.Prepares for removing hard-coded strings in syncer.
status_with_summary: "Last synced %{timestamp} ago • %{summary}" syncing: Syncing... update: Update connection + sync_phases: + importing: "Importing accounts from SimpleFin..." + checking: "Checking account configuration..." + processing: "Processing transactions and holdings..." + calculating: "Calculating balances..."app/models/simplefin_account/transactions/merchant_detector.rb (2)
10-11: Leverage indifferent_access directly.Simplify to a single lookup.
- payee = (transaction_data[:payee] || transaction_data["payee"])&.strip + payee = transaction_data[:payee]&.strip
25-28: Prefer SHA256 over MD5 for deterministic IDs.Low risk here, but SHA256 avoids MD5 baggage with negligible cost.
- "simplefin_#{Digest::MD5.hexdigest(merchant_name.downcase)}" + "simplefin_#{Digest::SHA256.hexdigest(merchant_name.downcase)}"Add at top of file if not already loaded elsewhere:
+require "digest/sha2"app/models/simplefin_item/syncer.rb (3)
10-11: Use i18n for status messages.Aligns with new locale keys.
- sync.update!(status_text: "Importing accounts from SimpleFin...") if sync.respond_to?(:status_text) + sync.update!(status_text: I18n.t("simplefin_items.sync_phases.importing")) if sync.respond_to?(:status_text) @@ - sync.update!(status_text: "Checking account configuration...") if sync.respond_to?(:status_text) + sync.update!(status_text: I18n.t("simplefin_items.sync_phases.checking")) if sync.respond_to?(:status_text) @@ - sync.update!(status_text: "#{unlinked_accounts.count} accounts need setup...") if sync.respond_to?(:status_text) + sync.update!(status_text: I18n.t("helpers.number.nth", default: "%{count} accounts need setup...", count: unlinked_accounts.count) % { count: unlinked_accounts.count }) if sync.respond_to?(:status_text) @@ - sync.update!(status_text: "Processing transactions and holdings...") if sync.respond_to?(:status_text) + sync.update!(status_text: I18n.t("simplefin_items.sync_phases.processing")) if sync.respond_to?(:status_text) @@ - sync.update!(status_text: "Calculating balances...") if sync.respond_to?(:status_text) + sync.update!(status_text: I18n.t("simplefin_items.sync_phases.calculating")) if sync.respond_to?(:status_text)Also applies to: 14-15, 29-31, 36-37, 40-45
15-24: Avoid extra queries; compute counts once.Removes materializing relations and duplicate COUNT/ANY queries.
- total_accounts = simplefin_item.simplefin_accounts.count - linked_accounts = simplefin_item.simplefin_accounts.joins(:account) - unlinked_accounts = simplefin_item.simplefin_accounts.includes(:account).where(accounts: { id: nil }) + total_accounts = simplefin_item.simplefin_accounts.count + linked_count = simplefin_item.simplefin_accounts.joins(:account).count + unlinked_count = total_accounts - linked_count @@ - sync_stats = { - total_accounts: total_accounts, - linked_accounts: linked_accounts.count, - unlinked_accounts: unlinked_accounts.count - } + sync_stats = { total_accounts: total_accounts, linked_accounts: linked_count, unlinked_accounts: unlinked_count } @@ - if unlinked_accounts.any? + if unlinked_count.positive? simplefin_item.update!(pending_account_setup: true) - sync.update!(status_text: "#{unlinked_accounts.count} accounts need setup...") if sync.respond_to?(:status_text) + sync.update!(status_text: "#{unlinked_count} accounts need setup...") if sync.respond_to?(:status_text) else simplefin_item.update!(pending_account_setup: false) end @@ - if linked_accounts.any? + if linked_count.positive? sync.update!(status_text: "Processing transactions and holdings...") if sync.respond_to?(:status_text) simplefin_item.process_accounts @@ - simplefin_item.schedule_account_syncs( + simplefin_item.schedule_account_syncs( parent_sync: sync, window_start_date: sync.window_start_date, window_end_date: sync.window_end_date ) endAlso applies to: 27-46
8-11: DRY the status update guard.Introduce a tiny helper to reduce repetition.
def perform_sync(sync) - # Phase 1: Import data from SimpleFin API - sync.update!(status_text: "Importing accounts from SimpleFin...") if sync.respond_to?(:status_text) + # Phase 1: Import data from SimpleFin API + update_status(sync, I18n.t("simplefin_items.sync_phases.importing")) @@ - sync.update!(status_text: "Checking account configuration...") if sync.respond_to?(:status_text) + update_status(sync, I18n.t("simplefin_items.sync_phases.checking")) @@ - sync.update!(status_text: "Processing transactions and holdings...") if sync.respond_to?(:status_text) + update_status(sync, I18n.t("simplefin_items.sync_phases.processing")) @@ - sync.update!(status_text: "Calculating balances...") if sync.respond_to?(:status_text) + update_status(sync, I18n.t("simplefin_items.sync_phases.calculating")) @@ - if sync.respond_to?(:sync_stats) - sync.update!(sync_stats: sync_stats) - end + sync.update!(sync_stats: sync_stats) if sync.respond_to?(:sync_stats) end @@ def perform_post_sync # no-op end + + private + def update_status(sync, text) + sync.update!(status_text: text) if sync.respond_to?(:status_text) + endAlso applies to: 13-15, 34-46, 48-51
app/models/simplefin_entry/processor.rb (1)
36-37: Transaction scope unnecessary for a single record saveThis block wraps one
save!. Unless you’re coordinating multiple writes, drop the transaction to reduce locking and noise.- def process - SimplefinAccount.transaction do + def process entry = account.entries.find_or_initialize_by(plaid_id: external_id) do |e| e.entryable = Transaction.new end ... entry.save! - end endapp/views/simplefin_items/_simplefin_item.html.erb (1)
9-17: Add alt text to institution logo for accessibilityProvide meaningful
alttext; fall back to display name.- <%= image_tag simplefin_item.logo, class: "rounded-full h-full w-full", loading: "lazy" %> + <%= image_tag simplefin_item.logo, + class: "rounded-full h-full w-full", + loading: "lazy", + alt: simplefin_item.institution_display_name %>app/models/simplefin_item.rb (4)
68-77: Normalize org data and derive domain from URL when missingSupport mixed keys and backfill domain from URL (
urlorsfin-url) to improve display consistency.- def upsert_institution_data!(org_data) - org = org_data.to_h.with_indifferent_access - assign_attributes( - institution_id: org[:id], - institution_name: org[:name], - institution_domain: org[:domain], - institution_url: org[:url] || org[:"sfin-url"], - raw_institution_payload: org_data - ) - end + def upsert_institution_data!(org_data) + org = org_data.to_h.with_indifferent_access + url = org[:url] || org[:"sfin-url"] || org["sfin-url"] + domain = org[:domain].presence + if domain.blank? && url.present? + begin + domain = URI.parse(url).host&.gsub(/^www\./, "") + rescue URI::InvalidURIError + Rails.logger.warn("Invalid SimpleFin institution URL: #{url.inspect}") + end + end + + assign_attributes( + institution_id: org[:id], + institution_name: org[:name], + institution_domain: domain, + institution_url: url, + raw_institution_payload: org + ) + end
85-117: Localize sync summary strings and support pluralizationThe method returns English strings; move to I18n for consistency and future locales.
- if total == 0 - "No accounts found" - elsif unlinked == 0 - "#{linked} #{'account'.pluralize(linked)} synced" - else - "#{linked} synced, #{unlinked} need setup" - end + return I18n.t("simplefin_items.sync_summary.none") if total == 0 + return I18n.t("simplefin_items.sync_summary.all_synced", count: linked) if unlinked == 0 + I18n.t("simplefin_items.sync_summary.partial", linked: linked, unlinked: unlinked)Add equivalent changes in the fallback branch.
124-131: Make connected institutions key-insensitive and dedupe robustlyEnsure consistent keys and dedupe by normalized domain or name.
- simplefin_accounts.includes(:account) - .where.not(org_data: nil) - .map { |acc| acc.org_data } - .uniq { |org| org["domain"] || org["name"] } + simplefin_accounts.includes(:account) + .where.not(org_data: nil) + .map { |acc| acc.org_data.to_h.with_indifferent_access } + .uniq { |org| (org[:domain].presence || org[:name]).to_s.downcase }
132-141: Localize institution summary outputThese strings are user-facing; move to I18n.
- case institutions.count - when 0 - "No institutions connected" - when 1 - institutions.first["name"] || institutions.first["domain"] || "1 institution" - else - "#{institutions.count} institutions" - end + count = institutions.count + return I18n.t("simplefin_items.institutions.none") if count.zero? + return (institutions.first["name"] || institutions.first["domain"] || I18n.t("simplefin_items.institutions.one")) if count == 1 + I18n.t("simplefin_items.institutions.many", count: count)test/controllers/simplefin_items_controller_test.rb (2)
81-90: Prefer asserting via I18n to avoid brittle literals.Instead of a hardcoded string, assert on the translated key to keep tests stable across copy changes.
Apply:
- assert_includes response.body, "Please enter a SimpleFin setup token" + assert_includes response.body, I18n.t("simplefin_items.update.errors.blank_setup_token")
92-197: Solid end-to-end transfer test; add an expectation for the creation call.To fully exercise the contract and avoid unintended DB paths, also expect/create_simplefin_item! with the token and item name.
Apply:
+ @family.expects(:create_simplefin_item!).with( + setup_token: "valid_token", + item_name: @simplefin_item.name + ).returns(SimplefinItem.create!(family: @family, name: "Replacement Connection", access_url: "https://example.com/new_access"))app/models/simplefin_item/importer.rb (6)
10-13: Useful observability—keep, but consider reducing to debug level.Startup logging is fine, but INFO may be noisy in production.
Apply:
- Rails.logger.info "SimplefinItem::Importer - Starting import for item #{simplefin_item.id}" - Rails.logger.info "SimplefinItem::Importer - last_synced_at: #{simplefin_item.last_synced_at.inspect}" - Rails.logger.info "SimplefinItem::Importer - sync_start_date: #{simplefin_item.sync_start_date.inspect}" + Rails.logger.debug "SimplefinItem::Importer - Starting import for item #{simplefin_item.id}" + Rails.logger.debug "SimplefinItem::Importer - last_synced_at: #{simplefin_item.last_synced_at.inspect}" + Rails.logger.debug "SimplefinItem::Importer - sync_start_date: #{simplefin_item.sync_start_date.inspect}"
27-33: Parameterize chunk sizing and request limits.Hard-coded 60-day chunks and 22 requests couple code to current provider quirks. Surface these as constants or configuration to adapt without code changes.
Apply:
- chunk_size_days = 60 - max_requests = 22 - current_end_date = Time.current + chunk_size_days = Rails.configuration.simplefin.fetch(:chunk_size_days, 60) + max_requests = Rails.configuration.simplefin.fetch(:max_daily_requests, 22) + current_end_date = Time.zone.now
73-75: Remove stdout debug prints; use Rails logger.puts will spam workers and tests. Prefer Rails.logger.debug.
Apply:
- puts "DEBUG: About to call API with start_date=#{chunk_start_date} end_date=#{chunk_end_date} (#{actual_days} days)" + Rails.logger.debug "SimpleFin chunked sync: start=#{chunk_start_date} end=#{chunk_end_date} (#{actual_days}d)"and
- puts "DEBUG: API call - start_date=#{start_date}, end_date=#{end_date}, days=#{days_requested}" + Rails.logger.debug "SimpleFin API call - start=#{start_date} end=#{end_date} days=#{days_requested}"Also applies to: 121-123
118-147: Good auth handling; minor cleanup.The rescue branches both re-raise; collapse to a single branch and keep the status update for :access_forbidden.
Apply:
- rescue Provider::Simplefin::SimplefinError => e - # Handle authentication errors by marking item as requiring update - if e.error_type == :access_forbidden - simplefin_item.update!(status: :requires_update) - raise e - else - raise e - end - end + rescue Provider::Simplefin::SimplefinError => e + simplefin_item.update!(status: :requires_update) if e.error_type == :access_forbidden + raise + end
149-159: First-sync logic is dead code here; keep it focused.determine_sync_start_date’s first-sync branch isn’t used (first sync goes through chunked path). Either remove the branch or ensure callers use it consistently.
Apply:
- unless simplefin_item.last_synced_at - return initial_sync_lookback_period.days.ago - end + # Not used during first sync (handled by chunked history).
232-240: Align initial lookback helper with 3-year goal and give units.Rename for clarity and return days matching usage, or switch callers to use durations directly.
Apply either:
-def initial_sync_lookback_period - # Default to 7 days for initial sync to avoid API limits - 7 -end +def initial_sync_lookback_period + # 3 years in days + 365 * 3 +endor change callers to use a duration:
- default_start_date = initial_sync_lookback_period.days.ago + default_start_date = 3.years.ago.beginning_of_day
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
app/controllers/simplefin_items_controller.rb(6 hunks)app/models/data_enrichment.rb(1 hunks)app/models/provider_merchant.rb(1 hunks)app/models/simplefin_account/investments/holdings_processor.rb(1 hunks)app/models/simplefin_account/investments/transactions_processor.rb(1 hunks)app/models/simplefin_account/transactions/merchant_detector.rb(1 hunks)app/models/simplefin_account/transactions/processor.rb(1 hunks)app/models/simplefin_entry/processor.rb(1 hunks)app/models/simplefin_item.rb(2 hunks)app/models/simplefin_item/importer.rb(2 hunks)app/models/simplefin_item/syncer.rb(1 hunks)app/models/sync.rb(1 hunks)app/views/simplefin_items/_simplefin_item.html.erb(3 hunks)app/views/simplefin_items/edit.html.erb(1 hunks)app/views/simplefin_items/setup_accounts.html.erb(3 hunks)config/locales/views/simplefin_items/en.yml(1 hunks)db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb(1 hunks)db/migrate/20250901004029_add_sync_start_date_to_simplefin_items.rb(1 hunks)db/migrate/20250901005519_add_sync_stats_to_syncs.rb(1 hunks)db/migrate/20250901182423_add_raw_holdings_payload_to_simplefin_accounts.rb(1 hunks)db/migrate/20250901183328_add_external_id_and_cost_basis_to_holdings.rb(1 hunks)db/schema.rb(6 hunks)test/controllers/simplefin_items_controller_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- app/models/simplefin_account/investments/transactions_processor.rb
- db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb
- app/views/simplefin_items/setup_accounts.html.erb
- app/models/simplefin_account/transactions/processor.rb
- app/controllers/simplefin_items_controller.rb
- app/views/simplefin_items/edit.html.erb
🧰 Additional context used
📓 Path-based instructions (19)
{app,lib}/**/*.{rb,erb}
📄 CodeRabbit inference engine (CLAUDE.md)
{app,lib}/**/*.{rb,erb}: Use Current.user for the authenticated user; do not use current_user
Use Current.family for the current family; do not use current_family
Files:
app/models/data_enrichment.rbapp/models/provider_merchant.rbapp/models/sync.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/transactions/merchant_detector.rbapp/views/simplefin_items/_simplefin_item.html.erbapp/models/simplefin_item.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/syncer.rb
{app/**/*.rb,app/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
{app/**/*.rb,app/**/*.erb}: UseCurrent.userfor the current user; do not usecurrent_userin Rails code and templates
UseCurrent.familyfor the current family; do not usecurrent_familyin Rails code and templates
Files:
app/models/data_enrichment.rbapp/models/provider_merchant.rbapp/models/sync.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/transactions/merchant_detector.rbapp/views/simplefin_items/_simplefin_item.html.erbapp/models/simplefin_item.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/syncer.rb
app/models/**/{*,**/*.rb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use Rails concerns and POROs organized around model traits in app/models and app/models/concerns
Files:
app/models/data_enrichment.rbapp/models/provider_merchant.rbapp/models/sync.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/transactions/merchant_detector.rbapp/models/simplefin_item.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/syncer.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/models/**/*.rb: Models should answer questions about themselves (e.g., Account#balance_series) rather than external service objects
Use ActiveRecord validations for complex validations and business logic (may mirror DB constraints for UX)Domain models should not call Provider::Registry directly; instead, add a Provided concern within the model’s namespace to choose providers and expose convenience methods (e.g., app/models//provided.rb).
Files:
app/models/data_enrichment.rbapp/models/provider_merchant.rbapp/models/sync.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/transactions/merchant_detector.rbapp/models/simplefin_item.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/syncer.rb
{app/helpers/**/*.rb,app/models/**/*.rb,app/views/**/*.html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates server-side (helpers/models) and pass formatted values to Stimulus for display only
Files:
app/models/data_enrichment.rbapp/models/provider_merchant.rbapp/models/sync.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/transactions/merchant_detector.rbapp/views/simplefin_items/_simplefin_item.html.erbapp/models/simplefin_item.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/syncer.rb
{app/models/**/*.rb,app/controllers/**/*.rb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/models/data_enrichment.rbapp/models/provider_merchant.rbapp/models/sync.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/transactions/merchant_detector.rbapp/models/simplefin_item.rbapp/models/simplefin_item/importer.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/syncer.rb
db/migrate/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Enforce simple validations (NOT NULL, unique indexes) at the database level
Files:
db/migrate/20250901183328_add_external_id_and_cost_basis_to_holdings.rbdb/migrate/20250901005519_add_sync_stats_to_syncs.rbdb/migrate/20250901004029_add_sync_start_date_to_simplefin_items.rbdb/migrate/20250901182423_add_raw_holdings_payload_to_simplefin_accounts.rb
test/**/*_test.rb
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*_test.rb: Use VCR for external API testing
Use the mocha gem for stubs and mocks in tests
Files:
test/controllers/simplefin_items_controller_test.rb
test/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
test/**/*.rb: Write tests with Minitest; do not use RSpec in this codebase
Use fixtures for test data; do not use factories
For tests needing many records, use Rails helpers to create them and inline the creation in the test (e.g., helpers in test/support)
Test boundaries correctly: for query methods, assert returned output; for command methods, assert the command was invoked with correct params
Do not test one class’s implementation details in another class’s test suite
Use the mocha gem for stubs and mocks
Prefer OpenStruct for simple mock instances, or define a small mock class for complex cases
Only mock what’s necessary; don’t mock return values you don’t assert on
Files:
test/controllers/simplefin_items_controller_test.rb
{app/views,app/components}/**/*.html.erb
📄 CodeRabbit inference engine (CLAUDE.md)
{app/views,app/components}/**/*.html.erb: In views, always use the icon helper; never call lucide_icon directly
Prefer semantic HTML and server-rendered Hotwire patterns (Turbo) over client-side JS widgets
Use Tailwind functional tokens (e.g., text-primary, bg-container, border-primary) instead of raw color utilities like text-white or bg-white
Keep domain/business logic out of view templates; views should primarily render data
Stimulus controllers must use declarative actions (data-action and data--target) rather than inline JavaScript
Pass data to Stimulus via data--value attributes; do not embed inline JavaScript
Files:
app/views/simplefin_items/_simplefin_item.html.erb
app/views/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/views/**/*.html.erb: Prefer native semantic HTML elements over custom JS components (e.g., for modals,/for disclosures)
Use Turbo Frames to segment pages instead of JS-driven client-side solutions where possible
Prefer client-side form validation when possible for UX
app/views/**/*.html.erb: Use ViewComponents when the element has complex logic/styling, is reused across contexts, needs variants/sizes or slots, requires interactivity/Stimulus, or needs accessibility/ARIA support
Use partials when the element is primarily static HTML with minimal logic, used in few contexts, simple template content, has no variants/configuration, and is mainly for content organization
Prefer components over partials: if a suitable component exists in app/components, use it; otherwise consider a partial or create a component based on the criteria
Integrate Stimulus declaratively in views: HTML declares actions/targets; controllers respond (avoid imperative JS in templates)
Files:
app/views/simplefin_items/_simplefin_item.html.erb
{app/views/**/*.{html,html+turbo-stream}.erb,app/helpers/**/*.rb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Always use the icon helper (icon) from application_helper.rb; never call lucide_icon directly
Files:
app/views/simplefin_items/_simplefin_item.html.erb
{app/javascript/controllers/**/*.@(js|ts),app/components/**/*.@(js|ts),app/views/**/*.html.erb,app/components/**/*.html.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Use declarative Stimulus actions in ERB (data-action) instead of imperative event listeners in controllers (e.g., avoid addEventListener in connect/initializers).
Files:
app/views/simplefin_items/_simplefin_item.html.erb
{app/components/**/*.@(js|ts),app/views/**/*.html.erb,app/components/**/*.html.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Component Stimulus controllers under app/components must only be used within their component views, not in app/views.
Files:
app/views/simplefin_items/_simplefin_item.html.erb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Author styles assuming TailwindCSS v4.x is in use across the app
Always start by referencing app/assets/tailwind/maybe-design-system.css to understand the base primitives, functional tokens, and component tokens
Prefer functional tokens from maybe-design-system.css over raw colors/utilities (e.g., usetext-primaryovertext-white,bg-containeroverbg-white,border border-primaryoverborder border-gray-200)
Files:
app/views/simplefin_items/_simplefin_item.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/simplefin_items/_simplefin_item.html.erb
{app/views/**/*.html.erb,app/components/**/*.html.erb}
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
{app/views/**/*.html.erb,app/components/**/*.html.erb}: Keep domain logic out of views; compute classes and logic in component/view helpers, not inside ERB templates
Pass data from Rails to Stimulus via data-*-value attributes; do not embed inline JavaScript in templates
Files:
app/views/simplefin_items/_simplefin_item.html.erb
app/views/**/_*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Name partials with an underscore prefix (e.g., _trend_change.html.erb, _form_errors.html.erb)
Files:
app/views/simplefin_items/_simplefin_item.html.erb
app/views/!(shared)/**/_*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Place context-specific partials in the relevant controller's view directory (not in shared/)
Files:
app/views/simplefin_items/_simplefin_item.html.erb
🧠 Learnings (6)
📚 Learning: 2025-08-12T01:01:56.633Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/general-rules.mdc:0-0
Timestamp: 2025-08-12T01:01:56.633Z
Learning: Ignore i18n methods and files; hardcode strings in English for now to optimize development speed
Applied to files:
config/locales/views/simplefin_items/en.yml
📚 Learning: 2025-08-12T01:04:37.152Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/ui-ux-design-guidelines.mdc:0-0
Timestamp: 2025-08-12T01:04:37.152Z
Learning: Applies to {app/views/**,app/helpers/**,app/javascript/controllers/**} : Prefer functional tokens from maybe-design-system.css over raw colors/utilities (e.g., use `text-primary` over `text-white`, `bg-container` over `bg-white`, `border border-primary` over `border border-gray-200`)
Applied to files:
app/views/simplefin_items/_simplefin_item.html.erb
📚 Learning: 2025-08-12T01:00:40.409Z
Learnt from: CR
PR: we-promise/sure#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-12T01:00:40.409Z
Learning: Applies to {app/views,app/components}/**/*.html.erb : Use Tailwind functional tokens (e.g., text-primary, bg-container, border-primary) instead of raw color utilities like text-white or bg-white
Applied to files:
app/views/simplefin_items/_simplefin_item.html.erb
📚 Learning: 2025-08-22T18:30:26.758Z
Learnt from: jjmata
PR: we-promise/sure#116
File: app/views/family_exports/_list.html.erb:16-28
Timestamp: 2025-08-22T18:30:26.758Z
Learning: In the family exports UI (app/views/family_exports/_list.html.erb), raw color utilities like bg-green-500/5, text-green-500, bg-red-500/5, text-red-500 are preferred over design system tokens for status indicators to maintain UI legibility and consistency with the existing import UI patterns. The user has indicated that design token replacements would make the UI illegible in this context.
Applied to files:
app/views/simplefin_items/_simplefin_item.html.erb
📚 Learning: 2025-08-12T01:04:37.152Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/ui-ux-design-guidelines.mdc:0-0
Timestamp: 2025-08-12T01:04:37.152Z
Learning: Applies to {app/views/**,app/helpers/**,app/javascript/controllers/**} : Always start by referencing app/assets/tailwind/maybe-design-system.css to understand the base primitives, functional tokens, and component tokens
Applied to files:
app/views/simplefin_items/_simplefin_item.html.erb
📚 Learning: 2025-08-22T23:27:46.033Z
Learnt from: jjmata
PR: we-promise/sure#104
File: db/migrate/20250813144520_add_institution_fields_to_simplefin_items.rb:6-6
Timestamp: 2025-08-22T23:27:46.033Z
Learning: Institution URLs in financial contexts are typically short (under 100 characters) and don't require text columns - varchar(255) via :string is sufficient for SimpleFin institution URLs.
Applied to files:
db/schema.rb
🧬 Code graph analysis (8)
app/models/data_enrichment.rb (1)
app/models/provider_merchant.rb (1)
enum(1-6)
app/models/simplefin_entry/processor.rb (4)
app/models/simplefin_account/transactions/merchant_detector.rb (3)
initialize(3-29)initialize(4-6)detect_merchant(8-20)app/models/simplefin_item/importer.rb (2)
initialize(4-7)attr_reader(1-241)app/models/simplefin_account/transactions/processor.rb (4)
initialize(4-6)process(8-21)account(39-41)attr_reader(1-42)app/models/concerns/enrichable.rb (1)
enrich_attribute(26-28)
app/models/simplefin_account/transactions/merchant_detector.rb (2)
app/models/simplefin_entry/processor.rb (3)
initialize(1-119)initialize(3-6)merchant(116-118)app/models/simplefin_account/transactions/processor.rb (2)
initialize(4-6)attr_reader(1-42)
test/controllers/simplefin_items_controller_test.rb (1)
app/models/simplefin_account/transactions/processor.rb (1)
account(39-41)
app/models/simplefin_item.rb (2)
app/models/concerns/syncable.rb (1)
latest_sync(67-69)app/models/account.rb (1)
institution_domain(143-156)
app/models/simplefin_item/importer.rb (5)
app/models/concerns/syncable.rb (1)
last_synced_at(58-60)app/models/simplefin_item.rb (1)
upsert_simplefin_snapshot!(52-66)app/models/simplefin_account.rb (1)
upsert_simplefin_snapshot!(9-27)app/models/simplefin_item/provided.rb (1)
simplefin_provider(4-6)app/models/provider/simplefin.rb (1)
get_accounts(27-74)
app/models/simplefin_account/investments/holdings_processor.rb (5)
app/models/simplefin_account/transactions/processor.rb (4)
initialize(4-6)process(8-21)account(39-41)attr_reader(1-42)app/models/simplefin_account/investments/transactions_processor.rb (4)
initialize(3-90)initialize(4-6)process(8-17)account(22-24)app/models/simplefin_account/investments/balance_calculator.rb (2)
initialize(3-49)initialize(4-6)app/models/trade/create_form.rb (1)
security(22-29)app/models/simplefin_entry/processor.rb (1)
external_id(51-53)
app/models/simplefin_item/syncer.rb (3)
app/controllers/simplefin_items_controller.rb (1)
sync(107-116)app/controllers/accounts_controller.rb (1)
sync(29-35)app/models/simplefin_item.rb (3)
import_latest_simplefin_data(32-34)process_accounts(36-40)schedule_account_syncs(42-50)
🪛 YAMLlint (1.37.1)
config/locales/views/simplefin_items/en.yml
[error] 47-47: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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 (18)
app/models/provider_merchant.rb (1)
2-2: LGTM: addedsimplefinsource enumLooks consistent with the rest of the system.
app/models/data_enrichment.rb (2)
4-4: LGTM:simplefinadded to enrichment sourcesEnum expansion looks correct.
4-4: Verify any hard-coded source lists/scopesIf any dashboards/scopes/case statements enumerate sources, add
simplefinthere too.db/schema.rb (1)
32-32: Generated classification expression change looks benignExpression style change is Rails-generated; semantics appear unchanged. No action.
app/models/simplefin_account/transactions/merchant_detector.rb (1)
14-19: Ensure uniqueness and case-insensitivity on (source, name).Avoid duplicate ProviderMerchants (e.g., “STARBUCKS” vs “Starbucks”).
Do we have a unique index on provider_merchants(source, name) with case-insensitive collation (CITEXT or functional index)? If not, I can generate a migration to add it. Also consider using create_or_find_by to reduce race risk on concurrent creates.
app/models/simplefin_entry/processor.rb (3)
28-34: No enum update needed TheProviderMerchantenum already includessimplefin, so no change is required.Likely an incorrect or invalid review comment.
28-34: Remove suggestion –entry.transactionis a valid, type-specific association generated bydelegated_type :entryableand behaves the same asentry.entryablewhen the entryable is a Transaction.Likely an incorrect or invalid review comment.
9-13: Add unique index on account_entries.plaid_id
The account_entries table only defines plaid_id (no external_id column exists); add a unique index on account_entries.plaid_id to guard against ID collisions.⛔ Skipped due to learnings
Learnt from: jjmata PR: we-promise/sure#104 File: app/models/simplefin_account/transactions/processor.rb:36-41 Timestamp: 2025-08-22T23:28:38.143Z Learning: SimpleFin transaction IDs used with "simplefin_" prefix as plaid_id values have very low collision risk and don't require scoped uniqueness constraints, as SimpleFin provides globally unique transaction identifiers across their system.Learnt from: jjmata PR: we-promise/sure#104 File: app/models/simplefin_account/transactions/processor.rb:36-41 Timestamp: 2025-08-22T23:28:38.143Z Learning: SimpleFin transaction IDs are globally unique within SimpleFin's system, and the "simplefin_" prefix provides sufficient namespace separation from other providers like Plaid, making composite uniqueness constraints on entries.plaid_id unnecessary for SimpleFin integrations.app/views/simplefin_items/_simplefin_item.html.erb (4)
26-30: Institution summary display — good use of derived metadataRenders concise, derived summary only when accounts exist. Solid.
33-35: Loading state improvements LGTMSpinner + localized text meets the UX guidelines.
63-71: Update flow via modal — LGTMUses DS::Link with modal frame and localized text; matches new reconnect UX.
100-104: Setup CTA is localized and consistentClear copy and DS components. Good.
app/models/simplefin_item.rb (1)
37-39: Process only linked accountsUsing
joins(:account)avoids nil-account records. Good call.test/controllers/simplefin_items_controller_test.rb (4)
35-35: Redirect target change looks good.Redirecting destroys to accounts_path matches the new UX flow.
42-43: Sync redirect assertion updated appropriately.Aligns with simplified sync flow; no issues.
45-49: Edit action happy-path covered.Good prerequisite state setup with requires_update.
199-246: Clarify domain rule: schedule-for-deletion when no accounts are returned?In this scenario we keep the old linkage (no transfer) but still mark the old item for deletion. If a background job later deletes it, accounts may dangle. Please confirm intended behavior or gate deletion on successful transfer.
app/models/simplefin_item/importer.rb (1)
80-82: Consider storing the latest snapshot, not necessarily the first chunk.If UI/status depends on raw_payload, saving only the first (latest time range) is OK. If you want the “final state after sync,” move this outside the loop to store the last successful chunk or compose a minimal snapshot.
Re-raise ArgumentError instead of silently returning BigDecimal("0")
to prevent misleading $0 transactions from invalid amount data.
Merge transaction arrays instead of overwriting to prevent data loss during chunked imports. Preserve most recent holdings data only.
- Fixed SimplefinItemsControllerTest by properly mocking Provider::Simplefin instead of over-mocking the create_simplefin_item! method - Removed DEBUG puts statements from SimplefinItem::Importer
|
@jjmata I think this is good to go. Note from my email with SimpleFin support:
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
app/models/simplefin_entry/processor.rb (1)
51-55: Good: fail fast when external id is missing.This addresses prior risk of non-unique external IDs.
app/models/simplefin_item.rb (1)
57-63: Good: indifferent access fixes org extraction.Prevents silent skips when keys are strings.
test/controllers/simplefin_items_controller_test.rb (1)
51-71: Ensure a new item is actually created; also confirms earlier review is addressed.Nice: you no longer stub create_simplefin_item! to return the same record, which fixes the masking issue noted previously. Add an explicit creation assertion and verify the new access_url.
Apply:
- patch simplefin_item_url(@simplefin_item), params: { - simplefin_item: { setup_token: "valid_token" } - } - - assert_redirected_to accounts_path + assert_difference -> { @family.simplefin_items.count }, +1 do + patch simplefin_item_url(@simplefin_item), params: { + simplefin_item: { setup_token: "valid_token" } + } + end + assert_redirected_to accounts_path assert_match(/updated successfully/, flash[:notice]) @simplefin_item.reload assert @simplefin_item.scheduled_for_deletion? + # verify new item exists with claimed URL + new_item = @family.simplefin_items.where.not(id: @simplefin_item.id).order(:created_at).last + assert_equal "https://example.com/new_access", new_item.access_url
🧹 Nitpick comments (20)
db/migrate/20250903010617_add_unique_index_to_holdings_external_id.rb (1)
3-3: Align index naming convention.If this migration remains, align the name with the filtered index name (
idx_holdings_on_account_id_external_id_unique) for consistency in tooling/alerts.db/migrate/20250903015009_fix_holdings_cost_basis_and_external_id_constraints.rb (2)
1-8: Optional: build the unique index concurrently to reduce locking.For large holdings tables, creating the index concurrently avoids long write locks. This requires disabling the migration transaction.
class FixHoldingsCostBasisAndExternalIdConstraints < ActiveRecord::Migration[7.2] + disable_ddl_transaction! def change change_column :holdings, :cost_basis, :decimal, precision: 19, scale: 4 remove_index :holdings, name: "index_holdings_on_account_and_external_id", if_exists: true - add_index :holdings, [ :account_id, :external_id ], + add_index :holdings, [ :account_id, :external_id ], unique: true, where: "external_id IS NOT NULL", - name: "idx_holdings_on_account_id_external_id_unique" + name: "idx_holdings_on_account_id_external_id_unique", + algorithm: :concurrently end endNote: If you prefer to keep transactions, split the index work into a separate migration with
disable_ddl_transaction!.
3-3: Consider guarding data quality: non-negative cost_basis check.If business rules disallow negative cost basis, add a check constraint.
def change change_column :holdings, :cost_basis, :decimal, precision: 19, scale: 4 + add_check_constraint :holdings, "cost_basis >= 0", name: "chk_holdings_cost_basis_non_negative", if_not_exists: trueapp/models/holding.rb (1)
9-12: Validate cost_basis numerically (non-negative)Prevents bad data from SimpleFin and mirrors DB intent.
validates :qty, :price, :amount, numericality: { greater_than_or_equal_to: 0 } validates :external_id, uniqueness: { scope: :account_id }, allow_blank: true + validates :cost_basis, numericality: { greater_than_or_equal_to: 0 }, allow_nil: trueapp/models/simplefin_account/investments/holdings_processor.rb (5)
41-45: Remove redundant|| 0; parse_decimal already returns zero on nil/invalidKeeps numeric types consistent (BigDecimal everywhere).
- else - parse_decimal(simplefin_holding["purchase_price"]) || 0 + else + parse_decimal(simplefin_holding["purchase_price"])
57-62: Unnecessary transaction around a single saveNo multi-write atomicity here; the transaction adds overhead without benefit.
- ActiveRecord::Base.transaction do - holding.save! - - # With external_id matching, each holding is uniquely tracked - # No need to delete other holdings since each has its own lifecycle - end + holding.save!
90-101: Broaden created-timestamp parsing (Float/Time/Date…)SimpleFin payloads can vary; mirror the TransactionsProcessor’s robustness.
def parse_holding_date(created_timestamp) return nil unless created_timestamp - - case created_timestamp - when Integer - Time.at(created_timestamp).to_date - when String - Date.parse(created_timestamp) - else - nil - end + case created_timestamp + when Integer, Float + Time.at(created_timestamp).to_date + when String + Date.parse(created_timestamp) + when Time, DateTime + created_timestamp.to_date + when Date + created_timestamp + else + nil + end
106-109: Return BigDecimal zero for type consistencyAvoid mixing Integer 0 with BigDecimal in comparisons and arithmetic.
- return 0 unless value.present? + return BigDecimal("0") unless value.present?
15-16: Optional: log skipped rows missing symbol/idA warn-level log helps diagnose upstream data issues without raising.
- next unless symbol.present? && holding_id.present? + unless symbol.present? && holding_id.present? + Rails.logger.warn "Skipping SimpleFin holding with missing symbol/id for account #{account.id}" + next + endapp/models/simplefin_entry/processor.rb (3)
85-87: Fix comment wording (“Maybe” → “our app”).Avoid confusing provider naming in comments.
- # Maybe expects opposite convention (expenses positive, income negative) - # So we negate the amount to convert from SimpleFin to Maybe format + # Our app expects expenses positive, income negative. + # Negate to convert from SimpleFin convention.
28-34: Guard merchant enrichment when entryable isn’t a Transaction.If an existing entry isn’t a Transaction,
entry.transactionmay be nil.- if merchant - entry.transaction.enrich_attribute( + if merchant && entry.transaction + entry.transaction.enrich_attribute( :merchant_id, merchant.id, source: "simplefin" ) - end + endIf there are entries for SimpleFin that are not Transactions, confirm and handle (log/skip).
101-103: Prefer Time.zone for epoch parsing (consistency).Use Rails time zone for consistent date conversion.
- # Unix timestamp - Time.at(data[:posted]).to_date + # Unix timestamp + Time.zone.at(data[:posted]).to_dateapp/models/simplefin_item/importer.rb (3)
95-97: Optional: eliminate chunk boundary overlap behind a flag.If SimpleFin treats end-date as inclusive, overlapping days cause duplicates. Keep current default but add a flag to switch to exclusive end to test safely.
- # Continue to next chunk - move the end date backwards - chunk_end_date = chunk_start_date + # Continue to next chunk - move the end date backwards + # Optional exclusive end to avoid boundary overlap if API is inclusive + chunk_end_date = ENV["SIMPLEFIN_EXCLUSIVE_END"] == "1" ? (chunk_start_date - 1.second) : chunk_start_date
169-183: Normalize account_data once with indifferent access.Avoid mixed string/symbol key pitfalls; keeps mapping consistent with other processors.
- # Store transactions and holdings separately from account data to avoid overwriting - transactions = account_data[:transactions] - holdings = account_data[:holdings] + # Store transactions and holdings separately from account data to avoid overwriting + snapshot = account_data.to_h.with_indifferent_access + transactions = snapshot[:transactions] + holdings = snapshot[:holdings] @@ - attrs = { - name: account_data[:name], - account_type: account_data["type"] || account_data[:type] || "unknown", - currency: account_data[:currency] || "USD", - current_balance: account_data[:balance], - available_balance: account_data[:"available-balance"], - balance_date: account_data[:"balance-date"], - raw_payload: account_data, - org_data: account_data[:org] - } + attrs = { + name: snapshot[:name], + account_type: snapshot["type"] || snapshot[:type] || "unknown", + currency: snapshot[:currency] || "USD", + current_balance: snapshot[:balance], + available_balance: snapshot[:"available-balance"], + balance_date: snapshot[:"balance-date"], + raw_payload: snapshot, + org_data: snapshot[:org] + }Also change
account_id = account_data[:id](Line 160) to usesnapshotif you adopt this.
84-88: Optional: count unique accounts imported.
total_accounts_imported += sizewill be inflated across chunks. Consider tracking unique account_ids for more accurate telemetry.Example:
- Use a Set to collect account_ids and log its size at the end.
app/models/simplefin_item.rb (1)
136-142: Optional: normalize org keys before dedup.
connected_institutionsassumes string keys; normalize each org to indifferent access for safety.- .map { |acc| acc.org_data } - .uniq { |org| org["domain"] || org["name"] } + .map { |acc| acc.org_data&.to_h&.with_indifferent_access } + .uniq { |org| org[:domain] || org[:name] }test/controllers/simplefin_items_controller_test.rb (4)
72-81: Short-circuit on empty token: assert no provider calls and no new records.Tighten the failure-path contract.
Apply:
- patch simplefin_item_url(@simplefin_item), params: { - simplefin_item: { setup_token: "" } - } + Provider::Simplefin.expects(:new).never + assert_no_difference -> { @family.simplefin_items.count } do + patch simplefin_item_url(@simplefin_item), params: { + simplefin_item: { setup_token: "" } + } + endOptional: assert translated copy via I18n.t(...) to avoid brittleness on string changes.
83-188: Strong end-to-end transfer test; add count assertions and a couple of invariants.The scenario is thorough. Add deltas to prove object creation and ensure IDs really change for the linked accounts.
Apply:
- # Perform the update - patch simplefin_item_url(@simplefin_item), params: { - simplefin_item: { setup_token: "valid_token" } - } + # Perform the update and assert creations: +1 item, +2 SimpleFin accounts + assert_difference -> { @family.simplefin_items.count }, +1 do + assert_difference "SimplefinAccount.count", +2 do + patch simplefin_item_url(@simplefin_item), params: { + simplefin_item: { setup_token: "valid_token" } + } + end + end @@ - new_simplefin_item = @family.simplefin_items.where.not(id: @simplefin_item.id).first + new_simplefin_item = @family.simplefin_items.where.not(id: @simplefin_item.id).order(:created_at).last + assert_equal "https://example.com/new_access", new_simplefin_item.access_url @@ assert_equal new_sf_account1.id, maybe_account1.simplefin_account_id assert_equal new_sf_account2.id, maybe_account2.simplefin_account_id + refute_equal old_simplefin_account1.id, maybe_account1.simplefin_account_id + refute_equal old_simplefin_account2.id, maybe_account2.simplefin_account_idMinor: consider moving the heavy object setup to fixtures or a small builder helper to reduce repetition in future controller tests.
190-237: Partial-match path: assert item creation and no new accounts.Prove we still create a replacement item even when no accounts are returned, and that we don’t create new SimpleFin accounts.
Apply:
- # Perform update - patch simplefin_item_url(@simplefin_item), params: { - simplefin_item: { setup_token: "valid_token" } - } + # Perform update: +1 item, +0 accounts + assert_difference -> { @family.simplefin_items.count }, +1 do + assert_no_difference "SimplefinAccount.count" do + patch simplefin_item_url(@simplefin_item), params: { + simplefin_item: { setup_token: "valid_token" } + } + end + end
42-43: Assert SyncJob enqueued in “should sync simplefin item” test
Addinclude ActiveJob::TestHelperto the test class and update the sync test:class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest include ActiveJob::TestHelper test "should sync simplefin item" do - post sync_simplefin_item_url(@simplefin_item) - assert_redirected_to accounts_path + assert_enqueued_jobs 1 do + post sync_simplefin_item_url(@simplefin_item) + end + assert_redirected_to accounts_path end endOptionally, tighten to the specific job:
assert_enqueued_with job: SyncJob do post sync_simplefin_item_url(@simplefin_item) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
app/controllers/simplefin_items_controller.rb(6 hunks)app/models/holding.rb(1 hunks)app/models/simplefin_account/investments/holdings_processor.rb(1 hunks)app/models/simplefin_entry/processor.rb(1 hunks)app/models/simplefin_item.rb(2 hunks)app/models/simplefin_item/importer.rb(2 hunks)db/migrate/20250903010617_add_unique_index_to_holdings_external_id.rb(1 hunks)db/migrate/20250903015009_fix_holdings_cost_basis_and_external_id_constraints.rb(1 hunks)db/schema.rb(6 hunks)test/controllers/simplefin_items_controller_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- db/schema.rb
- app/controllers/simplefin_items_controller.rb
🧰 Additional context used
📓 Path-based instructions (9)
db/migrate/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Enforce simple validations (NOT NULL, unique indexes) at the database level
Files:
db/migrate/20250903015009_fix_holdings_cost_basis_and_external_id_constraints.rbdb/migrate/20250903010617_add_unique_index_to_holdings_external_id.rb
{app,lib}/**/*.{rb,erb}
📄 CodeRabbit inference engine (CLAUDE.md)
{app,lib}/**/*.{rb,erb}: Use Current.user for the authenticated user; do not use current_user
Use Current.family for the current family; do not use current_family
Files:
app/models/holding.rbapp/models/simplefin_item.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/importer.rb
{app/**/*.rb,app/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
{app/**/*.rb,app/**/*.erb}: UseCurrent.userfor the current user; do not usecurrent_userin Rails code and templates
UseCurrent.familyfor the current family; do not usecurrent_familyin Rails code and templates
Files:
app/models/holding.rbapp/models/simplefin_item.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/importer.rb
app/models/**/{*,**/*.rb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use Rails concerns and POROs organized around model traits in app/models and app/models/concerns
Files:
app/models/holding.rbapp/models/simplefin_item.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/importer.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/models/**/*.rb: Models should answer questions about themselves (e.g., Account#balance_series) rather than external service objects
Use ActiveRecord validations for complex validations and business logic (may mirror DB constraints for UX)Domain models should not call Provider::Registry directly; instead, add a Provided concern within the model’s namespace to choose providers and expose convenience methods (e.g., app/models//provided.rb).
Files:
app/models/holding.rbapp/models/simplefin_item.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/importer.rb
{app/helpers/**/*.rb,app/models/**/*.rb,app/views/**/*.html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates server-side (helpers/models) and pass formatted values to Stimulus for display only
Files:
app/models/holding.rbapp/models/simplefin_item.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/importer.rb
{app/models/**/*.rb,app/controllers/**/*.rb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/models/holding.rbapp/models/simplefin_item.rbapp/models/simplefin_entry/processor.rbapp/models/simplefin_account/investments/holdings_processor.rbapp/models/simplefin_item/importer.rb
test/**/*_test.rb
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*_test.rb: Use VCR for external API testing
Use the mocha gem for stubs and mocks in tests
Files:
test/controllers/simplefin_items_controller_test.rb
test/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
test/**/*.rb: Write tests with Minitest; do not use RSpec in this codebase
Use fixtures for test data; do not use factories
For tests needing many records, use Rails helpers to create them and inline the creation in the test (e.g., helpers in test/support)
Test boundaries correctly: for query methods, assert returned output; for command methods, assert the command was invoked with correct params
Do not test one class’s implementation details in another class’s test suite
Use the mocha gem for stubs and mocks
Prefer OpenStruct for simple mock instances, or define a small mock class for complex cases
Only mock what’s necessary; don’t mock return values you don’t assert on
Files:
test/controllers/simplefin_items_controller_test.rb
🧠 Learnings (4)
📚 Learning: 2025-08-12T01:02:31.793Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-08-12T01:02:31.793Z
Learning: Applies to db/migrate/**/*.rb : Enforce simple validations (NOT NULL, unique indexes) at the database level
Applied to files:
app/models/holding.rbdb/migrate/20250903010617_add_unique_index_to_holdings_external_id.rbapp/models/simplefin_account/investments/holdings_processor.rb
📚 Learning: 2025-08-12T01:02:31.793Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-08-12T01:02:31.793Z
Learning: Applies to app/models/**/*.rb : Use ActiveRecord validations for complex validations and business logic (may mirror DB constraints for UX)
Applied to files:
app/models/holding.rb
📚 Learning: 2025-08-22T23:28:38.143Z
Learnt from: jjmata
PR: we-promise/sure#104
File: app/models/simplefin_account/transactions/processor.rb:36-41
Timestamp: 2025-08-22T23:28:38.143Z
Learning: SimpleFin transaction IDs used with "simplefin_" prefix as plaid_id values have very low collision risk and don't require scoped uniqueness constraints, as SimpleFin provides globally unique transaction identifiers across their system.
Applied to files:
app/models/simplefin_entry/processor.rb
📚 Learning: 2025-09-03T01:15:59.616Z
Learnt from: Himmelschmidt
PR: we-promise/sure#104
File: app/models/simplefin_item/importer.rb:34-45
Timestamp: 2025-09-03T01:15:59.616Z
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_item/importer.rb
🧬 Code graph analysis (7)
db/migrate/20250903015009_fix_holdings_cost_basis_and_external_id_constraints.rb (1)
db/migrate/20250903010617_add_unique_index_to_holdings_external_id.rb (2)
change(1-5)change(2-4)
db/migrate/20250903010617_add_unique_index_to_holdings_external_id.rb (1)
db/migrate/20250903015009_fix_holdings_cost_basis_and_external_id_constraints.rb (2)
change(1-9)change(2-8)
test/controllers/simplefin_items_controller_test.rb (1)
app/models/simplefin_account/transactions/processor.rb (1)
account(39-41)
app/models/simplefin_item.rb (2)
app/models/concerns/syncable.rb (1)
latest_sync(67-69)app/models/account.rb (1)
institution_domain(143-156)
app/models/simplefin_entry/processor.rb (4)
app/models/simplefin_item/importer.rb (2)
initialize(4-7)attr_reader(1-242)app/models/simplefin_account/transactions/merchant_detector.rb (3)
initialize(3-29)initialize(4-6)detect_merchant(8-20)app/models/simplefin_account/transactions/processor.rb (4)
initialize(4-6)process(8-21)account(39-41)attr_reader(1-42)app/models/concerns/enrichable.rb (1)
enrich_attribute(26-28)
app/models/simplefin_account/investments/holdings_processor.rb (5)
app/models/simplefin_account/transactions/processor.rb (4)
initialize(4-6)process(8-21)account(39-41)attr_reader(1-42)app/models/simplefin_account/investments/transactions_processor.rb (4)
initialize(3-90)initialize(4-6)process(8-17)account(22-24)app/models/simplefin_account/investments/balance_calculator.rb (2)
initialize(3-49)initialize(4-6)app/models/trade/create_form.rb (1)
security(22-29)app/models/simplefin_entry/processor.rb (1)
external_id(51-55)
app/models/simplefin_item/importer.rb (5)
app/models/concerns/syncable.rb (1)
last_synced_at(58-60)app/models/simplefin_item.rb (1)
upsert_simplefin_snapshot!(52-66)app/models/simplefin_account.rb (1)
upsert_simplefin_snapshot!(9-27)app/models/simplefin_item/provided.rb (1)
simplefin_provider(4-6)app/models/provider/simplefin.rb (1)
get_accounts(27-74)
⏰ 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 (11)
db/migrate/20250903015009_fix_holdings_cost_basis_and_external_id_constraints.rb (1)
4-8: Verify no duplicates exist before enforcing the unique index. Manually run a query against the holdings table to ensure no (account_id, external_id) pairs appear more than once when external_id is not null, and resolve any conflicts before applying this migration.app/models/holding.rb (1)
11-11: LGTM: app-level idempotency via scoped uniquenessAdding
validates :external_id, uniqueness: { scope: :account_id }is correct and aligns with the upsert pattern in processors.app/models/simplefin_account/investments/holdings_processor.rb (1)
7-9: Good guard railsEarly returns for empty data and non-Investment accounts prevent unintended writes.
app/models/simplefin_entry/processor.rb (1)
28-34: No action needed:simplefinis already included inProviderMerchant’ssourceenum.app/models/simplefin_item/importer.rb (2)
75-77: Confirm early-return behavior on per-chunk API error.Returning from the method aborts remaining chunks; decide if you want “all-or-nothing” or “best-effort” history. If best-effort, handle per-chunk errors and continue.
I can draft a minimal “continue on error” branch if desired.
233-241: Acknowledged: 7-day initial lookback is intentional.Retaining 7-day default aligns with the product decision captured in learnings; no change requested.
app/models/simplefin_item.rb (3)
37-37: Good: process only linked accounts.
joins(:account)avoids nil-account work and reduces errors.
68-89: Robust institution metadata handling.Deriving domain from URL and storing raw payload improves UX and traceability.
131-134: Minor: fallbacks look sane.Display prefers name, then domain, then item name.
test/controllers/simplefin_items_controller_test.rb (2)
35-35: Redirect target looks correct after removing SimpleFin-specific index.accounts_path align with the new unified Accounts landing. No further action.
45-49: Edit flow coverage LGTM.Good preconditioning with requires_update and a simple 200 assertion.
|
|
||
| validates :qty, :currency, :date, :price, :amount, presence: true | ||
| validates :qty, :price, :amount, numericality: { greater_than_or_equal_to: 0 } | ||
| validates :external_id, uniqueness: { scope: :account_id }, allow_blank: true |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify DB index alignment; avoid duplicate unique indexes
Ensure there isn’t both a partial and a global unique index on (account_id, external_id). Keep one canonical unique index strategy to match allow_blank: true.
🏁 Script executed:
#!/bin/bash
# Inspect schema/migrations for holdings external_id indexes
rg -nP "(add_index|index_).*(holdings).*(external_id)" db/migrate db/schema.rb -C2Length of output: 1897
Drop the redundant global unique index on (account_id, external_id)
Migrations currently add both a global index (index_holdings_on_account_and_external_id) and a partial index (idx_holdings_on_account_id_external_id_unique). Consolidate to a single unique index—prefer the partial one (where external_id IS NOT NULL) to align with allow_blank: true—by removing the global add_index call in db/migrate/20250903010617_add_unique_index_to_holdings_external_id.rb.
🤖 Prompt for AI Agents
In app/models/holding.rb around line 11, the model validates external_id
uniqueness scoped to account_id with allow_blank: true, but the migrations
currently create both a global unique index and a partial unique index; remove
the redundant global index by editing
db/migrate/20250903010617_add_unique_index_to_holdings_external_id.rb to delete
the add_index call that creates index_holdings_on_account_and_external_id,
leaving only the partial unique index (WHERE external_id IS NOT NULL) so the DB
index matches the allow_blank validation and avoid duplicate constraints.
| h.currency = simplefin_holding["currency"] || "USD" | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Default currency should fall back to account.currency, not hard-coded USD
Hard-coding "USD" risks mislabeled amounts for non-USD accounts.
- h.currency = simplefin_holding["currency"] || "USD"
+ h.currency = simplefin_holding["currency"].presence || account.currency
@@
- currency: simplefin_holding["currency"] || "USD",
+ currency: simplefin_holding["currency"].presence || account.currency,Also applies to: 50-51
🤖 Prompt for AI Agents
In app/models/simplefin_account/investments/holdings_processor.rb lines 32-33
(and similarly at 50-51), the default currency is hard-coded to "USD"; change
the fallback to use the account's currency instead by using
simplefin_holding["currency"] || account.currency (optionally with a final
fallback like "USD" only if account.currency can be nil), and apply the same
change to the other occurrence so holdings inherit the account's currency rather
than always "USD".
There was a problem hiding this comment.
This seems like a pretty good one to take, right @Himmelschmidt ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| def holdings_data | ||
| # Use the dedicated raw_holdings_payload field | ||
| simplefin_account.raw_holdings_payload || [] | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unify holdings source across components
BalanceCalculator reads from raw_payload["holdings"] while this uses raw_holdings_payload. Standardize the source to avoid drift in cash-balance vs. persisted holdings.
🏁 Script executed:
#!/bin/bash
# Locate all SimpleFin holdings sources to reconcile
rg -nP 'raw_holdings_payload|raw_payload.*holdings' app -C2Length of output: 1824
Use persisted raw_holdings_payload in BalanceCalculator.total_holdings_value
BalanceCalculator currently reads holdings from simplefin_account.raw_payload["holdings"]; update it to use holdings_data (i.e. simplefin_account.raw_holdings_payload) so it aligns with HoldingsProcessor and the importer.
Location: app/models/simplefin_account/investments/balance_calculator.rb lines 28–32.
🤖 Prompt for AI Agents
In app/models/simplefin_account/investments/balance_calculator.rb around lines
28–32, the calculator currently reads holdings from
simplefin_account.raw_payload["holdings"]; change it to use the persisted
holdings accessor (simplefin_account.holdings_data or
simplefin_account.raw_holdings_payload) so it aligns with HoldingsProcessor and
the importer, and handle nil by defaulting to an empty array (e.g., use the
holdings_data helper which returns [] when missing).
| def amount | ||
| parsed_amount = case data[:amount] | ||
| when String | ||
| BigDecimal(data[:amount]) | ||
| when Numeric | ||
| BigDecimal(data[:amount].to_s) | ||
| else | ||
| BigDecimal("0") | ||
| end | ||
|
|
||
| # SimpleFin uses banking convention (expenses negative, income positive) | ||
| # Maybe expects opposite convention (expenses positive, income negative) | ||
| # So we negate the amount to convert from SimpleFin to Maybe format | ||
| -parsed_amount | ||
| rescue ArgumentError => e | ||
| Rails.logger.error "Failed to parse SimpleFin transaction amount: #{data[:amount].inspect} - #{e.message}" | ||
| raise | ||
| end |
There was a problem hiding this comment.
Do not coerce unknown/missing amounts to 0; raise on invalid input.
Returning 0 for non-string/numeric inputs can create silent, misleading entries. Fail fast on nil/unsupported types.
Apply:
- def amount
- parsed_amount = case data[:amount]
- when String
- BigDecimal(data[:amount])
- when Numeric
- BigDecimal(data[:amount].to_s)
- else
- BigDecimal("0")
- end
-
- # SimpleFin uses banking convention (expenses negative, income positive)
- # Maybe expects opposite convention (expenses positive, income negative)
- # So we negate the amount to convert from SimpleFin to Maybe format
- -parsed_amount
- rescue ArgumentError => e
- Rails.logger.error "Failed to parse SimpleFin transaction amount: #{data[:amount].inspect} - #{e.message}"
- raise
- end
+ def amount
+ raw = data[:amount]
+ raise ArgumentError, "SimpleFin transaction missing amount: #{data.inspect}" if raw.nil?
+
+ parsed_amount =
+ case raw
+ when String
+ BigDecimal(raw)
+ when Numeric
+ BigDecimal(raw.to_s)
+ else
+ raise ArgumentError, "Unsupported amount type: #{raw.class}"
+ end
+
+ # SimpleFin uses banking convention (expenses negative, income positive).
+ # Our app expects the opposite; negate to convert.
+ -parsed_amount
+ rescue ArgumentError => e
+ Rails.logger.error "Failed to parse SimpleFin transaction amount: #{raw.inspect} - #{e.message}"
+ raise
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def amount | |
| parsed_amount = case data[:amount] | |
| when String | |
| BigDecimal(data[:amount]) | |
| when Numeric | |
| BigDecimal(data[:amount].to_s) | |
| else | |
| BigDecimal("0") | |
| end | |
| # SimpleFin uses banking convention (expenses negative, income positive) | |
| # Maybe expects opposite convention (expenses positive, income negative) | |
| # So we negate the amount to convert from SimpleFin to Maybe format | |
| -parsed_amount | |
| rescue ArgumentError => e | |
| Rails.logger.error "Failed to parse SimpleFin transaction amount: #{data[:amount].inspect} - #{e.message}" | |
| raise | |
| end | |
| def amount | |
| raw = data[:amount] | |
| raise ArgumentError, "SimpleFin transaction missing amount: #{data.inspect}" if raw.nil? | |
| parsed_amount = | |
| case raw | |
| when String | |
| BigDecimal(raw) | |
| when Numeric | |
| BigDecimal(raw.to_s) | |
| else | |
| raise ArgumentError, "Unsupported amount type: #{raw.class}" | |
| end | |
| # SimpleFin uses banking convention (expenses negative, income positive). | |
| # Our app expects the opposite; negate to convert. | |
| -parsed_amount | |
| rescue ArgumentError => e | |
| Rails.logger.error "Failed to parse SimpleFin transaction amount: #{raw.inspect} - #{e.message}" | |
| raise | |
| end |
| accounts_data[:accounts]&.each do |account_data| | ||
| import_account(account_data) | ||
| end |
There was a problem hiding this comment.
Holdings won’t update on regular syncs (stale positions).
import_account only sets holdings when blank, so subsequent (non-chunked) syncs never refresh positions.
Apply the following changes:
- Pass
replace_holdings: trueon regular sync:
- accounts_data[:accounts]&.each do |account_data|
- import_account(account_data)
- end
+ accounts_data[:accounts]&.each do |account_data|
+ import_account(account_data, replace_holdings: true)
+ end- Pass
replace_holdings: (chunk_count == 1)for chunked history (keep only newest holdings):
- accounts_data[:accounts]&.each do |account_data|
- import_account(account_data)
+ accounts_data[:accounts]&.each do |account_data|
+ import_account(account_data, replace_holdings: (chunk_count == 1))
end- Update method signature and condition:
- def import_account(account_data)
+ def import_account(account_data, replace_holdings: false)
@@
- if holdings.is_a?(Array) && holdings.any? && simplefin_account.raw_holdings_payload.blank?
+ if holdings.is_a?(Array) && holdings.any? && (replace_holdings || simplefin_account.raw_holdings_payload.blank?)
attrs[:raw_holdings_payload] = holdings
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| accounts_data[:accounts]&.each do |account_data| | |
| import_account(account_data) | |
| end | |
| # ──────────────── Regular (non-chunked) sync ──────────────── | |
| accounts_data[:accounts]&.each do |account_data| | |
| # always refresh holdings on a normal sync | |
| import_account(account_data, replace_holdings: true) | |
| end | |
| # ──────────────── Chunked history sync ──────────────── | |
| # (inside your chunked loop, e.g. `if chunked_history?` or similar) | |
| accounts_data[:accounts]&.each do |account_data| | |
| # only replace holdings on the very first (most recent) chunk | |
| import_account(account_data, replace_holdings: (chunk_count == 1)) | |
| end | |
| # ──────────────── import_account definition ──────────────── | |
| # Change the signature to accept the new flag | |
| def import_account(account_data, replace_holdings: false) | |
| # … existing setup: find_or_build simplefin_account, extract `holdings` variable, etc. … | |
| # only set/replace holdings if explicitly asked or if none exist yet | |
| if holdings.is_a?(Array) && | |
| holdings.any? && | |
| (replace_holdings || simplefin_account.raw_holdings_payload.blank?) | |
| attrs[:raw_holdings_payload] = holdings | |
| end | |
| # … rest of import_account: assign attrs, save!, etc. … | |
| end |
🤖 Prompt for AI Agents
In app/models/simplefin_item/importer.rb around lines 112-114, import_account is
called without the new replace_holdings flag causing holdings to only be set
when blank and leaving positions stale; update the call to pass
replace_holdings: true for regular (non-history-chunked) syncs, and for chunked
history calls pass replace_holdings: (chunk_count == 1) so only the newest chunk
replaces holdings; then update the import_account method signature to accept
replace_holdings (default false) and change the internal condition that
currently only sets holdings when blank to instead replace holdings when
replace_holdings is true or when holdings are blank.
There was a problem hiding this comment.
Can't decide if this is true of "hallucinated" @Himmelschmidt ... take a look? 🙏
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| class AddUniqueIndexToHoldingsExternalId < ActiveRecord::Migration[7.2] | ||
| def change | ||
| add_index :holdings, [ :account_id, :external_id ], unique: true, name: 'index_holdings_on_account_and_external_id' | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Drop this migration or make it a no-op; it conflicts with the filtered unique index added later.
The later migration adds a partial unique index on (account_id, external_id). Keeping this non-filtered unique index creates duplicate constraints and extra write overhead. Prefer the filtered index and remove this one, or make this migration a no-op before merge.
Apply one of the following:
Option A — Remove the index here (preferred; or delete the file):
class AddUniqueIndexToHoldingsExternalId < ActiveRecord::Migration[7.2]
def change
- add_index :holdings, [ :account_id, :external_id ], unique: true, name: 'index_holdings_on_account_and_external_id'
+ # superseded by FixHoldingsCostBasisAndExternalIdConstraints
end
endOption B — If you must keep an index here, match the later filtered constraint and then remove it from the later migration to avoid duplication:
- add_index :holdings, [ :account_id, :external_id ], unique: true, name: 'index_holdings_on_account_and_external_id'
+ add_index :holdings, [:account_id, :external_id],
+ unique: true,
+ where: "external_id IS NOT NULL",
+ name: "idx_holdings_on_account_id_external_id_unique"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class AddUniqueIndexToHoldingsExternalId < ActiveRecord::Migration[7.2] | |
| def change | |
| add_index :holdings, [ :account_id, :external_id ], unique: true, name: 'index_holdings_on_account_and_external_id' | |
| end | |
| end | |
| class AddUniqueIndexToHoldingsExternalId < ActiveRecord::Migration[7.2] | |
| def change | |
| # superseded by FixHoldingsCostBasisAndExternalIdConstraints | |
| end | |
| end |
🤖 Prompt for AI Agents
In db/migrate/20250903010617_add_unique_index_to_holdings_external_id.rb around
lines 1 to 5, this migration creates a non-filtered unique index that conflicts
with a later partial (filtered) unique index; either remove this migration file
(preferred) or make it a no-op by deleting the add_index line, or if you must
keep an index here change it to a partial index with the same WHERE clause used
in the later migration so they are identical and then remove the duplicate from
the later migration to avoid duplicate constraints and extra write overhead.
There was a problem hiding this comment.
Schema stuff is a pain to clean up later, did all the migrations get auto-generated or did you add them by hand @Himmelschmidt ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| add_index :holdings, [ :account_id, :external_id ], | ||
| unique: true, | ||
| where: "external_id IS NOT NULL", | ||
| name: "idx_holdings_on_account_id_external_id_unique" | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the earlier non-filtered index before creating the filtered one to avoid duplicate indexes.
Add a guarded drop for index_holdings_on_account_and_external_id prior to creating the partial unique index.
class FixHoldingsCostBasisAndExternalIdConstraints < ActiveRecord::Migration[7.2]
def change
change_column :holdings, :cost_basis, :decimal, precision: 19, scale: 4
+ remove_index :holdings, name: "index_holdings_on_account_and_external_id", if_exists: true
add_index :holdings, [ :account_id, :external_id ],
unique: true,
where: "external_id IS NOT NULL",
name: "idx_holdings_on_account_id_external_id_unique"
end
end🤖 Prompt for AI Agents
In
db/migrate/20250903015009_fix_holdings_cost_basis_and_external_id_constraints.rb
around lines 4 to 8, a new partial unique index on [:account_id, :external_id]
is being added but the earlier non-filtered index must be removed first to avoid
duplicate/contradictory indexes; modify the migration to drop the existing index
named index_holdings_on_account_and_external_id before adding the filtered
unique index, using a guarded removal (e.g. check index_exists? or use
remove_index with if_exists: true) so the migration is safe to run whether or
not the old index is present.
|
I'm testing this PR to check if I can find any remaining issues with the SimpleFin integration. In general it appears to be working well. I've had the sync running for the past several hours, and it appears that all the accounts have data downloaded, but the SimpleFin Connection page shows "Never synced" |
I had similar issues while working on the Enable Banking integration. In my case, it happened because of unhandled errors that silently failed and didn’t update the synchronization status. Do you see any errors in the logs? |
|
This branch seems to be working great for me! The current main release seems to error out for me exactly how #150 and and #110 describe, but this branch syncs properly with SimpleFin and seems to retrieve all data properly. |
Signed-off-by: Juan José Mata <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| ) | ||
| # Fallback to zero as suggested | ||
| cash_balance = 0 | ||
| end |
There was a problem hiding this comment.
Bug: Cash Balance Overwritten on Calculation Failure
The investment cash balance calculation's rescue block sets cash_balance to 0 on failure, overwriting the initial balance value. This can lead to significant data inaccuracies for investment accounts.
| calculator.cash_balance | ||
| else | ||
| balance | ||
| end |
There was a problem hiding this comment.
Bug: Cash Balance Calculation Error Handling
The investment cash balance calculation in process_account! (lines 42-45) lacks error handling for SimplefinAccount::Investments::BalanceCalculator. An exception here crashes the entire sync process, unlike similar calculations elsewhere that gracefully fall back.
|
|
||
| # Enforce NOT NULL constraints on required fields | ||
| change_column_null :simplefin_items, :institution_id, false | ||
| change_column_null :simplefin_items, :institution_name, false |
There was a problem hiding this comment.
Bug: Migration Fails on NULL Values in Institution Columns
The migration adds NOT NULL constraints to existing institution_id and institution_name columns. If any simplefin_items records currently have NULL values in these fields, the migration will fail due to a constraint violation.
| # Update parent relationship if one is provided and sync doesn't already have a parent | ||
| if parent_sync && !sync.parent_id | ||
| sync.update!(parent: parent_sync) | ||
| end |
There was a problem hiding this comment.
Bug: Sync Parent Update Race Condition
Updating an existing, incomplete sync's parent relationship introduces a race condition. This can interfere with sync completion logic, potentially causing premature finalization or indefinite hanging, and may lead to database consistency issues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
db/schema.rb (1)
720-720: json type inconsistency already flagged.This line uses
jsoninstead ofjsonb, which is inconsistent withplaid_items.raw_institution_payload(line 580) and reduces query capability. This was already raised in a previous review and acknowledged by the author as unintentional.Based on learnings: See the existing review thread where the migration needs to be updated to use
jsonbwithdefault: {}and the schema regenerated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/models/account.rb(1 hunks)config/routes.rb(1 hunks)db/schema.rb(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- config/routes.rb
- app/models/account.rb
🧰 Additional context used
📓 Path-based instructions (4)
db/schema.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Treat db/schema.rb as the authoritative source of truth for model relationships
Files:
db/schema.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
db/schema.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
db/schema.rb
**/*.{rb,erb,haml,slim}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb,haml,slim}: UseCurrent.userfor the current user; do not usecurrent_user
UseCurrent.familyfor the current family; do not usecurrent_family
Ignore i18n methods; hardcode strings in English for now (do not useI18n.t,t, or similar)
Files:
db/schema.rb
🧠 Learnings (1)
📚 Learning: 2025-09-23T22:20:16.727Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/general-rules.mdc:0-0
Timestamp: 2025-09-23T22:20:16.727Z
Learning: Applies to db/migrate/*.rb : ActiveRecord migrations must inherit from `ActiveRecord::Migration[7.2]`; do not use version 8.0
Applied to files:
db/schema.rb
⏰ 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). (2)
- GitHub Check: ci / test
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
db/schema.rb (3)
13-13: Schema version and virtual column syntax updates look good.The version bump reflects the applied migrations, and the updated
classificationvirtual column expression uses explicit PostgreSQL array casting which is semantically equivalent to the prior version.Also applies to: 32-32
705-705: SimpleFin metadata and holdings payload columns look good.The addition of
raw_holdings_payload(line 705) aligns with the investment holdings processing improvements, and the institution metadata fields (lines 724-726) support the enhanced multi-institution connection handling described in the PR objectives.Also applies to: 724-726
760-760: sync_stats column addition looks good.The
texttype is appropriate for serialized synchronization statistics, allowing flexibility in the serialization format.
| t.string "external_id" | ||
| t.decimal "cost_basis", precision: 19, scale: 4 | ||
| t.index ["account_id", "external_id"], name: "idx_holdings_on_account_id_external_id_unique", unique: true, where: "(external_id IS NOT NULL)" | ||
| t.index ["account_id", "external_id"], name: "index_holdings_on_account_and_external_id", unique: true | ||
| t.index ["account_id", "security_id", "date", "currency"], name: "idx_on_account_id_security_id_date_currency_5323e39f8b", unique: true |
There was a problem hiding this comment.
Remove redundant unique index that conflicts with partial unique index.
Lines 298 and 299 both define unique indexes on [account_id, external_id]. The partial unique index (line 298) correctly allows multiple holdings with NULL external_id per account while enforcing uniqueness on non-NULL values. However, the full unique index (line 299) would permit at most one holding with NULL external_id per account, which likely breaks SimpleFin imports when multiple holdings lack external IDs.
To fix this, remove the full unique index migration that created index_holdings_on_account_and_external_id and regenerate the schema. The partial unique index idx_holdings_on_account_id_external_id_unique at line 298 is sufficient.
- t.index ["account_id", "external_id"], name: "index_holdings_on_account_and_external_id", unique: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.string "external_id" | |
| t.decimal "cost_basis", precision: 19, scale: 4 | |
| t.index ["account_id", "external_id"], name: "idx_holdings_on_account_id_external_id_unique", unique: true, where: "(external_id IS NOT NULL)" | |
| t.index ["account_id", "external_id"], name: "index_holdings_on_account_and_external_id", unique: true | |
| t.index ["account_id", "security_id", "date", "currency"], name: "idx_on_account_id_security_id_date_currency_5323e39f8b", unique: true | |
| t.string "external_id" | |
| t.decimal "cost_basis", precision: 19, scale: 4 | |
| t.index ["account_id", "external_id"], name: "idx_holdings_on_account_id_external_id_unique", unique: true, where: "(external_id IS NOT NULL)" | |
| t.index ["account_id", "security_id", "date", "currency"], name: "idx_on_account_id_security_id_date_currency_5323e39f8b", unique: true |
There was a problem hiding this comment.
Do you know how this extra index crept in there, @Himmelschmidt ? Trying to get this PR in before it rots away ... sorry it's taking me so long!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Based on the last few comments I'll make a push to get it merged in the next couple of days. I've asked @Himmelschmidt for a last few comments, but this seems to improve things generally so I'll merge regardless. I just don't want to take schema migrations that add noise if possible. |
7 participants
Summary
This PR fixes broken SimpleFin investment holdings and enhances the SimpleFin integration with comprehensive improvements. The primary goal was to resolve investment account issues where holdings data wasn't properly processed.
Key Improvements
Critical Bug Fixes
total_balance - holdings_value)Enhanced User Experience
Architecture Improvements
Technical Changes
New Components
SimplefinAccount::Investments::BalanceCalculatorand related processorsLoadingButtonControllerfor form loading statesUpdated Components
Database Changes
simplefin_itemstableSummary by CodeRabbit
New Features
UI/Style
Bug Fixes
Tests & Data