Feat/loans overview new insights#1775
Conversation
superagent-security
Bot
added
contributor:verified
|
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 down_payment, start_date, insurance_rate, and insurance_rate_type; implements amortization schedule, timeline and cost calculations on Loan; updates controller permits, form and overview UI, locales, migrations, schema, fixtures, and tests. ChangesLoan Amortization and Cost Tracking System
Sequence DiagramsequenceDiagram
participant User as User/Form
participant Controller as LoansController
participant Loan as Loan Model
participant Cache as Rails.cache
participant Generator as AmortizationGenerator
participant View as Overview View
User->>Controller: submit loan form with down_payment, start_date, insurance_rate, insurance_rate_type
Controller->>Loan: create/update Loan with new attributes
Loan->>Loan: before_validation sets default start_date
Loan->>Loan: validate numeric and enum fields
View->>Loan: request amortization_schedule
Loan->>Cache: fetch by cache_key_with_version + original_balance_cache_key
Cache-->>Loan: return cached schedule or miss
Loan->>Generator: generate_amortization_schedule (month-by-month)
Generator->>Generator: compute interest, insurance (insurance_rate_type logic), principal, remaining_balance
Generator-->>Loan: return schedule array
Loan->>View: return cached schedule, payment_breakdown, totals, ratios
View->>User: render progress circle, cost cards, payment detail
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Not sure how much of this you had a chance to address, but reposting here for continuity: Independent review of the full diff. I verified which earlier comments were addressed, then read the code fresh. Several earlier raised issues are genuinely fixed; a few open threads remain, and I found additional problems. Still-open review thread issues1.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/loans/tabs/_overview.html.erb (1)
171-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the CTA text instead of hardcoding English.
Line 171 should use
t(...)for user-facing copy to keep locale parity.As per coding guidelines, user-facing strings in views/components must use
t().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/loans/tabs/_overview.html.erb` at line 171, Replace the hardcoded CTA string "Edit loan details" in the _overview.html.erb view with a translation helper call (t(...)) so the user-facing copy is localized; update the view to call t with a descriptive key (e.g. t('loans.tabs.overview.edit_loan_details') or a scoped key like t('.edit_loan_details')) and add the corresponding entry to your locale YAML (e.g. en.yml) under that key; ensure the same key is used wherever this CTA appears so tests and locales remain consistent.
🧹 Nitpick comments (2)
app/views/loans/_form.html.erb (1)
7-9: ⚡ Quick winUse
t()helper instead ofI18n.tin ERB user-facing copy.Switch the option label lookup to
t(...)for consistency with project i18n conventions in views.As per coding guidelines, user-facing strings in views/components must use
t()for translation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/loans/_form.html.erb` around lines 7 - 9, The view uses I18n.t inside the loan_form.select options mapping (Loan::SUBTYPES.map { |k, v| [I18n.t("loans.subtypes.#{k}.long", default: v[:long]), k] }) which violates the project convention for views; replace I18n.t with the view helper t(...) so the option labels use t("loans.subtypes.#{k}.long", default: v[:long]) while keeping the rest of the select call (loan_form.select, Loan::SUBTYPES, and the prompt/include_blank options) unchanged.app/views/loans/tabs/_overview.html.erb (1)
29-29: ⚡ Quick winReplace raw Tailwind palette classes with design-system tokens.
These SVG stroke classes bypass the design token system. Please switch to functional tokens defined by the project design system.
As per coding guidelines, ERB/CSS should use functional tokens from
app/assets/tailwind/sure-design-system.cssinstead of raw Tailwind palette classes.Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/loans/tabs/_overview.html.erb` at line 29, SVG elements currently use raw Tailwind palette classes like "stroke-zinc-700 fill-none"; replace those with the project's functional design-system token classes defined in app/assets/tailwind/sure-design-system.css (e.g., swap "stroke-zinc-700" for the equivalent token class from the design system and keep "fill-none" if appropriate). Locate the SVGs in app/views/loans/tabs/_overview.html.erb (the class attributes at the shown lines and the other occurrence around line 38) and update both occurrences to use the corresponding design-system stroke token class names so the code uses the project's tokens rather than raw palette classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/loan.rb`:
- Line 39: The model currently only validates start_date on create via the
validates :start_date, presence: true, on: :create line in Loan, which lets
DB-level NOT NULL errors surface on update; remove the on: :create option (or
change to on: :save) so start_date presence is validated on updates as well,
updating the Loan validation to run for both create and update.
In `@config/locales/views/loans/fr.yml`:
- Line 51: The FR locale entry for the key rate_types.adjustable is left in
English ("Adjustable"); update the value to the proper French translation (e.g.,
"Ajustable" or "Variable") so the rate_types.adjustable key is consistent with
surrounding French labels.
In `@db/migrate/20260512155616_add_start_date_to_loans.rb`:
- Line 10: This migration currently enforces NOT NULL with change_column_null
:loans, :start_date, false which can break running older app instances; instead
make this migration only add the start_date column as nullable (or remove the
change_column_null line), add a safe backfill step that populates start_date for
all existing loans (e.g., an UPDATE that sets a sensible default or derived
value), deploy the app so all writers include start_date, and then create a
separate follow-up migration that runs change_column_null :loans, :start_date,
false to enforce NOT NULL once rollout is complete.
---
Outside diff comments:
In `@app/views/loans/tabs/_overview.html.erb`:
- Line 171: Replace the hardcoded CTA string "Edit loan details" in the
_overview.html.erb view with a translation helper call (t(...)) so the
user-facing copy is localized; update the view to call t with a descriptive key
(e.g. t('loans.tabs.overview.edit_loan_details') or a scoped key like
t('.edit_loan_details')) and add the corresponding entry to your locale YAML
(e.g. en.yml) under that key; ensure the same key is used wherever this CTA
appears so tests and locales remain consistent.
---
Nitpick comments:
In `@app/views/loans/_form.html.erb`:
- Around line 7-9: The view uses I18n.t inside the loan_form.select options
mapping (Loan::SUBTYPES.map { |k, v| [I18n.t("loans.subtypes.#{k}.long",
default: v[:long]), k] }) which violates the project convention for views;
replace I18n.t with the view helper t(...) so the option labels use
t("loans.subtypes.#{k}.long", default: v[:long]) while keeping the rest of the
select call (loan_form.select, Loan::SUBTYPES, and the prompt/include_blank
options) unchanged.
In `@app/views/loans/tabs/_overview.html.erb`:
- Line 29: SVG elements currently use raw Tailwind palette classes like
"stroke-zinc-700 fill-none"; replace those with the project's functional
design-system token classes defined in
app/assets/tailwind/sure-design-system.css (e.g., swap "stroke-zinc-700" for the
equivalent token class from the design system and keep "fill-none" if
appropriate). Locate the SVGs in app/views/loans/tabs/_overview.html.erb (the
class attributes at the shown lines and the other occurrence around line 38) and
update both occurrences to use the corresponding design-system stroke token
class names so the code uses the project's tokens rather than raw palette
classes.
🪄 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: 608e2518-c62b-4628-98fc-5f113d5992f9
📒 Files selected for processing (14)
app/controllers/loans_controller.rbapp/models/loan.rbapp/views/loans/_form.html.erbapp/views/loans/tabs/_overview.html.erbconfig/locales/views/loans/en.ymlconfig/locales/views/loans/fr.ymldb/migrate/20260512155544_add_down_payment_to_loans.rbdb/migrate/20260512155616_add_start_date_to_loans.rbdb/migrate/20260512155636_add_insurance_rate_type_to_loans.rbdb/migrate/20260512155654_add_insurance_rate_to_loans.rbdb/schema.rbtest/fixtures/loans.ymltest/models/loan_test.rbtest/system/accounts_test.rb
Signed-off-by: HugoleDino <135261771+HugoleDino@users.noreply.github.com>
|
Hey @jjmata ! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/schema.rb`:
- Line 250: The schema.rb currently contains unrelated changes (e.g.
columns/categories.lucide_icon, families.vector_store_id,
import_rows.exchange_operating_mic, imports.rows_to_skip,
snaptrade_accounts.activities_fetch_pending,
sophtron_accounts.account_number_mask, sophtron_items timestamp reorder,
users.ui_layout) that must be removed by regenerating the schema from a clean DB
state; revert the current db/schema.rb to the upstream/base version, create a
fresh empty database, run all migrations from scratch so only the loan-related
migrations are applied, dump the schema (regenerate db/schema.rb) and verify
that the only diffs left reference the loan migration artifacts before
committing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
A few blockers to address before this is reviewable:
Generated by Claude Code |
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
db/schema.rb (1)
255-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegenerate
schema.rbfrom a clean migration state.This dump still carries non-loan schema drift: Line 255 loosens
binance_itemsnullability, Line 1341 adds an unrelated unique index onrecurring_transactions, and the other touched lines are unrelated table changes outside this feature. Sincedb:schema:loaduses this file, merging it would ship schema changes that are not part of the loan work.Also applies to: 344-344, 688-688, 849-849, 893-893, 1341-1341, 1540-1540, 1591-1591, 1615-1616, 1808-1808
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/schema.rb` around lines 255 - 257, The schema dump contains unrelated schema drift (e.g., loosened nullability on binance_items, a new unique index on recurring_transactions and other unrelated table changes); to fix this, reset to a clean DB state, run the full migrations for the current branch, then regenerate schema.rb so it only reflects intended loan-related migrations: run the Rails tasks to drop/create the test/dev DB, run migrations from scratch and then run rails db:schema:dump (or rails db:migrate followed by db:schema:dump) to overwrite db/schema.rb; verify that the resulting file no longer contains the unrelated changes (look for binance_items nullability, the recurring_transactions unique index, and the other listed line artifacts) before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@db/schema.rb`:
- Around line 255-257: The schema dump contains unrelated schema drift (e.g.,
loosened nullability on binance_items, a new unique index on
recurring_transactions and other unrelated table changes); to fix this, reset to
a clean DB state, run the full migrations for the current branch, then
regenerate schema.rb so it only reflects intended loan-related migrations: run
the Rails tasks to drop/create the test/dev DB, run migrations from scratch and
then run rails db:schema:dump (or rails db:migrate followed by db:schema:dump)
to overwrite db/schema.rb; verify that the resulting file no longer contains the
unrelated changes (look for binance_items nullability, the
recurring_transactions unique index, and the other listed line artifacts) before
committing.
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Please fix broken tests and merge conflicts.
* feat: add Cloudflare Containers PR preview deployments Add GitHub workflows to automatically deploy PRs to Cloudflare Containers after tests pass, with automatic cleanup after 24 hours. Components: - workers/preview/: Cloudflare Worker entry point that routes traffic to the Rails container - preview-deploy.yml: Deploys PRs after CI passes, comments preview URL on PR - preview-cleanup.yml: Cleans up previews on PR close or after 24 hours via scheduled job The container sleeps after 30 minutes of inactivity and wakes automatically on the next request. Required secrets: - CLOUDFLARE_API_TOKEN - CLOUDFLARE_ACCOUNT_ID - CLOUDFLARE_WORKERS_SUBDOMAIN https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: use development environment with embedded PostgreSQL for previews - Add preview-specific Dockerfile with PostgreSQL server included - Add docker-entrypoint.sh to start PostgreSQL and run migrations - Change RAILS_ENV from production to development - Auto-generate SECRET_KEY_BASE and DATABASE_URL for self-contained previews https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * feat: add Redis to preview container - Install redis-server in the preview Dockerfile - Start Redis in the entrypoint before PostgreSQL - Auto-configure REDIS_URL for Sidekiq background jobs https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: mark GitHub deployment inactive on manual PR cleanup When using workflow_dispatch with a specific pr_number, the workflow now also marks the associated GitHub deployment as inactive, mirroring the behavior of the batch cleanup path. https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: remove npm cache config that requires missing lockfile The setup-node action's cache feature requires a package-lock.json which doesn't exist in workers/preview/. Remove the cache configuration to fix the workflow. https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: only update deployment status when deployment ID exists Add condition to check steps.deployment.outputs.result exists before attempting to update deployment status. This prevents a JavaScript syntax error when the deployment step fails and no ID is available. https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: quote shell variables to fix SC2086 shellcheck warning Quote the --var argument and GITHUB_OUTPUT redirection to prevent word splitting issues. https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: add permissions for deployment status operations Add deployments: write permission to the cleanup workflow so the GITHUB_TOKEN can list and update deployment statuses. https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: specify build context for Dockerfile in wrangler config Use object syntax for image config to set build context to repository root, allowing the Dockerfile to reference files from both the root (Gemfile, .ruby-version) and workers/preview/ (docker-entrypoint.sh). https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: run wrangler from repo root for correct build context - Update workflow to run wrangler with --config flag from repo root - Update wrangler.toml paths (main, image) to be relative to repo root - Embed entrypoint script directly in Dockerfile using heredoc - Remove separate docker-entrypoint.sh file This ensures the Docker build context includes Gemfile, .ruby-version, and other files at the repo root. https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: move preview Dockerfile to repo root for correct build context Wrangler resolves paths relative to the config file, not the current directory. Moving Dockerfile.preview to repo root ensures: - Build context is the repo root (where Gemfile, .ruby-version are) - Path in wrangler.toml is ../../Dockerfile.preview (relative to config) - Worker runs from workers/preview/ directory again https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: use find to locate pg_hba.conf instead of glob in redirection Shell glob patterns don't work with redirection operators. Use find to locate the actual pg_hba.conf path before writing to it. https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix: enable workers_dev for preview deployments Add workers_dev = true to make the preview worker accessible via the workers.dev subdomain. https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * feat: enable observability for container logs https://claude.ai/code/session_013EZuzBxWPEEYp3TQptXWdP * fix preview container boot path * fix: set preview container startup command explicitly * fix: update preview worker compatibility date * chore: expose preview container diagnostics * fix: recover from stale preview container state * fix: harden preview container startup paths * chore: report preview startup stages * fix: bypass stale container helper state during recovery * fix: allow longer preview container startup * fix: upgrade preview container runtime * fix: use supported node version for preview deploy * fix: use public container startup flow * fix: simplify preview container startup * chore: retain preview container diagnostic history * fix: bypass systemctl redirect for postgres startup * chore: probe rails readiness from inside preview container * chore: capture rails process and port diagnostics * chore: capture rails startup logs on preview timeout * fix: align preview bind behavior with ipv6 startup model * chore: capture preview socket state on rails timeout * chore: capture rails wait state and child processes * fix: launch preview with puma directly * fix: run preview in production mode * chore: probe preview app boot before puma * fix: disable lookbook routes in production preview * chore: capture ruby backtrace from hung boot probe * fix: disable bootsnap in preview runtime * fix: disable sidekiq web routes in production preview * chore: trace hung preview boot probe with strace * fix: json-escape preview telemetry payloads * fix: pass preview telemetry env vars correctly * chore: signal ruby child for preview boot backtrace * fix: allow longer preview cold-start budget * fix: skip sidekiq web requires in production preview * chore: deploy hello world preview container * fix(preview): restore rails image without redundant warmup * feat(preview): seed demo dataset on boot * ci(preview): require preview-cf label * ci(preview): reuse pr workflow checks * fix(preview): avoid clearing demo data in production boot * fix(preview): tolerate already-running postgres on boot * fix(preview): check demo user via psql during boot * fix(preview): defer heavy demo seed until after boot * fix(preview): move demo-user creation after rails boot * fix(preview): fail fast on container lifecycle errors * fix(preview): validate manual cleanup pr input * fix(preview): parameterize preview pr number * ci(preview): use setup-node v6 --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: KiloClaw <kiloclaw@openclaw.ai>
* fix(preview): use worker list metadata for cleanup * fix(preview): handle cleanup edge cases * fix(preview): harden scheduled cleanup errors * feat(preview): add warmup screen and readiness gate * fix(preview): report success after image deploy * fix(preview): stop blocking healthy previews on stale status
…mise#1806) * Extract hardcoded strings to i18n Replace numerous hardcoded English strings with I18n lookups (t / I18n.t) across controllers, views, helpers, and components, and convert model validation error messages to symbol keys. Added multiple locale files under config/locales for models and views. This centralizes user-facing notices/alerts, UI text, import/validation messages, and prepares the app for localization and easier translation maintenance. * Update en.yml * Update preview-cleanup.yml * Revert "Update preview-cleanup.yml" This reverts commit 1ba6d3c. * test: align i18n assertions with translated messages * Standardize balance error key and tweak locales Replace SophtronAccount's :requires_balance error key with :no_balance and update related locale strings for sophtron, plaid, and simplefin accounts to use the new key and clearer copy. Also switch the QIF upload redirect notice to use a relative translation key (t('.qif_uploaded')), remove an unused SSO providers help line, and fix a trailing-newline/whitespace issue in the subscriptions locale. These changes standardize validation keys and improve translation consistency and messaging. --------- Co-authored-by: KiloClaw <kiloclaw@openclaw.ai>
DS Drift Patrol — 1 findingRule 2 — Non-functional Tailwind tokens ( An SVG class="text-gray-200 dark:text-gray-700"Replace with Generated by Claude Code |
DS Drift Patrol — 2 additional findings
Rule 2 — Non-functional Tailwind color literals ( The progress arc SVG uses raw amber palette tokens for both its stroke and label text: class="stroke-amber-500 transition-all duration-500"class: "font-medium text-amber-500"
class="stroke-warning transition-all duration-500"class: "font-medium text-warning"Generated by Claude Code |
@sure-design This suggestion renders too light in dark mode and too strong in light mode for the empty circle track. It gives too much visual weight to the design compared to the actual data represented by the text-warning filled circle — though this is subjective. No existing DS token matches these visual requirements, so the current raw classes remain the most accurate implementation for now. |
superagent-security
Bot
removed
contributor:verified
Any recommendations @gariasf? |
Replace the hand-rolled SVG progress ring with the shared `donut-chart` Stimulus controller (the same one Budgets and Goals use). The empty track now uses the `--budget-unused-fill` DS token (gray-200 light / gray-500 dark) instead of hardcoded `text-gray-200 dark:text-gray-700`, fixing the track rendering too light in dark mode and too strong in light mode. The filled arc keeps `var(--color-warning)` by marking the "paid" segment as the controller's overage segment, so its var() color passes straight to the SVG fill instead of being parsed by d3.color() (which returns null for var(...)). Adds Loan#to_donut_segments to build the segment data.
|
Rather than hand-tuning the track color, I wired the ring onto the shared Pushed in 7e0d9c1.
|
|
@HugoleDino small thing on the Overview tab — every money figure renders with cents, but they're all computed as whole-dollar integers, so the They're also projections (30-year total cost/interest, estimated monthly payment), so the cents imply a precision the numbers don't actually carry. I'd drop them with <%= format_money(loan.total_cost, precision: 0) %> # → $652,320Worth applying to original principal, total cost + breakdown, and monthly payment + breakdown. The real synced balance up in the account header can keep its cents — this is just about the projection grid. |
|
Did a design-system pass on the Overview tab — compared it against First, two things that look like they could be off but aren't — they match the peers exactly, so I'd leave them as-is:
Where I think there's room — ran a quick experiment, three versions:
A — drop the cents ( B — composition bars. The breakdown cards hold proportional data but show it as a flat list, so the reader has to do the math. A small stacked bar tells the story instantly: interest is ~53% of total cost and ~78% of the current monthly payment. Reuses existing tokens ( One tiny DS nit while you're in there: the Initial leverage value is |
Nice, I like the light theme render! Dark theme feels slightly too light to me but overall it's cool, I like it.
I really love the result of proposal B! The stacked bar is a great idea !! |
gariasf
left a comment
gariasf
left a comment
There was a problem hiding this comment.
Took it for a spin again on a clean DB and drove the Overview tab in the browser — the feature is nice, but there are blockers around the loan-balance sign convention.
1. Negative-balance handling is broken. Loan accounts carry a negative balance/valuation (the account header itself shows "Remaining principal balance −$11,100.00"). The new analytics derive from account.first_valuation_amount / account.balance and never take .abs, so the Overview renders:
- Total cost: −$12,900.00, Monthly payment: −$215.00 (negative)
- Repayment progress: "100.0% repaid" + a full ring, while the loan still owes −$11,100.
balance_paid_ratiomixes the positiveinitial_balancecolumn with the negativeaccount.balance→ clamps to 1.0. - Initial leverage: "Unknown" —
initial_leverage_ratio = original_balance(−11,100) / down_payment(5,000) = −2.22→ no band matches.
monthly_payment, total_cost, total_interest, balance_paid_ratio, and initial_leverage_ratio all need to normalize the balance sign.
2. Still conflicting with main — needs a rebase.
Medium — dual source of truth for the starting balance: the "Original principal" card + balance_paid_ratio read the initial_balance column ($25,000), while amortization + total_cost principal use original_balance = first_valuation_amount (−$11,100). They disagree on the same screen — pick one canonical source.
Nits:
monthly_paymentbuildsBigDecimal(interest_rate)/BigDecimal(balance_amount)without.to_s(a Float would raise);generate_amortization_schedulecorrectly uses.to_s— make it consistent.LEVERAGE_BANDSranges overlap at boundaries (0..4,4..8,8..) — use exclusive ranges.remove_obsolete_recurring_transactions_unique_indexmigration +demo/generator.rbchanges are unrelated to the loans feature (scope creep).
For coverage: loan_test.rb passes locally, but none of the model tests use a realistic negative-balance loan — that's exactly where the bugs live. A test with a negative-balance loan would catch all of the above.
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| months = term_months | ||
| date = start_date | ||
|
|
||
| months.times do |i| |
There was a problem hiding this comment.
P1: Unbounded term_months allows DoS via amortization schedule generation
generate_amortization_schedule loops term_months times with no upper bound.
Add a maximum validation for term_months to prevent DoS from huge schedules.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="app/models/loan.rb">
<violation number="1" location="app/models/loan.rb:271">
<priority>P1</priority>
<title>Unbounded term_months allows DoS via amortization schedule generation</title>
<evidence>The generate_amortization_schedule method performs `months.times do |i|` where `months` is `term_months`, but `term_months` only has a validation for `greater_than: 0` with no upper bound. An authenticated attacker can create a loan with an extremely large term_months value (e.g., 999999999) and viewing the loan overview will trigger a long-running loop that consumes CPU and memory, potentially causing a denial of service.</evidence>
<recommendation>Add a maximum value validation for `term_months` in the Loan model, such as `less_than_or_equal_to: 600` (50 years), to cap the amortization schedule. Additionally, add a guard clause in `generate_amortization_schedule` to short-circuit or raise when `term_months` exceeds the permitted maximum.</recommendation>
</violation>
</file>
DS Drift Patrol — resolved7 of 7 previously-flagged issue(s) addressed. No new findings on this scan. (The Overview tab's progress visualization was rewritten to use the shared Generated by Claude Code |
DS Drift Patrol — 1 finding
Rule 5 — i18n bypass via hardcoded
|
|
Flagging that @gariasf's June 1 review found blocking sign-convention bugs (negative loan balances producing negative total cost/monthly payment, and a bogus 100% "repaid" state) — these look unaddressed, since the branch hasn't had a substantive commit since May 30 (only a stale merge from main on June 14). Given it's now ~6 weeks stale relative to Generated by Claude Code |
7 participants
Summary
Add new editable loan parameters and enhance the overview with additional financial insights.
This PR replaces #1497
Changes
Editable fields added
Overview enhancements
These changes improve flexibility for users and provide clearer visibility into loan dynamics and costs.
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests
Chores