Skip to content

Conversation

@sokie
Copy link
Collaborator

@sokie sokie commented Nov 11, 2025

  • Add migration and fix for lunch flow bug where all accounts were imported.
  • Remove "Period comparison" section from reports
  • Completely rewrite dynamic config, this should stop both atomic updates, collisions or accidental removes
    • Stop using a shared hash for dynamic fields
    • treat each dynamic key as its own RailsSettings entry instead of storing them inside a hash.

Summary by CodeRabbit

  • Removed Features

    • Comparison chart removed from the reports dashboard (income/expenses/net comparison).
  • Database Updates

    • Recurring transactions now track a manual flag and expected amount min/max/avg.
  • Refactor

    • Dynamic settings migrated to per-key storage for more granular updates.
    • Account linking logic refined to focus on connected accounts; orphaned accounts cleaned up.
  • Style

    • Minor template/comment formatting tweaks.
  • Tests

    • Tests updated to reflect UI and storage changes.

- Fix issue with lunch flow accounts that were imported
- Remove the period comparison section from reports
@coderabbitai
Copy link

coderabbitai bot commented Nov 11, 2025

Walkthrough

Removes the period comparison report UI and its server-side builder, migrates Settings dynamic storage from a single hash to per-key entries, restricts Lunchflow importer to linked accounts, adds recurring_transactions metadata columns, and applies small template and initializer safeguards.

Changes

