feat(sync): add Brex provider connections#1672
feat(sync): add Brex provider connections#1672JSONbored wants to merge 22 commits intowe-promise:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds first-class Brex provider support: DB tables and models (BrexItem, BrexAccount), provider client and adapter, import/sync/processing flows, controllers and views for account linking/setup, encryption readiness centralization, Family integration, tests, and re-enables tracking of ChangesBrex Provider Integration
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Browser
participant C as BrexItemsController / AccountFlowsController
participant AF as BrexItem::AccountFlow
participant P as Provider::Brex
participant DB as Database
Browser->>C: GET /brex_items/:id/select_accounts
C->>AF: preload_payload()
AF->>P: get_accounts()
P-->>AF: accounts payload
AF-->>C: JSON payload
C-->>Browser: render modal with accounts
Browser->>C: POST /brex_items/:id/link_accounts
C->>AF: link_new_accounts_result(params)
AF->>DB: upsert BrexAccount records
AF->>DB: create internal Accounts & AccountProviders
AF-->>C: navigation result
C-->>Browser: redirect to accounts
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
JSONbored
force-pushed
the
codex/feat-brex-provider
branch
from
5e9d426 to
dfc4ad6
Compare
sure-design
left a comment
sure-design
left a comment
There was a problem hiding this comment.
Early review notes for when this comes out of draft:
1. Fat controller (critical convention violation)
brex_items_controller.rb is 891 lines. The project convention is explicitly "Skinny Controllers, Fat Models." API fetching, cache management, account linking, flow orchestration — all of this belongs in model-layer objects (BrexItem, BrexItem::Importer, etc.), not the controller. Compare to Mercury: the controller there delegates to model concerns. This controller needs a significant extraction pass before it can land.
2. Conditional token encryption
if encryption_ready?
encrypts :token, deterministic: true
endIn a default self-hosted install without ACTIVE_RECORD_ENCRYPTION_* env vars, the API token is stored in plaintext. Other providers (Mercury, Coinbase) handle this the same way, but it's worth calling out explicitly here since Brex tokens are long-lived service credentials with broad account access. Consider at minimum a clear warning in the UI/docs when encryption is not configured.
3. Duplicated brex_account_name logic
The name-resolution logic appears three times: in BrexItemsController (private brex_account_name), in BrexItemsHelper (private brex_account_display_name), and in BrexAccount (private brex_account_name). One canonical place — the model — is the right home for this.
4. config/locales/common/en.yml is a new load path
This introduces a common/ subdirectory under config/locales/ that doesn't exist elsewhere. The single key it adds (en.common.close: Close) should go into the main en.yml or an existing shared locale file rather than creating a new directory.
5. begin/rescue wrapping entire action bodies is unnecessary
preload_accounts, select_accounts, etc. wrap their entire body in begin … rescue. Since the rescue is already at the top level of the method, the begin keyword is redundant and adds visual noise.
The security hardening (URL allowlist, safe_return_to_path, payload sanitisation, pagination cap) is solid. The PR description and test coverage are thorough. Main blocker before it leaves draft is the controller weight.
Generated by Claude Code
|
Thanks for implementing this. A few things worth flagging before this moves out of draft: Scope and maintainer buy-in: At 4,631 additions across 48 files, this is the largest open PR in the repo. Adding a new financial data provider (with API token storage, sync processors, account linking, and UI flows) is a significant architectural commitment. It would be worth confirming with @jjmata whether Brex is on the roadmap before investing further in review cycles. Relationship to existing providers: The PR mirrors Mercury's multi-connection pattern, which is the right approach. However, since this is an unsolicited provider addition from a first-time contributor, maintainers will want to verify that the credential handling (blank-token preservation, whitespace stripping, URL allowlist) matches the security bar set by existing providers. Trust flag: Same as your other PRs — automated trust scanner shows score 68/100. Maintainers will scrutinize credential storage and API token handling closely. Suggestion: Get explicit confirmation from maintainers that Brex integration is wanted before continuing to iterate on this PR. If it is, consider a slimmer initial PR scoped to just the provider client + importer (no UI), to make review tractable. Generated by Claude Code Generated by Claude Code |
|
Good to see Brex following the same patterns as Plaid/SimpleFIN. A few things:
Several flows (preload/select/link/setup) contain logic that belongs in
if encryption_ready?
encrypts :token, deterministic: true
endThis runs when the class is first loaded. If SSRF protection in The
Unrelated to Brex — if this is a leftover from development tooling it should either be in the global Generated by Claude Code |
Adds Brex item and account tables with per-family credentials, scoped upstream account uniqueness, encrypted token storage, and sanitized provider payload columns.
Adds Brex item/account models, provider client and adapter support, family connection helpers, and provider enum registration for read-only Brex cash and card data.
Adds Brex account discovery, linked-account sync, cash/card balance processors, transaction import, sanitized metadata handling, and idempotent provider entry processing.
Adds Mercury-style Brex connection management, explicit item-scoped account selection and linking, settings provider UI, account index visibility, localized copy, and per-item cache handling.
Adds targeted coverage for Brex provider requests, adapter config, item/account guards, importer behavior, entry processing, and Mercury-style controller flows.
Tightens Brex account fetching against the official card-account response shape, sends transaction start filters as RFC3339 date-times, and keeps provider error bodies out of user-facing messages while expanding provider client guard coverage.
Restrict Brex API base URLs to official hosts, tighten account-selection UI behavior, and add tests for invalid credentials, cache scoping, and provider setup edge cases.
JSONbored
force-pushed
the
codex/feat-brex-provider
branch
from
04a9e9a to
c7ebd6d
Compare
Move remaining Brex review cleanup into focused model behavior, tighten link/setup edge cases, localize summaries, and add regression coverage from CodeRabbit feedback. Also records the security-review pass as no-findings after diff-scoped inspection and Brakeman validation.
Route Brex account selection and setup actions through small namespaced controllers while keeping existing URLs and helpers stable. Business flow remains in BrexItem::AccountFlow; the main Brex item controller now only handles connection CRUD, provider-panel rendering, destroy, and sync.
brin-security-scanner
Bot
added
contributor:flagged
![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: 40f924a6cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| enum :status, { good: "good", requires_update: "requires_update" }, default: :good | ||
|
|
||
| encrypts :token, deterministic: true |
There was a problem hiding this comment.
Make token encryption conditional here, otherwise Brex connections break in environments where Active Record encryption is intentionally not configured. This model calls encrypts :token unconditionally, but the rest of the codebase (and Encryptable comments/UI warning) supports a plaintext fallback when keys are absent; in that scenario creating/updating a BrexItem will raise encryption configuration errors instead of saving. Align this with other provider models by applying if encryption_ready? around the encrypts declaration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don’t think we should make Brex conditional here. This was intentionally hardened differently from Mercury because Brex tokens are long-lived service credentials. The app now treats Brex token storage as fail-closed: self-hosted installs get runtime AR encryption keys from SECRET_KEY_BASE, partial env config raises at boot, and BrexItem.encryption_ready? checks runtime config as well as explicit keys.
Making this conditional would reintroduce plaintext Brex token storage, which this PR intentionally avoids. If maintainers prefer all providers to retain plaintext fallback for consistency, I can make that tradeoff explicitly, but my recommendation is to keep Brex fail-closed and add a clarifying comment/test if needed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (19)
test/models/brex_entry/processor_test.rb (1)
25-34: ⚡ Quick winAssert the merchant payload is actually sanitized.
This fixture already includes
card_metadata.pan, but the test never proves that the savedextra["brex"]["merchant"]excludes it. Given the security-sensitive sanitization work in this PR, I'd lock that in here.🧪 Suggested assertion
assert_equal BigDecimal("12.34"), entry.amount assert_equal "USD", entry.currency assert_equal "brex", entry.source assert_equal Date.new(2026, 1, 2), entry.date assert_equal "STAPLES", entry.transaction.merchant.name assert_equal "card_1", entry.transaction.extra.dig("brex", "card_id") + refute_includes entry.transaction.extra.dig("brex", "merchant").to_s, "test-pan-placeholder"🤖 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/brex_entry/processor_test.rb` around lines 25 - 34, The test "imports card purchase with Brex signed amount preserved" doesn’t assert that merchant data in the saved payload is sanitized; update this test (involving BrexEntry::Processor and the resulting entry variable) to explicitly check entry.transaction.extra.dig("brex","merchant") does not include card_metadata.pan (or any PAN field) and that expected merchant fields remain present, ensuring the saved extra["brex"]["merchant"] has sensitive PAN removed while other merchant attributes are preserved.app/models/brex_entry/processor.rb (1)
112-116: ⚡ Quick winAdd provider-specific invalid-currency logging.
parse_currencycan reject provider data here, but this processor never overrideslog_invalid_currency, so those warnings won't include the Brex transaction/account context that the other provider processors log.Based on learnings, standardize invalid-currency logging by overriding `log_invalid_currency` in each provider processor with provider-specific context.🪵 Suggested change
def currency parse_currency(BrexAccount.currency_code_from_money(data[:amount])) || parse_currency(brex_account.currency) || "USD" end + + def log_invalid_currency(currency_code) + Rails.logger.warn( + "BrexEntry::Processor - Invalid currency '#{currency_code}' for transaction #{data[:id]} on brex_account #{brex_account.id}" + ) + 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/brex_entry/processor.rb` around lines 112 - 116, The currency resolution in BrexEntry::Processor (method currency) can trigger parse_currency rejections but this class never overrides log_invalid_currency, so those warnings lack Brex-specific context; add an override of log_invalid_currency in the BrexEntry::Processor class that formats and emits provider-specific details (e.g., include data[:id], data[:amount], BrexAccount.currency_from_money or brex_account.id/account identifier) and then calls or delegates to the shared logger behavior so invalid-currency warnings include Brex transaction/account context; reference the currency method, parse_currency call, BrexAccount.currency_code_from_money, and brex_account.currency when building the message.app/views/brex_items/_brex_item.html.erb (1)
87-102: ⚡ Quick winPrecompute sync/account data before rendering this partial.
This block does query/fallback work in ERB (
syncs.ordered.first, repeated association-backed checks/counts), which makes the partial responsible for data access and can add extra queries per Brex item on the accounts index. Pass the prepared stats/counts in, or wrap the lookup in a helper/component method so the template only renders values.As per coding guidelines "Avoid heavy logic in ERB views; prefer helpers and components" and "Keep domain logic out of views: compute values like button classes, conditional logic, and data transformations in the component file, not the template file".
🤖 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/brex_items/_brex_item.html.erb` around lines 87 - 102, The partial is performing data lookups (brex_item.syncs.ordered.first&.sync_stats and association-backed counts) which may trigger extra queries; move this logic out of the ERB by precomputing and passing in the values (stats, unlinked_count, linked_count, total_count, institutions_count) from the controller/presenter or by adding a helper method (e.g., fetch_brex_stats(brex_item, brex_sync_stats_map) or BrexItemPresenter methods) and update the call site that renders this partial to provide those locals instead of computing them inside the template; ensure ProviderSyncSummary still receives the stats param and replace uses of `@brex_sync_stats_map` and brex_item.*_accounts_count in the ERB with the passed-in locals.app/views/brex_items/select_accounts.html.erb (1)
11-12: ⚡ Quick winReplace raw
<form>withform_withto avoid manual CSRF token management.The manual
hidden_field_tag :authenticity_token, form_authenticity_tokenworks today, but it's easy to accidentally drop in a refactor.form_withhandles the CSRF token automatically and is idiomatic Rails.♻️ Proposed refactor
- <form action="<%= link_accounts_brex_items_path %>" method="post" class="space-y-4" data-turbo-frame="_top"> - <%= hidden_field_tag :authenticity_token, form_authenticity_token %> + <%= form_with url: link_accounts_brex_items_path, method: :post, class: "space-y-4", data: { turbo_frame: "_top" } do |_form| %>- </form> + <% 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/views/brex_items/select_accounts.html.erb` around lines 11 - 12, Replace the raw <form> and manual CSRF hidden_field_tag with Rails' form_with helper: call form_with url: link_accounts_brex_items_path, method: :post and pass the html options (class: "space-y-4", data: { turbo_frame: "_top" }) so the form uses the same action and attributes; remove the hidden_field_tag :authenticity_token, form_authenticity_token because form_with will automatically include the CSRF token. Ensure you update the closing tag/ERB block that currently wraps the form contents to use form_with's block form.app/models/brex_item/account_flow.rb (5)
622-627: 💤 Low valueHard-coded subtype strings — surface them as constants near the consuming model.
"credit_card"and"checking"are subtype identifiers tied toCreditCard::SUBTYPESandDepository::SUBTYPESrespectively. Hard-coding them here makes a future rename in those constants silently break Brex setup defaults. Consider referencing a constant on the accountable (e.g.CreditCard::DEFAULT_SUBTYPE/Depository::DEFAULT_SUBTYPE) so renames fail loudly at boot.🤖 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/brex_item/account_flow.rb` around lines 622 - 627, The method selected_subtype_for in account_flow.rb currently returns hard-coded strings "credit_card" and "checking"; replace those literals with constants on the related models (e.g. CreditCard::DEFAULT_SUBTYPE and Depository::DEFAULT_SUBTYPE) so renames fail loudly. Update selected_subtype_for to reference those constants when selected_type == "CreditCard" and selected_type == "Depository", and add/ensure the constants (DEFAULT_SUBTYPE or similarly named) exist on CreditCard and Depository to point to the current default values.
88-185: ⚖️ Poor tradeoffSignificant duplication between
select_accounts_resultandselect_existing_account_result.The two methods share substantial structure: both call
selection_failure_result, both filter accounts (with slightly different empty messages), both buildSelectionResultfor each branch, and both have nearly identicalrescuechains forNoApiTokenError/Provider::Brex::BrexError/StandardErrordiffering only in the i18n scope. Extracting a private helper that takes a config hash (scope_key,accountable_type, success/empty messages) would remove ~70 lines of repetition and make future status additions a one-place change.♻️ Sketch
def selection_result_for(scope:, accountable_type:, empty_message_key:) return selection_failure_result(scope, accountable_type: accountable_type) unless selected? accounts = filter_accounts(unlinked_available_accounts, accountable_type) return selection_result(:empty, accountable_type, I18n.t("#{scope}.#{empty_message_key}")) if accounts.empty? selection_result(:success, accountable_type, nil, available_accounts: accounts) rescue NoApiTokenError selection_result(:no_api_token, accountable_type, I18n.t("#{scope}.no_api_token")) rescue Provider::Brex::BrexError => e Rails.logger.error("Brex API error in #{scope}: #{e.message}") selection_result(:api_error, accountable_type, e.message) rescue StandardError => e Rails.logger.error("Unexpected error in #{scope}: #{e.class}: #{e.message}") selection_result(:unexpected_error, accountable_type, I18n.t("#{scope}.unexpected_error")) endAs per coding guidelines: avoid violations of DRY in changed code.
🤖 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/brex_item/account_flow.rb` around lines 88 - 185, The two methods select_accounts_result and select_existing_account_result are nearly identical; extract a private helper (e.g., selection_result_for or selection_result_with_scope) that accepts parameters like scope_key (string/symbol for i18n keys and logging), accountable_type, and an empty_message_key, centralizes the filter_accounts/unlinked_available_accounts logic, builds SelectionResult via a small selection_result(...) helper, and consolidates the rescue handlers (NoApiTokenError, Provider::Brex::BrexError, StandardError) so both original methods simply call this helper with their scope and message keys; update select_accounts_result and select_existing_account_result to delegate to that helper and remove the duplicated blocks.
286-301: 💤 Low value
import_accounts_from_api_if_neededwill not refresh accounts after the initial import.The early return
return nil unless brex_item.brex_accounts.empty?means once any account has been persisted, this method becomes a no-op — even if the user added new accounts on the Brex side and reopened the setup modal. As a result,BrexItems::AccountSetupsController#setup_accountsnever picks up newly-available accounts in the setup UI; users would need to wait for the next background sync. If that's intentional, a comment would help; otherwise consider re-fetching when the cache is also empty/stale, or distinguish "no accounts ever" from "no new accounts since last sync".🤖 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/brex_item/account_flow.rb` around lines 286 - 301, The current guard in import_accounts_from_api_if_needed (return nil unless brex_item.brex_accounts.empty?) prevents any subsequent refresh once a single account exists; change the logic to always fetch available_accounts (after still raising NoApiTokenError if credentials aren't configured), return nil only if fetched accounts are empty, then upsert only missing or updated accounts by comparing fetched account ids to existing brex_item.brex_accounts (e.g., existing_ids = brex_item.brex_accounts.pluck(:external_id)) and calling upsert_brex_account! for accounts whose id is not present or whose attributes differ; keep the NoApiTokenError and preserve final nil return.
67-86: 💤 Low value
preload_payloadignores cached value's emptiness for write-skipping but recomputes regardless on cache miss.Two small concerns:
- The cached array short-circuit at line 71-72 treats any cached value (including
[]) as "valid cache" viaunless cached_accounts.nil?, which is correct, but the booleanhas_accountsreturned forcached: trueis computed from the cached array. Make sure the writer inaccounts(private) and any cache invalidation paths never store a stale[]as if it were a real result; otherwise users will see "no accounts" until the 5-minute TTL expires.preload_payloadwrites the cache (line 75) and thenaccounts(line 570) re-reads + writes again. They sharecache_keyso it's fine in steady state, but a request that callspreload_payloadfollowed byselect_accounts_resultwill perform two cache reads where one would do.Optional cleanup: have
preload_payloaddelegate toaccountsrather than duplicate the read/fetch/write pattern.🤖 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/brex_item/account_flow.rb` around lines 67 - 86, preload_payload currently duplicates cache read/fetch/write logic and can cause double cache reads and stale empty-array behavior; change preload_payload to delegate to the existing private accounts method (call accounts or accounts(force: false) as appropriate) instead of re-implementing read/fetch/write, so it reuses the single authoritative cache_key/CACHE_TTL behavior and avoids a second read/write; also ensure the accounts writer only writes results when the fetch succeeds (so it does not persist a stale []), and keep cached presence checks driven by Rails.cache.exist?(cache_key) or the accounts method return value rather than testing for nil directly.
358-411: ⚖️ Poor tradeoff
complete_setup!holds transaction locks across multiple account creation iterations unnecessarily.With
skip_initial_sync: trueon eachAccount.create_and_synccall, no background jobs are enqueued during account creation—only database operations (save, opening balance, account shares) execute within the transaction. However, the entire loop runs inside a singleActiveRecord::Base.transaction, meaning all created accounts remain locked for the duration of all iterations. If any account creation fails late in the loop, every prior account is rolled back.Consider per-account transactions so a single bad account doesn't undo all prior successful setups, unless atomic-all-or-none behavior is strictly required for your use 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/brex_item/account_flow.rb` around lines 358 - 411, The current complete_setup! wraps the entire account_types loop in one ActiveRecord::Base.transaction which holds locks across all account creations; change this to use a per-account transaction around each create path (wrap the Account.create_and_sync and AccountProvider.create! for each brex_account in its own transaction) so a failure creating one account only rolls back that account instead of all prior ones, while leaving brex_item.sync_later and the final SetupResult behavior unchanged; alternatively, if you truly need atomic all-or-none semantics, document that and keep the outer transaction.app/models/family/brex_connectable.rb (1)
1-29: 💤 Low valueLGTM — concern follows the existing
*Connectablepattern.Symmetrical with other provider concerns:
has_many :brex_items, dependent: :destroy,create_*_item!followed bysync_later, and a credentials predicate. Triggeringsync_lateraftercreate!keeps the object-creation contract clean.One small note on
has_brex_credentials?:brex_items.active.any?(&:credentials_configured?)loads all active items into memory and iterates in Ruby. For most families this is fine, but ifcredentials_configured?is expressible as a SQL-side condition (e.g.where.not(token: [nil, ""])), pushing it into a scope (brex_items.active.with_credentials.exists?) avoids the N-row materialization. Optional.🤖 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/brex_connectable.rb` around lines 1 - 29, The has_brex_credentials? method currently materializes all active BrexItem records and calls credentials_configured? in Ruby; instead add a DB-side scope (e.g. in the BrexItem model define scope :with_credentials, -> { where.not(token: [nil, ""]) } or whichever SQL condition matches credentials_configured?) and change Family::BrexConnectable#has_brex_credentials? to use brex_items.active.with_credentials.exists? so the check runs in SQL and avoids loading all rows.app/models/brex_item/importer.rb (3)
164-179: 🏗️ Heavy lift
store_new_transactionsrewrites the entire transactions JSONB on every page — verify with bounded sync windows.
upsert_brex_transactions_snapshot!(existing_transactions + new_transactions)rewrites the full payload on each sync. WithProvider::Brexpagination caps (MAX_PAGES=250,DEFAULT_LIMIT=1000) raised in PR feedback, this can store hundreds of thousands of transactions in a single JSONB column, growing unbounded per account. Each subsequent sync deserializes/serializes the entire history, allocates awith_indifferent_accesswrapper per existing entry for dedup, and writes the union back.Two complementary improvements:
- Bound the appended history to a rolling window (e.g., last N days or last K rows) so the JSONB doesn't grow unboundedly.
- Pass
sync.window_start_date(per jjmata's review feedback) intodetermine_sync_start_dateso we don't re-fetch and re-merge the full history each time.🤖 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/brex_item/importer.rb` around lines 164 - 179, store_new_transactions currently reads the full brex_account.raw_transactions_payload, dedups against all IDs, then calls upsert_brex_transactions_snapshot! with the union—this rewrites an ever-growing JSONB each page; instead, add a window_start_date parameter (pass sync.window_start_date into determine_sync_start_date and thread it into store_new_transactions) and only keep/merge transactions within that rolling window (e.g., last N days or last K rows) before deduping and upserting; prune existing_transactions to the window, select new_transactions whose timestamps are >= window_start_date, and call upsert_brex_transactions_snapshot! with the pruned union so we no longer deserialize/serialize the entire history on every sync.
56-60: 💤 Low valueSnapshot persistence failure is silently swallowed.
store_item_snapshotrescues all errors and only logs. The import continues to callimport_accounts/import_transactionseven though the institution/raw_payload snapshot wasn't stored — meaning views that rely onraw_payload/raw_institution_payloadmay be stale or empty without any visible signal to the user (brex_item.statusstays whatever it was). Consider re-raising for non-transient failures, or at least reporting via Sentry likeBrexAccount::Processordoes.🤖 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/brex_item/importer.rb` around lines 56 - 60, The current store_item_snapshot method swallows all exceptions from brex_item.upsert_brex_snapshot! which allows the import to continue with missing raw_payload; change store_item_snapshot (and its rescue) so that on error you both report the exception to the error tracker (e.g. Sentry.capture_exception or the same mechanism used by BrexAccount::Processor) including context (brex_item id, accounts_data) and then re-raise the exception (or set a durable failure state on brex_item such as updating brex_item.status to a snapshot failure state before re-raising) so the overall import halts and the failure is visible.
23-23: 💤 Low valueStatus reset never marks healthy items back to
:goodafter partial failure recovery — but conversely, a non-credentials transient error keeps the item in whatever state it was previously in.
brex_item.update!(status: :good)only fires when the entire import has zero failures. After a transient API error causedmark_requires_update_if_credentials_errorto set:requires_updateonce (on a 401/403), a subsequent successful sync will move it back to:good. That's correct. However, if the prior failure was a non-credentials error that didn't change status, and the current sync has any failures (e.g., one transient transaction failure), the item may stay in a stale status indefinitely. Consider whether the success/health flag should reflect the latest sync regardless of prior state, or split status into "credentials valid" vs. "last sync ok".🤖 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/brex_item/importer.rb` at line 23, The current logic lets prior non-credentials failures leave brex_item in a stale non-:good state; ensure the item’s status reflects the latest sync by unconditionally setting status to :good when the current import had zero account and zero transaction failures (i.e., when account_result[:accounts_failed].zero? && transaction_result[:transactions_failed].zero?), and make sure mark_requires_update_if_credentials_error only sets :requires_update on 401/403 without blocking later status updates — move or adjust the brex_item.update!(status: :good) call and any early returns so the status reset runs based on the current results regardless of previous state.app/models/brex_account/transactions/processor.rb (2)
44-51: 💤 Low valueTrim full backtrace logging to first N frames for consistency.
Rails.logger.error Array(e.backtrace).join("\n")writes the full backtrace (often dozens of frames) for every failed transaction. With a malformed payload, this can flood logs. Other Brex code (e.g.BrexAccount::Processor#process_transactionsandBrexItem::AccountFlow#complete_setup_result) usesArray(e.backtrace).first(10). Consider matching that limit here.♻️ Proposed change
- Rails.logger.error Array(e.backtrace).join("\n") + Rails.logger.error Array(e.backtrace).first(10).join("\n")🤖 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/brex_account/transactions/processor.rb` around lines 44 - 51, The rescue block currently logs the full exception backtrace via Rails.logger.error Array(e.backtrace).join("\n"), which can flood logs; update the error logging in the rescue (the code around failed_count, transaction_id_for(transaction_data), and errors << ...) to instead log only the first N frames (match other Brex code by using Array(e.backtrace).first(10)) and join those frames for the Rails.logger.error call so the rest of the rescue behavior (incrementing failed_count and appending to errors) remains unchanged.
30-33: 💤 Low valueConsider a more specific signal for "skipped" vs. "failed" transactions.
A
nilreturn fromBrexEntry::Processoris currently bucketed intofailed_countwith the hard-coded reason"No linked account". If the processor ever returnsnilfor a different valid skip reason, both the count and the error message would become misleading. Returning a dedicated sentinel (e.g. a symbol:skipped) or splittingfailed_countfromskipped_countwould make the result safer for future evolution.🤖 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/brex_account/transactions/processor.rb` around lines 30 - 33, The current logic treats any nil result from BrexEntry::Processor as a failure with message "No linked account"; change this to a clear distinction by having BrexEntry::Processor return a sentinel (e.g. :skipped) for intentional skips and update the caller (the code handling result in processor.rb) to branch: if result == :skipped increment a new skipped_count and record a skip reason (or omit error), if result.nil? keep incrementing failed_count and record the real failure message (or transaction_id_for(transaction_data)); update any aggregated reporting to include skipped_count. Ensure you reference BrexEntry::Processor and the result handling block in processor.rb when making these changes.db/migrate/20260505010000_create_brex_items_and_accounts.rb (1)
32-57: ⚡ Quick winConsider tightening currency null constraint at the DB layer.
BrexAccountvalidatescurrencypresence in the model, but the column is nullable here. As per coding guidelines, simple validations (null checks, unique indexes) should be enforced in the database schema for PostgreSQL. Aligning the schema with the model invariant prevents bypass through direct SQL or future code paths.♻️ Proposed change
- t.string :currency + t.string :currency, null: falseYou may want to also add a default (e.g.
"USD") to make the migration safe to run on existing data, and align withBrexAccount.currency_code_from_moneydefaults used by the importer.As per coding guidelines: "Use simple database validations (null checks, unique indexes) in database schema".
🤖 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/20260505010000_create_brex_items_and_accounts.rb` around lines 32 - 57, The brex_accounts table currently allows NULL for the currency column but the BrexAccount model requires presence; update the migration to enforce this at DB level by changing the currency column to null: false and add a sensible default (e.g. "USD") to make the migration safe for existing rows (use t.string :currency, null: false, default: "USD"); ensure this aligns with BrexAccount.currency_code_from_money and any importer logic so defaults are consistent.app/models/brex_account/processor.rb (1)
38-41: 💤 Low valueConsider raising or alerting on persistent currency parse failures rather than silently defaulting to USD.
Defaulting to
"USD"whenparse_currencyfails keeps sync from breaking, but the warn log alone is easy to miss. If a non-USD account is silently converted to USD, downstream balance reporting will be subtly wrong. Consider a Sentry breadcrumb/capture_messagehere so it surfaces in monitoring without aborting the sync.🤖 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/brex_account/processor.rb` around lines 38 - 41, In BrexAccount::Processor, inside the branch where currency is nil after parse (the block referencing currency and brex_account.id), add monitoring so failures are visible: call Sentry.capture_message (or Sentry.add_breadcrumb + capture_message) with a clear message like "BrexAccount currency parse failed", include context/tags for brex_account.id and the raw brex_account.currency value, and set level to warning/error; keep the current fallback to "USD" so sync continues but ensure the capture includes useful context for debugging.app/controllers/brex_items/account_setups_controller.rb (1)
15-33: 💤 Low valueConsider strong params for
account_types/account_subtypes.
params[:account_types]andparams[:account_subtypes]are passed straight through as raw hashes. WhileBrexItem::AccountFlow#complete_setup!does whitelistselected_typeagainstProvider::BrexAdapter.supported_account_types, treating hash params asActionController::Parametersand explicitly permitting them keeps behavior consistent with the rest of the controller layer and avoids surprises if these params are used differently in future.As per coding guidelines: "Implement strong parameters and CSRF protection throughout the application".
🤖 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/brex_items/account_setups_controller.rb` around lines 15 - 33, The controller currently passes raw hashes from params[:account_types] and params[:account_subtypes] into brex_account_flow.complete_setup_result; update complete_account_setup to sanitize these by using strong params: build sanitized_account_types = params.fetch(:account_types, {}).permit(allowed_types) and sanitized_account_subtypes = params.fetch(:account_subtypes, {}).permit(allowed_subtypes), where allowed_types = Provider::BrexAdapter.supported_account_types.map(&:to_s) (and use the equivalent supported list for subtypes or an explicit whitelist), then pass those sanitized vars into brex_account_flow.complete_setup_result instead of the raw params to ensure only expected keys are forwarded.app/models/brex_item.rb (1)
33-44: ⚡ Quick winAvoid raising bare
StandardError.
raise StandardError.new("Brex provider is not configured")forces every caller to rescue the universal base class to catch this specific condition, which collides with unrelated failures. Define a domain-specific error (or reuseProvider::Brex::BrexError) so the importer/sync layers can branch on it deterministically.♻️ Proposed change
+ class ProviderNotConfiguredError < StandardError; end + def import_latest_brex_data(sync_start_date: nil) provider = brex_provider unless provider Rails.logger.error "BrexItem #{id} - Cannot import: provider is not configured" - raise StandardError.new("Brex provider is not configured") + raise ProviderNotConfiguredError, "Brex provider is not configured" 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/brex_item.rb` around lines 33 - 44, The method import_latest_brex_data currently raises a bare StandardError when the provider is missing; change this to raise a domain-specific error (e.g., Provider::Brex::BrexError or a new BrexProviderNotConfiguredError) so callers can rescue the specific condition; keep the existing Rails.logger.error line, and raise the chosen domain error from import_latest_brex_data (and ensure any existing rescue blocks that expect provider-specific errors use the new error type, e.g., in BrexItem::Importer and calling sync/import layers).
🤖 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/brex_items/account_flows_controller.rb`:
- Around line 98-111: The safe_return_to_path method allows paths like
"/\evil.com" because URI.parse normalizes backslashes; update
safe_return_to_path to additionally reject any return_to where the second
character (after the leading "/") is "\" or "/" or is a control/whitespace
character, and also strip/trust no surrounding whitespace — i.e., after
converting params[:return_to] to a string, ensure it starts with "/" and that
return_to[1] is present and is not "\\" or "/" and not a control/whitespace
codepoint before returning; keep the existing URI.parse and rescue
URI::InvalidURIError behavior but perform these extra validations (referencing
safe_return_to_path and params[:return_to]).
In `@app/models/brex_account.rb`:
- Around line 8-11: BrexAccount currently wraps encrypts :raw_payload and
:raw_transactions_payload with if encryption_ready? while BrexItem declares
encrypts :token unconditionally; make the policy consistent by removing the
conditional in BrexAccount and declaring encrypts :raw_payload and encrypts
:raw_transactions_payload unconditionally (matching BrexItem's approach), delete
or stop using the class-level encryption_ready? guard in this model, and ensure
the existing boot-time encryption health check (the one referenced in other PR
feedback) runs early and fails fast so missing keys are detected at boot rather
than leaving models partially unencrypted.
In `@app/models/brex_item.rb`:
- Around line 46-61: The loop in BrexItem#process_accounts causes an N+1 because
BrexAccount::Processor#process repeatedly accesses brex_account.current_account
(a delegation to the account association) but the query only uses
brex_accounts.joins(:account).merge(Account.visible) for filtering and does not
preload the association; update the query used in process_accounts (the
brex_accounts.joins(:account).merge(Account.visible).each call) to preload the
account association (e.g., add includes(:account) alongside the joins) so
current_account is eager loaded before iteration and prevents per-iteration
queries when BrexAccount::Processor#process reads or updates account fields.
In `@app/models/brex_item/importer.rb`:
- Around line 67-71: The pluck call in linked_account_ids is ambiguous because
the join to account_providers makes account_id refer to multiple tables; update
the pluck to explicitly reference the brex_accounts table name (use
"#{BrexAccount.table_name}.account_id") when building linked_account_ids (and
mirror the same explicit pluck used for all_existing_ids if applicable) so
PostgreSQL resolves the correct brex_accounts.account_id column; locate the
calls on brex_item.brex_accounts (variables linked_account_ids and
all_existing_ids) and replace the ambiguous pluck with the fully qualified
column reference.
In `@app/models/provider/brex.rb`:
- Around line 60-68: get_card_accounts currently calls get_json and
extract_records once, which can truncate large result sets; change it to use the
same paginated helper as get_cash_accounts (call
get_paginated("/v2/accounts/card")) so results honor MAX_PAGES/DEFAULT_LIMIT and
cursor handling, then map each account with
with_indifferent_access.merge(account_kind: "card") just like get_cash_accounts;
ensure you remove the single-page extract_records usage and rely on
get_paginated to return the full bounded list.
In `@app/views/settings/providers/_brex_panel.html.erb`:
- Around line 63-68: The delete button rendered by the button_to call using
brex_item_path currently only shows icon "trash-2" and lacks an accessible
label; update the button_to in _brex_panel.html.erb to include an aria-label (or
title) using a new I18n key (e.g., brex_items.provider_panel.disconnect_label)
that interpolates the item.name (e.g., "Disconnect %{name}"), add that key to
the Brex items locale file, and ensure the button_to passes aria-label:
t("brex_items.provider_panel.disconnect_label", name: item.name) so screen
readers receive a meaningful label while preserving the existing data: {
turbo_confirm: ... } and icon usage.
In `@app/views/settings/providers/show.html.erb`:
- Around line 64-68: Replace the hard-coded title "Brex (beta)" passed to the
settings_section helper with a translation call (use t(...)) and add the
corresponding key to config/locales/en.yml; locate the usage in the view where
settings_section is called (the title: argument in
app/views/settings/providers/show.html.erb) and replace it with a descriptive
i18n key (e.g., providers.brex.title or settings.providers.brex.title), then add
that key and value "Brex (beta)" to en.yml so the view renders the localized
string.
In `@config/locales/defaults/en.yml`:
- Around line 6-7: The translation key "close" is currently defined under common
(en.common.close) but the app expects defaults.common.close; update the YAML
structure so that common is nested under defaults (i.e., move the common block
inside a defaults mapping) so the key becomes defaults.common.close, ensuring
the "close" string is reachable where code looks it up.
In `@test/controllers/brex_items_controller_test.rb`:
- Around line 6-23: The tests currently call Rails.cache.clear in setup and
teardown which wipes global cache; instead only remove the Brex-related cache
entries: in the setup/teardown for this test file replace the global clear with
targeted deletion of keys created by these tests (use the same key patterns or
helper used by BrexItem or family caching—e.g. keys derived from
BrexItem.cache_key or a "brex" prefix) so that code in setup/teardown deletes
only keys for BrexItem/@existing_item/@second_item (or use
Rails.cache.delete_matched with the Brex key pattern) rather than calling
Rails.cache.clear.
---
Nitpick comments:
In `@app/controllers/brex_items/account_setups_controller.rb`:
- Around line 15-33: The controller currently passes raw hashes from
params[:account_types] and params[:account_subtypes] into
brex_account_flow.complete_setup_result; update complete_account_setup to
sanitize these by using strong params: build sanitized_account_types =
params.fetch(:account_types, {}).permit(allowed_types) and
sanitized_account_subtypes = params.fetch(:account_subtypes,
{}).permit(allowed_subtypes), where allowed_types =
Provider::BrexAdapter.supported_account_types.map(&:to_s) (and use the
equivalent supported list for subtypes or an explicit whitelist), then pass
those sanitized vars into brex_account_flow.complete_setup_result instead of the
raw params to ensure only expected keys are forwarded.
In `@app/models/brex_account/processor.rb`:
- Around line 38-41: In BrexAccount::Processor, inside the branch where currency
is nil after parse (the block referencing currency and brex_account.id), add
monitoring so failures are visible: call Sentry.capture_message (or
Sentry.add_breadcrumb + capture_message) with a clear message like "BrexAccount
currency parse failed", include context/tags for brex_account.id and the raw
brex_account.currency value, and set level to warning/error; keep the current
fallback to "USD" so sync continues but ensure the capture includes useful
context for debugging.
In `@app/models/brex_account/transactions/processor.rb`:
- Around line 44-51: The rescue block currently logs the full exception
backtrace via Rails.logger.error Array(e.backtrace).join("\n"), which can flood
logs; update the error logging in the rescue (the code around failed_count,
transaction_id_for(transaction_data), and errors << ...) to instead log only the
first N frames (match other Brex code by using Array(e.backtrace).first(10)) and
join those frames for the Rails.logger.error call so the rest of the rescue
behavior (incrementing failed_count and appending to errors) remains unchanged.
- Around line 30-33: The current logic treats any nil result from
BrexEntry::Processor as a failure with message "No linked account"; change this
to a clear distinction by having BrexEntry::Processor return a sentinel (e.g.
:skipped) for intentional skips and update the caller (the code handling result
in processor.rb) to branch: if result == :skipped increment a new skipped_count
and record a skip reason (or omit error), if result.nil? keep incrementing
failed_count and record the real failure message (or
transaction_id_for(transaction_data)); update any aggregated reporting to
include skipped_count. Ensure you reference BrexEntry::Processor and the result
handling block in processor.rb when making these changes.
In `@app/models/brex_entry/processor.rb`:
- Around line 112-116: The currency resolution in BrexEntry::Processor (method
currency) can trigger parse_currency rejections but this class never overrides
log_invalid_currency, so those warnings lack Brex-specific context; add an
override of log_invalid_currency in the BrexEntry::Processor class that formats
and emits provider-specific details (e.g., include data[:id], data[:amount],
BrexAccount.currency_from_money or brex_account.id/account identifier) and then
calls or delegates to the shared logger behavior so invalid-currency warnings
include Brex transaction/account context; reference the currency method,
parse_currency call, BrexAccount.currency_code_from_money, and
brex_account.currency when building the message.
In `@app/models/brex_item.rb`:
- Around line 33-44: The method import_latest_brex_data currently raises a bare
StandardError when the provider is missing; change this to raise a
domain-specific error (e.g., Provider::Brex::BrexError or a new
BrexProviderNotConfiguredError) so callers can rescue the specific condition;
keep the existing Rails.logger.error line, and raise the chosen domain error
from import_latest_brex_data (and ensure any existing rescue blocks that expect
provider-specific errors use the new error type, e.g., in BrexItem::Importer and
calling sync/import layers).
In `@app/models/brex_item/account_flow.rb`:
- Around line 622-627: The method selected_subtype_for in account_flow.rb
currently returns hard-coded strings "credit_card" and "checking"; replace those
literals with constants on the related models (e.g. CreditCard::DEFAULT_SUBTYPE
and Depository::DEFAULT_SUBTYPE) so renames fail loudly. Update
selected_subtype_for to reference those constants when selected_type ==
"CreditCard" and selected_type == "Depository", and add/ensure the constants
(DEFAULT_SUBTYPE or similarly named) exist on CreditCard and Depository to point
to the current default values.
- Around line 88-185: The two methods select_accounts_result and
select_existing_account_result are nearly identical; extract a private helper
(e.g., selection_result_for or selection_result_with_scope) that accepts
parameters like scope_key (string/symbol for i18n keys and logging),
accountable_type, and an empty_message_key, centralizes the
filter_accounts/unlinked_available_accounts logic, builds SelectionResult via a
small selection_result(...) helper, and consolidates the rescue handlers
(NoApiTokenError, Provider::Brex::BrexError, StandardError) so both original
methods simply call this helper with their scope and message keys; update
select_accounts_result and select_existing_account_result to delegate to that
helper and remove the duplicated blocks.
- Around line 286-301: The current guard in import_accounts_from_api_if_needed
(return nil unless brex_item.brex_accounts.empty?) prevents any subsequent
refresh once a single account exists; change the logic to always fetch
available_accounts (after still raising NoApiTokenError if credentials aren't
configured), return nil only if fetched accounts are empty, then upsert only
missing or updated accounts by comparing fetched account ids to existing
brex_item.brex_accounts (e.g., existing_ids =
brex_item.brex_accounts.pluck(:external_id)) and calling upsert_brex_account!
for accounts whose id is not present or whose attributes differ; keep the
NoApiTokenError and preserve final nil return.
- Around line 67-86: preload_payload currently duplicates cache read/fetch/write
logic and can cause double cache reads and stale empty-array behavior; change
preload_payload to delegate to the existing private accounts method (call
accounts or accounts(force: false) as appropriate) instead of re-implementing
read/fetch/write, so it reuses the single authoritative cache_key/CACHE_TTL
behavior and avoids a second read/write; also ensure the accounts writer only
writes results when the fetch succeeds (so it does not persist a stale []), and
keep cached presence checks driven by Rails.cache.exist?(cache_key) or the
accounts method return value rather than testing for nil directly.
- Around line 358-411: The current complete_setup! wraps the entire
account_types loop in one ActiveRecord::Base.transaction which holds locks
across all account creations; change this to use a per-account transaction
around each create path (wrap the Account.create_and_sync and
AccountProvider.create! for each brex_account in its own transaction) so a
failure creating one account only rolls back that account instead of all prior
ones, while leaving brex_item.sync_later and the final SetupResult behavior
unchanged; alternatively, if you truly need atomic all-or-none semantics,
document that and keep the outer transaction.
In `@app/models/brex_item/importer.rb`:
- Around line 164-179: store_new_transactions currently reads the full
brex_account.raw_transactions_payload, dedups against all IDs, then calls
upsert_brex_transactions_snapshot! with the union—this rewrites an ever-growing
JSONB each page; instead, add a window_start_date parameter (pass
sync.window_start_date into determine_sync_start_date and thread it into
store_new_transactions) and only keep/merge transactions within that rolling
window (e.g., last N days or last K rows) before deduping and upserting; prune
existing_transactions to the window, select new_transactions whose timestamps
are >= window_start_date, and call upsert_brex_transactions_snapshot! with the
pruned union so we no longer deserialize/serialize the entire history on every
sync.
- Around line 56-60: The current store_item_snapshot method swallows all
exceptions from brex_item.upsert_brex_snapshot! which allows the import to
continue with missing raw_payload; change store_item_snapshot (and its rescue)
so that on error you both report the exception to the error tracker (e.g.
Sentry.capture_exception or the same mechanism used by BrexAccount::Processor)
including context (brex_item id, accounts_data) and then re-raise the exception
(or set a durable failure state on brex_item such as updating brex_item.status
to a snapshot failure state before re-raising) so the overall import halts and
the failure is visible.
- Line 23: The current logic lets prior non-credentials failures leave brex_item
in a stale non-:good state; ensure the item’s status reflects the latest sync by
unconditionally setting status to :good when the current import had zero account
and zero transaction failures (i.e., when account_result[:accounts_failed].zero?
&& transaction_result[:transactions_failed].zero?), and make sure
mark_requires_update_if_credentials_error only sets :requires_update on 401/403
without blocking later status updates — move or adjust the
brex_item.update!(status: :good) call and any early returns so the status reset
runs based on the current results regardless of previous state.
In `@app/models/family/brex_connectable.rb`:
- Around line 1-29: The has_brex_credentials? method currently materializes all
active BrexItem records and calls credentials_configured? in Ruby; instead add a
DB-side scope (e.g. in the BrexItem model define scope :with_credentials, -> {
where.not(token: [nil, ""]) } or whichever SQL condition matches
credentials_configured?) and change
Family::BrexConnectable#has_brex_credentials? to use
brex_items.active.with_credentials.exists? so the check runs in SQL and avoids
loading all rows.
In `@app/views/brex_items/_brex_item.html.erb`:
- Around line 87-102: The partial is performing data lookups
(brex_item.syncs.ordered.first&.sync_stats and association-backed counts) which
may trigger extra queries; move this logic out of the ERB by precomputing and
passing in the values (stats, unlinked_count, linked_count, total_count,
institutions_count) from the controller/presenter or by adding a helper method
(e.g., fetch_brex_stats(brex_item, brex_sync_stats_map) or BrexItemPresenter
methods) and update the call site that renders this partial to provide those
locals instead of computing them inside the template; ensure ProviderSyncSummary
still receives the stats param and replace uses of `@brex_sync_stats_map` and
brex_item.*_accounts_count in the ERB with the passed-in locals.
In `@app/views/brex_items/select_accounts.html.erb`:
- Around line 11-12: Replace the raw <form> and manual CSRF hidden_field_tag
with Rails' form_with helper: call form_with url: link_accounts_brex_items_path,
method: :post and pass the html options (class: "space-y-4", data: {
turbo_frame: "_top" }) so the form uses the same action and attributes; remove
the hidden_field_tag :authenticity_token, form_authenticity_token because
form_with will automatically include the CSRF token. Ensure you update the
closing tag/ERB block that currently wraps the form contents to use form_with's
block form.
In `@db/migrate/20260505010000_create_brex_items_and_accounts.rb`:
- Around line 32-57: The brex_accounts table currently allows NULL for the
currency column but the BrexAccount model requires presence; update the
migration to enforce this at DB level by changing the currency column to null:
false and add a sensible default (e.g. "USD") to make the migration safe for
existing rows (use t.string :currency, null: false, default: "USD"); ensure this
aligns with BrexAccount.currency_code_from_money and any importer logic so
defaults are consistent.
In `@test/models/brex_entry/processor_test.rb`:
- Around line 25-34: The test "imports card purchase with Brex signed amount
preserved" doesn’t assert that merchant data in the saved payload is sanitized;
update this test (involving BrexEntry::Processor and the resulting entry
variable) to explicitly check entry.transaction.extra.dig("brex","merchant")
does not include card_metadata.pan (or any PAN field) and that expected merchant
fields remain present, ensuring the saved extra["brex"]["merchant"] has
sensitive PAN removed while other merchant attributes are preserved.
🪄 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: a60152d1-280d-4065-86e2-b8805996cecd
📒 Files selected for processing (55)
.gitignoreapp/controllers/accounts_controller.rbapp/controllers/brex_items/account_flows_controller.rbapp/controllers/brex_items/account_setups_controller.rbapp/controllers/brex_items_controller.rbapp/controllers/settings/providers_controller.rbapp/helpers/brex_items_helper.rbapp/models/brex_account.rbapp/models/brex_account/processor.rbapp/models/brex_account/transactions/processor.rbapp/models/brex_entry/processor.rbapp/models/brex_item.rbapp/models/brex_item/account_flow.rbapp/models/brex_item/importer.rbapp/models/brex_item/provided.rbapp/models/brex_item/syncer.rbapp/models/brex_item/unlinking.rbapp/models/concerns/encryptable.rbapp/models/data_enrichment.rbapp/models/family.rbapp/models/family/brex_connectable.rbapp/models/family/syncer.rbapp/models/provider/brex.rbapp/models/provider/brex_adapter.rbapp/models/provider_merchant.rbapp/views/accounts/index.html.erbapp/views/brex_items/_api_error.html.erbapp/views/brex_items/_brex_item.html.erbapp/views/brex_items/_setup_required.html.erbapp/views/brex_items/_subtype_select.html.erbapp/views/brex_items/select_accounts.html.erbapp/views/brex_items/select_existing_account.html.erbapp/views/brex_items/setup_accounts.html.erbapp/views/settings/providers/_brex_panel.html.erbapp/views/settings/providers/show.html.erbconfig/initializers/active_record_encryption.rbconfig/locales/defaults/en.ymlconfig/locales/models/brex_item/en.ymlconfig/locales/views/brex_items/en.ymlconfig/routes.rbdb/migrate/20260505010000_create_brex_items_and_accounts.rbdb/schema.rblib/active_record_encryption_config.rbtest/controllers/brex_items_controller_test.rbtest/fixtures/brex_accounts.ymltest/fixtures/brex_items.ymltest/lib/active_record_encryption_config_test.rbtest/models/brex_account_test.rbtest/models/brex_entry/processor_test.rbtest/models/brex_item/account_flow_test.rbtest/models/brex_item/importer_test.rbtest/models/brex_item/syncer_test.rbtest/models/brex_item_test.rbtest/models/provider/brex_adapter_test.rbtest/models/provider/brex_test.rb
💤 Files with no reviewable changes (1)
- .gitignore
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (17)
app/models/brex_item/account_flow.rb (2)
188-202: 💤 Low valueConsider wrapping
link_existing_account!in a transaction for symmetry withlink_new_accounts!.
link_new_accounts!(line 142) wraps upsert + AccountProvider creation inActiveRecord::Base.transaction, butlink_existing_account!does not. IfAccountProvider.create!(line 198) fails, the just-upsertedbrex_account(line 195) is committed without a link. Retries are idempotent so functional impact is low, but a transaction would keep the two paths symmetric and avoid orphaned snapshots when a partial failure occurs.🤖 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/brex_item/account_flow.rb` around lines 188 - 202, Wrap the upsert + provider creation in link_existing_account! in an ActiveRecord::Base.transaction to match link_new_accounts!: call upsert_brex_account! and AccountProvider.create! inside a transaction so a failed create! rolls back the upsert, then call brex_item.sync_later after the transaction and return brex_account; reference the existing methods link_existing_account!, upsert_brex_account!, AccountProvider.create!, and link_new_accounts! to mirror its behavior.
344-354: 💤 Low valueMethod name suggests a query but performs the import.
import_accounts_error_messagecallsimport_accounts_from_api_if_needed(a side-effecting operation that hits the Brex API and writes to the DB) and only returns an error message on failure. Callers reading this name on a controller would reasonably assume it's a pure error-formatting method. Consider a name likeimport_accounts_with_user_facing_erroror splitting the import call from the rescue/i18n translation.♻️ Possible rename
- def import_accounts_error_message + def import_accounts_or_error_message import_accounts_from_api_if_needed rescue NoApiTokenError I18n.t("brex_items.setup_accounts.no_api_token") ... endAdjust callers accordingly.
🤖 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/brex_item/account_flow.rb` around lines 344 - 354, The method import_accounts_error_message performs a side-effecting import by calling import_accounts_from_api_if_needed but its name implies a pure query; rename it to import_accounts_with_user_facing_error (or split into two: keep import_accounts_from_api_if_needed for the side-effect and create a pure import_accounts_error_message that only formats/errors) and update all callers to either call the new importer method then call the pure error-formatting helper, or call the new combined import_accounts_with_user_facing_error; ensure you update references to import_accounts_error_message and import_accounts_from_api_if_needed accordingly and preserve the same rescue/I18n behavior during the rename/split.app/views/accounts/index.html.erb (1)
52-62: 💤 Low valueInconsistent rendering pattern — Brex uses explicit locals while every other provider uses implicit collection rendering.
Every other provider in this file uses
<%= render@x_items.sort_by(&:created_at) %>and lets the partial pick up shared instance variables itself. The Brex block instead computes locals in the view viabrex_item_render_locals(...). Consider moving the helper call insideapp/views/brex_items/_brex_item.html.erb(or the helper invocation into the partial's first line) so the index can use the same one-liner pattern as Plaid/SimpleFIN/Mercury.♻️ Suggested change
- <% if `@brex_items.any`? %> - <% `@brex_items.sort_by`(&:created_at).each do |brex_item| %> - <%= render partial: "brex_items/brex_item", - locals: brex_item_render_locals( - brex_item, - sync_stats_map: `@brex_sync_stats_map`, - account_counts_map: `@brex_account_counts_map`, - institutions_count_map: `@brex_institutions_count_map` - ) %> - <% end %> - <% end %> + <% if `@brex_items.any`? %> + <%= render `@brex_items.sort_by`(&:created_at) %> + <% end %>Then have
_brex_item.html.erbopen with<% locals = brex_item_render_locals(brex_item, sync_stats_map:@brex_sync_stats_map, account_counts_map:@brex_account_counts_map, institutions_count_map:@brex_institutions_count_map) %>(or merge directly into the existing locals).🤖 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/accounts/index.html.erb` around lines 52 - 62, Replace the explicit per-item locals in the index view by rendering the Brex collection like the others (use render `@brex_items.sort_by`(&:created_at)), and move the brex_item_render_locals(...) call into the Brex partial so it computes needed locals for each brex_item; specifically, update app/views/brex_items/_brex_item.html.erb to begin by calling brex_item_render_locals(brex_item, sync_stats_map: `@brex_sync_stats_map`, account_counts_map: `@brex_account_counts_map`, institutions_count_map: `@brex_institutions_count_map`) (or merge its values into the partial’s existing locals) and adjust the partial to use those locals so the index block can be a one-liner like the other providers.test/models/brex_account/transactions/processor_test.rb (1)
24-34: 💤 Low valueCoverage is narrow — consider asserting the success/imported path too.
This file only validates the "no linked account" skip branch. The processor surface (
total/imported/skipped/failed/errors/skipped_transactions) is broad enough that a happy-path test (linked account + imported entry) and at least one failure-path test (e.g., processor raising for one entry while others succeed) would lock in the aggregation contract. Not a blocker; flagging as a coverage gap rather than a defect.🤖 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/brex_account/transactions/processor_test.rb` around lines 24 - 34, Add two more tests for BrexAccount::Transactions::Processor#process: one "happy-path" that sets up a linked account and verifies result[:success] true, result[:total] increments, result[:imported] increments, result[:skipped] and result[:failed] are zero, and skipped_transactions/errors are empty; and one "partial-failure" test that rigs one entry to raise (or simulate a failed import) while others succeed and then asserts aggregated counts (total, imported, skipped, failed), presence of an error in result[:errors], and that the failed entry is represented appropriately in result[:failed] or result[:errors]; use the same result keys (total/imported/skipped/failed/errors/skipped_transactions) and BrexAccount::Transactions::Processor.new(`@brex_account`).process to exercise the aggregation contract.app/views/brex_items/_brex_item.html.erb (1)
22-24: 💤 Low valueUse semantic markup consistent with surrounding
tag.p.Line 21 uses
tag.pfor the item name and line 23 uses a raw<p>for the deletion-in-progress label inside the same flex row. This works but is inconsistent with the helper-based tag usage elsewhere in the partial. Usingtag.pkeeps the file consistent and benefits from automatic escaping/CSP-friendliness.♻️ Suggested tweak
- <% if brex_item.scheduled_for_deletion? %> - <p class="text-destructive text-sm animate-pulse"><%= t(".deletion_in_progress") %></p> - <% end %> + <% if brex_item.scheduled_for_deletion? %> + <%= tag.p t(".deletion_in_progress"), class: "text-destructive text-sm animate-pulse" %> + <% 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/views/brex_items/_brex_item.html.erb` around lines 22 - 24, Replace the raw HTML paragraph used for the deletion label with the Rails tag helper to match the surrounding markup: in the conditional that checks brex_item.scheduled_for_deletion? swap the literal <p class="..."> containing t(".deletion_in_progress") for tag.p with the same class list (e.g., "text-destructive text-sm animate-pulse") so it uses the helper-based rendering and automatic escaping consistent with the other tag.p usage in this partial.app/controllers/brex_items/account_flows_controller.rb (2)
30-55: ⚡ Quick winHandle missing account ids without surfacing a 404 to the user.
Current.family.accounts.find(params[:account_id])on lines 33 and 48 will raiseActiveRecord::RecordNotFoundif the id is for an account from another family or simply doesn't exist. UnlessApplicationControlleralready rescues this for HTML/Turbo flows (and renders a friendly redirect), a malformed/expired link will produce a 404 inside a Turbo modal flow rather than aredirect_to accounts_pathwith a flash. Recommend either usingfind_by(id: …)with an explicit redirect (mirroring theparams[:account_id].blank?early return) or adding arescue_from ActiveRecord::RecordNotFoundfor this controller.♻️ Suggested change
def select_existing_account return redirect_to accounts_path, alert: t("brex_items.select_existing_account.no_account_specified") if params[:account_id].blank? - `@account` = Current.family.accounts.find(params[:account_id]) + `@account` = Current.family.accounts.find_by(id: params[:account_id]) + return redirect_to accounts_path, alert: t("brex_items.select_existing_account.no_account_specified") if `@account.nil`? result = brex_account_flow.select_existing_account_result(account: `@account`)🤖 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/brex_items/account_flows_controller.rb` around lines 30 - 55, The controller currently calls Current.family.accounts.find(params[:account_id]) in select_existing_account and link_existing_account which will raise ActiveRecord::RecordNotFound for invalid/other-family IDs; change both to use Current.family.accounts.find_by(id: params[:account_id]) and if that returns nil perform the same redirect used for blank params (redirect_to accounts_path, alert: t("brex_items.select_existing_account.no_account_specified") for select_existing_account and the analogous t("brex_items.link_existing_account.no_account_specified") or reuse the same message for link_existing_account), so that brex_account_flow.select_existing_account_result and brex_account_flow.link_existing_account_result are only called with a valid account and no 404 is surfaced in Turbo/modal flows.
98-116: 💤 Low valueConsider rejecting percent-encoded slash/backslash sequences in
safe_return_to_pathfor defense in depth.The current implementation correctly blocks
//evil,/\evil, control chars, whitespace, and absolute URLs. However, percent-encoded variants such as/%2fevil.example/pathand/%5cevil.example/pathwill pass:URI.parsekeeps them as a path with no scheme/host, and the second character check sees%, not/or\. While most browsers don't decode these inLocationheaders, intermediaries (proxies, link rewriters) sometimes do, and adding the check is essentially free.🛡️ Suggested hardening
second_character = return_to[1] return nil if second_character.blank? return nil if second_character == "/" || second_character == "\\" return nil if second_character.match?(/[[:space:][:cntrl:]]/) + return nil if return_to.match?(/\A\/(?:%2f|%5c)/i) uri = URI.parse(return_to)🤖 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/brex_items/account_flows_controller.rb` around lines 98 - 116, The safe_return_to_path method should reject percent-encoded slash/backslash sequences to harden against proxies that decode them; after computing return_to (from params[:return_to].to_s.strip) and before URI.parse, check the bytes at positions 1..3 (the percent-encoded sequence immediately after the leading "/") and return nil if they match %2f or %5c (case-insensitive) or otherwise decode to "/" or "\"; keep the existing second_character checks and the URI.parse host/scheme checks but add this explicit percent-encoding rejection so strings like "/%2fevil" or "/%5cevil" are rejected by safe_return_to_path.test/controllers/brex_items_controller_test.rb (1)
313-347: 💤 Low valueVerify the unsupported-type branch covers the actual production filter.
The test asserts that passing
account_types[unsupported_brex_account.id] = "Investment"results inunsupported_brex_account.account_providerbeingnilaftercomplete_account_setup. That's a meaningful assertion, but it only confirms a side-effect, not which guard rejected it (unsupportedaccountable_type, unsupportedaccount_kind, missingbrex_accountlookup, etc.). Consider also asserting thatflash[:notice](or an analogous message) reports a partial setup, or that the controller exposes a count of skipped/unsupported entries — otherwise a future refactor that silently swallows valid Investment requests would still pass this test.🤖 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/controllers/brex_items_controller_test.rb` around lines 313 - 347, The test currently only checks the side-effect that unsupported_brex_account.account_provider is nil after calling complete_account_setup_brex_item_url; update the test to also assert the controller reported the skipped/unsupported entry (e.g. assert_match /skipp|unsupported|partial/i, flash[:notice] || flash[:alert]) so you verify the actual guard that rejected the "Investment" account type is surfaced to the user; if the controller doesn't set such a flash, modify the complete_account_setup action to set flash[:notice] with a message or expose a skipped_count (e.g. `@skipped_count`) and assert that value in the test instead (referencing complete_account_setup_brex_item_url, unsupported_brex_account, and AccountProvider.count).app/views/brex_items/select_accounts.html.erb (1)
19-43: 💤 Low valueProvide an explicit submit-disabled state when no selectable accounts exist.
If every entry in
@available_accountshas a blank name, every checkbox isdisabled, but the "Link Accounts" submit button stays enabled. Submitting then postsaccount_ids=[]tolink_accounts_brex_items_path, which the controller has to special-case. Consider disabling the submit button (and/or rendering an empty-state message) when@available_accounts.none? { |a| !brex_account_display(a).blank_name? }so the UI doesn't invite a no-op submission.🤖 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/brex_items/select_accounts.html.erb` around lines 19 - 43, The form should disable the submit action when there are no selectable accounts: compute a boolean like has_selectable = `@available_accounts.any`? { |a| !brex_account_display(a).blank_name? } and use it to disable the "Link Accounts" submit button (e.g. add disabled: true and a disabled visual style/class) and/or render an explicit empty-state message; update any view helper usage around brex_account_display and check_box_tag to rely on has_selectable so the submit cannot be clicked when all checkboxes are disabled.app/controllers/brex_items/account_setups_controller.rb (1)
1-3: 💤 Low valueRun
require_admin!beforeset_brex_item.
set_brex_itemtriggers a database lookup using the request param before the admin check has run. Reordering the callbacks ensures non-admins are rejected without any DB access and avoids leaking error states (e.g.,RecordNotFound) that could differentiate between "missing item" and "not allowed" for non-admin callers.♻️ Proposed reorder
- before_action :set_brex_item - before_action :require_admin! + before_action :require_admin! + before_action :set_brex_item🤖 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/brex_items/account_setups_controller.rb` around lines 1 - 3, The before_action ordering in BrexItems::AccountSetupsController calls set_brex_item before require_admin!, causing a DB lookup (and potential RecordNotFound) before authorization; swap the callbacks so require_admin! runs first (ensure the controller uses before_action :require_admin! followed by before_action :set_brex_item) to reject non-admins without accessing the database or leaking error details.app/models/brex_account.rb (3)
161-167: 💤 Low valueReplace
assign_attributes+save!withupdate!.There's no intermediate logic between assignment and save, so
update!is more idiomatic and equivalent.♻️ Proposed simplification
def upsert_brex_transactions_snapshot!(transactions_snapshot) - assign_attributes( - raw_transactions_payload: self.class.sanitize_payload(transactions_snapshot) - ) - - save! + update!(raw_transactions_payload: self.class.sanitize_payload(transactions_snapshot)) 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/brex_account.rb` around lines 161 - 167, The method upsert_brex_transactions_snapshot!(transactions_snapshot) currently calls assign_attributes(...) followed by save!; replace that pair with a single call to update! to be idiomatic and atomic. Specifically, update the method to call update!(raw_transactions_payload: self.class.sanitize_payload(transactions_snapshot)) instead of assign_attributes(...) and save! so the sanitized payload is persisted in one step.
64-77: ⚡ Quick win
money_to_decimalrescue path can re-raiseArgumentError.If
BigDecimal(amount.to_s)raisesArgumentError(e.g., the API returns a non-numericamount), the rescue branch executesBigDecimal(payload[:amount].to_s)again on the same value, which will raise the same exception and propagate out of the method. Either (a) hard-default toBigDecimal("0")in the rescue or (b) catch the recurrence inside the rescue.🛡️ Suggested fix
rescue Money::Currency::UnknownCurrencyError, ArgumentError Rails.logger.warn("Invalid Brex money payload #{money_payload.inspect}, defaulting conversion to USD") - BigDecimal(payload[:amount].to_s) / BigDecimal(Money::Currency.new("USD").minor_unit_conversion.to_s) + begin + BigDecimal(payload[:amount].to_s) / BigDecimal(Money::Currency.new("USD").minor_unit_conversion.to_s) + rescue ArgumentError + nil + end 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/brex_account.rb` around lines 64 - 77, In money_to_decimal, the rescue currently retries BigDecimal(payload[:amount].to_s) which can re-raise the same ArgumentError; update the rescue block in the money_to_decimal method to avoid propagating ArgumentError by wrapping the fallback BigDecimal(...) in its own safe handler (or simply return BigDecimal("0") on failure), ensure you still log the original payload (Rails.logger.warn(...)) and avoid calling Money::Currency.new("USD").minor_unit_conversion on invalid amounts unless guarded; reference the money_to_decimal method, the rescue clause, BigDecimal(...) and Money::Currency usage when making the change.
13-15: 💤 Low value
linked_accountduplicatesaccount.Both
accountandlinked_accountare declaredhas_one :through => :account_provider, source: :account, so they resolve to the same record. Pick one canonical name — using both invites confusion (e.g., callinglinked_accountandaccounttriggers two distinct queries because each association caches independently).♻️ Proposed simplification
has_one :account_provider, as: :provider, dependent: :destroy has_one :account, through: :account_provider, source: :account - has_one :linked_account, through: :account_provider, source: :accountIf
linked_accountis required for compatibility with existing call sites, alias it instead:alias_method :linked_account, :account🤖 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/brex_account.rb` around lines 13 - 15, The BrexAccount model defines both has_one :account and has_one :linked_account through :account_provider with the same source (:account), causing duplicate, uncached queries and confusion; fix by removing the redundant has_one :linked_account and update callers to use :account, or if :linked_account must remain for compatibility, replace the association with an alias (alias_method :linked_account, :account) so both names refer to the same cached association; locate the duplicate declarations in the BrexAccount class (has_one :account, has_one :linked_account) and either delete the linked_account association or convert it to an alias.test/models/brex_item/importer_test.rb (1)
25-47: 💤 Low valueTighten the
start_datematcher to assert the exact value passed.This test passes
Date.new(2026, 1, 1)to the importer, so the matcher could assertstart_date: Date.new(2026, 1, 1)instead ofanythingand still verify the same boundary contract that "uses explicit sync start date..." (line 132) covers. Usinganythinghere weakens the assertion that the importer threadssync_start_dateintoget_cash_transactionsfor the discovery flow.♻️ Suggested tightening
- provider.expects(:get_cash_transactions).with("cash_1", start_date: anything).returns( + provider.expects(:get_cash_transactions).with("cash_1", start_date: Date.new(2026, 1, 1)).returns(🤖 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/brex_item/importer_test.rb` around lines 25 - 47, The provider expectation for get_cash_transactions uses a loose matcher (anything) for start_date; tighten it to expect the exact sync start date passed to BrexItem::Importer so the test verifies threading of sync_start_date through discovery. Update the mock expectation on provider.get_cash_transactions to use start_date: Date.new(2026, 1, 1) (matching the sync_start_date argument passed to BrexItem::Importer.new) instead of anything; keep the rest of the expectation and assertions unchanged.app/models/provider/brex_adapter.rb (1)
115-131: 💤 Low valuePrefer
metadata&.dig("name")overmetadata&.[]("name").The
&.[]form works but is unidiomatic and harder to read.digis the conventional Ruby way to safely access a nested hash key.♻️ Proposed simplification
def institution_name metadata = provider_account.institution_metadata - - metadata&.[]("name") || item&.institution_name + metadata&.dig("name") || item&.institution_name end def institution_url metadata = provider_account.institution_metadata - - metadata&.[]("url") || item&.institution_url + metadata&.dig("url") || item&.institution_url end def institution_color metadata = provider_account.institution_metadata - - metadata&.[]("color") || item&.institution_color + metadata&.dig("color") || item&.institution_color 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/provider/brex_adapter.rb` around lines 115 - 131, Replace the unidiomatic safe-access calls using metadata&.[]("key") with metadata&.dig("key") in the three accessors: institution_name, institution_url, and institution_color; update the code that reads provider_account.institution_metadata into metadata and return metadata&.dig("name") || item&.institution_name, metadata&.dig("url") || item&.institution_url, and metadata&.dig("color") || item&.institution_color respectively to make the hash lookups clearer and idiomatic.app/models/brex_item.rb (2)
128-133: 💤 Low value
includes(:account)is unused inconnected_institutions.The block only reads
acc.institution_metadata, which is a column onbrex_accountsitself — there's no traversal of theaccountassociation inside the iteration. The eager-load is dead weight and will issue an extra query per call.♻️ Proposed simplification
def connected_institutions - brex_accounts.includes(:account) - .where.not(institution_metadata: nil) + brex_accounts.where.not(institution_metadata: nil) .map { |acc| acc.institution_metadata } .uniq { |inst| inst["name"] || inst["institution_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/brex_item.rb` around lines 128 - 133, The includes(:account) in the connected_institutions method is unnecessary because the code only reads acc.institution_metadata (a column on brex_accounts) and does not traverse the account association; remove the eager-load and instead iterate or pluck directly from brex_accounts (e.g., use brex_accounts.where.not(institution_metadata: nil).pluck(:institution_metadata).uniq { |inst| inst["name"] || inst["institution_name"] }) to avoid the extra query and de-duplicate by the same uniqueness key.
85-91: 💤 Low valueUse
update!instead ofassign_attributes+save!.There's no in-between logic, so
update!is equivalent and more idiomatic. (Same pattern inBrexAccount#upsert_brex_transactions_snapshot!.)♻️ Proposed simplification
def upsert_brex_snapshot!(accounts_snapshot) - assign_attributes( - raw_payload: BrexAccount.sanitize_payload(accounts_snapshot) - ) - - save! + update!(raw_payload: BrexAccount.sanitize_payload(accounts_snapshot)) 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/brex_item.rb` around lines 85 - 91, Replace the assign_attributes + save! pattern in the method upsert_brex_snapshot! with a single update! call: call update!(raw_payload: BrexAccount.sanitize_payload(accounts_snapshot)) inside the upsert_brex_snapshot! method; do the same refactor for the analogous method BrexAccount#upsert_brex_transactions_snapshot! if present to keep the pattern consistent and idiomatic.
🤖 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/brex_items/account_flows_controller.rb`:
- Around line 4-28: The link_accounts path currently forwards
params[:accountable_type] into brex_account_flow.link_new_accounts_result →
link_new_accounts! without validating the value; add the same validation pattern
used in complete_setup! to link_new_accounts_result (or at the start of
link_new_accounts!) to reject unsupported accountable_type values before calling
Account.create_and_sync or BrexAccount.default_accountable_attributes, e.g.,
ensure accountable_type is one of the allowed types (matching filter_accounts)
and return a failed Result (or raise/handle) when invalid so attacker-supplied
values are not forwarded to Account.create_and_sync or
BrexAccount.default_accountable_attributes.
In `@app/models/brex_entry/processor.rb`:
- Around line 64-69: The fallback literal "Brex transaction" in the name method
should be replaced with a translation call and an i18n key; update the def name
method to use t('brex_items.entries.default_name') as the final fallback instead
of the hardcoded string (method name: name in the Brex entry processor /
Entry#name), and add the corresponding key brex_items.entries.default_name:
"Brex transaction" to config/locales/en.yml so the user-facing default is
localized.
- Around line 30-39: The rescue handlers call external_id during log
interpolation which can re-raise ArgumentError and swallow the intended log; fix
this by reading/caching the external id into a local variable once (e.g.,
cached_external_id = external_id or derive safely from data[:id]) before the
begin/rescue region and then use that cached_external_id in the rescue log lines
inside the rescue blocks (the handlers for ArgumentError,
ActiveRecord::RecordInvalid/RecordNotSaved, and the generic rescue) so
interpolation cannot raise while logging.
In `@app/models/family/brex_connectable.rb`:
- Around line 14-24: Add a before_validation callback on the BrexItem model to
strip and normalize the token the same way normalize_base_url does for base_url:
implement a private method (e.g., normalize_token) that replaces token with
token&.strip (or nil-safe equivalent) and register it via before_validation
:normalize_token; this ensures tokens created via Family#create_brex_item! or
any other non-controller code are normalized before validation/save.
In `@app/views/brex_items/_brex_item.html.erb`:
- Around line 36-40: The stored sync error may contain raw exception text
because Sync#perform currently does update(error: e.message) for any unhandled
exception; modify the error-handling so that BrexItem::Syncer#perform_sync (and
any helper methods it calls) rescue exceptions and raise or return only curated,
user-safe messages (e.g., standardized strings like "Invalid Brex API token or
account permissions" or "Sync failed, contact support") instead of raw
e.message, or alternatively sanitize inside Sync#perform before persisting (map
specific known exception classes to friendly messages and fall back to a generic
message for all others while logging the original exception). Ensure the
original exception is still logged/reported to Sentry/Logs but never written to
brex_item.sync_error that the view renders.
In `@app/views/brex_items/select_accounts.html.erb`:
- Line 22: The label element that conditions on account_display.blank_name? is
using non-existent Tailwind utilities (border-error, bg-error/5, text-error);
update that label's class logic to use design-system tokens: replace
border-error with border-destructive, replace bg-error/5 with a semantic
destructive background (either bg-destructive-soft if you add that token or use
an existing bg-destructive with an opacity wrapper), and swap text-error
occurrences to the appropriate token (text-primary for normal emphasis or
text-destructive if you introduce destructive text styling); keep the existing
conditional on account_display.blank_name? and ensure the disabled-style classes
(cursor-not-allowed opacity-60) remain applied when blank_name? is true.
In `@db/migrate/20260505010000_create_brex_items_and_accounts.rb`:
- Around line 15-17: The migration currently sets defaults but leaves nullable
columns for status, scheduled_for_deletion, and pending_account_setup; update
the migration that defines these columns (the CreateBrexItemsAndAccounts
migration) to add null: false to t.string :status, t.boolean
:scheduled_for_deletion, and t.boolean :pending_account_setup so the DB enforces
non-null values alongside their defaults.
---
Nitpick comments:
In `@app/controllers/brex_items/account_flows_controller.rb`:
- Around line 30-55: The controller currently calls
Current.family.accounts.find(params[:account_id]) in select_existing_account and
link_existing_account which will raise ActiveRecord::RecordNotFound for
invalid/other-family IDs; change both to use Current.family.accounts.find_by(id:
params[:account_id]) and if that returns nil perform the same redirect used for
blank params (redirect_to accounts_path, alert:
t("brex_items.select_existing_account.no_account_specified") for
select_existing_account and the analogous
t("brex_items.link_existing_account.no_account_specified") or reuse the same
message for link_existing_account), so that
brex_account_flow.select_existing_account_result and
brex_account_flow.link_existing_account_result are only called with a valid
account and no 404 is surfaced in Turbo/modal flows.
- Around line 98-116: The safe_return_to_path method should reject
percent-encoded slash/backslash sequences to harden against proxies that decode
them; after computing return_to (from params[:return_to].to_s.strip) and before
URI.parse, check the bytes at positions 1..3 (the percent-encoded sequence
immediately after the leading "/") and return nil if they match %2f or %5c
(case-insensitive) or otherwise decode to "/" or "\"; keep the existing
second_character checks and the URI.parse host/scheme checks but add this
explicit percent-encoding rejection so strings like "/%2fevil" or "/%5cevil" are
rejected by safe_return_to_path.
In `@app/controllers/brex_items/account_setups_controller.rb`:
- Around line 1-3: The before_action ordering in
BrexItems::AccountSetupsController calls set_brex_item before require_admin!,
causing a DB lookup (and potential RecordNotFound) before authorization; swap
the callbacks so require_admin! runs first (ensure the controller uses
before_action :require_admin! followed by before_action :set_brex_item) to
reject non-admins without accessing the database or leaking error details.
In `@app/models/brex_account.rb`:
- Around line 161-167: The method
upsert_brex_transactions_snapshot!(transactions_snapshot) currently calls
assign_attributes(...) followed by save!; replace that pair with a single call
to update! to be idiomatic and atomic. Specifically, update the method to call
update!(raw_transactions_payload:
self.class.sanitize_payload(transactions_snapshot)) instead of
assign_attributes(...) and save! so the sanitized payload is persisted in one
step.
- Around line 64-77: In money_to_decimal, the rescue currently retries
BigDecimal(payload[:amount].to_s) which can re-raise the same ArgumentError;
update the rescue block in the money_to_decimal method to avoid propagating
ArgumentError by wrapping the fallback BigDecimal(...) in its own safe handler
(or simply return BigDecimal("0") on failure), ensure you still log the original
payload (Rails.logger.warn(...)) and avoid calling
Money::Currency.new("USD").minor_unit_conversion on invalid amounts unless
guarded; reference the money_to_decimal method, the rescue clause,
BigDecimal(...) and Money::Currency usage when making the change.
- Around line 13-15: The BrexAccount model defines both has_one :account and
has_one :linked_account through :account_provider with the same source
(:account), causing duplicate, uncached queries and confusion; fix by removing
the redundant has_one :linked_account and update callers to use :account, or if
:linked_account must remain for compatibility, replace the association with an
alias (alias_method :linked_account, :account) so both names refer to the same
cached association; locate the duplicate declarations in the BrexAccount class
(has_one :account, has_one :linked_account) and either delete the linked_account
association or convert it to an alias.
In `@app/models/brex_item.rb`:
- Around line 128-133: The includes(:account) in the connected_institutions
method is unnecessary because the code only reads acc.institution_metadata (a
column on brex_accounts) and does not traverse the account association; remove
the eager-load and instead iterate or pluck directly from brex_accounts (e.g.,
use brex_accounts.where.not(institution_metadata:
nil).pluck(:institution_metadata).uniq { |inst| inst["name"] ||
inst["institution_name"] }) to avoid the extra query and de-duplicate by the
same uniqueness key.
- Around line 85-91: Replace the assign_attributes + save! pattern in the method
upsert_brex_snapshot! with a single update! call: call update!(raw_payload:
BrexAccount.sanitize_payload(accounts_snapshot)) inside the
upsert_brex_snapshot! method; do the same refactor for the analogous method
BrexAccount#upsert_brex_transactions_snapshot! if present to keep the pattern
consistent and idiomatic.
In `@app/models/brex_item/account_flow.rb`:
- Around line 188-202: Wrap the upsert + provider creation in
link_existing_account! in an ActiveRecord::Base.transaction to match
link_new_accounts!: call upsert_brex_account! and AccountProvider.create! inside
a transaction so a failed create! rolls back the upsert, then call
brex_item.sync_later after the transaction and return brex_account; reference
the existing methods link_existing_account!, upsert_brex_account!,
AccountProvider.create!, and link_new_accounts! to mirror its behavior.
- Around line 344-354: The method import_accounts_error_message performs a
side-effecting import by calling import_accounts_from_api_if_needed but its name
implies a pure query; rename it to import_accounts_with_user_facing_error (or
split into two: keep import_accounts_from_api_if_needed for the side-effect and
create a pure import_accounts_error_message that only formats/errors) and update
all callers to either call the new importer method then call the pure
error-formatting helper, or call the new combined
import_accounts_with_user_facing_error; ensure you update references to
import_accounts_error_message and import_accounts_from_api_if_needed accordingly
and preserve the same rescue/I18n behavior during the rename/split.
In `@app/models/provider/brex_adapter.rb`:
- Around line 115-131: Replace the unidiomatic safe-access calls using
metadata&.[]("key") with metadata&.dig("key") in the three accessors:
institution_name, institution_url, and institution_color; update the code that
reads provider_account.institution_metadata into metadata and return
metadata&.dig("name") || item&.institution_name, metadata&.dig("url") ||
item&.institution_url, and metadata&.dig("color") || item&.institution_color
respectively to make the hash lookups clearer and idiomatic.
In `@app/views/accounts/index.html.erb`:
- Around line 52-62: Replace the explicit per-item locals in the index view by
rendering the Brex collection like the others (use render
`@brex_items.sort_by`(&:created_at)), and move the brex_item_render_locals(...)
call into the Brex partial so it computes needed locals for each brex_item;
specifically, update app/views/brex_items/_brex_item.html.erb to begin by
calling brex_item_render_locals(brex_item, sync_stats_map: `@brex_sync_stats_map`,
account_counts_map: `@brex_account_counts_map`, institutions_count_map:
`@brex_institutions_count_map`) (or merge its values into the partial’s existing
locals) and adjust the partial to use those locals so the index block can be a
one-liner like the other providers.
In `@app/views/brex_items/_brex_item.html.erb`:
- Around line 22-24: Replace the raw HTML paragraph used for the deletion label
with the Rails tag helper to match the surrounding markup: in the conditional
that checks brex_item.scheduled_for_deletion? swap the literal <p class="...">
containing t(".deletion_in_progress") for tag.p with the same class list (e.g.,
"text-destructive text-sm animate-pulse") so it uses the helper-based rendering
and automatic escaping consistent with the other tag.p usage in this partial.
In `@app/views/brex_items/select_accounts.html.erb`:
- Around line 19-43: The form should disable the submit action when there are no
selectable accounts: compute a boolean like has_selectable =
`@available_accounts.any`? { |a| !brex_account_display(a).blank_name? } and use it
to disable the "Link Accounts" submit button (e.g. add disabled: true and a
disabled visual style/class) and/or render an explicit empty-state message;
update any view helper usage around brex_account_display and check_box_tag to
rely on has_selectable so the submit cannot be clicked when all checkboxes are
disabled.
In `@test/controllers/brex_items_controller_test.rb`:
- Around line 313-347: The test currently only checks the side-effect that
unsupported_brex_account.account_provider is nil after calling
complete_account_setup_brex_item_url; update the test to also assert the
controller reported the skipped/unsupported entry (e.g. assert_match
/skipp|unsupported|partial/i, flash[:notice] || flash[:alert]) so you verify the
actual guard that rejected the "Investment" account type is surfaced to the
user; if the controller doesn't set such a flash, modify the
complete_account_setup action to set flash[:notice] with a message or expose a
skipped_count (e.g. `@skipped_count`) and assert that value in the test instead
(referencing complete_account_setup_brex_item_url, unsupported_brex_account, and
AccountProvider.count).
In `@test/models/brex_account/transactions/processor_test.rb`:
- Around line 24-34: Add two more tests for
BrexAccount::Transactions::Processor#process: one "happy-path" that sets up a
linked account and verifies result[:success] true, result[:total] increments,
result[:imported] increments, result[:skipped] and result[:failed] are zero, and
skipped_transactions/errors are empty; and one "partial-failure" test that rigs
one entry to raise (or simulate a failed import) while others succeed and then
asserts aggregated counts (total, imported, skipped, failed), presence of an
error in result[:errors], and that the failed entry is represented appropriately
in result[:failed] or result[:errors]; use the same result keys
(total/imported/skipped/failed/errors/skipped_transactions) and
BrexAccount::Transactions::Processor.new(`@brex_account`).process to exercise the
aggregation contract.
In `@test/models/brex_item/importer_test.rb`:
- Around line 25-47: The provider expectation for get_cash_transactions uses a
loose matcher (anything) for start_date; tighten it to expect the exact sync
start date passed to BrexItem::Importer so the test verifies threading of
sync_start_date through discovery. Update the mock expectation on
provider.get_cash_transactions to use start_date: Date.new(2026, 1, 1) (matching
the sync_start_date argument passed to BrexItem::Importer.new) instead of
anything; keep the rest of the expectation and assertions unchanged.
🪄 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: 5056605e-3e60-4421-9eee-a28f3546a57f
📒 Files selected for processing (34)
app/components/DS/dialog.rbapp/controllers/accounts_controller.rbapp/controllers/brex_items/account_flows_controller.rbapp/controllers/brex_items/account_setups_controller.rbapp/helpers/brex_items_helper.rbapp/models/brex_account.rbapp/models/brex_account/processor.rbapp/models/brex_account/transactions/processor.rbapp/models/brex_entry/processor.rbapp/models/brex_item.rbapp/models/brex_item/account_flow.rbapp/models/brex_item/importer.rbapp/models/credit_card.rbapp/models/depository.rbapp/models/family/brex_connectable.rbapp/models/provider/brex.rbapp/models/provider/brex_adapter.rbapp/views/accounts/index.html.erbapp/views/brex_items/_brex_item.html.erbapp/views/brex_items/select_accounts.html.erbapp/views/settings/providers/_brex_panel.html.erbapp/views/settings/providers/show.html.erbconfig/locales/defaults/en.ymlconfig/locales/views/brex_items/en.ymlconfig/locales/views/settings/en.ymldb/migrate/20260505010000_create_brex_items_and_accounts.rbdb/schema.rbtest/controllers/brex_items_controller_test.rbtest/models/brex_account/transactions/processor_test.rbtest/models/brex_account_test.rbtest/models/brex_entry/processor_test.rbtest/models/brex_item/account_flow_test.rbtest/models/brex_item/importer_test.rbtest/models/provider/brex_test.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- app/views/settings/providers/_brex_panel.html.erb
- app/models/brex_item/importer.rb
- test/models/provider/brex_test.rb
- app/models/provider/brex.rb
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
app/models/brex_item/account_flow.rb (1)
67-83: ⚡ Quick winReplace the
exist?+readpair with a single cache read.
Rails.cache.exist?(cache_key)followed byaccounts(which callsRails.cache.readinternally on line 558) issues two cache lookups perpreload_payload. Beyond the extra round-trip, there's a small TOCTOU window where the entry can expire betweenexist?andread: in that caseaccountswill re-fetch from the Brex API and rewrite the cache, but the payload still reportscached: true. Reading once and derivingcachedfrom the result fixes both the inefficiency and the reporting drift.♻️ Suggested refactor
def preload_payload return selection_error_payload if !selected? return { success: false, error: "no_credentials", has_accounts: false } unless brex_item&.credentials_configured? - cached = Rails.cache.exist?(cache_key) - available_accounts = accounts + available_accounts = Rails.cache.read(cache_key) + cached = !available_accounts.nil? + unless cached + available_accounts = fetch_accounts + Rails.cache.write(cache_key, available_accounts, expires_in: CACHE_TTL) + end { success: true, has_accounts: available_accounts.any?, cached: cached }Note: the existing
accountshelper can stay for other call sites (unlinked_available_accounts,indexed_accounts) that don't care about cache-hit reporting.🤖 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/brex_item/account_flow.rb` around lines 67 - 83, Replace the two cache lookups by doing a single Rails.cache.read(cache_key) inside preload_payload: call cached_value = Rails.cache.read(cache_key), set cached = !cached_value.nil? (or cached_value.present? if you want to treat empty arrays as not-cached), then set available_accounts = cached_value || accounts (so accounts is only called when the cache miss happens). Keep using the existing accounts helper for the fetch/path that writes the cache; update the payload to return cached based on the single read result.app/views/brex_items/select_existing_account.html.erb (1)
24-24: 💤 Low valueUse a functional surface token instead of the raw
bg-red-tint-5color tint.The code pairs functional tokens (
border-destructive,text-destructive) with a raw color tint token (bg-red-tint-5), which violates the design system guideline: "Always prefer using functional tokens defined in sure-design-system.css rather than raw Tailwind utilities or arbitrary colors."The design system currently lacks a destructive surface token. Replace
bg-red-tint-5with an existing functional surface token (e.g.,bg-surface,bg-container) or add a new destructive surface utility (e.g.,bg-destructive-subtle) to the design system for consistency.This pattern appears in both
select_existing_account.html.erbandselect_accounts.html.erbat line 24, so a single fix covers both.🤖 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/brex_items/select_existing_account.html.erb` at line 24, Replace the raw color token bg-red-tint-5 used in the label class conditional with a functional surface token (e.g., bg-destructive-subtle or bg-surface/bg-container) so the destructive state uses design-system semantics; update the conditional in the label for account_display.blank_name? inside select_existing_account.html.erb (and the identical spot in select_accounts.html.erb) to use the chosen functional utility (e.g., change "bg-red-tint-5" to "bg-destructive-subtle" or "bg-surface") and if your design system lacks a destructive surface token, add a new utility (bg-destructive-subtle) to sure-design-system.css and use that class in the label conditional.app/models/brex_item.rb (1)
86-88: 💤 Low valueEncryption inconsistency:
BrexItem.raw_payloadvsBrexAccount.raw_payload.
BrexAccountdeclaresencrypts :raw_payload, butBrexItem.raw_payloadis stored in plaintext. Both callBrexAccount.sanitize_payloadto redact PII/secrets, so the immediate exposure is limited—but the asymmetry is surprising and a future change tosanitize_payload(or a missed sensitive key) loses the encryption-at-rest defense in depth on the parent record. Either encrypt both or document why the parent snapshot is intentionally plaintext.🤖 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/brex_item.rb` around lines 86 - 88, BrexItem currently stores raw_payload plaintext in upsert_brex_snapshot! while BrexAccount declares encrypts :raw_payload; to fix, add symmetric encryption on the parent model by declaring encrypts :raw_payload in the BrexItem model (so BrexItem.upsert_brex_snapshot! continues to call BrexAccount.sanitize_payload but the stored snapshot is encrypted at rest), or if intentional, add an explicit comment in BrexItem documenting why raw_payload is stored unencrypted and reference BrexAccount.sanitize_payload and the threat model.app/models/brex_item/syncer.rb (3)
50-50: ⚡ Quick winN+1 query:
current_accountnot preloaded.
current_accountdelegates to theaccountassociation, which ishas_one :account, through: :account_provider, source: :account.includes(:account_provider)only preloadsaccount_provider, not theaccountreachable through it. Eachma.current_account&.idcall triggers an additional query per linked account.Proposed fix to preload through the association chain
- account_ids = linked_accounts.includes(:account_provider).filter_map { |ma| ma.current_account&.id } + account_ids = linked_accounts.includes(account_provider: :account).filter_map { |ma| ma.current_account&.id }🤖 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/brex_item/syncer.rb` at line 50, The N+1 stems from calling ma.current_account on each linked_account while only preloading :account_provider; update the preload to include the through association so current_account is eager loaded (e.g. change the includes call on linked_accounts from includes(:account_provider) to include the account via the provider such as includes(account_provider: :account) or the direct association name if defined), then keep the filter_map { |ma| ma.current_account&.id } unchanged so no extra queries are issued.
22-31: 💤 Low valueMinor: redundant queries on
unlinked_accounts.
unlinked_accounts.any?(line 26) andunlinked_accounts.count(line 28) each fire their own LEFT JOIN query. Since the count is used both for branching and the status text, materializing once would halve the round-trips. Same pattern repeats forlinked_accounts.any?/linked_accounts.counton lines 34/37.Proposed micro-optimization
- if unlinked_accounts.any? + unlinked_count = unlinked_accounts.count + if unlinked_count.positive? brex_item.update!(pending_account_setup: true) - sync.update!(status_text: "#{unlinked_accounts.count} accounts need setup...") if sync.respond_to?(:status_text) + sync.update!(status_text: "#{unlinked_count} accounts need setup...") if sync.respond_to?(:status_text) else🤖 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/brex_item/syncer.rb` around lines 22 - 31, Compute and cache the counts for linked_accounts and unlinked_accounts once instead of calling any? and count separately: fetch brex_item.brex_accounts.joins(:account_provider).count into a variable (e.g., linked_count) and brex_item.brex_accounts.left_joins(:account_provider).where(account_providers: { id: nil }).count into unlinked_count, then use those cached integers for the branching (if unlinked_count > 0 / else) and for sync.update!(status_text: "#{unlinked_count} accounts need setup...") as well as for the linked_accounts logic; update pending_account_setup on brex_item and status_text on sync using these counts to avoid duplicate DB queries.
28-28: ⚡ Quick wini18n: hardcoded user-facing status strings.
Status text strings like
"Importing accounts from Brex...","Checking account configuration...","#{unlinked_accounts.count} accounts need setup...","Processing transactions...", and"Calculating balances..."are hardcoded throughoutperform_sync. These are user-visible (rendered in the UI viasync.status_text) and should go throughI18n.tto match the localization pattern used elsewhere in the file (e.g.,user_safe_error_messagealready usesI18n.t("brex_items.syncer.*")).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/models/brex_item/syncer.rb` at line 28, The hardcoded user-facing status strings in perform_sync (e.g., the arguments passed to sync.update!(status_text: ...)) must be replaced with I18n lookup calls; locate perform_sync in BrexItem::Syncer and replace each literal like "Importing accounts from Brex...", "Checking account configuration...", "#{unlinked_accounts.count} accounts need setup...", "Processing transactions...", and "Calculating balances..." with t(...) calls (e.g., t("brex_items.syncer.importing_accounts") etc.), passing dynamic values (like unlinked_accounts.count) as interpolation locals to I18n and ensure the same translation key namespace used by user_safe_error_message is used so the UI-texts are localizable.
🤖 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/views/brex_items/select_existing_account.html.erb`:
- Around line 48-50: Add parameterized tests that mirror the existing return_to
validation coverage for select_accounts but target the select_existing_account
action: test that unsafe return_to values (protocol-relative URLs like //evil,
backslash-containing paths like \evil, percent-encoded hosts, whitespace-only
values, and a solitary "/") are rejected and that safe_return_to_path
sanitization is applied; use the same test helper/approach as the
select_accounts suite to assert the response redirects to a safe fallback
(`@return_to` or accounts_path) and does not permit external hosts or malformed
paths, referencing the select_existing_account controller action and the
safe_return_to_path behavior in your assertions.
In `@db/migrate/20260505010000_create_brex_items_and_accounts.rb`:
- Around line 5-28: The migration should enforce non-null at the DB level for
brex_items.name and brex_items.token: modify the create_table block (brex_items)
to declare t.string :name, null: false and t.text :token, null: false; if this
migration has already been run in production instead create a new migration that
uses change_column_null :brex_items, :name, false and change_column_null
:brex_items, :token, false (and ensure any existing NULLs are backfilled before
flipping the constraint).
---
Nitpick comments:
In `@app/models/brex_item.rb`:
- Around line 86-88: BrexItem currently stores raw_payload plaintext in
upsert_brex_snapshot! while BrexAccount declares encrypts :raw_payload; to fix,
add symmetric encryption on the parent model by declaring encrypts :raw_payload
in the BrexItem model (so BrexItem.upsert_brex_snapshot! continues to call
BrexAccount.sanitize_payload but the stored snapshot is encrypted at rest), or
if intentional, add an explicit comment in BrexItem documenting why raw_payload
is stored unencrypted and reference BrexAccount.sanitize_payload and the threat
model.
In `@app/models/brex_item/account_flow.rb`:
- Around line 67-83: Replace the two cache lookups by doing a single
Rails.cache.read(cache_key) inside preload_payload: call cached_value =
Rails.cache.read(cache_key), set cached = !cached_value.nil? (or
cached_value.present? if you want to treat empty arrays as not-cached), then set
available_accounts = cached_value || accounts (so accounts is only called when
the cache miss happens). Keep using the existing accounts helper for the
fetch/path that writes the cache; update the payload to return cached based on
the single read result.
In `@app/models/brex_item/syncer.rb`:
- Line 50: The N+1 stems from calling ma.current_account on each linked_account
while only preloading :account_provider; update the preload to include the
through association so current_account is eager loaded (e.g. change the includes
call on linked_accounts from includes(:account_provider) to include the account
via the provider such as includes(account_provider: :account) or the direct
association name if defined), then keep the filter_map { |ma|
ma.current_account&.id } unchanged so no extra queries are issued.
- Around line 22-31: Compute and cache the counts for linked_accounts and
unlinked_accounts once instead of calling any? and count separately: fetch
brex_item.brex_accounts.joins(:account_provider).count into a variable (e.g.,
linked_count) and
brex_item.brex_accounts.left_joins(:account_provider).where(account_providers: {
id: nil }).count into unlinked_count, then use those cached integers for the
branching (if unlinked_count > 0 / else) and for sync.update!(status_text:
"#{unlinked_count} accounts need setup...") as well as for the linked_accounts
logic; update pending_account_setup on brex_item and status_text on sync using
these counts to avoid duplicate DB queries.
- Line 28: The hardcoded user-facing status strings in perform_sync (e.g., the
arguments passed to sync.update!(status_text: ...)) must be replaced with I18n
lookup calls; locate perform_sync in BrexItem::Syncer and replace each literal
like "Importing accounts from Brex...", "Checking account configuration...",
"#{unlinked_accounts.count} accounts need setup...", "Processing
transactions...", and "Calculating balances..." with t(...) calls (e.g.,
t("brex_items.syncer.importing_accounts") etc.), passing dynamic values (like
unlinked_accounts.count) as interpolation locals to I18n and ensure the same
translation key namespace used by user_safe_error_message is used so the
UI-texts are localizable.
In `@app/views/brex_items/select_existing_account.html.erb`:
- Line 24: Replace the raw color token bg-red-tint-5 used in the label class
conditional with a functional surface token (e.g., bg-destructive-subtle or
bg-surface/bg-container) so the destructive state uses design-system semantics;
update the conditional in the label for account_display.blank_name? inside
select_existing_account.html.erb (and the identical spot in
select_accounts.html.erb) to use the chosen functional utility (e.g., change
"bg-red-tint-5" to "bg-destructive-subtle" or "bg-surface") and if your design
system lacks a destructive surface token, add a new utility
(bg-destructive-subtle) to sure-design-system.css and use that class in the
label conditional.
🪄 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: 52be0c02-c9f4-4537-8974-90311609657e
📒 Files selected for processing (23)
app/controllers/brex_items/account_flows_controller.rbapp/controllers/brex_items/account_setups_controller.rbapp/models/brex_account.rbapp/models/brex_entry/processor.rbapp/models/brex_item.rbapp/models/brex_item/account_flow.rbapp/models/brex_item/syncer.rbapp/models/provider/brex_adapter.rbapp/views/accounts/index.html.erbapp/views/brex_items/_brex_item.html.erbapp/views/brex_items/select_accounts.html.erbapp/views/brex_items/select_existing_account.html.erbconfig/locales/views/brex_items/en.ymldb/migrate/20260505010000_create_brex_items_and_accounts.rbdb/schema.rbtest/controllers/brex_items_controller_test.rbtest/models/brex_account/transactions/processor_test.rbtest/models/brex_account_test.rbtest/models/brex_entry/processor_test.rbtest/models/brex_item/account_flow_test.rbtest/models/brex_item/importer_test.rbtest/models/brex_item/syncer_test.rbtest/models/brex_item_test.rb
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/models/brex_item/account_flow.rb (2)
297-347: ⚡ Quick winN+1 in
complete_setup!— preload brex_accounts with theiraccount_providerassociation.The loop issues two queries per submitted account (one
find_by+ oneaccount_provider.present?association load), totalling 2N queries. Preloading avoids this:♻️ Proposed refactor
def complete_setup!(account_types:, account_subtypes:) created_accounts = [] skipped_count = 0 valid_types = Provider::BrexAdapter.supported_account_types failed_count = 0 + brex_accounts_by_id = brex_item.brex_accounts + .includes(:account_provider) + .where(id: account_types.keys) + .index_by { |a| a.id.to_s } account_types.each do |brex_account_id, selected_type| ... - brex_account = brex_item.brex_accounts.find_by(id: brex_account_id) + brex_account = brex_accounts_by_id[brex_account_id.to_s] unless brex_account🤖 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/brex_item/account_flow.rb` around lines 297 - 347, The loop in complete_setup! is causing an N+1 by calling brex_item.brex_accounts.find_by(...) and then checking brex_account.account_provider.present? for each entry; preload the brex_accounts with their account_provider association once before iterating (e.g., load the submitted accounts via brex_item.brex_accounts.where(id: account_types.keys).includes(:account_provider) and index them by id) and then use that in the loop instead of calling find_by each time so the account_provider check does not fire extra queries.
106-116: ⚡ Quick win
link_new_accounts_resultandlink_existing_account_resultleaveStandardErrorunhandled — inconsistent withselection_result_for.Both write-path methods rescue only known domain errors. An unexpected
ActiveRecord::RecordInvalid(e.g., fromAccount.create_and_syncorAccountProvider.create!) propagates uncaught through these methods to the controller, resulting in a 500 rather than a user-facing alert.selection_result_for(Line 444) already rescuesStandardError—the same defensive pattern should be applied here for consistency.♻️ Proposed fix
def link_new_accounts_result(account_ids:, accountable_type:) ... link_navigation_result(link_new_accounts!(account_ids: account_ids, accountable_type: accountable_type)) rescue NoApiTokenError navigation(:new_account, :alert, I18n.t("brex_items.link_accounts.no_api_token")) rescue Provider::Brex::BrexError => e navigation(:new_account, :alert, I18n.t("brex_items.link_accounts.api_error", message: e.message)) + rescue StandardError => e + Rails.logger.error("Unexpected error linking Brex accounts: #{e.class}: #{e.message}") + navigation(:new_account, :alert, I18n.t("brex_items.errors.unexpected_error")) end def link_existing_account_result(account:, brex_account_id:) ... rescue Provider::Brex::BrexError => e navigation(:accounts, :alert, I18n.t("brex_items.link_existing_account.api_error", message: e.message)) + rescue StandardError => e + Rails.logger.error("Unexpected error linking existing Brex account: #{e.class}: #{e.message}") + navigation(:accounts, :alert, I18n.t("brex_items.errors.unexpected_error")) endAlso applies to: 118-136
🤖 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/brex_item/account_flow.rb` around lines 106 - 116, The methods link_new_accounts_result and link_existing_account_result currently only rescue domain errors and let unexpected exceptions (e.g., ActiveRecord::RecordInvalid) bubble up; add a catch-all rescue StandardError (as selection_result_for does) to each method so unexpected errors are converted to a user-facing navigation alert (use the same I18n key/pattern used for API errors and include the exception message), ensuring any ActiveRecord::RecordInvalid or other StandardError gets handled and returns a navigation(...) instead of raising to the controller.test/controllers/brex_items_controller_test.rb (1)
141-142: ⚡ Quick winAssert translated flash copy via I18n instead of English literals.
These assertions are checking user-facing copy, so harmless translation or wording updates will fail the suite even when controller behavior is unchanged. Prefer
I18n.t(...)in the expectations.As per coding guidelines "Always use
t()helper for user-facing strings."Also applies to: 268-268, 337-338, 370-370, 383-383, 430-430
🤖 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/controllers/brex_items_controller_test.rb` around lines 141 - 142, Change assertions that compare flash text to hard-coded English to use the I18n helper instead; e.g. replace assert_equal "Choose a Brex connection in Provider Settings.", flash[:alert] with assert_equal t('choose_brex_connection_in_provider_settings', default: "Choose a Brex connection in Provider Settings."), flash[:alert] (and do the same replacement for the other occurrences called out). Use t(...) (or I18n.t(...)) with an appropriate translation key and a default that matches the current English copy so tests remain stable while allowing future translations.
🤖 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/models/brex_item.rb`:
- Around line 13-17: The model currently strips whitespace in normalize_token
before validation, allowing persisted records to accept updates like
update!(token: " ") because presence: true is only enforced on :create; add a
model-level validation that prevents blanking the token on updates by checking
for token changes and rejecting blank values: implement a custom validation
method (e.g., token_cannot_be_blank_when_changed) that runs after
normalize_token and calls errors.add(:token, "...") when token_changed? &&
token.blank? (retain the existing validates :token, presence: true, on: :create
and the before_validation :normalize_token hook).
In `@app/models/brex_item/syncer.rb`:
- Around line 15-16: perform_sync is marking syncs healthy unconditionally
because it ignores return values from BrexItem::Importer#import
(brex_item.import_latest_brex_data), BrexItem#process_accounts and
BrexItem#schedule_account_syncs which can return success: false or per-item
failures; update perform_sync to capture and aggregate those results (check the
return value of brex_item.import_latest_brex_data and the hashes/arrays returned
by process_accounts and schedule_account_syncs), convert failures into an errors
array or boolean, and pass that into collect_health_stats(sync, errors: ...) so
partial/complete failures are propagated before marking the sync healthy.
---
Nitpick comments:
In `@app/models/brex_item/account_flow.rb`:
- Around line 297-347: The loop in complete_setup! is causing an N+1 by calling
brex_item.brex_accounts.find_by(...) and then checking
brex_account.account_provider.present? for each entry; preload the brex_accounts
with their account_provider association once before iterating (e.g., load the
submitted accounts via brex_item.brex_accounts.where(id:
account_types.keys).includes(:account_provider) and index them by id) and then
use that in the loop instead of calling find_by each time so the
account_provider check does not fire extra queries.
- Around line 106-116: The methods link_new_accounts_result and
link_existing_account_result currently only rescue domain errors and let
unexpected exceptions (e.g., ActiveRecord::RecordInvalid) bubble up; add a
catch-all rescue StandardError (as selection_result_for does) to each method so
unexpected errors are converted to a user-facing navigation alert (use the same
I18n key/pattern used for API errors and include the exception message),
ensuring any ActiveRecord::RecordInvalid or other StandardError gets handled and
returns a navigation(...) instead of raising to the controller.
In `@test/controllers/brex_items_controller_test.rb`:
- Around line 141-142: Change assertions that compare flash text to hard-coded
English to use the I18n helper instead; e.g. replace assert_equal "Choose a Brex
connection in Provider Settings.", flash[:alert] with assert_equal
t('choose_brex_connection_in_provider_settings', default: "Choose a Brex
connection in Provider Settings."), flash[:alert] (and do the same replacement
for the other occurrences called out). Use t(...) (or I18n.t(...)) with an
appropriate translation key and a default that matches the current English copy
so tests remain stable while allowing future translations.
🪄 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: afbea875-0d41-4575-96df-fc1149c29e98
📒 Files selected for processing (14)
app/assets/tailwind/sure-design-system/_generated.cssapp/models/brex_item.rbapp/models/brex_item/account_flow.rbapp/models/brex_item/syncer.rbapp/views/brex_items/select_accounts.html.erbapp/views/brex_items/select_existing_account.html.erbconfig/locales/views/brex_items/en.ymldb/migrate/20260505010000_create_brex_items_and_accounts.rbdb/schema.rbdesign/tokens/sure.tokens.jsontest/controllers/brex_items_controller_test.rbtest/models/brex_item/account_flow_test.rbtest/models/brex_item/syncer_test.rbtest/models/brex_item_test.rb
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
config/locales/views/brex_items/en.yml (1)
165-166: 💤 Low valueVendor-prefixed internal keys under
link_existing_account.Keys
brex_account_already_linkedandbrex_account_not_foundembed the vendor name in the YAML key itself, which is already namespaced underen.brex_items. Per the project convention, internal keys (YAML keys, class names, log identifiers) should not carry vendor branding — only user-facing values should. Considerprovider_account_already_linked/provider_account_not_foundfor consistency.Based on learnings from jjmata (repo: we-promise/sure PR: 464): "In YAML locale files, ensure vendor branding appears only in end-user visible strings (UI text, translations, user-facing messages). Internal keys such as logs, class names, and variable identifiers should not include the vendor branding."
🤖 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 `@config/locales/views/brex_items/en.yml` around lines 165 - 166, Rename the vendor-prefixed locale keys under en.brex_items.link_existing_account from brex_account_already_linked and brex_account_not_found to provider_account_already_linked and provider_account_not_found, keeping the human-readable values identical; then update all code/partials/logs that reference the old keys (search for brex_account_already_linked / brex_account_not_found) to use the new provider_* keys so internal identifiers no longer include the vendor name while user-facing strings remain unchanged.test/controllers/brex_items_controller_test.rb (1)
448-450: ⚡ Quick winConsider delegating to
BrexItem::AccountFlow.cache_keyto avoid key-format drift.The test helper replicates the same string interpolation used in the production class. If the cache key format ever changes in
BrexItem::AccountFlow, this helper silently drifts and mock expectations stop matching the real calls.♻️ Proposed refactor
def brex_cache_key(brex_item) - "brex_accounts_#{`@family.id`}_#{brex_item.id}" + BrexItem::AccountFlow.cache_key(`@family`, brex_item) 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 `@test/controllers/brex_items_controller_test.rb` around lines 448 - 450, Replace the manual string interpolation in the test helper method brex_cache_key so it calls the canonical implementation BrexItem::AccountFlow.cache_key(brex_item, `@family`) (or the appropriate argument order used in production) instead of constructing "brex_accounts_#{`@family.id`}_#{brex_item.id}" directly; this ensures the test uses the same cache key format as the production class and prevents drift if BrexItem::AccountFlow.cache_key changes.
🤖 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/models/brex_item/account_flow.rb`:
- Around line 116-119: Remove user-exposed exception text from the rescue
handlers in link_new_accounts_result and link_existing_account_result: instead
of interpolating e.message into the i18n alert, use the same generic i18n
key/pattern used by other rescue blocks in this file (i.e., call navigation(...,
I18n.t("brex_items.link_accounts.api_error")) or the existing generic key used
elsewhere) so no raw exception text is shown; make the same change for the
duplicate rescue at the other block (the one around lines 140–143) so all
catch-all StandardError handlers follow the same generic, non-verbose
user-facing message pattern.
In `@config/locales/views/brex_items/en.yml`:
- Line 61: The locale string partial_invalid is missing the noun after
%{already_linked_count}; update the value for partial_invalid to include the
noun (e.g., "%{already_linked_count} account(s) were already linked") so it
matches the other interpolations and renders grammatically correct; locate the
partial_invalid key in the file and edit the string to append "account(s)" after
%{already_linked_count}.
- Line 112: Update the inaccurate locale string under the key
default_description in the en.yml locale for brex_items: replace "Connect to
your bank via Brex" with a wording that does not call Brex a bank (for example,
"Connect to your Brex account" or "Connect to Brex to manage corporate spend and
payments") so the description accurately reflects Brex as a financial/corporate
spend platform.
---
Nitpick comments:
In `@config/locales/views/brex_items/en.yml`:
- Around line 165-166: Rename the vendor-prefixed locale keys under
en.brex_items.link_existing_account from brex_account_already_linked and
brex_account_not_found to provider_account_already_linked and
provider_account_not_found, keeping the human-readable values identical; then
update all code/partials/logs that reference the old keys (search for
brex_account_already_linked / brex_account_not_found) to use the new provider_*
keys so internal identifiers no longer include the vendor name while user-facing
strings remain unchanged.
In `@test/controllers/brex_items_controller_test.rb`:
- Around line 448-450: Replace the manual string interpolation in the test
helper method brex_cache_key so it calls the canonical implementation
BrexItem::AccountFlow.cache_key(brex_item, `@family`) (or the appropriate argument
order used in production) instead of constructing
"brex_accounts_#{`@family.id`}_#{brex_item.id}" directly; this ensures the test
uses the same cache key format as the production class and prevents drift if
BrexItem::AccountFlow.cache_key changes.
🪄 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: 6f2b00e4-ba7b-4fb1-bb27-d9e18ba4a9c0
📒 Files selected for processing (8)
app/models/brex_item.rbapp/models/brex_item/account_flow.rbapp/models/brex_item/syncer.rbconfig/locales/views/brex_items/en.ymltest/controllers/brex_items_controller_test.rbtest/models/brex_item/account_flow_test.rbtest/models/brex_item/syncer_test.rbtest/models/brex_item_test.rb
|
(apologies for the bot/AI slop noise, removing some comments here and in other PRs as wel speak ...) |
brin-security-scanner
Bot
added
contributor:verified
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
|
Merge from main received (May 6). jjmata's review raised two items that need addressing before this can move forward:
Also: there's still no explicit confirmation from @jjmata that Brex is on the product roadmap. Getting that answer first will avoid wasted review cycles if the answer is no. Code review note from Claude Code Generated by Claude Code |
I've addressed these issues as well. |
|
New commits pushed 2026-05-07. A few issues that should be resolved before this enters formal review:
Recommend either returning to draft until the design token question is resolved with @sure-design, or splitting out the design token change as a prerequisite. Generated by Claude Code Generated by Claude Code |
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
|
We either merge this real quick before #1710 lands or I foresee a lot of conflicts, @JSONbored? Actually, can you merge that branch into yours locally just to see how bad it will be? |
3 participants
Summary
Feature Overview
This PR adds Brex as a new read-only financial provider in Sure, following the same post-Mercury multi-connection pattern already used for Mercury.
Users can add one or more Brex API token connections, discover available Brex cash/card accounts, select which accounts to link, and sync Brex balances and transactions into Sure. Cash accounts are imported as depository accounts, while Brex card activity is synced into an aggregate credit card account per Brex connection.
The implementation includes per-connection account discovery, explicit connection selection when multiple Brex credentials exist, scoped duplicate upstream account IDs, per-item cache isolation, credential preservation on blank edits, token stripping/rejection rules, and a Brex API URL allowlist.
This is intentionally read-only and does not add any public API/OpenAPI changes.
What changed
Why
Validation
109 runs, 389 assertions.bin/rails zeitwerk:checkpassed.bin/rubocoppassed.bin/brakeman --no-pagerpassed.git diff --checkpassed.bin/rails testcompleted with3599 runs, 14666 assertions, 0 failures, 1 errors, 23 skips; the single error is the known unrelated exact-devcontainerlibvips.so.42image-processing failure.app/controllers/api/v1,spec/requests/api/v1, ordocs/api/openapi.yamlchanges were made, so rswag/OpenAPI regeneration was not run.Screenshots
Screenshots were captured with headless Chromium. Local artifact links for manual attachment/review:
Provider settings
Expanded Brex provider panel
Edit Brex connection
Add Brex connection
Explicit connection required
Accounts index with Brex groups
Brex setup accounts modal
New account provider choice
Notes
1.0, while the supported account/transaction paths used here are/v2/...; this PR does not add user-selectable API versions or arbitrary endpoint paths.Summary by CodeRabbit
New Features
Documentation / Localization
Style