Add support for dynamic config UI #256
Conversation
WalkthroughAdds a provider configuration DSL and registry, dynamic Setting storage for runtime credentials, adapter autoloading and reload hooks, a new admin Settings::ProvidersController with routes and views to manage provider credentials, and adapter configuration updates for Plaid, Plaid EU, and SimpleFIN. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant Browser as Browser/UI
participant Controller as Settings::ProvidersController
participant Registry as Provider::ConfigurationRegistry
participant Factory as Provider::Factory
participant Setting as Setting
participant Adapter as Provider Adapter
Admin->>Browser: GET /settings/providers
Browser->>Controller: request
Controller->>Factory: ensure_adapters_loaded()
Factory->>Registry: trigger adapter registrations (autoload)
Controller->>Registry: all() -> provider_configurations
Controller->>Browser: render provider forms
Admin->>Browser: PATCH /settings/providers (params)
Browser->>Controller: submit params
Controller->>Controller: sanitize & permit fields (trim, ignore "********")
Controller->>Setting: Setting[key] = value (for changed keys)
Controller->>Controller: collect updated fields
Controller->>Factory: determine affected adapters
Controller->>Adapter: call Adapter.reload_configuration for affected adapters
Adapter->>Setting: read config values (Setting -> ENV -> default)
Adapter->>Adapter: apply runtime config (e.g., Rails.application.config.plaid)
Controller->>Browser: redirect with notice / or render 422 on error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Added dynamic_fields hash field - Stores all undeclared settings [] method - Checks declared fields first, then falls back to dynamic hash []= method - Updates declared fields normally, stores others in hash No runtime field declaration - Fields are never dynamically created on the class
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
app/models/provider/configurable.rb (1)
132-143: Consider validating field name uniqueness.The
fieldmethod doesn't prevent duplicate field names. While unlikely in practice (developers control the DSL), duplicate names would causeget_valueto return only the first match.Add validation if you want to catch configuration errors early:
def field(name, label:, required: false, secret: false, env_key: nil, default: nil, description: nil) + if @fields.any? { |f| f.name == name } + raise ArgumentError, "Field '#{name}' is already defined for provider '#{@provider_key}'" + end + @fields << ConfigField.new( name: name,app/models/provider/plaid_adapter.rb (1)
43-57: Simplify redundant ENV fallback logic.The
config_value(:field).presence || ENV["KEY"]pattern is redundant becauseconfig_valuealready checksENVviaConfigField.value. The explicit|| ENV["..."]fallback will never be reached.Apply this diff to simplify:
def self.reload_configuration - client_id = config_value(:client_id).presence || ENV["PLAID_CLIENT_ID"] - secret = config_value(:secret).presence || ENV["PLAID_SECRET"] - environment = config_value(:environment).presence || ENV["PLAID_ENV"] || "sandbox" + client_id = config_value(:client_id) + secret = config_value(:secret) + environment = config_value(:environment) || "sandbox" if client_id.present? && secret.present?app/models/provider/factory.rb (1)
47-58: Consider thread-safety and userequireinstead ofrequire_dependency.The eager loading pattern is necessary to trigger adapter registration side-effects, but has two minor issues:
- Deprecated API:
require_dependencyis deprecated in Rails 7+. Userequireinstead.- Thread-safety: The
@adapters_loadedcheck-then-act is not thread-safe, though unlikely to cause issues since loading typically happens at boot or on first access.Apply this diff:
def ensure_adapters_loaded - return if @adapters_loaded + return if defined?(@adapters_loaded) && @adapters_loaded # Require all adapter files to trigger registration Dir[Rails.root.join("app/models/provider/*_adapter.rb")].each do |file| - require_dependency file + require file end @adapters_loaded = true endAlternatively, use a mutex for proper thread-safety:
def ensure_adapters_loaded @load_mutex ||= Mutex.new return if @adapters_loaded @load_mutex.synchronize do return if @adapters_loaded Dir[Rails.root.join("app/models/provider/*_adapter.rb")].each do |file| require file end @adapters_loaded = true end endapp/views/settings/providers/_provider_form.html.erb (2)
42-72: Extract field rendering logic to a helper method.The inline field rendering logic (lines 42-63) is complex and violates the guideline to "Keep heavy logic out of ERB views." Consider extracting this to a helper method for better testability and maintainability.
As per coding guidelines.
Add to
app/helpers/settings/providers_helper.rb:module Settings::ProvidersHelper def provider_field_display_value(field) env_value = ENV[field.env_key] if field.env_key setting_value = Setting[field.setting_key] current_value = setting_value.presence || env_value if field.secret && current_value.present? "********" else current_value end end def provider_field_type(field) field.secret ? "password" : "text" end endThen simplify the view:
<% configuration.fields.each do |field| %> <%= form.text_field field.setting_key, label: field.label, type: provider_field_type(field), placeholder: field.default || "", value: provider_field_display_value(field), data: { "auto-submit-form-target": "auto" } %> <% end %>
62-62: Remove unnecessarydisabled = falseassignment.The
disabledvariable is always set tofalseand never changes. Either remove it or add a comment explaining why it's preserved for future use.Apply this diff:
- # Don't disable fields - allow overriding ENV variables - disabled = false %> <%= form.text_field field.setting_key, type: input_type, placeholder: field.default || "", value: display_value, - disabled: disabled, data: { "auto-submit-form-target": "auto" } %>app/models/provider/simplefin_adapter.rb (1)
9-23: Solid config DSL; consider one‑time token hygieneThe field is correctly marked secret with ENV override. Given setup_token is one‑time use, consider clearing it after first successful SimpleFIN setup to avoid storing stale secrets. If not in scope, add a note to the UI description.
app/controllers/settings/providers_controller.rb (1)
22-38: Update only when values change; centralize secret placeholderSkip writing when unchanged and avoid magic string duplication for secret placeholders.
+ SECRET_PLACEHOLDER = "********".freeze @@ - provider_params.each do |param_key, param_value| + provider_params.each do |param_key, param_value| setting_key = param_key.to_sym - # Clean the value - value = param_value.to_s.strip + # Clean the value + value = param_value.to_s.strip @@ - if value == "********" + if value == SECRET_PLACEHOLDER next end @@ - Setting[setting_key] = value - updated_fields << param_key + current = Setting[setting_key] + next if current.to_s == value + Setting[setting_key] = value + updated_fields << param_keyOptionally expose SECRET_PLACEHOLDER to the form partial to keep UI/backend in sync.
app/models/setting.rb (1)
52-101: Bracket API is clean; add tests and note concurrency edge
- Reads declared fields first, then dynamic hash — aligns with provider precedence logic. Good.
- Add tests for:
- precedence (declared > dynamic),
- ENV fallback when value is blank,
- delete semantics,
- secret round‑trip masking in controller/view.
- Minor: dynamic_fields writes are last‑write‑wins; if concurrent admin edits are possible, consider a lightweight merge/retry or per‑key storage later.
I can draft the test cases if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/controllers/settings/providers_controller.rb(1 hunks)app/models/provider/configurable.rb(1 hunks)app/models/provider/factory.rb(1 hunks)app/models/provider/plaid_adapter.rb(1 hunks)app/models/provider/simplefin_adapter.rb(1 hunks)app/models/setting.rb(2 hunks)app/views/settings/_settings_nav.html.erb(1 hunks)app/views/settings/providers/_provider_form.html.erb(1 hunks)app/views/settings/providers/show.html.erb(1 hunks)config/routes.rb(1 hunks)test/models/provider/registry_test.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (19)
config/**
📄 CodeRabbit inference engine (AGENTS.md)
Store application and environment configuration under config/
Files:
config/routes.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
config/routes.rbtest/models/provider/registry_test.rbapp/controllers/settings/providers_controller.rbapp/models/provider/configurable.rbapp/models/provider/plaid_adapter.rbapp/models/provider/simplefin_adapter.rbapp/models/provider/factory.rbapp/models/setting.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
config/routes.rbtest/models/provider/registry_test.rbapp/controllers/settings/providers_controller.rbapp/models/provider/configurable.rbapp/models/provider/plaid_adapter.rbapp/models/provider/simplefin_adapter.rbapp/models/provider/factory.rbapp/models/setting.rb
app/views/**/*.erb
📄 CodeRabbit inference engine (AGENTS.md)
app/views/**/*.erb: Keep heavy logic out of ERB views; prefer helpers/components instead
ERB templates are linted by erb-lint per .erb_lint.ymlAlways use the icon helper (icon) for icons; never call lucide_icon directly
Files:
app/views/settings/providers/show.html.erbapp/views/settings/providers/_provider_form.html.erbapp/views/settings/_settings_nav.html.erb
app/views/**/*.html.*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/views/**/*.html.*: Use partials only for simple, context-specific, mostly static HTML content.
Prefer semantic HTML; use Turbo Frames where possible instead of client-side solutions.
Use query params for state, not localStorage or sessionStorage.
Always perform server-side formatting for currencies, numbers, and dates.
Files:
app/views/settings/providers/show.html.erbapp/views/settings/providers/_provider_form.html.erbapp/views/settings/_settings_nav.html.erb
**/*.{html,erb,slim,js,jsx,ts,tsx,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always use functional design tokens (e.g., text-primary, bg-container) from the design system; do not use raw colors or ad-hoc classes.
Files:
app/views/settings/providers/show.html.erbapp/views/settings/providers/_provider_form.html.erbapp/views/settings/_settings_nav.html.erb
app/views/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/views/**/*.html.erb: Prefer native HTML elements (e.g., ,) over JS-based components
Leverage Turbo frames to break up pages instead of JS-driven client-side solutions
Prefer native client-side form validation when possible
Files:
app/views/settings/providers/show.html.erbapp/views/settings/providers/_provider_form.html.erbapp/views/settings/_settings_nav.html.erb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/views/settings/providers/show.html.erbapp/controllers/settings/providers_controller.rbapp/models/provider/configurable.rbapp/models/provider/plaid_adapter.rbapp/models/provider/simplefin_adapter.rbapp/models/provider/factory.rbapp/views/settings/providers/_provider_form.html.erbapp/models/setting.rbapp/views/settings/_settings_nav.html.erb
{app/javascript/controllers/**/*.{js,ts},app/views/**/*.erb,app/components/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Use declarative Stimulus actions in ERB (data-action) instead of imperative event listeners in controller lifecycle (e.g., avoid addEventListener in connect); controllers should just respond to actions
Files:
app/views/settings/providers/show.html.erbapp/views/settings/providers/_provider_form.html.erbapp/views/settings/_settings_nav.html.erb
{app/components/**/*.{js,ts,erb},app/views/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Component-scoped Stimulus controllers in app/components must be used only within their component views, not in app/views
Files:
app/views/settings/providers/show.html.erbapp/views/settings/providers/_provider_form.html.erbapp/views/settings/_settings_nav.html.erb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Style UI using TailwindCSS v4.x with the custom design system defined in app/assets/tailwind/maybe-design-system.css
Always start by referencing app/assets/tailwind/maybe-design-system.css to understand available primitives, functional tokens, and component tokens before styling
Prefer functional tokens from the design system over raw Tailwind values (e.g., use text-primary, bg-container, border border-primary instead of text-white, bg-white, border-gray-200)
Files:
app/views/settings/providers/show.html.erbapp/views/settings/providers/_provider_form.html.erbapp/views/settings/_settings_nav.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/settings/providers/show.html.erbapp/views/settings/providers/_provider_form.html.erbapp/views/settings/_settings_nav.html.erb
{app/views,app/components}/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
{app/views,app/components}/**/*.html.erb: Keep domain logic out of ERB templates; compute values in component/controller code and reference them in the template
Integrate Stimulus declaratively in ERB: templates declare data-controller/data-action; controllers respond to those declarations
Pass data from Rails to Stimulus via data-*-value attributes, not inline JavaScript
Files:
app/views/settings/providers/show.html.erbapp/views/settings/providers/_provider_form.html.erbapp/views/settings/_settings_nav.html.erb
test/**/*_test.rb
📄 CodeRabbit inference engine (AGENTS.md)
Name Minitest files with *_test.rb and mirror the app/ structure under test/
test/**/*_test.rb: ALWAYS use Minitest + fixtures + Mocha for tests; NEVER RSpec or FactoryBot.
Use Mocha for mocking in tests when necessary.
Use VCR for external API tests.
test/**/*_test.rb: Always use Minitest for tests; do not use RSpec
Do not use factories (e.g., FactoryBot) in tests; rely on fixtures instead
For tests needing many records, use Rails helpers to construct data and inline the creation in the test
Only write tests for critical and important code paths
Avoid tests that merely verify framework/ActiveRecord behavior
Test boundaries correctly: for queries, assert returned values; for commands, assert collaborators are called with correct params
Never test the implementation details of one class in another class’s test suite
Use the mocha gem for stubs and mocks
Prefer OpenStruct for mock instances; use a mock class for complex cases
Only mock what’s necessary; don’t mock return values unless they are under test
Files:
test/models/provider/registry_test.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/controllers/settings/providers_controller.rbapp/models/provider/configurable.rbapp/models/provider/plaid_adapter.rbapp/models/provider/simplefin_adapter.rbapp/models/provider/factory.rbapp/models/setting.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Use ActiveRecord validations for forms and complex domain constraints.
app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required
Files:
app/models/provider/configurable.rbapp/models/provider/plaid_adapter.rbapp/models/provider/simplefin_adapter.rbapp/models/provider/factory.rbapp/models/setting.rb
app/models/provider/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Concrete provider classes must be under the Provider:: namespace, inherit from Provider, wrap calls with with_provider_response, and raise on invalid/unavailable data
Files:
app/models/provider/configurable.rbapp/models/provider/plaid_adapter.rbapp/models/provider/simplefin_adapter.rbapp/models/provider/factory.rb
app/{helpers,models}/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates on the server side (Ruby) before sending to the client
Files:
app/models/provider/configurable.rbapp/models/provider/plaid_adapter.rbapp/models/provider/simplefin_adapter.rbapp/models/provider/factory.rbapp/models/setting.rb
app/views/**/_*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Name partials with a leading underscore (e.g., _trend_change.html.erb, _form_errors.html.erb)
Files:
app/views/settings/providers/_provider_form.html.erbapp/views/settings/_settings_nav.html.erb
🧠 Learnings (1)
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/**/*.rb : Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Applied to files:
app/models/provider/factory.rb
🧬 Code graph analysis (7)
test/models/provider/registry_test.rb (1)
app/models/provider/registry.rb (4)
for_concept(11-13)providers(85-87)get_provider(15-19)get_provider(89-95)
app/controllers/settings/providers_controller.rb (5)
app/models/provider/factory.rb (1)
ensure_adapters_loaded(49-58)app/models/provider/configurable.rb (8)
all(223-225)setting_key(183-185)value(188-202)field(132-143)provider_key(68-70)provider_key(92-94)get_adapter_class(232-234)reload_configuration(85-88)app/models/current.rb (1)
user(8-10)app/models/user.rb (1)
admin?(67-69)app/models/provider/plaid_adapter.rb (1)
reload_configuration(44-57)
app/models/provider/configurable.rb (1)
app/models/provider/plaid_adapter.rb (1)
reload_configuration(44-57)
app/models/provider/plaid_adapter.rb (2)
app/models/provider/simplefin_adapter.rb (2)
include(1-77)provider_name(25-27)app/models/provider/configurable.rb (8)
register(214-217)configure(55-59)description(120-122)field(132-143)reload_configuration(85-88)config_value(73-75)config_value(100-102)present?(205-207)
app/models/provider/simplefin_adapter.rb (2)
app/models/provider/plaid_adapter.rb (1)
include(1-95)app/models/provider/configurable.rb (4)
register(214-217)configure(55-59)description(120-122)field(132-143)
app/models/provider/factory.rb (1)
app/models/provider/configurable.rb (1)
registry(237-239)
app/models/setting.rb (1)
app/models/provider/configurable.rb (2)
field(132-143)value(188-202)
🔇 Additional comments (9)
test/models/provider/registry_test.rb (1)
6-13: LGTM! Proper environment scoping.The use of
ClimateControl.modifycorrectly scopes theOPENAI_ACCESS_TOKENenvironment variable for this test, preventing side effects on other tests.app/models/provider/configurable.rb (1)
53-89: LGTM! Well-designed configuration API.The class methods provide a clean DSL for provider configuration with sensible defaults and proper nil handling. The
provider_keyderivation works correctly for the documented naming convention.app/views/settings/_settings_nav.html.erb (1)
31-31: LGTM! Consistent with existing navigation items.The new "Providers" navigation item follows the established pattern and is correctly placed in the admin-only advanced section. Note that it uses a hardcoded label like some other items in this section (e.g., "LLM Usage", "SimpleFIN").
config/routes.rb (1)
79-79: LGTM! Correct use of singleton resource.The singular
resource :providerswithshowandupdateactions is appropriate for a single configuration page managing multiple providers.app/views/settings/providers/show.html.erb (1)
1-15: LGTM! Clean and straightforward view.The view correctly iterates over provider configurations and delegates rendering to the partial. The use of
titleizefor display names is appropriate.app/models/provider/plaid_adapter.rb (1)
9-37: LGTM! Well-structured Plaid configuration.The configuration block properly declares all required fields with appropriate metadata (labels, descriptions, env_keys). The use of
secret: truefor the secret key and the setup instructions are helpful.app/views/settings/providers/_provider_form.html.erb (1)
33-39: LGTM! Auto-submit on blur provides good UX.The form setup with
auto-submit-formcontroller and blur trigger allows seamless saving of provider credentials without requiring a submit button.app/models/provider/simplefin_adapter.rb (1)
4-4: Configurable inclusion looks goodAdapter now participates in the dynamic config system as intended. No issues.
app/models/setting.rb (1)
14-17: Dynamic fields fit the use‑caseSingle hash for arbitrary provider settings keeps the schema stable and pairs well with the controller’s permit list. LGTM.
- Also validate configurable values properly. - Change Provider factory to use Rails autoloading (Zeitwerk)
The derive_adapter_name method relies on string manipulation ("PlaidAccount".sub(/Account$/, "") + "Adapter" → "PlaidAdapter"), but we already have explicit registration in place.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
app/controllers/settings/providers_controller.rb (2)
69-71: Nil-safe admin checkCurrent.user can be nil; this will raise. Use safe navigation.
-redirect_to settings_providers_path, alert: "Not authorized" unless Current.user.admin? +redirect_to settings_providers_path, alert: "Not authorized" unless Current.user&.admin?
73-94: Avoid NameError(Set) and simplify provider reload mappingYou use Set without requiring it; also the scan is O(N^2). Build a field->provider map once and uniq the keys; no Set needed. Ensure adapters are loaded before resolving.
-# Reload provider configurations after settings update -def reload_provider_configs(updated_fields) - # Build a set of provider keys that had fields updated - updated_provider_keys = Set.new - - # Look up the provider key directly from the configuration registry - updated_fields.each do |field_key| - Provider::ConfigurationRegistry.all.each do |config| - field = config.fields.find { |f| f.setting_key.to_s == field_key.to_s } - if field - updated_provider_keys.add(field.provider_key) - break - end - end - end - - # Reload configuration for each updated provider - updated_provider_keys.each do |provider_key| - adapter_class = Provider::ConfigurationRegistry.get_adapter_class(provider_key) - adapter_class&.reload_configuration - end -end +def reload_provider_configs(updated_fields) + Provider::Factory.ensure_adapters_loaded + # Build mapping once: setting_key -> provider_key + field_to_provider = {} + Provider::ConfigurationRegistry.all.each do |config| + config.fields.each do |f| + field_to_provider[f.setting_key.to_s] = config.provider_key + end + end + updated_provider_keys = updated_fields.filter_map { |k| field_to_provider[k.to_s] }.uniq + updated_provider_keys.each do |provider_key| + Provider::ConfigurationRegistry.get_adapter_class(provider_key)&.reload_configuration + end +endAdditionally (top of file), if you keep Set anywhere else:
+require "set"app/models/provider/configurable.rb (1)
187-202: Empty-string precedence causes ENV fallback; align with intended semanticsCurrent logic uses present?, so a saved empty string won’t override ENV/default. Decide policy, then implement consistently:
Option A (recommended with controller change): treat blank UI values as “unset” (controller sets nil). Keep this method, no change required.
Option B (if you want “UI-saved even if blank overrides ENV”): check key existence, not value presence.
- setting_value = Setting[setting_key] - return normalize_value(setting_value) if setting_value.present? + if Setting.respond_to?(:key?) ? Setting.key?(setting_key) : !Setting[setting_key].nil? + return normalize_value(Setting[setting_key]) + end @@ - if env_key.present? - env_value = ENV[env_key] - return normalize_value(env_value) if env_value.present? - end + if env_key.present? && ENV.key?(env_key) + return normalize_value(ENV[env_key]) + endIf migrating from prior behavior where “” may be stored, consider a one-off data cleanup to set such values to NULL.
🧹 Nitpick comments (3)
app/controllers/settings/providers_controller.rb (1)
48-52: Rescue is fine; consider surfacing validation errorsIf field.validate! raised inside the transaction, return those messages instead of a generic error.
app/models/provider/factory.rb (1)
66-69: Also clear state consistentlyclear_registry! should reset the same object or clear both registries if needed by tests. Optional.
def clear_registry! - @registry = {} + @registry&.clear + @registry ||= {} endapp/models/provider/configurable.rb (1)
277-284: Thread-safety of registries (dev reloads)Memoizing Hashes with ||= can race under multithreaded dev reload. Consider Concurrent::Map or a mutex. Low risk in prod.
- def registry - @registry ||= {} - end + def registry + @registry ||= {} + end # consider Concurrent::Map if you observe races @@ - def adapter_registry - @adapter_registry ||= {} - end + def adapter_registry + @adapter_registry ||= {} + end # consider Concurrent::Map if you observe races
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/settings/providers_controller.rb(1 hunks)app/models/provider/configurable.rb(1 hunks)app/models/provider/factory.rb(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/controllers/settings/providers_controller.rbapp/models/provider/factory.rbapp/models/provider/configurable.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
app/controllers/settings/providers_controller.rbapp/models/provider/factory.rbapp/models/provider/configurable.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
app/controllers/settings/providers_controller.rbapp/models/provider/factory.rbapp/models/provider/configurable.rb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/controllers/settings/providers_controller.rbapp/models/provider/factory.rbapp/models/provider/configurable.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Use ActiveRecord validations for forms and complex domain constraints.
app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required
Files:
app/models/provider/factory.rbapp/models/provider/configurable.rb
app/models/provider/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Concrete provider classes must be under the Provider:: namespace, inherit from Provider, wrap calls with with_provider_response, and raise on invalid/unavailable data
Files:
app/models/provider/factory.rbapp/models/provider/configurable.rb
app/{helpers,models}/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates on the server side (Ruby) before sending to the client
Files:
app/models/provider/factory.rbapp/models/provider/configurable.rb
🧠 Learnings (1)
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/**/*.rb : Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Applied to files:
app/models/provider/factory.rb
🧬 Code graph analysis (3)
app/controllers/settings/providers_controller.rb (5)
app/models/provider/factory.rb (1)
ensure_adapters_loaded(45-57)app/models/provider/configurable.rb (8)
all(264-266)setting_key(183-185)value(188-202)field(132-143)provider_key(68-70)provider_key(92-94)get_adapter_class(273-275)reload_configuration(85-88)app/models/current.rb (1)
user(8-10)app/models/user.rb (1)
admin?(67-69)app/models/provider/plaid_adapter.rb (1)
reload_configuration(44-57)
app/models/provider/factory.rb (1)
app/models/provider/base.rb (1)
provider_type(36-38)
app/models/provider/configurable.rb (2)
app/models/provider/factory.rb (3)
register(1-116)register(8-10)registry(73-75)app/models/provider/plaid_adapter.rb (1)
reload_configuration(44-57)
🪛 GitHub Check: ci / lint
app/models/provider/factory.rb
[failure] 116-116:
Layout/TrailingEmptyLines: Final newline missing.
[failure] 73-73:
Layout/IndentationWidth: Use 2 (not 0) spaces for indented_internal_methods indentation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (1)
app/models/provider/configurable.rb (1)
153-156: verified: configured? behavior is correct; key? method available for Option BVerification complete. The
key?method is available in Setting class (line 81-84), checking both declared fields and dynamic_fields. Theconfigured?method behavior is correct: blank UI values are normalized tonilvianormalize_value()(line 227-231), which then fail thepresent?check, preventing required fields from being satisfied. This implementation matches the described UX behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/models/provider/factory.rb (1)
73-99: Fix indentation and add final newline.Static analysis has flagged two formatting issues that were already noted in previous review comments:
- Line 73: Methods under
privateneed 2 additional spaces of indentation- Line 101: File is missing a final newline
Apply this diff to fix the indentation:
- private - - def registry - @registry ||= {} - end - - # Find adapter class, attempting to load all adapters if not registered - def find_adapter_class(provider_type) - # Return if already registered - return registry[provider_type] if registry[provider_type] - - # Load all adapters to ensure they're registered - # This triggers their self-registration calls - ensure_adapters_loaded - - # Check registry again after loading - registry[provider_type] - end - - # Discover all adapter files in the provider directory - # Returns adapter class names (e.g., ["PlaidAdapter", "SimplefinAdapter"]) - def adapter_files - return [] unless defined?(Rails) - - pattern = Rails.root.join("app/models/provider/*_adapter.rb") - Dir[pattern].map do |file| - File.basename(file, ".rb").camelize + private + + def registry + @registry ||= {} + end + + # Find adapter class, attempting to load all adapters if not registered + def find_adapter_class(provider_type) + # Return if already registered + return registry[provider_type] if registry[provider_type] + + # Load all adapters to ensure they're registered + # This triggers their self-registration calls + ensure_adapters_loaded + + # Check registry again after loading + registry[provider_type] + end + + # Discover all adapter files in the provider directory + # Returns adapter class names (e.g., ["PlaidAdapter", "SimplefinAdapter"]) + def adapter_files + return [] unless defined?(Rails) + + pattern = Rails.root.join("app/models/provider/*_adapter.rb") + Dir[pattern].map do |file| + File.basename(file, ".rb").camelize + end end - end end end +
🧹 Nitpick comments (1)
app/models/provider/factory.rb (1)
43-57: Consider rescuing LoadError in addition to NameError.The error handling currently only rescues
NameError, which may miss other loading failures such asLoadError(missing dependencies) orSyntaxError. Consider broadening the rescue to catch more potential issues during adapter loading.Apply this diff to improve error handling robustness:
begin adapter_class_name.constantize - rescue NameError => e + rescue NameError, LoadError => e Rails.logger.warn("Failed to load adapter: #{adapter_class_name} - #{e.message}") end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/provider/factory.rb(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Use ActiveRecord validations for forms and complex domain constraints.
app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required
Files:
app/models/provider/factory.rb
app/models/provider/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Concrete provider classes must be under the Provider:: namespace, inherit from Provider, wrap calls with with_provider_response, and raise on invalid/unavailable data
Files:
app/models/provider/factory.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/models/provider/factory.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
app/models/provider/factory.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
app/models/provider/factory.rb
app/{helpers,models}/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates on the server side (Ruby) before sending to the client
Files:
app/models/provider/factory.rb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/models/provider/factory.rb
🧠 Learnings (1)
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/**/*.rb : Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Applied to files:
app/models/provider/factory.rb
🧬 Code graph analysis (1)
app/models/provider/factory.rb (1)
app/models/provider/base.rb (1)
provider_type(36-38)
🪛 GitHub Check: ci / lint
app/models/provider/factory.rb
[failure] 101-101:
Layout/TrailingEmptyLines: Final newline missing.
[failure] 73-73:
Layout/IndentationWidth: Use 2 (not 0) spaces for indented_internal_methods indentation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (4)
app/models/provider/factory.rb (4)
2-2: LGTM: Custom exception improves error semantics.The new
AdapterNotFoundErrorprovides clearer error semantics than a genericArgumentErrorand follows Ruby exception conventions.
20-22: LGTM: Adapter lookup now supports lazy loading.The change from direct registry lookup to
find_adapter_classenables dynamic adapter discovery, and the new exception type improves error clarity.
38-41: LGTM: Ensures adapters are loaded and returns sorted keys.The changes ensure all adapters are discovered before returning registered types, and sorting provides a deterministic, user-friendly order.
59-69: LGTM: Registry helpers are well-implemented.The
registered?method correctly triggers adapter loading if needed, andclear_registry!provides a clean way to reset state for testing.
- This looks like partial support atm
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/models/provider/plaid_adapter.rb (2)
55-68: Consider adding logging when credentials are missing.When
client_idorsecretare missing, the configuration is silently set to nil. This could make troubleshooting difficult for users who expect Plaid integration to work but haven't configured credentials.Apply this diff to add informative logging:
def self.reload_configuration client_id = config_value(:client_id).presence || ENV["PLAID_CLIENT_ID"] secret = config_value(:secret).presence || ENV["PLAID_SECRET"] environment = config_value(:environment).presence || ENV["PLAID_ENV"] || "sandbox" if client_id.present? && secret.present? Rails.application.config.plaid = Plaid::Configuration.new Rails.application.config.plaid.server_index = Plaid::Configuration::Environment[environment] Rails.application.config.plaid.api_key["PLAID-CLIENT-ID"] = client_id Rails.application.config.plaid.api_key["PLAID-SECRET"] = secret else + Rails.logger.info("Plaid US credentials not configured - Plaid integration disabled") Rails.application.config.plaid = nil end end
55-68: Consider extracting shared reload logic to reduce duplication.The
reload_configurationmethod is duplicated almost identically inProvider::PlaidEuAdapter(lines 47-60). The only differences are ENV variable names and the config property being set.Consider extracting shared logic into a private helper method in
Provider::Configurableor a shared concern. This would ensure both adapters remain synchronized if the reload logic needs to change. However, given the simplicity and clarity of the current implementation, this refactor can be deferred.app/models/provider/plaid_eu_adapter.rb (1)
47-60: Consider adding logging when credentials are missing.Similar to
Provider::PlaidAdapter, when credentials are missing, the configuration is silently set to nil, which could make troubleshooting difficult.Apply this diff to add informative logging:
def self.reload_configuration client_id = config_value(:client_id).presence || ENV["PLAID_EU_CLIENT_ID"] secret = config_value(:secret).presence || ENV["PLAID_EU_SECRET"] environment = config_value(:environment).presence || ENV["PLAID_EU_ENV"] || "sandbox" if client_id.present? && secret.present? Rails.application.config.plaid_eu = Plaid::Configuration.new Rails.application.config.plaid_eu.server_index = Plaid::Configuration::Environment[environment] Rails.application.config.plaid_eu.api_key["PLAID-CLIENT-ID"] = client_id Rails.application.config.plaid_eu.api_key["PLAID-SECRET"] = secret else + Rails.logger.info("Plaid EU credentials not configured - Plaid EU integration disabled") Rails.application.config.plaid_eu = nil end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/models/provider/plaid_adapter.rb(1 hunks)app/models/provider/plaid_eu_adapter.rb(1 hunks)config/initializers/plaid.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
config/**
📄 CodeRabbit inference engine (AGENTS.md)
Store application and environment configuration under config/
Files:
config/initializers/plaid.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
config/initializers/plaid.rbapp/models/provider/plaid_eu_adapter.rbapp/models/provider/plaid_adapter.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
config/initializers/plaid.rbapp/models/provider/plaid_eu_adapter.rbapp/models/provider/plaid_adapter.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Use ActiveRecord validations for forms and complex domain constraints.
app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required
Files:
app/models/provider/plaid_eu_adapter.rbapp/models/provider/plaid_adapter.rb
app/models/provider/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Concrete provider classes must be under the Provider:: namespace, inherit from Provider, wrap calls with with_provider_response, and raise on invalid/unavailable data
Files:
app/models/provider/plaid_eu_adapter.rbapp/models/provider/plaid_adapter.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/models/provider/plaid_eu_adapter.rbapp/models/provider/plaid_adapter.rb
app/{helpers,models}/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates on the server side (Ruby) before sending to the client
Files:
app/models/provider/plaid_eu_adapter.rbapp/models/provider/plaid_adapter.rb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/models/provider/plaid_eu_adapter.rbapp/models/provider/plaid_adapter.rb
🧬 Code graph analysis (3)
config/initializers/plaid.rb (4)
config/application.rb (1)
config(10-44)app/models/provider/factory.rb (1)
ensure_adapters_loaded(45-57)app/models/provider/plaid_adapter.rb (1)
reload_configuration(55-68)app/models/provider/plaid_eu_adapter.rb (1)
reload_configuration(47-60)
app/models/provider/plaid_eu_adapter.rb (2)
app/models/provider/plaid_adapter.rb (2)
include(12-106)reload_configuration(55-68)app/models/provider/configurable.rb (2)
configure(55-59)present?(205-207)
app/models/provider/plaid_adapter.rb (2)
app/models/provider/plaid_eu_adapter.rb (2)
include(13-61)reload_configuration(47-60)app/models/provider/configurable.rb (8)
register(255-258)configure(55-59)description(120-122)field(132-143)reload_configuration(85-88)config_value(73-75)config_value(100-102)present?(205-207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (3)
app/models/provider/plaid_adapter.rb (1)
1-11: LGTM - Excellent documentation!The comment block clearly explains the dual-purpose nature of this adapter (configuration manager vs. instance adapter) and the separation of concerns with PlaidEuAdapter. This will help future maintainers understand the design.
config/initializers/plaid.rb (1)
1-5: LGTM - Clean initialization pattern.Explicitly initializing both Plaid configurations to nil ensures a clean slate before the adapters reload their configurations. This prevents any stale configuration from persisting.
app/models/provider/plaid_eu_adapter.rb (1)
1-13: LGTM - Clear architectural documentation!The comment block effectively explains why this adapter exists as a configuration-only manager rather than being registered as a provider type. This design rationale will help future maintainers understand the separation between US and EU configurations.
| field :environment, | ||
| label: "Environment", | ||
| required: false, | ||
| env_key: "PLAID_ENV", | ||
| default: "sandbox", | ||
| description: "Plaid environment: sandbox, development, or production" | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Validate environment values to prevent runtime errors.
The environment field accepts any string, but Line 62 uses Plaid::Configuration::Environment[environment], which may raise an error if an invalid value is provided (not "sandbox", "development", or "production").
Consider adding validation to the field definition or handling invalid values gracefully in reload_configuration. Run this script to check how Plaid handles invalid environment values:
🏁 Script executed:
#!/bin/bash
# Search for Plaid::Configuration::Environment usage and error handling patterns
rg -nP --type=ruby -C3 'Plaid::Configuration::Environment\[' Length of output: 1824
🏁 Script executed:
#!/bin/bash
# Get full context of the environment field definition and reload_configuration method
rg -nP --type=ruby -B20 'field :environment' app/models/provider/plaid_adapter.rb | head -40Length of output: 826
🏁 Script executed:
#!/bin/bash
# Check the full reload_configuration method for error handling
rg -nP --type=ruby -A10 'def reload_configuration' app/models/provider/plaid_adapter.rbLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any validation patterns used elsewhere in provider adapters
rg -nP --type=ruby 'field.*validation:|options:' app/models/provider/ | head -20Length of output: 231
🏁 Script executed:
#!/bin/bash
# Check Plaid adapter imports to understand the gem version being used
rg -nP --type=ruby 'require.*plaid|gem.*plaid' app/ config/Length of output: 123
🏁 Script executed:
#!/bin/bash
# Search for any error handling around Plaid::Configuration::Environment usage
rg -nP --type=ruby -B3 -A5 'Plaid::Configuration::Environment\[environment\]' app/models/provider/plaid_adapter.rbLength of output: 441
🏁 Script executed:
#!/bin/bash
# Check if there's a Gemfile to see Plaid version
cat Gemfile | grep -i plaidLength of output: 11
🏁 Script executed:
#!/bin/bash
# Check for any tests that might show expected behavior
fd -e rb -p '**/*spec.rb' -o '**/*_test.rb' | xargs rg -l 'plaid.*environment|Environment\[' 2>/dev/null | head -5Length of output: 287
🏁 Script executed:
#!/bin/bash
# Look for validation patterns in the Configurable concern
fd -e rb -p '**/provider/configurable.rb' -o '**/concerns/*configurable*.rb' | xargs cat 2>/dev/null | head -100Length of output: 177
🏁 Script executed:
#!/bin/bash
# Search for 'options' key usage in field definitions that might indicate validation
rg -nP --type=ruby 'field.*options:|options:\s*\[' app/models/provider/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the PlaidAdapter class definition for any validation
rg -nP --type=ruby -A50 'class PlaidAdapter' app/models/provider/plaid_adapter.rb | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if Plaid::Configuration::Environment is documented or has error handling
rg -nP --type=ruby 'Environment.*=' app/models/provider/ -C2Length of output: 0
🌐 Web query:
What does Plaid ruby gem Plaid::Configuration::Environment do with invalid values
💡 Result:
It raises a KeyError for invalid names (e.g. Plaid::Configuration::Environment["not_a_real_env"] will raise). [1]
Sources:
- Plaid ruby README (shows Environment[...] usage). [1]
🌐 Web query:
Plaid ruby gem environment configuration options
💡 Result:
Short summary — how to configure environments for the Plaid Ruby gem
-
New (current client) style (Configuration / ApiClient):
- configuration.server_index = Plaid::Configuration::Environment["sandbox"] (available values: "sandbox", "development" or "production").[1][2]
- set API credentials and headers via configuration.api_key:
- configuration.api_key["PLAID-CLIENT-ID"] = ENV["..."]
- configuration.api_key["PLAID-SECRET"] = ENV["..."]
- configuration.api_key["Plaid-Version"] = "YYYY-MM-DD" (optional/recommended to pin API version).[1][2]
- build ApiClient and client:
- api_client = Plaid::ApiClient.new(configuration)
- client = Plaid::PlaidApi.new(api_client).[1][2]
-
Faraday / HTTP tuning:
- adjust timeouts: api_client.connection.options[:timeout] = seconds
- replace/add middleware by reinitializing connection with api_client.create_connection { |builder| builder.use ... }.[6]
-
Legacy/older client (Plaid::Client):
- initialize with env: :tartan or :production (or full URL string); client = Plaid::Client.new(env: :sandbox/:production, client_id: ..., secret: ...).[0]
References
- Plaid Ruby GitHub README / migration & examples. [1][4]
- RubyDoc / Plaid gem usage (Configuration, server_index, Faraday tuning). [6][2]
- Legacy Plaid::Client docs (env symbols and URL behavior). [0]
🏁 Script executed:
#!/bin/bash
# Get complete plaid_adapter.rb file to see full class structure
cat app/models/provider/plaid_adapter.rbLength of output: 3323
🏁 Script executed:
#!/bin/bash
# Find where reload_configuration is defined (parent class or concern)
rg -nP --type=ruby 'def reload_configuration' app/models/provider/Length of output: 67
🏁 Script executed:
#!/bin/bash
# Check Provider base class
fd -e rb 'provider.rb' app/models/provider/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any field definitions with validation examples
rg -nP --type=ruby 'field.*required.*true' app/models/provider/ -A2 | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the Configurable concern to understand field validation options
cat app/models/provider/configurable.rbLength of output: 8285
🏁 Script executed:
#!/bin/bash
# Search for any options parameter in other field definitions
rg -nP --type=ruby 'field :' app/models/provider/ -A3 | grep -E 'field|options|validation' | head -40Length of output: 640
Add validation for environment field to prevent KeyError at runtime.
The environment field accepts any string, but line 62 uses Plaid::Configuration::Environment[environment], which raises KeyError if the value is not "sandbox", "development", or "production". Currently, the Configurable framework does not support enum validation, so add error handling or validation logic.
- app/models/provider/plaid_adapter.rb:62 – Wrap with error handling (try/catch) or validate before assignment
- app/models/provider/plaid_eu_adapter.rb:54 – Same issue; apply the same fix
Consider either:
- Add try/catch around
Plaid::Configuration::Environment[environment]to raise a meaningful error, or - Validate the environment value in
reload_configurationbefore calling Plaid::Configuration::Environment[], or - Implement enum validation support in Provider::Configurable::ConfigField to reject invalid values
| field :environment, | ||
| label: "Environment", | ||
| required: false, | ||
| env_key: "PLAID_EU_ENV", | ||
| default: "sandbox", | ||
| description: "Plaid environment: sandbox, development, or production" | ||
| end |
There was a problem hiding this comment.
Validate environment values to prevent runtime errors.
Similar to Provider::PlaidAdapter, the environment field accepts any string, but Line 54 uses Plaid::Configuration::Environment[environment], which may raise an error if an invalid value is provided.
Consider adding validation to the field definition or handling invalid values gracefully in reload_configuration (see review comment on app/models/provider/plaid_adapter.rb lines 42-48 for the same issue).
🤖 Prompt for AI Agents
In app/models/provider/plaid_eu_adapter.rb around lines 38 to 44, the
environment field currently accepts any string but later usage
(Plaid::Configuration::Environment[environment]) can raise on invalid values;
add validation to the field to restrict values to the allowed set ("sandbox",
"development", "production") (e.g., inclusion validator or enum) and/or make
reload_configuration defensively handle unknown values by mapping invalid inputs
to a safe default (like "sandbox") and logging a warning before using
Plaid::Configuration::Environment to avoid runtime errors.
| # Load Plaid configuration from adapters after initialization | ||
| Rails.application.config.after_initialize do | ||
| # Ensure provider adapters are loaded | ||
| Provider::Factory.ensure_adapters_loaded | ||
|
|
||
| if ENV["PLAID_EU_CLIENT_ID"].present? && ENV["PLAID_EU_SECRET"].present? | ||
| config.plaid_eu = Plaid::Configuration.new | ||
| config.plaid_eu.server_index = Plaid::Configuration::Environment[ENV["PLAID_ENV"] || "sandbox"] | ||
| config.plaid_eu.api_key["PLAID-CLIENT-ID"] = ENV["PLAID_EU_CLIENT_ID"] | ||
| config.plaid_eu.api_key["PLAID-SECRET"] = ENV["PLAID_EU_SECRET"] | ||
| end | ||
| # Reload configurations from settings/ENV | ||
| Provider::PlaidAdapter.reload_configuration # US region | ||
| Provider::PlaidEuAdapter.reload_configuration # EU region | ||
| end |
There was a problem hiding this comment.
Add error handling to prevent initialization failures.
If reload_configuration raises an exception (e.g., invalid environment value, network issue, or missing adapter constant), the application will fail to start. This is especially risky in production where a configuration error could cause downtime.
Apply this diff to wrap the reload calls in error handling:
# Load Plaid configuration from adapters after initialization
Rails.application.config.after_initialize do
# Ensure provider adapters are loaded
Provider::Factory.ensure_adapters_loaded
# Reload configurations from settings/ENV
- Provider::PlaidAdapter.reload_configuration # US region
- Provider::PlaidEuAdapter.reload_configuration # EU region
+ begin
+ Provider::PlaidAdapter.reload_configuration # US region
+ rescue => e
+ Rails.logger.error("Failed to load Plaid US configuration: #{e.message}")
+ end
+
+ begin
+ Provider::PlaidEuAdapter.reload_configuration # EU region
+ rescue => e
+ Rails.logger.error("Failed to load Plaid EU configuration: #{e.message}")
+ end
end🤖 Prompt for AI Agents
In config/initializers/plaid.rb around lines 7 to 15, the calls to
Provider::PlaidAdapter.reload_configuration and
Provider::PlaidEuAdapter.reload_configuration run without error handling and can
raise exceptions that abort app startup; wrap each reload call in begin/rescue
that catches StandardError (or specific expected exceptions), log the error with
context (including adapter name and exception message/backtrace) using
Rails.logger.error, and continue startup (do not re-raise) so a configuration
failure for one adapter does not crash the application; ensure any sensitive
data is not logged and return a safe default or skip the adapter when errors
occur.
…from-upstream * 'main' of https://github.com/we-promise/sure: (60 commits) Add delay after disabling AI Assistant Avoid auto-categorizing without user categories (we-promise#261) Localization for an accountable group and sidebar (we-promise#257) Recurring transactions (we-promise#271) Lunch flow improvements (we-promise#268) Update ai.md (we-promise#263) Fix "invisible" merchants (we-promise#262) Lunchflow integration (we-promise#259) Build up to 0.6.5-alpha.6 Update AI model recommendations section Update AI assistant documentation with version caution Add support for dynamic config UI (we-promise#256) Feature/yahoo finance (we-promise#123) Update pages/dashboard locales (we-promise#255) Providers factory (we-promise#250) Add onboarding state selector for self-hosted signup (we-promise#251) Added a clickable drop down arrow that reveals list of instructions for Brand Fetch Client ID (we-promise#246) Added a clickable drop down arrow that reveals list of instructions for Twelve Data Secret Key (we-promise#247) Add runServices for db and redis in devcontainer Add PikaPods ... # Conflicts: # .devcontainer/docker-compose.yml # app/controllers/concerns/self_hostable.rb # app/models/provider/twelve_data.rb # app/views/pages/redis_configuration_error.html.erb # config/cable.yml # config/database.yml # config/initializers/sidekiq.rb
* Add support for dynamic config UI
* Add support for section description
* Better dynamic class settings
Added dynamic_fields hash field - Stores all undeclared settings
[] method - Checks declared fields first, then falls back to dynamic hash
[]= method - Updates declared fields normally, stores others in hash
No runtime field declaration - Fields are never dynamically created on the class
* FIX proper lookup for provider keys
- Also validate configurable values properly.
- Change Provider factory to use Rails autoloading (Zeitwerk)
* Fix factory
The derive_adapter_name method relies on string manipulation ("PlaidAccount".sub(/Account$/, "") + "Adapter" → "PlaidAdapter"), but we already have explicit registration in place.
* Make updates atomic, field-aware, and handle blanks explicitly
* Small UX detail
* Add support for PlaidEU in UI also
- This looks like partial support atm
* Add support for dynamic config UI
* Add support for section description
* Better dynamic class settings
Added dynamic_fields hash field - Stores all undeclared settings
[] method - Checks declared fields first, then falls back to dynamic hash
[]= method - Updates declared fields normally, stores others in hash
No runtime field declaration - Fields are never dynamically created on the class
* FIX proper lookup for provider keys
- Also validate configurable values properly.
- Change Provider factory to use Rails autoloading (Zeitwerk)
* Fix factory
The derive_adapter_name method relies on string manipulation ("PlaidAccount".sub(/Account$/, "") + "Adapter" → "PlaidAdapter"), but we already have explicit registration in place.
* Make updates atomic, field-aware, and handle blanks explicitly
* Small UX detail
* Add support for PlaidEU in UI also
- This looks like partial support atm
* Add support for dynamic config UI
* Add support for section description
* Better dynamic class settings
Added dynamic_fields hash field - Stores all undeclared settings
[] method - Checks declared fields first, then falls back to dynamic hash
[]= method - Updates declared fields normally, stores others in hash
No runtime field declaration - Fields are never dynamically created on the class
* FIX proper lookup for provider keys
- Also validate configurable values properly.
- Change Provider factory to use Rails autoloading (Zeitwerk)
* Fix factory
The derive_adapter_name method relies on string manipulation ("PlaidAccount".sub(/Account$/, "") + "Adapter" → "PlaidAdapter"), but we already have explicit registration in place.
* Make updates atomic, field-aware, and handle blanks explicitly
* Small UX detail
* Add support for PlaidEU in UI also
- This looks like partial support atm
2 participants
My idea is to have a module for providers to declare their configuration requirements.
It supports any number of fields and they will automatically show in UI.
Example below with Plaid and SimpleFIN
When you add a new provider (e.g., a Stripe adapter):
It will automatically appear in the UI with:
Some assumptions:
Screenshots:
Summary by CodeRabbit
New Features
Tests