Cohort / File(s) Summary
Reports Comparison Removal
app/controllers/reports_controller.rb, app/views/reports/_comparison_chart.html.erb, app/views/reports/index.html.erb, config/locales/views/reports/en.yml, test/controllers/reports_controller_test.rb, test/models/period_test.rb
Deletes build_comparison_data and its use, removes comparison partial and its rendering, deletes related i18n keys and tests asserting comparison UI/labels.
Dynamic Settings → Per-Key Entries
app/models/setting.rb, db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb, app/controllers/settings/providers_controller.rb, app/models/provider/configurable.rb, test/models/setting_test.rb, test/controllers/settings/providers_controller_test.rb
Replaces single dynamic_fields hash with individual dynamic:KEY Setting rows; updates [], []=, key?, delete, dynamic_keys, adds dynamic_key_name helper; migration moves data; controller no longer batches dynamic_fields, writes per-key.
Lunchflow Account Import Filtering
app/models/lunchflow_item/importer.rb, db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb
Limits existing_account_ids to lunchflow accounts joined to an account_provider; adds migration to remove orphaned LunchflowAccount records.
Recurring Transactions Schema
db/schema.rb
Adds manual (boolean, default false) and expected_amount_min, expected_amount_max, expected_amount_avg (decimal) columns to recurring_transactions.
Views / Template Fixes
app/views/lunchflow_items/select_accounts.html.erb, app/views/lunchflow_items/select_existing_account.html.erb, app/views/settings/providers/_provider_form.html.erb
Minor ERB quoting changes and conversion of an ERB-tagged comment to a proper ERB comment (no behavioral change).
Initializer Guard
config/initializers/plaid.rb
Adds guards/rescue to skip Plaid reload when DB or settings table is unavailable.
Tests Adjusted
test/* (see above)
Removes comparison-related tests, updates/extends Setting and provider controller tests to reflect per-key dynamic storage and teardown cleanup.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant ProvidersController as ProvidersController
    participant SettingModel as Setting
    participant DB as Database

    rect rgb(240,248,255)
    Note over Client,ProvidersController: Old (batch update)
    Client->>ProvidersController: PATCH provider params (dynamic fields)
    ProvidersController->>SettingModel: read Setting.var == "dynamic_fields"
    SettingModel->>DB: SELECT dynamic_fields row
    ProvidersController->>SettingModel: write merged dynamic_fields hash
    SettingModel->>DB: UPDATE dynamic_fields row
    end

    rect rgb(245,245,255)
    Note over Client,ProvidersController: New (per-key writes)
    Client->>ProvidersController: PATCH provider params (dynamic fields)
    ProvidersController->>SettingModel: for each key -> Setting[key] = value
    loop per key
      SettingModel->>DB: INSERT/UPDATE var = "dynamic:<key>"
    end
    Note over SettingModel,DB: nil -> delete corresponding row
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Pay special attention to:
    • app/models/setting.rb — correctness of lookup precedence (declared vs dynamic) and nil/deletion semantics.
    • Migration db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb — ensure data is migrated safely and down path aggregates correctly.
    • app/controllers/settings/providers_controller.rb — transactional semantics after moving to per-key writes.
    • app/models/lunchflow_item/importer.rb and db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb — ensure no valid accounts are excluded and orphan deletion is safe.
    • Test changes — verify coverage reflects new storage model.

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • jjmata
  • matthieuEv

Poem

🐰 I hopped through settings, one key at a time,
Comparison charts tucked away, quiet and sublime,
Lunchflow now pairs only what’s truly linked,
Recurring rows gain numbers neatly inked—
A tiny rabbit clap, a carrot-shaped rhyme 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Fixes & Improvements' is vague and generic, using non-descriptive terms that don't convey meaningful information about the substantial changes in this pull request. Consider using a more specific title that reflects the main objectives, such as 'Refactor dynamic settings storage and remove comparison charts' or 'Migrate dynamic fields to per-key entries and remove period comparison UI'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

❤️ Share

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/models/setting.rb (1)

65-83: Race condition: find_or_initialize_by + save! is not atomic.

The comment on line 77 claims to use upsert, but the implementation uses find_or_initialize_by followed by save!, which is not atomic. Two concurrent processes can both find no record, initialize separate instances, and then encounter a collision on save.

This contradicts the PR's objective to prevent atomic update issues and collisions.

Apply this diff to use Rails' atomic upsert:

       else
         # Store as individual dynamic entry
         dynamic_key = dynamic_key_name(key_str)
         if value.nil?
           where(var: dynamic_key).destroy_all
         else
-          # Use upsert to avoid conflicts
-          record = find_or_initialize_by(var: dynamic_key)
-          record.value = value
-          record.save!
+          # Use upsert for atomic operation to avoid race conditions
+          upsert({ var: dynamic_key, value: value }, unique_by: :var)
         end
       end

Note: Ensure the database has a unique index on the var column for upsert to work correctly.

🧹 Nitpick comments (1)
db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb (1)

6-17: Migration logic is correct and well-logged.

The migration properly identifies and removes orphaned accounts. The logging provides good visibility into the operation.

Consider using delete_all instead of destroy_all for better performance since these are orphaned records where callbacks are unlikely to be necessary:

-      orphaned_accounts.destroy_all
+      orphaned_accounts.delete_all

However, if callbacks are needed for proper cleanup (e.g., removing associated records), destroy_all is the correct choice.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7f5cf4c and a8b4b25.

📒 Files selected for processing (15)
  • app/controllers/reports_controller.rb (0 hunks)
  • app/controllers/settings/providers_controller.rb (1 hunks)
  • app/models/lunchflow_item/importer.rb (1 hunks)
  • app/models/provider/configurable.rb (2 hunks)
  • app/models/setting.rb (3 hunks)
  • app/views/reports/_comparison_chart.html.erb (0 hunks)
  • app/views/reports/index.html.erb (0 hunks)
  • config/locales/views/reports/en.yml (0 hunks)
  • db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb (1 hunks)
  • db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb (1 hunks)
  • db/schema.rb (2 hunks)
  • test/controllers/reports_controller_test.rb (0 hunks)
  • test/controllers/settings/providers_controller_test.rb (5 hunks)
  • test/models/period_test.rb (0 hunks)
  • test/models/setting_test.rb (2 hunks)
💤 Files with no reviewable changes (6)
  • config/locales/views/reports/en.yml
  • test/models/period_test.rb
  • test/controllers/reports_controller_test.rb
  • app/controllers/reports_controller.rb
  • app/views/reports/index.html.erb
  • app/views/reports/_comparison_chart.html.erb
🧰 Additional context used
📓 Path-based instructions (11)
db/migrate/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Place database migrations under db/migrate/

Simple validations like null checks and unique constraints should be handled by database constraints in migration files.

Files:

  • db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb
  • db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb
**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules

Files:

  • db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb
  • app/models/provider/configurable.rb
  • test/models/setting_test.rb
  • app/models/lunchflow_item/importer.rb
  • app/models/setting.rb
  • app/controllers/settings/providers_controller.rb
  • db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb
  • db/schema.rb
  • test/controllers/settings/providers_controller_test.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:

  • db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb
  • app/models/provider/configurable.rb
  • test/models/setting_test.rb
  • app/models/lunchflow_item/importer.rb
  • app/models/setting.rb
  • app/controllers/settings/providers_controller.rb
  • db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb
  • db/schema.rb
  • test/controllers/settings/providers_controller_test.rb
db/migrate/**/*.rb

📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)

Enforce simple validations in the database (NULL constraints, unique indexes, etc.)

Files:

  • db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb
  • db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.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.rb
  • app/models/lunchflow_item/importer.rb
  • app/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.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/configurable.rb
  • app/models/lunchflow_item/importer.rb
  • app/models/setting.rb
  • app/controllers/settings/providers_controller.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.rb
  • app/models/lunchflow_item/importer.rb
  • app/models/setting.rb
app/{models,controllers,views}/**/*.{rb,erb}

📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)

Avoid N+1 queries

Files:

  • app/models/provider/configurable.rb
  • app/models/lunchflow_item/importer.rb
  • app/models/setting.rb
  • app/controllers/settings/providers_controller.rb
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/setting_test.rb
  • test/controllers/settings/providers_controller_test.rb
db/schema.rb

📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)

Treat db/schema.rb as the authoritative source of truth for model relationships

Files:

  • db/schema.rb
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-16T13:17:53.155Z
Learning: PRs should include clear description, linked issues, screenshots for UI changes, and migration notes; ensure CI, tests, RuboCop, and Biome are clean
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/provider/concepts/**/*.rb : Define concept interfaces under app/models/provider/concepts (e.g., exchange_rate interface) to enable pluggable providers

Applied to files:

  • app/models/provider/configurable.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/provider/**/*.rb : Concrete provider classes must be under the Provider:: namespace, inherit from Provider, wrap calls with with_provider_response, and raise on invalid/unavailable data

Applied to files:

  • app/models/provider/configurable.rb
  • test/controllers/settings/providers_controller_test.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/**/provided.rb : Implement Provided concerns in each model namespace to (1) choose the concept provider and (2) expose convenience methods for accessing provider data

Applied to files:

  • app/models/provider/configurable.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 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/configurable.rb
📚 Learning: 2025-09-23T22:22:48.511Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-09-23T22:22:48.511Z
Learning: Applies to test/**/*_test.rb : Avoid tests that merely verify framework/ActiveRecord behavior

Applied to files:

  • test/models/setting_test.rb
📚 Learning: 2025-09-23T22:22:48.511Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-09-23T22:22:48.511Z
Learning: Applies to test/**/*_test.rb : For tests needing many records, use Rails helpers to construct data and inline the creation in the test

Applied to files:

  • test/models/setting_test.rb
📚 Learning: 2025-09-23T22:22:48.511Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-09-23T22:22:48.511Z
Learning: Applies to test/**/*_test.rb : Do not use factories (e.g., FactoryBot) in tests; rely on fixtures instead

Applied to files:

  • test/models/setting_test.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/account.rb : Account syncs are orchestrated by Account#sync_data (auto-match transfers, compute balances from start_date..today, enrich transactions) and should run whenever an Entry is updated

Applied to files:

  • app/models/lunchflow_item/importer.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.

Applied to files:

  • app/models/lunchflow_item/importer.rb
📚 Learning: 2025-09-13T11:27:55.271Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.

Applied to files:

  • app/models/lunchflow_item/importer.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to db/schema.rb : Treat db/schema.rb as the authoritative source of truth for model relationships

Applied to files:

  • db/schema.rb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to db/migrate/**/*.rb : Enforce simple validations in the database (NULL constraints, unique indexes, etc.)

Applied to files:

  • db/schema.rb
📚 Learning: 2025-09-16T13:17:53.155Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-16T13:17:53.155Z
Learning: Applies to db/seeds.rb : Keep seed data definitions in db/seeds.rb

Applied to files:

  • db/schema.rb
🧬 Code graph analysis (5)
db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb (2)
app/models/provider/configurable.rb (1)
  • value (188-202)
app/models/setting.rb (2)
  • key (53-63)
  • key (65-83)
test/models/setting_test.rb (2)
app/models/provider/configurable.rb (1)
  • value (188-202)
app/models/setting.rb (3)
  • delete (95-103)
  • key? (86-92)
  • dynamic_keys (106-108)
app/models/setting.rb (1)
app/models/provider/configurable.rb (1)
  • value (188-202)
app/controllers/settings/providers_controller.rb (1)
app/models/provider/configurable.rb (1)
  • value (188-202)
test/controllers/settings/providers_controller_test.rb (2)
test/test_helper.rb (1)
  • with_self_hosting (72-75)
app/models/setting.rb (1)
  • key? (86-92)
