feat(provider): unified provider framework + TrueLayer integration#1662
Closed
thewillwilson wants to merge 10 commits intowe-promise:mainfrom
Closed
feat(provider): unified provider framework + TrueLayer integration#1662thewillwilson wants to merge 10 commits intowe-promise:mainfrom
thewillwilson wants to merge 10 commits intowe-promise:mainfrom
Conversation
brin-security-scanner
Bot
added
the
contributor:flagged
Contributor
|
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 a provider OAuth/connect framework and a TrueLayer integration: DB tables and models for family configs, connections, and provider accounts; OAuth auth layer, registry, TrueLayer client/adapter/syncer; admin controllers, routes, views, locale strings, a consent-expiry job and Sidekiq cron registration, plus tests and fixtures. ChangesProvider OAuth + TrueLayer integration
Sequence Diagram(s)sequenceDiagram
participant Admin
participant Settings
participant OAuthCB as OAuthCallbacksController
participant Registry as Provider::Registry
participant Auth as Provider::Auth::OAuth2
participant Connection as Provider::Connection
participant Syncer as Provider::TruelayerSyncer
participant TrueLayer as Provider::Truelayer API
Admin->>Settings: click "Add connection"
Settings->>OAuthCB: POST /connect/:provider (new)
OAuthCB->>Registry: lookup oauth adapter/config
OAuthCB->>Connection: create pending connection (store redirect_uri, psu_ip, state)
OAuthCB->>Admin: redirect to provider authorize_url (state)
Admin->>TrueLayer: grant consent (OAuth)
TrueLayer->>OAuthCB: callback with code & state
OAuthCB->>Auth: exchange_code(code)
Auth->>TrueLayer: POST /connect/token
TrueLayer->>Auth: return tokens + consent_expires_at
Auth->>Connection: store tokens & metadata
OAuthCB->>Syncer: discover_accounts!
Syncer->>TrueLayer: GET accounts/cards
TrueLayer->>Syncer: return account/card payloads
Syncer->>Connection: upsert provider_accounts
Admin->>ProviderConnectionsController: submit mappings
ProviderConnectionsController->>Connection: link/create accounts (transaction)
ProviderConnectionsController->>Syncer: queue sync_later
Syncer->>Auth: fresh_access_token
Syncer->>TrueLayer: GET transactions (settled + pending)
TrueLayer->>Syncer: return transactions
Syncer->>Application: import transactions, anchor balance, update status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
brin-security-scanner
Bot
added
the
pr:flagged
Brin PR Security ScanThis PR has findings that should be reviewed.
Findings:
Analyzed by Brin |
![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: d208346a77
ℹ️ 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".
Comment on lines
+19
to
+21
| <% else %> | ||
| <span class="inline-flex items-center gap-1 px-2 py-0.5 rounded-full text-xs font-medium bg-success/10 text-success"> | ||
| <%= t("settings.providers.oauth_provider_panel.status_good") %> |
There was a problem hiding this comment.
This branch marks every non-requires_update connection as status_good, but callback failures explicitly persist connections as status: :disconnected in OauthCallbacksController#create (connection&.update!(status: :disconnected, ...)). In that state the settings panel will still show a green “good” badge (and no reconnect CTA), which misleads admins after failed OAuth completion and can hide the need for corrective action.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,82 @@ | |||
| <%# locals: (connection:) %> | |||
|
|
|||
| <% linked_accounts = connection.provider_accounts.filter_map(&:account).select(&:active?) %> | |||
There was a problem hiding this comment.
linked_accounts = connection.provider_accounts.filter_map(&:account) will issue one query per provider account unless account is preloaded, and this partial is rendered for each connection on the accounts index. With multiple linked institutions/accounts this becomes an N+1 query pattern that can significantly slow the page; preload provider_accounts: :account (or otherwise avoid per-row association fetches) before rendering.
Useful? React with 👍 / 👎.
thewillwilson
force-pushed
the
feat/unified-provider-framework
branch
from
631d339 to
582e9e6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (8)
app/views/settings/bank_sync/_provider_link.html.erb (1)
15-16: ⚡ Quick winLocalize the beta badge text.
This is user-facing copy in an ERB template, so it should go through
t()instead of being hard-coded.Suggested fix
- <span class="px-1.5 py-0.5 rounded text-xs font-medium bg-warning/10 text-warning">beta</span> + <span class="px-1.5 py-0.5 rounded text-xs font-medium bg-warning/10 text-warning"><%= t("settings.bank_sync.providers.beta") %></span>As per coding guidelines
**/*.{erb,rb}: Organize i18n keys hierarchically by feature:accounts.index.title,transactions.form.amount_label. Always uset()helper for user-facing strings. Updateconfig/locales/en.ymlfor new strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/settings/bank_sync/_provider_link.html.erb` around lines 15 - 16, Replace the hard-coded "beta" badge text in _provider_link.html.erb (the provider_link[:beta] branch) with a call to the Rails i18n helper t(), e.g. t('settings.bank_sync.provider_link.beta'), and add that key to config/locales/en.yml under the hierarchical feature namespace (settings -> bank_sync -> provider_link -> beta) with the "beta" value; ensure you use the same key in the template so the UI text is localized.app/controllers/concerns/accountable_resource.rb (1)
88-92: ⚡ Quick winAdd a nil guard for
adapterbefore calling methods on it.
Provider::Registry.oauth_provider_adapter(key)could theoretically returnnilif the registry is ever in an inconsistent state (e.g., keys and adapters registered separately). Without a guard,adapter.supported_account_typeson Line 90 would raiseNoMethodErrorinside theset_link_optionsbefore_action, producing a 500 for every accountable-resourcenewroute.🛡️ Proposed defensive fix
`@provider_configs` += Provider::Registry.oauth_provider_keys.flat_map do |key| adapter = Provider::Registry.oauth_provider_adapter(key) - next [] unless adapter.supported_account_types.include?(account_type_name) + next [] unless adapter&.supported_account_types&.include?(account_type_name) adapter.connection_configs(family: Current.family) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/concerns/accountable_resource.rb` around lines 88 - 92, The code calls Provider::Registry.oauth_provider_adapter(key) and then invokes adapter.supported_account_types and adapter.connection_configs without guarding for a nil adapter; update the block that builds `@provider_configs` to skip when adapter is nil (e.g., next [] or next unless adapter) before calling adapter.supported_account_types, ensuring adapter is non-nil before using supported_account_types and connection_configs (which uses Current.family and account_type_name) so set_link_options/new routes cannot raise NoMethodError.app/views/settings/providers/_oauth_provider_panel.html.erb (1)
2-4: ⚖️ Poor tradeoffMove database queries out of the view partial.
Lines 2–4 run two ActiveRecord queries (one
find_by, onewhere(...).includes(...)) directly in the partial, which is rendered once per registered OAuth provider. This violates the "keep domain logic out of views" guideline and multiplies queries with each additional provider.Move the data loading to the controller (or a presenter) and pass the resolved
configandconnectionsas locals to the partial.As per coding guidelines: "Avoid heavy logic in ERB views; prefer helpers and components."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/settings/providers/_oauth_provider_panel.html.erb` around lines 2 - 4, The view partial runs ActiveRecord queries (Current.family.provider_family_configs.find_by and Current.family.provider_connections.where(...).includes(...)) for each provider; move these queries into the controller action or a presenter and resolve `config` and `connections` there, then pass them into the partial as locals (e.g., render partial: 'oauth_provider_panel', locals: { config: config, connections: connections }) so the partial only uses the provided `config` and `connections` variables and does not perform any DB lookups itself.app/views/provider_connections/_connection_card.html.erb (1)
46-51: Consider usingbutton_todirectly for consistency, but the currenticonapproach will work correctly.While the
iconhelper withas_button: trueandhref:is functional (it internally callsbutton_to, which defaults to POST), the pattern inshow.html.erbdemonstrates the more explicit and idiomatic approach. Usebutton_todirectly to align with the established pattern elsewhere in this file:🔧 Suggested improvement
-<%= icon( - "refresh-cw", - as_button: true, - href: sync_provider_connection_path(connection), - disabled: false -) %> +<%= button_to sync_provider_connection_path(connection), + method: :post, + class: "p-1 rounded hover:bg-surface transition-colors", + title: t("provider.connections.sync_now") do %> + <%= icon("refresh-cw") %> +<% end %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/provider_connections/_connection_card.html.erb` around lines 46 - 51, The current use of the icon helper with as_button: true and href: sync_provider_connection_path(connection) is functional but inconsistent with the explicit button_to pattern used elsewhere (e.g., in show.html.erb); replace the icon(..., as_button: true, href: sync_provider_connection_path(connection)) call with a direct button_to that wraps the icon (or uses the icon content as the button label) and preserves the same path, method (POST), CSS/classes and disabled state so behavior and appearance remain identical; update references in this partial around the icon call to use button_to with sync_provider_connection_path(connection) and the same options.test/controllers/oauth_callbacks_controller_test.rb (1)
34-39: ⚡ Quick winRFC1918 private addresses are not covered by the PSU IP filtering tests.
The test name says "private or loopback" but only exercises loopback (
127.0.0.1). RFC 1918 defines three separate private blocks —10.0.0.0/8,172.16.0.0/12, and192.168.0.0/16— none of which are tested here.The PR objectives explicitly list "PSU IP filtering (RFC1918/loopback/link-local/CGNAT)" as a hardening item, so a regression in the RFC1918 check would be silent.
Consider adding at least one RFC1918 address to the suite:
🛡️ Suggested additional test
test "new omits psu_ip when client IP is private or loopback" do post new_oauth_callbacks_path(provider: "truelayer"), headers: { "REMOTE_ADDR" => "127.0.0.1" } conn = Provider::Connection.order(created_at: :desc).first assert_nil conn.metadata["psu_ip"] end + test "new omits psu_ip for RFC1918 private addresses" do + ["10.0.0.1", "172.16.5.10", "192.168.1.100"].each do |private_ip| + post new_oauth_callbacks_path(provider: "truelayer"), + headers: { "REMOTE_ADDR" => private_ip } + conn = Provider::Connection.order(created_at: :desc).first + assert_nil conn.metadata["psu_ip"], "Expected psu_ip to be omitted for #{private_ip}" + end + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/oauth_callbacks_controller_test.rb` around lines 34 - 39, The test "new omits psu_ip when client IP is private or loopback" only covers loopback; add RFC1918 coverage by testing at least one address from each private block (e.g. 10.0.0.1, 172.16.0.1, 192.168.0.1). Update the test in oauth_callbacks_controller_test.rb (the test method named "new omits psu_ip when client IP is private or loopback") to either iterate over those addresses or add separate assertions: for each address, POST to new_oauth_callbacks_path with headers { "REMOTE_ADDR" => "<address>" }, fetch Provider::Connection.order(created_at: :desc).first and assert_nil conn.metadata["psu_ip"]. Keep the existing loopback assertion intact.test/models/provider/connection_test.rb (1)
61-72: ⚡ Quick winRegistry cleanup via private instance variable access is brittle.
The
ensureblock reads@oauth_providersdirectly off theProvider::Registrysingleton. If that internal variable is ever renamed or restructured,instance_variable_getreturnsnil, the&.deleteis a no-op, and"stub_no_syncer"stays registered for the remainder of the test run — silently polluting any test that resolves adapters afterward.A small public teardown API on the Registry (or wrapping the registration inside a
stubblock) would make this more robust:♻️ Possible alternatives
Option 1 — add a
deregister_oauth_providerclass method toProvider::Registry(test-use only):- ensure - Provider::Registry.instance_variable_get(:`@oauth_providers`)&.delete("stub_no_syncer") - end + ensure + Provider::Registry.deregister_oauth_provider("stub_no_syncer") + endOption 2 — use Mocha to stub the registry lookup for the duration of the test, avoiding permanent state mutation:
- stub_adapter = Class.new - Provider::Registry.register_oauth_provider("stub_no_syncer", stub_adapter) - conn = Provider::Connection.new(...) - error = assert_raises(NotImplementedError) { conn.send(:syncer) } - assert_match "does not define syncer_class", error.message - ensure - Provider::Registry.instance_variable_get(:`@oauth_providers`)&.delete("stub_no_syncer") + Provider::Registry.stubs(:[]).with("stub_no_syncer").returns(Class.new) + conn = Provider::Connection.new(...) + error = assert_raises(NotImplementedError) { conn.send(:syncer) } + assert_match "does not define syncer_class", error.message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/provider/connection_test.rb` around lines 61 - 72, Replace the brittle direct instance_variable_get call with a proper public teardown API: add a Provider::Registry.deregister_oauth_provider(provider_key) class method that removes the entry from its internal oauth providers mapping (mirroring register_oauth_provider's behavior), then update the test's ensure block to call Provider::Registry.deregister_oauth_provider("stub_no_syncer") instead of touching `@oauth_providers` directly; alternatively, wrap the registration in a temporary stub of Provider::Registry.lookup (or similar public lookup method) for the test, but prefer adding deregister_oauth_provider so tests can cleanly undo register_oauth_provider.app/models/provider/truelayer_adapter.rb (1)
79-85: 💤 Low valueConsider narrowing the bare rescue.
The bare
rescueon Line 83 catches all exceptions includingSystemExitandInterrupt. While the intent to gracefully returnnilon any failure is valid, consider rescuingStandardErrorexplicitly to avoid masking critical exceptions.Suggested fix
def fetch_consent_expiry(connection, access_token) response = Provider::Truelayer.new(access_token, sandbox: connection.metadata["sandbox"]).me raw = response.dig("results", 0, "consent_expires_at") Time.parse(raw) if raw - rescue + rescue StandardError nil end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/provider/truelayer_adapter.rb` around lines 79 - 85, The bare rescue in fetch_consent_expiry hides critical exceptions; change it to explicitly rescue StandardError (e.g. rescue StandardError) so only regular runtime errors are swallowed and not SystemExit/Interrupt; locate the method fetch_consent_expiry (uses Provider::Truelayer.new(...).me and Time.parse(raw)) and replace the bare rescue with an explicit StandardError rescue (optionally capture the error into a variable for logging) to preserve intended nil-on-failure behavior without masking critical exceptions.app/models/provider/truelayer_syncer.rb (1)
111-117: 💤 Low valueConsider whether zero available credit should be saved.
The
avail > 0guard prevents updatingavailable_creditwhen it's zero or negative. A fully maxed-out credit card legitimately has zero available credit. If this is intentional (defensive against bad provider data), consider adding a brief comment explaining the rationale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/provider/truelayer_syncer.rb` around lines 111 - 117, The code currently skips updating available_credit when avail is zero or negative; decide to persist legitimate zero available credit by removing the "if avail > 0" guard and instead update the credit card whenever avail is present (i.e., call pa.account.credit_card.update!(available_credit: avail) after computing avail), or if the original >0 check is intentional to guard bad provider data, add a concise comment above the condition explaining that rationale (reference: pa.account.accountable_type, raw["available"] / raw["credit_limit"], and pa.account.credit_card.update!(available_credit: avail)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/oauth_callbacks_controller.rb`:
- Around line 42-49: The rescue currently treats Provider::Auth::TransientError
as a terminal failure and calls connection&.update!(status: :disconnected, ...),
which disconnects valid OAuth grants; modify the rescue in
OauthCallbacksController so that when e is a Provider::Auth::TransientError you
do NOT set connection.status to :disconnected (either skip the update! entirely
or only set a sync_error without changing status), while preserving the
Rails.logger.warn and the redirect_to settings_providers_path alert; use the
exception class test (e.is_a?(Provider::Auth::TransientError)) around the
connection&.update! call to selectively avoid changing status for transient
errors and keep existing behavior for the other rescued exceptions.
In `@app/controllers/provider_connections_controller.rb`:
- Around line 30-31: The skip branches only set skipped: true which can leave a
stale account_id and forget to enqueue syncing; update both skip paths (the one
guarded by account_id.blank? and the inline skip action referenced around lines
87-93) to perform a consistent transition: call pa.update!(skipped: true,
account_id: nil) to clear any stale account linkage, and ensure you enqueue or
invoke pa.sync_later (or the existing sync enqueue method) after the update so
linked accounts are scheduled for sync; make the same change in both places to
keep behavior consistent.
In `@app/controllers/settings/bank_sync_controller.rb`:
- Around line 50-59: The provider entries currently set path: nil for non-admins
causing broken links; update the oauth_providers construction (in
oauth_providers / Provider::Registry.oauth_provider_keys block) to set path to
settings_providers_path when Current.user&.admin? is true and to a safe fallback
(e.g., '#' or omitted) for non-admins, and add a boolean flag like disabled:
!Current.user&.admin? so the view/partial (_provider_link.html.erb) can render a
disabled/read-only card instead of passing nil into link_to; ensure the partial
checks disabled and avoids calling link_to with a nil path.
In `@app/models/account.rb`:
- Around line 311-312: The provider_accounts.first call in logo_url can trigger
N+1 queries; modify the fallback to avoid firing a query when associations are
not preloaded by using a loaded-check (e.g., use provider_accounts.loaded? ?
provider_accounts.first&.safe_logo_uri : nil) and update callers that render
lists to include provider_accounts via includes/preload so the first association
is available without extra queries; target the logo_url method and the
provider_accounts/safe_logo_uri access when making this change.
In `@app/models/data_enrichment.rb`:
- Line 4: The validation now only checks presence for DataEnrichment#source;
restore a bounded domain by replacing the loose presence-only check with an
inclusion validation on the DataEnrichment model (validates :source, presence:
true, inclusion: { in: [...] }) using the canonical list of allowed sources, and
add a DB-level constraint via a migration (either a CHECK constraint against the
same list or convert the column to a DB enum) so invalid source values cannot be
persisted; update any related tests to use the allowed constants and ensure the
model and migration use the same canonical source list (e.g., a SOURCES constant
on DataEnrichment) to avoid drift.
In `@app/models/provider/auth.rb`:
- Around line 2-6: Change the four auth exception classes to inherit from
ProviderError instead of StandardError so they participate in the provider error
hierarchy; update ConsentExpiredError, TokenRevokedError, ReauthRequiredError,
and TransientError to subclass ProviderError (so with_provider_response can
catch and normalize them) and run tests to ensure existing rescue/handling paths
still work.
In `@app/models/provider/auth/oauth2.rb`:
- Around line 43-48: The update block in oauth2 refresh replaces the stored
refresh_token with tokens.refresh_token even when nil; change the
`@connection.update`! call (the credentials: `@connection.credentials.merge`(...)
inside the OAuth2 refresh handler) to only set "refresh_token" when
tokens.refresh_token is present (i.e., merge the new "refresh_token"
conditionally), so the existing stored refresh token is preserved if the
provider omits it while still updating access_token and expires_at.
In `@app/models/provider/family_config.rb`:
- Around line 20-23: Add a presence validation to require both client_id and
client_secret so BYOK configs cannot save with blank/partial credentials: add
validates :client_id, :client_secret, presence: true, if:
:byok_credentials_required? and implement a small predicate method
byok_credentials_required? that checks whether the incoming credential keys
indicate a BYOK config (e.g., credential keys include client_id/client_secret)
so the new validation is applied in the same cases credential_keys_are_known
runs; reference credential_keys_are_known, client_id, and client_secret when
locating where to add this.
In `@app/models/provider/registry.rb`:
- Around line 40-55: The oauth_provider_adapter method performs a registry
lookup without ensuring adapters are loaded, causing failures in cold processes;
update oauth_provider_adapter to call Provider::Factory.ensure_adapters_loaded
before accessing the registry so all direct reads follow the same initialization
path (this will also fix oauth_config_for which calls oauth_provider_adapter);
ensure the call is placed at the start of oauth_provider_adapter so the
subsequent access to (`@oauth_providers` ||= {})[key.to_s] is safe.
In `@app/models/provider/truelayer.rb`:
- Around line 37-64: The POST-based OAuth methods (exchange, refresh, and
reauth_uri) currently call Provider::Truelayer.post directly and therefore
bypass the transient/retry classification used by the GET path—update these
methods so POST failures are classified the same as GET: ensure
Provider::Truelayer.post responses that are 429/5xx are mapped to the same
transient error class and that socket/timeout exceptions raised during
Provider::Truelayer.post are rescued and re-raised as the transient error (the
same one used by the GET path), or refactor to call the common request wrapper
that already applies retry logic; adjust exchange, refresh and reauth_uri to use
that wrapper or to raise Provider::Truelayer::TransientError for transient
status codes and network exceptions so retries/reauth are triggered.
In `@app/views/provider_connections/_connection_card.html.erb`:
- Line 18: The view calls connection.institution_name.first.upcase which raises
NoMethodError when institution_name is nil; update the tag.p to safely handle
nil by using safe navigation or converting to string and providing a default
(e.g., use connection.institution_name&.first&.upcase || "" or
connection.institution_name.to_s.first&.upcase || "") so the call to tag.p (and
the expression around connection.institution_name) never invokes .first on nil
and renders an empty string when the name is absent.
- Line 3: The partial uses filter_map(&:account) which causes N+1 queries
because provider_accounts' account association isn't preloaded; update the query
that builds `@provider_connections` (in the AccountsController where
`@provider_connections` = family.provider_connections...) to include the nested
association by adding includes(provider_accounts: :account) (i.e.,
`@provider_connections` = family.provider_connections.where.not(status:
:pending).order(:created_at).includes(provider_accounts: :account)); apply the
same includes(provider_accounts: :account) change to the code that loads
connections for the providers panel (the query used in
app/views/settings/providers/_oauth_provider_panel.html.erb) so
provider_accounts' account is eager-loaded and the partial's
filter_map(&:account) no longer triggers extra queries.
In `@app/views/provider_connections/show.html.erb`:
- Line 31: Replace the hardcoded " ago" suffix with a translated string using
the t() helper and interpolation: call t("provider.connections.last_synced",
time: time_ago_in_words(`@connection.last_synced_at`)) instead of appending `"
ago"` to time_ago_in_words(`@connection.last_synced_at`); mirror the
implementation used in the _connection_card.html.erb partial so the dynamic time
is passed via the :time interpolation key to the translation.
In `@app/views/provider_family_configs/edit.html.erb`:
- Around line 14-16: The form currently uses FormBuilder#text_field to render
:client_secret which emits the decrypted secret into the value attribute; change
those calls to FormBuilder#password_field (i.e., replace f.text_field
:client_secret with f.password_field :client_secret) so the input value is not
rendered, keep the same label and placeholder logic, and make the identical
change in the new.html.erb template where :client_secret is rendered.
In `@app/views/provider_family_configs/new.html.erb`:
- Line 15: The form is using the form builder's text_field for :client_secret
which renders the secret value into the HTML value attribute; change the form
helper to use the form builder's password_field for the :client_secret field in
new.html.erb (the f.text_field :client_secret usage) so the password value is
not emitted on re-render and Rails will suppress the value attribute.
In `@app/views/settings/providers/_oauth_provider_panel.html.erb`:
- Line 39: Replace the raw Tailwind color class on the small status dot div (the
element with class string "w-2 h-2 bg-gray-400 rounded-full" in
_oauth_provider_panel.html.erb) with a design-system token like bg-surface or
bg-secondary; update both occurrences (the one at the shown line and the similar
one around line 56) so the class string becomes "w-2 h-2 bg-surface
rounded-full" (or "bg-secondary") to comply with the token-based styling
guideline.
In `@config/locales/views/settings/en.yml`:
- Line 232: The locale entry "connected" currently uses a literal "(s)" and
should be converted to a Rails pluralization entry so Rails handles
singular/plural automatically; replace the single-string "connected" key with a
pluralized key (e.g., provide "one" and "other" variants for the connected key)
and continue passing count: to t("connected", count: ...) where used so
translation selects the correct form; update any references that call
t("connected") to pass the count argument as needed.
In `@db/migrate/20260502153816_create_provider_tables.rb`:
- Around line 11-28: The migration allows
provider_connections.provider_family_config_id to reference any
provider_family_configs row regardless of family; add a same-family invariant by
creating a DB constraint that enforces provider_family_config_id is NULL or
belongs to the same family_id—e.g. in the migration that defines
provider_connections, add a CHECK (provider_family_config_id IS NULL OR EXISTS
(SELECT 1 FROM provider_family_configs p WHERE p.id = provider_family_config_id
AND p.family_id = family_id)) constraint (give it a clear name like
chk_provider_connections_pf_same_family) so provider_connections rows cannot
attach another family's BYOK credentials.
In `@test/fixtures/provider_connections.yml`:
- Around line 4-7: The fixture named monzo_connection conflicts with its
provider fields; update the fixture so name and provider match: either rename
monzo_connection to truelayer_connection (keeping provider_key: truelayer and
provider_family_config: truelayer_family_one) or change provider_key and
provider_family_config to Monzo equivalents if you intend a Monzo fixture;
adjust any test references to the fixture name (monzo_connection or
truelayer_connection) accordingly to keep names consistent.
---
Nitpick comments:
In `@app/controllers/concerns/accountable_resource.rb`:
- Around line 88-92: The code calls
Provider::Registry.oauth_provider_adapter(key) and then invokes
adapter.supported_account_types and adapter.connection_configs without guarding
for a nil adapter; update the block that builds `@provider_configs` to skip when
adapter is nil (e.g., next [] or next unless adapter) before calling
adapter.supported_account_types, ensuring adapter is non-nil before using
supported_account_types and connection_configs (which uses Current.family and
account_type_name) so set_link_options/new routes cannot raise NoMethodError.
In `@app/models/provider/truelayer_adapter.rb`:
- Around line 79-85: The bare rescue in fetch_consent_expiry hides critical
exceptions; change it to explicitly rescue StandardError (e.g. rescue
StandardError) so only regular runtime errors are swallowed and not
SystemExit/Interrupt; locate the method fetch_consent_expiry (uses
Provider::Truelayer.new(...).me and Time.parse(raw)) and replace the bare rescue
with an explicit StandardError rescue (optionally capture the error into a
variable for logging) to preserve intended nil-on-failure behavior without
masking critical exceptions.
In `@app/models/provider/truelayer_syncer.rb`:
- Around line 111-117: The code currently skips updating available_credit when
avail is zero or negative; decide to persist legitimate zero available credit by
removing the "if avail > 0" guard and instead update the credit card whenever
avail is present (i.e., call pa.account.credit_card.update!(available_credit:
avail) after computing avail), or if the original >0 check is intentional to
guard bad provider data, add a concise comment above the condition explaining
that rationale (reference: pa.account.accountable_type, raw["available"] /
raw["credit_limit"], and pa.account.credit_card.update!(available_credit:
avail)).
In `@app/views/provider_connections/_connection_card.html.erb`:
- Around line 46-51: The current use of the icon helper with as_button: true and
href: sync_provider_connection_path(connection) is functional but inconsistent
with the explicit button_to pattern used elsewhere (e.g., in show.html.erb);
replace the icon(..., as_button: true, href:
sync_provider_connection_path(connection)) call with a direct button_to that
wraps the icon (or uses the icon content as the button label) and preserves the
same path, method (POST), CSS/classes and disabled state so behavior and
appearance remain identical; update references in this partial around the icon
call to use button_to with sync_provider_connection_path(connection) and the
same options.
In `@app/views/settings/bank_sync/_provider_link.html.erb`:
- Around line 15-16: Replace the hard-coded "beta" badge text in
_provider_link.html.erb (the provider_link[:beta] branch) with a call to the
Rails i18n helper t(), e.g. t('settings.bank_sync.provider_link.beta'), and add
that key to config/locales/en.yml under the hierarchical feature namespace
(settings -> bank_sync -> provider_link -> beta) with the "beta" value; ensure
you use the same key in the template so the UI text is localized.
In `@app/views/settings/providers/_oauth_provider_panel.html.erb`:
- Around line 2-4: The view partial runs ActiveRecord queries
(Current.family.provider_family_configs.find_by and
Current.family.provider_connections.where(...).includes(...)) for each provider;
move these queries into the controller action or a presenter and resolve
`config` and `connections` there, then pass them into the partial as locals
(e.g., render partial: 'oauth_provider_panel', locals: { config: config,
connections: connections }) so the partial only uses the provided `config` and
`connections` variables and does not perform any DB lookups itself.
In `@test/controllers/oauth_callbacks_controller_test.rb`:
- Around line 34-39: The test "new omits psu_ip when client IP is private or
loopback" only covers loopback; add RFC1918 coverage by testing at least one
address from each private block (e.g. 10.0.0.1, 172.16.0.1, 192.168.0.1). Update
the test in oauth_callbacks_controller_test.rb (the test method named "new omits
psu_ip when client IP is private or loopback") to either iterate over those
addresses or add separate assertions: for each address, POST to
new_oauth_callbacks_path with headers { "REMOTE_ADDR" => "<address>" }, fetch
Provider::Connection.order(created_at: :desc).first and assert_nil
conn.metadata["psu_ip"]. Keep the existing loopback assertion intact.
In `@test/models/provider/connection_test.rb`:
- Around line 61-72: Replace the brittle direct instance_variable_get call with
a proper public teardown API: add a
Provider::Registry.deregister_oauth_provider(provider_key) class method that
removes the entry from its internal oauth providers mapping (mirroring
register_oauth_provider's behavior), then update the test's ensure block to call
Provider::Registry.deregister_oauth_provider("stub_no_syncer") instead of
touching `@oauth_providers` directly; alternatively, wrap the registration in a
temporary stub of Provider::Registry.lookup (or similar public lookup method)
for the test, but prefer adding deregister_oauth_provider so tests can cleanly
undo register_oauth_provider.
🪄 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: 2b4a722b-fb82-4238-872f-757a5e7f2edb
📒 Files selected for processing (61)
app/controllers/accounts_controller.rbapp/controllers/concerns/accountable_resource.rbapp/controllers/oauth_callbacks_controller.rbapp/controllers/provider_connections_controller.rbapp/controllers/provider_family_configs_controller.rbapp/controllers/settings/bank_sync_controller.rbapp/jobs/provider/consent_expiry_check_job.rbapp/models/account.rbapp/models/account/provider_import_adapter.rbapp/models/data_enrichment.rbapp/models/family.rbapp/models/family/syncer.rbapp/models/provider/account.rbapp/models/provider/auth.rbapp/models/provider/auth/oauth2.rbapp/models/provider/connection.rbapp/models/provider/connection/sync_complete_event.rbapp/models/provider/family_config.rbapp/models/provider/registry.rbapp/models/provider/truelayer.rbapp/models/provider/truelayer_adapter.rbapp/models/provider/truelayer_syncer.rbapp/models/provider_merchant.rbapp/models/transaction.rbapp/views/accounts/index.html.erbapp/views/provider_connections/_connection_card.html.erbapp/views/provider_connections/select.html.erbapp/views/provider_connections/setup.html.erbapp/views/provider_connections/show.html.erbapp/views/provider_family_configs/_truelayer_instructions.html.erbapp/views/provider_family_configs/edit.html.erbapp/views/provider_family_configs/new.html.erbapp/views/settings/bank_sync/_provider_link.html.erbapp/views/settings/providers/_oauth_provider_panel.html.erbapp/views/settings/providers/show.html.erbconfig/brakeman.ignoreconfig/initializers/inflections.rbconfig/initializers/sidekiq.rbconfig/locales/views/provider_connections/en.ymlconfig/locales/views/provider_family_configs/en.ymlconfig/locales/views/settings/en.ymlconfig/routes.rbdb/migrate/20260502153816_create_provider_tables.rbdb/schema.rbtest/controllers/oauth_callbacks_controller_test.rbtest/controllers/provider_connections_controller_test.rbtest/controllers/provider_family_configs_controller_test.rbtest/controllers/settings/providers_controller_test.rbtest/fixtures/provider_accounts.ymltest/fixtures/provider_connections.ymltest/fixtures/provider_family_configs.ymltest/jobs/provider/consent_expiry_check_job_test.rbtest/models/account_test.rbtest/models/family/syncer_test.rbtest/models/provider/account_test.rbtest/models/provider/auth/oauth2_test.rbtest/models/provider/connection_test.rbtest/models/provider/family_config_test.rbtest/models/provider/truelayer_adapter_test.rbtest/models/provider/truelayer_syncer_test.rbtest/models/provider/truelayer_test.rb
💤 Files with no reviewable changes (1)
- app/models/provider_merchant.rb
Comment on lines
+30
to
+31
| if account_id.blank? | ||
| pa.update!(skipped: true) |
Contributor
There was a problem hiding this comment.
Make the skip transition consistent.
Both skip paths only flip skipped: true. That can preserve a stale account_id on replayed/stale submissions, and the inline skip action also forgets to enqueue sync_later, so linked accounts can sit unsynced after the user finishes setup by skipping the rest.
Suggested fix
if account_id.blank?
- pa.update!(skipped: true)
+ pa.update!(account_id: nil, skipped: true)
@@
def skip
pa = find_provider_account_for_family(params[:provider_account_id])
return head :not_found unless pa
- pa.update!(skipped: true)
+ pa.update!(account_id: nil, skipped: true)
+ pa.provider_connection.sync_later
redirect_to provider_connection_path(pa.provider_connection),
notice: t("provider.connections.account_skipped")
endAlso applies to: 87-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/provider_connections_controller.rb` around lines 30 - 31, The
skip branches only set skipped: true which can leave a stale account_id and
forget to enqueue syncing; update both skip paths (the one guarded by
account_id.blank? and the inline skip action referenced around lines 87-93) to
perform a consistent transition: call pa.update!(skipped: true, account_id: nil)
to clear any stale account linkage, and ensure you enqueue or invoke
pa.sync_later (or the existing sync enqueue method) after the update so linked
accounts are scheduled for sync; make the same change in both places to keep
behavior consistent.
Comment on lines
+50
to
+59
| oauth_providers = Provider::Registry.oauth_provider_keys.map do |key| | ||
| adapter = Provider::Registry.oauth_provider_adapter(key) | ||
| { | ||
| name: adapter.display_name, | ||
| description: adapter.respond_to?(:description) ? adapter.description : nil, | ||
| color: adapter.respond_to?(:brand_color) ? adapter.brand_color : "#6B7280", | ||
| beta: adapter.beta?, | ||
| path: Current.user&.admin? ? settings_providers_path : nil | ||
| } | ||
| end |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find how bank_sync provider path is consumed in views
rg -n 'provider\[:path\]\|provider\["path"\]' app/views/settings/bank_sync/ app/views/settings/ -A3 -B3Repository: we-promise/sure
Length of output: 41
🏁 Script executed:
fd -type f 'bank_sync' app/views/Repository: we-promise/sure
Length of output: 230
🏁 Script executed:
cat -n app/views/settings/bank_sync/show.html.erbRepository: we-promise/sure
Length of output: 1284
🏁 Script executed:
rg -n 'oauth_providers' app/views/settings/ -A5 -B2Repository: we-promise/sure
Length of output: 41
🏁 Script executed:
fd 'provider_link' app/views/Repository: we-promise/sure
Length of output: 112
🏁 Script executed:
cat -n app/views/settings/bank_sync/_provider_link.html.erbRepository: we-promise/sure
Length of output: 1291
The path: nil for non-admin OAuth providers creates broken links in the view.
At line 57 of the controller, non-admin users receive path: nil for OAuth providers. The _provider_link.html.erb partial (line 3) unconditionally passes this path to link_to as the first argument. When link_to receives nil, it generates an anchor tag without an href attribute, rendering the provider cards as non-functional elements. Non-admin users should either see disabled/read-only provider cards or the cards should be hidden entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/settings/bank_sync_controller.rb` around lines 50 - 59, The
provider entries currently set path: nil for non-admins causing broken links;
update the oauth_providers construction (in oauth_providers /
Provider::Registry.oauth_provider_keys block) to set path to
settings_providers_path when Current.user&.admin? is true and to a safe fallback
(e.g., '#' or omitted) for non-admins, and add a boolean flag like disabled:
!Current.user&.admin? so the view/partial (_provider_link.html.erb) can render a
disabled/read-only card instead of passing nil into link_to; ensure the partial
checks disabled and avoids calling link_to with a nil path.
Comment on lines
+311
to
+312
| elsif (uri = provider_accounts.first&.safe_logo_uri).present? | ||
| uri |
Contributor
There was a problem hiding this comment.
provider_accounts.first adds an N+1 query when logo_url is called in list context.
If provider_accounts is not eagerly loaded (e.g., on the accounts index), each call to logo_url falling through to this branch fires an extra SELECT per account.
Ensure that callers loading accounts for display include provider_accounts in their preload, or use provider_accounts.loaded? ? provider_accounts.first : nil to avoid incidental queries.
As per coding guidelines: "Optimize database access with proper indexes and prevent N+1 queries via includes/joins."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/account.rb` around lines 311 - 312, The provider_accounts.first
call in logo_url can trigger N+1 queries; modify the fallback to avoid firing a
query when associations are not preloaded by using a loaded-check (e.g., use
provider_accounts.loaded? ? provider_accounts.first&.safe_logo_uri : nil) and
update callers that render lists to include provider_accounts via
includes/preload so the first association is available without extra queries;
target the logo_url method and the provider_accounts/safe_logo_uri access when
making this change.
| @@ -1,5 +1,5 @@ | |||
| class DataEnrichment < ApplicationRecord | |||
| belongs_to :enrichable, polymorphic: true | |||
|
|
|||
| enum :source, { rule: "rule", plaid: "plaid", simplefin: "simplefin", lunchflow: "lunchflow", synth: "synth", ai: "ai", enable_banking: "enable_banking", coinstats: "coinstats", mercury: "mercury", indexa_capital: "indexa_capital", sophtron: "sophtron" } | |||
| validates :source, presence: true | |||
Contributor
There was a problem hiding this comment.
Restore a bounded source domain.
Line 4 now accepts any non-blank source. If removing the enum was intentional, please replace it with an explicit inclusion validation and a DB-level constraint/check so invalid source values cannot be persisted.
As per coding guidelines, "Simple validations (null checks, unique indexes) should be in the database. ActiveRecord validations for convenience in forms."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/data_enrichment.rb` at line 4, The validation now only checks
presence for DataEnrichment#source; restore a bounded domain by replacing the
loose presence-only check with an inclusion validation on the DataEnrichment
model (validates :source, presence: true, inclusion: { in: [...] }) using the
canonical list of allowed sources, and add a DB-level constraint via a migration
(either a CHECK constraint against the same list or convert the column to a DB
enum) so invalid source values cannot be persisted; update any related tests to
use the allowed constants and ensure the model and migration use the same
canonical source list (e.g., a SOURCES constant on DataEnrichment) to avoid
drift.
Comment on lines
+11
to
+28
| create_table :provider_connections, id: :uuid do |t| | ||
| t.references :family, null: false, foreign_key: true, type: :uuid | ||
| # Null for providers using global Rails config credentials (e.g. Plaid reads PLAID_CLIENT_ID from env). | ||
| # Present for BYOK providers where each family supplies their own client_id/secret (e.g. TrueLayer, EnableBanking). | ||
| t.references :provider_family_config, null: true, foreign_key: true, type: :uuid | ||
| t.string :provider_key, null: false | ||
| t.string :auth_type, null: false | ||
| t.jsonb :credentials, default: {}, null: false | ||
| t.string :status, null: false, default: "pending" | ||
| t.jsonb :metadata, default: {}, null: false | ||
| t.string :sync_error | ||
| t.date :sync_start_date | ||
| t.datetime :last_synced_at | ||
| t.timestamps | ||
| end | ||
| # Non-unique: a family can have multiple bank connections per provider | ||
| # (e.g., Monzo + Barclays both via TrueLayer = two provider_connections) | ||
| add_index :provider_connections, [ :family_id, :provider_key ] |
Contributor
There was a problem hiding this comment.
Prevent cross-family credential attachment.
provider_connections.provider_family_config_id only checks that the config exists, not that it belongs to the same family_id. As written, a bad write can bind family A's connection to family B's BYOK credentials. Add a same-family invariant here, e.g. a composite constraint from (provider_family_config_id, family_id) to provider_family_configs(id, family_id).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20260502153816_create_provider_tables.rb` around lines 11 - 28,
The migration allows provider_connections.provider_family_config_id to reference
any provider_family_configs row regardless of family; add a same-family
invariant by creating a DB constraint that enforces provider_family_config_id is
NULL or belongs to the same family_id—e.g. in the migration that defines
provider_connections, add a CHECK (provider_family_config_id IS NULL OR EXISTS
(SELECT 1 FROM provider_family_configs p WHERE p.id = provider_family_config_id
AND p.family_id = family_id)) constraint (give it a clear name like
chk_provider_connections_pf_same_family) so provider_connections rows cannot
attach another family's BYOK credentials.
Comment on lines
+4
to
+7
| monzo_connection: | ||
| family: dylan_family | ||
| provider_family_config: truelayer_family_one | ||
| provider_key: truelayer |
Contributor
There was a problem hiding this comment.
Fixture name monzo_connection conflicts with its TrueLayer content.
The fixture is named after Monzo (a different bank) but references provider_key: truelayer and provider_family_config: truelayer_family_one. This mismatch will cause confusion when tests reference this fixture by name.
🔧 Proposed fix
-monzo_connection:
+truelayer_connection:
family: dylan_family
provider_family_config: truelayer_family_one
provider_key: truelayer📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| monzo_connection: | |
| family: dylan_family | |
| provider_family_config: truelayer_family_one | |
| provider_key: truelayer | |
| truelayer_connection: | |
| family: dylan_family | |
| provider_family_config: truelayer_family_one | |
| provider_key: truelayer |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/fixtures/provider_connections.yml` around lines 4 - 7, The fixture named
monzo_connection conflicts with its provider fields; update the fixture so name
and provider match: either rename monzo_connection to truelayer_connection
(keeping provider_key: truelayer and provider_family_config:
truelayer_family_one) or change provider_key and provider_family_config to Monzo
equivalents if you intend a Monzo fixture; adjust any test references to the
fixture name (monzo_connection or truelayer_connection) accordingly to keep
names consistent.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/models/data_enrichment.rb (1)
4-4:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnbounded
source— inclusion validation and DB constraint still missing.This is already flagged by an existing review comment. The removal of
enum :sourceleavessourceconstrained only to be non-blank; any arbitrary string can now be persisted. As per coding guidelines, simple domain constraints should live in the database (check constraint or DB enum), with a matchinginclusion:validation on the model for form-level feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/data_enrichment.rb` at line 4, The DataEnrichment model currently only has validates :source, presence: true leaving source unbounded; add an inclusion validation on DataEnrichment (e.g., validates :source, presence: true, inclusion: { in: SOURCE_VALUES }) to restrict allowed values and expose form-level errors, and add a matching DB-level constraint via a migration (either a DB enum or a CHECK constraint on the source column listing the same SOURCE_VALUES) so the database enforces the domain; ensure the model’s constant (e.g., SOURCE_VALUES or SOURCE_ENUM) is the single source of truth used in both the validation and the migration.
🧹 Nitpick comments (3)
test/controllers/oauth_callbacks_controller_test.rb (2)
22-22: ⚡ Quick winExtract repeated
Provider::Connection.order(created_at: :desc).firstinto a private helper.This one-liner appears 8 times. A private helper would reduce duplication and make the tests easier to read.
♻️ Suggested refactor
+ private + + def last_created_connection + Provider::Connection.order(created_at: :desc).first + endThen replace each occurrence:
- conn = Provider::Connection.order(created_at: :desc).first + conn = last_created_connectionAlso applies to: 30-30, 37-37, 44-44, 51-51, 58-58, 64-64, 90-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/oauth_callbacks_controller_test.rb` at line 22, Add a private helper in the test class/describe block (e.g., def last_provider_connection) that returns Provider::Connection.order(created_at: :desc).first, then replace every inline usage of Provider::Connection.order(created_at: :desc).first (the occurrences around lines 22, 30, 37, 44, 51, 58, 64, 90) with a call to last_provider_connection to remove duplication and improve readability.
62-73: ⚡ Quick win
exchange_codeexpectation returnsnilby default — may not accurately validate the happy path.Mocha's
expectswithout.returns(...)returnsnil. IfProvider::Auth::OAuth2#exchange_codenormally returns a token hash that the controller persists before callingdiscover_accounts!, the test is exercising the controller against anilcredential payload rather than realistic data. This can mask silent failures in token-persistence logic on the happy path.🔧 Suggested fix
- Provider::Auth::OAuth2.any_instance.expects(:exchange_code).once + Provider::Auth::OAuth2.any_instance.expects(:exchange_code).once + .returns({ "access_token" => "tok_test", "refresh_token" => "ref_test", "expires_in" => 3600 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/oauth_callbacks_controller_test.rb` around lines 62 - 73, The test sets an expectation on Provider::Auth::OAuth2#exchange_code but doesn't return a realistic token, so the controller is tested with nil; update the expectation to .returns(...) with a realistic token hash (e.g. access_token, refresh_token, expires_at or expires_in) so the controller's token-persistence path runs, then assert the Provider::Connection record has the persisted token values; modify the Provider::Auth::OAuth2.any_instance.expects(:exchange_code) call to return that token payload while keeping Provider::Connection.any_instance.expects(:discover_accounts!).once and the redirect assertion against setup_provider_connection_path(conn).app/models/provider/truelayer_syncer.rb (1)
111-117: 💤 Low valueConsider allowing zero available credit.
The condition
if avail > 0skips updatingavailable_creditwhen it's exactly zero, which could be a valid state (fully utilized credit line). If the intent is to only skip invalid/missing data, the check should be>= 0.♻️ Suggested change
if avail_raw.present? avail = BigDecimal(avail_raw.to_s) - pa.account.credit_card.update!(available_credit: avail) if avail > 0 + pa.account.credit_card.update!(available_credit: avail) if avail >= 0 end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/provider/truelayer_syncer.rb` around lines 111 - 117, The code currently skips updating available_credit when avail == 0; update the conditional that gates pa.account.credit_card.update!(available_credit: avail) to allow zero values by changing the check from avail > 0 to avail >= 0 (or check avail >= 0 to ensure non-negative values), keeping the BigDecimal conversion of avail_raw as-is and using the same pa.account.credit_card.update! call so a zero available_credit is persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/jobs/provider/consent_expiry_check_job.rb`:
- Around line 12-13: Time.zone.parse(raw) can return nil causing "undefined
method '<=' for nil" later; update the code around the expiry assignment and
conditional (where Time.zone.parse(raw) is assigned to expiry and compared to
EXPIRY_WARNING_WINDOW.from_now) to explicitly guard against nil: after calling
Time.zone.parse(raw) check expiry.nil? and handle malformed input by logging a
clear data-quality message (including the raw value) and skipping/continuing
that record instead of letting the <= comparison raise; ensure the subsequent
conditional becomes expiry && expiry <= EXPIRY_WARNING_WINDOW.from_now (or an
equivalent nil check) so the rescue no longer logs a misleading NoMethodError.
In `@app/models/provider/connection/sync_complete_event.rb`:
- Around line 9-12: The call to Turbo::StreamsChannel.broadcast_refresh_to is
passing an unsupported requestId: option; remove the requestId parameter or
replace the call with Turbo::StreamsChannel.broadcast_refresh_later_to and pass
request_id: SecureRandom.uuid if you need Turbo deduplication. Locate the
invocation in sync_complete_event.rb (the call to
Turbo::StreamsChannel.broadcast_refresh_to) and either change it to just
broadcast_refresh_to(`@connection.family`) to match Family::SyncCompleteEvent /
Account::SyncCompleteEvent patterns, or switch to
broadcast_refresh_later_to(`@connection.family`, request_id: SecureRandom.uuid) to
enable deduplication.
In `@test/controllers/oauth_callbacks_controller_test.rb`:
- Around line 34-39: The test only covers loopback but must also assert RFC1918
private ranges are filtered; add one or more test cases that POST to
new_oauth_callbacks_path (same as the existing test) with headers "REMOTE_ADDR"
set to representative private addresses (e.g., "10.1.2.3", "172.16.5.4",
"192.168.0.1"), then load Provider::Connection.order(created_at: :desc).first
and assert_nil conn.metadata["psu_ip"] for each case; you can either create
separate tests or parameterize the existing test to iterate those addresses to
ensure metadata["psu_ip"] is nil for all private ranges.
---
Duplicate comments:
In `@app/models/data_enrichment.rb`:
- Line 4: The DataEnrichment model currently only has validates :source,
presence: true leaving source unbounded; add an inclusion validation on
DataEnrichment (e.g., validates :source, presence: true, inclusion: { in:
SOURCE_VALUES }) to restrict allowed values and expose form-level errors, and
add a matching DB-level constraint via a migration (either a DB enum or a CHECK
constraint on the source column listing the same SOURCE_VALUES) so the database
enforces the domain; ensure the model’s constant (e.g., SOURCE_VALUES or
SOURCE_ENUM) is the single source of truth used in both the validation and the
migration.
---
Nitpick comments:
In `@app/models/provider/truelayer_syncer.rb`:
- Around line 111-117: The code currently skips updating available_credit when
avail == 0; update the conditional that gates
pa.account.credit_card.update!(available_credit: avail) to allow zero values by
changing the check from avail > 0 to avail >= 0 (or check avail >= 0 to ensure
non-negative values), keeping the BigDecimal conversion of avail_raw as-is and
using the same pa.account.credit_card.update! call so a zero available_credit is
persisted.
In `@test/controllers/oauth_callbacks_controller_test.rb`:
- Line 22: Add a private helper in the test class/describe block (e.g., def
last_provider_connection) that returns Provider::Connection.order(created_at:
:desc).first, then replace every inline usage of
Provider::Connection.order(created_at: :desc).first (the occurrences around
lines 22, 30, 37, 44, 51, 58, 64, 90) with a call to last_provider_connection to
remove duplication and improve readability.
- Around line 62-73: The test sets an expectation on
Provider::Auth::OAuth2#exchange_code but doesn't return a realistic token, so
the controller is tested with nil; update the expectation to .returns(...) with
a realistic token hash (e.g. access_token, refresh_token, expires_at or
expires_in) so the controller's token-persistence path runs, then assert the
Provider::Connection record has the persisted token values; modify the
Provider::Auth::OAuth2.any_instance.expects(:exchange_code) call to return that
token payload while keeping
Provider::Connection.any_instance.expects(:discover_accounts!).once and the
redirect assertion against setup_provider_connection_path(conn).
🪄 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: 63a306f8-0ae8-4651-bacb-74924a1a9001
📒 Files selected for processing (61)
app/controllers/accounts_controller.rbapp/controllers/concerns/accountable_resource.rbapp/controllers/oauth_callbacks_controller.rbapp/controllers/provider_connections_controller.rbapp/controllers/provider_family_configs_controller.rbapp/controllers/settings/bank_sync_controller.rbapp/jobs/provider/consent_expiry_check_job.rbapp/models/account.rbapp/models/account/provider_import_adapter.rbapp/models/data_enrichment.rbapp/models/family.rbapp/models/family/syncer.rbapp/models/provider/account.rbapp/models/provider/auth.rbapp/models/provider/auth/oauth2.rbapp/models/provider/connection.rbapp/models/provider/connection/sync_complete_event.rbapp/models/provider/family_config.rbapp/models/provider/registry.rbapp/models/provider/truelayer.rbapp/models/provider/truelayer_adapter.rbapp/models/provider/truelayer_syncer.rbapp/models/provider_merchant.rbapp/models/transaction.rbapp/views/accounts/index.html.erbapp/views/provider_connections/_connection_card.html.erbapp/views/provider_connections/select.html.erbapp/views/provider_connections/setup.html.erbapp/views/provider_connections/show.html.erbapp/views/provider_family_configs/_truelayer_instructions.html.erbapp/views/provider_family_configs/edit.html.erbapp/views/provider_family_configs/new.html.erbapp/views/settings/bank_sync/_provider_link.html.erbapp/views/settings/providers/_oauth_provider_panel.html.erbapp/views/settings/providers/show.html.erbconfig/brakeman.ignoreconfig/initializers/inflections.rbconfig/initializers/sidekiq.rbconfig/locales/views/provider_connections/en.ymlconfig/locales/views/provider_family_configs/en.ymlconfig/locales/views/settings/en.ymlconfig/routes.rbdb/migrate/20260502153816_create_provider_tables.rbdb/schema.rbtest/controllers/oauth_callbacks_controller_test.rbtest/controllers/provider_connections_controller_test.rbtest/controllers/provider_family_configs_controller_test.rbtest/controllers/settings/providers_controller_test.rbtest/fixtures/provider_accounts.ymltest/fixtures/provider_connections.ymltest/fixtures/provider_family_configs.ymltest/jobs/provider/consent_expiry_check_job_test.rbtest/models/account_test.rbtest/models/family/syncer_test.rbtest/models/provider/account_test.rbtest/models/provider/auth/oauth2_test.rbtest/models/provider/connection_test.rbtest/models/provider/family_config_test.rbtest/models/provider/truelayer_adapter_test.rbtest/models/provider/truelayer_syncer_test.rbtest/models/provider/truelayer_test.rb
💤 Files with no reviewable changes (1)
- app/models/provider_merchant.rb
✅ Files skipped from review due to trivial changes (22)
- config/initializers/inflections.rb
- test/fixtures/provider_family_configs.yml
- app/views/provider_family_configs/new.html.erb
- test/fixtures/provider_connections.yml
- config/locales/views/provider_family_configs/en.yml
- app/views/provider_connections/setup.html.erb
- app/models/provider/auth.rb
- db/migrate/20260502153816_create_provider_tables.rb
- config/initializers/sidekiq.rb
- config/locales/views/settings/en.yml
- app/models/provider/family_config.rb
- test/models/family/syncer_test.rb
- test/models/provider/family_config_test.rb
- config/locales/views/provider_connections/en.yml
- app/views/settings/providers/show.html.erb
- app/views/settings/bank_sync/_provider_link.html.erb
- test/controllers/settings/providers_controller_test.rb
- test/fixtures/provider_accounts.yml
- test/jobs/provider/consent_expiry_check_job_test.rb
- test/models/provider/auth/oauth2_test.rb
- test/models/provider/account_test.rb
- app/views/provider_family_configs/edit.html.erb
🚧 Files skipped from review as they are similar to previous changes (19)
- app/models/family.rb
- test/controllers/provider_family_configs_controller_test.rb
- app/controllers/concerns/accountable_resource.rb
- app/models/account.rb
- config/routes.rb
- app/views/settings/providers/_oauth_provider_panel.html.erb
- app/models/provider/registry.rb
- app/controllers/oauth_callbacks_controller.rb
- app/views/provider_family_configs/_truelayer_instructions.html.erb
- app/models/account/provider_import_adapter.rb
- app/views/provider_connections/show.html.erb
- app/models/provider/connection.rb
- app/views/accounts/index.html.erb
- test/controllers/provider_connections_controller_test.rb
- test/models/provider/truelayer_syncer_test.rb
- app/models/provider/truelayer_adapter.rb
- app/models/family/syncer.rb
- test/models/provider/connection_test.rb
- app/models/provider/auth/oauth2.rb
Comment on lines
+12
to
+13
| expiry = Time.zone.parse(raw) | ||
| if expiry <= EXPIRY_WARNING_WINDOW.from_now |
Contributor
There was a problem hiding this comment.
Guard against nil result from Time.zone.parse.
Time.zone.parse(raw) can return nil for certain malformed inputs (rather than raising ArgumentError). When that happens, nil <= EXPIRY_WARNING_WINDOW.from_now raises NoMethodError, which the rescue catches and logs as "undefined method '<=' for nil" — a misleading message that looks like a code defect rather than a data quality issue.
🛡️ Proposed fix
expiry = Time.zone.parse(raw)
+ next unless expiry
if expiry <= EXPIRY_WARNING_WINDOW.from_now📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expiry = Time.zone.parse(raw) | |
| if expiry <= EXPIRY_WARNING_WINDOW.from_now | |
| expiry = Time.zone.parse(raw) | |
| next unless expiry | |
| if expiry <= EXPIRY_WARNING_WINDOW.from_now |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/jobs/provider/consent_expiry_check_job.rb` around lines 12 - 13,
Time.zone.parse(raw) can return nil causing "undefined method '<=' for nil"
later; update the code around the expiry assignment and conditional (where
Time.zone.parse(raw) is assigned to expiry and compared to
EXPIRY_WARNING_WINDOW.from_now) to explicitly guard against nil: after calling
Time.zone.parse(raw) check expiry.nil? and handle malformed input by logging a
clear data-quality message (including the raw value) and skipping/continuing
that record instead of letting the <= comparison raise; ensure the subsequent
conditional becomes expiry && expiry <= EXPIRY_WARNING_WINDOW.from_now (or an
equivalent nil check) so the rescue no longer logs a misleading NoMethodError.
Comment on lines
+9
to
+12
| Turbo::StreamsChannel.broadcast_refresh_to( | ||
| @connection.family, | ||
| requestId: SecureRandom.uuid | ||
| ) |
Contributor
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
# First, confirm the code in the file
cat -n app/models/provider/connection/sync_complete_event.rbRepository: we-promise/sure
Repository: we-promise/sure
Exit code: 0
stdout:
1 class Provider::Connection::SyncCompleteEvent
2 def initialize(connection)
3 `@connection` = connection
4 end
5
6 def broadcast
7 # Placeholder: full-page refresh. Replace with surgical Turbo stream updates
8 # (following EnableBankingItem::SyncCompleteEvent) when the TrueLayer UI is built out.
9 Turbo::StreamsChannel.broadcast_refresh_to(
10 `@connection.family`,
11 requestId: SecureRandom.uuid
12 )
13 end
14 end
Script executed:
# Search for turbo-rails API documentation or usage patterns in the codebase
rg "broadcast_refresh" -A 3 -B 1Repository: we-promise/sure
Repository: we-promise/sure
Exit code: 0
stdout:
app/models/simplefin_item/syncer.rb- # list is correctly scoped to the current user.
app/models/simplefin_item/syncer.rb: simplefin_item.family.broadcast_refresh
app/models/simplefin_item/syncer.rb- rescue => e
app/models/simplefin_item/syncer.rb- Rails.logger.warn("SimplefinItem::Syncer broadcast failed: #{e.class} - #{e.message}")
app/models/simplefin_item/syncer.rb- end
--
app/models/provider/connection/sync_complete_event.rb- # (following EnableBankingItem::SyncCompleteEvent) when the TrueLayer UI is built out.
app/models/provider/connection/sync_complete_event.rb: Turbo::StreamsChannel.broadcast_refresh_to(
app/models/provider/connection/sync_complete_event.rb- `@connection.family`,
app/models/provider/connection/sync_complete_event.rb- requestId: SecureRandom.uuid
app/models/provider/connection/sync_complete_event.rb- )
--
app/models/family/sync_complete_event.rb- # nil in background jobs, which would produce an unscoped family-wide view).
app/models/family/sync_complete_event.rb: family.broadcast_refresh
app/models/family/sync_complete_event.rb-
app/models/family/sync_complete_event.rb- # Schedule recurring transaction pattern identification (debounced to run after all syncs complete)
app/models/family/sync_complete_event.rb- begin
--
app/models/account/sync_complete_event.rb- # Refresh entire account page (only applies if currently viewing this account)
app/models/account/sync_complete_event.rb: account.broadcast_refresh
app/models/account/sync_complete_event.rb- end
app/models/account/sync_complete_event.rb-end
--
app/jobs/simplefin_item/balances_only_job.rb- # list is correctly scoped to the current user.
app/jobs/simplefin_item/balances_only_job.rb: item.family.broadcast_refresh
app/jobs/simplefin_item/balances_only_job.rb- rescue => e
app/jobs/simplefin_item/balances_only_job.rb- Rails.logger.warn("SimpleFin BalancesOnlyJob broadcast failed: #{e.class} - #{e.message}")
app/jobs/simplefin_item/balances_only_job.rb- end
--
app/components/UI/account_page.rb-
app/components/UI/account_page.rb: def broadcast_refresh!
app/components/UI/account_page.rb- Turbo::StreamsChannel.broadcast_replace_to(broadcast_channel, target: id, renderable: self, layout: false)
app/components/UI/account_page.rb- end
app/components/UI/account_page.rb-
--
app/components/UI/account/activity_feed.rb-
app/components/UI/account/activity_feed.rb: def broadcast_refresh!
app/components/UI/account/activity_feed.rb- Turbo::StreamsChannel.broadcast_replace_to(
app/components/UI/account/activity_feed.rb- broadcast_channel,
app/components/UI/account/activity_feed.rb- target: id,
--
app/components/UI/account/activity_date.rb-
app/components/UI/account/activity_date.rb: def broadcast_refresh!
app/components/UI/account/activity_date.rb- Turbo::StreamsChannel.broadcast_replace_to(
app/components/UI/account/activity_date.rb- broadcast_channel,
app/components/UI/account/activity_date.rb- target: id,
turbo-rails broadcast_refresh_to API request_id parameter
In turbo-rails, the “broadcast_refresh_to” family does not have a general API parameter named request_id. Instead, request_id is only supported by the asynchronous variant: - broadcast_refresh_later_to(*streamables, request_id: Turbo.current_request_id, opts) What this means: 1) broadcast_refresh_to - Signature: broadcast_refresh_to(*streamables, opts) - No request_id parameter is accepted by this method; it just calls Turbo::StreamsChannel.broadcast_refresh_to(*streamables) with a “refresh” turbo-stream tag (without the request-id attribute). 2) broadcast_refresh_later_to - Signature includes request_id: - broadcast_refresh_later_to(*streamables, request_id: Turbo.current_request_id, opts) - That request_id is used to build the refresh turbo-stream payload (turbo_stream_refresh_tag(request_id: request_id, opts)) and is part of how Turbo can ignore a page refresh that originated from the same browser request. So if you’re trying to pass request_id with broadcast_refresh_to, the supported approach is to use broadcast_refresh_later_to (or broadcast_refresh_later, which uses the current model as the streamable and passes Turbo.current_request_id).
Citations:
- 1: https://rubydoc.info/github/hotwired/turbo-rails/Turbo%2FBroadcastable:broadcast_refresh_to
- 2: https://www.rubydoc.info/github/hotwired/turbo-rails/Turbo/Streams/Broadcasts
- 3: https://rubydoc.info/github/hotwired/turbo-rails/Turbo%2FStreams%2FActionHelper:turbo_stream_refresh_tag
- 4: https://www.rubydoc.info/gems/turbo-rails/2.0.5/Turbo/Broadcastable
- 5: https://www.rubydoc.info/github/hotwired/turbo-rails/Turbo%2FBroadcastable:broadcast_refresh_later_to
- 6: https://rubydoc.info/github/hotwired/turbo-rails/main/Turbo%2FBroadcastable:broadcast_update_to
- 7: Adding X-Turbo-Request-Id support rails/request.js#73
- 8: Page refreshes hotwired/turbo#1019
- 9: https://github.com/hotwired/turbo-rails/blob/main/app/channels/turbo/streams/broadcasts.rb
Script executed:
# Check other SyncCompleteEvent implementations for comparison
fd "sync_complete_event.rb" -type fRepository: we-promise/sure
Repository: we-promise/sure
Exit code: 2
stderr:
error: 'ype' isn't a valid value for '--type <filetype>'
[possible values: file, directory, symlink, executable, empty, socket, pipe]
For more information try '--help'
Script executed:
# Read related implementations to understand the pattern
cat -n app/models/family/sync_complete_event.rb
cat -n app/models/account/sync_complete_event.rbRepository: we-promise/sure
Repository: we-promise/sure
Exit code: 0
stdout:
1 class Family::SyncCompleteEvent
2 attr_reader :family
3
4 def initialize(family)
5 `@family` = family
6 end
7
8 def broadcast
9 # Broadcast a refresh signal instead of rendered HTML. Each user's browser
10 # re-fetches via their own authenticated request, so the balance sheet and
11 # net worth chart are correctly scoped to the current user (Current.user is
12 # nil in background jobs, which would produce an unscoped family-wide view).
13 family.broadcast_refresh
14
15 # Schedule recurring transaction pattern identification (debounced to run after all syncs complete)
16 begin
17 RecurringTransaction.identify_patterns_for(family)
18 rescue => e
19 Rails.logger.error("Family::SyncCompleteEvent recurring transaction identification failed: #{e.message}\n#{e.backtrace&.join("\n")}")
20 end
21 end
22 end
1 class Account::SyncCompleteEvent
2 attr_reader :account
3
4 Error = Class.new(StandardError)
5
6 def initialize(account)
7 `@account` = account
8 end
9
10 def broadcast
11 # Replace account row in accounts list
12 account.broadcast_replace_to(
13 account.family,
14 target: "account_#{account.id}",
15 partial: "accounts/account",
16 locals: { account: account }
17 )
18
19 # If this is a manual, unlinked account (i.e. not part of a Plaid Item),
20 # trigger the family sync complete broadcast so net worth graph is updated
21 unless account.linked?
22 account.family.broadcast_sync_complete
23 end
24
25 # Refresh entire account page (only applies if currently viewing this account)
26 account.broadcast_refresh
27 end
28 end
Remove the unsupported requestId: parameter—broadcast_refresh_to does not accept it.
The broadcast_refresh_to method does not have a request_id parameter. Only the async variant broadcast_refresh_later_to supports request_id: for deduplication. Passing requestId: to broadcast_refresh_to has it silently ignored as an unknown option, so no Turbo deduplication occurs. Either remove the parameter to match the pattern used in Family::SyncCompleteEvent and Account::SyncCompleteEvent (family.broadcast_refresh), or switch to broadcast_refresh_later_to if deduplication is needed.
Proposed fix
Turbo::StreamsChannel.broadcast_refresh_to(
- `@connection.family`,
- requestId: SecureRandom.uuid
+ `@connection.family`
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/provider/connection/sync_complete_event.rb` around lines 9 - 12,
The call to Turbo::StreamsChannel.broadcast_refresh_to is passing an unsupported
requestId: option; remove the requestId parameter or replace the call with
Turbo::StreamsChannel.broadcast_refresh_later_to and pass request_id:
SecureRandom.uuid if you need Turbo deduplication. Locate the invocation in
sync_complete_event.rb (the call to Turbo::StreamsChannel.broadcast_refresh_to)
and either change it to just broadcast_refresh_to(`@connection.family`) to match
Family::SyncCompleteEvent / Account::SyncCompleteEvent patterns, or switch to
broadcast_refresh_later_to(`@connection.family`, request_id: SecureRandom.uuid) to
enable deduplication.
582e9e6 to
fb0c8eb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/controllers/provider_connections_controller.rb (1)
70-80: 💤 Low valueConsider wrapping both branches in the transaction.
The
linkaction wraps the "new" account creation in a transaction (lines 71-75) but the existing-account linking (line 79) happens outside. Ifsync_laterat line 82 were to fail before completion, both paths should have consistent transactional guarantees.Suggested improvement
def link pa = find_provider_account_for_family(params[:provider_account_id]) return head :not_found unless pa - if params[:account_id] == "new" - ActiveRecord::Base.transaction do - account = pa.build_sure_account(family: Current.family) - account.save! - pa.update!(account_id: account.id, skipped: false) - end - else - target = Current.family.accounts.find_by(id: params[:account_id]) - return head :unprocessable_entity unless target - pa.update!(account_id: target.id, skipped: false) - end + ActiveRecord::Base.transaction do + if params[:account_id] == "new" + account = pa.build_sure_account(family: Current.family) + account.save! + pa.update!(account_id: account.id, skipped: false) + else + target = Current.family.accounts.find_by(id: params[:account_id]) + return head :unprocessable_entity unless target + pa.update!(account_id: target.id, skipped: false) + end + end pa.provider_connection.sync_later🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/provider_connections_controller.rb` around lines 70 - 80, The link action currently opens an ActiveRecord::Base.transaction only for the "new" account branch (pa.build_sure_account and account.save!), leaving the existing-account branch (Current.family.accounts.find_by and pa.update!) outside the transaction and before the subsequent sync_later call; wrap the entire if/else (both the pa.build_sure_account/account.save!/pa.update! path and the Current.family.accounts.find_by/pa.update! path) inside a single ActiveRecord::Base.transaction so both linking flows are atomic with respect to the later sync_later; ensure the target account lookup (Current.family.accounts.find_by) happens inside that transaction and handle the not-found case by raising or rolling back (instead of returning) so the transaction can be aborted consistently.app/controllers/concerns/accountable_resource.rb (1)
83-92: Remove the duplicate configs concern; the two registries are mutually exclusive. Consider extracting the oauth augmentation pattern to eliminate code duplication.The
Provider::FactoryandProvider::Registryregistries are entirely separate—Factory uses its own@registryhash for account adapters, while Registry maintains a separate@oauth_providershash. No risk of duplicate configs.However, the 5-line oauth augmentation block (lines 88–92) is identical to
accounts_controller.rblines 195–199. Extracting this to a unified method likeProvider::Factory.all_connection_configs_for_account_typewould eliminate duplication and keep the two call sites in sync:♻️ Suggested extraction
# app/models/provider/factory.rb def self.all_connection_configs_for_account_type(account_type:, family:) configs = connection_configs_for_account_type(account_type: account_type, family: family) configs + Registry.oauth_provider_keys.flat_map do |key| adapter = Registry.oauth_provider_adapter(key) next [] unless adapter.supported_account_types.include?(account_type) adapter.connection_configs(family: family) end endThen simplify both call sites:
-@provider_configs = Provider::Factory.connection_configs_for_account_type( - account_type: account_type_name, - family: Current.family -) - -@provider_configs += Provider::Registry.oauth_provider_keys.flat_map do |key| - adapter = Provider::Registry.oauth_provider_adapter(key) - next [] unless adapter.supported_account_types.include?(account_type_name) - adapter.connection_configs(family: Current.family) -end +@provider_configs = Provider::Factory.all_connection_configs_for_account_type( + account_type: account_type_name, + family: Current.family +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/concerns/accountable_resource.rb` around lines 83 - 92, The oauth-augmentation block duplicates logic and is unnecessary because Factory and Registry are mutually exclusive; extract the shared behavior into a single factory method and update callers. Add a new method Provider::Factory.all_connection_configs_for_account_type(account_type:, family:) that calls the existing Provider::Factory.connection_configs_for_account_type(...) and then iterates Provider::Registry.oauth_provider_keys / Provider::Registry.oauth_provider_adapter(key) to append adapter.connection_configs(family: Current.family) only when adapter.supported_account_types.include?(account_type). Replace the inline oauth flat_map in accountable_resource (and the duplicate block in AccountsController) with a call to Provider::Factory.all_connection_configs_for_account_type to keep logic centralized and prevent future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/settings/bank_sync/_provider_link.html.erb`:
- Line 16: Replace the hardcoded "beta" string inside the span element in the
_provider_link.html.erb partial with a call to the i18n helper t(), e.g.
t('providers.beta'), and add the corresponding key providers.beta: "beta" to
config/locales/en.yml; ensure the span still renders the t() output and
preserves the existing classes (px-1.5 py-0.5 rounded text-xs font-medium
bg-warning/10 text-warning).
In `@test/fixtures/accounts.yml`:
- Around line 103-113: Remove the permanent truelayer_depository fixture from
accounts.yml and instead instantiate that provider-specific Depository account
within the TrueLayer-specific test or its setup/let block; reuse the base
accounts(:depository) when possible or create a new Account/Depository via
Account.create! and Depository.create! (or equivalent factory) in the test so
the provider edge-case is scoped to tests that need it rather than adding a
global truelayer_depository fixture.
In `@test/models/provider/truelayer_adapter_test.rb`:
- Around line 95-98: The oauth_config_for path can fail under lazy-load because
oauth_provider_adapter is called before adapters are loaded; update the Registry
to call Provider::Factory.ensure_adapters_loaded before accessing
`@oauth_providers`: add a call to Provider::Factory.ensure_adapters_loaded at the
start of oauth_config_for (or inside oauth_provider_adapter) so `@oauth_providers`
is populated before oauth_provider_adapter/lookups run, ensuring
oauth_config_for and other callers reliably find the registered adapter (e.g.,
Provider::TruelayerAdapter).
In `@test/models/provider/truelayer_syncer_test.rb`:
- Around line 86-97: Rename the test method so the name accurately reflects the
expectation that Account#sync_later is invoked exactly once via
set_current_balance when the balance anchor succeeds; update the test method
name (currently "sync_later not called when balance anchor succeeds") to
something like "sync_later called once via set_current_balance when balance
anchor succeeds" and leave the body intact (references: test method name,
Account.any_instance.expects(:sync_later).once, set_current_balance,
`@syncer.perform_sync`).
---
Nitpick comments:
In `@app/controllers/concerns/accountable_resource.rb`:
- Around line 83-92: The oauth-augmentation block duplicates logic and is
unnecessary because Factory and Registry are mutually exclusive; extract the
shared behavior into a single factory method and update callers. Add a new
method Provider::Factory.all_connection_configs_for_account_type(account_type:,
family:) that calls the existing
Provider::Factory.connection_configs_for_account_type(...) and then iterates
Provider::Registry.oauth_provider_keys /
Provider::Registry.oauth_provider_adapter(key) to append
adapter.connection_configs(family: Current.family) only when
adapter.supported_account_types.include?(account_type). Replace the inline oauth
flat_map in accountable_resource (and the duplicate block in AccountsController)
with a call to Provider::Factory.all_connection_configs_for_account_type to keep
logic centralized and prevent future drift.
In `@app/controllers/provider_connections_controller.rb`:
- Around line 70-80: The link action currently opens an
ActiveRecord::Base.transaction only for the "new" account branch
(pa.build_sure_account and account.save!), leaving the existing-account branch
(Current.family.accounts.find_by and pa.update!) outside the transaction and
before the subsequent sync_later call; wrap the entire if/else (both the
pa.build_sure_account/account.save!/pa.update! path and the
Current.family.accounts.find_by/pa.update! path) inside a single
ActiveRecord::Base.transaction so both linking flows are atomic with respect to
the later sync_later; ensure the target account lookup
(Current.family.accounts.find_by) happens inside that transaction and handle the
not-found case by raising or rolling back (instead of returning) so the
transaction can be aborted consistently.
🪄 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: e8cd09aa-1972-422f-80c8-ae6b260113fc
📒 Files selected for processing (62)
app/controllers/accounts_controller.rbapp/controllers/concerns/accountable_resource.rbapp/controllers/oauth_callbacks_controller.rbapp/controllers/provider_connections_controller.rbapp/controllers/provider_family_configs_controller.rbapp/controllers/settings/bank_sync_controller.rbapp/jobs/provider/consent_expiry_check_job.rbapp/models/account.rbapp/models/account/provider_import_adapter.rbapp/models/data_enrichment.rbapp/models/family.rbapp/models/family/syncer.rbapp/models/provider/account.rbapp/models/provider/auth.rbapp/models/provider/auth/oauth2.rbapp/models/provider/connection.rbapp/models/provider/connection/sync_complete_event.rbapp/models/provider/family_config.rbapp/models/provider/registry.rbapp/models/provider/truelayer.rbapp/models/provider/truelayer_adapter.rbapp/models/provider/truelayer_syncer.rbapp/models/provider_merchant.rbapp/models/transaction.rbapp/views/accounts/index.html.erbapp/views/provider_connections/_connection_card.html.erbapp/views/provider_connections/select.html.erbapp/views/provider_connections/setup.html.erbapp/views/provider_connections/show.html.erbapp/views/provider_family_configs/_truelayer_instructions.html.erbapp/views/provider_family_configs/edit.html.erbapp/views/provider_family_configs/new.html.erbapp/views/settings/bank_sync/_provider_link.html.erbapp/views/settings/providers/_oauth_provider_panel.html.erbapp/views/settings/providers/show.html.erbconfig/brakeman.ignoreconfig/initializers/inflections.rbconfig/initializers/sidekiq.rbconfig/locales/views/provider_connections/en.ymlconfig/locales/views/provider_family_configs/en.ymlconfig/locales/views/settings/en.ymlconfig/routes.rbdb/migrate/20260502153816_create_provider_tables.rbdb/schema.rbtest/controllers/oauth_callbacks_controller_test.rbtest/controllers/provider_connections_controller_test.rbtest/controllers/provider_family_configs_controller_test.rbtest/controllers/settings/providers_controller_test.rbtest/fixtures/accounts.ymltest/fixtures/provider_accounts.ymltest/fixtures/provider_connections.ymltest/fixtures/provider_family_configs.ymltest/jobs/provider/consent_expiry_check_job_test.rbtest/models/account_test.rbtest/models/family/syncer_test.rbtest/models/provider/account_test.rbtest/models/provider/auth/oauth2_test.rbtest/models/provider/connection_test.rbtest/models/provider/family_config_test.rbtest/models/provider/truelayer_adapter_test.rbtest/models/provider/truelayer_syncer_test.rbtest/models/provider/truelayer_test.rb
💤 Files with no reviewable changes (1)
- app/models/provider_merchant.rb
✅ Files skipped from review due to trivial changes (24)
- test/fixtures/provider_accounts.yml
- config/initializers/inflections.rb
- app/views/provider_connections/setup.html.erb
- test/fixtures/provider_family_configs.yml
- app/models/provider/auth.rb
- test/models/account_test.rb
- app/views/provider_family_configs/_truelayer_instructions.html.erb
- test/fixtures/provider_connections.yml
- test/jobs/provider/consent_expiry_check_job_test.rb
- app/views/provider_family_configs/edit.html.erb
- test/controllers/oauth_callbacks_controller_test.rb
- app/views/settings/providers/_oauth_provider_panel.html.erb
- db/migrate/20260502153816_create_provider_tables.rb
- test/models/family/syncer_test.rb
- app/models/provider/connection/sync_complete_event.rb
- app/views/provider_connections/show.html.erb
- app/views/provider_connections/_connection_card.html.erb
- test/models/provider/account_test.rb
- config/brakeman.ignore
- config/locales/views/provider_connections/en.yml
- app/models/family.rb
- test/controllers/provider_connections_controller_test.rb
- app/models/family/syncer.rb
- test/models/provider/auth/oauth2_test.rb
🚧 Files skipped from review as they are similar to previous changes (25)
- config/locales/views/settings/en.yml
- config/locales/views/provider_family_configs/en.yml
- app/controllers/accounts_controller.rb
- app/models/transaction.rb
- test/controllers/provider_family_configs_controller_test.rb
- app/models/provider/registry.rb
- app/models/data_enrichment.rb
- app/views/settings/providers/show.html.erb
- app/views/provider_connections/select.html.erb
- app/controllers/settings/bank_sync_controller.rb
- test/controllers/settings/providers_controller_test.rb
- app/controllers/oauth_callbacks_controller.rb
- app/jobs/provider/consent_expiry_check_job.rb
- test/models/provider/connection_test.rb
- app/models/account/provider_import_adapter.rb
- config/initializers/sidekiq.rb
- app/models/provider/connection.rb
- test/models/provider/truelayer_test.rb
- app/controllers/provider_family_configs_controller.rb
- app/models/provider/truelayer_adapter.rb
- app/models/provider/auth/oauth2.rb
- app/models/provider/truelayer_syncer.rb
- db/schema.rb
- app/models/provider/account.rb
- app/models/provider/truelayer.rb
| <%= provider_link[:name] %> | ||
| </p> | ||
| <% if provider_link[:beta] %> | ||
| <span class="px-1.5 py-0.5 rounded text-xs font-medium bg-warning/10 text-warning">beta</span> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use t() helper for the "beta" string.
Hardcoded user-facing string "beta" bypasses i18n. As per coding guidelines, always use the t() helper for user-facing strings and add the key to config/locales/en.yml.
♻️ Proposed fix
In config/locales/en.yml, add:
# under an appropriate namespace, e.g.:
providers:
beta: "beta"- <span class="px-1.5 py-0.5 rounded text-xs font-medium bg-warning/10 text-warning">beta</span>
+ <span class="px-1.5 py-0.5 rounded text-xs font-medium bg-warning/10 text-warning"><%= t("providers.beta") %></span>As per coding guidelines: "Always use t() helper for user-facing strings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/settings/bank_sync/_provider_link.html.erb` at line 16, Replace the
hardcoded "beta" string inside the span element in the _provider_link.html.erb
partial with a call to the i18n helper t(), e.g. t('providers.beta'), and add
the corresponding key providers.beta: "beta" to config/locales/en.yml; ensure
the span still renders the t() output and preserves the existing classes (px-1.5
py-0.5 rounded text-xs font-medium bg-warning/10 text-warning).
Comment on lines
+103
to
+113
| # Linked to provider_accounts(:monzo_current). Keep separate from :depository | ||
| # so that fixture stays in the manual scope for unrelated tests. | ||
| truelayer_depository: | ||
| family: dylan_family | ||
| owner: family_admin | ||
| name: Monzo Current Account | ||
| balance: 1500 | ||
| currency: GBP | ||
| accountable_type: Depository | ||
| accountable: one | ||
| status: active |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Guideline violation: provider-specific edge-case fixture should be created on the fly, not as a permanent fixture.
The new truelayer_depository is a second Depository-typed account that differs from the existing depository fixture only in its provider-account association. The inline comment even calls out the isolation concern — that is the canonical signal that this is a test-scoped edge case, not a shared base case.
Per the fixture guidelines, edge cases should be created inside the test (or its setup/let block) that requires them. Leaving it here means every future provider integration is likely to add another Depository variant, and developers reading unrelated account tests will encounter surprising extra rows.
Consider removing this fixture and building the account inline within the TrueLayer-specific test context, e.g.:
# In the relevant TrueLayer test
account = accounts(:depository) # reuse base case, or:
account = Account.create!(
family: families(:dylan_family),
owner: users(:family_admin),
name: "Monzo Current Account",
balance: 1500,
currency: "GBP",
accountable: Depository.create!,
status: "active"
)As per coding guidelines: "Edge cases should be created on the fly within the context of the test which it is needed."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/fixtures/accounts.yml` around lines 103 - 113, Remove the permanent
truelayer_depository fixture from accounts.yml and instead instantiate that
provider-specific Depository account within the TrueLayer-specific test or its
setup/let block; reuse the base accounts(:depository) when possible or create a
new Account/Depository via Account.create! and Depository.create! (or equivalent
factory) in the test so the provider edge-case is scoped to tests that need it
rather than adding a global truelayer_depository fixture.
Comment on lines
+86
to
+97
| test "sync_later not called when balance anchor succeeds" do | ||
| # Isolate to a single linked provider_account so assertion counts are deterministic. | ||
| # Skip (rather than unlink) monzo_current so pending_setup? stays false. | ||
| provider_accounts(:monzo_current).update!(account: nil, skipped: true) | ||
| provider_accounts(:monzo_unlinked).update!(account: accounts(:depository)) | ||
| Provider::Auth::OAuth2.any_instance.stubs(:fresh_access_token).returns("tok") | ||
| Provider::TruelayerAdapter.any_instance.stubs(:fetch_accounts).returns([]) | ||
| Provider::TruelayerAdapter.any_instance.stubs(:fetch_transactions).returns([]) | ||
| Provider::TruelayerAdapter.any_instance.stubs(:fetch_balance).returns({ "current" => 100.0 }) | ||
| Account.any_instance.expects(:sync_later).once # triggered by set_current_balance, not explicitly | ||
| @syncer.perform_sync(@sync) | ||
| end |
Contributor
There was a problem hiding this comment.
Misleading test name contradicts its own assertion.
The name says sync_later is not called when anchor succeeds, but the assertion is expects(:sync_later).once. A reader (or future maintainer) following only the name could incorrectly change the expectation to never. The intent — that sync_later fires exactly once via set_current_balance, not via the explicit error-recovery path — should be expressed in the name itself.
✏️ Suggested rename
- test "sync_later not called when balance anchor succeeds" do
+ test "sync_later triggered once by set_current_balance when balance anchor succeeds" do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test "sync_later not called when balance anchor succeeds" do | |
| # Isolate to a single linked provider_account so assertion counts are deterministic. | |
| # Skip (rather than unlink) monzo_current so pending_setup? stays false. | |
| provider_accounts(:monzo_current).update!(account: nil, skipped: true) | |
| provider_accounts(:monzo_unlinked).update!(account: accounts(:depository)) | |
| Provider::Auth::OAuth2.any_instance.stubs(:fresh_access_token).returns("tok") | |
| Provider::TruelayerAdapter.any_instance.stubs(:fetch_accounts).returns([]) | |
| Provider::TruelayerAdapter.any_instance.stubs(:fetch_transactions).returns([]) | |
| Provider::TruelayerAdapter.any_instance.stubs(:fetch_balance).returns({ "current" => 100.0 }) | |
| Account.any_instance.expects(:sync_later).once # triggered by set_current_balance, not explicitly | |
| @syncer.perform_sync(@sync) | |
| end | |
| test "sync_later triggered once by set_current_balance when balance anchor succeeds" do | |
| # Isolate to a single linked provider_account so assertion counts are deterministic. | |
| # Skip (rather than unlink) monzo_current so pending_setup? stays false. | |
| provider_accounts(:monzo_current).update!(account: nil, skipped: true) | |
| provider_accounts(:monzo_unlinked).update!(account: accounts(:depository)) | |
| Provider::Auth::OAuth2.any_instance.stubs(:fresh_access_token).returns("tok") | |
| Provider::TruelayerAdapter.any_instance.stubs(:fetch_accounts).returns([]) | |
| Provider::TruelayerAdapter.any_instance.stubs(:fetch_transactions).returns([]) | |
| Provider::TruelayerAdapter.any_instance.stubs(:fetch_balance).returns({ "current" => 100.0 }) | |
| Account.any_instance.expects(:sync_later).once # triggered by set_current_balance, not explicitly | |
| `@syncer.perform_sync`(`@sync`) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/models/provider/truelayer_syncer_test.rb` around lines 86 - 97, Rename
the test method so the name accurately reflects the expectation that
Account#sync_later is invoked exactly once via set_current_balance when the
balance anchor succeeds; update the test method name (currently "sync_later not
called when balance anchor succeeds") to something like "sync_later called once
via set_current_balance when balance anchor succeeds" and leave the body intact
(references: test method name, Account.any_instance.expects(:sync_later).once,
set_current_balance, `@syncer.perform_sync`).
fb0c8eb to
85ef92a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/views/provider_connections/_connection_card.html.erb (1)
18-18:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHandle nil
institution_namein avatar fallback to prevent render crash.Line 18 calls
.firstonconnection.institution_name; when that value isnil, this raisesNoMethodErrorand breaks the card render.Proposed fix
- <%= tag.p connection.institution_name.first.upcase, class: "text-success text-xs font-medium" %> + <%= tag.p(connection.institution_name&.first&.upcase || "?", class: "text-success text-xs font-medium") %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/provider_connections/_connection_card.html.erb` at line 18, In the _connection_card.html.erb view the expression connection.institution_name.first can raise NoMethodError when institution_name is nil; update the tag.p usage to safely handle nil by deriving the first character defensively (e.g. use safe navigation or to_s/presence) and then upcase it before rendering so tag.p receives a string (for example, compute a fallback like (connection.institution_name || "").first&.upcase or connection.institution_name.to_s.first.upcase) and render that value instead of calling .first directly on connection.institution_name.
🧹 Nitpick comments (1)
test/models/provider/auth/oauth2_test.rb (1)
20-20: Prefer instance-scoped expectations overany_instancefor refresh stubs.These tests can be made less brittle by scoping the expectation to
@authinstead of allProvider::Auth::OAuth2instances.Suggested refactor
- Provider::Auth::OAuth2.any_instance.expects(:fetch_new_tokens).returns(mock_tokens) + `@auth.expects`(:fetch_new_tokens).returns(mock_tokens) @@ - Provider::Auth::OAuth2.any_instance.expects(:fetch_new_tokens).raises(Provider::Auth::ConsentExpiredError) + `@auth.expects`(:fetch_new_tokens).raises(Provider::Auth::ConsentExpiredError) @@ - Provider::Auth::OAuth2.any_instance.expects(:fetch_new_tokens).raises(Provider::Auth::TokenRevokedError) + `@auth.expects`(:fetch_new_tokens).raises(Provider::Auth::TokenRevokedError)Also applies to: 28-28, 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/provider/auth/oauth2_test.rb` at line 20, Replace the global stub on Provider::Auth::OAuth2.any_instance with an instance-scoped expectation on the test's `@auth` object: locate the tests that call Provider::Auth::OAuth2.any_instance.expects(:fetch_new_tokens) (and the similar occurrences mentioned) and change them to `@auth.expects`(:fetch_new_tokens).returns(mock_tokens) so only that specific OAuth2 instance is stubbed; ensure you update all occurrences (the ones around lines 20, 28 and 35) referencing Provider::Auth::OAuth2 and the fetch_new_tokens method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/settings/providers/_oauth_provider_panel.html.erb`:
- Around line 4-5: The query that builds connections (the local variable
connections from
Current.family.provider_connections.where(provider_family_config:
config).includes(:provider_accounts)) must be narrowed to only
connected/displayable states so pending connections created by
OauthCallbacksController#new do not render as "Connected"; update that call to
use your model scope for visible connections (e.g. .displayable or .connected)
or explicitly exclude pending states (e.g. add .where.not(status: :pending) or
.where(status: ProviderConnection.displayable_states)) before
.includes(:provider_accounts); apply the same change to the other occurrences
that build connections in this partial.
In `@config/initializers/sidekiq.rb`:
- Around line 72-80: The cron job registration currently logs success
unconditionally; capture the return value of Sidekiq::Cron::Job.create (for the
job named "consent_expiry_check" for class "Provider::ConsentExpiryCheckJob")
and validate it before logging success: if the create call returns false, log an
error (including failure context) and handle it similarly to
app/services/auto_sync_scheduler.rb (do not log success or assume registration
if false), otherwise log the successful registration; keep the existing rescue
block for exceptions.
---
Duplicate comments:
In `@app/views/provider_connections/_connection_card.html.erb`:
- Line 18: In the _connection_card.html.erb view the expression
connection.institution_name.first can raise NoMethodError when institution_name
is nil; update the tag.p usage to safely handle nil by deriving the first
character defensively (e.g. use safe navigation or to_s/presence) and then
upcase it before rendering so tag.p receives a string (for example, compute a
fallback like (connection.institution_name || "").first&.upcase or
connection.institution_name.to_s.first.upcase) and render that value instead of
calling .first directly on connection.institution_name.
---
Nitpick comments:
In `@test/models/provider/auth/oauth2_test.rb`:
- Line 20: Replace the global stub on Provider::Auth::OAuth2.any_instance with
an instance-scoped expectation on the test's `@auth` object: locate the tests that
call Provider::Auth::OAuth2.any_instance.expects(:fetch_new_tokens) (and the
similar occurrences mentioned) and change them to
`@auth.expects`(:fetch_new_tokens).returns(mock_tokens) so only that specific
OAuth2 instance is stubbed; ensure you update all occurrences (the ones around
lines 20, 28 and 35) referencing Provider::Auth::OAuth2 and the fetch_new_tokens
method.
🪄 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: 542abbc0-9fe0-4bc2-9f90-60034d4a6f7a
📒 Files selected for processing (62)
app/controllers/accounts_controller.rbapp/controllers/concerns/accountable_resource.rbapp/controllers/oauth_callbacks_controller.rbapp/controllers/provider_connections_controller.rbapp/controllers/provider_family_configs_controller.rbapp/controllers/settings/bank_sync_controller.rbapp/jobs/provider/consent_expiry_check_job.rbapp/models/account.rbapp/models/account/provider_import_adapter.rbapp/models/data_enrichment.rbapp/models/family.rbapp/models/family/syncer.rbapp/models/provider/account.rbapp/models/provider/auth.rbapp/models/provider/auth/oauth2.rbapp/models/provider/connection.rbapp/models/provider/connection/sync_complete_event.rbapp/models/provider/family_config.rbapp/models/provider/registry.rbapp/models/provider/truelayer.rbapp/models/provider/truelayer_adapter.rbapp/models/provider/truelayer_syncer.rbapp/models/provider_merchant.rbapp/models/transaction.rbapp/views/accounts/index.html.erbapp/views/provider_connections/_connection_card.html.erbapp/views/provider_connections/select.html.erbapp/views/provider_connections/setup.html.erbapp/views/provider_connections/show.html.erbapp/views/provider_family_configs/_truelayer_instructions.html.erbapp/views/provider_family_configs/edit.html.erbapp/views/provider_family_configs/new.html.erbapp/views/settings/bank_sync/_provider_link.html.erbapp/views/settings/providers/_oauth_provider_panel.html.erbapp/views/settings/providers/show.html.erbconfig/brakeman.ignoreconfig/initializers/inflections.rbconfig/initializers/sidekiq.rbconfig/locales/views/provider_connections/en.ymlconfig/locales/views/provider_family_configs/en.ymlconfig/locales/views/settings/en.ymlconfig/routes.rbdb/migrate/20260502153816_create_provider_tables.rbdb/schema.rbtest/controllers/oauth_callbacks_controller_test.rbtest/controllers/provider_connections_controller_test.rbtest/controllers/provider_family_configs_controller_test.rbtest/controllers/settings/providers_controller_test.rbtest/fixtures/accounts.ymltest/fixtures/provider_accounts.ymltest/fixtures/provider_connections.ymltest/fixtures/provider_family_configs.ymltest/jobs/provider/consent_expiry_check_job_test.rbtest/models/account_test.rbtest/models/family/syncer_test.rbtest/models/provider/account_test.rbtest/models/provider/auth/oauth2_test.rbtest/models/provider/connection_test.rbtest/models/provider/family_config_test.rbtest/models/provider/truelayer_adapter_test.rbtest/models/provider/truelayer_syncer_test.rbtest/models/provider/truelayer_test.rb
💤 Files with no reviewable changes (1)
- app/models/provider_merchant.rb
✅ Files skipped from review due to trivial changes (34)
- test/fixtures/provider_family_configs.yml
- test/fixtures/provider_accounts.yml
- test/fixtures/provider_connections.yml
- config/locales/views/provider_family_configs/en.yml
- app/models/provider/connection/sync_complete_event.rb
- app/models/provider/auth.rb
- config/brakeman.ignore
- test/fixtures/accounts.yml
- test/jobs/provider/consent_expiry_check_job_test.rb
- app/views/provider_family_configs/edit.html.erb
- app/controllers/accounts_controller.rb
- app/views/provider_connections/setup.html.erb
- app/views/settings/bank_sync/_provider_link.html.erb
- test/controllers/provider_family_configs_controller_test.rb
- test/models/provider/account_test.rb
- config/locales/views/settings/en.yml
- app/views/provider_family_configs/new.html.erb
- app/views/provider_connections/select.html.erb
- app/views/settings/providers/show.html.erb
- app/models/data_enrichment.rb
- config/routes.rb
- test/controllers/settings/providers_controller_test.rb
- config/locales/views/provider_connections/en.yml
- app/models/account/provider_import_adapter.rb
- app/models/family/syncer.rb
- db/migrate/20260502153816_create_provider_tables.rb
- app/views/provider_family_configs/_truelayer_instructions.html.erb
- test/controllers/provider_connections_controller_test.rb
- test/models/account_test.rb
- app/models/provider/truelayer_syncer.rb
- app/models/provider/auth/oauth2.rb
- app/models/provider/connection.rb
- test/models/provider/truelayer_test.rb
- app/models/provider/truelayer.rb
🚧 Files skipped from review as they are similar to previous changes (20)
- app/views/accounts/index.html.erb
- app/controllers/settings/bank_sync_controller.rb
- config/initializers/inflections.rb
- app/models/provider/registry.rb
- test/models/provider/family_config_test.rb
- test/models/provider/connection_test.rb
- test/controllers/oauth_callbacks_controller_test.rb
- app/models/transaction.rb
- app/controllers/oauth_callbacks_controller.rb
- app/views/provider_connections/show.html.erb
- app/models/provider/family_config.rb
- app/jobs/provider/consent_expiry_check_job.rb
- app/models/provider/account.rb
- test/models/family/syncer_test.rb
- test/models/provider/truelayer_syncer_test.rb
- app/controllers/provider_connections_controller.rb
- app/controllers/provider_family_configs_controller.rb
- test/models/provider/truelayer_adapter_test.rb
- db/schema.rb
- app/models/provider/truelayer_adapter.rb
Comment on lines
+4
to
+5
| <% connections = Current.family.provider_connections.where(provider_family_config: config).includes(:provider_accounts) %> | ||
| <% if connections.any? %> |
Contributor
There was a problem hiding this comment.
Filter out non-connected states before rendering “Connected”.
OauthCallbacksController#new creates status: :pending connections; with the current query, those can show up as connected and even get a “status good” badge. Please scope connections to connected/displayable states before Line 5.
Proposed fix
- <% connections = Current.family.provider_connections.where(provider_family_config: config).includes(:provider_accounts) %>
+ <% connections = Current.family.provider_connections
+ .where(provider_family_config: config)
+ .where.not(status: :pending) %>
@@
- <p class="text-sm text-secondary"><%= t("settings.providers.oauth_provider_panel.connected", count: connections.count) %></p>
+ <p class="text-sm text-secondary"><%= t("settings.providers.oauth_provider_panel.connected", count: connections.size) %></p>Also applies to: 8-8, 20-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/settings/providers/_oauth_provider_panel.html.erb` around lines 4 -
5, The query that builds connections (the local variable connections from
Current.family.provider_connections.where(provider_family_config:
config).includes(:provider_accounts)) must be narrowed to only
connected/displayable states so pending connections created by
OauthCallbacksController#new do not render as "Connected"; update that call to
use your model scope for visible connections (e.g. .displayable or .connected)
or explicitly exclude pending states (e.g. add .where.not(status: :pending) or
.where(status: ProviderConnection.displayable_states)) before
.includes(:provider_accounts); apply the same change to the other occurrences
that build connections in this partial.
Comment on lines
+72
to
+80
| Sidekiq::Cron::Job.create( | ||
| name: "consent_expiry_check", | ||
| cron: "0 6 * * *", | ||
| class: "Provider::ConsentExpiryCheckJob", | ||
| queue: "scheduled", | ||
| description: "Marks provider connections with expiring consent as requires_update" | ||
| ) | ||
| Rails.logger.info("[ConsentExpiryCheckJob] Registered consent_expiry_check cron job") | ||
| rescue => e |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For sidekiq-cron (the version in this repo), what does Sidekiq::Cron::Job.create return when job validation fails or creation is rejected? Does it return false/nil without raising?
💡 Result:
Sidekiq::Cron::Job.create returns false when job validation fails or creation is rejected. It does not raise an exception; instead, it returns false without raising, allowing the caller to check job.errors for details.
Citations:
- 1: https://github.com/sidekiq-cron/sidekiq-cron/blob/master/lib/sidekiq/cron/job.rb
- 2: http://github.com/sidekiq-cron/sidekiq-cron
- 3: https://github.com/sidekiq-cron/sidekiq-cron/blob/8c25ca29/test/unit/job_test.rb
- 4: https://www.rubydoc.info/gems/sidekiq-cron/Sidekiq/Cron/Job
🏁 Script executed:
# Check if the file exists and read the relevant section
if [ -f config/initializers/sidekiq.rb ]; then
echo "=== File exists, showing lines 70-82 ==="
sed -n '70,82p' config/initializers/sidekiq.rb
else
echo "File not found at config/initializers/sidekiq.rb"
# Try to find it
find . -name "sidekiq.rb" -type f 2>/dev/null
fiRepository: we-promise/sure
Length of output: 596
🏁 Script executed:
# Check sidekiq-cron version
echo "=== Checking Gemfile ==="
grep -i "sidekiq-cron" Gemfile 2>/dev/null || echo "Not found in Gemfile"
echo -e "\n=== Checking Gemfile.lock ==="
grep -A 2 "sidekiq-cron" Gemfile.lock 2>/dev/null | head -5 || echo "Not found in Gemfile.lock"
echo -e "\n=== Checking gemspec files ==="
find . -name "*.gemspec" -type f -exec grep -l "sidekiq-cron" {} \; 2>/dev/null || echo "Not found in gemspec files"Repository: we-promise/sure
Length of output: 266
🏁 Script executed:
# Look for similar cron job registrations to understand the pattern
rg "Sidekiq::Cron::Job.create" -A 5 -B 2Repository: we-promise/sure
Length of output: 1116
Check cron creation result before logging success.
Line 79 logs successful registration without validating the Sidekiq::Cron::Job.create result. The sidekiq-cron API returns false on validation failure without raising an exception, so this silently disables consent-expiry checks if creation fails. This pattern is inconsistently applied—app/services/auto_sync_scheduler.rb already captures and validates the return value.
Proposed fix
begin
- Sidekiq::Cron::Job.create(
+ job = Sidekiq::Cron::Job.create(
name: "consent_expiry_check",
cron: "0 6 * * *",
class: "Provider::ConsentExpiryCheckJob",
queue: "scheduled",
description: "Marks provider connections with expiring consent as requires_update"
)
- Rails.logger.info("[ConsentExpiryCheckJob] Registered consent_expiry_check cron job")
+ if job
+ Rails.logger.info("[ConsentExpiryCheckJob] Registered consent_expiry_check cron job")
+ else
+ Rails.logger.error("[ConsentExpiryCheckJob] Cron job registration returned false")
+ end
rescue => e
Rails.logger.error("[ConsentExpiryCheckJob] Failed to register cron job: #{e.message}")
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/initializers/sidekiq.rb` around lines 72 - 80, The cron job
registration currently logs success unconditionally; capture the return value of
Sidekiq::Cron::Job.create (for the job named "consent_expiry_check" for class
"Provider::ConsentExpiryCheckJob") and validate it before logging success: if
the create call returns false, log an error (including failure context) and
handle it similarly to app/services/auto_sync_scheduler.rb (do not log success
or assume registration if false), otherwise log the successful registration;
keep the existing rescue block for exceptions.
85ef92a to
3cc9ab1
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
app/jobs/provider/consent_expiry_check_job.rb (1)
12-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Time.zone.parsecan returnnil, causing a misleadingNoMethodErrorin the rescue log.
Time.zone.parse(raw)returnsnilfor malformed input (it does not always raise). The subsequentexpiry <= ...then raisesNoMethodError: undefined method '<=' for nil, which therescuecatches and logs as a code defect rather than a data-quality issue.🛡️ Proposed fix
expiry = Time.zone.parse(raw) + unless expiry + Rails.logger.warn("[ConsentExpiryCheckJob] Skipping connection #{connection.id}: unparseable consent_expires_at #{raw.inspect}") + next + end if expiry <= EXPIRY_WARNING_WINDOW.from_now🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/jobs/provider/consent_expiry_check_job.rb` around lines 12 - 13, Time.zone.parse(raw) can return nil which causes expiry <= EXPIRY_WARNING_WINDOW.from_now to raise a NoMethodError; update the logic in consent_expiry_check_job (the expiry variable assignment and subsequent comparison) to explicitly handle a nil expiry: after expiry = Time.zone.parse(raw) check for nil and treat it as a parse/data-quality issue (log a clear data-validation message including raw) and skip further processing instead of letting the comparison raise and be caught as a code defect; keep EXPIRY_WARNING_WINDOW and existing rescue for unexpected exceptions unchanged.app/views/settings/providers/_oauth_provider_panel.html.erb (1)
4-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPending connections still appear as "Connected".
The query at Line 4 fetches all connections for the config, including those with
status: :pendingcreated byOauthCallbacksController#new(which creates a pending connection before the OAuth redirect). A user who abandons mid-flow will see a spurious "Connected" entry.Proposed fix
- <% connections = Current.family.provider_connections.where(provider_family_config: config).includes(:provider_accounts) %> + <% connections = Current.family.provider_connections + .where(provider_family_config: config) + .where.not(status: :pending) + .includes(:provider_accounts) %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/settings/providers/_oauth_provider_panel.html.erb` around lines 4 - 5, The query that builds connections (connections = Current.family.provider_connections.where(provider_family_config: config).includes(:provider_accounts)) includes pending records created by OauthCallbacksController#new; update the query to exclude pending connections (e.g., add a condition to filter out status: :pending or explicitly select only connected/active statuses) so the view only shows truly connected provider connections; reference the connections variable and the provider_connections relation when making this change.test/controllers/oauth_callbacks_controller_test.rb (1)
34-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRFC1918 private ranges (10.x, 172.16.x, 192.168.x) remain untested.
CGNAT, link-local, and loopback are all covered, but the three RFC1918 blocks explicitly listed in the PR description as filtered ranges have no representative test cases.
✅ Suggested additional test cases
+ test "new omits psu_ip for RFC1918 (10.x) addresses" do + post new_oauth_callbacks_path(provider: "truelayer"), + headers: { "REMOTE_ADDR" => "10.1.2.3" } + conn = Provider::Connection.order(created_at: :desc).first + assert_nil conn.metadata["psu_ip"] + end + + test "new omits psu_ip for RFC1918 (172.16.x) addresses" do + post new_oauth_callbacks_path(provider: "truelayer"), + headers: { "REMOTE_ADDR" => "172.16.5.4" } + conn = Provider::Connection.order(created_at: :desc).first + assert_nil conn.metadata["psu_ip"] + end + + test "new omits psu_ip for RFC1918 (192.168.x) addresses" do + post new_oauth_callbacks_path(provider: "truelayer"), + headers: { "REMOTE_ADDR" => "192.168.0.1" } + conn = Provider::Connection.order(created_at: :desc).first + assert_nil conn.metadata["psu_ip"] + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/oauth_callbacks_controller_test.rb` around lines 34 - 39, Add tests that cover the RFC1918 private ranges missing from the suite: extend or add test cases similar to test "new omits psu_ip when client IP is private or loopback" that POST to new_oauth_callbacks_path(provider: "truelayer") with headers "REMOTE_ADDR" set to representative addresses from 10.0.0.0/8 (e.g. 10.1.2.3), 172.16.0.0/12 (e.g. 172.16.5.4) and 192.168.0.0/16 (e.g. 192.168.1.1), then fetch Provider::Connection.order(created_at: :desc).first and assert_nil on conn.metadata["psu_ip"] for each case so the RFC1918 ranges are explicitly validated.
🧹 Nitpick comments (1)
app/views/provider_connections/show.html.erb (1)
26-26: 💤 Low value
status.humanizebypasses i18n; wrap int()or use an enum translation.
@connection.status.humanizeemits raw Ruby-humanized strings (e.g.,"Requires update") that won't be translated. The same applies topa.external_subtype&.humanizeat Line 62. Railsenumtranslation keys (activerecord.attributes.provider/connection.status.*) or a dedicated helper keep these localizable.As per coding guidelines: "Always use
t()helper for user-facing strings."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/provider_connections/show.html.erb` at line 26, Replace user-facing calls to `@connection.status.humanize` and pa.external_subtype&.humanize with i18n-backed translations: look up the enum value and call the translation key (e.g. use I18n.t with the activerecord enum keys such as activerecord.attributes.provider/connection.status.<status> for `@connection.status` and a corresponding activerecord key for external_subtype), or call a helper that maps enum values to t(...) so the displayed strings are localizable; update the view references to use those translation keys instead of .humanize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/jobs/provider/consent_expiry_check_job.rb`:
- Around line 12-13: Time.zone.parse(raw) can return nil which causes expiry <=
EXPIRY_WARNING_WINDOW.from_now to raise a NoMethodError; update the logic in
consent_expiry_check_job (the expiry variable assignment and subsequent
comparison) to explicitly handle a nil expiry: after expiry =
Time.zone.parse(raw) check for nil and treat it as a parse/data-quality issue
(log a clear data-validation message including raw) and skip further processing
instead of letting the comparison raise and be caught as a code defect; keep
EXPIRY_WARNING_WINDOW and existing rescue for unexpected exceptions unchanged.
In `@app/views/settings/providers/_oauth_provider_panel.html.erb`:
- Around line 4-5: The query that builds connections (connections =
Current.family.provider_connections.where(provider_family_config:
config).includes(:provider_accounts)) includes pending records created by
OauthCallbacksController#new; update the query to exclude pending connections
(e.g., add a condition to filter out status: :pending or explicitly select only
connected/active statuses) so the view only shows truly connected provider
connections; reference the connections variable and the provider_connections
relation when making this change.
In `@test/controllers/oauth_callbacks_controller_test.rb`:
- Around line 34-39: Add tests that cover the RFC1918 private ranges missing
from the suite: extend or add test cases similar to test "new omits psu_ip when
client IP is private or loopback" that POST to
new_oauth_callbacks_path(provider: "truelayer") with headers "REMOTE_ADDR" set
to representative addresses from 10.0.0.0/8 (e.g. 10.1.2.3), 172.16.0.0/12 (e.g.
172.16.5.4) and 192.168.0.0/16 (e.g. 192.168.1.1), then fetch
Provider::Connection.order(created_at: :desc).first and assert_nil on
conn.metadata["psu_ip"] for each case so the RFC1918 ranges are explicitly
validated.
---
Nitpick comments:
In `@app/views/provider_connections/show.html.erb`:
- Line 26: Replace user-facing calls to `@connection.status.humanize` and
pa.external_subtype&.humanize with i18n-backed translations: look up the enum
value and call the translation key (e.g. use I18n.t with the activerecord enum
keys such as activerecord.attributes.provider/connection.status.<status> for
`@connection.status` and a corresponding activerecord key for external_subtype),
or call a helper that maps enum values to t(...) so the displayed strings are
localizable; update the view references to use those translation keys instead of
.humanize.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a75bae61-df43-4e65-841e-20731e8a7d16
📒 Files selected for processing (62)
app/controllers/accounts_controller.rbapp/controllers/concerns/accountable_resource.rbapp/controllers/oauth_callbacks_controller.rbapp/controllers/provider_connections_controller.rbapp/controllers/provider_family_configs_controller.rbapp/controllers/settings/bank_sync_controller.rbapp/jobs/provider/consent_expiry_check_job.rbapp/models/account.rbapp/models/account/provider_import_adapter.rbapp/models/data_enrichment.rbapp/models/family.rbapp/models/family/syncer.rbapp/models/provider/account.rbapp/models/provider/auth.rbapp/models/provider/auth/oauth2.rbapp/models/provider/connection.rbapp/models/provider/connection/sync_complete_event.rbapp/models/provider/family_config.rbapp/models/provider/registry.rbapp/models/provider/truelayer.rbapp/models/provider/truelayer_adapter.rbapp/models/provider/truelayer_syncer.rbapp/models/provider_merchant.rbapp/models/transaction.rbapp/views/accounts/index.html.erbapp/views/provider_connections/_connection_card.html.erbapp/views/provider_connections/select.html.erbapp/views/provider_connections/setup.html.erbapp/views/provider_connections/show.html.erbapp/views/provider_family_configs/_truelayer_instructions.html.erbapp/views/provider_family_configs/edit.html.erbapp/views/provider_family_configs/new.html.erbapp/views/settings/bank_sync/_provider_link.html.erbapp/views/settings/providers/_oauth_provider_panel.html.erbapp/views/settings/providers/show.html.erbconfig/brakeman.ignoreconfig/initializers/inflections.rbconfig/initializers/sidekiq.rbconfig/locales/views/provider_connections/en.ymlconfig/locales/views/provider_family_configs/en.ymlconfig/locales/views/settings/en.ymlconfig/routes.rbdb/migrate/20260502153816_create_provider_tables.rbdb/schema.rbtest/controllers/oauth_callbacks_controller_test.rbtest/controllers/provider_connections_controller_test.rbtest/controllers/provider_family_configs_controller_test.rbtest/controllers/settings/providers_controller_test.rbtest/fixtures/accounts.ymltest/fixtures/provider_accounts.ymltest/fixtures/provider_connections.ymltest/fixtures/provider_family_configs.ymltest/jobs/provider/consent_expiry_check_job_test.rbtest/models/account_test.rbtest/models/family/syncer_test.rbtest/models/provider/account_test.rbtest/models/provider/auth/oauth2_test.rbtest/models/provider/connection_test.rbtest/models/provider/family_config_test.rbtest/models/provider/truelayer_adapter_test.rbtest/models/provider/truelayer_syncer_test.rbtest/models/provider/truelayer_test.rb
💤 Files with no reviewable changes (1)
- app/models/provider_merchant.rb
✅ Files skipped from review due to trivial changes (26)
- test/fixtures/provider_family_configs.yml
- app/views/provider_family_configs/_truelayer_instructions.html.erb
- test/fixtures/provider_connections.yml
- app/models/provider/auth.rb
- test/fixtures/provider_accounts.yml
- app/controllers/concerns/accountable_resource.rb
- db/migrate/20260502153816_create_provider_tables.rb
- app/views/settings/bank_sync/_provider_link.html.erb
- app/views/provider_connections/select.html.erb
- config/locales/views/provider_family_configs/en.yml
- app/views/settings/providers/show.html.erb
- config/locales/views/provider_connections/en.yml
- test/fixtures/accounts.yml
- app/models/provider/connection/sync_complete_event.rb
- test/models/provider/truelayer_test.rb
- test/models/provider/connection_test.rb
- test/controllers/settings/providers_controller_test.rb
- app/controllers/provider_family_configs_controller.rb
- app/views/provider_connections/_connection_card.html.erb
- config/brakeman.ignore
- test/models/provider/family_config_test.rb
- test/models/provider/truelayer_adapter_test.rb
- app/models/provider/truelayer_adapter.rb
- test/models/account_test.rb
- app/models/provider/connection.rb
- app/models/provider/truelayer_syncer.rb
🚧 Files skipped from review as they are similar to previous changes (23)
- app/models/family.rb
- app/views/provider_family_configs/edit.html.erb
- app/views/provider_connections/setup.html.erb
- config/locales/views/settings/en.yml
- config/initializers/sidekiq.rb
- app/controllers/accounts_controller.rb
- app/models/data_enrichment.rb
- app/views/accounts/index.html.erb
- test/models/provider/account_test.rb
- test/models/provider/auth/oauth2_test.rb
- app/models/account.rb
- app/views/provider_family_configs/new.html.erb
- test/controllers/provider_family_configs_controller_test.rb
- app/models/account/provider_import_adapter.rb
- app/models/transaction.rb
- test/controllers/provider_connections_controller_test.rb
- test/jobs/provider/consent_expiry_check_job_test.rb
- config/routes.rb
- app/controllers/provider_connections_controller.rb
- test/models/provider/truelayer_syncer_test.rb
- app/models/provider/truelayer.rb
- app/controllers/oauth_callbacks_controller.rb
- app/models/provider/account.rb
Contributor
Author
|
@coderabbitai review PR has been squashed and the following review feedback has been addressed since your previous review: Codex (P2)
CodeRabbit must-fix
CodeRabbit worth-fixing
Pushed back on (intentional, not changing):
Latest commit: 3cc9ab1 |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
Collaborator
|
This is big! Going to need @sokie's help ... have a huge backlog of other PRs that I am getting through little by little. |
jjmata
removed
the
contributor:flagged
Collaborator
|
Took some time to go through this properly.
So before getting to the meat, a small high level overview of what is there now:
There are some clear gaps as new providers were added: Oauth abstraction, no standard error hierarchy, the generators drift sometimes as contributors forget to update it. This PR introduces a fourth subsystem (OAuth-specific) without consolidating the existing three. Migration is a stated intent, but no code, deprecation warnings or migration helpers exist; not a single existing adapter is rewritten on the new framework as a worked example. Some concerns from my side:
Something unclear to me:
And lastly documentation drift will get worse, not better. |
Collaborator
|
How do you feel about moving forward with TrueLayer only, and splitting the other work out @thewillwilson? |
brin-security-scanner
Bot
added
the
contributor:flagged
jjmata
removed
contributor:flagged
Collaborator
|
The security hardening section is thorough (POST-only OAuth, state binding, PSU IP filtering, HTTPS-only logos, A few things to flag before merge: PR has merge conflicts PR scope
Consent expiry job Overall the framework design — Generated by Claude Code |
|
The architecture of the Dual-dispatch in controllers —
Migration path — the PR description says existing providers are "intended to migrate onto this framework over time." A rough timeline or a tracking issue would help prevent this from becoming permanent dead weight. What's the plan for Plaid, SimpleFIN, etc.? Scope — 64 files is a lot to review safely. If there's any way to keep the framework-only changes in one PR and TrueLayer as a follow-up (or vice versa), future reviewers will thank you. Generated by Claude Code |
Collaborator
|
(deleted some "agentic eager beaver" comments that were not useful and just added repetition, sorry about that ... trying to fine-tune things!) |
Introduces a provider abstraction (Provider::Connection + Provider::Account + Provider::FamilyConfig) for bank connection providers, adds TrueLayer as the first integration on it, and migrates Plaid onto the same framework. Framework - Provider::ConnectionAdapter: declarative adapter contract (display_name, supported_account_types, syncer_class, connection_configs, build_sure_account, auth_class). Optional methods for EmbeddedLink and webhook delivery. - Provider::ConnectionRegistry: provider registration + lookup. - Provider::Auth::OAuth2 / Provider::Auth::EmbeddedLink: pluggable auth backends. OAuth2 manages the redirect grant + token refresh + consent expiry detection. EmbeddedLink manages widget-flow public_token exchange + webhook-driven requires_update transitions. - Provider::FamilyConfig: per-family BYOK credentials (encrypted), with sandbox toggle support. - Provider::Connection: lifecycle (pending → healthy → requires_update → disconnected), polymorphic dispatch to syncer + auth backend. - ProviderAuthCallbacksController: unified GET /auth dispatcher routing by adapter.auth_class. Single URL every provider registers in their developer dashboard. - ProviderAuthFlowSession: session-bound flow state, single-use, CSRF-safe (state == flow_id), with TTL pruning + bounded size. - Webhooks::ProviderController: generic /webhooks/providers/:provider_key receiver, adapter-driven signature verification + handler dispatch. TrueLayer - Provider::Truelayer (HTTP client): rate-limit retry honouring Retry-After, transient/reauth/consent-expired error classification, PSU IP forwarding. - Provider::Truelayer::Adapter / Syncer: discovery, transactions, balance anchoring, FX metadata, pending-transaction handling. - Provider::Truelayer::CategoryTaxonomy + shared Provider::CategoryMatcher for transaction auto-categorization. Plaid cutover - Plaid framework adapter (Provider::Plaid::Adapter / Syncer / AccountImporter / AccountsSnapshot / sub-processors) replaces PlaidItem / PlaidAccount. - Legacy plaid_items + plaid_accounts tables dropped. - holdings.source column added; Holding#from_provider? unifies legacy account_provider_id and new source for cross-provider scoping. Settings + ops - /settings/providers + per-provider config/connect panels, BYOK form with sandbox toggle, provider connection cards. - Provider::ConsentExpiryCheckJob: scheduled scan, transitions connections nearing consent expiry to requires_update.
holdings.account_provider_id is an FK to the legacy polymorphic account_providers table; provider_accounts.id lives in a different table. Without a framework-era equivalent, MigrateLegacyPlaidToFramework would hit a FK violation on Plaid investment users. - AddSourceToHoldings migration + Holding#from_provider? and scopes unifying legacy account_provider_id with the new source column - Read-sites use from_provider/not_from_provider: Account, MarketDataImporter, LinkedInvestmentSeriesNormalizer, Materializer, PortfolioSnapshot - Plaid HoldingsProcessor + Account::ProviderImportAdapter persist source: "plaid" - MigrateLegacyPlaidToFramework: backfill source before destroying stale AccountProvider rows so dependent: :nullify fires; re-point Plaid webhooks inline - FamilyResetJob clears provider_connections - Rake tasks ported to framework primitives: new data_migration:migrate_plaid_webhooks (operator backstop); dev_sync_stats and security_backfill use Provider::Connection / Provider::Account
The Plaid syncer only rescued Plaid::ApiError and re-raised everything else with sync_error set, so a network blip would surface "Connection refused" in the UI before Sidekiq retried. TrueLayer's syncer correctly treats these as Provider::Auth::TransientError; Plaid now mirrors that contract: - Plaid::ApiError with HTTP code >= 500 → raise TransientError - Net::OpenTimeout, Net::ReadTimeout, Errno::ECONNREFUSED, Errno::ECONNRESET, Errno::EHOSTUNREACH, SocketError → raise TransientError - Other Plaid::ApiError + non-transient errors keep the existing sync_error + re-raise behaviour
ProviderConnectionsController#reauth hardcoded auth_type == "embedded_link" to decide between the Link-widget and OAuth reauth paths. The unified ProviderAuthCallbacksController dispatches on adapter.auth_class — mirror that here so a new EmbeddedLink-class provider doesn't have to remember to update string comparisons in two places.
thewillwilson
force-pushed
the
feat/unified-provider-framework
branch
from
ef512a7 to
637f19b
Compare
brin-security-scanner
Bot
added
pr:flagged
Contributor
Author
|
Force-pushed 4 commits now on the branch:
Test status (verified on remote Docker):
@coderabbitai review |
Contributor
|
Kicking off a fresh review against the updated branch now. ✅ Actions performedFull review triggered. |
… URI change The Plaid framework cutover changes the OAuth redirect URI from accounts_url to provider_auth_url(provider_key: "plaid"). Webhook URLs are auto-migrated by MigrateLegacyPlaidToFramework, but redirect URIs live in the operator's Plaid Dashboard and cannot be auto-migrated — without manual action, OAuth-bank Plaid connections (Chase, Wells Fargo, Capital One, etc.) will fail at next reauth. Renders a destructive-styled banner at the top of /settings/providers when the family has at least one Plaid Provider::Connection, surfacing the new URI in a copyable code block. Reuses the existing clipboard Stimulus controller pattern from setup_error / setup_instructions. Non-OAuth Plaid connections continue syncing without action; the banner explains this so operators don't panic about other connections.
…laid banner The Plaid OAuth redirect URI banner shipped in the previous commit had no dismiss path — once an operator added the URI to their Plaid Dashboard the banner kept showing. This commit generalises the pattern into a platform-wide registry so future operator-action notices (env var requirements, schema-backfill rake tasks, webhook URL changes, deprecations, etc.) reuse the same components. Components: - AddDismissedMigrationNoticesToFamilies migration: adds families.dismissed_migration_notices (string array, NOT NULL, default []) - Family#dismissed_migration_notice? + #dismiss_migration_notice!: family-scoped acknowledgment - MigrationNotice registry: declarative .register(key:, scope:, condition:, copyable_value:); .active_for(family:, view:, scope:) returns hashes filtered by scope + dismissal + condition - MigrationNoticesController#destroy: admin-only DELETE /migration_notices/:key - migration_notices/_notice.html.erb: generic destructive-styled callout with optional clipboard block + dismiss button, fully i18n-driven - ApplicationHelper#render_migration_notices(scope:): one-liner for views - config/initializers/migration_notices.rb: registers the Plaid notice - config/locales/views/migration_notices/en.yml: locale namespace The bespoke app/views/settings/providers/_plaid_redirect_uri_migration_notice.html.erb and its locale block under settings.providers.plaid_redirect_uri_migration are removed; settings/providers/show.html.erb now calls render_migration_notices(scope: :providers). Adding a new notice in a future PR is: one .register call + one locale block. No new partial, controller, or route.
The plaid_oauth_redirect_uri MigrationNotice's condition matched any Plaid Provider::Connection, so fresh installs setting up Plaid for the first time post-deploy would also see "redirect URI changed" warnings — even though their Plaid Dashboard has only ever known the framework URI. MigrateLegacyPlaidToFramework now stamps metadata.migrated_from_legacy: true on the connections it creates. The notice condition filters on that flag, so only operators upgrading from legacy plaid_items see the warning. Fresh installs see nothing. Adds an integration test against the production-registered notice covering: empty family, fresh-install connection (no flag), legacy-migrated connection (flag = true), and the explicit-false edge case. Test lives outside MigrationNoticeTest so the registry isn't reset around it.
…-framework # Conflicts: # app/views/settings/providers/_provider_form.html.erb # db/schema.rb # test/models/account/linkable_test.rb # test/models/sync_test.rb
The merge of main pulled in code that referenced the legacy Plaid models removed in the framework cutover: - accounts.plaid_account_id (dropped column) referenced in Account::Linkable.linked scope and three eager-load includes (accounts_controller#index, balance_sheet/account_totals, and views/accounts/index/_account_groups.erb) - ProviderConnectionStatus iterates family.plaid_items / .plaid_accounts, both of which are now framework-managed via Provider::Connection / Provider::Account; remove plaid from the iterator's PROVIDERS list (plaid health is surfaced through the framework's own status field) Replaces all four eager-load sites with :provider_accounts (the has_many added by the framework) and updates the linked scope to LEFT JOIN the new association instead of probing the dropped column.
…cutover Two more sites that called the dropped Family#plaid_items association: - Api::V1::UsersController#reset_target_counts: surfaced count under the "plaid_items" key. Renamed the key to "provider_connections" to reflect what's actually being counted now (the framework's bank-sync connections, which include Plaid). Test updated. - ProviderConnectionStatus iterator was the same shape; the api/v1 provider_connections "fails closed" test asserted a default plaid entry from the legacy iterator. After the framework cutover that defensive default no longer applies; removed the test with a TODO pointing at the follow-up (adapt for_family to handle Provider::Connection-shaped providers so plaid health is reported via the framework path).
Contributor
Author
|
Reopening after merge conflicts resolved (head: 19d4018) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #1586.
Where #1586 introduced TrueLayer-specific models (
TruelayerItem,TruelayerAccount,TruelayerItem::Importer/Syncer,TruelayerEntry::Processor), this PR generalises the integration into a reusableProvider::*framework — TrueLayer is the first concrete adopter, but the design intent is for the existing per-provider integrations to migrate onto it over time.Summary
Introduces a generic
Provider::*namespace for third-party financial-data integrations, with TrueLayer (UK & EU Open Banking) as the first concrete adopter. Existing per-provider integrations (Plaid, SimpleFIN, Lunchflow, Sophtron, Mercury, Coinbase, Binance, IndexaCapital, SnapTrade) are intended to migrate onto this framework over time.What's new
Framework (
app/models/provider/)provider_family_configs(per-family BYOK OAuth credentials, encrypted via Railsencrypts),provider_connections(one OAuth grant per family/institution),provider_accounts(mapping provider accounts → Sure accounts).Provider::Auth::OAuth2— generic token exchange / refresh / reauth helpers, with explicitTransientError/ReauthRequiredErrorclassification so transient upstream issues don't surface scary text insync_error.Provider::Registry— adapter registration (register_oauth_provider) and lookup (oauth_provider_adapter,syncer_class_for).Provider::Connection::SyncCompleteEvent— broadcast hook intoFamily::Syncerauto-sync.Provider::ConsentExpiryCheckJob— nightly scan that marks connections with consent expiring within 7 days asrequires_update.TrueLayer adapter
Provider::Truelayer— HTTP client with rate-limit retry (capped at 30sRetry-After), 5xx→TransientError classification, sandbox/production routing, PSU IP forwarding.Provider::TruelayerAdapter— payload normalisation, multi-stage counterparty name extraction (merchant_name→metacounterparty fields → category fallback → humanised description), TrueLayer reauth URL via/v1/reauthuri.Provider::TruelayerSyncer— discovery, per-account 90-day initial sync window, transaction import with merchant entity creation from counterparty data, balance anchoring (with credit-card available-credit fallback tocredit_limit).Auto-sync, UI, and integration
Family::Syncerincludesprovider_connectionsin itsRELATIONS_TO_SYNC.requires_update.PENDING_PROVIDERS, scoped scopes inAccount::ProviderImportAdapter).Security hardening
invalid_grant).X-PSU-IP.Provider::FamilyConfigvalidates credential keys against a known allow-list.save_setupandlinkactions are transactional; mappings can't be left half-applied.OauthCallbacksController#createrescue is narrowed to known error classes; state-mismatch is logged and reported distinctly.Provider::Account#build_sure_accountraisesUnsupportedAccountableTyperather than silently mis-categorising unknown external types asDepository.Known follow-ups for migrating other providers
These are intentional limits today, called out so future migrations can address them:
Provider::Connection.statusenum is generic butrequires_updatesemantics are currently OAuth-consent-shaped. Non-OAuth providers (SimpleFIN, static-credential adapters) won't use the state.Provider::Registrytoday only exposes OAuth2 registration helpers. A non-OAuth registration path will be added when the first non-OAuth provider migrates.connection_configsis a duck-typed contract shared with existing per-provider adapters. Extracting aProvider::Adaptermodule that documents the interface is a good follow-up once a second adapter starts migrating onto the framework.Provider::Account::ACCOUNTABLE_MAPcoversdepository/credit. When an adapter brings investments throughbuild_sure_account, the map must be extended —UnsupportedAccountableTypeis raised loudly to prevent silent mis-categorisation.Migration
One new migration:
db/migrate/20260502153816_create_provider_tables.rbprovider_family_configs(UUID PK, encrypted credentials)provider_connections(UUID PK, encrypted credentials, jsonb metadata)provider_accounts(UUID PK, encrypted raw payloads, FK to accounts withON DELETE NULLIFY,skippedboolean)No data migration required.
Summary by CodeRabbit
New Features
Bug Fixes