UI Suggestions for Account Types in Setup Modal + Stats-Based Inactive Handling #368
Conversation
…reation` - Implement `Simplefin::AccountTypeMapper` for account type inference with fallback-only logic - Enhance inactive state handling for `SimplefinItem::Importer` - Improve subtype selection handling in views with confidence-based inference
|
Warning Rate limit exceeded@luckyPipewrench has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughPrecomputes UI-only account-type inferences for SimpleFIN setup (new mapper + controller wiring), requires explicit non-"unknown" account_type for Account.create_from_simplefin_account, adds importer inactivity detection tracking consecutive zero-balance runs and marking accounts inactive, updates subtype selection logic in views, expands Investment SUBTYPES, and adds tests for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as SimplefinItemsController
participant Mapper as Simplefin::AccountTypeMapper
participant View as Setup View
rect rgb(230,245,255)
Note over Controller,Mapper: Precompute UI-only inferences
Controller->>Controller: iterate simplefin_accounts
Controller->>Mapper: infer(name, holdings, extra, balance, available_balance, institution)
Mapper-->>Controller: Inference(accountable_type, subtype, confidence)
Controller->>View: render with `@inferred_map`
View->>View: preselect type/subtype when inference.confidence high
end
sequenceDiagram
participant Importer as SimplefinItem::Importer
participant Account as SimplefinAccount
participant Stats as Sync.sync_stats
rect rgb(255,245,235)
Note over Importer,Stats: Inactivity detection during import (non-blocking)
Importer->>Importer: import_account(account_data)
Importer->>Importer: update_inactive_state(simplefin_account, account_data)
alt payload closed/hidden
Importer->>Stats: mark inactive = true, add hint
else zero-ish balance && no holdings
Importer->>Stats: increment zero_runs (capped)
alt zero_runs >= threshold
Importer->>Stats: set inactive = true
end
else activity present
Importer->>Stats: reset zero_runs = 0, inactive = false
end
Importer->>Stats: persist stats (ensure)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/models/simplefin/account_type_mapper.rb (2)
11-11: Unused constant: BROKERAGE_KEYWORD.The
BROKERAGE_KEYWORDconstant is defined but never referenced in the inference logic. Either remove it or document why it's reserved for future use.- BROKERAGE_KEYWORD = /\bbrokerage\b/i.freeze
26-27: Consider more explicit error handling.The inline
rescue nilpattern works but could be more explicit about what exceptions you're rescuing from.- bal = (balance.to_d rescue nil) - avail = (available_balance.to_d rescue nil) + bal = balance.respond_to?(:to_d) ? (balance.to_d rescue nil) : nil + avail = available_balance.respond_to?(:to_d) ? (available_balance.to_d rescue nil) : nilAlternatively, you could use a helper method to make the intent clearer:
def self.safe_to_decimal(value) value.to_d rescue StandardError nil endtest/models/simplefin/account_type_mapper_test.rb (1)
18-23: Minor formatting: extra space before.each.Line 19 has an extra space before
.each- this is a very minor style inconsistency.- [ "Chase Credit Card", "VISA Card", "CREDIT" ] .each do |name| + [ "Chase Credit Card", "VISA Card", "CREDIT" ].each do |name|app/models/simplefin_item/importer.rb (3)
150-153: Optionally dedupe or capstats["hints"]to avoid repeated identical messagesBoth the closed/hidden and zero-runs paths append fixed strings to
stats["hints"]without de-duplication. With multiple accounts and repeated syncs, this array can contain many identical messages.If you care about keeping the hints list tight, consider normalizing it, for example:
- stats["hints"] = Array(stats["hints"]) + [ "One or more accounts show no balance/holdings for multiple syncs — consider relinking or marking inactive." ] + hint = "One or more accounts show no balance/holdings for multiple syncs — consider relinking or marking inactive." + stats["hints"] = (Array(stats["hints"]) + [hint]).uniqSame idea can be applied to the closed/hidden hint.
504-510: Be awareimport_accountnow persists stats in addition to outer callers
import_accountnow callspersist_stats!in its ensure block, but its callers (import_with_chunked_history,import_regular_sync,perform_account_discovery) already callpersist_stats!in their own per-accountensureblocks. This means each account import during a normal sync can trigger multipleupdate_columnswrites ofsync_stats.That’s probably fine for current volumes and gives nicer semantics for direct unit tests calling
import_account, but if you ever see syncs spending noticeable time in DB writes, you could:
- Let only the top-level loops own
persist_stats!and keep this ensure only for direct/test usage (e.g., guarded by a flag), or- Remove the outer per-account ensures and rely on this inner guarantee instead.
Not urgent, but worth keeping in mind from a performance/IO perspective.
Also applies to: 528-532
68-83: Confirm whether balances-only imports should also track inactive/zero_runs
update_inactive_stateis invoked fromimport_accountbut not fromimport_account_minimal_and_balance, which powers the balances-only discovery path. That means:
- Full imports will accumulate
zero_runs/inactivestats and hints.- Balances-only runs will not, even though they have the same
balance/"available-balance"/holdingscontext available inaccount_data.If the intent is for inactivity hints to apply consistently whenever we see balance snapshots (including balances-only runs), consider invoking
update_inactive_statefromimport_account_minimal_and_balanceas well, reusing the same stats/hints flow.If, instead, you explicitly want balances-only discovery to be “stats-light” and avoid these hints, then this asymmetry is fine but should probably be called out in a brief comment.
Also applies to: 112-154
test/models/simplefin_item/importer_inactive_test.rb (1)
3-47: Inactive-state tests look solid; consider adding a missing-balance edge caseThese tests nicely cover:
- Closed-flag-based inactivity,
- Zero-run counting and threshold,
- Reset of
zero_runsandinactivewhen activity returns.Once you adjust the zero-balance detection to avoid treating “no balance keys” as zero, it would be helpful to add a test where
balanceand"available-balance"are both absent and there are no holdings, asserting thatzero_runsdoes not advance and the account is not marked inactive. That will lock in the intended behavior for partial payloads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/controllers/settings/hostings_controller.rb(1 hunks)app/controllers/simplefin_items_controller.rb(1 hunks)app/models/account.rb(1 hunks)app/models/simplefin/account_type_mapper.rb(1 hunks)app/models/simplefin_item/importer.rb(3 hunks)app/views/simplefin_items/_subtype_select.html.erb(1 hunks)app/views/simplefin_items/setup_accounts.html.erb(1 hunks)test/models/account_simplefin_creation_test.rb(1 hunks)test/models/simplefin/account_type_mapper_test.rb(1 hunks)test/models/simplefin_item/importer_inactive_test.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
📚 Learning: 2025-09-13T11:27:55.271Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Applied to files:
test/models/account_simplefin_creation_test.rb
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
test/models/account_simplefin_creation_test.rbapp/models/account.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/models/simplefin/account_type_mapper.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/models/account.rb
🧬 Code graph analysis (5)
app/controllers/simplefin_items_controller.rb (2)
app/models/provider/base.rb (1)
raw_payload(49-51)app/models/simplefin/account_type_mapper.rb (1)
infer(23-57)
app/models/simplefin_item/importer.rb (1)
app/models/provider/base.rb (1)
raw_payload(49-51)
test/models/account_simplefin_creation_test.rb (1)
app/models/account.rb (1)
create_from_simplefin_account(89-134)
test/models/simplefin/account_type_mapper_test.rb (1)
app/models/simplefin/account_type_mapper.rb (1)
infer(23-57)
test/models/simplefin_item/importer_inactive_test.rb (1)
app/models/simplefin_item/importer.rb (1)
stats(108-110)
⏰ 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 (10)
app/controllers/settings/hostings_controller.rb (1)
77-79: LGTM! Blank URI strings now coerce to nil.Using
.presenceensures that empty string submissions are stored asnil, which is cleaner for optional configuration and aligns with the validation logic above.app/views/simplefin_items/setup_accounts.html.erb (1)
81-84: LGTM! Conservative UI preselection.The inference logic only preselects when confidence is
:high, otherwise defaults to an empty selection. This respects user choice while providing helpful suggestions.app/views/simplefin_items/_subtype_select.html.erb (1)
5-9: LGTM! Improved readability.The multi-line conditional flow is clearer than the nested ternary and achieves the same result.
app/models/simplefin/account_type_mapper.rb (2)
23-57: Well-structured inference pipeline.The fallback-only inference logic is conservative and well-documented. The priority order (holdings → loan keywords → credit signals → retirement → default) makes sense for UI suggestions.
59-66: retirement_hint? safely handles nil extra.The use of
with_indifferent_accessonextra || {}safely handles nil values, and the compact.join pattern is a clean way to search multiple potential fields.app/models/account.rb (1)
89-94: Excellent defensive guard.Requiring an explicit
account_typeprevents silent failures and enforces the design principle that type inference is UI-only. The clear error message aids debugging.test/models/account_simplefin_creation_test.rb (2)
9-22: Good coverage of the validation guard.This test verifies that the
ArgumentErroris raised whenaccount_typeis nil, ensuring the defensive guard works as intended.
24-37: Verifies explicit type handling.This test confirms that the provided
account_typeis used directly without inference, and that the balance conversion logic (abs for loans) works correctly.app/controllers/simplefin_items_controller.rb (1)
168-180: Clean integration of inference logic.The controller appropriately prepares inference data for the view without altering the core setup flow. The fallback from
raw_holdings_payloadtoraw_payload["holdings"]handles different data sources gracefully.test/models/simplefin/account_type_mapper_test.rb (1)
4-35: Comprehensive test coverage.The test suite covers all major inference paths: holdings-based, keyword-based (retirement, credit, loan), and the default fallback. This provides good confidence in the mapper's behavior.
…add regression test for missing balances scenario
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/models/simplefin_item/importer.rb (2)
138-138: Consider deduplicating hint messages.The hint strings are appended to the stats array on each account that meets the criteria. If multiple accounts are closed or inactive within the same sync, identical hints will accumulate in the array.
Consider deduplicating hints when persisting stats:
# In persist_stats! or at the end of update_inactive_state stats["hints"] = Array(stats["hints"]).uniqAlso applies to: 153-153
517-533: Redundant persist_stats! call in rescue block.
persist_stats!is called both in the rescue block (line 527) and in the ensure block (line 532). Since the ensure block always runs, the call in rescue is redundant and results in duplicate writes.Apply this diff to remove the redundant call:
rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e # Treat duplicates/validation failures as partial success: count and surface friendly error, then continue stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1 msg = e.message.to_s if msg.downcase.include?("already been taken") || msg.downcase.include?("unique") msg = "Duplicate upstream account detected for SimpleFin (account_id=#{account_id}). Try relinking to an existing manual account." end register_error(message: msg, category: "other", account_id: account_id, name: account_data[:name]) - persist_stats! nil ensure # Ensure stats like zero_runs/inactive are persisted even when no errors occur, # particularly helpful for focused unit tests that call import_account directly. persist_stats! endtest/models/simplefin_item/importer_inactive_test.rb (1)
51-62: Consolidate test class definition for clarity.The test class is reopened at line 52 to add the regression test, which works in Ruby but is unconventional and may confuse readers. The setup block and helper from lines 4-12 are available to this test, but the split definition obscures that relationship.
Apply this diff to consolidate the test into the first class definition:
+ test "does not count zero run when both balances are missing and no holdings" do + account_data = { id: "a4", name: "Unknown", currency: "USD" } # no balance keys, no holdings + + importer.send(:import_account, account_data) + stats = @sync.reload.sync_stats + + assert_equal 0, stats.dig("zero_runs", "a4").to_i + assert_equal false, stats.dig("inactive", "a4") + end end - - -# Additional regression: no balances present should not increment zero_runs or mark inactive -class SimplefinItem::ImporterInactiveTest < ActiveSupport::TestCase - test "does not count zero run when both balances are missing and no holdings" do - account_data = { id: "a4", name: "Unknown", currency: "USD" } # no balance keys, no holdings - - importer.send(:import_account, account_data) - stats = @sync.reload.sync_stats - - assert_equal 0, stats.dig("zero_runs", "a4").to_i - assert_equal false, stats.dig("inactive", "a4") - end -end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/simplefin_item/importer.rb(3 hunks)test/models/simplefin_item/importer_inactive_test.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
app/models/simplefin_item/importer.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/models/simplefin_item/importer.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/models/simplefin_item/importer.rb
🧬 Code graph analysis (2)
test/models/simplefin_item/importer_inactive_test.rb (1)
app/models/simplefin_item/importer.rb (1)
stats(108-110)
app/models/simplefin_item/importer.rb (1)
app/models/provider/base.rb (1)
raw_payload(49-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (3)
app/models/simplefin_item/importer.rb (2)
122-128: Vacuous truth issue has been addressed.The past review comment about
zeroish_balancereturning true when both balances are missing has been fixed. The code now checksamounts.any?before evaluating.all?, which correctly prevents false positives when no balance data is present.
69-107: Verify whether balances-only import should track inactivity.The
import_account_minimal_and_balancemethod doesn't callupdate_inactive_state, whileimport_accountdoes (lines 505-510). The AI summary suggests inactivity evaluation occurs in both paths, but the implementation only shows it in the full import path.Please confirm whether balances-only imports should also track inactive state, or if this is intentionally limited to full imports. If inactivity tracking is desired during balances-only mode, add the same non-blocking evaluation:
# After line 106, before the method ends begin update_inactive_state(sfa, account_data) rescue => e Rails.logger.warn("SimpleFin: inactive-state evaluation failed during balances-only for sfa=#{sfa.id || account_id}: #{e.class} - #{e.message}") ensure persist_stats! endtest/models/simplefin_item/importer_inactive_test.rb (1)
53-61: Excellent regression test coverage for the vacuous truth fix.This test validates that accounts with missing balance data don't incorrectly increment zero_runs or get marked inactive, addressing the concern from the previous review about the vacuous truth issue.
|
Made some good top-level guesses, but missed others like Apple Card: 
EDIT: ah, wait ... the account name is "Juan"! Why not look at institution too? |
|
That dang apple card again 😂 I had it looking at institutions but that started getting complicated since some Providers offer several account types, so I simplified it a bit to this. I can tweak it a bit more if you think so, to catch things keywords like 'card' probably without many chances for false positives. |
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
We could merge as-is ... let me know if you would rather take care of some of the improvements comments here or leave it for a future PR, @luckyPipewrench!
|
You've raises some good points, let me tinker with it a bit to catch a few more before it merges. |
…ment subtype mapping, improved regex handling, and institution-based credit card detection
…xplicit case-based assertions
|
Should be good now @jjmata |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/views/simplefin_items/_subtype_select.html.erb (1)
5-21: Make Investment subtype selection resilient to Inference struct vs hash storageThe new depository heuristics look good (and cover the CK/SV/MM cases mentioned in prior feedback), but the Investment branch assumes
@inferred_mapentries have a:typekey. If instead you’re storing theSimplefin::AccountTypeMapper::Inferencestruct directly,inferred[:type]will always benil(the struct member is:accountable_type), so the Investment subtype preselect will never trigger.You can make this robust to either representation with a small change:
- <% elsif account_type == "Investment" %> - <% inferred = @inferred_map&.dig(simplefin_account.id) || {} %> - <% if inferred[:confidence] == :high && inferred[:type] == "Investment" && inferred[:subtype].present? %> - <% selected_value = inferred[:subtype] %> - <% end %> - <% end %> + <% elsif account_type == "Investment" %> + <% inferred = @inferred_map&.dig(simplefin_account.id) || {} %> + <% inferred_type = inferred[:type] || inferred[:accountable_type] %> + <% if inferred[:confidence] == :high && inferred_type == "Investment" && inferred[:subtype].present? %> + <% selected_value = inferred[:subtype] %> + <% end %> + <% end %>This keeps the view logic simple while handling both a hash with
:typeand the Inference struct with:accountable_type.app/models/simplefin/account_type_mapper.rb (2)
37-48: Align explicit-token matching with nm_norm for 401(k) variants
nm_normremoves non-alphanumerics (gsub(/[^a-z0-9]+/, " ")), so"401(k)"becomes"401 k". However, the explicit 401k patterns still include parentheses:/\broth\s+401\(k\)\b|\broth\s*401k\b/i /\b401\(k\)\b|\b401k\b/iWhen these run against
nm_norm, the\(k\)alternatives will never match, so"Roth 401(k)"/"401(k)"only get the generic retirement mapping fromRETIREMENT_KEYWORDS, not the explicit401ksubtype.If you want the explicit 401k subtype for those forms, you can simplify the patterns to match the normalized shape:
- /\broth\s+401\(k\)\b|\broth\s*401k\b/i => "roth_401k", - /\b401\(k\)\b|\b401k\b/i => "401k", + /\broth\s*401k\b|\broth\s*401\s*k\b/i => "roth_401k", + /\b401k\b|\b401\s*k\b/i => "401k",This keeps the behavior conservative but better reflects the intent in the comment about handling variants like
401(k)while still leveraging the normalized form.
89-96: Consider either using or droppingretirement_hint?
retirement_hint?is a nice encapsulation of name/extra-based retirement detection, but within this file it’s only defined and marked private, not actually used ininfer. If it’s not called from elsewhere, you could either:
- Fold its logic into
inferwhere appropriate, or- Remove it until there’s a concrete caller to avoid maintaining unused code.
If you do have external callers in other parts of the app, then leaving it as-is is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/simplefin_items_controller.rb(1 hunks)app/models/investment.rb(1 hunks)app/models/simplefin/account_type_mapper.rb(1 hunks)app/views/simplefin_items/_subtype_select.html.erb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/simplefin_items_controller.rb
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
⏰ 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)
app/models/investment.rb (1)
4-18: 403b/TSP subtype additions look consistentAdding
"403b"and"tsp"toSUBTYPESis consistent with the existing mapping style (short/long labels) and should play nicely with both manual selection and the inference logic once wired in viaAccountTypeMapper(see separate comment there).
…b` and `tsp` with updated regex definitions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/simplefin/account_type_mapper.rb (1)
44-45: Consider explicit exception handling for balance conversions.The inline
rescue nilonto_dconversions will silently catch all exceptions. While this is acceptable for defensive parsing, consider being more explicit about the expected exception types (e.g.,rescue ArgumentError).- bal = (balance.to_d rescue nil) - avail = (available_balance.to_d rescue nil) + bal = begin + balance.to_d + rescue ArgumentError, TypeError + nil + end + avail = begin + available_balance.to_d + rescue ArgumentError, TypeError + nil + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/simplefin/account_type_mapper.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
🧬 Code graph analysis (1)
app/models/simplefin/account_type_mapper.rb (4)
app/controllers/simplefin_items_controller.rb (1)
new(80-82)app/models/account_order.rb (1)
find(52-54)app/models/setting.rb (1)
key?(90-96)app/controllers/impersonation_sessions_controller.rb (1)
join(10-14)
⏰ 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 (5)
app/models/simplefin/account_type_mapper.rb (5)
1-8: LGTM! Clean module and struct definition.The frozen string literal, module structure, and keyword-initialized Struct follow Ruby best practices.
10-14: LGTM! Comprehensive keyword patterns.The regex patterns appropriately use word boundaries and case-insensitive matching. The inclusion of "apple card" in CREDIT_BRAND_KEYWORDS addresses the feedback mentioned in the PR discussion.
16-28: LGTM! 403b and TSP tokens properly wired.The addition of 403b and tsp patterns at lines 26-27 successfully addresses the previous review feedback, ensuring these subtypes can be explicitly inferred from account names.
67-77: LGTM! Comprehensive credit card detection.The multi-signal approach (name keywords, brand patterns in both name and institution, plus balance heuristics) provides robust credit card inference while maintaining high confidence. The institution check addresses feedback from the PR discussion.
37-88: Strong inference logic with clear priority ordering.The
infermethod's step-by-step fallback approach (explicit tokens → holdings → loan keywords → credit signals → retirement keywords → default) is well-designed and conservative. The defensive checks forInvestment::SUBTYPESexistence and the brokerage exception for retirement accounts demonstrate thoughtful edge-case handling.
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
It now guessed "Apple Card" right into Credit Card and same with BofA Checking! 🚀
|
Curious: when does the "Sync Summary" show/hide? Seems a little random to me currently (without reading code), @luckyPipewrench ? |
It should show right after the sync finishes, but right now it requires a page refresh to show up. I was planning a PR to fix it, do you have other instances you think it should be showing? |
No, that sounds right. I saw it, then it disappeared. That's what was confusing. It's there after page re-fresh. Frankly, I think we should promote that to be |
…e Handling (#368) * - Add tests for `Simplefin::AccountTypeMapper` and `AccountSimplefinCreation` - Implement `Simplefin::AccountTypeMapper` for account type inference with fallback-only logic - Enhance inactive state handling for `SimplefinItem::Importer` - Improve subtype selection handling in views with confidence-based inference * Remove unnecessary `.presence` check for `openai_uri_base` in hostings settings * Refine zero balance detection logic in `SimplefinItem::Importer` and add regression test for missing balances scenario * Enhance account type and subtype inference logic with explicit investment subtype mapping, improved regex handling, and institution-based credit card detection * Refine retirement subtype mapping in `AccountTypeMapper` tests with explicit case-based assertions * Expand `AccountTypeMapper` investment subtype mapping to include `403b` and `tsp` with updated regex definitions * Remove unused `retirement_hint?` method in `AccountTypeMapper` to simplify codebase --------- Co-authored-by: Josh Waldrep <[email protected]>
2 participants
Enhances the SimpleFIN account setup modal with smart, UI-only type suggestions: infers and preselects types like Investment (on holdings), Loan (keywords), or CreditCard (brands/explicit terms) when high-confidence, defaulting to Depository otherwise for a seamless manual flow.
Tightened retirement mapping to avoid conflicts (e.g., skips subtype on "brokerage").
Moved inactive detection (zeros/closed flags) to non-DB sync_stats with capped counters and gentle hints. Backend creation requires explicit types (no auto-fallback).
Includes tests for mapper, creation, inactive logic, and UI defaults. No schema changes; files added: account_type_mapper.rb and related tests.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.