Indexa Capital `Provider` unaddressed review comments

[jjmata]

PR #933 unaddressed review comments

Source: #933

Review comments (no replies)

• chatgpt-codex-connector[bot]: Handle zero balances when importing Indexa Capital accounts

  Full comment

  **<sub><sub>![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)</sub></sub>  Handle zero balances when importing Indexa Capital accounts**
  
  The current balance is only applied when `data[:current_balance].present?`, which treats `0` as blank. If an Indexa Capital account legitimately has a zero balance, this skips the update and leaves whatever stale balance was previously stored, so UI and sync stats can be incorrect. Consider checking for `nil` instead (or using `data.key?`) so zero balances are persisted.
  
  Useful? React with 👍 / 👎.
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟠 Major

  Full comment

  _⚠️ Potential issue_ | _🟠 Major_
  
  **Synchronous API calls on a request thread — blocks the user and risks timeouts.**
  
  `fetch_accounts_synchronously` (Line 128) performs N+1 HTTP requests (one `list_accounts` + one `get_account_balance` per account) inline during a GET request. If the Indexa Capital API is slow or unreachable, this will hang the request and may exceed web-server timeouts. As per coding guidelines, heavy operations should use background jobs (Sidekiq).
  
  Additionally, if `fetch_accounts_synchronously` rescues an API error at Lines 373-378, it sets `flash.now[:alert]` but `@indexa_capital_accounts` on Line 130 may not be populated, leading to a potential `NoMethodError` in the view.
  
  <details>
  <summary>Minimum fix for nil safety</summary>
  
  ```diff
       # Always fetch fresh data (accounts + balances) when user visits this page
       fetch_accounts_synchronously(indexa_capital_item)
   
  -    `@indexa_capital_accounts` = indexa_capital_item.indexa_capital_accounts
  +    `@indexa_capital_accounts` = indexa_capital_item.indexa_capital_accounts
                                                   .left_joins(:account_provider)
                                                   .where(account_providers: { id: nil })
                                                   .order(:name)
  +  rescue => e
  +    Rails.logger.error "IndexaCapitalItemsController#select_accounts - #{e.class}: #{e.message}"
  +    `@indexa_capital_accounts` = indexa_capital_item.indexa_capital_accounts
  +                                                 .left_joins(:account_provider)
  +                                                 .where(account_providers: { id: nil })
  +                                                 .order(:name)
  +    flash.now[:alert] = t(".api_error", message: e.message)
  

  🤖 Prompt for AI Agents

  In `@app/controllers/indexa_capital_items_controller.rb` around lines 117 - 134,
  select_accounts is making synchronous network calls via
  fetch_accounts_synchronously which can block the request; change this to
  schedule a background job (e.g., IndexaCapital::RefreshAccountsJob.enqueue with
  Current.family.indexa_capital_items.first.id) to perform
  list_accounts/get_account_balance off-request and remove the inline
  fetch_accounts_synchronously call, and make the controller render the page using
  the cached records instead. Also harden nil-safety by initializing
  `@indexa_capital_accounts` = indexa_capital_item&.indexa_capital_accounts || []
  before using it and ensure flash.now[:alert] from fetch_accounts_synchronously’s
  rescue is only set by the background job or by a lightweight check that does not
  rely on populated `@indexa_capital_accounts`.
  

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟠 Major

  Full comment

  _⚠️ Potential issue_ | _🟠 Major_
  
  **`fetch_accounts_synchronously` makes per-account balance calls in a loop — N+1 HTTP requests.**
  
  Lines 353-362 iterate all accounts from `list_accounts` and make an individual `get_account_balance` HTTP call for each. With many accounts this multiplies latency and API quota usage. If the provider API supports batch balance retrieval, that would be preferable. Otherwise, consider parallel requests or moving this to a background job.
  
  <details>
  <summary>🤖 Prompt for AI Agents</summary>
  
  

  In @app/controllers/indexa_capital_items_controller.rb around lines 347 - 379,
  fetch_accounts_synchronously currently does an N+1 by calling
  provider.get_account_balance per account after provider.list_accounts; replace
  this with a single batched retrieval (e.g. use a provider method like
  batch_get_account_balances or get_accounts_balances that accepts an array of
  account_numbers and returns a map of balances) and merge those balances into
  accounts_data before upserting, or if the provider has no batch API, move the
  per-account balance fetch into an async/background job (or use
  parallel/concurrent requests) to avoid blocking the request thread; update
  references to indexa_capital_item.indexa_capital_provider, list_accounts and
  get_account_balance accordingly and remove the per-account rescue inside the
  loop so errors are handled centrally (auth/API errors remain handled by the
  existing rescue clauses).

  
  </details>
  
  <!-- fingerprinting:phantom:medusa:phoenix -->
  
  <!-- This is an auto-generated comment by CodeRabbit -->
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟠 Major

  Full comment

  _⚠️ Potential issue_ | _🟠 Major_
  
  <details>
  <summary>🧩 Analysis chain</summary>
  
  🏁 Script executed:
  
  ```shell
  rg "lock_args_method" --type=ruby -C3
  

  Repository: we-promise/sure

  Length of output: 1195

  ---

  🏁 Script executed:

  cat -n app/jobs/indexa_capital_activities_fetch_job.rb

  Repository: we-promise/sure

  Length of output: 1941

  ---

  🏁 Script executed:

  cat -n app/jobs/snaptrade_activities_fetch_job.rb | head -20

  Repository: we-promise/sure

  Length of output: 941

  ---

  🏁 Script executed:

  rg "sidekiq_options lock:" --type=ruby -A1 | head -40

  Repository: we-promise/sure

  Length of output: 686

  ---

  Fix lock_args_method to extract the account ID for stable lock key.

  args.first passes the ActiveRecord object, whose stringification may be unstable. Extract the ID instead, following the pattern established in snaptrade_activities_fetch_job.rb:

  -  sidekiq_options lock: :until_executed,
  -                  lock_args_method: ->(args) { args.first },
  +  sidekiq_options lock: :until_executed,
  +                  lock_args_method: ->(args) { [ args.first.id ] },
                     on_conflict: :log

  🤖 Prompt for AI Agents

  In `@app/jobs/indexa_capital_activities_fetch_job.rb` around lines 8 - 10, The
  Sidekiq lock currently uses lock_args_method: ->(args) { args.first } which
  passes an ActiveRecord object (unstable stringification); update the
  lock_args_method in indexa_capital_activities_fetch_job (the sidekiq_options
  block) to extract a stable identifier like the account ID (e.g., use
  args.first.id or args.first.to_i if the job may already receive an ID), matching
  the pattern used in snaptrade_activities_fetch_job.rb so the lock key is based
  on the account ID rather than the AR object.
  

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🔴 Critical

  Full comment

  _⚠️ Potential issue_ | _🔴 Critical_
  
  <details>
  <summary>🧩 Analysis chain</summary>
  
  🏁 Script executed:
  
  ```shell
  #!/bin/bash
  # Search for any index definitions on the securities table and ticker column
  echo "=== Searching for index definitions on securities table ==="
  rg -n 'add_index.*securities' --type=rb db/migrate/
  
  echo ""
  echo "=== Searching specifically for ticker-related indexes ==="
  rg -n 'ticker' --type=rb db/migrate/ | grep -E '(add_index|securities)'
  
  echo ""
  echo "=== Listing migration files to examine ==="
  ls -lhS db/migrate/ | head -20
  

  Repository: we-promise/sure

  Length of output: 2193

  ---

  🏁 Script executed:

  #!/bin/bash
  # Search for securities table creation
  echo "=== Searching for securities table creation ==="
  rg -n 'create_table.*securities' --type=ruby db/migrate/
  
  echo ""
  echo "=== Searching for any ticker mentions in migrations ==="
  rg -n 'ticker' --type=ruby db/migrate/ | head -20
  
  echo ""
  echo "=== Searching for unique constraints ==="
  rg -n 'unique' --type=ruby db/migrate/ | grep -i secur
  
  echo ""
  echo "=== Find the Security model definition ==="
  find . -name "*.rb" -path "*/models/*" | xargs grep -l "class Security" 2>/dev/null

  Repository: we-promise/sure

  Length of output: 4464

  ---

  🏁 Script executed:

  #!/bin/bash
  # Check the initial securities table creation
  echo "=== Initial securities table creation ==="
  cat db/migrate/20240710182529_create_securities.rb
  
  echo ""
  echo "=== Most recent security index migration ==="
  cat db/migrate/20250522201031_stronger_unique_index_on_security.rb
  
  echo ""
  echo "=== Security model validations ==="
  cat app/models/security.rb | head -50

  Repository: we-promise/sure

  Length of output: 2538

  ---

  🏁 Script executed:

  #!/bin/bash
  # Check the resolve_security method implementation
  echo "=== resolve_security method in data_helpers.rb ==="
  cat -n app/models/indexa_capital_account/data_helpers.rb | sed -n '62,92p'
  
  echo ""
  echo "=== Check extract_exchange method ==="
  rg -A 5 "def extract_exchange" app/models/indexa_capital_account/data_helpers.rb
  
  echo ""
  echo "=== Check how resolve_security is called ==="
  rg -B 2 -A 2 "resolve_security" app/models/indexa_capital_account/data_helpers.rb | head -30

  Repository: we-promise/sure

  Length of output: 2130

  ---

  Fix find_by to include exchange when resolving securities.

  resolve_security calls Security.find_by(ticker: ticker) without considering exchange_operating_mic, but the database allows multiple securities with the same ticker (one per exchange). The unique constraint is composite: [ticker, exchange_operating_mic]. This causes the code to return an arbitrary security when multiple exist with the same ticker, risking lookup of the wrong security.

  Update the lookup to:

  • Include exchange_operating_mic in both find_by calls (lines 66 and 91)
  • Extract the exchange earlier and use it consistently throughout the method
  • Or add a unique index on ticker alone if single-ticker uniqueness is the intended design
  🤖 Prompt for AI Agents

  In `@app/models/indexa_capital_account/data_helpers.rb` around lines 62 - 92, The
  resolve_security method currently calls Security.find_by(ticker: ticker) which
  can return the wrong row when the same ticker exists on multiple exchanges;
  extract the exchange via extract_exchange(symbol_data) at the start (e.g.
  exchange = extract_exchange(symbol_data)), then change both Security.find_by
  calls to include exchange_operating_mic: exchange (i.e. Security.find_by(ticker:
  ticker, exchange_operating_mic: exchange)), use that same exchange variable when
  creating the new Security (exchange_mic: exchange), and also use it in the
  rescue fallback (Security.find_by(ticker: ticker, exchange_operating_mic:
  exchange)) to ensure lookups and creation are consistent by exchange.
  

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🔴 Critical

  Full comment

  _⚠️ Potential issue_ | _🔴 Critical_
  
  **`build_provider` ignores API-token authentication — inconsistent with `Provided` concern.**
  
  `Provided#indexa_capital_provider` (in `app/models/indexa_capital_item/provided.rb`) implements a token-priority strategy: stored `api_token` → ENV `INDEXA_API_TOKEN` → username/document/password. However, `build_provider` here only queries for username/document/password items and always constructs the provider with those credentials, completely bypassing token auth.
  
  If a user configures only an `api_token`, this method returns `nil` because the query (Line 49) filters them out and the provider is never built with a token.
  
  Additionally, the query `.where.not(username: nil, document: nil, password: nil)` generates `WHERE NOT (username IS NULL AND document IS NULL AND password IS NULL)`, which matches items where *at least one* of the three fields is non-nil — not items where *all three* are non-nil. This could return partially-configured items.
  
  <details>
  <summary>Suggested fix — align with Provided concern's token-priority logic</summary>
  
  ```diff
     def self.build_provider(family: nil)
       return nil unless family.present?
   
  -    # Get family-specific credentials
  -    indexa_capital_item = family.indexa_capital_items.where.not(username: nil, document: nil, password: nil).first
  +    indexa_capital_item = family.indexa_capital_items.first
       return nil unless indexa_capital_item&.credentials_configured?
   
  -    Provider::IndexaCapital.new(
  -      username: indexa_capital_item.username,
  -      document: indexa_capital_item.document,
  -      password: indexa_capital_item.password
  -    )
  +    indexa_capital_item.indexa_capital_provider
     end
  

  🤖 Prompt for AI Agents

  In `@app/models/provider/indexa_capital_adapter.rb` around lines 45 - 57,
  build_provider currently ignores api_token and uses a faulty .where.not(...)
  that matches items with any one field present; update it to follow the
  token-first logic used by Provided#indexa_capital_provider: first look for an
  indexa_capital_item with a non-nil api_token (or use ENV['INDEXA_API_TOKEN'] if
  no item token), and if present instantiate Provider::IndexaCapital with that
  token, otherwise find an item where username, document and password are all
  present (use explicit checks such as chaining .where.not(username:
  nil).where.not(document: nil).where.not(password: nil) or equivalent) and
  instantiate Provider::IndexaCapital with username/document/password; ensure
  indexa_capital_item variable names (indexa_capital_item) and
  Provider::IndexaCapital constructor are used consistently.
  

  ✅ Addressed in commit 98f1898

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟠 Major

  Full comment

  _⚠️ Potential issue_ | _🟠 Major_
  
  **Path traversal risk: `account_number` is interpolated directly into the URL without sanitization.**
  
  On Lines 52 and 63, `account_number` is embedded in the URL path without validation. If `account_number` contains characters like `../` or URL-encoded path separators, it could alter the target endpoint.
  
  
  <details>
  <summary>Proposed fix — validate account_number format</summary>
  
  ```diff
  +    def sanitize_account_number!(account_number)
  +      unless account_number.match?(/\A[A-Za-z0-9\-_]+\z/)
  +        raise Error.new("Invalid account number format: #{account_number}", :bad_request)
  +      end
  +      account_number
  +    end
  +
       def get_holdings(account_number:)
         with_retries("get_holdings") do
  +        sanitize_account_number!(account_number)
           response = self.class.get(
             "#{base_url}/accounts/#{account_number}/fiscal-results",
  

  📝 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 get_holdings(account_number:)
    with_retries("get_holdings") do
      validate_account_number!(account_number)
      response = self.class.get(
        "#{base_url}/accounts/#{account_number}/fiscal-results",
        headers: auth_headers
      )
      handle_response(response)
    end
  end
  
  private
  
  def validate_account_number!(account_number)
    raise ProviderError.new("Invalid account number format") unless account_number.match?(/\A[A-Za-z0-9\-_]+\z/)
  end
  

  🤖 Prompt for AI Agents

  In `@app/models/provider/indexa_capital.rb` around lines 49 - 57, The
  account_number is interpolated directly into the URL in get_holdings (and
  similar methods) which allows path traversal; validate and sanitize
  account_number before using it in the path by enforcing a strict allowlist
  pattern (e.g., only digits and allowed separators) or by rejecting any input
  containing "/", "\" or percent-encoded separators, and raise/return an error
  when validation fails; update get_holdings to call a new helper (e.g.,
  sanitize_account_number or validate_account_number) and use the
  sanitized/validated value in the URL construction and any other places that
  interpolate account_number.
  

  ✅ Addressed in commit 98f1898

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟠 Major

  Full comment

  _⚠️ Potential issue_ | _🟠 Major_
  
  **IndexaCapital section is rendered outside the `@encryption_error` guard.**
  
  All other provider sections (lines 23–74) are wrapped inside `<% unless `@encryption_error` %>`, but the new IndexaCapital block starts after the `<% end %>` on line 75. This means it will always render — even when encryption is broken and credentials cannot be decrypted.
  
  Move it inside the guard:
  
  
  <details>
  <summary>Proposed fix</summary>
  
  ```diff
       <%= settings_section title: "SnapTrade (beta)", collapsible: true, open: false, auto_open_param: "manage" do %>
         <turbo-frame id="snaptrade-providers-panel">
           <%= render "settings/providers/snaptrade_panel" %>
         </turbo-frame>
       <% end %>
  -  <% end %>
   
  -<%= settings_section title: "IndexaCapital", collapsible: true, open: false do %>
  -  <turbo-frame id="indexa_capital-providers-panel">
  -    <%= render "settings/providers/indexa_capital_panel" %>
  -  </turbo-frame>
  -<% end %>
  +    <%= settings_section title: "IndexaCapital", collapsible: true, open: false do %>
  +      <turbo-frame id="indexa_capital-providers-panel">
  +        <%= render "settings/providers/indexa_capital_panel" %>
  +      </turbo-frame>
  +    <% end %>
  +  <% end %>
   </div>
  

  🤖 Prompt for AI Agents

  In `@app/views/settings/providers/show.html.erb` around lines 76 - 81, The
  IndexaCapital provider block (settings_section titled "IndexaCapital" containing
  the turbo-frame id "indexa_capital-providers-panel" and rendering the partial
  "settings/providers/indexa_capital_panel") is placed outside the
  `@encryption_error` guard and thus always renders; move that entire block inside
  the existing <% unless `@encryption_error` %> ... <% end %> guard that wraps the
  other provider sections so the IndexaCapital section only renders when
  `@encryption_error` is false.
  

  ✅ Addressed in commit 98f1898

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟠 Major

  Full comment

  _⚠️ Potential issue_ | _🟠 Major_
  
  <details>
  <summary>🧩 Analysis chain</summary>
  
  🏁 Script executed:
  
  ```shell
  # Find IndexaCapitalItem model file
  fd -t f 'indexa_capital_item' --extension rb app/models/
  

  Repository: we-promise/sure

  Length of output: 92

  ---

  🏁 Script executed:

  # Check the migration file to see full context
  cat -n db/migrate/20260205110328_create_indexa_capital_items_and_accounts.rb | head -80

  Repository: we-promise/sure

  Length of output: 2935

  ---

  🏁 Script executed:

  # Check the IndexaCapitalItem model for encryption declarations
  cat -n app/models/indexa_capital_item.rb

  Repository: we-promise/sure

  Length of output: 7170

  ---

  Encrypt username and document credential fields alongside password.

  The password field is correctly encrypted at the model level (line 19 of app/models/indexa_capital_item.rb). However, username and document remain unencrypted despite being part of the credential set. Add encrypts :username, :document, deterministic: true to the encryption block (lines 18-21) so all credentials are protected at rest.

  🤖 Prompt for AI Agents

  In `@db/migrate/20260205110328_create_indexa_capital_items_and_accounts.rb` around
  lines 29 - 33, Model IndexaCapitalItem currently encrypts only :password; add
  :username and :document to the encryption block so credentials are all
  encrypted. Update the encrypts call in app/models/indexa_capital_item.rb to
  include username and document with deterministic: true (i.e., change encrypts
  :password, deterministic: true to encrypts :username, :document, :password,
  deterministic: true) so the DB columns (t.string :username, t.string :document,
  t.text :password) are protected at rest.
  

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟠 Major

  Full comment

  _⚠️ Potential issue_ | _🟠 Major_
  
  **All tests are skipped — no actual coverage for the Processor.**
  
  Every test in this file is `skip`-ped with commented-out bodies, providing zero coverage for `IndexaCapitalAccount::Processor`, `HoldingsProcessor`, and `ActivitiesProcessor`. Given this PR introduces real API integration with data processing, shipping without any active processor tests is a reliability risk.
  
  
  
  Would you like me to help wire up the fixtures and implement active tests for the processor, or open an issue to track this?
  
  <details>
  <summary>🤖 Prompt for AI Agents</summary>
  
  

  In @test/models/indexa_capital_account/processor_test.rb around lines 29 - 132,
  Unskip and activate the processor tests by wiring a real test fixture for
  IndexaCapitalAccount used by tests like "processor initializes with
  indexa_capital_account", "holdings processor creates holdings from raw payload",
  and "activities processor creates trades from raw payload": in the test setup
  create and assign @indexa_capital_account with a linked Account and
  AccountProvider (or remove the link for the "skips processing when no linked
  account" test), ensure @account exists, create any related Security records
  (e.g., ticker "AAPL") and set raw payloads on @indexa_capital_account
  (raw_holdings_payload, raw_activities_payload) for HoldingsProcessor and
  ActivitiesProcessor tests, then remove the skip calls and restore the commented
  assertions so the tests instantiate IndexaCapitalAccount::Processor,
  IndexaCapitalAccount::HoldingsProcessor, and
  IndexaCapitalAccount::ActivitiesProcessor, call process, and assert expected
  results (account balance, holding qty, created entry/trade, and no-op behaviors
  when symbol or external_id are blank).

  
  </details>
  
  <!-- fingerprinting:phantom:poseidon:phoenix -->
  
  <!-- This is an auto-generated comment by CodeRabbit -->
  
  ✅ Addressed in commit 98f1898
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟡 Minor

  Full comment

  _⚠️ Potential issue_ | _🟡 Minor_
  
  **Lint failure: `Layout/SpaceInsideArrayLiteralBrackets` on lines 27–28.**
  
  The CI lint check reports multiple `SpaceInsideArrayLiteralBrackets` violations. Add spaces inside the array brackets to satisfy the project's RuboCop config.
  
  Additionally, the multi-key `.where.not(username: …, document: …, password: …)` negates the *conjunction*, so it matches items where **at least one** of the three fields is non-blank — not items where **all three** are present. This could return `true` for partially-configured items.
  
  <details>
  <summary>Proposed fix</summary>
  
  ```diff
   def has_indexa_capital_credentials?
  -  indexa_capital_items.where.not(api_token: [nil, ""]).or(
  -    indexa_capital_items.where.not(username: [nil, ""], document: [nil, ""], password: [nil, ""])
  -  ).exists?
  +  indexa_capital_items.where.not(api_token: [ nil, "" ]).or(
  +    indexa_capital_items.where.not(username: [ nil, "" ])
  +                        .where.not(document: [ nil, "" ])
  +                        .where.not(password: [ nil, "" ])
  +  ).exists?
   end
  

  📝 Committable suggestion

  ‼️ IMPORTANT
  Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    def has_indexa_capital_credentials?
      indexa_capital_items.where.not(api_token: [ nil, "" ]).or(
        indexa_capital_items.where.not(username: [ nil, "" ])
                            .where.not(document: [ nil, "" ])
                            .where.not(password: [ nil, "" ])
      ).exists?
    end
  

  🧰 Tools

  🪛 GitHub Check: ci / lint

  [failure] 28-28:
  Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.

  ---

  [failure] 28-28:
  Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.

  ---

  [failure] 28-28:
  Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.

  ---

  [failure] 28-28:
  Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.

  ---

  [failure] 28-28:
  Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.

  ---

  [failure] 28-28:
  Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.

  ---

  [failure] 27-27:
  Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.

  ---

  [failure] 27-27:
  Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.

  🤖 Prompt for AI Agents

  In `@app/models/family/indexa_capital_connectable.rb` around lines 26 - 30, The
  method has_indexa_capital_credentials? must fix RuboCop spacing and ensure the
  multi-key negation requires all three fields present; change
  indexa_capital_items.where.not(api_token: [nil, ""]) to use spaces inside the
  array brackets (api_token: [ nil, "" ]) and replace the multi-key
  where.not(username: ..., document: ..., password: ...) with chained where.not
  calls so all three must be non-blank (e.g. .where.not(username: [ nil, ""
  ]).where.not(document: [ nil, "" ]).where.not(password: [ nil, "" ]) ), keeping
  the overall .or(...) and .exists? logic in has_indexa_capital_credentials?.
  

  ✅ Addressed in commit b69d66d

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟠 Major

  Full comment

  _⚠️ Potential issue_ | _🟠 Major_
  
  **Partial credentials silently accepted — will fail at `authenticate!` with a confusing 401.**
  
  `validate_configuration!` passes if *any* of username/document/password is present. For example, providing only `username` (without document and password) passes validation but will fail during `authenticate!` with an opaque API error. Consider requiring all three when token auth is not used.
  
  <details>
  <summary>Proposed fix</summary>
  
  ```diff
   def validate_configuration!
     return if `@api_token.present`?
   
  -  if `@username.blank`? && `@document.blank`? && `@password.blank`?
  +  if `@username.blank`? || `@document.blank`? || `@password.blank`?
       raise ConfigurationError, "Either API token or username/document/password credentials are required"
     end
   end
  

  🤖 Prompt for AI Agents

  In `@app/models/provider/indexa_capital.rb` around lines 97 - 103, The
  validate_configuration! method currently allows partial credential sets when
  `@api_token` is absent; change it to require all of `@username`, `@document` and
  `@password` to be present (not just any one) and raise ConfigurationError with a
  clear message if any are missing; update the validation branch that checks
  `@username/`@document/@password to assert all three are present and reference
  authenticate! failure context in the error text so callers know to provide
  either a complete username/document/password trio or an API token.
  

  ✅ Addressed in commit b69d66d

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟡 Minor

  Full comment

  _⚠️ Potential issue_ | _🟡 Minor_
  
  **No-op assertion: `count >= 0` always passes.**
  
  `@account.holdings.count >= 0` can never be false, so this test proves nothing about whether the processor actually created holdings. If securities may not exist, assert on the expected side-effect (e.g., that `Security.find_by(isin: ...)` was attempted, or stub it) — otherwise this test gives false confidence.
  
  <details>
  <summary>Possible improvement</summary>
  
  ```diff
  -    # Should have created a holding via the account
  -    assert `@account.holdings.count` >= 0 # Holdings may need securities to exist first
  +    # Processor should attempt to create holdings; at minimum verify no errors
  +    # and that the holding count increased or a security lookup was attempted.
  +    assert_operator `@account.holdings.count`, :>=, 1, "Expected at least one holding to be created"
  

  If the security doesn't exist yet and that's expected, mock the security lookup or create the security in setup so the assertion is meaningful.

  📝 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.

      # Processor should attempt to create holdings; at minimum verify no errors
      # and that the holding count increased or a security lookup was attempted.
      assert_operator `@account.holdings.count`, :>=, 1, "Expected at least one holding to be created"
    end
  

  🤖 Prompt for AI Agents

  In `@test/models/indexa_capital_account/processor_test.rb` around lines 86 - 88,
  The assertion `@account.holdings.count >= 0` is a no-op; replace it with a
  meaningful check such as asserting the holding count changed or a specific
  holding exists after running the processor: capture the initial count (e.g.,
  before_count = `@account.holdings.count`) then assert `@account.holdings.count` ==
  before_count + 1 or assert `@account.holdings.exists`?(expected_attributes) to
  verify the processor created a holding; alternatively, if missing securities are
  expected, create the required Security in test setup or stub Security.find_by
  (or mock the repository method used by the processor) so the test can assert the
  intended side-effect of the processor.
  

  ✅ Addressed in commit b69d66d

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟡 Minor

  Full comment

  _⚠️ Potential issue_ | _🟡 Minor_
  
  **Unhandled `JSON::ParserError` on malformed 2xx responses.**
  
  If the API returns a 200/201 with a non-JSON body (e.g., HTML error page from a proxy), `JSON.parse` raises `JSON::ParserError`, which isn't in `RETRYABLE_ERRORS` and won't produce a meaningful error message. Wrapping this in a rescue improves debuggability.
  
  
  <details>
  <summary>Proposed fix</summary>
  
  ```diff
        when 200, 201
  -       JSON.parse(response.body, symbolize_names: true)
  +       begin
  +         JSON.parse(response.body, symbolize_names: true)
  +       rescue JSON::ParserError => e
  +         raise Error.new("Invalid JSON in response: #{e.message}", :bad_response)
  +       end
  

  🤖 Prompt for AI Agents

  In `@app/models/provider/indexa_capital.rb` around lines 177 - 181, In
  handle_response, guard against JSON::ParserError when parsing 200/201 responses:
  wrap the JSON.parse(response.body, symbolize_names: true) call in a begin/rescue
  that catches JSON::ParserError and raises or re-raises a clearer error (or adds
  it to the same error flow used by RETRYABLE_ERRORS) including the response.code
  and a snippet of the response.body for debugging; reference the handle_response
  method, the 200/201 branch, JSON::ParserError, and RETRYABLE_ERRORS so the fix
  is applied to the parsing path for successful HTTP responses.
  

  ✅ Addressed in commit 5c91774

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟡 Minor

  Full comment

  _⚠️ Potential issue_ | _🟡 Minor_
  
  **Weak assertion: balance is already > 0 before processing.**
  
  `@account` is created with `balance: 10000` in setup, so `assert `@account.balance` > 0` passes even if the processor is a no-op. Assert on the expected value or at least that it changed:
  
  <details>
  <summary>Suggested fix</summary>
  
  ```diff
       processor = IndexaCapitalAccount::Processor.new(`@indexa_capital_account`)
       processor.process
   
       `@account.reload`
  -    assert `@account.balance` > 0
  +    assert_equal 38905.21, `@account.balance.to_f`
  

  Alternatively, if the exact value isn't stable, set balance: 0 in setup for this test and assert it changed:

  +    `@account.update`!(balance: 0)
       processor = IndexaCapitalAccount::Processor.new(`@indexa_capital_account`)
       processor.process
   
       `@account.reload`
  -    assert `@account.balance` > 0
  +    assert_in_delta 38905.21, `@account.balance.to_f`, 0.01

  🤖 Prompt for AI Agents

  In `@test/models/indexa_capital_account/processor_test.rb` around lines 57 - 61,
  The test uses IndexaCapitalAccount::Processor.new(...).process but asserts
  `@account.balance` > 0 even though setup creates `@account` with balance: 10000, so
  the assertion is meaningless; fix by making the assertion meaningful: either (A)
  set the account's starting balance to 0 for this test (e.g., adjust the factory
  or set `@account.balance` = 0 and save before calling processor.process) then call
  processor.process, `@account.reload` and assert the balance changed (or equals the
  expected value), or (B) assert the concrete expected balance after processing
  (use processor.process and `@account.reload` then assert_equal expected_balance,
  `@account.balance`); ensure you reference IndexaCapitalAccount::Processor.process
  and `@account.reload` in the change.
  

  ✅ Addressed in commit 5c91774

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟡 Minor

  Full comment

  _⚠️ Potential issue_ | _🟡 Minor_
  
  **Memoized token is never invalidated — stale tokens cause persistent 401 failures.**
  
  If the cached `@token` expires during the lifetime of this object, all subsequent API calls will fail with `AuthenticationError` without attempting re-authentication. Consider clearing `@token` on 401 and retrying once with a fresh token.
  
  <details>
  <summary>Proposed fix — re-authenticate on 401</summary>
  
  ```diff
       def token
  -      `@token` ||= token_auth? ? `@api_token` : authenticate!
  +      `@token` ||= resolve_token
  +    end
  +
  +    def resolve_token
  +      token_auth? ? `@api_token` : authenticate!
  +    end
  +
  +    def invalidate_token!
  +      `@token` = nil unless token_auth?
       end
  

  Then in handle_response, before raising on 401, call invalidate_token! so the next retry can re-authenticate. Alternatively, catch AuthenticationError in with_retries for one re-auth attempt.

  🤖 Prompt for AI Agents

  In `@app/models/provider/indexa_capital.rb` around lines 156 - 158, The memoized
  `@token` returned by token (def token; `@token` ||= token_auth? ? `@api_token` :
  authenticate! end) is never invalidated causing persistent 401s; update error
  handling so that when handle_response detects a 401 (or is about to raise
  AuthenticationError) it calls an invalidate_token! method (or clear `@token`) and
  then retries the request once (e.g., integrate retry logic in with_retries to
  catch AuthenticationError and attempt one re-auth by calling authenticate! via
  token, or have handle_response trigger invalidate_token! before raising so a
  surrounding retry will refresh the token). Ensure you reference and update
  token, authenticate!, invalidate_token!, handle_response, and with_retries to
  perform a single re-authentication attempt on 401 and avoid infinite retry
  loops.
  

  </details>
  

[jjmata]

• coderabbitai[bot]: ⚠️ Potential issue | 🟠 Major

  Full comment

  _⚠️ Potential issue_ | _🟠 Major_
  
  **`nil.to_d` will raise `NoMethodError` if `total_amount` is missing from the API response.**
  
  On Line 235, if the latest portfolio entry lacks a `total_amount` key, `latest[:total_amount]` returns `nil`, and `NilClass` does not define `to_d` — this will crash.
  
  Additionally on Line 234, the bare `rescue` catches *all* exceptions (including `NoMethodError`, `TypeError`, etc.), which masks real bugs. Prefer rescuing only `ArgumentError` or `Date::Error`.
  
  <details>
  <summary>Proposed fix</summary>
  
  ```diff
       def extract_balance(performance_data)
         portfolios = performance_data[:portfolios]
         return 0 unless portfolios.is_a?(Array) && portfolios.any?
  
  -      latest = portfolios.max_by { |p| Date.parse(p[:date].to_s) rescue Date.new }
  -      latest[:total_amount].to_d
  +      latest = portfolios.max_by { |p| begin; Date.parse(p[:date].to_s); rescue ArgumentError; Date.new; end }
  +      latest[:total_amount]&.to_d || 0
       end
  

  🤖 Prompt for AI Agents

  In `@app/models/provider/indexa_capital.rb` around lines 229 - 236, In
  extract_balance(performance_data) the call latest[:total_amount].to_d will raise
  if total_amount is nil and the bare rescue around Date.parse masks unrelated
  errors; update the method to only rescue parsing errors (e.g.,
  ArgumentError/Date::Error) when computing latest via Date.parse (use a safe
  parsed_date variable), and when reading latest[:total_amount] guard against nil
  before calling to_d (e.g., return 0 or BigDecimal("0") if latest or
  latest[:total_amount] is nil or not numeric); reference symbols:
  extract_balance, portfolios, latest, Date.parse, and to_d.
  

  </details>
  
  

• coderabbitai[bot]: ⚠️ Potential issue | 🟡 Minor

  Full comment

  _⚠️ Potential issue_ | _🟡 Minor_
  
  <details>
  <summary>🧩 Analysis chain</summary>
  
  🏁 Script executed:
  
  ```shell
  #!/bin/bash
  # Find IndexaCapitalItem model
  fd -t f -i 'indexa_capital_item' | grep -E '\.rb$'
  
  # Check for active scope in all item models
  rg -n 'scope.*:active' --type=rb -B2 -A2 | grep -E '(app/models|scope.*active)' | head -30
  
  # Check Family model for relationships
  rg -n 'has_many.*items' --type=rb | head -20
  

  Repository: we-promise/sure

  Length of output: 448

  ---

  🏁 Script executed:

  #!/bin/bash
  # Get the IndexaCapitalItem model file path and content
  fd -t f -i 'indexa_capital_item' --type=rb
  
  # Also search for IndexaCapital references in models
  rg -l 'IndexaCapital' --type=rb app/models/

  Repository: we-promise/sure

  Length of output: 255

  ---

  🏁 Script executed:

  #!/bin/bash
  # Read IndexaCapitalItem model
  cat -n app/models/indexa_capital_item.rb
  
  # Read PlaidItem model for comparison
  cat -n app/models/plaid_item.rb
  
  # Check family syncer file context
  cat -n app/models/family/syncer.rb | head -40

  Repository: we-promise/sure

  Length of output: 14108

  ---

  Apply .active scope filter to avoid syncing disconnected items.

  IndexaCapitalItem defines an .active scope (where(scheduled_for_deletion: false)) but doesn't use it in child_syncables. This is inconsistent with SimpleFin, Lunchflow, and EnableBanking which all apply .active filtering. Update line 29:

  family.indexa_capital_items.active
  

  Note: PlaidItem has the same .active scope defined but also lacks the filter in the syncer—this may warrant a separate fix for consistency.

  🤖 Prompt for AI Agents

  In `@app/models/family/syncer.rb` around lines 28 - 30, In child_syncables (method
  child_syncables in app/models/family/syncer.rb) add the .active scope to
  family.indexa_capital_items so it reads family.indexa_capital_items.active to
  match the other item collections (family.simplefin_items.active,
  family.lunchflow_items.active, family.enable_banking_items.active); leave
  family.plaid_items and family.accounts.manual unchanged (note: PlaidItem also
  defines .active but addressing that is outside this change).
  

  </details>
  
  

Issue comments

• coderabbitai[bot]:

  Full comment

  <!-- This is an auto-generated comment: summarize by coderabbit.ai -->
  <!-- walkthrough_start -->
  
  <details>
  <summary>📝 Walkthrough</summary>
  
  ## Walkthrough
  
  Adds a full Indexa Capital integration: new provider client and adapter, DB tables/models for items and accounts, import/sync processors and jobs, controllers/routes/views for multi-step linking and setup, enum/i18n additions, and extensive tests/fixtures.
  
  ## Changes
  
  |Cohort / File(s)|Summary|
  |---|---|
  |**API client & Adapter** <br> `app/models/provider/indexa_capital.rb`, `app/models/provider/indexa_capital_adapter.rb`|New HTTParty-based provider client with token/credentials auth, retries/error mapping, account/holdings/balance endpoints; adapter registers provider, exposes connection configs, routing, and institution metadata.|
  |**Core models & concerns** <br> `app/models/indexa_capital_item.rb`, `app/models/indexa_capital_item/...`, `app/models/indexa_capital_account.rb`, `app/models/family/indexa_capital_connectable.rb`, `app/models/family.rb`|New ActiveRecord models and concerns (Provided, Importer, Syncer, Unlinking); Family mixin added; credential handling (api_token or username/document/password), associations, sync/status APIs, unlink logic, and lifecycle hooks.|
  |**Import / Processing** <br> `app/models/indexa_capital_item/importer.rb`, `app/models/indexa_capital_account/processor.rb`, `app/models/indexa_capital_account/{holdings_processor,activities_processor,data_helpers}.rb`|Importer and processors to fetch/normalize accounts, holdings, and activities; security resolution, data helpers, import adapters, per-account stats, and robust per-item/per-account error handling.|
  |**Controllers, Routes & UI** <br> `app/controllers/indexa_capital_items_controller.rb`, `app/controllers/accounts_controller.rb`, `app/controllers/settings/providers_controller.rb`, `config/routes.rb`, `app/views/indexa_capital_items/*`, `app/views/settings/providers/*`, `app/views/accounts/index.html.erb`|New controller with multi-step linking/setup flows and sync actions; Accounts/Settings controllers updated to surface items; routes added; multiple views/partials for selection, linking, setup, and provider settings.|
  |**Background jobs** <br> `app/jobs/indexa_capital_activities_fetch_job.rb`, `app/jobs/indexa_capital_connection_cleanup_job.rb`|Jobs for activities fetch (pending flag management, broadcasts) and connection cleanup (guard checks and placeholder API deletion logic).|
  |**DB & Schema** <br> `db/migrate/...create_indexa_capital_items_and_accounts.rb`, `db/migrate/...add_api_token_to_indexa_capital_items.rb`, `db/schema.rb`|Migrations add `indexa_capital_items` and `indexa_capital_accounts` with credential fields (including `api_token`), payloads, sync metadata, indexes, and foreign keys.|
  |**Enums & small model updates** <br> `app/models/data_enrichment.rb`, `app/models/provider_merchant.rb`|Added `indexa_capital` enum value to relevant enums.|
  |**Views / I18n** <br> `app/views/indexa_capital_items/*`, `app/views/settings/providers/*`, `config/locales/views/indexa_capital_items/en.yml`|New partials and pages for provider panel, item panels, selection and setup flows; full English translations added.|
  |**Tests & fixtures** <br> `test/**/*indexa_capital*`, `test/fixtures/indexa_capital_*`|Extensive unit and integration tests plus fixtures covering provider client, models, processors, controllers, and data helpers.|
  |**Small controller tweak** <br> `app/controllers/accounts_controller.rb`|`AccountsController#index` now loads `@indexa_capital_items` and builds per-item sync-stats map analogous to other providers.|
  
  ## Sequence Diagram(s)
  
  ```mermaid
  sequenceDiagram
      participant User
      participant Controller as IndexaCapitalItemsController
      participant Item as IndexaCapitalItem
      participant Syncer as IndexaCapitalItem::Syncer
      participant Importer as IndexaCapitalItem::Importer
      participant Provider as Provider::IndexaCapital
      participant Account as IndexaCapitalAccount
  
      User->>Controller: POST create (credentials)
      Controller->>Item: create! (name, credentials)
      Controller->>Item: enqueue sync (sync_later)
      Syncer->>Importer: import(sync)
      Importer->>Provider: list_accounts
      Provider-->>Importer: accounts data
      Importer->>Account: upsert_from_indexa_capital! (per account)
      Importer->>Provider: get_holdings(account_number)
      Provider-->>Importer: holdings payload
      Importer->>Account: upsert_holdings_snapshot!
      Syncer->>Item: schedule_account_syncs / perform_post_sync
      Syncer-->>Controller: broadcast UI update
  

  sequenceDiagram
      participant User
      participant Controller as IndexaCapitalItemsController
      participant Item as IndexaCapitalItem
      participant Account as IndexaCapitalAccount
      participant Processor as IndexaCapitalAccount::Processor
      participant Holdings as HoldingsProcessor
      participant Activities as ActivitiesProcessor
      participant ImportAdapter as Account::ProviderImportAdapter
  
      User->>Controller: POST setup_accounts (select/link)
      Controller->>Item: process_accounts(params)
      Item->>Account: iterate selected indexa_capital_accounts
      Account->>Processor: Processor.new(account).process
      Processor->>Holdings: HoldingsProcessor.process (if holdings)
      Holdings->>ImportAdapter: import holding
      Processor->>Activities: ActivitiesProcessor.process (if activities)
      Activities->>ImportAdapter: import trades/transactions
      Processor-->>Item: per-account stats
      Item-->>Controller: aggregate results
      Controller->>User: redirect with status/flash
  

  Loading

  Estimated code review effort

  🎯 4 (Complex) | ⏱️ ~60 minutes

  Possibly related PRs

  • Providers factory #250 — provider factory / account_provider wiring that this adapter and models integrate with.
  • Add Coinbase exchange integration with CDP API support #704 — related changes to AccountsController#index adding per-provider items and sync-stats maps.
  • Mercury integration #723 — another provider integration touching similar controllers, views, and family connectable hooks.

  Suggested labels

  enhancement

  Suggested reviewers

  • jjmata

  Poem

  🐰 I hopped through code with curious cheer,
  New Indexa items now appear so near.
  Holdings stitched and syncs set right,
  Accounts linked tidy, morning and night.
  Hop, hop — a rabbit's little byte of delight. 🎉

  🚥 Pre-merge checks | ✅ 2 | ❌ 1

  ❌ Failed checks (1 warning)

  Check name	Status	Explanation	Resolution
  Docstring Coverage	⚠️ Warning	Docstring coverage is 21.74% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

  ✅ Passed checks (2 passed)

  Check name	Status	Explanation
  Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
  Title check	✅ Passed	The title 'Implement Indexa Capital provider with real API integration' directly and accurately describes the main objective of the changeset—implementing a complete Indexa Capital provider with actual API integration rather than a broken scaffold.

  _{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

  ✨ Finishing touches

  • 📝 Generate docstrings

  🧪 Generate unit tests (beta)

  • Create PR with unit tests
  • Post copyable unit tests in a comment

  ---

  Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

  ❤️ Share

  • X
  • Mastodon
  • Reddit
  • LinkedIn

  _{Comment @coderabbitai help to get the list of available commands and usage tips.}

  </details>
  
  

• dgilperez: Note: Indexa Capital was not included in Family::Syncer#child_syncables, which meant balances and holdings would only refresh on manual sync — not duri...

  Full comment

  **Note:** Indexa Capital was not included in `Family::Syncer#child_syncables`, which meant balances and holdings would only refresh on manual sync — not during daily auto-sync or login sync. Fixed in latest commit.
  
  Worth noting: other newer providers (CoinStats, Coinbase, Mercury, SnapTrade) are also absent from `child_syncables` and have the same issue. That's outside the scope of this PR but should probably be addressed separately.