🪛 GitHub Check: ci / lint
app/models/setting.rb

[failure] 112-112:
Layout/IndentationWidth: Use 2 (not 0) spaces for indented_internal_methods indentation.

🔇 Additional comments (14)
app/models/lunchflow_item/importer.rb (1)

32-37: LGTM! Critical bug fix to prevent importing all accounts.

The change correctly filters to only accounts with an associated account_provider, preventing the bug where all accounts (including orphaned ones) were being imported. This aligns with the migration that removes orphaned accounts and ensures only user-selected accounts are updated during sync.

app/models/provider/configurable.rb (1)

4-5: LGTM! Documentation accurately reflects the new storage model.

The updated comments clearly describe the new bracket-style access pattern and per-field storage approach, helping developers understand how settings are now stored individually rather than in a shared hash.

Also applies to: 189-190

db/schema.rb (1)

13-13: LGTM! Schema changes for recurring transaction enhancements.

The new columns for tracking expected amounts (min, max, avg) and manual flag are well-defined with appropriate types and constraints. The nullable decimals allow flexibility for transactions without historical data.

Also applies to: 690-693

test/models/setting_test.rb (2)

10-13: LGTM! Proper test teardown for dynamic fields.

The teardown ensures test isolation by cleaning up all dynamic field entries after each test, preventing cross-test pollution.


70-133: LGTM! Comprehensive test coverage for dynamic field storage.

The new tests thoroughly cover the per-field storage functionality including setting, getting, deletion, existence checking, and interaction with declared fields. Well-structured and clear test cases.

app/controllers/settings/providers_controller.rb (1)

57-59: LGTM! Critical fix to prevent atomic update issues and collisions.

The change from batched dynamic_fields updates to per-field writes (Setting[key_str] = value) is a key improvement that prevents race conditions and accidental removals. Each field is now stored independently, eliminating the collision issues that occurred with the shared hash approach. The transaction boundary still ensures consistency.

test/controllers/settings/providers_controller_test.rb (1)

44-55: LGTM! Tests thoroughly updated for per-field storage model.

The tests comprehensively validate the new per-field storage behavior including:

  • Individual field updates using bracket notation
  • Preservation of unrelated fields during updates
  • Proper nil value handling and entry deletion
  • Sequential updates without conflicts
  • Atomicity within transactions

All assertions and comments properly reflect the new storage semantics.

Also applies to: 59-80, 84-112, 114-140, 165-181, 184-219

db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb (2)

2-18: LGTM! Migration correctly converts hash to individual entries.

The up migration properly:

  • Returns early if no dynamic_fields record exists (line 5)
  • Handles empty hash case gracefully
  • Creates individual entries with dynamic: prefix
  • Cleans up the old record

20-34: LGTM! Reversible migration with proper safeguards.

The down migration correctly reverses the change by:

  • Collecting all dynamic entries back into a hash
  • Destroying the individual entries
  • Only recreating the dynamic_fields record if there are entries (line 33)

This ensures the migration is fully reversible.

app/models/setting.rb (5)

14-15: Clear documentation of the new approach.

The updated comments accurately explain the shift to individual dynamic entries and the benefits of this approach.


52-63: LGTM!

The bracket-read implementation correctly prioritizes declared fields and safely falls back to individual dynamic entries using find_by with safe navigation.


86-92: LGTM!

The key? method correctly checks both declared fields and individual dynamic entries.


94-103: LGTM!

The delete method correctly prevents deletion of declared fields and returns the deleted value, following Ruby conventions.


106-108: LGTM!

The dynamic_keys method appropriately uses a LIKE query to enumerate all dynamic entries and strips the prefix.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
app/models/setting.rb (1)

110-114: Fix indentation per RuboCop

dynamic_key_name is still indented two spaces too deep inside the class << self block, so lint continues to fail. Align it with the other class methods.

Apply this diff to correct the indentation:

-    private
-
-      def dynamic_key_name(key_str)
-        "dynamic:#{key_str}"
-      end
+    private
+
+    def dynamic_key_name(key_str)
+      "dynamic:#{key_str}"
+    end
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a8b4b25 and 9f9f415.

