Feature/simplefin integration #94
Conversation
- Add HTTParty gem for making HTTP requests to SimpleFin API - Required for SimpleFin protocol implementation
- Create simplefin_items table for SimpleFin connections - Create simplefin_accounts table for account metadata - Add simplefin_account_id to accounts table for linking - Add external_id to transactions for deduplication - Enable encrypted storage of SimpleFin access URLs
- Add SimplefinItem model with sync capabilities and encryption - Add SimplefinAccount model for account data mapping - Implement Provider::Simplefin API client with token exchange - Add SimpleFin protocol support with proper error handling - Include sync jobs, importers, and processors for data flow - Add family SimpleFin connectivity mixin
- Add SimpleFin account creation methods to Account model - Implement intelligent account type mapping from names - Add SimpleFin linkable functionality to Account - Include SimpleFin items in Family model associations - Support account creation with user-selected types
- Create SimplefinItemsController with CRUD operations - Add account setup flow with user type selection - Include sync management and error handling - Update AccountsController to display SimpleFin items - Add routes for SimpleFin item management and setup
- Create SimpleFin connection management views - Add account setup modal with type selection - Include connection form with token input and instructions - Update accounts index to display SimpleFin items - Add SimpleFin option to account method selector - Include SimpleFin in settings navigation
- Add pending_account_setup field to SimpleFin items - Enable pausing sync for user account type selection - Allow users to choose account types during import - Prevent automatic account creation until user confirms
- Add SimplefinItem model tests with fixtures - Add SimplefinAccount model tests - Add SimplefinItemsController tests - Include test coverage for sync and account creation
- Update sync button routing to handle SimpleFin accounts - Add SimpleFin item sync path alongside existing Plaid support - Prevent NoMethodError when viewing SimpleFin-linked accounts - Support proper sync routing for Plaid, SimpleFin, and manual accounts
- Add subtype database columns to all accountable models - Create Stimulus controller for dynamic subtype dropdown interaction - Add delegation from Account to accountable subtype for clean API access - Update SimpleFin account setup form with working subtype selection - Fix account display to show proper subtype labels instead of generic "Cash" Users can now select both account type and subtype during SimpleFin import, and the selected subtypes are properly saved and displayed in the UI.
- Replace hardcoded colors with design system tokens throughout SimpleFin views - Fix method selector hover states to use bg-surface instead of bg-gray-50 - Update SimpleFin form to use styled_form_with and standard form patterns - Replace custom button styling with design system button components - Fix info boxes and containers to use bg-surface and border-primary - Replace hardcoded green/blue colors with text-primary, text-secondary, text-link - Remove custom text area styling to allow form builder defaults (dark mode support) All SimpleFin components now properly adapt to both light and dark themes with correct contrast and visibility.
- Fix upsert method to handle string/symbol keys with indifferent access - Add missing show route and view for SimpleFin items - Fix test fixtures to use correct user references - Update test data to match real-world JSON format (string keys, BigDecimal) - Apply code formatting and linting fixes (rubocop, erb_lint) - Ensure all SimpleFin tests pass (16/16 passing)
- Remove map_simplefin_type_to_accountable_type method (no longer needed) - Remove create_from_simplefin_account method (manual setup only) - Simplify account type selection UI to not pre-select defaults - Update processor to log error if account missing (safety check) - All account creation now goes through manual user selection flow
SimpleFin is primarily for North American financial institutions, so only show the option when US banking connections are available.
![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!
I've implemented the initial integration for SimpleFin, a new way for users to connect their bank accounts and automatically import financial data. This feature provides an alternative to existing methods, expanding the range of financial institutions and data sources that can be linked to the application. The changes include new backend models and services for handling SimpleFin API interactions, as well as new UI components for managing these connections and setting up imported accounts.
Highlights
- SimpleFin Integration: I've introduced a comprehensive integration with SimpleFin, an open-source financial data bridge. This allows users to connect their bank accounts and automatically sync account balances and transactions, similar to the existing Plaid integration. This feature enhances the application's ability to provide automated financial tracking.
- Enhanced Account Management: I've updated the core
Accountmodel to support linking withSimplefinAccountrecords. TheAccountsControllerand associated views have been modified to display these new SimpleFin connections alongside manual and Plaid-linked accounts, providing a unified view of all user financial data. - Dedicated SimpleFin UI: New controllers and views have been added specifically for managing SimpleFin items. Users can now add new SimpleFin connections by providing a setup token, initiate manual syncs, and go through a guided process to categorize their imported SimpleFin accounts by type and subtype (e.g., checking, savings, credit card).
- Database Schema Expansion: To support the new integration, I've added several new database tables (
simplefin_items,simplefin_accounts) and columns. This includes linkingaccountstosimplefin_accounts, adding anexternal_idtotransactionsfor deduplication, and introducing asubtypecolumn to various accountable types for more detailed categorization.
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 or fill out our survey to provide feedback.
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 pull request introduces a significant new feature: integration with SimpleFin for bank account syncing. The changes are extensive, touching models, controllers, views, and adding new infrastructure like migrations and background jobs. Overall, the implementation is solid and follows existing patterns in the codebase. I've provided several suggestions to improve maintainability, reduce code duplication, and enhance error handling, particularly around the new controller logic and data processing classes. Great work on adding this new provider integration!
- Move SimpleFin item creation logic from controller to Family#create_simplefin_item! - Remove duplication between controller and model - Simplify controller to focus on web request/response handling - Remove unused simplefin_provider method - Follow Rails best practices for fat models, skinny controllers
- Remove fallback to Date.current when transaction dates fail to parse - Raise ArgumentError instead to ensure data integrity - Log detailed error messages for debugging - Skip transactions with invalid dates rather than using incorrect dates - Prevents hard-to-debug issues with balances and financial reports
- Remove debug console.log statements from JavaScript controller - Consolidate duplicate SimpleFin account creation methods into single method - Refactor SimplefinItemsController to reduce complexity with helper methods - Fix HTTParty thread-safety by moving SSL options to class level - Remove redundant HTTParty options from individual requests - Add proper error logging for invalid currency URIs - Extract sync button path logic to AccountsHelper#sync_path_for method - DRY up repeated subtype dropdown code with reusable partial and data structure All SimpleFin tests passing (16/16). Code quality improvements maintain backward compatibility while following Rails best practices.
The subtype attribute was moved from Account to individual accountable models to enable users to select specific subtypes during SimpleFin account import. This change allows for better account categorization and more precise display of account types (e.g., "HSA" instead of generic "Cash"). However, tests and the PlaidAccount processor weren't updated to work with the new delegation pattern. This commit fixes: - PlaidAccount::Processor now sets subtype on accountable and uses enrichable pattern to respect user locks - PropertiesController updated to handle subtype via accountable_attributes - Test fixtures corrected to set subtype on accountable models not Account - Tests updated to work with the delegated subtype pattern All originally failing tests now pass: - PropertiesControllerTest#test_updates_property_overview - PlaidAccount::ProcessorTest (2 failing tests) - AccountTest#test_gets_short/long_subtype_label
|
Super smooth experience until I added my accounts from BofA ... hitting this error: 
Will debug some more. |
|
I also don't see an obvious way to add new accounts that have been added to a "connection" (the UI asks me to create a new connection/token) |
|
I reverted the gate behind us commit as this was mistakenly tied to plaid being setup due to a confusingly labeled variable |
- Change date parameters from string format to Unix timestamps as required by SimpleFin API - Add better error handling for 400 Bad Request responses - Add more detailed error logging for debugging failed API calls This fixes the issue where SimpleFin was only returning recent transactions instead of historical data when start_date was provided.
- Add start_date parameter to SimpleFin API calls for historical data - Use 100-year lookback for first sync to capture all available history - Use 7-day buffer for incremental syncs to catch late-posting transactions - Fix transaction storage to prevent data loss during account updates - Remove verbose logging for cleaner output This ensures users get all their historical transactions on first sync, not just recent ones.
- Negate SimpleFin amounts to convert from banking convention to Maybe's format - SimpleFin: expenses negative, income positive (banking convention) - Maybe: expenses positive, income negative (internal convention) - Improve date parsing to handle multiple date formats (Unix timestamps, strings, Date objects) This fixes the issue where expenses showed as negative in the UI instead of positive.
…ities - Add belongs_to :simplefin_account association to Account model - Fix balance handling for credit cards and loans (use absolute value) - SimpleFin returns negative balances for liabilities, but Maybe expects positive This enables displaying organization names and ensures correct balance display.
- Show institution names under SimpleFin connection titles - Display organization names next to account names (e.g., "360 Checking • Capital One") - Add organization info to all SimpleFin account displays: - Account setup page - SimpleFin item details page - Regular account lists for SimpleFin accounts - Use org_data from SimpleFin accounts with fallback to institution_name This improves account identification by showing which financial institution each account belongs to throughout the SimpleFin workflow.
- Replace custom styles with DS components (DS::FilledIcon, DS::Link, DS::Button) - Use proper design system tokens instead of hardcoded colors - Fix form select styling to match design system patterns - Update empty states to use consistent styling - Ensure all SimpleFin views follow the app's design system This makes the SimpleFin interface consistent with the rest of the app.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces SimpleFin integration as an alternative to Plaid for financial data connectivity. SimpleFin is a financial data aggregation service that allows users to connect their bank accounts and automatically sync transactions and balances.
- Adds complete SimpleFin integration including models, controllers, views, and API client
- Refactors account subtype handling by moving
subtypefield from Account to accountable models - Adds account setup flow for manually selecting account types when importing from SimpleFin
Reviewed Changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/models/simplefin_*.rb | Core SimpleFin models for items, accounts, syncing, and data processing |
| app/models/provider/simplefin.rb | HTTP client for SimpleFin API communication |
| app/controllers/simplefin_items_controller.rb | Controller for managing SimpleFin connections and account setup |
| app/views/simplefin_items/ | Complete UI for SimpleFin connection management and account setup |
| db/migrate/ | Database migrations for SimpleFin tables and subtype refactoring |
| app/models/family/simplefin_connectable.rb | Family concern for SimpleFin integration |
| test/ | Comprehensive test coverage for SimpleFin functionality |
Comments suppressed due to low confidence (1)
| # Then save transactions separately (so they don't get overwritten) | ||
| if transactions && transactions.any? | ||
| simplefin_account.update!(raw_transactions_payload: transactions) | ||
| else |
There was a problem hiding this comment.
Empty else block should be removed or contain a comment explaining why it's intentionally empty
| else |
| class: "block text-sm font-medium text-primary mb-2" %> | ||
| <% selected_value = account_type == "Depository" ? | ||
| (simplefin_account.name.downcase.include?("checking") ? "checking" : | ||
| simplefin_account.name.downcase.include?("savings") ? "savings" : "") : "" %> |
There was a problem hiding this comment.
Complex logic for determining selected_value should be extracted to a helper method for better readability and testability
| simplefin_account.name.downcase.include?("savings") ? "savings" : "") : "" %> | |
| <%# Helper method for determining selected_value %> | |
| <% def selected_subtype_value(account_type, account) -%> | |
| if account_type == "Depository" | |
| name = account.name.downcase | |
| if name.include?("checking") | |
| "checking" | |
| elsif name.include?("savings") | |
| "savings" | |
| else | |
| "" | |
| end | |
| else | |
| "" | |
| end | |
| end -%> | |
| <div class="subtype-select" data-type="<%= account_type %>" style="display: none;"> | |
| <% if subtype_config[:options].present? %> | |
| <%= label_tag "account_subtypes[#{simplefin_account.id}]", subtype_config[:label], | |
| class: "block text-sm font-medium text-primary mb-2" %> | |
| <% selected_value = selected_subtype_value(account_type, simplefin_account) %> |
| # Handle both string and symbol keys | ||
| data = transaction_data.with_indifferent_access | ||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] Empty line after variable assignment should be removed for consistent code formatting
Co-authored-by: Copilot <[email protected]> Signed-off-by: Juan José Mata <[email protected]>
Signed-off-by: Juan José Mata <[email protected]>
|
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 SimpleFin integration across the app: new provider client, models, syncer/importer pipeline, controller and views, routes, and migrations. Updates Accounts and Family to link SimpleFin data, adjusts properties strong params, introduces a Stimulus controller for subtype selection, and centralizes sync path logic. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as App (UI)
participant C as SimplefinItemsController
participant F as Family
participant P as Provider::Simplefin
participant I as SimplefinItem
U->>A: Enter setup token
A->>C: POST /simplefin_items
C->>F: create_simplefin_item!(setup_token, item_name)
F->>P: claim_access_url(token)
P-->>F: access_url
F->>I: create! (name, access_url)
I->>I: sync_later
C-->>A: Redirect with notice
sequenceDiagram
participant U as User
participant A as App (UI)
participant C as SimplefinItemsController
participant I as SimplefinItem
participant SY as SimplefinItem::Syncer
participant IM as SimplefinItem::Importer
participant P as Provider::Simplefin
participant SA as SimplefinAccount
participant AC as Account
U->>A: Click Sync
A->>C: POST /simplefin_items/:id/sync
C->>I: sync_later
I->>SY: perform_sync(sync)
SY->>I: import_latest_simplefin_data
I->>IM: import
IM->>P: get_accounts(access_url, window)
P-->>IM: accounts snapshot
IM->>I: upsert_simplefin_snapshot!(...)
IM->>SA: upsert_simplefin_snapshot!/transactions
SY->>I: detect unlinked accounts?
alt Unlinked exist
SY->>I: update!(pending_account_setup: true)
SY-->>C: return
else None
SY->>I: process_accounts
I->>AC: schedule_account_syncs
end
sequenceDiagram
participant U as User
participant A as App (UI)
participant C as SimplefinItemsController
participant I as SimplefinItem
participant SA as SimplefinAccount
participant AC as Account
U->>A: Open Setup Accounts
A->>C: GET /simplefin_items/:id/setup_accounts
C-->>A: Render form (types/subtypes)
U->>A: Submit selections
A->>C: POST /.../complete_account_setup
loop each selected SA
C->>AC: Account.create_from_simplefin_account(SA, type, subtype)
C->>SA: link to Account
end
C->>I: update!(pending_account_setup: false)
C-->>A: Redirect with success
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (10)
app/views/accounts/show/_header.html.erb (1)
35-35: Excellent refactoring to use helper method.This addresses the previous review feedback by centralizing the sync path logic into the
sync_path_for(account)helper method, significantly improving code maintainability and readability.app/views/simplefin_items/_subtype_select.html.erb (1)
5-7: Extract selected_value logic to a helper for readability and reuse.Inline inference from account name is brittle and hard to test. Move it to a helper and call it here.
Apply this diff:
- <% selected_value = account_type == "Depository" ? - (simplefin_account.name.downcase.include?("checking") ? "checking" : - simplefin_account.name.downcase.include?("savings") ? "savings" : "") : "" %> + <% selected_value = selected_simplefin_subtype(account_type, simplefin_account.name) %>Add helper (app/helpers/simplefin_items_helper.rb):
module SimplefinItemsHelper def selected_simplefin_subtype(account_type, account_name) return "" unless account_type.to_s.casecmp("Depository").zero? name = account_name.to_s.downcase return "checking" if name.include?("checking") return "savings" if name.include?("savings") "" end endapp/models/simplefin_item/importer.rb (3)
13-15: Remove empty conditional blockThe empty if-else block serves no purpose and should be removed for cleaner code.
# Determine start date based on sync history start_date = determine_sync_start_date - if start_date - else - end - accounts_data = simplefin_provider.get_accounts(
43-43: Typo in comment- return 100.years.ago # Set to 100 years for first sync to get everything just to be sure + return 100.years.ago # Set to 100 years for first sync to get everything just to be sure
68-71: Remove empty else blockThe empty else block after checking for transactions should be removed.
# Then save transactions separately (so they don't get overwritten) if transactions && transactions.any? simplefin_account.update!(raw_transactions_payload: transactions) - else endapp/models/provider/simplefin.rb (3)
4-5: Thread-safety concern with class-level configurationThe
default_options.merge!modifies shared class-level state, which can lead to thread-safety issues in multi-threaded environments. Consider using class-level configuration properly.class Provider::Simplefin include HTTParty headers "User-Agent" => "Sure Finance SimpleFin Client" - default_options.merge!(verify: true, ssl_verify_mode: :peer) + + # Set default options at class level + default_options[:verify] = true + default_options[:ssl_verify_mode] = :peer
49-49: Add explicit SSL/TLS options for consistencyFor consistency and to ensure secure connections, add SSL/TLS options explicitly.
# The access URL already contains HTTP Basic Auth credentials - response = HTTParty.get(accounts_url) + response = HTTParty.get(accounts_url, verify: true, ssl_verify_mode: :peer)
14-14: Add explicit SSL/TLS options for securityThe HTTParty request doesn't specify SSL/TLS options explicitly. Since the class-level defaults might not be applied correctly, consider adding them explicitly to ensure secure connections.
- response = HTTParty.post(claim_url) + response = HTTParty.post(claim_url, verify: true, ssl_verify_mode: :peer)app/models/simplefin_account/processor.rb (1)
38-38: Remove unnecessary empty line# Handle both string and symbol keys data = transaction_data.with_indifferent_access - # Convert SimpleFin transaction to internal Transaction formatapp/controllers/simplefin_items_controller.rb (1)
16-42: Consider extracting the SimpleFin connection logic into a service objectThe
createaction contains significant business logic including token validation, API calls, object creation, and comprehensive error handling. This violates the "fat model, skinny controller" principle and makes the code harder to test and maintain.Consider creating a service object like
Simplefin::ConnectionCreatorthat encapsulates all this logic:def create setup_token = simplefin_params[:setup_token] return render_error("Please enter a SimpleFin setup token.") if setup_token.blank? result = Simplefin::ConnectionCreator.new( family: Current.family, setup_token: setup_token ).call if result.success? redirect_to simplefin_items_path, notice: "SimpleFin connection added successfully! Your accounts will appear shortly as they sync in the background." else render_error(result.error_message, setup_token) end end
🧹 Nitpick comments (24)
test/fixtures/depositories.yml (1)
2-2: Add missing newline at end of file.The static analysis tool correctly identified a formatting issue. YAML files should end with a newline character.
Apply this diff to add the required newline:
-two: { subtype: checking } +two: { subtype: checking } +db/migrate/20250807170943_add_subtype_to_accountables.rb (1)
3-11: Consider adding database indexes for subtype columns.Since subtype is likely to be used for filtering and querying operations (especially in reports and account categorization), adding indexes on frequently queried subtype columns would improve performance.
Consider adding indexes for the most commonly queried accountable types:
add_column :depositories, :subtype, :string + add_index :depositories, :subtype add_column :investments, :subtype, :string + add_index :investments, :subtype add_column :loans, :subtype, :string + add_index :loans, :subtype add_column :credit_cards, :subtype, :string + add_index :credit_cards, :subtypedb/migrate/20250807143728_create_simplefin_items.rb (1)
10-11: Consider using an enum for status field.The
statusfield with a default of "good" suggests multiple status values. Consider defining an enum in the corresponding model to ensure type safety and better maintainability.- t.string :status, default: "good" + t.integer :status, default: 0Then in the
SimplefinItemmodel, add:enum status: { good: 0, error: 1, pending: 2 }app/views/settings/_settings_nav.html.erb (1)
13-13: Consider conditional rendering for SimpleFin navigation item.The SimpleFin navigation item is always visible, but you might want to conditionally show it based on feature flags or user permissions, similar to how billing/self-hosting items are handled.
- { label: "SimpleFin", path: simplefin_items_path, icon: "building-2" }, + { label: "SimpleFin", path: simplefin_items_path, icon: "building-2", if: can_connect_simplefin? },app/views/accounts/_account.html.erb (1)
21-23: Consider extracting organization name logic to a helper method.While the current implementation works, consider extracting this logic to improve readability and reusability.
In
app/helpers/accounts_helper.rb:def simplefin_org_name(account) account.simplefin_account&.org_data&.dig('name') endThen update the view:
- <% if account.simplefin_account&.org_data&.dig('name') %> - <span class="text-secondary">• <%= account.simplefin_account.org_data["name"] %></span> - <% end %> + <% if simplefin_org_name(account) %> + <span class="text-secondary">• <%= simplefin_org_name(account) %></span> + <% end %>db/migrate/20250808141424_add_balance_date_to_simplefin_accounts.rb (1)
3-3: Consider precision and indexing for balance_date (temporal queries).If you’ll query or sort by balance_date (e.g., “latest snapshot”), add microsecond precision and an index.
Apply this diff:
- add_column :simplefin_accounts, :balance_date, :datetime + add_column :simplefin_accounts, :balance_date, :datetime, precision: 6 + add_index :simplefin_accounts, :balance_dateapp/models/account/linkable.rb (1)
9-12: linked? logic is correct; consider using associations for clarity.Minor: Using associations (plaid_account.present? || simplefin_account.present?) avoids stale foreign key presence if associations change.
- def linked? - plaid_account_id.present? || simplefin_account_id.present? - end + def linked? + plaid_account.present? || simplefin_account.present? + endapp/views/simplefin_items/index.html.erb (1)
6-8: Consider i18n for static UI strings.Hard-coded strings make localization harder. Extract to I18n keys (e.g., simplefin.items.index.title, subtitle).
app/views/simplefin_items/_subtype_select.html.erb (2)
8-10: Option label text reads well; consider i18n and placeholder clarity.Use I18n for “Select subtype/type” and consider a disabled placeholder option to avoid accidental empty submission.
Apply this diff:
- options_for_select([["Select #{account_type == 'Depository' ? 'subtype' : 'type'}", ""]] + subtype_config[:options], selected_value), + options_for_select([[t('simplefin.subtypes.placeholder', default: "Select #{account_type == 'Depository' ? 'subtype' : 'type'}"), "", { disabled: true }]] + subtype_config[:options], selected_value),
12-13: Ensure subtype_config[:message] is user-safe and localized.ERB escapes by default, but prefer I18n-managed messages to centralize copy.
test/controllers/simplefin_items_controller_test.rb (2)
30-38: Soft-delete test is good; add an assertion on side-effects if any background cleanup is queued.If destroy triggers background cleanup (e.g., canceling tokens or removing webhooks), assert the job enqueue to better cover behavior.
If you have a job, add:
assert_enqueued_with(job: Simplefin::CleanupItemJob, args: [@simplefin_item.id]) do delete simplefin_item_url(@simplefin_item) end
40-44: Sync test: consider asserting job enqueue to improve coverage.Verifying redirect and flash is good; asserting the sync job enqueued will better validate behavior.
If applicable:
assert_enqueued_with(job: Simplefin::SyncItemJob, args: [@simplefin_item.id]) do post sync_simplefin_item_url(@simplefin_item) endapp/models/simplefin_item/sync_complete_event.rb (1)
15-20: Prefer passing the record as the Turbo target over stringly-typed DOM idsUsing the model as the target lets Turbo compute dom_id internally and avoids coupling to the id format.
- simplefin_item.broadcast_replace_to( - simplefin_item.family, - target: "simplefin_item_#{simplefin_item.id}", - partial: "simplefin_items/simplefin_item", - locals: { simplefin_item: simplefin_item } - ) + simplefin_item.broadcast_replace_to( + simplefin_item.family, + target: simplefin_item, + partial: "simplefin_items/simplefin_item", + locals: { simplefin_item: simplefin_item } + )app/javascript/controllers/account_type_selector_controller.js (1)
18-27: Avoid shadowing variable names inside the loopThe inner selectElement variable shadows the outer one, which is easy to misread. Rename for clarity.
- const subtypeSelects = container.querySelectorAll('.subtype-select') - subtypeSelects.forEach(select => { - select.style.display = 'none' - // Clear the name attribute so it doesn't get submitted - const selectElement = select.querySelector('select') - if (selectElement) { - selectElement.removeAttribute('name') - } - }) + const subtypeSelects = container.querySelectorAll('.subtype-select') + subtypeSelects.forEach(wrapper => { + wrapper.style.display = 'none' + // Clear the name attribute so it doesn't get submitted + const innerSelect = wrapper.querySelector('select') + if (innerSelect) { + innerSelect.removeAttribute('name') + } + })app/views/simplefin_items/show.html.erb (2)
13-16: Use Turbo’s confirm attribute for consistencyWhen using Turbo, prefer data-turbo-confirm over the legacy data-confirm to ensure the confirm dialog is honored uniformly.
- <%= button_to simplefin_item_path(@simplefin_item), method: :delete, data: { confirm: "Are you sure?" }, class: "inline-flex items-center gap-2 px-4 py-2 bg-destructive border border-destructive rounded-lg text-white font-medium hover:bg-destructive-hover focus:ring-2 focus:ring-destructive focus:ring-offset-2" do %> + <%= button_to simplefin_item_path(@simplefin_item), method: :delete, data: { turbo_confirm: "Are you sure?" }, class: "inline-flex items-center gap-2 px-4 py-2 bg-destructive border border-destructive rounded-lg text-white font-medium hover:bg-destructive-hover focus:ring-2 focus:ring-destructive focus:ring-offset-2" do %>
63-66: Unify currency formatting and honor per-account currency codesThis uses default currency (likely USD). Elsewhere (setup view) you pass unit: simplefin_account.currency. Prefer a dedicated helper to consistently format amounts given an ISO code, avoiding mixing symbols and codes.
- <p class="text-sm font-medium text-primary"> - <%= number_to_currency(simplefin_account.current_balance || 0) %> - </p> + <p class="text-sm font-medium text-primary"> + <%= format_currency(simplefin_account.current_balance, simplefin_account.currency) %> + </p>Example helper to add (outside this file):
# app/helpers/currency_helper.rb module CurrencyHelper # Simple formatter that shows ISO code prefix for clarity across currencies. # Swap to MoneyRails or a real formatter if available. def format_currency(amount, currency_code) value = amount || 0 number_to_currency(value, unit: (currency_code.presence || "USD"), format: "%u %n") end endWant me to wire this helper everywhere we show SimpleFin balances?
app/views/simplefin_items/setup_accounts.html.erb (1)
51-52: Align currency formatting with a consistent helperTo keep currency display consistent with the show page recommendation and respect ISO codes, use a helper.
- Balance: <%= number_to_currency(simplefin_account.current_balance || 0, unit: simplefin_account.currency) %> + Balance: <%= format_currency(simplefin_account.current_balance, simplefin_account.currency) %>test/models/simplefin_account_test.rb (2)
11-18: Use BigDecimal in fixtures/setup to avoid float rounding in testsInitialize monetary values with BigDecimal to prevent subtle float precision issues.
- @simplefin_account = SimplefinAccount.create!( + @simplefin_account = SimplefinAccount.create!( simplefin_item: @simplefin_item, name: "Test Checking Account", account_id: "test_checking_123", currency: "USD", account_type: "checking", - current_balance: 1500.50 + current_balance: BigDecimal("1500.50") )
47-72: Consider asserting currency and subtype persistence in snapshot upsertYou already validate balances, dates, name, extra, and org_data. Adding assertions that currency and subtype are persisted would tighten coverage for data mapping.
app/models/simplefin_account/processor.rb (1)
82-83: Clarify the application name in commentThe comment mentions "Maybe" as the application name, but based on the repository context (we-promise/sure), this appears to be the "Sure" application. Please verify and update the comment for clarity.
# 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 + # Sure expects opposite convention (expenses positive, income negative) + # So we negate the amount to convert from SimpleFin to Sure format -parsed_amountapp/models/simplefin_account.rb (1)
89-92: Missing space before method definitionAdd a blank line before the method definition for consistent formatting.
nil end + def has_balance return if current_balance.present? || available_balance.present?app/controllers/simplefin_items_controller.rb (1)
55-90: Add null checks for associated records to prevent NoMethodErrorThe
setup_accountsaction loads SimpleFin accounts that don't have associated Account records, but doesn't verify that the SimplefinItem actually has any accounts to set up. This could lead to confusing UX if the view doesn't handle empty states properly.Consider adding a check and redirect if there are no accounts to set up:
def setup_accounts @simplefin_accounts = @simplefin_item.simplefin_accounts.includes(:account).where(accounts: { id: nil }) + + if @simplefin_accounts.empty? + redirect_to simplefin_items_path, notice: "All accounts have already been set up." + return + end + @account_type_options = [app/models/simplefin_item.rb (2)
52-58: Add error handling for upsert operationsThe
upsert_simplefin_snapshot!method usessave!which will raise an exception if validation fails. Since this is called during the import process, a validation failure could leave the sync in an inconsistent state.Consider adding validation error handling or using a transaction:
def upsert_simplefin_snapshot!(accounts_snapshot) + transaction do assign_attributes( raw_payload: accounts_snapshot, ) save! + end +rescue ActiveRecord::RecordInvalid => e + Rails.logger.error "Failed to save SimpleFin snapshot: #{e.message}" + raise end
27-30: Consider adding a state machine for deletion workflowThe
destroy_latermethod sets a flag and enqueues a job, but there's no tracking of the deletion state (e.g., pending, in_progress, failed). This could lead to orphaned records if the job fails.Consider using a state machine or enum for tracking deletion status:
+ enum :deletion_status, { + active: "active", + pending_deletion: "pending_deletion", + deleting: "deleting", + deleted: "deleted" + }, default: :active + def destroy_later - update!(scheduled_for_deletion: true) + update!(deletion_status: :pending_deletion) DestroyJob.perform_later(self) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
Gemfile(1 hunks)app/controllers/accounts_controller.rb(1 hunks)app/controllers/properties_controller.rb(1 hunks)app/controllers/simplefin_items_controller.rb(1 hunks)app/helpers/accounts_helper.rb(1 hunks)app/javascript/controllers/account_type_selector_controller.js(1 hunks)app/models/account.rb(3 hunks)app/models/account/linkable.rb(1 hunks)app/models/family.rb(1 hunks)app/models/family/simplefin_connectable.rb(1 hunks)app/models/plaid_account/processor.rb(1 hunks)app/models/provider/simplefin.rb(1 hunks)app/models/simplefin_account.rb(1 hunks)app/models/simplefin_account/processor.rb(1 hunks)app/models/simplefin_item.rb(1 hunks)app/models/simplefin_item/importer.rb(1 hunks)app/models/simplefin_item/provided.rb(1 hunks)app/models/simplefin_item/sync_complete_event.rb(1 hunks)app/models/simplefin_item/syncer.rb(1 hunks)app/views/accounts/_account.html.erb(1 hunks)app/views/accounts/index.html.erb(1 hunks)app/views/accounts/new/_method_selector.html.erb(2 hunks)app/views/accounts/show/_header.html.erb(1 hunks)app/views/settings/_settings_nav.html.erb(1 hunks)app/views/simplefin_items/_simplefin_item.html.erb(1 hunks)app/views/simplefin_items/_subtype_select.html.erb(1 hunks)app/views/simplefin_items/index.html.erb(1 hunks)app/views/simplefin_items/new.html.erb(1 hunks)app/views/simplefin_items/setup_accounts.html.erb(1 hunks)app/views/simplefin_items/show.html.erb(1 hunks)app/views/transactions/show.html.erb(1 hunks)config/routes.rb(1 hunks)db/migrate/20250807143728_create_simplefin_items.rb(1 hunks)db/migrate/20250807143819_create_simplefin_accounts.rb(1 hunks)db/migrate/20250807144230_add_simplefin_account_id_to_accounts.rb(1 hunks)db/migrate/20250807144857_add_external_id_to_transactions.rb(1 hunks)db/migrate/20250807163541_add_pending_account_setup_to_simplefin_items.rb(1 hunks)db/migrate/20250807170943_add_subtype_to_accountables.rb(1 hunks)db/migrate/20250808141424_add_balance_date_to_simplefin_accounts.rb(1 hunks)db/migrate/20250808143007_add_extra_simplefin_account_fields.rb(1 hunks)db/schema.rb(13 hunks)test/controllers/properties_controller_test.rb(1 hunks)test/controllers/simplefin_items_controller_test.rb(1 hunks)test/fixtures/accounts.yml(0 hunks)test/fixtures/depositories.yml(1 hunks)test/models/account_test.rb(1 hunks)test/models/plaid_account/processor_test.rb(1 hunks)test/models/simplefin_account_test.rb(1 hunks)test/models/simplefin_item_test.rb(1 hunks)
💤 Files with no reviewable changes (1)
- test/fixtures/accounts.yml
🧰 Additional context used
🧬 Code Graph Analysis (26)
test/models/simplefin_item_test.rb (2)
test/models/simplefin_account_test.rb (1)
setup(3-84)test/controllers/simplefin_items_controller_test.rb (1)
setup(3-45)
app/controllers/accounts_controller.rb (3)
app/controllers/accountable_sparklines_controller.rb (1)
family(26-28)app/models/assistant/function.rb (1)
family(78-80)app/models/sync.rb (1)
family(190-196)
app/helpers/accounts_helper.rb (1)
app/models/plaid_item/webhook_processor.rb (1)
plaid_item(40-42)
test/models/simplefin_account_test.rb (2)
app/models/simplefin_account.rb (2)
upsert_simplefin_snapshot!(9-27)upsert_simplefin_transactions_snapshot!(29-35)app/models/account/current_balance_manager.rb (1)
current_balance(16-23)
db/migrate/20250807143819_create_simplefin_accounts.rb (3)
db/migrate/20250807143728_create_simplefin_items.rb (2)
change(1-20)change(2-19)db/migrate/20250807144230_add_simplefin_account_id_to_accounts.rb (2)
change(1-5)change(2-4)db/migrate/20250807144857_add_external_id_to_transactions.rb (2)
change(1-6)change(2-5)
db/migrate/20250807143728_create_simplefin_items.rb (2)
db/migrate/20250807143819_create_simplefin_accounts.rb (2)
change(1-20)change(2-19)db/migrate/20250807144230_add_simplefin_account_id_to_accounts.rb (2)
change(1-5)change(2-4)
app/models/simplefin_item/syncer.rb (5)
app/models/simplefin_account/processor.rb (2)
attr_reader(1-109)initialize(4-6)app/models/simplefin_item/importer.rb (2)
attr_reader(1-107)initialize(4-7)app/models/provider/simplefin.rb (3)
attr_reader(79-86)initialize(7-8)initialize(82-85)app/controllers/simplefin_items_controller.rb (1)
sync(49-52)app/models/simplefin_item.rb (3)
import_latest_simplefin_data(32-34)process_accounts(36-40)schedule_account_syncs(42-50)
app/models/family.rb (2)
app/models/account.rb (1)
include(1-212)app/models/plaid_item.rb (1)
include(1-120)
test/models/account_test.rb (1)
app/models/account.rb (2)
short_subtype_label(185-187)long_subtype_label(190-192)
app/javascript/controllers/account_type_selector_controller.js (1)
app/javascript/controllers/multi_select_controller.js (1)
event(17-17)
app/models/family/simplefin_connectable.rb (2)
app/models/provider/simplefin.rb (1)
claim_access_url(10-25)app/models/concerns/syncable.rb (1)
sync_later(14-35)
app/models/simplefin_item/provided.rb (1)
app/controllers/simplefin_items_controller.rb (1)
new(12-14)
test/models/plaid_account/processor_test.rb (9)
app/models/plaid_account/investments/transactions_processor.rb (1)
account(22-24)app/models/plaid_account/investments/holdings_processor.rb (1)
account(43-45)app/models/plaid_account/liabilities/credit_processor.rb (1)
account(18-20)app/models/plaid_account/liabilities/mortgage_processor.rb (1)
account(18-20)app/models/plaid_account/liabilities/student_loan_processor.rb (1)
account(20-22)app/models/plaid_account/transactions/processor.rb (1)
account(41-43)app/models/plaid_entry/processor.rb (1)
account(52-54)app/controllers/accountable_sparklines_controller.rb (1)
accountable(30-32)app/models/concerns/enrichable.rb (1)
lock_attr!(61-63)
test/controllers/properties_controller_test.rb (1)
app/controllers/accountable_sparklines_controller.rb (1)
accountable(30-32)
app/models/simplefin_item/importer.rb (4)
app/models/provider/simplefin.rb (4)
attr_reader(79-86)initialize(7-8)initialize(82-85)get_accounts(27-66)app/models/simplefin_item/provided.rb (1)
simplefin_provider(4-6)app/models/simplefin_item.rb (2)
upsert_simplefin_snapshot!(52-58)upsert_simplefin_institution_snapshot!(60-69)app/models/concerns/syncable.rb (1)
last_synced_at(53-55)
app/models/simplefin_account/processor.rb (2)
app/models/simplefin_item/importer.rb (2)
attr_reader(1-107)initialize(4-7)app/models/provider/simplefin.rb (3)
attr_reader(79-86)initialize(7-8)initialize(82-85)
test/controllers/simplefin_items_controller_test.rb (2)
test/models/simplefin_account_test.rb (1)
setup(3-84)test/models/simplefin_item_test.rb (1)
setup(3-64)
db/migrate/20250807144230_add_simplefin_account_id_to_accounts.rb (2)
db/migrate/20250807143728_create_simplefin_items.rb (2)
change(1-20)change(2-19)db/migrate/20250807143819_create_simplefin_accounts.rb (2)
change(1-20)change(2-19)
app/models/provider/simplefin.rb (3)
app/models/account.rb (1)
start_date(166-169)app/models/simplefin_account/processor.rb (2)
initialize(4-6)attr_reader(1-109)app/models/simplefin_item/importer.rb (2)
initialize(4-7)attr_reader(1-107)
app/models/plaid_account/processor.rb (2)
app/models/plaid_account/type_mappable.rb (1)
map_accountable(6-17)app/models/concerns/enrichable.rb (1)
enrich_attributes(34-51)
app/models/simplefin_item.rb (5)
app/models/provider/simplefin.rb (1)
include(1-87)app/models/simplefin_account.rb (1)
upsert_simplefin_snapshot!(9-27)app/models/simplefin_item/provided.rb (1)
simplefin_provider(4-6)app/models/simplefin_item/importer.rb (1)
import(9-35)app/models/concerns/syncable.rb (1)
sync_later(14-35)
app/models/simplefin_item/sync_complete_event.rb (4)
app/models/provider/simplefin.rb (3)
attr_reader(79-86)initialize(7-8)initialize(82-85)app/models/simplefin_item/syncer.rb (2)
attr_reader(1-34)initialize(4-6)app/models/plaid_item/accounts_snapshot.rb (1)
accounts(10-12)app/models/concerns/syncable.rb (1)
broadcast_sync_complete(45-47)
app/models/simplefin_account.rb (2)
app/models/simplefin_item.rb (1)
upsert_simplefin_snapshot!(52-58)app/models/account/current_balance_manager.rb (1)
current_balance(16-23)
app/controllers/simplefin_items_controller.rb (3)
app/models/family/simplefin_connectable.rb (1)
create_simplefin_item!(12-24)app/models/account.rb (1)
create_from_simplefin_account(78-99)app/models/concerns/syncable.rb (1)
sync_later(14-35)
app/models/account.rb (2)
app/models/simplefin_account.rb (1)
belongs_to(1-93)lib/money/arithmetic.rb (1)
abs(41-43)
db/schema.rb (2)
app/controllers/simplefin_items_controller.rb (1)
index(4-7)app/controllers/accounts_controller.rb (1)
index(5-11)
🪛 YAMLlint (1.37.1)
test/fixtures/depositories.yml
[error] 2-2: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (41)
db/migrate/20250807143819_create_simplefin_accounts.rb (1)
8-9: Review decimal precision for new balance fieldsThe new columns in
db/migrate/20250807143819_create_simplefin_accounts.rbuse:t.decimal :current_balance, precision: 19, scale: 4 t.decimal :available_balance, precision: 19, scale: 4• This matches the existing definitions in
account_balances.balance(precision: 19, scale: 4)security_prices.price(precision: 19, scale: 4)• With precision 19 and scale 4 you get up to 15 integer digits and 4 decimals (max value ≈ 9 999 999 999 999 999.9999).
Please confirm that:
- This range covers your largest expected balances (including any high-value or crypto use cases).
- It aligns with the SimpleFin API’s currency precision requirements.
db/migrate/20250807163541_add_pending_account_setup_to_simplefin_items.rb (1)
3-3: LGTM!The migration properly adds a boolean column with appropriate defaults and null constraints. The
default: false, null: falseensures data consistency.app/models/plaid_account/processor.rb (3)
37-41: LGTM! Improved accountable initialization.The explicit accountable creation and assignment improves code clarity and ensures the accountable exists before enrichment operations. This pattern aligns well with the subtype delegation changes mentioned in the AI summary.
45-50: LGTM! Consistent use of enrichable pattern.The refactor to use
enrich_attributesfor the name field maintains consistency with the enrichable pattern and properly respects locked attributes. The source attribution to "plaid" is appropriate for auditing.
52-58: Verify accountable exists before enrichment.While the code creates the accountable at lines 37-41, there's a potential race condition or edge case where
account.accountablecould still be nil when reaching this enrichment call.Consider adding a safety check:
# Enrich subtype on the accountable, respecting locks -account.accountable.enrich_attributes( +if account.accountable + account.accountable.enrich_attributes( { subtype: map_subtype(plaid_account.plaid_type, plaid_account.plaid_subtype) }, source: "plaid" - ) + ) +enddb/migrate/20250807144857_add_external_id_to_transactions.rb (1)
3-4: LGTM! Proper external ID implementation.The migration correctly adds an
external_idcolumn with an index, which is essential for efficient lookups when syncing with external systems like SimpleFin. The index will improve performance for deduplication queries.db/migrate/20250807143728_create_simplefin_items.rb (2)
4-4: LGTM!The foreign key relationship to
familyusing UUID type is correctly implemented and aligns with the existing database schema patterns.
14-15: LGTM!Using
jsonbcolumns for raw payloads is appropriate for storing flexible API response data from SimpleFin.app/views/transactions/show.html.erb (1)
87-87: Minor formatting improvement applied.The ERB tag closing was moved to the same line for consistency. This is a good cleanup change.
app/controllers/accounts_controller.rb (1)
8-8: LGTM!The addition of
@simplefin_itemsfollows the same pattern as@plaid_itemsand properly integrates the SimpleFin data into the accounts index view.test/models/plaid_account/processor_test.rb (2)
52-52: LGTM! Subtype delegation change implemented correctly.The test correctly updates the accountable's subtype instead of the account's subtype, aligning with the architectural change where subtype is now delegated to the accountable.
55-55: LGTM! Lock attribute call updated correctly.The
lock_attr!call has been correctly moved from the account to the accountable, maintaining consistency with the subtype delegation pattern.test/controllers/properties_controller_test.rb (2)
42-45: LGTM! Nested attributes update implemented correctly.The test correctly uses
accountable_attributesto update the subtype through the nested relationship, which aligns with the controller changes that moved subtype handling to the accountable level.
52-52: LGTM! Test expectation updated correctly.The test expectation correctly reflects that
account.subtypenow delegates to the accountable's subtype, and the value "condominium" matches what was set in the nested attributes.app/views/accounts/_account.html.erb (1)
19-24: LGTM! Safe navigation prevents nil errors.The use of safe navigation (
&.) throughout the chain (account.simplefin_account&.org_data&.dig('name')) properly handles cases where SimpleFin data might not be present, preventing potential nil reference errors.db/migrate/20250808143007_add_extra_simplefin_account_fields.rb (1)
1-6: LGTM!The migration correctly adds JSONB columns for storing extra data and organization information from SimpleFin. The column types and names align with the usage in the SimpleFin integration codebase.
config/routes.rb (1)
254-261: LGTM!The SimpleFin routes are properly structured with all necessary actions for the SimpleFin integration workflow. The member routes for sync, setup_accounts, and complete_account_setup align with the controller implementation and match the pattern used for Plaid items.
app/views/accounts/index.html.erb (2)
24-24: LGTM!The empty state check now correctly includes all three connection types (manual, Plaid, and SimpleFin) to determine when to show the empty state.
32-35: LGTM!SimpleFin items are properly integrated into the accounts index view, following the same pattern as Plaid items with consistent sorting by created_at.
app/models/simplefin_item/provided.rb (1)
1-7: LGTM!The module correctly provides a memoized SimpleFin provider instance following the same pattern as the existing Plaid integration. The implementation is clean and follows Rails conventions.
app/helpers/accounts_helper.rb (1)
7-15: LGTM!The centralized sync path logic correctly handles all three account types (Plaid, SimpleFin, and manual) with proper fallback behavior. This eliminates code duplication and provides a consistent interface for determining sync paths.
test/models/account_test.rb (2)
18-24: LGTM!The test setup correctly creates an Investment instance as the accountable and properly wires it to the account. This aligns with the delegation pattern where subtype is delegated to the accountable.
30-30: LGTM!The test correctly updates the subtype on the accountable object rather than the account itself, which aligns with the delegation pattern where
subtypeis delegated to theaccountable.db/migrate/20250807144230_add_simplefin_account_id_to_accounts.rb (1)
1-5: LGTM!The migration correctly adds a nullable UUID foreign key reference to link accounts with SimpleFin accounts. The use of
add_referencewith proper options (null: true, foreign_key: true, type: :uuid) follows Rails best practices and maintains referential integrity.app/models/family.rb (1)
2-2: LGTM!The SimplefinConnectable mixin is correctly added to the Family model, enabling SimpleFin integration capabilities alongside the existing Plaid integration. The placement in the include list follows a logical order.
app/controllers/properties_controller.rb (1)
90-93: No action needed: nested attributes foraccountableare already enabled
TheAccountmodel declares
accepts_nested_attributes_for :accountable, update_only: true
(app/models/account.rb:33), so permittingaccountable_attributes(including :subtype) inproperty_paramswill take effect as expected.app/views/simplefin_items/index.html.erb (1)
10-15: Nice use of DS::Link for primary action.Clear CTA and consistent styling with design system.
app/views/simplefin_items/_subtype_select.html.erb (1)
1-1: Ignore account_type casing mismatch suggestion
Theaccount_typestring is sourced from the static@account_type_optionsin SimplefinItemsController (e.g."Depository"), not from upstream payloads—so the equality check against"Depository"will always match. No change required.Likely an incorrect or invalid review comment.
app/views/simplefin_items/new.html.erb (2)
9-15: Form setup is sound; double-check Turbo opt-out is necessary.If the create action responds with standard redirects/stream, you can likely keep Turbo enabled. If the provider flow depends on a full reload, leaving data: { turbo: false } is fine.
Would you like a follow-up commit to enable Turbo and adjust the controller response if needed?
17-22: Good guidance UX and external link handling.Clear instructions and target: "_blank" link are appropriate.
app/models/account/linkable.rb (1)
9-12: manual? now correctly includes SimpleFin linkage
Verified thatmanual?(alias for!linked?) reflects bothplaid_account_idandsimplefin_account_id. The sole usage ofaccount.manual?inapp/components/UI/account/activity_feed.html.erbwill now behave as expected, and there are no ActiveRecord scopes relying on the old “lack of Plaid” logic. No further changes needed.db/migrate/20250808141424_add_balance_date_to_simplefin_accounts.rb (1)
1-5: Migration is straightforward and reversible via schema rollback.No defaults or backfills—safe for online migration.
app/javascript/controllers/account_type_selector_controller.js (1)
29-43: No changes needed for subtype matching
TheselectedTypevalues ("Depository","CreditCard","Investment","Loan","OtherAsset","Skip") are defined in@account_type_optionsand used verbatim in yourdata-typeattributes via the same Ruby keys. This guarantees exact matches between the select’s value and the container’sdata-type. The initialif (selectedType === 'Skip') returncheck correctly handles the Skip option, and there’s no variance in casing or format to normalize.Likely an incorrect or invalid review comment.
app/models/family/simplefin_connectable.rb (1)
12-24: LGTM: Centralizes SimpleFin item creation and schedules initial syncCreation logic is appropriately moved here (per earlier feedback), with a sensible default name and immediate sync scheduling.
app/models/simplefin_item/syncer.rb (1)
20-29: LGTM: Post-import processing and downstream account sync schedulingProcessing and fan-out to account-level syncs aligns with the existing sync pipeline.
app/models/simplefin_item/importer.rb (1)
98-100: Verifiedrequires_updatestatus: enum defined correctly
TheSimplefinItemmodel declares:enum :status, { good: "good", requires_update: "requires_update" }, default: :goodand the
statuscolumn exists indb/schema.rb. No further changes needed.app/models/simplefin_account/processor.rb (1)
90-108: parse_date errors are already isolated to a single transaction
TheArgumentErroryou see inparse_dateis caught by the broadrescueinprocess_transaction(lines 66–69), so only that one transaction is skipped—the overall sync continues. By contrast,parse_amountquietly falls back to zero but still lets you know via a log. If you really want consistency, you could haveparse_amountraise and let the outer rescue skip bad‐amount transactions; otherwise, no changes toparse_dateare required here.Likely an incorrect or invalid review comment.
app/models/simplefin_account.rb (1)
63-65: Logging improvement already addressedGood to see the warning log was added as suggested in the previous review. This properly tracks malformed currency URLs from SimpleFin.
app/controllers/simplefin_items_controller.rb (2)
28-41: Improve error handling specificityThe generic
rescue => eblock might catch and suppress important errors that should bubble up to the application error handler (like database errors or programming errors). It's better to be specific about which exceptions you want to handle.Consider catching only expected exceptions:
- rescue => e + rescue StandardError => e + # Only catch StandardError and its descendants, not system errors + raise unless e.is_a?(Net::HTTPError) || e.is_a?(Timeout::Error) || e.is_a?(JSON::ParserError) Rails.logger.error("SimpleFin connection error: #{e.message}") render_error("An unexpected error occurred. Please try again or contact support.", setup_token)Likely an incorrect or invalid review comment.
107-112: No nestedbalanceis passed to CreditCard
Thecreate_from_simplefin_accountworkflow merges the top-levelbalanceinto the Account’s owncash_balanceand only places{ subtype, available_credit }intoaccountable_attributes. At no point doesbalanceenter the nestedCreditCardattributes, so you won’t see anUnknownAttributeError: balanceon the CreditCard model.db/schema.rb (1)
807-809: Good addition of external_id for transaction deduplicationThe addition of
external_idwith an index on thetransactionstable is a good practice for preventing duplicate transaction imports from SimpleFin. This will help maintain data integrity when syncing transactions.
| def complete_account_setup | ||
| account_types = params[:account_types] || {} | ||
| account_subtypes = params[:account_subtypes] || {} | ||
|
|
||
| account_types.each do |simplefin_account_id, selected_type| | ||
| # Skip accounts that the user chose not to add | ||
| next if selected_type == "Skip" |
There was a problem hiding this comment.
Fix the constant reference error that causes crashes when accessing credit card accounts
Based on the PR comments, users are encountering uninitialized constant Skip errors. The issue is on line 98 where selected_type == "Skip" is being checked as a string, but the error suggests the code might be trying to reference it as a constant elsewhere.
The error is likely not in this exact line but related to how the Skip value is being processed. Let me verify if there's an issue with the account creation logic that might be causing the reported errors.
#!/bin/bash
# Check for any references to Skip as a constant in the codebase
rg "Skip[^'\"]" --type ruby
# Check the Account model's create_from_simplefin_account method implementation
ast-grep --pattern 'def create_from_simplefin_account($$$)'
# Look for any Skip-related constant definitions
rg "^\s*(class|module)\s+Skip" --type ruby| if account_type == "CreditCard" || account_type == "Loan" | ||
| balance = balance.abs | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify balance conversion logic
The comment states SimpleFin returns negative balances for credit cards, but the logic uses abs for both CreditCard and Loan types. Verify this is correct for loans as well.
🌐 Web query:
Does SimpleFin API return negative balances for loan accounts similar to credit cards, or only for credit card accounts?
💡 Result:
Short answer: SimpleFIN itself does not guarantee that loan account balances will be returned as negative. The SimpleFIN protocol documents the balance and transaction-sign conventions but does not mandate “loan = negative” semantics — institutions/servers can and do return either sign. (simplefin.org)
Supporting details and recommendations
- The SimpleFIN spec defines "balance" as a numeric string and states that transaction amounts use the convention "positive = money deposited into the account" (so signs are relative to cash flow), but it does not spell out that loans must be negative like some other APIs do. That means how a loan or credit-card balance is signed is up to the server implementation. (simplefin.org)
- In practice developers have reported inconsistent signs for liability accounts (credit cards, car loans, mortgages) when importing via SimpleFIN — some institutions return positive balances for loans or credit cards, which can make them look like assets unless you normalize them. (github.com, reddit.com)
- By contrast, APIs such as Plaid explicitly document sign semantics for credit and loan accounts (credit: positive = amount owed; negative = lender owes you; loans: typically positive = principal remaining, negative only if lender owes you). If you need that behavior, you should normalize SimpleFIN data in your app. (plaid.com)
Practical approach (recommended)
- Treat the value from SimpleFIN as “raw.” Detect whether the account is a liability (credit card / loan / mortgage) from whatever metadata your SimpleFIN server provides (account name/type in extra fields or org conventions). Then convert to the sign convention your app expects (for example, if you want liabilities to appear negative for net-worth calculations, set normalized_balance = -abs(raw_balance) for liabilities).
- Always test with the specific institutions you plan to support — SimpleFIN servers vary by bank and you’ll see different behavior across providers. (simplefin.org, github.com)
If you want, tell me:
- which institutions or account types you’re integrating and what sign convention your app expects, and I’ll suggest specific normalization logic or sample code.
Citations:
- 1: https://www.simplefin.org/protocol.html?utm_source=chatgpt.com
- 2: https://www.simplefin.org/protocol.html?utm_source=chatgpt.com
- 3: https://github.com/actualbudget/actual/issues/3162?utm_source=chatgpt.com
- 4: https://www.reddit.com/r/budgetwithbuckets/comments/x2xajy?utm_source=chatgpt.com
- 5: https://plaid.com/docs/api/products/balance/?utm_source=chatgpt.com
- 6: https://www.simplefin.org/protocol.html?utm_source=chatgpt.com
- 7: https://github.com/actualbudget/actual/issues/3162?utm_source=chatgpt.com
Verify liability account sign normalization
The SimpleFIN spec does not guarantee that loan balances will be returned as negative—implementations vary by institution. Right now you’re calling balance.abs for both credit-card and loan accounts, which forces a positive value whether SimpleFIN returns + or -. That’s fine if your app treats liabilities as positive “debt” amounts, but if you need liabilities to appear negative (e.g. for net-worth calculations), you should instead normalize to -balance.abs.
Please confirm your intended sign convention and adjust accordingly:
- If you want liabilities (CreditCard and Loan) to be negative:
if %w[CreditCard Loan].include?(account_type) balance = -balance.abs end
- If you want liabilities to be shown as positive debt amounts, leaving the code as-is (
balance.abs) is correct.
🤖 Prompt for AI Agents
In app/models/account.rb around lines 84 to 86, the code currently forces
liabilities (CreditCard and Loan) to be positive by calling balance.abs, but the
app’s sign convention for liabilities must be explicit; if you want liabilities
represented as negative amounts for net-worth calculations, change the
normalization to set balance = -balance.abs for those account types, otherwise
leave as balance.abs if you intend liabilities to be shown as positive debt
amounts—update the conditional accordingly (use a clear %w[...] include? check)
and ensure tests reflect the chosen convention.
| if simplefin_account.available_balance.present? && simplefin_account.available_balance > 0 | ||
| attributes[:available_credit] = simplefin_account.available_balance |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential bug with balance logic for credit cards
Based on the PR comments, there's an issue with credit card accounts showing "unknown attribute 'balance' for CreditCard". The condition checks if available_balance > 0 before setting available_credit, but for credit cards, the available balance from SimpleFin might be negative (representing available credit). This logic might need adjustment.
when "CreditCard"
# For credit cards, available_balance often represents available credit
- if simplefin_account.available_balance.present? && simplefin_account.available_balance > 0
- attributes[:available_credit] = simplefin_account.available_balance
+ # SimpleFin may return negative values for available credit
+ if simplefin_account.available_balance.present?
+ attributes[:available_credit] = simplefin_account.available_balance.abs
end🏁 Script executed:
#!/bin/bash
# Check if CreditCard model has a balance attribute or if it's only on Account
echo "Checking CreditCard model attributes..."
rg "class CreditCard" app/models --type ruby -A 20
# Check schema for credit_cards table
rg "create_table.*credit_cards" db/schema.rb -A 15
# Check if there's a balance attribute definition
rg "attribute.*balance|attr_.*balance" app/models/credit_card.rbLength of output: 1838
Update credit card available_credit logic to handle negative balances
For CreditCard accounts, SimpleFin may return a negative available_balance to represent available credit. Remove the > 0 guard and assign the absolute value instead.
• File: app/models/account.rb (around lines 112–115)
when "CreditCard"
# SimpleFin may return negative values for available credit
- if simplefin_account.available_balance.present? && simplefin_account.available_balance > 0
- attributes[:available_credit] = simplefin_account.available_balance
+ if simplefin_account.available_balance.present?
+ attributes[:available_credit] = simplefin_account.available_balance.abs
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.
| if simplefin_account.available_balance.present? && simplefin_account.available_balance > 0 | |
| attributes[:available_credit] = simplefin_account.available_balance | |
| when "CreditCard" | |
| # SimpleFin may return negative values for available credit | |
| if simplefin_account.available_balance.present? | |
| attributes[:available_credit] = simplefin_account.available_balance.abs | |
| end |
🤖 Prompt for AI Agents
In app/models/account.rb around lines 112–113, the current guard only assigns
available_credit when simplefin_account.available_balance > 0; update it so
CreditCard accounts handle negative balances by removing the `> 0` guard and
assigning the absolute value instead: when
simplefin_account.available_balance.present? and the account is a CreditCard,
set attributes[:available_credit] = simplefin_account.available_balance.abs; for
non-credit-card accounts preserve the existing behavior (only assign when
balance > 0).
|
|
||
| included do | ||
| belongs_to :plaid_account, optional: true | ||
| belongs_to :simplefin_account, optional: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Association looks good; add exclusivity guard to prevent double-linking (Plaid + SimpleFin).
Right now an account could potentially have both plaid_account_id and simplefin_account_id set. Enforce mutual exclusivity to avoid ambiguous data sources.
Apply this diff:
included do
belongs_to :plaid_account, optional: true
belongs_to :simplefin_account, optional: true
+ validate :provider_link_exclusivity
end
+
+ private
+ def provider_link_exclusivity
+ if plaid_account_id.present? && simplefin_account_id.present?
+ errors.add(:base, "Account cannot be linked to multiple providers (Plaid and SimpleFin)")
+ end
+ 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.
| belongs_to :simplefin_account, optional: true | |
| # app/models/account/linkable.rb | |
| module Account | |
| module Linkable | |
| extend ActiveSupport::Concern | |
| included do | |
| belongs_to :plaid_account, optional: true | |
| belongs_to :simplefin_account, optional: true | |
| validate :provider_link_exclusivity | |
| end | |
| private | |
| def provider_link_exclusivity | |
| if plaid_account_id.present? && simplefin_account_id.present? | |
| errors.add( | |
| :base, | |
| "Account cannot be linked to multiple providers (Plaid and SimpleFin)" | |
| ) | |
| end | |
| end | |
| end | |
| end |
🤖 Prompt for AI Agents
In app/models/account/linkable.rb around line 6, currently the model allows both
plaid_account_id and simplefin_account_id to be set; add a validation to enforce
mutual exclusivity so an account cannot be linked to both sources
simultaneously. Implement a custom validator method (e.g., validate
:ensure_single_account_source) that adds an error on :base when both
plaid_account_id and simplefin_account_id are present, and ensure the model
returns invalid in that case; update or add specs to cover the validation.
| end | ||
|
|
||
| def get_info(base_url) | ||
| response = HTTParty.get("#{base_url}/info") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit SSL/TLS options for consistency
For consistency with other requests, add SSL/TLS options explicitly.
- response = HTTParty.get("#{base_url}/info")
+ response = HTTParty.get("#{base_url}/info", verify: true, ssl_verify_mode: :peer)📝 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.
| response = HTTParty.get("#{base_url}/info") | |
| response = HTTParty.get("#{base_url}/info", verify: true, ssl_verify_mode: :peer) |
🤖 Prompt for AI Agents
In app/models/provider/simplefin.rb around line 69, the HTTParty.get call does
not supply explicit SSL/TLS options; update the call to include the same SSL
options used elsewhere in the codebase for consistency (for example include
verify: true, ssl_version: 'TLSv1_2' and the project's ssl_ca_file or cert store
setting, or mirror the exact options used in neighboring requests), ensuring you
pass the SSL options as HTTParty request parameters so the request enforces peer
verification and the desired TLS version.
| create_table :simplefin_accounts, id: :uuid do |t| | ||
| t.references :simplefin_item, null: false, foreign_key: true, type: :uuid | ||
| t.string :name | ||
| t.string :account_id |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unique constraint on account_id.
The account_id field appears to be an external identifier from SimpleFin. Without a unique constraint, duplicate accounts could be created, leading to data integrity issues.
Apply this diff to add uniqueness constraint:
- t.string :account_id
+ t.string :account_id, null: falseAnd update the index to be unique:
- t.index :account_id
+ t.index :account_id, unique: true🤖 Prompt for AI Agents
In db/migrate/20250807143819_create_simplefin_accounts.rb around line 6, the
account_id column lacks a uniqueness constraint; modify the migration to enforce
uniqueness by adding a unique index on account_id (for example add_index
:simplefin_accounts, :account_id, unique: true or t.index :account_id, unique:
true) and ensure the column definition and index are applied together in the
migration (and if this migration already ran in environments, create a new
migration to add the unique index instead).
| t.date "expiration_date" | ||
| t.decimal "annual_fee", precision: 10, scale: 2 | ||
| t.jsonb "locked_attributes", default: {} | ||
| t.string "subtype" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Credit card schema doesn't have a balance attribute
According to the PR comments, users are encountering "unknown attribute 'balance' for CreditCard" errors. The schema confirms that the credit_cards table indeed doesn't have a balance column - it only has available_credit, while the balance is stored in the parent accounts table.
This confirms the issue reported in the PR comments. The error occurs because the code is trying to set balance directly on the CreditCard model instead of on the Account model.
Let me verify how the polymorphic relationship is structured:
🏁 Script executed:
#!/bin/bash
# Check how CreditCard relates to Account
rg "class CreditCard" -A 20 --type ruby
# Check if Account uses accepts_nested_attributes_for
rg "accepts_nested_attributes_for.*accountable" --type ruby
# Look for the specific line causing the balance assignment issue
rg "accountable_attributes.*balance" --type rubyLength of output: 3754
Use initial_balance instead of undefined balance on CreditCard creation
The credit_cards table (and the CreditCard model) does not have a balance column, and the parent accounts table expects its opening balance in the initial_balance field via nested attributes. Update any code or tests that pass a balance param to use initial_balance (or add explicit mapping for balance → initial_balance in your controller/concern).
• In test/controllers/credit_cards_controller_test.rb, change:
post credit_cards_path, params: {
account: {
- balance: 1000,
+ initial_balance: 1000,
…
}
}• Review your AccountableResource concern or controller strong params—ensure only initial_balance is permitted (or add logic to translate a balance key into initial_balance).
• Confirm any API clients/docs are updated to reflect the correct initial_balance attribute.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In db/schema.rb around lines 193 to 196 the credit_cards table defines no
`balance` column (accounts expect an opening balance via `initial_balance`), so
tests and controller logic that pass `balance` must be updated: change any test
data in test/controllers/credit_cards_controller_test.rb to use
`initial_balance` in nested account attributes, and in your AccountableResource
concern or the credit cards controller permit `initial_balance` (or add explicit
mapping that translates incoming `balance` => `initial_balance` before
strong-params filtering); finally update API docs/clients to reference
`initial_balance` instead of `balance`.
| create_table "simplefin_accounts", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| | ||
| t.uuid "simplefin_item_id", null: false | ||
| t.string "name" | ||
| t.string "account_id" | ||
| t.string "currency" | ||
| t.decimal "current_balance", precision: 19, scale: 4 | ||
| t.decimal "available_balance", precision: 19, scale: 4 | ||
| t.string "account_type" | ||
| t.string "account_subtype" | ||
| t.jsonb "raw_payload" | ||
| t.jsonb "raw_transactions_payload" | ||
| t.datetime "created_at", null: false | ||
| t.datetime "updated_at", null: false | ||
| t.datetime "balance_date" | ||
| t.jsonb "extra" | ||
| t.jsonb "org_data" | ||
| t.index ["account_id"], name: "index_simplefin_accounts_on_account_id" | ||
| t.index ["simplefin_item_id"], name: "index_simplefin_accounts_on_simplefin_item_id" | ||
| end |
There was a problem hiding this comment.
Missing index on simplefin_accounts.account_id foreign key reference
The simplefin_accounts table has an account_id column with an index, but based on the foreign key relationships in the schema, this appears to be linking to the accounts table. However, there's no foreign key constraint defined for simplefin_accounts.account_id to reference accounts.id.
This missing foreign key constraint could lead to data integrity issues. Add the foreign key constraint:
add_foreign_key "simplefin_accounts", "accounts"🤖 Prompt for AI Agents
In db/schema.rb around lines 685 to 703, the simplefin_accounts.account_id
column lacks a foreign key constraint to accounts.id; add a migration that adds
the foreign key (e.g. add_foreign_key "simplefin_accounts", "accounts", column:
"account_id") so the column references accounts.id (keep or reuse the existing
index), and run migrations to update the schema.rb accordingly.
| gem "redcarpet" | ||
| gem "stripe" | ||
| gem "plaid" | ||
| gem "httparty" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pinning HTTParty to a specific version range.
Adding httparty without version constraints could lead to unexpected behavior if breaking changes are introduced in future releases. For production stability, it's recommended to specify a version constraint.
Apply this diff to pin HTTParty to a stable version range:
-gem "httparty"
+gem "httparty", "~> 0.22.0"📝 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.
| gem "httparty" | |
| -gem "httparty" | |
| +gem "httparty", "~> 0.22.0" |
🤖 Prompt for AI Agents
In Gemfile around line 72, the httparty gem is added without a version
constraint; update that line to pin httparty to a stable version range (for
example "~> 0.18" or another vetted range used by the project), then run bundle
install (or bundle update httparty) to update Gemfile.lock and run the test
suite to ensure compatibility.
| test "scopes work correctly" do | ||
| # Create one for deletion | ||
| item_for_deletion = SimplefinItem.create!( | ||
| family: @family, | ||
| name: "Delete Me", | ||
| access_url: "https://example.com/delete_token", | ||
| scheduled_for_deletion: true | ||
| ) | ||
|
|
||
| active_items = SimplefinItem.active | ||
| ordered_items = SimplefinItem.ordered | ||
|
|
||
| assert_includes active_items, @simplefin_item | ||
| refute_includes active_items, item_for_deletion | ||
|
|
||
| assert_equal [ @simplefin_item, item_for_deletion ].sort_by(&:created_at).reverse, | ||
| ordered_items.to_a | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize ordering test to avoid flakiness on same-second timestamps.
If the DB/time resolution collapses timestamps to the same second, order can be non-deterministic.
Apply this diff to ensure distinct created_at:
test "scopes work correctly" do
# Create one for deletion
item_for_deletion = SimplefinItem.create!(
family: @family,
name: "Delete Me",
access_url: "https://example.com/delete_token",
scheduled_for_deletion: true
)
+ # Ensure created_at differs for deterministic ordering
+ travel 1.second
+ @simplefin_item.touch
+ travel_back
+
active_items = SimplefinItem.active
ordered_items = SimplefinItem.orderedAnd include TimeHelpers if not already:
# Add at top of this test file or in test_helper.rb
include ActiveSupport::Testing::TimeHelpers🤖 Prompt for AI Agents
In test/models/simplefin_item_test.rb around lines 46 to 63, the ordering
assertion is flaky when multiple records share the same-second created_at; make
created_at values deterministic by using ActiveSupport::Testing::TimeHelpers to
advance time between creations (e.g., travel or travel 1.second forward before
creating item_for_deletion) so each record has a distinct timestamp, and add
include ActiveSupport::Testing::TimeHelpers at the top of this test file (or in
test_helper.rb) if not already present.
github-project-automation
bot
moved this from In progress
to Done
in Rebrand to Sure (v0.7.0)
2 participants
As mentioned in Discussion #93
Summary by CodeRabbit
New Features
Style
Tests
Chores