Add comprehensive bond accounts and API integration#1330
Draft
UberDudePL wants to merge 75 commits intowe-promise:mainfrom
Draft
Add comprehensive bond accounts and API integration#1330UberDudePL wants to merge 75 commits intowe-promise:mainfrom
UberDudePL wants to merge 75 commits intowe-promise:mainfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Bond account support with Bond and BondLot models, CPI/inflation provider integrations and importers, new provider adapters, jobs/schedules, controllers/routes, Stimulus form controllers, view components/partials, DB migrations/schema updates, balance/ledger integrations, settings/env vars, presenters, and extensive tests/locales/docs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client
participant BondLotsCtrl as BondLotsController
participant BondLotModel as BondLot
participant Entry as Entry/Transaction
participant Account
participant Sync as SyncJob
Client->>BondLotsCtrl: POST /bond_lots (params)
BondLotsCtrl->>BondLotModel: build & validate
BondLotsCtrl->>BondLotModel: save_with_purchase_entry!
BondLotModel->>Entry: create purchase Entry & Transaction (embed bond_lot_id)
BondLotModel-->>BondLotsCtrl: persisted
BondLotsCtrl->>Account: schedule sync_later(window_start_date)
Account->>Sync: enqueue
BondLotsCtrl-->>Client: redirect with notice
sequenceDiagram
actor Scheduler
participant ImportJob as ImportInflationRatesJob
participant Importer as InflationRateImporter
participant Gus as GusInflationRate
participant Intl as InflationRate
participant Setting
Scheduler->>ImportJob: perform(start_year,end_year,providers)
ImportJob->>Importer: new(...) / import_all
Importer->>Gus: import_range!(...)
Importer->>Intl: import_year! (per-year, per-provider)
Gus-->>Importer: counts
Intl-->>Importer: counts
Importer-->>ImportJob: results hash
ImportJob->>Setting: update last_import_at/count/range/details
sequenceDiagram
actor Scheduler
participant SettleJob as SettleMaturedBondLotsJob
participant BondLot as BondLot
participant Provider as InflationProvider
participant InflationRateModel as InflationRate
participant Entry
Scheduler->>SettleJob: perform(on: Date.current)
SettleJob->>BondLot: find auto-close matured lots
loop per lot
BondLot->>Provider: request rates (may trigger import)
Provider->>InflationRateModel: for_date/import as needed
InflationRateModel-->>Provider: record
BondLot->>BondLot: settle_if_matured! (compute gain/tax/net)
BondLot->>Entry: create settlement entry/transaction
BondLot->>BondLot: mark closed, persist settlement_amount
end
SettleJob-->>Scheduler: complete or raise first error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92a8fad207
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds first-class Bond accounts and BondLot purchases with automated maturity settlement and optional inflation-linked (EOD/ROD) support backed by GUS SDP CPI imports, plus UI/dashboard surfaces and self-hosting settings to configure the integration.
Changes:
- Introduces
Bond/BondLotmodels, controllers, views, routes, and fixtures/tests to support bond accounts and purchase lots (including settlement automation and replacement lots). - Adds GUS SDP provider +
GusInflationRatemodel and scheduled/manual import job, with self-hosting settings UI and env-based configuration. - Extends account UI (tabs, chart tooltip, dashboard section) and balance calculations to account for bond holdings and cash.
Reviewed changes
Copilot reviewed 74 out of 74 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| test/models/gus_inflation_rate_test.rb | Adds unit tests for CPI lag lookup and import error handling. |
| test/models/bond_test.rb | Adds tests for bond defaults and derived maturity date/original balance. |
| test/models/bond_lot_test.rb | Adds extensive tests for lot valuation, inflation logic, and settlement automation. |
| test/jobs/settle_matured_bond_lots_job_test.rb | Tests scheduled settlement job behavior. |
| test/jobs/import_gus_inflation_rates_job_test.rb | Tests CPI import job gating + status persistence. |
| test/fixtures/bonds.yml | Adds bond fixture for tests. |
| test/fixtures/bond_lots.yml | Adds bond lot fixtures for tests. |
| test/fixtures/accounts.yml | Adds bond account fixture for tests. |
| test/controllers/transactions/bulk_deletions_controller_test.rb | Ensures bulk deletion removes linked bond lots via entry association. |
| test/controllers/settings/hostings_controller_test.rb | Tests manual CPI import enqueueing and disabled gating. |
| test/controllers/pages_controller_test.rb | Verifies dashboard bond summary + notice behavior. |
| test/controllers/bonds_controller_test.rb | Tests bond account CRUD via AccountableResource. |
| test/controllers/bond_lots_controller_test.rb | Tests bond lot CRUD and entry linkage. |
| test/controllers/accounts_controller_test.rb | Tests bond tabs and auto-settlement on account open. |
| docs/hosting/docker.md | Documents optional GUS CPI env vars and settings location. |
| db/schema.rb | Adds bonds/bond_lots/gus_inflation_rates schema and FKs. |
| db/migrate/20260331211000_add_requires_rate_review_to_bond_lots.rb | Adds requires_rate_review flag + index. |
| db/migrate/20260331210000_add_tax_wrapper_and_auto_buy_to_bonds.rb | Adds bond tax wrapper + auto-buy setting. |
| db/migrate/20260331193000_add_maturity_settlement_fields_to_bond_lots.rb | Adds settlement/tax fields and indexes. |
| db/migrate/20260331170000_add_gus_inflation_rates_and_auto_fetch_to_bond_lots.rb | Adds CPI table + per-lot auto-fetch flag. |
| db/migrate/20260331153000_reconfigure_bond_products_for_eod_rod.rb | Adds inflation-linked fields and normalizes subtype/defaults. |
| db/migrate/20260331133000_add_purchase_fields_to_bond_lots.rb | Adds subtype/rate fields and entry reference. |
| db/migrate/20260331120000_create_bond_lots.rb | Creates bond lots table + indexes. |
| db/migrate/20260330233052_create_bonds.rb | Creates bonds table. |
| config/schedule.yml | Schedules CPI import and bond lot settlement jobs. |
| config/routes.rb | Adds bonds/bond_lots resources and settings import endpoint. |
| config/locales/views/settings/hostings/en.yml | Adds hostings copy/alerts for GUS settings and import. |
| config/locales/views/pages/en.yml | Adds dashboard notice string and bond summary strings. |
| config/locales/views/bonds/en.yml | Adds bond/bond lot UI text, labels, and activity messages. |
| config/locales/views/accounts/en.yml | Adds “bond” account type label. |
| compose.example.yml | Adds env wiring for GUS CPI imports. |
| compose.example.ai.yml | Adds env wiring for GUS CPI imports (AI compose). |
| app/views/settings/hostings/show.html.erb | Renders new GUS SDP settings panel. |
| app/views/settings/hostings/_gus_sdp_settings.html.erb | Adds UI for enabling imports, setting API key, and triggering manual import. |
| app/views/pages/dashboard/_bond_summary.html.erb | Adds dashboard “Bonds” section summary + top lots table. |
| app/views/bonds/tabs/_positions.html.erb | Adds bond “positions” tab UI listing open lots and cash. |
| app/views/bonds/tabs/_closed.html.erb | Adds bond “closed lots” tab UI. |
| app/views/bonds/new.html.erb | Adds bond account creation modal. |
| app/views/bonds/edit.html.erb | Adds bond account edit modal. |
| app/views/bonds/_purchase_holding.html.erb | Adds open lot row partial for positions tab. |
| app/views/bonds/_form.html.erb | Adds bond account fields (initial balance, wrapper, auto-buy). |
| app/views/bonds/_closed_purchase_holding.html.erb | Adds closed lot row partial with settlement/interest/tax meta. |
| app/views/bonds/_cash_holding.html.erb | Adds bond cash position row partial. |
| app/views/bond_lots/show.html.erb | Adds drawer view with edit form (open) and history section. |
| app/views/bond_lots/new.html.erb | Adds bond lot creation modal. |
| app/views/bond_lots/edit.html.erb | Adds bond lot edit modal. |
| app/views/bond_lots/_form.html.erb | Adds bond lot form with inflation-linked conditional fields + tax/settlement controls. |
| app/views/accounts/show/_activity.html.erb | Adjusts “New activity” link to create bond lots for bond accounts. |
| app/views/accounts/new.html.erb | Adds bond as an account type option. |
| app/models/setting.rb | Adds GUS settings fields + env override helper. |
| app/models/provider/gus_sdp.rb | Implements GUS SDP CPI YoY fetch client via Faraday. |
| app/models/gus_inflation_rate.rb | Adds CPI record model with import + lookup logic. |
| app/models/entry.rb | Adds has_one :bond_lot, dependent: :destroy for cascading deletes. |
| app/models/concerns/accountable.rb | Adds Bond to Accountable types list. |
| app/models/bond.rb | Adds bond account domain model and wrapper behavior. |
| app/models/bond_lot.rb | Adds lot valuation, inflation logic, settlement automation, and backfill enqueueing. |
| app/models/balance/base_calculator.rb | Extends holdings value derivation for bond lots. |
| app/models/account.rb | Adds bond wrapper label behavior and balance_type mapping. |
| app/jobs/settle_matured_bond_lots_job.rb | Adds scheduled bond lot settlement job. |
| app/jobs/import_gus_inflation_rates_job.rb | Adds scheduled/manual CPI import job + status tracking. |
| app/javascript/controllers/bond_lot_form_controller.js | Adds Stimulus controller to toggle lot form fields by subtype + global CPI toggle. |
| app/javascript/controllers/bond_account_form_controller.js | Adds Stimulus controller to show/hide wrapper-only fields. |
| app/controllers/settings/hostings_controller.rb | Adds GUS settings persistence + manual import endpoint. |
| app/controllers/pages_controller.rb | Adds dashboard bond section locals + session-scoped rate review notice. |
| app/controllers/bonds_controller.rb | Adds Bond controller via AccountableResource. |
| app/controllers/bond_lots_controller.rb | Adds bond lot CRUD controller with entry linkage. |
| app/controllers/accounts_controller.rb | Auto-settles matured lots when viewing bond accounts. |
| app/components/UI/account/chart.rb | Treats Bond like investment accounts for chart title behavior. |
| app/components/UI/account/chart.html.erb | Enables holdings/cash view and tooltip for bond accounts. |
| app/components/UI/account/activity_feed.html.erb | Adds “New activity” menu item for bond accounts. |
| app/components/UI/account_page.rb | Adds bond-specific tabs and tab fallback behavior. |
| .env.test.example | Documents GUS env vars for tests. |
| .env.local.example | Documents GUS env vars for local dev. |
| .env.example | Documents GUS env vars for general setup. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (28)
.env.example (1)
58-62: Consider relocating this block for better organization.The GUS SDP variables are inserted between the "Provider selection for exchange rates" section (lines 53-57) and the "Alternative: Use Yahoo Finance" section (lines 64-66), which disrupts the logical grouping of data provider settings. Consider moving this block either before the provider selection section or after the Yahoo Finance alternative.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 58 - 62, The GUS SDP env vars (GUS_INFLATION_IMPORT_ENABLED and GUS_SDP_API_KEY) are placed between the "Provider selection for exchange rates" and the "Alternative: Use Yahoo Finance" sections, breaking the provider settings group; relocate the two lines so they sit either before the provider selection block or immediately after the Yahoo Finance alternative block to keep all data-provider related variables grouped together and update surrounding comments if needed to preserve readability.test/controllers/pages_controller_test.rb (1)
35-35: Consider using i18n lookup for flash message assertion.The hard-coded English string
"1 bond lot(s) need updated issue rates: US T-Bill."is fragile—if the translation changes, this test will fail even though the functionality is correct. Consider using the i18n helper to build the expected message:expected = I18n.t("pages.dashboard.bond_rate_review_notice", count: 1, accounts: "US T-Bill") assert_equal expected, flash[:notice]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/pages_controller_test.rb` at line 35, Replace the hard-coded assertion string in the test's assert_equal against flash[:notice] with an i18n lookup; build the expected value via I18n.t("pages.dashboard.bond_rate_review_notice", count: 1, accounts: "US T-Bill") and assert_equal that expected value to flash[:notice] instead of the literal "1 bond lot(s) need updated issue rates: US T-Bill."; update the assert_equal call that currently references flash[:notice] to use the expected variable derived from I18n.t.app/models/balance/base_calculator.rb (1)
19-26: Useaccount.bond?predicate for consistency.The
chart.html.erbcomponent usesaccount.bond?to check the account type. Using the predicate here instead of the string comparison would be more consistent with the codebase style and more resilient to refactoring:- `@holdings_value_for_date`[date] ||= if account.accountable_type == "Bond" + `@holdings_value_for_date`[date] ||= if account.bond?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/balance/base_calculator.rb` around lines 19 - 26, Replace the string type check in the memoized holdings calculation with the model predicate: in the block that assigns `@holdings_value_for_date`[date] (the conditional currently using account.accountable_type == "Bond"), use account.bond? instead and keep the existing branches (use account.bond.bond_lots... for bonds, else use sync_cache.get_holdings). This keeps behavior the same but uses the predicate used elsewhere (e.g., chart.html.erb) and is resilient to refactors.db/migrate/20260330233052_create_bonds.rb (1)
1-15: Consider adding indexes on frequently queried columns.The migration creates the base
bondstable correctly with appropriate decimal precisions for financial data. However, ifsubtypeorrate_typewill be used for filtering/grouping bonds in queries, consider adding indexes:t.jsonb :locked_attributes, default: {}, null: false t.timestamps end + + add_index :bonds, :subtype + add_index :bonds, :rate_type endThis can be deferred if query patterns are not yet established.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20260330233052_create_bonds.rb` around lines 1 - 15, Add indexes on frequently filtered columns in the CreateBonds migration: inside the change method where create_table :bonds is declared (or immediately after it) add indexes for the subtype and rate_type columns (e.g., add_index :bonds, :subtype and add_index :bonds, :rate_type) so queries that filter or group by those fields are fast; include names or options (using: :btree) if you need explicit index naming or a specific index type, or use t.index within the create_table block to define them inline.test/jobs/settle_matured_bond_lots_job_test.rb (1)
4-24: Consider adding one boundary case for the job scope.Please add a companion assertion that a lot with
auto_close_on_maturity: false(or not yet matured) remains open afterperform_now, so regressions in the query filter are caught early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/jobs/settle_matured_bond_lots_job_test.rb` around lines 4 - 24, Add a boundary case to the test by creating a second BondLot via BondLot.create! with the same matured criteria but auto_close_on_maturity: false (or create a lot with purchased_on such that it's not yet matured), call SettleMaturedBondLotsJob.perform_now as before, then assert that the second lot.reload.closed_on is still nil to verify non-auto-close lots (or not-yet-matured lots) are not closed by SettleMaturedBondLotsJob.app/jobs/settle_matured_bond_lots_job.rb (1)
5-8: Add/confirm an index for the settlement query pattern.This scheduled path should be backed by an index matching
open + auto_close_on_maturity + maturity_datefiltering to avoid full scans on large datasets.As per coding guidelines, “Optimize database queries with proper indexes to prevent N+1 queries using includes/joins.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/jobs/settle_matured_bond_lots_job.rb` around lines 5 - 8, The scheduled query over BondLot.open.where(auto_close_on_maturity: true).where("maturity_date <= ?", on) needs a composite index to avoid full table scans: add a migration that creates an index on the column(s) used by the BondLot.open scope (e.g. the state/status column), plus auto_close_on_maturity and maturity_date (e.g. [state, auto_close_on_maturity, maturity_date] or [status, auto_close_on_maturity, maturity_date] depending on your scope implementation), update schema, and run migrations; ensure the index name is descriptive and include a test or note in the model/README that the index supports the BondLot.open + auto_close_on_maturity + maturity_date query pattern.test/controllers/settings/hostings_controller_test.rb (1)
251-277: Add an explicit non-admin authorization test for this new endpoint.Given this action can trigger background imports, please mirror the existing admin-gate tests (
clear_cache,disconnect_external_assistant) forimport_gus_inflation_rates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/settings/hostings_controller_test.rb` around lines 251 - 277, Add a new authorization test that mirrors the existing admin-gate tests (e.g. clear_cache, disconnect_external_assistant): create a test like "non-admin cannot enqueue gus inflation import" that uses with_self_hosting but does not sign in as an admin, then POST to import_gus_inflation_rates_settings_hosting_url with the same params and assert that no ImportGusInflationRatesJob is enqueued (assert_no_enqueued_jobs only: ImportGusInflationRatesJob) and that the request is rejected per your app policy (match the existing admin-gate pattern: assert_redirected_to and flash alert or assert_response :forbidden depending on how clear_cache/disconnect_external_assistant assert authorization).db/migrate/20260331170000_add_gus_inflation_rates_and_auto_fetch_to_bond_lots.rb (1)
14-14: Consider default value for existing bond lots.The
auto_fetch_inflationcolumn defaults totrue, meaning all existing bond lots will automatically opt-in to GUS inflation fetching after migration. If there are existing bond lots that were manually managed, this could cause unexpected behavior changes.If this is intentional (making the new feature opt-out rather than opt-in), this is fine. Otherwise, consider defaulting to
falseto preserve existing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20260331170000_add_gus_inflation_rates_and_auto_fetch_to_bond_lots.rb` at line 14, The migration currently adds auto_fetch_inflation to :bond_lots with default: true which opts all existing records into automatic GUS inflation fetching; change the migration to use default: false (add_column :bond_lots, :auto_fetch_inflation, :boolean, null: false, default: false) so existing bond lots preserve prior behavior, and if you need selective opt-in, add an explicit data backfill/update step after the column add to set true only for records that should auto-fetch. Ensure you update any tests or seed logic that assume the new default.app/jobs/import_gus_inflation_rates_job.rb (1)
5-5: Consider usinggus_inflation_import_enabled_effectivefor consistency.The
Settingmodel definesgus_inflation_import_enabled_effective(lines 69-73 insetting.rb) which properly handles the ENV override. Using the base field here could cause inconsistent behavior if the ENV var is set differently than the persisted setting.Proposed fix
def perform(opts = {}) - return unless Setting.gus_inflation_import_enabled + return unless Setting.gus_inflation_import_enabled_effective🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/jobs/import_gus_inflation_rates_job.rb` at line 5, Replace the direct Setting.gus_inflation_import_enabled check in ImportGusInflationRatesJob with the effective accessor Setting.gus_inflation_import_enabled_effective so the job honors the ENV override; update the guard (the line currently using gus_inflation_import_enabled) to call gus_inflation_import_enabled_effective instead.app/views/bonds/_purchase_holding.html.erb (1)
35-35: Nested ternary reduces readability.The triple-nested ternary on line 35 is difficult to parse at a glance.
Proposed refactor using explicit conditionals
- <% rate_text = lot.requires_rate_review? ? t(".update_needed") : current_rate.present? ? number_to_percentage(current_rate, precision: 3) : t(".unknown") %> + <% rate_text = if lot.requires_rate_review? + t(".update_needed") + elsif current_rate.present? + number_to_percentage(current_rate, precision: 3) + else + t(".unknown") + end %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/bonds/_purchase_holding.html.erb` at line 35, Replace the nested ternary that assigns rate_text with a clear conditional block: first check lot.requires_rate_review? and set rate_text to t(".update_needed"); elsif check current_rate.present? and set rate_text to number_to_percentage(current_rate, precision: 3); else set rate_text to t(".unknown"). Update the expression around the variable rate_text (referencing lot.requires_rate_review?, current_rate, number_to_percentage, t(".update_needed"), and t(".unknown")) to use if/elsif/else for readability.app/views/bond_lots/_form.html.erb (2)
9-9: Consider usinggus_inflation_import_enabled_effectivefor consistency.The form uses
Setting.gus_inflation_import_enabledin multiple places (lines 9, 100, 109, 123), but the model providesgus_inflation_import_enabled_effectivewhich properly handles ENV override. If the ENV var is set differently than the persisted value, this could cause inconsistent behavior.Proposed fix (example for line 9)
- bond_lot_form_global_auto_fetch_enabled_value: Setting.gus_inflation_import_enabled + bond_lot_form_global_auto_fetch_enabled_value: Setting.gus_inflation_import_enabled_effectiveApply similar changes to lines 100, 109, and 123.
Also applies to: 100-100, 109-109, 123-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/bond_lots/_form.html.erb` at line 9, Replace direct calls to Setting.gus_inflation_import_enabled with the ENV-aware accessor Setting.gus_inflation_import_enabled_effective in the bond_lot form partial; specifically update the occurrences referenced (the bond_lot_form_global_auto_fetch_enabled_value and the other usages at lines noted) so the form uses gus_inflation_import_enabled_effective to reflect ENV overrides and avoid inconsistent behavior between persisted and env-provided values.
74-84: Inconsistent indentation in rate_type/coupon_frequency section.The
<div>wrapper on line 74 and closing on line 84 use different indentation than surrounding elements (extra 8 spaces).Proposed fix
- <div data-bond-lot-form-target="otherFields"> + <div data-bond-lot-form-target="otherFields"> <%= form.select :rate_type, Bond::RATE_TYPES.map { |value| [ value.titleize, value ] }, { label: t("bond_lots.form.rate_type"), required: true }, { data: { bond_lot_form_target: "otherRequiredInput" } } %> <%= form.select :coupon_frequency, Bond::COUPON_FREQUENCIES.map { |value| [ value.humanize, value ] }, { label: t("bond_lots.form.coupon_frequency"), required: true }, { data: { bond_lot_form_target: "otherRequiredInput" } } %> - </div> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/bond_lots/_form.html.erb` around lines 74 - 84, The indentation for the div wrapping the rate_type and coupon_frequency selects is inconsistent with surrounding markup; adjust the leading spaces so the <div data-bond-lot-form-target="otherFields"> and its closing tag align with neighbouring elements, and ensure the nested lines for form.select calls (for :rate_type using Bond::RATE_TYPES and for :coupon_frequency using Bond::COUPON_FREQUENCIES) are indented one level deeper to match the file's indentation style.app/views/bond_lots/show.html.erb (2)
90-90: Use design token instead of hardcoded color.
text-red-500should be replaced with the functional tokentext-destructiveper project design system conventions.As per coding guidelines: "Use Tailwind design tokens (e.g., text-primary, bg-container, border-secondary) instead of raw color names."
Proposed fix
- class: "rounded-lg px-3 py-2 text-red-500 text-sm font-medium border border-secondary", + class: "rounded-lg px-3 py-2 text-destructive text-sm font-medium border border-secondary",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/bond_lots/show.html.erb` at line 90, In the view template change the hardcoded Tailwind color class on the element that currently has the class string "rounded-lg px-3 py-2 text-red-500 text-sm font-medium border border-secondary" by replacing text-red-500 with the design token text-destructive so the class becomes "rounded-lg px-3 py-2 text-destructive text-sm font-medium border border-secondary"; ensure no other hardcoded color variants remain in the same element.
46-47: Useformat_moneyhelper for consistency.Lines 46-47 use
Money.new(...).formatdirectly, while line 20-21 useformat_money(). The helper should be used consistently throughout for uniform formatting behavior.Proposed fix
- <p class="text-primary"><%= t(".history_balance", opening: Money.new(event[:opening_balance], `@account.currency`).format, closing: Money.new(event[:closing_balance], `@account.currency`).format) %></p> - <p class="text-secondary"><%= t(".history_interest", interest: Money.new(event[:interest_earned], `@account.currency`).format, rate: number_to_percentage(event[:annual_rate_percent], precision: 3)) %></p> + <p class="text-primary"><%= t(".history_balance", opening: format_money(Money.new(event[:opening_balance], `@account.currency`)), closing: format_money(Money.new(event[:closing_balance], `@account.currency`))) %></p> + <p class="text-secondary"><%= t(".history_interest", interest: format_money(Money.new(event[:interest_earned], `@account.currency`)), rate: number_to_percentage(event[:annual_rate_percent], precision: 3)) %></p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/bond_lots/show.html.erb` around lines 46 - 47, Replace the direct Money.new(...).format calls with the existing format_money helper for consistency: update the arguments passed to the translation helpers t(".history_balance", ...) and t(".history_interest", ...) so opening: and closing: use format_money(event[:opening_balance], `@account.currency`) and format_money(event[:closing_balance], `@account.currency`) respectively, and interest: uses format_money(event[:interest_earned], `@account.currency`); keep number_to_percentage(event[:annual_rate_percent], precision: 3) unchanged.app/controllers/bond_lots_controller.rb (2)
18-26: Inconsistent early return pattern ineditandshow.Lines 20 and 25 use
nil unless require_account_permission!which is unconventional. Thenewaction (line 6) and other actions usereturn unless, which is clearer and consistent with Rails conventions.def edit `@account` = `@bond_lot.account` - nil unless require_account_permission!(`@account`) + return unless require_account_permission!(`@account`) end def show `@account` = `@bond_lot.account` - nil unless require_account_permission!(`@account`) + return unless require_account_permission!(`@account`) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/bond_lots_controller.rb` around lines 18 - 26, The edit and show actions use the unconventional "nil unless require_account_permission!(`@account`)" pattern; replace those lines with explicit early returns matching the existing convention by calling require_account_permission!(`@account`) and returning immediately on failure (e.g., "return unless require_account_permission!(`@account`)") after assigning `@account` = `@bond_lot.account` so edit and show mirror the new action and other controllers and maintain consistent control flow.
34-38: Consider combining save and update into a single operation.The
@bond_lot.save!followed immediately by@bond_lot.update!(entry: ...)within the same transaction results in two database writes. This could be simplified.♻️ Proposed refactor
if `@bond_lot.valid`? ActiveRecord::Base.transaction do + entry = create_purchase_entry!(`@account`, `@bond_lot`) + `@bond_lot.entry` = entry `@bond_lot.save`! - `@bond_lot.update`!(entry: create_purchase_entry!(`@account`, `@bond_lot`)) endNote: This requires adjusting
create_purchase_entry!to not referencebond_lot.idif it's not yet persisted, or using a different approach for theextradata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/bond_lots_controller.rb` around lines 34 - 38, The two writes (save! then update!) on `@bond_lot` should be combined into a single persistence operation: call create_purchase_entry! in a way that does not require bond_lot.id (or change it to accept the in-memory `@bond_lot`), assign the resulting entry to `@bond_lot.entry` (or build the association) and then call `@bond_lot.save`! inside the ActiveRecord::Base.transaction so only one DB write occurs; update the create_purchase_entry! signature/implementation to use the bond_lot object (or its attributes) instead of relying on a persisted id if necessary.db/schema.rb (1)
1602-1603: Consider addingon_deletebehavior forbond_lots.entry_idforeign key.The foreign key from
bond_lotstoentriesdoesn't specifyon_deletebehavior. If an entry is deleted independently (entries haveon_delete: :cascadefrom accounts), associatedbond_lotswould have danglingentry_idreferences.Consider whether
on_delete: :nullify(allow orphan lots) oron_delete: :cascade(delete lot with entry) better matches the domain model. Based on the controller logic where entries and lots are deleted together in a transaction,nullifymay be appropriate as a safety net.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/schema.rb` around lines 1602 - 1603, The foreign key from bond_lots to entries currently lacks on_delete behavior; update the constraint for bond_lots.entry_id (the add_foreign_key call referencing "entries") to include an appropriate on_delete option—preferably on_delete: :nullify (or :cascade if you decide deletion semantics require removing lots with their entry)—so that deleting an Entry won't leave dangling entry_id references; locate the add_foreign_key "bond_lots", "entries" invocation and change it to include the chosen on_delete behavior (and add a migration to alter the foreign key if this is already in production schema).app/views/pages/dashboard/_bond_summary.html.erb (2)
3-5: Potential N+1 query when collecting open lots.The
flat_mapiterates over each account and callsaccount.bond.bond_lots.open, which may trigger separate queries per account. Consider eager loading or refactoring to a single query.-<% open_lots = bond_accounts.flat_map { |account| account.bond.bond_lots.open.map { |lot| [ account, lot ] } } %> +<% open_lots = bond_accounts.includes(bond: :bond_lots).flat_map { |account| account.bond.bond_lots.open.map { |lot| [ account, lot ] } } %>Alternatively, ensure the controller preloads
bond: { bond_lots: ... }when fetching@bond_accounts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/pages/dashboard/_bond_summary.html.erb` around lines 3 - 5, open_lots is built with bond_accounts.flat_map calling account.bond.bond_lots.open which may trigger an N+1; fix it by loading bond_lots in a single query or eager-loading them when fetching bond_accounts. Update the controller where `@bond_accounts` is set to include/ preload the associations (bond and bond_lots with the open scope) so that view code using open_lots, bond_accounts, bond.bond_lots.open does not fire per-account queries; alternatively, replace the view aggregation with a single query (e.g., query BondLot.open joined to bonds/accounts) and assign that to open_lots so the view only iterates over already-loaded records.
40-66: Heavy logic in ERB template violates coding guidelines.Lines 40–66 contain substantial conditional logic for determining
label,total_return_amount,total_return_label,rate_text, andrate_meta. As per coding guidelines, avoid heavy logic in ERB view files and prefer helpers or presenters. Consider extracting this into a helper method or a decorator/presenter object.♻️ Example refactor to helper
# app/helpers/bond_lots_helper.rb module BondLotsHelper def bond_lot_display_data(lot) { label: Bond.long_subtype_label_for(lot.subtype) || t("bonds.purchase_holding.unknown"), rate_text: compute_rate_text(lot), rate_meta: compute_rate_meta(lot), total_return: compute_total_return_display(lot) } end private def compute_rate_text(lot) # move lines 49-59 logic here end # ... endThen in the partial:
<% display = bond_lot_display_data(lot) %> <%= display[:label] %>As per coding guidelines: "Avoid heavy logic in ERB view files; prefer helpers and components instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/pages/dashboard/_bond_summary.html.erb` around lines 40 - 66, The ERB partial contains heavy presentation logic (in the top_lots loop using Bond.long_subtype_label_for and computing total_return_amount/total_return_label, rate_text, and rate_meta) that should be moved into a helper or presenter; create a helper method (e.g. bond_lot_display_data(lot)) that returns a hash with keys label, rate_text, rate_meta, and total_return (or separate private methods like compute_rate_text(lot), compute_rate_meta(lot), compute_total_return_display(lot)), move the conditional logic from the template into those helper methods, and then replace the in-template logic with a single call to bond_lot_display_data(lot) inside the partial while keeping references to lot.inflation_linked?, lot.requires_rate_review?, lot.current_rate_percent, lot.interest_rate, lot.current_inflation_component_percent, lot.current_margin_percent, lot.rate_type, and lot.coupon_frequency in the helper.app/models/bond.rb (1)
45-53: Consider combining existence check and sum into a single query.
original_balancefirst checksbond_lots.exists?then callsbond_lots.sum(:amount), resulting in two database queries.sumreturns0for empty relations, so the existence check may be unnecessary unless the fallback tofirst_valuation_amountis specifically for accounts without any lots.If the intent is "use lots sum if any lots exist, otherwise fallback", consider:
def original_balance - principal_amount = if bond_lots.exists? - bond_lots.sum(:amount) - else - account.first_valuation_amount - end + lots_sum = bond_lots.sum(:amount) + principal_amount = lots_sum.positive? ? lots_sum : account.first_valuation_amount Money.new(principal_amount, account.currency) endHowever, if a lot with
amount: 0is valid, the current logic is more correct. Verify the intended behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/bond.rb` around lines 45 - 53, The method original_balance currently does two DB queries (bond_lots.exists? and bond_lots.sum(:amount)); replace them by calling total = bond_lots.sum(:amount) once and use that as principal_amount, falling back to account.first_valuation_amount only when total == 0 if that matches intended behavior (i.e., change logic in original_balance to compute principal_amount = bond_lots.sum(:amount); principal_amount = account.first_valuation_amount if principal_amount.zero?); if a lot with amount: 0 is a valid case and should prefer the lots presence, keep the existing exists? check instead—verify which behavior is desired.app/models/bond_lot.rb (5)
403-403: Redundant ternary expression.
inflation_linked? ? nil : nilalways evaluates tonil. This appears to be a copy-paste artifact.♻️ Proposed fix
- interest_rate: inflation_linked? ? nil : nil, + interest_rate: nil,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/bond_lot.rb` at line 403, The line setting interest_rate uses a redundant ternary `inflation_linked? ? nil : nil`; update the BondLot model to assign interest_rate directly to nil and remove the ternary. Locate the attribute/hash key `interest_rate:` in app/models/bond_lot.rb (and the `inflation_linked?` predicate nearby) and replace the ternary expression with a simple `nil` value to eliminate the dead code.
474-476: Side-effect-heavy return is confusing.Line 475 combines assignment and return in a way that obscures intent. The return value is unused (this is a callback), and the assignment is the actual side effect.
♻️ Proposed clarity improvement
def normalize_tax_settings - return self.tax_rate = 0 if apply_tax_exempt_wrapper! + if apply_tax_exempt_wrapper! + self.tax_rate = 0 + return + end self.tax_strategy = "standard" if tax_strategy.blank?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/bond_lot.rb` around lines 474 - 476, In normalize_tax_settings, avoid the side-effect-heavy inline return/assignment; instead, perform the assignment as a clear statement when apply_tax_exempt_wrapper! is true (e.g., set self.tax_rate = 0 in an if block) and do not rely on returning that assignment value since the callback ignores the return—update normalize_tax_settings to separate the predicate check (apply_tax_exempt_wrapper!) from the side-effect (setting self.tax_rate) for clarity.
187-240: Consider extracting shared loop logic.The
whileloop incapitalization_historyclosely mirrorsestimated_current_value. While the outputs differ, the period iteration logic could potentially be extracted into a shared iterator method to reduce duplication. This is a minor observation for future maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/bond_lot.rb` around lines 187 - 240, The capitalization_history method duplicates period iteration logic that's also used in estimated_current_value; extract that shared loop into a single iterator method (e.g., iterate_periods or with_capitalization_periods) that accepts start (purchased_on), end (history_end or maturity/now), and yields per-period data (period_number, start_on, end_on, days_in_step, days_in_year, full_year_capitalization, rate_context, opening_balance) so both capitalization_history and estimated_current_value can call it and build their respective events/results using the yielded values; update capitalization_history to call the new iterator and compute interest_earned/closing_balance from the yielded values and remove duplicated loop code.
523-530: Auto-buy is restricted to tax-exempt inflation-linked bonds.The
should_auto_buy_new_issue?method requires bothtax_exempt_wrapper?andinflation_linked?. If this is intentional business logic (e.g., specific to Polish IKE/IKZE accounts with EOD/ROD bonds), consider adding a brief comment explaining the restriction for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/bond_lot.rb` around lines 523 - 530, The method should_auto_buy_new_issue? currently gates auto-buy behind bond&.auto_buy_new_issues?, bond&.tax_exempt_wrapper?, and inflation_linked? which is a deliberate business restriction; add a brief explanatory comment above should_auto_buy_new_issue? stating that auto-buy is limited to tax-exempt, inflation-linked bonds (e.g., Polish IKE/IKZE with EOD/ROD bonds) so future maintainers understand why bond&.tax_exempt_wrapper? and inflation_linked? are both required, and keep the existing checks (referencing nominal_per_unit and the floor calculation) unchanged.
94-94: Simplify days-in-year calculation.The expression
(cursor + 1.year - cursor).to_iis a convoluted way to determine leap year days. Consider using a clearer approach.♻️ Proposed simplification
- days_in_year = [ (cursor + 1.year - cursor).to_i, 1 ].max + days_in_year = Date.leap?(cursor.year) ? 366 : 365🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/bond_lot.rb` at line 94, Replace the convoluted days_in_year calculation with a clear leap-year check: use Date.leap?(cursor.year) to set days_in_year to 366 for leap years and 365 otherwise (i.e. days_in_year = Date.leap?(cursor.year) ? 366 : 365), updating the expression around the days_in_year variable that currently uses (cursor + 1.year - cursor).to_i.app/models/gus_inflation_rate.rb (3)
81-84: Redundant.presencecall.
indicator_idis already assigned with.presenceon line 82, so the second.presenceon line 83 is unnecessary.♻️ Proposed simplification
def cpi_indicator_id - indicator_id = ENV["GUS_SDP_CPI_INDICATOR_ID"].presence - indicator_id.presence || Provider::GusSdp::DEFAULT_CPI_INDICATOR_ID + ENV["GUS_SDP_CPI_INDICATOR_ID"].presence || Provider::GusSdp::DEFAULT_CPI_INDICATOR_ID end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/gus_inflation_rate.rb` around lines 81 - 84, The cpi_indicator_id method uses .presence twice; remove the redundant call by assigning indicator_id = ENV["GUS_SDP_CPI_INDICATOR_ID"].presence and then returning indicator_id || Provider::GusSdp::DEFAULT_CPI_INDICATOR_ID (i.e., drop the second .presence). Update the cpi_indicator_id method to return either the present ENV value or Provider::GusSdp::DEFAULT_CPI_INDICATOR_ID directly.
104-108: PostgreSQL-specificmake_datefunction.The query uses PostgreSQL's
make_datefunction, which ties this model to PostgreSQL. This is acceptable if PostgreSQL is the only supported database, but worth noting for portability awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/gus_inflation_rate.rb` around lines 104 - 108, The method latest_before_or_on uses PostgreSQL's make_date(year, month, 1) making the query DB-specific; change latest_before_or_on to avoid make_date by comparing year and month numerically: compute target_year = target_date.year and target_month = target_date.month and replace the where clause with a portable condition like "year < ? OR (year = ? AND month <= ?)" (binding target_year, target_year, target_month), keep the same order(year: :desc, month: :desc). This removes the make_date dependency while preserving behavior.
86-98: Refactor error detection to use structured error types instead of string matching.The error detection methods in
not_found_error?andrate_limited_error?rely on substring matching againsterror.message, which is fragile. The project already uses a more robust pattern elsewhere—seeProvider::YahooFinancewhich defines error subclasses (RateLimitError,AuthenticationError) and overridesdefault_error_transformerto detect specific Faraday error types.Apply the same pattern here: Have
Provider::GusSdpdefineNotFoundErrorandRateLimitErrorsubclasses, overridedefault_error_transformerto detectFaraday::NotFoundErrorandFaraday::TooManyRequestsError, then updateGusInflationRateto checkerror.is_a?(Provider::GusSdp::RateLimitError)instead of parsing error messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/gus_inflation_rate.rb` around lines 86 - 98, Replace fragile string-matching in GusInflationRate#not_found_error? and `#rate_limited_error`? with structured error type checks: add Provider::GusSdp::NotFoundError and Provider::GusSdp::RateLimitError classes, implement Provider::GusSdp.default_error_transformer to map Faraday::NotFoundError and Faraday::TooManyRequestsError to those subclasses, then update GusInflationRate to detect errors via error.is_a?(Provider::GusSdp::NotFoundError) and error.is_a?(Provider::GusSdp::RateLimitError) instead of checking error.message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/accounts_controller.rb`:
- Line 45: Wrap the call to `@account.bond`&.settle_matured_lots! (guarded by
`@account.bond`?) in a rescue block so any exception during settlement is caught
and does not bubble up to the show action; log the exception with context (e.g.
logger.error or Rails.logger.error including `@account.id` and the
exception.message/backtrace) and optionally report to the app's error-tracking
(Sentry/Raven) instead of re-raising, then continue rendering the page.
In `@app/models/bond.rb`:
- Around line 96-98: The inflation_linked? method calls
subtype.in?(INFLATION_LINKED_SUBTYPES) but subtype can be nil, causing a
NoMethodError; update inflation_linked? to guard against nil (e.g., use safe
navigation or an explicit presence check) before calling in? or switch to
calling INFLATION_LINKED_SUBTYPES.include?(subtype) so the method safely returns
false when subtype is nil; ensure the change is applied in the inflation_linked?
method and references the existing INFLATION_LINKED_SUBTYPES constant.
In `@app/models/gus_inflation_rate.rb`:
- Around line 73-74: The provider method currently memoizes `@provider` and can
hold stale credentials when Setting.gus_sdp_api_key or cpi_indicator_id change;
update the provider method (provider) to avoid class-level memoization by either
always instantiating a new Provider::GusSdp with current api_key and
cpi_indicator_id or implement a cache keyed by "#{api_key}-#{cpi_indicator_id}"
(e.g., store in a hash like provider_cache[cache_key]) so the
Provider::GusSdp.new(api_key: api_key, cpi_indicator_id: cpi_indicator_id) is
recreated when credentials change; remove the use of `@provider` as a long-lived
cache unless tied to those keys.
In `@app/models/provider/gus_sdp.rb`:
- Around line 22-30: The JSON parsing block that builds rows from
JSON.parse(response.body) is brittle: replace the current logic around parsed
and rows so it defensively handles nil, non-hash/non-array types, and hashes
missing "data"; specifically, after parsed = JSON.parse(response.body) ensure
parsed is an Array (use it), or if it's a Hash fetch "data" only when present
and an Array, otherwise fall back to an empty Array before calling rows.map;
update the code around JSON.parse(response.body), rows, and the rows.map block
to use this safe fallback.
- Around line 37-49: The Faraday connection in the client method lacks request
timeouts; update the Faraday.new block in method client to set reasonable open
and read/request timeouts (e.g. open_timeout and timeout) so requests to
base_url cannot hang indefinitely. Locate the Faraday.new initialization inside
client and add the request timeout options (or use faraday.request :timeout
settings) alongside the existing retry and raise_error configuration, ensuring
api_key header behavior remains unchanged.
In `@app/views/bonds/_form.html.erb`:
- Line 28: The toggle helper is calling account.accountable.auto_buy_new_issues?
which can raise NoMethodError when account.accountable is nil; update the call
in the bond_form toggle (the :auto_buy_new_issues field) to safely handle nil
(e.g., use safe navigation or a default boolean) so the helper receives a
boolean even for new accounts—ensure the expression around
account.accountable.auto_buy_new_issues? is replaced with a nil-safe
alternative.
In `@config/locales/views/pages/en.yml`:
- Line 18: The translation key bond_rate_review_notice currently uses "bond
lot(s)"—replace it with Rails i18n pluralization by changing
bond_rate_review_notice into a hash with count variants (one and other) and
interpolate %{count} and %{accounts} (e.g., bond_rate_review_notice.one and
bond_rate_review_notice.other) so the UI shows "bond lot" for count 1 and "bond
lots" otherwise; update any callers that pass a count to ensure they use
I18n.t('bond_rate_review_notice', count: n, accounts: accounts).
In `@db/migrate/20260331120000_create_bond_lots.rb`:
- Around line 3-12: Add explicit PostgreSQL CHECK constraints to the bond_lots
table to enforce domain invariants: ensure amount > 0, term_months > 0,
maturity_date >= purchased_on, and interest_rate IS NULL OR interest_rate >= 0
(or a sensible upper bound if desired). Update the migration that defines
create_table :bond_lots (the block that sets t.references :bond, t.date
:purchased_on, t.decimal :amount, t.integer :term_months, t.date :maturity_date,
t.decimal :interest_rate) to add database-level checks (using
add_check_constraint or SQL CHECK clauses) for these conditions so invalid rows
cannot be inserted even if application validations are bypassed.
In `@db/migrate/20260331133000_add_purchase_fields_to_bond_lots.rb`:
- Around line 3-5: The three new columns on bond_lots (subtype, rate_type,
coupon_frequency) must be non-null at the DB level; modify the migration to add
them with null: false and a safe default (e.g., add_column :bond_lots, :subtype,
:string, null: false, default: '' and likewise for :rate_type and
:coupon_frequency), or implement the two-step pattern (add nullable columns,
backfill existing rows, then change_column_null to enforce null: false) so
existing data isn't broken. Ensure you reference the bond_lots table and the
three column names when making the change.
In `@db/migrate/20260331153000_reconfigure_bond_products_for_eod_rod.rb`:
- Around line 75-85: The down method currently overwrites all subtype values via
the two execute blocks (updating bonds and bond_lots), which irreversibly loses
original values; to fix, remove those destructive UPDATEs in the down method and
instead raise ActiveRecord::IrreversibleMigration (i.e., replace the execute
<<~SQL blocks in the down method with raise ActiveRecord::IrreversibleMigration)
so rollbacks are explicitly prevented and signal the migration is not
reversible; alternatively, if you prefer reversible behavior implement a backup
in up (add original_subtype columns and copy subtype from bonds and bond_lots)
and then restore those values and drop the backup columns in down.
In `@test/controllers/settings/hostings_controller_test.rb`:
- Line 253: The tests set the global flag Setting.gus_inflation_import_enabled
and do not restore it, causing state bleed; update the test(s) in
hostings_controller_test.rb to capture the original value before mutating
Setting.gus_inflation_import_enabled and ensure it is restored after the test
(use an ensure block, a teardown hook, or an around wrapper) so the original
value is always reassigned regardless of test outcome; apply the same restore
pattern for every place where Setting.gus_inflation_import_enabled is mutated
(the occurrences around the current test and the other mutation noted).
---
Nitpick comments:
In @.env.example:
- Around line 58-62: The GUS SDP env vars (GUS_INFLATION_IMPORT_ENABLED and
GUS_SDP_API_KEY) are placed between the "Provider selection for exchange rates"
and the "Alternative: Use Yahoo Finance" sections, breaking the provider
settings group; relocate the two lines so they sit either before the provider
selection block or immediately after the Yahoo Finance alternative block to keep
all data-provider related variables grouped together and update surrounding
comments if needed to preserve readability.
In `@app/controllers/bond_lots_controller.rb`:
- Around line 18-26: The edit and show actions use the unconventional "nil
unless require_account_permission!(`@account`)" pattern; replace those lines with
explicit early returns matching the existing convention by calling
require_account_permission!(`@account`) and returning immediately on failure
(e.g., "return unless require_account_permission!(`@account`)") after assigning
`@account` = `@bond_lot.account` so edit and show mirror the new action and other
controllers and maintain consistent control flow.
- Around line 34-38: The two writes (save! then update!) on `@bond_lot` should be
combined into a single persistence operation: call create_purchase_entry! in a
way that does not require bond_lot.id (or change it to accept the in-memory
`@bond_lot`), assign the resulting entry to `@bond_lot.entry` (or build the
association) and then call `@bond_lot.save`! inside the
ActiveRecord::Base.transaction so only one DB write occurs; update the
create_purchase_entry! signature/implementation to use the bond_lot object (or
its attributes) instead of relying on a persisted id if necessary.
In `@app/jobs/import_gus_inflation_rates_job.rb`:
- Line 5: Replace the direct Setting.gus_inflation_import_enabled check in
ImportGusInflationRatesJob with the effective accessor
Setting.gus_inflation_import_enabled_effective so the job honors the ENV
override; update the guard (the line currently using
gus_inflation_import_enabled) to call gus_inflation_import_enabled_effective
instead.
In `@app/jobs/settle_matured_bond_lots_job.rb`:
- Around line 5-8: The scheduled query over
BondLot.open.where(auto_close_on_maturity: true).where("maturity_date <= ?", on)
needs a composite index to avoid full table scans: add a migration that creates
an index on the column(s) used by the BondLot.open scope (e.g. the state/status
column), plus auto_close_on_maturity and maturity_date (e.g. [state,
auto_close_on_maturity, maturity_date] or [status, auto_close_on_maturity,
maturity_date] depending on your scope implementation), update schema, and run
migrations; ensure the index name is descriptive and include a test or note in
the model/README that the index supports the BondLot.open +
auto_close_on_maturity + maturity_date query pattern.
In `@app/models/balance/base_calculator.rb`:
- Around line 19-26: Replace the string type check in the memoized holdings
calculation with the model predicate: in the block that assigns
`@holdings_value_for_date`[date] (the conditional currently using
account.accountable_type == "Bond"), use account.bond? instead and keep the
existing branches (use account.bond.bond_lots... for bonds, else use
sync_cache.get_holdings). This keeps behavior the same but uses the predicate
used elsewhere (e.g., chart.html.erb) and is resilient to refactors.
In `@app/models/bond_lot.rb`:
- Line 403: The line setting interest_rate uses a redundant ternary
`inflation_linked? ? nil : nil`; update the BondLot model to assign
interest_rate directly to nil and remove the ternary. Locate the attribute/hash
key `interest_rate:` in app/models/bond_lot.rb (and the `inflation_linked?`
predicate nearby) and replace the ternary expression with a simple `nil` value
to eliminate the dead code.
- Around line 474-476: In normalize_tax_settings, avoid the side-effect-heavy
inline return/assignment; instead, perform the assignment as a clear statement
when apply_tax_exempt_wrapper! is true (e.g., set self.tax_rate = 0 in an if
block) and do not rely on returning that assignment value since the callback
ignores the return—update normalize_tax_settings to separate the predicate check
(apply_tax_exempt_wrapper!) from the side-effect (setting self.tax_rate) for
clarity.
- Around line 187-240: The capitalization_history method duplicates period
iteration logic that's also used in estimated_current_value; extract that shared
loop into a single iterator method (e.g., iterate_periods or
with_capitalization_periods) that accepts start (purchased_on), end (history_end
or maturity/now), and yields per-period data (period_number, start_on, end_on,
days_in_step, days_in_year, full_year_capitalization, rate_context,
opening_balance) so both capitalization_history and estimated_current_value can
call it and build their respective events/results using the yielded values;
update capitalization_history to call the new iterator and compute
interest_earned/closing_balance from the yielded values and remove duplicated
loop code.
- Around line 523-530: The method should_auto_buy_new_issue? currently gates
auto-buy behind bond&.auto_buy_new_issues?, bond&.tax_exempt_wrapper?, and
inflation_linked? which is a deliberate business restriction; add a brief
explanatory comment above should_auto_buy_new_issue? stating that auto-buy is
limited to tax-exempt, inflation-linked bonds (e.g., Polish IKE/IKZE with
EOD/ROD bonds) so future maintainers understand why bond&.tax_exempt_wrapper?
and inflation_linked? are both required, and keep the existing checks
(referencing nominal_per_unit and the floor calculation) unchanged.
- Line 94: Replace the convoluted days_in_year calculation with a clear
leap-year check: use Date.leap?(cursor.year) to set days_in_year to 366 for leap
years and 365 otherwise (i.e. days_in_year = Date.leap?(cursor.year) ? 366 :
365), updating the expression around the days_in_year variable that currently
uses (cursor + 1.year - cursor).to_i.
In `@app/models/bond.rb`:
- Around line 45-53: The method original_balance currently does two DB queries
(bond_lots.exists? and bond_lots.sum(:amount)); replace them by calling total =
bond_lots.sum(:amount) once and use that as principal_amount, falling back to
account.first_valuation_amount only when total == 0 if that matches intended
behavior (i.e., change logic in original_balance to compute principal_amount =
bond_lots.sum(:amount); principal_amount = account.first_valuation_amount if
principal_amount.zero?); if a lot with amount: 0 is a valid case and should
prefer the lots presence, keep the existing exists? check instead—verify which
behavior is desired.
In `@app/models/gus_inflation_rate.rb`:
- Around line 81-84: The cpi_indicator_id method uses .presence twice; remove
the redundant call by assigning indicator_id =
ENV["GUS_SDP_CPI_INDICATOR_ID"].presence and then returning indicator_id ||
Provider::GusSdp::DEFAULT_CPI_INDICATOR_ID (i.e., drop the second .presence).
Update the cpi_indicator_id method to return either the present ENV value or
Provider::GusSdp::DEFAULT_CPI_INDICATOR_ID directly.
- Around line 104-108: The method latest_before_or_on uses PostgreSQL's
make_date(year, month, 1) making the query DB-specific; change
latest_before_or_on to avoid make_date by comparing year and month numerically:
compute target_year = target_date.year and target_month = target_date.month and
replace the where clause with a portable condition like "year < ? OR (year = ?
AND month <= ?)" (binding target_year, target_year, target_month), keep the same
order(year: :desc, month: :desc). This removes the make_date dependency while
preserving behavior.
- Around line 86-98: Replace fragile string-matching in
GusInflationRate#not_found_error? and `#rate_limited_error`? with structured error
type checks: add Provider::GusSdp::NotFoundError and
Provider::GusSdp::RateLimitError classes, implement
Provider::GusSdp.default_error_transformer to map Faraday::NotFoundError and
Faraday::TooManyRequestsError to those subclasses, then update GusInflationRate
to detect errors via error.is_a?(Provider::GusSdp::NotFoundError) and
error.is_a?(Provider::GusSdp::RateLimitError) instead of checking error.message.
In `@app/views/bond_lots/_form.html.erb`:
- Line 9: Replace direct calls to Setting.gus_inflation_import_enabled with the
ENV-aware accessor Setting.gus_inflation_import_enabled_effective in the
bond_lot form partial; specifically update the occurrences referenced (the
bond_lot_form_global_auto_fetch_enabled_value and the other usages at lines
noted) so the form uses gus_inflation_import_enabled_effective to reflect ENV
overrides and avoid inconsistent behavior between persisted and env-provided
values.
- Around line 74-84: The indentation for the div wrapping the rate_type and
coupon_frequency selects is inconsistent with surrounding markup; adjust the
leading spaces so the <div data-bond-lot-form-target="otherFields"> and its
closing tag align with neighbouring elements, and ensure the nested lines for
form.select calls (for :rate_type using Bond::RATE_TYPES and for
:coupon_frequency using Bond::COUPON_FREQUENCIES) are indented one level deeper
to match the file's indentation style.
In `@app/views/bond_lots/show.html.erb`:
- Line 90: In the view template change the hardcoded Tailwind color class on the
element that currently has the class string "rounded-lg px-3 py-2 text-red-500
text-sm font-medium border border-secondary" by replacing text-red-500 with the
design token text-destructive so the class becomes "rounded-lg px-3 py-2
text-destructive text-sm font-medium border border-secondary"; ensure no other
hardcoded color variants remain in the same element.
- Around line 46-47: Replace the direct Money.new(...).format calls with the
existing format_money helper for consistency: update the arguments passed to the
translation helpers t(".history_balance", ...) and t(".history_interest", ...)
so opening: and closing: use format_money(event[:opening_balance],
`@account.currency`) and format_money(event[:closing_balance], `@account.currency`)
respectively, and interest: uses format_money(event[:interest_earned],
`@account.currency`); keep number_to_percentage(event[:annual_rate_percent],
precision: 3) unchanged.
In `@app/views/bonds/_purchase_holding.html.erb`:
- Line 35: Replace the nested ternary that assigns rate_text with a clear
conditional block: first check lot.requires_rate_review? and set rate_text to
t(".update_needed"); elsif check current_rate.present? and set rate_text to
number_to_percentage(current_rate, precision: 3); else set rate_text to
t(".unknown"). Update the expression around the variable rate_text (referencing
lot.requires_rate_review?, current_rate, number_to_percentage,
t(".update_needed"), and t(".unknown")) to use if/elsif/else for readability.
In `@app/views/pages/dashboard/_bond_summary.html.erb`:
- Around line 3-5: open_lots is built with bond_accounts.flat_map calling
account.bond.bond_lots.open which may trigger an N+1; fix it by loading
bond_lots in a single query or eager-loading them when fetching bond_accounts.
Update the controller where `@bond_accounts` is set to include/ preload the
associations (bond and bond_lots with the open scope) so that view code using
open_lots, bond_accounts, bond.bond_lots.open does not fire per-account queries;
alternatively, replace the view aggregation with a single query (e.g., query
BondLot.open joined to bonds/accounts) and assign that to open_lots so the view
only iterates over already-loaded records.
- Around line 40-66: The ERB partial contains heavy presentation logic (in the
top_lots loop using Bond.long_subtype_label_for and computing
total_return_amount/total_return_label, rate_text, and rate_meta) that should be
moved into a helper or presenter; create a helper method (e.g.
bond_lot_display_data(lot)) that returns a hash with keys label, rate_text,
rate_meta, and total_return (or separate private methods like
compute_rate_text(lot), compute_rate_meta(lot),
compute_total_return_display(lot)), move the conditional logic from the template
into those helper methods, and then replace the in-template logic with a single
call to bond_lot_display_data(lot) inside the partial while keeping references
to lot.inflation_linked?, lot.requires_rate_review?, lot.current_rate_percent,
lot.interest_rate, lot.current_inflation_component_percent,
lot.current_margin_percent, lot.rate_type, and lot.coupon_frequency in the
helper.
In `@db/migrate/20260330233052_create_bonds.rb`:
- Around line 1-15: Add indexes on frequently filtered columns in the
CreateBonds migration: inside the change method where create_table :bonds is
declared (or immediately after it) add indexes for the subtype and rate_type
columns (e.g., add_index :bonds, :subtype and add_index :bonds, :rate_type) so
queries that filter or group by those fields are fast; include names or options
(using: :btree) if you need explicit index naming or a specific index type, or
use t.index within the create_table block to define them inline.
In
`@db/migrate/20260331170000_add_gus_inflation_rates_and_auto_fetch_to_bond_lots.rb`:
- Line 14: The migration currently adds auto_fetch_inflation to :bond_lots with
default: true which opts all existing records into automatic GUS inflation
fetching; change the migration to use default: false (add_column :bond_lots,
:auto_fetch_inflation, :boolean, null: false, default: false) so existing bond
lots preserve prior behavior, and if you need selective opt-in, add an explicit
data backfill/update step after the column add to set true only for records that
should auto-fetch. Ensure you update any tests or seed logic that assume the new
default.
In `@db/schema.rb`:
- Around line 1602-1603: The foreign key from bond_lots to entries currently
lacks on_delete behavior; update the constraint for bond_lots.entry_id (the
add_foreign_key call referencing "entries") to include an appropriate on_delete
option—preferably on_delete: :nullify (or :cascade if you decide deletion
semantics require removing lots with their entry)—so that deleting an Entry
won't leave dangling entry_id references; locate the add_foreign_key
"bond_lots", "entries" invocation and change it to include the chosen on_delete
behavior (and add a migration to alter the foreign key if this is already in
production schema).
In `@test/controllers/pages_controller_test.rb`:
- Line 35: Replace the hard-coded assertion string in the test's assert_equal
against flash[:notice] with an i18n lookup; build the expected value via
I18n.t("pages.dashboard.bond_rate_review_notice", count: 1, accounts: "US
T-Bill") and assert_equal that expected value to flash[:notice] instead of the
literal "1 bond lot(s) need updated issue rates: US T-Bill."; update the
assert_equal call that currently references flash[:notice] to use the expected
variable derived from I18n.t.
In `@test/controllers/settings/hostings_controller_test.rb`:
- Around line 251-277: Add a new authorization test that mirrors the existing
admin-gate tests (e.g. clear_cache, disconnect_external_assistant): create a
test like "non-admin cannot enqueue gus inflation import" that uses
with_self_hosting but does not sign in as an admin, then POST to
import_gus_inflation_rates_settings_hosting_url with the same params and assert
that no ImportGusInflationRatesJob is enqueued (assert_no_enqueued_jobs only:
ImportGusInflationRatesJob) and that the request is rejected per your app policy
(match the existing admin-gate pattern: assert_redirected_to and flash alert or
assert_response :forbidden depending on how
clear_cache/disconnect_external_assistant assert authorization).
In `@test/jobs/settle_matured_bond_lots_job_test.rb`:
- Around line 4-24: Add a boundary case to the test by creating a second BondLot
via BondLot.create! with the same matured criteria but auto_close_on_maturity:
false (or create a lot with purchased_on such that it's not yet matured), call
SettleMaturedBondLotsJob.perform_now as before, then assert that the second
lot.reload.closed_on is still nil to verify non-auto-close lots (or
not-yet-matured lots) are not closed by SettleMaturedBondLotsJob.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
app/controllers/bond_lots_controller.rb (3)
18-26: Inconsistent permission check patterns.Lines 20 and 25 use
nil unless require_account_permission!while lines 6, 30, 51, and 68 usereturn unless require_account_permission!. Thenil unlesspattern is unusual and inconsistent with the rest of the controller.♻️ Proposed fix for consistency
def edit `@account` = `@bond_lot.account` - nil unless require_account_permission!(`@account`) + return unless require_account_permission!(`@account`) end def show `@account` = `@bond_lot.account` - nil unless require_account_permission!(`@account`) + return unless require_account_permission!(`@account`) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/bond_lots_controller.rb` around lines 18 - 26, The edit and show actions use the unusual pattern "nil unless require_account_permission!" which is inconsistent with other actions; update both methods (edit and show) to use "return unless require_account_permission!" after assigning `@account` = `@bond_lot.account` so the permission check behavior matches other controller actions that call require_account_permission!.
36-40: Consider atomic bond lot creation with entry.The current flow saves the bond lot, then immediately updates it to set the entry. This requires two database writes and could leave orphaned records if the entry creation fails (though the transaction should rollback).
♻️ Alternative: Create entry first, then create lot with entry reference
if `@bond_lot.valid`? ActiveRecord::Base.transaction do - `@bond_lot.save`! - `@bond_lot.update`!(entry: create_purchase_entry!(`@account`, `@bond_lot`)) + `@bond_lot.save`! + entry = create_purchase_entry!(`@account`, `@bond_lot`) + `@bond_lot.update_column`(:entry_id, entry.id) endOr consider having
create_purchase_entry!accept the bond_lot and set the association directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/bond_lots_controller.rb` around lines 36 - 40, The current transaction saves `@bond_lot` then immediately calls `@bond_lot.update`!(entry: create_purchase_entry!(`@account`, `@bond_lot`)), causing two writes and potential ordering issues; change the flow so the purchase entry is created (or prepared) before persisting the bond lot and then set the entry association on `@bond_lot` prior to save!, or refactor create_purchase_entry! to accept the bond lot and assign the association directly; keep the whole operation inside ActiveRecord::Base.transaction and replace the separate save! + update! with a single save! that persists the bond lot with its entry association.
117-134: Entry update logic could be extracted to the model.Per coding guidelines, controllers should be lightweight with domain logic in models. The
update_purchase_entry!method contains business logic about how bond lot changes should propagate to entries.Consider moving this to
BondLot#sync_purchase_entry!or similar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/bond_lots_controller.rb` around lines 117 - 134, The controller method update_purchase_entry! contains domain logic and should be moved to the model: implement a BondLot#sync_purchase_entry! (or BondLot#update_purchase_entry!) instance method that performs the same work (guarding on entry presence, computing subtype_label via Bond.long_subtype_label_for(subtype) || Bond.display_name.singularize, and calling entry.update! with date, name (use I18n.t same key), amount and entryable_attributes merging bond_subtype, bond_term_months and bond_interest_rate into entry.entryable.extra). Replace the controller’s update_purchase_entry! call with a single call to the new BondLot instance method so the controller remains thin and behavior is unchanged.app/models/bond_lot.rb (1)
162-188: Indentation inconsistency insettle_if_matured!method.The
with_lockblock (line 164) and nestedtransactionblock (line 175) have inconsistent indentation making the code structure hard to follow. While syntactically valid, the indentation suggests incorrect nesting.♻️ Proposed fix for consistent indentation
def settle_if_matured!(on: Date.current) # Lock the row to prevent concurrent settlements. with_lock do - return false unless auto_close_on_maturity? - return false unless open? - return false unless matured?(on:) + return false unless auto_close_on_maturity? + return false unless open? + return false unless matured?(on:) - settlement_date = [ on, maturity_date ].compact.min - gross_value = estimated_current_value(on: settlement_date) - gain = [ gross_value - amount.to_d, 0.to_d ].max - tax_withheld_amount = (gain * settlement_tax_rate_percent / 100).round(4) - net_value = (gross_value - tax_withheld_amount).round(4) + settlement_date = [ on, maturity_date ].compact.min + gross_value = estimated_current_value(on: settlement_date) + gain = [ gross_value - amount.to_d, 0.to_d ].max + tax_withheld_amount = (gain * settlement_tax_rate_percent / 100).round(4) + net_value = (gross_value - tax_withheld_amount).round(4) - ActiveRecord::Base.transaction do - create_settlement_entry!(settlement_date:, net_value:, tax_withheld_amount:, gross_value:) - update!( - closed_on: settlement_date, - settlement_amount: net_value, - tax_withheld: tax_withheld_amount - ) - create_reinvestment_lot!(settlement_date:, net_value:) if should_auto_buy_new_issue?(net_value:) - end + ActiveRecord::Base.transaction do + create_settlement_entry!(settlement_date:, net_value:, tax_withheld_amount:, gross_value:) + update!( + closed_on: settlement_date, + settlement_amount: net_value, + tax_withheld: tax_withheld_amount + ) + create_reinvestment_lot!(settlement_date:, net_value:) if should_auto_buy_new_issue?(net_value:) + end - account.sync_later(window_start_date: settlement_date) - true - end + account.sync_later(window_start_date: settlement_date) + true + end + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/bond_lot.rb` around lines 162 - 188, The with_lock/do and ActiveRecord::Base.transaction blocks in settle_if_matured! are misindented, which obscures nesting; reformat the method so with_lock do begins a block that clearly contains the transaction block and the subsequent account.sync_later and true return, and indent the ActiveRecord::Base.transaction do block and its inner calls (create_settlement_entry!, update!, create_reinvestment_lot!) consistently so the end keywords line up with their corresponding do keywords and the method’s control flow is visually correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/bond_lot.rb`:
- Around line 313-331: The orphaned block after inflation_linked_rate_context
must be wrapped into the missing method definition inflation_snapshot_for(on:);
locate the loose code that queries GusInflationRate and returns the hash
(currently sitting after inflation_linked_rate_context) and move or wrap it
inside def inflation_snapshot_for(on:) ... end, ensuring to remove the stray
extra end and keep references to GusInflationRate.for_date,
auto_fetch_inflation?, cpi_lag_months, inflation_rate_assumption, and
current_inflation_indicator_id so the method returns either the "gus" hash or
the "manual" hash as originally intended.
In `@db/migrate/20260331153000_reconfigure_bond_products_for_eod_rod.rb`:
- Around line 63-79: The down method currently defines a nested def down (a
syntax bug) so the irreversible raise is never executed; fix by removing the
inner method definition and making the outer down immediately raise
ActiveRecord::IrreversibleMigration (or move the raise to the top of the outer
down) instead of performing the column removals; update the method named down in
this migration (remove the nested def down block and any destructive
remove_column/remove_index calls) so the migration is correctly marked
irreversible.
In `@test/controllers/settings/hostings_controller_test.rb`:
- Around line 253-254: The test captures old_val after mutating
Setting.gus_inflation_import_enabled, so restore logic is ineffective; update
both occurrences to read the original value into old_val before assigning
Setting.gus_inflation_import_enabled = true (i.e., call old_val =
Setting.gus_inflation_import_enabled prior to the mutation) in the
hostings_controller_test.rb tests (the blocks that reference
Setting.gus_inflation_import_enabled and old_val) and apply the same change to
the second test block around lines 270-287 so the original setting is preserved
and restored correctly.
---
Nitpick comments:
In `@app/controllers/bond_lots_controller.rb`:
- Around line 18-26: The edit and show actions use the unusual pattern "nil
unless require_account_permission!" which is inconsistent with other actions;
update both methods (edit and show) to use "return unless
require_account_permission!" after assigning `@account` = `@bond_lot.account` so the
permission check behavior matches other controller actions that call
require_account_permission!.
- Around line 36-40: The current transaction saves `@bond_lot` then immediately
calls `@bond_lot.update`!(entry: create_purchase_entry!(`@account`, `@bond_lot`)),
causing two writes and potential ordering issues; change the flow so the
purchase entry is created (or prepared) before persisting the bond lot and then
set the entry association on `@bond_lot` prior to save!, or refactor
create_purchase_entry! to accept the bond lot and assign the association
directly; keep the whole operation inside ActiveRecord::Base.transaction and
replace the separate save! + update! with a single save! that persists the bond
lot with its entry association.
- Around line 117-134: The controller method update_purchase_entry! contains
domain logic and should be moved to the model: implement a
BondLot#sync_purchase_entry! (or BondLot#update_purchase_entry!) instance method
that performs the same work (guarding on entry presence, computing subtype_label
via Bond.long_subtype_label_for(subtype) || Bond.display_name.singularize, and
calling entry.update! with date, name (use I18n.t same key), amount and
entryable_attributes merging bond_subtype, bond_term_months and
bond_interest_rate into entry.entryable.extra). Replace the controller’s
update_purchase_entry! call with a single call to the new BondLot instance
method so the controller remains thin and behavior is unchanged.
In `@app/models/bond_lot.rb`:
- Around line 162-188: The with_lock/do and ActiveRecord::Base.transaction
blocks in settle_if_matured! are misindented, which obscures nesting; reformat
the method so with_lock do begins a block that clearly contains the transaction
block and the subsequent account.sync_later and true return, and indent the
ActiveRecord::Base.transaction do block and its inner calls
(create_settlement_entry!, update!, create_reinvestment_lot!) consistently so
the end keywords line up with their corresponding do keywords and the method’s
control flow is visually correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c61f03c4-f5d4-4d06-8f8f-cf7b5cd00c76
📒 Files selected for processing (20)
app/controllers/accounts_controller.rbapp/controllers/bond_lots_controller.rbapp/controllers/pages_controller.rbapp/controllers/settings/hostings_controller.rbapp/jobs/import_gus_inflation_rates_job.rbapp/models/bond.rbapp/models/bond_lot.rbapp/models/gus_inflation_rate.rbapp/models/provider/gus_sdp.rbapp/views/bond_lots/_form.html.erbapp/views/bonds/_form.html.erbapp/views/pages/dashboard/_bond_summary.html.erbapp/views/settings/hostings/_gus_sdp_settings.html.erbconfig/locales/views/pages/en.ymlconfig/locales/views/settings/hostings/en.ymldb/migrate/20260331120000_create_bond_lots.rbdb/migrate/20260331133000_add_purchase_fields_to_bond_lots.rbdb/migrate/20260331153000_reconfigure_bond_products_for_eod_rod.rbdocs/localization/pl-localization-prep.mdtest/controllers/settings/hostings_controller_test.rb
✅ Files skipped from review due to trivial changes (1)
- docs/localization/pl-localization-prep.md
🚧 Files skipped from review as they are similar to previous changes (13)
- app/controllers/accounts_controller.rb
- db/migrate/20260331120000_create_bond_lots.rb
- db/migrate/20260331133000_add_purchase_fields_to_bond_lots.rb
- app/controllers/pages_controller.rb
- app/jobs/import_gus_inflation_rates_job.rb
- config/locales/views/settings/hostings/en.yml
- app/controllers/settings/hostings_controller.rb
- app/views/settings/hostings/_gus_sdp_settings.html.erb
- app/views/bond_lots/_form.html.erb
- app/views/pages/dashboard/_bond_summary.html.erb
- app/views/bonds/_form.html.erb
- app/models/gus_inflation_rate.rb
- config/locales/views/pages/en.yml
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
app/views/settings/hostings/_gus_sdp_settings.html.erb (2)
93-98: Consider consolidating database queries.This section makes 4 separate queries to
GusInflationRate(count,exists?,minimum,maximum). For a settings page with low traffic this is acceptable, but could be consolidated into a single query if desired.Optional optimization
You could precompute the stats in the controller or use a single aggregation:
# In controller `@gus_stats` = GusInflationRate.pick(Arel.sql('COUNT(*), MIN(year), MAX(year)')) # Returns [count, min_year, max_year] or nil if no recordsThen in the view:
<% count, min_year, max_year = `@gus_stats` || [0, nil, nil] %> <p><strong><%= t(".stored_records") %>:</strong> <%= count %></p> <% if count > 0 %> <p><strong><%= t(".stored_range") %>:</strong> <%= min_year %>-<%= max_year %></p> <% end %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/settings/hostings/_gus_sdp_settings.html.erb` around lines 93 - 98, Consolidate the four GusInflationRate queries by computing the stats once (count, min year, max year) in the controller and passing them to the view (e.g. `@gus_stats`), then destructure those values in the view and use count to decide whether to render the stored_range; update references to GusInflationRate.count, .exists?, .minimum(:year), and .maximum(:year) to use the precomputed values (count, min_year, max_year) instead.
50-52: Use functional design token instead of raw Tailwind color variant.
text-secondary-600should be replaced with the design system tokentext-secondaryfor consistency with the rest of the file and the project's styling conventions.Proposed fix
- <div class="text-secondary-600 text-sm"> + <div class="text-secondary text-sm">As per coding guidelines: "Use Tailwind design tokens (e.g., text-primary, bg-container, border-secondary) instead of raw color names."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/settings/hostings/_gus_sdp_settings.html.erb` around lines 50 - 52, Replace the raw Tailwind color class "text-secondary-600" with the design token "text-secondary" in the ERB block that renders the configured_via_env message (the div containing <%= t(".configured_via_env") %>), so the div's class list uses the project design token for text color instead of the raw color variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/bond_lots_controller.rb`:
- Around line 29-33: The create action renders :new for non-bond accounts before
`@bond_lot` is initialized; change create to use the exact non-bond rejection path
used by new (rather than render :new) — either call the same redirect/alert
helper or move the non-bond check into a shared before_action used by both new
and create; alternatively, if you must render :new, ensure `@bond_lot` is
initialized the same way new does before rendering. Update the create method
(and related before_action if added) to reference create, new and `@bond_lot` so
the behavior and instance setup match.
In `@app/models/bond_lot.rb`:
- Around line 86-92: The loop in the compound/settlement logic stops iterating
when annual_rate_for(on: cursor) returns blank but later code still sets
closed_on and settlement_amount, allowing auto-settlement with incomplete rates;
update the logic in the methods that use cursor/period_end (the loop that
computes days_in_step and calls annual_rate_for) so that if annual_rate_for
returns nil/blank you do not persist closed_on or settlement_amount and instead
mark the lot as needing review (e.g., set requires_rate_review or return/raise
to abort settlement); ensure the same guard is applied in the other affected
block (around the code that sets closed_on/settlement_amount at lines ~168-189)
so no matured lot is auto-closed when rates are unresolved.
- Around line 303-312: The code is currently coercing a missing CPI
(inflation_component from inflation_snapshot_for(on:)) to 0 and computing an
annual_rate_decimal, which hides a missing CPI; change the logic in the method
that calls inflation_snapshot_for(on:) (and touches inflation_component,
inflation_margin and inflation_rate_assumption) so that when
inflation_snapshot[:inflation_component] is nil and there is no
inflation_rate_assumption provided you return annual_rate_decimal: nil (do not
treat nil as 0); only perform the numeric sum/division when inflation_component
is present or an explicit inflation_rate_assumption exists (i.e. use the
explicit assumption or the snapshot value, otherwise bail out), and remove the
fallback that coerces nil to 0 in the calculation currently using
((inflation_component || 0.to_d) + margin_component) / 100.
- Around line 80-100: The loop uses cursor + 1.year as the anniversary anchor
but should use the bond's issue_date to compute annual anniversaries (while
still starting accrual at purchased_on). Fix by computing an anniversary
boundary for the current cursor using issue_date (e.g. compute years_since_issue
= ((cursor - issue_date) / 1.year).floor and next_anniversary = issue_date +
(years_since_issue + 1).years), then replace uses of "cursor + 1.year" (and
related calculations for days_in_year and the full-year branch) with
next_anniversary and the corresponding anniversary_start; keep value accrual
starting from purchased_on and keep calling annual_rate_for(on: cursor) for the
rate value but use the issue_date-derived next_anniversary for
rate-reset/maturity boundary logic in the loop (apply same change to the other
similar blocks referenced).
In `@app/views/bond_lots/_form.html.erb`:
- Around line 31-36: The form's number_field for :units currently allows 0 but
the BondLot model validation rejects values <= 0; update the form.number_field
(the field with data-bond-lot-form-target "inflationInput" inside the
inflationFields block) to use a min value greater than 0 (e.g., min: 0.01 or
whatever smallest valid unit your model expects) and keep the step consistent so
the client-side constraint matches the BondLot validation.
In `@app/views/pages/dashboard/_bond_summary.html.erb`:
- Line 33: top_lots is being selected by comparing raw estimated_current_value
across mixed currencies which yields incorrect ordering; convert each lot's
estimated_current_value into the family currency before sorting (the same
normalization used for summary totals) by using the existing exchange conversion
path (e.g., use Current.family.currency / ExchangeRate lookup or the helper used
elsewhere) when computing the sort key for open_lots so the .sort_by block
compares values all in Current.family.currency, then take .first(5).
---
Nitpick comments:
In `@app/views/settings/hostings/_gus_sdp_settings.html.erb`:
- Around line 93-98: Consolidate the four GusInflationRate queries by computing
the stats once (count, min year, max year) in the controller and passing them to
the view (e.g. `@gus_stats`), then destructure those values in the view and use
count to decide whether to render the stored_range; update references to
GusInflationRate.count, .exists?, .minimum(:year), and .maximum(:year) to use
the precomputed values (count, min_year, max_year) instead.
- Around line 50-52: Replace the raw Tailwind color class "text-secondary-600"
with the design token "text-secondary" in the ERB block that renders the
configured_via_env message (the div containing <%= t(".configured_via_env") %>),
so the div's class list uses the project design token for text color instead of
the raw color variant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ec3a114-38f4-4a96-a5f9-64ce4a9bc832
📒 Files selected for processing (14)
app/controllers/accounts_controller.rbapp/controllers/bond_lots_controller.rbapp/controllers/settings/hostings_controller.rbapp/models/bond_lot.rbapp/views/bond_lots/_form.html.erbapp/views/pages/dashboard/_bond_summary.html.erbapp/views/settings/hostings/_gus_sdp_settings.html.erbconfig/locales/views/bonds/en.ymldb/migrate/20260331120000_create_bond_lots.rbdb/migrate/20260331133000_add_purchase_fields_to_bond_lots.rbdb/migrate/20260331153000_reconfigure_bond_products_for_eod_rod.rbtest/controllers/accounts_controller_test.rbtest/controllers/pages_controller_test.rbtest/controllers/settings/hostings_controller_test.rb
✅ Files skipped from review due to trivial changes (2)
- test/controllers/accounts_controller_test.rb
- db/migrate/20260331120000_create_bond_lots.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- test/controllers/settings/hostings_controller_test.rb
- db/migrate/20260331133000_add_purchase_fields_to_bond_lots.rb
- config/locales/views/bonds/en.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/controllers/pages_controller.rb (1)
359-363: Consolidate query loads to avoid duplicate database queries.Line 362-363 triggers two separate queries on
pending_lots: a COUNT query via.countand a SELECT query via.includes(...).map(...). Load the records once with eager loading and reuse:♻️ Suggested optimization
def show_bond_rate_review_notice! return if session[:bond_rate_review_prompted] - pending_lots = BondLot.needs_rate_review.joins(bond: :account).merge(Account.accessible_by(Current.user)) - return if pending_lots.empty? + lots = BondLot.needs_rate_review + .joins(bond: :account) + .merge(Account.accessible_by(Current.user)) + .includes(bond: :account) + .to_a + return if lots.empty? - account_names = pending_lots.includes(bond: :account).map { |lot| lot.account.name }.uniq.first(3).join(", ") - flash.now[:notice] = t("pages.dashboard.bond_rate_review_notice", count: pending_lots.count, accounts: account_names) + account_names = lots.map { |lot| lot.account.name }.uniq.first(3).join(", ") + flash.now[:notice] = t("pages.dashboard.bond_rate_review_notice", count: lots.size, accounts: account_names) session[:bond_rate_review_prompted] = true end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/pages_controller.rb` around lines 359 - 363, The code triggers two DB queries by enumerating pending_lots twice; change the logic to eager-load and materialize the relation once (e.g., call load or to_a on the relation returned by BondLot.needs_rate_review.joins(bond: :account).merge(Account.accessible_by(Current.user))) and then reuse that in calculating count and account_names (use the loaded collection for .count and .includes(...).map(...) logic) so the records are fetched only once before building flash.now[:notice] in pages_controller (referencing pending_lots, account_names, and the flash assignment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/controllers/pages_controller.rb`:
- Around line 359-363: The code triggers two DB queries by enumerating
pending_lots twice; change the logic to eager-load and materialize the relation
once (e.g., call load or to_a on the relation returned by
BondLot.needs_rate_review.joins(bond:
:account).merge(Account.accessible_by(Current.user))) and then reuse that in
calculating count and account_names (use the loaded collection for .count and
.includes(...).map(...) logic) so the records are fetched only once before
building flash.now[:notice] in pages_controller (referencing pending_lots,
account_names, and the flash assignment).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0b2a19b-8885-490c-ad96-c32fe05e657d
📒 Files selected for processing (3)
app/controllers/pages_controller.rbapp/models/bond_lot.rbconfig/locales/views/bonds/en.yml
✅ Files skipped from review due to trivial changes (1)
- config/locales/views/bonds/en.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/bond_lot.rb
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/controllers/settings/hostings_controller.rb (2)
175-190: Consider server-side year bounds validation.The view enforces
min: 1990andmax: Date.current.yearon the form fields, but these bounds aren't validated server-side. An admin could bypass the form and submit extreme values. Since this is admin-only and the external API would return empty data for invalid years, this is low risk—but adding server-side validation would make the API more defensive.Optional: Add server-side year bounds
start_year = import_params[:gus_inflation_start_year].presence&.to_i || (Date.current.year - 20) end_year = import_params[:gus_inflation_end_year].presence&.to_i || (Date.current.year - 1) + + min_year = 1990 + max_year = Date.current.year + start_year = start_year.clamp(min_year, max_year) + end_year = end_year.clamp(min_year, max_year) if start_year > end_year return redirect_to settings_hosting_path, alert: t(".invalid_import_range") end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/settings/hostings_controller.rb` around lines 175 - 190, Add server-side validation in import_gus_inflation_rates to enforce the same year bounds as the form: parse import_params[:gus_inflation_start_year] and :gus_inflation_end_year to integers (fall back to defaults as currently done), then ensure start_year and end_year are within 1990..Date.current.year (and start_year <= end_year); if validation fails, redirect_to settings_hosting_path with an appropriate alert (reuse t(".invalid_import_range") or add a new i18n key) instead of enqueuing ImportGusInflationRatesJob. Use the existing import_params and ImportGusInflationRatesJob identifiers to locate the code.
210-217: Optional: Consolidate into a single query.This makes up to 3 database queries. Since it's on an admin-only page, performance is acceptable, but could be simplified.
Optional: Single query approach
def set_gus_stats - gus_count = GusInflationRate.count - `@gus_stats` = { - count: gus_count, - min_year: gus_count > 0 ? GusInflationRate.minimum(:year) : nil, - max_year: gus_count > 0 ? GusInflationRate.maximum(:year) : nil - } + stats = GusInflationRate.pick(Arel.sql("COUNT(*), MIN(year), MAX(year)")) + count, min_year, max_year = stats || [0, nil, nil] + `@gus_stats` = { count: count, min_year: min_year, max_year: max_year } end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/settings/hostings_controller.rb` around lines 210 - 217, The method set_gus_stats currently issues up to three DB queries; replace these with a single aggregate query on GusInflationRate that selects COUNT(*), MIN(year), and MAX(year) (e.g., using GusInflationRate.select('COUNT(*) AS count, MIN(year) AS min_year, MAX(year) AS max_year').take), then build `@gus_stats` from the returned aggregate row (convert count to integer and set min_year/max_year to nil when count is zero). Update set_gus_stats to use that single query and map the aggregate fields into the same keys: count, min_year, max_year.app/models/bond_lot.rb (1)
429-465: Consider inheriting fixed-rate params for reinvestment lots.Lines 448-450 set
interest_rate,rate_type, andcoupon_frequencytonilfor non-inflation-linked reinvestments. Since these are fixed-rate bonds, the replacement lot should likely inherit the parent's rates to enable accurate valuation until the user reviews.💡 Suggested change
subtype: subtype, - interest_rate: inflation_linked? ? nil : interest_rate, - rate_type: inflation_linked? ? nil : rate_type, - coupon_frequency: inflation_linked? ? nil : coupon_frequency, + interest_rate: interest_rate, # Inherit for fixed-rate continuity + rate_type: rate_type, + coupon_frequency: coupon_frequency,This allows fixed-rate reinvestments to have valid rates immediately while still being flagged for review via
requires_rate_review: true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/bond_lot.rb` around lines 429 - 465, In create_reinvestment_lot! ensure non-inflation-linked replacement lots inherit the parent's fixed-rate parameters instead of nil: when building replacement_lot (method create_reinvestment_lot! in BondLot/bond_lot.rb) set interest_rate, rate_type and coupon_frequency to the current lot's values for non-inflation-linked reinvestments (i.e. mirror interest_rate, rate_type, coupon_frequency from self), while keeping requires_rate_review: true so users can still review/update the inherited rates.app/controllers/bond_lots_controller.rb (1)
74-79: Simplify destroy: let Entry cascade handle BondLot deletion.Per the
Entrymodel (has_one :bond_lot, dependent: :destroy), destroying the entry will automatically destroy the associated bond_lot. The current order destroys the bond_lot first, then the entry—but since the bond_lot is already gone, the cascade is a no-op. This works but is confusing and could break if the association changes.♻️ Suggested simplification
entry = `@bond_lot.entry` ActiveRecord::Base.transaction do - `@bond_lot.destroy`! - entry&.destroy! + entry&.destroy! || `@bond_lot.destroy`! endOr more explicitly, destroy only the entry when it exists (it cascades), otherwise destroy the orphan lot:
entry = `@bond_lot.entry` ActiveRecord::Base.transaction do + # Entry has `dependent: :destroy` on bond_lot, so destroying entry cascades. + # If no entry exists, destroy the lot directly. - `@bond_lot.destroy`! - entry&.destroy! + if entry + entry.destroy! + else + `@bond_lot.destroy`! + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/bond_lots_controller.rb` around lines 74 - 79, The controller currently destroys `@bond_lot` before its Entry, which is confusing given Entry has_one :bond_lot, dependent: :destroy; change the transaction to destroy the Entry first when present so the cascade removes the BondLot (and only destroy `@bond_lot` directly if there is no associated entry). Update the logic in bond_lots_controller (the block using `@bond_lot` and entry) to: within ActiveRecord::Base.transaction, if entry.present? call entry.destroy! else call `@bond_lot.destroy`! so the dependent: :destroy cascade is relied upon.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/bond_lot.rb`:
- Around line 375-386: The method apply_product_defaults unconditionally assigns
self.term_months = defaults[:term_months], overwriting user-provided values;
change this to only set the default when term_months is nil/unset (e.g. use ||=
or a nil-check) so apply_product_defaults behaves like
Bond#apply_subtype_defaults and preserves existing term_months; update the
apply_product_defaults method (referenced by the method name and
Bond::PRODUCT_DEFAULTS/subtype and the term_months attribute) accordingly.
---
Nitpick comments:
In `@app/controllers/bond_lots_controller.rb`:
- Around line 74-79: The controller currently destroys `@bond_lot` before its
Entry, which is confusing given Entry has_one :bond_lot, dependent: :destroy;
change the transaction to destroy the Entry first when present so the cascade
removes the BondLot (and only destroy `@bond_lot` directly if there is no
associated entry). Update the logic in bond_lots_controller (the block using
`@bond_lot` and entry) to: within ActiveRecord::Base.transaction, if
entry.present? call entry.destroy! else call `@bond_lot.destroy`! so the
dependent: :destroy cascade is relied upon.
In `@app/controllers/settings/hostings_controller.rb`:
- Around line 175-190: Add server-side validation in import_gus_inflation_rates
to enforce the same year bounds as the form: parse
import_params[:gus_inflation_start_year] and :gus_inflation_end_year to integers
(fall back to defaults as currently done), then ensure start_year and end_year
are within 1990..Date.current.year (and start_year <= end_year); if validation
fails, redirect_to settings_hosting_path with an appropriate alert (reuse
t(".invalid_import_range") or add a new i18n key) instead of enqueuing
ImportGusInflationRatesJob. Use the existing import_params and
ImportGusInflationRatesJob identifiers to locate the code.
- Around line 210-217: The method set_gus_stats currently issues up to three DB
queries; replace these with a single aggregate query on GusInflationRate that
selects COUNT(*), MIN(year), and MAX(year) (e.g., using
GusInflationRate.select('COUNT(*) AS count, MIN(year) AS min_year, MAX(year) AS
max_year').take), then build `@gus_stats` from the returned aggregate row (convert
count to integer and set min_year/max_year to nil when count is zero). Update
set_gus_stats to use that single query and map the aggregate fields into the
same keys: count, min_year, max_year.
In `@app/models/bond_lot.rb`:
- Around line 429-465: In create_reinvestment_lot! ensure non-inflation-linked
replacement lots inherit the parent's fixed-rate parameters instead of nil: when
building replacement_lot (method create_reinvestment_lot! in
BondLot/bond_lot.rb) set interest_rate, rate_type and coupon_frequency to the
current lot's values for non-inflation-linked reinvestments (i.e. mirror
interest_rate, rate_type, coupon_frequency from self), while keeping
requires_rate_review: true so users can still review/update the inherited rates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05c1d905-adb7-4381-b5ec-32ed422d2a41
📒 Files selected for processing (19)
app/controllers/bond_lots_controller.rbapp/controllers/pages_controller.rbapp/controllers/settings/hostings_controller.rbapp/jobs/import_gus_inflation_rates_job.rbapp/jobs/settle_matured_bond_lots_job.rbapp/models/bond.rbapp/models/bond_lot.rbapp/models/gus_inflation_rate.rbapp/views/bond_lots/_form.html.erbapp/views/bond_lots/new.html.erbapp/views/bonds/_form.html.erbapp/views/bonds/edit.html.erbapp/views/bonds/new.html.erbapp/views/pages/dashboard/_bond_summary.html.erbapp/views/settings/hostings/_gus_sdp_settings.html.erbconfig/locales/views/pages/en.ymldb/migrate/20260401120000_add_settlement_composite_index_to_bond_lots.rbdb/schema.rbtest/controllers/pages_controller_test.rb
✅ Files skipped from review due to trivial changes (5)
- app/views/bond_lots/new.html.erb
- db/migrate/20260401120000_add_settlement_composite_index_to_bond_lots.rb
- config/locales/views/pages/en.yml
- app/views/bonds/_form.html.erb
- app/views/bond_lots/_form.html.erb
🚧 Files skipped from review as they are similar to previous changes (7)
- app/views/bonds/edit.html.erb
- app/views/bonds/new.html.erb
- test/controllers/pages_controller_test.rb
- app/jobs/import_gus_inflation_rates_job.rb
- app/jobs/settle_matured_bond_lots_job.rb
- app/models/gus_inflation_rate.rb
- db/schema.rb
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@coderabbitai resume |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
db/schema.rb (1)
1603-1604: Consider addingON DELETEbehavior for referential integrity.While Rails handles deletion via
dependent: :destroy, adding database-level constraints ensures integrity if records are ever deleted outside Rails (admin tools, raw SQL, data migrations):♻️ Suggested improvement
- add_foreign_key "bond_lots", "bonds" - add_foreign_key "bond_lots", "entries" + add_foreign_key "bond_lots", "bonds", on_delete: :cascade + add_foreign_key "bond_lots", "entries", on_delete: :nullifyThis aligns with similar patterns in this schema (e.g.,
balances -> accountsuseson_delete: :cascade,accounts -> usersuseson_delete: :nullify).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/schema.rb` around lines 1603 - 1604, The schema adds foreign keys for bond_lots -> bonds and bond_lots -> entries without specifying ON DELETE behavior; update the two add_foreign_key calls for "bond_lots", "bonds" and "bond_lots", "entries" to include an appropriate on_delete option (e.g., on_delete: :cascade or on_delete: :nullify) to enforce referential integrity at the DB level and match the project's existing patterns. Ensure you choose the correct behavior for each relationship and run migrations/schema dump to persist the change.app/jobs/settle_matured_bond_lots_job.rb (1)
5-14: Consider eager loading to avoid N+1 in error logging.The query chain accesses
lot.account.idin the rescue block, butaccountis delegated throughbond. If multiple lots fail, each failure triggers a separate query. While this only affects the error path, adding.includes(bond: :account)would be more robust.♻️ Suggested optimization
def perform(on: Date.current) BondLot.open .where(auto_close_on_maturity: true) .where("maturity_date <= ?", on) + .includes(bond: :account) .find_each do |lot| lot.settle_if_matured!(on:) rescue StandardError => e Rails.logger.error( "SettleMaturedBondLotsJob failed for lot_id=#{lot.id} account_id=#{lot.account.id}: #{e.class}: #{e.message}" ) end endAs per coding guidelines: Optimize database queries with proper indexes to prevent N+1 queries using includes/joins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/jobs/settle_matured_bond_lots_job.rb` around lines 5 - 14, The rescue block in the BondLot.open ... .find_each loop dereferences lot.account (via bond) causing an N+1 on the error path; update the query that builds the iterator (the chain starting at BondLot.open.where(...).where("maturity_date <= ?", on).find_each) to eager-load the associated bond and account (e.g. add includes(bond: :account)) so lot.settle_if_matured!(on:) and the rescue logging can access lot.account.id without triggering extra queries.app/controllers/bond_lots_controller.rb (1)
74-82: Clarify destroy dependency chain.The logic destroys
entrywhen present (relying onEntry has_one :bond_lot, dependent: :destroyto cascade), or destroys the lot directly if no entry exists. This is correct but subtle—a brief comment would help future maintainers understand the cascade.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/bond_lots_controller.rb` around lines 74 - 82, Add a brief in-line comment above the transaction explaining the destroy dependency: note that when entry is present we call entry.destroy! and rely on Entry's has_one :bond_lot, dependent: :destroy association to cascade and remove the associated `@bond_lot`, otherwise we explicitly destroy `@bond_lot` when no entry exists; place this comment near the entry / `@bond_lot` destruction logic so future maintainers understand the cascade behavior.app/views/pages/dashboard/_bond_summary.html.erb (1)
44-65: Consider extracting rate display logic to a helper or presenter.The nested conditionals for
rate_textandrate_metaare quite dense. While functionally correct, this logic could be extracted to a view helper or presenter for better testability and readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/pages/dashboard/_bond_summary.html.erb` around lines 44 - 65, Extract the nested display logic for rate_text and rate_meta into a view helper or presenter (e.g., a BondRatePresenter or helper methods rate_text_for(lot) and rate_meta_for(lot)) so the template only calls those methods; move the conditional branches that reference lot.inflation_linked?, lot.requires_rate_review?, lot.current_rate_percent, lot.interest_rate, lot.current_inflation_source, lot.current_inflation_component_percent, lot.current_margin_percent, lot.current_inflation_indicator_id, lot.rate_type and lot.coupon_frequency into the new presenter/helper, implement tests for the presenter methods, and replace the complex ERB block with simple calls like rate_text_for(lot) and rate_meta_for(lot).app/models/bond_lot.rb (1)
84-109: Consider extracting duplicate anniversary calculation logic.The anniversary boundary calculation (lines 88-91 and 230-233) is duplicated between
estimated_current_valueandcapitalization_history. Consider extracting to a private helper method for DRY compliance.♻️ Suggested extraction
def anniversary_boundaries_for(cursor:, issue_base:) years_since_issue = 0 years_since_issue += 1 while issue_base + years_since_issue.years <= cursor next_anniversary = issue_base + years_since_issue.years anniversary_start = issue_base + (years_since_issue - 1).years [next_anniversary, anniversary_start] endAlso applies to: 226-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/bond_lot.rb` around lines 84 - 109, Duplicate anniversary boundary calculation appears in estimated_current_value and capitalization_history; extract it into a private helper (e.g., anniversary_boundaries_for(cursor:, issue_base:)) that returns [next_anniversary, anniversary_start], replace the repeated block in both methods with a call to that helper, and ensure callers still compute days_in_step, days_in_year and use next_anniversary/anniversary_start as before; keep the helper private in the BondLot class and update any tests if they reference the original logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/bond_lots_controller.rb`:
- Around line 129-137: The code calls entryable.extra.merge(...) which will
raise if entryable.extra is nil; in bond_lots_controller (around the
entryable_attributes block) guard against nil by coercing entryable.extra to a
hash before merging (e.g., use entry.entryable.extra.to_h or
entry.entryable.extra || {}), then merge the bond_subtype, bond_term_months and
bond_interest_rate into that safe hash so bond_lot.entry.entryable.extra cannot
cause a NoMethodError.
---
Nitpick comments:
In `@app/controllers/bond_lots_controller.rb`:
- Around line 74-82: Add a brief in-line comment above the transaction
explaining the destroy dependency: note that when entry is present we call
entry.destroy! and rely on Entry's has_one :bond_lot, dependent: :destroy
association to cascade and remove the associated `@bond_lot`, otherwise we
explicitly destroy `@bond_lot` when no entry exists; place this comment near the
entry / `@bond_lot` destruction logic so future maintainers understand the cascade
behavior.
In `@app/jobs/settle_matured_bond_lots_job.rb`:
- Around line 5-14: The rescue block in the BondLot.open ... .find_each loop
dereferences lot.account (via bond) causing an N+1 on the error path; update the
query that builds the iterator (the chain starting at
BondLot.open.where(...).where("maturity_date <= ?", on).find_each) to eager-load
the associated bond and account (e.g. add includes(bond: :account)) so
lot.settle_if_matured!(on:) and the rescue logging can access lot.account.id
without triggering extra queries.
In `@app/models/bond_lot.rb`:
- Around line 84-109: Duplicate anniversary boundary calculation appears in
estimated_current_value and capitalization_history; extract it into a private
helper (e.g., anniversary_boundaries_for(cursor:, issue_base:)) that returns
[next_anniversary, anniversary_start], replace the repeated block in both
methods with a call to that helper, and ensure callers still compute
days_in_step, days_in_year and use next_anniversary/anniversary_start as before;
keep the helper private in the BondLot class and update any tests if they
reference the original logic.
In `@app/views/pages/dashboard/_bond_summary.html.erb`:
- Around line 44-65: Extract the nested display logic for rate_text and
rate_meta into a view helper or presenter (e.g., a BondRatePresenter or helper
methods rate_text_for(lot) and rate_meta_for(lot)) so the template only calls
those methods; move the conditional branches that reference
lot.inflation_linked?, lot.requires_rate_review?, lot.current_rate_percent,
lot.interest_rate, lot.current_inflation_source,
lot.current_inflation_component_percent, lot.current_margin_percent,
lot.current_inflation_indicator_id, lot.rate_type and lot.coupon_frequency into
the new presenter/helper, implement tests for the presenter methods, and replace
the complex ERB block with simple calls like rate_text_for(lot) and
rate_meta_for(lot).
In `@db/schema.rb`:
- Around line 1603-1604: The schema adds foreign keys for bond_lots -> bonds and
bond_lots -> entries without specifying ON DELETE behavior; update the two
add_foreign_key calls for "bond_lots", "bonds" and "bond_lots", "entries" to
include an appropriate on_delete option (e.g., on_delete: :cascade or on_delete:
:nullify) to enforce referential integrity at the DB level and match the
project's existing patterns. Ensure you choose the correct behavior for each
relationship and run migrations/schema dump to persist the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ded27873-f071-4aea-87ea-427f8b8e9897
📒 Files selected for processing (20)
app/controllers/bond_lots_controller.rbapp/controllers/pages_controller.rbapp/controllers/settings/hostings_controller.rbapp/javascript/controllers/bond_lot_form_controller.jsapp/jobs/import_gus_inflation_rates_job.rbapp/jobs/settle_matured_bond_lots_job.rbapp/models/bond.rbapp/models/bond_lot.rbapp/models/gus_inflation_rate.rbapp/views/bond_lots/_form.html.erbapp/views/bond_lots/new.html.erbapp/views/bonds/_form.html.erbapp/views/bonds/edit.html.erbapp/views/bonds/new.html.erbapp/views/pages/dashboard/_bond_summary.html.erbapp/views/settings/hostings/_gus_sdp_settings.html.erbconfig/locales/views/pages/en.ymldb/migrate/20260401120000_add_settlement_composite_index_to_bond_lots.rbdb/schema.rbtest/controllers/pages_controller_test.rb
✅ Files skipped from review due to trivial changes (8)
- app/views/bonds/edit.html.erb
- app/views/bonds/new.html.erb
- app/views/bond_lots/new.html.erb
- config/locales/views/pages/en.yml
- app/views/bond_lots/_form.html.erb
- app/views/bonds/_form.html.erb
- app/jobs/import_gus_inflation_rates_job.rb
- app/models/gus_inflation_rate.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- test/controllers/pages_controller_test.rb
- db/migrate/20260401120000_add_settlement_composite_index_to_bond_lots.rb
- app/javascript/controllers/bond_lot_form_controller.js
- app/controllers/pages_controller.rb
✅ Actions performedReviews resumed. |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/models/bond_lot_test.rb (1)
598-614: Lock the non-par pricing path in with an output assertion.This test only proves the 950 purchase price survives validation. It still passes if coupon or maturity math keeps using
amountinstead of the 1000 face value, so the new non-par path is not covered end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/bond_lot_test.rb` around lines 598 - 614, The test only checks the stored amount, so lock the non-par pricing path by asserting the derived face-value used for coupon/maturity calculations: call BondLot#derive_amount_from_units (or invoke the public method that computes the lot's derived face value) and assert it equals units * nominal_per_unit (10 * 100 = 1000), and/or assert any coupon/maturity-calculation helper on BondLot that uses the derived amount returns values based on 1000 rather than 950; update the test to include this additional assertion using the lot instance's units, nominal_per_unit, and derive_amount_from_units (or the relevant coupon/maturity method).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/bond_lot.rb`:
- Around line 174-209: The method estimated_current_value (and other places that
use amount as the coupon/redemption base) wrongly uses amount as the cash-flow
principal; add a private helper cashflow_principal (suggested implementation:
units.present? && nominal_per_unit.present? ? units.to_d * nominal_per_unit.to_d
: amount.to_d) and replace uses of amount.to_d when calculating coupon
accruals/redemption/coupon reinvestment (e.g., in estimated_current_value and
the other similar blocks referenced) with cashflow_principal, while keeping
amount for cost/return/tax basis logic unchanged.
- Around line 46-47: The two conversions that set converted_value and
converted_return are forcing a 1:1 FX by passing custom_rate: 1; remove the
custom_rate override so Money.new(...,
account.currency).exchange_to(family_currency) uses the real rate lookup
(ExchangeRate.find_or_fetch_rate) and returns correct normalized amounts; update
the calls that construct converted_value and converted_return to drop the
custom_rate parameter and keep the rest of the logic the same.
In `@app/models/bond/inflation_provider.rb`:
- Around line 3-9: The current DEFAULT_PROVIDER_BY_LOCALE maps generic "en" to
"us_bls", which causes blank product_code flows to silently default to US CPI;
remove the "en" => "us_bls" entry (keep explicit "us" and "en-us" mappings only)
and update the code that selects the provider when product_code is blank to
treat a missing mapping as nil (no default) rather than falling back to US;
ensure the lookup that uses DEFAULT_PROVIDER_BY_LOCALE and the product_code
branch return nil/unset for generic "en" so manual/custom lots outside the US do
not get assigned "us_bls".
---
Nitpick comments:
In `@test/models/bond_lot_test.rb`:
- Around line 598-614: The test only checks the stored amount, so lock the
non-par pricing path by asserting the derived face-value used for
coupon/maturity calculations: call BondLot#derive_amount_from_units (or invoke
the public method that computes the lot's derived face value) and assert it
equals units * nominal_per_unit (10 * 100 = 1000), and/or assert any
coupon/maturity-calculation helper on BondLot that uses the derived amount
returns values based on 1000 rather than 950; update the test to include this
additional assertion using the lot instance's units, nominal_per_unit, and
derive_amount_from_units (or the relevant coupon/maturity method).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afdfb9a6-1264-4610-bb14-2cfcbec9a7a2
📒 Files selected for processing (15)
app/components/UI/account/chart.rbapp/models/account.rbapp/models/balance/sync_cache.rbapp/models/binance_account/usd_converter.rbapp/models/bond/inflation_provider.rbapp/models/bond_lot.rbapp/models/gus_inflation_rate.rbapp/models/purchase_holding_presenter.rbcompose.example.ymlconfig/locales/views/accounts/en.ymlconfig/routes.rbdb/migrate/20260407170000_fix_bond_lots_non_inflation_check_constraint.rbtest/models/bond/inflation_provider_test.rbtest/models/bond_lot_test.rbtest/models/gus_inflation_rate_test.rb
✅ Files skipped from review due to trivial changes (2)
- config/locales/views/accounts/en.yml
- compose.example.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- app/components/UI/account/chart.rb
- app/models/account.rb
- config/routes.rb
- app/models/balance/sync_cache.rb
- Add cashflow_principal helper (units * nominal_per_unit fallback to amount) and use it in estimated_current_value, coupon_amount_per_period, capitalization_history — keeps purchase price as cost basis only - Remove custom_rate: 1 override in dashboard_summary; use real FX lookup - Remove 'en' => 'us_bls' from DEFAULT_PROVIDER_BY_LOCALE; generic English no longer silently defaults to US BLS - Memoize BondSummaryRow#projected_total_return? with defined? guard - Add nil fallback for CPI reference date in inflation_data_unavailable - Add cashflow_principal assertion to non-par pricing test
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Comment on lines
+30
to
+34
| <%= render Settings::Hostings::InflationImportSettingsComponent.new( | ||
| inflation_import_enabled_effective: Setting.inflation_import_enabled_effective, | ||
| env_inflation_import_enabled: ENV["INFLATION_IMPORT_ENABLED"].presence || ENV["GUS_INFLATION_IMPORT_ENABLED"], | ||
| last_import_at: Setting.inflation_last_import_at, | ||
| last_import_range: Setting.inflation_last_import_range, |
There was a problem hiding this comment.
env_inflation_import_enabled uses ENV["GUS_INFLATION_IMPORT_ENABLED"] without .presence. If the legacy env var is defined but empty (common with dotenv-style GUS_INFLATION_IMPORT_ENABLED=), this will still be treated as “present” and will incorrectly lock the UI toggle. Use .presence on the legacy var as well so the lock behavior matches Setting.inflation_import_enabled_effective.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/UI/dashboard/bond_summary_row.rb (1)
90-121: Memoize and reuse inflation source to avoid repeated source resolution per row.Line 90 and Line 97-121 can resolve inflation source multiple times in the same render path. Cache once and branch on that local value for cheaper dashboard rendering with many lots.
♻️ Proposed refactor
def inflation_linked_rate_meta return t("bonds.purchase_holding.pending_review") if lot.requires_rate_review? @@ inflation = helpers.number_to_percentage(inflation_component.to_d, precision: 3) margin = helpers.number_to_percentage(margin_component.to_d, precision: 3) + source = current_inflation_source_key - if lot.gus_inflation_source?(allow_import: false) + if source == "gus_sdp" t( "bonds.purchase_holding.inflation_meta_gus", inflation: inflation, margin: margin, indicator: lot.current_inflation_indicator_id ) - elsif current_inflation_source_key.blank? || current_inflation_source_key == "manual" + elsif source.blank? || source == "manual" t( "bonds.purchase_holding.inflation_meta_manual", inflation: inflation, margin: margin ) @@ def current_inflation_source_key - lot.current_inflation_source(allow_import: false).to_s.presence + `@current_inflation_source_key` ||= lot.current_inflation_source(allow_import: false).to_s.presence endBased on learnings: GUS provider detection must match
"gus_sdp"when branching oncurrent_inflation_source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/UI/dashboard/bond_summary_row.rb` around lines 90 - 121, Compute and cache the inflation source once per row instead of calling lot.current_inflation_source(allow_import: false) multiple times: introduce a memoized/local variable (e.g., `@inflation_source` or inflation_source) used by current_inflation_source_key and localized_inflation_provider so the source is resolved only once; update the branching that currently checks gus_inflation_source? to compare against the actual provider key "gus_sdp" when deciding the GUS branch; ensure current_inflation_source_key uses the memoized value and localized_inflation_provider returns the translated provider or fallback based on that same cached value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/UI/dashboard/bond_summary_row.rb`:
- Around line 90-121: Compute and cache the inflation source once per row
instead of calling lot.current_inflation_source(allow_import: false) multiple
times: introduce a memoized/local variable (e.g., `@inflation_source` or
inflation_source) used by current_inflation_source_key and
localized_inflation_provider so the source is resolved only once; update the
branching that currently checks gus_inflation_source? to compare against the
actual provider key "gus_sdp" when deciding the GUS branch; ensure
current_inflation_source_key uses the memoized value and
localized_inflation_provider returns the translated provider or fallback based
on that same cached value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb2a9eec-0479-41c0-86fa-198f83a37b89
📒 Files selected for processing (4)
app/components/UI/dashboard/bond_summary_row.rbapp/models/bond/inflation_provider.rbapp/models/bond_lot.rbtest/models/bond_lot_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/bond_lot.rb
- Use .presence for GUS_INFLATION_IMPORT_ENABLED fallback in hostings view\n- Memoize and reuse inflation source key in BondSummaryRow\n- Branch GUS path by explicit provider key (gus_sdp)
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/UI/dashboard/bond_summary_row.rb (1)
114-116: Memoization does not cachenilresults.Line 115 uses
||=, so when source isnilthis recomputes on every call. Preferdefined?-based memoization to cache nullable values.♻️ Proposed fix
def current_inflation_source_key - `@current_inflation_source_key` ||= lot.current_inflation_source(allow_import: false).to_s.presence + return `@current_inflation_source_key` if defined?(`@current_inflation_source_key`) + + `@current_inflation_source_key` = lot.current_inflation_source(allow_import: false).to_s.presence end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/UI/dashboard/bond_summary_row.rb` around lines 114 - 116, current_inflation_source_key uses ||= which won't cache nil, causing repeated calls to lot.current_inflation_source; replace the memoization with a defined?-style check (e.g. use defined?(`@current_inflation_source_key`) or instance_variable_defined? to detect prior assignment) so `@current_inflation_source_key` is set once to the string or nil and returned on subsequent calls; keep the existing call to lot.current_inflation_source(allow_import: false).to_s.presence but assign it unconditionally on first access and return the cached value thereafter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/UI/dashboard/bond_summary_row.rb`:
- Around line 114-116: current_inflation_source_key uses ||= which won't cache
nil, causing repeated calls to lot.current_inflation_source; replace the
memoization with a defined?-style check (e.g. use
defined?(`@current_inflation_source_key`) or instance_variable_defined? to detect
prior assignment) so `@current_inflation_source_key` is set once to the string or
nil and returned on subsequent calls; keep the existing call to
lot.current_inflation_source(allow_import: false).to_s.presence but assign it
unconditionally on first access and return the cached value thereafter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e98b588a-996f-4bbe-b8cc-5ffb29ba26a6
📒 Files selected for processing (2)
app/components/UI/dashboard/bond_summary_row.rbapp/views/settings/hostings/show.html.erb
✅ Files skipped from review due to trivial changes (1)
- app/views/settings/hostings/show.html.erb
…n no-op - Use defined? guard in BondSummaryRow#current_inflation_source_key to cache nil values (not just presence) - Make 20260407183000 migration down a no-op since constraints already exist in CreateBondLots baseline
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/UI/dashboard/bond_summary_row.rb (1)
14-20: Avoid recomputing return amounts across selector + value methods.
lot.total_return_amount/lot.projected_total_return_amountcan be evaluated more than once per row render. Consider memoizing raw values once and reusing in both methods.♻️ Suggested refactor
def total_return_amount - `@total_return_amount` ||= if projected_total_return? - lot.projected_total_return_amount(allow_import: false) - else - lot.total_return_amount(allow_import: false) - end + `@total_return_amount` ||= projected_total_return? ? projected_return_amount : current_return_amount end private + def current_return_amount + `@current_return_amount` ||= lot.total_return_amount(allow_import: false) + end + + def projected_return_amount + `@projected_return_amount` ||= lot.projected_total_return_amount(allow_import: false) + end + def projected_total_return? return `@projected_total_return` if defined?(`@projected_total_return`) - `@projected_total_return` = lot.total_return_amount(allow_import: false).abs < 0.01.to_d && - lot.projected_total_return_amount(allow_import: false).positive? + `@projected_total_return` = current_return_amount.abs < 0.01.to_d && + projected_return_amount.positive? endAlso applies to: 66-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/UI/dashboard/bond_summary_row.rb` around lines 14 - 20, Memoize the raw return values once and reuse them instead of calling lot.total_return_amount or lot.projected_total_return_amount multiple times: add a single memoized helper (e.g., fetch_return_amounts or `@cached_return_amounts`) that computes both the projected and actual raw amounts from lot (calling lot.projected_total_return_amount(allow_import: false) and lot.total_return_amount(allow_import: false) once), then update total_return_amount and the other methods referenced around lines 66-71 to read from that cache rather than invoking lot methods again.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/UI/dashboard/bond_summary_row.rb`:
- Line 43: The inflation-linked branch double-formats the rate:
current_rate_percent() already returns a percent (e.g., 2.5) and passing that
into helpers.number_to_percentage causes a ×100 again; update the conditional
that uses current_rate_percent() so you either render current_rate_percent()
directly (no number_to_percentage) or divide current_rate_percent() by 100 and
then call helpers.number_to_percentage to convert back to a fractional value;
adjust where lot.interest_rate and current_rate_percent() are used so inflation
and non-inflation branches produce consistent percent formatting.
---
Nitpick comments:
In `@app/components/UI/dashboard/bond_summary_row.rb`:
- Around line 14-20: Memoize the raw return values once and reuse them instead
of calling lot.total_return_amount or lot.projected_total_return_amount multiple
times: add a single memoized helper (e.g., fetch_return_amounts or
`@cached_return_amounts`) that computes both the projected and actual raw amounts
from lot (calling lot.projected_total_return_amount(allow_import: false) and
lot.total_return_amount(allow_import: false) once), then update
total_return_amount and the other methods referenced around lines 66-71 to read
from that cache rather than invoking lot methods again.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f565e65f-d8ee-4f34-89a0-c7c3a89b4c15
📒 Files selected for processing (3)
app/components/UI/dashboard/bond_summary_row.rbapp/views/settings/hostings/show.html.erbdb/migrate/20260407183000_add_bond_lot_enum_check_constraints.rb
✅ Files skipped from review due to trivial changes (1)
- app/views/settings/hostings/show.html.erb
🚧 Files skipped from review as they are similar to previous changes (1)
- db/migrate/20260407183000_add_bond_lot_enum_check_constraints.rb
- Extract current_return_amount and projected_return_amount helpers to avoid repeated evaluation of lot.total_return_amount() and lot.projected_total_return_amount(allow_import: false) per row render. Use single memoized values in both total_return_amount and projected_total_return? - Fix double-percent formatting in rate_text: current_rate_percent() already returns percent (e.g., 2.5) so format directly as string instead of calling number_to_percentage which would multiply by 100 again. For fixed-rate bonds, multiply interest_rate by 100 before formatting. All 60 bond tests passing, RuboCop clean.
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| t("bonds.purchase_holding.unknown") | ||
| else | ||
| rate = lot.interest_rate | ||
| rate.present? ? "#{(rate * 100).round(3)}%" : t("bonds.purchase_holding.unknown") |
There was a problem hiding this comment.
rate_text multiplies lot.interest_rate by 100 before formatting. Elsewhere in the bond UI/tests interest_rate is already stored as a percent value (e.g., 4.25 meaning 4.25%), so this will display 425% instead of 4.25%. Format the stored percent directly (e.g., via number_to_percentage(rate, precision: 3) or "#{rate.round(3)}%").
Suggested change
| rate.present? ? "#{(rate * 100).round(3)}%" : t("bonds.purchase_holding.unknown") | |
| rate.present? ? "#{rate.round(3)}%" : t("bonds.purchase_holding.unknown") |
| @@ -0,0 +1,83 @@ | |||
| class Settings::Hostings::InflationImportSettingsComponent < ApplicationComponent | |||
| PROVIDERS = %w[gus_sdp us_bls es_ine].freeze | |||
There was a problem hiding this comment.
PROVIDERS is a hard-coded list that duplicates Bond::InflationProvider::PROVIDERS.keys. This can drift if a provider is added/removed elsewhere (UI would silently omit it). Consider deriving this list from the canonical provider registry so stats rendering stays in sync.
Suggested change
| PROVIDERS = %w[gus_sdp us_bls es_ine].freeze | |
| PROVIDERS = Bond::InflationProvider::PROVIDERS.keys.map(&:to_s).freeze |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/UI/dashboard/bond_summary_row.rb`:
- Around line 71-76: The predicate method projected_total_return? should guard
against nils on current_return_amount and projected_return_amount to avoid
NoMethodError; update projected_total_return? (and its memoization
`@projected_total_return`) to return false if either current_return_amount or
projected_return_amount is nil, and only call numeric methods (.abs, .positive?,
.to_d) when those values are present (e.g., use explicit nil checks or safe
navigation) so missing data yields false instead of raising.
- Around line 26-28: The helper total_return_class currently calls
total_return_amount.negative? which can raise NoMethodError when
total_return_amount is nil; update total_return_class to guard against nil
(e.g., return a neutral class like "" or "text-muted" if
total_return_amount.nil?) or coerce safely (e.g., use
total_return_amount.to_f.negative?) and ensure you reference total_return_amount
(and the model callers lot.total_return_amount /
lot.projected_total_return_amount) so the view also handles nil by rendering a
placeholder instead of the numeric value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c46de9e-a9f5-4ca8-b34d-f9fedc9abc10
📒 Files selected for processing (1)
app/components/UI/dashboard/bond_summary_row.rb
Comment on lines
+26
to
+28
| def total_return_class | ||
| total_return_amount.negative? ? "text-destructive" : "text-success" | ||
| end |
There was a problem hiding this comment.
Potential NoMethodError if total_return_amount is nil.
Based on the model code, both lot.total_return_amount and lot.projected_total_return_amount can return nil (e.g., when estimated_current_value is nil or interest_rate/annual_rate is missing). Calling .negative? on nil will raise NoMethodError.
Suggested fix
def total_return_class
- total_return_amount.negative? ? "text-destructive" : "text-success"
+ return "text-secondary" if total_return_amount.nil?
+
+ total_return_amount.negative? ? "text-destructive" : "text-success"
endYou may also want to handle the nil case in the view template to avoid rendering invalid return amounts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/UI/dashboard/bond_summary_row.rb` around lines 26 - 28, The
helper total_return_class currently calls total_return_amount.negative? which
can raise NoMethodError when total_return_amount is nil; update
total_return_class to guard against nil (e.g., return a neutral class like "" or
"text-muted" if total_return_amount.nil?) or coerce safely (e.g., use
total_return_amount.to_f.negative?) and ensure you reference total_return_amount
(and the model callers lot.total_return_amount /
lot.projected_total_return_amount) so the view also handles nil by rendering a
placeholder instead of the numeric value.
Comment on lines
+71
to
+76
| def projected_total_return? | ||
| return @projected_total_return if defined?(@projected_total_return) | ||
|
|
||
| @projected_total_return = current_return_amount.abs < 0.01.to_d && | ||
| projected_return_amount.positive? | ||
| end |
There was a problem hiding this comment.
Nil values will cause NoMethodError in predicate logic.
Both current_return_amount and projected_return_amount can be nil (per context snippets showing the underlying model methods return nil when data is missing). Calling .abs or .positive? on nil raises NoMethodError.
Suggested fix with nil guards
def projected_total_return?
return `@projected_total_return` if defined?(`@projected_total_return`)
- `@projected_total_return` = current_return_amount.abs < 0.01.to_d &&
- projected_return_amount.positive?
+ current = current_return_amount
+ projected = projected_return_amount
+
+ `@projected_total_return` = current.present? &&
+ projected.present? &&
+ current.abs < 0.01.to_d &&
+ projected.positive?
endThis ensures the method returns false when data is unavailable rather than raising an exception.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def projected_total_return? | |
| return @projected_total_return if defined?(@projected_total_return) | |
| @projected_total_return = current_return_amount.abs < 0.01.to_d && | |
| projected_return_amount.positive? | |
| end | |
| def projected_total_return? | |
| return `@projected_total_return` if defined?(`@projected_total_return`) | |
| current = current_return_amount | |
| projected = projected_return_amount | |
| `@projected_total_return` = current.present? && | |
| projected.present? && | |
| current.abs < 0.01.to_d && | |
| projected.positive? | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/UI/dashboard/bond_summary_row.rb` around lines 71 - 76, The
predicate method projected_total_return? should guard against nils on
current_return_amount and projected_return_amount to avoid NoMethodError; update
projected_total_return? (and its memoization `@projected_total_return`) to return
false if either current_return_amount or projected_return_amount is nil, and
only call numeric methods (.abs, .positive?, .to_d) when those values are
present (e.g., use explicit nil checks or safe navigation) so missing data
yields false instead of raising.
Collaborator
|
@UberDudePL this is a massive undertaking, so first of all want to take time to thank you for the work. Product Fit & Complexity AssessmentThe Good
The Concern: Inflation-Linked + GUS CPI
My honest assessment: The inflation subsystem is over-engineered for v1. If the goal is "simple, easy to understand, good flexibility for bond portfolio oversight," then:
Suggestion: Consider shipping bonds with inflation-linked support using manual rate entry only (the inflation_rate_assumption field already handles this). The auto-fetch CPI infrastructure could be a fast-follow PR. This would cut ~40% of the PR's net-new complexity. Some concerns worth addressing anyway:
|
Collaborator
|
Starting to see codebase drift here, what do you think of @sokie's suggestion @UberDudePL? |
Author
# Conflicts: # app/controllers/settings/hostings_controller.rb # db/schema.rb # test/controllers/accounts_controller_test.rb # test/controllers/settings/hostings_controller_test.rb
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces comprehensive support for bond accounts, including new UI components, controller logic, automation for bond lot settlement, and infrastructure for handling inflation-linked bonds via GUS CPI data. It also adds several environment and settings options for configuring GUS inflation import. The most important changes are grouped below:
Bond Account and Bond Lot Support:
BondLotsControllerandBondsControllerto handle CRUD operations for bond lots and accounts, including creation, editing, updating, and deletion, with appropriate permission checks and transaction management. (app/controllers/bond_lots_controller.rb[1]app/controllers/bonds_controller.rb[2]app/components/UI/account/activity_feed.html.erb[1]app/components/UI/account/chart.html.erb[2] [3]app/components/UI/account/chart.rb[4]app/components/UI/account_page.rb[5] [6]app/controllers/pages_controller.rb[7] [8]app/controllers/accounts_controller.rb[1]app/jobs/settle_matured_bond_lots_job.rb[2]Inflation-Linked Bond and GUS CPI Integration:
app/jobs/import_gus_inflation_rates_job.rb[1]app/controllers/settings/hostings_controller.rb[2] [3] [4].env.example[5].env.local.example[6].env.test.example[7]app/javascript/controllers/bond_account_form_controller.js[1]app/javascript/controllers/bond_lot_form_controller.js[2]User Experience and Automation:
app/controllers/pages_controller.rbapp/controllers/pages_controller.rbR355-R365)app/models/account.rbapp/models/account.rbR341-R349)These changes significantly expand the platform's support for bonds, especially inflation-linked government bonds, and automate important workflows for users and admins.
Validation
I ran focused regression tests for:
All executed tests are green locally.
This PR is intentionally English-only for Bond features.
Polish localization have been merged in a separate PR #1356 - any changes to pl.yml files now are related to bonds and should be part of this PR
Summary by CodeRabbit
New Features
Client-side
Tests
Documentation
Chores