📒 Files selected for processing (4)
  • app/models/setting.rb (3 hunks)
  • app/views/lunchflow_items/select_accounts.html.erb (1 hunks)
  • app/views/lunchflow_items/select_existing_account.html.erb (1 hunks)
  • app/views/settings/providers/_provider_form.html.erb (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • app/views/lunchflow_items/select_existing_account.html.erb
  • app/views/settings/providers/_provider_form.html.erb
🧰 Additional context used
📓 Path-based instructions (15)
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.yml

Always use the icon helper (icon) for icons; never call lucide_icon directly

Files:

  • app/views/lunchflow_items/select_accounts.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/lunchflow_items/select_accounts.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/lunchflow_items/select_accounts.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/lunchflow_items/select_accounts.html.erb
app/{models,controllers,views}/**/*.{rb,erb}

📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)

Avoid N+1 queries

Files:

  • app/views/lunchflow_items/select_accounts.html.erb
  • app/models/setting.rb
{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/lunchflow_items/select_accounts.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/lunchflow_items/select_accounts.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/lunchflow_items/select_accounts.html.erb
{app/views/**,app/helpers/**}

📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)

Always generate semantic HTML

Files:

  • app/views/lunchflow_items/select_accounts.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/lunchflow_items/select_accounts.html.erb
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/setting.rb
app/**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/

Files:

  • app/models/setting.rb
**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules

Files:

  • app/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:

  • app/models/setting.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/setting.rb
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-16T13:17:53.155Z
Learning: PRs should include clear description, linked issues, screenshots for UI changes, and migration notes; ensure CI, tests, RuboCop, and Biome are clean
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/{views,components}/**/*.html.erb : Keep domain logic out of ERB templates; compute classes and logic in component/Ruby code, not inline in the template

Applied to files:

  • app/views/lunchflow_items/select_accounts.html.erb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.

Applied to files:

  • app/views/lunchflow_items/select_accounts.html.erb
📚 Learning: 2025-09-16T13:17:53.155Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-16T13:17:53.155Z
Learning: Applies to **/*.rb : Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules

Applied to files:

  • app/models/setting.rb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/models/**/*.rb : Model-level validations may mirror DB constraints but are not strictly required

Applied to files:

  • app/models/setting.rb
🧬 Code graph analysis (1)
app/models/setting.rb (1)
app/models/provider/configurable.rb (1)
  • value (188-202)
🔇 Additional comments (1)
app/views/lunchflow_items/select_accounts.html.erb (1)

19-22: LGTM: Harmless formatting changes.

The quote style adjustments have no functional impact and maintain consistency in the template.

Reuse the base class’ atomic setter to leverage its locking and cache invalidation.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
app/models/setting.rb (1)

77-80: Upsert fixes the race condition from previous review.

The atomic upsert operation at line 78 correctly addresses the prior race condition flagged in the previous review (where find_or_initialize_by + save! was non-atomic). Cache invalidation at line 79 is also correct.

Optional refactor: Consider whether the base class RailsSettings::Base provides an atomic setter you can delegate to (e.g., calling a base method with the dynamic key and value). This would centralize cache invalidation and leverage any existing locking/transaction logic.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 09c512b and cb78a5e.

📒 Files selected for processing (1)
  • app/models/setting.rb (3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/setting.rb
app/**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/

Files:

  • app/models/setting.rb
**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules

Files:

  • app/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:

  • app/models/setting.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/setting.rb
app/{models,controllers,views}/**/*.{rb,erb}

📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)

Avoid N+1 queries

Files:

  • app/models/setting.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-16T13:17:53.155Z
Learning: PRs should include clear description, linked issues, screenshots for UI changes, and migration notes; ensure CI, tests, RuboCop, and Biome are clean
📚 Learning: 2025-09-16T13:17:53.155Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-16T13:17:53.155Z
Learning: Applies to **/*.rb : Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules

Applied to files:

  • app/models/setting.rb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/models/**/*.rb : Model-level validations may mirror DB constraints but are not strictly required

Applied to files:

  • app/models/setting.rb
🧬 Code graph analysis (1)
app/models/setting.rb (1)
app/models/provider/configurable.rb (1)
  • value (188-202)
🔇 Additional comments (5)
app/models/setting.rb (5)

14-15: Good documentation of the storage model change.

The comments clearly explain the shift from a shared hash to per-key entries and the motivation (preventing race conditions).


51-63: LGTM: Bracket reader correctly prioritizes declared fields.

The fallback to find_by with dynamic key prefix is sound, and safe navigation handles missing entries gracefully.


84-91: LGTM: Key existence check is correct.

The method correctly prioritizes declared fields and efficiently checks dynamic entry existence with exists?.


104-107: LGTM: Dynamic keys enumeration is correct.

The SQL LIKE query with parameterized placeholder safely retrieves all dynamic entries, and the prefix stripping produces clean key names.


109-113: LGTM: Private helper is well-scoped and correctly indented.

The dynamic_key_name helper centralizes the "dynamic:" prefix logic, and the indentation now follows Ruby style (2 spaces).

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
app/models/setting.rb (2)

53-63: Consider leveraging the parent class's getter for caching.

The current implementation uses find_by directly (line 61), which queries the database on every access. The parent class RailsSettings::Base likely has a caching layer for reads. For undeclared dynamic fields, you might benefit from calling the parent's [] accessor with the dynamic_key to leverage that cache:

def [](key)
  key_str = key.to_s
  if respond_to?(key_str)
    public_send(key_str)
  else
    # Delegate to parent's cached getter
    super(dynamic_key_name(key_str))
  end
end

This would align with the parent's caching strategy and potentially improve performance for frequently accessed dynamic settings.


72-81: Consider delegating to the parent class's setter for consistency.

While upsert (line 79) is atomic and resolves the race condition from earlier code, it bypasses the parent class's []= logic, which may include callbacks, validations, or more sophisticated caching. Delegating to the base class ensures consistency and leverages its built-in mechanisms:

       else
         # Store as individual dynamic entry
         dynamic_key = dynamic_key_name(key_str)
         if value.nil?
           where(var: dynamic_key).destroy_all
           clear_cache
         else
-          # Use upsert for atomic insert/update to avoid race conditions
-          upsert({ var: dynamic_key, value: value.to_yaml }, unique_by: :var)
-          clear_cache
+          # Delegate to parent's atomic setter
+          super(dynamic_key, value)
         end
       end

This approach reuses the parent's atomic update logic and ensures that any parent-level side effects (logging, auditing, etc.) are preserved.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb78a5e and ddc23ad.

📒 Files selected for processing (1)
  • app/models/setting.rb (3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/setting.rb
app/**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/

Files:

  • app/models/setting.rb
**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules

Files:

  • app/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:

  • app/models/setting.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/setting.rb
app/{models,controllers,views}/**/*.{rb,erb}

📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)

Avoid N+1 queries

Files:

  • app/models/setting.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-16T13:17:53.155Z
Learning: PRs should include clear description, linked issues, screenshots for UI changes, and migration notes; ensure CI, tests, RuboCop, and Biome are clean
📚 Learning: 2025-09-16T13:17:53.155Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-16T13:17:53.155Z
Learning: Applies to **/*.rb : Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules

Applied to files:

  • app/models/setting.rb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/models/**/*.rb : Model-level validations may mirror DB constraints but are not strictly required

Applied to files:

  • app/models/setting.rb
🧬 Code graph analysis (1)
app/models/setting.rb (1)
app/models/provider/configurable.rb (1)
  • value (188-202)
🔇 Additional comments (2)
app/models/setting.rb (2)

94-104: LGTM—cache invalidation now present.

The delete method correctly invalidates the cache (line 102) after removing dynamic entries. This addresses the previous concern about stale cache entries after deletion.


111-115: LGTM—indentation corrected.

The dynamic_key_name helper is properly indented per Ruby style guidelines. The previous indentation issue has been resolved.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb (1)

23-27: Consider raising ActiveRecord::IrreversibleMigration for clarity.

The down method logs that restoration is impossible but doesn't raise an error, allowing rollback to silently succeed without actually restoring data. This could be confusing. Rails convention is to explicitly raise ActiveRecord::IrreversibleMigration for truly irreversible migrations.

Apply this diff:

   def down
-    # Cannot restore orphaned accounts that were deleted
-    # These were unused accounts from old behavior that shouldn't have been created
-    Rails.logger.info "Cannot restore orphaned LunchflowAccount records (data migration is irreversible)"
+    raise ActiveRecord::IrreversibleMigration, "Cannot restore orphaned LunchflowAccount records"
   end
config/initializers/plaid.rb (1)

18-20: Consider adding optional logging for debugging.

The rescue block appropriately handles database-not-ready scenarios. However, adding a debug-level log message could help troubleshoot initialization issues during development.

 rescue ActiveRecord::NoDatabaseError, PG::ConnectionBad
   # Database doesn't exist yet, skip initialization
+  Rails.logger.debug("Skipping Plaid initialization: database not ready") if defined?(Rails.logger)
   nil
db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb (1)

2-18: Consider using upsert to handle interrupted migrations gracefully.

The create! call on line 10 will raise an error if a dynamic:#{key} entry already exists. If the migration is interrupted after creating some entries but before destroying the old dynamic_fields record, re-running the migration would fail.

Apply this diff to use upsert for safer re-execution:

     # Parse the hash and create individual entries
     dynamic_fields_hash = dynamic_fields_record.value || {}
     dynamic_fields_hash.each do |key, value|
-      Setting.create!(
+      Setting.upsert(
+        { var: "dynamic:#{key}", value: value },
+        unique_by: :var
+      )
+    end
+
+    # Alternative: use find_or_create_by if upsert is not preferred
+    dynamic_fields_hash.each do |key, value|
+      Setting.find_or_create_by!(var: "dynamic:#{key}") do |setting|
-        var: "dynamic:#{key}",
-        value: value
-      )
+        setting.value = value
+      end
     end
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ddc23ad and 32c6d70.

📒 Files selected for processing (3)
  • config/initializers/plaid.rb (1 hunks)
  • db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb (1 hunks)
  • db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
db/migrate/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Place database migrations under db/migrate/

Simple validations like null checks and unique constraints should be handled by database constraints in migration files.

Files:

  • db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb
  • db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb
**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules

Files:

  • db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb
  • db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb
  • config/initializers/plaid.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:

  • db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb
  • db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb
  • config/initializers/plaid.rb
db/migrate/**/*.rb

📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)

Enforce simple validations in the database (NULL constraints, unique indexes, etc.)

Files:

  • db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb
  • db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb
config/**

📄 CodeRabbit inference engine (AGENTS.md)

Store application and environment configuration under config/

Files:

  • config/initializers/plaid.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-16T13:17:53.155Z
Learning: PRs should include clear description, linked issues, screenshots for UI changes, and migration notes; ensure CI, tests, RuboCop, and Biome are clean
📚 Learning: 2025-09-13T11:40:26.913Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_entry/processor.rb:10-12
Timestamp: 2025-09-13T11:40:26.913Z
Learning: The plaid_id field is used consistently across multiple banking integrations (Plaid, SimpleFin, and Enable Banking) as a common external identifier field. A comprehensive refactor to use a proper external_id field should be done across all integrations in a separate PR to maintain consistency and avoid scope creep.

Applied to files:

  • config/initializers/plaid.rb
📚 Learning: 2025-09-13T12:23:31.324Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.

Applied to files:

  • config/initializers/plaid.rb
🧬 Code graph analysis (2)
db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb (2)
app/models/provider/configurable.rb (1)
  • value (188-202)
app/models/setting.rb (2)
  • key (53-63)
  • key (65-83)
config/initializers/plaid.rb (4)
app/models/provider/factory.rb (1)
  • ensure_adapters_loaded (45-57)
app/models/provider/configurable.rb (1)
  • reload_configuration (85-88)
app/models/provider/plaid_adapter.rb (1)
  • reload_configuration (55-68)
app/models/provider/plaid_eu_adapter.rb (1)
  • reload_configuration (47-60)
⏰ 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)
db/migrate/20251111084519_remove_orphaned_lunchflow_accounts.rb (2)

1-1: LGTM!

Migration class declaration follows Rails 7.2 conventions correctly.


6-15: The original review comment is incorrect and should be disregarded.

The migration correctly uses destroy_all. The LunchflowAccount model has a dependent association (has_one :account_provider, as: :provider, dependent: :destroy), which means deleting a LunchflowAccount must trigger deletion of its associated account_provider record. Using delete_all as suggested would skip these callbacks and leave orphaned account_provider records behind, creating a different data consistency problem. The current implementation is appropriate for this cleanup task.

Likely an incorrect or invalid review comment.

config/initializers/plaid.rb (1)

9-10: Guard correctly prevents initialization when settings table is missing.

The table existence check appropriately handles scenarios where migrations haven't run yet. The rescue block below (lines 18-20) catches exceptions that might be raised by the connection or table_exists? calls themselves when the database doesn't exist.

Comment on lines +20 to +34
def down
# Collect all dynamic: entries back into a hash
dynamic_fields_hash = {}
Setting.where("var LIKE ?", "dynamic:%").find_each do |record|
key = record.var.sub(/^dynamic:/, "")
dynamic_fields_hash[key] = record.value
record.destroy
end

# Recreate the dynamic_fields record with the hash
Setting.create!(
var: "dynamic_fields",
value: dynamic_fields_hash
) if dynamic_fields_hash.any?
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reorder operations to prevent potential data loss during rollback.

The current implementation destroys individual dynamic:* entries (line 26) before creating the dynamic_fields record (line 30). If the create! call fails, the dynamic entries are already destroyed, resulting in data loss.

Apply this diff to create the record first, then destroy entries:

   def down
     # Collect all dynamic: entries back into a hash
     dynamic_fields_hash = {}
     Setting.where("var LIKE ?", "dynamic:%").find_each do |record|
       key = record.var.sub(/^dynamic:/, "")
       dynamic_fields_hash[key] = record.value
-      record.destroy
     end
 
-    # Recreate the dynamic_fields record with the hash
-    Setting.create!(
-      var: "dynamic_fields",
-      value: dynamic_fields_hash
-    ) if dynamic_fields_hash.any?
+    # Recreate the dynamic_fields record first to preserve data
+    if dynamic_fields_hash.any?
+      Setting.create!(
+        var: "dynamic_fields",
+        value: dynamic_fields_hash
+      )
+      
+      # Only destroy individual entries after successfully creating the aggregated record
+      Setting.where("var LIKE ?", "dynamic:%").destroy_all
+    end
   end
🤖 Prompt for AI Agents
In db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb
around lines 20 to 34, change the rollback sequence so you create the
consolidated dynamic_fields record before deleting the individual dynamic:* rows
to avoid data loss; specifically, build the dynamic_fields hash from existing
dynamic:* records, then call Setting.create! for var: "dynamic_fields" with that
hash, and only after a successful create (ideally inside a single transaction)
delete the individual dynamic:* records (or destroy them after create!),
ensuring you wrap the create + delete steps in a transaction and let exceptions
bubble to prevent partial destructive changes.

@jjmata jjmata self-requested a review November 11, 2025 18:48
@jjmata jjmata self-assigned this Nov 11, 2025
@jjmata jjmata merged commit fad241c into we-promise:main Nov 11, 2025
6 checks passed
alessiocappa pushed a commit to alessiocappa/sure that referenced this pull request Nov 16, 2025
* Some improvements

- Fix issue with lunch flow accounts that were imported
- Remove the period comparison section from reports

* Add cleanup migration

* FIX for dynamic config

* Fix linter

* FIX settings setter

Reuse the base class’ atomic setter to leverage its locking and cache invalidation.

* Make upsert atomic

* Remove migration file

Signed-off-by: soky srm <[email protected]>

* Delete db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb

Signed-off-by: soky srm <[email protected]>

* Fix cache reset

* Revert "Remove migration file"

This reverts commit 1f2a21e.

* Revert "Delete db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb"

This reverts commit 29dcaaa.

* Fix Plaid initialiser

---------

Signed-off-by: soky srm <[email protected]>
alessiocappa pushed a commit to alessiocappa/sure that referenced this pull request Nov 17, 2025
* Some improvements

- Fix issue with lunch flow accounts that were imported
- Remove the period comparison section from reports

* Add cleanup migration

* FIX for dynamic config

* Fix linter

* FIX settings setter

Reuse the base class’ atomic setter to leverage its locking and cache invalidation.

* Make upsert atomic

* Remove migration file

Signed-off-by: soky srm <[email protected]>

* Delete db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb

Signed-off-by: soky srm <[email protected]>

* Fix cache reset

* Revert "Remove migration file"

This reverts commit 1f2a21e.

* Revert "Delete db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb"

This reverts commit 29dcaaa.

* Fix Plaid initialiser

---------

Signed-off-by: soky srm <[email protected]>
alessiocappa pushed a commit to alessiocappa/sure that referenced this pull request Nov 17, 2025
* Some improvements

- Fix issue with lunch flow accounts that were imported
- Remove the period comparison section from reports

* Add cleanup migration

* FIX for dynamic config

* Fix linter

* FIX settings setter

Reuse the base class’ atomic setter to leverage its locking and cache invalidation.

* Make upsert atomic

* Remove migration file

Signed-off-by: soky srm <[email protected]>

* Delete db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb

Signed-off-by: soky srm <[email protected]>

* Fix cache reset

* Revert "Remove migration file"

This reverts commit 1f2a21e.

* Revert "Delete db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb"

This reverts commit 29dcaaa.

* Fix Plaid initialiser

---------

Signed-off-by: soky srm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants