Add Interactive Brokers Provider#1712
Conversation
brin-security-scanner
Bot
added
the
contributor:verified
📝 WalkthroughWalkthroughThis PR implements comprehensive Interactive Brokers (IBKR) integration via Flex Query API, enabling account discovery, trade/activity imports, historical balance syncs, and account linking workflows. It adds IBKR models, a provider adapter, controller actions, account setup UI, exchange rate support on trades, and extensive test coverage, alongside privacy-sensitive CSS updates to existing balance/holdings displays. ChangesIBKR Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3056b2a642
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return | ||
| end | ||
|
|
||
| provider = ibkr_account.ensure_account_provider!(account) |
There was a problem hiding this comment.
When an admin submits a stale/crafted link_existing_account request with an ibkr_account_id that is already linked, ensure_account_provider!(account) updates the existing AccountProvider to the new account instead of failing. That silently detaches the IBKR feed from the original account and starts importing future data into another manual account, leaving the old account with orphaned imported rows; guard here that ibkr_account.account_provider is blank (as complete_account_setup does via current_account.present?) before creating the link.
Useful? React with 👍 / 👎.
| <%= button_to sync_ibkr_item_path(ibkr_item), | ||
| method: :post, | ||
| class: "inline-flex items-center gap-1 px-3 py-1.5 text-sm font-medium text-secondary hover:text-primary border border-secondary rounded-lg hover:border-primary", | ||
| disabled: ibkr_item.syncing? do %> |
There was a problem hiding this comment.
button_to controls out of the edit form
For an already-configured IBKR item, this button_to is emitted inside the surrounding styled_form_with credentials form, which produces nested <form> elements (the delete button below has the same issue). Browsers do not handle nested forms consistently, so the Sync/Delete controls can either be ignored or disrupt the Update Configuration form; render these buttons outside the edit form like the other provider panels do.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/account/provider_import_adapter.rb (1)
556-596:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t clear persisted FX rates when the provider omits the field.
Because
exchange_ratenow defaults toniland is always assigned, a re-import that does not send this keyword will wipe any existing trade-level rate. That can silently change historical conversions on later syncs. Please only writeexchange_ratewhen the caller explicitly provided it, and add a regression test for the “re-import without exchange_rate preserves the stored value” case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/account/provider_import_adapter.rb` around lines 556 - 596, The import_trade method currently always assigns exchange_rate (via entry.entryable.assign_attributes), which overwrites persisted FX rates when callers omit that keyword; change the method to use a sentinel default for exchange_rate (e.g. UNSET_EXCHANGE_RATE = Object.new) in the import_trade signature and only include exchange_rate in the attributes hash when exchange_rate != UNSET_EXCHANGE_RATE, leaving existing DB values intact; update the assign_attributes usage in import_trade and add a regression test that re-imports a Trade without passing exchange_rate to verify the stored exchange_rate is preserved.
🟡 Minor comments (9)
test/models/transaction_test.rb-143-154 (1)
143-154:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a test for nested
extra["security"]["id"]fallback.The new coverage only exercises top-level
security_idand empty metadata. The fallback branch in Line [148] (extra["security"]["id"]) is still untested; add one case to lock that behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/transaction_test.rb` around lines 143 - 154, Add a unit test in test/models/transaction_test.rb to exercise the fallback branch of Transaction#activity_security by creating a Transaction with extra: { "security" => { "id" => security.id } } (use the same securities fixture like securities(:aapl)) and assert that transaction.activity_security returns that security; this mirrors the existing top-level security_id test but targets the nested extra["security"]["id"] path to cover the untested branch.db/migrate/20260508110000_create_ibkr_items_and_accounts.rb-9-11 (1)
9-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
t.textfor credential columns to match other provider migrations.The IBKR migration uses
t.stringfortokenandquery_id, but other provider credentials in this codebase uset.text: mercury (token), lunchflow (api_key), coinbase (api_key,api_secret), indexa_capital (api_token), and binance (api_key,api_secret). Both column types work with ActiveRecord'sencrypts(which is properly configured for these fields inIbkrItem), but standardize ont.textfor consistency.- t.string :query_id - t.string :token + t.text :query_id + t.text :token🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/migrate/20260508110000_create_ibkr_items_and_accounts.rb` around lines 9 - 11, The migration defines credential columns using t.string for token and query_id; change both to t.text to match other provider migrations and the project's convention (these columns are used with ActiveRecord encryption in IbkrItem). Update the migration's column definitions for token and query_id from t.string to t.text, run/prepare a new migration or rollback+recreate this migration if already applied, and ensure the schema.rb is updated so the database column types and encrypted fields remain consistent with other providers.app/views/settings/providers/show.html.erb-82-86 (1)
82-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the new provider title.
Line 82 introduces a user-facing hard-coded string; this should use
t()and a locale key.Suggested fix
- <%= settings_section title: "Interactive Brokers Flex Query (beta)", collapsible: true, open: false do %> + <%= settings_section title: t("settings.providers.ibkr.title"), collapsible: true, open: false do %>Also add
settings.providers.ibkr.titleinconfig/locales/en.yml.As per coding guidelines:
**/*.{erb,rb}“Always uset()helper for user-facing strings… Updateconfig/locales/en.ymlfor new strings.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/show.html.erb` around lines 82 - 86, Replace the hard-coded title in the settings_section call (the title: "Interactive Brokers Flex Query (beta)" in show.html.erb) with a translation helper call (e.g. t('settings.providers.ibkr.title')), and add the corresponding key settings.providers.ibkr.title to config/locales/en.yml with the English text "Interactive Brokers Flex Query (beta)"; ensure the view uses t(...) and the locale file contains the new key.app/views/settings/providers/_ibkr_panel.html.erb-11-53 (1)
11-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix invalid list nesting in setup instructions
Several
<ul>blocks are direct children of<ol>(e.g., Line 15, Line 45), which is invalid semantic HTML and hurts accessibility. Nest each sub-list inside its parent<li>.As per coding guidelines, "Always generate semantic HTML."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_ibkr_panel.html.erb` around lines 11 - 53, The ordered list has direct child <ul> elements which is invalid; for each sub-list (e.g., the <ul> blocks under the "Cash Report:", "Cash Transactions:", "Net Asset Value (NAV) in Base:", "Open Positions:", "Trades:" and the configuration options) move the <ul> so it is nested inside the corresponding parent <li> (wrap each sub-<ul> inside the <li> that contains the header text like "Cash Report:", "Cash Transactions:", "Open Positions:", etc.), ensure all <li> elements enclose their child <ul> and that tags are properly opened/closed to produce semantic HTML.app/models/family/ibkr_connectable.rb-14-14 (1)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle blank
item_namewhen applying default provider nameLine 14 uses
item_name || ..., so an empty string won’t fall back to"Interactive Brokers". Useitem_name.presence || ...instead.Proposed fix
- name: item_name || "Interactive Brokers", + name: item_name.presence || "Interactive Brokers",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/family/ibkr_connectable.rb` at line 14, The name assignment in ibkr_connectable.rb currently uses `item_name || "Interactive Brokers"`, which treats an empty string as truthy; update the fallback to use presence so blank strings fall back: change the expression to use `item_name.presence || "Interactive Brokers"` where the `name` is set (refer to the `name: item_name || "Interactive Brokers"` assignment) so empty or whitespace-only item_name values will correctly default to "Interactive Brokers".test/models/account/syncer_test.rb-25-29 (1)
25-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert execution order explicitly to match the test intent
Line 25-Line 27 only checks call counts. The test title asserts ordering (“after materialization”), but this can still pass if order regresses.
Proposed fix
- Account::MarketDataImporter.any_instance.expects(:import_all).once - Balance::Materializer.any_instance.expects(:materialize_balances).once - IbkrAccount::HistoricalBalancesSync.any_instance.expects(:sync!).once + sequence = sequence("ibkr_sync_order") + Account::MarketDataImporter.any_instance.expects(:import_all).once.in_sequence(sequence) + Balance::Materializer.any_instance.expects(:materialize_balances).once.in_sequence(sequence) + IbkrAccount::HistoricalBalancesSync.any_instance.expects(:sync!).once.in_sequence(sequence)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/account/syncer_test.rb` around lines 25 - 29, The test currently only asserts call counts for Account::MarketDataImporter.import_all, Balance::Materializer.materialize_balances, and IbkrAccount::HistoricalBalancesSync.sync! but not their order; modify the test around Account::Syncer.new(account).perform_sync to enforce ordering by creating a Mocha sequence (or equivalent) and attach each expectation (.expects(...)) to that sequence in the desired order: import_all first, materialize_balances second, and sync! third so the calls are asserted to occur in that exact sequence when Account::Syncer#perform_sync is invoked.app/controllers/ibkr_items_controller.rb-53-86 (1)
53-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
updatealways forcesstatus: :goodregardless of validation outcome.Line 58 merges
status: :goodinto the update payload even when the user only editedname. This unconditionally clears anyrequires_updatestate on save (e.g., when credentials remain unchanged but were previously broken), masking a real failure mode until the next sync runs. Consider only resettingstatuswhenquery_idortokenactually change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/ibkr_items_controller.rb` around lines 53 - 86, The update action currently forces status: :good by merging status into attrs unconditionally; change it so status is only reset when the credentials actually changed: compute attrs = ibkr_item_params.to_h, set attrs["query_id"] = `@ibkr_item.query_id` if blank and attrs["token"] = `@ibkr_item.token` if blank as before, then detect if attrs["query_id"] != `@ibkr_item.query_id` || attrs["token"] != `@ibkr_item.token` and only then merge status: :good into the payload passed to `@ibkr_item.update`; keep the existing sync_later call around `@ibkr_item.sync_later` unless `@ibkr_item.syncing`? but only trigger it when you have reset status (i.e., when you merged status: :good).app/controllers/ibkr_items_controller.rb-167-178 (1)
167-178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading success message when processing fails after link.
After
IbkrAccount::Processor.new(...).processis rescued silently (only logged), control still falls through toredirect_to account_path(account), notice: "Successfully linked...". The user is told linking succeeded even when the subsequent processing raised. Either let the rescue alter the redirect (e.g., a different alert/notice indicating the linkage succeeded but the initial sync failed and will be retried), or schedule processing asynchronously so failure doesn't influence this synchronous user-facing flow.Additionally, the outer
rescue => eat Line 175 insertse.messagedirectly into the flash alert returned to the browser. Internal exception messages can leak implementation/PII details — prefer logging the full error and showing a generic alert to the user.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/ibkr_items_controller.rb` around lines 167 - 178, The current flow rescues errors from IbkrAccount::Processor.new(ibkr_account.reload).process but always continues to redirect with the success notice, and the outer rescue exposes e.message to users; fix by making processing failures change the user-facing response: in the inner rescue (around IbkrAccount::Processor.process) set a local status flag (e.g., processing_failed = true) or enqueue the work asynchronously instead of letting the failure fall through, then branch before redirect_to account_path(account) to show a clear notice like "Linked, initial sync failed — will retry" (or schedule async processing via ibkr_account.ibkr_item.sync_later) when processing_failed is true; also change the outer rescue to log the full error (Rails.logger.error including e.class and backtrace) but present a generic flash alert (e.g., "Failed to link Interactive Brokers account") rather than injecting e.message into the alert.app/models/ibkr_account/activities_processor.rb-161-162 (1)
161-162:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove commission field requirements from
supported_trade?.Trades without commission data are being rejected entirely, but IBKR Flex Query reports have commission as an optional field—it's not guaranteed for every trade (e.g., commission-free promotions, certain ETF trades). The trade itself can be processed without these fields, and
import_commission_transactionalready handles missing commission gracefully by returning early (line 117). These checks prevent valid trades from being imported.♻️ Suggested change
- row[:trade_price].present? && - row[:transaction_id].present? && - row[:ib_commission].present? && - row[:ib_commission_currency].present? && - fx_rate_available?(row) + row[:trade_price].present? && + row[:transaction_id].present? && + fx_rate_available?(row)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/ibkr_account/activities_processor.rb` around lines 161 - 162, supported_trade? is currently rejecting trades when row[:ib_commission] or row[:ib_commission_currency] are missing; remove the commission presence checks so trades without commission are allowed (delete the lines checking row[:ib_commission].present? && row[:ib_commission_currency].present?). Keep import_commission_transaction as-is (it already returns early when commission is absent), so only modify supported_trade? to stop requiring commission fields.
🧹 Nitpick comments (11)
app/models/ibkr_item/unlinking.rb (1)
9-10: 💤 Low valueUse the polymorphic association instead of a hardcoded
provider_typestring.
AccountProvider.where(provider: provider_account)sets bothprovider_type/provider_idfrom the polymorphic mapping, avoiding the hardcoded"IbkrAccount"literal that would silently break under any rename/namespacing.♻️ Proposed change
- links = AccountProvider.where(provider_type: "IbkrAccount", provider_id: provider_account.id).to_a + links = AccountProvider.where(provider: provider_account).to_a🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/ibkr_item/unlinking.rb` around lines 9 - 10, Replace the hardcoded polymorphic type lookup with the polymorphic association: in the block where ibkr_accounts.find_each yields provider_account and you currently call AccountProvider.where(provider_type: "IbkrAccount", provider_id: provider_account.id), change the query to use AccountProvider.where(provider: provider_account) so Rails sets provider_type and provider_id via the polymorphic mapping (update any variable names like provider_account and the surrounding ibkr_accounts.find_each loop accordingly).app/views/ibkr_items/select_existing_account.html.erb (1)
3-33: ⚡ Quick winLocalize the IBKR linking copy with
t()keysUser-facing strings in this modal are hard-coded (e.g., header, empty-state guidance, balance label, action labels). Please move them to locale keys and render with
t(...).As per coding guidelines, "Always use
t()helper for user-facing strings" and "Updateconfig/locales/en.ymlfor new strings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/ibkr_items/select_existing_account.html.erb` around lines 3 - 33, Replace all hard-coded user-facing strings in this view with i18n keys and calls to t(...): change the dialog header (dialog.with_header), empty-state paragraphs and list items, the balance label inside the span that uses number_to_currency, and the DS::Button/DS::Link text props to use t("...") keys; keep the dynamic interpolations (like ibkr_account.name, currency and number_to_currency) intact. Add corresponding entries to config/locales/en.yml under a sensible namespace (e.g. ibkr_items.select_existing_account.header, .empty_state.line1, .empty_state.line2, .balance_label, .actions.link, .actions.cancel). Ensure you reference the same keys in the view via t("ibkr_items.select_existing_account.<key>") to locate and replace each hard-coded string.app/views/ibkr_items/_ibkr_item.html.erb (2)
11-12: ⚡ Quick winReplace raw IB badge colors with design-system tokens
Line 11-Line 12 uses hard-coded hex color classes. Please use design-system token classes so the badge follows theme/system constraints.
As per coding guidelines, "Use Tailwind CSS design tokens ... instead of raw Tailwind classes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/ibkr_items/_ibkr_item.html.erb` around lines 11 - 12, The IB badge uses hard-coded hex Tailwind classes; replace the raw color classes on the badge container and the span (the div with class "bg-[`#D32F2F`]/10" and the span with "text-[`#D32F2F`]") with the project's Tailwind design-system token classes (e.g., use the design token for the danger/background token on the container and the corresponding text color token on the span) so the badge follows theme tokens instead of raw hex values.
19-107: ⚡ Quick winLocalize status and action copy via
t()This partial introduces many hard-coded user-facing strings (status text, empty-state text, action labels). Move them to locale keys and render via
t(...).As per coding guidelines, "Always use
t()helper for user-facing strings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/ibkr_items/_ibkr_item.html.erb` around lines 19 - 107, The partial contains hard-coded user-facing strings (e.g., "Deletion in progress", "Flex Web Service", "Syncing", "Credentials need attention", "Error", "Synced ... ago.", "Never synced.", "Set up accounts", "Delete", "Accounts need setup", "No IBKR accounts discovered yet.", and link/button labels used with DS::Link, DS::Tooltip, menu.with_item, icon/tag.span, and ProviderSyncSummary rendering); replace each literal with a call to t(...) using descriptive locale keys (e.g., ibkr_items.status.deletion_in_progress, ibkr_items.service.flex_web, ibkr_items.status.syncing, ibkr_items.actions.setup_accounts, ibkr_items.empty.no_accounts, etc.), update calls in the partial (tag.span, render DS::Tooltip, DS::Link, menu.with_item) to use the t(...) results, and add corresponding keys to the locale YAML so all user-facing strings are localized.app/views/settings/providers/_ibkr_panel.html.erb (2)
10-118: ⚡ Quick winMove panel copy to i18n keys
This partial introduces substantial user-visible text as hard-coded literals. Please move these strings to locale keys and use
t(...)consistently.As per coding guidelines, "Always use
t()helper for user-facing strings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_ibkr_panel.html.erb` around lines 10 - 118, The partial contains many hard-coded user-facing strings; extract them into i18n keys (e.g., under a new locale namespace like ibkr_panel.*) and replace literals with t('ibkr_panel.<key>') usages throughout the partial: move the "Setup instructions:" heading, every list item and note paragraph, the placeholders for form.text_field :query_id and :token, button labels used with button_to (Sync, Disconnect Interactive Brokers?), the form.submit text, the status messages ("Not configured.", the sync summary label if any) and link text into locale keys; keep view logic and helpers (styled_form_with, form.text_field, button_to sync_ibkr_item_path, form.submit, ibkr_item.sync_status_summary) intact and reference the newly created keys in those calls so all user-visible strings use the t(...) helper.
116-116: ⚡ Quick winUse a design token class instead of raw
bg-gray-400Line 116 uses a raw Tailwind color utility. Prefer a design-system token class to keep theme consistency.
As per coding guidelines, "Always prefer using functional tokens defined in sure-design-system.css rather than raw Tailwind utilities."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_ibkr_panel.html.erb` at line 116, Replace the raw Tailwind color utility on the small status dot (the div with classes "w-2 h-2 bg-gray-400 rounded-full") with the corresponding design-system token class from sure-design-system.css; locate that div in _ibkr_panel.html.erb and swap out "bg-gray-400" for the project's functional token class that maps to the intended neutral color (the named token used across sure-design-system.css) so the component uses the design token instead of a raw Tailwind utility.app/models/ibkr_item/report_parser.rb (1)
78-100: 💤 Low valueOptional: simplify
section_rows— consider extracting the fallback path.The dual-mode logic (children-of-container vs container-as-row vs statement-direct fallback) is correct but dense. A short comment describing the three input shapes IBKR can produce here would help future readers without changing behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/ibkr_item/report_parser.rb` around lines 78 - 100, The section_rows method handles three IBKR XML shapes (rows as children of container elements, container elements themselves as rows, or rows directly under statement) but the logic is dense; extract the fallback "statement-direct" path into a small helper (e.g., a private method like statement_direct_rows(statement, row_names)) and add a short comment above section_rows describing the three input shapes and the chosen resolution order; update section_rows to call the new helper when rows.empty? to keep behavior identical while making the fallback path explicit and easier to read.app/models/ibkr_item.rb (1)
35-43: 💤 Low valuePrefer a custom error class over raw
StandardError.Raising
StandardError, "IBKR provider is not configured"makes catch sites unable to discriminate this configuration failure from any unrelated runtime error. Consider an explicit error class (e.g.,IbkrItem::ProviderNotConfiguredError) so callers and the surroundingrescue => e(which re-raises) can be acted upon by tests and upstream handlers without string matching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/ibkr_item.rb` around lines 35 - 43, Replace the generic StandardError with a dedicated error class so callers can distinguish provider-configuration failures: define a new error class (e.g., IbkrItem::ProviderNotConfiguredError) and raise that from import_latest_ibkr_data instead of StandardError, update the rescue/logging behavior if necessary to reference the new class (method import_latest_ibkr_data and the IbkrItem::Importer call are the relevant symbols), and ensure any tests or upstream handlers catch IbkrItem::ProviderNotConfiguredError rather than matching the error message.app/controllers/ibkr_items_controller.rb (1)
134-142: 💤 Low valuePossible N+1 / large in-memory filter on
select_existing_account.
Current.family.ibkr_items.includes(...).flat_map(&:ibkr_accounts).select { |a| a.account_provider.nil? }materializes every ibkr_account and account_provider for the family before filtering in Ruby. Consider scoping at the DB level, e.g.,Current.family.ibkr_items.joins(:ibkr_accounts).left_joins(ibkr_accounts: :account_provider).where(account_providers: { id: nil })..., returning only the needed ibkr_account records ordered byupdated_at/created_atdesc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/ibkr_items_controller.rb` around lines 134 - 142, The current code materializes all ibkr_accounts in Ruby via Current.family.ibkr_items.flat_map(&:ibkr_accounts).select and filters account_provider there; instead perform the filtering and ordering at the DB level: start from Current.family.ibkr_items, join to :ibkr_accounts and left_join ibkr_accounts: :account_provider, add where(account_providers: { id: nil }) to only return ibkr_accounts without providers, select the ibkr_accounts records (not the parent ibkr_items), and order by COALESCE(ibkr_accounts.updated_at, ibkr_accounts.created_at) DESC so `@available_ibkr_accounts` is a DB-scoped collection for the select_existing_account render.app/models/ibkr_account/activities_processor.rb (2)
195-213: 💤 Low value
security_symbol_for_coniddoes redundantwith_indifferent_accesswork per call.For each cash transaction the helper iterates
raw_holdings_payloadand (potentially)raw_activities_payload[:trades], callingwith_indifferent_accessper row. When there are many cash transactions this is O(N×M). Consider memoizing aconid → symbolmap per processor instance.♻️ Suggested change
+ def conid_symbol_map + `@conid_symbol_map` ||= begin + map = {} + Array(`@ibkr_account.raw_holdings_payload`).each do |holding| + h = holding.with_indifferent_access + map[h[:conid].to_s] ||= h[:symbol] if h[:conid].present? + end + Array(`@ibkr_account.raw_activities_payload`&.dig("trades") || `@ibkr_account.raw_activities_payload`&.dig(:trades)).each do |trade| + t = trade.with_indifferent_access + map[t[:conid].to_s] ||= t[:symbol] if t[:conid].present? + end + map + end + end + def security_symbol_for_conid(conid) return nil if conid.blank? - - holding_symbol = Array(`@ibkr_account.raw_holdings_payload`).find do |holding| - holding.with_indifferent_access[:conid].to_s == conid.to_s - end&.with_indifferent_access&.dig(:symbol) - return holding_symbol if holding_symbol.present? - - Array(`@ibkr_account.raw_activities_payload`&.dig("trades") || `@ibkr_account.raw_activities_payload`&.dig(:trades)).find do |trade| - trade.with_indifferent_access[:conid].to_s == conid.to_s - end&.with_indifferent_access&.dig(:symbol) + conid_symbol_map[conid.to_s] end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/ibkr_account/activities_processor.rb` around lines 195 - 213, security_symbol_for_conid is doing repeated with_indifferent_access conversions over `@ibkr_account.raw_holdings_payload` and raw_activities_payload[:trades], causing O(N×M) work; instead build and memoize a conid→symbol lookup on the processor instance (e.g. `@conid_symbol_map`) the first time it’s needed. Populate that map once by iterating Array(`@ibkr_account.raw_holdings_payload`) and Array(`@ibkr_account.raw_activities_payload`&.dig("trades") || `@ibkr_account.raw_activities_payload`&.dig(:trades)), calling with_indifferent_access only while creating the map, then change security_symbol_for_conid to return `@conid_symbol_map`[conid.to_s] (or nil) so resolve_security_for_cash_transaction and resolve_security remain unchanged but much faster.
150-164: 💤 Low valueFilter out zero-quantity trades.
supported_trade?only checkspresent?forquantity, so a row withquantity: "0"passes validation and proceeds to import a no-op trade withsigned_quantity = 0andnative_amount = 0. Worth rejecting these explicitly to avoid creating empty trade entries.♻️ Suggested change
def supported_trade?(row) row[:asset_category].to_s == "STK" && row[:buy_sell].present? && row[:conid].present? && row[:currency].present? && row[:quantity].present? && + parse_decimal(row[:quantity])&.nonzero? && row[:symbol].present? &&🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/ibkr_account/activities_processor.rb` around lines 150 - 164, The supported_trade? predicate currently allows rows with quantity "0" because it only checks presence; update supported_trade? to explicitly reject zero quantities by converting row[:quantity] to a numeric (e.g., using to_d or to_f) and returning false if the numeric value is zero or effectively zero, ensuring any zero or zero-equivalent quantity rows are filtered out before further processing (leave other checks like fx_rate_available?(row) intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/ibkr_items_controller.rb`:
- Around line 27-83: Replace all hard-coded user-facing strings in this
controller with i18n calls using the t() helper: update flash notices/alerts in
the create and update flows (e.g., the strings "Successfully configured
Interactive Brokers.", "Successfully updated Interactive Brokers
configuration.", and any redirect alert messages), and the `@error_message`
construction that joins `@ibkr_item.errors.full_messages` so you pass localized
wrapper keys; use hierarchical keys such as ibkr_items.flash.created,
ibkr_items.flash.updated, and ibkr_items.errors.not_configured (or similar)
referenced from the create and update methods and from any places using
attrs["query_id"]/attrs["token"] fallbacks, and add corresponding entries to
config/locales/en.yml. Ensure you call t(...) where the current literal strings
are used (and keep the original messages as the default locale values in
en.yml).
In `@app/models/account/syncer.rb`:
- Around line 39-46: The IBKR-specific call in apply_provider_balance_overrides
(the IbkrAccount::HistoricalBalancesSync.new(ibkr_account).sync! invocation) can
raise and currently bubbles up to abort perform_sync; wrap that sync! call in a
rescue that captures StandardError (or Exception subclass you prefer) so
provider failures are swallowed and do not stop the overall account sync, and
log the exception (including message and backtrace) using the existing logger or
process logger; keep the early guards (account.linked_to? and
find_by(provider_type: "IbkrAccount")) but ensure any exception from
HistoricalBalancesSync.sync! is rescued and handled gracefully.
In `@app/models/ibkr_account/activities_processor.rb`:
- Line 65: The extra["exchange_rate"] is stored as BigDecimal for trades
(exchange_rate: parse_decimal(row[:fx_rate_to_base]) in ActivitiesProcessor) but
as Float for cash and commission paths; make exchange_rate consistently a
BigDecimal across all activity types by replacing uses of &.to_f with the same
parse_decimal(...) conversion (or a single helper that returns BigDecimal) in
the cash transaction and commission branches referenced in this file so all
extras store BigDecimal for exchange_rate.
In `@app/models/ibkr_item.rb`:
- Around line 107-123: Update sync_status_summary and institution_display_name
to use Rails I18n helpers instead of hard-coded strings: replace the literal "No
IBKR accounts discovered yet" with a t(...) lookup, use t(..., count:
linked_count) or Rails pluralization via t with count for the linked account
text (e.g., key that supports pluralization) and similarly for the
"#{linked_count} linked, #{unlinked_count} need setup" message use a translation
key that accepts both counts (or compose using two t(...) calls with count:
linked_count and count: unlinked_count), and replace institution_display_name's
"Interactive Brokers" with a t(...) lookup; reference the existing methods
total_accounts_count, linked_accounts_count, and unlinked_accounts_count to pass
counts into the translation calls and add appropriate keys to the locale files.
In `@app/models/ibkr_item/importer.rb`:
- Around line 13-24: The snapshot write (ibkr_item.upsert_ibkr_snapshot!) can
succeed while subsequent account upserts
(ibkr_item.ibkr_accounts.find_or_initialize_by /
ibkr_account.upsert_from_ibkr_statement!) fail, leaving partial state; wrap the
snapshot, the per-account upserts, and the final status update
(ibkr_item.update!(status: :good)) inside a single DB transaction (e.g.,
ActiveRecord::Base.transaction or ibkr_item.transaction) so any exception rolls
back all writes and preserves atomicity.
In `@app/models/provider/ibkr_adapter.rb`:
- Around line 14-26: The provider definition in ibkr_adapter.rb currently uses
hard-coded user-facing strings for the "name" and "description" keys (and also
the institution_name elsewhere in this file), so replace those string literals
with i18n lookups (e.g., use t("providers.ibkr.name"),
t("providers.ibkr.connection_description") and
t("providers.ibkr.institution_name") respectively) and add corresponding entries
under config/locales/en.yml (providers: ibkr: name:, connection_description:,
institution_name:) so the UI strings are localized; ensure you call Rails' t()
helper in the same context where the provider hash is built (preserving the
existing lambdas/new_account_path/existing_account_path entries).
In `@app/models/provider/ibkr_flex.rb`:
- Around line 1-17: Change Provider::IbkrFlex to inherit from Provider (class
Provider::IbkrFlex < Provider) and stop exposing a separate provider-specific
error stack for public consumption; instead wrap the class’s public data
retrieval entrypoint (the fetch / public fetch path method) with
with_provider_response so errors are normalized by the shared Provider behavior.
Update the public fetch method to call with_provider_response { ...actual IBKR
HTTP/data logic... } and remove or keep the internal
ApiError/AuthenticationError only for internal mapping (do not let callers
bypass with_provider_response).
In `@app/models/snaptrade_account/processor.rb`:
- Around line 74-76: The info log in SnaptradeAccount::Processor exposes a
sensitive monetary value; remove the balance amount from the info-level message
(or demote the log to debug) and log only non-sensitive context/identifiers
(e.g., snaptrade_account.id or account external_id) instead; update the log call
in the block guarded by use_api_total_balance? that currently references
snaptrade_account.current_balance so it no longer prints the amount but
preserves the return of snaptrade_account.current_balance || 0.
In `@app/models/trade.rb`:
- Around line 21-29: The setter exchange_rate= currently accepts
"NaN"/"Infinity" because Float(value) doesn't raise; after converting
(normalized_value = Float(value)) add a check using normalized_value.finite? (or
normalized_value.nan?/infinite?) and if not finite treat it as invalid (set
"exchange_rate_invalid" => true and store the original value or nil as
appropriate) instead of marking valid; likewise update the validator that
performs the numeric <= 0 check (the method that validates exchange_rate) to
first reject non-finite values by checking value.finite? (or
value.nan?/value.infinite?) and mark them invalid before doing the <= 0
comparison so NaN/Infinity can't pass validation.
In `@app/models/transaction.rb`:
- Around line 151-156: The activity_security method currently does a
per-instance lookup via Security.find_by(id: activity_security_id), causing N+1
queries; change callers to preload all required Security records (e.g., fetch
Security.where(id: transactions.map(&:activity_security_id)).index_by(&:id)) and
replace the per-row lookup with a map lookup (use the
preloaded_map[activity_security_id]) or convert activity_security to read from
an association and use ActiveRecord preload/includes in the query that builds
the transactions list; remove or short-circuit the Security.find_by usage in the
activity_security method (or make it use a provided preloaded map) so rendering
uses the bulk-loaded securities instead of issuing individual queries.
- Around line 147-149: The accessor activity_security_id currently reads
top-level keys from Transaction#extra which breaks provider namespacing; update
activity_security_id to be provider-aware by reading under the provider
namespace in extra (e.g., extra&.dig(provider_key, "security_id") ||
extra&.dig(provider_key, "security", "id")), where provider_key is derived from
the transaction (use an existing provider attribute/method such as provider or
provider_name or add an optional provider argument to the method), and
remove/avoid reading top-level "security_id" or "security" keys.
In `@app/views/ibkr_items/setup_accounts.html.erb`:
- Around line 1-173: Replace all hard-coded user-facing strings in this view
with i18n calls using the t() helper and add corresponding keys under a
hierarchical namespace like ibkr_items.setup_accounts in config/locales/en.yml;
specifically update content_for :title, the DS::Dialog header title, the info
box headings and bullets, status messages
(`@waiting_for_sync/`@no_accounts_found), form labels and checkbox label text
(e.g., account_<%= ibkr_account.id %> label), button and link texts ("Create
selected accounts", "Cancel", "Refresh", "Back to Settings", "Link", "Done"),
section headers ("Available accounts", "Already linked"), the select prompt
("Select an account...") and any interpolated strings that include
currency/amounts so they use t() with interpolation where needed; ensure keys
follow the suggested hierarchy (e.g., ibkr_items.setup_accounts.title,
.info_box.title, .info_box.bullets.item1, .buttons.create_selected,
.status.fetching_accounts, .select.prompt, .linked.already_linked) and replace
the inline literals with t('ibkr_items.setup_accounts.xxx').
---
Outside diff comments:
In `@app/models/account/provider_import_adapter.rb`:
- Around line 556-596: The import_trade method currently always assigns
exchange_rate (via entry.entryable.assign_attributes), which overwrites
persisted FX rates when callers omit that keyword; change the method to use a
sentinel default for exchange_rate (e.g. UNSET_EXCHANGE_RATE = Object.new) in
the import_trade signature and only include exchange_rate in the attributes hash
when exchange_rate != UNSET_EXCHANGE_RATE, leaving existing DB values intact;
update the assign_attributes usage in import_trade and add a regression test
that re-imports a Trade without passing exchange_rate to verify the stored
exchange_rate is preserved.
---
Minor comments:
In `@app/controllers/ibkr_items_controller.rb`:
- Around line 53-86: The update action currently forces status: :good by merging
status into attrs unconditionally; change it so status is only reset when the
credentials actually changed: compute attrs = ibkr_item_params.to_h, set
attrs["query_id"] = `@ibkr_item.query_id` if blank and attrs["token"] =
`@ibkr_item.token` if blank as before, then detect if attrs["query_id"] !=
`@ibkr_item.query_id` || attrs["token"] != `@ibkr_item.token` and only then merge
status: :good into the payload passed to `@ibkr_item.update`; keep the existing
sync_later call around `@ibkr_item.sync_later` unless `@ibkr_item.syncing`? but only
trigger it when you have reset status (i.e., when you merged status: :good).
- Around line 167-178: The current flow rescues errors from
IbkrAccount::Processor.new(ibkr_account.reload).process but always continues to
redirect with the success notice, and the outer rescue exposes e.message to
users; fix by making processing failures change the user-facing response: in the
inner rescue (around IbkrAccount::Processor.process) set a local status flag
(e.g., processing_failed = true) or enqueue the work asynchronously instead of
letting the failure fall through, then branch before redirect_to
account_path(account) to show a clear notice like "Linked, initial sync failed —
will retry" (or schedule async processing via ibkr_account.ibkr_item.sync_later)
when processing_failed is true; also change the outer rescue to log the full
error (Rails.logger.error including e.class and backtrace) but present a generic
flash alert (e.g., "Failed to link Interactive Brokers account") rather than
injecting e.message into the alert.
In `@app/models/family/ibkr_connectable.rb`:
- Line 14: The name assignment in ibkr_connectable.rb currently uses `item_name
|| "Interactive Brokers"`, which treats an empty string as truthy; update the
fallback to use presence so blank strings fall back: change the expression to
use `item_name.presence || "Interactive Brokers"` where the `name` is set (refer
to the `name: item_name || "Interactive Brokers"` assignment) so empty or
whitespace-only item_name values will correctly default to "Interactive
Brokers".
In `@app/models/ibkr_account/activities_processor.rb`:
- Around line 161-162: supported_trade? is currently rejecting trades when
row[:ib_commission] or row[:ib_commission_currency] are missing; remove the
commission presence checks so trades without commission are allowed (delete the
lines checking row[:ib_commission].present? &&
row[:ib_commission_currency].present?). Keep import_commission_transaction as-is
(it already returns early when commission is absent), so only modify
supported_trade? to stop requiring commission fields.
In `@app/views/settings/providers/_ibkr_panel.html.erb`:
- Around line 11-53: The ordered list has direct child <ul> elements which is
invalid; for each sub-list (e.g., the <ul> blocks under the "Cash Report:",
"Cash Transactions:", "Net Asset Value (NAV) in Base:", "Open Positions:",
"Trades:" and the configuration options) move the <ul> so it is nested inside
the corresponding parent <li> (wrap each sub-<ul> inside the <li> that contains
the header text like "Cash Report:", "Cash Transactions:", "Open Positions:",
etc.), ensure all <li> elements enclose their child <ul> and that tags are
properly opened/closed to produce semantic HTML.
In `@app/views/settings/providers/show.html.erb`:
- Around line 82-86: Replace the hard-coded title in the settings_section call
(the title: "Interactive Brokers Flex Query (beta)" in show.html.erb) with a
translation helper call (e.g. t('settings.providers.ibkr.title')), and add the
corresponding key settings.providers.ibkr.title to config/locales/en.yml with
the English text "Interactive Brokers Flex Query (beta)"; ensure the view uses
t(...) and the locale file contains the new key.
In `@db/migrate/20260508110000_create_ibkr_items_and_accounts.rb`:
- Around line 9-11: The migration defines credential columns using t.string for
token and query_id; change both to t.text to match other provider migrations and
the project's convention (these columns are used with ActiveRecord encryption in
IbkrItem). Update the migration's column definitions for token and query_id from
t.string to t.text, run/prepare a new migration or rollback+recreate this
migration if already applied, and ensure the schema.rb is updated so the
database column types and encrypted fields remain consistent with other
providers.
In `@test/models/account/syncer_test.rb`:
- Around line 25-29: The test currently only asserts call counts for
Account::MarketDataImporter.import_all,
Balance::Materializer.materialize_balances, and
IbkrAccount::HistoricalBalancesSync.sync! but not their order; modify the test
around Account::Syncer.new(account).perform_sync to enforce ordering by creating
a Mocha sequence (or equivalent) and attach each expectation (.expects(...)) to
that sequence in the desired order: import_all first, materialize_balances
second, and sync! third so the calls are asserted to occur in that exact
sequence when Account::Syncer#perform_sync is invoked.
In `@test/models/transaction_test.rb`:
- Around line 143-154: Add a unit test in test/models/transaction_test.rb to
exercise the fallback branch of Transaction#activity_security by creating a
Transaction with extra: { "security" => { "id" => security.id } } (use the same
securities fixture like securities(:aapl)) and assert that
transaction.activity_security returns that security; this mirrors the existing
top-level security_id test but targets the nested extra["security"]["id"] path
to cover the untested branch.
---
Nitpick comments:
In `@app/controllers/ibkr_items_controller.rb`:
- Around line 134-142: The current code materializes all ibkr_accounts in Ruby
via Current.family.ibkr_items.flat_map(&:ibkr_accounts).select and filters
account_provider there; instead perform the filtering and ordering at the DB
level: start from Current.family.ibkr_items, join to :ibkr_accounts and
left_join ibkr_accounts: :account_provider, add where(account_providers: { id:
nil }) to only return ibkr_accounts without providers, select the ibkr_accounts
records (not the parent ibkr_items), and order by
COALESCE(ibkr_accounts.updated_at, ibkr_accounts.created_at) DESC so
`@available_ibkr_accounts` is a DB-scoped collection for the
select_existing_account render.
In `@app/models/ibkr_account/activities_processor.rb`:
- Around line 195-213: security_symbol_for_conid is doing repeated
with_indifferent_access conversions over `@ibkr_account.raw_holdings_payload` and
raw_activities_payload[:trades], causing O(N×M) work; instead build and memoize
a conid→symbol lookup on the processor instance (e.g. `@conid_symbol_map`) the
first time it’s needed. Populate that map once by iterating
Array(`@ibkr_account.raw_holdings_payload`) and
Array(`@ibkr_account.raw_activities_payload`&.dig("trades") ||
`@ibkr_account.raw_activities_payload`&.dig(:trades)), calling
with_indifferent_access only while creating the map, then change
security_symbol_for_conid to return `@conid_symbol_map`[conid.to_s] (or nil) so
resolve_security_for_cash_transaction and resolve_security remain unchanged but
much faster.
- Around line 150-164: The supported_trade? predicate currently allows rows with
quantity "0" because it only checks presence; update supported_trade? to
explicitly reject zero quantities by converting row[:quantity] to a numeric
(e.g., using to_d or to_f) and returning false if the numeric value is zero or
effectively zero, ensuring any zero or zero-equivalent quantity rows are
filtered out before further processing (leave other checks like
fx_rate_available?(row) intact).
In `@app/models/ibkr_item.rb`:
- Around line 35-43: Replace the generic StandardError with a dedicated error
class so callers can distinguish provider-configuration failures: define a new
error class (e.g., IbkrItem::ProviderNotConfiguredError) and raise that from
import_latest_ibkr_data instead of StandardError, update the rescue/logging
behavior if necessary to reference the new class (method import_latest_ibkr_data
and the IbkrItem::Importer call are the relevant symbols), and ensure any tests
or upstream handlers catch IbkrItem::ProviderNotConfiguredError rather than
matching the error message.
In `@app/models/ibkr_item/report_parser.rb`:
- Around line 78-100: The section_rows method handles three IBKR XML shapes
(rows as children of container elements, container elements themselves as rows,
or rows directly under statement) but the logic is dense; extract the fallback
"statement-direct" path into a small helper (e.g., a private method like
statement_direct_rows(statement, row_names)) and add a short comment above
section_rows describing the three input shapes and the chosen resolution order;
update section_rows to call the new helper when rows.empty? to keep behavior
identical while making the fallback path explicit and easier to read.
In `@app/models/ibkr_item/unlinking.rb`:
- Around line 9-10: Replace the hardcoded polymorphic type lookup with the
polymorphic association: in the block where ibkr_accounts.find_each yields
provider_account and you currently call AccountProvider.where(provider_type:
"IbkrAccount", provider_id: provider_account.id), change the query to use
AccountProvider.where(provider: provider_account) so Rails sets provider_type
and provider_id via the polymorphic mapping (update any variable names like
provider_account and the surrounding ibkr_accounts.find_each loop accordingly).
In `@app/views/ibkr_items/_ibkr_item.html.erb`:
- Around line 11-12: The IB badge uses hard-coded hex Tailwind classes; replace
the raw color classes on the badge container and the span (the div with class
"bg-[`#D32F2F`]/10" and the span with "text-[`#D32F2F`]") with the project's
Tailwind design-system token classes (e.g., use the design token for the
danger/background token on the container and the corresponding text color token
on the span) so the badge follows theme tokens instead of raw hex values.
- Around line 19-107: The partial contains hard-coded user-facing strings (e.g.,
"Deletion in progress", "Flex Web Service", "Syncing", "Credentials need
attention", "Error", "Synced ... ago.", "Never synced.", "Set up accounts",
"Delete", "Accounts need setup", "No IBKR accounts discovered yet.", and
link/button labels used with DS::Link, DS::Tooltip, menu.with_item,
icon/tag.span, and ProviderSyncSummary rendering); replace each literal with a
call to t(...) using descriptive locale keys (e.g.,
ibkr_items.status.deletion_in_progress, ibkr_items.service.flex_web,
ibkr_items.status.syncing, ibkr_items.actions.setup_accounts,
ibkr_items.empty.no_accounts, etc.), update calls in the partial (tag.span,
render DS::Tooltip, DS::Link, menu.with_item) to use the t(...) results, and add
corresponding keys to the locale YAML so all user-facing strings are localized.
In `@app/views/ibkr_items/select_existing_account.html.erb`:
- Around line 3-33: Replace all hard-coded user-facing strings in this view with
i18n keys and calls to t(...): change the dialog header (dialog.with_header),
empty-state paragraphs and list items, the balance label inside the span that
uses number_to_currency, and the DS::Button/DS::Link text props to use t("...")
keys; keep the dynamic interpolations (like ibkr_account.name, currency and
number_to_currency) intact. Add corresponding entries to config/locales/en.yml
under a sensible namespace (e.g. ibkr_items.select_existing_account.header,
.empty_state.line1, .empty_state.line2, .balance_label, .actions.link,
.actions.cancel). Ensure you reference the same keys in the view via
t("ibkr_items.select_existing_account.<key>") to locate and replace each
hard-coded string.
In `@app/views/settings/providers/_ibkr_panel.html.erb`:
- Around line 10-118: The partial contains many hard-coded user-facing strings;
extract them into i18n keys (e.g., under a new locale namespace like
ibkr_panel.*) and replace literals with t('ibkr_panel.<key>') usages throughout
the partial: move the "Setup instructions:" heading, every list item and note
paragraph, the placeholders for form.text_field :query_id and :token, button
labels used with button_to (Sync, Disconnect Interactive Brokers?), the
form.submit text, the status messages ("Not configured.", the sync summary label
if any) and link text into locale keys; keep view logic and helpers
(styled_form_with, form.text_field, button_to sync_ibkr_item_path, form.submit,
ibkr_item.sync_status_summary) intact and reference the newly created keys in
those calls so all user-visible strings use the t(...) helper.
- Line 116: Replace the raw Tailwind color utility on the small status dot (the
div with classes "w-2 h-2 bg-gray-400 rounded-full") with the corresponding
design-system token class from sure-design-system.css; locate that div in
_ibkr_panel.html.erb and swap out "bg-gray-400" for the project's functional
token class that maps to the intended neutral color (the named token used across
sure-design-system.css) so the component uses the design token instead of a raw
Tailwind utility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b04e6f85-d173-429f-9aa7-b36a033fd7f5
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (68)
app/components/UI/account/activity_date.html.erbapp/controllers/accounts_controller.rbapp/controllers/ibkr_items_controller.rbapp/javascript/controllers/time_series_chart_controller.jsapp/models/account.rbapp/models/account/provider_import_adapter.rbapp/models/account/syncer.rbapp/models/balance/sync_cache.rbapp/models/data_enrichment.rbapp/models/family.rbapp/models/family/ibkr_connectable.rbapp/models/holding.rbapp/models/holding/materializer.rbapp/models/holding/portfolio_cache.rbapp/models/ibkr_account.rbapp/models/ibkr_account/activities_processor.rbapp/models/ibkr_account/data_helpers.rbapp/models/ibkr_account/historical_balances_sync.rbapp/models/ibkr_account/holdings_processor.rbapp/models/ibkr_account/processor.rbapp/models/ibkr_item.rbapp/models/ibkr_item/importer.rbapp/models/ibkr_item/provided.rbapp/models/ibkr_item/report_parser.rbapp/models/ibkr_item/syncer.rbapp/models/ibkr_item/unlinking.rbapp/models/provider/ibkr_adapter.rbapp/models/provider/ibkr_flex.rbapp/models/snaptrade_account/processor.rbapp/models/trade.rbapp/models/transaction.rbapp/views/accounts/index.html.erbapp/views/holdings/_cash.html.erbapp/views/holdings/_cost_basis_cell.html.erbapp/views/holdings/_holding.html.erbapp/views/ibkr_items/_ibkr_item.html.erbapp/views/ibkr_items/select_existing_account.html.erbapp/views/ibkr_items/setup_accounts.html.erbapp/views/settings/providers/_ibkr_panel.html.erbapp/views/settings/providers/show.html.erbapp/views/trades/_trade.html.erbapp/views/transactions/_transaction.html.erbconfig/routes.rbdb/migrate/20260508110000_create_ibkr_items_and_accounts.rbdb/migrate/20260509153000_simplify_ibkr_account_payloads.rbdb/migrate/20260509170000_add_extra_to_trades.rbdb/migrate/20260509171000_add_raw_equity_summary_payload_to_ibkr_accounts.rbdb/schema.rbflake.nixtest/controllers/ibkr_items_controller_test.rbtest/fixtures/files/ibkr/flex_statement.xmltest/fixtures/ibkr_accounts.ymltest/fixtures/ibkr_items.ymltest/models/account/provider_import_adapter_test.rbtest/models/account/syncer_test.rbtest/models/account_ibkr_creation_test.rbtest/models/account_test.rbtest/models/balance/sync_cache_test.rbtest/models/holding/materializer_test.rbtest/models/holding/portfolio_cache_test.rbtest/models/holding_test.rbtest/models/ibkr_account/historical_balances_sync_test.rbtest/models/ibkr_account_processor_test.rbtest/models/ibkr_item_importer_test.rbtest/models/ibkr_item_report_parser_test.rbtest/models/snaptrade_account_processor_test.rbtest/models/trade_test.rbtest/models/transaction_test.rb
| flash.now[:notice] = "Successfully configured Interactive Brokers." | ||
| render turbo_stream: [ | ||
| turbo_stream.replace( | ||
| "ibkr-providers-panel", | ||
| partial: "settings/providers/ibkr_panel" | ||
| ), | ||
| *flash_notification_stream_items | ||
| ] | ||
| else | ||
| redirect_to settings_providers_path, notice: "Successfully configured Interactive Brokers.", status: :see_other | ||
| end | ||
| else | ||
| @error_message = @ibkr_item.errors.full_messages.join(", ") | ||
|
|
||
| if turbo_frame_request? | ||
| render turbo_stream: turbo_stream.replace( | ||
| "ibkr-providers-panel", | ||
| partial: "settings/providers/ibkr_panel", | ||
| locals: { error_message: @error_message } | ||
| ), status: :unprocessable_entity | ||
| else | ||
| redirect_to settings_providers_path, alert: @error_message, status: :unprocessable_entity | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def update | ||
| attrs = ibkr_item_params.to_h | ||
| attrs["query_id"] = @ibkr_item.query_id if attrs["query_id"].blank? | ||
| attrs["token"] = @ibkr_item.token if attrs["token"].blank? | ||
|
|
||
| if @ibkr_item.update(attrs.merge(status: :good)) | ||
| @ibkr_item.sync_later unless @ibkr_item.syncing? | ||
|
|
||
| if turbo_frame_request? | ||
| flash.now[:notice] = "Successfully updated Interactive Brokers configuration." | ||
| render turbo_stream: [ | ||
| turbo_stream.replace( | ||
| "ibkr-providers-panel", | ||
| partial: "settings/providers/ibkr_panel" | ||
| ), | ||
| *flash_notification_stream_items | ||
| ] | ||
| else | ||
| redirect_to settings_providers_path, notice: "Successfully updated Interactive Brokers configuration.", status: :see_other | ||
| end | ||
| else | ||
| @error_message = @ibkr_item.errors.full_messages.join(", ") | ||
|
|
||
| if turbo_frame_request? | ||
| render turbo_stream: turbo_stream.replace( | ||
| "ibkr-providers-panel", | ||
| partial: "settings/providers/ibkr_panel", | ||
| locals: { error_message: @error_message } | ||
| ), status: :unprocessable_entity | ||
| else | ||
| redirect_to settings_providers_path, alert: @error_message, status: :unprocessable_entity |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
i18n: extract flash/notice copy via t().
All flash notices and alerts in this controller (and the inline attrs["query_id"]/token fallback messages) are hard-coded English strings. As per coding guidelines, every user-facing string must go through t(), with keys organized hierarchically (e.g., ibkr_items.flash.created, ibkr_items.flash.updated, ibkr_items.errors.not_configured, etc.) and entries added to config/locales/en.yml. The same applies to lines 96, 111, 122, 130, 155, 160, 174, 177, and 232–236.
As per coding guidelines: "Always use t() helper for user-facing strings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/controllers/ibkr_items_controller.rb` around lines 27 - 83, Replace all
hard-coded user-facing strings in this controller with i18n calls using the t()
helper: update flash notices/alerts in the create and update flows (e.g., the
strings "Successfully configured Interactive Brokers.", "Successfully updated
Interactive Brokers configuration.", and any redirect alert messages), and the
`@error_message` construction that joins `@ibkr_item.errors.full_messages` so you
pass localized wrapper keys; use hierarchical keys such as
ibkr_items.flash.created, ibkr_items.flash.updated, and
ibkr_items.errors.not_configured (or similar) referenced from the create and
update methods and from any places using attrs["query_id"]/attrs["token"]
fallbacks, and add corresponding entries to config/locales/en.yml. Ensure you
call t(...) where the current literal strings are used (and keep the original
messages as the default locale values in en.yml).
| def apply_provider_balance_overrides | ||
| return unless account.linked_to?("IbkrAccount") | ||
|
|
||
| ibkr_account = account.account_providers.find_by(provider_type: "IbkrAccount")&.provider | ||
| return unless ibkr_account | ||
|
|
||
| IbkrAccount::HistoricalBalancesSync.new(ibkr_account).sync! | ||
| end |
There was a problem hiding this comment.
Prevent IBKR override failures from failing the entire account sync.
Line 45 can raise and currently aborts perform_sync; this makes a provider-specific enrichment step capable of failing the whole sync path.
Suggested fix
def apply_provider_balance_overrides
return unless account.linked_to?("IbkrAccount")
ibkr_account = account.account_providers.find_by(provider_type: "IbkrAccount")&.provider
return unless ibkr_account
IbkrAccount::HistoricalBalancesSync.new(ibkr_account).sync!
+ rescue => e
+ Rails.logger.error("Error syncing IBKR historical balances for account #{account.id}: #{e.message}")
+ Sentry.capture_exception(e)
end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/account/syncer.rb` around lines 39 - 46, The IBKR-specific call in
apply_provider_balance_overrides (the
IbkrAccount::HistoricalBalancesSync.new(ibkr_account).sync! invocation) can
raise and currently bubbles up to abort perform_sync; wrap that sync! call in a
rescue that captures StandardError (or Exception subclass you prefer) so
provider failures are swallowed and do not stop the overall account sync, and
log the exception (including message and backtrace) using the existing logger or
process logger; keep the early guards (account.linked_to? and
find_by(provider_type: "IbkrAccount")) but ensure any exception from
HistoricalBalancesSync.sync! is rescued and handled gracefully.
| name: build_trade_name(security.ticker, signed_quantity), | ||
| source: "ibkr", | ||
| activity_label: buy_sell == "SELL" ? "Sell" : "Buy", | ||
| exchange_rate: parse_decimal(row[:fx_rate_to_base]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Inconsistent exchange_rate value types stored in extra.
For trades the FX rate is passed as a BigDecimal (Line 65: exchange_rate: parse_decimal(row[:fx_rate_to_base])), but in cash transactions and commissions it is converted with &.to_f to a Float (Lines 95 and 130). This mixes precision/representation across activity types persisted under the same key, which can lead to subtle discrepancies in equality checks (e.g., BigDecimal("0.9") == 0.9 is false) and JSON round-trip differences. Pick one (BigDecimal preferred for monetary FX) and apply uniformly.
Also applies to: 95-95, 130-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/ibkr_account/activities_processor.rb` at line 65, The
extra["exchange_rate"] is stored as BigDecimal for trades (exchange_rate:
parse_decimal(row[:fx_rate_to_base]) in ActivitiesProcessor) but as Float for
cash and commission paths; make exchange_rate consistently a BigDecimal across
all activity types by replacing uses of &.to_f with the same parse_decimal(...)
conversion (or a single helper that returns BigDecimal) in the cash transaction
and commission branches referenced in this file so all extras store BigDecimal
for exchange_rate.
| def resolve_security(row) | ||
| data = row.with_indifferent_access | ||
| ticker = data[:symbol].to_s.strip.upcase | ||
| return nil if ticker.blank? | ||
|
|
||
| Security.find_by(ticker: ticker) || create_security_from_row(ticker) | ||
| rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique | ||
| Security.find_by(ticker: ticker) | ||
| end | ||
|
|
||
| def trade_date_for(row) | ||
| parse_date(row.with_indifferent_access[:trade_date]) || Date.current | ||
| end | ||
|
|
||
| def extract_currency(row, fallback: nil) | ||
| value = row.with_indifferent_access[:currency] | ||
| value.present? ? value.to_s.upcase : fallback | ||
| end | ||
|
|
||
| def create_security_from_row(ticker) | ||
| Security.create!(ticker: ticker, name: ticker) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect parsing/normalization in the IBKR provider/import paths to find which keys are populated alongside :symbol.
fd -t f -e rb -E node_modules . | xargs rg -n -C2 ":symbol" 2>/dev/null
rg -nP --type=rb -C3 '\b(listing_exchange|listingExchange|exchange|exch)\b'Repository: we-promise/sure
Length of output: 35581
🏁 Script executed:
cat -n app/models/ibkr_account/data_helpers.rbRepository: we-promise/sure
Length of output: 2327
🏁 Script executed:
# Find where IBKR data is imported and what fields are available
rg -A5 -B5 "def process" app/models/ibkr_account/ | head -100Repository: we-promise/sure
Length of output: 4773
🏁 Script executed:
# Look for IBKR test fixtures or sample data
fd -t f -e rb app/models/ibkr_account | xargs ls -laRepository: we-promise/sure
Length of output: 3906
🏁 Script executed:
# Search for where rows are passed to resolve_security in IBKR context
rg "resolve_security" app/models/ibkr_account/ -B3 -A1Repository: we-promise/sure
Length of output: 2245
🏁 Script executed:
# Find test fixtures with IBKR data to see what fields are available
find test -name "*ibkr*" -type f | head -20Repository: we-promise/sure
Length of output: 335
🏁 Script executed:
# Look at actual IBKR test data
find . -path ./node_modules -prune -o -type f -name "*.yml" -o -name "*.json" | xargs grep -l "ibkr" 2>/dev/null | head -10Repository: we-promise/sure
Length of output: 93
🏁 Script executed:
# Search for sample IBKR payloads in test files
rg "raw_holdings_payload|raw_activities_payload" test/ -A10 | head -100Repository: we-promise/sure
Length of output: 7203
🏁 Script executed:
# Look for IBKR field names like listing_exchange, exchange in the codebase
rg "listing.*exchange|listingExchange" . --type=rb | head -20Repository: we-promise/sure
Length of output: 86
🏁 Script executed:
cat -n test/models/ibkr_item_importer_test.rb | head -150Repository: we-promise/sure
Length of output: 1463
🏁 Script executed:
cat -n test/fixtures/ibkr_accounts.ymlRepository: we-promise/sure
Length of output: 912
🏁 Script executed:
cat -n test/fixtures/ibkr_items.yml | head -200Repository: we-promise/sure
Length of output: 462
🏁 Script executed:
# Find the IBKR flex statement fixture to see actual data structure
find test -name "*flex*" -type fRepository: we-promise/sure
Length of output: 103
🏁 Script executed:
# Look at the IBKR parser to understand how data is extracted from the XML
rg "class.*Parser" app/models/ibkr_item/ -A5Repository: we-promise/sure
Length of output: 549
🏁 Script executed:
# Check for any mention of exchange or listing in IBKR related files
rg -i "exchange|listing" app/models/ibkr_account/ app/models/ibkr_item/ | head -30Repository: we-promise/sure
Length of output: 401
🏁 Script executed:
cat test/fixtures/files/ibkr/flex_statement.xml | head -300Repository: we-promise/sure
Length of output: 3050
🏁 Script executed:
# Look at the ReportParser to see what fields it extracts from the XML
cat -n app/models/ibkr_item/report_parser.rb | head -200Repository: we-promise/sure
Length of output: 6047
resolve_security does not disambiguate by exchange — a valid concern, but the suggested fix assumes unavailable data.
Security has a unique index on upper(ticker), COALESCE(upper(exchange_operating_mic), ''), so multiple rows can exist for the same ticker across different exchanges. Security.find_by(ticker: ticker) returns whichever row matches first, and create_security_from_row creates a row with no exchange info, potentially conflating exchange-distinct securities.
However, IBKR Flex Query data does not expose exchange information. The available fields in parsed rows include symbol, conid, currency, assetCategory, securityID, securityIDType, etc., but no exchange or listing exchange identifier. The suggested approach of passing exchange data through resolve_security cannot be implemented without a data source for the exchange value.
Consider whether:
- Exchange information can be inferred from
securityID(e.g., ISIN → exchange lookup via a provider) conid(IBKR contract ID) already uniquely identifies a security across exchanges and could be used as the primary key instead of ticker- This is a limitation of the IBKR data format that needs an alternative approach
There was a problem hiding this comment.
I don't want to make changes in the core Security logic, because that's used throughout the application and also affects other providers and features. Unfortunately, IBKR's exchange identifiers are proprietary, and it doesn't provide clean MIC identifiers... I'll keep this as-is for now, as incorrect matches can be manually fixed by the user and I have not experienced any weird matches in my testing (with a handful of global stocks).
There was a problem hiding this comment.
@gian-reto, understood — that's a reasonable trade-off given the data limitations. Using conid as a lookup hint (via the holdings-based fallback already in activities_processor.rb) partially mitigates the risk, and for most mainstream stocks a ticker-only match will be correct. I'll note this constraint for future reference.
（´･ω･`）
✏️ Learnings added
Learnt from: gian-reto
Repo: we-promise/sure PR: 1712
File: app/models/ibkr_account/data_helpers.rb:48-69
Timestamp: 2026-05-09T20:14:21.086Z
Learning: In app/models/ibkr_account/data_helpers.rb, `resolve_security` intentionally resolves securities by ticker only (no exchange). IBKR Flex Query data does not expose clean MIC exchange identifiers — IBKR exchange codes are proprietary. Multi-exchange ticker disambiguation is therefore not feasible without an external lookup. This is an accepted limitation: incorrect matches are rare in practice and can be manually corrected. Do not flag the absence of exchange-based disambiguation in IBKR-related security resolution code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jjmata
Repo: we-promise/sure PR: 593
File: app/controllers/pages_controller.rb:200-203
Timestamp: 2026-01-09T12:25:59.839Z
Learning: Do not compare hard-coded strings to identify the 'Uncategorized Investments' category in Ruby/Rails code (e.g., ct.category.name != "Uncategorized Investments"). Localization/internationalization can change strings. Prefer: (1) a model-level predicate such as ct.category.uncategorized_investment? or (2) compare by identity with a canonical category, e.g., ct.category != Category.uncategorized_investments. This improves maintainability and correctness across translations. Apply this pattern to Ruby files that perform category checks (controllers, models, or elsewhere).
Learnt from: dgilperez
Repo: we-promise/sure PR: 934
File: app/models/family/syncer.rb:47-57
Timestamp: 2026-02-09T21:30:32.649Z
Learning: In Rails model reviews, for files under app/models, also inspect included concerns (e.g., Family::PlaidConnectable). Check the associations, scopes, and methods defined in the included concern modules. Do not flag a behavior as missing on the main model if it is implemented in a concern; instead verify whether the concern provides the expected behavior and is properly included. This pattern applies to other model files that use concerns.
Learnt from: UberDudePL
Repo: we-promise/sure PR: 1330
File: app/models/bond_lot.rb:399-401
Timestamp: 2026-04-08T06:24:33.376Z
Learning: In financial event code (e.g., settlement/maturity flows in app/models/**/*.rb), do not log sensitive monetary amounts (such as gross, tax, net) at info level. Instead, log only non-sensitive identifiers (e.g., record IDs, event names/status) or lower the log level to debug if amounts are required for troubleshooting. Avoid introducing new info-level log lines that include monetary fields.
Learnt from: EdeAbreu23
Repo: we-promise/sure PR: 1692
File: app/models/holding.rb:53-57
Timestamp: 2026-05-06T21:57:46.943Z
Learning: Guideline: store cost_basis on Holding as a per-share value (not total position cost). Templates should render total cost as cost_basis * qty. remap_security! should continue computing weighted-average per-share cost from per-share inputs. When provider data (e.g., Schwab via SimpleFIN or Plaid) supplies total position cost basis, normalize by dividing by qty before persisting to cost_basis. Implement this normalization in the provider/import/materialization path, not in Holding#avg_cost. This applies to app/models/holding.rb and related code paths, including app/views/holdings/_cost_basis_cell.html.erb and any importer/remap logic.
| def sync_status_summary | ||
| total_accounts = total_accounts_count | ||
| linked_count = linked_accounts_count | ||
| unlinked_count = unlinked_accounts_count | ||
|
|
||
| if total_accounts.zero? | ||
| "No IBKR accounts discovered yet" | ||
| elsif unlinked_count.zero? | ||
| "#{linked_count} account#{'s' unless linked_count == 1} linked" | ||
| else | ||
| "#{linked_count} linked, #{unlinked_count} need setup" | ||
| end | ||
| end | ||
|
|
||
| def institution_display_name | ||
| "Interactive Brokers" | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
i18n: replace hard-coded strings and inline pluralization with t() helpers.
sync_status_summary and institution_display_name return user-facing English strings. As per coding guidelines, use t() for user-facing strings and Rails pluralization for count-aware text.
♻️ Suggested change
def sync_status_summary
total_accounts = total_accounts_count
linked_count = linked_accounts_count
unlinked_count = unlinked_accounts_count
if total_accounts.zero?
- "No IBKR accounts discovered yet"
+ t("ibkr_items.sync_status.none_discovered")
elsif unlinked_count.zero?
- "#{linked_count} account#{'s' unless linked_count == 1} linked"
+ t("ibkr_items.sync_status.linked", count: linked_count)
else
- "#{linked_count} linked, #{unlinked_count} need setup"
+ t("ibkr_items.sync_status.partial", linked: linked_count, unlinked: unlinked_count)
end
end
def institution_display_name
- "Interactive Brokers"
+ t("ibkr_items.institution_display_name")
endAs per coding guidelines: "Always use t() helper for user-facing strings... Use Rails pluralization: t("transactions.count", count: @transactions.count)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def sync_status_summary | |
| total_accounts = total_accounts_count | |
| linked_count = linked_accounts_count | |
| unlinked_count = unlinked_accounts_count | |
| if total_accounts.zero? | |
| "No IBKR accounts discovered yet" | |
| elsif unlinked_count.zero? | |
| "#{linked_count} account#{'s' unless linked_count == 1} linked" | |
| else | |
| "#{linked_count} linked, #{unlinked_count} need setup" | |
| end | |
| end | |
| def institution_display_name | |
| "Interactive Brokers" | |
| end | |
| def sync_status_summary | |
| total_accounts = total_accounts_count | |
| linked_count = linked_accounts_count | |
| unlinked_count = unlinked_accounts_count | |
| if total_accounts.zero? | |
| t("ibkr_items.sync_status.none_discovered") | |
| elsif unlinked_count.zero? | |
| t("ibkr_items.sync_status.linked", count: linked_count) | |
| else | |
| t("ibkr_items.sync_status.partial", linked: linked_count, unlinked: unlinked_count) | |
| end | |
| end | |
| def institution_display_name | |
| t("ibkr_items.institution_display_name") | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/ibkr_item.rb` around lines 107 - 123, Update sync_status_summary
and institution_display_name to use Rails I18n helpers instead of hard-coded
strings: replace the literal "No IBKR accounts discovered yet" with a t(...)
lookup, use t(..., count: linked_count) or Rails pluralization via t with count
for the linked account text (e.g., key that supports pluralization) and
similarly for the "#{linked_count} linked, #{unlinked_count} need setup" message
use a translation key that accepts both counts (or compose using two t(...)
calls with count: linked_count and count: unlinked_count), and replace
institution_display_name's "Interactive Brokers" with a t(...) lookup; reference
the existing methods total_accounts_count, linked_accounts_count, and
unlinked_accounts_count to pass counts into the translation calls and add
appropriate keys to the locale files.
| if use_api_total_balance? | ||
| Rails.logger.info "SnaptradeAccount::Processor - Using API total for multi-currency holdings: #{snaptrade_account.current_balance}" | ||
| return snaptrade_account.current_balance || 0 |
There was a problem hiding this comment.
Avoid logging the balance amount at info level here.
This adds a new info log with current_balance, which is privacy-sensitive financial data. Please drop the amount from the message or move it to debug with account/context identifiers only.
Suggested change
- Rails.logger.info "SnaptradeAccount::Processor - Using API total for multi-currency holdings: #{snaptrade_account.current_balance}"
+ Rails.logger.debug "SnaptradeAccount::Processor - Using API total for multi-currency holdings for snaptrade_account=#{snaptrade_account.id}"Based on learnings: “In financial event code (e.g., settlement/maturity flows in app/models/**/*.rb), do not log sensitive monetary amounts (such as gross, tax, net) at info level.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/snaptrade_account/processor.rb` around lines 74 - 76, The info log
in SnaptradeAccount::Processor exposes a sensitive monetary value; remove the
balance amount from the info-level message (or demote the log to debug) and log
only non-sensitive context/identifiers (e.g., snaptrade_account.id or account
external_id) instead; update the log call in the block guarded by
use_api_total_balance? that currently references
snaptrade_account.current_balance so it no longer prints the amount but
preserves the return of snaptrade_account.current_balance || 0.
| def exchange_rate=(value) | ||
| if value.blank? | ||
| self.extra = (extra || {}).merge("exchange_rate" => nil, "exchange_rate_invalid" => false) | ||
| else | ||
| begin | ||
| normalized_value = Float(value) | ||
| self.extra = (extra || {}).merge("exchange_rate" => normalized_value, "exchange_rate_invalid" => false) | ||
| rescue ArgumentError, TypeError | ||
| self.extra = (extra || {}).merge("exchange_rate" => value, "exchange_rate_invalid" => true) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Ruby, does Float("NaN") or Float("Infinity") raise an exception, and how do BigDecimal NaN/Infinity values behave in comparisons like <= 0?
💡 Result:
Float("NaN") and Float("Infinity") do not raise an exception; they produce non-finite Float values (NaN / +Infinity or -Infinity) as valid results [1]. For BigDecimal, parsing/creating NaN and infinities via strings like "NaN" and "Infinity" is supported, and NaN/infinity are represented as special BigDecimal values [2]. Comparison behavior: 1) Float - Ruby documents that the result of comparisons involving NaN is “undefined” / implementation-dependent (e.g., NaN < NaN) [3]. 2) BigDecimal - BigDecimal’s NaN is not considered equal to any value, even itself [2]. - Ruby’s BigDecimal comparison operators are expected to follow that “NaN is incomparable / no meaningful ordering” idea; in practice, comparisons involving BigDecimal NaN do not behave like ordinary total ordering (e.g., <=> returns nil; and using <= can raise an ArgumentError for some operand type combinations) [4][5]. So specifically for the question “how do BigDecimal NaN/Infinity values behave in comparisons like <= 0?”: - NaN: comparisons with <= are not a meaningful numeric ordering; NaN is not equal to itself [2] and comparisons can yield non-boolean outcomes / nil ordering (and in some cases raise ArgumentError) [4][5]. - Infinity: BigDecimal supports Infinity and -Infinity as distinct values [2], so comparisons against finite numbers like 0 follow the expected infinite ordering (Infinity should satisfy <= only in the negative-infinity case; +Infinity should not satisfy <= 0). (The doc confirms Infinity/−Infinity are valid BigDecimal values, but the exact truth table for each operator is not explicitly shown in the excerpts retrieved.) [2]. If you want robust handling, check nan? / infinite? before comparing: BigDecimal provides #nan? and #infinite? predicates [2].
Citations:
- 1: https://ruby-doc.org/core/Float.html
- 2: https://docs.ruby-lang.org/en/3.0/BigDecimal.html
- 3: https://ruby-doc.org/core-3.0.2/Float.html
- 4: https://stackoverflow.com/questions/38261689/ruby-enumerable-cant-compare-bigdecimal-nan-in-min-function
- 5: https://bugs.ruby-lang.org/issues/2661
Reject non-finite exchange rates in the validator.
Float(value) accepts "NaN" and "Infinity" without raising exceptions—they produce valid Float objects. The setter's rescue clause only catches ArgumentError and TypeError, so these non-finite values pass through and are marked as valid. The validator's <= 0 check then fails to detect them because NaN comparisons have undefined behavior in Ruby. Explicitly reject non-finite values using nan? or infinite? predicates to prevent poisoning downstream currency conversions.
Applies to lines 21-29 (setter) and 79-87 (validator).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/trade.rb` around lines 21 - 29, The setter exchange_rate=
currently accepts "NaN"/"Infinity" because Float(value) doesn't raise; after
converting (normalized_value = Float(value)) add a check using
normalized_value.finite? (or normalized_value.nan?/infinite?) and if not finite
treat it as invalid (set "exchange_rate_invalid" => true and store the original
value or nil as appropriate) instead of marking valid; likewise update the
validator that performs the numeric <= 0 check (the method that validates
exchange_rate) to first reject non-finite values by checking value.finite? (or
value.nan?/value.infinite?) and mark them invalid before doing the <= 0
comparison so NaN/Infinity can't pass validation.
| def activity_security | ||
| return @activity_security if defined?(@activity_security) | ||
| return @activity_security = nil if activity_security_id.blank? | ||
|
|
||
| @activity_security = Security.find_by(id: activity_security_id) | ||
| end |
There was a problem hiding this comment.
activity_security adds an N+1 query path for transaction rows.
Line [155] performs a Security.find_by per transaction instance. In list rendering, this becomes one extra query per row. Please preload referenced securities in bulk and resolve from a map in the render context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/transaction.rb` around lines 151 - 156, The activity_security
method currently does a per-instance lookup via Security.find_by(id:
activity_security_id), causing N+1 queries; change callers to preload all
required Security records (e.g., fetch Security.where(id:
transactions.map(&:activity_security_id)).index_by(&:id)) and replace the
per-row lookup with a map lookup (use the preloaded_map[activity_security_id])
or convert activity_security to read from an association and use ActiveRecord
preload/includes in the query that builds the transactions list; remove or
short-circuit the Security.find_by usage in the activity_security method (or
make it use a provided preloaded map) so rendering uses the bulk-loaded
securities instead of issuing individual queries.
| <% content_for :title, "Set Up Interactive Brokers Accounts" %> | ||
|
|
||
| <%= render DS::Dialog.new(disable_click_outside: true) do |dialog| %> | ||
| <% dialog.with_header(title: "Set Up Your Interactive Brokers Accounts") do %> | ||
| <div class="flex items-center gap-2"> | ||
| <%= icon "chart-line", class: "text-primary" %> | ||
| <span class="text-primary">Select which IBKR brokerage accounts to link.</span> | ||
| </div> | ||
| <% end %> | ||
|
|
||
| <% dialog.with_body do %> | ||
| <div class="space-y-6"> | ||
| <div class="bg-surface border border-primary p-4 rounded-lg"> | ||
| <div class="flex items-start gap-3"> | ||
| <%= icon "info", size: "sm", class: "text-primary mt-0.5 flex-shrink-0" %> | ||
| <div> | ||
| <p class="text-sm text-primary mb-2"><strong>IBKR Flex Query Import</strong></p> | ||
| <ul class="text-xs text-secondary space-y-1 list-disc list-inside"> | ||
| <li>Holdings with current prices and quantities</li> | ||
| <li>Cost basis per position</li> | ||
| <li>Trades, dividends, commissions, and cash deposits or withdrawals</li> | ||
| </ul> | ||
| <p class="text-xs text-warning mt-2"> | ||
| <%= icon "alert-triangle", size: "xs", class: "inline-block mr-1" %> | ||
| Historical activity is limited to the report window of the Flex Query | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <% if @waiting_for_sync %> | ||
| <div id="ibkr-sync-spinner" class="flex flex-col items-center justify-center py-6 space-y-3"> | ||
| <div class="animate-spin rounded-full h-6 w-6 border-b-2 border-primary"></div> | ||
| <p class="text-secondary text-center">Fetching accounts from Interactive Brokers...</p> | ||
| </div> | ||
| <div class="flex gap-3 justify-center"> | ||
| <%= render DS::Link.new( | ||
| text: "Refresh", | ||
| variant: "secondary", | ||
| icon: "refresh-cw", | ||
| href: setup_accounts_ibkr_item_path(@ibkr_item), | ||
| frame: "_top" | ||
| ) %> | ||
| <%= render DS::Link.new( | ||
| text: "Cancel", | ||
| variant: "ghost", | ||
| href: accounts_path, | ||
| frame: "_top" | ||
| ) %> | ||
| </div> | ||
| <% elsif @no_accounts_found %> | ||
| <div class="no-accounts-found flex flex-col items-center justify-center py-6 space-y-3"> | ||
| <%= icon "alert-circle", size: "lg", class: "text-warning" %> | ||
| <p class="text-primary text-center font-medium">No accounts found.</p> | ||
| <p class="text-secondary text-center text-sm">Sure could not find any IBKR accounts in the latest Flex report.</p> | ||
| </div> | ||
| <div class="flex gap-3 justify-center"> | ||
| <%= render DS::Link.new( | ||
| text: "Back to Settings", | ||
| variant: "secondary", | ||
| href: settings_providers_path, | ||
| frame: "_top" | ||
| ) %> | ||
| </div> | ||
| <% else %> | ||
| <%= form_with url: complete_account_setup_ibkr_item_path(@ibkr_item), method: :post, data: { turbo_frame: "_top" } do %> | ||
| <% if @unlinked_accounts.any? %> | ||
| <div class="space-y-4"> | ||
| <h3 class="font-medium text-primary">Available accounts</h3> | ||
|
|
||
| <% @unlinked_accounts.each do |ibkr_account| %> | ||
| <div class="border border-primary rounded-lg p-4 hover:bg-surface transition-colors"> | ||
| <div class="flex items-center gap-3"> | ||
| <input type="checkbox" | ||
| id="account_<%= ibkr_account.id %>" | ||
| name="account_ids[]" | ||
| value="<%= ibkr_account.id %>" | ||
| checked | ||
| class="cursor-pointer"> | ||
| <label for="account_<%= ibkr_account.id %>" class="flex-1 cursor-pointer"> | ||
| <h4 class="font-medium text-primary"><%= ibkr_account.name %></h4> | ||
| <p class="text-sm text-secondary"> | ||
| Investment • | ||
| Balance: <%= number_to_currency(ibkr_account.current_balance || 0, unit: Money::Currency.new(ibkr_account.currency || "USD").symbol) %> | ||
| </p> | ||
| <% if ibkr_account.ibkr_account_id.present? %> | ||
| <p class="text-xs text-secondary">Account ID: <%= ibkr_account.ibkr_account_id %></p> | ||
| <% end %> | ||
| </label> | ||
| </div> | ||
| </div> | ||
| <% end %> | ||
| </div> | ||
|
|
||
| <div class="flex gap-3 mt-6"> | ||
| <%= render DS::Button.new( | ||
| text: "Create selected accounts", | ||
| variant: "primary", | ||
| icon: "plus", | ||
| type: "submit", | ||
| class: "flex-1" | ||
| ) %> | ||
| <%= render DS::Link.new( | ||
| text: "Cancel", | ||
| variant: "secondary", | ||
| href: accounts_path, | ||
| frame: "_top" | ||
| ) %> | ||
| </div> | ||
| <% end %> | ||
| <% end %> | ||
|
|
||
| <% if @unlinked_accounts.any? && @linkable_accounts.any? %> | ||
| <div class="border-t border-secondary pt-4 mt-2"> | ||
| <p class="text-xs text-secondary mb-3">Or link a discovered IBKR account to an existing manual investment account:</p> | ||
| <div class="space-y-2"> | ||
| <% @unlinked_accounts.each do |ibkr_account| %> | ||
| <%= form_with url: link_existing_account_ibkr_items_path, method: :post, data: { turbo_frame: "_top" } do |link_form| %> | ||
| <%= link_form.hidden_field :ibkr_account_id, value: ibkr_account.id %> | ||
| <p class="text-xs text-primary mb-1"><%= ibkr_account.name %></p> | ||
| <div class="flex items-center gap-2"> | ||
| <%= link_form.select :account_id, | ||
| options_for_select(@linkable_accounts.map { |account| ["#{account.name} (#{number_to_currency(account.balance, unit: Money::Currency.new(account.currency || 'USD').symbol)})", account.id] }), | ||
| { prompt: "Select an account..." }, | ||
| class: "bg-container border border-primary rounded px-2 py-1 text-sm text-primary flex-1 min-w-0" %> | ||
| <%= render DS::Button.new( | ||
| text: "Link", | ||
| variant: "secondary", | ||
| size: "sm", | ||
| type: "submit" | ||
| ) %> | ||
| </div> | ||
| <% end %> | ||
| <% end %> | ||
| </div> | ||
| </div> | ||
| <% end %> | ||
|
|
||
| <% if @linked_accounts.any? %> | ||
| <div class="<%= 'border-t border-primary pt-6 mt-4' if @unlinked_accounts.any? %>"> | ||
| <h3 class="font-medium text-primary mb-4">Already linked</h3> | ||
| <% @linked_accounts.each do |ibkr_account| %> | ||
| <div class="border border-success/20 bg-success/5 rounded-lg p-4 mb-2"> | ||
| <div class="flex items-center justify-between"> | ||
| <div class="flex items-center gap-3"> | ||
| <%= icon "check-circle", class: "text-success" %> | ||
| <div> | ||
| <h4 class="font-medium text-primary"><%= ibkr_account.name %></h4> | ||
| <p class="text-sm text-secondary"> | ||
| Linked to: <%= link_to ibkr_account.current_account.name, account_path(ibkr_account.current_account), class: "link" %> | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <% end %> | ||
| </div> | ||
|
|
||
| <% if @unlinked_accounts.blank? %> | ||
| <div class="flex justify-end mt-4"> | ||
| <%= render DS::Link.new( | ||
| text: "Done", | ||
| variant: "primary", | ||
| href: accounts_path, | ||
| frame: "_top" | ||
| ) %> | ||
| </div> | ||
| <% end %> | ||
| <% end %> | ||
| <% end %> | ||
| </div> | ||
| <% end %> | ||
| <% end %> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add i18n support for all user-facing strings.
The view contains numerous hard-coded English strings (e.g., "Set Up Your Interactive Brokers Accounts", "Available accounts", "Create selected accounts", etc.) that should use the t() helper for proper internationalization. All user-facing text should be moved to config/locales/en.yml with appropriate hierarchical keys.
As per coding guidelines: Always use t() helper for user-facing strings. Organize i18n keys hierarchically by feature: ibkr_items.setup_accounts.title, ibkr_items.setup_accounts.info_box.title, etc.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/views/ibkr_items/setup_accounts.html.erb` around lines 1 - 173, Replace
all hard-coded user-facing strings in this view with i18n calls using the t()
helper and add corresponding keys under a hierarchical namespace like
ibkr_items.setup_accounts in config/locales/en.yml; specifically update
content_for :title, the DS::Dialog header title, the info box headings and
bullets, status messages (`@waiting_for_sync/`@no_accounts_found), form labels and
checkbox label text (e.g., account_<%= ibkr_account.id %> label), button and
link texts ("Create selected accounts", "Cancel", "Refresh", "Back to Settings",
"Link", "Done"), section headers ("Available accounts", "Already linked"), the
select prompt ("Select an account...") and any interpolated strings that include
currency/amounts so they use t() with interpolation where needed; ensure keys
follow the suggested hierarchy (e.g., ibkr_items.setup_accounts.title,
.info_box.title, .info_box.bullets.item1, .buttons.create_selected,
.status.fetching_accounts, .select.prompt, .linked.already_linked) and replace
the inline literals with t('ibkr_items.setup_accounts.xxx').
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
|
Nice! If you can get the PR comments resolved I can merge quickly before this other work lands/creates conflicts. EDIT: and the CI tests passing. :-) |
|
@jjmata very cool, thanks! I'll probably have time later today to go through all the comments, implement the suggestions and make tests pass 👍 |
gian-reto
force-pushed
the
gta/ibkr-provider
branch
from
3056b2a to
a8b0aef
Compare
github-project-automation
Bot
moved this from Todo
to Done
in Latest release train (v0.7.1) - May 2026
|
Sorry, my bad... I didn't want to close this. Working on it! |
2 participants
The Lunch Flow and SnapTrade integrations unfortunately don't support syncing trade history and other activity from Interactive Brokers. Additionally, holdings and cost basis values might be incomplete, see #898.
Therefore, I decided to implement a separate provider for IBKR, based on Flex Queries. Setting it up requires a bit of manual configuration in the IBKR dashboard, but once the Flex Query has been created, syncing happens automatically.
Features:
Currently has some restrictions:
Misc.
privacy-sensitiveto fix privacy view in the account detail views (activity & holdings list).Summary by CodeRabbit
Release Notes
New Features
Improvements